search

hashicorp / terraform-provider-azurerm

Terraform provider for Azure Resource Manager
https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs
Mozilla Public License 2.0
4.6k stars 4.64k forks source link

retention_in_days setting falsely removed on any other change in linux_web_app #23713

Closed mm-supernice closed 5 months ago

mm-supernice commented 1 year ago

Is there an existing issue for this?

Community Note

Terraform is deleting the "retention_in_days" on "any" (or most) changes you apply to that resource without notifying. The reason for this behaviour is:

however, setting "retention_in_days" is the proper way to do and of course also present in the state On Azure itself, it does not matter if you add/set the environment variable or the explicit setting, if you change one of them, it alters the other automatically.

In our case it causes the following:

  1. You run terraform apply, because you made some change (independent of the logging stuff) in the linux_web_app resource.
  2. Terraform Plan (except of your actual change) detects no change in the log retention setting because there is actually no change.
  3. Terraform Apply pushes the JSON to the Azure API containing the actual configuration including your change.

    Here we have the problem, the JSON which is PUT'ed to the Azure API contains all the environment variables but not the "WEBSITE_HTTPLOGGING_RETENTION_DAYS" because it is not allowed to set and deleted as suggested bythis post (edit: this post is referencing to a wrong and deprecated resource) Furthermore, the "retention_in_days" is not part of the resource/api endpoint terraform currenty PUTs a request to, there must be another request containing that setting to /config/logs which does not happen here. also there is no second PUT with a json to set the "retention_in_days" correctly because while terraform plan, the setting was set correctly and therefore terraform apply does not know about that change just made by himself.

  4. change done, WEBSITE_HTTPLOGGING_RETENTION_DAYS removed, therefore automatically also "retention_in_days" removed by azure.

on the next run, terraform plan detects the missing "retention_in_days" setting and updates it, which is what you actually see as the unwanted change.

Voting

Terraform Version

1.6.2

AzureRM Provider Version

v3.78.0

Affected Resource(s)/Data Source(s)

azurerm_linux_web_app

Terraform Configuration Files

resource "azurerm_linux_web_app" "my-web-app" {
  name                = "my-web-app"
  location            = var.resource_group_location
  resource_group_name = var.resource_group_name
  virtual_network_subnet_id = var.virtual_network_subnet_id
  service_plan_id     = azurerm_service_plan.my-web-app-plan.id
  https_only          = true
  public_network_access_enabled = true

  logs {
    detailed_error_messages = false
    failed_request_tracing  = false

    http_logs {
      file_system {
        retention_in_days = 5
        retention_in_mb   = 35
      }
    }
  }

  site_config {
    application_stack {
      node_version = "18-lts"
    }
    app_command_line = "while true; do touch /`uuidgen`"
  }
  app_settings = {
    MY_FANCY_ENV_VAR              = "iam so useless"
  }
}

Debug Output/Panic Output

on any change in the azurerm_linux_web_app resource, the JSON which is PUT' to the API is lacking the WEBSITE_HTTPLOGGING_RETENTION_DAYS setting which causes a removal of that environment variable. unfortunately this then let the Azure setting to remove the setting "retention_in_days" since the Env variable and that setting are hard-linked.

Additionally, there is no second PUT which makes sure the retention_in_days setting is set again, after removal of the "WEBSITE_HTTPLOGGING_RETENTION_DAYS" environment variable.

The only PUT request sent to the API removes the "WEBSITE_HTTPLOGGING_RETENTION_DAYS" by not explicitly having it in the json. this change is not shown by the terraform plan, since the WEBSITE_HTTPLOGGING_RETENTION_DAYS is not covered by the provider and even actively removed since the proper place to configure is the "retention_in_days" setting.

