azurerm_monitor_diagnostic_setting does not remove enabled_log

almir0 commented 1 year ago

Is there an existing issue for this?

[X] I have searched the existing issues

Community Note

Please vote on this issue by adding a :thumbsup: reaction to the original issue to help the community and maintainers prioritize this request
Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
If you are interested in working on this issue or have submitted a pull request, please leave a comment and review the contribution guide to help.

Terraform Version

1.5.0

AzureRM Provider Version

3.72.0

Affected Resource(s)/Data Source(s)

azurerm_monitor_diagnostic_setting

Terraform Configuration Files

variable "name" {
  description = "Specifies the name of the Diagnostic Setting. Changing this forces a new resource to be created."
  type        = string
}

variable "resource_id" {
  type        = string
  description = "The ID of the resource on which activate the diagnostic settings."
}

variable "log_categories" {
  type        = list(string)
  default     = null
  description = "List of log categories. Defaults to all available."
}

variable "excluded_log_categories" {
  type        = list(string)
  default     = []
  description = "List of log categories to exclude."
}

variable "metric_categories" {
  type        = list(string)
  default     = null
  description = "List of metric categories. Defaults to all available."
}

variable "logs_destinations_ids" {
  type        = list(string)
  nullable    = false
  description = <<EOD
List of destination resources IDs for logs diagnostic destination.
Can be `Storage Account`, `Log Analytics Workspace` and `Event Hub`. No more than one of each can be set.
If you want to use Azure EventHub as destination, you must provide a formatted string with both the EventHub Namespace authorization send ID and the EventHub name (name of the queue to use in the Namespace) separated by the <code>&#124;</code> character.
EOD
}

variable "log_analytics_destination_type" {
  type        = string
  default     = "AzureDiagnostics"
  description = "When set to 'Dedicated' logs sent to a Log Analytics workspace will go into resource specific tables, instead of the legacy AzureDiagnostics table."
}

locals {
  enabled = length(var.log_categories) > 0 || length(var.metric_categories) > 0 ? true : false

  log_categories = [
    for log in
    (
      var.log_categories != null ?
      var.log_categories :
      try(data.azurerm_monitor_diagnostic_categories.main[0].log_category_types, [])
    ) : log if !contains(var.excluded_log_categories, log)
  ]

  metric_categories = (
    var.metric_categories != null ?
    var.metric_categories :
    try(data.azurerm_monitor_diagnostic_categories.main[0].metrics, [])
  )

  metrics = {
    for metric in try(data.azurerm_monitor_diagnostic_categories.main[0].metrics, []) : metric => {
      enabled = contains(local.metric_categories, metric)
    }
  }

  storage_id       = coalescelist([for r in var.logs_destinations_ids : r if contains(split("/", lower(r)), "microsoft.storage")], [null])[0]
  log_analytics_id = coalescelist([for r in var.logs_destinations_ids : r if contains(split("/", lower(r)), "microsoft.operationalinsights")], [null])[0]

  eventhub_authorization_rule_id = coalescelist([for r in var.logs_destinations_ids : split("|", r)[0] if contains(split("/", lower(r)), "microsoft.eventhub")], [null])[0]
  eventhub_name                  = coalescelist([for r in var.logs_destinations_ids : try(split("|", r)[1], null) if contains(split("/", lower(r)), "microsoft.eventhub")], [null])[0]

  log_analytics_destination_type = local.log_analytics_id != null ? var.log_analytics_destination_type : null
}

data "azurerm_monitor_diagnostic_categories" "main" {
  count = local.enabled ? 1 : 0

  resource_id = var.resource_id
}

resource "azurerm_monitor_diagnostic_setting" "main" {
  count = local.enabled ? 1 : 0

  name               = var.name
  target_resource_id = var.resource_id

  storage_account_id             = local.storage_id
  log_analytics_workspace_id     = local.log_analytics_id
  log_analytics_destination_type = local.log_analytics_destination_type
  eventhub_authorization_rule_id = local.eventhub_authorization_rule_id
  eventhub_name                  = local.eventhub_name

  dynamic "enabled_log" {
    for_each = local.log_categories

    content {
      category = enabled_log.value
    }
  }

  dynamic "metric" {
    for_each = local.metrics

    content {
      category = metric.key
      enabled  = metric.value.enabled
    }
  }

  lifecycle {
    ignore_changes = [log_analytics_destination_type]
  }
}

Debug Output/Panic Output

Apply complete! Resources: 0 added, 0 changed, 0 destoryed.

Expected Behaviour

Enable one or more log categories and one metric on any azure resource and apply this code and it will enable provided logs and metrics. Then disable all log categories by removing category name and provide null value and leave metrics be and apply again. The log categories should be removed.

Actual Behaviour

The log categories are not removed.

Steps to Reproduce

No response

Important Factoids

No response

References

No response

teowa commented 1 year ago

Hi @almir0 , this should be the Terraform config code issue. From the provided config,

