From 9badf5c7ca04203368ef4f4c6561ed18348353a1 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Wed, 1 Dec 2021 10:02:27 -0500 Subject: [PATCH 1/7] Initialism caps --- internal/verify/json_test.go | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/internal/verify/json_test.go b/internal/verify/json_test.go index a8f5c3851c3..f046fe1b80e 100644 --- a/internal/verify/json_test.go +++ b/internal/verify/json_test.go @@ -6,19 +6,19 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" ) -func TestLooksLikeJsonString(t *testing.T) { - looksLikeJson := ` {"abc":"1"} ` - doesNotLookLikeJson := `abc: 1` +func TestLooksLikeJSONString(t *testing.T) { + looksLikeJSON := ` {"abc":"1"} ` + doesNotLookLikeJSON := `abc: 1` - if !looksLikeJSONString(looksLikeJson) { - t.Errorf("Expected looksLikeJson to return true for %s", looksLikeJson) + if !looksLikeJSONString(looksLikeJSON) { + t.Errorf("Expected looksLikeJSON to return true for %s", looksLikeJSON) } - if looksLikeJSONString(doesNotLookLikeJson) { - t.Errorf("Expected looksLikeJson to return false for %s", doesNotLookLikeJson) + if looksLikeJSONString(doesNotLookLikeJSON) { + t.Errorf("Expected looksLikeJSON to return false for %s", doesNotLookLikeJSON) } } -func TestJsonBytesEqualQuotedAndUnquoted(t *testing.T) { +func TestJSONBytesEqualQuotedAndUnquoted(t *testing.T) { unquoted := `{"test": "test"}` quoted := "{\"test\": \"test\"}" @@ -34,7 +34,7 @@ func TestJsonBytesEqualQuotedAndUnquoted(t *testing.T) { } } -func TestJsonBytesEqualWhitespaceAndNoWhitespace(t *testing.T) { +func TestJSONBytesEqualWhitespaceAndNoWhitespace(t *testing.T) { noWhitespace := `{"test":"test"}` whitespace := ` { @@ -362,13 +362,13 @@ func TestNormalizeJSONOrYAMLString(t *testing.T) { var err error var actual string - validNormalizedJson := `{"abc":"1"}` - actual, err = NormalizeJSONOrYAMLString(validNormalizedJson) + 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) + if actual != validNormalizedJSON { + t.Fatalf("Got:\n\n%s\n\nExpected:\n\n%s\n", actual, validNormalizedJSON) } validNormalizedYaml := `abc: 1 From 0043c4a4ff402a543904afb236bb1abcee2e8343 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Wed, 1 Dec 2021 13:48:32 -0500 Subject: [PATCH 2/7] Minor improvement for edge cases --- internal/verify/json.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/internal/verify/json.go b/internal/verify/json.go index 9dda71dafe0..8b66cc15c0b 100644 --- a/internal/verify/json.go +++ b/internal/verify/json.go @@ -6,6 +6,7 @@ import ( "log" "reflect" "regexp" + "strings" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/structure" @@ -82,7 +83,11 @@ func JSONBytesEqual(b1, b2 []byte) bool { 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 == "" { + if strings.TrimSpace(new) == "" { + return "", nil + } + + if strings.TrimSpace(old) == "" { return new, nil } From 1970d249c9a9e23cd15dd1ae991a3f0cc3c23458 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Wed, 1 Dec 2021 13:49:27 -0500 Subject: [PATCH 3/7] kms/replica_external_key: Preserve JSON order --- internal/service/kms/replica_external_key.go | 10 +++++++++- internal/service/kms/replica_external_key_test.go | 12 +++++------- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/internal/service/kms/replica_external_key.go b/internal/service/kms/replica_external_key.go index b917c9a2e9f..6ad036bbcdb 100644 --- a/internal/service/kms/replica_external_key.go +++ b/internal/service/kms/replica_external_key.go @@ -237,7 +237,15 @@ func resourceReplicaExternalKeyRead(d *schema.ResourceData, meta interface{}) er d.Set("key_id", key.metadata.KeyId) d.Set("key_state", key.metadata.KeyState) d.Set("key_usage", key.metadata.KeyUsage) - d.Set("policy", 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) + } + + d.Set("policy", policyToSet) + d.Set("primary_key_arn", key.metadata.MultiRegionConfiguration.PrimaryKey.Arn) if key.metadata.ValidTo != nil { d.Set("valid_to", aws.TimeValue(key.metadata.ValidTo).Format(time.RFC3339)) diff --git a/internal/service/kms/replica_external_key_test.go b/internal/service/kms/replica_external_key_test.go index 58ba3b293a8..91e6b812a91 100644 --- a/internal/service/kms/replica_external_key_test.go +++ b/internal/service/kms/replica_external_key_test.go @@ -61,7 +61,7 @@ func TestAccKMSReplicaExternalKey_basic(t *testing.T) { }) } -func TestAccKMSReplicaExternalKey_DescriptionAndEnabled(t *testing.T) { +func TestAccKMSReplicaExternalKey_descriptionAndEnabled(t *testing.T) { var providers []*schema.Provider var key kms.KeyMetadata rName1 := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) @@ -117,13 +117,13 @@ func TestAccKMSReplicaExternalKey_DescriptionAndEnabled(t *testing.T) { }) } -func TestAccKMSReplicaExternalKey_Policy(t *testing.T) { +func TestAccKMSReplicaExternalKey_policy(t *testing.T) { var providers []*schema.Provider var key kms.KeyMetadata rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) resourceName := "aws_kms_replica_external_key.test" - policy1 := `{"Version":"2012-10-17","Id":"kms-tf-1","Statement":[{"Sid":"Enable IAM User Permissions 1","Effect":"Allow","Principal":{"AWS":"*"},"Action":"kms:*","Resource":"*"}]}` - policy2 := `{"Version":"2012-10-17","Id":"kms-tf-1","Statement":[{"Sid":"Enable IAM User Permissions 2","Effect":"Allow","Principal":{"AWS":"*"},"Action":"kms:*","Resource":"*"}]}` + policy1 := `{"Id":"kms-tf-1","Statement":[{"Action":"kms:*","Effect":"Allow","Principal":{"AWS":"*"},"Resource":"*","Sid":"Enable IAM User Permissions 1"}],"Version":"2012-10-17"}` + policy2 := `{"Id":"kms-tf-1","Statement":[{"Action":"kms:*","Effect":"Allow","Principal":{"AWS":"*"},"Resource":"*","Sid":"Enable IAM User Permissions 2"}],"Version":"2012-10-17"}` resource.ParallelTest(t, resource.TestCase{ PreCheck: func() { @@ -290,9 +290,7 @@ resource "aws_kms_replica_external_key" "test" { bypass_policy_lockout_safety_check = %[3]t - policy = < Date: Wed, 1 Dec 2021 13:49:59 -0500 Subject: [PATCH 4/7] kms/replica_key: Preserve JSON order --- internal/service/kms/replica_key.go | 10 +++++++++- internal/service/kms/replica_key_test.go | 16 +++++++--------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/internal/service/kms/replica_key.go b/internal/service/kms/replica_key.go index 6615b7dfd56..b74c476d3c6 100644 --- a/internal/service/kms/replica_key.go +++ b/internal/service/kms/replica_key.go @@ -209,7 +209,15 @@ func resourceReplicaKeyRead(d *schema.ResourceData, meta interface{}) error { d.Set("key_rotation_enabled", key.rotation) d.Set("key_spec", key.metadata.KeySpec) d.Set("key_usage", key.metadata.KeyUsage) - d.Set("policy", 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) + } + + d.Set("policy", policyToSet) + d.Set("primary_key_arn", key.metadata.MultiRegionConfiguration.PrimaryKey.Arn) tags := key.tags.IgnoreAWS().IgnoreConfig(ignoreTagsConfig) diff --git a/internal/service/kms/replica_key_test.go b/internal/service/kms/replica_key_test.go index f60b202c979..b8bf3edbf90 100644 --- a/internal/service/kms/replica_key_test.go +++ b/internal/service/kms/replica_key_test.go @@ -82,7 +82,7 @@ func TestAccKMSReplicaKey_disappears(t *testing.T) { }) } -func TestAccKMSReplicaKey_DescriptionAndEnabled(t *testing.T) { +func TestAccKMSReplicaKey_descriptionAndEnabled(t *testing.T) { var providers []*schema.Provider var key kms.KeyMetadata rName1 := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) @@ -134,13 +134,13 @@ func TestAccKMSReplicaKey_DescriptionAndEnabled(t *testing.T) { }) } -func TestAccKMSReplicaKey_Policy(t *testing.T) { +func TestAccKMSReplicaKey_policy(t *testing.T) { var providers []*schema.Provider var key kms.KeyMetadata rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) resourceName := "aws_kms_replica_key.test" - policy1 := `{"Version":"2012-10-17","Id":"kms-tf-1","Statement":[{"Sid":"Enable IAM User Permissions 1","Effect":"Allow","Principal":{"AWS":"*"},"Action":"kms:*","Resource":"*"}]}` - policy2 := `{"Version":"2012-10-17","Id":"kms-tf-1","Statement":[{"Sid":"Enable IAM User Permissions 2","Effect":"Allow","Principal":{"AWS":"*"},"Action":"kms:*","Resource":"*"}]}` + policy1 := `{"Id":"kms-tf-1","Statement":[{"Action":"kms:*","Effect":"Allow","Principal":{"AWS":"*"},"Resource":"*","Sid":"Enable IAM User Permissions 1"}],"Version":"2012-10-17"}` + policy2 := `{"Id":"kms-tf-1","Statement":[{"Action":"kms:*","Effect":"Allow","Principal":{"AWS":"*"},"Resource":"*","Sid":"Enable IAM User Permissions 2"}],"Version":"2012-10-17"}` resource.ParallelTest(t, resource.TestCase{ PreCheck: func() { @@ -177,7 +177,7 @@ func TestAccKMSReplicaKey_Policy(t *testing.T) { }) } -func TestAccKMSReplicaKey_Tags(t *testing.T) { +func TestAccKMSReplicaKey_tags(t *testing.T) { var providers []*schema.Provider var key kms.KeyMetadata rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) @@ -227,7 +227,7 @@ func TestAccKMSReplicaKey_Tags(t *testing.T) { }) } -func TestAccKMSReplicaKey_TwoReplicas(t *testing.T) { +func TestAccKMSReplicaKey_twoReplicas(t *testing.T) { var providers []*schema.Provider var key kms.KeyMetadata rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) @@ -307,9 +307,7 @@ resource "aws_kms_replica_key" "test" { bypass_policy_lockout_safety_check = %[3]t - policy = < Date: Wed, 1 Dec 2021 13:50:26 -0500 Subject: [PATCH 5/7] kms/external_key: Preserve JSON policy order --- internal/service/kms/external_key.go | 10 +++++++++- internal/service/kms/external_key_test.go | 14 +++++--------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/internal/service/kms/external_key.go b/internal/service/kms/external_key.go index 91de2c7d3a5..a2989be9915 100644 --- a/internal/service/kms/external_key.go +++ b/internal/service/kms/external_key.go @@ -226,7 +226,15 @@ func resourceExternalKeyRead(d *schema.ResourceData, meta interface{}) error { d.Set("key_state", key.metadata.KeyState) d.Set("key_usage", key.metadata.KeyUsage) d.Set("multi_region", key.metadata.MultiRegion) - d.Set("policy", 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) + } + + d.Set("policy", policyToSet) + if key.metadata.ValidTo != nil { d.Set("valid_to", aws.TimeValue(key.metadata.ValidTo).Format(time.RFC3339)) } else { diff --git a/internal/service/kms/external_key_test.go b/internal/service/kms/external_key_test.go index 45f33aea7ee..30f987258e6 100644 --- a/internal/service/kms/external_key_test.go +++ b/internal/service/kms/external_key_test.go @@ -287,8 +287,8 @@ func TestAccKMSExternalKey_keyMaterialBase64(t *testing.T) { func TestAccKMSExternalKey_policy(t *testing.T) { var key1, key2 kms.KeyMetadata rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) - policy1 := `{"Version":"2012-10-17","Id":"kms-tf-1","Statement":[{"Sid":"Enable IAM User Permissions 1","Effect":"Allow","Principal":{"AWS":"*"},"Action":"kms:*","Resource":"*"}]}` - policy2 := `{"Version":"2012-10-17","Id":"kms-tf-1","Statement":[{"Sid":"Enable IAM User Permissions 2","Effect":"Allow","Principal":{"AWS":"*"},"Action":"kms:*","Resource":"*"}]}` + policy1 := `{"Id":"kms-tf-1","Statement":[{"Action":"kms:*","Effect":"Allow","Principal":{"AWS":"*"},"Resource":"*","Sid":"Enable IAM User Permissions 1"}],"Version":"2012-10-17"}` + policy2 := `{"Id":"kms-tf-1","Statement":[{"Action":"kms:*","Effect":"Allow","Principal":{"AWS":"*"},"Resource":"*","Sid":"Enable IAM User Permissions 2"}],"Version":"2012-10-17"}` resourceName := "aws_kms_external_key.test" resource.ParallelTest(t, resource.TestCase{ @@ -329,7 +329,7 @@ func TestAccKMSExternalKey_policy(t *testing.T) { func TestAccKMSExternalKey_policyBypass(t *testing.T) { var key kms.KeyMetadata rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) - policy := `{"Version":"2012-10-17","Id":"kms-tf-1","Statement":[{"Sid":"Enable IAM User Permissions 1","Effect":"Allow","Principal":{"AWS":"*"},"Action":"kms:*","Resource":"*"}]}` + policy := `{"Id":"kms-tf-1","Statement":[{"Action":"kms:*","Effect":"Allow","Principal":{"AWS":"*"},"Resource":"*","Sid":"Enable IAM User Permissions 1"}],"Version":"2012-10-17"}` resourceName := "aws_kms_external_key.test" resource.ParallelTest(t, resource.TestCase{ @@ -642,9 +642,7 @@ resource "aws_kms_external_key" "test" { description = %[1]q deletion_window_in_days = 7 - policy = < Date: Wed, 1 Dec 2021 13:52:59 -0500 Subject: [PATCH 6/7] Add changelog --- .changelog/21990.txt | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 .changelog/21990.txt diff --git a/.changelog/21990.txt b/.changelog/21990.txt new file mode 100644 index 00000000000..c18c9ba70d0 --- /dev/null +++ b/.changelog/21990.txt @@ -0,0 +1,11 @@ +```release-note:bug +resource/aws_kms_external_key: Fix order-related diffs in `policy` +``` + +```release-note:bug +resource/aws_kms_replica_external_key: Fix order-related diffs in `policy` +``` + +```release-note:bug +resource/aws_kms_replica_key: Fix order-related diffs in `policy` +``` \ No newline at end of file From 6e3a5c1f5695e29d88426ae3f1d2900c226e40a2 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Wed, 1 Dec 2021 13:57:45 -0500 Subject: [PATCH 7/7] Capits --- internal/service/kms/replica_external_key_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/service/kms/replica_external_key_test.go b/internal/service/kms/replica_external_key_test.go index 91e6b812a91..8357d1ab142 100644 --- a/internal/service/kms/replica_external_key_test.go +++ b/internal/service/kms/replica_external_key_test.go @@ -164,7 +164,7 @@ func TestAccKMSReplicaExternalKey_policy(t *testing.T) { }) } -func TestAccKMSReplicaExternalKey_Tags(t *testing.T) { +func TestAccKMSReplicaExternalKey_tags(t *testing.T) { var providers []*schema.Provider var key kms.KeyMetadata rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)