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.32k stars 1.72k forks source link

Auto generate import id for *_iam_* generated resources instead of manually supplying by using ImportStateIdFunc #8712

Open ScottSuarez opened 3 years ago

ScottSuarez commented 3 years ago

Affected Resource(s)

Community Note

Description

Currently you must manually supply MMv1 with primary_resource_name and construct the import case for the first example. We should be able to avoid this by auto generating the import id by parsing the id from the current state of the resource by using ImportStateIdFunc

New or Affected Resource(s)

Potential Test Configuration

func TestAccServiceAccountIamMember_federatedIdentity(t *testing.T) {
    t.Parallel()

    account := fmt.Sprintf("tf-test-%d", randInt(t))
    pool := fmt.Sprintf("tf-test-%d", randInt(t))
    poolProvider := fmt.Sprintf("tf-test-%d", randInt(t))

    vcrTest(t, resource.TestCase{
        PreCheck:  func() { testAccPreCheck(t) },
        Providers: testAccProviders,
        Steps: []resource.TestStep{
            {
                Config: testAccServiceAccountIamMember_federatedIdentity(account, pool, poolProvider),
            },
            {
                ResourceName:      "google_service_account_iam_member.impersonate",
                ImportStateIdFunc: generateFedereatedIdentityStateId,
                ImportState:       true,
                ImportStateVerify: true,
            },
        },
    })
}

func generateFedereatedIdentityStateId(state *terraform.State) (string, error) {
    resourceName := "google_service_account_iam_member.impersonate"
    var rawState map[string]string
    for _, m := range state.Modules {
        if len(m.Resources) > 0 {
            if v, ok := m.Resources[resourceName]; ok {
                rawState = v.Primary.Attributes
            }
        }
    }
    fmt.Printf("raw state %s", rawState)
    return fmt.Sprintf("%s %s %s", rawState["service_account_id"], rawState["role"], rawState["member"]), nil
}

References

melinath commented 1 year ago

~It may not be possible to always autogenerate this, but we could almost certainly autogenerate the trivial case.~

Correction: this is extracting the id rather than autogenerating it from vars or similar, so this probably would work automatically for all resources.

rileykarson commented 6 months ago

@ScottSuarez do you remember the details of the changes you were proposing here? We reviewed this and couldn't figure out what the delta on an existing resource would be.

melinath commented 6 months ago

I believe the goal here would be to eliminate lines like https://github.com/GoogleCloudPlatform/magic-modules/blob/ca3b14e9c32eac9356cd050ecc428f5df5f04fa2/mmv1/products/apigee/Environment.yaml#L66, which are a common cause of errors on PRs (because they require understanding exactly how primary_resource_name is used and properly escaping the string).

related: https://github.com/hashicorp/terraform-provider-google/issues/13983

rileykarson commented 6 months ago

Ah- primary_resource_name is distinct from primary_resource_id, right.

melinath commented 4 months ago

Just ran into this again on https://github.com/GoogleCloudPlatform/magic-modules/pull/10670 - this ended up causing several rounds of extra review.

melinath commented 4 months ago

If there is any usage of primary_resource_name left, we should rename the field as discussed in https://github.com/hashicorp/terraform-provider-google/issues/13983

melinath commented 4 months ago

ran into this again on https://github.com/GoogleCloudPlatform/magic-modules/pull/10786 - the contributor skipped import tests to avoid the issue, which would have caused bugs in the import_format to be missed.