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

Handle new default label #2357

Closed
guineveresaenger opened this issue Aug 30, 2024 · 6 comments · Fixed by #2355
Closed

Handle new default label #2357

guineveresaenger opened this issue Aug 30, 2024 · 6 comments · Fixed by #2355
Assignees
Labels
kind/enhancement Improvements or new features resolution/fixed This issue was fixed
Milestone

Comments

@guineveresaenger
Copy link
Contributor

Upstream has added a new default attribution label.

Currently tests on storage.Bucket (but only Bucket) are failing due to a bug (also present upstream) on that resource:

  1. pulumi up successfully creates bucket and adds default label goog-terraform-provisioned
  2. A subsequent pulumi up shows a diff on the (non-authoritative)labels field, alerting the user that labels has a diff on it and will remove goog-terraform-provisioned: true.
  3. The actual bucket in the cloud, however, retains the label. Read-only labels fiels pulumiLabels and effectiveLabelsare unchanged and continue to display thegoog-terraform-provisioned` label.

It appears that Bucket is the only resource affected in this way. Upstream issue is filed
In the meantime, we'll want to explore user-friendly workarounds for this resource:

  • continue to allow this setting to be opt-in rather than default, as was the case in v7
  • alert any user of the Bucket resource that their second pulumi up will have a spurious diff
  • skip or adjust Bucket tests to ignore the "no changes expected" error

We also want to explore if and how we'd like to rename this label on our users' behalf, for clarity.

@pulumi-bot pulumi-bot added the needs-triage Needs attention from the triage team label Aug 30, 2024
@guineveresaenger guineveresaenger self-assigned this Aug 30, 2024
@iwahbe iwahbe added kind/enhancement Improvements or new features and removed needs-triage Needs attention from the triage team labels Aug 30, 2024
guineveresaenger added a commit that referenced this issue Aug 30, 2024
@guineveresaenger
Copy link
Contributor Author

Update here.

Provider setting is a config value rename in resources.go:
addTerraformAttributionLabel --> addPulumiAttributionLabel

Label itself is a patch:
goog-terraform-provisioned --> goog-pulumi-provisioned

However, we do see some unexpected behavior under certain conditions.

Running a simple storage.Bucket pulumi up throws a diff on labels the first preview after create, showing the removal of this label from pulumi state. Notice that this label does not get removed from the cloud, and it does get encrypted to pulumiLabels and effectiveLabels - but not retained in labels.
This also only happens if the labels field is otherwise unset - otherwise, the labels field remains untouched.

Import is affected - when importing a resource, this label is suggested in the import code. Adding or removing it, however, has no effect on the presence of the label. The only way to remove this label from the cloud resource is to set addPulumiAttributionLabel to false.

Sample code:

import * as pulumi from "@pulumi/pulumi";
import * as gcp from "@pulumi/gcp";

const bucket = new gcp.storage.Bucket("my-bucket", {
    location: "US",
});
export const labels = bucket.labels;

hashicorp/terraform-provider-google/issues/19323 has revealed this to be a Terraform core bug that for them only appears if you set Labels in an output block, and they suggest giving a default label as a workaround. On our end, it appears that our detailed diff output is our downfall - terraform only shows an anonymous diff on labels{}. In Terraform, this behavior was true in older versions as well. On our end, this diff is new. It does not feel like a desirable experience for our users, especially given the unintuitive behavior of goog-pulumi-provisioned only being removed when the global setting is set as false.

I wonder if #1946 is involved here - I will verify if this behavior replicates for resources other than Bucket.

@guineveresaenger
Copy link
Contributor Author

Update: It is definitely the Bucket patch that is causing this. A Compute Instance does not have this issue.

guineveresaenger added a commit that referenced this issue Sep 6, 2024
@guineveresaenger
Copy link
Contributor Author

Hm, the patch fix is now resulting in a diff on pulumiLabels for import.

@guineveresaenger
Copy link
Contributor Author

It turned out that the conditions set in SetLabelsNoDefaults were accidentally writing the goog-pulumi-provisioned label to the resource labels field in the patch - goog-pulumi-provisioned is never in the provider-set defaultLabels, but the incoming set of labels to partition out according to lineage is also never empty.
Additionally, this label setting happens during Create, so that a newly created Bucket always had an extra label on the resource.

An extra check for label key = goog-pulumi-provisioned and lineage = labels addresses this discrepancy. Here is the updated patch, with correct behavior: https://github.com/pulumi/pulumi-gcp/blob/e19dbc14d43a9304b6fc46c10bdd819b6a005eb8/patches/0009-Bucket-Skip-default-labels-for-Import-and-Create.patch

Finally, the randomized labels tests are verifying against pulumiLabels. We do expect this new label to appear in pulumiLabels, so this labels was set as an additional expectation in the label tests.

@cleverguy25
Copy link

Added to epic #2290

@pulumi-bot
Copy link
Contributor

This issue has been addressed in PR #2355 and shipped in release v8.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvements or new features resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants