From d8b91ebaca0cb14d223f5229e4a395977f3a01e8 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Thu, 7 Mar 2019 13:18:52 -0500 Subject: [PATCH] resource/aws_iam_role_policy_attachment: Prevent NoSuchEntity errors from race conditions Ensure we catch the `NoSuchEntity` error in the Read and Delete logic to prevent this error during race conditions as we know how to appropriately handle it in both cases. We also ensure the acceptance testing is correctly verifying the removal of IAM Role Policy Attachments. Output from acceptance testing: ``` --- PASS: TestAccAWSRolePolicyAttachment_disappears (10.62s) --- PASS: TestAccAWSRolePolicyAttachment_disappears_Role (10.70s) --- PASS: TestAccAWSRolePolicyAttachment_basic (16.23s) ``` --- ...resource_aws_iam_role_policy_attachment.go | 69 +++++----- ...rce_aws_iam_role_policy_attachment_test.go | 125 ++++++++++++++++++ 2 files changed, 164 insertions(+), 30 deletions(-) diff --git a/aws/resource_aws_iam_role_policy_attachment.go b/aws/resource_aws_iam_role_policy_attachment.go index d4f4975f917..d35db1c862e 100644 --- a/aws/resource_aws_iam_role_policy_attachment.go +++ b/aws/resource_aws_iam_role_policy_attachment.go @@ -6,7 +6,6 @@ import ( "strings" "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/service/iam" "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/helper/schema" @@ -54,44 +53,29 @@ func resourceAwsIamRolePolicyAttachmentCreate(d *schema.ResourceData, meta inter func resourceAwsIamRolePolicyAttachmentRead(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).iamconn role := d.Get("role").(string) - arn := d.Get("policy_arn").(string) - - _, err := conn.GetRole(&iam.GetRoleInput{ - RoleName: aws.String(role), - }) + policyARN := d.Get("policy_arn").(string) - if err != nil { - if awsErr, ok := err.(awserr.Error); ok { - if awsErr.Code() == "NoSuchEntity" { - log.Printf("[WARN] No such entity found for Policy Attachment (%s)", role) - d.SetId("") - return nil - } - } - return err - } + hasPolicyAttachment, err := iamRoleHasPolicyARNAttachment(conn, role, policyARN) - args := iam.ListAttachedRolePoliciesInput{ - RoleName: aws.String(role), + if isAWSErr(err, iam.ErrCodeNoSuchEntityException, "") { + log.Printf("[WARN] IAM Role (%s) not found, removing from state", role) + d.SetId("") + return nil } - var policy string - err = conn.ListAttachedRolePoliciesPages(&args, func(page *iam.ListAttachedRolePoliciesOutput, lastPage bool) bool { - for _, p := range page.AttachedPolicies { - if *p.PolicyArn == arn { - policy = *p.PolicyArn - } - } - return policy == "" - }) if err != nil { - return err + return fmt.Errorf("error finding IAM Role (%s) Policy Attachment (%s): %s", role, policyARN, err) } - if policy == "" { - log.Printf("[WARN] No such policy found for Role Policy Attachment (%s)", role) + + if !hasPolicyAttachment { + log.Printf("[WARN] IAM Role (%s) Policy Attachment (%s) not found, removing from state", role, policyARN) d.SetId("") + return nil } + d.Set("policy_arn", policyARN) + d.Set("role", role) + return nil } @@ -101,6 +85,11 @@ func resourceAwsIamRolePolicyAttachmentDelete(d *schema.ResourceData, meta inter arn := d.Get("policy_arn").(string) err := detachPolicyFromRole(conn, role, arn) + + if isAWSErr(err, iam.ErrCodeNoSuchEntityException, "") { + return nil + } + if err != nil { return fmt.Errorf("Error removing policy %s from IAM Role %s: %v", arn, role, err) } @@ -138,3 +127,23 @@ func detachPolicyFromRole(conn *iam.IAM, role string, arn string) error { }) return err } + +func iamRoleHasPolicyARNAttachment(conn *iam.IAM, role string, policyARN string) (bool, error) { + hasPolicyAttachment := false + input := &iam.ListAttachedRolePoliciesInput{ + RoleName: aws.String(role), + } + + err := conn.ListAttachedRolePoliciesPages(input, func(page *iam.ListAttachedRolePoliciesOutput, lastPage bool) bool { + for _, p := range page.AttachedPolicies { + if aws.StringValue(p.PolicyArn) == policyARN { + hasPolicyAttachment = true + return false + } + } + + return !lastPage + }) + + return hasPolicyAttachment, err +} diff --git a/aws/resource_aws_iam_role_policy_attachment_test.go b/aws/resource_aws_iam_role_policy_attachment_test.go index 6f0918629fd..52b21f529ef 100644 --- a/aws/resource_aws_iam_role_policy_attachment_test.go +++ b/aws/resource_aws_iam_role_policy_attachment_test.go @@ -62,7 +62,84 @@ func TestAccAWSRolePolicyAttachment_basic(t *testing.T) { }, }) } + +func TestAccAWSRolePolicyAttachment_disappears(t *testing.T) { + var attachedRolePolicies iam.ListAttachedRolePoliciesOutput + + rName := acctest.RandomWithPrefix("tf-acc-test") + resourceName := "aws_iam_role_policy_attachment.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSRolePolicyAttachmentDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAWSIAMRolePolicyAttachmentConfig(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSRolePolicyAttachmentExists(resourceName, 1, &attachedRolePolicies), + testAccCheckAWSIAMRolePolicyAttachmentDisappears(resourceName), + ), + ExpectNonEmptyPlan: true, + }, + }, + }) +} + +func TestAccAWSRolePolicyAttachment_disappears_Role(t *testing.T) { + var attachedRolePolicies iam.ListAttachedRolePoliciesOutput + var role iam.GetRoleOutput + + rName := acctest.RandomWithPrefix("tf-acc-test") + iamRoleResourceName := "aws_iam_role.test" + resourceName := "aws_iam_role_policy_attachment.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSRolePolicyAttachmentDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAWSIAMRolePolicyAttachmentConfig(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSRoleExists(iamRoleResourceName, &role), + testAccCheckAWSRolePolicyAttachmentExists(resourceName, 1, &attachedRolePolicies), + // DeleteConflict: Cannot delete entity, must detach all policies first. + testAccCheckAWSIAMRolePolicyAttachmentDisappears(resourceName), + testAccCheckAWSRoleDisappears(&role), + ), + ExpectNonEmptyPlan: true, + }, + }, + }) +} + func testAccCheckAWSRolePolicyAttachmentDestroy(s *terraform.State) error { + conn := testAccProvider.Meta().(*AWSClient).iamconn + + for _, rs := range s.RootModule().Resources { + if rs.Type != "aws_iam_role_policy_attachment" { + continue + } + + policyARN := rs.Primary.Attributes["policy_arn"] + role := rs.Primary.Attributes["role"] + + hasPolicyAttachment, err := iamRoleHasPolicyARNAttachment(conn, role, policyARN) + + if isAWSErr(err, iam.ErrCodeNoSuchEntityException, "") { + continue + } + + if err != nil { + return err + } + + if hasPolicyAttachment { + return fmt.Errorf("IAM Role (%s) Policy Attachment (%s) still exists", role, policyARN) + } + } + return nil } @@ -114,6 +191,23 @@ func testAccCheckAWSRolePolicyAttachmentAttributes(policies []string, out *iam.L } } +func testAccCheckAWSIAMRolePolicyAttachmentDisappears(resourceName string) resource.TestCheckFunc { + return func(s *terraform.State) error { + conn := testAccProvider.Meta().(*AWSClient).iamconn + + rs, ok := s.RootModule().Resources[resourceName] + + if !ok { + return fmt.Errorf("Not found: %s", resourceName) + } + + policyARN := rs.Primary.Attributes["policy_arn"] + role := rs.Primary.Attributes["role"] + + return detachPolicyFromRole(conn, role, policyARN) + } +} + func testAccAWSIAMRolePolicyAttachmentImportStateIdFunc(resourceName string) resource.ImportStateIdFunc { return func(s *terraform.State) (string, error) { rs, ok := s.RootModule().Resources[resourceName] @@ -259,3 +353,34 @@ EOF policy_arn = "${aws_iam_policy.policy3.arn}" }`, rInt, rInt, rInt, rInt) } + +func testAccAWSIAMRolePolicyAttachmentConfig(rName string) string { + return fmt.Sprintf(` +data "aws_partition" "current" {} + +resource "aws_iam_role" "test" { + name = %[1]q + + assume_role_policy = <