Support HTTP Websocket Connections in wsclient #899
Conversation
| @@ -349,6 +350,19 @@ func (cs *ClientServerImpl) handleMessage(data []byte) { | |||
| } | |||
| } | |||
|
|
|||
| func websocketScheme(httpScheme string) (wsScheme string, err error) { | |||
There was a problem hiding this comment.
In general, we've avoided using named returns. Please change this.
| case "https": | ||
| wsScheme = "wss" | ||
| default: | ||
| err = fmt.Errorf("Unknown httpScheme %s", httpScheme) |
There was a problem hiding this comment.
Use format $context: msg and package errors.New()
errors.New("wsclient: Unknown httpScheme %s", httpScheme)
| // TODO replace with gomock | ||
| func StartMockServer(t *testing.T, closeWS <-chan []byte) (*httptest.Server, chan<- string, <-chan string, <-chan error, error) { | ||
| func GetMockServer(t *testing.T, closeWS <-chan []byte) (*httptest.Server, chan<- string, <-chan string, <-chan error, error) { |
There was a problem hiding this comment.
"Should we gate this code path with an 'allowUnsecureACS' option?"
So my argument against is that we do allow using an http endpoint to talk to ECS via the SDK. If there were a flag, it should apply to both.
I'm somewhat okay with it being ungated since the endpoint is not actually configurable; you only get the endpoint from the result of a |
Summary
This pull requests allows the wsclient (and therefore acs and tcs) to connect over http and upgrade to a plain websocket connection.
Implementation details
wss (secure websocket) was hardcoded. This change makes a determination based on http or https. Maybe there will be some crazy httpX in the future so I made it a switch.
Testing
make release)go build -out amazon-ecs-agent.exe ./agent)make test) passgo test -timeout=25s ./agent/...) passmake run-integ-tests) pass.\scripts\run-integ-tests.ps1) passmake run-functional-tests) pass.\scripts\run-functional-tests.ps1) passNew tests cover the changes: yes
Description for the changelog
Enhancement - Allow plain HTTP connections through wsclient
Licensing
This contribution is under the terms of the Apache 2.0 License: yes