locals {
  log_categories = [
    for log in
    (
      var.log_categories != null ?
      var.log_categories :
      try(data.azurerm_monitor_diagnostic_categories.main[0].log_category_types, [])
    ) : log if !contains(var.excluded_log_categories, log)
  ]
}

If we set var.log_categories as null, local.log_categories will be set as data.azurerm_monitor_diagnostic_categories.main[0].log_category_types, which means all log categories for the given resource type. So does not remove enabled_log is the expected behavior. From above code, if want remove enabled_log, we need to set var.log_categories to [], which means empty list.

Thanks.

almir0 commented 1 year ago

Thanks for reply, just tried this with much simpler example:

provider "azurerm" {
  features {
    key_vault {
      purge_soft_delete_on_destroy    = true
      recover_soft_deleted_key_vaults = false
    }
  }
}

resource "azurerm_resource_group" "example" {
  name     = "rg-diag-settings-test"
  location = "West Europe"
}

data "azurerm_client_config" "current" {}

resource "azurerm_key_vault" "example" {
  name                       = "keyvault0fr567"
  location                   = azurerm_resource_group.example.location
  resource_group_name        = azurerm_resource_group.example.name
  tenant_id                  = data.azurerm_client_config.current.tenant_id
  soft_delete_retention_days = 7
  purge_protection_enabled   = false
  sku_name                   = "standard"
}

resource "azurerm_monitor_diagnostic_setting" "example" {
  name                       = "example"
  target_resource_id         = azurerm_key_vault.example.id
  log_analytics_workspace_id = "/subscriptions/<subscriptionID>/resourceGroups/myRG/providers/Microsoft.OperationalInsights/workspaces/log-workspace"

  enabled_log {
    category = "AuditEvent"
  }

  metric {
    category = "AllMetrics"

    retention_policy {
      enabled = false
    }
  }
}

If I just remove enabled_log and run an apply and leave metrics as is no changes will be done but it is expected that it removes log categories. If I set category = [] I'm getting incorrect attribute since string is required. If I set category = null I'm getting Status=400 Code="BadRequest" Message="Category '' is not supported.

teowa commented 1 year ago

If I set category = null I'm getting Status=400 Code="BadRequest" Message="Category '' is not supported.

The provider is sending the category value directly to the Azure API, the the error is returned by Azure API. Usually in the provider we should not set null values. To remove the enabled_log, we should remove the whole block.

almir0 commented 1 year ago

If I set category = null I'm getting Status=400 Code="BadRequest" Message="Category '' is not supported.

The provider is sending the category value directly to the Azure API, the the error is returned by Azure API. Usually in the provider we should not set null values. To remove the enabled_log, we should remove the whole block.

If I'm getting you right this should remove the enabled_log which it does not.

resource "azurerm_monitor_diagnostic_setting" "example" {
  name                       = "example"
  target_resource_id         = azurerm_key_vault.example.id
  log_analytics_workspace_id = "/subscriptions/<subscriptionID>/resourceGroups/myRG/providers/Microsoft.OperationalInsights/workspaces/log-workspace"

#   enabled_log {
#     category = "AuditEvent"
#   }

  metric {
    category = "AllMetrics"

    retention_policy {
      enabled = false
    }
  }
}

teowa commented 1 year ago

Hi @almir0 , we are not supposed to set null value to a string field. I will enhance the validation for this. Please remove the enabled_log block to disable the log.

almir0 commented 1 year ago

Hi @almir0 , we are not supposed to set null value to a string field. I will enhance the validation for this. Please remove the enabled_log block to disable the log.

@teowa already tried, see above but unfortunately it does not disable the log.

teowa commented 1 year ago

@almir0 , apologize for misunderstanding, now I know the issue: remove all of the enabled_log, but no diff in terraform plan.

This is due to https://github.com/hashicorp/terraform-provider-azurerm/pull/19504 set the enabled_log as optional+computed field in 3.x version, which means if we don't set it in config, no diff will shown for this field. The computed will be removed in 4.0 version. Before that, the workaround is call terraform apply -replace=azurerm_monitor_diagnostic_setting.example to recreate the diagnostic setting or more complicated way is setting log with enabled=false to disable. After this is disabled the log can be removed.


    log {
        category = "AuditEvent"
        enabled  = false

        retention_policy {
            days    = 0
            enabled = false
        }
    }

almir0 commented 1 year ago

@teowa thanks for the clarification, I missed the computed part when replacing log with enabled_log, makes sense now. :)

BenjaminHerbert commented 11 months ago

@teowa I tried to use the -replace ... option, but receive this error:

 ╷
│ Error: at least one type of Log or Metric must be enabled
│ 
│   with azurerm_monitor_diagnostic_setting.log_analytics_workspace,
│   on diagnostic.tf line 1, in resource "azurerm_monitor_diagnostic_setting" "log_analytics_workspace":
│    1: resource "azurerm_monitor_diagnostic_setting" "log_analytics_workspace" {
│

It looks like I have to remove the resource if no log/metric is selected.

hashicorp / terraform-provider-azurerm