azfw quickstarts #245

cshea-msft · 2023-08-22T23:25:05Z

Created two quickstarts for Azure Firewall.

Deploy Azure Firewall with Azure Policy
Deploy Azure Firewall with Azure Policy in a Secure Hub

The plan is to add these two quickstarts to the Firewall Manager docs. https://learn.microsoft.com/en-us/azure/firewall-manager/quick-secure-virtual-hub-bicep?tabs=CLI

adding azfw quickstarts

stemaMSFT · 2023-08-23T00:37:57Z

The README.md here seems like a copy paste of the main.tf file, is there something more akin to the Bicep/ARM Template quickstarts? https://learn.microsoft.com/en-us/azure/firewall-manager/quick-secure-virtual-hub-bicep?tabs=CLI
Or was there a different intention for the README? For the code stuff, I think it looks fine, but I'll defer to @lonegunmanb to review and approve before kicking off E2E tests.

grayzu

Thanks for the PR!

Looks great. I have a few minor comments and questions.

quickstart/101-azfw-with-fwpolicy/main.tf

quickstart/201-azfw-with-secure-hub/main.tf

quickstart/201-azfw-with-secure-hub/provider.tf

grayzu · 2023-08-23T01:21:18Z

quickstart/201-azfw-with-secure-hub/provider.tf

+provider "azurerm" {
+  features {
+    resource_group {
+      prevent_deletion_if_contains_resources = false // Set to True for Production


Is there a reason to set this to false?

since this is quickstart its mainly used for testing so if they want to destroy and tear it down this will delete everything. I have run into issues in the past where some resources is hanging around and doesnt delete the resource group so this at least forces it all to be removed and customers dont have resources hanging around.

can remove if needed.

You're using a pattern where you're specifying a flag at the RG level to delete everything in the RG should the encompassed entity not automatically delete. However, many times, a lower-level entity has its own flag for this purpose. For example, let's say you have a VM with a disk. instead of using prevent_deletion_if_contains_resources at the RG level, you can use delete_data_disks_on_termination at the VM level, which deletes the disk when the VM is deleted. The Terraform PG prefers this latter pattern.

got it thanks Tom, i updated it with the virtual machine feature.

grayzu · 2023-08-23T01:24:44Z

quickstart/201-azfw-with-secure-hub/variables.tf

+variable "admin_username" {
+  default = "azureuser"
+}
+variable "admin_password" {


This should be marked as sensitive. Just add

sensitive = true

after default

added senstive to the admin_password in the output.tf

TomArcherMsft

Added my comments.

TomArcherMsft · 2023-08-23T10:09:30Z

quickstart/101-azfw-with-fwpolicy/README.md

@@ -0,0 +1,228 @@
+# Deploy Azure Firewall and a Firewall Policy


README.md file name needs to be renamed to lowercase.

I did that and it broke it. There seems to be no consistency with other quickstarts since some are lower case and some are upper case.

Yes, there is inconsistency as many samples were created before we started defining a standard for the samples. All new samples have a much higher bar than previous samples. However, we don't have the resources to go back and retroactively update every sample. Instead, we enforce the standards on new samples and when we need to fix older samples, we apply the standards then. The lack of consistency across the repo is why I referred to specific examples for you to follow in my comments.

quickstart/201-azfw-with-secure-hub/README.md

quickstart/101-azfw-with-fwpolicy/README.md

quickstart/201-azfw-with-secure-hub/README.md

TomArcherMsft · 2023-08-23T10:18:15Z

quickstart/101-azfw-with-fwpolicy/README.md

+| `tags`     | tags to organize your resources                |
+| `fw_sku`   | Sku size for your Firewall and Firewall Policy |
+
+## Example


Please remove the plan output from this section. This section should just point to the article. See https://github.com/Azure/terraform/tree/master/quickstart/101-front-door-standard-premium. If you don't know the title and URL of the article, I'll update those values when I generate the article from this sample code.

removed code. Doc is not ready yet. Can update once doc is published.

quickstart/201-azfw-with-secure-hub/variables.tf

quickstart/101-azfw-with-fwpolicy/variables.tf

quickstart/201-azfw-with-secure-hub/variables.tf

TomArcherMsft · 2023-08-23T11:52:34Z

quickstart/201-azfw-with-secure-hub/variables.tf

+variable "admin_username" {
+  default = "azureuser"
+}
+variable "admin_password" {


To maintain consistency with the sample showing how to create a Windows VM, please generate the password as in the Windows VM creation article. Remember to output the randomly generated password.

I updated the generate password to the main.tf and output.tf password for each vm following the docs. Please let me know if I need to fix it.

cshea-msft · 2023-08-28T00:34:42Z

README.md file name needs to be renamed to lowercase.

Happy to change it but it seems there is no consistency when looking at the different quickstarts.

cshea-msft · 2023-08-28T00:37:29Z

Please remove the plan output from this section. This section should just point to the article. See https://github.com/Azure/terraform/tree/master/quickstart/101-front-door-standard-premium. If you don't know the title and URL of the article, I'll update those values when I generate the article from this sample code.

This needs to be updated in other quickstarts because the plan output is in other readme's

cshea-msft · 2023-08-28T14:44:28Z

The README.md here seems like a copy paste of the main.tf file, is there something more akin to the Bicep/ARM Template quickstarts? https://learn.microsoft.com/en-us/azure/firewall-manager/quick-secure-virtual-hub-bicep?tabs=CLI Or was there a different intention for the README? For the code stuff, I think it looks fine, but I'll defer to @lonegunmanb to review and approve before kicking off E2E tests.

I was following the FIrewall terraform quickstart that had the plan output in the README. I removed the output from the README.

cshea-msft · 2023-08-30T18:55:06Z

@grayzu @TomArcherMsft Just checking in to see if there are any more changes are needed. Thanks!

TomArcherMsft · 2023-08-30T18:59:22Z

@grayzu @TomArcherMsft Just checking in to see if there are any more changes are needed. Thanks!

Hi, @cshea15. I wasn't aware that you were ready for another review. I'll do that either today or tomorrow. Thanks!

cshea-msft · 2023-08-30T19:03:01Z

@grayzu @TomArcherMsft Just checking in to see if there are any more changes are needed. Thanks!

Hi, @cshea15. I wasn't aware that you were ready for another review. I'll do that either today or tomorrow. Thanks!

no problem, I reviewed all the comments, made the changes, and added comments. Let me know if there is anything I missed. Thanks again!

TomArcherMsft · 2023-08-31T17:29:13Z

@cshea15 Reviewing now. Please do not push any new commits until I'm done. Thanks!

TomArcherMsft · 2023-08-31T23:21:10Z

@grayzu @lonegunmanb
cc: @cshea15

I've completed my review. Could you please review & merge when you get a chance? Thanks!

lonegunmanb

Thanks @cshea15, LGTM!

cshea-msft and others added 3 commits August 22, 2023 19:02

adding azfw quickstarts

ed29f3c

Merge pull request #1 from cshea15/master

9e2da22

adding azfw quickstarts

UPDATE format

a79609c

grayzu assigned lonegunmanb and stemaMSFT Aug 23, 2023

grayzu requested changes Aug 23, 2023

View reviewed changes

TomArcherMsft requested changes Aug 23, 2023

View reviewed changes

cshea-msft and others added 5 commits August 28, 2023 11:11

updated files with feedback from pr

85c7f84

made some changes to clean up deployment

1cadacf

changing the provider feature

39f24e4

updated rg prefixes and removed commented lines

4f1a6c1

fixed output

79fa9bd

cshea-msft temporarily deployed to acctests August 30, 2023 21:03 — with GitHub Actions Inactive

cshea-msft requested review from grayzu and TomArcherMsft August 31, 2023 15:21

cshea-msft added 2 commits August 31, 2023 14:20

fixing mistakes

010c19f

fixed the readme's

23a7bf1

cshea-msft temporarily deployed to acctests August 31, 2023 19:16 — with GitHub Actions Inactive

lonegunmanb approved these changes Sep 4, 2023

View reviewed changes

lonegunmanb merged commit 8aa0c51 into Azure:master Sep 4, 2023
3 checks passed

cshea-msft deleted the chashea-branch-1 branch September 5, 2023 00:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

azfw quickstarts #245

azfw quickstarts #245

cshea-msft commented Aug 22, 2023

stemaMSFT commented Aug 23, 2023

grayzu left a comment

grayzu Aug 23, 2023

cshea-msft Aug 28, 2023

cshea-msft Aug 28, 2023

TomArcherMsft Aug 28, 2023

cshea-msft Aug 28, 2023

grayzu Aug 23, 2023

cshea-msft Aug 28, 2023

TomArcherMsft left a comment

TomArcherMsft Aug 23, 2023

cshea-msft Aug 28, 2023

TomArcherMsft Aug 28, 2023

TomArcherMsft Aug 23, 2023

cshea-msft Aug 28, 2023

TomArcherMsft Aug 23, 2023 •

edited

Loading

cshea-msft Aug 28, 2023

cshea-msft commented Aug 28, 2023

cshea-msft commented Aug 28, 2023

cshea-msft commented Aug 28, 2023

cshea-msft commented Aug 30, 2023

TomArcherMsft commented Aug 30, 2023

cshea-msft commented Aug 30, 2023

TomArcherMsft commented Aug 31, 2023

TomArcherMsft commented Aug 31, 2023

lonegunmanb left a comment

		@@ -0,0 +1,228 @@
		# Deploy Azure Firewall and a Firewall Policy

azfw quickstarts #245

azfw quickstarts #245

Conversation

cshea-msft commented Aug 22, 2023

stemaMSFT commented Aug 23, 2023

grayzu 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

TomArcherMsft 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

TomArcherMsft Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cshea-msft commented Aug 28, 2023

cshea-msft commented Aug 28, 2023

cshea-msft commented Aug 28, 2023

cshea-msft commented Aug 30, 2023

TomArcherMsft commented Aug 30, 2023

cshea-msft commented Aug 30, 2023

TomArcherMsft commented Aug 31, 2023

TomArcherMsft commented Aug 31, 2023

lonegunmanb left a comment

Choose a reason for hiding this comment

TomArcherMsft Aug 23, 2023 •

edited

Loading