Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

azurerm_web_application_firewall_policy add enabled / disabled state for Custom Rules #23163

Merged

Conversation

UppyAU
Copy link
Contributor

@UppyAU UppyAU commented Sep 4, 2023

The azurerm_web_application_firewall_policy resource does not support and is unaware of the State property on the CustomRule present from API Version (2022-09-01).

  • This is an issue as you can't add this property to an ignore_changes block on this resource
  • Currently the resource does not capture the current state of a custom rule
  • Could potentially result in it being re-enabled whilst not indicating so in the terraform plan

Fixes #23123

This is my first PR ever. Please be kind.

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR @UppyAU.

Although we don't usually expose the enabled field for blocks, after internal discussion we consider it valid in this particular instance.

Overall this looks good, my only request is that we add or extend a test to cover the scenario where the field enabled is toggled. Currently we're only testing the creation of a custom_rule with enabled set to true or false from the get go.

@UppyAU
Copy link
Contributor Author

UppyAU commented Sep 8, 2023

TF_ACC=1 go test -v ./internal/services/network -run=TestAccWebApplicationFirewallPolicy_ -timeout 60m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccWebApplicationFirewallPolicy_basic
=== PAUSE TestAccWebApplicationFirewallPolicy_basic
=== RUN   TestAccWebApplicationFirewallPolicy_complete
=== PAUSE TestAccWebApplicationFirewallPolicy_complete
=== RUN   TestAccWebApplicationFirewallPolicy_update
=== PAUSE TestAccWebApplicationFirewallPolicy_update
=== RUN   TestAccWebApplicationFirewallPolicy_updateOverrideRules
=== PAUSE TestAccWebApplicationFirewallPolicy_updateOverrideRules
=== RUN   TestAccWebApplicationFirewallPolicy_knownCVEs
=== PAUSE TestAccWebApplicationFirewallPolicy_knownCVEs
=== RUN   TestAccWebApplicationFirewallPolicy_OperatorAny
=== PAUSE TestAccWebApplicationFirewallPolicy_OperatorAny
=== RUN   TestAccWebApplicationFirewallPolicy_excludedRules
=== PAUSE TestAccWebApplicationFirewallPolicy_excludedRules
=== RUN   TestAccWebApplicationFirewallPolicy_updateDisabledRules
=== PAUSE TestAccWebApplicationFirewallPolicy_updateDisabledRules
=== RUN   TestAccWebApplicationFirewallPolicy_LogScrubbing
=== PAUSE TestAccWebApplicationFirewallPolicy_LogScrubbing
=== RUN   TestAccWebApplicationFirewallPolicy_updateCustomRules
=== PAUSE TestAccWebApplicationFirewallPolicy_updateCustomRules
=== CONT  TestAccWebApplicationFirewallPolicy_basic
=== CONT  TestAccWebApplicationFirewallPolicy_OperatorAny
=== CONT  TestAccWebApplicationFirewallPolicy_updateOverrideRules
=== CONT  TestAccWebApplicationFirewallPolicy_updateCustomRules
=== CONT  TestAccWebApplicationFirewallPolicy_knownCVEs
=== CONT  TestAccWebApplicationFirewallPolicy_LogScrubbing
=== CONT  TestAccWebApplicationFirewallPolicy_updateDisabledRules
=== CONT  TestAccWebApplicationFirewallPolicy_excludedRules
=== CONT  TestAccWebApplicationFirewallPolicy_update
=== CONT  TestAccWebApplicationFirewallPolicy_complete
--- PASS: TestAccWebApplicationFirewallPolicy_OperatorAny (207.53s)
--- PASS: TestAccWebApplicationFirewallPolicy_basic (207.90s)
--- PASS: TestAccWebApplicationFirewallPolicy_LogScrubbing (208.13s)
--- PASS: TestAccWebApplicationFirewallPolicy_complete (211.93s)
--- PASS: TestAccWebApplicationFirewallPolicy_knownCVEs (217.78s)
--- PASS: TestAccWebApplicationFirewallPolicy_update (220.77s)
--- PASS: TestAccWebApplicationFirewallPolicy_updateDisabledRules (229.69s)
--- PASS: TestAccWebApplicationFirewallPolicy_updateCustomRules (234.29s)
--- PASS: TestAccWebApplicationFirewallPolicy_excludedRules (235.12s)
--- PASS: TestAccWebApplicationFirewallPolicy_updateOverrideRules (250.58s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/network       252.303s

@github-actions github-actions bot added size/XL and removed size/S labels Sep 8, 2023
@UppyAU
Copy link
Contributor Author

UppyAU commented Sep 8, 2023

Added a test where the each state a rule can be in Enabled, Disabled and Not Set and switching to each of the other possibilities. LMK is that works for you @stephybun

@UppyAU
Copy link
Contributor Author

UppyAU commented Sep 11, 2023

Vendor Dependencies Check / depscheck (pull_request) Failing after 53s

Should be fixed by #23235 ?

@stephybun
Copy link
Member

@UppyAU could you do a rebase to pull that fix in? I can't run the acceptance tests without it (the GHA on the PR is just our standard CI for linting, unit tests, formatting etc.)

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the very thorough test @UppyAU! LGTM 🍰

@stephybun stephybun merged commit 8c670ea into hashicorp:main Sep 12, 2023
23 checks passed
@github-actions github-actions bot added this to the v3.73.0 milestone Sep 12, 2023
stephybun added a commit that referenced this pull request Sep 12, 2023
@UppyAU UppyAU deleted the WebApplicationFirewallCustomRule-EnabledState branch September 12, 2023 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enabled / Disabled State for custom_rules in azurerm_web_application_firewall_policy
2 participants