{
  "kind": "app,linux",
  "location": "North Europe",
  "properties": {
    "clientAffinityEnabled": false,
    "clientCertEnabled": false,
    "clientCertMode": "Required",
    "containerSize": 0,
    "customDomainVerificationId": "XXXXXXXXXXXXXXXXXXXXXXXXXXx",
    "dailyMemoryTimeQuota": 0,
    "enabled": true,
    "hostNameSslStates": [
      {
        "name": "my-web-app.azurewebsites.net",
        "sslState": "Disabled",
        "hostType": "Standard"
      },
      {
        "name": "my-web-app.scm.azurewebsites.net",
        "sslState": "Disabled",
        "hostType": "Repository"
      }
    ],
    "hostNamesDisabled": false,
    "httpsOnly": true,
    "hyperV": false,
    "isXenon": false,
    "keyVaultReferenceIdentity": "SystemAssigned",
    "publicNetworkAccess": "Enabled",
    "redundancyMode": "None",
    "reserved": true,
    "scmSiteAlsoStopped": false,
    "serverFarmId": "/subscriptions/XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/resourceGroups/my-rg/providers/Microsoft.Web/serverfarms/my-web-app-plan",
    "siteConfig": {
      "acrUseManagedIdentityCreds": false,
      "alwaysOn": true,
      "appSettings": [
        {
          "name": "MY_FANCY_ENV_VAR",
          "value": "iam so useless"
        }
      ],
      "autoHealEnabled": false,
      "functionAppScaleLimit": 0,
      "http20Enabled": false,
      "linuxFxVersion": "NODE|18-lts",
      "localMySqlEnabled": false,
      "minimumElasticInstanceCount": 0,
      "numberOfWorkers": 1,
      "remoteDebuggingEnabled": false,
      "scmIpSecurityRestrictionsUseMain": false,
      "use32BitWorkerProcess": true,
      "vnetRouteAllEnabled": false,
      "webSocketsEnabled": false
    },
    "storageAccountRequired": false,
    "virtualNetworkSubnetId": "/subscriptions/XXXXXXXXXXXXXXXXXXXXXXXXXX/resourceGroups/my-rg/providers/Microsoft.Network/virtualNetworks/my-vnet/subnets/my-subnet",
    "vnetContentShareEnabled": false,
    "vnetImagePullEnabled": false,
    "vnetRouteAllEnabled": false
  },
  "tags": {}
}

Expected Behaviour

Expected API Request

on any change in the azurerm_linux_web_app resource, the JSON which is PUT' to the API is lacking the WEBSITE_HTTPLOGGING_RETENTION_DAYS setting which causes a removal of that environment variable. unfortunately this then let the Azure setting to remove the setting "retention_in_days" since the Env variable and that setting are hard-linked.

Additionally, there is no second PUT which makes sure the retention_in_days setting is set again, after removal of the "WEBSITE_HTTPLOGGING_RETENTION_DAYS" environment variable.

The only PUT request sent to the API removes the "WEBSITE_HTTPLOGGING_RETENTION_DAYS" by not explicitly having it in the json. this change is not shown by the terraform plan, since the WEBSITE_HTTPLOGGING_RETENTION_DAYS is not covered by the provider and even actively removed since the proper place to configure is the "retention_in_days" setting.

{
  "kind": "app,linux",
  "location": "North Europe",
  "properties": {
    "clientAffinityEnabled": false,
    "clientCertEnabled": false,
    "clientCertMode": "Required",
    "containerSize": 0,
    "customDomainVerificationId": "XXXXXXXXXXXXXXXXXXXXXXXXXXx",
    "dailyMemoryTimeQuota": 0,
    "enabled": true,
    "hostNameSslStates": [
      {
        "name": "my-web-app.azurewebsites.net",
        "sslState": "Disabled",
        "hostType": "Standard"
      },
      {
        "name": "my-web-app.scm.azurewebsites.net",
        "sslState": "Disabled",
        "hostType": "Repository"
      }
    ],
    "hostNamesDisabled": false,
    "httpsOnly": true,
    "hyperV": false,
    "isXenon": false,
    "keyVaultReferenceIdentity": "SystemAssigned",
    "publicNetworkAccess": "Enabled",
    "redundancyMode": "None",
    "reserved": true,
    "scmSiteAlsoStopped": false,
    "serverFarmId": "/subscriptions/XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/resourceGroups/my-rg/providers/Microsoft.Web/serverfarms/my-web-app-plan",
    "siteConfig": {
      "acrUseManagedIdentityCreds": false,
      "alwaysOn": true,
      "appSettings": [
        {
          "name": "MY_FANCY_ENV_VAR",
          "value": "iam so useless"
        },
        {
          "name": "WEBSITE_HTTPLOGGING_RETENTION_DAYS",
          "value": "5"
        }
      ],
      "autoHealEnabled": false,
      "functionAppScaleLimit": 0,
      "http20Enabled": false,
      "linuxFxVersion": "NODE|18-lts",
      "localMySqlEnabled": false,
      "minimumElasticInstanceCount": 0,
      "numberOfWorkers": 1,
      "remoteDebuggingEnabled": false,
      "scmIpSecurityRestrictionsUseMain": false,
      "use32BitWorkerProcess": true,
      "vnetRouteAllEnabled": false,
      "webSocketsEnabled": false
    },
    "storageAccountRequired": false,
    "virtualNetworkSubnetId": "/subscriptions/XXXXXXXXXXXXXXXXXXXXXXXXXX/resourceGroups/my-rg/providers/Microsoft.Network/virtualNetworks/my-vnet/subnets/my-subnet",
    "vnetContentShareEnabled": false,
    "vnetImagePullEnabled": false,
    "vnetRouteAllEnabled": false
  },
  "tags": {}
}

Would cause the following resource object

{
  "id": "/subscriptions/XXXXXXXXXXXXXXXXXXXXXXX/resourceGroups/my-rg/providers/Microsoft.Web/sites/my-web-app/config/logs",
  "name": "logs",
  "type": "Microsoft.Web/sites/config",
  "location": "North Europe",
  "properties": {
    "applicationLogs": {
      "fileSystem": {
        "level": "Off"
      },
      "azureTableStorage": {
        "level": "Off",
        "sasUrl": null
      },
      "azureBlobStorage": {
        "level": "Off",
        "sasUrl": null,
        "retentionInDays": null
      }
    },
    "httpLogs": {
      "fileSystem": {
        "retentionInMb": 35,
        "retentionInDays": 5,
        "enabled": true
      },
      "azureBlobStorage": {
        "sasUrl": null,
        "retentionInDays": 5,
        "enabled": false
      }
    },
    "failedRequestsTracing": {
      "enabled": false
    },
    "detailedErrorMessages": {
      "enabled": false
    }
  }
}

Actual Behaviour

But retentionInDays gets removed entirely 

{
  "id": "/subscriptions/XXXXXXXXXXXXXXXXXXXXXXX/resourceGroups/my-rg/providers/Microsoft.Web/sites/my-web-app/config/logs",
  "name": "logs",
  "type": "Microsoft.Web/sites/config",
  "location": "North Europe",
  "properties": {
    "applicationLogs": {
      "fileSystem": {
        "level": "Off"
      },
      "azureTableStorage": {
        "level": "Off",
        "sasUrl": null
      },
      "azureBlobStorage": {
        "level": "Off",
        "sasUrl": null,
        "retentionInDays": null
      }
    },
    "httpLogs": {
      "fileSystem": {
        "retentionInMb": 35,
        "retentionInDays": null,
        "enabled": true
      },
      "azureBlobStorage": {
        "sasUrl": null,
        "retentionInDays": null,
        "enabled": false
      }
    },
    "failedRequestsTracing": {
      "enabled": false
    },
    "detailedErrorMessages": {
      "enabled": false
    }
  }
}

Steps to Reproduce

  1. create any azurerm_linux_web_app resource

  2. make sure to set the following setting in the resource definition:

    logs {
detailed_error_messages = false
failed_request_tracing  = false

http_logs {
  file_system {
    retention_in_days = 5
    retention_in_mb   = 35
  }
}
}

    and

    app_settings = {
MY_FANCY_ENV_VAR              = "iam so useless"
}

  3. apply your setting

  4. in the portal, check the following settings:

    in settings -> configuration

    you should see your + the automatically added "WEBSITE_HTTPLOGGING_RETENTION_DAYS" environment variable in Monitoring -> app Service logs you should see: Retention Period (Days): 5

  5. change a setting, in my case, add a whitelist rule.

  6. apply again

  7. you will see: just the whitelist will be and was added

  8. check the following settings:

    in settings -> configuration

    "WEBSITE_HTTPLOGGING_RETENTION_DAYS" is gone in Monitoring -> app Service logs Retention Period (Days): UNSET

  9. apply again

  10. you will see:

# azurerm_linux_web_app.my-web-app will be updated in-place
  ~ resource "azurerm_linux_web_app" "my-web-app" {
        id                                = "/subscriptions/XXXXXXXXXXXXXXXXXXXX/resourceGroups/my-rg/providers/Microsoft.Web/sites/my-web-app"
        name                              = "my-web-app"
        tags                              = {}
        # (20 unchanged attributes hidden)

      ~ logs {
          ~ detailed_error_messages = true -> false
          ~ failed_request_tracing  = true -> false

          ~ http_logs {
              ~ file_system {
                  ~ retention_in_days = 0 -> 5
                    # (1 unchanged attribute hidden)
                }
            }
        }

        # (2 unchanged blocks hidden)
    }

Important Factoids

No response

References

No response

mm-supernice commented 1 year ago

@rcskosir if there are any further information required, please let me know, i'am here to help

mm-supernice commented 1 year ago

there is a comment by @jackofallops in internal/services/appservice/linux_web_app_resource.go:806

// (@jackofallops) - App Settings can clobber logs configuration so must be updated before we send any Log updates

followed by no implementation to mitigate the clobbing.

In the now deprecated resource for the webapp internal/services/web/app_service_resource.go:555 there is (or was) actually a implementation to mitigate this condition. It seems that this functionality was never ported when refactoring

