Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

azurerm_windows_web_app - fix health_check_eviction_time_in_min not being set issue #17656

Closed
wants to merge 10 commits into from
Next Next commit
fix health check not being set issue
  • Loading branch information
xiaxyi committed Jul 18, 2022
commit 39875da15e534385acc173b130d321d8a89aaa57
10 changes: 8 additions & 2 deletions internal/services/appservice/helpers/web_app_schema.go
Original file line number Diff line number Diff line change
@@ -3785,8 +3785,8 @@ func ExpandAppSettingsForCreate(settings map[string]string) *[]web.NameValuePair
return nil
}

func FlattenAppSettings(input web.StringDictionary) (map[string]string, *int) {
maxPingFailures := "WEBSITE_HEALTHCHECK_MAXPINGFAILURE"
func FlattenAppSettings(input web.StringDictionary, d sdk.ResourceMetaData) (map[string]string, *int) {
maxPingFailures := "WEBSITE_HEALTHCHECK_MAXPINGFAILURES"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the metadata is not needed here, just the correction of the typo in the WEBSITE_HEALTHCHECK_MAXPINGFAILURES string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to move the WEBSITE_HEALTHCHECK_MAXPINGFAILURES from the unmanagedSettings list if user adds the key explicitly in the config?

unmanagedSettings := []string{
"DIAGNOSTICS_AZUREBLOBCONTAINERSASURL",
"DIAGNOSTICS_AZUREBLOBRETENTIONINDAYS",
@@ -3803,6 +3803,12 @@ func FlattenAppSettings(input web.StringDictionary) (map[string]string, *int) {
healthCheckCount = &h
}

if userInputAppSetting := d.ResourceData.Get("app_settings").(map[string]interface{}); userInputAppSetting != nil {
if _, ok := userInputAppSetting[maxPingFailures]; ok {
unmanagedSettings = unmanagedSettings[:len(unmanagedSettings)-1]
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be removed, users should set this value via health_check_eviction_time_in_min and not in the app_settings map. Leaving this in will lead to diffs in that map value. The second returned value from this function should be carried to be set in the site_config.0.health_check_eviction_time_in_min property.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But azure provided such way to set this property and lots of users are aware of this usage as well. shall we just let them set it via app_setting?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have moved it out of the map into its own property, therefor it should only be set there as @jackofallops, and if the user does put it into the app settings map we should maybe toss up an error telling the user to set it there?

// Remove the settings the service adds for legacy reasons.
for _, v := range unmanagedSettings { //nolint:typecheck
delete(appSettings, v)
2 changes: 1 addition & 1 deletion internal/services/appservice/linux_web_app_data_source.go
Original file line number Diff line number Diff line change
@@ -275,7 +275,7 @@ func (r LinuxWebAppDataSource) Read() sdk.ResourceFunc {
}

var healthCheckCount *int
webApp.AppSettings, healthCheckCount = helpers.FlattenAppSettings(appSettings)
webApp.AppSettings, healthCheckCount = helpers.FlattenAppSettings(appSettings, metadata)
webApp.Kind = utils.NormalizeNilableString(existing.Kind)
webApp.Location = location.NormalizeNilable(existing.Location)
webApp.Tags = tags.ToTypedObject(existing.Tags)
15 changes: 10 additions & 5 deletions internal/services/appservice/linux_web_app_resource.go
Original file line number Diff line number Diff line change
@@ -3,6 +3,7 @@ package appservice
import (
"context"
"fmt"
"github.com/hashicorp/terraform-provider-azurerm/internal/features"
"strconv"
"strings"
"time"
@@ -338,8 +339,10 @@ func (r LinuxWebAppResource) Create() sdk.ResourceFunc {
metadata.SetID(id)

appSettings := helpers.ExpandAppSettingsForUpdate(webApp.AppSettings)
if metadata.ResourceData.HasChange("site_config.0.health_check_eviction_time_in_min") {
appSettings.Properties["WEBSITE_HEALTHCHECK_MAXPINGFAILURE"] = utils.String(strconv.Itoa(webApp.SiteConfig[0].HealthCheckEvictionTime))
if !features.FourPointOhBeta() {
if metadata.ResourceData.HasChange("site_config.0.health_check_eviction_time_in_min") {
appSettings.Properties["WEBSITE_HEALTHCHECK_MAXPINGFAILURES"] = utils.String(strconv.Itoa(webApp.SiteConfig[0].HealthCheckEvictionTime))
}
}

if appSettings.Properties != nil {
@@ -507,7 +510,7 @@ func (r LinuxWebAppResource) Read() sdk.ResourceFunc {
}

var healthCheckCount *int
state.AppSettings, healthCheckCount = helpers.FlattenAppSettings(appSettings)
state.AppSettings, healthCheckCount = helpers.FlattenAppSettings(appSettings, metadata)

if v := props.OutboundIPAddresses; v != nil {
state.OutboundIPAddresses = *v
@@ -676,8 +679,10 @@ func (r LinuxWebAppResource) Update() sdk.ResourceFunc {
// (@jackofallops) - App Settings can clobber logs configuration so must be updated before we send any Log updates
if metadata.ResourceData.HasChange("app_settings") {
appSettingsUpdate := helpers.ExpandAppSettingsForUpdate(state.AppSettings)
if metadata.ResourceData.HasChange("site_config.0.health_check_eviction_time_in_min") {
appSettingsUpdate.Properties["WEBSITE_HEALTHCHECK_MAXPINGFAILURE"] = utils.String(strconv.Itoa(state.SiteConfig[0].HealthCheckEvictionTime))
if !features.FourPointOh() {
if metadata.ResourceData.HasChange("site_config.0.health_check_eviction_time_in_min") {
appSettingsUpdate.Properties["WEBSITE_HEALTHCHECK_MAXPINGFAILURES"] = utils.String(strconv.Itoa(state.SiteConfig[0].HealthCheckEvictionTime))
}
}
if _, err := client.UpdateApplicationSettings(ctx, id.ResourceGroup, id.SiteName, *appSettingsUpdate); err != nil {
return fmt.Errorf("updating App Settings for Linux %s: %+v", id, err)
Original file line number Diff line number Diff line change
@@ -455,7 +455,7 @@ func (r LinuxWebAppSlotResource) Read() sdk.ResourceFunc {
}

var healthCheckCount *int
state.AppSettings, healthCheckCount = helpers.FlattenAppSettings(appSettings)
state.AppSettings, healthCheckCount = helpers.FlattenAppSettings(appSettings, metadata)

if v := props.OutboundIPAddresses; v != nil {
state.OutboundIPAddresses = *v
Original file line number Diff line number Diff line change
@@ -259,7 +259,7 @@ func (d WindowsWebAppDataSource) Read() sdk.ResourceFunc {
}

var healthCheckCount *int
webApp.AppSettings, healthCheckCount = helpers.FlattenAppSettings(appSettings)
webApp.AppSettings, healthCheckCount = helpers.FlattenAppSettings(appSettings, metadata)
webApp.Kind = utils.NormalizeNilableString(existing.Kind)
webApp.Location = location.NormalizeNilable(existing.Location)
webApp.Tags = tags.ToTypedObject(existing.Tags)
15 changes: 14 additions & 1 deletion internal/services/appservice/windows_web_app_resource.go
Original file line number Diff line number Diff line change
@@ -3,6 +3,8 @@ package appservice
import (
"context"
"fmt"
"github.com/hashicorp/terraform-provider-azurerm/internal/features"
"strconv"
"strings"
"time"

@@ -333,6 +335,12 @@ func (r WindowsWebAppResource) Create() sdk.ResourceFunc {
}

appSettings := helpers.ExpandAppSettingsForUpdate(webApp.AppSettings)
if !features.FourPointOh() {
if metadata.ResourceData.HasChange("site_config.0.health_check_eviction_time_in_min") {
appSettings.Properties["WEBSITE_HEALTHCHECK_MAXPINGFAILURES"] = utils.String(strconv.Itoa(webApp.SiteConfig[0].HealthCheckEvictionTime))
}
}

if appSettings != nil {
if _, err := client.UpdateApplicationSettings(ctx, id.ResourceGroup, id.SiteName, *appSettings); err != nil {
return fmt.Errorf("setting App Settings for Windows %s: %+v", id, err)
@@ -511,7 +519,7 @@ func (r WindowsWebAppResource) Read() sdk.ResourceFunc {
}

var healthCheckCount *int
state.AppSettings, healthCheckCount = helpers.FlattenAppSettings(appSettings)
state.AppSettings, healthCheckCount = helpers.FlattenAppSettings(appSettings, metadata)

if v := props.OutboundIPAddresses; v != nil {
state.OutboundIPAddresses = *v
@@ -687,6 +695,11 @@ func (r WindowsWebAppResource) Update() sdk.ResourceFunc {
// (@jackofallops) - App Settings can clobber logs configuration so must be updated before we send any Log updates
if metadata.ResourceData.HasChange("app_settings") {
appSettingsUpdate := helpers.ExpandAppSettingsForUpdate(state.AppSettings)
if !features.FourPointOhBeta() {
if metadata.ResourceData.HasChange("site_config.0.health_check_eviction_time_in_min") {
appSettingsUpdate.Properties["WEBSITE_HEALTHCHECK_MAXPINGFAILURES"] = utils.String(strconv.Itoa(state.SiteConfig[0].HealthCheckEvictionTime))
}
}
if _, err := client.UpdateApplicationSettings(ctx, id.ResourceGroup, id.SiteName, *appSettingsUpdate); err != nil {
return fmt.Errorf("updating App Settings for Windows %s: %+v", id, err)
}
Original file line number Diff line number Diff line change
@@ -451,7 +451,7 @@ func (r WindowsWebAppSlotResource) Read() sdk.ResourceFunc {
}

var healthCheckCount *int
state.AppSettings, healthCheckCount = helpers.FlattenAppSettings(appSettings)
state.AppSettings, healthCheckCount = helpers.FlattenAppSettings(appSettings, metadata)

if v := props.OutboundIPAddresses; v != nil {
state.OutboundIPAddresses = *v