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

azurerm_monitor_diagnostic_setting in state file there is always empty string of "log_analytics_destination_type": "", despite the value is defined in .tf #8131

Closed
eissko opened this issue Aug 14, 2020 · 27 comments
Labels
service/monitor upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR

Comments

@eissko
Copy link

eissko commented Aug 14, 2020

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 (and AzureRM Provider) Version

Terraform v0.12.29

  • provider.azurerm v2.23.0

Affected Resource(s)

  • azurerm_monitor_diagnostic_setting

Terraform Configuration Files

variable secrets {}

provider "azurerm" {
  subscription_id = var.secrets.subscription_id
  tenant_id       = var.secrets.tenant_id
  client_id       = var.secrets.client_id
  client_secret   = var.secrets.client_secret
  version         = ">= 2.23.0"
  features {}
}

resource "azurerm_resource_group" "example" {
  name      = "diagnostics-rgp"
  location  = "southafricanorth"
}

resource "azurerm_storage_account" "example" {
  name                     = "sadiagxxxyyyzzz"
  resource_group_name      = azurerm_resource_group.example.name
  location                 = azurerm_resource_group.example.location
  account_tier             = "Standard"
  account_replication_type = "GRS"

  tags = {
    environment = "staging"
  }
}

resource "azurerm_virtual_network" "example" {
  name                = "virtnetname"
  address_space       = ["172.23.23.0/28"]
  location            = azurerm_resource_group.example.location
  resource_group_name = azurerm_resource_group.example.name
}

resource "azurerm_log_analytics_workspace" "example" {
  name                = "test-log-analtics-wrk"
  location            = azurerm_resource_group.example.location
  resource_group_name = azurerm_resource_group.example.name
  sku                 = "PerGB2018"
  retention_in_days   = 30
}

resource "azurerm_monitor_diagnostic_setting" "diag" {

  name                             = "vnet-diag"
  target_resource_id               = azurerm_virtual_network.example.id
  log_analytics_workspace_id       = azurerm_log_analytics_workspace.example.id
  log_analytics_destination_type   = "Dedicated"
  storage_account_id               = azurerm_storage_account.example.id

    
  log {
      category    = "VMProtectionAlerts"
      enabled     = true 
      retention_policy {
          enabled = true
          days    = 30
      }
  }
    
  metric {
    category    = "AllMetrics"
    enabled     = true
    retention_policy {
        enabled = true
        days    = 30
    }
  }
  
}

Debug Output

2020/08/14 21:22:58 [WARN] Provider "registry.terraform.io/-/azurerm" produced an unexpected new value for azurerm_monitor_diagnostic_setting.diag, but we are tolerating it because it is using the legacy plugin SDK. The following problems may be the cause of any confusing errors from downstream operations: - .eventhub_name: was null, but now cty.StringVal("") **- .log_analytics_destination_type: was cty.StringVal("Dedicated"), but now cty.StringVal("")** - .eventhub_authorization_rule_id: was null, but now cty.StringVal("")

Panic Output

none

Expected Behavior

In the tfstate must be stored proper value as defined in terraform configuration. Hence this is expect value in terraform state file "log_analytics_destination_type": "Dedicated".

Actual Behavior

