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

ACL Auth Method is not updated correctly #240

Closed
whiskeysierra opened this issue Feb 5, 2021 · 10 comments · Fixed by #244
Closed

ACL Auth Method is not updated correctly #240

whiskeysierra opened this issue Feb 5, 2021 · 10 comments · Fixed by #244

Comments

@whiskeysierra
Copy link

whiskeysierra commented Feb 5, 2021

Hi there,

Thank you for opening an issue. Please note that we try to keep the Terraform issue tracker reserved for bug reports and feature requests. For general usage questions, please see: https://www.terraform.io/community.html.

Terraform Version

Terraform v0.14.5

Affected Resource(s)

Please list the resources as a list, for example:

  • consul_acl_auth_method

If this issue appears to affect multiple resources, it may be an issue with Terraform's core, so please mention this.

Terraform Configuration Files

data "kubernetes_secret" "consul-server-token" {
  metadata {
    name      = kubernetes_service_account.consul-server.default_secret_name
    namespace = kubernetes_namespace.consul.metadata[0].name
  }

  depends_on = [kubernetes_service_account.consul-server]
}

# TODO this doesn't properly update the ServiceAccountJWT if it changes
//noinspection MissingProperty
resource "consul_acl_auth_method" "default" {
  name = var.k8s.name
  type = "kubernetes"
  config_json = jsonencode({
    Host              = var.k8s.kube_config.host
    CACert            = var.k8s.kube_config.cluster_ca_certificate
    ServiceAccountJWT = data.kubernetes_secret.consul-server-token.data["token"]
  })
}

Debug Output

Please provider a link to a GitHub Gist containing the complete debug output: https://www.terraform.io/docs/internals/debugging.html. Please do NOT paste the debug output in the issue; just paste a link to the Gist.

terraform plan output
  # module.consul.consul_acl_auth_method.default will be updated in-place
  ~ resource "consul_acl_auth_method" "default" {
      ~ config         = {
          - "CACert"            = <<-EOT
                -----BEGIN CERTIFICATE-----
                <snip>
                -----END CERTIFICATE-----
            EOT -> null
          - "Host"              = "https://my.hcp.westeurope.azmk8s.io:443" -> null
          - "ServiceAccountJWT" = "<old-value>" -> null
        }
        id             = "auth-method-aks-k8s-network-alpha"
        name           = "aks-k8s-network-alpha"
        # (6 unchanged attributes hidden)
    }

Panic Output

If Terraform produced a panic, please provide a link to a GitHub Gist containing the output of the crash.log.

Expected Behavior

What should have happened?

ServiceAccountJWT (or rather config_json as a whole) should have been updated.

Actual Behavior

What actually happened?

Update never happens. If I run consul acl auth-method read -name <name> after terraform apply it still shows the old JWT. Consequentially consul login fails with a 500.

Steps to Reproduce

Please list the steps required to reproduce the issue, for example:

  1. terraform apply (initial)
  2. Force an update of the service account token, e.g. delete the service account or rename it or something
  3. terraform apply
  4. Observe no update in consul

Important Factoids

Are there anything atypical about your accounts that we should know? For example: Running in EC2 Classic? Custom version of OpenStack? Tight ACLs?

  • Azure AKS
  • Default policy deny
  • Kubernetes Auth method w/ service account token

References

Are there any other GitHub issues (open or closed) or Pull Requests that should be linked here? For example:

@whiskeysierra
Copy link
Author

whiskeysierra commented Feb 17, 2021

I worked around the issue by including a hash in the name because that forces a replacement:

locals {
  # Needed to workaround https://github.com/hashicorp/terraform-provider-consul/issues/240
  # We can't use random_id resource, because our keepers are sensitive
  hash = substr(sha256(join("", [
    var.k8s.kube_config.host,
    var.k8s.kube_config.cluster_ca_certificate,
    data.kubernetes_secret.consul-server-token.data["token"],
  ])), 0, 7)
}

