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

node_config tags shouldn't cause diffs on ordering change #12966

Open
tomasgareau opened this issue Nov 7, 2022 · 3 comments
Open

node_config tags shouldn't cause diffs on ordering change #12966

tomasgareau opened this issue Nov 7, 2022 · 3 comments

Comments

@tomasgareau
Copy link

tomasgareau commented Nov 7, 2022

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment. If the issue is assigned to the "modular-magician" user, it is either in the process of being autogenerated, or is planned to be autogenerated soon. If the issue is assigned to a user, that user is claiming responsibility for the issue. If the issue is assigned to "hashibot", a community member has claimed the issue already.

Description

The google_container_cluster and google_container_node_pool's node_config block should ideally treat tags as an unordered set of tags, rather than an (ordered) list of tags. This would help to avoid unnecessary diffs in case the order of the tags changes.

For example, in the Terraform plan output below, this node pool needs to be replaced because the order of tags changed, even though there are no changes to the set of tags on this node pool:

      # Terraform plan output...
      
      ~ node_config {
          ~ tags              = [ # forces replacement
                # (4 unchanged elements hidden)
                "tag-a",
              - "tag-c",
                "tag-b",
              + "tag-c",
            ]
       
      # more Terraform plan...
        }

The main place I've bumped into this is when using the terraform-google-kubernetes-engine module, which sets the tags variable by concatenating a number of input variables. I've had situations where I've had to move tags from applying to "all" node pools vs a specific node pool, and this changed the order of tags built by the module.

New or Affected Resource(s)

  • google_container_cluster
  • google_container_node_pool

Potential Terraform Configuration

resource "google_container_node_pool" "my_pool" {
  # ...
  node_config {
    tags = [
      "a",
      "b",
      "c",
    ]
  }
}

The change below should cause no diffs:

resource "google_container_node_pool" "my_pool" {
  # ...
  node_config {
    tags = [
      "c",
      "a",
      "b",
    ]
  }
}

Considerations

The GKE API spec for NodeConfig models tags as an (ordered) list of strings.

I don't know if it's the GKE API that determines whether that list of tags forces replacement (in which case we're probably out of luck) or whether the Terraform Google provider can determine that the set of tags has not changed, even if the order has.

b/300616557

@github-actions github-actions bot added service/container forward/review In review; remove label to forward labels Aug 17, 2023
@edwardmedia edwardmedia removed the forward/review In review; remove label to forward label Sep 14, 2023
@wyardley
Copy link

My changes in GoogleCloudPlatform/magic-modules#12014 partially resolve this - there will be a diff on ordering change, but no permadiff, as the tags will get updated in-place and so the diff will resolve after the first apply

I am guessing running a sort() on the tags could help further minimize this?

@roaks3
Copy link
Collaborator

roaks3 commented Oct 23, 2024

Agreed that @wyardley 's fix might help here, I think we should check back once that is merged. It would depend how deterministic things are behaving. But also, the "real" fix here would be to update the implementation to be order-agnostic.

@wyardley
Copy link

Thanks, and agree on the "real" fix (not sure whether the API itself has ordering, though I'm guessing not, so it might be enough to use a fixed order within the state representation?).

I did do a functional test locally with the built provider when I commented, and it did fix the permadiff. So probably this is Good Enough (TM) fix.

It does actually try to update the nodepool, but I'm not sure if the actual labels as returned by the API change at all, or if it just updates the representation of it within Terraform state.

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

No branches or pull requests

6 participants