Actual behavior is that the value of attribute "log_analytics_destination_type" is always empty string "" regardless what is defined in the terraform code. Which leads every terraform plan showing CHANGE every time it is run.

   {
      "mode": "managed",
      "type": "azurerm_monitor_diagnostic_setting",
      "name": "diag",
      "provider": "provider.azurerm",
      "instances": [
        {
          "schema_version": 0,
          "attributes": {
            "eventhub_authorization_rule_id": "",
            "eventhub_name": "",
            "id": "****/virtnetname|vnet-diag",
            "log": [
              {
                "category": "VMProtectionAlerts",
                "enabled": true,
                "retention_policy": [
                  {
                    "days": 30,
                    "enabled": true
                  }
                ]
              }
            ],
            "log_analytics_destination_type": "",
   ...}

Steps to Reproduce

Take terraform configuration provided above. Attach proper secrets as defined in configuration :
secrets = {
subscription_id = ""
tenant_id = ""
client_id = ""
client_secret = ""
}

1 terraform init
2 terraform apply

3 - manual step - check the terraform state where you are gonna find the empty string for "log_analytics_destination_type": ""

4 - terraform plan - plan will result in the 1 change of attribute "log_analytics_destination_type": "" - which is infinite process

@eissko
Copy link
Author

eissko commented Aug 14, 2020

FYI
related issue:
Azure/azure-rest-api-specs#9281

@eissko
Copy link
Author

eissko commented Aug 14, 2020

This is from DEBUG log:

2020/08/14 21:22:58 [WARN] Provider "registry.terraform.io/-/azurerm" produced an unexpected new value for azurerm_monitor_diagnostic_setting.diag, but we are tolerating it because it is using the legacy plugin SDK.
    The following problems may be the cause of any confusing errors from downstream operations:
      - .eventhub_name: was null, but now cty.StringVal("")
      **- .log_analytics_destination_type: was cty.StringVal("Dedicated"), but now cty.StringVal("")**
      - .eventhub_authorization_rule_id: was null, but now cty.StringVal("")

Also from DEBUG log I see API operations executed on "terraform apply" as following:

1, PUT operations returns the correct response "logAnalyticsDestinationType":"Dedicated"
2, but next following GET operations returns "logAnalyticsDestinationType": null

It uses this specific microsoft azure api:
https://docs.microsoft.com/en-us/azure/templates/microsoft.insights/2017-05-01-preview/diagnosticsettings

@magodo magodo added the upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR label Aug 17, 2020
@magodo
Copy link
Collaborator

magodo commented Aug 17, 2020

Hey @eissko Thank you for submitting this 👍

As you have noticed, there is an upstream issue about the confusing logAnalyticsDestinationType property. Let's just wait for their response.

In regards to your specific issue, you can simply leave log_analytics_destination_type empty, then it should work.

@eissko
Copy link
Author

eissko commented Aug 17, 2020

@magodo Yes there is upstream issue. But it has been open again when I mentioned it in my issue. Before, it had been closed by bot. I assumed nobody were taking care of it.

And unfortunately, I can't leave it empty. Hence:

Error: expected log_analytics_destination_type to be one of [Dedicated AzureDiagnostics], got

thanks

Peter

@magodo
Copy link
Collaborator

magodo commented Aug 17, 2020

@eissko Have you tried to leave log_analytics_destination_type empty? I think it should work.

@eissko
Copy link
Author

eissko commented Aug 24, 2020

@magodo yes I tried to leave it empty. as you see above or below I got an error:

Error: expected log_analytics_destination_type to be one of [Dedicated AzureDiagnostics], got

@magodo
Copy link
Collaborator

magodo commented Aug 24, 2020

@eissko, I mean just to comment out that attribute at all (rather than setting it an empty string). I have verified it locally.

@eissko
Copy link
Author

eissko commented Aug 24, 2020

@magodo I can try that but still for some resource we need to set it as "Dedicated" and for some "AzureDiagnostics". The clean solution would fix the bug in API anyway.

@hashitop
Copy link
Contributor

hashitop commented Oct 7, 2020

The argument log_analytics_destination_type is optional, without this argument defined in the configuration file, the resource will pass null to the API which is default value and as per the API document, the default null value is equivalent to AzureDiagnostics.

Though some resources support AzureDiagnostics but it is not documented and I don't see why azurerm_monitor_diagnostic_setting resource will need to support undocumented value where setting it to null or remove the log_analytics_destination_type optional argument from the configuration will yield the same result.

It is, in fact, supporting AzureDiagnostics can cause an issue to the state when creating diagnostic_setting on a resource that does not support resource-specific destination type, AzureDiagnostics will convert the value to null and the value in the state file will aways be null due to the behaviour of the API.

This CLI document can be used to confirm it too. The command only takes the flag --export-to-resource-specific which can be set to true or false, when set to true, the attribute logAnalyticsDestinationType in the payload will be set to Dedicated or null when set to false. There is no such value like AzureDiagnostics in the CLI. The support to AzureDiagnostics should be removed.

@sumanghosh131990
Copy link

even if the destination is event hub and the property is not present at all, then also it is showing a plan change for log_analytics_destination_type from "AzureDiagnostics" to "Null". Hence needed some fix here

@vijred
Copy link

vijred commented Aug 9, 2021

even if the destination is event hub and the property is not present at all, then also it is showing a plan change for log_analytics_destination_type from "AzureDiagnostics" to "Null". Hence needed some fix here

Updating log_analytics_destination_type to "AzureDiagnostics" avoids plan change, however if there is any update to azurerm_monitor_diagnostic_setting, apply fails with following error meaning there is no workaround for this problem!

Error: log_analytics_workspace_id must be set for log_analytics_destination_type to be used

@SenorRagequit
Copy link

Having the same problem, I don't even use the log_analytics_destination_type, but terraform always has to change it for each run.
grafik

Happening for me for the API-Manager and the CosmosDB.

@bradhannah
Copy link

Also happens with Recovery Services Vaults.

@Noel-Jones
Copy link
Contributor

Came across this with a Synapse SQL Pool. I checked and onfirmed the Rest API rturns
"logAnalyticsDestinationType": null
and also that it returns a metrics entry and a log entry that are disabled and in the console there is no evidence they can be enabled. I suspect every resource will have different quirks! Specific to this resource (perhaps) and this issue I worked around the issue by setting the log_analytics_destinbation_type = null. Confirmed that I can create the setting and no drift after.

resource "azurerm_monitor_diagnostic_setting" "log" {
  for_each                       = var.environments
  name                           = "log_${each.key}"
  target_resource_id             = azurerm_synapse_sql_pool.static_sql_pool[each.value].id
  log_analytics_workspace_id     = azurerm_log_analytics_workspace.log_analytics_workspace.id
  log_analytics_destination_type = null
  dynamic "log" {
    for_each = var.diagnostic_logs
    content {
      category = log.key
      retention_policy {
        enabled = true
        days    = log.value.days
      }
    }
  }
  # The following 2 are added to avoid drift.
  # The console does not allow these to be set or show them but the API returns them to Terraform.
  # May need to readress this fix if we did want to send these to our logws but for now this will do.
  log {
    category = "SQLSecurityAuditEvents"
    enabled  = false
    retention_policy {
      days    = 0
      enabled = false
    }
  }
  metric {
    category = "AllMetrics"
    enabled  = false
    retention_policy {
      days    = 0
      enabled = false
    }
  }
}

@nmaupu
Copy link

nmaupu commented Sep 27, 2022

I have this issue on APIM resources where log_analytics_destination_type keeps reading AzureDiagnostics from API.
This ends in the state and the result is the following error no matter what:

Error: `log_analytics_workspace_id` must be set for `log_analytics_destination_type` to be used

Editing the state to remove this field, setting it to null or not at all does not change anything. Resource can be created but never updated.

EDIT:
Ok, if I remove my lifecycle ignore_changes it applies... But indeed, I do have a change when plan executes.

@Thaval
Copy link

Thaval commented Nov 24, 2022

How long will it take to fix this? Any plans on this?

@robsoncloud
Copy link

Hello guys,
Anyone from MS looking at this issue? I'm facing the same issue but with Azure Gateways

@websolut
Copy link

websolut commented Dec 8, 2022

Hi, I am facing the same issue with one of the key vaults (strangely enough it just one out of 15). The state is not updated with 'AzureDiagnostics' value; portal also shows as value as 'null'.

TF reports that it updates the resource.

@a-z-i-z
Copy link

a-z-i-z commented Jan 20, 2023

Facing the same issue with MSSQL server and DB. It updates log_analytics_destination_type everytime if we apply some other changes. We don't want to have unnecessary modifications made at Database level.
image
@hashitop Can we know the status for this issue ?

@ValeruS
Copy link

ValeruS commented Jan 27, 2023

The same issue with azurerm > 3.38 version
terraform is trying to add every time

  # some-of-module.azurerm_monitor_diagnostic_setting.monitoring will be updated in-place
  ~ resource "azurerm_monitor_diagnostic_setting" "monitoring" {
        id                             = "/...../name-of-log"
      + log_analytics_destination_type = "AzureDiagnostics"
        name                           = "dev4-aks-log"
        # (2 unchanged attributes hidden)

        # (23 unchanged blocks hidden)
    }

if you apply the changes and then make a terraform plan
it shows that log_analytics_destination_type should be added

not seeing the issue with azurerm 3.38

@zhjuncai
Copy link

zhjuncai commented Feb 9, 2023

same on version 3.42.0 which is the latest version as of now.

in-place
  ~ resource "azurerm_monitor_diagnostic_setting" "metrics" {
        id                             = "/subscriptions/xxxxx/metrics"
      + log_analytics_destination_type = "AzureDiagnostics"
        name                           = "metrics"
        # (2 unchanged attributes hidden)

        # (7 unchanged blocks hidden)
    }

@Noel-Jones
Copy link
Contributor

It seems to me that hashitop has hit the nail on the head. The resource azurerm_monitor_diagnostic_setting applies to many different underlying resources and the responses can differ according to the underlying resource. For example when pointed at an AzureFirewall the API does return "logAnalyticsDestinationType": "AzureDiagnostics" but against an automation account and bastion I get back "logAnalyticsDestinationType": null. Feel free to try your own cases here try it

To quote again the API documentation "properties.logAnalyticsDestinationType = A string indicating whether the export to Log Analytics should use the default destination type, i.e. AzureDiagnostics, or use a destination type constructed as follows: Possible values are: Dedicated and null (null is default.)"

I assume the logic is currently that when set to AzureDiagnostics we send null via the API but we record AzureDiagnostics in the state. Hence the drift when we get null back, except in cases such as the firewall when we get AzureDiagnostics back. Conversely for the firewall I'd guess when we sent null we'd get AzureDiagnostics back and see a change, which probably led to the original Terraform change in this area, but I can't test this easily at this time.

I'd suggest we allow three states; not set/null => we send null, AzureDiagnostics we send AzureDiagnostics, Dedicated we send Dedicated. This ought to cater for any case. I haven't looked at the code to check my assumption. If get time I'll see if I can play with the code.

@Noel-Jones
Copy link
Contributor

I had a play. By making the attribute optional and allowing it to be set to null, it allows the appropriate setting for a resource so there is no drift and no need for lifecycle ignore. You need to work out what to set it too but a plan after an apply will tell you if you got it wrong (or you can see what the API sends back). It isn't ideal but it does give the greatest flexibility and ought to be good for all resource types. With the change in place then I need to set as follows for my three cases:

automation account: log_analytics_destination_type = null
bastion host: log_analytics_destination_type = null
azure firewall: log_analytics_destination_type = "AzureDiagnostics"

Up to now for the first two I've had to add a lifecycle ignore. If people think this is ok I can do a PR once I work out the test cases and a suitable documentation update.

@WouterVan
Copy link

Seems like this issue has been fixed as a Bug Fix in the latest Azurerm version 3.45.0, this update fixed my issue after running one apply.
https://github.com/hashicorp/terraform-provider-azurerm/blob/main/CHANGELOG.md

@eissko
Copy link
Author

eissko commented Feb 27, 2023

@WouterVan looks like it. Here #20203

Maybe, we can close this issue too. But I created it 3 years ago. And I am not able to verify it now.

@eissko eissko closed this as completed Feb 27, 2023
@sikemullivan
Copy link

I was able to verify that no change is detected after upgrading to 3.45.0.

@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 Apr 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
service/monitor upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR
Projects
None yet
Development

No branches or pull requests