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

azfw quickstarts #245

Merged
merged 10 commits into from
Sep 4, 2023
Merged

azfw quickstarts #245

merged 10 commits into from
Sep 4, 2023

Conversation

cshea-msft
Copy link
Contributor

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

@stemaMSFT
Copy link
Member

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.

Copy link
Contributor

@grayzu grayzu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

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

quickstart/101-azfw-with-fwpolicy/main.tf Outdated Show resolved Hide resolved
quickstart/101-azfw-with-fwpolicy/main.tf Outdated Show resolved Hide resolved
quickstart/101-azfw-with-fwpolicy/main.tf Outdated Show resolved Hide resolved
quickstart/101-azfw-with-fwpolicy/main.tf Outdated Show resolved Hide resolved
quickstart/101-azfw-with-fwpolicy/main.tf Outdated Show resolved Hide resolved
quickstart/201-azfw-with-secure-hub/main.tf Outdated Show resolved Hide resolved
quickstart/201-azfw-with-secure-hub/main.tf Outdated Show resolved Hide resolved
quickstart/201-azfw-with-secure-hub/provider.tf Outdated Show resolved Hide resolved
provider "azurerm" {
features {
resource_group {
prevent_deletion_if_contains_resources = false // Set to True for Production
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to set this to false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can remove if needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

variable "admin_username" {
default = "azureuser"
}
variable "admin_password" {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be marked as sensitive. Just add

sensitive = true

after default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added senstive to the admin_password in the output.tf

Copy link
Collaborator

@TomArcherMsft TomArcherMsft left a comment

Choose a reason for hiding this comment

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

Added my comments.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
quickstart/101-azfw-with-fwpolicy/README.md Outdated Show resolved Hide resolved
quickstart/201-azfw-with-secure-hub/README.md Outdated Show resolved Hide resolved
| `tags` | tags to organize your resources |
| `fw_sku` | Sku size for your Firewall and Firewall Policy |

## Example
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

quickstart/201-azfw-with-secure-hub/variables.tf Outdated Show resolved Hide resolved
quickstart/201-azfw-with-secure-hub/variables.tf Outdated Show resolved Hide resolved
quickstart/101-azfw-with-fwpolicy/variables.tf Outdated Show resolved Hide resolved
quickstart/201-azfw-with-secure-hub/variables.tf Outdated Show resolved Hide resolved
variable "admin_username" {
default = "azureuser"
}
variable "admin_password" {
Copy link
Collaborator

@TomArcherMsft TomArcherMsft Aug 23, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@TomArcherMsft
Copy link
Collaborator

@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
Copy link
Contributor Author

@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!

@cshea-msft cshea-msft temporarily deployed to acctests August 30, 2023 21:03 — with GitHub Actions Inactive
@TomArcherMsft
Copy link
Collaborator

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

@cshea-msft cshea-msft temporarily deployed to acctests August 31, 2023 19:16 — with GitHub Actions Inactive
@TomArcherMsft
Copy link
Collaborator

@grayzu @lonegunmanb
cc: @cshea15

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

Copy link
Member

@lonegunmanb lonegunmanb left a comment

Choose a reason for hiding this comment

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

Thanks @cshea15, LGTM!

@lonegunmanb lonegunmanb merged commit 8aa0c51 into Azure:master Sep 4, 2023
@cshea-msft cshea-msft deleted the chashea-branch-1 branch September 5, 2023 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants