Implement Service Account Impersonation #4015

upodroid · 2020-09-25T16:33:08Z

Fixes: hashicorp/terraform-provider-google#7064
Fixes: hashicorp/terraform-provider-google#7351
Part of: hashicorp/terraform-provider-google#7332

If this PR is for Terraform, I acknowledge that I have:

Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

provider: added support for service account impersonation.

Fork Update

modular-magician · 2020-09-25T16:34:17Z

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review.

Thanks for your contribution! A human will be with you soon.

@megan07, please review this PR or find an appropriate assignee.

modular-magician · 2020-09-25T16:40:50Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 117 insertions(+), 24 deletions(-))
Terraform Beta: Diff ( 4 files changed, 117 insertions(+), 24 deletions(-))
TF Conversion: Diff ( 1 file changed, 32 insertions(+), 10 deletions(-))

modular-magician · 2020-09-25T16:45:07Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 117 insertions(+), 24 deletions(-))
Terraform Beta: Diff ( 4 files changed, 117 insertions(+), 24 deletions(-))
TF Conversion: Diff ( 1 file changed, 32 insertions(+), 10 deletions(-))

upodroid · 2020-09-28T18:35:15Z

@rileykarson I'm going to use this PR to also fix hashicorp/terraform-provider-google#7351 because I rewrote the auth docs of the provider.

modular-magician · 2020-09-28T18:59:12Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 119 insertions(+), 24 deletions(-))
Terraform Beta: Diff ( 4 files changed, 119 insertions(+), 24 deletions(-))
TF Conversion: Diff ( 1 file changed, 32 insertions(+), 10 deletions(-))

rileykarson · 2020-09-28T19:01:51Z

@upodroid: Sounds good! Marked you as assigned to that issue as well.

upodroid · 2020-09-28T19:35:20Z

Have a look at the latest commit.

megan07 · 2020-09-30T21:14:28Z

/gcbrun

modular-magician · 2020-09-30T21:14:46Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=149631"

third_party/terraform/website/docs/guides/provider_reference.html.markdown

megan07 · 2020-09-30T21:16:03Z

third_party/terraform/website/docs/guides/provider_reference.html.markdown

@@ -72,15 +94,6 @@ same configuration.

 ### Quick Reference

-* `credentials` - (Optional) Either the path to or the contents of a


Do we want to remove this?

I do. I want to bury this configuration parameter at the bottom of the page. IMHO, it shouldn't be used at all and I have an open proposal to remove it entirely from terraform in the next major release.

I believe credentials is the most commonly used authentication method for the provider. Depriorisiting it in the middle of the 3.X chain is probably something we should avoid doing, at least until we've committed to removing the field in 4.0.0 (assuming we do so).

I'll leave it in the Quick reference but I'll move it down.

megan07 · 2020-09-30T21:17:25Z

third_party/terraform/website/docs/guides/provider_reference.html.markdown

@@ -91,6 +104,9 @@ region is specified on a regional resource, it will take precedence.
 zone should be within the default region you specified. If another zone is
 specified on a zonal resource, it will take precedence.

+* `impersonate_service_account` - (Optional) The service account to impersonate for all Google API Calls.


Do we need to add an impersonate_service_account_delegates as well?

Delegates is a complex configuration and in most cases direct impersonation is generally sufficient. It is documented at the bottom

modular-magician · 2020-09-30T21:22:24Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 119 insertions(+), 24 deletions(-))
Terraform Beta: Diff ( 4 files changed, 119 insertions(+), 24 deletions(-))
TF Conversion: Diff ( 1 file changed, 32 insertions(+), 10 deletions(-))

modular-magician · 2020-09-30T22:31:32Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 122 insertions(+), 24 deletions(-))
Terraform Beta: Diff ( 4 files changed, 122 insertions(+), 24 deletions(-))
TF Conversion: Diff ( 1 file changed, 32 insertions(+), 10 deletions(-))

third_party/terraform/utils/config_test.go

