monitor_diagnostic_setting: suppress diff on metric/log against default value

[magodo]

Some resource type (specified by target_resource_id) has its own set of diagnostic settings. If the user didn't explicitly specify some of the settings in PUT request body, the representation of the diag settings will still contain those settings, with the default values (0/disabled). This is not an idempotent API, IMHO.

Under this situation, each time user has to specify all the available diagnostic settings of the target resource, even though some of these settings are just contain the default value(0/disbaled). This violates the convention over configuration principle.

This PR adds suppress function for those settings (metric and log), the function will suppress diff only when the old settings have "net" more elements than the new one, and the delta settings have default value.

Test Result

💤 via 🦉 v1.14.4 make testacc TEST=./azurerm/internal/services/monitor/tests TESTARGS='-run TestAccAzureRMMonitorDiagnosticSetting_'       

==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test ./azurerm/internal/services/monitor/tests -v -run TestAccAzureRMMonitorDiagnosticSetting_ -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMMonitorDiagnosticSetting_eventhub
=== PAUSE TestAccAzureRMMonitorDiagnosticSetting_eventhub
=== RUN   TestAccAzureRMMonitorDiagnosticSetting_metricOnly
=== PAUSE TestAccAzureRMMonitorDiagnosticSetting_metricOnly
=== RUN   TestAccAzureRMMonitorDiagnosticSetting_requiresImport
=== PAUSE TestAccAzureRMMonitorDiagnosticSetting_requiresImport
=== RUN   TestAccAzureRMMonitorDiagnosticSetting_logAnalyticsWorkspace
=== PAUSE TestAccAzureRMMonitorDiagnosticSetting_logAnalyticsWorkspace
=== RUN   TestAccAzureRMMonitorDiagnosticSetting_logAnalyticsWorkspaceDedicated
=== PAUSE TestAccAzureRMMonitorDiagnosticSetting_logAnalyticsWorkspaceDedicated
=== RUN   TestAccAzureRMMonitorDiagnosticSetting_storageAccount
=== PAUSE TestAccAzureRMMonitorDiagnosticSetting_storageAccount
=== RUN   TestAccAzureRMMonitorDiagnosticSetting_activityLog
=== PAUSE TestAccAzureRMMonitorDiagnosticSetting_activityLog
=== CONT  TestAccAzureRMMonitorDiagnosticSetting_eventhub
=== CONT  TestAccAzureRMMonitorDiagnosticSetting_logAnalyticsWorkspaceDedicated
=== CONT  TestAccAzureRMMonitorDiagnosticSetting_activityLog
=== CONT  TestAccAzureRMMonitorDiagnosticSetting_metricOnly
=== CONT  TestAccAzureRMMonitorDiagnosticSetting_logAnalyticsWorkspace
=== CONT  TestAccAzureRMMonitorDiagnosticSetting_storageAccount
=== CONT  TestAccAzureRMMonitorDiagnosticSetting_requiresImport
    TestAccAzureRMMonitorDiagnosticSetting_requiresImport: testing.go:640: Step 0 error: errors during apply:
        
        Error: ID was missing the `AuthorizationRules` element
        
          on /tmp/tf-test209852923/main.tf line 29:
          (source code not available)
        
        
    TestAccAzureRMMonitorDiagnosticSetting_eventhub: testing.go:640: Step 0 error: errors during apply:
        
        Error: ID was missing the `AuthorizationRules` element
        
          on /tmp/tf-test193034397/main.tf line 29:
          (source code not available)
        
        
    TestAccAzureRMMonitorDiagnosticSetting_eventhub: testing.go:701: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.
        
        Error: errors during refresh: ID was missing the `AuthorizationRules` element
        
        State: <nil>
--- FAIL: TestAccAzureRMMonitorDiagnosticSetting_eventhub (169.80s)
    TestAccAzureRMMonitorDiagnosticSetting_requiresImport: testing.go:701: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.
        
        Error: errors during refresh: ID was missing the `AuthorizationRules` element
        
        State: <nil>
--- FAIL: TestAccAzureRMMonitorDiagnosticSetting_requiresImport (170.61s)
--- PASS: TestAccAzureRMMonitorDiagnosticSetting_activityLog (201.61s)
--- PASS: TestAccAzureRMMonitorDiagnosticSetting_logAnalyticsWorkspaceDedicated (219.37s)
--- PASS: TestAccAzureRMMonitorDiagnosticSetting_metricOnly (265.58s)
--- PASS: TestAccAzureRMMonitorDiagnosticSetting_storageAccount (339.96s)
--- PASS: TestAccAzureRMMonitorDiagnosticSetting_logAnalyticsWorkspace (347.12s)
FAIL
FAIL    github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/monitor/tests       347.171s
FAIL
make: *** [GNUmakefile:98: testacc] Error 1

The two failures are not related to this PR, but has its own issue: #7310

Further Thoughts

The implementation of the suppress function is not ideal as it will be called multiple times for the including elements of the Set (because the implementation of Set in terraform store its elements via a map). This means there will be a little performance penalty.

On the other hand, Tom has suggested using a custom hash function, while it seems not fit to fix this issue.

(fix: #7235)

[magodo]

The test result of the modified test case:

terraform-provider-azurerm on  monitor_diag_setting_diffsuppress [$!] took 5m 11s 
💤 😡 130 via 🦉 v1.14.4 make testacc TEST=./azurerm/internal/services/monitor/tests TESTARGS="-run='TestAccAzureRMMonitorDiagnosticSetting_logAnalyticsWorkspaceDedicated'"
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test ./azurerm/internal/services/monitor/tests -v -run='TestAccAzureRMMonitorDiagnosticSetting_logAnalyticsWorkspaceDedicated' -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMMonitorDiagnosticSetting_logAnalyticsWorkspaceDedicated
=== PAUSE TestAccAzureRMMonitorDiagnosticSetting_logAnalyticsWorkspaceDedicated
=== CONT  TestAccAzureRMMonitorDiagnosticSetting_logAnalyticsWorkspaceDedicated
--- PASS: TestAccAzureRMMonitorDiagnosticSetting_logAnalyticsWorkspaceDedicated (385.52s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/monitor/tests       385.536s

[manicminer]

I tested this a little with the example configuration in the linked issue. After applying the configuration, if I then remove a log or metric block from the configuration, there's no diff and it is not removed.

resource "azurerm_monitor_diagnostic_setting" "test" {
  name               = "aaa-tbamford-test-api-diagnostics"
  target_resource_id = azurerm_api_management.test.id

  log_analytics_workspace_id     = azurerm_log_analytics_workspace.test.id
  log_analytics_destination_type = "Dedicated"

//  log {
//    category = "GatewayLogs"
//    enabled  = true
//
//    retention_policy {
//      enabled = true
//      days    = 30
//    }
//  }

//  metric {
//    category = "Gateway Requests"
//    enabled  = true
//
//    retention_policy {
//      enabled = true
//      days    = 30
//    }
//  }
}

[magodo]

Hey @manicminer You're right!
This is because the suppress diff happens before Diff is generated, the d.GetChange in suppress diff function can only compare the given attribute based on the state from either "state" or "config". That means we have no way to tell whether an attribute is removed from config (in which case it will use the one read from "state" instead).
Instead, the customized diff function, the ResourceData has access to the actual diff. Let me see if I can come up with a solution using the customized diff. One concern is that the ResourceDiff.Clear is only allowed on computed keys.

[magodo]

I have switched to using customized diff instead. But because of bug hashicorp/terraform-plugin-sdk#497 in plugin SDK, it is not able to access nested block of type set in CustmoizedDiff now. Let's hold on this PR until that issue being fixed.

[tombuildsstuff]

Closing this in favour of hashicorp/terraform-plugin-sdk#497 - once that's fixed we can re-open this and take another look

[ghost]

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!