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

r/azurerm_application_gateway: add firewall_policy_id config to path rules #11239

Merged

Conversation

patst
Copy link
Contributor

@patst patst commented Apr 7, 2021

Fixes #9040 .

Enables the configuration of firewall_policy_id on path rules:

...
  path_rule {
        name                       = local.path_rule_name
        backend_address_pool_name  = local.backend_address_pool_name
        backend_http_settings_name = local.http_setting_name
        firewall_policy_id         = azurerm_web_application_firewall_policy.testfwp_path_rule.id
        paths = [
          "/test",
        ]
      }
...

The new Test passed:

make testacc TEST='./azurerm/internal/services/network/application_gateway_resource_test.go' TESTARGS='-run=TestAccApplicationGateway_customPathRuleFirewallPolicy' TESTTIMEOUT='120m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test ./azurerm/internal/services/network/application_gateway_resource_test.go -v -run=TestAccApplicationGateway_customPathRuleFirewallPolicy -timeout 120m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
2021/04/07 19:18:45 [DEBUG] not using binary driver name, it's no longer needed
2021/04/07 19:18:46 [DEBUG] not using binary driver name, it's no longer needed
=== RUN   TestAccApplicationGateway_customPathRuleFirewallPolicy
=== PAUSE TestAccApplicationGateway_customPathRuleFirewallPolicy
=== CONT  TestAccApplicationGateway_customPathRuleFirewallPolicy
--- PASS: TestAccApplicationGateway_customPathRuleFirewallPolicy (1465.99s)
PASS
ok      command-line-arguments  1467.732s

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @patst

Thanks for this PR.

I've taken a look through and left a couple of comments inline but on the whole this looks pretty good - if we can fix those up then this should be good to merge 👍

Thanks!

remove not required test assertion
@ghost ghost added size/XL and removed size/L labels Apr 8, 2021
@patst
Copy link
Contributor Author

patst commented Apr 8, 2021

@tombuildsstuff thanks for the feedback, I applied your suggestions and generated the custom validator!

@ghost ghost removed the waiting-response label Apr 8, 2021
@tombuildsstuff tombuildsstuff added this to the v2.55.0 milestone Apr 8, 2021
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for pushing those changes @patst

@katbyte
Copy link
Collaborator

katbyte commented Apr 8, 2021

@patst - while running the tests for this i got a failure:

------- Stdout: -------
=== RUN   TestAccApplicationGateway_customPathRuleFirewallPolicy
=== PAUSE TestAccApplicationGateway_customPathRuleFirewallPolicy
=== CONT  TestAccApplicationGateway_customPathRuleFirewallPolicy
    testing.go:620: Step 1/2 error: Error running apply: 
        Error: ID was missing the `firewallPolicies` element
        
          on config010598691/terraform_plugin_test.tf line 90, in resource "azurerm_application_gateway" "test":
          90: resource "azurerm_application_gateway" "test" {

if you can fix this up we can get this merged

@katbyte katbyte modified the milestones: v2.55.0, v2.56.0 Apr 9, 2021
@patst
Copy link
Contributor Author

patst commented Apr 9, 2021

@katbyte sorry for that. The validator crashed it.

I renamed it and fixed the test:

make testacc TEST='./azurerm/internal/services/network/application_gateway_resource_test.go' TESTARGS='-run=TestAccApplicationGateway_customPathRuleFirewallPolicy' TESTTIMEOUT='120m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test ./azurerm/internal/services/network/application_gateway_resource_test.go -v -run=TestAccApplicationGateway_customPathRuleFirewallPolicy -timeout 120m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
2021/04/09 09:38:51 [DEBUG] not using binary driver name, it's no longer needed
2021/04/09 09:38:51 [DEBUG] not using binary driver name, it's no longer needed
=== RUN   TestAccApplicationGateway_customPathRuleFirewallPolicy
=== PAUSE TestAccApplicationGateway_customPathRuleFirewallPolicy
=== CONT  TestAccApplicationGateway_customPathRuleFirewallPolicy
--- PASS: TestAccApplicationGateway_customPathRuleFirewallPolicy (1560.17s)
PASS
ok      command-line-arguments  1561.850s

@hashicorp hashicorp deleted a comment Apr 12, 2021
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@tombuildsstuff
Copy link
Contributor

Tests look good 👍

@ghost
Copy link

ghost commented Apr 16, 2021

This has been released in version 2.56.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.56.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented May 12, 2021

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!

@ghost ghost locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for setting FirewallPolicy ID in ApplicationGateway PathRules
3 participants