Merge pull request #28868 from hashicorp/b-iam-policy-fixes

iam: Provider produced inconsistent final plan

.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
+```

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)
+}

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)
+}

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

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",