pulumi / pulumi-gcp

A Google Cloud Platform (GCP) Pulumi resource package, providing multi-language access to GCP
Apache License 2.0
182 stars 52 forks source link

`gcp.iam.WorkloadIdentityPool` is a soft delete #1149

Open jkodroff opened 1 year ago

jkodroff commented 1 year ago

What happened?

gcloud.iam.WorkloadIdentityPool resources do not actually delete. Per hashicorp/terraform-provider-google#14805, it's the API behavior that's at fault: it's a soft delete.

Expected Behavior

Not sure. We might do well to add some supplemental docs to the resource to indicate that we're at least aware of this issue since it's not easy (possible?) to fix programmatically.

If there's a way to work around this in a user's program, we should note. (And I'll comment on this issue if I figure it out - might be possible using random.String or similar.

At the very least, having this issue here noted could help folks in the future.

Steps to reproduce

Create a program with the following content:

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

const example = new gcloud.iam.WorkloadIdentityPool("example", {workloadIdentityPoolId: "example-pool"});

Then run:

pulumi config set gcp:project <your-project-id>
pulumi up -y
pulumi destroy -y
pulumi up -y

And you'll see the following error:

Diagnostics:
  pulumi:pulumi:Stack (test-gcp-delete-workloadidentitypool-dev):
    error: update failed

  gcp:iam:WorkloadIdentityPool (example):
    error: 1 error occurred:
        * Error creating WorkloadIdentityPool: googleapi: Error 409: Requested entity already exists

Output of pulumi about

CLI          
Version      3.77.1
Go Version   go1.20.7
Go Compiler  gc

Plugins
NAME    VERSION
gcp     6.62.0
nodejs  unknown

Host     
OS       darwin
Version  13.5
Arch     arm64

This project is written in nodejs: executable='/opt/homebrew/bin/node' version='v20.4.0'

Current Stack: jkodrofftest/test-gcp-delete-workloadidentitypool/dev

TYPE                  URN
pulumi:pulumi:Stack   urn:pulumi:dev::test-gcp-delete-workloadidentitypool::pulumi:pulumi:Stack::test-gcp-delete-workloadidentitypool-dev
pulumi:providers:gcp  urn:pulumi:dev::test-gcp-delete-workloadidentitypool::pulumi:providers:gcp::default_6_62_0

Found no pending operations associated with dev

Backend        
Name           pulumi.com
URL            https://app.pulumi.com/jkodroff
User           jkodroff
Organizations  jkodroff, oracle-workshop, jkodrofftest, zephyr, demo, pulumi

Dependencies:
NAME            VERSION
@types/node     16.18.39
@pulumi/gcp     6.62.0
@pulumi/pulumi  3.77.1

Pulumi locates its logs in /var/folders/5m/4n1x3f8151s35wc80w06z5k80000gn/T/ by default

Additional context

No response

Contributing

Vote on this issue by adding a 👍 reaction. To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

jkodroff commented 1 year ago

Taking a closer look, I believe this resource is mis-configured on the Pulumi side. I think workloadIdentityPoolId should maybe be autonamed?

This program:

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

const example = new gcloud.iam.WorkloadIdentityPool("resource-name", {
  workloadIdentityPoolId: "identity-pool-id"
});

Creates this in the Google Cloud console:

image

I can't see any field that is auto-named, which is what's leading me to believe the resource is misconfigured (regardless of the soft delete behavior). At the time of writing, there are no options set for this resource in resources.go - it's just a straight name to name mapping.

I believe if we make workloadIdentityPoolId the name field, and make it no longer required, we can implement this fix as a non-breaking change and everyone is happy. (Please double-check my logic and refute if I have it twisted.)

If this should be autonamed, be careful to validate the length of the field, etc. when implementing.

Hope this analysis helps.

iwahbe commented 1 year ago

@jkodroff thanks for telling us about this. It sounds like we are dealing with two separate items:

  1. The delete is a soft delete. This is noted in our documentation, but buried in the state variable.

  2. We don't have auto-naming for the workloadIdentityPoolId field, which means that up; down; up will try to create the same resource multiple times.

Let me know if this sounds right.

jkodroff commented 1 year ago

That is correct. I think point 2 is the more important thing to address. I'm like... > 90% sure it should be autonamed, but per a convo with @t0yv0 in Slack, I'm worried about the breaking change if I'm wrong and we have to revert the change.

iwahbe commented 1 year ago

That is correct. I think point 2 is the more important thing to address. I'm like... > 90% sure it should be autonamed, but per a convo with @t0yv0 in Slack, I'm worried about the breaking change if I'm wrong and we have to revert the change.

Any idea on how we can confirm that we can auto-name workloadIdentityPoolId?

t0yv0 commented 1 year ago

AutoName is something that should work here. One sequence of steps here is add AutoName to that, then try to make a resource without specifying workloadIdentityPoolId in the program, and observe that it was provisioned with a randomized workloadIdentityPoolId value.

t0yv0 commented 1 year ago

I don't think adding AutoName is breaking unless the underlying field is optional. If it was optional, adding AutoName will start pushing values into it where there were none before. Sounds like the field is required. So that's good. AutoName will make it optional.

In terms of migrating we'd need to ask users to replace the resource for autoname to kick in I think. This will happen if they drop the manual workloadIdentityPool value from the program.

jkodroff commented 1 year ago

Adding auto name wouldn't be breaking, but if we had to revert after releasing, it obviously would be a breaking change. That was my (probably unnecessary) worry.