Azure / bicep-registry-modules

Bicep registry modules
MIT License
435 stars 287 forks source link

[AVM Module Issue]: Retention policy for diagnostic settings #1292

Closed StianSaeten closed 4 months ago

StianSaeten commented 4 months ago

Check for previous/existing GitHub issues

Issue Type?

Feature Request

Module Name

avm/res/key-vault/vault

(Optional) Module Name if not listed above

No response

(Optional) Module Version

No response

Description

It would be highly advantageous to have the ability to set the retentionPolicy on the diagnostic settings. I noticed that version 0.4.0 introduced more options for the diagnostic settings, however, it seems to lack support for retentionPolicy.

Is it possible to include this in the next update?

Add parameters to be able to set this in both logs and metrics

retentionPolicy: {
   days: int
   enabled: bool
}
resource symbolicname 'Microsoft.Insights/diagnosticSettings@2021-05-01-preview' = {
  name: 'string'
  scope: resourceSymbolicName
  properties: {
    eventHubAuthorizationRuleId: 'string'
    eventHubName: 'string'
    logAnalyticsDestinationType: 'string'
    logs: [
      {
        category: 'string'
        categoryGroup: 'string'
        enabled: bool
        retentionPolicy: {
          days: int
          enabled: bool
        }
      }
    ]
    marketplacePartnerId: 'string'
    metrics: [
      {
        category: 'string'
        enabled: bool
        retentionPolicy: {
          days: int
          enabled: bool
        }
        timeGrain: 'string'
      }
    ]
    serviceBusRuleId: 'string'
    storageAccountId: 'string'
    workspaceId: 'string'
  }
}

(Optional) Correlation Id

No response

github-actions[bot] commented 4 months ago

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

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

fblix commented 4 months ago

@AlexanderSehr I'd say this is more of a general request than a request specific to the keyvault module, right?

AlexanderSehr commented 4 months ago

It would be a general request - and I think it's technically not supported anymore. I recall we used to have the rentention policy and removed it back in CARML after it became deprecated. For reference: https://learn.microsoft.com/en-us/azure/azure-monitor/essentials/migrate-to-azure-storage-lifecycle-policy?tabs=templates .

@StianSaeten I would suggest to close this issue. As described in the reference, the rention policy should not be set on the target storage, not the source anymore. I recall it actually started failing at some point when trying to set the rentention policy.

The original issue in CARML was this one.

StianSaeten commented 4 months ago

@AlexanderSehr thank you for the reply. My main reason for the request was that I wanted to be able to get rid of some noise from the what if since the module is not setting days to 0.

 ~ properties.metrics: [
      ~ 0:
        - retentionPolicy.days: 0
   ]

I really appreciate your complementary answer, and then I'll just have to live with it or create our own module. (something I'd rather avoid, as we'd very much rather contribute to the AVM initiative. It's a very good initiative from you. 👍 )

AlexanderSehr commented 4 months ago

ll just have to live with it or create ou

Hm that's interesting. I would think this is a leftover by the change of implementation in the backend. I think the location where you could raise an issue for this is here: https://github.com/Azure/arm-template-whatif/issues . But please doublecheck. The correct link should show each time you run the WhatIf as they're asking for raising issues if any are noticed :) Thanks @StianSaeten 💪