#7013 add tls config support to consul provider

[maxenglander]

Resolves #7013.

Note that I updated the vendored Consul API by running govendor fetch github.com/hashicorp/consul/api.

[stack72]

Hi @maxenglander

Thanks for the PR here, sorry it has taken so long to get to it! I think in order to merge it, we would really love to see some updates to the documentation to reflect this. Have you been able to test that this works as expected? Could you possibly also turn in an acceptance test as well?

Thanks

Paul

[maxenglander]

Hey @stack72 thanks, and no worries! Will add documentation. Have been using it in production for some time, but will also add an acceptance test. Will ping again when the changes are in.

[stack72]

Awesome! Thanks :)

[maxenglander]

Hey @stack72, I created a test that validates the provider config when ca_file, key_file and ca_cert are used. This test does not connect to a Consul server.

It's also possible to run all of the existing Consul acceptance tests so that any which connect to a Consul agent will use TLS. This requires first setting some environment variables, and pointing the Consul provider to an agent that serves (and optionally authenticates) HTTPS requests.

I included a README which will hopefully make this clear. Please note that, if you configure the acceptance tests to connect to a Consul agent not addressable at 127.0.0.1, the provided key material won't be of any use. In that case, you'll have to generate new ones valid for whatever IP address or hostname routes to your Consul agent.

I hope this is satisfactory - let me know if there's anything I can do to improve this.

[stack72]

Can these values be empty? Or should we check they are set in the config object before being set as a tlsConfig object?

[maxenglander]

consul/api.TLSConfig{CAFile,CertFile,KeyFile} are all string. Their default values are "", so checking to see if terraform/consul.TLSConfig.{CAFile,CertFile,KeyFile} (also string) are "" before assigning is unnecessary.

(Answer to your other question below.)

[stack72]

Hi @maxenglander

So i ran the tests here and they look good. I have a few small questions inline - so if you can have a think about those before i merge, then that would be awesome

TF_ACC=1 go test ./builtin/providers/consul -v -run= -timeout 120m
=== RUN   TestAccDataConsulKeys_basic
--- PASS: TestAccDataConsulKeys_basic (0.03s)
=== RUN   TestAccConsulAgentService_basic
--- PASS: TestAccConsulAgentService_basic (0.02s)
=== RUN   TestAccConsulCatalogEntry_basic
--- PASS: TestAccConsulCatalogEntry_basic (0.03s)
=== RUN   TestAccConsulKeyPrefix_basic
--- PASS: TestAccConsulKeyPrefix_basic (0.08s)
=== RUN   TestConsulKeysMigrateState
--- PASS: TestConsulKeysMigrateState (0.00s)
=== RUN   TestConsulKeysMigrateState_empty
--- PASS: TestConsulKeysMigrateState_empty (0.00s)
=== RUN   TestAccConsulKeys_basic
--- PASS: TestAccConsulKeys_basic (0.05s)
=== RUN   TestAccConsulNode_basic
--- PASS: TestAccConsulNode_basic (0.03s)
=== RUN   TestAccConsulService_basic
--- PASS: TestAccConsulService_basic (0.03s)
=== RUN   TestResourceProvider
--- PASS: TestResourceProvider (0.00s)
=== RUN   TestResourceProvider_impl
--- PASS: TestResourceProvider_impl (0.00s)
=== RUN   TestResourceProvider_Configure
--- PASS: TestResourceProvider_Configure (0.00s)
=== RUN   TestResourceProvider_ConfigureTLS
--- PASS: TestResourceProvider_ConfigureTLS (0.00s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/consul 0.289s

Thanks for all the work here!

Paul

[stack72]

@maxenglander thanks for the very good explanations :) the changes work on a non-tls enabled cluster so we look good to merge!

[maxenglander]

@stack72 Awesome - thanks for the review and merge!

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.