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

Rework the provider's labels model #14293

Closed
rileykarson opened this issue Apr 12, 2023 · 5 comments
Closed

Rework the provider's labels model #14293

rileykarson opened this issue Apr 12, 2023 · 5 comments

Comments

@rileykarson
Copy link
Collaborator

rileykarson commented Apr 12, 2023

Label fields- labels, annotations (a Kubernetes-style variant), and single product variants of the field- are string:string maps in GCP that are managed by multiple owners: the end user, the user's workspace's platform team, the GCP client used to manage the resource, and the API service itself.

Terraform's model for these fields is extremely simple: it thinks it's the exclusive owner of the field, and detects diffs on any labels that are present. This made some amount of sense a few years ago when GCE was the majority share of the provider, but has made increasingly less sense as other clients have begun to manage client-specific metadata in labels, services (particularly Kubernetes-adjacent ones) have begun to manage system labels, etc.

Our reaction so far has been to mark label fields as "Optional + Computed" rather than Optional- this makes them detect diffs when any label is set but ignore them when no labels are set. This isn't a great solution- if a Cloud Run user wants to add a label, for example, they need to add their label and deal with a handful of system labels all at once. Other mitigations have ignored individual label keys or sets of label prefixes using a provider DiffSuppressFunc ("DSF")- but this approach works badly when a field has a system default & a user might override it.

Users can also somewhat self-manage with lifecycle.ignore_changes, but like Optional+Computed and DSFs, but frankly, that's not been a sufficient solution for a problem that exists platform-wide.


With 5.0.0 we should revisit our model. Labels are owned by multiple actors and Terraform's just one of them. I propose the following change to every labels-style field across every resource:

  • Change the current labels field to be non-authoritative and to only manage Terraform-managed labels (as described below)
  • Add an output-only field effective_labels with all labels present on the resource in GCP

Fields not literally called labels would have the same change- annotations would become non-authoritative, effective_annotations would output the complete list of annotations.

This would mean that if a GCE instance had the following state:

{
  "name": "gke-my-container-clu-my-container-nod-c93622e7-x505",
  "labels": {
      "blue": "defined-value",
      "green": "old-value",
    }
}

and was being managed by the following configuration:

resource "google_compute_instance" "default" {
  name         = "my-instance"
  
  labels = {
    red = "value"
    green = "other-value"
  }
}

Terraform would see a diff adding "red" with a value of "value" and modifying "green" to a value or "other-value" from "old-value". Blue would not appear in the diff (but would appear as an output under effective_labels post-apply) and would be unaffected by the change.

If that was applied- and Terraform was managing "green" and "red" with the values set in that configuration- applying the following configuration would create a plan removing the value for "red". Terraform would clear the value on apply and then stop managing the value. If a value was later added out of band, Terraform would not touch it (unless "red" was added to the configuration again)

resource "google_compute_instance" "default" {
  name         = "my-instance"
  
  labels = {
    green = "other-value"
  }
}

In addition to supporting multiple actors, this change to our model would help enable a handful of other labels-related features:

  • Default labels on the provider (default labels on provider #7325)
  • "provisioned-by": "hashicorp/terraform" labels allowing customers to audit Terraform-managed resources
  • Hooks for engines to attach labels (TFC, TFE, Backstage, Jenkins, GH Actions, etc.)
@toumorokoshi
Copy link

drive-by thought: I think the idea of a partially managed map for labels / annotations makes a lot of sense. But I'm wondering if there's valid use cases for fully authoritative labels or annotations?

This would be much more complex, but I'm wonder if it would be valuable to have a "partial labels" and an "authoritative labels" field, with the two conflicting with each other so you can only set one.

That way you keep the old use case, and enable the new one.

@rileykarson
Copy link
Collaborator Author

rileykarson commented Apr 13, 2023

Yeah, we could consider keeping authoritative behaviour around guarded by a flag on labels, or could make it possible to specify effective_labels directly to achieve as much. The interaction with provider-default labels & provisioned-by labels would require a decision- I'd lean towards sending only what's defined on the resource. I've thought about how we'd tackle it, but omitted that from the initial writeup- partially to fish for feedback on the omission to see if it's necessary.

I do think that the change to non-authoritative by default needs to happen, across-the-board, as users are affected by the current model all the time when services add new system labels. So there will be churn for users who want to preserve the authoritative model.

@toumorokoshi
Copy link

Yeah, we could consider keeping authoritative behaviour around guarded by a flag on labels, or could make it possible to specify effective_labels directly to achieve as much. The interaction with provider-default labels & provisioned-by labels would require a decision- I'd lean towards sending only what's defined on the resource. I've thought about how we'd tackle it, but omitted that from the initial writeup- partially to fish for feedback on the omission to see if it's necessary.

Yeah makes sense. I might consider designing for the possibility of an authoritative field, but not adding one yet.

w.r.t. how provider-defaults would play a role: I kind of feel like they should be included, since it's more about whether the provider is a singular source of truth or it works alongside a separate management system (e.g. server-side labels).

But you know the audience better than I do here.

I do think that the change to non-authoritative by default needs to happen, across-the-board, as users are affected by the current model all the time when services add new system labels. So there will be churn for users who want to preserve the authoritative model.

Definitely, if that's the intuition users have on what the labels field does, then it makes sense to change the current behavior.

@rileykarson
Copy link
Collaborator Author

This is merged- it was reasonably complete at the time that #7325 was closed.

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants