From 083f474ea3e559b0092b257688fdb14a0d90352e Mon Sep 17 00:00:00 2001 From: jackofallops <11830746+jackofallops@users.noreply.github.com> Date: Thu, 30 Jan 2025 16:08:22 +0100 Subject: [PATCH] `containerapps` - fix breaking schema behavioural change (#28639) * fixup breaking behavioural change * fixup CustomizeDiff for computed value * add 5.0 feature flagging * update locations for testing as northcentralus does not support availability zones * remove unnecessary schema description entry * Apply suggestions from code review Co-authored-by: stephybun * Update 5.0-upgrade-guide * expand deprecation description to cover default behaviour --------- Co-authored-by: stephybun --- .teamcity/components/settings.kt | 2 +- .../container_app_environment_resource.go | 99 ++++++++++++++----- website/docs/5.0-upgrade-guide.html.markdown | 4 + .../r/container_app_environment.html.markdown | 2 +- 4 files changed, 79 insertions(+), 28 deletions(-) diff --git a/.teamcity/components/settings.kt b/.teamcity/components/settings.kt index 2df87ed68bd9..bbf575c64790 100644 --- a/.teamcity/components/settings.kt +++ b/.teamcity/components/settings.kt @@ -69,7 +69,7 @@ var serviceTestConfigurationOverrides = mapOf( // Container App Managed Environments are limited to 20 per location, using 10 as they can take some time to clear // Enable rotation test to mitigate resource burden in a single region - "containerapps" to testConfiguration(parallelism = 10, locationOverride = LocationConfiguration("eastus2","westus2","northcentralus", true)), + "containerapps" to testConfiguration(parallelism = 10, locationOverride = LocationConfiguration("eastus2","westus2","southcentralus", true)), // The AKS API has a low rate limit "containers" to testConfiguration(parallelism = 5, locationOverride = LocationConfiguration("eastus","westeurope","eastus2", false), timeout = 18), diff --git a/internal/services/containerapps/container_app_environment_resource.go b/internal/services/containerapps/container_app_environment_resource.go index c61cb9cd4906..838351e3bfa5 100644 --- a/internal/services/containerapps/container_app_environment_resource.go +++ b/internal/services/containerapps/container_app_environment_resource.go @@ -18,6 +18,7 @@ import ( "github.com/hashicorp/go-azure-helpers/resourcemanager/tags" "github.com/hashicorp/go-azure-sdk/resource-manager/containerapps/2024-03-01/managedenvironments" "github.com/hashicorp/go-azure-sdk/resource-manager/operationalinsights/2020-08-01/workspaces" + "github.com/hashicorp/terraform-provider-azurerm/internal/features" "github.com/hashicorp/terraform-provider-azurerm/internal/sdk" "github.com/hashicorp/terraform-provider-azurerm/internal/services/containerapps/helpers" "github.com/hashicorp/terraform-provider-azurerm/internal/services/containerapps/validate" @@ -28,7 +29,7 @@ import ( const ( LogsDestinationLogAnalytics string = "log-analytics" LogsDestinationAzureMonitor string = "azure-monitor" - LogsDestinationAzureNone string = "" + LogsDestinationNone string = "" ) type ContainerAppEnvironmentResource struct{} @@ -74,7 +75,7 @@ func (r ContainerAppEnvironmentResource) IDValidationFunc() pluginsdk.SchemaVali } func (r ContainerAppEnvironmentResource) Arguments() map[string]*pluginsdk.Schema { - return map[string]*pluginsdk.Schema{ + schema := map[string]*pluginsdk.Schema{ "name": { Type: pluginsdk.TypeString, Required: true, @@ -106,12 +107,12 @@ func (r ContainerAppEnvironmentResource) Arguments() map[string]*pluginsdk.Schem "logs_destination": { Type: pluginsdk.TypeString, Optional: true, - Default: LogsDestinationAzureNone, + Default: LogsDestinationNone, ValidateFunc: validation.StringInSlice([]string{ - LogsDestinationLogAnalytics, LogsDestinationAzureMonitor, + LogsDestinationLogAnalytics, }, false), - Description: "The destination for the application logs. Possible values are `log-analytics` or `azure-monitor`.", + Description: "The destination for the application logs. Possible values include `log-analytics` and `azure-monitor`. Omitting this value will result in logs being streamed only.", }, "infrastructure_resource_group_name": { @@ -170,6 +171,21 @@ func (r ContainerAppEnvironmentResource) Arguments() map[string]*pluginsdk.Schem "tags": commonschema.Tags(), } + + if !features.FivePointOh() { + schema["logs_destination"] = &pluginsdk.Schema{ + Type: pluginsdk.TypeString, + Optional: true, + Computed: true, // NOTE: O+C as the introduction of this property is a behavioural change where we previously set it behind the scenes if `log_analytics_workspace_id` was set. + ValidateFunc: validation.StringInSlice([]string{ + LogsDestinationAzureMonitor, + LogsDestinationNone, + LogsDestinationLogAnalytics, + }, false), + } + } + + return schema } func (r ContainerAppEnvironmentResource) Attributes() map[string]*pluginsdk.Schema { @@ -275,6 +291,8 @@ func (r ContainerAppEnvironmentResource) Create() sdk.ResourceFunc { return fmt.Errorf("retrieving access keys to Log Analytics Workspace for %s: %+v", id, err) } + managedEnvironment.Properties.AppLogsConfiguration.Destination = pointer.To(LogsDestinationLogAnalytics) + managedEnvironment.Properties.AppLogsConfiguration.LogAnalyticsConfiguration = &managedenvironments.LogAnalyticsConfiguration{ CustomerId: customerId, SharedKey: sharedKey, @@ -424,6 +442,11 @@ func (r ContainerAppEnvironmentResource) Update() sdk.ResourceFunc { } if metadata.ResourceData.HasChanges("logs_destination", "log_analytics_workspace_id") { + // For 4.x we need to be compensate for the legacy behaviour of setting log destination based on the presence of log_analytics_workspace_id + if !features.FivePointOh() && metadata.ResourceData.GetRawConfig().AsValueMap()["logs_destination"].IsNull() && state.LogAnalyticsWorkspaceId == "" { + state.LogsDestination = LogsDestinationNone + } + switch state.LogsDestination { case LogsDestinationAzureMonitor: existing.Model.Properties.AppLogsConfiguration = &managedenvironments.AppLogsConfiguration{ @@ -431,17 +454,19 @@ func (r ContainerAppEnvironmentResource) Update() sdk.ResourceFunc { LogAnalyticsConfiguration: nil, } case LogsDestinationLogAnalytics: - customerId, sharedKey, err := getSharedKeyForWorkspace(ctx, metadata, state.LogAnalyticsWorkspaceId) - if err != nil { - return fmt.Errorf("retrieving access keys to Log Analytics Workspace for %s: %+v", id, err) - } + if state.LogAnalyticsWorkspaceId != "" { + customerId, sharedKey, err := getSharedKeyForWorkspace(ctx, metadata, state.LogAnalyticsWorkspaceId) + if err != nil { + return fmt.Errorf("retrieving access keys to Log Analytics Workspace for %s: %+v", id, err) + } - existing.Model.Properties.AppLogsConfiguration = &managedenvironments.AppLogsConfiguration{ - Destination: pointer.To(LogsDestinationLogAnalytics), - LogAnalyticsConfiguration: &managedenvironments.LogAnalyticsConfiguration{ - CustomerId: customerId, - SharedKey: sharedKey, - }, + existing.Model.Properties.AppLogsConfiguration = &managedenvironments.AppLogsConfiguration{ + Destination: pointer.To(LogsDestinationLogAnalytics), + LogAnalyticsConfiguration: &managedenvironments.LogAnalyticsConfiguration{ + CustomerId: customerId, + SharedKey: sharedKey, + }, + } } default: @@ -498,20 +523,42 @@ func (r ContainerAppEnvironmentResource) CustomizeDiff() sdk.ResourceFunc { } } - if metadata.ResourceDiff.HasChanges("logs_destination", "log_analytics_workspace_id") { - logsDestination := metadata.ResourceDiff.Get("logs_destination").(string) - logAnalyticsWorkspaceID := metadata.ResourceDiff.Get("log_analytics_workspace_id").(string) - logAnalyticsWorkspaceIDIsNull := metadata.ResourceDiff.GetRawConfig().AsValueMap()["log_analytics_workspace_id"].IsNull() + if !features.FivePointOh() { // in 4.x `logs_destination` is Computed due to legacy code implying destination from presence of a valid id in `log_analytics_workspace_id` so we need to check explicit config values here + if metadata.ResourceDiff.HasChanges("logs_destination", "log_analytics_workspace_id") { + logsDestination := metadata.ResourceDiff.Get("logs_destination").(string) + logDestinationIsNull := metadata.ResourceDiff.GetRawConfig().AsValueMap()["logs_destination"].IsNull() + logAnalyticsWorkspaceID := metadata.ResourceDiff.Get("log_analytics_workspace_id").(string) + logAnalyticsWorkspaceIDIsNull := metadata.ResourceDiff.GetRawConfig().AsValueMap()["log_analytics_workspace_id"].IsNull() + + if !logDestinationIsNull || !logAnalyticsWorkspaceIDIsNull { + switch logsDestination { + case LogsDestinationLogAnalytics: + if logAnalyticsWorkspaceIDIsNull { + return fmt.Errorf("`log_analytics_workspace_id` must be set when `logs_destination` is set to `log-analytics`") + } - switch logsDestination { - case LogsDestinationLogAnalytics: - if logAnalyticsWorkspaceIDIsNull { - return fmt.Errorf("`log_analytics_workspace_id` must be set when `logs_destination` is set to `log-analytics`") + case LogsDestinationAzureMonitor, LogsDestinationNone: + if (logAnalyticsWorkspaceID != "" || !logAnalyticsWorkspaceIDIsNull) && !logDestinationIsNull { + return fmt.Errorf("`log_analytics_workspace_id` can only be set when `logs_destination` is set to `log-analytics` or omitted") + } + } } + } + } else { + if metadata.ResourceDiff.HasChanges("logs_destination", "log_analytics_workspace_id") { + logsDestination := metadata.ResourceDiff.Get("logs_destination").(string) + logAnalyticsWorkspaceID := metadata.ResourceDiff.Get("log_analytics_workspace_id").(string) + + switch logsDestination { + case LogsDestinationLogAnalytics: + if logAnalyticsWorkspaceID == "" { + return fmt.Errorf("`log_analytics_workspace_id` must be set when `logs_destination` is set to `log-analytics`") + } - case LogsDestinationAzureMonitor, LogsDestinationAzureNone: - if logAnalyticsWorkspaceID != "" || !logAnalyticsWorkspaceIDIsNull { - return fmt.Errorf("`log_analytics_workspace_id` can only be set when `logs_destination` is set to `log-analytics`") + case LogsDestinationAzureMonitor, LogsDestinationNone: + if logAnalyticsWorkspaceID != "" { + return fmt.Errorf("`log_analytics_workspace_id` can only be set when `logs_destination` is set to `log-analytics` or omitted") + } } } } diff --git a/website/docs/5.0-upgrade-guide.html.markdown b/website/docs/5.0-upgrade-guide.html.markdown index 6b9b829cd828..2707dda75031 100644 --- a/website/docs/5.0-upgrade-guide.html.markdown +++ b/website/docs/5.0-upgrade-guide.html.markdown @@ -97,6 +97,10 @@ Please follow the format in the example below for listing breaking changes in re * The `minimal_tls_version` property no longer accepts `Tls` or `Tls11` as a value. +### `azurerm_container_app_environment` + +* The `logs_destination` property is no longer Computed and now must be set to `log-analytics` to be able to specify a value for `log_analytics_workspace_id`. It will now default to empty, meaning Streaming Only in the Azure Portal. + ### `azurerm_eventhub` * The deprecated `namespace_name` property has been removed in favour of the `namespace_id` property. diff --git a/website/docs/r/container_app_environment.html.markdown b/website/docs/r/container_app_environment.html.markdown index 097a7eac5ed8..be15fe1a3e2d 100644 --- a/website/docs/r/container_app_environment.html.markdown +++ b/website/docs/r/container_app_environment.html.markdown @@ -69,7 +69,7 @@ The following arguments are supported: ~> **Note:** required if `logs_destination` is set to `log-analytics`. Cannot be set if `logs_destination` is set to `azure-monitor`. -* `logs_destination` - (Optional) Where the application logs will be saved for this Container Apps Managed Environment. Options are `log-analytics` or `azure-monitor`. +* `logs_destination` - (Optional) Where the application logs will be saved for this Container Apps Managed Environment. Possible values include `log-analytics` and `azure-monitor`. Omitting this value will result in logs being streamed only. * `workload_profile` - (Optional) The profile of the workload to scope the container app execution. A `workload_profile` block as defined below.