azurerm_web_application_firewall_policy - support managed_rules

[sirlatrom]

Adds managed_rules to azurerm_web_application_firewall_policy. Until this commit, at least since SDK version 2019-09-01 (as imported in the touched file), it was not possible to create a new resource of that type, as reported in #5354.

Note: Also streamline singular plurality for optional blocks allowing multiple repetitions to comply with how other azurerm_ resources use singular for block names when there may be multiple specified.

Fixes #5727.
Fixes #5354.

[sirlatrom]

Ping @katbyte @tombuildsstuff, this is a prerequisite for #6105 since the test in that one requires successfully creating an azurerm_web_application_firewall_policy (which this PR fixes).

[katbyte]

Thanks for this PR @sirlatrom,

I've left some comments inline but overall this is looking great. However i am unsure why you renamed custom_rules? If we are to do that we'll need to keep both an deprecate the old one as not to break existing use of the property by users so i'm thinking we revert that change?

[katbyte]

Thanks for making those changes @sirlatrom. I just noticed there is not test for the new properties, could we set them in the complete test? once that's done this should be good to merge.

[sirlatrom]

Thanks for making those changes @sirlatrom. I just noticed there is not test for the new properties, could we set them in the complete test? once that's done this should be good to merge.

This should be done in cefd1c8.

[katbyte]

@sirlatrom,

we can't add a new required property, one it clearly works without it 🙂 and it would break the resource for existing users. As can be see from the tests:

Test Failed

------- Stdout: -------
=== RUN   TestAccAzureRMWebApplicationFirewallPolicy_basic
=== PAUSE TestAccAzureRMWebApplicationFirewallPolicy_basic
=== CONT  TestAccAzureRMWebApplicationFirewallPolicy_basic
--- FAIL: TestAccAzureRMWebApplicationFirewallPolicy_basic (0.76s)
    testing.go:640: Step 0 error: config is invalid: "managed_rules": required field is not set

[sirlatrom]

@sirlatrom,

we can't add a new required property, one it clearly works without it 🙂 and it would break the resource for existing users. As can be see from the tests:

Test Failed

------- Stdout: -------
=== RUN   TestAccAzureRMWebApplicationFirewallPolicy_basic
=== PAUSE TestAccAzureRMWebApplicationFirewallPolicy_basic
=== CONT  TestAccAzureRMWebApplicationFirewallPolicy_basic
--- FAIL: TestAccAzureRMWebApplicationFirewallPolicy_basic (0.76s)
    testing.go:640: Step 0 error: config is invalid: "managed_rules": required field is not set

The property is required on the ARM side since API version 2019-08-01, but was not implemented when this resource was switched to that version. I'll try and add a reasonable default value.

[sirlatrom]

@katbyte When providing an empty ManagedRules object I get this API error from Azure, so I think it really must be specified:

Error: Error creating Web Application Firewall Policy "foobarPolicy" (Resource Group "rgFooBar"): network.WebApplicationFirewallPoliciesClient#CreateOrUpdate: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="ApplicationGatewayFirewallManagedRuleSetsNoValidCrsRuleSetsAttached" Message="Firewall policy /subscriptions/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx/resourceGroups/rgFooBar/providers/Microsoft.Network/ApplicationGatewayWebApplicationFirewallPolicies/foobarPolicy does not have any valid Core Rule Set attached to it." Details=[]

the essence of which is: foobarPolicy does not have any valid Core Rule Set attached to it.

It's all caused by #5004 which updated the SDK version from 2019-07-01 to 2019-09-01 without implementing the required field. See here: https://github.com/terraform-providers/terraform-provider-azurerm/blame/91d54d8ef85c82b2aa38723a12f5709dd9ef1abe/azurerm/resource_arm_web_application_firewall_policy.go#L8.

Anyone with a provider version prior to this change will not be affected. Anyone with the current version and later are currently not able to create firewall policy resources, and will need this fix to be able to do so again. Given how both the SDK and the API say the field is required and cannot just be created without properly configuring it, I would claim it actually does have to be required.

If we have to fill out default values for the ManagedRules field, it has to contain a choice of a managed rule set, which I don't think either of us are able to choose without breaking other things for the end users. Hence, I believe they should choose one themselves when upgrading to the provider version that adds this field.

[katbyte]

@sirlatrom, thanks for investigating this. I agree with everything you said and looks like we just have to mark it required and move forward as checking in our CI systems i can see the error and that this has been broken for quite some time. Triggering CI and taking a look now

[katbyte]

@sirlatrom, this looks good aside for we need to update the existing tests so they pass, basic with the minimal rules, complete with everything set.

[sirlatrom]

@katbyte Tests pass after rebase on origin/master which includes #6540. Please let me know if acceptance tests still fail after I added policy_settings to the basic test.

[katbyte]

thanks @sirlatrom! tests pass and LGTM now 🙂

[ghost]

This has been released in version 2.8.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.8.0"
}
# ... other configuration ...

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!