Set google_service_account IAM-related fields during plan stage

[mikesmitty]

This sets the IAM-related fields on google_service_account with CustomizeDiff so they won't be "known after apply" and can be used to set IAM rules in a single TF run. I couldn't find any existing issues around it, but it has been a thorn in my side for a while.

Release Note Template for Downstream PRs (will be copied)

resourcemanager: made `google_service_account` `email` and `member` fields available during plan

[github-actions]

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

@melinath, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

[mikesmitty]

Did some local tests and it appears to be working as intended:

Terraform will perform the following actions:

  # google_service_account.service_account will be created
  + resource "google_service_account" "service_account" {
      + account_id = "test-account"
      + disabled   = false
      + email      = "[email protected]"
      + id         = (known after apply)
      + member     = "serviceAccount:[email protected]"
      + name       = (known after apply)
      + project    = "my-project-id"
      + unique_id  = (known after apply)
    }

Plan: 1 to add, 0 to change, 0 to destroy.

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 1 file changed, 22 insertions(+))
google-beta provider: Diff ( 1 file changed, 22 insertions(+))

[melinath]

Thanks for your contribution & patience; apologies for the delayed review. This is a slightly unusual contribution that we don't have a lot of precedent for so I'm going to check with teammates about whether there's anything specific to look out for here.

I've left a couple comments down below in the meantime. No worries if you'd prefer to wait to hear if there are any blockers before proceeding.

[modular-magician]

Tests analytics

Total tests: 147
Passed tests: 117
Skipped tests: 30
Affected tests: 0

Click here to see the affected service packages

• resourcemanager

🟢 All tests passed!

View the build log

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 1 file changed, 22 insertions(+))
google-beta provider: Diff ( 1 file changed, 22 insertions(+))

[modular-magician]

Tests analytics

Total tests: 147
Passed tests: 117
Skipped tests: 30
Affected tests: 0

Click here to see the affected service packages

• resourcemanager

🟢 All tests passed!

View the build log

[github-actions]

@melinath This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

[melinath]

We're okay with this change in a general sense. I'm double-checking to make sure that it's safe for us to always assume the service account will have the expected format. (I think it should be?)

note to self: yaqs/6281398878809882624.

[melinath]

Regarding the failed TGC tests - you'll need to account for the fact that references to service account emails are known before apply in two files:

• example_google_composer_environment.json in the project IAM policy (error is here)
• [example_container_cluster.json)[https://github.com/GoogleCloudPlatform/magic-modules/blob/b0450ba0e17ac08ac7e07aa3bbb6eeea51785db4/mmv1/third_party/tgc/tests/data/example_container_cluster.json#L49] in the NodePool (error is here)

[melinath]

It looks like TGC already has custom logic to add the email into the API object (to get around the fact it's missing in the plan): https://github.com/GoogleCloudPlatform/terraform-google-conversion/blob/aa4f4879b9078957d64d3dc68342817020601124/tfplan2cai/converters/google/resources/services/resourcemanager/service_account.go#L104-L107 So at least the person who made that change also thought that was safe to compute ahead of time.

[melinath]

The format for the service account is documented here: https://cloud.google.com/iam/docs/service-accounts-create#creating so it should definitely be safe to precompute as long as the universe_domain is googleapis.com. I'm working on figuring out the best way to express that conditional - we may want to add a reusable function for it.

[github-actions]

@mikesmitty, this PR is waiting for action from you. If no action is taken, this PR will be closed in 28 days.

Please address any comments or change requests, or re-request review from a core reviewer if no action is required.

This notification can be disabled with the disable-automatic-closure label.

[mikesmitty]

Thanks for the tips, I'll be back to work on this in a bit

[melinath]

ah, thanks! I don't have any updates on a "reusable function" but config.UniverseDomain is where you can check whether it's in the standard universe domain I mentioned above.

[mikesmitty]

@melinath I added a basic reusable function to get the UniverseDomain since I wasn't sure what other use cases there might be for it. I'm open to suggestions if you had something else in mind however

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 5 files changed, 205 insertions(+))
google-beta provider: Diff ( 5 files changed, 205 insertions(+))
terraform-google-conversion: Diff ( 2 files changed, 3 insertions(+), 2 deletions(-))

[modular-magician]

Tests analytics

Total tests: 4303
Passed tests: 3891
Skipped tests: 411
Affected tests: 1

Click here to see the affected service packages

All service packages are affected

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

• TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy

Get to know how VCR tests work

[modular-magician]

🔴 Tests failed during RECORDING mode:
TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy [Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 5 files changed, 178 insertions(+))
google-beta provider: Diff ( 5 files changed, 178 insertions(+))
terraform-google-conversion: Diff ( 2 files changed, 3 insertions(+), 2 deletions(-))

Errors

Other:

• Failed to update breaking-change status check with state: success

[melinath]

/gcbrun

[github-actions]

@melinath This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 5 files changed, 178 insertions(+))
google-beta provider: Diff ( 5 files changed, 178 insertions(+))
terraform-google-conversion: Diff ( 2 files changed, 3 insertions(+), 2 deletions(-))

[modular-magician]

Tests analytics

Total tests: 4322
Passed tests: 3907
Skipped tests: 407
Affected tests: 8

Click here to see the affected service packages

All service packages are affected

Action taken

Found 8 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

• TestAccOracleDatabaseAutonomousDatabase_basic
• TestAccOracleDatabaseAutonomousDatabases_basic
• TestAccOracleDatabaseCloudExadataInfrastructure_basic
• TestAccOracleDatabaseCloudExadataInfrastructures_basic
• TestAccOracleDatabaseCloudVmCluster_basic
• TestAccOracleDatabaseDbNodes_basic
• TestAccOracleDatabaseDbServers_basic
• TestAccSecureSourceManagerInstance_secureSourceManagerInstancePrivatePscBackendExample

Get to know how VCR tests work

[modular-magician]

🔴 Tests failed during RECORDING mode:
TestAccOracleDatabaseAutonomousDatabase_basic [Error message] [Debug log]
TestAccOracleDatabaseAutonomousDatabases_basic [Error message] [Debug log]
TestAccOracleDatabaseCloudExadataInfrastructure_basic [Error message] [Debug log]
TestAccOracleDatabaseCloudExadataInfrastructures_basic [Error message] [Debug log]
TestAccOracleDatabaseCloudVmCluster_basic [Error message] [Debug log]
TestAccOracleDatabaseDbNodes_basic [Error message] [Debug log]
TestAccOracleDatabaseDbServers_basic [Error message] [Debug log]
TestAccSecureSourceManagerInstance_secureSourceManagerInstancePrivatePscBackendExample [Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

[melinath]

The test failures are unrelated.

[melinath]

LGTM - I did some additional manual testing and this seems to work as I'd expect. Thanks for sticking with this and adding thorough tests!