Skip to content
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.

Sign up for GitHub

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

Fix Service Account creation by ignoring 403 errors on read polling #11811

Merged
merged 1 commit into from
Oct 2, 2024
Merged

Fix Service Account creation by ignoring 403 errors on read polling #11811

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion mmv1/third_party/terraform/services/resourcemanager/resource_google_service_account.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,8 @@ func resourceGoogleServiceAccountCreate(d *schema.ResourceData, meta interface{}

// We poll until the resource is found due to eventual consistency issue
// on part of the api https://cloud.google.com/iam/docs/overview#consistency
err = transport_tpg.PollingWaitTime(resourceServiceAccountPollRead(d, meta), transport_tpg.PollCheckForExistence, "Creating Service Account", d.Timeout(schema.TimeoutCreate), 1)
// IAM API returns 403 when the queried SA is not found, so we must ignore both 404 & 403 errors
err = transport_tpg.PollingWaitTime(resourceServiceAccountPollRead(d, meta), transport_tpg.PollCheckForExistenceWith403, "Creating Service Account", d.Timeout(schema.TimeoutCreate), 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have eventual consistency checks for 403's in https://github.com/GoogleCloudPlatform/magic-modules/pull/11811/files#diff-0541a83ba5cbaa8bd8a0cd9128218cc5766b18aeaa97015ccaa06b301511e08cR145

Wouldn't this just add even more polling for 403's?

In your testing, is the resource failing at this specific line? If so, what if we just moved the existing sleep already present in https://github.com/GoogleCloudPlatform/magic-modules/pull/11811/files#diff-0541a83ba5cbaa8bd8a0cd9128218cc5766b18aeaa97015ccaa06b301511e08cR165 to before this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have eventual consistency checks for 403'
Yes, the addition was actually introduced by me. The problem how I understand it is, it only handles the first Read after initial creation of the account. When the initial Create + Read succeeds, the resource is added to the TF state.

The poller then polls to check if the account indeed exists to be sure the SA has been eventually created. But here is the problem - as the IAM API is eventually consistent, it returns 403 when the SA is not found (yes, it's not a mistake, not 404, but 403 is returned here, it's a bit misleading as it masks not found behind IAM_PERMISSION_DENIED for seurity reasons). As the poller immediatelly exits on any other error than 404, all the other defensive logic that attempts to deal with the EC is ineffective as the provider immediately fails.

Also, the 10s sleep that was also meant to deal with that is actually never reached https://github.com/GoogleCloudPlatform/magic-modules/pull/11811/files#diff-0541a83ba5cbaa8bd8a0cd9128218cc5766b18aeaa97015ccaa06b301511e08cR162-R165

You can see it in the log output in the issue I 've submitted hashicorp/terraform-provider-google#19624 - notice it fails within 1s as the poll read returns 403 and the whole provider fails.
image

I personally think, the 10s sleep could even be removed iif the poller would handle 403 as I'm proposing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thank you for providing the logs. I think this makes sense, and looks like the previous sleep did not actually address this issue.

Considering we are just swapping out the PollCheck used, I think this should be good to merge.


if err != nil {
return err
Expand Down
Loading