modular-magician · 2020-10-09T20:46:54Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 176 insertions(+), 26 deletions(-))
Terraform Beta: Diff ( 4 files changed, 176 insertions(+), 26 deletions(-))
TF Conversion: Diff ( 1 file changed, 42 insertions(+), 12 deletions(-))

modular-magician · 2020-10-09T22:16:32Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccConfigLoadValidate_accessTokenImpersonated|TestAccConfigLoadValidate_impersonated|TestAccDataSourceGoogleActiveFolder_default|TestAccDataSourceGoogleActiveFolder_space|TestAccDataSourceGoogleForwardingRule|TestAccDataSourceGoogleServiceAccountIdToken_basic|TestAccProviderMeta_setModuleName|TestAccAccessApprovalOrganizationSettings_update|TestAccBigQueryJob_bigqueryJobQueryExample|TestAccBigQueryJob_bigqueryJobLoadExample|TestAccBigQueryJob_bigqueryJobQueryTableReferenceExample|TestAccBigQueryJob_bigqueryJobLoadTableReferenceExample|TestAccBigQueryJob_bigqueryJobExtractExample|TestAccBigQueryJob_bigqueryJobExtractTableReferenceExample|TestAccBigQueryJob_bigqueryJobCopyExample|TestAccBigQueryJob_bigqueryJobCopyTableReferenceExample|TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample|TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample|TestAccContainerCluster_withNodeConfig|TestAccContainerCluster_withSandboxConfig|TestAccContainerNodePool_withSandboxConfig|TestAccDNSPolicy_dnsPolicyBasicExample|TestAccDNSPolicy_update|TestAccProjectServiceIdentity_basic|TestAccRedisInstance_redisInstanceAuthEnabled You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=151460"

joe-a-t · 2020-10-14T22:22:19Z

How is this looking in terms of making it into the next release?

megan07 · 2020-10-15T14:44:42Z

Sorry for the delay on this, I've had numerous internal deadlines I've needed to hit, but have time to focus on this more this morning. I left off and the TestAccConfigLoadValidate_impersonated is passing, but it seems that a bunch of other tests are now failing with permissions errors - I need to check how these are related. Also, the TestAccConfigLoadValidate_accessTokenImpersonated is failing with PERMISSION_DENIED, which seems odd since it's using the exact same caller credentials as the other test. I need to look into that. Any ideas @upodroid?

As for when it will be released, @joe-a-t, it won't be in the next release, as that has already been cut, however, if we can get this out within the next week, it should be able to make 3.45.0 - but this is pending all the tests passing by then.

upodroid · 2020-10-15T15:57:26Z

Hmm, that is weird. I'll run a selection of the failed tests with debug logging enabled and will report back.

megan07 · 2020-10-15T16:04:16Z

Hi @upodroid ! I think I figured out the other tests failing, as I was suspecting - I had given the impersonate service account only permission for compute.viewer for that specific test, however, because the GOOGLE_IMPERSONATE_SERVICE_ACCOUNT environment variable was set, the config was defaulting to use that service account rather than the caller service account we typically use - thus failing with no permissions.
I talked this over with @rileykarson just now, and I'm testing this out, but I think what we've decided is that instead of setting GOOGLE_IMPERSONATE_SERVICE_ACCOUNT for our acceptance testing, we'll use a different environment variable (such as IMPERSONATE_SERVICE_ACCOUNT_ACCTEST?) and manually set it in the config

	config := &Config{
		Credentials:               creds,
		ImpersonateServiceAccount: serviceaccount,
		Project:                   proj,
		Region:                    "us-central1",
	}

so that just the TestAccConfigLoadValidate_impersonated will use ImpersonateServiceAccount. Does that make sense?

upodroid · 2020-10-15T16:35:45Z

I fixed it now. I assumed a highly credentialed identity was being impersonated. I was doing that.

modular-magician · 2020-10-15T16:42:26Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 176 insertions(+), 26 deletions(-))
Terraform Beta: Diff ( 4 files changed, 176 insertions(+), 26 deletions(-))
TF Conversion: Diff ( 1 file changed, 42 insertions(+), 12 deletions(-))

megan07 · 2020-10-15T18:02:34Z

third_party/terraform/utils/config_test.go

+	proj := getTestProjectFromEnv()
+	serviceaccount := multiEnvSearch([]string{"IMPERSONATE_SERVICE_ACCOUNT_ACCTEST"})
+
+	c, err := google.CredentialsFromJSON(context.Background(), []byte(creds), testOauthScope)


One last thing @rileykarson found, we need to pass the DefaultClientScopes along with testOauthScope here, I think thats why this test was failing.

Hmm, i'll replace testOauthScope with DefaultClientScopes. Instead of appending the two, you can use the default one as it in includes compute + cloud-platform

megan07 · 2020-10-15T18:03:45Z

@upodroid thank you for the last fix! I'll run the tests again after the last comment here is addressed. Thank you so much for your patience and all of your work on this!

modular-magician · 2020-10-15T18:28:34Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 176 insertions(+), 26 deletions(-))
Terraform Beta: Diff ( 4 files changed, 176 insertions(+), 26 deletions(-))
TF Conversion: Diff ( 1 file changed, 42 insertions(+), 12 deletions(-))

third_party/terraform/utils/config_test.go

megan07 · 2020-10-15T18:38:56Z

/gcbrun

modular-magician · 2020-10-15T18:39:11Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=152337"

modular-magician · 2020-10-15T18:47:35Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 176 insertions(+), 26 deletions(-))
Terraform Beta: Diff ( 4 files changed, 176 insertions(+), 26 deletions(-))
TF Conversion: Diff ( 1 file changed, 42 insertions(+), 12 deletions(-))

modular-magician · 2020-10-15T18:55:46Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 176 insertions(+), 26 deletions(-))
Terraform Beta: Diff ( 4 files changed, 176 insertions(+), 26 deletions(-))
TF Conversion: Diff ( 1 file changed, 42 insertions(+), 12 deletions(-))

megan07 · 2020-10-15T19:04:44Z

/gcbrun

modular-magician · 2020-10-15T19:04:58Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=152341"

modular-magician · 2020-10-15T19:12:50Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 176 insertions(+), 26 deletions(-))
Terraform Beta: Diff ( 4 files changed, 176 insertions(+), 26 deletions(-))
TF Conversion: Diff ( 1 file changed, 42 insertions(+), 12 deletions(-))

