new resource: azurerm_firewall_policy_rule_collection_group #8603

magodo · 2020-09-24T05:37:28Z

Test Result

💤 make testacc TEST=./azurerm/internal/services/network/tests TESTARGS='-run TestAccAzureRMFirewallPolicyRuleCollectionGroup'

==> 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 TestAccAzureRMFirewallPolicyRuleCollectionGroup -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMFirewallPolicyRuleCollectionGroup_basic
=== PAUSE TestAccAzureRMFirewallPolicyRuleCollectionGroup_basic
=== RUN   TestAccAzureRMFirewallPolicyRuleCollectionGroup_complete
=== PAUSE TestAccAzureRMFirewallPolicyRuleCollectionGroup_complete
=== RUN   TestAccAzureRMFirewallPolicyRuleCollectionGroup_update
=== PAUSE TestAccAzureRMFirewallPolicyRuleCollectionGroup_update
=== RUN   TestAccAzureRMFirewallPolicyRuleCollectionGroup_requiresImport
=== PAUSE TestAccAzureRMFirewallPolicyRuleCollectionGroup_requiresImport
=== CONT  TestAccAzureRMFirewallPolicyRuleCollectionGroup_basic
=== CONT  TestAccAzureRMFirewallPolicyRuleCollectionGroup_requiresImport
=== CONT  TestAccAzureRMFirewallPolicyRuleCollectionGroup_update
=== CONT  TestAccAzureRMFirewallPolicyRuleCollectionGroup_complete
--- PASS: TestAccAzureRMFirewallPolicyRuleCollectionGroup_basic (124.12s)
--- PASS: TestAccAzureRMFirewallPolicyRuleCollectionGroup_complete (124.13s)
--- PASS: TestAccAzureRMFirewallPolicyRuleCollectionGroup_requiresImport (134.30s)
--- PASS: TestAccAzureRMFirewallPolicyRuleCollectionGroup_update (216.12s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/network/tests       216.163s

Related issue: #7319

katbyte

Thanks for the PR @magodo - overall this is looking good and i've left some comments inline.

azurerm/internal/services/network/firewall_policy_rule_collection_group_data_source.go

azurerm/internal/services/network/firewall_policy_rule_collection_group_resource.go

website/docs/r/firewall_policy_rule_collection_group.html.markdown

magodo · 2020-10-12T06:23:07Z

@katbyte I have updated the code per your review comments, please take another look!

manicminer

Hi @magodo, this is looking great. I have one more suggestion and a couple questions below if you could help me understand some of the design decisions. Thanks!

manicminer · 2020-10-26T17:01:57Z

azurerm/internal/services/network/parse/firewall_policy.go

+	Name          string
+}
+
+func FirewallPolicyRuleCollectionGroupID(input string) (*FirewallPolicyRuleCollectionGroupId, error) {


Could you make this function use azure.ParseAzureResourceID rather than depending on FirewallPolicyID and a regex match?

manicminer · 2020-10-26T17:07:04Z

azurerm/internal/services/network/firewall_policy_rule_collection_group_resource.go

+		return err
+	}
+
+	policyResp, err := policyClient.Get(ctx, id.ResourceGroup, id.PolicyName, "")


Why do we need to check the existence of the linked policy before retrieving the rule collection group?

The reason is that we will need to use the policyResp to set the firewall_policy_id later. Instead, we should opt in to using the ID Formatter to save this API call.

manicminer · 2020-10-26T17:33:44Z

azurerm/internal/services/network/firewall_policy_rule_collection_group_resource.go

+											ValidateFunc: validation.StringIsNotEmpty,
+										},
+									},
+									"destination_address": {


Why do we accept a single address/CIDR when the API accepts a list?

The API actually doesn't support a list, it will raise error:

Invalid DNat destination address. Multiple values, ranges and * are not supported.

magodo · 2020-10-29T03:44:45Z

@manicminer I have updated per your commments, please take another look, thank you!

manicminer

@magodo Thanks, there's one other issue I noticed - can we store the subscription ID in the FirewallPolicyId and FirewallPolicyRuleCollectionGroupId structs, taking it from the original ID string, instead of passing it in afterwards?

magodo · 2020-10-30T01:17:49Z

@manicminer I assume you mean this part, this is actually how the Formatter interface defined, and is used prevalently elsewhere.

manicminer · 2020-10-30T16:28:23Z

@magodo Ah ok thanks. I still prefer the subscriptionId be sourced from the resource rather than from the client, but this looks good to merge 👍

manicminer · 2020-10-30T18:15:08Z

Test results:

magodo · 2020-11-02T01:44:56Z

@manicminer TBH, my first adoptions to the id structures imbedded the subscriptionId inside 😅

ghost · 2020-11-05T14:16:48Z

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

ghost · 2020-11-30T14: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 👉 [email protected]. Thanks!

ghost added size/XXL documentation labels Sep 24, 2020

magodo requested a review from WodansSon September 24, 2020 06:19

WodansSon added service/firewall new-data-source new-resource labels Sep 24, 2020

WodansSon added this to the v2.30.0 milestone Sep 24, 2020

new resource /data source: azurerm_firewall_policy_rule_collection_group

9e5627d

magodo force-pushed the firewall_policy_rule_collection_group branch from 3ffeee0 to 9e5627d Compare September 28, 2020 05:08

tombuildsstuff modified the milestones: v2.30.0, v2.31.0 Oct 1, 2020

katbyte requested changes Oct 2, 2020

View reviewed changes

katbyte added the waiting-response label Oct 2, 2020

tombuildsstuff modified the milestones: v2.31.0, v2.32.0 Oct 8, 2020

magodo added 2 commits October 12, 2020 13:28

remove data source azurerm_firewall_policy_rule_collection_group

d38128c

update per review

e581597

magodo removed the waiting-response label Oct 12, 2020

magodo added 4 commits October 14, 2020 14:25

format

4b3e7de

remove data source from registration

b3f3020

network rule add destination_fqdns && fix a delete bug

79cc061

make goimports

e70290e

jackofallops modified the milestones: v2.32.0, v2.33.0 Oct 15, 2020

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

manicminer suggested changes Oct 26, 2020

View reviewed changes

manicminer assigned magodo Oct 26, 2020

manicminer added the waiting-response label Oct 26, 2020

opt in ID formatter

35d3cba

magodo removed the waiting-response label Oct 29, 2020

manicminer suggested changes Oct 29, 2020

View reviewed changes

manicminer modified the milestones: v2.34.0, v2.35.0 Oct 29, 2020

manicminer approved these changes Oct 30, 2020

View reviewed changes

manicminer changed the title ~~new resource /data source: azurerm_firewall_policy_rule_collection_group~~ new resource: azurerm_firewall_policy_rule_collection_group Oct 30, 2020

manicminer merged commit 94561a6 into hashicorp:master Oct 30, 2020

manicminer added a commit that referenced this pull request Oct 30, 2020

Update changelog for #8603

fcccea5

ghost locked as resolved and limited conversation to collaborators Nov 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new resource: azurerm_firewall_policy_rule_collection_group #8603

new resource: azurerm_firewall_policy_rule_collection_group #8603

magodo commented Sep 24, 2020

katbyte left a comment

magodo commented Oct 12, 2020 •

edited

Loading

manicminer left a comment

manicminer Oct 26, 2020

magodo Oct 27, 2020

manicminer Oct 26, 2020

magodo Oct 27, 2020

manicminer Oct 26, 2020

magodo Oct 27, 2020

magodo commented Oct 29, 2020

manicminer left a comment

magodo commented Oct 30, 2020

manicminer commented Oct 30, 2020

manicminer commented Oct 30, 2020

magodo commented Nov 2, 2020

ghost commented Nov 5, 2020

ghost commented Nov 30, 2020

new resource: azurerm_firewall_policy_rule_collection_group #8603

new resource: azurerm_firewall_policy_rule_collection_group #8603

Conversation

magodo commented Sep 24, 2020

Test Result

katbyte left a comment

Choose a reason for hiding this comment

magodo commented Oct 12, 2020 • edited Loading

manicminer left a comment

Choose a reason for hiding this comment

manicminer Oct 26, 2020

Choose a reason for hiding this comment

magodo Oct 27, 2020

Choose a reason for hiding this comment

manicminer Oct 26, 2020

Choose a reason for hiding this comment

magodo Oct 27, 2020

Choose a reason for hiding this comment

manicminer Oct 26, 2020

Choose a reason for hiding this comment

magodo Oct 27, 2020

Choose a reason for hiding this comment

magodo commented Oct 29, 2020

manicminer left a comment

Choose a reason for hiding this comment

magodo commented Oct 30, 2020

manicminer commented Oct 30, 2020

manicminer commented Oct 30, 2020

magodo commented Nov 2, 2020

ghost commented Nov 5, 2020

ghost commented Nov 30, 2020

magodo commented Oct 12, 2020 •

edited

Loading