From e74a54cb4559abcbed4fdf80c936ad1a97310b57 Mon Sep 17 00:00:00 2001 From: emily Date: Fri, 19 Apr 2019 13:45:14 -0700 Subject: [PATCH] Add retries/backoff when (only) reading IAM policies (#1665) Merged PR #1665. --- build/terraform | 2 +- build/terraform-beta | 2 +- .../resources/resource_iam_audit_config.go | 2 +- .../resources/resource_iam_binding.go | 2 +- .../resources/resource_iam_member.go | 2 +- .../resources/resource_iam_policy.go | 2 +- third_party/terraform/utils/iam.go | 25 ++++++++++++++++++- 7 files changed, 30 insertions(+), 7 deletions(-) diff --git a/build/terraform b/build/terraform index 4850ec52fd8f..462c3678425f 160000 --- a/build/terraform +++ b/build/terraform @@ -1 +1 @@ -Subproject commit 4850ec52fd8fa60635ba4af439a379520661a14a +Subproject commit 462c3678425f5b4156738b793b9f4b5d947fed77 diff --git a/build/terraform-beta b/build/terraform-beta index 64d5b8f90e52..77f415b46ec1 160000 --- a/build/terraform-beta +++ b/build/terraform-beta @@ -1 +1 @@ -Subproject commit 64d5b8f90e529e001431343b10a713ee9f7c4a29 +Subproject commit 77f415b46ec1c5aca4c2a825a844d62ef1a188d6 diff --git a/third_party/terraform/resources/resource_iam_audit_config.go b/third_party/terraform/resources/resource_iam_audit_config.go index 77efa7a20264..9f2010cc30e4 100644 --- a/third_party/terraform/resources/resource_iam_audit_config.go +++ b/third_party/terraform/resources/resource_iam_audit_config.go @@ -86,7 +86,7 @@ func resourceIamAuditConfigRead(newUpdaterFunc newResourceIamUpdaterFunc) schema } eAuditConfig := getResourceIamAuditConfig(d) - p, err := updater.GetResourceIamPolicy() + p, err := iamPolicyReadWithRetry(updater) if err != nil { if isGoogleApiErrorWithCode(err, 404) { log.Printf("[DEBUG]: AuditConfig for service %q not found for non-existent resource %s, removing from state file.", eAuditConfig.Service, updater.DescribeResource()) diff --git a/third_party/terraform/resources/resource_iam_binding.go b/third_party/terraform/resources/resource_iam_binding.go index 5a8d072adfcb..099bdf9f69a1 100644 --- a/third_party/terraform/resources/resource_iam_binding.go +++ b/third_party/terraform/resources/resource_iam_binding.go @@ -77,7 +77,7 @@ func resourceIamBindingRead(newUpdaterFunc newResourceIamUpdaterFunc) schema.Rea } eBinding := getResourceIamBinding(d) - p, err := updater.GetResourceIamPolicy() + p, err := iamPolicyReadWithRetry(updater) if err != nil { if isGoogleApiErrorWithCode(err, 404) { log.Printf("[DEBUG]: Binding for role %q not found for non-existent resource %s, removing from state file.", updater.DescribeResource(), eBinding.Role) diff --git a/third_party/terraform/resources/resource_iam_member.go b/third_party/terraform/resources/resource_iam_member.go index 76f48bcd1dae..d1a37544caec 100644 --- a/third_party/terraform/resources/resource_iam_member.go +++ b/third_party/terraform/resources/resource_iam_member.go @@ -112,7 +112,7 @@ func resourceIamMemberRead(newUpdaterFunc newResourceIamUpdaterFunc) schema.Read } eMember := getResourceIamMember(d) - p, err := updater.GetResourceIamPolicy() + p, err := iamPolicyReadWithRetry(updater) if err != nil { if isGoogleApiErrorWithCode(err, 404) { log.Printf("[DEBUG]: Binding of member %q with role %q does not exist for non-existent resource %s, removing from state.", eMember.Members[0], eMember.Role, updater.DescribeResource()) diff --git a/third_party/terraform/resources/resource_iam_policy.go b/third_party/terraform/resources/resource_iam_policy.go index 0b7408c9b9e1..25d84c89a9a7 100644 --- a/third_party/terraform/resources/resource_iam_policy.go +++ b/third_party/terraform/resources/resource_iam_policy.go @@ -81,7 +81,7 @@ func ResourceIamPolicyRead(newUpdaterFunc newResourceIamUpdaterFunc) schema.Read return err } - policy, err := updater.GetResourceIamPolicy() + policy, err := iamPolicyReadWithRetry(updater) if err != nil { if isGoogleApiErrorWithCode(err, 404) { log.Printf("[DEBUG]: Policy does not exist for non-existent resource %q", updater.GetResourceId()) diff --git a/third_party/terraform/utils/iam.go b/third_party/terraform/utils/iam.go index 869bdb60149e..1385a1f40454 100644 --- a/third_party/terraform/utils/iam.go +++ b/third_party/terraform/utils/iam.go @@ -10,6 +10,8 @@ import ( "google.golang.org/api/cloudresourcemanager/v1" ) +const maxBackoffSeconds = 30 + // The ResourceIamUpdater interface is implemented for each GCP resource supporting IAM policy. // // Implementations should keep track of the resource identifier. @@ -44,6 +46,26 @@ type iamPolicyModifyFunc func(p *cloudresourcemanager.Policy) error type resourceIdParserFunc func(d *schema.ResourceData, config *Config) error +// Wrapper around updater.GetResourceIamPolicy() to handle retry/backoff +// for just reading policies from IAM +func iamPolicyReadWithRetry(updater ResourceIamUpdater) (*cloudresourcemanager.Policy, error) { + mutexKey := updater.GetMutexKey() + mutexKV.Lock(mutexKey) + defer mutexKV.Unlock(mutexKey) + + log.Printf("[DEBUG] Retrieving policy for %s\n", updater.DescribeResource()) + var policy *cloudresourcemanager.Policy + err := retryTime(func() (perr error) { + policy, perr = updater.GetResourceIamPolicy() + return perr + }, 10) + if err != nil { + return nil, err + } + log.Printf("[DEBUG] Retrieved policy for %s: %+v\n", updater.DescribeResource(), policy) + return policy, nil +} + func iamPolicyReadModifyWrite(updater ResourceIamUpdater, modify iamPolicyModifyFunc) error { mutexKey := updater.GetMutexKey() mutexKV.Lock(mutexKey) @@ -54,6 +76,7 @@ func iamPolicyReadModifyWrite(updater ResourceIamUpdater, modify iamPolicyModify log.Printf("[DEBUG]: Retrieving policy for %s\n", updater.DescribeResource()) p, err := updater.GetResourceIamPolicy() if isGoogleApiErrorWithCode(err, 429) { + log.Printf("[DEBUG] 429 while attempting to read policy for %s, waiting %v before attempting again", updater.DescribeResource(), backoff) time.Sleep(backoff) continue } else if err != nil { @@ -71,7 +94,7 @@ func iamPolicyReadModifyWrite(updater ResourceIamUpdater, modify iamPolicyModify if err == nil { fetchBackoff := 1 * time.Second for successfulFetches := 0; successfulFetches < 3; { - if fetchBackoff > 30*time.Second { + if fetchBackoff > maxBackoffSeconds*time.Second { return fmt.Errorf("Error applying IAM policy to %s: Waited too long for propagation.\n", updater.DescribeResource()) } time.Sleep(fetchBackoff)