app_service_resource.go#L555

    // the logging configuration has a dependency on the app settings in Azure
    // e.g. configuring logging to blob storage will add the DIAGNOSTICS_AZUREBLOBCONTAINERSASURL
    // and DIAGNOSTICS_AZUREBLOBRETENTIONINDAYS app settings to the app service.
    // If the app settings are updated, also update the logging configuration if it exists, otherwise
    // updating the former will clobber the log settings
    hasLogs := len(d.Get("logs").([]interface{})) > 0
    if d.HasChange("logs") || (hasLogs && d.HasChange("app_settings")) {
        logs := expandAppServiceLogs(d.Get("logs"))
        logsResource := web.SiteLogsConfig{
            ID:                       utils.String(d.Id()),
            SiteLogsConfigProperties: &logs,
        }

        if _, err := client.UpdateDiagnosticLogsConfig(ctx, id.ResourceGroup, id.SiteName, logsResource); err != nil {
            return fmt.Errorf("updating Diagnostics Logs for App Service %q: %+v", id.SiteName, err)
        }
    }

Possible Fix

So i worked out two possible fix but i'am unable to test it, may someone implement it and open a PR <3

Fix 1

At linux_web_app_resource.go#L898

Change the if condition to:

if metadata.ResourceData.HasChange("logs") || updateLogs || metadata.ResourceData.HasChange("app_settings") {
//
}

Or

Fix 2

At linux_web_app_resource.go#L807

Add a block to update "logs" too

// (@jackofallops) - App Settings can clobber logs configuration so must be updated before we send any Log updates
      if metadata.ResourceData.HasChange("app_settings") || metadata.ResourceData.HasChange("site_config.0.health_check_eviction_time_in_min") {
        appSettingsUpdate := helpers.ExpandAppSettingsForUpdate(state.AppSettings)
        appSettingsUpdate.Properties["WEBSITE_HEALTHCHECK_MAXPINGFAILURES"] = pointer.To(strconv.Itoa(state.SiteConfig[0].HealthCheckEvictionTime))

        logsUpdate := helpers.ExpandLogsConfig(state.LogsConfig)
        if logsUpdate.SiteLogsConfigProperties == nil {
          logsUpdate = helpers.DisabledLogsConfig() // The API is update only, so we need to send an update with everything switched of when a user removes the "logs" block
        }
        if _, err := client.UpdateDiagnosticLogsConfig(ctx, id.ResourceGroup, id.SiteName, *logsUpdate); err != nil {
          return fmt.Errorf("updating Logs Config for Linux %s: %+v", id, err)
        }

        if _, err := client.UpdateApplicationSettings(ctx, id.ResourceGroup, id.SiteName, *appSettingsUpdate); err != nil {
          return fmt.Errorf("updating App Settings for Linux %s: %+v", id, err)
        }
      }
xiaxyi commented 9 months ago

@mm-supernice thanks for raising this issue, are you still experiencing this issue after the mentioned fix was merged?

mm-supernice commented 9 months ago

@mm-supernice thanks for raising this issue, are you still experiencing this issue after the mentioned fix was merged?

Hi, thanks for checking.

Unfortunately the PR does not fix the issue (tested with 3.87.0).

after a quick review of the PR the issue seems to be that at Line 858 now not only app_settings is considered but also site_config which is great. however "logs" is also separated from them and not within site_config. that might be the reason why the fix doesn't affect this issue.

xiaxyi commented 9 months ago

@mm-supernice Thanks for the update, let me check.

vanmash commented 8 months ago

any progress on this?

gsmith077 commented 8 months ago

Also bumping this issue.

Drift detection doesn't work, and we have resources in a state of thrash due to this bug.

Setting an ignore_changes is harmful, since we need to ensure that we have proper log retention set at all times.

mm-supernice commented 8 months ago

Same here, still not fixed and super annoying.

pacorreia commented 8 months ago

for everyone facing this issue, don't forget to vote, raise awareness on this one. Version 3.94.0 and it's still there. This kind of issue seems to be recurrent. Faced a similar thing with docker configuration.

karlosss commented 8 months ago

fix would be appreciated from here too :)

strayer commented 6 months ago

For what its worth: I reported this as a bug through our Enterprise support plan and was told a fix is expected between August and October.

pacorreia commented 6 months ago

Thanks, for that

A quinta, 2/05/2024, 12:49, Sven Grunewaldt @.***> escreveu:

For what its worth: I reported this as a bug through our Enterprise support plan and was told a fix is expected between August and October.

— Reply to this email directly, view it on GitHub https://github.com/hashicorp/terraform-provider-azurerm/issues/23713#issuecomment-2090305299, or unsubscribe https://github.com/notifications/unsubscribe-auth/AM6OPV5STTS2HICPSUB5ULDZAIR3LAVCNFSM6AAAAAA6SZKYNWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJQGMYDKMRZHE . You are receiving this because you commented.Message ID: @.***>

ppa-fyayc commented 6 months ago

a fix is expected between August and October.

this year?

github-actions[bot] commented 4 months ago

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.