From dc1b8099d869f929645947e1f7079ee77f694556 Mon Sep 17 00:00:00 2001 From: Derek Helmick Date: Sun, 30 Jan 2022 17:18:36 -0500 Subject: [PATCH 1/3] aws_cloudformation_stack: use retry logic on IAM role error --- .changelog/22840.txt | 3 + internal/service/cloudformation/stack.go | 45 ++++++++++- internal/service/cloudformation/stack_test.go | 80 +++++++++++++++++++ 3 files changed, 124 insertions(+), 4 deletions(-) create mode 100644 .changelog/22840.txt diff --git a/.changelog/22840.txt b/.changelog/22840.txt new file mode 100644 index 00000000000..7d39bedd95d --- /dev/null +++ b/.changelog/22840.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/aws_cloudformation_stack: Add retry logic to create and update function on IAM Role failure +``` diff --git a/internal/service/cloudformation/stack.go b/internal/service/cloudformation/stack.go index 5feadbb88d8..b5f3a832b53 100644 --- a/internal/service/cloudformation/stack.go +++ b/internal/service/cloudformation/stack.go @@ -13,7 +13,9 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" "github.com/hashicorp/terraform-provider-aws/internal/conns" "github.com/hashicorp/terraform-provider-aws/internal/flex" + tfiam "github.com/hashicorp/terraform-provider-aws/internal/service/iam" tftags "github.com/hashicorp/terraform-provider-aws/internal/tags" + "github.com/hashicorp/terraform-provider-aws/internal/tfresource" "github.com/hashicorp/terraform-provider-aws/internal/verify" ) @@ -181,14 +183,32 @@ func resourceStackCreate(d *schema.ResourceData, meta interface{}) error { } log.Printf("[DEBUG] Creating CloudFormation Stack: %s", input) - output, err := conn.CreateStack(input) + + err := resource.Retry(tfiam.PropagationTimeout, func() *resource.RetryError { + output, err := conn.CreateStack(input) + if tfawserr.ErrMessageContains(err, "ValidationError", "is invalid or cannot be assumed") { + return resource.RetryableError(err) + } + + if err != nil { + return resource.NonRetryableError(err) + } + + d.SetId(aws.StringValue(output.StackId)) + return nil + }) + + var output *cloudformation.CreateStackOutput + + if tfresource.TimedOut(err) { + output, err = conn.CreateStack(input) + d.SetId(aws.StringValue(output.StackId)) + } if err != nil { return fmt.Errorf("error creating CloudFormation Stack (%s): %w", name, err) } - d.SetId(aws.StringValue(output.StackId)) - if _, err := WaitStackCreated(conn, d.Id(), requestToken, d.Timeout(schema.TimeoutCreate)); err != nil { return fmt.Errorf("error waiting for CloudFormation Stack (%s) create: %w", d.Id(), err) } @@ -359,7 +379,24 @@ func resourceStackUpdate(d *schema.ResourceData, meta interface{}) error { } log.Printf("[DEBUG] Updating CloudFormation stack: %s", input) - _, err := conn.UpdateStack(input) + + err := resource.Retry(tfiam.PropagationTimeout, func() *resource.RetryError { + _, err := conn.UpdateStack(input) + if tfawserr.ErrMessageContains(err, "ValidationError", "is invalid or cannot be assumed") { + return resource.RetryableError(err) + } + + if err != nil { + return resource.NonRetryableError(err) + } + + return nil + }) + + if tfresource.TimedOut(err) { + _, err = conn.UpdateStack(input) + } + if tfawserr.ErrMessageContains(err, "ValidationError", "No updates are to be performed.") { log.Printf("[DEBUG] Current CloudFormation stack has no updates") } else if err != nil { diff --git a/internal/service/cloudformation/stack_test.go b/internal/service/cloudformation/stack_test.go index 48af97c0bb8..f8ddb81ac7b 100644 --- a/internal/service/cloudformation/stack_test.go +++ b/internal/service/cloudformation/stack_test.go @@ -444,6 +444,27 @@ func TestAccCloudFormationStack_onFailure(t *testing.T) { }) } +func TestAccCloudFormationStack_withIAM(t *testing.T) { + var stack cloudformation.Stack + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_cloudformation_stack.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + ErrorCheck: acctest.ErrorCheck(t, cloudformation.EndpointsID), + Providers: acctest.Providers, + CheckDestroy: testAccCheckDestroy, + Steps: []resource.TestStep{ + { + Config: testAccStackConfig_withIAM(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckCloudFormationStackExists(resourceName, &stack), + ), + }, + }, + }) +} + func testAccCheckCloudFormationStackExists(n string, stack *cloudformation.Stack) resource.TestCheckFunc { return func(s *terraform.State) error { rs, ok := s.RootModule().Resources[n] @@ -1011,3 +1032,62 @@ resource "aws_cloudformation_stack" "test" { } `, rName) } + +func testAccStackConfig_withIAM(rName string) string { + return fmt.Sprintf(` +data "aws_iam_policy_document" "test" { + statement { + actions = ["sts:AssumeRole"] + principals { + type = "Service" + identifiers = ["cloudformation.amazonaws.com"] + } + } +} + +resource "aws_iam_role" "test" { + assume_role_policy = data.aws_iam_policy_document.test.json +} + +resource "aws_iam_policy" "test" { + policy = jsonencode({ + Version = "2012-10-17", + Statement = [ + { + Effect = "Allow" + Action = ["s3:*"] + Resource = "*" + } + ] + }) +} + +resource "aws_iam_role_policy_attachment" "test" { + policy_arn = aws_iam_policy.test.arn + role = aws_iam_role.test.name +} + +data "aws_iam_role" "test" { + # This is introduced so the aws_cloudformation_stack has a dependency on the role_attachment + # instead of the role directly. Without it, the policy may not exist before cloudformation + # has a chance to delete it's resources + name = aws_iam_role_policy_attachment.test.role +} + +resource "aws_cloudformation_stack" "test" { + name = %[1]q + + on_failure = "DO_NOTHING" + iam_role_arn = aws_iam_role.test.arn + + template_body = jsonencode({ + AWSTemplateFormatVersion = "2010-09-09" + Resources = { + S3Bucket = { + Type = "AWS::S3::Bucket" + } + } + }) +} +`, rName) +} From 7830a70050a696bc53b48c6eb33e2878a34c5e9e Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Mon, 31 Jan 2022 08:14:33 -0500 Subject: [PATCH 2/3] Update 22840.txt --- .changelog/22840.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changelog/22840.txt b/.changelog/22840.txt index 7d39bedd95d..63044a75bef 100644 --- a/.changelog/22840.txt +++ b/.changelog/22840.txt @@ -1,3 +1,3 @@ ```release-note:bug -resource/aws_cloudformation_stack: Add retry logic to create and update function on IAM Role failure +resource/aws_cloudformation_stack: Retry resource Create and Update for IAM eventual consistency ``` From 8841f7798fd09e7be54577cc5ace0df7084ff2b2 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Mon, 31 Jan 2022 08:57:38 -0500 Subject: [PATCH 3/3] Use 'tfresource.RetryWhen'. --- internal/service/cloudformation/stack.go | 72 +++++++---------- internal/service/cloudformation/stack_test.go | 80 ------------------- 2 files changed, 28 insertions(+), 124 deletions(-) diff --git a/internal/service/cloudformation/stack.go b/internal/service/cloudformation/stack.go index b5f3a832b53..f8aad350b1a 100644 --- a/internal/service/cloudformation/stack.go +++ b/internal/service/cloudformation/stack.go @@ -183,32 +183,25 @@ func resourceStackCreate(d *schema.ResourceData, meta interface{}) error { } log.Printf("[DEBUG] Creating CloudFormation Stack: %s", input) + outputRaw, err := tfresource.RetryWhen(tfiam.PropagationTimeout, + func() (interface{}, error) { + return conn.CreateStack(input) + }, + func(err error) (bool, error) { + if tfawserr.ErrMessageContains(err, "ValidationError", "is invalid or cannot be assumed") { + return true, err + } - err := resource.Retry(tfiam.PropagationTimeout, func() *resource.RetryError { - output, err := conn.CreateStack(input) - if tfawserr.ErrMessageContains(err, "ValidationError", "is invalid or cannot be assumed") { - return resource.RetryableError(err) - } - - if err != nil { - return resource.NonRetryableError(err) - } - - d.SetId(aws.StringValue(output.StackId)) - return nil - }) - - var output *cloudformation.CreateStackOutput - - if tfresource.TimedOut(err) { - output, err = conn.CreateStack(input) - d.SetId(aws.StringValue(output.StackId)) - } + return false, err + }, + ) if err != nil { return fmt.Errorf("error creating CloudFormation Stack (%s): %w", name, err) } + d.SetId(aws.StringValue(outputRaw.(*cloudformation.CreateStackOutput).StackId)) + if _, err := WaitStackCreated(conn, d.Id(), requestToken, d.Timeout(schema.TimeoutCreate)); err != nil { return fmt.Errorf("error waiting for CloudFormation Stack (%s) create: %w", d.Id(), err) } @@ -378,38 +371,29 @@ func resourceStackUpdate(d *schema.ResourceData, meta interface{}) error { input.RoleARN = aws.String(d.Get("iam_role_arn").(string)) } - log.Printf("[DEBUG] Updating CloudFormation stack: %s", input) - - err := resource.Retry(tfiam.PropagationTimeout, func() *resource.RetryError { - _, err := conn.UpdateStack(input) - if tfawserr.ErrMessageContains(err, "ValidationError", "is invalid or cannot be assumed") { - return resource.RetryableError(err) - } - - if err != nil { - return resource.NonRetryableError(err) - } - - return nil - }) + log.Printf("[DEBUG] Updating CloudFormation Stack: %s", input) + _, err := tfresource.RetryWhen(tfiam.PropagationTimeout, + func() (interface{}, error) { + return conn.UpdateStack(input) + }, + func(err error) (bool, error) { + if tfawserr.ErrMessageContains(err, "ValidationError", "is invalid or cannot be assumed") { + return true, err + } - if tfresource.TimedOut(err) { - _, err = conn.UpdateStack(input) - } + return false, err + }, + ) - if tfawserr.ErrMessageContains(err, "ValidationError", "No updates are to be performed.") { - log.Printf("[DEBUG] Current CloudFormation stack has no updates") - } else if err != nil { - return fmt.Errorf("error updating CloudFormation stack (%s): %w", d.Id(), err) + if err != nil { + return fmt.Errorf("error updating CloudFormation Stack (%s): %w", d.Id(), err) } _, err = WaitStackUpdated(conn, d.Id(), requestToken, d.Timeout(schema.TimeoutUpdate)) if err != nil { - return fmt.Errorf("error waiting for CloudFormation Stack update: %w", err) + return fmt.Errorf("error waiting for CloudFormation Stack (%s) update: %w", d.Id(), err) } - log.Printf("[INFO] CloudFormation stack (%s) updated", d.Id()) - return resourceStackRead(d, meta) } diff --git a/internal/service/cloudformation/stack_test.go b/internal/service/cloudformation/stack_test.go index f8ddb81ac7b..48af97c0bb8 100644 --- a/internal/service/cloudformation/stack_test.go +++ b/internal/service/cloudformation/stack_test.go @@ -444,27 +444,6 @@ func TestAccCloudFormationStack_onFailure(t *testing.T) { }) } -func TestAccCloudFormationStack_withIAM(t *testing.T) { - var stack cloudformation.Stack - rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) - resourceName := "aws_cloudformation_stack.test" - - resource.ParallelTest(t, resource.TestCase{ - PreCheck: func() { acctest.PreCheck(t) }, - ErrorCheck: acctest.ErrorCheck(t, cloudformation.EndpointsID), - Providers: acctest.Providers, - CheckDestroy: testAccCheckDestroy, - Steps: []resource.TestStep{ - { - Config: testAccStackConfig_withIAM(rName), - Check: resource.ComposeTestCheckFunc( - testAccCheckCloudFormationStackExists(resourceName, &stack), - ), - }, - }, - }) -} - func testAccCheckCloudFormationStackExists(n string, stack *cloudformation.Stack) resource.TestCheckFunc { return func(s *terraform.State) error { rs, ok := s.RootModule().Resources[n] @@ -1032,62 +1011,3 @@ resource "aws_cloudformation_stack" "test" { } `, rName) } - -func testAccStackConfig_withIAM(rName string) string { - return fmt.Sprintf(` -data "aws_iam_policy_document" "test" { - statement { - actions = ["sts:AssumeRole"] - principals { - type = "Service" - identifiers = ["cloudformation.amazonaws.com"] - } - } -} - -resource "aws_iam_role" "test" { - assume_role_policy = data.aws_iam_policy_document.test.json -} - -resource "aws_iam_policy" "test" { - policy = jsonencode({ - Version = "2012-10-17", - Statement = [ - { - Effect = "Allow" - Action = ["s3:*"] - Resource = "*" - } - ] - }) -} - -resource "aws_iam_role_policy_attachment" "test" { - policy_arn = aws_iam_policy.test.arn - role = aws_iam_role.test.name -} - -data "aws_iam_role" "test" { - # This is introduced so the aws_cloudformation_stack has a dependency on the role_attachment - # instead of the role directly. Without it, the policy may not exist before cloudformation - # has a chance to delete it's resources - name = aws_iam_role_policy_attachment.test.role -} - -resource "aws_cloudformation_stack" "test" { - name = %[1]q - - on_failure = "DO_NOTHING" - iam_role_arn = aws_iam_role.test.arn - - template_body = jsonencode({ - AWSTemplateFormatVersion = "2010-09-09" - Resources = { - S3Bucket = { - Type = "AWS::S3::Bucket" - } - } - }) -} -`, rName) -}