Add support for TLSSkipVerify for https consul fabio check #268
Conversation
|
|
config/default.go
Outdated
| Register: true, | ||
| ServiceAddr: ":9998", | ||
| ServiceName: "fabio", | ||
| ServiceTLSSkipVerify: false, |
There was a problem hiding this comment.
You can omit that since this is the default
config/load_test.go
Outdated
| @@ -426,6 +426,13 @@ func TestLoad(t *testing.T) { | |||
| }, | |||
| }, | |||
| { | |||
| args: []string{"-registry.consul.register.tlsskipverify=false"}, | |||
There was a problem hiding this comment.
pls test with true since false is the default
config/config.go
Outdated
| Register bool | ||
| ServiceAddr string | ||
| ServiceName string | ||
| ServiceTLSSkipVerify bool |
There was a problem hiding this comment.
This should be CheckTLSSkipVerify and pls move it to the bottom below CheckScheme
config/load.go
Outdated
| @@ -158,6 +158,7 @@ func load(cmdline, environ, envprefix []string, props *properties.Properties) (c | |||
| f.BoolVar(&cfg.Registry.Consul.Register, "registry.consul.register.enabled", defaultConfig.Registry.Consul.Register, "register fabio in consul") | |||
| f.StringVar(&cfg.Registry.Consul.ServiceAddr, "registry.consul.register.addr", defaultConfig.Registry.Consul.ServiceAddr, "service registration address") | |||
| f.StringVar(&cfg.Registry.Consul.ServiceName, "registry.consul.register.name", defaultConfig.Registry.Consul.ServiceName, "service registration name") | |||
| f.BoolVar(&cfg.Registry.Consul.ServiceTLSSkipVerify, "registry.consul.register.tlsskipverify", defaultConfig.Registry.Consul.ServiceTLSSkipVerify, "service tls verification") | |||
There was a problem hiding this comment.
pls change to registry.consul.register.checkTLSSkipVerify since I've - unfortunately - used camelCase with the other check parameters as well. The comment should be service check TLS verifcation
Ginja
force-pushed
the
service-tls-skip-verification
branch
from
402ce99
to
4946b9e
Compare
|
@magiconair Thanks for the review! Made the changes, and squashed everything into one commit. |
Ginja
force-pushed
the
service-tls-skip-verification
branch
from
4946b9e
to
3703802
Compare
config/default.go
Outdated
| CheckInterval: time.Second, | ||
| CheckTimeout: 3 * time.Second, | ||
| CheckScheme: "http", | ||
| CheckTLSSkipVerify: false, |
There was a problem hiding this comment.
pls remove this since false is the default
There was a problem hiding this comment.
It is, but then how do I expose the parameter?
Do you want defaultConfig.Registry.Consul.CheckTLSSkipVerify just to be false in the link above?
There was a problem hiding this comment.
The field itself is defined in config.Config and not here. The value that you are setting in defaultConfig is automatically set by the compiler and therefore redundant. false is the default value for any bool variable.
There was a problem hiding this comment.
Edit: Ignore that last question, need more coffee this morning! I'll update this PR later today.
Thanks for your help!
|
|
|
You should remove the line from the defaultConfig only. Leave the rest.
|
When serving the UI/API over HTTPS, the fabio consul health check will most likely fail as it's trying to connect to the page over its IP. Most certificates don't include IP SANs. This flag allows users to toggle whether or not to skip TLS verification for this particular check.
Ginja
force-pushed
the
service-tls-skip-verification
branch
from
3703802
to
0db7b84
Compare
|
Changes made. |
Fix text and whitespace
magiconair
changed the title
|
Made some minor text changes in the |
When serving the UI/API over HTTPS, the fabio consul health check will most likely fail as it's trying to connect to the page over its IP. Most certificates don't include IP SANs. This flag allows users to toggle whether or not to skip TLS verification for this particular check.
Edit: This will require Consul >= 0.7.2
hashicorp/consul#1984
hashicorp/consul#2530