diff --git a/.changelog/22171.txt b/.changelog/22171.txt new file mode 100644 index 00000000000..957c07a5a54 --- /dev/null +++ b/.changelog/22171.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/aws_networkfirewall_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/networkfirewall/resource_policy.go b/internal/service/networkfirewall/resource_policy.go index 0c2fecdc21d..d15c7aef049 100644 --- a/internal/service/networkfirewall/resource_policy.go +++ b/internal/service/networkfirewall/resource_policy.go @@ -10,6 +10,7 @@ import ( "github.com/hashicorp/aws-sdk-go-base/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" "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" @@ -31,7 +32,11 @@ func ResourceResourcePolicy() *schema.Resource { Type: schema.TypeString, Required: true, ValidateFunc: validation.StringIsJSON, - DiffSuppressFunc: verify.SuppressEquivalentJSONDiffs, + DiffSuppressFunc: verify.SuppressEquivalentPolicyDiffs, + StateFunc: func(v interface{}) string { + json, _ := structure.NormalizeJsonString(v) + return json + }, }, "resource_arn": { Type: schema.TypeString, @@ -46,16 +51,23 @@ func ResourceResourcePolicy() *schema.Resource { func resourceResourcePolicyPut(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { conn := meta.(*conns.AWSClient).NetworkFirewallConn resourceArn := d.Get("resource_arn").(string) + + policy, err := structure.NormalizeJsonString(d.Get("policy").(string)) + + if err != nil { + return diag.Errorf("policy (%s) is invalid JSON: %s", policy, err) + } + input := &networkfirewall.PutResourcePolicyInput{ ResourceArn: aws.String(resourceArn), - Policy: aws.String(d.Get("policy").(string)), + Policy: aws.String(policy), } log.Printf("[DEBUG] Putting NetworkFirewall Resource Policy for resource: %s", resourceArn) - _, err := conn.PutResourcePolicyWithContext(ctx, input) + _, err = conn.PutResourcePolicyWithContext(ctx, input) if err != nil { - return diag.FromErr(fmt.Errorf("error putting NetworkFirewall Resource Policy (for resource: %s): %w", resourceArn, err)) + return diag.Errorf("error putting NetworkFirewall Resource Policy (for resource: %s): %s", resourceArn, err) } d.SetId(resourceArn) @@ -83,9 +95,16 @@ func resourceResourcePolicyRead(ctx context.Context, d *schema.ResourceData, met return diag.FromErr(fmt.Errorf("error reading NetworkFirewall Resource Policy (for resource: %s): empty output", resourceArn)) } - d.Set("policy", policy) d.Set("resource_arn", resourceArn) + policyToSet, err := verify.PolicyToSet(d.Get("policy").(string), aws.StringValue(policy)) + + if err != nil { + return diag.Errorf("setting policy %s: %s", aws.StringValue(policy), err) + } + + d.Set("policy", policyToSet) + return nil } diff --git a/internal/service/networkfirewall/resource_policy_test.go b/internal/service/networkfirewall/resource_policy_test.go index 4e2afe30d2c..3fe17d4761d 100644 --- a/internal/service/networkfirewall/resource_policy_test.go +++ b/internal/service/networkfirewall/resource_policy_test.go @@ -16,7 +16,7 @@ import ( tfnetworkfirewall "github.com/hashicorp/terraform-provider-aws/internal/service/networkfirewall" ) -func TestAccNetworkFirewallResourcePolicy_firewallPolicy(t *testing.T) { +func TestAccNetworkFirewallResourcePolicy_basic(t *testing.T) { rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) resourceName := "aws_networkfirewall_resource_policy.test" @@ -27,7 +27,7 @@ func TestAccNetworkFirewallResourcePolicy_firewallPolicy(t *testing.T) { CheckDestroy: testAccCheckResourcePolicyDestroy, Steps: []resource.TestStep{ { - Config: testAccNetworkFirewallResourcePolicy_firewallPolicy(rName), + Config: testAccResourcePolicyConfig(rName), Check: resource.ComposeTestCheckFunc( testAccCheckResourcePolicyExists(resourceName), resource.TestCheckResourceAttrPair(resourceName, "resource_arn", "aws_networkfirewall_firewall_policy.test", "arn"), @@ -35,17 +35,38 @@ func TestAccNetworkFirewallResourcePolicy_firewallPolicy(t *testing.T) { ), }, { - // Update the policy's Actions - Config: testAccNetworkFirewallResourcePolicy_firewallPolicy_updatePolicy(rName), + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + }, + }) +} + +func TestAccNetworkFirewallResourcePolicy_ignoreEquivalent(t *testing.T) { + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_networkfirewall_resource_policy.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t); testAccPreCheck(t) }, + ErrorCheck: acctest.ErrorCheck(t, networkfirewall.EndpointsID), + Providers: acctest.Providers, + CheckDestroy: testAccCheckResourcePolicyDestroy, + Steps: []resource.TestStep{ + { + Config: testAccResourcePolicyConfig(rName), Check: resource.ComposeTestCheckFunc( testAccCheckResourcePolicyExists(resourceName), - resource.TestMatchResourceAttr(resourceName, "policy", regexp.MustCompile(`"Action":\["network-firewall:UpdateFirewall","network-firewall:AssociateFirewallPolicy","network-firewall:ListFirewallPolicies","network-firewall:CreateFirewall"\]`)), + resource.TestCheckResourceAttrPair(resourceName, "resource_arn", "aws_networkfirewall_firewall_policy.test", "arn"), + resource.TestMatchResourceAttr(resourceName, "policy", regexp.MustCompile(`"Action":\["network-firewall:CreateFirewall","network-firewall:UpdateFirewall","network-firewall:AssociateFirewallPolicy","network-firewall:ListFirewallPolicies"\]`)), ), }, { - ResourceName: resourceName, - ImportState: true, - ImportStateVerify: true, + Config: testAccResourcePolicyEquivalentConfig(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckResourcePolicyExists(resourceName), + resource.TestMatchResourceAttr(resourceName, "policy", regexp.MustCompile(`"Action":\["network-firewall:CreateFirewall","network-firewall:UpdateFirewall","network-firewall:AssociateFirewallPolicy","network-firewall:ListFirewallPolicies"\]`)), + ), }, }, }) @@ -62,7 +83,7 @@ func TestAccNetworkFirewallResourcePolicy_ruleGroup(t *testing.T) { CheckDestroy: testAccCheckResourcePolicyDestroy, Steps: []resource.TestStep{ { - Config: testAccNetworkFirewallResourcePolicy_ruleGroup(rName), + Config: testAccResourcePolicy_ruleGroup(rName), Check: resource.ComposeTestCheckFunc( testAccCheckResourcePolicyExists(resourceName), resource.TestCheckResourceAttrPair(resourceName, "resource_arn", "aws_networkfirewall_rule_group.test", "arn"), @@ -71,10 +92,10 @@ func TestAccNetworkFirewallResourcePolicy_ruleGroup(t *testing.T) { }, { // Update the policy's Actions - Config: testAccNetworkFirewallResourcePolicy_ruleGroup_updatePolicy(rName), + Config: testAccResourcePolicy_ruleGroup_updatePolicy(rName), Check: resource.ComposeTestCheckFunc( testAccCheckResourcePolicyExists(resourceName), - resource.TestMatchResourceAttr(resourceName, "policy", regexp.MustCompile(`"Action":\["network-firewall:UpdateFirewallPolicy","network-firewall:ListRuleGroups","network-firewall:CreateFirewallPolicy"\]`)), + resource.TestMatchResourceAttr(resourceName, "policy", regexp.MustCompile(`"Action":\["network-firewall:CreateFirewallPolicy","network-firewall:UpdateFirewallPolicy","network-firewall:ListRuleGroups"\]`)), ), }, { @@ -97,7 +118,7 @@ func TestAccNetworkFirewallResourcePolicy_disappears(t *testing.T) { CheckDestroy: testAccCheckResourcePolicyDestroy, Steps: []resource.TestStep{ { - Config: testAccNetworkFirewallResourcePolicy_firewallPolicy(rName), + Config: testAccResourcePolicyConfig(rName), Check: resource.ComposeTestCheckFunc( testAccCheckResourcePolicyExists(resourceName), acctest.CheckResourceDisappears(acctest.Provider, tfnetworkfirewall.ResourceResourcePolicy(), resourceName), @@ -119,7 +140,7 @@ func TestAccNetworkFirewallResourcePolicy_Disappears_firewallPolicy(t *testing.T CheckDestroy: testAccCheckResourcePolicyDestroy, Steps: []resource.TestStep{ { - Config: testAccNetworkFirewallResourcePolicy_firewallPolicy(rName), + Config: testAccResourcePolicyConfig(rName), Check: resource.ComposeTestCheckFunc( testAccCheckResourcePolicyExists(resourceName), acctest.CheckResourceDisappears(acctest.Provider, tfnetworkfirewall.ResourceFirewallPolicy(), "aws_networkfirewall_firewall_policy.test"), @@ -141,7 +162,7 @@ func TestAccNetworkFirewallResourcePolicy_Disappears_ruleGroup(t *testing.T) { CheckDestroy: testAccCheckResourcePolicyDestroy, Steps: []resource.TestStep{ { - Config: testAccNetworkFirewallResourcePolicy_ruleGroup(rName), + Config: testAccResourcePolicy_ruleGroup(rName), Check: resource.ComposeTestCheckFunc( testAccCheckResourcePolicyExists(resourceName), acctest.CheckResourceDisappears(acctest.Provider, tfnetworkfirewall.ResourceRuleGroup(), "aws_networkfirewall_rule_group.test"), @@ -200,14 +221,14 @@ func testAccCheckResourcePolicyExists(n string) resource.TestCheckFunc { } } -func testAccNetworkFirewallResourcePolicyFirewallPolicyBaseConfig(rName string) string { +func testAccResourcePolicyFirewallPolicyBaseConfig(rName string) string { return fmt.Sprintf(` data "aws_partition" "current" {} data "aws_caller_identity" "current" {} resource "aws_networkfirewall_firewall_policy" "test" { - name = %q + name = %[1]q firewall_policy { stateless_fragment_default_actions = ["aws:drop"] stateless_default_actions = ["aws:pass"] @@ -216,9 +237,9 @@ resource "aws_networkfirewall_firewall_policy" "test" { `, rName) } -func testAccNetworkFirewallResourcePolicy_firewallPolicy(rName string) string { +func testAccResourcePolicyConfig(rName string) string { return acctest.ConfigCompose( - testAccNetworkFirewallResourcePolicyFirewallPolicyBaseConfig(rName), ` + testAccResourcePolicyFirewallPolicyBaseConfig(rName), ` resource "aws_networkfirewall_resource_policy" "test" { resource_arn = aws_networkfirewall_firewall_policy.test.arn # policy's Action element must include all of the following operations @@ -228,7 +249,7 @@ resource "aws_networkfirewall_resource_policy" "test" { "network-firewall:CreateFirewall", "network-firewall:UpdateFirewall", "network-firewall:AssociateFirewallPolicy", - "network-firewall:ListFirewallPolicies" + "network-firewall:ListFirewallPolicies", ] Effect = "Allow" Resource = aws_networkfirewall_firewall_policy.test.arn @@ -242,9 +263,9 @@ resource "aws_networkfirewall_resource_policy" "test" { `) } -func testAccNetworkFirewallResourcePolicy_firewallPolicy_updatePolicy(rName string) string { +func testAccResourcePolicyEquivalentConfig(rName string) string { return acctest.ConfigCompose( - testAccNetworkFirewallResourcePolicyFirewallPolicyBaseConfig(rName), ` + testAccResourcePolicyFirewallPolicyBaseConfig(rName), ` resource "aws_networkfirewall_resource_policy" "test" { resource_arn = aws_networkfirewall_firewall_policy.test.arn # policy's Action element must include all of the following operations @@ -254,7 +275,7 @@ resource "aws_networkfirewall_resource_policy" "test" { "network-firewall:UpdateFirewall", "network-firewall:AssociateFirewallPolicy", "network-firewall:ListFirewallPolicies", - "network-firewall:CreateFirewall" + "network-firewall:CreateFirewall", ] Effect = "Allow" Resource = aws_networkfirewall_firewall_policy.test.arn @@ -268,7 +289,7 @@ resource "aws_networkfirewall_resource_policy" "test" { `) } -func testAccNetworkFirewallResourcePolicyRuleGroupBaseConfig(rName string) string { +func testAccResourcePolicyRuleGroupBaseConfig(rName string) string { return fmt.Sprintf(` data "aws_partition" "current" {} @@ -291,9 +312,9 @@ resource "aws_networkfirewall_rule_group" "test" { `, rName) } -func testAccNetworkFirewallResourcePolicy_ruleGroup(rName string) string { +func testAccResourcePolicy_ruleGroup(rName string) string { return acctest.ConfigCompose( - testAccNetworkFirewallResourcePolicyRuleGroupBaseConfig(rName), ` + testAccResourcePolicyRuleGroupBaseConfig(rName), ` resource "aws_networkfirewall_resource_policy" "test" { resource_arn = aws_networkfirewall_rule_group.test.arn # policy's Action element must include all of the following operations @@ -316,9 +337,9 @@ resource "aws_networkfirewall_resource_policy" "test" { `) } -func testAccNetworkFirewallResourcePolicy_ruleGroup_updatePolicy(rName string) string { +func testAccResourcePolicy_ruleGroup_updatePolicy(rName string) string { return acctest.ConfigCompose( - testAccNetworkFirewallResourcePolicyRuleGroupBaseConfig(rName), ` + testAccResourcePolicyRuleGroupBaseConfig(rName), ` resource "aws_networkfirewall_resource_policy" "test" { resource_arn = aws_networkfirewall_rule_group.test.arn # policy's Action element must include all of the following operations @@ -327,7 +348,7 @@ resource "aws_networkfirewall_resource_policy" "test" { Action = [ "network-firewall:UpdateFirewallPolicy", "network-firewall:ListRuleGroups", - "network-firewall:CreateFirewallPolicy" + "network-firewall:CreateFirewallPolicy", ] Effect = "Allow" Resource = aws_networkfirewall_rule_group.test.arn