From 7ae7c1b7c5a0cefa21c075f6a30375bce219ddda Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Fri, 10 Dec 2021 15:31:52 -0500 Subject: [PATCH 1/4] glue/resource_policy: Fix diffs with equivalent policies --- internal/service/glue/resource_policy.go | 23 +- internal/service/glue/resource_policy_test.go | 225 ++++++++++++------ 2 files changed, 169 insertions(+), 79 deletions(-) diff --git a/internal/service/glue/resource_policy.go b/internal/service/glue/resource_policy.go index 01ee324d7f4..0a38e47d025 100644 --- a/internal/service/glue/resource_policy.go +++ b/internal/service/glue/resource_policy.go @@ -8,6 +8,7 @@ import ( "github.com/aws/aws-sdk-go/service/glue" "github.com/hashicorp/aws-sdk-go-base/tfawserr" "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" "github.com/hashicorp/terraform-provider-aws/internal/verify" @@ -29,6 +30,10 @@ func ResourceResourcePolicy() *schema.Resource { Required: true, ValidateFunc: validation.StringIsJSON, DiffSuppressFunc: verify.SuppressEquivalentPolicyDiffs, + StateFunc: func(v interface{}) string { + json, _ := structure.NormalizeJsonString(v) + return json + }, }, "enable_hybrid": { Type: schema.TypeString, @@ -43,8 +48,14 @@ func resourceResourcePolicyPut(condition string) func(d *schema.ResourceData, me return func(d *schema.ResourceData, meta interface{}) error { conn := meta.(*conns.AWSClient).GlueConn + policy, err := structure.NormalizeJsonString(d.Get("policy").(string)) + + if err != nil { + return fmt.Errorf("policy (%s) is invalid JSON: %w", policy, err) + } + input := &glue.PutResourcePolicyInput{ - PolicyInJson: aws.String(d.Get("policy").(string)), + PolicyInJson: aws.String(policy), PolicyExistsCondition: aws.String(condition), } @@ -52,7 +63,7 @@ func resourceResourcePolicyPut(condition string) func(d *schema.ResourceData, me input.EnableHybrid = aws.String(v.(string)) } - _, err := conn.PutResourcePolicy(input) + _, err = conn.PutResourcePolicy(input) if err != nil { return fmt.Errorf("error putting policy request: %s", err) } @@ -78,7 +89,13 @@ func resourceResourcePolicyRead(d *schema.ResourceData, meta interface{}) error //Since the glue resource policy is global we expect it to be deleted when the policy is empty d.SetId("") } else { - d.Set("policy", resourcePolicy.PolicyInJson) + policyToSet, err := verify.PolicyToSet(d.Get("policy").(string), aws.StringValue(resourcePolicy.PolicyInJson)) + + if err != nil { + return err + } + + d.Set("policy", policyToSet) } return nil } diff --git a/internal/service/glue/resource_policy_test.go b/internal/service/glue/resource_policy_test.go index 9cbe8289a40..2b3e14f51d5 100644 --- a/internal/service/glue/resource_policy_test.go +++ b/internal/service/glue/resource_policy_test.go @@ -15,24 +15,6 @@ import ( tfglue "github.com/hashicorp/terraform-provider-aws/internal/service/glue" ) -func CreateTablePolicy(action string) string { - return fmt.Sprintf(`{ - "Version" : "2012-10-17", - "Statement" : [ - { - "Effect" : "Allow", - "Action" : [ - "%s" - ], - "Principal" : { - "AWS": "*" - }, - "Resource" : "arn:%s:glue:%s:%s:*" - } - ] -}`, action, acctest.Partition(), acctest.Region(), acctest.AccountID()) -} - func testAccResourcePolicy_basic(t *testing.T) { resourceName := "aws_glue_resource_policy.test" resource.Test(t, resource.TestCase{ @@ -42,7 +24,7 @@ func testAccResourcePolicy_basic(t *testing.T) { CheckDestroy: testAccCheckResourcePolicyDestroy, Steps: []resource.TestStep{ { - Config: testAccResourcePolicy_Required("glue:CreateTable"), + Config: testAccResourcePolicyRequiredConfig("glue:CreateTable"), Check: resource.ComposeTestCheckFunc( testAccResourcePolicy(resourceName, "glue:CreateTable"), ), @@ -65,7 +47,7 @@ func testAccResourcePolicy_hybrid(t *testing.T) { CheckDestroy: testAccCheckResourcePolicyDestroy, Steps: []resource.TestStep{ { - Config: testAccResourcePolicyHybrid("glue:CreateTable", "TRUE"), + Config: testAccResourcePolicyHybridConfig("glue:CreateTable", "TRUE"), Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr(resourceName, "enable_hybrid", "TRUE"), ), @@ -77,13 +59,13 @@ func testAccResourcePolicy_hybrid(t *testing.T) { ImportStateVerifyIgnore: []string{"enable_hybrid"}, }, { - Config: testAccResourcePolicyHybrid("glue:CreateTable", "FALSE"), + Config: testAccResourcePolicyHybridConfig("glue:CreateTable", "FALSE"), Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr(resourceName, "enable_hybrid", "FALSE"), ), }, { - Config: testAccResourcePolicyHybrid("glue:CreateTable", "TRUE"), + Config: testAccResourcePolicyHybridConfig("glue:CreateTable", "TRUE"), Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr(resourceName, "enable_hybrid", "TRUE"), ), @@ -100,7 +82,7 @@ func testAccResourcePolicy_disappears(t *testing.T) { CheckDestroy: testAccCheckResourcePolicyDestroy, Steps: []resource.TestStep{ { - Config: testAccResourcePolicy_Required("glue:CreateTable"), + Config: testAccResourcePolicyRequiredConfig("glue:CreateTable"), Check: resource.ComposeTestCheckFunc( testAccResourcePolicy(resourceName, "glue:CreateTable"), acctest.CheckResourceDisappears(acctest.Provider, tfglue.ResourceResourcePolicy(), resourceName), @@ -112,57 +94,6 @@ func testAccResourcePolicy_disappears(t *testing.T) { }) } -func testAccResourcePolicy_Required(action string) string { - return fmt.Sprintf(` -data "aws_caller_identity" "current" {} - -data "aws_partition" "current" {} - -data "aws_region" "current" {} - -data "aws_iam_policy_document" "glue-example-policy" { - statement { - actions = ["%s"] - resources = ["arn:${data.aws_partition.current.partition}:glue:${data.aws_region.current.name}:${data.aws_caller_identity.current.account_id}:*"] - principals { - identifiers = ["*"] - type = "AWS" - } - } -} - -resource "aws_glue_resource_policy" "test" { - policy = data.aws_iam_policy_document.glue-example-policy.json -} -`, action) -} - -func testAccResourcePolicyHybrid(action, hybrid string) string { - return fmt.Sprintf(` -data "aws_caller_identity" "current" {} - -data "aws_partition" "current" {} - -data "aws_region" "current" {} - -data "aws_iam_policy_document" "glue-example-policy" { - statement { - actions = ["%[1]s"] - resources = ["arn:${data.aws_partition.current.partition}:glue:${data.aws_region.current.name}:${data.aws_caller_identity.current.account_id}:*"] - principals { - identifiers = ["*"] - type = "AWS" - } - } -} - -resource "aws_glue_resource_policy" "test" { - policy = data.aws_iam_policy_document.glue-example-policy.json - enable_hybrid = %[2]q -} -`, action, hybrid) -} - func testAccResourcePolicy_update(t *testing.T) { resourceName := "aws_glue_resource_policy.test" resource.Test(t, resource.TestCase{ @@ -172,13 +103,13 @@ func testAccResourcePolicy_update(t *testing.T) { CheckDestroy: testAccCheckResourcePolicyDestroy, Steps: []resource.TestStep{ { - Config: testAccResourcePolicy_Required("glue:CreateTable"), + Config: testAccResourcePolicyRequiredConfig("glue:CreateTable"), Check: resource.ComposeTestCheckFunc( testAccResourcePolicy(resourceName, "glue:CreateTable"), ), }, { - Config: testAccResourcePolicy_Required("glue:DeleteTable"), + Config: testAccResourcePolicyRequiredConfig("glue:DeleteTable"), Check: resource.ComposeTestCheckFunc( testAccResourcePolicy(resourceName, "glue:DeleteTable"), ), @@ -192,6 +123,29 @@ func testAccResourcePolicy_update(t *testing.T) { }) } +func testAccResourcePolicy_ignoreEquivalent(t *testing.T) { + resourceName := "aws_glue_resource_policy.test" + + resource.Test(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + ErrorCheck: acctest.ErrorCheck(t, glue.EndpointsID), + Providers: acctest.Providers, + CheckDestroy: testAccCheckResourcePolicyDestroy, + Steps: []resource.TestStep{ + { + Config: testAccResourcePolicyOrderConfig(), + Check: resource.ComposeTestCheckFunc( + testAccResourcePolicy(resourceName, "glue:CreateTable"), + ), + }, + { + Config: testAccResourcePolicyNewOrderConfig(), + PlanOnly: true, + }, + }, + }) +} + func testAccResourcePolicy(n string, action string) resource.TestCheckFunc { return func(s *terraform.State) error { rs, ok := s.RootModule().Resources[n] @@ -243,3 +197,122 @@ func testAccCheckResourcePolicyDestroy(s *terraform.State) error { } return nil } + +func CreateTablePolicy(action string) string { + return fmt.Sprintf(`{ + "Version" : "2012-10-17", + "Statement" : [ + { + "Effect" : "Allow", + "Action" : [ + "%s" + ], + "Principal" : { + "AWS": "*" + }, + "Resource" : "arn:%s:glue:%s:%s:*" + } + ] +}`, action, acctest.Partition(), acctest.Region(), acctest.AccountID()) +} + +func testAccResourcePolicyRequiredConfig(action string) string { + return fmt.Sprintf(` +data "aws_caller_identity" "current" {} + +data "aws_partition" "current" {} + +data "aws_region" "current" {} + +data "aws_iam_policy_document" "glue-example-policy" { + statement { + actions = [%[1]q] + resources = ["arn:${data.aws_partition.current.partition}:glue:${data.aws_region.current.name}:${data.aws_caller_identity.current.account_id}:*"] + principals { + identifiers = ["*"] + type = "AWS" + } + } +} + +resource "aws_glue_resource_policy" "test" { + policy = data.aws_iam_policy_document.glue-example-policy.json +} +`, action) +} + +func testAccResourcePolicyHybridConfig(action, hybrid string) string { + return fmt.Sprintf(` +data "aws_caller_identity" "current" {} + +data "aws_partition" "current" {} + +data "aws_region" "current" {} + +data "aws_iam_policy_document" "glue-example-policy" { + statement { + actions = [%[1]q] + resources = ["arn:${data.aws_partition.current.partition}:glue:${data.aws_region.current.name}:${data.aws_caller_identity.current.account_id}:*"] + principals { + identifiers = ["*"] + type = "AWS" + } + } +} + +resource "aws_glue_resource_policy" "test" { + policy = data.aws_iam_policy_document.glue-example-policy.json + enable_hybrid = %[2]q +} +`, action, hybrid) +} + +func testAccResourcePolicyOrderConfig() string { + return ` +data "aws_caller_identity" "current" {} + +data "aws_partition" "current" {} + +data "aws_region" "current" {} + +resource "aws_glue_resource_policy" "test" { + policy = jsonencode({ + Statement = { + Action = [ + "glue:CreateTable", + "glue:DeleteTable", + ] + Resource = ["arn:${data.aws_partition.current.partition}:glue:${data.aws_region.current.name}:${data.aws_caller_identity.current.account_id}:*"] + Principal = { + AWS = "*" + } + } + }) +} +` +} + +func testAccResourcePolicyNewOrderConfig() string { + return ` +data "aws_caller_identity" "current" {} + +data "aws_partition" "current" {} + +data "aws_region" "current" {} + +resource "aws_glue_resource_policy" "test" { + policy = jsonencode({ + Statement = { + Action = [ + "glue:DeleteTable", + "glue:CreateTable", + ] + Resource = "arn:${data.aws_partition.current.partition}:glue:${data.aws_region.current.name}:${data.aws_caller_identity.current.account_id}:*" + Principal = { + AWS = ["*"] + } + } + }) +} +` +} From 53440395c1783f225a045597e385084e57e67865 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Fri, 10 Dec 2021 15:33:14 -0500 Subject: [PATCH 2/4] Add changelog --- .changelog/22167.txt | 3 +++ .../data_catalog_encryption_settings_data_source_test.go | 2 +- .../service/glue/data_catalog_encryption_settings_test.go | 2 +- internal/service/glue/glue_test.go | 5 +++-- 4 files changed, 8 insertions(+), 4 deletions(-) create mode 100644 .changelog/22167.txt diff --git a/.changelog/22167.txt b/.changelog/22167.txt new file mode 100644 index 00000000000..e488b521f84 --- /dev/null +++ b/.changelog/22167.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/aws_glue_resource_policy: Fix erroneous diffs in `policy` when no changes made or policies are equivalent +``` \ No newline at end of file diff --git a/internal/service/glue/data_catalog_encryption_settings_data_source_test.go b/internal/service/glue/data_catalog_encryption_settings_data_source_test.go index 2240399aeba..cac06538a1d 100644 --- a/internal/service/glue/data_catalog_encryption_settings_data_source_test.go +++ b/internal/service/glue/data_catalog_encryption_settings_data_source_test.go @@ -8,7 +8,7 @@ import ( "github.com/hashicorp/terraform-provider-aws/internal/acctest" ) -func testAccGlueDataCatalogEncryptionSettingsDataSource_basic(t *testing.T) { +func testAccDataCatalogEncryptionSettingsDataSource_basic(t *testing.T) { resourceName := "aws_glue_data_catalog_encryption_settings.test" dataSourceName := "data.aws_glue_data_catalog_encryption_settings.test" diff --git a/internal/service/glue/data_catalog_encryption_settings_test.go b/internal/service/glue/data_catalog_encryption_settings_test.go index 56bef0e8047..a5df0736e2a 100644 --- a/internal/service/glue/data_catalog_encryption_settings_test.go +++ b/internal/service/glue/data_catalog_encryption_settings_test.go @@ -13,7 +13,7 @@ import ( "github.com/hashicorp/terraform-provider-aws/internal/conns" ) -func testAccGlueDataCatalogEncryptionSettings_basic(t *testing.T) { +func testAccDataCatalogEncryptionSettings_basic(t *testing.T) { var settings glue.DataCatalogEncryptionSettings rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) diff --git a/internal/service/glue/glue_test.go b/internal/service/glue/glue_test.go index a8276df1053..3b81c091715 100644 --- a/internal/service/glue/glue_test.go +++ b/internal/service/glue/glue_test.go @@ -5,14 +5,15 @@ import "testing" func TestAccGlue_serial(t *testing.T) { testCases := map[string]map[string]func(t *testing.T){ "DataCatalogEncryptionSettings": { - "basic": testAccGlueDataCatalogEncryptionSettings_basic, - "DataSource": testAccGlueDataCatalogEncryptionSettingsDataSource_basic, + "basic": testAccDataCatalogEncryptionSettings_basic, + "dataSource": testAccDataCatalogEncryptionSettingsDataSource_basic, }, "ResourcePolicy": { "basic": testAccResourcePolicy_basic, "update": testAccResourcePolicy_update, "hybrid": testAccResourcePolicy_hybrid, "disappears": testAccResourcePolicy_disappears, + "equivalent": testAccResourcePolicy_ignoreEquivalent, }, } From f668f0ee4e3fe6a167ef201ec2ff0f2c3cd893a1 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Fri, 10 Dec 2021 15:34:54 -0500 Subject: [PATCH 3/4] Explicit effect --- internal/service/glue/resource_policy_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/service/glue/resource_policy_test.go b/internal/service/glue/resource_policy_test.go index 2b3e14f51d5..38321f86f9e 100644 --- a/internal/service/glue/resource_policy_test.go +++ b/internal/service/glue/resource_policy_test.go @@ -282,6 +282,7 @@ resource "aws_glue_resource_policy" "test" { "glue:CreateTable", "glue:DeleteTable", ] + Effect = "Allow" Resource = ["arn:${data.aws_partition.current.partition}:glue:${data.aws_region.current.name}:${data.aws_caller_identity.current.account_id}:*"] Principal = { AWS = "*" @@ -303,6 +304,7 @@ data "aws_region" "current" {} resource "aws_glue_resource_policy" "test" { policy = jsonencode({ Statement = { + Effect = "Allow" Action = [ "glue:DeleteTable", "glue:CreateTable", From cbb996844585fc8efb9baec87b2a2c2c03791fc5 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Fri, 10 Dec 2021 15:53:23 -0500 Subject: [PATCH 4/4] Fix up test --- internal/service/glue/resource_policy_test.go | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/internal/service/glue/resource_policy_test.go b/internal/service/glue/resource_policy_test.go index 38321f86f9e..ff4bfa6b207 100644 --- a/internal/service/glue/resource_policy_test.go +++ b/internal/service/glue/resource_policy_test.go @@ -133,13 +133,13 @@ func testAccResourcePolicy_ignoreEquivalent(t *testing.T) { CheckDestroy: testAccCheckResourcePolicyDestroy, Steps: []resource.TestStep{ { - Config: testAccResourcePolicyOrderConfig(), + Config: testAccResourcePolicyEquivalentConfig(), Check: resource.ComposeTestCheckFunc( testAccResourcePolicy(resourceName, "glue:CreateTable"), ), }, { - Config: testAccResourcePolicyNewOrderConfig(), + Config: testAccResourcePolicyEquivalent2Config(), PlanOnly: true, }, }, @@ -267,7 +267,7 @@ resource "aws_glue_resource_policy" "test" { `, action, hybrid) } -func testAccResourcePolicyOrderConfig() string { +func testAccResourcePolicyEquivalentConfig() string { return ` data "aws_caller_identity" "current" {} @@ -277,13 +277,13 @@ data "aws_region" "current" {} resource "aws_glue_resource_policy" "test" { policy = jsonencode({ + Version = "2012-10-17" Statement = { - Action = [ - "glue:CreateTable", - "glue:DeleteTable", - ] + Action = "glue:CreateTable" Effect = "Allow" - Resource = ["arn:${data.aws_partition.current.partition}:glue:${data.aws_region.current.name}:${data.aws_caller_identity.current.account_id}:*"] + Resource = [ + "arn:${data.aws_partition.current.partition}:glue:${data.aws_region.current.name}:${data.aws_caller_identity.current.account_id}:*" + ] Principal = { AWS = "*" } @@ -293,7 +293,7 @@ resource "aws_glue_resource_policy" "test" { ` } -func testAccResourcePolicyNewOrderConfig() string { +func testAccResourcePolicyEquivalent2Config() string { return ` data "aws_caller_identity" "current" {} @@ -303,10 +303,10 @@ data "aws_region" "current" {} resource "aws_glue_resource_policy" "test" { policy = jsonencode({ + Version = "2012-10-17" Statement = { Effect = "Allow" Action = [ - "glue:DeleteTable", "glue:CreateTable", ] Resource = "arn:${data.aws_partition.current.partition}:glue:${data.aws_region.current.name}:${data.aws_caller_identity.current.account_id}:*"