modular-magician · 2020-10-15T20:04:19Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDataSourceGoogleActiveFolder_default|TestAccDataSourceGoogleActiveFolder_space|TestAccDataSourceGoogleForwardingRule|TestAccDataSourceComputeInstanceSerialPort_basic|TestAccDataSourceGoogleComputeInstanceGroup_basic|TestAccDataSourceGoogleComputeInstanceGroup_withNamedPort|TestAccDataSourceGoogleComputeInstanceGroup_fromIGM|TestAccDataSourceSpannerInstance_basic|TestAccComputeInstanceIamBindingGenerated|TestAccComputeInstanceIamMemberGenerated|TestAccComputeInstanceIamMemberGenerated_withCondition|TestAccComputeInstanceIamBindingGenerated_withCondition|TestAccComputeInstanceIamPolicyGenerated|TestAccComputeInstanceIamPolicyGenerated_withCondition|TestAccIapTunnelInstanceIamBindingGenerated|TestAccIapTunnelInstanceIamMemberGenerated|TestAccIapTunnelInstanceIamPolicyGenerated|TestAccIapTunnelInstanceIamBindingGenerated_withCondition|TestAccIapTunnelInstanceIamMemberGenerated_withCondition|TestAccIapTunnelInstanceIamPolicyGenerated_withCondition|TestAccStorageBucketIamPolicyGenerated|TestAccStorageBucketIamPolicyGenerated_withCondition|TestAccProviderMeta_setModuleName|TestAccBigQueryJob_bigqueryJobQueryExample|TestAccBigQueryJob_bigqueryJobQueryTableReferenceExample|TestAccBigQueryJob_bigqueryJobLoadExample|TestAccBigQueryJob_bigqueryJobLoadTableReferenceExample|TestAccBigQueryJob_bigqueryJobExtractExample|TestAccBigQueryJob_bigqueryJobExtractTableReferenceExample|TestAccBigQueryJob_bigqueryJobCopyExample|TestAccBigQueryJob_bigqueryJobCopyTableReferenceExample|TestAccCloudTasksQueue_update|TestAccCloudTasksQueue_update2Basic|TestAccComputeAutoscaler_autoscalerSingleInstanceExample|TestAccComputeAutoscaler_autoscalerBasicExample|TestAccComputeAutoscaler_update|TestAccComputeAutoscaler_multicondition|TestAccComputeAutoscaler_scaleDownControl|TestAccComputeBackendService_withBackend|TestAccComputeBackendService_withBackendAndIAP|TestAccComputeBackendService_withMaxConnections|TestAccComputeBackendService_withMaxConnectionsPerInstance|TestAccComputeBackendService_withMaxRatePerEndpoint|TestAccComputeBackendService_withMaxConnectionsPerEndpoint|TestAccComputeBackendService_internalLoadBalancing|TestAccComputeDisk_deleteDetach|TestAccComputeForwardingRule_forwardingRuleHttpLbExample|TestAccComputeGlobalForwardingRule_globalForwardingRuleInternalExample|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccComputeHealthCheck_tcp_update|TestAccComputeInstanceFromTemplate_overrideBootDisk|TestAccComputeInstanceFromTemplate_overrideAttachedDisk|TestAccComputeInstanceFromTemplate_overrideScheduling|TestAccComputeInstanceFromTemplate_012_removableFields|TestAccComputeInstanceFromTemplate_overrideMetadataDotStartupScript|TestAccInstanceGroupManager_targetSizeZero|TestAccInstanceGroupManager_basic|TestAccInstanceGroupManager_versions|TestAccInstanceGroupManager_update|TestAccInstanceGroupManager_autoHealingPolicies|TestAccInstanceGroupManager_stateful|TestAccComputeInstanceGroup_basic|TestAccComputeInstanceGroup_update|TestAccComputeInstanceGroup_rename|TestAccComputeInstanceGroup_outOfOrderInstances|TestAccComputeInstanceGroup_network|TestAccComputeInstanceIamPolicy|TestAccComputeInstanceTemplate_preemptible|TestAccComputeInstanceTemplate_basic|TestAccComputeInstanceTemplate_imageShorthand|TestAccComputeInstanceTemplate_IP|TestAccComputeInstanceTemplate_networkTier|TestAccComputeInstanceTemplate_networkIP|TestAccComputeInstanceTemplate_disks|TestAccComputeInstanceTemplate_subnet_auto|TestAccComputeInstanceTemplate_networkIPAddress|TestAccComputeInstanceTemplate_regionDisks|TestAccComputeInstanceTemplate_primaryAliasIpRange|TestAccComputeInstanceTemplate_subnet_custom|TestAccComputeInstanceTemplate_metadata_startup_script|TestAccComputeInstanceTemplate_secondaryAliasIpRange|TestAccComputeInstanceTemplate_guestAccelerator|TestAccComputeInstanceTemplate_minCpuPlatform|TestAccComputeInstanceTemplate_shieldedVmConfig1|TestAccComputeInstanceTemplate_guestAcceleratorSkip|TestAccComputeInstanceTemplate_soleTenantNodeAffinities|TestAccComputeInstanceTemplate_EncryptKMS|TestAccComputeInstanceTemplate_shieldedVmConfig2|TestAccComputeInstanceTemplate_withScratchDisk|TestAccComputeInstanceTemplate_enableDisplay|TestAccComputeInstance_basic2|TestAccComputeInstance_basic1|TestAccComputeInstance_IP|TestAccComputeInstance_basic4|TestAccComputeInstance_basic3|TestAccComputeInstance_PTRRecord|TestAccComputeInstance_basic5|TestAccComputeInstance_networkTier|TestAccComputeInstance_diskEncryptionRestart|TestAccComputeInstance_diskEncryption You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=152361"

megan07

Tests have passed! Thanks so much!

