From 092796d14583401b5bccd448e3c215f9cf647a52 Mon Sep 17 00:00:00 2001 From: Catriona Date: Tue, 29 Nov 2022 14:52:14 +0000 Subject: [PATCH 1/9] deprecate log in favour of enabked_log and disabled_log --- .../monitor_diagnostic_setting_resource.go | 381 ++++++++++++++++-- ...onitor_diagnostic_setting_resource_test.go | 201 ++++++++- 2 files changed, 543 insertions(+), 39 deletions(-) diff --git a/internal/services/monitor/monitor_diagnostic_setting_resource.go b/internal/services/monitor/monitor_diagnostic_setting_resource.go index ce1e898a65af..19bb6ae9ed87 100644 --- a/internal/services/monitor/monitor_diagnostic_setting_resource.go +++ b/internal/services/monitor/monitor_diagnostic_setting_resource.go @@ -27,9 +27,9 @@ import ( func resourceMonitorDiagnosticSetting() *pluginsdk.Resource { return &pluginsdk.Resource{ - Create: resourceMonitorDiagnosticSettingCreateUpdate, + Create: resourceMonitorDiagnosticSettingCreate, Read: resourceMonitorDiagnosticSettingRead, - Update: resourceMonitorDiagnosticSettingCreateUpdate, + Update: resourceMonitorDiagnosticSettingUpdate, Delete: resourceMonitorDiagnosticSettingDelete, Importer: pluginsdk.ImporterValidatingResourceId(func(id string) error { @@ -103,8 +103,10 @@ func resourceMonitorDiagnosticSetting() *pluginsdk.Resource { }, "log": { - Type: pluginsdk.TypeSet, - Optional: true, + Type: pluginsdk.TypeSet, + Optional: true, + Computed: true, + Deprecated: "`log` will be removed in favour of the properties `enabled_log` and `disabled_log` in version 4.0 of the AzureRM Provider.", Elem: &pluginsdk.Resource{ Schema: map[string]*pluginsdk.Schema{ "category": { @@ -147,6 +149,84 @@ func resourceMonitorDiagnosticSetting() *pluginsdk.Resource { Set: resourceMonitorDiagnosticLogSettingHash, }, + "enabled_log": { + Type: pluginsdk.TypeSet, + Optional: true, + Computed: true, + ConflictsWith: []string{"log"}, + Elem: &pluginsdk.Resource{ + Schema: map[string]*pluginsdk.Schema{ + "category": { + Type: pluginsdk.TypeString, + Optional: true, + }, + + "category_group": { + Type: pluginsdk.TypeString, + Optional: true, + }, + + "retention_policy": { + Type: pluginsdk.TypeList, + Optional: true, + MaxItems: 1, + Elem: &pluginsdk.Resource{ + Schema: map[string]*pluginsdk.Schema{ + "enabled": { + Type: pluginsdk.TypeBool, + Required: true, + }, + + "days": { + Type: pluginsdk.TypeInt, + Optional: true, + ValidateFunc: validation.IntAtLeast(0), + }, + }, + }, + }, + }, + }, + Set: resourceMonitorDiagnosticLogSettingHash, + }, + + "disabled_log": { + Type: pluginsdk.TypeSet, + Computed: true, + Elem: &pluginsdk.Resource{ + Schema: map[string]*pluginsdk.Schema{ + "category": { + Type: pluginsdk.TypeString, + Computed: true, + }, + + "category_group": { + Type: pluginsdk.TypeString, + Computed: true, + }, + + "retention_policy": { + Type: pluginsdk.TypeList, + Computed: true, + Elem: &pluginsdk.Resource{ + Schema: map[string]*pluginsdk.Schema{ + "enabled": { + Type: pluginsdk.TypeBool, + Computed: true, + }, + + "days": { + Type: pluginsdk.TypeInt, + Computed: true, + }, + }, + }, + }, + }, + }, + Set: resourceMonitorDiagnosticLogSettingHash, + }, + "metric": { Type: pluginsdk.TypeSet, Optional: true, @@ -190,7 +270,7 @@ func resourceMonitorDiagnosticSetting() *pluginsdk.Resource { } } -func resourceMonitorDiagnosticSettingCreateUpdate(d *pluginsdk.ResourceData, meta interface{}) error { +func resourceMonitorDiagnosticSettingCreate(d *pluginsdk.ResourceData, meta interface{}) error { client := meta.(*clients.Client).Monitor.DiagnosticSettingsClient ctx, cancel := timeouts.ForCreateUpdate(meta.(*clients.Client).StopContext, d) defer cancel() @@ -200,37 +280,48 @@ func resourceMonitorDiagnosticSettingCreateUpdate(d *pluginsdk.ResourceData, met actualResourceId := d.Get("target_resource_id").(string) diagnosticSettingId := diagnosticsettings.NewScopedDiagnosticSettingID(actualResourceId, name) - if d.IsNewResource() { - existing, err := client.Get(ctx, diagnosticSettingId) - if err != nil { - if !response.WasNotFound(existing.HttpResponse) { - return fmt.Errorf("checking for presence of existing Monitor Diagnostic Setting %q for Resource %q: %s", diagnosticSettingId.Name, diagnosticSettingId.ResourceUri, err) - } + existing, err := client.Get(ctx, diagnosticSettingId) + if err != nil { + if !response.WasNotFound(existing.HttpResponse) { + return fmt.Errorf("checking for presence of existing Monitor Diagnostic Setting %q for Resource %q: %s", diagnosticSettingId.Name, diagnosticSettingId.ResourceUri, err) } + } - if existing.Model != nil && existing.Model.Id != nil && *existing.Model.Id != "" { - return tf.ImportAsExistsError("azurerm_monitor_diagnostic_setting", *existing.Model.Id) - } + if existing.Model != nil && existing.Model.Id != nil && *existing.Model.Id != "" { + return tf.ImportAsExistsError("azurerm_monitor_diagnostic_setting", *existing.Model.Id) } logsRaw := d.Get("log").(*pluginsdk.Set).List() - logs := expandMonitorDiagnosticsSettingsLogs(logsRaw) + enabledLogs := d.Get("enabled_log").(*pluginsdk.Set).List() metricsRaw := d.Get("metric").(*pluginsdk.Set).List() metrics := expandMonitorDiagnosticsSettingsMetrics(metricsRaw) + var logs []diagnosticsettings.LogSettings + hasEnabledLogs := true + if len(logsRaw) > 0 { + hasEnabledLogs = false + logs = expandMonitorDiagnosticsSettingsLogs(logsRaw) + for _, v := range logs { + if v.Enabled { + hasEnabledLogs = true + break + } + } + } else if len(enabledLogs) == 0 { + hasEnabledLogs = false + } + + if len(enabledLogs) > 0 { + logs = expandMonitorDiagnosticsSettingsEnabledLogs(enabledLogs) + } + // if no blocks are specified the API "creates" but 404's on Read - if len(logs) == 0 && len(metrics) == 0 { - return fmt.Errorf("At least one `log` or `metric` block must be specified") + if !hasEnabledLogs && len(metrics) == 0 { + return fmt.Errorf("at least one `log` or `metric` block must be specified") } // also if there's none enabled valid := false - for _, v := range logs { - if v.Enabled { - valid = true - break - } - } if !valid { for _, v := range metrics { if v.Enabled { @@ -240,8 +331,8 @@ func resourceMonitorDiagnosticSettingCreateUpdate(d *pluginsdk.ResourceData, met } } - if !valid { - return fmt.Errorf("At least one `log` or `metric` must be enabled") + if !valid && !hasEnabledLogs { + return fmt.Errorf("at least one `log` or `metric` must be enabled") } parameters := diagnosticsettings.DiagnosticSettingsResource{ @@ -299,7 +390,7 @@ func resourceMonitorDiagnosticSettingCreateUpdate(d *pluginsdk.ResourceData, met return err } if read.Model == nil && read.Model.Id == nil { - return fmt.Errorf("Cannot read ID for Monitor Diagnostics %q for Resource ID %q", diagnosticSettingId.Name, diagnosticSettingId.ResourceUri) + return fmt.Errorf("cannot read ID for Monitor Diagnostics %q for Resource ID %q", diagnosticSettingId.Name, diagnosticSettingId.ResourceUri) } d.SetId(fmt.Sprintf("%s|%s", actualResourceId, name)) @@ -307,6 +398,153 @@ func resourceMonitorDiagnosticSettingCreateUpdate(d *pluginsdk.ResourceData, met return resourceMonitorDiagnosticSettingRead(d, meta) } +func resourceMonitorDiagnosticSettingUpdate(d *pluginsdk.ResourceData, meta interface{}) error { + client := meta.(*clients.Client).Monitor.DiagnosticSettingsClient + ctx, cancel := timeouts.ForCreateUpdate(meta.(*clients.Client).StopContext, d) + defer cancel() + log.Printf("[INFO] preparing arguments for Azure ARM Diagnostic Settings.") + name := d.Get("name").(string) + actualResourceId := d.Get("target_resource_id").(string) + diagnosticSettingId := diagnosticsettings.NewScopedDiagnosticSettingID(actualResourceId, name) + + metricsRaw := d.Get("metric").(*pluginsdk.Set).List() + metrics := expandMonitorDiagnosticsSettingsMetrics(metricsRaw) + + var logs []diagnosticsettings.LogSettings + hasEnabledLogs := true + if d.HasChange("log") { + logsRaw := d.Get("log").(*pluginsdk.Set).List() + logs = expandMonitorDiagnosticsSettingsLogs(logsRaw) + hasEnabledLogs = false + for _, v := range logs { + if v.Enabled { + hasEnabledLogs = true + break + } + } + } else if d.HasChange("enabled_log") { + enabledLogs := d.Get("enabled_log").(*pluginsdk.Set).List() + logs = expandMonitorDiagnosticsSettingsEnabledLogs(enabledLogs) + } + + // if no blocks are specified the API "creates" but 404's on Read + if !hasEnabledLogs && len(metrics) == 0 { + return fmt.Errorf("at least one `log`, `enabled_log` or `metric` block must be specified") + } + + // also if there's none enabled + valid := false + if !valid { + for _, v := range metrics { + if v.Enabled { + valid = true + break + } + } + } + + if !valid && !hasEnabledLogs { + return fmt.Errorf("at least one `log` or `metric` must be enabled") + } + + if d.HasChange("enabled_log") { + oldEnabledLogs, newEnabledLogs := d.GetChange("enabled_log") + + for _, oldLog := range oldEnabledLogs.(*pluginsdk.Set).List() { + logRemoved := true + oldLogMap := oldLog.(map[string]interface{}) + + for _, newLog := range newEnabledLogs.(*pluginsdk.Set).List() { + newLogMap := newLog.(map[string]interface{}) + + // check if an enabled_log has been removed from config and if so, set to disabled + if strings.EqualFold(oldLogMap["category"].(string), newLogMap["category"].(string)) || (oldLogMap["category_group"].(string) != "" && strings.EqualFold(oldLogMap["category_group"].(string), newLogMap["category_group"].(string))) { + logRemoved = false + break + } + } + + if logRemoved { + + disabledLog := diagnosticsettings.LogSettings{ + Category: utils.String(oldLogMap["category"].(string)), + CategoryGroup: utils.String(oldLogMap["category_group"].(string)), + Enabled: false, + } + + retentionPolicy := diagnosticsettings.RetentionPolicy{} + if v, ok := oldLogMap["retention_policy"].([]interface{}); ok { + if len(v) > 0 { + + policyMap := v[0].(map[string]interface{}) + if days, ok := policyMap["days"].(int); ok { + retentionPolicy.Days = int64(days) + } + + if enabled, ok := policyMap["enabled"].(bool); ok { + retentionPolicy.Enabled = enabled + } + } + } + disabledLog.RetentionPolicy = &retentionPolicy + + logs = append(logs, disabledLog) + } + } + } + + parameters := diagnosticsettings.DiagnosticSettingsResource{ + Properties: &diagnosticsettings.DiagnosticSettings{ + Logs: &logs, + Metrics: &metrics, + }, + } + + valid = false + eventHubAuthorizationRuleId := d.Get("eventhub_authorization_rule_id").(string) + eventHubName := d.Get("eventhub_name").(string) + if eventHubAuthorizationRuleId != "" { + parameters.Properties.EventHubAuthorizationRuleId = utils.String(eventHubAuthorizationRuleId) + parameters.Properties.EventHubName = utils.String(eventHubName) + valid = true + } + + workspaceId := d.Get("log_analytics_workspace_id").(string) + if workspaceId != "" { + parameters.Properties.WorkspaceId = utils.String(workspaceId) + valid = true + } + + storageAccountId := d.Get("storage_account_id").(string) + if storageAccountId != "" { + parameters.Properties.StorageAccountId = utils.String(storageAccountId) + valid = true + } + + partnerSolutionId := d.Get("partner_solution_id").(string) + if partnerSolutionId != "" { + parameters.Properties.MarketplacePartnerId = utils.String(partnerSolutionId) + valid = true + } + + if v := d.Get("log_analytics_destination_type").(string); v != "" { + if workspaceId != "" { + parameters.Properties.LogAnalyticsDestinationType = &v + } else { + return fmt.Errorf("`log_analytics_workspace_id` must be set for `log_analytics_destination_type` to be used") + } + } + + if !valid { + return fmt.Errorf("either a `eventhub_authorization_rule_id`, `log_analytics_workspace_id`, `partner_solution_id` or `storage_account_id` must be set") + } + + if _, err := client.CreateOrUpdate(ctx, diagnosticSettingId, parameters); err != nil { + return fmt.Errorf("updating Monitor Diagnostics Setting %q for Resource %q: %+v", name, actualResourceId, err) + } + return resourceMonitorDiagnosticSettingRead(d, meta) +} + func resourceMonitorDiagnosticSettingRead(d *pluginsdk.ResourceData, meta interface{}) error { client := meta.(*clients.Client).Monitor.DiagnosticSettingsClient ctx, cancel := timeouts.ForRead(meta.(*clients.Client).StopContext, d) @@ -376,7 +614,14 @@ func resourceMonitorDiagnosticSettingRead(d *pluginsdk.ResourceData, meta interf d.Set("log_analytics_destination_type", resp.Model.Properties.LogAnalyticsDestinationType) - if err := d.Set("log", flattenMonitorDiagnosticLogs(resp.Model.Properties.Logs)); err != nil { + enabledLogs, disabledLogs := flattenMonitorDiagnosticEnabledAndDisabledLogs(resp.Model.Properties.Logs) + if err = d.Set("enabled_log", enabledLogs); err != nil { + return fmt.Errorf("setting `enabled_log`: %+v", err) + } + if err = d.Set("disabled_log", disabledLogs); err != nil { + return fmt.Errorf("setting `disabled_log`: %+v", err) + } + if err = d.Set("log", flattenMonitorDiagnosticLogs(resp.Model.Properties.Logs)); err != nil { return fmt.Errorf("setting `log`: %+v", err) } @@ -475,6 +720,42 @@ func expandMonitorDiagnosticsSettingsLogs(input []interface{}) []diagnosticsetti return results } +func expandMonitorDiagnosticsSettingsEnabledLogs(input []interface{}) []diagnosticsettings.LogSettings { + results := make([]diagnosticsettings.LogSettings, 0) + + for _, raw := range input { + v := raw.(map[string]interface{}) + + category := v["category"].(string) + categoryGroup := v["category_group"].(string) + policiesRaw := v["retention_policy"].([]interface{}) + var retentionPolicy *diagnosticsettings.RetentionPolicy + if len(policiesRaw) != 0 { + policyRaw := policiesRaw[0].(map[string]interface{}) + retentionDays := policyRaw["days"].(int) + retentionEnabled := policyRaw["enabled"].(bool) + retentionPolicy = &diagnosticsettings.RetentionPolicy{ + Days: int64(retentionDays), + Enabled: retentionEnabled, + } + } + + output := diagnosticsettings.LogSettings{ + Enabled: true, + RetentionPolicy: retentionPolicy, + } + if category != "" { + output.Category = utils.String(category) + } else { + output.CategoryGroup = utils.String(categoryGroup) + } + + results = append(results, output) + } + + return results +} + func flattenMonitorDiagnosticLogs(input *[]diagnosticsettings.LogSettings) []interface{} { results := make([]interface{}, 0) if input == nil { @@ -514,6 +795,52 @@ func flattenMonitorDiagnosticLogs(input *[]diagnosticsettings.LogSettings) []int return results } +func flattenMonitorDiagnosticEnabledAndDisabledLogs(input *[]diagnosticsettings.LogSettings) ([]interface{}, []interface{}) { + enabledLogs := make([]interface{}, 0) + disabledLogs := make([]interface{}, 0) + if input == nil { + return enabledLogs, disabledLogs + } + + for _, v := range *input { + output := make(map[string]interface{}) + + category := "" + if v.Category != nil { + category = *v.Category + } + output["category"] = category + + categoryGroup := "" + if v.CategoryGroup != nil { + categoryGroup = *v.CategoryGroup + } + output["category_group"] = categoryGroup + + policies := make([]interface{}, 0) + + if inputPolicy := v.RetentionPolicy; inputPolicy != nil { + outputPolicy := make(map[string]interface{}) + + outputPolicy["days"] = int(inputPolicy.Days) + + outputPolicy["enabled"] = inputPolicy.Enabled + + policies = append(policies, outputPolicy) + } + + output["retention_policy"] = policies + + if v.Enabled == true { + enabledLogs = append(enabledLogs, output) + } else { + disabledLogs = append(disabledLogs, output) + } + } + + return enabledLogs, disabledLogs +} + func expandMonitorDiagnosticsSettingsMetrics(input []interface{}) []diagnosticsettings.MetricSettings { results := make([]diagnosticsettings.MetricSettings, 0) diff --git a/internal/services/monitor/monitor_diagnostic_setting_resource_test.go b/internal/services/monitor/monitor_diagnostic_setting_resource_test.go index 8885effd8c33..2b7f0460447b 100644 --- a/internal/services/monitor/monitor_diagnostic_setting_resource_test.go +++ b/internal/services/monitor/monitor_diagnostic_setting_resource_test.go @@ -26,6 +26,7 @@ func TestAccMonitorDiagnosticSetting_eventhub(t *testing.T) { check.That(data.ResourceName).Key("eventhub_name").Exists(), check.That(data.ResourceName).Key("eventhub_authorization_rule_id").Exists(), check.That(data.ResourceName).Key("log.#").HasValue("2"), + check.That(data.ResourceName).Key("disabled_log.#").HasValue("1"), check.That(data.ResourceName).Key("metric.#").HasValue("1"), ), }, @@ -153,13 +154,44 @@ func TestAccMonitorDiagnosticSetting_activityLog(t *testing.T) { }) } +func TestAccMonitorDiagnosticSetting_enabledAndDisabledLogs(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_monitor_diagnostic_setting", "test") + r := MonitorDiagnosticSettingResource{} + data.ResourceTest(t, r, []acceptance.TestStep{ + { + Config: r.enabledAndDisabledLogs(data), + Check: acceptance.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + check.That(data.ResourceName).Key("enabled_log.#").HasValue("2"), + check.That(data.ResourceName).Key("disabled_log.#").HasValue("0"), + ), + }, + data.ImportStep(), + { + Config: r.enabledAndDisabledLogsUpdated(data), + Check: acceptance.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + check.That(data.ResourceName).Key("enabled_log.#").HasValue("1"), + check.That(data.ResourceName).Key("disabled_log.#").HasValue("1"), + ), + }, + data.ImportStep(), + { + Config: r.enabledAndDisabledLogs(data), + Check: acceptance.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + check.That(data.ResourceName).Key("enabled_log.#").HasValue("2"), + check.That(data.ResourceName).Key("disabled_log.#").HasValue("0"), + ), + }, + }) +} + func (t MonitorDiagnosticSettingResource) Exists(ctx context.Context, clients *clients.Client, state *pluginsdk.InstanceState) (*bool, error) { id, err := monitor.ParseMonitorDiagnosticId(state.ID) if err != nil { return nil, err } - // actualResourceId := id.ResourceUri - // targetResourceId := strings.TrimPrefix(actualResourceId, "/") resp, err := clients.Monitor.DiagnosticSettingsClient.Get(ctx, *id) if err != nil { @@ -230,16 +262,6 @@ resource "azurerm_monitor_diagnostic_setting" "test" { } } - log { - category = "AzurePolicyEvaluationDetails" - enabled = false - - retention_policy { - days = 0 - enabled = false - } - } - metric { category = "AllMetrics" enabled = true @@ -768,3 +790,158 @@ resource "azurerm_monitor_diagnostic_setting" "test" { } `, data.RandomInteger, data.Locations.Primary, data.RandomIntOfLength(17)) } + +func (MonitorDiagnosticSettingResource) enabledAndDisabledLogs(data acceptance.TestData) string { + return fmt.Sprintf(` +provider "azurerm" { + features {} +} + +data "azurerm_client_config" "current" { +} + +resource "azurerm_resource_group" "test" { + name = "acctestRG-%[1]d" + location = "%[2]s" +} + +resource "azurerm_eventhub_namespace" "test" { + name = "acctest-EHN-%[1]d" + location = azurerm_resource_group.test.location + resource_group_name = azurerm_resource_group.test.name + sku = "Basic" +} + +resource "azurerm_eventhub" "test" { + name = "acctest-EH-%[1]d" + namespace_name = azurerm_eventhub_namespace.test.name + resource_group_name = azurerm_resource_group.test.name + partition_count = 2 + message_retention = 1 +} + +resource "azurerm_eventhub_namespace_authorization_rule" "test" { + name = "example" + namespace_name = azurerm_eventhub_namespace.test.name + resource_group_name = azurerm_resource_group.test.name + listen = true + send = true + manage = true +} + +resource "azurerm_key_vault" "test" { + name = "acctest%[3]d" + location = azurerm_resource_group.test.location + resource_group_name = azurerm_resource_group.test.name + tenant_id = data.azurerm_client_config.current.tenant_id + sku_name = "standard" +} + +resource "azurerm_monitor_diagnostic_setting" "test" { + name = "acctest-DS-%[1]d" + target_resource_id = azurerm_key_vault.test.id + eventhub_authorization_rule_id = azurerm_eventhub_namespace_authorization_rule.test.id + eventhub_name = azurerm_eventhub.test.name + + enabled_log { + category = "AuditEvent" + + retention_policy { + enabled = false + } + } + + enabled_log { + category = "AzurePolicyEvaluationDetails" + + retention_policy { + days = 0 + enabled = false + } + } + + metric { + category = "AllMetrics" + enabled = true + + retention_policy { + enabled = false + days = 7 + } + } +} +`, data.RandomInteger, data.Locations.Primary, data.RandomIntOfLength(17)) +} + +func (MonitorDiagnosticSettingResource) enabledAndDisabledLogsUpdated(data acceptance.TestData) string { + return fmt.Sprintf(` +provider "azurerm" { + features {} +} + +data "azurerm_client_config" "current" { +} + +resource "azurerm_resource_group" "test" { + name = "acctestRG-%[1]d" + location = "%[2]s" +} + +resource "azurerm_eventhub_namespace" "test" { + name = "acctest-EHN-%[1]d" + location = azurerm_resource_group.test.location + resource_group_name = azurerm_resource_group.test.name + sku = "Basic" +} + +resource "azurerm_eventhub" "test" { + name = "acctest-EH-%[1]d" + namespace_name = azurerm_eventhub_namespace.test.name + resource_group_name = azurerm_resource_group.test.name + partition_count = 2 + message_retention = 1 +} + +resource "azurerm_eventhub_namespace_authorization_rule" "test" { + name = "example" + namespace_name = azurerm_eventhub_namespace.test.name + resource_group_name = azurerm_resource_group.test.name + listen = true + send = true + manage = true +} + +resource "azurerm_key_vault" "test" { + name = "acctest%[3]d" + location = azurerm_resource_group.test.location + resource_group_name = azurerm_resource_group.test.name + tenant_id = data.azurerm_client_config.current.tenant_id + sku_name = "standard" +} + +resource "azurerm_monitor_diagnostic_setting" "test" { + name = "acctest-DS-%[1]d" + target_resource_id = azurerm_key_vault.test.id + eventhub_authorization_rule_id = azurerm_eventhub_namespace_authorization_rule.test.id + eventhub_name = azurerm_eventhub.test.name + + enabled_log { + category = "AuditEvent" + + retention_policy { + enabled = false + } + } + + metric { + category = "AllMetrics" + enabled = true + + retention_policy { + enabled = false + days = 7 + } + } +} +`, data.RandomInteger, data.Locations.Primary, data.RandomIntOfLength(17)) +} From c0c9bcdce5656ffb08cfe9bd562e0013654954e8 Mon Sep 17 00:00:00 2001 From: Catriona Date: Wed, 30 Nov 2022 18:17:19 +0000 Subject: [PATCH 2/9] deprecate log in favour of enabled_log --- .../monitor_diagnostic_setting_resource.go | 89 +++++-------------- ...onitor_diagnostic_setting_resource_test.go | 15 ++-- .../monitor_diagnostic_setting.html.markdown | 18 +++- 3 files changed, 45 insertions(+), 77 deletions(-) diff --git a/internal/services/monitor/monitor_diagnostic_setting_resource.go b/internal/services/monitor/monitor_diagnostic_setting_resource.go index 19bb6ae9ed87..35d30e5ba4d0 100644 --- a/internal/services/monitor/monitor_diagnostic_setting_resource.go +++ b/internal/services/monitor/monitor_diagnostic_setting_resource.go @@ -103,10 +103,11 @@ func resourceMonitorDiagnosticSetting() *pluginsdk.Resource { }, "log": { - Type: pluginsdk.TypeSet, - Optional: true, - Computed: true, - Deprecated: "`log` will be removed in favour of the properties `enabled_log` and `disabled_log` in version 4.0 of the AzureRM Provider.", + Type: pluginsdk.TypeSet, + Optional: true, + Computed: true, + AtLeastOneOf: []string{"enabled_log", "log", "metric"}, + Deprecated: "`log` will be removed in favour of the properties `enabled_log` in version 4.0 of the AzureRM Provider.", Elem: &pluginsdk.Resource{ Schema: map[string]*pluginsdk.Schema{ "category": { @@ -154,6 +155,7 @@ func resourceMonitorDiagnosticSetting() *pluginsdk.Resource { Optional: true, Computed: true, ConflictsWith: []string{"log"}, + AtLeastOneOf: []string{"enabled_log", "log", "metric"}, Elem: &pluginsdk.Resource{ Schema: map[string]*pluginsdk.Schema{ "category": { @@ -190,46 +192,10 @@ func resourceMonitorDiagnosticSetting() *pluginsdk.Resource { Set: resourceMonitorDiagnosticLogSettingHash, }, - "disabled_log": { - Type: pluginsdk.TypeSet, - Computed: true, - Elem: &pluginsdk.Resource{ - Schema: map[string]*pluginsdk.Schema{ - "category": { - Type: pluginsdk.TypeString, - Computed: true, - }, - - "category_group": { - Type: pluginsdk.TypeString, - Computed: true, - }, - - "retention_policy": { - Type: pluginsdk.TypeList, - Computed: true, - Elem: &pluginsdk.Resource{ - Schema: map[string]*pluginsdk.Schema{ - "enabled": { - Type: pluginsdk.TypeBool, - Computed: true, - }, - - "days": { - Type: pluginsdk.TypeInt, - Computed: true, - }, - }, - }, - }, - }, - }, - Set: resourceMonitorDiagnosticLogSettingHash, - }, - "metric": { - Type: pluginsdk.TypeSet, - Optional: true, + Type: pluginsdk.TypeSet, + Optional: true, + AtLeastOneOf: []string{"enabled_log", "log", "metric"}, Elem: &pluginsdk.Resource{ Schema: map[string]*pluginsdk.Schema{ "category": { @@ -307,22 +273,15 @@ func resourceMonitorDiagnosticSettingCreate(d *pluginsdk.ResourceData, meta inte break } } - } else if len(enabledLogs) == 0 { - hasEnabledLogs = false } if len(enabledLogs) > 0 { logs = expandMonitorDiagnosticsSettingsEnabledLogs(enabledLogs) } - // if no blocks are specified the API "creates" but 404's on Read - if !hasEnabledLogs && len(metrics) == 0 { - return fmt.Errorf("at least one `log` or `metric` block must be specified") - } - // also if there's none enabled valid := false - if !valid { + if !hasEnabledLogs { for _, v := range metrics { if v.Enabled { valid = true @@ -614,13 +573,11 @@ func resourceMonitorDiagnosticSettingRead(d *pluginsdk.ResourceData, meta interf d.Set("log_analytics_destination_type", resp.Model.Properties.LogAnalyticsDestinationType) - enabledLogs, disabledLogs := flattenMonitorDiagnosticEnabledAndDisabledLogs(resp.Model.Properties.Logs) + enabledLogs := flattenMonitorDiagnosticEnabledLogs(resp.Model.Properties.Logs) if err = d.Set("enabled_log", enabledLogs); err != nil { return fmt.Errorf("setting `enabled_log`: %+v", err) } - if err = d.Set("disabled_log", disabledLogs); err != nil { - return fmt.Errorf("setting `disabled_log`: %+v", err) - } + if err = d.Set("log", flattenMonitorDiagnosticLogs(resp.Model.Properties.Logs)); err != nil { return fmt.Errorf("setting `log`: %+v", err) } @@ -795,16 +752,19 @@ func flattenMonitorDiagnosticLogs(input *[]diagnosticsettings.LogSettings) []int return results } -func flattenMonitorDiagnosticEnabledAndDisabledLogs(input *[]diagnosticsettings.LogSettings) ([]interface{}, []interface{}) { +func flattenMonitorDiagnosticEnabledLogs(input *[]diagnosticsettings.LogSettings) []interface{} { enabledLogs := make([]interface{}, 0) - disabledLogs := make([]interface{}, 0) if input == nil { - return enabledLogs, disabledLogs + return enabledLogs } for _, v := range *input { output := make(map[string]interface{}) + if !v.Enabled { + continue + } + category := "" if v.Category != nil { category = *v.Category @@ -824,21 +784,16 @@ func flattenMonitorDiagnosticEnabledAndDisabledLogs(input *[]diagnosticsettings. outputPolicy["days"] = int(inputPolicy.Days) - outputPolicy["enabled"] = inputPolicy.Enabled + outputPolicy["enabled"] = true policies = append(policies, outputPolicy) } output["retention_policy"] = policies - if v.Enabled == true { - enabledLogs = append(enabledLogs, output) - } else { - disabledLogs = append(disabledLogs, output) - } + enabledLogs = append(enabledLogs, output) } - - return enabledLogs, disabledLogs + return enabledLogs } func expandMonitorDiagnosticsSettingsMetrics(input []interface{}) []diagnosticsettings.MetricSettings { @@ -911,7 +866,7 @@ func flattenMonitorDiagnosticMetrics(input *[]diagnosticsettings.MetricSettings) func ParseMonitorDiagnosticId(monitorId string) (*diagnosticsettings.ScopedDiagnosticSettingId, error) { v := strings.Split(monitorId, "|") if len(v) != 2 { - return nil, fmt.Errorf("Expected the Monitor Diagnostics ID to be in the format `{resourceId}|{name}` but got %d segments", len(v)) + return nil, fmt.Errorf("expected the Monitor Diagnostics ID to be in the format `{resourceId}|{name}` but got %d segments", len(v)) } identifier := diagnosticsettings.ScopedDiagnosticSettingId{ diff --git a/internal/services/monitor/monitor_diagnostic_setting_resource_test.go b/internal/services/monitor/monitor_diagnostic_setting_resource_test.go index 2b7f0460447b..a1a01c79da34 100644 --- a/internal/services/monitor/monitor_diagnostic_setting_resource_test.go +++ b/internal/services/monitor/monitor_diagnostic_setting_resource_test.go @@ -154,34 +154,31 @@ func TestAccMonitorDiagnosticSetting_activityLog(t *testing.T) { }) } -func TestAccMonitorDiagnosticSetting_enabledAndDisabledLogs(t *testing.T) { +func TestAccMonitorDiagnosticSetting_enabledLogs(t *testing.T) { data := acceptance.BuildTestData(t, "azurerm_monitor_diagnostic_setting", "test") r := MonitorDiagnosticSettingResource{} data.ResourceTest(t, r, []acceptance.TestStep{ { - Config: r.enabledAndDisabledLogs(data), + Config: r.enabledLogs(data), Check: acceptance.ComposeTestCheckFunc( check.That(data.ResourceName).ExistsInAzure(r), check.That(data.ResourceName).Key("enabled_log.#").HasValue("2"), - check.That(data.ResourceName).Key("disabled_log.#").HasValue("0"), ), }, data.ImportStep(), { - Config: r.enabledAndDisabledLogsUpdated(data), + Config: r.enabledLogsUpdated(data), Check: acceptance.ComposeTestCheckFunc( check.That(data.ResourceName).ExistsInAzure(r), check.That(data.ResourceName).Key("enabled_log.#").HasValue("1"), - check.That(data.ResourceName).Key("disabled_log.#").HasValue("1"), ), }, data.ImportStep(), { - Config: r.enabledAndDisabledLogs(data), + Config: r.enabledLogs(data), Check: acceptance.ComposeTestCheckFunc( check.That(data.ResourceName).ExistsInAzure(r), check.That(data.ResourceName).Key("enabled_log.#").HasValue("2"), - check.That(data.ResourceName).Key("disabled_log.#").HasValue("0"), ), }, }) @@ -791,7 +788,7 @@ resource "azurerm_monitor_diagnostic_setting" "test" { `, data.RandomInteger, data.Locations.Primary, data.RandomIntOfLength(17)) } -func (MonitorDiagnosticSettingResource) enabledAndDisabledLogs(data acceptance.TestData) string { +func (MonitorDiagnosticSettingResource) enabledLogs(data acceptance.TestData) string { return fmt.Sprintf(` provider "azurerm" { features {} @@ -873,7 +870,7 @@ resource "azurerm_monitor_diagnostic_setting" "test" { `, data.RandomInteger, data.Locations.Primary, data.RandomIntOfLength(17)) } -func (MonitorDiagnosticSettingResource) enabledAndDisabledLogsUpdated(data acceptance.TestData) string { +func (MonitorDiagnosticSettingResource) enabledLogsUpdated(data acceptance.TestData) string { return fmt.Sprintf(` provider "azurerm" { features {} diff --git a/website/docs/r/monitor_diagnostic_setting.html.markdown b/website/docs/r/monitor_diagnostic_setting.html.markdown index 84edcfe604ef..121fff48505b 100644 --- a/website/docs/r/monitor_diagnostic_setting.html.markdown +++ b/website/docs/r/monitor_diagnostic_setting.html.markdown @@ -75,7 +75,9 @@ The following arguments are supported: * `log` - (Optional) One or more `log` blocks as defined below. --> **NOTE:** At least one `log` or `metric` block must be specified. +* `enabled_log` - (Optional) One or more `enabled_log` blocks as defined below. + +-> **NOTE:** At least one `log`, `enabled_log` or `metric` block must be specified. * `log_analytics_workspace_id` - (Optional) Specifies the ID of a Log Analytics Workspace where Diagnostics Data should be sent. @@ -115,6 +117,20 @@ A `log` block supports the following: --- +An `enabled_log` block supports the following: + +* `category` - (Optional) The name of a Diagnostic Log Category for this Resource. + +-> **NOTE:** The Log Categories available vary depending on the Resource being used. You may wish to use [the `azurerm_monitor_diagnostic_categories` Data Source](../d/monitor_diagnostic_categories.html) or [list of service specific schemas](https://docs.microsoft.com/azure/azure-monitor/platform/resource-logs-schema#service-specific-schemas) to identify which categories are available for a given Resource. + +* `category_group` - (Optional) The name of a Diagnostic Log Category Group for this Resource. + +-> **NOTE:** Not all resources have category groups available.**** + +* `retention_policy` - (Optional) A `retention_policy` block as defined below. + +--- + A `metric` block supports the following: * `category` - (Required) The name of a Diagnostic Metric Category for this Resource. From 649e610833845903e075b731859b2b1d82fb1204 Mon Sep 17 00:00:00 2001 From: Catriona Date: Wed, 30 Nov 2022 19:02:28 +0000 Subject: [PATCH 3/9] add check for empty string --- .../monitor/monitor_diagnostic_setting_resource.go | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/internal/services/monitor/monitor_diagnostic_setting_resource.go b/internal/services/monitor/monitor_diagnostic_setting_resource.go index 35d30e5ba4d0..8b2846744626 100644 --- a/internal/services/monitor/monitor_diagnostic_setting_resource.go +++ b/internal/services/monitor/monitor_diagnostic_setting_resource.go @@ -279,7 +279,7 @@ func resourceMonitorDiagnosticSettingCreate(d *pluginsdk.ResourceData, meta inte logs = expandMonitorDiagnosticsSettingsEnabledLogs(enabledLogs) } - // also if there's none enabled + // if no logs/metrics are not enabled the API "creates" but 404's on Read valid := false if !hasEnabledLogs { for _, v := range metrics { @@ -386,14 +386,9 @@ func resourceMonitorDiagnosticSettingUpdate(d *pluginsdk.ResourceData, meta inte logs = expandMonitorDiagnosticsSettingsEnabledLogs(enabledLogs) } - // if no blocks are specified the API "creates" but 404's on Read - if !hasEnabledLogs && len(metrics) == 0 { - return fmt.Errorf("at least one `log`, `enabled_log` or `metric` block must be specified") - } - - // also if there's none enabled + // if no logs/metrics are not enabled the API "creates" but 404's on Read valid := false - if !valid { + if !hasEnabledLogs { for _, v := range metrics { if v.Enabled { valid = true @@ -417,7 +412,7 @@ func resourceMonitorDiagnosticSettingUpdate(d *pluginsdk.ResourceData, meta inte newLogMap := newLog.(map[string]interface{}) // check if an enabled_log has been removed from config and if so, set to disabled - if strings.EqualFold(oldLogMap["category"].(string), newLogMap["category"].(string)) || (oldLogMap["category_group"].(string) != "" && strings.EqualFold(oldLogMap["category_group"].(string), newLogMap["category_group"].(string))) { + if (oldLogMap["category"].(string) != "" && strings.EqualFold(oldLogMap["category"].(string), newLogMap["category"].(string))) || (oldLogMap["category_group"].(string) != "" && strings.EqualFold(oldLogMap["category_group"].(string), newLogMap["category_group"].(string))) { logRemoved = false break } From 713a06ce2d227622c33e0cd9283d7e08409f2a6c Mon Sep 17 00:00:00 2001 From: Catriona Date: Tue, 6 Dec 2022 21:39:35 +0000 Subject: [PATCH 4/9] add 4.0 beta flag and review comments --- .../monitor_diagnostic_setting_resource.go | 154 ++++++++++-------- ...onitor_diagnostic_setting_resource_test.go | 11 +- 2 files changed, 93 insertions(+), 72 deletions(-) diff --git a/internal/services/monitor/monitor_diagnostic_setting_resource.go b/internal/services/monitor/monitor_diagnostic_setting_resource.go index 8b2846744626..f4d6571de96b 100644 --- a/internal/services/monitor/monitor_diagnostic_setting_resource.go +++ b/internal/services/monitor/monitor_diagnostic_setting_resource.go @@ -15,6 +15,7 @@ import ( "github.com/hashicorp/terraform-provider-azurerm/helpers/azure" "github.com/hashicorp/terraform-provider-azurerm/helpers/tf" "github.com/hashicorp/terraform-provider-azurerm/internal/clients" + "github.com/hashicorp/terraform-provider-azurerm/internal/features" eventhubValidate "github.com/hashicorp/terraform-provider-azurerm/internal/services/eventhub/validate" "github.com/hashicorp/terraform-provider-azurerm/internal/services/monitor/validate" storageParse "github.com/hashicorp/terraform-provider-azurerm/internal/services/storage/parse" @@ -26,7 +27,7 @@ import ( ) func resourceMonitorDiagnosticSetting() *pluginsdk.Resource { - return &pluginsdk.Resource{ + resource := &pluginsdk.Resource{ Create: resourceMonitorDiagnosticSettingCreate, Read: resourceMonitorDiagnosticSettingRead, Update: resourceMonitorDiagnosticSettingUpdate, @@ -102,58 +103,10 @@ func resourceMonitorDiagnosticSetting() *pluginsdk.Resource { }, false), }, - "log": { - Type: pluginsdk.TypeSet, - Optional: true, - Computed: true, - AtLeastOneOf: []string{"enabled_log", "log", "metric"}, - Deprecated: "`log` will be removed in favour of the properties `enabled_log` in version 4.0 of the AzureRM Provider.", - Elem: &pluginsdk.Resource{ - Schema: map[string]*pluginsdk.Schema{ - "category": { - Type: pluginsdk.TypeString, - Optional: true, - }, - - "category_group": { - Type: pluginsdk.TypeString, - Optional: true, - }, - - "enabled": { - Type: pluginsdk.TypeBool, - Optional: true, - Default: true, - }, - - "retention_policy": { - Type: pluginsdk.TypeList, - Optional: true, - MaxItems: 1, - Elem: &pluginsdk.Resource{ - Schema: map[string]*pluginsdk.Schema{ - "enabled": { - Type: pluginsdk.TypeBool, - Required: true, - }, - - "days": { - Type: pluginsdk.TypeInt, - Optional: true, - ValidateFunc: validation.IntAtLeast(0), - }, - }, - }, - }, - }, - }, - Set: resourceMonitorDiagnosticLogSettingHash, - }, - "enabled_log": { Type: pluginsdk.TypeSet, Optional: true, - Computed: true, + Computed: !features.FourPointOhBeta(), ConflictsWith: []string{"log"}, AtLeastOneOf: []string{"enabled_log", "log", "metric"}, Elem: &pluginsdk.Resource{ @@ -234,6 +187,57 @@ func resourceMonitorDiagnosticSetting() *pluginsdk.Resource { }, }, } + if !features.FourPointOhBeta() { + resource.Schema["log"] = &pluginsdk.Schema{ + Type: pluginsdk.TypeSet, + Optional: true, + Computed: true, + AtLeastOneOf: []string{"enabled_log", "log", "metric"}, + Deprecated: "`log` will be removed in favour of the properties `enabled_log` in version 4.0 of the AzureRM Provider.", + Elem: &pluginsdk.Resource{ + Schema: map[string]*pluginsdk.Schema{ + "category": { + Type: pluginsdk.TypeString, + Optional: true, + }, + + "category_group": { + Type: pluginsdk.TypeString, + Optional: true, + }, + + "enabled": { + Type: pluginsdk.TypeBool, + Optional: true, + Default: true, + }, + + "retention_policy": { + Type: pluginsdk.TypeList, + Optional: true, + MaxItems: 1, + Elem: &pluginsdk.Resource{ + Schema: map[string]*pluginsdk.Schema{ + "enabled": { + Type: pluginsdk.TypeBool, + Required: true, + }, + + "days": { + Type: pluginsdk.TypeInt, + Optional: true, + ValidateFunc: validation.IntAtLeast(0), + }, + }, + }, + }, + }, + }, + Set: resourceMonitorDiagnosticLogSettingHash, + } + } + + return resource } func resourceMonitorDiagnosticSettingCreate(d *pluginsdk.ResourceData, meta interface{}) error { @@ -257,26 +261,28 @@ func resourceMonitorDiagnosticSettingCreate(d *pluginsdk.ResourceData, meta inte return tf.ImportAsExistsError("azurerm_monitor_diagnostic_setting", *existing.Model.Id) } - logsRaw := d.Get("log").(*pluginsdk.Set).List() enabledLogs := d.Get("enabled_log").(*pluginsdk.Set).List() metricsRaw := d.Get("metric").(*pluginsdk.Set).List() metrics := expandMonitorDiagnosticsSettingsMetrics(metricsRaw) var logs []diagnosticsettings.LogSettings - hasEnabledLogs := true - if len(logsRaw) > 0 { - hasEnabledLogs = false - logs = expandMonitorDiagnosticsSettingsLogs(logsRaw) - for _, v := range logs { - if v.Enabled { - hasEnabledLogs = true - break + hasEnabledLogs := false + if !features.FourPointOhBeta() { + logsRaw := d.Get("log").(*pluginsdk.Set).List() + if len(logsRaw) > 0 { + logs = expandMonitorDiagnosticsSettingsLogs(logsRaw) + for _, v := range logs { + if v.Enabled { + hasEnabledLogs = true + break + } } } } if len(enabledLogs) > 0 { logs = expandMonitorDiagnosticsSettingsEnabledLogs(enabledLogs) + hasEnabledLogs = true } // if no logs/metrics are not enabled the API "creates" but 404's on Read @@ -370,20 +376,24 @@ func resourceMonitorDiagnosticSettingUpdate(d *pluginsdk.ResourceData, meta inte metrics := expandMonitorDiagnosticsSettingsMetrics(metricsRaw) var logs []diagnosticsettings.LogSettings - hasEnabledLogs := true - if d.HasChange("log") { - logsRaw := d.Get("log").(*pluginsdk.Set).List() - logs = expandMonitorDiagnosticsSettingsLogs(logsRaw) - hasEnabledLogs = false - for _, v := range logs { - if v.Enabled { - hasEnabledLogs = true - break + hasEnabledLogs := false + if !features.FourPointOhBeta() { + if d.HasChange("log") { + logsRaw := d.Get("log").(*pluginsdk.Set).List() + logs = expandMonitorDiagnosticsSettingsLogs(logsRaw) + for _, v := range logs { + if v.Enabled { + hasEnabledLogs = true + break + } } } - } else if d.HasChange("enabled_log") { + } + + if d.HasChange("enabled_log") { enabledLogs := d.Get("enabled_log").(*pluginsdk.Set).List() logs = expandMonitorDiagnosticsSettingsEnabledLogs(enabledLogs) + hasEnabledLogs = true } // if no logs/metrics are not enabled the API "creates" but 404's on Read @@ -573,8 +583,10 @@ func resourceMonitorDiagnosticSettingRead(d *pluginsdk.ResourceData, meta interf return fmt.Errorf("setting `enabled_log`: %+v", err) } - if err = d.Set("log", flattenMonitorDiagnosticLogs(resp.Model.Properties.Logs)); err != nil { - return fmt.Errorf("setting `log`: %+v", err) + if !features.FourPointOhBeta() { + if err = d.Set("log", flattenMonitorDiagnosticLogs(resp.Model.Properties.Logs)); err != nil { + return fmt.Errorf("setting `log`: %+v", err) + } } if err := d.Set("metric", flattenMonitorDiagnosticMetrics(resp.Model.Properties.Metrics)); err != nil { diff --git a/internal/services/monitor/monitor_diagnostic_setting_resource_test.go b/internal/services/monitor/monitor_diagnostic_setting_resource_test.go index a1a01c79da34..78690ce2355c 100644 --- a/internal/services/monitor/monitor_diagnostic_setting_resource_test.go +++ b/internal/services/monitor/monitor_diagnostic_setting_resource_test.go @@ -26,7 +26,6 @@ func TestAccMonitorDiagnosticSetting_eventhub(t *testing.T) { check.That(data.ResourceName).Key("eventhub_name").Exists(), check.That(data.ResourceName).Key("eventhub_authorization_rule_id").Exists(), check.That(data.ResourceName).Key("log.#").HasValue("2"), - check.That(data.ResourceName).Key("disabled_log.#").HasValue("1"), check.That(data.ResourceName).Key("metric.#").HasValue("1"), ), }, @@ -259,6 +258,16 @@ resource "azurerm_monitor_diagnostic_setting" "test" { } } + log { + category = "AzurePolicyEvaluationDetails" + enabled = false + + retention_policy { + days = 0 + enabled = false + } + } + metric { category = "AllMetrics" enabled = true From 357ca655de98d1b55647c2a84bc867d3d1ef1f6e Mon Sep 17 00:00:00 2001 From: Catriona Date: Wed, 7 Dec 2022 18:49:29 +0000 Subject: [PATCH 5/9] fix ids --- .../monitor_diagnostic_setting_resource.go | 52 ++++++++----------- 1 file changed, 21 insertions(+), 31 deletions(-) diff --git a/internal/services/monitor/monitor_diagnostic_setting_resource.go b/internal/services/monitor/monitor_diagnostic_setting_resource.go index f4d6571de96b..dce17171f51c 100644 --- a/internal/services/monitor/monitor_diagnostic_setting_resource.go +++ b/internal/services/monitor/monitor_diagnostic_setting_resource.go @@ -242,18 +242,16 @@ func resourceMonitorDiagnosticSetting() *pluginsdk.Resource { func resourceMonitorDiagnosticSettingCreate(d *pluginsdk.ResourceData, meta interface{}) error { client := meta.(*clients.Client).Monitor.DiagnosticSettingsClient - ctx, cancel := timeouts.ForCreateUpdate(meta.(*clients.Client).StopContext, d) + ctx, cancel := timeouts.ForCreate(meta.(*clients.Client).StopContext, d) defer cancel() log.Printf("[INFO] preparing arguments for Azure ARM Diagnostic Settings.") - name := d.Get("name").(string) - actualResourceId := d.Get("target_resource_id").(string) - diagnosticSettingId := diagnosticsettings.NewScopedDiagnosticSettingID(actualResourceId, name) + id := diagnosticsettings.NewScopedDiagnosticSettingID(d.Get("target_resource_id").(string), d.Get("name").(string)) - existing, err := client.Get(ctx, diagnosticSettingId) + existing, err := client.Get(ctx, id) if err != nil { if !response.WasNotFound(existing.HttpResponse) { - return fmt.Errorf("checking for presence of existing Monitor Diagnostic Setting %q for Resource %q: %s", diagnosticSettingId.Name, diagnosticSettingId.ResourceUri, err) + return fmt.Errorf("checking for presence of existing Monitor Diagnostic Setting %q for Resource %q: %s", id.Name, id.ResourceUri, err) } } @@ -261,7 +259,6 @@ func resourceMonitorDiagnosticSettingCreate(d *pluginsdk.ResourceData, meta inte return tf.ImportAsExistsError("azurerm_monitor_diagnostic_setting", *existing.Model.Id) } - enabledLogs := d.Get("enabled_log").(*pluginsdk.Set).List() metricsRaw := d.Get("metric").(*pluginsdk.Set).List() metrics := expandMonitorDiagnosticsSettingsMetrics(metricsRaw) @@ -280,9 +277,12 @@ func resourceMonitorDiagnosticSettingCreate(d *pluginsdk.ResourceData, meta inte } } - if len(enabledLogs) > 0 { - logs = expandMonitorDiagnosticsSettingsEnabledLogs(enabledLogs) - hasEnabledLogs = true + if enabledLogs, ok := d.GetOk("enabled_log"); ok { + enabledLogsList := enabledLogs.(*pluginsdk.Set).List() + if len(enabledLogsList) > 0 { + logs = expandMonitorDiagnosticsSettingsEnabledLogs(enabledLogsList) + hasEnabledLogs = true + } } // if no logs/metrics are not enabled the API "creates" but 404's on Read @@ -346,31 +346,22 @@ func resourceMonitorDiagnosticSettingCreate(d *pluginsdk.ResourceData, meta inte return fmt.Errorf("either a `eventhub_authorization_rule_id`, `log_analytics_workspace_id`, `partner_solution_id` or `storage_account_id` must be set") } - if _, err := client.CreateOrUpdate(ctx, diagnosticSettingId, parameters); err != nil { - return fmt.Errorf("creating Monitor Diagnostics Setting %q for Resource %q: %+v", name, actualResourceId, err) - } - - read, err := client.Get(ctx, diagnosticSettingId) - if err != nil { - return err - } - if read.Model == nil && read.Model.Id == nil { - return fmt.Errorf("cannot read ID for Monitor Diagnostics %q for Resource ID %q", diagnosticSettingId.Name, diagnosticSettingId.ResourceUri) + if _, err := client.CreateOrUpdate(ctx, id, parameters); err != nil { + return fmt.Errorf("creating Monitor Diagnostics Setting %q for Resource %q: %+v", id.Name, id.ResourceUri, err) } - d.SetId(fmt.Sprintf("%s|%s", actualResourceId, name)) + d.SetId(fmt.Sprintf("%s|%s", id.ResourceUri, id.Name)) return resourceMonitorDiagnosticSettingRead(d, meta) } func resourceMonitorDiagnosticSettingUpdate(d *pluginsdk.ResourceData, meta interface{}) error { client := meta.(*clients.Client).Monitor.DiagnosticSettingsClient - ctx, cancel := timeouts.ForCreateUpdate(meta.(*clients.Client).StopContext, d) + ctx, cancel := timeouts.ForUpdate(meta.(*clients.Client).StopContext, d) defer cancel() log.Printf("[INFO] preparing arguments for Azure ARM Diagnostic Settings.") - name := d.Get("name").(string) - actualResourceId := d.Get("target_resource_id").(string) - diagnosticSettingId := diagnosticsettings.NewScopedDiagnosticSettingID(actualResourceId, name) + + id := diagnosticsettings.NewScopedDiagnosticSettingID(d.Get("target_resource_id").(string), d.Get("name").(string)) metricsRaw := d.Get("metric").(*pluginsdk.Set).List() metrics := expandMonitorDiagnosticsSettingsMetrics(metricsRaw) @@ -503,8 +494,8 @@ func resourceMonitorDiagnosticSettingUpdate(d *pluginsdk.ResourceData, meta inte return fmt.Errorf("either a `eventhub_authorization_rule_id`, `log_analytics_workspace_id`, `partner_solution_id` or `storage_account_id` must be set") } - if _, err := client.CreateOrUpdate(ctx, diagnosticSettingId, parameters); err != nil { - return fmt.Errorf("updating Monitor Diagnostics Setting %q for Resource %q: %+v", name, actualResourceId, err) + if _, err := client.CreateOrUpdate(ctx, id, parameters); err != nil { + return fmt.Errorf("updating Monitor Diagnostics Setting %q for Resource %q: %+v", id.Name, id.ResourceUri, err) } return resourceMonitorDiagnosticSettingRead(d, meta) } @@ -519,16 +510,15 @@ func resourceMonitorDiagnosticSettingRead(d *pluginsdk.ResourceData, meta interf return err } - actualResourceId := id.ResourceUri resp, err := client.Get(ctx, *id) if err != nil { if response.WasNotFound(resp.HttpResponse) { - log.Printf("[WARN] Monitor Diagnostics Setting %q was not found for Resource %q - removing from state!", id.Name, actualResourceId) + log.Printf("[WARN] Monitor Diagnostics Setting %q was not found for Resource %q - removing from state!", id.Name, id.ResourceUri) d.SetId("") return nil } - return fmt.Errorf("retrieving Monitor Diagnostics Setting %q for Resource %q: %+v", id.Name, actualResourceId, err) + return fmt.Errorf("retrieving Monitor Diagnostics Setting %q for Resource %q: %+v", id.Name, id.ResourceUri, err) } d.Set("name", id.Name) @@ -791,7 +781,7 @@ func flattenMonitorDiagnosticEnabledLogs(input *[]diagnosticsettings.LogSettings outputPolicy["days"] = int(inputPolicy.Days) - outputPolicy["enabled"] = true + outputPolicy["enabled"] = inputPolicy.Enabled policies = append(policies, outputPolicy) } From 49cb70f82c319c116800f02c0cdd8cc9568fb959 Mon Sep 17 00:00:00 2001 From: Catriona Date: Thu, 8 Dec 2022 10:31:14 +0000 Subject: [PATCH 6/9] use parser in update --- .../monitor/monitor_diagnostic_setting_resource.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/services/monitor/monitor_diagnostic_setting_resource.go b/internal/services/monitor/monitor_diagnostic_setting_resource.go index dce17171f51c..b4fcaa1981be 100644 --- a/internal/services/monitor/monitor_diagnostic_setting_resource.go +++ b/internal/services/monitor/monitor_diagnostic_setting_resource.go @@ -361,7 +361,10 @@ func resourceMonitorDiagnosticSettingUpdate(d *pluginsdk.ResourceData, meta inte defer cancel() log.Printf("[INFO] preparing arguments for Azure ARM Diagnostic Settings.") - id := diagnosticsettings.NewScopedDiagnosticSettingID(d.Get("target_resource_id").(string), d.Get("name").(string)) + id, err := ParseMonitorDiagnosticId(d.Id()) + if err != nil { + return err + } metricsRaw := d.Get("metric").(*pluginsdk.Set).List() metrics := expandMonitorDiagnosticsSettingsMetrics(metricsRaw) @@ -494,7 +497,7 @@ func resourceMonitorDiagnosticSettingUpdate(d *pluginsdk.ResourceData, meta inte return fmt.Errorf("either a `eventhub_authorization_rule_id`, `log_analytics_workspace_id`, `partner_solution_id` or `storage_account_id` must be set") } - if _, err := client.CreateOrUpdate(ctx, id, parameters); err != nil { + if _, err := client.CreateOrUpdate(ctx, *id, parameters); err != nil { return fmt.Errorf("updating Monitor Diagnostics Setting %q for Resource %q: %+v", id.Name, id.ResourceUri, err) } return resourceMonitorDiagnosticSettingRead(d, meta) From 3c3da296d8adb11a65b7fd695831be42defbe066 Mon Sep 17 00:00:00 2001 From: Catriona Date: Thu, 5 Jan 2023 10:41:15 +0000 Subject: [PATCH 7/9] fix tests --- .../monitor_diagnostic_setting_resource.go | 12 +---- ...onitor_diagnostic_setting_resource_test.go | 45 ++++++++++++++----- 2 files changed, 35 insertions(+), 22 deletions(-) diff --git a/internal/services/monitor/monitor_diagnostic_setting_resource.go b/internal/services/monitor/monitor_diagnostic_setting_resource.go index b4fcaa1981be..09fef1f44b30 100644 --- a/internal/services/monitor/monitor_diagnostic_setting_resource.go +++ b/internal/services/monitor/monitor_diagnostic_setting_resource.go @@ -335,11 +335,7 @@ func resourceMonitorDiagnosticSettingCreate(d *pluginsdk.ResourceData, meta inte } if v := d.Get("log_analytics_destination_type").(string); v != "" { - if workspaceId != "" { - parameters.Properties.LogAnalyticsDestinationType = &v - } else { - return fmt.Errorf("`log_analytics_workspace_id` must be set for `log_analytics_destination_type` to be used") - } + parameters.Properties.LogAnalyticsDestinationType = &v } if !valid { @@ -486,11 +482,7 @@ func resourceMonitorDiagnosticSettingUpdate(d *pluginsdk.ResourceData, meta inte } if v := d.Get("log_analytics_destination_type").(string); v != "" { - if workspaceId != "" { - parameters.Properties.LogAnalyticsDestinationType = &v - } else { - return fmt.Errorf("`log_analytics_workspace_id` must be set for `log_analytics_destination_type` to be used") - } + parameters.Properties.LogAnalyticsDestinationType = &v } if !valid { diff --git a/internal/services/monitor/monitor_diagnostic_setting_resource_test.go b/internal/services/monitor/monitor_diagnostic_setting_resource_test.go index 78690ce2355c..d9dffe20c8fe 100644 --- a/internal/services/monitor/monitor_diagnostic_setting_resource_test.go +++ b/internal/services/monitor/monitor_diagnostic_setting_resource_test.go @@ -248,12 +248,14 @@ resource "azurerm_monitor_diagnostic_setting" "test" { target_resource_id = azurerm_key_vault.test.id eventhub_authorization_rule_id = azurerm_eventhub_namespace_authorization_rule.test.id eventhub_name = azurerm_eventhub.test.name + log_analytics_destination_type = "AzureDiagnostics" log { category = "AuditEvent" enabled = false retention_policy { + days = 0 enabled = false } } @@ -332,6 +334,7 @@ resource "azurerm_monitor_diagnostic_setting" "test" { target_resource_id = azurerm_key_vault.test.id eventhub_authorization_rule_id = azurerm_eventhub_namespace_authorization_rule.test.id eventhub_name = azurerm_eventhub.test.name + log_analytics_destination_type = "AzureDiagnostics" log { category_group = "Audit" @@ -357,6 +360,7 @@ resource "azurerm_monitor_diagnostic_setting" "test" { category = "AllMetrics" retention_policy { + days = 0 enabled = false } } @@ -378,6 +382,7 @@ resource "azurerm_monitor_diagnostic_setting" "import" { enabled = false retention_policy { + days = 0 enabled = false } } @@ -386,6 +391,7 @@ resource "azurerm_monitor_diagnostic_setting" "import" { category = "AllMetrics" retention_policy { + days = 0 enabled = false } } @@ -424,15 +430,17 @@ resource "azurerm_key_vault" "test" { } resource "azurerm_monitor_diagnostic_setting" "test" { - name = "acctest-DS-%[1]d" - target_resource_id = azurerm_key_vault.test.id - log_analytics_workspace_id = azurerm_log_analytics_workspace.test.id + name = "acctest-DS-%[1]d" + target_resource_id = azurerm_key_vault.test.id + log_analytics_workspace_id = azurerm_log_analytics_workspace.test.id + log_analytics_destination_type = "AzureDiagnostics" log { category = "AuditEvent" enabled = false retention_policy { + days = 0 enabled = false } } @@ -451,6 +459,7 @@ resource "azurerm_monitor_diagnostic_setting" "test" { category = "AllMetrics" retention_policy { + days = 0 enabled = false } } @@ -584,6 +593,7 @@ resource "azurerm_monitor_diagnostic_setting" "test" { metric { category = "AllMetrics" retention_policy { + days = 0 enabled = false } } @@ -622,15 +632,17 @@ resource "azurerm_elastic_cloud_elasticsearch" "test" { } resource "azurerm_monitor_diagnostic_setting" "test" { - name = "acctest-DS-%[1]d" - target_resource_id = azurerm_key_vault.test.id - partner_solution_id = azurerm_elastic_cloud_elasticsearch.test.id + name = "acctest-DS-%[1]d" + target_resource_id = azurerm_key_vault.test.id + partner_solution_id = azurerm_elastic_cloud_elasticsearch.test.id + log_analytics_destination_type = "AzureDiagnostics" log { category = "AuditEvent" enabled = false retention_policy { + days = 0 enabled = false } } @@ -649,6 +661,7 @@ resource "azurerm_monitor_diagnostic_setting" "test" { category = "AllMetrics" retention_policy { + days = 0 enabled = false } } @@ -687,15 +700,17 @@ resource "azurerm_key_vault" "test" { } resource "azurerm_monitor_diagnostic_setting" "test" { - name = "acctest-DS-%[1]d" - target_resource_id = azurerm_key_vault.test.id - storage_account_id = azurerm_storage_account.test.id + name = "acctest-DS-%[1]d" + target_resource_id = azurerm_key_vault.test.id + storage_account_id = azurerm_storage_account.test.id + log_analytics_destination_type = "AzureDiagnostics" log { category = "AuditEvent" enabled = false retention_policy { + days = 0 enabled = false } } @@ -714,6 +729,7 @@ resource "azurerm_monitor_diagnostic_setting" "test" { category = "AllMetrics" retention_policy { + days = 0 enabled = false } } @@ -750,9 +766,10 @@ resource "azurerm_storage_account" "test" { resource "azurerm_monitor_diagnostic_setting" "test" { - name = "acctest-DS-%[1]d" - target_resource_id = data.azurerm_subscription.current.id - storage_account_id = azurerm_storage_account.test.id + name = "acctest-DS-%[1]d" + target_resource_id = data.azurerm_subscription.current.id + storage_account_id = azurerm_storage_account.test.id + log_analytics_destination_type = "AzureDiagnostics" log { category = "Administrative" @@ -848,11 +865,13 @@ resource "azurerm_monitor_diagnostic_setting" "test" { target_resource_id = azurerm_key_vault.test.id eventhub_authorization_rule_id = azurerm_eventhub_namespace_authorization_rule.test.id eventhub_name = azurerm_eventhub.test.name + log_analytics_destination_type = "AzureDiagnostics" enabled_log { category = "AuditEvent" retention_policy { + days = 0 enabled = false } } @@ -930,11 +949,13 @@ resource "azurerm_monitor_diagnostic_setting" "test" { target_resource_id = azurerm_key_vault.test.id eventhub_authorization_rule_id = azurerm_eventhub_namespace_authorization_rule.test.id eventhub_name = azurerm_eventhub.test.name + log_analytics_destination_type = "AzureDiagnostics" enabled_log { category = "AuditEvent" retention_policy { + days = 0 enabled = false } } From f4ade82451a2d1f5e89bb8af727b719d24acde8c Mon Sep 17 00:00:00 2001 From: Catriona Date: Thu, 12 Jan 2023 12:21:50 +0000 Subject: [PATCH 8/9] review comments --- .../monitor/monitor_diagnostic_setting_resource.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/services/monitor/monitor_diagnostic_setting_resource.go b/internal/services/monitor/monitor_diagnostic_setting_resource.go index 09fef1f44b30..7c4c3bf62fa1 100644 --- a/internal/services/monitor/monitor_diagnostic_setting_resource.go +++ b/internal/services/monitor/monitor_diagnostic_setting_resource.go @@ -193,7 +193,7 @@ func resourceMonitorDiagnosticSetting() *pluginsdk.Resource { Optional: true, Computed: true, AtLeastOneOf: []string{"enabled_log", "log", "metric"}, - Deprecated: "`log` will be removed in favour of the properties `enabled_log` in version 4.0 of the AzureRM Provider.", + Deprecated: "`log` has been superseded by `enabled_log` and will be removed in version 4.0 of the AzureRM Provider.", Elem: &pluginsdk.Resource{ Schema: map[string]*pluginsdk.Schema{ "category": { @@ -265,9 +265,9 @@ func resourceMonitorDiagnosticSettingCreate(d *pluginsdk.ResourceData, meta inte var logs []diagnosticsettings.LogSettings hasEnabledLogs := false if !features.FourPointOhBeta() { - logsRaw := d.Get("log").(*pluginsdk.Set).List() - if len(logsRaw) > 0 { - logs = expandMonitorDiagnosticsSettingsLogs(logsRaw) + logsRaw, ok := d.GetOk("log") + if ok && len(logsRaw.(*pluginsdk.Set).List()) > 0 { + logs = expandMonitorDiagnosticsSettingsLogs(logsRaw.(*pluginsdk.Set).List()) for _, v := range logs { if v.Enabled { hasEnabledLogs = true @@ -297,7 +297,7 @@ func resourceMonitorDiagnosticSettingCreate(d *pluginsdk.ResourceData, meta inte } if !valid && !hasEnabledLogs { - return fmt.Errorf("at least one `log` or `metric` must be enabled") + return fmt.Errorf("at least one type of Log or Metric must be enabled") } parameters := diagnosticsettings.DiagnosticSettingsResource{ @@ -398,7 +398,7 @@ func resourceMonitorDiagnosticSettingUpdate(d *pluginsdk.ResourceData, meta inte } if !valid && !hasEnabledLogs { - return fmt.Errorf("at least one `log` or `metric` must be enabled") + return fmt.Errorf("at least one type of Log or Metric must be enabled") } if d.HasChange("enabled_log") { From f9fcfa44851718090a65ff0af571f198bfb5e41c Mon Sep 17 00:00:00 2001 From: Catriona Date: Thu, 12 Jan 2023 14:54:44 +0000 Subject: [PATCH 9/9] fix import id error --- .../monitor/monitor_diagnostic_setting_resource.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/services/monitor/monitor_diagnostic_setting_resource.go b/internal/services/monitor/monitor_diagnostic_setting_resource.go index 7c4c3bf62fa1..ed6662dc2e23 100644 --- a/internal/services/monitor/monitor_diagnostic_setting_resource.go +++ b/internal/services/monitor/monitor_diagnostic_setting_resource.go @@ -247,6 +247,7 @@ func resourceMonitorDiagnosticSettingCreate(d *pluginsdk.ResourceData, meta inte log.Printf("[INFO] preparing arguments for Azure ARM Diagnostic Settings.") id := diagnosticsettings.NewScopedDiagnosticSettingID(d.Get("target_resource_id").(string), d.Get("name").(string)) + resourceId := fmt.Sprintf("%s|%s", id.ResourceUri, id.Name) existing, err := client.Get(ctx, id) if err != nil { @@ -255,8 +256,8 @@ func resourceMonitorDiagnosticSettingCreate(d *pluginsdk.ResourceData, meta inte } } - if existing.Model != nil && existing.Model.Id != nil && *existing.Model.Id != "" { - return tf.ImportAsExistsError("azurerm_monitor_diagnostic_setting", *existing.Model.Id) + if !response.WasNotFound(existing.HttpResponse) { + return tf.ImportAsExistsError("azurerm_monitor_diagnostic_setting", resourceId) } metricsRaw := d.Get("metric").(*pluginsdk.Set).List() @@ -346,7 +347,7 @@ func resourceMonitorDiagnosticSettingCreate(d *pluginsdk.ResourceData, meta inte return fmt.Errorf("creating Monitor Diagnostics Setting %q for Resource %q: %+v", id.Name, id.ResourceUri, err) } - d.SetId(fmt.Sprintf("%s|%s", id.ResourceUri, id.Name)) + d.SetId(resourceId) return resourceMonitorDiagnosticSettingRead(d, meta) }