[AVM Module Issue]: KeyVault Diagnostics defaults are over inclusive

[greg-double]

Check for previous/existing GitHub issues

• I have checked for previous/existing GitHub issues

Issue Type?

Bug

Module Name

avm/res/key-vault/vault

(Optional) Module Version

No response

Description

Module Link

When defining KV diagnostic settings, by default, the module is including data which wasn't requested. For example, the code below indicates I would like just Audit logs enabled as I have excluded the Metrics settings. However, the module defaults to 'All Metrics' if no Metric settings are provided.

    diagnosticSettings: [
      {
        name: 'Audit Logs - LogAnalytics'
        workspaceResourceId: logAnalytics.id
        logCategoriesAndGroups: [
          {
            categoryGroup: 'audit' 
            enabled: true
          }
        ]
      }
    ]

This is the current AVM module code...

      metrics: [
        for group in (diagnosticSetting.?metricCategories ?? [{ category: 'AllMetrics' }]): {
          category: group.category
          enabled: group.?enabled ?? true
          timeGrain: null
        }
      ]
      logs: [
        for group in (diagnosticSetting.?logCategoriesAndGroups ?? [{ categoryGroup: 'allLogs' }]): {
          categoryGroup: group.?categoryGroup
          category: group.?category
          enabled: group.?enabled ?? true
        }
      ]

In this example, I would think it would be more expected to have the Metrics excluded. The inverse is also true, if Metrics are defined and Logs excluded, you'll get all logs enabled,

With the current AVM code, unexpected log ingress and retention costs could result.

Obviously I can work around this by adding a Metrics array and explicitly disabling it, but I think we can do better.

    diagnosticSettings: [
      {
        name: 'Audit Logs - LogAnalytics'
        workspaceResourceId: logAnalytics.id
        logCategoriesAndGroups: [
          {
            categoryGroup: 'audit' 
            enabled: true
          }
        ]
        metricCategories: [
          {
            category: 'AllMetrics'
            enabled: false
          }
        ]
      }
    ]

(Optional) Correlation Id

No response

[microsoft-github-policy-service]

Important

The "Needs: Triage 🔍" label must be removed once the triage process is complete!

Tip

For additional guidance on how to triage this issue/PR, see the BRM Issue Triage documentation.

[avm-organizer]

@greg-double, thanks for submitting this issue for the avm/res/key-vault/vault module!

Important

A member of the @Azure/avm-res-keyvault-vault-module-owners-bicep team will review it soon!

[microsoft-github-policy-service]

Warning

Tagging the AVM Core Team (@Azure/avm-core-team-technical-bicep) due to a module owner or contributor having not responded to this issue within 3 business days. The AVM Core Team will attempt to contact the module owners/contributors directly.

Tip

• To prevent further actions to take effect, the "Status: Response Overdue 🚩" label must be removed, once this issue has been responded to.
• To avoid this rule being (re)triggered, the ""Needs: Triage 🔍" label must be removed as part of the triage process (when the issue is first responded to)!

[microsoft-github-policy-service]

Caution

This issue requires the AVM Core Team's (@Azure/avm-core-team-technical-bicep) immediate attention as it hasn't been responded to within 6 business days.

Tip

• To avoid this rule being (re)triggered, the "Needs: Triage 🔍" and "Status: Response Overdue 🚩" labels must be removed when the issue is first responded to!
• Remove the "Needs: Immediate Attention ‼️" label once the issue has been responded to.

[AlexanderSehr]

Hola @greg-double,
and thank you for the issue. I'll jump in here on behalf of the module owner as this is not a module-specific issue but the interface is the same across all AVM modules.
I guess the rationale behind the current implementation is relatively clear. As soon as you specify any diagnostic settings it assume you want to track everything (which is the most common use case I've seen).
If I'm not mistaken you should be able to not only avoid this by setting the setting to false as per your example (thank you btw), but also by specifying an empty list of metrics. For example:

diagnosticSettings: [
  {
    name: 'Audit Logs - LogAnalytics'
    workspaceResourceId: logAnalytics.id
    logCategoriesAndGroups: [
      {
        categoryGroup: 'audit' 
        enabled: true
      }
    ]
    metricCategories: []
  }
]

@Azure/avm-core-team-technical, what's your take on this? I guess we could update in the implementation so that it does not only check for logCategoriesAndGroups or metricCategories and then falls back to a default, but go one step further and check if the user specified anything in either or, deducting that they'd want to specifying everything themselves. Example

• Scenario 1: If I provide a workspaceResourceId and nothing else it will assume I want to collect everthing
• Scenario 2: If I provide workspaceResourceId and specify either logCategoriesAndGroups or metricCategories it will not assume any defaults anymore. I.e., in the example of @greg-double it would only collect the logs and ignore the metrics.
• Scenario 3: If provide workspaceResourceId and logCategoriesAndGroups and metricCategories it will collect both as per the user's specification