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

Updates for Azure Diags to support CategoryGroups #40

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

JimGBritt
Copy link
Owner

No description provided.

@SenthuranSivananthan
Copy link

SenthuranSivananthan commented Nov 15, 2021

@JimGBritt Looks like with Category Groups, all categories need to be selected for a policy to be considered compliant. Steps to reproduce:

  1. Created an Automation Account, generated policy via Create-AzDiagPolicy.ps1 and deployed.
  2. Create a new Automation Account and wait for DINE effect
  3. Run a compliance scan via az policy state trigger-scan --subscription sub_id

Observations:

Looks like for Automation Account, both allLogs and audit needs to be selected.

Automation Account Diagnostic Settings
image

Policy compliance failure reason
image

image


Policy is only compliant when both category groups are selected.

image

image

@JimGBritt
Copy link
Owner Author

Will investigate. This is the shift from categories (logs) to CategoryGroups (all logs). The compliance state has to do with that. Thanks for raising.

@JimGBritt
Copy link
Owner Author

Update - logic fixed and will be making an update to the PR once some other pieces are remediated in the platform. Huge thanks for leaning in and raising this initially so quickly.

@JimGBritt
Copy link
Owner Author

One other bug will be fixed which has to do with the addition of the proxy resources for Storage - on export they are not deduplicated, so we end up with multiples. This is something in the logic due to how the others are dynamically checked and storage proxies are manually added. Will be fixed on the next update of PR.

@JimGBritt
Copy link
Owner Author

One other bug will be fixed which has to do with the addition of the proxy resources for Storage - on export they are not deduplicated, so we end up with multiples. This is something in the logic due to how the others are dynamically checked and storage proxies are manually added. Will be fixed on the next update of PR.

This has been resolved in the latest update to the PR. Still pending merging till things are ready on the backend.

@SenthuranSivananthan
Copy link

My tests all passed @JimGBritt. We should add a note that moving from Category to Category Groups requires existing diag. settings to be updated, otherwise resources will show as non-compliant.

My scenarios:

  1. Existing Automation Account w/ diag settings turned on using categories (the old model). Outcome: Azure Policy compliance marked the resource non-compliant (stated that it's expecting 1 element in the array, but had 4. This is because Azure Automation has 4 log categories but we now just want 1 categoryGroup for 'allLogs'). I ran the remediation task and it updated the existing diag setting config.

When customers move to allLogs category group, there is a mass remediation that needs to take place to realign config w/ policy conditions.

  1. Existing Automation Account w/o diag settings. Outcome: Diag settings were added with allLogs.

  2. New Automation Account. Outcome: Diag settings were added with allLogs.

@SenthuranSivananthan
Copy link

SenthuranSivananthan commented Dec 17, 2021

@JimGBritt, I think we found another scenario where allLogs may have an issue: Diagnostic Settings for Recovery Service Vault (RSV).

RSV is used for Backup and Site Recovery. For Backup, the logs need to go to a Resource specific table, but for Site Recovery, it needs to go to Azure diagnostics table. The documentation asks that we create 2 Diagnostic Settings, one for each and select only the log categories that are appropriate for each service.

Reference: https://docs.microsoft.com/en-us/azure/backup/backup-azure-diagnostic-events

Credit: @ghostme for finding this discrepancy.

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.

2 participants