-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ephemeral
: add google_service_account_id_token
#12141
base: FEATURE-BRANCH-ephemeral-resource
Are you sure you want to change the base?
ephemeral
: add google_service_account_id_token
#12141
Conversation
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Errors
|
Tests analyticsTotal tests: 0 Click here to see the affected service packages
View the build log |
e6dfed7
to
c92521f
Compare
Marking as ready for review with tests passing locally: === RUN TestEphemeralServiceAccountIdToken_basic
=== PAUSE TestEphemeralServiceAccountIdToken_basic
=== RUN TestEphemeralServiceAccountIdToken_withDelegates
=== PAUSE TestEphemeralServiceAccountIdToken_withDelegates
=== RUN TestEphemeralServiceAccountIdToken_withIncludeEmail
=== PAUSE TestEphemeralServiceAccountIdToken_withIncludeEmail
=== CONT TestEphemeralServiceAccountIdToken_basic
=== CONT TestEphemeralServiceAccountIdToken_withIncludeEmail
=== CONT TestEphemeralServiceAccountIdToken_withDelegates
2024/11/06 13:57:01 [INFO] Authenticating using configured Google JSON 'credentials'...
2024/11/06 13:57:01 [INFO] -- Scopes: [https://www.googleapis.com/auth/cloud-platform https://www.googleapis.com/auth/userinfo.email]
2024/11/06 13:57:01 [INFO] Authenticating using configured Google JSON 'credentials'...
2024/11/06 13:57:01 [INFO] -- Scopes: [https://www.googleapis.com/auth/cloud-platform https://www.googleapis.com/auth/userinfo.email]
2024/11/06 13:57:01 [INFO] Authenticating using configured Google JSON 'credentials'...
2024/11/06 13:57:01 [INFO] -- Scopes: [https://www.googleapis.com/auth/cloud-platform https://www.googleapis.com/auth/userinfo.email]
2024/11/06 13:57:01 [INFO] Authenticating using configured Google JSON 'credentials'...
2024/11/06 13:57:01 [INFO] -- Scopes: [https://www.googleapis.com/auth/cloud-platform https://www.googleapis.com/auth/userinfo.email]
2024/11/06 13:57:01 [INFO] Authenticating using configured Google JSON 'credentials'...
2024/11/06 13:57:01 [INFO] -- Scopes: [https://www.googleapis.com/auth/cloud-platform https://www.googleapis.com/auth/userinfo.email]
2024/11/06 13:57:01 [DEBUG] Waiting for state to become: [success]
2024/11/06 13:57:01 [DEBUG] Waiting for state to become: [success]
2024/11/06 13:57:01 [INFO] Authenticating using configured Google JSON 'credentials'...
2024/11/06 13:57:01 [INFO] -- Scopes: [https://www.googleapis.com/auth/cloud-platform https://www.googleapis.com/auth/userinfo.email]
2024/11/06 13:57:01 [DEBUG] Waiting for state to become: [success]
2024/11/06 13:57:01 [INFO] Terraform is using this identity: mauricio-alvarezleon@hc-terraform-testing.iam.gserviceaccount.com
2024/11/06 13:57:01 [DEBUG] Verifying projects/hc-terraform-testing/serviceAccounts/tf-bootstrap-sa-idtoken@hc-terraform-testing.iam.gserviceaccount.com as bootstrapped service account.
2024/11/06 13:57:01 [INFO] Instantiating Google Cloud IAM client for path https://iam.googleapis.com/
2024/11/06 13:57:01 [DEBUG] Retry Transport: starting RoundTrip retry loop
2024/11/06 13:57:01 [DEBUG] Retry Transport: request attempt 0
2024/11/06 13:57:01 [INFO] Terraform is using this identity: mauricio-alvarezleon@hc-terraform-testing.iam.gserviceaccount.com
2024/11/06 13:57:01 [DEBUG] Verifying projects/hc-terraform-testing/serviceAccounts/tf-bootstrap-sa-idtoken-email@hc-terraform-testing.iam.gserviceaccount.com as bootstrapped service account.
2024/11/06 13:57:01 [INFO] Instantiating Google Cloud IAM client for path https://iam.googleapis.com/
2024/11/06 13:57:01 [DEBUG] Retry Transport: starting RoundTrip retry loop
2024/11/06 13:57:01 [DEBUG] Retry Transport: request attempt 0
2024/11/06 13:57:01 [INFO] Terraform is using this identity: mauricio-alvarezleon@hc-terraform-testing.iam.gserviceaccount.com
2024/11/06 13:57:01 [DEBUG] Verifying projects/hc-terraform-testing/serviceAccounts/tf-bootstrap-sa-target@hc-terraform-testing.iam.gserviceaccount.com as bootstrapped service account.
2024/11/06 13:57:01 [INFO] Instantiating Google Cloud IAM client for path https://iam.googleapis.com/
2024/11/06 13:57:01 [DEBUG] Retry Transport: starting RoundTrip retry loop
2024/11/06 13:57:01 [DEBUG] Retry Transport: request attempt 0
2024/11/06 13:57:02 [DEBUG] Retry Transport: Stopping retries, last request was successful
2024/11/06 13:57:02 [DEBUG] Retry Transport: Returning after 1 attempts
2024/11/06 13:57:02 [DEBUG] Setting service account permissions.
2024/11/06 13:57:02 [INFO] Instantiating Google Cloud IAM client for path https://iam.googleapis.com/
2024/11/06 13:57:02 [DEBUG] Retry Transport: starting RoundTrip retry loop
2024/11/06 13:57:02 [DEBUG] Retry Transport: request attempt 0
2024/11/06 13:57:02 [DEBUG] Retry Transport: Stopping retries, last request was successful
2024/11/06 13:57:02 [DEBUG] Retry Transport: Returning after 1 attempts
2024/11/06 13:57:02 [DEBUG] Setting service account permissions.
2024/11/06 13:57:02 [INFO] Instantiating Google Cloud IAM client for path https://iam.googleapis.com/
2024/11/06 13:57:02 [DEBUG] Retry Transport: starting RoundTrip retry loop
2024/11/06 13:57:02 [DEBUG] Retry Transport: request attempt 0
2024/11/06 13:57:02 [DEBUG] Retry Transport: Stopping retries, last request was successful
2024/11/06 13:57:02 [DEBUG] Retry Transport: Returning after 1 attempts
2024/11/06 13:57:02 [DEBUG] Setting service account permissions.
2024/11/06 13:57:02 [INFO] Instantiating Google Cloud IAM client for path https://iam.googleapis.com/
2024/11/06 13:57:02 [DEBUG] Retry Transport: starting RoundTrip retry loop
2024/11/06 13:57:02 [DEBUG] Retry Transport: request attempt 0
2024/11/06 13:57:02 [DEBUG] Retry Transport: Stopping retries, last request was successful
2024/11/06 13:57:02 [DEBUG] Retry Transport: Returning after 1 attempts
--- PASS: TestEphemeralServiceAccountIdToken_basic (22.84s)
--- PASS: TestEphemeralServiceAccountIdToken_withIncludeEmail (23.23s)
--- PASS: TestEphemeralServiceAccountIdToken_withDelegates (23.35s)
PASS
ok github.com/hashicorp/terraform-provider-google/google/services/resourcemanager 30.963s |
.../third_party/terraform/services/resourcemanager/ephemeral_google_service_account_id_token.go
Outdated
Show resolved
Hide resolved
.../third_party/terraform/services/resourcemanager/ephemeral_google_service_account_id_token.go
Outdated
Show resolved
Hide resolved
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 4267 Click here to see the affected service packages
Action takenFound 234 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🔴 Tests failed when rerunning REPLAYING mode: Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial review, based off the feedback I gave in #12140
...d_party/terraform/services/resourcemanager/ephemeral_google_service_account_id_token_test.go
Outdated
Show resolved
Hide resolved
.../third_party/terraform/services/resourcemanager/ephemeral_google_service_account_id_token.go
Outdated
Show resolved
Hide resolved
c92521f
to
f4825f5
Compare
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 4267 Click here to see the affected service packages
Action takenFound 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
.../third_party/terraform/services/resourcemanager/ephemeral_google_service_account_id_token.go
Outdated
Show resolved
Hide resolved
f4825f5
to
a5c6da5
Compare
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 4267 Click here to see the affected service packages
Action takenFound 5 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an initial pass of review - as well as the individual comments please make an initial draft of the documentation markdown page for this ephemeral resource
resp.Diagnostics.Append(req.Config.Get(ctx, &data)...) | ||
|
||
targetAudience := data.TargetAudience.ValueString() | ||
creds := fwtransport.GetCredentials(ctx, fwmodels.ProviderModel{}, false, &resp.Diagnostics) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This'll be something that'll need refactoring after the muxing fixes are merged; the GetCredentials on the (SDK) Config
struct is different to the version implemented on the FrameworkProviderConfig
struct.
.../third_party/terraform/services/resourcemanager/ephemeral_google_service_account_id_token.go
Outdated
Show resolved
Hide resolved
.../third_party/terraform/services/resourcemanager/ephemeral_google_service_account_id_token.go
Outdated
Show resolved
Hide resolved
.../third_party/terraform/services/resourcemanager/ephemeral_google_service_account_id_token.go
Show resolved
Hide resolved
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 4267 Click here to see the affected service packages
Action takenFound 6 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...d_party/terraform/services/resourcemanager/ephemeral_google_service_account_id_token_test.go
Outdated
Show resolved
Hide resolved
...d_party/terraform/services/resourcemanager/ephemeral_google_service_account_id_token_test.go
Outdated
Show resolved
Hide resolved
...d_party/terraform/services/resourcemanager/ephemeral_google_service_account_id_token_test.go
Outdated
Show resolved
Hide resolved
looks like there's an issue with the boostrapping of the service accounts. |
@BBBmau the credentials in the test environment were revoked, it's not a problem with the test |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick review - the tests currently fail because the GetCredentials function isn't being passed any information about credentials etc. I've suggested a solution, but it's gross and we're being restricted by the way the framework GetCredentials function is written versus the original SDK version. Note how the SDK version is a method where the receiver is the Config struct (i.e. the meta) and the framework version is just a function 🙃
data "google_service_account_access_token" "impersonated" { | ||
provider = google | ||
target_service_account = "impersonated-account@project.iam.gserviceaccount.com" | ||
delegates = [] | ||
scopes = ["userinfo-email", "cloud-platform"] | ||
lifetime = "300s" | ||
} | ||
|
||
provider "google" { | ||
alias = "impersonated" | ||
access_token = data.google_service_account_access_token.impersonated.access_token | ||
} | ||
|
||
ephemeral "google_service_account_id_token" "oidc" { | ||
provider = google.impersonated | ||
target_service_account = "impersonated-account@project.iam.gserviceaccount.com" | ||
delegates = [] | ||
include_email = true | ||
target_audience = "https://foo.bar/" | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to manually check this in future - see TFECO-8280
resp.Diagnostics.Append(req.Config.Get(ctx, &data)...) | ||
|
||
targetAudience := data.TargetAudience.ValueString() | ||
creds := fwtransport.GetCredentials(ctx, fwmodels.ProviderModel{}, false, &resp.Diagnostics) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is gross, but necessary to make sure the framework version of GetCredentials receives the data it needs:
creds := fwtransport.GetCredentials(ctx, fwmodels.ProviderModel{}, false, &resp.Diagnostics) | |
model := fwmodels.ProviderModel{ | |
Credentials: p.providerConfig.Credentials, | |
AccessToken: p.providerConfig.AccessToken, | |
ImpersonateServiceAccount: p.providerConfig.ImpersonateServiceAccount, | |
ImpersonateServiceAccountDelegates: p.providerConfig.ImpersonateServiceAccountDelegates, | |
Project: p.providerConfig.Project, | |
BillingProject: p.providerConfig.BillingProject, | |
Scopes: p.providerConfig.Scopes, | |
UniverseDomain: p.providerConfig.UniverseDomain, | |
} | |
creds := fwtransport.GetCredentials(ctx, model, false, &resp.Diagnostics) |
fwmodels.ProviderModel is the struct that is populated with data from the provider block when the provider is being configured. The fwtransport.GetCredentials function is written in a way that makes it very coupled with being used in the context of a provider being configured, whereas the SDK version of GetCredentials is a method on the Config struct and can be run outside the context of a provider being configured.
Just more evidence that the original muxing was poorly-planned and 💩
@BBBmau I noticed some failing checks and I realised we need to make a change to allow the new documentation type. If you have time, please make a PR similar to hashicorp/terraform-provider-google#17509 into the |
}) | ||
} | ||
|
||
func TestAccEphemeralServiceAccountIdToken_withIncludeEmail(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the edit to GetCredentials' arguments is included, this test fails due to:
ephemeral_google_service_account_id_token_test.go:62: Step 1/1 error: Error running pre-apply refresh: exit status 1
Error: Error calling iamcredentials.GenerateIdToken
with ephemeral.google_service_account_id_token.token,
on terraform_plugin_test.tf line 3, in ephemeral "google_service_account_id_token" "token":
3: ephemeral "google_service_account_id_token" "token" {
googleapi: Error 403: Permission 'iam.serviceAccounts.getOpenIdToken' denied
on resource (or it may not exist).
Details:
[
{
"@type": "type.googleapis.com/google.rpc.ErrorInfo",
"domain": "iam.googleapis.com",
"metadata": {
"permission": "iam.serviceAccounts.getOpenIdToken"
},
"reason": "IAM_PERMISSION_DENIED"
}
]
, forbidden
This code bootstraps the existence of both service accounts and makes sure that serviceAccount
has permissions to imitate targetServiceAccountEmail
via roles/iam.serviceAccountTokenCreator. That's insufficient for this test though, hence the error.
Please add a step in the test where the missing permission is created. Then the ephemeral resource can be added in the second step.
Tests analyticsTotal tests: 4270 Click here to see the affected service packages
Action takenFound 9 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
An ephemeral resource that mimics the google_service_account_id_token data source
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.