azurerm_firewall: support for firewall manager (firewall policy) #8879

magodo · 2020-10-14T07:18:56Z

This PR adds the required properties to azurerm_firewall for make use of azurerm_firewall_policy, under both secured virtual hub cases and hub virtual network cases.

This implements part of feature request in #7319.

Test Result

💤 make testacc TEST=./azurerm/internal/services/network/tests TESTARGS='-run "TestAccAzureRMFirewall_basic|TestAccAzureRMFirewall_inVirtualHub|TestAccAzureRMFirewall_withFirewallPolicy"'
                                                                      
==> 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/tests -v -run "TestAccAzureRMFirewall_basic|TestAccAzureRMFirewall_inVirtualHub|TestAccAzureRMFirewall_withFirewallPolicy" -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersi
on=acc"
=== RUN   TestAccAzureRMFirewall_basic
=== PAUSE TestAccAzureRMFirewall_basic                          
=== RUN   TestAccAzureRMFirewall_withFirewallPolicy
=== PAUSE TestAccAzureRMFirewall_withFirewallPolicy
=== RUN   TestAccAzureRMFirewall_inVirtualHub      
=== PAUSE TestAccAzureRMFirewall_inVirtualHub                                                                                                
=== CONT  TestAccAzureRMFirewall_basic                                                                                                       
=== CONT  TestAccAzureRMFirewall_inVirtualHub            
=== CONT  TestAccAzureRMFirewall_withFirewallPolicy
--- PASS: TestAccAzureRMFirewall_basic (1139.61s)
--- PASS: TestAccAzureRMFirewall_withFirewallPolicy (1616.77s)                                                                               
--- PASS: TestAccAzureRMFirewall_inVirtualHub (2658.22s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/network/tests       2658.259s

katbyte

Thanks for the pr @magodo - left some comments inline to address

azurerm/internal/services/network/firewall_resource.go

azurerm/internal/services/network/tests/firewall_resource_test.go

website/docs/r/firewall.html.markdown

katbyte · 2020-11-05T03:23:48Z

website/docs/r/firewall.html.markdown

+
+* `public_ip_addresses` - (Optional) A list of public IP addresses associated with the Firewall.
+
+-> **NOTE** In most cases, users do not need to specify this argument, except when scaling down the public IPs (by changing the `public_ip_count`), this argument allows users to specify the existing IPs to be retained. Otherwise, the first existing `public_ip_count` public ip addresses will be retained.


punctuation

Suggested change

-> **NOTE** In most cases, users do not need to specify this argument, except when scaling down the public IPs (by changing the `public_ip_count`), this argument allows users to specify the existing IPs to be retained. Otherwise, the first existing `public_ip_count` public ip addresses will be retained.

-> **NOTE** In most cases, users do not need to specify this argument, except when scaling down the public IPs (by changing the `public_ip_count`). This argument allows users to specify the existing IPs to be retained, otherwise, the first existing public ip addresses will be retained.

The last change here will change the meaning a bit, it will affect the first existing N public ip addresses actually, where N equals to public_ip_count.

I'm not sure how better to word it as i still don't fully understand what it is doing :/

This means when you scale down the public IPs of the firewall, say from (pip1, pip2, pip3) to only one pip. If you simply change the public_ip_count from 3 to 1. Then it ends up with the pip1. However, if you intend to keep the pip3, then you can specify it using this property, say public_ip_addresses = [pip3].

azurerm/internal/services/network/tests/firewall_resource_test.go

azurerm/internal/services/network/firewall_resource.go

…rvers` Since API will raise errors when it is set as `false` in case firewall enables the firewall policy.

magodo · 2020-11-06T08:14:37Z

Hi @katbyte Thank you for the review comments! I have updated the code accordingly, and fix one coflict setting introduced in #8878 after rebasing to the master.

Both the test introduced in this PR and #8878 are passed now:

terraform-provider-azurerm on  new_firewall_support_policy [$] via 🐹 v1.15.3 
💢 make testacc TEST=./azurerm/internal/services/network/tests TESTARGS='-run "TestAccAzureRMFirewall_inVirtualHub"'
==> 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/tests -v -run "TestAccAzureRMFirewall_inVirtualHub" -timeout 180m -ldflags="-X=github.com/terraform-provi
ders/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMFirewall_inVirtualHub                                                                                                                 === PAUSE TestAccAzureRMFirewall_inVirtualHub
=== CONT  TestAccAzureRMFirewall_inVirtualHub
--- PASS: TestAccAzureRMFirewall_inVirtualHub (2742.87s)
PASS                                 
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/network/tests       2742.907s

💤 make testacc TEST=./azurerm/internal/services/network/tests TESTARGS='-run "TestAccAzureRMFirewall_withFirewallPolicy|TestAccAzureRMFirewall_enableDNS"'
==> 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/tests -v -run "TestAccAzureRMFirewall_withFirewallPolicy|TestAccAzureRMFirewall_enableDNS" -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMFirewall_enableDNS
=== PAUSE TestAccAzureRMFirewall_enableDNS
=== RUN   TestAccAzureRMFirewall_withFirewallPolicy
=== PAUSE TestAccAzureRMFirewall_withFirewallPolicy
=== CONT  TestAccAzureRMFirewall_enableDNS
=== CONT  TestAccAzureRMFirewall_withFirewallPolicy
--- PASS: TestAccAzureRMFirewall_withFirewallPolicy (2013.98s)
--- PASS: TestAccAzureRMFirewall_enableDNS (3747.25s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/network/tests       3747.293s

manicminer · 2020-11-09T15:10:22Z

azurerm/internal/services/network/firewall_resource.go

+							Default:      1,
+						},
+						"public_ip_addresses": {
+							Type: schema.TypeList,


@magodo Is it possible to test scenarios involving the public_ip_addresses attribute? I'm curious what happens if you scale out the number of public IPs and only specify a subset of them. I also suspect if there are ordering issues, this attribute may need to be a set.

@manicminer The reason why this is a list is because it is a Computed attribute, a list is more user friendly. Regarding to test for public_ip_addresses, I guess we need to enhance the data source of the firewall resource so that we can retrieve the Computed public_ip_addresses and feed a subset of them to the resource for another apply?

@manicminer I added the test accordingly, please take another look!

@magodo thanks, that's good for the scaling in case, but can we also add a test for the scaling out case?

@manicminer I meant that I've added the scaling out case in the last commit here.

@magodo Thanks, I saw that. I'd describe that case as scaling in / down. The use case I'm interested in is scaling up / out and how that'll affect the public_ip_addresses attribute.

For example, say I have public_ip_count = 2 and public_ip_addresses = ["1.2.3.4", "1.2.3.5"], and then I increase public_ip_count = 4 and apply. Are the newly provisioned public IP addresses returned in the AzureFirewallPropertiesFormat.HubIPAddresses.PublicIPs.Addresses field? If so, what happens to the Terraform plan?

@manicminer Thank you for clarify that (i mistakely regard scaling out as scaling down...).

In this case, the first apply will take effect and then set the public_ip_addresses with 4 elements (adding two new ip addresses on top of the other two ips you've specified). The second terraform plan will show diff if the user is literally setting two elements in the config. However, in the scaling out/up use case, the user should not set that property at all, as I mentioned in the document. This property is mostly meant to be computed, and only useful in scale in/down use case.

@magodo Thanks for clarifying, that was my interpretation. In this case, the API is treating the field differently depending on how the value of public_ip_count is changing - essentially this should be two separate fields in the API. However, with it being one field having multiple behaviors, we can't really support this. Attributes that are only set for the duration of a single apply are going to produce inconsistent plans in pretty much any change scenario.

I suggest we simply make this field Computed (read-only), which will expose the allocated IPs for reference by the operator in other resources and I think is more important to enable.

@manicminer This makes total sense to me! I have made the changes to make public_ip_addresses to be computed only in the latest commit, please take another look!

katbyte · 2020-11-15T02:57:24Z

I believe this is blocked by a breaking change in the service? see #9312

tests are failing with:

        Error: creating Firewall Policy "acctest-networkfw-Policy-201114233159948085" (Resource Group "acctestRG-networkfw-201114233159948085"): network.FirewallPoliciesClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="FirewallPolicyDeprecatedRequireProxyForNetworkRules" Message="RequireProxyForNetworkRules ( DNS Settings' Property) in Firewall Policy /subscriptions/*******/resourceGroups/acctestRG-networkfw-201114233159948085/providers/Microsoft.Network/firewallPolicies/acctest-networkfw-Policy-201114233159948085 is deprecated for all the API Versions. Please remove the property and try again."

magodo · 2020-11-16T07:20:57Z

@katbyte The test failure is not related to the issue #9312 (though I created another PR to fix this issue), since the firewall test cases do not touch dns setting for firewall policy at all.

I submit another commit to fix the test failures.

💢 make testacc TEST=./azurerm/internal/services/network/tests TESTARGS='-run "TestAccAzureRMFirewall_withFirewallPolicy|TestAccAzureRMFirewall_inVirtualHub"'
==> 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/tests -v -run "TestAccAzureRMFirewall_withFirewallPolicy|TestAccAzureRMFirewall_inVirtualHub" -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMFirewall_withFirewallPolicy
=== PAUSE TestAccAzureRMFirewall_withFirewallPolicy
=== RUN   TestAccAzureRMFirewall_inVirtualHub
=== PAUSE TestAccAzureRMFirewall_inVirtualHub
=== CONT  TestAccAzureRMFirewall_withFirewallPolicy
=== CONT  TestAccAzureRMFirewall_inVirtualHub
--- PASS: TestAccAzureRMFirewall_withFirewallPolicy (1090.44s)
--- PASS: TestAccAzureRMFirewall_inVirtualHub (2831.62s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/network/tests       2831.662s

katbyte

Thanks @magodo - was able to get the tests to pass! LGTM 👍

ghost · 2020-11-20T01:45:58Z

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

woo-Erupt · 2020-12-11T15:45:12Z

Hi .. can you tell me when the "Add security to virtual networks" feature will be added for the Firewall Manager service?
The features required would be:
feature #1 "Create a new hub virtual network"
feature #2: "Convert virtual network"

ghost · 2020-12-18T14:50:00Z

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 👉 hashibot-feedback@hashicorp.com. Thanks!

ghost added size/XL documentation labels Oct 14, 2020

magodo added enhancement service/firewall labels Oct 14, 2020

jackofallops linked an issue Oct 15, 2020 that may be closed by this pull request

Support for Azure Firewall Manager resources #7319

Closed

jackofallops added this to the v2.33.0 milestone Oct 15, 2020

tombuildsstuff modified the milestones: v2.33.0, v2.34.0 Oct 22, 2020

jackofallops modified the milestones: v2.34.0, v.2.36.0 Oct 29, 2020

tombuildsstuff modified the milestones: v2.36.0, v2.35.0 Oct 30, 2020

magodo added 2 commits November 5, 2020 07:58

azurerm_firewall: support for firewall manager (firewall policy)

7595532

terrafmt

59e15f0

manicminer force-pushed the firewall_support_policy branch from f18fea3 to 59e15f0 Compare November 5, 2020 08:01

jackofallops modified the milestones: v2.35.0, v2.36.0 Nov 5, 2020

katbyte requested changes Nov 6, 2020

View reviewed changes

magodo added 2 commits November 6, 2020 12:53

update document, rename virtual_hub property

07db315

not set the Network.DNS.EnableProxy if users doesnt specify `dns_se…

78e6527

…rvers` Since API will raise errors when it is set as `false` in case firewall enables the firewall policy.

tesharp mentioned this pull request Nov 9, 2020

Support for Azure Firewall Manager resources #7319

Closed

manicminer reviewed Nov 9, 2020

View reviewed changes

katbyte added the waiting-response label Nov 9, 2020

Complement firewall ds and use it for scale down test

b98806e

magodo removed the waiting-response label Nov 11, 2020

jackofallops modified the milestones: v2.36.0, v2.37.0 Nov 12, 2020

disable virtual_hub.public_ip_addresses as argument to avoid confusion

ed31ba6

fix scale up/down issue caused by special API behavior

1b55d0a

katbyte approved these changes Nov 17, 2020

View reviewed changes

katbyte merged commit aa851d4 into hashicorp:master Nov 17, 2020

katbyte added a commit that referenced this pull request Nov 17, 2020

update CHANGELOG.md for #8879

86e5a61

katbyte mentioned this pull request Nov 17, 2020

Add sku to azurerm_firewall #9237

Closed

jackofallops mentioned this pull request Nov 19, 2020

Add SKU argument for azurerm_firewall #9145

Closed

ghost locked as resolved and limited conversation to collaborators Dec 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

azurerm_firewall: support for firewall manager (firewall policy) #8879

azurerm_firewall: support for firewall manager (firewall policy) #8879

magodo commented Oct 14, 2020

katbyte left a comment

katbyte Nov 5, 2020

magodo Nov 6, 2020

katbyte Nov 6, 2020

magodo Nov 6, 2020

magodo commented Nov 6, 2020

manicminer Nov 9, 2020

magodo Nov 11, 2020

magodo Nov 11, 2020

manicminer Nov 12, 2020

magodo Nov 13, 2020

manicminer Nov 13, 2020

magodo Nov 13, 2020

manicminer Nov 13, 2020

magodo Nov 13, 2020

katbyte commented Nov 15, 2020

magodo commented Nov 16, 2020

katbyte left a comment

ghost commented Nov 20, 2020

woo-Erupt commented Dec 11, 2020

ghost commented Dec 18, 2020


		* `public_ip_addresses` - (Optional) A list of public IP addresses associated with the Firewall.

		-> NOTE In most cases, users do not need to specify this argument, except when scaling down the public IPs (by changing the `public_ip_count`), this argument allows users to specify the existing IPs to be retained. Otherwise, the first existing `public_ip_count` public ip addresses will be retained.

azurerm_firewall: support for firewall manager (firewall policy) #8879

azurerm_firewall: support for firewall manager (firewall policy) #8879

Conversation

magodo commented Oct 14, 2020

Test Result

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magodo commented Nov 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte commented Nov 15, 2020

magodo commented Nov 16, 2020

katbyte left a comment

Choose a reason for hiding this comment

ghost commented Nov 20, 2020

woo-Erupt commented Dec 11, 2020

ghost commented Dec 18, 2020