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

consul_key_prefix: Fix update of subkey blocks. #189

Merged
merged 4 commits into from
Apr 11, 2020

Conversation

cyrilgdn
Copy link
Contributor

@cyrilgdn cyrilgdn commented Apr 2, 2020

Description

Resource consul_key_prefix: Update of subkey blocks does not work properly.

In the first place, we find out that if one or more keys (created with subkey block) are manually removed in Consul, the provider does not detect it and doesn't recreate the missing key(s) (even if there's not key at all under this prefix).

When writing the fix for that, I find out that even adding new subkey blocks does not work properly.
It will add the new key but remove the existing ones (because of this continue which continues the wrong for loop)

Then the provider will not detect that the mistakenly deleted keys are missing.

How to reproduce

  • I start a Consul server in a Docker container with
$ docker run --rm -p 8500:8500 consul

With that the default configuration of the provider is working.

  • Create a test file with a consul_key_prefix resource with one subkey
resource "consul_key_prefix" "test" {
  path_prefix = "test/"

  subkey {
    path  = "key1"
    value = "plop"
  }
}
  • Apply it
$ terraform init
[...]
$ terraform apply
[...]
Terraform will perform the following actions:

  # consul_key_prefix.test will be created
  + resource "consul_key_prefix" "test" {
      + datacenter  = (known after apply)
      + id          = (known after apply)
      + path_prefix = "test/"

      + subkey {
          + flags = 0
          + path  = "key1"
          + value = "plop"
        }
    }

Plan: 1 to add, 0 to change, 0 to destroy.
[...]
Apply complete! Resources: 1 added, 0 changed, 0 destroyed.
  • Check keys in Consul:
$ curl "localhost:8500/v1/kv/test?recurse=true&keys=true"
[
    "test/key1"
]
  • Add a second key:
resource "consul_key_prefix" "test" {
  path_prefix = "test/"

  subkey {
    path  = "key1"
    value = "plop"
  }

  subkey {
    path  = "key2"
    value = "plop"
  }
}
  • And apply
$ terraform apply
consul_key_prefix.test: Refreshing state... [id=test/]
[...]
  # consul_key_prefix.test will be updated in-place
  ~ resource "consul_key_prefix" "test" {
        datacenter  = "dc1"
        id          = "test/"
        path_prefix = "test/"
        subkeys     = {}

        subkey {
            flags = 0
            path  = "key1"
            value = "plop"
        }
      + subkey {
          + flags = 0
          + path  = "key2"
          + value = "plop"
        }
    }

Plan: 0 to add, 1 to change, 0 to destroy.
[...]
Apply complete! Resources: 0 added, 1 changed, 0 destroyed.

The plan is OK but it does not what the plan says, here are the keys in Consul:

$ curl "localhost:8500/v1/kv/test?recurse=true&keys=true"
[
    "test/key2"
]

It created the second key but removed the first one.
Further plans will not detect the missing key:

$ terraform plan
[...]
consul_key_prefix.test: Refreshing state... [id=test/]

------------------------------------------------------------------------

No changes. Infrastructure is up-to-date.

@ghost ghost added the size/L label Apr 2, 2020
@cyrilgdn cyrilgdn force-pushed the fix-consul-key-prefix-update branch from 8c752a4 to cfab3ed Compare April 2, 2020 15:40
@cyrilgdn
Copy link
Contributor Author

cyrilgdn commented Apr 2, 2020

Looking at the failing test

@cyrilgdn cyrilgdn force-pushed the fix-consul-key-prefix-update branch from cfab3ed to 988930b Compare April 2, 2020 15:57
@cyrilgdn
Copy link
Contributor Author

cyrilgdn commented Apr 2, 2020

As I added subkey blocks in testAccConsulKeyPrefixConfig_Update definition (to assert the update is working as expected), the test for terraform import fails because it's not able to determine if these keys should be part of the subkeys map or the subey blocks.

As I think there's no way to do that and it's not linked to this bug fix, I created a specific test definition for the import test with only subkeys (like it was before).

@remilapeyre
Copy link
Contributor

Hi @cyrilgdn, thanks for looking into this issue, your fix is correct. I'm looking into the issue with the import and will make a complete review soon.

@remilapeyre
Copy link
Contributor

I could not find a way to improve the import support but it's not huge inconvenience anyway.

Thanks for the fix!

@remilapeyre remilapeyre merged commit 0bca9b5 into hashicorp:master Apr 11, 2020
@cyrilgdn cyrilgdn deleted the fix-consul-key-prefix-update branch April 12, 2020 13:47
@cyrilgdn
Copy link
Contributor Author

cyrilgdn commented May 1, 2020

@remilapeyre Thanks 😊 ! Do you have an idea on when it could be released?

@remilapeyre
Copy link
Contributor

Hi @cyrilgdn, a new release has just been made!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants