Add the config_entry resource

[remilapeyre]

No description provided.

[banks]

He @remilapeyre this is looking great!

I think it would be even better if we could make these more generic so that we lower the overhead to maintain the schema and docs as we add more types (we've already added 3 more to be released this week for example!). I'm not familiar enough with TF provider code to know the best way to do that but please let me know what you think.

[remilapeyre]

Hi @banks, I've been trying to use a more generic config attribute in the Terraform schema but get stuck because Terraform wants a strict schema so I could find nothing that works for every ConfigEntry types.

What if we just use a config string field in which the user can write arbitrary HCL that we parse and give to structs.DecodeConfigEntry?

For example :

resource "consul_config_entry" "service-router" {
  kind = "service-router"
  name = "web"
  
  config = <<-EOF
    routes = [
      {
        match {
          http {
            path_prefix = "/admin"
          }
        }

        destination {
          service = "admin"
        }
      }
    ]
  EOF
}

This could make it work albeit the integration with Terraform and user-experience might not be the best.

[banks]

Sorry it took so long to respond @remilapeyre - HashiConf was upon us last week!

I'm honestly not sure about that UX - I think it would be good to loop in some Terraform folks here to steer us towards the best path. There may well be good reasons not to do the embedded HCL route beyond just looking a bit ugly that are too subtle for my experience to tease out.

I'll ask around internally first and see what the consensus is.

[banks]

Hey @remilapeyre I have spoken to our Terraform Gurus (@paultyng) and have some good ideas now.

The Future

First up the TF providers team are trying to solve this very problem for Kube right now - 0.12 dynamic types make it possible in the language to do much better but the SDK to support that in providers is still work in progress.

Even when it does land, it will only work for 0.12 so keep an eye out for a major SDK update soon but we need another solution medium term for 0.11 support anyway.

The Present

The present prior art here is pretty much what you came up with. You probably know this but for the purpose of future readers, Nomad job files can inline HCL or JSON: https://www.terraform.io/docs/providers/nomad/r/job.html and AWS IAM policies inline JSON: https://www.terraform.io/docs/providers/aws/r/iam_policy.html.

Paul's suggestion for us now is that we go with inlining the Config Entry payload but he proposes that we do it as JSON (or at least support JSON too - Nomad can do either). The main reason for this is that in 0.12 people can use the (jsonencode)[https://www.terraform.io/docs/configuration/functions/jsonencode.html] built-in function which means they can write pure HCL objects which will still syntax highlight and allow interpolation etc.

resource "consul_config_entry" "service-router" {
  kind = "service-router"
  name = "web"
  
  config_json = jsonencode({
    routes = [
      {
        match {
          http {
            path_prefix = "/admin"
          }
        }

        destination {
          service = "admin"
        }
      }
    ]
  })
}

I'd be fine with just JSON for now and possibly naming the field explicitly like in example above so that when we do get to update to use 0.12 dynamic types that can be added in a backwards compatible way with just config or similar.

What do you think?

[remilapeyre]

Hi @banks, thanks for this feedback!

I made the changes to have the configuration in a config_json attribute and updated the documentation to show an example with all config entry kinds.

To update config_json when reading from Consul I had to marshal, unmarshal and remove some fields at https://github.com/terraform-providers/terraform-provider-consul/pull/127/files#diff-c8616d95befb55df565252daa4b44f84R125-R134, it's not great but I'm not sure it's possible to better as there is no helper function to do this in github.com/hashicorp/consul/api.

An other thing to note, is that if the user does not set fields with default values like MeshGateway (https://github.com/terraform-providers/terraform-provider-consul/pull/127/files#diff-88aef67908852df198cd4fd86a154965R78), the next plan will be dirty. I think it could be possible to make them optional by creating two ConfigEntry objects, one with the local configuration and one with thee one read from the API and comparing those objects instead of the JSON strings. I'm not sure it's worth it though.

If the user set a configuration like https://github.com/terraform-providers/terraform-provider-consul/pull/127/files#diff-88aef67908852df198cd4fd86a154965R226-R235, Consul will silently replace the name attribute which means we won't be able to read back the config entry from the API and an error will be raised (https://github.com/terraform-providers/terraform-provider-consul/pull/127/files#diff-88aef67908852df198cd4fd86a154965R66). We could hardcode the cases where this happen but it would get cumbersome to maintain and would break as soon as Consul supports namespaces.

Let me know what you think about this implementation, I hope the extensive example in thee documentation will make it easy enough for the user to adapt it to its need despite the less than ideal API :)

[banks]

Thanks @remilapeyre!

I had to marshal, unmarshal

Commented inline but seems like the best option for now.

if the user does not set fields with default values like MeshGateway the next plan will be dirty

That does seem a bit gross - wont that happen in pretty much every case? I think it's worth doing the deep-equals comparison of structs if it helps avoid that as we expect most fields to be defaulted most of the time especially as we add more. But maybe I'm misunderstanding how often this might trip people up?

Consul will silently replace the name attribute which means we won't be able to read back the config entry from the API and an error will be raised

Yeah that ones a bit gross but you're right hard coding exceptions is ugly and Consul will change this behaviour "Soon". I guess the key would be documenting the valid value here well, and maybe even calling out the potential error if it's typoed or misconfigured?

What do you think @remilapeyre about adding a "notice" to the docs under the name field that calls out proxy-defaults only supports global and any other value will cause an error? We'd need to update that notice when namespace support lands but then I think we'll need a bunch of changes to the provider at that point so maybe we can leave a TODO or something we'll remember to grep for there so we don't forget?

[remilapeyre]

That does seem a bit gross - wont that happen in pretty much every case? I think it's worth doing the deep-equals comparison of structs if it helps avoid that as we expect most fields to be defaulted most of the time especially as we add more. But maybe I'm misunderstanding how often this might trip people up?

One thing that worries me with doing deep compares is that api.DecodeConfigEntry will happily ignore any mistyped or snake cased attributes, for example this configuration:

resource "consul_config_entry" "proxy_defaults" {
  kind = "proxy-defaults"
  name = "global"

  config_json = jsonencode({
    mesh_gateway = {
      mode = "local"
    }
    Config = {
      local_connect_timeout_ms = 1000
      handshake_timeout_ms     = 10000
    }
  })
}

will actually create this config entry:

    {
        "Kind": "proxy-defaults",
        "Name": "global",
        "Config": {
            "handshake_timeout_ms": 10000,
            "local_connect_timeout_ms": 1000
        },
        "MeshGateway": {},
        "CreateIndex": 16,
        "ModifyIndex": 40
    }

and it seems to me that using a deep-equals comparison would not detect any difference here and there is no way to detect that the mesh_gateway field has been ignored by api.DecodeConfigEntry.

What do you think @remilapeyre about adding a "notice" to the docs under the name field that calls out proxy-defaults only supports global and any other value will cause an error?

This is a good idea, this way we keep the same behavior as Consul HTTP API but warn the user. I will add this.

[remilapeyre]

Hi @banks, if you are OK with it I propose we merge with the current behavior and we improve over it when the support for multiple schemas land in the Terraform SDK.

[banks]

Just realised I never submitted these with my last comments 2 weeks ago...

[banks]

Sorry @remilapeyre, I read your last update but never responded.

In your example for why deep equals was bad, is that different from not doing a deep equal check?

If we don't accept a snake_case version in the api.DecodeConfigEntry then using snake case will always cause that issue (what is submitted to server won't include the field) whether we do deep equal checking or not wouldn't it? Do we get any better error message now that would help the user?

What would you need exposed in the api package to make it clean? If we could expose the snake -> camel normalising in api.DecodeConfigEntry for example would that make things work better? That seems totally reasonable to me.

[banks]

To be clearer. I think my confusion is that as far as I understand, the terraform in your example would actually not create a ConfigEntry with an empty MeshGateway field - I think our server would translate snake to camel on the way in and store it as desired.

There is still the issue of the TF provider comparison of the CamelCase output to what the user provided, but if we made the same normalisation the server uses available in the api package does that solve it?

Another thing we could do (and arguably should have done) would be to make MeshGateway a pointer to struct rather than an inline struct so it actually gets omitted properly. We'll think about that too as I think that is the worst offender here. But that breaks backwards compat in the api module technically so we'd need to think about it some more.

In fact is that the only problematic field for defaults?

[remilapeyre]

Hi @banks, I finally came back to this, I think the UX is better now:

• Optional fields can be omitted from the config and won't generate a dirty plan anymore,
• Extra invalid fields won't be silently discarded anymore but will display an error to the end user

It seems to me that this the best we can do until the support for dynamic fields is ready.

[banks]

@remilapeyre amazing job thanks for getting those UX improvements in.

I'm approving as this all looks good although I'd be keen if possible to remove the unnecessary MeshGateway: {} lines from the docs. Tests matter less although I'd probably stick with the more common pattern of not specifying the empty field in most cases there too.

Thanks again, this is great work and we have a few people asking for this already!