`azurerm_linux_function_app`, `azurerm_linux_function_app_slot` - support for the `vnet_image_pull_enabled` property. #25249

xiaxyi · 2024-03-14T08:58:08Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave "+1" or "me too" comments, they generate extra noise for PR followers and do not help prioritize for review

PR Checklist

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
(For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Documentation Changes

Documentation is written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.

Description

The property is controlling the image pull feature in virtual network integration. Although the feature can be set via key WEBSITE_PULL_IMAGE_OVER_VNET under app_setting , given the fact that the service team would like to guide user to use this property under site_config, added it in Terraform as well.

Testing Logs/Evidence

Related Issue(s)

Fixes #0000

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

azurerm_linux_function_app - support for the vnet_image_pull_enabled property

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

stephybun · 2024-03-14T15:12:04Z

website/docs/r/linux_function_app.html.markdown

+* `vnet_image_pull_enabled` - (Optional) Should the traffic for the image pull be routed over virtual network enabled. Defaults to `false`.
+
+~> **Note:** The feature can also be enabled via the app setting `WEBSITE_PULL_IMAGE_OVER_VNET`.


What happens when a user sets this through the app settings block. Will they get a diff? Also what is the behaviour vice versa?

It looks like this property might need to be managed like WEBSITE_VNET_ROUTE_ALL/vnet_route_all_enabled

stephybun

@xiaxyi having looked at this a bit closer I discovered this behaviour after performing the following steps:

Create a linux function app with Terraform
Set WEBSITE_PULL_IMAGE_OVER_VNET to true in the app settings for the app over the portal
Running a Terraform plan the API returns the property vnetImagePullEnabled returns false

The documentation linked implies that both properties can be used to toggle the routing over the vnet, but from the behaviour shown above it's unclear which property takes precedence?

xiaxyi · 2024-03-22T09:13:34Z

Thanks @stephybun for the comment! I checked with the API team, and they still suggest the user to set the feature via the property under siteConfig not app setting, besides, they indicate that they are trying to phase out the kvp under app setting so we need to lead the user to start use the property vnetImagePullEnabled .

As for your concerns, the api will check both of the properties, so the feature will be enabled if either of them is set to true, the true one "wins".

stephybun · 2024-03-22T09:20:17Z

Thanks for clarifying @xiaxyi! Would you be able to find out from the service team what the timeline is for phasing out the WEBSITE_PULL_IMAGE_OVER_VNET property in app settings? Is this something that is going to happen in the near future or are we looking at a longer time frame here?

xiaxyi · 2024-03-25T07:46:59Z

@stephybun The removal of the app_setting won't be happening in the near future as there are lots of users are consuming this property, but the service team wish to guide the user to use the correct setting and that's why I'm adding it in Terraform side as well. Let me know if you have any concerns. :)

rcskosir · 2024-03-25T19:34:04Z

@xiaxyi Thank you for using our new PR Template! Can you please update it to include only the relevant check box sections? For this PR, since you are adding support to an existing resource, you can remove the "New Feature Submissions" check boxes for example, but should be checking the boxes for changes to an existing resource as well as the general PR checklist boxes. Please always include a description under the description heading. Thank you, this helps with the review process, as well as aids our users to better understand what's happening in this PR.

…roperties

stephybun

Thanks @xiaxyi, I left some suggestions to simplify and make this more consistent with other resources across the provider.

Given the behaviour of this property for Linux Function Apps, I don't think we can release this until 4.0 since in it's current state it would be a breaking change for users running in an App Service Environment. Can you please wrap those changes in the features.FourPointOhBeta() flag.

stephybun · 2024-03-27T14:03:51Z

internal/services/appservice/linux_function_app_resource.go

+			if isAseEnvironment && !functionApp.VnetImagePullEnabled {
+				return fmt.Errorf("`vnet_image_pull_enabled` cannot be disabled for app running in an app service environment.")
+			}
+


Let's move this into theif block on line 415 and simplify it to

Suggested change

if isAseEnvironment && !functionApp.VnetImagePullEnabled {

return fmt.Errorf("`vnet_image_pull_enabled` cannot be disabled for app running in an app service environment.")

}

if !functionApp.VnetImagePullEnabled {

return fmt.Errorf("`vnet_image_pull_enabled` cannot be disabled for app running in an app service environment.")

}

stephybun · 2024-03-27T14:05:09Z

internal/services/appservice/linux_function_app_resource.go

@@ -394,6 +402,7 @@ func (r LinuxFunctionAppResource) Create() sdk.ResourceFunc {
 			}

 			var planSKU *string
+			var isAseEnvironment bool


This can be removed then

Suggested change

var isAseEnvironment bool

stephybun · 2024-03-27T14:05:18Z

internal/services/appservice/linux_function_app_resource.go

@@ -404,6 +413,7 @@ func (r LinuxFunctionAppResource) Create() sdk.ResourceFunc {
 				}

 				if ase := servicePlanModel.Properties.HostingEnvironmentProfile; ase != nil {
+					isAseEnvironment = true


As can this

Suggested change

isAseEnvironment = true

stephybun · 2024-03-27T14:07:06Z

internal/services/appservice/linux_function_app_resource.go

+				_, newValue := rd.GetChange("vnet_image_pull_enabled")
+				servicePlanId, err := commonids.ParseAppServicePlanID(planId.(string))
+				if err != nil {
+					return fmt.Errorf("reading service plan id %+v", err)


The parse functions already return verbose information so we don't need to wrap the error

Suggested change

return fmt.Errorf("reading service plan id %+v", err)

return err

stephybun · 2024-03-27T14:07:44Z

internal/services/appservice/linux_function_app_resource.go

+
+				asp, err := client.Get(ctx, *servicePlanId)
+				if err != nil {
+					return fmt.Errorf("could not read Service Plan %s: %+v", servicePlanId, err)


Simplify this error message like we do elsewhere

Suggested change

return fmt.Errorf("could not read Service Plan %s: %+v", servicePlanId, err)

return fmt.Errorf("retrieving %s: %+v", servicePlanId, err)

stephybun · 2024-03-27T14:11:21Z

website/docs/r/linux_function_app.html.markdown

@@ -139,6 +139,10 @@ The following arguments are supported:

 ~> **Note:** Assigning the `virtual_network_subnet_id` property requires [RBAC permissions on the subnet](https://docs.microsoft.com/en-us/azure/app-service/overview-vnet-integration#permissions)

+* `vnet_image_pull_enabled` - (Optional) Should the traffic for the image pull be routed over virtual network enabled. Defaults to `false`.


Can we rephrase this to

Suggested change

* `vnet_image_pull_enabled` - (Optional) Should the traffic for the image pull be routed over virtual network enabled. Defaults to `false`.

* `vnet_image_pull_enabled` - (Optional) Specifies whether traffic for the image pull should be routed over virtual network. Defaults to `false`.

stephybun · 2024-03-27T14:25:33Z

website/docs/r/linux_function_app.html.markdown

@@ -139,6 +139,10 @@ The following arguments are supported:

 ~> **Note:** Assigning the `virtual_network_subnet_id` property requires [RBAC permissions on the subnet](https://docs.microsoft.com/en-us/azure/app-service/overview-vnet-integration#permissions)

+* `vnet_image_pull_enabled` - (Optional) Should the traffic for the image pull be routed over virtual network enabled. Defaults to `false`.
+
+~> **Note:** The feature can also be enabled via the app setting `WEBSITE_PULL_IMAGE_OVER_VNET`. The Setting is enabled by default for app running in the App Service Environment. 


Suggested change

~> **Note:** The feature can also be enabled via the app setting `WEBSITE_PULL_IMAGE_OVER_VNET`. The Setting is enabled by default for app running in the App Service Environment.

~> **Note:** The feature can also be enabled via the app setting `WEBSITE_PULL_IMAGE_OVER_VNET`. Must be set to `true` when running in an App Service Environment.

stephybun · 2024-03-27T14:29:28Z

website/docs/r/linux_function_app_slot.html.markdown

@@ -131,7 +131,11 @@ The following arguments are supported:
 ~> **NOTE on regional virtual network integration:** The AzureRM Terraform provider provides regional virtual network integration via the standalone resource [app_service_virtual_network_swift_connection](app_service_virtual_network_swift_connection.html) and in-line within this resource using the `virtual_network_subnet_id` property. You cannot use both methods simultaneously. If the virtual network is set via the resource `app_service_virtual_network_swift_connection` then `ignore_changes` should be used in the function app slot configuration.

 ~> **Note:** Assigning the `virtual_network_subnet_id` property requires [RBAC permissions on the subnet](https://docs.microsoft.com/en-us/azure/app-service/overview-vnet-integration#permissions)
-
+
+* `vnet_image_pull_enabled` - (Optional) Should the traffic for the image pull be routed over virtual network enabled. Defaults to `false`.


Suggested change

* `vnet_image_pull_enabled` - (Optional) Should the traffic for the image pull be routed over virtual network enabled. Defaults to `false`.

* `vnet_image_pull_enabled` - (Optional) Specifies whether traffic for the image pull should be routed over virtual network. Defaults to `false`.

…roperties

xiaxyi · 2024-03-29T08:55:48Z

@stephybun Thanks for the comment, of course I can wrap it in the features.FourPointOhBeta block, however, I need to use the metadata.resourceData.set() to set the property in Read function. Is it ok to have it in resource which is using the type SDK?

xiaxyi · 2024-04-05T10:34:52Z

Thanks @stephybun ! I updated the comment and test cases. For test cases related to ASE, as the vnet related features have to be enabled for app running in the app service environment, so I still put it in the ASE test and added comment accordingly. Let me know about your feedbacks.

stephybun

Thanks @xiaxyi. This is nearly ready but it is unfortunately not sufficient to leave a 4.0 property commented out in a test config as explained in the review comment. Can you please resolve all the comments, after that this should be good to go!

stephybun · 2024-04-18T05:58:39Z

internal/services/appservice/linux_function_app_resource.go

@@ -1210,6 +1237,29 @@ func (r LinuxFunctionAppResource) CustomizeDiff() sdk.ResourceFunc {
 		Func: func(ctx context.Context, metadata sdk.ResourceMetaData) error {
 			client := metadata.Client.AppService.ServicePlanClient
 			rd := metadata.ResourceDiff
+			if rd.HasChange("vnet_image_pull_enabled") {


Although this shouldn't be problematic, can we still add the FourPointOhBeta flag here to make it clear and consistent that this is a 4.0 property

Suggested change

if rd.HasChange("vnet_image_pull_enabled") {

if rd.HasChange("vnet_image_pull_enabled") && features.FourPointOhBeta() {

Please apply this to the other resources too

stephybun · 2024-04-18T06:01:09Z

internal/services/appservice/linux_function_app_resource_test.go

@@ -4244,6 +4247,7 @@ resource "azurerm_linux_function_app" "test" {
  storage_account_name       = azurerm_storage_account.test.name
  storage_account_access_key = azurerm_storage_account.test.primary_access_key

+  // vnet_image_pull_enabled = true 


When we hook the feature flag FourPointOhBeta up to an environment variable to be able to enable the beta and to test that the 4.0 features work, this will be problematic and require manual intervention to uncomment the line in order to test. This needs to be removed and added to a new test that is skipped prior to 4.0 e.g.

if !features.FourPointOhBeta() { t.Skip("this test requires 4.0 mode") }

Please make this change to the other resource tests too

stephybun · 2024-05-15T14:13:57Z

@xiaxyi this is good to go once the last round of review comments has been resolved

…roperties

…lag enabled

…roperties

…to appservice/addVnetProperties # Conflicts: # internal/services/appservice/linux_function_app_resource_test.go # internal/services/appservice/linux_function_app_slot_resource_test.go

…roperties

jackofallops

Hi @xiaxyi - Can you take a look at updating this PR based on the below comment?

Thanks!

jackofallops · 2024-07-22T13:35:51Z

internal/services/appservice/linux_function_app_resource.go

@@ -71,6 +72,8 @@ type LinuxFunctionAppModel struct {
 	PublishingFTPBasicAuthEnabled    bool                                       `tfschema:"ftp_publish_basic_authentication_enabled"`
 	Identity                         []identity.ModelSystemAssignedUserAssigned `tfschema:"identity"`

+	// VnetImagePullEnabled             bool                                       `tfschema:"vnet_image_pull_enabled"` // TODO 4.0 not supported on Consumption plans


This can be included now using the additional struct tag

Suggested change

// VnetImagePullEnabled bool `tfschema:"vnet_image_pull_enabled"` // TODO 4.0 not supported on Consumption plans

VnetImagePullEnabled bool `tfschema:"vnet_image_pull_enabled,addedInNextMajorVersion"` // TODO 4.0 not supported on Consumption plans

Can you also look to address the 4.0 TODO here within this PR?

jackofallops · 2024-08-12T11:04:21Z

Hi @xiaxyi - as I mentioned offline, I made some updates to this to move it forward. Unfortunately I can't push to your fork any more for some reason, so I've pushed your commits to a new branch to be able to update and I'm going to close this PR in favour of that.

github-actions · 2024-09-14T02:13:28Z

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.

adding vnetImagePullEnabled property to linux function app

5bf320a

github-actions bot added documentation service/app-service size/S labels Mar 14, 2024

stephybun requested changes Mar 14, 2024

View reviewed changes

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

change the default value for ase

4fa4d96

github-actions bot added size/M waiting-response and removed size/S waiting-response labels Mar 18, 2024

fix error

206124e

github-actions bot removed the waiting-response label Mar 18, 2024

xiaxyi added 2 commits March 18, 2024 19:29

update customize diff func

4f7253b

update for ase

62d0f52

stephybun reviewed Mar 22, 2024

View reviewed changes

github-actions bot added the waiting-response label Mar 22, 2024

github-actions bot removed the waiting-response label Mar 22, 2024

Merge remote-tracking branch 'upstream/main' into appService/addVnetP…

f444765

…roperties

stephybun requested changes Mar 27, 2024

View reviewed changes

stephybun added enhancement breaking-change labels Mar 27, 2024

github-actions bot added the waiting-response label Mar 27, 2024

Merge remote-tracking branch 'upstream/main' into appservice/addVnetP…

d958e2c

…roperties

github-actions bot removed the waiting-response label Mar 29, 2024

github-actions bot added size/XL and removed waiting-response size/L labels Apr 5, 2024

sorting imports

552e769

github-actions bot added the waiting-response label Apr 5, 2024

fmt

99d5023

github-actions bot removed the waiting-response label Apr 5, 2024

stephybun requested changes Apr 18, 2024

View reviewed changes

github-actions bot added the waiting-response label Apr 18, 2024

xiaxyi added 5 commits May 28, 2024 17:18

Merge remote-tracking branch 'upstream/main' into appservice/addVnetP…

472f314

…roperties

add new test cases for the vnet_image_pull_enable property with 4.0 f…

1c36adb

…lag enabled

Merge remote-tracking branch 'upstream/main' into appservice/addVnetP…

ca319a5

…roperties

Merge remote-tracking branch 'origin/appService/addVnetProperties' in…

caa7c04

…to appservice/addVnetProperties # Conflicts: # internal/services/appservice/linux_function_app_resource_test.go # internal/services/appservice/linux_function_app_slot_resource_test.go

add 4.0 flag for test cases based on the reviewer's comments

432dfe5

github-actions bot added waiting-response and removed waiting-response labels May 31, 2024

add unparam linter

a1fa339

github-actions bot added waiting-response and removed waiting-response labels May 31, 2024

Merge remote-tracking branch 'upstream/main' into appservice/addVnetP…

4ee6865

…roperties

github-actions bot removed the waiting-response label May 31, 2024

jackofallops requested changes Jul 22, 2024

View reviewed changes

github-actions bot added the waiting-response label Jul 22, 2024

jackofallops mentioned this pull request Aug 12, 2024

azurerm_[linux|windows]_function_app[_slot] - add support for vnet_image_pull_enabled property in 4.0 #27001

Merged

14 tasks

jackofallops closed this Aug 12, 2024

github-actions bot locked as resolved and limited conversation to collaborators Sep 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_linux_function_app`, `azurerm_linux_function_app_slot` - support for the `vnet_image_pull_enabled` property. #25249

`azurerm_linux_function_app`, `azurerm_linux_function_app_slot` - support for the `vnet_image_pull_enabled` property. #25249

xiaxyi commented Mar 14, 2024 •

edited

Loading

stephybun Mar 14, 2024

stephybun left a comment

xiaxyi commented Mar 22, 2024

stephybun commented Mar 22, 2024 •

edited

Loading

xiaxyi commented Mar 25, 2024

rcskosir commented Mar 25, 2024

stephybun left a comment

stephybun Mar 27, 2024

stephybun Mar 27, 2024

stephybun Mar 27, 2024

stephybun Mar 27, 2024

stephybun Mar 27, 2024

stephybun Mar 27, 2024

stephybun Mar 27, 2024

stephybun Mar 27, 2024

xiaxyi commented Mar 29, 2024

xiaxyi commented Apr 5, 2024

stephybun left a comment

stephybun Apr 18, 2024

stephybun Apr 18, 2024

stephybun commented May 15, 2024

jackofallops left a comment

jackofallops Jul 22, 2024

jackofallops commented Aug 12, 2024

github-actions bot commented Sep 14, 2024

		* `vnet_image_pull_enabled` - (Optional) Should the traffic for the image pull be routed over virtual network enabled. Defaults to `false`.

		~> Note: The feature can also be enabled via the app setting `WEBSITE_PULL_IMAGE_OVER_VNET`.

	return fmt.Errorf("reading service plan id %+v", err)
	return err

	return fmt.Errorf("could not read Service Plan %s: %+v", servicePlanId, err)
	return fmt.Errorf("retrieving %s: %+v", servicePlanId, err)

		@@ -139,6 +139,10 @@ The following arguments are supported:

		~> Note: Assigning the `virtual_network_subnet_id` property requires [RBAC permissions on the subnet](https://docs.microsoft.com/en-us/azure/app-service/overview-vnet-integration#permissions)

		* `vnet_image_pull_enabled` - (Optional) Should the traffic for the image pull be routed over virtual network enabled. Defaults to `false`.

	if rd.HasChange("vnet_image_pull_enabled") {
	if rd.HasChange("vnet_image_pull_enabled") && features.FourPointOhBeta() {

	// VnetImagePullEnabled bool `tfschema:"vnet_image_pull_enabled"` // TODO 4.0 not supported on Consumption plans
	VnetImagePullEnabled bool `tfschema:"vnet_image_pull_enabled,addedInNextMajorVersion"` // TODO 4.0 not supported on Consumption plans

azurerm_linux_function_app, azurerm_linux_function_app_slot - support for the vnet_image_pull_enabled property. #25249

azurerm_linux_function_app, azurerm_linux_function_app_slot - support for the vnet_image_pull_enabled property. #25249

Conversation

xiaxyi commented Mar 14, 2024 • edited Loading

Community Note

PR Checklist

Changes to existing Resource / Data Source

Documentation Changes

Description

Testing Logs/Evidence

Related Issue(s)

Change Log

Choose a reason for hiding this comment

stephybun left a comment

Choose a reason for hiding this comment

xiaxyi commented Mar 22, 2024

stephybun commented Mar 22, 2024 • edited Loading

xiaxyi commented Mar 25, 2024

rcskosir commented Mar 25, 2024

stephybun 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaxyi commented Mar 29, 2024

xiaxyi commented Apr 5, 2024

stephybun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephybun commented May 15, 2024

jackofallops left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackofallops commented Aug 12, 2024

github-actions bot commented Sep 14, 2024

`azurerm_linux_function_app`, `azurerm_linux_function_app_slot` - support for the `vnet_image_pull_enabled` property. #25249

`azurerm_linux_function_app`, `azurerm_linux_function_app_slot` - support for the `vnet_image_pull_enabled` property. #25249

xiaxyi commented Mar 14, 2024 •

edited

Loading

stephybun commented Mar 22, 2024 •

edited

Loading