//noinspection MissingProperty
resource "consul_acl_auth_method" "default" {
  name = "${var.k8s.name}-${local.hash}"
  type = "kubernetes"
  config_json = jsonencode({
    Host              = var.k8s.kube_config.host
    CACert            = var.k8s.kube_config.cluster_ca_certificate
    ServiceAccountJWT = data.kubernetes_secret.consul-server-token.data["token"]
  })
}

For me that's OK because all the places where I refer to that auth method by name are managed within the same terraform module, so they would be updated/replaced automatically as well.

@whiskeysierra
Copy link
Author

I worked around the issue by including a hash in the name because that forces a replacement:

And I was only able to perform a valid update of any kind by forcing a full replacement.

@whiskeysierra
Copy link
Author

My workaround actually breaks all clients that are using a token that was issued by that auth method. The moment the auth method is deleted and re-created, all of the associated ACL tokens become invalid and will be rejected.

@whiskeysierra
Copy link
Author

The plan reports changes on the "config" property which I'm not even using. I'm using the "config_json" version because "config" is deprecated. But I saw that after the initial apply, terraform will still read "config" and detect incorrect changes. I tried to work around that by ignoring changes to "config". Maybe the handling of "config" vs "config_json" both in terraform and my usage of it might be the problem here.

@whiskeysierra
Copy link
Author

I debugged it a bit more and using config_json with an ignore changes on config will never cause an update. For some reason, there are no changes detected in config_json.

resource "consul_acl_auth_method" "default" {
  name = var.k8s.name
  type = "kubernetes"
  config_json = jsonencode({
    Host              = var.k8s.kube_config.host
    CACert            = var.k8s.kube_config.cluster_ca_certificate
    ServiceAccountJWT = data.kubernetes_secret.consul-server-token.data["token"]
  })

  lifecycle {
    # reports changes incorrectly
    ignore_changes = [config]
  }
}

@whiskeysierra
Copy link
Author

If I remove the ignore_changes/lifecycle block, then there will always changes be reported (always being changed to null) but never actually applied in a way that it can be observed in consul.

@whiskeysierra
Copy link
Author

whiskeysierra commented Feb 25, 2021

Changing from config_json back to config causes:

When expanding the plan for
module.main.module.consul.consul_acl_auth_method.default to include new values
learned so far during apply, provider "registry.terraform.io/hashicorp/consul"
produced an invalid new value for .config_json: was null, but now
cty.StringVal("{"CACert":...

This is a bug in the provider, which should be reported in the provider's own
issue tracker.

@whiskeysierra
Copy link
Author

So I tried two different ways, both starting from a clean state:

  1. Create an auth method with config and update the jwt
  2. Create an auth method with config_json and update the jwt

Both will plan a change and claim that they applied it but they never do. The jwt never updates and a new plan is always dirty, i.e. the change never happened.

@whiskeysierra
Copy link
Author

I guess the issue that changes to config need to be ignored if config_json is used is due to the missing DiffSuppressFunc for config?

"config": {
Type: schema.TypeMap,
Optional: true,
Description: "The raw configuration for this ACL auth method.",
Deprecated: "The config attribute is deprecated, please use config_json instead.",
Elem: &schema.Schema{
Type: schema.TypeString,
},
ConflictsWith: []string{"config_json"},
},
"config_json": {
Type: schema.TypeString,
Optional: true,
Description: "The raw configuration for this ACL auth method.",
ConflictsWith: []string{"config"},
DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
config := d.Get("config").(map[string]interface{})
return len(config) != 0
},
},

@jbarnes7952 jbarnes7952 mentioned this issue Mar 3, 2021
2 tasks
remilapeyre added a commit that referenced this issue May 9, 2021
@remilapeyre
Copy link
Contributor

Hi @whiskeysierra, thanks for reporting the issue and looking into it. The consul_acl_auth_method does not have a schema that can be represented in Terraform nicely with the Terraform SDK v1, I plan to update to the v2 in the coming weeks when we drop the support for Terraform 0.11.

In the meantime #244 should fix the diff suppression logic. This change should be released n the coming days.

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

Successfully merging a pull request may close this issue.

2 participants