Fix TLS events #177
Fix TLS events #177
Conversation
Anyone who needs to override HTTPClient.Transport for TLS will also need to set, TLSConfig to support event monitoring, so let's export it
|
I've actually got a test ready for this now. It doesn't verify the certificates, but it works. I'll push shortly. |
The PEM files here were generated for the test and are no longer in use
|
Sorry for the noisy commits on this PR. I can squash it if you like. I think this code is good to go, assuming someone else can test it out. Without these changes, TLS support is incomplete since the event monitoring API cannot be used. |
| @@ -581,7 +585,11 @@ func parseEndpoint(endpoint string) (*url.URL, error) { | |||
| return nil, ErrInvalidEndpoint | |||
| } | |||
| if u.Scheme == "tcp" { | |||
| u.Scheme = "http" | |||
| if strings.HasSuffix(u.Host, ":2376") { | |||
There was a problem hiding this comment.
This method of choosing a scheme should probably be overridable through the environment, and the port should probably be determined by net.SplitHostPort(u.Host) - for example: https://github.com/rafecolton/docker-builder/blob/master/dclient/dclient.go#L172-L186
There was a problem hiding this comment.
That seems like a reasonable suggestion. I went with the approach I did based on this comment by @fsouza.
I'm happy to update this if there's some agreement to take your approach (or some compromise or other approach), but I mainly want the DOCKER_HOST=tcp://X.X.X.X:2376 form used by the docker client and boot2docker to be supported without alteration as the endpoint argument to NewTLSClient.
There was a problem hiding this comment.
I'm totally fine with 2376 => https as the default. The issue is that since the docker daemon allows the user to pass -H with whatever option they want, on any machine that's not boot2docker, the user may be using a different port. I'd hate for somebody to have to change the settings of their docker daemon (and reboot it!) just to use the go-dockerclient
There was a problem hiding this comment.
@rafecolton in this case, the user could use https://: as the endpoint.
@md5 could you change the code to use net.SplitHostPort instead of strings.HasSuffix? The change looks good otherwise.
Thank you for working on it!
|
Thanks for adding this! |
|
@rafecolton thanks for taking a look at the code. |
|
+1 |
|
@md5 thanks for working on it! |
|
Glad to help. |
Event monitoring is not working under TLS because it isn't using Client.HTTPClient. This patch fixes it.
Includes a test using TLS mutual authentication. Probably needs some negative testing (e.g connecting HTTP client to HTTPS server or vice versa) to better validate error handling.