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

Diagnostic Setting fails to apply if logAnalyticsDestinationType is set #20019

Closed
1 task done
cpressland opened this issue Jan 14, 2023 · 15 comments · Fixed by #20048
Closed
1 task done

Diagnostic Setting fails to apply if logAnalyticsDestinationType is set #20019

cpressland opened this issue Jan 14, 2023 · 15 comments · Fixed by #20048
Labels

Comments

@cpressland
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Community Note

  • Please vote on this issue by adding a 👍 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

Terraform Version

1.3.7

AzureRM Provider Version

3.39.1

Affected Resource(s)/Data Source(s)

azurerm_monitor_diagnostic_setting

Terraform Configuration Files

resource "azurerm_monitor_diagnostic_setting" "i" {
    name = "diags"
    target_resource_id = azurerm_key_vault.i.id
    log_analytics_workspace_id = var.loganalytics_id

    enabled_log {
        category = "AuditEvent"
        retention_policy {
            days = 0
            enabled = false
        }
    }
}

Debug Output/Panic Output

During Plan:
  # module.uksouth_sandbox_environment.azurerm_monitor_diagnostic_setting.add_kv["bink-uksouth-retail"] will be updated in-place
  ~ resource "azurerm_monitor_diagnostic_setting" "add_kv" {
        id                             = "<snipped>"
      - log_analytics_destination_type = "AzureDiagnostics" -> null
        name                           = "diags"
        # (2 unchanged attributes hidden)

        # (4 unchanged blocks hidden)
    }


During Apply:
│ Error: at least one type of Log or Metric must be enabled
│ 
│   with module.uksouth_sandbox_environment.azurerm_monitor_diagnostic_setting.add_kv["bink-uksouth-retail"],
│   on ../tf-azurerm_environment/keyvault_additional.tf line 78, in resource "azurerm_monitor_diagnostic_setting" "add_kv":
│   78: resource "azurerm_monitor_diagnostic_setting" "add_kv" {
│

Expected Behaviour

No change should be applied as the default IS AzureDiagnostics according to Documentation here

Actual Behaviour

We have a single diagnostic setting which has "logAnalyticsDestinationType": "AzureDiagnostics" set, while all other Diagnostics Settings have "logAnalyticsDestinationType": null.

AzureRM Provider appears to be trying to set "AzureDiagnostics" to null during a plan/apply, but the Azure API appears to be rejecting this.

I have destroyed other diagnostics settings, upon recreating them they always get set with "AzureDiagnostics" instead of null, this suggests an API change within Azure that the provider needs to handle better.

Steps to Reproduce

  1. Create a Diagnostic Setting via Terraform
  2. terraform plan on the same Diagnostic Setting

Important Factoids

No response

References

No response

@cpressland cpressland added the bug label Jan 14, 2023
@github-actions github-actions bot removed the bug label Jan 14, 2023
@MoeTopaji
Copy link

Not sure what caused this behaviour but deleting the diagnostic settings on the portal and re-running the pipeline fixed it for me. I re-ran the pipeline several times after that and no issues

@cpressland
Copy link
Author

I’ll try deleting a few diagnostics settings this morning - I can confirm they create just fine, but then on a second run they all want to be updated.

@teowa
Copy link
Contributor

teowa commented Jan 16, 2023

Hi @cpressland , thanks for submitting issue!
About the log_analytics_destination_type, recently service has changed API behaviour, returns the log_analytics_destination_type: AzureDiagnostics even if we don't set it in API request. But this should not fail the apply process. I will try to amend the code to suppress the plan diff.

From the provided error message Error: at least one type of Log or Metric must be enabled, the apply failure should caused by lack of an enabled log or metrics. Please check the add_kv diagnostic setting.

@cpressland
Copy link
Author

cpressland commented Jan 16, 2023

@teowa thanks for the reply, this is happening on 100% of our diagnostics settings, even ones that were written over four years ago with very complex log setups. Of course, the issue only occurs after we taint or manually delete the setting via the Portal. It gets created just fine, with all of the correct settings, and then fails to reconcile on future runs.

I plan to handle the removal of log {} and replace with enabled_log {} after this, but I can confirm that this issue happens on both types of diagnostics settings for now.

@cpressland
Copy link
Author

So this is fun, tainting and replacing Diagnostic Settings gives me a completely random chance of getting "logAnalyticsDestinationType": "AzureDiagnostics" or "logAnalyticsDestinationType": null. So the issue is a lot harder to replicate consistently.

@thrubovc
Copy link

I have the same problem. Removing the diagnostic setting manually worked for me as a workaround to fix my terraform apply!

@teowa
Copy link
Contributor

teowa commented Jan 16, 2023

By checking the code, this is a bug in the provider when updating the diagnostic setting, I am going to submit PR to fix this.

@briukhanov
Copy link

Same for me. If I delete monitoring setting on portal all work fine but second run produce error. Only for apply step. terraform plan works with no error.

@jackyou84
Copy link

jackyou84 commented Jan 29, 2023

terraform plan always report drift. try to upgrade to 3.40 , but not work

# azurerm_monitor_diagnostic_setting.anthem_keyvault_oms_auditing will be updated in-place
  ~ resource "azurerm_monitor_diagnostic_setting" "anthem_keyvault_oms_auditing" {
        id                             = ""
      + log_analytics_destination_type = "AzureDiagnostics"
        name                           = "anthemcustomsaudit"
        # (2 unchanged attributes hidden)

        # (4 unchanged blocks hidden)
    }

@4c74356b41
Copy link

4c74356b41 commented Feb 3, 2023

resource "azurerm_monitor_diagnostic_setting" "network_security_groups" {
  for_each = local.network_security_groups

  name                           = "${each.key}-diagnostic-setting"
  target_resource_id             = yyy
  log_analytics_workspace_id     = xxx
  log_analytics_destination_type = "AzureDiagnostics"

  enabled_log {
    category_group = "allLogs"

    retention_policy {
      enabled = false
      days    = 0
    }
  }
}

constant updates to the resource with diff identical to the above
TF: 1.3.7, provider: 3.41.0

@4c74356b41
Copy link

@teowa does it work for you?

@teowa
Copy link
Contributor

teowa commented Feb 3, 2023

Please see #20140

@Soufianux
Copy link

Same in 3.42.0 . it's still reporting update of locanalytics_destination_type without taking any effect after apply:

resource "azurerm_monitor_diagnostic_setting" "logs" {
id = "/subscriptions/xxxxxx-xxxxx-xxxxx-xxxx-xxxxxxxxresourceGroups/xxxxxx/providers/Microsoft.Network/virtualNetworks/xxxxxxx|Diagnostic_logs"
+ log_analytics_destination_type = "AzureDiagnostics"
name = "Diagnostic_logs"

@cpressland
Copy link
Author

Yep, no change in behaviour on 3.42.0 for me.

@github-actions
Copy link

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants