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 insecure option as requested in GH-6 #31

Merged

Conversation

natefaerber
Copy link
Contributor

#6

This adds the insecure option. I used insecure instead of ssl_verify because I saw the same option in OpenStack provider so thought it would be more acceptable. https://github.com/terraform-providers/terraform-provider-openstack/blob/master/openstack/config.go#L28

I haven't been able to do a real world test against an insecure endpoint. I might be able to do that tomorrow.

$ make testacc TESTARGS='-run=TestResourceProvider'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestResourceProvider -timeout 120m
?   	github.com/terraform-providers/terraform-provider-consul	[no test files]
=== RUN   TestResourceProvider
--- PASS: TestResourceProvider (0.00s)
=== RUN   TestResourceProvider_impl
--- PASS: TestResourceProvider_impl (0.00s)
=== RUN   TestResourceProvider_Configure
2017/12/26 21:52:13 [INFO] Initializing Consul client
2017/12/26 21:52:13 [INFO] Consul Client configured with address: 'demo.consul.io:80', scheme: 'https', datacenter: 'nyc3', insecure: 'false'
--- PASS: TestResourceProvider_Configure (0.00s)
=== RUN   TestResourceProvider_ConfigureTLS
2017/12/26 21:52:13 [INFO] Initializing Consul client
2017/12/26 21:52:13 [INFO] Consul Client configured with address: 'demo.consul.io:80', scheme: 'https', datacenter: 'nyc3', insecure: 'false'
--- PASS: TestResourceProvider_ConfigureTLS (0.00s)
=== RUN   TestResourceProvider_ConfigureTLSInsecure
2017/12/26 21:52:13 [INFO] Initializing Consul client
2017/12/26 21:52:13 [INFO] Consul Client configured with address: 'demo.consul.io:80', scheme: 'https', datacenter: 'nyc3', insecure: 'true'
--- PASS: TestResourceProvider_ConfigureTLSInsecure (0.00s)
PASS
ok  	github.com/terraform-providers/terraform-provider-consul/consul	0.020s

@natefaerber natefaerber changed the title Add insecure option as requested in terraform-providers/terraform-provider-consul#6 Add insecure option as requested in GH-6 Dec 27, 2017
@natefaerber
Copy link
Contributor Author

Note that this would close #4, too.

@apparentlymart
Copy link
Contributor

Hi @natefaerber! Thanks for working on this.

The functionality here looks good to me. The name feels a little off though, since "insecure" sounds to me like "disable TLS" rather than "skip certificate verification".

What do you think about calling it insecure_https instead, to clarify that it's a modifier for the HTTPS behavior rather than disabling HTTPS altogether? Ideally this would also raise an error if scheme is set to anything other than https, to further clarify that this setting adjusts the behavior for HTTPS mode.

@@ -37,6 +39,12 @@ func (c *Config) Client() (*consulapi.Client, error) {
tlsConfig.CAFile = c.CAFile
tlsConfig.CertFile = c.CertFile
tlsConfig.KeyFile = c.KeyFile
if c.InsecureHttps {
if config.Scheme != "https" {
return nil, fmt.Errorf("insecure_https is meant to be used when scheme is https")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Error string if insecure_https is set but scheme is not https.

}
}

func TestResourceProvider_ConfigureTLSInsecureHttpsMismatch(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tests that we error if insecure_https is used without https

@natefaerber
Copy link
Contributor Author

Good idea, @apparentlymart. I have made these changes. Please review the strings to see if they make sense.

$ make testacc TESTARGS='-run=TestResourceProvider'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestResourceProvider -timeout 120m
?   	github.com/terraform-providers/terraform-provider-consul	[no test files]
=== RUN   TestResourceProvider
--- PASS: TestResourceProvider (0.00s)
=== RUN   TestResourceProvider_impl
--- PASS: TestResourceProvider_impl (0.00s)
=== RUN   TestResourceProvider_Configure
2018/01/06 11:03:58 [INFO] Initializing Consul client
2018/01/06 11:03:58 [INFO] Consul Client configured with address: 'demo.consul.io:80', scheme: 'https', datacenter: 'nyc3', insecure_https: 'false'
--- PASS: TestResourceProvider_Configure (0.00s)
=== RUN   TestResourceProvider_ConfigureTLS
2018/01/06 11:03:58 [INFO] Initializing Consul client
2018/01/06 11:03:58 [INFO] Consul Client configured with address: 'demo.consul.io:80', scheme: 'https', datacenter: 'nyc3', insecure_https: 'false'
--- PASS: TestResourceProvider_ConfigureTLS (0.00s)
=== RUN   TestResourceProvider_ConfigureTLSInsecureHttps
2018/01/06 11:03:58 [INFO] Initializing Consul client
2018/01/06 11:03:58 [INFO] Consul Client configured with address: 'demo.consul.io:80', scheme: 'https', datacenter: 'nyc3', insecure_https: 'true'
--- PASS: TestResourceProvider_ConfigureTLSInsecureHttps (0.00s)
=== RUN   TestResourceProvider_ConfigureTLSInsecureHttpsMismatch
2018/01/06 11:03:58 [INFO] Initializing Consul client
--- PASS: TestResourceProvider_ConfigureTLSInsecureHttpsMismatch (0.00s)
PASS
ok  	github.com/terraform-providers/terraform-provider-consul/consul	0.030s

Copy link
Contributor

@vancluever vancluever left a comment

Choose a reason for hiding this comment

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

This looks good and I've tested the connection on a local Consul dev container with TLS enabled on a self-signed cert. Good work! 👍

@vancluever vancluever merged commit 1c0c165 into hashicorp:master Feb 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants