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.

Sign up for GitHub

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

Jump to bottom

azurerm_function_app - fix regressions in function app storage #13580

Merged
merged 3 commits into from
Oct 5, 2021
Merged

azurerm_function_app - fix regressions in function app storage #13580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0ba50ce
fix regressions in contentshare
jackofallops Oct 1, 2021
f8fdae6
restore elasticpremium to send
jackofallops Oct 5, 2021
ce49bde
remove premium, pass through setting if already set
jackofallops Oct 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 12 additions & 11 deletions internal/services/web/function_app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ func schemaFunctionAppDataSourceSiteConfig() *pluginsdk.Schema {
}
}

func getBasicFunctionAppAppSettings(d *pluginsdk.ResourceData, appServiceTier, endpointSuffix string, existingSettings *[]web.NameValuePair) ([]web.NameValuePair, error) {
func getBasicFunctionAppAppSettings(d *pluginsdk.ResourceData, appServiceTier, endpointSuffix string, existingSettings map[string]*string) ([]web.NameValuePair, error) {
// TODO: This is a workaround since there are no public Functions API
// You may track the API request here: https://github.com/Azure/azure-rest-api-specs/issues/3750
dashboardPropName := "AzureWebJobsDashboard"
Expand Down Expand Up @@ -323,17 +323,13 @@ func getBasicFunctionAppAppSettings(d *pluginsdk.ResourceData, appServiceTier, e

functionVersion := d.Get("version").(string)
var contentShare string
if existingSettings == nil {
// generate and use a new value
contentSharePreviouslySet := false
if currentContentShare, ok := existingSettings[contentSharePropName]; ok {
contentShare = *currentContentShare
contentSharePreviouslySet = true
} else {
suffix := uuid.New().String()[0:4]
contentShare = strings.ToLower(d.Get("name").(string)) + suffix
} else {
// find and re-use the current
for _, v := range *existingSettings {
if v.Name != nil && *v.Name == contentSharePropName {
contentShare = *v.Name
}
}
}

basicSettings := []web.NameValuePair{
Expand All @@ -353,8 +349,13 @@ func getBasicFunctionAppAppSettings(d *pluginsdk.ResourceData, appServiceTier, e
{Name: &contentFileConnStringPropName, Value: &storageConnection},
}

// If there's an existing value for content, we need to send it. This can be the case for PremiumV2/PremiumV3 plans where the value has been previously configured.
if contentSharePreviouslySet {
return append(basicSettings, consumptionSettings...), nil
}

// On consumption and premium plans include WEBSITE_CONTENT components, unless it's a Linux consumption plan
// (see https://github.com/Azure/azure-functions-python-worker/issues/598)
// Note: The docs on this are misleading. Premium here refers explicitly to `ElasticPremium`, and not `PremiumV2` / `PremiumV3` etc.
if !(strings.EqualFold(appServiceTier, "dynamic") && strings.EqualFold(d.Get("os_type").(string), "linux")) &&
(strings.EqualFold(appServiceTier, "dynamic") || strings.HasPrefix(strings.ToLower(appServiceTier), "elastic")) {
Copy link
Member

Choose a reason for hiding this comment

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

Just looking to confirm that we don't need to check for elastic anymore? Is there any chance someone has that still?

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I know, Azure Files is only required for Consumption (Windows) and Elastic Premium (Windows / Linux). I think it is correct to leave this conditional expression as elastic. See also #13218 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi both, from the docs:

Only used when deploying to a Premium plan or to a Consumption plan running on Windows. Not supported for Consumptions plans running Linux.

I initially misinterpreted this as ElasticPremium also, however, this is actually App Service Premium Plans. For context see #13566.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is very confusing, but the Premium V2 / V3 of the App Service Plan and the Azure Functions Premium Plan are completely different services. The Premium Plan in this document refers to the latter.

If the File Share setting is added to the App Service Plan (Premium V2 / V3), the current working application may be lost. This is a very dangerous change and the application I manage is also affected.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is very confusing,

You are not wrong there. 🙃

The resource has always previously sent the content details for premium SKU's, and elasticpremium plans see here

The change in 2.77 was intended to align the resource behaviour with the docs, but appears to have created the issue in 13218 where these settings were being sent where they had previously not been. Are you reporting that the Premium plans did not get these settings as intended?

The other change in this PR addresses the change of setting for the share directory, which is a bug in the 2.77 implementation which attempted to maintain any existing value configured. (see 13536) If the property already exists in the Function App AppSettings, then it will be used and not added/created. Additionally this is intended to address a related bug whereby the provider created the share with a -content suffix that prevented proper operation of slots. This will also mean that if the share property is set for a Premium Plan, it will be honoured/maintained and not overwritten.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think your change in #13349 is correct. I actually created and tested the resource, and it behaved as intended. Isn't the only thing this PR needs to do is fix the problem of WEBSITE_CONTENTSHARE not being reused?

The only thing I would strongly argue is that the App Service Plan (Premium V2 / Premium V3) does not require Azure Files configuration. At least, when I tested using this branch's code against Azure Functions running on Premium V2, I saw that unnecessary Azure Files settings (WEBSITE_CONTENTSHARE and WEBSITE_CONTENTAZUREFILECONNECTIONSTRING) were added and the application was lost. I can share the definition if you want to reproduce it on your device.

In my opinion, for each tier (Consumption / App Service Plan / Premium Plan), the test cases need to be checked for the existence of WEBSITE_CONTENTSHARE and WEBSITE_CONTENTAZUREFILECONNECTIONSTRING.

The resource has always previously sent the content details for premium SKU's, and elasticpremium plans see here

Yes, that change will break your application if you are already using Premium V2 / V3. I have confirmed that it breaks with this comment. This buggy code has been fixed in v2.77.

Copy link
Contributor

Choose a reason for hiding this comment

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

As for #13566, it is different from this issue.

Azure Functions has an option to operate without File Share, so I think it's more appropriate to add a property (e.g. disabled_builtin_file_share or etc) to not auto-set WEBSITE_CONTENTSHARE and WEBSITE_CONTENTAZUREFILECONNECTIONSTRING.

https://docs.microsoft.com/en-us/azure/azure-functions/storage-considerations#create-an-app-without-azure-files

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your insight @shibayan - I'm taking this back to the drawing board to work it back through...

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated to correct the behaviour for PremiumV2/V3 (i.e. not send) and added the reuse of an existing value if defined (in case users do have it defined). I believe this is going to be sufficient for this stage, as presently this resource is not usable since 2.77 because of the share recreation bug. I'm reticent to introduce another property at this stage that will further affect behaviour of the resource. Upon re-reading, I believe 13566 to be a symptom of the share recreation bug, so should be resolved by this PR in any case.

fwiw - I'm in the process of completely rewriting this resource for the beta / v3.0, so I've made notes for addressing this properly there. I'll also be speaking to the Service Team on this as soon as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the quick response. I've checked the changed code and I'm sure it works ideally with Provider v2.x.

return append(basicSettings, consumptionSettings...), nil
Expand Down
11 changes: 5 additions & 6 deletions internal/services/web/function_app_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,14 +428,13 @@ func resourceFunctionAppUpdate(d *pluginsdk.ResourceData, meta interface{}) erro
return err
}

existing, err := client.GetConfiguration(ctx, id.ResourceGroup, id.SiteName)
var currentAppSettings map[string]*string
appSettingsList, err := client.ListApplicationSettings(ctx, id.ResourceGroup, id.SiteName)
if err != nil {
return fmt.Errorf("reading %s: %+v", id, err)
return fmt.Errorf("reading App Settings for %s: %+v", id, err)
}

var currentAppSettings *[]web.NameValuePair
if existing.AppSettings != nil {
currentAppSettings = existing.AppSettings
if appSettingsList.Properties != nil {
currentAppSettings = appSettingsList.Properties
}

basicAppSettings, err := getBasicFunctionAppAppSettings(d, appServiceTier, endpointSuffix, currentAppSettings)
Expand Down
64 changes: 64 additions & 0 deletions internal/services/web/function_app_resource_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,22 @@ func TestAccFunctionApp_linuxFxVersion(t *testing.T) {
})
}

func TestAccFunctionApp_elasticPremiumPlanLinux(t *testing.T) {
data := acceptance.BuildTestData(t, "azurerm_function_app", "test")
r := FunctionAppResource{}

data.ResourceTest(t, r, []acceptance.TestStep{
{
Config: r.linuxElasticPremium(data),
Check: acceptance.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
acceptance.TestCheckResourceAttr(data.ResourceName, "kind", "functionapp,linux"),
),
},
data.ImportStep(),
})
}

func TestAccFunctionApp_siteConfigVnetRouteAllEnabled(t *testing.T) {
data := acceptance.BuildTestData(t, "azurerm_function_app", "test")
r := FunctionAppResource{}
Expand Down Expand Up @@ -1452,6 +1468,7 @@ resource "azurerm_function_app" "test" {
}
`, data.RandomInteger, data.Locations.Primary, data.RandomString)
}

func (r FunctionAppResource) appSettingsLinux(data acceptance.TestData) string {
return fmt.Sprintf(`
provider "azurerm" {
Expand Down Expand Up @@ -1689,6 +1706,53 @@ resource "azurerm_function_app" "test" {
`, data.RandomInteger, data.Locations.Primary, data.RandomString)
}

func (r FunctionAppResource) linuxElasticPremium(data acceptance.TestData) string {
return fmt.Sprintf(`
provider "azurerm" {
features {}
}

resource "azurerm_resource_group" "test" {
name = "acctestRG-%[1]d"
location = "%[2]s"
}

resource "azurerm_storage_account" "test" {
name = "acctestsa%[3]s"
resource_group_name = azurerm_resource_group.test.name
location = azurerm_resource_group.test.location
account_tier = "Standard"
account_replication_type = "LRS"
}

resource "azurerm_app_service_plan" "test" {
name = "acctestASP-%[1]d"
location = azurerm_resource_group.test.location
resource_group_name = azurerm_resource_group.test.name
kind = "elastic"

sku {
tier = "ElasticPremium"
size = "EP1"
}

reserved = true
}

resource "azurerm_function_app" "test" {
name = "acctest-%[1]d-func"
location = azurerm_resource_group.test.location
version = "~1"
resource_group_name = azurerm_resource_group.test.name
app_service_plan_id = azurerm_app_service_plan.test.id
storage_account_name = azurerm_storage_account.test.name
storage_account_access_key = azurerm_storage_account.test.primary_access_key
os_type = "linux"

}
`, data.RandomInteger, data.Locations.Primary, data.RandomString)
}

func (r FunctionAppResource) siteConfigVnetRouteAllEnabled(data acceptance.TestData) string {
return fmt.Sprintf(`
provider "azurerm" {
Expand Down