Skip to content
This repository has been archived by the owner on Feb 15, 2019. It is now read-only.

WebSocket use gorilla/websocket #40

Merged
merged 3 commits into from
Nov 16, 2017
Merged

Conversation

juliens
Copy link
Member

@juliens juliens commented Nov 15, 2017

related to:

#17
#20
#21
#22
#26
#27
#28
#31

forward/fwd.go Outdated
f.log.Debugf("vulcand/oxy/forward/websocket: Dialing insecure (non-tls) tcp connection to host %s", host)
targetConn, err = net.Dial("tcp", host)
dialer.TLSClientConfig = f.tlsClientConfig.Clone()
//WebSocket is only in http/1.1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a space after //

forward/fwd.go Outdated
return
}
underlyingConn, _, err := hijacker.Hijack()

//Only the targetConn choose to CheckOrigin or not
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a space after //

forward/fwd.go Outdated
go replicate(targetConn.UnderlyingConn(), underlyingConn.UnderlyingConn(), "backend", "client")

// Try to read the first message
t, msg, err := targetConn.ReadMessage()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please rename t to msgType

forward/fwd.go Outdated
}

go replicate(underlyingConn.UnderlyingConn(), targetConn.UnderlyingConn(), "client", "backend")
<-errc
}

// copyRequest makes a copy of the specified request.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please fix your comment // copyWebSocketRequest makes a copy of the specified web socket request

forward/fwd.go Outdated

outReq.Header = make(http.Header)
//gorilla websocket use this header to set the request.Host tested in checkSameOrigin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add space after //


req.Write(conn)

//First request works with 400
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a space after //

Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👏

Copy link
Member

@nmengin nmengin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

@ldez ldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ldez ldez merged commit 57f1f0d into containous:containous-fork Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants