Merge pull request #22067 from hashicorp/b-iam-ignore-policy-order

IAM: Only set policy if actually changed
hashicorp · Dec 7, 2021 · cba4e74 · cba4e74
2 parents b578f31 + c0ee20d
commit cba4e74
Show file tree

Hide file tree

Showing 13 changed files with 601 additions and 246 deletions.
diff --git a/.changelog/22067.txt b/.changelog/22067.txt
@@ -0,0 +1,15 @@
+```release-note:bug
+resource/aws_iam_group_policy: Fix order-related diffs in `policy`
+```
+
+```release-note:bug
+resource/aws_iam_policy: Fix order-related diffs in `policy`
+```
+
+```release-note:bug
+resource/aws_iam_role_policy: Fix order-related diffs in `policy`
+```
+
+```release-note:bug
+resource/aws_iam_user_policy: Fix order-related diffs in `policy`
+```
diff --git a/go.mod b/go.mod
@@ -10,7 +10,7 @@ require (
 	github.com/fatih/color v1.9.0 // indirect
 	github.com/hashicorp/aws-cloudformation-resource-schema-sdk-go v0.14.0
 	github.com/hashicorp/aws-sdk-go-base v1.0.0
-	github.com/hashicorp/awspolicyequivalence v1.3.0
+	github.com/hashicorp/awspolicyequivalence v1.4.0
 	github.com/hashicorp/go-cleanhttp v0.5.2
 	github.com/hashicorp/go-cty v1.4.1-0.20200414143053-d3edf31b6320
 	github.com/hashicorp/go-multierror v1.1.1

diff --git a/go.sum b/go.sum
@@ -170,8 +170,8 @@ github.com/hashicorp/aws-cloudformation-resource-schema-sdk-go v0.14.0 h1:2Usl5C
 github.com/hashicorp/aws-cloudformation-resource-schema-sdk-go v0.14.0/go.mod h1:C6GVuO9RWOrt6QCGTmLCOYuSHpkfQSBDuRqTteOlo0g=
 github.com/hashicorp/aws-sdk-go-base v1.0.0 h1:J7MMLOfSoDWkusy+cSzKYG1/aFyCzYJmdE0mod3/WLw=
 github.com/hashicorp/aws-sdk-go-base v1.0.0/go.mod h1:2fRjWDv3jJBeN6mVWFHV6hFTNeFBx2gpDLQaZNxUVAY=
-github.com/hashicorp/awspolicyequivalence v1.3.0 h1:T6GloJqof+yP6Gf4NAfIRPrTIIsJohVAk5z6MHuJK2U=
-github.com/hashicorp/awspolicyequivalence v1.3.0/go.mod h1:9IOaIHx+a7C0NfUNk1A93M7kHd5rJ19aoUx37LZGC14=
+github.com/hashicorp/awspolicyequivalence v1.4.0 h1:mpQ7/MnyOsaMcXQcJiYbE3LAONzMH1MnwEK/HMvE6Ss=
+github.com/hashicorp/awspolicyequivalence v1.4.0/go.mod h1:9IOaIHx+a7C0NfUNk1A93M7kHd5rJ19aoUx37LZGC14=
 github.com/hashicorp/errwrap v1.0.0 h1:hLrqtEDnRye3+sgx6z4qVLNuviH3MR5aQ0ykNJa/UYA=
 github.com/hashicorp/errwrap v1.0.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4=
 github.com/hashicorp/go-checkpoint v0.5.0 h1:MFYpPZCnQqQTE18jFwSII6eUQrD/oxMFp3mlgcqk5mU=

diff --git a/internal/service/iam/group_policy.go b/internal/service/iam/group_policy.go
@@ -138,10 +138,14 @@ func resourceGroupPolicyRead(d *schema.ResourceData, meta interface{}) error {
 		return err
 	}
 
-	if err := d.Set("policy", policy); err != nil {
-		return fmt.Errorf("error setting policy: %s", err)
+	policyToSet, err := verify.SecondJSONUnlessEquivalent(d.Get("policy").(string), policy)
+
+	if err != nil {
+		return fmt.Errorf("while setting policy (%s), encountered: %w", policyToSet, err)
 	}
 
+	d.Set("policy", policyToSet)
+
 	if err := d.Set("name", name); err != nil {
 		return fmt.Errorf("error setting name: %s", err)
 	}

diff --git a/internal/service/iam/group_policy_test.go b/internal/service/iam/group_policy_test.go
@@ -18,15 +18,16 @@ import (
 
 func TestAccIAMGroupPolicy_basic(t *testing.T) {
 	var groupPolicy1, groupPolicy2 iam.GetGroupPolicyOutput
-	rInt := sdkacctest.RandInt()
+	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
+
 	resource.ParallelTest(t, resource.TestCase{
 		PreCheck:     func() { acctest.PreCheck(t) },
 		ErrorCheck:   acctest.ErrorCheck(t, iam.EndpointsID),
 		Providers:    acctest.Providers,
 		CheckDestroy: testAccCheckIAMGroupPolicyDestroy,
 		Steps: []resource.TestStep{
 			{
-				Config: testAccIAMGroupPolicyConfig(rInt),
+				Config: testAccIAMGroupPolicyConfig(rName),
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckIAMGroupPolicyExists(
 						"aws_iam_group.group",
@@ -41,7 +42,7 @@ func TestAccIAMGroupPolicy_basic(t *testing.T) {
 				ImportStateVerify: true,
 			},
 			{
-				Config: testAccIAMGroupPolicyConfigUpdate(rInt),
+				Config: testAccIAMGroupPolicyConfigUpdate(rName),
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckIAMGroupPolicyExists(
 						"aws_iam_group.group",
@@ -62,7 +63,7 @@ func TestAccIAMGroupPolicy_basic(t *testing.T) {
 
 func TestAccIAMGroupPolicy_disappears(t *testing.T) {
 	var out iam.GetGroupPolicyOutput
-	rInt := sdkacctest.RandInt()
+	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
 
 	resource.ParallelTest(t, resource.TestCase{
 		PreCheck:     func() { acctest.PreCheck(t) },
@@ -71,7 +72,7 @@ func TestAccIAMGroupPolicy_disappears(t *testing.T) {
 		CheckDestroy: testAccCheckIAMGroupPolicyDestroy,
 		Steps: []resource.TestStep{
 			{
-				Config: testAccIAMGroupPolicyConfig(rInt),
+				Config: testAccIAMGroupPolicyConfig(rName),
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckIAMGroupPolicyExists(
 						"aws_iam_group.group",
@@ -88,15 +89,16 @@ func TestAccIAMGroupPolicy_disappears(t *testing.T) {
 
 func TestAccIAMGroupPolicy_namePrefix(t *testing.T) {
 	var groupPolicy1, groupPolicy2 iam.GetGroupPolicyOutput
-	rInt := sdkacctest.RandInt()
+	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
+
 	resource.ParallelTest(t, resource.TestCase{
 		PreCheck:     func() { acctest.PreCheck(t) },
 		ErrorCheck:   acctest.ErrorCheck(t, iam.EndpointsID),
 		Providers:    acctest.Providers,
 		CheckDestroy: testAccCheckIAMGroupPolicyDestroy,
 		Steps: []resource.TestStep{
 			{
-				Config: testAccIAMGroupPolicyConfig_namePrefix(rInt, "*"),
+				Config: testAccIAMGroupPolicyConfig_namePrefix(rName, "*"),
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckIAMGroupPolicyExists(
 						"aws_iam_group.test",
@@ -106,7 +108,7 @@ func TestAccIAMGroupPolicy_namePrefix(t *testing.T) {
 				),
 			},
 			{
-				Config: testAccIAMGroupPolicyConfig_namePrefix(rInt, "ec2:*"),
+				Config: testAccIAMGroupPolicyConfig_namePrefix(rName, "ec2:*"),
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckIAMGroupPolicyExists(
 						"aws_iam_group.test",
@@ -128,15 +130,16 @@ func TestAccIAMGroupPolicy_namePrefix(t *testing.T) {
 
 func TestAccIAMGroupPolicy_generatedName(t *testing.T) {
 	var groupPolicy1, groupPolicy2 iam.GetGroupPolicyOutput
-	rInt := sdkacctest.RandInt()
+	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
+
 	resource.ParallelTest(t, resource.TestCase{
 		PreCheck:     func() { acctest.PreCheck(t) },
 		ErrorCheck:   acctest.ErrorCheck(t, iam.EndpointsID),
 		Providers:    acctest.Providers,
 		CheckDestroy: testAccCheckIAMGroupPolicyDestroy,
 		Steps: []resource.TestStep{
 			{
-				Config: testAccIAMGroupPolicyConfig_generatedName(rInt, "*"),
+				Config: testAccIAMGroupPolicyConfig_generatedName(rName, "*"),
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckIAMGroupPolicyExists(
 						"aws_iam_group.test",
@@ -146,7 +149,7 @@ func TestAccIAMGroupPolicy_generatedName(t *testing.T) {
 				),
 			},
 			{
-				Config: testAccIAMGroupPolicyConfig_generatedName(rInt, "ec2:*"),
+				Config: testAccIAMGroupPolicyConfig_generatedName(rName, "ec2:*"),
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckIAMGroupPolicyExists(
 						"aws_iam_group.test",
@@ -274,15 +277,15 @@ func testAccCheckGroupPolicyNameMatches(i, j *iam.GetGroupPolicyOutput) resource
 	}
 }
 
-func testAccIAMGroupPolicyConfig(rInt int) string {
+func testAccIAMGroupPolicyConfig(rName string) string {
 	return fmt.Sprintf(`
 resource "aws_iam_group" "group" {
-  name = "test_group_%d"
+  name = %[1]q
   path = "/"
 }
 
 resource "aws_iam_group_policy" "foo" {
-  name  = "foo_policy_%d"
+  name  = %[1]q
   group = aws_iam_group.group.name
 
   policy = <<EOF
@@ -296,38 +299,38 @@ resource "aws_iam_group_policy" "foo" {
 }
 EOF
 }
-`, rInt, rInt)
+`, rName)
 }
 
-func testAccIAMGroupPolicyConfig_namePrefix(rInt int, policyAction string) string {
+func testAccIAMGroupPolicyConfig_namePrefix(rName, policyAction string) string {
 	return fmt.Sprintf(`
 resource "aws_iam_group" "test" {
-  name = "test_group_%d"
+  name = %[1]q
   path = "/"
 }
 
 resource "aws_iam_group_policy" "test" {
-  name_prefix = "test-%d"
+  name_prefix = %[1]q
   group       = aws_iam_group.test.name
 
   policy = <<EOF
 {
   "Version": "2012-10-17",
   "Statement": {
     "Effect": "Allow",
-    "Action": "%s",
+    "Action": %[2]q,
     "Resource": "*"
   }
 }
 EOF
 }
-`, rInt, rInt, policyAction)
+`, rName, policyAction)
 }
 
-func testAccIAMGroupPolicyConfig_generatedName(rInt int, policyAction string) string {
+func testAccIAMGroupPolicyConfig_generatedName(rName, policyAction string) string {
 	return fmt.Sprintf(`
 resource "aws_iam_group" "test" {
-  name = "test_group_%d"
+  name = %[1]q
   path = "/"
 }
 
@@ -339,24 +342,24 @@ resource "aws_iam_group_policy" "test" {
   "Version": "2012-10-17",
   "Statement": {
     "Effect": "Allow",
-    "Action": "%s",
+    "Action": %[2]q,
     "Resource": "*"
   }
 }
 EOF
 }
-`, rInt, policyAction)
+`, rName, policyAction)
 }
 
-func testAccIAMGroupPolicyConfigUpdate(rInt int) string {
+func testAccIAMGroupPolicyConfigUpdate(rName string) string {
 	return fmt.Sprintf(`
 resource "aws_iam_group" "group" {
-  name = "test_group_%d"
+  name = %[1]q
   path = "/"
 }
 
 resource "aws_iam_group_policy" "foo" {
-  name  = "foo_policy_%d"
+  name  = %[1]q
   group = aws_iam_group.group.name
 
   policy = <<EOF
@@ -372,7 +375,7 @@ EOF
 }
 
 resource "aws_iam_group_policy" "bar" {
-  name  = "bar_policy_%d"
+  name  = "%[1]s-2"
   group = aws_iam_group.group.name
 
   policy = <<EOF
@@ -386,5 +389,5 @@ resource "aws_iam_group_policy" "bar" {
 }
 EOF
 }
-`, rInt, rInt, rInt)
+`, rName)
 }
diff --git a/internal/service/iam/policy.go b/internal/service/iam/policy.go
@@ -11,6 +11,7 @@ import (
 	"github.com/hashicorp/aws-sdk-go-base/tfawserr"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/structure"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
 	"github.com/hashicorp/terraform-provider-aws/internal/conns"
 	tftags "github.com/hashicorp/terraform-provider-aws/internal/tags"
@@ -97,10 +98,16 @@ func resourcePolicyCreate(d *schema.ResourceData, meta interface{}) error {
 		name = resource.UniqueId()
 	}
 
+	policy, err := structure.NormalizeJsonString(d.Get("policy").(string))
+
+	if err != nil {
+		return fmt.Errorf("policy (%s) is invalid JSON: %w", policy, err)
+	}
+
 	request := &iam.CreatePolicyInput{
 		Description:    aws.String(d.Get("description").(string)),
 		Path:           aws.String(d.Get("path").(string)),
-		PolicyDocument: aws.String(d.Get("policy").(string)),
+		PolicyDocument: aws.String(policy),
 		PolicyName:     aws.String(name),
 		Tags:           Tags(tags.IgnoreAWS()),
 	}
@@ -227,7 +234,19 @@ func resourcePolicyRead(d *schema.ResourceData, meta interface{}) error {
 		}
 	}
 
-	d.Set("policy", policyDocument)
+	policyToSet, err := verify.SecondJSONUnlessEquivalent(d.Get("policy").(string), policyDocument)
+
+	if err != nil {
+		return fmt.Errorf("while setting policy (%s), encountered: %w", policyToSet, err)
+	}
+
+	policyToSet, err = structure.NormalizeJsonString(policyToSet)
+
+	if err != nil {
+		return fmt.Errorf("policy (%s) is invalid JSON: %w", policyToSet, err)
+	}
+
+	d.Set("policy", policyToSet)
 
 	return nil
 }
@@ -241,9 +260,15 @@ func resourcePolicyUpdate(d *schema.ResourceData, meta interface{}) error {
 			return err
 		}
 
+		policy, err := structure.NormalizeJsonString(d.Get("policy").(string))
+
+		if err != nil {
+			return fmt.Errorf("policy (%s) is invalid JSON: %w", policy, err)
+		}
+
 		request := &iam.CreatePolicyVersionInput{
 			PolicyArn:      aws.String(d.Id()),
-			PolicyDocument: aws.String(d.Get("policy").(string)),
+			PolicyDocument: aws.String(policy),
 			SetAsDefault:   aws.Bool(true),
 		}
 

diff --git a/internal/service/iam/policy_test.go b/internal/service/iam/policy_test.go
@@ -20,19 +20,8 @@ func TestAccIAMPolicy_basic(t *testing.T) {
 	var out iam.GetPolicyOutput
 	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
 	resourceName := "aws_iam_policy.test"
-	expectedPolicyText := `{
-  "Version": "2012-10-17",
-  "Statement": [
-    {
-      "Action": [
-        "ec2:Describe*"
-      ],
-      "Effect": "Allow",
-      "Resource": "*"
-    }
-  ]
-}
-`
+	expectedPolicyText := `{"Statement":[{"Action":["ec2:Describe*"],"Effect":"Allow","Resource":"*"}],"Version":"2012-10-17"}`
+
 	resource.ParallelTest(t, resource.TestCase{
 		PreCheck:     func() { acctest.PreCheck(t) },
 		ErrorCheck:   acctest.ErrorCheck(t, iam.EndpointsID),
@@ -158,7 +147,6 @@ func TestAccIAMPolicy_disappears(t *testing.T) {
 
 func TestAccIAMPolicy_namePrefix(t *testing.T) {
 	var out iam.GetPolicyOutput
-	namePrefix := "tf-acc-test-"
 	resourceName := "aws_iam_policy.test"
 
 	resource.ParallelTest(t, resource.TestCase{
@@ -168,10 +156,10 @@ func TestAccIAMPolicy_namePrefix(t *testing.T) {
 		CheckDestroy: testAccCheckPolicyDestroy,
 		Steps: []resource.TestStep{
 			{
-				Config: testAccPolicyNamePrefixConfig(namePrefix),
+				Config: testAccPolicyNamePrefixConfig(acctest.ResourcePrefix),
 				Check: resource.ComposeTestCheckFunc(
 					testAccCheckPolicyExists(resourceName, &out),
-					resource.TestMatchResourceAttr(resourceName, "name", regexp.MustCompile(fmt.Sprintf("^%s", namePrefix))),
+					resource.TestMatchResourceAttr(resourceName, "name", regexp.MustCompile(fmt.Sprintf("^%s", acctest.ResourcePrefix))),
 				),
 			},
 			{
@@ -215,8 +203,8 @@ func TestAccIAMPolicy_policy(t *testing.T) {
 	var out iam.GetPolicyOutput
 	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
 	resourceName := "aws_iam_policy.test"
-	policy1 := "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Action\":[\"ec2:Describe*\"],\"Effect\":\"Allow\",\"Resource\":\"*\"}]}"
-	policy2 := "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Action\":[\"ec2:*\"],\"Effect\":\"Allow\",\"Resource\":\"*\"}]}"
+	policy1 := `{"Statement":[{"Action":["ec2:Describe*"],"Effect":"Allow","Resource":"*"}],"Version":"2012-10-17"}`
+	policy2 := `{"Statement":[{"Action":["ec2:*"],"Effect":"Allow","Resource":"*"}],"Version":"2012-10-17"}`
 
 	resource.ParallelTest(t, resource.TestCase{
 		PreCheck:     func() { acctest.PreCheck(t) },