Merge pull request #39842 from hashicorp/b-iam_policy-corrupted-state

internal/verify: handle policy parsing errors of state content

.changelog/39842.txt

@@ -0,0 +1,19 @@
+```release-note:bug
+resource/aws_iam_policy: Fix persistent validation errors when malformed `policy` content is written to state
+```
+```release-note:bug
+resource/aws_iam_role_policy: Fix persistent validation errors when malformed `policy` content is written to state
+```
+```release-note:bug
+resource/aws_ecr_repository_policy: Fix persistent validation errors when malformed `policy` content is written to state
+```
+```release-note:bug
+resource/aws_secretsmanager_secret: Fix persistent validation errors when malformed `policy` content is written to state
+```
+```release-note:bug
+resource/aws_s3_bucket_policy: Fix persistent validation errors when malformed `policy` content is written to state
+```
+```release-note:bug
+resource/aws_kms_key: Fix persistent validation errors when malformed `policy` content is written to state
+```
+

internal/service/iam/policy_test.go

@@ -349,6 +349,57 @@ func TestAccIAMPolicy_policyDuplicateKeys(t *testing.T) {
 	})
 }
 
+// TestAccIAMPolicy_malformedCondition verifies that malformed policy content
+// that is stored in state does not prevent subsequent plan and apply operations
+// from proceeding.
+//
+// Ref: https://github.com/hashicorp/terraform-provider-aws/issues/39833
+func TestAccIAMPolicy_malformedCondition(t *testing.T) {
+	ctx := acctest.Context(t)
+	var out awstypes.Policy
+	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
+	resourceName := "aws_iam_policy.test"
+	expectedPolicyText1 := `{"Statement":[{"Action":["s3:ListBucket"],"Effect":"Allow","Resource":"*"}],"Version":"2012-10-17"}`
+	expectedPolicyText2 := `{"Statement":[{"Action":["s3:ListBucket"],"Condition":{"StringLike":["demo-prefix/"]}",Effect":"Allow","Resource":"*"}],"Version":"2012-10-17"}`
+	expectedPolicyText3 := `{"Statement":[{"Action":["s3:ListBucket"],"Condition":{"StringLike":{"s3:prefix":["demo-prefix/"]}},"Effect":"Allow","Resource":"*"}],"Version":"2012-10-17"}`
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:                 func() { acctest.PreCheck(ctx, t) },
+		ErrorCheck:               acctest.ErrorCheck(t, names.IAMServiceID),
+		ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
+		CheckDestroy:             testAccCheckPolicyDestroy(ctx),
+		Steps: []resource.TestStep{
+			{
+				Config: testAccPolicyConfig_MalformedCondition_setup(rName),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckPolicyExists(ctx, resourceName, &out),
+					resource.TestCheckResourceAttr(resourceName, names.AttrName, rName),
+					resource.TestCheckResourceAttr(resourceName, names.AttrPolicy, expectedPolicyText1),
+				),
+			},
+			{
+				Config:      testAccPolicyConfig_MalformedCondition_failure(rName),
+				ExpectError: regexache.MustCompile(`MalformedPolicyDocument: Syntax errors in policy`),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckPolicyExists(ctx, resourceName, &out),
+					resource.TestCheckResourceAttr(resourceName, names.AttrName, rName),
+					// Because this is a Plugin SDK V2-based resource, the malformed content
+					// is stored in state despite the failed update.
+					resource.TestCheckResourceAttr(resourceName, names.AttrPolicy, expectedPolicyText2),
+				),
+			},
+			{
+				Config: testAccPolicyConfig_MalformedCondition_fix(rName),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckPolicyExists(ctx, resourceName, &out),
+					resource.TestCheckResourceAttr(resourceName, names.AttrName, rName),
+					resource.TestCheckResourceAttr(resourceName, names.AttrPolicy, expectedPolicyText3),
+				),
+			},
+		},
+	})
+}
+
 func testAccCheckPolicyExists(ctx context.Context, n string, v *awstypes.Policy) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
 		rs, ok := s.RootModule().Resources[n]
@@ -624,3 +675,74 @@ EOF
 }
 `, rName)
 }
+
+func testAccPolicyConfig_MalformedCondition_setup(rName string) string {
+	return fmt.Sprintf(`
+resource "aws_iam_policy" "test" {
+  name = %q
+
+  policy = jsonencode({
+    Version = "2012-10-17"
+    Statement = [
+      {
+        Effect   = "Allow"
+        Resource = "*"
+        Action = [
+          "s3:ListBucket",
+        ]
+      },
+    ]
+  })
+}
+`, rName)
+}
+
+func testAccPolicyConfig_MalformedCondition_failure(rName string) string {
+	return fmt.Sprintf(`
+resource "aws_iam_policy" "test" {
+  name = %q
+
+  policy = jsonencode({
+    Version = "2012-10-17"
+    Statement = [
+      {
+        Effect   = "Allow"
+        Resource = "*"
+        Action = [
+          "s3:ListBucket",
+        ]
+        "Condition" : {
+          "StringLike" : ["demo-prefix/"]
+        }
+      },
+    ]
+  })
+}
+`, rName)
+}
+
+func testAccPolicyConfig_MalformedCondition_fix(rName string) string {
+	return fmt.Sprintf(`
+resource "aws_iam_policy" "test" {
+  name = %q
+
+  policy = jsonencode({
+    Version = "2012-10-17"
+    Statement = [
+      {
+        Effect   = "Allow"
+        Resource = "*"
+        Action = [
+          "s3:ListBucket",
+        ]
+        "Condition" : {
+          "StringLike" : {
+            "s3:prefix" : ["demo-prefix/"]
+          }
+        }
+      },
+    ]
+  })
+}
+`, rName)
+}

internal/verify/json.go

@@ -15,6 +15,7 @@ import (
 	awspolicy "github.com/hashicorp/awspolicyequivalence"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/structure"
+	"github.com/hashicorp/terraform-provider-aws/internal/errs"
 )
 
 // SuppressEquivalentPolicyDiffs returns a difference suppression function that compares
@@ -126,6 +127,11 @@ func JSONBytesEqual(b1, b2 []byte) bool {
 	return reflect.DeepEqual(o1, o2)
 }
 
+// SecondJSONUnlessEquivalent returns the second JSON string unless
+// the AWS policy content is deemed equivalent.
+//
+// If parsing of the policy content from the first argument fails, the
+// second is returned and no error is raised.
 func SecondJSONUnlessEquivalent(old, new string) (string, error) {
 	// valid empty JSON is "{}" not "" so handle special case to avoid
 	// Error unmarshaling policy: unexpected end of JSON input
@@ -144,6 +150,17 @@ func SecondJSONUnlessEquivalent(old, new string) (string, error) {
 	equivalent, err := awspolicy.PoliciesAreEquivalent(old, new)
 
 	if err != nil {
+		// Plugin SDK V2 based resources can set malformed policy content in state
+		// despite a failed update. In these cases, parsing the "old" content
+		// will fail. Surfacing this error during read operations causes a
+		// persistent plan-time validation error, so return the "new" content
+		// read directly from the remote resource instead.
+		//
+		// Ref: https://github.com/hashicorp/terraform-provider-aws/issues/39833
+		if errs.Contains(err, "parsing policy 1") {
+			return new, nil
+		}
+
 		return "", err
 	}
 

internal/verify/json_test.go

@@ -354,6 +354,48 @@ func TestSecondJSONUnlessEquivalent(t *testing.T) {
 			newPolicy: "",
 			want:      "",
 		},
+		{
+			name: "malformed old",
+			oldPolicy: `{
+  "Version": "2012-10-17",
+  "Statement": [
+    {
+      "Effect": "Allow",
+      "Action": [
+        "s3:ListBucket"
+      ],
+      "Condition" : {
+        "StringLike" : ["demo-prefix/"]
+      },
+      "Resource": "*"
+    }
+  ]
+}`,
+			newPolicy: `{
+  "Version": "2012-10-17",
+  "Statement": [
+    {
+      "Effect": "Allow",
+      "Action": [
+        "s3:ListBucket"
+      ],
+      "Resource": "*"
+    }
+  ]
+}`,
+			want: `{
+  "Version": "2012-10-17",
+  "Statement": [
+    {
+      "Effect": "Allow",
+      "Action": [
+        "s3:ListBucket"
+      ],
+      "Resource": "*"
+    }
+  ]
+}`,
+		},
 	}
 
 	for _, v := range testCases {