Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tls client options to api/cli #2914

Merged
merged 3 commits into from
Apr 14, 2017
Merged

Add tls client options to api/cli #2914

merged 3 commits into from
Apr 14, 2017

Conversation

kyhavlov
Copy link
Contributor

This adds TLS client options to the api and cli for providing client certs and setting the TLS server name when connecting to Consul.

@kyhavlov kyhavlov requested a review from slackpad April 14, 2017 20:42
Copy link
Contributor

@slackpad slackpad left a comment

Choose a reason for hiding this comment

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

Just one fix to the help text, looks good!

"can also be specified via the CONSUL_CACERT environment variable.")
f.Var(&c.caPath, "ca-path",
"Path to a directory of CA certificates to use for TLS when communicating "+
"with Consul. is enabled. This can also be specified via the CONSUL_CAPATH "+
Copy link
Contributor

Choose a reason for hiding this comment

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

"when `verify_incoming` is enabled"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shouldn't the "is enabled." just be removed here? the CA settings can be used even when verify_incoming isn't enabled

Copy link
Contributor

Choose a reason for hiding this comment

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

that's true

@kyhavlov kyhavlov merged commit 43818f5 into master Apr 14, 2017
@kyhavlov kyhavlov deleted the tls-client-options branch April 14, 2017 21:55
@@ -347,6 +368,41 @@ func NewClient(config *Config) (*Client, error) {
config.HttpClient = defConfig.HttpClient
}
Copy link

Choose a reason for hiding this comment

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

So, with below I can pass my own HttpClient, with my own RoundTripper

if config.HttpClient == nil {
    config.HttpClient = defConfig.HttpClient
}

but the line

config.HttpClient.Transport.(*http.Transport).TLSClientConfig = tlsClientConfig

brakes it as it assumes standard RoundTripper implementation

@arvenil
Copy link

arvenil commented Apr 18, 2017

One way to solve that is to add new func NewHttpClient that will return ready to use consul http client - which is logical as this is what consul tries to do, provide his own HttpClient. Then user can use/modify it and pass to NewClient() via config. Now NewClient would either use provided HttpClient or if nil then run NewHttpClient() to create default one. This means I could always provide 1) my own client 2) my own client based on default consul client 3) default consul client when I don't provide any; and consul will never panic as it will never try to modify provided client (which he also shouldn't do as it might be default HttpClient).

I might be missing something with my train of thoughts so let me know if that's the case ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants