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

add diagnostics settings for firewall, public IP, and network security groups #473

Merged
merged 7 commits into from
Oct 21, 2021

Conversation

glennmusa
Copy link
Contributor

@glennmusa glennmusa commented Oct 20, 2021

Description

Defaults the diagnostic logs and metrics settings for these resources:

Azure Firewall

  • AzureFirewallApplicationRule
  • AzureFirewallNetworkRule
  • AzureFirewallDnsProxy
  • AllMetrics

Public IP Address

  • DDoSProtectionNotifications
  • DDoSMitigationFlowLogs
  • DDoSMitigationReports
  • AllMetrics

Network Security Group

  • NetworkSecurityGroupEvent
  • NetworkSecurityGroupRuleCounter

Virtual Network

  • (user provided, empty otherwise)

Demo

To demo this, deploy MLZ as you usually would with az deployment sub create.

You can visually see these diagnostic settings by selecting "Diagnostic Settings" from any of the resource groups. For example, here's a Hub Resource Group Diagnostic Settings blade:

image

Issue reference

The issue this PR will close: #465

Checklist

Please make sure you've completed the relevant tasks for this PR out of the following list:

  • All acceptance criteria in the backlog item are met
  • The documentation is updated to cover any new or changed features
  • Manual tests have passed
  • Relevant issues are linked to this PR

@glennmusa glennmusa requested review from shawngib and a team October 20, 2021 18:59
@shawngib
Copy link
Member

Should we make 'metrics' optional since they add expense that may not be required. Should logs even be optional? I thought we were also removing the TF virtual network diagnostic logging in this also, maybe making it an option parameter,

@glennmusa
Copy link
Contributor Author

glennmusa commented Oct 21, 2021

Should we make 'metrics' optional since they add expense that may not be required.

I don't think logs should be optional, but can see how Metrics might be. If we decide to make Metrics optional, let's do that in a different change in tandem with optional metrics in Terraform so that the implementations are consistent.

I thought we were also removing the TF virtual network diagnostic logging in this also, maybe making it an option parameter

There are no logs or metrics configured with the virtual network diagnostic setting resource, but they're opt-in in that a user can provide their own categories if they choose by providing values for those parameters.

Copy link
Member

@shawngib shawngib left a comment

Choose a reason for hiding this comment

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

Works as designed.

@glennmusa glennmusa merged commit a41b051 into main Oct 21, 2021
@glennmusa glennmusa deleted the glenn/addDiagnosticsBicep branch October 21, 2021 14:22
@glennmusa
Copy link
Contributor Author

cool beans @shawngib set up another item to trickle down opt-in metrics: #475

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.

Create diagnostic settings module and incorporate into all modules
2 participants