Added HTTPS support via a new HTTPS Port configuration option #478
Conversation
… to the HTTP Port.
| var err error | ||
| var servers []*HTTPServer | ||
|
|
||
| if config.Ports.HTTPS > 0 { |
There was a problem hiding this comment.
It might make tests easier if this test was for >= 0. That way we can just bind to port 0 for tests to get a vacant port.
There was a problem hiding this comment.
I left this logic the way it was for just the HTTP case before. Also, using 0 will mean that the underlying OS will use the next available dynamic port which is not what we want since HTTP/HTTPS need to be on known ports for clients to communicate with them and so it would be better not to get into that situation by keeping the test > 0 like it was already for HTTP.
|
@amalaviy This is looking good. I've left a few minor comments above. Other than that, if we can get some tests and comment all the methods in here, we will be in good shape! |
|
Sounds good, let me go through the comments and will update tomorrow. On Nov 17, 2014 6:08 PM, Ryan Uber notifications@github.com wrote: @amalaviyhttps://github.com/amalaviy This is looking good. I've left a few minor comments above. Other than that, if we can get some tests and comment all the methods in here, we will be in good shape! — |
|
Quick thoughts:
|
| @@ -93,7 +94,16 @@ func NewClient(config *Config) (*Client, error) { | |||
| // Create the tlsConfig | |||
| var tlsConfig *tls.Config | |||
| var err error | |||
| if tlsConfig, err = config.OutgoingTLSConfig(); err != nil { | |||
| tlsConf := &tlsutil.Config{ | |||
There was a problem hiding this comment.
Can we add a helper to consul.Config() to generate the tlsutil.Config{} struct? This way we can avoid repetition of this setup. We can also do config.tlsConfig().OutgoingTLSConfig() this way by chaining
…cond keepalive, use keepalive for HTTP and HTTPS
|
LGTM! Thanks! |
Added HTTPS support via a new HTTPS Port configuration option
Removes default service name annotation Previously, this was always added to connect-injected pods. With Tproxy, the Consul service name will be determined by the Kubernetes service name and the annotation can optionally override that. All of the checks for whether service account name doesn't match the Consul service name have been moved to the connect-init command so users will see the error if appropriate during Pod startup, since that's where we can get Consul service name information.
Similar to the HTTP Port, defaults to disabled.