diff --git a/.changelog/28868.txt b/.changelog/28868.txt new file mode 100644 index 00000000000..3de13428b0c --- /dev/null +++ b/.changelog/28868.txt @@ -0,0 +1,15 @@ +```release-note:bug +resource/aws_iam_role_policy: Fixed issue that could result in "inconsistent final plan" errors +``` + +```release-note:bug +resource/aws_iam_group_policy: Fixed issue that could result in "inconsistent final plan" errors +``` + +```release-note:bug +resource/aws_iam_role: Fixed issue that could result in "inconsistent final plan" errors +``` + +```release-note:bug +resource/aws_iam_user_policy: Fixed issue that could result in "inconsistent final plan" errors +``` diff --git a/internal/service/iam/group_policy_test.go b/internal/service/iam/group_policy_test.go index 41a226c8262..b32af7e0da1 100644 --- a/internal/service/iam/group_policy_test.go +++ b/internal/service/iam/group_policy_test.go @@ -168,6 +168,34 @@ func TestAccIAMGroupPolicy_generatedName(t *testing.T) { }) } +// When there are unknowns in the policy (interpolation), TF puts a +// random GUID (e.g., 14730d5f-efa3-5a5e-94b5-f8bad6f88282) in state +// at first for the policy which, obviously, behaves differently than +// a JSON policy. This test checks to make sure nothing goes wrong +// during that step. +func TestAccIAMGroupPolicy_unknownsInPolicy(t *testing.T) { + var gp iam.GetGroupPolicyOutput + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_iam_group_policy.test" + groupName := "aws_iam_group.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + ErrorCheck: acctest.ErrorCheck(t, iam.EndpointsID), + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + CheckDestroy: testAccCheckRolePolicyDestroy, + Steps: []resource.TestStep{ + { + Config: testAccGroupPolicyConfig_unknowns(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckGroupPolicyExists(groupName, resourceName, &gp), + resource.TestCheckResourceAttr(resourceName, "name", rName), + ), + }, + }, + }) +} + func testAccCheckGroupPolicyDestroy(s *terraform.State) error { conn := acctest.Provider.Meta().(*conns.AWSClient).IAMConn() @@ -391,3 +419,50 @@ EOF } `, rName) } + +func testAccGroupPolicyConfig_unknowns(rName string) string { + return fmt.Sprintf(` +data "aws_caller_identity" "current" {} + +data "aws_partition" "current" {} + +resource "aws_iam_group" "test" { + name = %[1]q + path = "/" +} + +resource "aws_s3_bucket" "test" { + bucket = %[1]q +} + +resource "aws_s3_bucket_acl" "test" { + bucket = aws_s3_bucket.test.id + acl = "private" +} + +resource "aws_iam_group_policy" "test" { + name = %[1]q + group = aws_iam_group.test.id + + policy = jsonencode({ + Version = "2012-10-17" + Statement = [{ + Sid = "" + Effect = "Allow" + Action = [ + "s3:AbortMultipartUpload", + "s3:GetBucketLocation", + "s3:GetObject", + "s3:ListBucket", + "s3:ListBucketMultipartUploads", + "s3:PutObject", + ] + Resource = [ + aws_s3_bucket.test.arn, + "${aws_s3_bucket.test.arn}/*", + ] + }] + }) +} +`, rName) +} diff --git a/internal/service/iam/role_policy_test.go b/internal/service/iam/role_policy_test.go index ef5aa467627..68bb4798fb6 100644 --- a/internal/service/iam/role_policy_test.go +++ b/internal/service/iam/role_policy_test.go @@ -247,6 +247,34 @@ func TestAccIAMRolePolicy_Policy_invalidResource(t *testing.T) { }) } +// When there are unknowns in the policy (interpolation), TF puts a +// random GUID (e.g., 14730d5f-efa3-5a5e-94b5-f8bad6f88282) in state +// at first for the policy which, obviously, behaves differently than +// a JSON policy. This test checks to make sure nothing goes wrong +// during that step. +func TestAccIAMRolePolicy_unknownsInPolicy(t *testing.T) { + var rolePolicy1 iam.GetRolePolicyOutput + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_iam_role_policy.test" + roleName := "aws_iam_role.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + ErrorCheck: acctest.ErrorCheck(t, iam.EndpointsID), + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + CheckDestroy: testAccCheckRolePolicyDestroy, + Steps: []resource.TestStep{ + { + Config: testAccRolePolicyConfig_unknowns(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckRolePolicyExists(roleName, resourceName, &rolePolicy1), + resource.TestCheckResourceAttr(resourceName, "name", rName), + ), + }, + }, + }) +} + func testAccCheckRolePolicyDestroy(s *terraform.State) error { conn := acctest.Provider.Meta().(*conns.AWSClient).IAMConn() @@ -704,3 +732,62 @@ EOF } `, rName) } + +func testAccRolePolicyConfig_unknowns(rName string) string { + return fmt.Sprintf(` +data "aws_caller_identity" "current" {} + +data "aws_partition" "current" {} + +resource "aws_iam_role" "test" { + name = %[1]q + + assume_role_policy = jsonencode({ + Version = "2012-10-17" + Statement = [{ + Sid = "" + Effect = "Allow" + Principal = { + Service = "firehose.amazonaws.com" + } + Action = "sts:AssumeRole" + }] + }) +} + +resource "aws_s3_bucket" "test" { + bucket = %[1]q +} + +resource "aws_s3_bucket_acl" "test" { + bucket = aws_s3_bucket.test.id + acl = "private" +} + +resource "aws_iam_role_policy" "test" { + name = %[1]q + role = aws_iam_role.test.id + + # tflint-ignore: terraform_deprecated_interpolation + policy = jsonencode({ + Version = "2012-10-17" + Statement = [{ + Sid = "" + Effect = "Allow" + Action = [ + "s3:AbortMultipartUpload", + "s3:GetBucketLocation", + "s3:GetObject", + "s3:ListBucket", + "s3:ListBucketMultipartUploads", + "s3:PutObject", + ] + Resource = [ + aws_s3_bucket.test.arn, + "${aws_s3_bucket.test.arn}/*" + ] + }] + }) +} +`, rName) +} diff --git a/internal/verify/json.go b/internal/verify/json.go index 38edca68e5d..23646e134e1 100644 --- a/internal/verify/json.go +++ b/internal/verify/json.go @@ -150,19 +150,22 @@ func PolicyToSet(exist, new string) (string, error) { // Version not being first is one reason for this error: // MalformedPolicyDocument: The policy failed legacy parsing func LegacyPolicyNormalize(policy interface{}) (string, error) { + if policy == nil || policy.(string) == "" { + return "", nil + } + np, err := structure.NormalizeJsonString(policy) if err != nil { - return "", fmt.Errorf("legacy policy (%s) is invalid JSON: %w", policy, err) + return policy.(string), fmt.Errorf("legacy policy (%s) is invalid JSON: %w", policy, err) } - //fmt.Printf("first norm: %s\n", np) - m := regexp.MustCompile(`(?s)^(\{\n?)(.*?)(,\s*)?( )?("Version":\s*"2012-10-17")(,)?(\n)?(.*?)(\})`) n := m.ReplaceAllString(np, `$1$4$5$3$2$6$7$8$9`) + _, err = structure.NormalizeJsonString(n) if err != nil { - return "", fmt.Errorf("LegacyPolicyNormalize created a policy (%s) that is invalid JSON: %w", n, err) + return policy.(string), fmt.Errorf("LegacyPolicyNormalize created a policy (%s) that is invalid JSON: %w", n, err) } return n, nil diff --git a/internal/verify/json_test.go b/internal/verify/json_test.go index eb758125c2d..daa4d52256d 100644 --- a/internal/verify/json_test.go +++ b/internal/verify/json_test.go @@ -654,8 +654,16 @@ func TestLegacyPolicyNormalize(t *testing.T) { "Version": "2012-10-17" } `, - Expected: ``, - Error: true, + Expected: `{ + "Statement": { + "Effect": "Allow", + "Action": "*", + "Resource": "*" + } + "Version": "2012-10-17" +} +`, + Error: true, }, { Name: "principal",