New Resources: `azurerm_firewall` & `azurerm_firewall_network_rule_collection` #1627

hbuckle · 2018-07-22T21:53:51Z

I'm opening this to get some feedback on the design - so far I've split network rules into a separate resource from the firewall to make them easier to manage.
This does have a couple of drawbacks, first it makes the code more complicated because there isn't a separate client for rules, everything has to be done through the AzureFirewalls client. Second it's slow, because all the rules have to be added or updated individually and one at a time.
The alternative is to implement the whole thing as one resource including rules as a property, but this could be harder to manage from the user's perspective.
Thoughts?

tombuildsstuff

hey @hbuckle

Thanks for this PR :)

I've taken a look through and left some comments inline but this is off to a good start. I'd agree this makes sense split out as sub-resources - whilst it takes a little longer to provision it's a better user experience; do you think it'd make sense to do the same thing with the Rule resource too? I've held off reviewing the docs/tests for the moment whilst we hash out the design, but this is looking good 👍

Thanks!

tombuildsstuff · 2018-07-23T05:09:40Z

azurerm/resource_arm_azure_firewall.go

+	"github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils"
+)
+
+var azureFirewallResourceName = "azurerm_azure_firewall"


we should be able to make this azurerm_firewall

tombuildsstuff · 2018-07-23T05:10:16Z

azurerm/resource_arm_azure_firewall.go

