`azurerm_windows_function_app`, `azurerm_linux_function_app` - fix content share not being created automatically issue #18258

xiaxyi · 2022-09-06T09:03:45Z

Fix content share not being created automatically if WEBSITE_CONTENTOVERVNET sets to 1.

According to the official guidance, "There are scenarios where you must set the WEBSITE_CONTENTSHARE value to a predefined share, such as when you use a secured storage account in a virtual network. In this case, you must set a unique share name for the main function app and the app for each deployment slot."
If the property WEBSITE_CONTENTOVERVNET set to 1 in app_setting while the users don't specify or specify a non-existed storage file share, they get the error message:

reading logs configuration for Windows Function App: (Site Name "xxx" / Resource Group "xxx"): web.AppsClient#GetDiagnosticLogsConfiguration: Failure responding to request: StatusCode=503 -- Original Error: autorest/azure: Service returned an error. Status=503 Code="ServiceUnavailable" Message="The operation timed out and could not be completed. Please retry the action or try again later." Details=[{"Message":"The operation timed out and could not be completed. Please retry the action or try again later."},{"Code":"ServiceUnavailable"},{"ErrorEntity":{"Code":"ServiceUnavailable","ExtendedCode":"51007","Message":"The operation timed out and could not be completed. Please retry the action or try again later.","MessageTemplate":"The operation timed out and could not be completed. Please retry the action or try again later."}}]
│

fix #17254
fix #14167
fix #19134

jackofallops

Hi @xiaxyi - Thanks for this PR.

I've left some comments inline below that will need to be addressed before we can continue review. I think this PR should probably have been split into smaller specific PRs to address the disparate concerns as the changes do not fully match the description / title, which makes the changes difficult to review and will unfortunately slow the process. That said, if you can make the changes here we can get this moving.

Thanks!

jackofallops · 2022-10-14T09:16:45Z

internal/services/appservice/helpers/service_plan.go

@@ -137,7 +137,7 @@ func PlanTypeFromSku(input string) string {
 }

 // ServicePlanInfoForApp returns the OS type and Service Plan SKU for a given App Service Resource
-func ServicePlanInfoForApp(ctx context.Context, metadata sdk.ResourceMetaData, id interface{}) (osType *string, planSku *string, err error) {
+func ServicePlanInfoForApp(ctx context.Context, metadata sdk.ResourceMetaData, id interface{}, serviceFarmId string) (osType *string, planSku *string, err error) {


This function is intended to retrieve the Service Plan ID from the App ID, it should not take the ID directly to circumvent this.

jackofallops · 2022-10-14T09:35:17Z

internal/services/appservice/linux_function_app_resource.go

+			// Some service plan updates are allowed - see customiseDiff for exceptions
+			var serviceFarmId string
+			if metadata.ResourceData.HasChange("service_plan_id") {
+				serviceFarmId = state.ServicePlanId
+				existing.SiteProperties.ServerFarmID = utils.String(serviceFarmId)
+			}
+
+			_, planSKU, err := helpers.ServicePlanInfoForApp(ctx, metadata, *id, serviceFarmId)


This should be a call to the new service plan to get the SKU, and not repurpose the helper function.

internal/services/appservice/windows_function_app_resource.go

jackofallops · 2022-10-14T09:36:32Z

internal/services/appservice/windows_function_app_resource.go

+			var storageAccountName, storageFileShare string
+			if sendContentSettings || helpers.PlanIsConsumption(planSku) {
+				if appSettingsResp.Properties != nil {
+					if appSettingsResp.Properties["WEBSITE_CONTENTAZUREFILECONNECTIONSTRING"] != nil {
+						saConnectionString := strings.Split(*appSettingsResp.Properties["WEBSITE_CONTENTAZUREFILECONNECTIONSTRING"], ";")
+						for _, part := range saConnectionString {
+							if strings.Contains(part, "AccountName") {
+								storageAccountName = strings.TrimPrefix(part, "AccountName=")
+								break
+							}
+						}
+					}
+					if appSettingsResp.Properties["WEBSITE_CONTENTSHARE"] != nil {
+						storageFileShare = *appSettingsResp.Properties["WEBSITE_CONTENTSHARE"]
+					}
+					saClient := metadata.Client.Storage
+					account, err := saClient.FindAccount(ctx, storageAccountName)
+					if err != nil {
+						return fmt.Errorf("retrieving Account %q for Share %q: %s", storageAccountName, storageFileShare, err)
+					}
+					if account == nil {
+						return fmt.Errorf("unable to locate Storage Account %s", storageAccountName)
+					}
+					saFileShareClient, err := metadata.Client.Storage.FileSharesClient(ctx, *account)
+					if err != nil {
+						return fmt.Errorf("building File Share Client: %s", err)
+					}
+					exists, err := saFileShareClient.Exists(ctx, account.ResourceGroup, storageAccountName, storageFileShare)
+					if err != nil {
+						return fmt.Errorf("checking for existence of Storage Share %q (Account %q) : %+v", storageFileShare, storageAccountName, err)
+					}
+					if exists == nil {
+						return fmt.Errorf("the storage file share %s for function app %s does not exists, please specify an existing one", storageFileShare, id.ID())
+					}
+				}
+			}
+


As above, this shouldn't be in the Read.

Thanks for the review, this is to address the timeout issue. if the user doesn't specify a predefined file share and also set the WEBSITE_CONTENTOVERVNET in the app_setting, the provider will timeout by trying to access a non-existence file share. I can add the 4.0 condition to this check block.

agreed - this shouldn't be in the Read function - we don't retrieve the file share in the Read, so this is unnecessary

Thanks @tombuildsstuff for the comments. If we don't check the storage file share, shall we just let the read function timeout?

@xiaxyi why will the read function timeout? it's calling the App Service ARM API and not interacting with the Storage Account in the read function?

The function app is trying to connect to the file share specified in the [WEBSITE_CONTENTSHARE](https://learn.microsoft.com/en-us/azure/azure-functions/functions-app-settings), Generally, the file share is created by function app automatically, however, the it cannot be created if user specify the key WEBSITE_CONTENTOVERVNET in app_setting, it requires the user to create a valid file share in advance and put the value in WEBSITE_CONTENTSHARE`. We didn't add such notes to user in the current provider, so the function(Read function) times out.

If we don't check the file share in read function, then what's left is to add notes in docs and let the read function timeout.

jackofallops · 2022-10-14T09:38:08Z

internal/services/appservice/windows_function_app_resource.go

+			var serviceFarmId string
+			if metadata.ResourceData.HasChange("service_plan_id") {
+				serviceFarmId = state.ServicePlanId
+				existing.SiteProperties.ServerFarmID = utils.String(serviceFarmId)
+			}
+
+			_, planSKU, err := helpers.ServicePlanInfoForApp(ctx, metadata, *id, serviceFarmId)


As above, the helper should not be repurposed, and the SKU should be read from the new service plan ID. A new helper for that would be acceptable.

internal/services/appservice/linux_function_app_resource.go

…share

xiaxyi · 2022-10-20T03:09:07Z

Hi @xiaxyi - Thanks for this PR.

I've left some comments inline below that will need to be addressed before we can continue review. I think this PR should probably have been split into smaller specific PRs to address the disparate concerns as the changes do not fully match the description / title, which makes the changes difficult to review and will unfortunately slow the process. That said, if you can make the changes here we can get this moving.

Thanks!

Thanks @jackofallops for the feedbacks. All the code changes in this pr is tend to resolve one issue which is the content share issue under elastic plan.

As I mentioned in the description, if user sets WEBSITE_CONTENTOVERVNET without specifying a predefined storage file share, there will be error accessing that share.

The file share check in read function is to ensure the predefined file share exists, otherwise, we throw error instead of waiting for the read function to time out.

The service plan check is to determine whether the function is elastic plan.

…share

xiaxyi · 2022-11-09T03:20:25Z

@jackofallops @tombuildsstuff , updated th read function, removed the storage file share check.

jackofallops

Hi @xiaxyi - apologies for the long overdue re-review on this. I hope you don't mind, but I pushed some changes to simplify the updates and remove the need for re-processing some data. Could you look to add a test to cover the case being addressed here, and I think we'll be good to merge once that's in and passing.

Thanks

xiaxyi · 2023-03-10T02:07:08Z

Thanks @jackofallops for the update, I updated the code and added the test cases for both linux and windows.

feel free to let me know if there is anything else needed.

jackofallops

Hi @xiaxyi
Thanks for adding the tests. I've re-pushed the changes that were lost during the merge main. The ignores in the test suggest this needs some more work, possibly in the functions I've called out in line below if you can take a look?

Thanks!

internal/services/appservice/linux_function_app_resource_test.go

internal/services/appservice/windows_function_app_resource_test.go

…share

xiaxyi · 2023-03-15T02:34:14Z

Thanks @jackofallops for the comments, I applied your change and pushed again, apologize for my mistakes last time.

Regarding the acc test import steps, I think we still need to keep the ignore step for those app setting:

"app_settings.WEBSITE_CONTENTSHARE", "app_settings.WEBSITE_CONTENTAZUREFILECONNECTIONSTRING", "app_settings.%"

As I mentioned in my previous comments, service will try to add the WEBSITE_CONTENTSHARE and WEBSITE_CONTENTAZUREFILECONNECTIONSTRING for the function app instance (in elastic or consumption plan) automatically if no WEBSITE_CONTENTOVERVNET is added by user, which means, we need to determine whether to set those kvp back to state by checking the user's TF config and that's why we need to ignore the changes when doing the import. So I'm not going to change the original unpackappsetting function:https://github.com/hashicorp/terraform-provider-azurerm/blob/main/internal/services/appservice/linux_function_app_resource.go#L1090

jackofallops · 2023-03-15T06:08:45Z

Thanks @jackofallops for the comments, I applied your change and pushed again, apologize for my mistakes last time.

Regarding the acc test import steps, I think we still need to keep the ignore step for those app setting:
"app_settings.WEBSITE_CONTENTSHARE", "app_settings.WEBSITE_CONTENTAZUREFILECONNECTIONSTRING", "app_settings.%"
As I mentioned in my previous comments, service will try to add the WEBSITE_CONTENTSHARE and WEBSITE_CONTENTAZUREFILECONNECTIONSTRING for the function app instance (in elastic or consumption plan) automatically if no WEBSITE_CONTENTOVERVNET is added by user, which means, we need to determine whether to set those kvp back to state by checking the user's TF config and that's why we need to ignore the changes when doing the import. So I'm not going to change the original unpackappsetting function:main/internal/services/appservice/linux_function_app_resource.go#L1090

Hi @xiaxyi - Apologies, you are correct. These should be ignored on Import as the service does not manage them if absent. I'll message you offline to discuss.

jackofallops · 2023-03-15T14:09:34Z

Tests look good

jackofallops

Thanks @xiaxyi for the updates and patience. This LGTM now 👍

github-actions · 2023-03-16T21:59:14Z

This functionality has been released in v3.48.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

github-actions · 2023-04-16T02:12:21Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

fix contentshare property for function apps

30bc77f

github-actions bot added documentation service/app-service labels Sep 6, 2022

xiaxyi changed the title ~~fix contentshare property for function apps~~ azurerm_windows_function_app, azurerm_linux_function_app: fix content share not being created automatically issue Sep 6, 2022

xiaxyi mentioned this pull request Sep 17, 2022

Function app erroneously changes WEBSITE_CONTENTSHARE after apply #18388

Closed

1 task

jackofallops requested changes Oct 14, 2022

View reviewed changes

github-actions bot added the waiting-response label Oct 14, 2022

katbyte assigned jackofallops Oct 18, 2022

Merge remote-tracking branch 'upstream/main' into functionapp/content…

1ed5a51

…share

github-actions bot removed the waiting-response label Oct 19, 2022

github-actions bot added waiting-response and removed waiting-response labels Oct 20, 2022

xiaxyi added 2 commits November 7, 2022 16:41

Merge remote-tracking branch 'upstream/main' into functionapp/content…

eeaceb1

…share

update

f3a2ec7

github-actions bot added the size/L label Nov 9, 2022

xiaxyi mentioned this pull request Nov 9, 2022

azurerm_linux_function_app Storage options not working #19134

Closed

1 task

xiaxyi added 2 commits November 9, 2022 15:04

update sku

cff8ae1

update

705f9b0

xiaxyi changed the title ~~azurerm_windows_function_app, azurerm_linux_function_app: fix content share not being created automatically issue~~ azurerm_windows_function_app, azurerm_linux_function_app - fix content share not being created automatically issue Nov 11, 2022

jackofallops force-pushed the functions/contentShare branch from 705f9b0 to edfa6b5 Compare March 7, 2023 22:09

github-actions bot added dependencies service/api-management service/application-insights service/automation service/cdn service/digital-twins labels Mar 7, 2023

github-actions bot added size/M and removed size/XL labels Mar 7, 2023

jackofallops requested changes Mar 7, 2023

View reviewed changes

github-actions bot added the waiting-response label Mar 7, 2023

xiaxyi added 3 commits March 9, 2023 17:10

merging upstream

c0dbfe5

Merge remote-tracking branch 'upstream' into functionapp/contentshare

84381e8

update content share

6c38648

github-actions bot added size/L and removed waiting-response size/M labels Mar 10, 2023

xiaxyi and others added 3 commits March 10, 2023 10:29

update test case

4f40f9e

update

4bfcde0

re-apply lost changes from merge main

6d995f7

jackofallops requested changes Mar 14, 2023

View reviewed changes

internal/services/appservice/linux_function_app_resource_test.go Show resolved Hide resolved

internal/services/appservice/windows_function_app_resource_test.go Show resolved Hide resolved

github-actions bot added the waiting-response label Mar 14, 2023

Merge remote-tracking branch 'upstream/main' into functionapp/content…

10adae1

…share

github-actions bot removed the waiting-response label Mar 15, 2023

update test case

8122a01

jackofallops approved these changes Mar 15, 2023

View reviewed changes

jackofallops merged commit 60264b8 into hashicorp:main Mar 15, 2023

github-actions bot added this to the v3.48.0 milestone Mar 15, 2023

jackofallops added a commit that referenced this pull request Mar 15, 2023

update for #18258

7ba3fb9

Dmitrii034 mentioned this pull request Mar 27, 2023

azurerm_linux_function_app - WEBSITE_CONTENTAZUREFILECONNECTIONSTRING app setting does not get set by storage-related attributes #21140

Closed

1 task

github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_windows_function_app`, `azurerm_linux_function_app` - fix content share not being created automatically issue #18258

`azurerm_windows_function_app`, `azurerm_linux_function_app` - fix content share not being created automatically issue #18258

xiaxyi commented Sep 6, 2022 •

edited

Loading

jackofallops left a comment

jackofallops Oct 14, 2022

jackofallops Oct 14, 2022

jackofallops Oct 14, 2022

xiaxyi Oct 19, 2022

tombuildsstuff Oct 20, 2022

xiaxyi Oct 21, 2022

tombuildsstuff Oct 21, 2022 •

edited

Loading

xiaxyi Oct 24, 2022

jackofallops Oct 14, 2022

xiaxyi commented Oct 20, 2022

xiaxyi commented Nov 9, 2022

jackofallops left a comment

xiaxyi commented Mar 10, 2023

jackofallops left a comment

xiaxyi commented Mar 15, 2023 •

edited

Loading

jackofallops commented Mar 15, 2023

jackofallops commented Mar 15, 2023

jackofallops left a comment

github-actions bot commented Mar 16, 2023

github-actions bot commented Apr 16, 2023

azurerm_windows_function_app, azurerm_linux_function_app - fix content share not being created automatically issue #18258

azurerm_windows_function_app, azurerm_linux_function_app - fix content share not being created automatically issue #18258

Conversation

xiaxyi commented Sep 6, 2022 • edited Loading

jackofallops left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaxyi commented Oct 20, 2022

xiaxyi commented Nov 9, 2022

jackofallops left a comment

Choose a reason for hiding this comment

xiaxyi commented Mar 10, 2023

jackofallops left a comment

Choose a reason for hiding this comment

xiaxyi commented Mar 15, 2023 • edited Loading

jackofallops commented Mar 15, 2023

jackofallops commented Mar 15, 2023

jackofallops left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 16, 2023

github-actions bot commented Apr 16, 2023

`azurerm_windows_function_app`, `azurerm_linux_function_app` - fix content share not being created automatically issue #18258

`azurerm_windows_function_app`, `azurerm_linux_function_app` - fix content share not being created automatically issue #18258

xiaxyi commented Sep 6, 2022 •

edited

Loading

tombuildsstuff Oct 21, 2022 •

edited

Loading

xiaxyi commented Mar 15, 2023 •

edited

Loading