From 80bf0baa44c3fcfe00b9f293d55a71376875c572 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Tue, 30 Nov 2021 14:42:52 -0500 Subject: [PATCH 1/9] Add helper func for equivalent policies --- internal/verify/policies.go | 32 ++++ internal/verify/policies_test.go | 305 +++++++++++++++++++++++++++++++ 2 files changed, 337 insertions(+) create mode 100644 internal/verify/policies.go create mode 100644 internal/verify/policies_test.go diff --git a/internal/verify/policies.go b/internal/verify/policies.go new file mode 100644 index 00000000000..057ed3ed98d --- /dev/null +++ b/internal/verify/policies.go @@ -0,0 +1,32 @@ +package verify + +import ( + awspolicy "github.com/jen20/awspolicyequivalence" +) + +func PolicyToSet(old, new string) (string, error) { + // valid empty JSON is "{}" not "" so handle special case to avoid + // Error unmarshaling policy: unexpected end of JSON input + if old == "" || new == "" { + return new, nil + } + + equivalent, err := awspolicy.PoliciesAreEquivalent(old, new) + + if err != nil { + return "", err + } + + if equivalent { + return old, nil + } + return new, nil + /* + buff := bytes.NewBufferString("") + if err := json.Compact(buff, []byte(new)); err != nil { + return "", fmt.Errorf("unable to compact JSON (%s): %w", new, err) + } + + return buff.String(), nil + */ +} diff --git a/internal/verify/policies_test.go b/internal/verify/policies_test.go new file mode 100644 index 00000000000..8ad584d7516 --- /dev/null +++ b/internal/verify/policies_test.go @@ -0,0 +1,305 @@ +package verify + +import "testing" + +func TestPolicyToSet(t *testing.T) { + testCases := []struct { + name string + oldPolicy string + newPolicy string + want string + }{ + { + name: "new in random order", + oldPolicy: `{ + "Version": "2012-10-17", + "Id": "kms-tf-1", + "Statement": [ + { + "Sid": "Enable IAM User Permissions", + "Effect": "Allow", + "Principal": { + "AWS": [ + "arn:aws:iam::012345678901:role/felixjaehn", + "arn:aws:iam::012345678901:role/garethemery", + "arn:aws:iam::012345678901:role/kidnap", + "arn:aws:iam::012345678901:role/paulvandyk", + "arn:aws:iam::012345678901:role/tinlicker" + ] + }, + "Action": [ + "kms:CreateKey", + "kms:DescribeKey", + "kms:ScheduleKeyDeletion", + "kms:Describe*", + "kms:Get*", + "kms:List*", + "kms:TagResource", + "kms:UntagResource" + ], + "Resource": "*" + } + ] +}`, + newPolicy: `{ + "Version": "2012-10-17", + "Id": "kms-tf-1", + "Statement": [ + { + "Sid": "Enable IAM User Permissions", + "Effect": "Allow", + "Principal": { + "AWS": [ + "arn:aws:iam::012345678901:role/tinlicker", + "arn:aws:iam::012345678901:role/paulvandyk", + "arn:aws:iam::012345678901:role/kidnap", + "arn:aws:iam::012345678901:role/garethemery", + "arn:aws:iam::012345678901:role/felixjaehn" + ] + }, + "Action": [ + "kms:CreateKey", + "kms:DescribeKey", + "kms:ScheduleKeyDeletion", + "kms:Describe*", + "kms:Get*", + "kms:List*", + "kms:TagResource", + "kms:UntagResource" + ], + "Resource": "*" + } + ] +}`, + want: `{ + "Version": "2012-10-17", + "Id": "kms-tf-1", + "Statement": [ + { + "Sid": "Enable IAM User Permissions", + "Effect": "Allow", + "Principal": { + "AWS": [ + "arn:aws:iam::012345678901:role/felixjaehn", + "arn:aws:iam::012345678901:role/garethemery", + "arn:aws:iam::012345678901:role/kidnap", + "arn:aws:iam::012345678901:role/paulvandyk", + "arn:aws:iam::012345678901:role/tinlicker" + ] + }, + "Action": [ + "kms:CreateKey", + "kms:DescribeKey", + "kms:ScheduleKeyDeletion", + "kms:Describe*", + "kms:Get*", + "kms:List*", + "kms:TagResource", + "kms:UntagResource" + ], + "Resource": "*" + } + ] +}`, + }, + { + name: "actual change", + oldPolicy: `{ + "Version": "2012-10-17", + "Id": "kms-tf-1", + "Statement": [ + { + "Sid": "Enable IAM User Permissions", + "Effect": "Allow", + "Principal": { + "AWS": [ + "arn:aws:iam::012345678901:role/felixjaehn", + "arn:aws:iam::012345678901:role/garethemery", + "arn:aws:iam::012345678901:role/kidnap", + "arn:aws:iam::012345678901:role/paulvandyk", + "arn:aws:iam::012345678901:role/tinlicker" + ] + }, + "Action": [ + "kms:CreateKey", + "kms:DescribeKey", + "kms:ScheduleKeyDeletion", + "kms:Describe*", + "kms:Get*", + "kms:List*", + "kms:TagResource", + "kms:UntagResource" + ], + "Resource": "*" + } + ] +}`, + newPolicy: `{ + "Version": "2012-10-17", + "Id": "kms-tf-1", + "Statement": [ + { + "Sid": "Enable IAM User Permissions", + "Effect": "Allow", + "Principal": { + "AWS": [ + "arn:aws:iam::012345678901:role/tinlicker", + "arn:aws:iam::012345678901:role/paulvandyk", + "arn:aws:iam::012345678901:role/garethemery", + "arn:aws:iam::012345678901:role/felixjaehn" + ] + }, + "Action": [ + "kms:CreateKey", + "kms:DescribeKey", + "kms:ScheduleKeyDeletion", + "kms:Describe*", + "kms:Get*", + "kms:List*", + "kms:TagResource", + "kms:UntagResource" + ], + "Resource": "*" + } + ] +}`, + want: `{ + "Version": "2012-10-17", + "Id": "kms-tf-1", + "Statement": [ + { + "Sid": "Enable IAM User Permissions", + "Effect": "Allow", + "Principal": { + "AWS": [ + "arn:aws:iam::012345678901:role/tinlicker", + "arn:aws:iam::012345678901:role/paulvandyk", + "arn:aws:iam::012345678901:role/garethemery", + "arn:aws:iam::012345678901:role/felixjaehn" + ] + }, + "Action": [ + "kms:CreateKey", + "kms:DescribeKey", + "kms:ScheduleKeyDeletion", + "kms:Describe*", + "kms:Get*", + "kms:List*", + "kms:TagResource", + "kms:UntagResource" + ], + "Resource": "*" + } + ] +}`, + }, + { + name: "empty old", + oldPolicy: "", + newPolicy: `{ + "Version": "2012-10-17", + "Id": "kms-tf-1", + "Statement": [ + { + "Sid": "Enable IAM User Permissions", + "Effect": "Allow", + "Principal": { + "AWS": [ + "arn:aws:iam::012345678901:role/tinlicker", + "arn:aws:iam::012345678901:role/paulvandyk", + "arn:aws:iam::012345678901:role/garethemery", + "arn:aws:iam::012345678901:role/felixjaehn" + ] + }, + "Action": [ + "kms:CreateKey", + "kms:DescribeKey", + "kms:ScheduleKeyDeletion", + "kms:Describe*", + "kms:Get*", + "kms:List*", + "kms:TagResource", + "kms:UntagResource" + ], + "Resource": "*" + } + ] +}`, + want: `{ + "Version": "2012-10-17", + "Id": "kms-tf-1", + "Statement": [ + { + "Sid": "Enable IAM User Permissions", + "Effect": "Allow", + "Principal": { + "AWS": [ + "arn:aws:iam::012345678901:role/tinlicker", + "arn:aws:iam::012345678901:role/paulvandyk", + "arn:aws:iam::012345678901:role/garethemery", + "arn:aws:iam::012345678901:role/felixjaehn" + ] + }, + "Action": [ + "kms:CreateKey", + "kms:DescribeKey", + "kms:ScheduleKeyDeletion", + "kms:Describe*", + "kms:Get*", + "kms:List*", + "kms:TagResource", + "kms:UntagResource" + ], + "Resource": "*" + } + ] +}`, + }, + { + name: "empty new", + oldPolicy: `{ + "Version": "2012-10-17", + "Id": "kms-tf-1", + "Statement": [ + { + "Sid": "Enable IAM User Permissions", + "Effect": "Allow", + "Principal": { + "AWS": [ + "arn:aws:iam::012345678901:role/tinlicker", + "arn:aws:iam::012345678901:role/paulvandyk", + "arn:aws:iam::012345678901:role/garethemery", + "arn:aws:iam::012345678901:role/felixjaehn" + ] + }, + "Action": [ + "kms:CreateKey", + "kms:DescribeKey", + "kms:ScheduleKeyDeletion", + "kms:Describe*", + "kms:Get*", + "kms:List*", + "kms:TagResource", + "kms:UntagResource" + ], + "Resource": "*" + } + ] +}`, + newPolicy: "", + want: "", + }, + } + + for _, v := range testCases { + got, err := PolicyToSet(v.oldPolicy, v.newPolicy) + + if err != nil { + t.Fatalf("unexpected error with test case %s: %s", v.name, err) + } + + if got != v.want { + t.Fatalf("for test case %s, got %s, wanted %s", v.name, got, v.want) + } + } +} From 9e88c9f5063a7ee1d6ea8b9d869197de99bb99be Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Tue, 30 Nov 2021 14:44:43 -0500 Subject: [PATCH 2/9] Update key to ignore equivalent JSON on read --- internal/service/kms/key.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/internal/service/kms/key.go b/internal/service/kms/key.go index 510f541fe88..67772bd7c41 100644 --- a/internal/service/kms/key.go +++ b/internal/service/kms/key.go @@ -199,7 +199,14 @@ func resourceKeyRead(d *schema.ResourceData, meta interface{}) error { d.Set("key_id", key.metadata.KeyId) d.Set("key_usage", key.metadata.KeyUsage) d.Set("multi_region", key.metadata.MultiRegion) - d.Set("policy", key.policy) + + policyToSet, err := verify.PolicyToSet(d.Get("policy").(string), key.policy) + + if err != nil { + return fmt.Errorf("while setting policy (%s), encountered: %w", key.policy, err) + } + + d.Set("policy", policyToSet) tags := key.tags.IgnoreAWS().IgnoreConfig(ignoreTagsConfig) From 29dd279f7629559e2a174bbdf7189798a203a977 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Tue, 30 Nov 2021 14:45:16 -0500 Subject: [PATCH 3/9] Update tests for equivalent JSON --- internal/service/kms/key_test.go | 263 +++++++++++++++++++++++++------ 1 file changed, 213 insertions(+), 50 deletions(-) diff --git a/internal/service/kms/key_test.go b/internal/service/kms/key_test.go index 3522deb2bf4..f8ad562d02b 100644 --- a/internal/service/kms/key_test.go +++ b/internal/service/kms/key_test.go @@ -121,11 +121,11 @@ func TestAccKMSKey_asymmetricKey(t *testing.T) { }) } -func TestAccKMSKey_policy(t *testing.T) { +func TestAccKMSKey_Policy_basic(t *testing.T) { var key kms.KeyMetadata rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) resourceName := "aws_kms_key.test" - expectedPolicyText := `{"Version":"2012-10-17","Id":"kms-tf-1","Statement":[{"Sid":"Enable IAM User Permissions","Effect":"Allow","Principal":{"AWS":"*"},"Action":"kms:*","Resource":"*"}]}` + expectedPolicyText := fmt.Sprintf(`{"Version":"2012-10-17","Id":%[1]q,"Statement":[{"Sid":"Enable IAM User Permissions","Effect":"Allow","Principal":{"AWS":"*"},"Action":"kms:*","Resource":"*"}]}`, rName) resource.ParallelTest(t, resource.TestCase{ PreCheck: func() { acctest.PreCheck(t) }, @@ -156,7 +156,7 @@ func TestAccKMSKey_policy(t *testing.T) { }) } -func TestAccKMSKey_policyBypass(t *testing.T) { +func TestAccKMSKey_Policy_bypass(t *testing.T) { var key kms.KeyMetadata rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) resourceName := "aws_kms_key.test" @@ -188,7 +188,7 @@ func TestAccKMSKey_policyBypass(t *testing.T) { }) } -func TestAccKMSKey_policyBypassUpdate(t *testing.T) { +func TestAccKMSKey_Policy_bypassUpdate(t *testing.T) { var before, after kms.KeyMetadata rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) resourceName := "aws_kms_key.test" @@ -244,6 +244,49 @@ func TestAccKMSKey_Policy_iamRole(t *testing.T) { }) } +// Reference: https://github.com/hashicorp/terraform-provider-aws/issues/11801 +func TestAccKMSKey_Policy_iamRoleOrder(t *testing.T) { + var key kms.KeyMetadata + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_kms_key.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + ErrorCheck: acctest.ErrorCheck(t, kms.EndpointsID), + Providers: acctest.Providers, + CheckDestroy: testAccCheckKeyDestroy, + Steps: []resource.TestStep{ + { + Config: testAccKeyPolicyIAMMultiRoleConfig(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckKeyExists(resourceName, &key), + ), + }, + { + Config: testAccKeyPolicyIAMMultiRoleConfig(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckKeyExists(resourceName, &key), + ), + PlanOnly: true, + }, + { + Config: testAccKeyPolicyIAMMultiRoleConfig(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckKeyExists(resourceName, &key), + ), + PlanOnly: true, + }, + { + Config: testAccKeyPolicyIAMMultiRoleConfig(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckKeyExists(resourceName, &key), + ), + PlanOnly: true, + }, + }, + }) +} + // Reference: https://github.com/hashicorp/terraform-provider-aws/issues/7646 func TestAccKMSKey_Policy_iamServiceLinkedRole(t *testing.T) { var key kms.KeyMetadata @@ -495,23 +538,19 @@ resource "aws_kms_key" "test" { description = %[1]q deletion_window_in_days = 7 - policy = < Date: Tue, 30 Nov 2021 16:34:16 -0500 Subject: [PATCH 4/9] Add changelog --- .changelog/21969.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/21969.txt diff --git a/.changelog/21969.txt b/.changelog/21969.txt new file mode 100644 index 00000000000..6ef9286b97b --- /dev/null +++ b/.changelog/21969.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/aws_kms_key: Fix order-related diffs with `policy` +``` \ No newline at end of file From fa22542919442acb73a4ad7cefd31de3fda29370 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Tue, 30 Nov 2021 16:34:45 -0500 Subject: [PATCH 5/9] Switch to data.aws_iam_policy_document like op --- internal/service/kms/key_test.go | 76 ++++++++++++++++---------------- 1 file changed, 38 insertions(+), 38 deletions(-) diff --git a/internal/service/kms/key_test.go b/internal/service/kms/key_test.go index f8ad562d02b..41502179d5a 100644 --- a/internal/service/kms/key_test.go +++ b/internal/service/kms/key_test.go @@ -732,48 +732,48 @@ resource "aws_iam_role" "test5" { }) } +data "aws_iam_policy_document" "test" { + policy_id = %[1]q + statement { + actions = [ + "kms:*", + ] + effect = "Allow" + principals { + identifiers = ["*"] + type = "AWS" + } + resources = ["*"] + } + + statement { + actions = [ + "kms:Encrypt", + "kms:Decrypt", + "kms:ReEncrypt*", + "kms:GenerateDataKey*", + "kms:DescribeKey", + ] + effect = "Allow" + principals { + identifiers = [ + aws_iam_role.test2.arn, + aws_iam_role.test1.arn, + aws_iam_role.test4.arn, + aws_iam_role.test3.arn, + aws_iam_role.test5.arn, + ] + type = "AWS" + } + resources = ["*"] + } +} + resource "aws_kms_key" "test" { description = %[1]q deletion_window_in_days = 7 - policy = jsonencode({ - Id = %[1]q - Statement = [ - { - Action = "kms:*" - Effect = "Allow" - Principal = { - AWS = "*" - } - - Resource = "*" - Sid = "Enable IAM User Permissions" - }, - { - Action = [ - "kms:Encrypt", - "kms:Decrypt", - "kms:ReEncrypt*", - "kms:GenerateDataKey*", - "kms:DescribeKey", - ] - Effect = "Allow" - Principal = { - AWS = [ - aws_iam_role.test2.arn, - aws_iam_role.test1.arn, - aws_iam_role.test4.arn, - aws_iam_role.test3.arn, - aws_iam_role.test5.arn, - ] - } - - Resource = "*" - Sid = "Enable IAM User Permissions" - }, - ] - Version = "2012-10-17" - }) + policy = data.aws_iam_policy_document.test.json } `, rName) } From a814d63853ba4a9588243cd741a5b7fe384c94a6 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Tue, 30 Nov 2021 16:55:45 -0500 Subject: [PATCH 6/9] Minor cleanup, randomizing --- .changelog/21969.txt | 2 +- internal/service/kms/key.go | 2 +- internal/service/kms/key_test.go | 2 +- internal/verify/policies.go | 11 ++--------- internal/verify/policies_test.go | 2 +- 5 files changed, 6 insertions(+), 13 deletions(-) diff --git a/.changelog/21969.txt b/.changelog/21969.txt index 6ef9286b97b..595760cf968 100644 --- a/.changelog/21969.txt +++ b/.changelog/21969.txt @@ -1,3 +1,3 @@ ```release-note:bug -resource/aws_kms_key: Fix order-related diffs with `policy` +resource/aws_kms_key: Fix order-related diffs in `policy` ``` \ No newline at end of file diff --git a/internal/service/kms/key.go b/internal/service/kms/key.go index 67772bd7c41..c5612a6d62a 100644 --- a/internal/service/kms/key.go +++ b/internal/service/kms/key.go @@ -200,7 +200,7 @@ func resourceKeyRead(d *schema.ResourceData, meta interface{}) error { d.Set("key_usage", key.metadata.KeyUsage) d.Set("multi_region", key.metadata.MultiRegion) - policyToSet, err := verify.PolicyToSet(d.Get("policy").(string), key.policy) + policyToSet, err := verify.NewPolicyUnlessEquivalent(d.Get("policy").(string), key.policy) if err != nil { return fmt.Errorf("while setting policy (%s), encountered: %w", key.policy, err) diff --git a/internal/service/kms/key_test.go b/internal/service/kms/key_test.go index 41502179d5a..98090628a9e 100644 --- a/internal/service/kms/key_test.go +++ b/internal/service/kms/key_test.go @@ -618,7 +618,7 @@ resource "aws_kms_key" "test" { deletion_window_in_days = 7 policy = jsonencode({ - Id = %[1]q + Id = %[1]q Statement = [ { Action = "kms:*" diff --git a/internal/verify/policies.go b/internal/verify/policies.go index 057ed3ed98d..b73ab9482ae 100644 --- a/internal/verify/policies.go +++ b/internal/verify/policies.go @@ -4,7 +4,7 @@ import ( awspolicy "github.com/jen20/awspolicyequivalence" ) -func PolicyToSet(old, new string) (string, error) { +func NewPolicyUnlessEquivalent(old, new string) (string, error) { // valid empty JSON is "{}" not "" so handle special case to avoid // Error unmarshaling policy: unexpected end of JSON input if old == "" || new == "" { @@ -20,13 +20,6 @@ func PolicyToSet(old, new string) (string, error) { if equivalent { return old, nil } - return new, nil - /* - buff := bytes.NewBufferString("") - if err := json.Compact(buff, []byte(new)); err != nil { - return "", fmt.Errorf("unable to compact JSON (%s): %w", new, err) - } - return buff.String(), nil - */ + return new, nil } diff --git a/internal/verify/policies_test.go b/internal/verify/policies_test.go index 8ad584d7516..31e1d0955b1 100644 --- a/internal/verify/policies_test.go +++ b/internal/verify/policies_test.go @@ -292,7 +292,7 @@ func TestPolicyToSet(t *testing.T) { } for _, v := range testCases { - got, err := PolicyToSet(v.oldPolicy, v.newPolicy) + got, err := NewPolicyUnlessEquivalent(v.oldPolicy, v.newPolicy) if err != nil { t.Fatalf("unexpected error with test case %s: %s", v.name, err) From e659453c4fe278ba81cf527025f5f0b73007411c Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Tue, 30 Nov 2021 16:58:24 -0500 Subject: [PATCH 7/9] Switch test to correct name --- internal/verify/policies_test.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/internal/verify/policies_test.go b/internal/verify/policies_test.go index 31e1d0955b1..583086acd0a 100644 --- a/internal/verify/policies_test.go +++ b/internal/verify/policies_test.go @@ -2,7 +2,7 @@ package verify import "testing" -func TestPolicyToSet(t *testing.T) { +func TestNewPolicyUnlessEquivalent(t *testing.T) { testCases := []struct { name string oldPolicy string @@ -58,14 +58,14 @@ func TestPolicyToSet(t *testing.T) { ] }, "Action": [ - "kms:CreateKey", "kms:DescribeKey", - "kms:ScheduleKeyDeletion", - "kms:Describe*", - "kms:Get*", "kms:List*", "kms:TagResource", - "kms:UntagResource" + "kms:UntagResource", + "kms:CreateKey", + "kms:Get*", + "kms:ScheduleKeyDeletion", + "kms:Describe*" ], "Resource": "*" } @@ -152,10 +152,10 @@ func TestPolicyToSet(t *testing.T) { "Action": [ "kms:CreateKey", "kms:DescribeKey", - "kms:ScheduleKeyDeletion", "kms:Describe*", - "kms:Get*", "kms:List*", + "kms:ScheduleKeyDeletion", + "kms:Get*", "kms:TagResource", "kms:UntagResource" ], @@ -181,10 +181,10 @@ func TestPolicyToSet(t *testing.T) { "Action": [ "kms:CreateKey", "kms:DescribeKey", - "kms:ScheduleKeyDeletion", "kms:Describe*", - "kms:Get*", "kms:List*", + "kms:ScheduleKeyDeletion", + "kms:Get*", "kms:TagResource", "kms:UntagResource" ], From 2ac6492a853d2aeabfb678e215c405b81258ed2d Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Tue, 30 Nov 2021 17:01:05 -0500 Subject: [PATCH 8/9] Align the stars --- internal/service/kms/key_test.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/internal/service/kms/key_test.go b/internal/service/kms/key_test.go index 98090628a9e..a6aa15814fb 100644 --- a/internal/service/kms/key_test.go +++ b/internal/service/kms/key_test.go @@ -539,10 +539,10 @@ resource "aws_kms_key" "test" { deletion_window_in_days = 7 policy = jsonencode({ - Id = %[1]q + Id = %[1]q Statement = [{ - Sid = "Enable IAM User Permissions" - Effect = "Allow" + Sid = "Enable IAM User Permissions" + Effect = "Allow" Principal = { AWS = "*" } @@ -566,7 +566,7 @@ resource "aws_kms_key" "test" { bypass_policy_lockout_safety_check = %[2]t policy = jsonencode({ - Id = %[1]q + Id = %[1]q Statement = [ { Action = [ @@ -579,11 +579,10 @@ resource "aws_kms_key" "test" { "kms:TagResource", "kms:UntagResource", ] - Effect = "Allow" + Effect = "Allow" Principal = { AWS = data.aws_caller_identity.current.arn } - Resource = "*" Sid = "Enable IAM User Permissions" }, From e7ede031182d641810147a17416714f0ae8af491 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Wed, 1 Dec 2021 09:05:43 -0500 Subject: [PATCH 9/9] Housekeep JSON in verify --- internal/service/kms/key.go | 2 +- internal/verify/diff.go | 47 +-- internal/verify/diff_test.go | 220 ------------- internal/verify/json.go | 77 ++++- internal/verify/json_test.go | 550 ++++++++++++++++++++++++++++++- internal/verify/policies.go | 25 -- internal/verify/policies_test.go | 305 ----------------- internal/verify/validate.go | 2 +- internal/verify/verify.go | 9 - internal/verify/verify_test.go | 24 -- 10 files changed, 628 insertions(+), 633 deletions(-) delete mode 100644 internal/verify/policies.go delete mode 100644 internal/verify/policies_test.go diff --git a/internal/service/kms/key.go b/internal/service/kms/key.go index c5612a6d62a..232b36966d4 100644 --- a/internal/service/kms/key.go +++ b/internal/service/kms/key.go @@ -200,7 +200,7 @@ func resourceKeyRead(d *schema.ResourceData, meta interface{}) error { d.Set("key_usage", key.metadata.KeyUsage) d.Set("multi_region", key.metadata.MultiRegion) - policyToSet, err := verify.NewPolicyUnlessEquivalent(d.Get("policy").(string), key.policy) + policyToSet, err := verify.SecondJSONUnlessEquivalent(d.Get("policy").(string), key.policy) if err != nil { return fmt.Errorf("while setting policy (%s), encountered: %w", key.policy, err) diff --git a/internal/verify/diff.go b/internal/verify/diff.go index 93ae8e1f09e..b9085768184 100644 --- a/internal/verify/diff.go +++ b/internal/verify/diff.go @@ -1,19 +1,17 @@ package verify import ( - "bytes" "context" - "encoding/json" "fmt" - "log" "github.com/aws/aws-sdk-go/aws" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-provider-aws/internal/conns" tftags "github.com/hashicorp/terraform-provider-aws/internal/tags" - awspolicy "github.com/jen20/awspolicyequivalence" ) +// Find JSON diff functions in the json.go file. + // SetTagsDiff sets the new plan difference with the result of // merging resource tags on to those defined at the provider-level; // returns an error if unsuccessful or if the resource tags are identical @@ -55,15 +53,6 @@ func SetTagsDiff(_ context.Context, diff *schema.ResourceDiff, meta interface{}) return nil } -func SuppressEquivalentPolicyDiffs(k, old, new string, d *schema.ResourceData) bool { - equivalent, err := awspolicy.PoliciesAreEquivalent(old, new) - if err != nil { - return false - } - - return equivalent -} - // SuppressEquivalentTypeStringBoolean provides custom difference suppression for TypeString booleans // Some arguments require three values: true, false, and "" (unspecified), but // confusing behavior exists when converting bare true/false values with state. @@ -85,38 +74,6 @@ func SuppressMissingOptionalConfigurationBlock(k, old, new string, d *schema.Res return old == "1" && new == "0" } -func SuppressEquivalentJSONDiffs(k, old, new string, d *schema.ResourceData) bool { - ob := bytes.NewBufferString("") - if err := json.Compact(ob, []byte(old)); err != nil { - return false - } - - nb := bytes.NewBufferString("") - if err := json.Compact(nb, []byte(new)); err != nil { - return false - } - - return JSONBytesEqual(ob.Bytes(), nb.Bytes()) -} - -func SuppressEquivalentJSONOrYAMLDiffs(k, old, new string, d *schema.ResourceData) bool { - normalizedOld, err := NormalizeJSONOrYAMLString(old) - - if err != nil { - log.Printf("[WARN] Unable to normalize Terraform state CloudFormation template body: %s", err) - return false - } - - normalizedNew, err := NormalizeJSONOrYAMLString(new) - - if err != nil { - log.Printf("[WARN] Unable to normalize Terraform configuration CloudFormation template body: %s", err) - return false - } - - return normalizedOld == normalizedNew -} - // DiffStringMaps returns the set of keys and values that must be created, the set of keys // and values that must be destroyed, and the set of keys and values that are unchanged. func DiffStringMaps(oldMap, newMap map[string]interface{}) (map[string]*string, map[string]*string, map[string]*string) { diff --git a/internal/verify/diff_test.go b/internal/verify/diff_test.go index 466d0c201ee..0ae32f231dc 100644 --- a/internal/verify/diff_test.go +++ b/internal/verify/diff_test.go @@ -3,34 +3,8 @@ package verify import ( "reflect" "testing" - - "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" ) -func TestSuppressEquivalentJSONDiffsWhitespaceAndNoWhitespace(t *testing.T) { - d := new(schema.ResourceData) - - noWhitespace := `{"test":"test"}` - whitespace := ` -{ - "test": "test" -}` - - if !SuppressEquivalentJSONDiffs("", noWhitespace, whitespace, d) { - t.Errorf("Expected SuppressEquivalentJSONDiffs to return true for %s == %s", noWhitespace, whitespace) - } - - noWhitespaceDiff := `{"test":"test"}` - whitespaceDiff := ` -{ - "test": "tested" -}` - - if SuppressEquivalentJSONDiffs("", noWhitespaceDiff, whitespaceDiff, d) { - t.Errorf("Expected SuppressEquivalentJSONDiffs to return false for %s == %s", noWhitespaceDiff, whitespaceDiff) - } -} - func TestSuppressEquivalentTypeStringBoolean(t *testing.T) { testCases := []struct { old string @@ -72,200 +46,6 @@ func TestSuppressEquivalentTypeStringBoolean(t *testing.T) { } } -func TestSuppressEquivalentJSONOrYAMLDiffs(t *testing.T) { - testCases := []struct { - description string - equivalent bool - old string - new string - }{ - { - description: `JSON no change`, - equivalent: true, - old: ` -{ - "Resources": { - "TestVpc": { - "Type": "AWS::EC2::VPC", - "Properties": { - "CidrBlock": "10.0.0.0/16" - } - } - }, - "Outputs": { - "TestVpcID": { - "Value": { "Ref" : "TestVpc" } - } - } -} -`, - new: ` -{ - "Resources": { - "TestVpc": { - "Type": "AWS::EC2::VPC", - "Properties": { - "CidrBlock": "10.0.0.0/16" - } - } - }, - "Outputs": { - "TestVpcID": { - "Value": { "Ref" : "TestVpc" } - } - } -} -`, - }, - { - description: `JSON whitespace`, - equivalent: true, - old: `{"Resources":{"TestVpc":{"Type":"AWS::EC2::VPC","Properties":{"CidrBlock":"10.0.0.0/16"}}},"Outputs":{"TestVpcID":{"Value":{"Ref":"TestVpc"}}}}`, - new: ` -{ - "Resources": { - "TestVpc": { - "Type": "AWS::EC2::VPC", - "Properties": { - "CidrBlock": "10.0.0.0/16" - } - } - }, - "Outputs": { - "TestVpcID": { - "Value": { "Ref" : "TestVpc" } - } - } -} -`, - }, - { - description: `JSON change`, - equivalent: false, - old: ` -{ - "Resources": { - "TestVpc": { - "Type": "AWS::EC2::VPC", - "Properties": { - "CidrBlock": "10.0.0.0/16" - } - } - }, - "Outputs": { - "TestVpcID": { - "Value": { "Ref" : "TestVpc" } - } - } -} -`, - new: ` -{ - "Resources": { - "TestVpc": { - "Type": "AWS::EC2::VPC", - "Properties": { - "CidrBlock": "172.16.0.0/16" - } - } - }, - "Outputs": { - "TestVpcID": { - "Value": { "Ref" : "TestVpc" } - } - } -} -`, - }, - { - description: `YAML no change`, - equivalent: true, - old: ` -Resources: - TestVpc: - Type: AWS::EC2::VPC - Properties: - CidrBlock: 10.0.0.0/16 -Outputs: - TestVpcID: - Value: !Ref TestVpc -`, - new: ` -Resources: - TestVpc: - Type: AWS::EC2::VPC - Properties: - CidrBlock: 10.0.0.0/16 -Outputs: - TestVpcID: - Value: !Ref TestVpc -`, - }, - { - description: `YAML whitespace`, - equivalent: false, - old: ` -Resources: - TestVpc: - Type: AWS::EC2::VPC - Properties: - CidrBlock: 10.0.0.0/16 - -Outputs: - TestVpcID: - Value: !Ref TestVpc - -`, - new: ` -Resources: - TestVpc: - Type: AWS::EC2::VPC - Properties: - CidrBlock: 10.0.0.0/16 -Outputs: - TestVpcID: - Value: !Ref TestVpc -`, - }, - { - description: `YAML change`, - equivalent: false, - old: ` -Resources: - TestVpc: - Type: AWS::EC2::VPC - Properties: - CidrBlock: 172.16.0.0/16 -Outputs: - TestVpcID: - Value: !Ref TestVpc -`, - new: ` -Resources: - TestVpc: - Type: AWS::EC2::VPC - Properties: - CidrBlock: 10.0.0.0/16 -Outputs: - TestVpcID: - Value: !Ref TestVpc -`, - }, - } - - for _, tc := range testCases { - value := SuppressEquivalentJSONOrYAMLDiffs("test_property", tc.old, tc.new, nil) - - if tc.equivalent && !value { - t.Fatalf("expected test case (%s) to be equivalent", tc.description) - } - - if !tc.equivalent && value { - t.Fatalf("expected test case (%s) to not be equivalent", tc.description) - } - } -} - func TestDiffStringMaps(t *testing.T) { cases := []struct { Old, New map[string]interface{} diff --git a/internal/verify/json.go b/internal/verify/json.go index 9980b39fda9..9dda71dafe0 100644 --- a/internal/verify/json.go +++ b/internal/verify/json.go @@ -1,12 +1,67 @@ package verify import ( + "bytes" "encoding/json" + "log" "reflect" "regexp" + + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/structure" + awspolicy "github.com/jen20/awspolicyequivalence" ) -func looksLikeJsonString(s interface{}) bool { +func SuppressEquivalentPolicyDiffs(k, old, new string, d *schema.ResourceData) bool { + equivalent, err := awspolicy.PoliciesAreEquivalent(old, new) + if err != nil { + return false + } + + return equivalent +} + +func SuppressEquivalentJSONDiffs(k, old, new string, d *schema.ResourceData) bool { + ob := bytes.NewBufferString("") + if err := json.Compact(ob, []byte(old)); err != nil { + return false + } + + nb := bytes.NewBufferString("") + if err := json.Compact(nb, []byte(new)); err != nil { + return false + } + + return JSONBytesEqual(ob.Bytes(), nb.Bytes()) +} + +func SuppressEquivalentJSONOrYAMLDiffs(k, old, new string, d *schema.ResourceData) bool { + normalizedOld, err := NormalizeJSONOrYAMLString(old) + + if err != nil { + log.Printf("[WARN] Unable to normalize Terraform state CloudFormation template body: %s", err) + return false + } + + normalizedNew, err := NormalizeJSONOrYAMLString(new) + + if err != nil { + log.Printf("[WARN] Unable to normalize Terraform configuration CloudFormation template body: %s", err) + return false + } + + return normalizedOld == normalizedNew +} + +func NormalizeJSONOrYAMLString(templateString interface{}) (string, error) { + if looksLikeJSONString(templateString) { + return structure.NormalizeJsonString(templateString.(string)) + } + + return checkYAMLString(templateString) +} + +func looksLikeJSONString(s interface{}) bool { return regexp.MustCompile(`^\s*{`).MatchString(s.(string)) } @@ -23,3 +78,23 @@ func JSONBytesEqual(b1, b2 []byte) bool { return reflect.DeepEqual(o1, o2) } + +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 + if old == "" || new == "" { + return new, nil + } + + equivalent, err := awspolicy.PoliciesAreEquivalent(old, new) + + if err != nil { + return "", err + } + + if equivalent { + return old, nil + } + + return new, nil +} diff --git a/internal/verify/json_test.go b/internal/verify/json_test.go index 5f11bbbf209..a8f5c3851c3 100644 --- a/internal/verify/json_test.go +++ b/internal/verify/json_test.go @@ -2,16 +2,18 @@ package verify import ( "testing" + + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" ) func TestLooksLikeJsonString(t *testing.T) { looksLikeJson := ` {"abc":"1"} ` doesNotLookLikeJson := `abc: 1` - if !looksLikeJsonString(looksLikeJson) { + if !looksLikeJSONString(looksLikeJson) { t.Errorf("Expected looksLikeJson to return true for %s", looksLikeJson) } - if looksLikeJsonString(doesNotLookLikeJson) { + if looksLikeJSONString(doesNotLookLikeJson) { t.Errorf("Expected looksLikeJson to return false for %s", doesNotLookLikeJson) } } @@ -53,3 +55,547 @@ func TestJsonBytesEqualWhitespaceAndNoWhitespace(t *testing.T) { t.Errorf("Expected JSONBytesEqual to return false for %s == %s", noWhitespaceDiff, whitespaceDiff) } } + +func TestSecondJSONUnlessEquivalent(t *testing.T) { + testCases := []struct { + name string + oldPolicy string + newPolicy string + want string + }{ + { + name: "new in random order", + oldPolicy: `{ + "Version": "2012-10-17", + "Id": "kms-tf-1", + "Statement": [ + { + "Sid": "Enable IAM User Permissions", + "Effect": "Allow", + "Principal": { + "AWS": [ + "arn:aws:iam::012345678901:role/felixjaehn", + "arn:aws:iam::012345678901:role/garethemery", + "arn:aws:iam::012345678901:role/kidnap", + "arn:aws:iam::012345678901:role/paulvandyk", + "arn:aws:iam::012345678901:role/tinlicker" + ] + }, + "Action": [ + "kms:CreateKey", + "kms:DescribeKey", + "kms:ScheduleKeyDeletion", + "kms:Describe*", + "kms:Get*", + "kms:List*", + "kms:TagResource", + "kms:UntagResource" + ], + "Resource": "*" + } + ] +}`, + newPolicy: `{ + "Version": "2012-10-17", + "Id": "kms-tf-1", + "Statement": [ + { + "Sid": "Enable IAM User Permissions", + "Effect": "Allow", + "Principal": { + "AWS": [ + "arn:aws:iam::012345678901:role/tinlicker", + "arn:aws:iam::012345678901:role/paulvandyk", + "arn:aws:iam::012345678901:role/kidnap", + "arn:aws:iam::012345678901:role/garethemery", + "arn:aws:iam::012345678901:role/felixjaehn" + ] + }, + "Action": [ + "kms:DescribeKey", + "kms:List*", + "kms:TagResource", + "kms:UntagResource", + "kms:CreateKey", + "kms:Get*", + "kms:ScheduleKeyDeletion", + "kms:Describe*" + ], + "Resource": "*" + } + ] +}`, + want: `{ + "Version": "2012-10-17", + "Id": "kms-tf-1", + "Statement": [ + { + "Sid": "Enable IAM User Permissions", + "Effect": "Allow", + "Principal": { + "AWS": [ + "arn:aws:iam::012345678901:role/felixjaehn", + "arn:aws:iam::012345678901:role/garethemery", + "arn:aws:iam::012345678901:role/kidnap", + "arn:aws:iam::012345678901:role/paulvandyk", + "arn:aws:iam::012345678901:role/tinlicker" + ] + }, + "Action": [ + "kms:CreateKey", + "kms:DescribeKey", + "kms:ScheduleKeyDeletion", + "kms:Describe*", + "kms:Get*", + "kms:List*", + "kms:TagResource", + "kms:UntagResource" + ], + "Resource": "*" + } + ] +}`, + }, + { + name: "actual change", + oldPolicy: `{ + "Version": "2012-10-17", + "Id": "kms-tf-1", + "Statement": [ + { + "Sid": "Enable IAM User Permissions", + "Effect": "Allow", + "Principal": { + "AWS": [ + "arn:aws:iam::012345678901:role/felixjaehn", + "arn:aws:iam::012345678901:role/garethemery", + "arn:aws:iam::012345678901:role/kidnap", + "arn:aws:iam::012345678901:role/paulvandyk", + "arn:aws:iam::012345678901:role/tinlicker" + ] + }, + "Action": [ + "kms:CreateKey", + "kms:DescribeKey", + "kms:ScheduleKeyDeletion", + "kms:Describe*", + "kms:Get*", + "kms:List*", + "kms:TagResource", + "kms:UntagResource" + ], + "Resource": "*" + } + ] +}`, + newPolicy: `{ + "Version": "2012-10-17", + "Id": "kms-tf-1", + "Statement": [ + { + "Sid": "Enable IAM User Permissions", + "Effect": "Allow", + "Principal": { + "AWS": [ + "arn:aws:iam::012345678901:role/tinlicker", + "arn:aws:iam::012345678901:role/paulvandyk", + "arn:aws:iam::012345678901:role/garethemery", + "arn:aws:iam::012345678901:role/felixjaehn" + ] + }, + "Action": [ + "kms:CreateKey", + "kms:DescribeKey", + "kms:Describe*", + "kms:List*", + "kms:ScheduleKeyDeletion", + "kms:Get*", + "kms:TagResource", + "kms:UntagResource" + ], + "Resource": "*" + } + ] +}`, + want: `{ + "Version": "2012-10-17", + "Id": "kms-tf-1", + "Statement": [ + { + "Sid": "Enable IAM User Permissions", + "Effect": "Allow", + "Principal": { + "AWS": [ + "arn:aws:iam::012345678901:role/tinlicker", + "arn:aws:iam::012345678901:role/paulvandyk", + "arn:aws:iam::012345678901:role/garethemery", + "arn:aws:iam::012345678901:role/felixjaehn" + ] + }, + "Action": [ + "kms:CreateKey", + "kms:DescribeKey", + "kms:Describe*", + "kms:List*", + "kms:ScheduleKeyDeletion", + "kms:Get*", + "kms:TagResource", + "kms:UntagResource" + ], + "Resource": "*" + } + ] +}`, + }, + { + name: "empty old", + oldPolicy: "", + newPolicy: `{ + "Version": "2012-10-17", + "Id": "kms-tf-1", + "Statement": [ + { + "Sid": "Enable IAM User Permissions", + "Effect": "Allow", + "Principal": { + "AWS": [ + "arn:aws:iam::012345678901:role/tinlicker", + "arn:aws:iam::012345678901:role/paulvandyk", + "arn:aws:iam::012345678901:role/garethemery", + "arn:aws:iam::012345678901:role/felixjaehn" + ] + }, + "Action": [ + "kms:CreateKey", + "kms:DescribeKey", + "kms:ScheduleKeyDeletion", + "kms:Describe*", + "kms:Get*", + "kms:List*", + "kms:TagResource", + "kms:UntagResource" + ], + "Resource": "*" + } + ] +}`, + want: `{ + "Version": "2012-10-17", + "Id": "kms-tf-1", + "Statement": [ + { + "Sid": "Enable IAM User Permissions", + "Effect": "Allow", + "Principal": { + "AWS": [ + "arn:aws:iam::012345678901:role/tinlicker", + "arn:aws:iam::012345678901:role/paulvandyk", + "arn:aws:iam::012345678901:role/garethemery", + "arn:aws:iam::012345678901:role/felixjaehn" + ] + }, + "Action": [ + "kms:CreateKey", + "kms:DescribeKey", + "kms:ScheduleKeyDeletion", + "kms:Describe*", + "kms:Get*", + "kms:List*", + "kms:TagResource", + "kms:UntagResource" + ], + "Resource": "*" + } + ] +}`, + }, + { + name: "empty new", + oldPolicy: `{ + "Version": "2012-10-17", + "Id": "kms-tf-1", + "Statement": [ + { + "Sid": "Enable IAM User Permissions", + "Effect": "Allow", + "Principal": { + "AWS": [ + "arn:aws:iam::012345678901:role/tinlicker", + "arn:aws:iam::012345678901:role/paulvandyk", + "arn:aws:iam::012345678901:role/garethemery", + "arn:aws:iam::012345678901:role/felixjaehn" + ] + }, + "Action": [ + "kms:CreateKey", + "kms:DescribeKey", + "kms:ScheduleKeyDeletion", + "kms:Describe*", + "kms:Get*", + "kms:List*", + "kms:TagResource", + "kms:UntagResource" + ], + "Resource": "*" + } + ] +}`, + newPolicy: "", + want: "", + }, + } + + for _, v := range testCases { + got, err := SecondJSONUnlessEquivalent(v.oldPolicy, v.newPolicy) + + if err != nil { + t.Fatalf("unexpected error with test case %s: %s", v.name, err) + } + + if got != v.want { + t.Fatalf("for test case %s, got %s, wanted %s", v.name, got, v.want) + } + } +} + +func TestNormalizeJSONOrYAMLString(t *testing.T) { + var err error + var actual string + + validNormalizedJson := `{"abc":"1"}` + actual, err = NormalizeJSONOrYAMLString(validNormalizedJson) + if err != nil { + t.Fatalf("Expected not to throw an error while parsing template, but got: %s", err) + } + if actual != validNormalizedJson { + t.Fatalf("Got:\n\n%s\n\nExpected:\n\n%s\n", actual, validNormalizedJson) + } + + validNormalizedYaml := `abc: 1 +` + actual, err = NormalizeJSONOrYAMLString(validNormalizedYaml) + if err != nil { + t.Fatalf("Expected not to throw an error while parsing template, but got: %s", err) + } + if actual != validNormalizedYaml { + t.Fatalf("Got:\n\n%s\n\nExpected:\n\n%s\n", actual, validNormalizedYaml) + } +} + +func TestSuppressEquivalentJSONDiffsWhitespaceAndNoWhitespace(t *testing.T) { + d := new(schema.ResourceData) + + noWhitespace := `{"test":"test"}` + whitespace := ` +{ + "test": "test" +}` + + if !SuppressEquivalentJSONDiffs("", noWhitespace, whitespace, d) { + t.Errorf("Expected SuppressEquivalentJSONDiffs to return true for %s == %s", noWhitespace, whitespace) + } + + noWhitespaceDiff := `{"test":"test"}` + whitespaceDiff := ` +{ + "test": "tested" +}` + + if SuppressEquivalentJSONDiffs("", noWhitespaceDiff, whitespaceDiff, d) { + t.Errorf("Expected SuppressEquivalentJSONDiffs to return false for %s == %s", noWhitespaceDiff, whitespaceDiff) + } +} + +func TestSuppressEquivalentJSONOrYAMLDiffs(t *testing.T) { + testCases := []struct { + description string + equivalent bool + old string + new string + }{ + { + description: `JSON no change`, + equivalent: true, + old: ` +{ + "Resources": { + "TestVpc": { + "Type": "AWS::EC2::VPC", + "Properties": { + "CidrBlock": "10.0.0.0/16" + } + } + }, + "Outputs": { + "TestVpcID": { + "Value": { "Ref" : "TestVpc" } + } + } +} +`, + new: ` +{ + "Resources": { + "TestVpc": { + "Type": "AWS::EC2::VPC", + "Properties": { + "CidrBlock": "10.0.0.0/16" + } + } + }, + "Outputs": { + "TestVpcID": { + "Value": { "Ref" : "TestVpc" } + } + } +} +`, + }, + { + description: `JSON whitespace`, + equivalent: true, + old: `{"Resources":{"TestVpc":{"Type":"AWS::EC2::VPC","Properties":{"CidrBlock":"10.0.0.0/16"}}},"Outputs":{"TestVpcID":{"Value":{"Ref":"TestVpc"}}}}`, + new: ` +{ + "Resources": { + "TestVpc": { + "Type": "AWS::EC2::VPC", + "Properties": { + "CidrBlock": "10.0.0.0/16" + } + } + }, + "Outputs": { + "TestVpcID": { + "Value": { "Ref" : "TestVpc" } + } + } +} +`, + }, + { + description: `JSON change`, + equivalent: false, + old: ` +{ + "Resources": { + "TestVpc": { + "Type": "AWS::EC2::VPC", + "Properties": { + "CidrBlock": "10.0.0.0/16" + } + } + }, + "Outputs": { + "TestVpcID": { + "Value": { "Ref" : "TestVpc" } + } + } +} +`, + new: ` +{ + "Resources": { + "TestVpc": { + "Type": "AWS::EC2::VPC", + "Properties": { + "CidrBlock": "172.16.0.0/16" + } + } + }, + "Outputs": { + "TestVpcID": { + "Value": { "Ref" : "TestVpc" } + } + } +} +`, + }, + { + description: `YAML no change`, + equivalent: true, + old: ` +Resources: + TestVpc: + Type: AWS::EC2::VPC + Properties: + CidrBlock: 10.0.0.0/16 +Outputs: + TestVpcID: + Value: !Ref TestVpc +`, + new: ` +Resources: + TestVpc: + Type: AWS::EC2::VPC + Properties: + CidrBlock: 10.0.0.0/16 +Outputs: + TestVpcID: + Value: !Ref TestVpc +`, + }, + { + description: `YAML whitespace`, + equivalent: false, + old: ` +Resources: + TestVpc: + Type: AWS::EC2::VPC + Properties: + CidrBlock: 10.0.0.0/16 + +Outputs: + TestVpcID: + Value: !Ref TestVpc + +`, + new: ` +Resources: + TestVpc: + Type: AWS::EC2::VPC + Properties: + CidrBlock: 10.0.0.0/16 +Outputs: + TestVpcID: + Value: !Ref TestVpc +`, + }, + { + description: `YAML change`, + equivalent: false, + old: ` +Resources: + TestVpc: + Type: AWS::EC2::VPC + Properties: + CidrBlock: 172.16.0.0/16 +Outputs: + TestVpcID: + Value: !Ref TestVpc +`, + new: ` +Resources: + TestVpc: + Type: AWS::EC2::VPC + Properties: + CidrBlock: 10.0.0.0/16 +Outputs: + TestVpcID: + Value: !Ref TestVpc +`, + }, + } + + for _, tc := range testCases { + value := SuppressEquivalentJSONOrYAMLDiffs("test_property", tc.old, tc.new, nil) + + if tc.equivalent && !value { + t.Fatalf("expected test case (%s) to be equivalent", tc.description) + } + + if !tc.equivalent && value { + t.Fatalf("expected test case (%s) to not be equivalent", tc.description) + } + } +} diff --git a/internal/verify/policies.go b/internal/verify/policies.go deleted file mode 100644 index b73ab9482ae..00000000000 --- a/internal/verify/policies.go +++ /dev/null @@ -1,25 +0,0 @@ -package verify - -import ( - awspolicy "github.com/jen20/awspolicyequivalence" -) - -func NewPolicyUnlessEquivalent(old, new string) (string, error) { - // valid empty JSON is "{}" not "" so handle special case to avoid - // Error unmarshaling policy: unexpected end of JSON input - if old == "" || new == "" { - return new, nil - } - - equivalent, err := awspolicy.PoliciesAreEquivalent(old, new) - - if err != nil { - return "", err - } - - if equivalent { - return old, nil - } - - return new, nil -} diff --git a/internal/verify/policies_test.go b/internal/verify/policies_test.go deleted file mode 100644 index 583086acd0a..00000000000 --- a/internal/verify/policies_test.go +++ /dev/null @@ -1,305 +0,0 @@ -package verify - -import "testing" - -func TestNewPolicyUnlessEquivalent(t *testing.T) { - testCases := []struct { - name string - oldPolicy string - newPolicy string - want string - }{ - { - name: "new in random order", - oldPolicy: `{ - "Version": "2012-10-17", - "Id": "kms-tf-1", - "Statement": [ - { - "Sid": "Enable IAM User Permissions", - "Effect": "Allow", - "Principal": { - "AWS": [ - "arn:aws:iam::012345678901:role/felixjaehn", - "arn:aws:iam::012345678901:role/garethemery", - "arn:aws:iam::012345678901:role/kidnap", - "arn:aws:iam::012345678901:role/paulvandyk", - "arn:aws:iam::012345678901:role/tinlicker" - ] - }, - "Action": [ - "kms:CreateKey", - "kms:DescribeKey", - "kms:ScheduleKeyDeletion", - "kms:Describe*", - "kms:Get*", - "kms:List*", - "kms:TagResource", - "kms:UntagResource" - ], - "Resource": "*" - } - ] -}`, - newPolicy: `{ - "Version": "2012-10-17", - "Id": "kms-tf-1", - "Statement": [ - { - "Sid": "Enable IAM User Permissions", - "Effect": "Allow", - "Principal": { - "AWS": [ - "arn:aws:iam::012345678901:role/tinlicker", - "arn:aws:iam::012345678901:role/paulvandyk", - "arn:aws:iam::012345678901:role/kidnap", - "arn:aws:iam::012345678901:role/garethemery", - "arn:aws:iam::012345678901:role/felixjaehn" - ] - }, - "Action": [ - "kms:DescribeKey", - "kms:List*", - "kms:TagResource", - "kms:UntagResource", - "kms:CreateKey", - "kms:Get*", - "kms:ScheduleKeyDeletion", - "kms:Describe*" - ], - "Resource": "*" - } - ] -}`, - want: `{ - "Version": "2012-10-17", - "Id": "kms-tf-1", - "Statement": [ - { - "Sid": "Enable IAM User Permissions", - "Effect": "Allow", - "Principal": { - "AWS": [ - "arn:aws:iam::012345678901:role/felixjaehn", - "arn:aws:iam::012345678901:role/garethemery", - "arn:aws:iam::012345678901:role/kidnap", - "arn:aws:iam::012345678901:role/paulvandyk", - "arn:aws:iam::012345678901:role/tinlicker" - ] - }, - "Action": [ - "kms:CreateKey", - "kms:DescribeKey", - "kms:ScheduleKeyDeletion", - "kms:Describe*", - "kms:Get*", - "kms:List*", - "kms:TagResource", - "kms:UntagResource" - ], - "Resource": "*" - } - ] -}`, - }, - { - name: "actual change", - oldPolicy: `{ - "Version": "2012-10-17", - "Id": "kms-tf-1", - "Statement": [ - { - "Sid": "Enable IAM User Permissions", - "Effect": "Allow", - "Principal": { - "AWS": [ - "arn:aws:iam::012345678901:role/felixjaehn", - "arn:aws:iam::012345678901:role/garethemery", - "arn:aws:iam::012345678901:role/kidnap", - "arn:aws:iam::012345678901:role/paulvandyk", - "arn:aws:iam::012345678901:role/tinlicker" - ] - }, - "Action": [ - "kms:CreateKey", - "kms:DescribeKey", - "kms:ScheduleKeyDeletion", - "kms:Describe*", - "kms:Get*", - "kms:List*", - "kms:TagResource", - "kms:UntagResource" - ], - "Resource": "*" - } - ] -}`, - newPolicy: `{ - "Version": "2012-10-17", - "Id": "kms-tf-1", - "Statement": [ - { - "Sid": "Enable IAM User Permissions", - "Effect": "Allow", - "Principal": { - "AWS": [ - "arn:aws:iam::012345678901:role/tinlicker", - "arn:aws:iam::012345678901:role/paulvandyk", - "arn:aws:iam::012345678901:role/garethemery", - "arn:aws:iam::012345678901:role/felixjaehn" - ] - }, - "Action": [ - "kms:CreateKey", - "kms:DescribeKey", - "kms:Describe*", - "kms:List*", - "kms:ScheduleKeyDeletion", - "kms:Get*", - "kms:TagResource", - "kms:UntagResource" - ], - "Resource": "*" - } - ] -}`, - want: `{ - "Version": "2012-10-17", - "Id": "kms-tf-1", - "Statement": [ - { - "Sid": "Enable IAM User Permissions", - "Effect": "Allow", - "Principal": { - "AWS": [ - "arn:aws:iam::012345678901:role/tinlicker", - "arn:aws:iam::012345678901:role/paulvandyk", - "arn:aws:iam::012345678901:role/garethemery", - "arn:aws:iam::012345678901:role/felixjaehn" - ] - }, - "Action": [ - "kms:CreateKey", - "kms:DescribeKey", - "kms:Describe*", - "kms:List*", - "kms:ScheduleKeyDeletion", - "kms:Get*", - "kms:TagResource", - "kms:UntagResource" - ], - "Resource": "*" - } - ] -}`, - }, - { - name: "empty old", - oldPolicy: "", - newPolicy: `{ - "Version": "2012-10-17", - "Id": "kms-tf-1", - "Statement": [ - { - "Sid": "Enable IAM User Permissions", - "Effect": "Allow", - "Principal": { - "AWS": [ - "arn:aws:iam::012345678901:role/tinlicker", - "arn:aws:iam::012345678901:role/paulvandyk", - "arn:aws:iam::012345678901:role/garethemery", - "arn:aws:iam::012345678901:role/felixjaehn" - ] - }, - "Action": [ - "kms:CreateKey", - "kms:DescribeKey", - "kms:ScheduleKeyDeletion", - "kms:Describe*", - "kms:Get*", - "kms:List*", - "kms:TagResource", - "kms:UntagResource" - ], - "Resource": "*" - } - ] -}`, - want: `{ - "Version": "2012-10-17", - "Id": "kms-tf-1", - "Statement": [ - { - "Sid": "Enable IAM User Permissions", - "Effect": "Allow", - "Principal": { - "AWS": [ - "arn:aws:iam::012345678901:role/tinlicker", - "arn:aws:iam::012345678901:role/paulvandyk", - "arn:aws:iam::012345678901:role/garethemery", - "arn:aws:iam::012345678901:role/felixjaehn" - ] - }, - "Action": [ - "kms:CreateKey", - "kms:DescribeKey", - "kms:ScheduleKeyDeletion", - "kms:Describe*", - "kms:Get*", - "kms:List*", - "kms:TagResource", - "kms:UntagResource" - ], - "Resource": "*" - } - ] -}`, - }, - { - name: "empty new", - oldPolicy: `{ - "Version": "2012-10-17", - "Id": "kms-tf-1", - "Statement": [ - { - "Sid": "Enable IAM User Permissions", - "Effect": "Allow", - "Principal": { - "AWS": [ - "arn:aws:iam::012345678901:role/tinlicker", - "arn:aws:iam::012345678901:role/paulvandyk", - "arn:aws:iam::012345678901:role/garethemery", - "arn:aws:iam::012345678901:role/felixjaehn" - ] - }, - "Action": [ - "kms:CreateKey", - "kms:DescribeKey", - "kms:ScheduleKeyDeletion", - "kms:Describe*", - "kms:Get*", - "kms:List*", - "kms:TagResource", - "kms:UntagResource" - ], - "Resource": "*" - } - ] -}`, - newPolicy: "", - want: "", - }, - } - - for _, v := range testCases { - got, err := NewPolicyUnlessEquivalent(v.oldPolicy, v.newPolicy) - - if err != nil { - t.Fatalf("unexpected error with test case %s: %s", v.name, err) - } - - if got != v.want { - t.Fatalf("for test case %s, got %s, wanted %s", v.name, got, v.want) - } - } -} diff --git a/internal/verify/validate.go b/internal/verify/validate.go index 8d9d01b16b9..2b3ecb552b4 100644 --- a/internal/verify/validate.go +++ b/internal/verify/validate.go @@ -230,7 +230,7 @@ func ValidOnceAWeekWindowFormat(v interface{}, k string) (ws []string, errors [] } func ValidStringIsJSONOrYAML(v interface{}, k string) (ws []string, errors []error) { - if looksLikeJsonString(v) { + if looksLikeJSONString(v) { if _, err := structure.NormalizeJsonString(v); err != nil { errors = append(errors, fmt.Errorf("%q contains an invalid JSON: %s", k, err)) } diff --git a/internal/verify/verify.go b/internal/verify/verify.go index c45f6155dac..3e10b39f4b3 100644 --- a/internal/verify/verify.go +++ b/internal/verify/verify.go @@ -1,7 +1,6 @@ package verify import ( - "github.com/hashicorp/terraform-plugin-sdk/v2/helper/structure" "gopkg.in/yaml.v2" ) @@ -17,14 +16,6 @@ func SliceContainsString(slice []interface{}, s string) (int, bool) { return -1, false } -func NormalizeJSONOrYAMLString(templateString interface{}) (string, error) { - if looksLikeJsonString(templateString) { - return structure.NormalizeJsonString(templateString.(string)) - } - - return checkYAMLString(templateString) -} - func PointersMapToStringList(pointers map[string]*string) map[string]interface{} { list := make(map[string]interface{}, len(pointers)) for i, v := range pointers { diff --git a/internal/verify/verify_test.go b/internal/verify/verify_test.go index 9786ea2352b..7b465209564 100644 --- a/internal/verify/verify_test.go +++ b/internal/verify/verify_test.go @@ -4,30 +4,6 @@ import ( "testing" ) -func TestNormalizeJSONOrYAMLString(t *testing.T) { - var err error - var actual string - - validNormalizedJson := `{"abc":"1"}` - actual, err = NormalizeJSONOrYAMLString(validNormalizedJson) - if err != nil { - t.Fatalf("Expected not to throw an error while parsing template, but got: %s", err) - } - if actual != validNormalizedJson { - t.Fatalf("Got:\n\n%s\n\nExpected:\n\n%s\n", actual, validNormalizedJson) - } - - validNormalizedYaml := `abc: 1 -` - actual, err = NormalizeJSONOrYAMLString(validNormalizedYaml) - if err != nil { - t.Fatalf("Expected not to throw an error while parsing template, but got: %s", err) - } - if actual != validNormalizedYaml { - t.Fatalf("Got:\n\n%s\n\nExpected:\n\n%s\n", actual, validNormalizedYaml) - } -} - func TestCheckYAMLString(t *testing.T) { var err error var actual string