+			"ip_configuration": {
+				Type:     schema.TypeList,
+				Required: true,
+				MinItems: 1,


minor since this is Required - MinItems is inferred so we could remove this

tombuildsstuff · 2018-07-23T05:12:34Z

azurerm/resource_arm_azure_firewall.go

+	if resp.IPConfigurations != nil {
+		ipConfigs := flattenArmAzureFirewallIPConfigurations(resp.IPConfigurations)
+		d.Set("ip_configuration", ipConfigs)
+	}


so there's a couple of things here:

we should set this all the time (even if it's nil / when the API's broken) - since that allows changes to be detected

since ip_configurations is a complex object - we should check for errors when setting it

as such can we update this to be:

ipConfigs := flattenArmAzureFirewallIPConfigurations(resp.IPConfigurations) if err := d.Set("ip_configuration", ipConfigs); err != nil { return fmt.Errorf("Error setting `ip_configuration`: %+v", err) }

(we can then check if resp.IPConfigurations is nil within the flatten function, and return an empty list there if needed)

on a separate note - is IPConfigurations available within the [XX]Properties object? the API's generally return these in the Properties block, which would be better to use if it's an option

tombuildsstuff · 2018-07-23T05:21:46Z

azurerm/resource_arm_azure_firewall.go

+
+func flattenArmAzureFirewallIPConfigurations(ipConfigs *[]network.AzureFirewallIPConfiguration) []interface{} {
+	result := make([]interface{}, 0, len(*ipConfigs))
+	for _, ipConfig := range *ipConfigs {


there's a couple of potential crashes here; can we make this:

result := make([]interface{}, 0) if ipConfigs == nil { return result }

tombuildsstuff · 2018-07-23T05:22:35Z

azurerm/resource_arm_azure_firewall.go

+	result := make([]interface{}, 0, len(*ipConfigs))
+	for _, ipConfig := range *ipConfigs {
+		afIPConfig := make(map[string]interface{})
+		props := ipConfig.AzureFirewallIPConfigurationPropertiesFormat


given props can be nil here - I think we want a nil-check after this? e.g.

if props == nil { continue }

tombuildsstuff · 2018-07-23T08:13:07Z

azurerm/resource_arm_azure_firewall_network_rule_collection.go

+	collections := *firewall.AzureFirewallPropertiesFormat.NetworkRuleCollections
+	for i, collection := range collections {
+		if collection.Name != nil && *collection.Name == name {
+			index = i


I think it may also be possible to call delete(collections, collection) here, since we're breaking anyway?

tombuildsstuff · 2018-07-23T08:13:48Z

azurerm/resource_arm_azure_firewall_network_rule_collection.go

+		ruleToAdd := network.AzureFirewallNetworkRule{
+			Name: &name,
+		}
+		if description := rule["description"].(string); description != "" {


I think we're safe to assign this regardless (since if the key's nil in TF, we should reset the description?)

tombuildsstuff · 2018-07-23T08:14:21Z

azurerm/resource_arm_azure_firewall_network_rule_collection.go

+			ruleToAdd.Protocols = &protocols
+		}
+		rules = append(rules, ruleToAdd)
+	}


(same for all of these) if the lists are nil can we set them anyway, since this'll reset the values to an empty list as needed

tombuildsstuff · 2018-07-23T08:14:34Z

azurerm/resource_arm_azure_firewall_network_rule_collection.go

+	if rules != nil {
+		for _, rule := range *rules {
+			fwRule := make(map[string]interface{})
+			fwRule["name"] = *rule.Name


can we nil-check this?

tombuildsstuff · 2018-07-23T08:15:01Z

azurerm/resource_arm_azure_firewall_network_rule_collection.go

+func flattenArmAzureFirewallNetworkRules(rules *[]network.AzureFirewallNetworkRule) []map[string]interface{} {
+	result := make([]map[string]interface{}, 0)
+
+	if rules != nil {


minor we could probably invert this to make it more readable, but it's not a blocker by any means :)

hbuckle · 2018-07-23T08:51:25Z

Thanks for the review so far - I'll carry on and add the application rules as a third resource.
I did originally want to make rule collections and rules separate resources, unfortunately the API doesn't allow rule collections to be created without rules in them, so I don't think it will be possible.

tombuildsstuff

hey @hbuckle

Thanks for pushing those changes - I've taken a look through and reviewed the other half of this (Network Rule Collections). Regarding the other new resource, since this PR's already quite large / whilst it's related - would you mind if we pulled that out into a separate PR?

Thanks!

tombuildsstuff · 2018-07-24T10:19:46Z

azurerm/provider.go

@@ -138,6 +138,8 @@ func Provider() terraform.ResourceProvider {
 			"azurerm_automation_schedule":                     resourceArmAutomationSchedule(),
 			"azurerm_autoscale_setting":                       resourceArmAutoScaleSetting(),
 			"azurerm_availability_set":                        resourceArmAvailabilitySet(),
+			"azurerm_azure_firewall":                          resourceArmAzureFirewall(),
+			"azurerm_azure_firewall_network_rule_collection":  resourceArmAzureFirewallNetworkRuleCollection(),


can we make this azurerm_firewall and azurerm_firewall_network_rule respectively? I think the extra azure is redundant in each case?

Although it looks a little unwieldy Azure Firewall is the name of the product, so the naming is consistent. Lots of things have firewalls in Azure so I think this makes it clear exactly which product is being managed.

that's a fair point - but in general we ditch the word Azure from the front (e.g. Azure Kubernetes Service is azurerm_kubernetes_cluster / Azure Managed Database for MySQL becomes azurerm_mysql_server); so I think we should ditch it to be consistent with the other resources (the sole exception to this is azurerm_azuread, because that's how it's commonly referred too).

@tombuildsstuff OK, i've renamed everything to azurerm_firewall

tombuildsstuff · 2018-07-24T10:20:37Z

azurerm/resource_arm_azure_firewall.go

+	name := d.Get("name").(string)
+	location := azureRMNormalizeLocation(d.Get("location").(string))
+	tags := d.Get("tags").(map[string]interface{})
+	ipConfigs, subnetToLock, vnetToLock, sgErr := expandArmAzureFirewallIPConfigurations(d)


minor we can rename this from sgErr to err and reuse the variable below, rather than defining a specific error type

tombuildsstuff · 2018-07-24T10:22:12Z

azurerm/resource_arm_azure_firewall.go

+		return fmt.Errorf("Error making Read request on Azure Firewall %q (Resource Group %q): %+v", name, resourceGroup, err)
+	}
+
+	ipConfigs := flattenArmAzureFirewallIPConfigurations(firewall.AzureFirewallPropertiesFormat.IPConfigurations)


it's possible that firewall.AzureFirewallPropertiesFormat could be nil here, either through a bad API response or invalid Swagger in future; can we make this:

if props := firewall.AzureFirewallPropertiesFormat; props != nil { ipConfigs := flattenArmAzureFirewallIPConfigurations(firewall.AzureFirewallPropertiesFormat.IPConfigurations) if err := d.Set("ip_configuration", ipConfigs); err != nil { return fmt.Errorf("Error setting `ip_configuration`: %+v", err) } }

tombuildsstuff · 2018-07-24T10:32:03Z

azurerm/resource_arm_azure_firewall.go

+	return err
+}
+
+func expandArmAzureFirewallIPConfigurations(d *schema.ResourceData) ([]network.AzureFirewallIPConfiguration, *[]string, *[]string, error) {


we can return a pointer to a list here (*[]network.AzureFirewallIPConfiguration) which would allow us to return nil below where we currently return an empty list. the benefit of doing this is it means any unexpected code paths/accesses of this returned value would crash (which will be caught during testing)

tombuildsstuff · 2018-07-24T10:33:09Z

azurerm/resource_arm_azure_firewall.go

+			}
+		}
+
+		if ipAddress := ipConfig.PrivateIPAddress; ipAddress != nil {


can we access this via props instead of via ipConfig?

tombuildsstuff · 2018-07-24T11:16:50Z

azurerm/resource_arm_azure_firewall_network_rule_collection.go

+		ruleToAdd.Description = &description
+		ruleToAdd.SourceAddresses = expandArmAzureFirewallSet(sourceAddresses)
+		ruleToAdd.DestinationAddresses = expandArmAzureFirewallSet(destinationAddresses)
+		ruleToAdd.DestinationPorts = expandArmAzureFirewallSet(destinationPorts)


minor can we in-line these? (also we can use utils.String(val) rather than & for consistency with the other resources) e.g.

ruleToAdd := network.AzureFirewallNetworkRule{ Name: &name, Description: utils.String(description), SourceAddresses: expandArmAzureFirewallSet(sourceAddresses), DestinationAddresses: expandArmAzureFirewallSet(destinationAddresses), DestinationPorts: expandArmAzureFirewallSet(destinationPorts), }

tombuildsstuff · 2018-07-24T11:18:48Z

website/docs/r/azure_firewall.html.markdown

+page_title: "Azure Resource Manager: azurerm_azure_firewall"
+sidebar_current: "docs-azurerm-resource-azurefirewall-x"
+description: |-
+  Create an Azure Firewall.


minor can we make this Manages an Azure Firewall

tombuildsstuff · 2018-07-24T11:19:07Z

website/docs/r/azure_firewall.html.markdown

+
+Create an Azure Firewall.
+
+~> **NOTE** This resource is currently in public preview.


is this in invite-only preview, or can customers opt-in?

tombuildsstuff · 2018-07-24T11:20:00Z

website/docs/r/azure_firewall.html.markdown

+
+* `name` - (Required) Specifies the name of the ip configuration.
+* `subnet_id` - (Required) Reference to the subnet associated with the ip configuration.
+* `internal_public_ip_address_id` - (Required) Reference to the public IP address associated with the firewall.


can we add the above note below this resource?

~> **NOTE** The public IP must have a `Static` allocation and `Standard` sku

tombuildsstuff · 2018-07-24T11:20:01Z

website/docs/r/azure_firewall.html.markdown

+`ip_configuration` supports the following:
+
+* `name` - (Required) Specifies the name of the ip configuration.
+* `subnet_id` - (Required) Reference to the subnet associated with the ip configuration.


can we add the above note below this resource?

~> **NOTE** The firewall subnet must be called `AzureFirewallSubnet` and the subnet mask must be at least `/25`

hbuckle · 2018-07-25T07:39:45Z

@tombuildsstuff Thanks, think I have fixed everything up. I'll review all the tests today then look at a separate PR to add application rule collections

tombuildsstuff · 2018-08-09T09:41:39Z

@hbuckle hey sorry for the delayed re-review here - I'll take another look through this today

dismissing since I've pushed changes

- Locking on the Firewall Name - Handling resources being deleted outside of Terraform - Removing some crash points - Making the Protocol and Action type case-sensitive - Refactoring the virtual resource to allow for - Parsing the ID rather than using the config for the delete and read functions (so delete's are successful when the config's gone) - Rewriting some of the tests for the Network Rule Collections, to check the resource's state rather than the object - Updating the documentation (and including Import support for Network Rule Collections)

tombuildsstuff · 2018-09-12T16:48:00Z

@hbuckle I thought I'd posted this message yesterday - apologies.

I ended up rebasing this yesterday and taking a look into what's needed to get this merged - and I've pushed a commit with some changes to finish this off (I hope you don't mind!) - in particular I've updated the Read and Delete functions to parse the ID of the Network Rule Collection rather than reading from the config (since this isn't always present during Import/Delete's) and updated some of the tests.

Since I've pushed some larger changes here (since this is a couple of resources) - I'm going to as @katbyte to take a look into this - but I'm going to kick off the tests now.

Thanks!

katbyte

@hbuckle, @tombuildsstuff,

This pretty much LGTM 👍 All the comments I have left inline are pretty minor and not blockers so i'm going to approve this.

My main concern is TestAccAzureRMFirewall_importBasic being separate, while the rule collection is part of the standard tests. I think it should be merged in to the standard TestAccAzureRMFirewall tests as well.

azurerm/import_arm_azure_firewall_test.go

azurerm/resource_arm_firewall.go

azurerm/resource_arm_firewall_network_rule_collection.go

website/docs/r/firewall_network_rule_collection.html.markdown

azurerm/helpers/azure/firewall.go

``` $ acctests azurerm TestValidateFirewallName === RUN TestValidateFirewallName --- PASS: TestValidateFirewallName (0.00s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 0.344s ```

hbuckle · 2018-09-13T08:39:22Z

@tombuildsstuff Thanks for the update, I'm sure my go skills still leave something to be desired. I'll have another look through this today. I've started the application rule collection on another branch as well.
The publicipaddress vs internalpublicipaddress has been resolved in the specs as well so we should be able to remove the workaround for that once it gets into the SDK

hbuckle · 2018-09-13T08:39:48Z

@tombuildsstuff Thanks for the update, I'm sure my go skills still leave something to be desired. I'll have another look through this today. I've started the application rule collection on another branch as well.
The publicipaddress vs internalpublicipaddress has been resolved in the specs as well so we should be able to remove the workaround for that once it gets into the SDK

tombuildsstuff · 2018-09-13T10:27:46Z

@tombuildsstuff Thanks for the update, I'm sure my go skills still leave something to be desired. I'll have another look through this today.

Not at all, thanks for getting this most of the way there - apologies this sat for a while, we're still trying to work through the open PR's.

I've started the application rule collection on another branch as well.

That's awesome 👍 - thanks!

The publicipaddress vs internalpublicipaddress has been resolved in the specs as well so we should be able to remove the workaround for that once it gets into the SDK

Cool, that's good to know - I'll keep an eye on the SDK release and add a TODO for that

Thanks!

…collection

tombuildsstuff · 2018-09-13T20:46:49Z

Tests pass:

ghost · 2019-03-06T14:35:39Z

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!

tombuildsstuff added new-resource service/firewall labels Jul 23, 2018

tombuildsstuff self-assigned this Jul 23, 2018

tombuildsstuff previously requested changes Jul 23, 2018

View reviewed changes

tombuildsstuff reviewed Jul 24, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Jul 24, 2018

tombuildsstuff removed the waiting-response label Aug 10, 2018

hbuckle added 20 commits September 12, 2018 18:38

add firewall resource skeleton

0029c47

basic test

62944bf

create method

424a331

complete firewall resource

e442d60

begin network rule resource

d545e06

add firewall import

60b2619

fix firewall tests

d29457a

tweak firewall tests

69da76a

complete network rule collection

0a47400

start docs

fc348ec

nil checks

0ddd61b

update nil checks

bd5ef37

update find rule collection

a5fb666

formatting updates

3d09b99

docs

2c36ce6

rename files

af65621

rename arm_azure_firewall to arm_firewall

faa331a

formatting

325fedb

rename doc

1a94461

rename nrc doc

89b08ab

ghost added the size/XXL label Sep 12, 2018

tombuildsstuff changed the title ~~[WIP] Azure Firewall~~ Azure Firewall Sep 12, 2018

tombuildsstuff force-pushed the azure_firewall branch from d289b1e to a25e3d1 Compare September 12, 2018 16:45

ghost added the size/XXL label Sep 12, 2018

tombuildsstuff requested a review from katbyte September 12, 2018 16:48

tombuildsstuff added this to the 1.15.0 milestone Sep 12, 2018

tombuildsstuff changed the title ~~Azure Firewall~~ New Resources: azurerm_firewall & azurerm_firewall_network_rule_collection Sep 12, 2018

katbyte approved these changes Sep 12, 2018

View reviewed changes

tombuildsstuff added 3 commits September 13, 2018 09:09

Deleting the separate test file

2faa078

Adding validation to fields

ca7ad08

``` $ acctests azurerm TestValidateFirewallName === RUN TestValidateFirewallName --- PASS: TestValidateFirewallName (0.00s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 0.344s ```

Renaming the fix ip method

81bfbf6

ghost added the size/XXL label Sep 13, 2018

Fixing issues highlighted in code review

8167fc7

tombuildsstuff force-pushed the azure_firewall branch from 3d16742 to 8167fc7 Compare September 13, 2018 10:49

ghost added the size/XXL label Sep 13, 2018

tombuildsstuff added 3 commits September 13, 2018 13:12

Fixing the test check function to load the firewall, rather than the …

dbcd251

…collection

Fixing a bad refactor

57271fb

Fixing a broken test

18948fe

Docs: adding a sidebar link

947ae81

tombuildsstuff merged commit 08ef14e into hashicorp:master Sep 13, 2018

tombuildsstuff added a commit that referenced this pull request Sep 13, 2018

Updating to include #1627

91afd3d

ghost locked and limited conversation to collaborators Mar 6, 2019


		Create an Azure Firewall.

		~> NOTE This resource is currently in public preview.

New Resources: azurerm_firewall & azurerm_firewall_network_rule_collection #1627

New Resources: azurerm_firewall & azurerm_firewall_network_rule_collection #1627

Conversation

hbuckle commented Jul 22, 2018

tombuildsstuff 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

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

hbuckle commented Jul 23, 2018

tombuildsstuff 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

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

hbuckle commented Jul 25, 2018

tombuildsstuff commented Aug 9, 2018

tombuildsstuff commented Sep 12, 2018

katbyte left a comment

Choose a reason for hiding this comment

hbuckle commented Sep 13, 2018

hbuckle commented Sep 13, 2018

tombuildsstuff commented Sep 13, 2018 • edited Loading

tombuildsstuff commented Sep 13, 2018

ghost commented Mar 6, 2019

New Resources: `azurerm_firewall` & `azurerm_firewall_network_rule_collection` #1627

New Resources: `azurerm_firewall` & `azurerm_firewall_network_rule_collection` #1627

tombuildsstuff commented Sep 13, 2018 •

edited

Loading