upodroid · 2020-10-15T20:54:13Z

Thanks

upodroid added 9 commits August 3, 2020 12:29

Merge pull request #1 from GoogleCloudPlatform/master

71c3aeb

Fork Update

Merge branch 'master' of github.com:GoogleCloudPlatform/magic-modules

e1a9488

Merge branch 'master' of github.com:GoogleCloudPlatform/magic-modules

51eaff6

add service account impersonation

fa96756

Merge branch 'master' of github.com:GoogleCloudPlatform/magic-modules

649e97c

Merge branch 'master' into impersonated-credentials

cf3edc0

fix conflicts

a1d06ed

add the scopes block removed by error

567fc55

add service account impersonation

2010e78

google-cla bot added the cla: yes label Sep 25, 2020

modular-magician requested a review from megan07 September 25, 2020 16:34

undo spacing

7bb8487

upodroid mentioned this pull request Sep 25, 2020

GCP: add service account impersonation hashicorp/packer#9968

Merged

update docs for quota project

89dbaec

megan07 suggested changes Sep 30, 2020

View reviewed changes

fix typos in docs

4a2ffe7

Merge branch 'master' of github.com:GoogleCloudPlatform/magic-modules

ee373cc

megan07 reviewed Oct 5, 2020

View reviewed changes

third_party/terraform/utils/config_test.go Outdated Show resolved Hide resolved

fix typo in test and the docs

e90c0c4

replace the envs used for testing impersonation

b70ff76

megan07 reviewed Oct 15, 2020

View reviewed changes

add additional scopes

7339153

megan07 reviewed Oct 15, 2020

View reviewed changes

third_party/terraform/utils/config_test.go Outdated Show resolved Hide resolved

typo fix

fcfb721

megan07 approved these changes Oct 15, 2020

View reviewed changes

megan07 merged commit 8726df5 into GoogleCloudPlatform:master Oct 15, 2020

bharathkkb mentioned this pull request Nov 2, 2020

Use provider native SA impersonation terraform-google-modules/terraform-example-foundation#281

Closed

		@@ -72,15 +94,6 @@ same configuration.

		### Quick Reference

		* `credentials` - (Optional) Either the path to or the contents of a

Implement Service Account Impersonation #4015

Implement Service Account Impersonation #4015

Conversation

upodroid commented Sep 25, 2020 • edited Loading

modular-magician commented Sep 25, 2020

modular-magician commented Sep 25, 2020

Diff report:

modular-magician commented Sep 25, 2020

Diff report:

upodroid commented Sep 28, 2020

modular-magician commented Sep 28, 2020

Diff report:

rileykarson commented Sep 28, 2020

upodroid commented Sep 28, 2020

megan07 commented Sep 30, 2020

modular-magician commented Sep 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Sep 30, 2020

Diff report:

modular-magician commented Sep 30, 2020

Diff report:

modular-magician commented Oct 9, 2020

Diff report:

modular-magician commented Oct 9, 2020

joe-a-t commented Oct 14, 2020

megan07 commented Oct 15, 2020

upodroid commented Oct 15, 2020

megan07 commented Oct 15, 2020

upodroid commented Oct 15, 2020

modular-magician commented Oct 15, 2020

Diff report:

Choose a reason for hiding this comment

upodroid Oct 15, 2020 • edited Loading

Choose a reason for hiding this comment

megan07 commented Oct 15, 2020

modular-magician commented Oct 15, 2020

Diff report:

megan07 commented Oct 15, 2020

modular-magician commented Oct 15, 2020

modular-magician commented Oct 15, 2020

Diff report:

modular-magician commented Oct 15, 2020

Diff report:

megan07 commented Oct 15, 2020

modular-magician commented Oct 15, 2020

modular-magician commented Oct 15, 2020

Diff report:

modular-magician commented Oct 15, 2020

megan07 left a comment

Choose a reason for hiding this comment

upodroid commented Oct 15, 2020

upodroid commented Sep 25, 2020 •

edited

Loading

upodroid Oct 15, 2020 •

edited

Loading