From 6d260dfb14164883e8b3224c57ff720d832bed7c Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 12 Jan 2023 18:48:15 -0500 Subject: [PATCH 1/5] iam: Provider produced inconsistent final plan --- internal/service/iam/group_policy_test.go | 75 ++++++++++++++++++++ internal/service/iam/role_policy.go | 7 +- internal/service/iam/role_policy_test.go | 86 +++++++++++++++++++++++ internal/verify/json.go | 11 +-- 4 files changed, 172 insertions(+), 7 deletions(-) diff --git a/internal/service/iam/group_policy_test.go b/internal/service/iam/group_policy_test.go index 41a226c8262..1fc6ff871e8 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.go b/internal/service/iam/role_policy.go index 33ca348f606..22957c8f561 100644 --- a/internal/service/iam/role_policy.go +++ b/internal/service/iam/role_policy.go @@ -11,6 +11,7 @@ import ( "github.com/hashicorp/aws-sdk-go-base/v2/awsv1shim/v2/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-provider-aws/internal/conns" "github.com/hashicorp/terraform-provider-aws/internal/tfresource" "github.com/hashicorp/terraform-provider-aws/internal/verify" @@ -41,7 +42,7 @@ func ResourceRolePolicy() *schema.Resource { DiffSuppressFunc: verify.SuppressEquivalentPolicyDiffs, DiffSuppressOnRefresh: true, StateFunc: func(v interface{}) string { - json, _ := verify.LegacyPolicyNormalize(v) + json, _ := structure.NormalizeJsonString(v) return json }, }, @@ -73,7 +74,7 @@ func ResourceRolePolicy() *schema.Resource { func resourceRolePolicyPut(d *schema.ResourceData, meta interface{}) error { conn := meta.(*conns.AWSClient).IAMConn() - policy, err := verify.LegacyPolicyNormalize(d.Get("policy").(string)) + policy, err := structure.NormalizeJsonString(d.Get("policy").(string)) if err != nil { return fmt.Errorf("policy (%s) is invalid JSON: %w", policy, err) } @@ -155,7 +156,7 @@ func resourceRolePolicyRead(d *schema.ResourceData, meta interface{}) error { return err } - policyToSet, err := verify.LegacyPolicyToSet(d.Get("policy").(string), policy) + policyToSet, err := verify.PolicyToSet(d.Get("policy").(string), policy) if err != nil { return fmt.Errorf("while setting policy (%s), encountered: %w", policyToSet, err) } diff --git a/internal/service/iam/role_policy_test.go b/internal/service/iam/role_policy_test.go index ef5aa467627..95419817270 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,61 @@ 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 + + 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 From 7e3dd3ed27ac71f849a05ee05d8694776abcc178 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 12 Jan 2023 18:55:08 -0500 Subject: [PATCH 2/5] Add changelog --- .changelog/28868.txt | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 .changelog/28868.txt 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 +``` From 347ae0d15687ad793b4d18a1a69b4409c9c4477b Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 12 Jan 2023 19:02:11 -0500 Subject: [PATCH 3/5] Revert to legacy policy handling --- internal/service/iam/role_policy.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/internal/service/iam/role_policy.go b/internal/service/iam/role_policy.go index 22957c8f561..33ca348f606 100644 --- a/internal/service/iam/role_policy.go +++ b/internal/service/iam/role_policy.go @@ -11,7 +11,6 @@ import ( "github.com/hashicorp/aws-sdk-go-base/v2/awsv1shim/v2/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-provider-aws/internal/conns" "github.com/hashicorp/terraform-provider-aws/internal/tfresource" "github.com/hashicorp/terraform-provider-aws/internal/verify" @@ -42,7 +41,7 @@ func ResourceRolePolicy() *schema.Resource { DiffSuppressFunc: verify.SuppressEquivalentPolicyDiffs, DiffSuppressOnRefresh: true, StateFunc: func(v interface{}) string { - json, _ := structure.NormalizeJsonString(v) + json, _ := verify.LegacyPolicyNormalize(v) return json }, }, @@ -74,7 +73,7 @@ func ResourceRolePolicy() *schema.Resource { func resourceRolePolicyPut(d *schema.ResourceData, meta interface{}) error { conn := meta.(*conns.AWSClient).IAMConn() - policy, err := structure.NormalizeJsonString(d.Get("policy").(string)) + policy, err := verify.LegacyPolicyNormalize(d.Get("policy").(string)) if err != nil { return fmt.Errorf("policy (%s) is invalid JSON: %w", policy, err) } @@ -156,7 +155,7 @@ func resourceRolePolicyRead(d *schema.ResourceData, meta interface{}) error { return err } - policyToSet, err := verify.PolicyToSet(d.Get("policy").(string), policy) + policyToSet, err := verify.LegacyPolicyToSet(d.Get("policy").(string), policy) if err != nil { return fmt.Errorf("while setting policy (%s), encountered: %w", policyToSet, err) } From d55ca04d25cf60a5bad10aebc44605f84023a13b Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 12 Jan 2023 21:59:34 -0500 Subject: [PATCH 4/5] Fix tests/ci --- internal/service/iam/group_policy_test.go | 1 + internal/service/iam/role_policy_test.go | 1 + internal/verify/json_test.go | 12 ++++++++++-- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/internal/service/iam/group_policy_test.go b/internal/service/iam/group_policy_test.go index 1fc6ff871e8..1e60ad05c79 100644 --- a/internal/service/iam/group_policy_test.go +++ b/internal/service/iam/group_policy_test.go @@ -444,6 +444,7 @@ resource "aws_iam_group_policy" "test" { name = %[1]q group = aws_iam_group.test.id + # tflint-ignore: terraform_deprecated_interpolation policy = jsonencode({ Version = "2012-10-17" Statement = [{ diff --git a/internal/service/iam/role_policy_test.go b/internal/service/iam/role_policy_test.go index 95419817270..51a301efbf3 100644 --- a/internal/service/iam/role_policy_test.go +++ b/internal/service/iam/role_policy_test.go @@ -768,6 +768,7 @@ 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 = [{ 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", From 26b24ec70236406f89dc668845954b5d807f1e82 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 12 Jan 2023 22:02:17 -0500 Subject: [PATCH 5/5] iam: Interpolation fixes --- internal/service/iam/group_policy_test.go | 3 +-- internal/service/iam/role_policy_test.go | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/internal/service/iam/group_policy_test.go b/internal/service/iam/group_policy_test.go index 1e60ad05c79..b32af7e0da1 100644 --- a/internal/service/iam/group_policy_test.go +++ b/internal/service/iam/group_policy_test.go @@ -444,7 +444,6 @@ resource "aws_iam_group_policy" "test" { name = %[1]q group = aws_iam_group.test.id - # tflint-ignore: terraform_deprecated_interpolation policy = jsonencode({ Version = "2012-10-17" Statement = [{ @@ -459,7 +458,7 @@ resource "aws_iam_group_policy" "test" { "s3:PutObject", ] Resource = [ - "${aws_s3_bucket.test.arn}", + aws_s3_bucket.test.arn, "${aws_s3_bucket.test.arn}/*", ] }] diff --git a/internal/service/iam/role_policy_test.go b/internal/service/iam/role_policy_test.go index 51a301efbf3..68bb4798fb6 100644 --- a/internal/service/iam/role_policy_test.go +++ b/internal/service/iam/role_policy_test.go @@ -783,7 +783,7 @@ resource "aws_iam_role_policy" "test" { "s3:PutObject", ] Resource = [ - "${aws_s3_bucket.test.arn}", + aws_s3_bucket.test.arn, "${aws_s3_bucket.test.arn}/*" ] }]