hashicorp / terraform-provider-google

Terraform Provider for Google Cloud Platform
https://registry.terraform.io/providers/hashicorp/google/latest/docs
Mozilla Public License 2.0
2.34k stars 1.74k forks source link

google_clouddeploy_target resource's execution_configs block doesn't support string to null for service_account and artifact_storage option. #12263

Open SudharsaneSivamany opened 2 years ago

SudharsaneSivamany commented 2 years ago

Community Note

Terraform Version

Terraform v1.2.5 on linux_amd64

Affected Resource(s)

resource "google_clouddeploy_target" "primary" {
    project  = "my-project"
    location = "us-central1"
    name     = "target"
    gke {
         cluster = "projects/my-project/locations/us-central1-c/clusters/cluster"
    }
    execution_configs {
                  usages               = ["RENDER", "DEPLOY"]
                  service_account = "exe_sa@my-project.iam.gserviceaccount.com"
                  worker_pool       = null
                  artifact_storage  = "gs://somebucket/"
    }
}

Expected Behavior

Updating the service_account and artifact_storage values to null, then target should get updated to default service account and default artifact storage.

Actual Behavior

The above expected behavior is not happening. As an output , the value remains the same.

Note: worker_pool option works fine i.e., null => string : success , string => null : success. Same behavior expected for service_account and artifact_storage

Debug logs

Config: TF configuration used to create logs

State: State file details

Logs:

b/303616062

SarahFrench commented 2 years ago

Hi @SudharsaneSivamany - thank you for bringing this to our attention!

So if I understand correctly, the issue is that unsetting service_account or artifact_storage in the execution_configs block doesn't cause the API to make the resource use the default values for these fields. But if worker_pool is first set and then unset in separate apply steps the expected behaviour does happen.

I've recreated the scenario and I've attached some debug logs to the issue description.

While we're fixing this you can overcome the issue temporarily by using the google_compute_default_service_account and google_storage_bucket data sources to get the default values to explicitly pass into the resource (instead of using null). I appreciate that it'd be a lot nicer to just be able to unset the fields though!

SudharsaneSivamany commented 2 years ago

HI @SarahFrench - thank you for the response!!

Yes, your understanding is correct.

Regarding the temporary fix. If we pass google_compute_default_service_account and google_storage_bucket as an input to the execution config, the cloud deploy target will consider that as a user input. When we try to trigger the deploy ,it will fail by saying "ERROR: (gcloud.deploy.releases.create) INVALID_ARGUMENT: invalid release target: target target has invalid user-managed service account"

The reason behind this , if it is going to be the default service account then cloud deploy should pick it on its own while creating or updating the target, it will not work even if we pass default service account as input.(only it will work when it is null)

Also , w.r.t artifact storage . Default gcs bucket for artifact storage will be created on its own when we create a target by specifying artifact storage is null. If we pass a bucket url(either default or user created), cloud deploy target will consider it has user created bucket. (So again the fix is null)

SarahFrench commented 2 years ago

Ah, sorry, I didn't realise that there was a 'runtime problem' if the user provides the details of Google-managed resources. Thank you for checking!

The updates to required to fix this bug in the provider touches a repo that is internal to Google, which I am unable to access, but I've passed on details about this problem to people who can do the fix

SudharsaneSivamany commented 2 years ago

Thank you @SarahFrench !!

yashwanth-l commented 1 year ago

@SarahFrench any timeframe on when this issue would be fixed?

SarahFrench commented 1 year ago

👋 The issue has been forwarded to the Cloud Deploy service team for attention, and I don't have insight into when they would be working on this. I'm afraid I can't address the issue myself because the code change is in a Google-owned repo that I don't have access to.

melinath commented 1 year ago

@SarahFrench in terms of the fix here, what's the recommended path forward? It looks like the fields in question are both optional + computed, which is why they persist values.

SarahFrench commented 1 year ago

Hi @melinath

After reviewing my past comments I'm not sure what the route forward is.

To summarise the problem:

The execution_configs block contains worker_pool, service_account and artifact_storage fields that link the a google_clouddeploy_target resource to other resources in GCP. If you create a cloud deploy target without providing these values in your config then Cloud Deploy will use the default Cloud Build pool, will set a default service account, and will create a GCS bucket to be used.

If you update the resource to use a user-supplied worker_pool it will update to use that pool. If you remove worker_pool from your Terraform config the API will make the resource resume using the default Cloud Build pool. This appears to be possible because the worker_pool is Optional : true, Computed: false.

Users expect the other, related fields to behave in a similar way. However if you have a google_clouddeploy_target resource created with user-supplied service_account and artifact_storage fields and you remove those fields from your Terraform configuration Cloud Deploy will not change the resource to fall back to the default service account and GCS bucket created by Cloud Deploy. Like you said, these fields are Optional: true, Computed: true so the change in configuration doesn't impact the resource.

I think it's reasonable for users to expect the different fields to behave the same way. Currently users aren't able to update an existing google_clouddeploy_target resource so that is uses the default service account, as there's validation on the API that stops them supplying that information via their Terraform configuration (see this comment).


Possible routes forward could be:

melinath commented 1 year ago

Thanks! I wonder if it might also be possible to use encoders / decoders to handle this better, similar to what's described in https://github.com/hashicorp/terraform-provider-google/issues/14903#issuecomment-1764923141, since it's a complex API-side default. But it might be tricky to make work without being a breaking change.