From 6ff98fb950745e3ae15e82de6ebe2ef167a18ee6 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 9 Dec 2021 10:01:10 -0500 Subject: [PATCH 1/3] logs/resource_policy: Fix equivalent policy diffs --- .../service/cloudwatchlogs/resource_policy.go | 30 ++++- .../cloudwatchlogs/resource_policy_test.go | 117 +++++++++++++++--- 2 files changed, 129 insertions(+), 18 deletions(-) diff --git a/internal/service/cloudwatchlogs/resource_policy.go b/internal/service/cloudwatchlogs/resource_policy.go index dafebfac137..bb7550c841e 100644 --- a/internal/service/cloudwatchlogs/resource_policy.go +++ b/internal/service/cloudwatchlogs/resource_policy.go @@ -7,6 +7,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/cloudwatchlogs" "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/verify" ) @@ -36,6 +37,10 @@ func ResourceResourcePolicy() *schema.Resource { Required: true, ValidateFunc: validResourcePolicyDocument, DiffSuppressFunc: verify.SuppressEquivalentPolicyDiffs, + StateFunc: func(v interface{}) string { + json, _ := structure.NormalizeJsonString(v) + return json + }, }, }, } @@ -44,15 +49,21 @@ func ResourceResourcePolicy() *schema.Resource { func resourceResourcePolicyPut(d *schema.ResourceData, meta interface{}) error { conn := meta.(*conns.AWSClient).CloudWatchLogsConn + policy, err := structure.NormalizeJsonString(d.Get("policy_document").(string)) + + if err != nil { + return fmt.Errorf("policy (%s) is invalid JSON: %w", policy, err) + } + policyName := d.Get("policy_name").(string) input := &cloudwatchlogs.PutResourcePolicyInput{ - PolicyDocument: aws.String(d.Get("policy_document").(string)), + PolicyDocument: aws.String(policy), PolicyName: aws.String(policyName), } log.Printf("[DEBUG] Writing CloudWatch log resource policy: %#v", input) - _, err := conn.PutResourcePolicy(input) + _, err = conn.PutResourcePolicy(input) if err != nil { return fmt.Errorf("Writing CloudWatch log resource policy failed: %s", err.Error()) @@ -75,7 +86,19 @@ func resourceResourcePolicyRead(d *schema.ResourceData, meta interface{}) error return nil } - d.Set("policy_document", resourcePolicy.PolicyDocument) + policyToSet, err := verify.SecondJSONUnlessEquivalent(d.Get("policy_document").(string), aws.StringValue(resourcePolicy.PolicyDocument)) + + if err != nil { + return fmt.Errorf("while setting policy (%s), encountered: %w", policyToSet, err) + } + + policyToSet, err = structure.NormalizeJsonString(policyToSet) + + if err != nil { + return fmt.Errorf("policy (%s) is invalid JSON: %w", policyToSet, err) + } + + d.Set("policy_document", policyToSet) return nil } @@ -99,6 +122,7 @@ func LookupResourcePolicy(conn *cloudwatchlogs.CloudWatchLogs, input := &cloudwatchlogs.DescribeResourcePoliciesInput{ NextToken: nextToken, } + log.Printf("[DEBUG] Reading CloudWatch log resource policies: %#v", input) resp, err := conn.DescribeResourcePolicies(input) if err != nil { diff --git a/internal/service/cloudwatchlogs/resource_policy_test.go b/internal/service/cloudwatchlogs/resource_policy_test.go index 240c78225d7..d8081329e35 100644 --- a/internal/service/cloudwatchlogs/resource_policy_test.go +++ b/internal/service/cloudwatchlogs/resource_policy_test.go @@ -14,7 +14,7 @@ import ( ) func TestAccCloudWatchLogsResourcePolicy_basic(t *testing.T) { - name := sdkacctest.RandString(5) + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) resourceName := "aws_cloudwatch_log_resource_policy.test" var resourcePolicy cloudwatchlogs.ResourcePolicy @@ -22,14 +22,14 @@ func TestAccCloudWatchLogsResourcePolicy_basic(t *testing.T) { PreCheck: func() { acctest.PreCheck(t) }, ErrorCheck: acctest.ErrorCheck(t, cloudwatchlogs.EndpointsID), Providers: acctest.Providers, - CheckDestroy: testAccCheckCloudWatchLogResourcePolicyDestroy, + CheckDestroy: testAccCheckResourcePolicyDestroy, Steps: []resource.TestStep{ { - Config: testAccCheckResourcePolicyResourceBasic1Config(name), + Config: testAccCheckResourcePolicyResourceBasic1Config(rName), Check: resource.ComposeTestCheckFunc( testAccCheckCloudWatchLogResourcePolicy(resourceName, &resourcePolicy), - resource.TestCheckResourceAttr(resourceName, "policy_name", name), - resource.TestCheckResourceAttr(resourceName, "policy_document", fmt.Sprintf("{\"Version\":\"2012-10-17\",\"Statement\":[{\"Sid\":\"\",\"Effect\":\"Allow\",\"Principal\":{\"Service\":\"rds.%s\"},\"Action\":[\"logs:PutLogEvents\",\"logs:CreateLogStream\"],\"Resource\":\"arn:%s:logs:*:*:log-group:/aws/rds/*\"}]}", acctest.PartitionDNSSuffix(), acctest.Partition())), + resource.TestCheckResourceAttr(resourceName, "policy_name", rName), + resource.TestCheckResourceAttr(resourceName, "policy_document", fmt.Sprintf("{\"Statement\":[{\"Action\":[\"logs:PutLogEvents\",\"logs:CreateLogStream\"],\"Effect\":\"Allow\",\"Principal\":{\"Service\":\"rds.%s\"},\"Resource\":\"arn:%s:logs:*:*:log-group:/aws/rds/*\",\"Sid\":\"\"}],\"Version\":\"2012-10-17\"}", acctest.PartitionDNSSuffix(), acctest.Partition())), ), }, { @@ -38,11 +38,42 @@ func TestAccCloudWatchLogsResourcePolicy_basic(t *testing.T) { ImportStateVerify: true, }, { - Config: testAccCheckResourcePolicyResourceBasic2Config(name), + Config: testAccCheckResourcePolicyResourceBasic2Config(rName), Check: resource.ComposeTestCheckFunc( testAccCheckCloudWatchLogResourcePolicy(resourceName, &resourcePolicy), - resource.TestCheckResourceAttr(resourceName, "policy_name", name), - resource.TestCheckResourceAttr(resourceName, "policy_document", fmt.Sprintf("{\"Version\":\"2012-10-17\",\"Statement\":[{\"Sid\":\"\",\"Effect\":\"Allow\",\"Principal\":{\"Service\":\"rds.%s\"},\"Action\":[\"logs:PutLogEvents\",\"logs:CreateLogStream\"],\"Resource\":\"arn:%s:logs:*:*:log-group:/aws/rds/example.com\"}]}", acctest.PartitionDNSSuffix(), acctest.Partition())), + resource.TestCheckResourceAttr(resourceName, "policy_name", rName), + resource.TestCheckResourceAttr(resourceName, "policy_document", fmt.Sprintf("{\"Statement\":[{\"Action\":[\"logs:PutLogEvents\",\"logs:CreateLogStream\"],\"Effect\":\"Allow\",\"Principal\":{\"Service\":\"rds.%s\"},\"Resource\":\"arn:%s:logs:*:*:log-group:/aws/rds/example.com\",\"Sid\":\"\"}],\"Version\":\"2012-10-17\"}", acctest.PartitionDNSSuffix(), acctest.Partition())), + ), + }, + }, + }) +} + +func TestAccCloudWatchLogsResourcePolicy_ignoreEquivalent(t *testing.T) { + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_cloudwatch_log_resource_policy.test" + var resourcePolicy cloudwatchlogs.ResourcePolicy + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + ErrorCheck: acctest.ErrorCheck(t, cloudwatchlogs.EndpointsID), + Providers: acctest.Providers, + CheckDestroy: testAccCheckResourcePolicyDestroy, + Steps: []resource.TestStep{ + { + Config: testAccCheckResourcePolicyResourceOrderConfig(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckCloudWatchLogResourcePolicy(resourceName, &resourcePolicy), + resource.TestCheckResourceAttr(resourceName, "policy_name", rName), + resource.TestCheckResourceAttr(resourceName, "policy_document", fmt.Sprintf("{\"Statement\":[{\"Action\":[\"logs:CreateLogStream\",\"logs:PutLogEvents\"],\"Effect\":\"Allow\",\"Principal\":{\"Service\":[\"rds.%s\"]},\"Resource\":[\"arn:%s:logs:*:*:log-group:/aws/rds/example.com\"]}],\"Version\":\"2012-10-17\"}", acctest.PartitionDNSSuffix(), acctest.Partition())), + ), + }, + { + Config: testAccCheckResourcePolicyResourceNewOrderConfig(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckCloudWatchLogResourcePolicy(resourceName, &resourcePolicy), + resource.TestCheckResourceAttr(resourceName, "policy_name", rName), + resource.TestCheckResourceAttr(resourceName, "policy_document", fmt.Sprintf("{\"Statement\":[{\"Action\":[\"logs:CreateLogStream\",\"logs:PutLogEvents\"],\"Effect\":\"Allow\",\"Principal\":{\"Service\":[\"rds.%s\"]},\"Resource\":[\"arn:%s:logs:*:*:log-group:/aws/rds/example.com\"]}],\"Version\":\"2012-10-17\"}", acctest.PartitionDNSSuffix(), acctest.Partition())), ), }, }, @@ -75,7 +106,7 @@ func testAccCheckCloudWatchLogResourcePolicy(pr string, resourcePolicy *cloudwat } } -func testAccCheckCloudWatchLogResourcePolicyDestroy(s *terraform.State) error { +func testAccCheckResourcePolicyDestroy(s *terraform.State) error { conn := acctest.Provider.Meta().(*conns.AWSClient).CloudWatchLogsConn for _, rs := range s.RootModule().Resources { @@ -97,7 +128,7 @@ func testAccCheckCloudWatchLogResourcePolicyDestroy(s *terraform.State) error { return nil } -func testAccCheckResourcePolicyResourceBasic1Config(name string) string { +func testAccCheckResourcePolicyResourceBasic1Config(rName string) string { return fmt.Sprintf(` data "aws_partition" "current" {} @@ -118,13 +149,13 @@ data "aws_iam_policy_document" "test" { } resource "aws_cloudwatch_log_resource_policy" "test" { - policy_name = "%s" + policy_name = %[1]q policy_document = data.aws_iam_policy_document.test.json } -`, name) +`, rName) } -func testAccCheckResourcePolicyResourceBasic2Config(name string) string { +func testAccCheckResourcePolicyResourceBasic2Config(rName string) string { return fmt.Sprintf(` data "aws_partition" "current" {} @@ -145,8 +176,64 @@ data "aws_iam_policy_document" "test" { } resource "aws_cloudwatch_log_resource_policy" "test" { - policy_name = "%s" + policy_name = %[1]q policy_document = data.aws_iam_policy_document.test.json } -`, name) +`, rName) +} + +func testAccCheckResourcePolicyResourceOrderConfig(rName string) string { + return fmt.Sprintf(` +data "aws_partition" "current" {} + +resource "aws_cloudwatch_log_resource_policy" "test" { + policy_name = %[1]q + policy_document = jsonencode({ + Statement = [{ + Action = [ + "logs:CreateLogStream", + "logs:PutLogEvents", + ] + Effect = "Allow" + Resource = [ + "arn:${data.aws_partition.current.partition}:logs:*:*:log-group:/aws/rds/example.com", + ] + Principal = { + Service = [ + "rds.${data.aws_partition.current.dns_suffix}", + ] + } + }] + Version = "2012-10-17" + }) +} +`, rName) +} + +func testAccCheckResourcePolicyResourceNewOrderConfig(rName string) string { + return fmt.Sprintf(` +data "aws_partition" "current" {} + +resource "aws_cloudwatch_log_resource_policy" "test" { + policy_name = %[1]q + policy_document = jsonencode({ + Statement = [{ + Action = [ + "logs:PutLogEvents", + "logs:CreateLogStream", + ] + Effect = "Allow" + Resource = [ + "arn:${data.aws_partition.current.partition}:logs:*:*:log-group:/aws/rds/example.com", + ] + Principal = { + Service = [ + "rds.${data.aws_partition.current.dns_suffix}", + ] + } + }] + Version = "2012-10-17" + }) +} +`, rName) } From 0006872763d592babb025a6c57c781957cc19946 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 9 Dec 2021 10:02:35 -0500 Subject: [PATCH 2/3] Add changelog --- .changelog/22135.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/22135.txt diff --git a/.changelog/22135.txt b/.changelog/22135.txt new file mode 100644 index 00000000000..77875a294dd --- /dev/null +++ b/.changelog/22135.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/aws_cloudwatch_log_resource_policy: Fix erroneous diffs in `policy_document` when no changes made or policies are equivalent +``` \ No newline at end of file From 1f91f0412ef77e939357171df1fccfea148deb58 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 9 Dec 2021 10:06:08 -0500 Subject: [PATCH 3/3] Lint Eastwood --- internal/service/cloudwatchlogs/resource_policy_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/service/cloudwatchlogs/resource_policy_test.go b/internal/service/cloudwatchlogs/resource_policy_test.go index d8081329e35..a3dbd8ed30e 100644 --- a/internal/service/cloudwatchlogs/resource_policy_test.go +++ b/internal/service/cloudwatchlogs/resource_policy_test.go @@ -187,7 +187,7 @@ func testAccCheckResourcePolicyResourceOrderConfig(rName string) string { data "aws_partition" "current" {} resource "aws_cloudwatch_log_resource_policy" "test" { - policy_name = %[1]q + policy_name = %[1]q policy_document = jsonencode({ Statement = [{ Action = [ @@ -215,7 +215,7 @@ func testAccCheckResourcePolicyResourceNewOrderConfig(rName string) string { data "aws_partition" "current" {} resource "aws_cloudwatch_log_resource_policy" "test" { - policy_name = %[1]q + policy_name = %[1]q policy_document = jsonencode({ Statement = [{ Action = [