azurerm_windows_web_app, azurerm_windows_web_app_slot, azurerm_linux_web_app, azurerm_linux_web_app_slot - add the default ip action

[xiaxyi]

For windows and linux web app, there is a switch to turn on/ off the network access from the site level called: ipSecurityRestrictionsDefaultAction for the main site and scmIpSecurityRestrictionsDefaultAction for the scm site.

referfence:https://learn.microsoft.com/en-us/azure/app-service/app-service-ip-restrictions?tabs=azurecli
User can't deny all access based on current Terraform provider as we have the ip address validation when specifying the ip_address in ip_restriction block.

Acc test failure doesn't relate to the added property:

make acctests SERVICE='appservice' TESTARGS=' -run=TestAccWindowsWebApp_withDefaultIPAccessEnabled' TESTTIMEOUT='60m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./internal/services/appservice -run=TestAccWindowsWebApp_withDefaultIPAccessEnabled -timeout 60m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccWindowsWebApp_withDefaultIPAccessEnabled
=== PAUSE TestAccWindowsWebApp_withDefaultIPAccessEnabled
=== CONT  TestAccWindowsWebApp_withDefaultIPAccessEnabled
    testcase.go:113: Step 1/5 error: After applying this test step, the plan was not empty.
        stdout:


        Terraform used the selected providers to generate the following execution
        plan. Resource actions are indicated with the following symbols:
          ~ update in-place

        Terraform will perform the following actions:

          # azurerm_windows_web_app.test will be updated in-place
          ~ resource "azurerm_windows_web_app" "test" {
              ~ ftp_publish_basic_authentication_enabled       = false -> true
                id                                             = "/subscriptions/85b3dbca-5974-4067-9669-67a141095a76/resourceGroups/acctestRG-240117142551902316/providers/Microsoft.Web/sites/acctestWA-240117142551902316"
                name                                           = "acctestWA-240117142551902316"
        Plan: 0 to add, 1 to change, 0 to destroy.
--- FAIL: TestAccWindowsWebApp_withDefaultIPAccessEnabled (211.77s)
FAIL
FAIL    github.com/hashicorp/terraform-provider-azurerm/internal/services/appservice    220.669s
FAIL

fix #22593
Partially duplicate to PR #24464 as it only covered Linux web app which the feature is also available to windows web app as well.

[TrueSvenpai]

Thanks @xiaxyi for opening the PR!
One thing I'd like to mention is that in the ExpandForUpdate method, the IPSecurityRestrictionsDefaultAction and ScmIPSecurityRestrictionsDefaultAction are set only if they have changed. When dealing with Azure policies related to public access or public IPs, these policies can deny any new updates one wants to apply because the properties were not sent. My solution and the main reason for creating my pull request (#24464) was to always send these properties no matter what, so the policies are satisfied. Any ideas or opinions on that?
Thanks!

[xiaxyi]

Thanks @TrueSvenpai for the comment, would you mind be more specific about the update part? Do you mean the azure policy may change the status of the property IPSecurityRestrictionsDefaultAction ?

[TrueSvenpai]

Hi @xiaxyi ,
We have some company policies which check ipSecurityRestrictionsDefaultAction and publicNetworkAccess. The provider sends the ipSecurityRestrictionsDefaultAction attribute only if its resource is created or if the value of the attribute is updated. If ipSecurityRestrictionsDefaultAction is not updated after creation, the provider omits the value from the request and in combination with publicNetworkAccess = Enabled the policies deny the update.
Therefore we need those values to be present in create and update everytime even if there are no changes.
In your code, this would mean to remove the if condition if metadata.ResourceData.HasChange ... for scm_ip_access_enabled and ip_access_enabled.
I hope this clarifies my previous comment

[xiaxyi]

Thanks @TrueSvenpai for the explanation, if my understanding is correct, the property ipSecurityRestrictionsDefaultAction will be set to null if we don't include it in the request body? if this is the case, the behavior should always be as above(null to override previous value) no matter if public_network_access presents or not.

I tested the behavior, firstly not include the public ip access as :

resource "azurerm_linux_web_app" "test" {
  name                = "xiaxintestWA-vanguard"
  location            = azurerm_resource_group.test.location
  resource_group_name = azurerm_resource_group.test.name
  service_plan_id     = azurerm_service_plan.test.id

  site_config {
    ip_restriction {
      ip_address = "10.10.10.10/32"
      name       = "test-restriction"
      priority   = 123
      action     = "Allow"
      headers {
        x_azure_fdid      = ["55ce4ed1-4b06-4bf1-b40e-4638452104da"]
        x_fd_health_probe = ["1"]
        x_forwarded_for   = ["9.9.9.9/32", "2002::1234:abcd:ffff:c0a8:101/64"]
        x_forwarded_host  = ["example.com"]
      }
    }
    ip_access_enabled = false
  }
}

then update leaving the default ip unchanged and added the public access:

resource "azurerm_linux_web_app" "test" {
  name                = "xiaxintestWA-vanguard"
  location            = azurerm_resource_group.test.location
  resource_group_name = azurerm_resource_group.test.name
  service_plan_id     = azurerm_service_plan.test.id

  public_network_access_enabled = true
  site_config {
    ip_restriction {
      ip_address = "10.10.10.10/32"
      name       = "test-restriction"
      priority   = 123
      action     = "Allow"
      headers {
        x_azure_fdid      = ["55ce4ed1-4b06-4bf1-b40e-4638452104da"]
        x_fd_health_probe = ["1"]
        x_forwarded_for   = ["9.9.9.9/32", "2002::1234:abcd:ffff:c0a8:101/64"]
        x_forwarded_host  = ["example.com"]
      }
    }
    ip_access_enabled = false
  }
}

The network setting remains the same: (API GET response)

   "ipSecurityRestrictions": [
            {
                "ipAddress": "10.10.10.10/32",
                "action": "Allow",
                "tag": "Default",
                "priority": 123,
                "name": "test-restriction",
                "headers": {
                    "x-azure-fdid": [
                        "55ce4ed1-4b06-4bf1-b40e-4638452104da"
                    ],
                    "x-fd-healthprobe": [
                        "1"
                    ],
                    "x-forwarded-for": [
                        "9.9.9.9/32",
                        "2002::1234:abcd:ffff:c0a8:101/64"
                    ],
                    "x-forwarded-host": [
                        "example.com"
                    ]
                }
            },
            {
                "ipAddress": "Any",
                "action": "Deny",
                "priority": 2147483647,
                "name": "Deny all",
                "description": "Deny all access"
            }
        ],
        "ipSecurityRestrictionsDefaultAction": "Deny",

It seems to me that the property remains what it was if no changes in the TF config. Please correct me if there is any misunderstanding, or it would be better if you can share some simple config with me for testing.

[xiaxyi]

@TrueSvenpai Good day! Any comments on my last response would be appreciated! :)

[TrueSvenpai]

@xiaxyi Good Day!
Sorry for not answering, I wasn't able to test anything the last days.
I will update you soon, most likely tomorrow morning and provide some more information and send you corresponding error messages in our setup.

[xiaxyi]

@TrueSvenpai No worries about the delay response. Let me know once you have any updates

[andaryjo]

Hi @xiaxyi, thanks a lot for implementing this. I originally reported the issue in #22593 and my colleagues made me aware of this PR.

The way we originally noticed that these properties are missing is because we have deny-mode Azure Policies in place that prevent customers from deploying App Service Web Apps with public network access enabled in case no IP security restrictions are configured. Our policy needs to evaluate the properties "ipSecurityRestrictionsDefaultAction" and "scmIpSecurityRestrictionsDefaultAction", because we cannot allow deployment of Web Apps that allow all Internet traffic by default.

The way deny-mode Azure policies work is that they only can evaluate deployment requests (meaning: API request payloads). They do not know what the corresponding Azure resource provider decides to do with that deployment request after it passed policy validation. Because of that it is crucial that API clients always perform proper PUT requests and always send all the attributes of the resource, even if they did not change.

I'm not too familiar with how the Terraform provider works internally, but as far as I can tell from the resulting API requests, this is how other resources are implemented as well. Also the Azure CLI and Portal usually do this.

[andaryjo]

Another suggestion: Maybe the property name in Terraform could be something along "ip_restriction_allow_by_default" if it has to be a bool.

The name "ip_access_enabled" could be somewhat misleading because the property does not enable or disable IP access (that's what "public_network_access" does), but rather configures the firewall action in case no more specific rules apply.

I think users would have a hard time understanding why they need to set ip_access_enabled = false, if they want to allow public network access, but only for a specific IP address.

[TrueSvenpai]

Hi @xiaxyi,
Adding to @andaryjo comments:
I made a quick test and saved the response. I created a LinuxWebApp and set publicNetworkAccess: true as well as scmIpAccessEnabled: false and ipAccessEnabled: false. During the creation of the resource, all attributes are set and sent to azure.
After the creation, I changed some attributes of the LinuxWebApp and deployed the changes.
Because neither scmIpAccessEnabled nor ipAccessEnabled changed both will not be present in the request. Therefor our policy won't allow this updated, because it can only check the given update not the current state of the resource

In my PR, both scmIpAccessEnabled and ipAccessEnabled are set to the update regardless of any changes:

func (s *SiteConfigLinux) ExpandForUpdate(metadata sdk.ResourceMetaData, existing *web.SiteConfig, appSettings map[string]string) (*web.SiteConfig, error) {
    expanded := *existing
    ...
    expanded.ScmIPSecurityRestrictionsDefaultAction = web.DefaultActionAllow
    expanded.IPSecurityRestrictionsDefaultAction = web.DefaultActionAllow
    
    if !s.IpAccessEnabled {
    expanded.IPSecurityRestrictionsDefaultAction = web.DefaultActionDeny
    }
    if !s.ScmIpAccessEnabled {
        expanded.ScmIPSecurityRestrictionsDefaultAction = web.DefaultActionDeny
    }

Then the policy can check in the update for both attributes and allow the update. This is the only difference between our code for the linuxwebapp

[xiaxyi]

Thanks @TrueSvenpai and @andaryjo for your comment, if my understanding is correct, are you suggesting that your azure policy is checking the request body and then decide whether to accept/ reject the request? In my tests result, the value won't be changed even if it's not included in the request payload and does that mean if the create action is allowed by Azure policy at first place, the web app should be allowed if the property is not included in the request body when doing the update? I don't think it's a good idea of removing the hasChange check for it as the ignore_change won't work properly...

[xiaxyi]

@andaryjo I will consider changing the name foir these two properties, but now we need to hold the change as we are migrating the app service SDK to Hashicorp azure go sdk, after the sdk change, we need to rebase the code, so any changes will need be waited after that...

[xiaxyi]

@TrueSvenpai good day! Do you have any questions to my last comment about the update part? let me know if you have any concern.

[andaryjo]

@xiaxyi Sorry for my late response.

does that mean if the create action is allowed by Azure policy at first place, the web app should be allowed if the property is not included in the request body when doing the update?

Unfortunately the policy framework has no access to the resource's current state. It can only evaluate what is present in the API request. So when an Azure policy evaluates a resource deployment (either create or update), it does not know whether the resource already exists and gets updated or whether it gets newly created. It also cannot access current properties of a potential already existing resource.

To not allow constellations which should normally get denied, when you author Azure policies and a property gets omitted from a request, you need to suppose that in the worst case the resource does not yet exist.

This means that always all properties of a resource must end up in the deployment request. Even if you only update the resource and the properties did not change, they must be included in the request. The Azure REST API handles it like this:

• If a client performs a PUT request, it is expected to supply ALL resource properties which then get passed to the Azure policy evaluation framework.
• If a client performs a PATCH request, it only needs to supply the properties that changed. The Azure REST API will then calculate the diff to the currently existing resource and pass a deployment request containing ALL resource properties to the policy evaluation framework.

I'm not familar enough with the Terraform provider to say how it should handle this. All I know is, if it performs a PUT request, it must supply all resource properties for policy evaluations to work correctly. If it performs a PATCH request, it should be sufficient to only provide the properties that changed.

But there is a catch. I noticed PATCH API requests not passing our custom Azure policy when they don't explicitly provide the ipSecurityRestrictionsDefaultAction. And I have to take a wild guess here, because I don't know anything about how the Azure API handles this internally, but the underlying problem might be that the API data model for the App Service Web App does not include the property ipSecurityRestrictionsDefaultAction in some API versions even though it gets used by Azure clients (I reported this bug here: Azure/azure-rest-api-specs#24215). Because the property is not known to the API, I suspect that the calculation of the diff for PATCH requests does not work properly and the deployment request passed to the policy does not contain ipSecurityRestrictionsDefaultAction.

To see a confirmation of what I'm talking about, simply deploy an App Service Web App through the Azure Portal and look into the HTTP requests your browser sends. You will see the Azure Portal performing PUT requests to the Azure Management API which always include the siteConfig.ipSecurityRestrictionsDefaultAction, even though that property does not even exist in this API version.

To reproduce:

1. Create a Web App with public network access enabled.
2. Under Networking > public network access, set unmatched rule action to "Deny".
3. Change some setting, for example Configuration > General settings > Major version. This will trigger a request to the Azure API batch endpoint, which contains an encoded PUT request which contains ALL resource properties, even siteConfig.ipSecurityRestrictionsDefaultAction.

[xiaxyi]

Thanks @andaryjo for the explanation, let me involve our PR reviewer @jackofallops here for a discussion. @jackofallops Do you have any comment about removing the hasChange condition for the property ipSecurityRestrictionsDefaultAction and ScmIPSecurityRestrictionsDefaultAction and always include them in the payload in update function?

Besides of the hasChange check, I updated the property naming from ip_access_enabled to default_ip_access_enabled.

[andaryjo]

I've ran some tests to confirm what @TrueSvenpai said. I've created a azurerm_linux_web_app resource like this:

resource "azurerm_linux_web_app" "test" {
  name                = "policytestxxx"
  resource_group_name = data.azurerm_resource_group.rg.name
  location            = azurerm_service_plan.plan.location
  service_plan_id     = azurerm_service_plan.plan.id

  public_network_access_enabled = true

  site_config {
    default_ip_access_enabled = false
    scm_default_ip_access_enabled = false

    ip_restriction {
      name       = "rule"
      action     = "Allow"
      priority   = 100
      ip_address = "1.1.1.1/32"
    }
  }
}

Using trace logging mode on the provider reveals the API requests that the provider makes to the Azure REST API. For initial creation, the provider performs a PUT request which contains the new ipSecurityRestrictionsDefaultAction properties.

xxx [DEBUG] provider.terraform-provider-azurerm: AzureRM Request: 
PUT /subscriptions/xxx/resourceGroups/xxx/providers/Microsoft.Web/sites/policytestxxx?api-version=2023-01-01 HTTP/1.1
Host: management.azure.com
User-Agent: HashiCorp/go-azure-sdk (Go-http-Client/1.1 webapps/2023-01-01) HashiCorp Terraform/1.7.3 (+https://www.terraform.io) Terraform Plugin SDK/2.10.1 terraform-provider-azurerm/dev pid-xxx
Content-Length: 1087
Content-Type: application/json; charset=utf-8
X-Ms-Correlation-Request-Id: xxx
Accept-Encoding: gzip

{"identity":{"type":"None","userAssignedIdentities":null},"location":"westeurope","properties":{"clientAffinityEnabled":false,"clientCertEnabled":false,"clientCertMode":"Required","enabled":true,"httpsOnly":false,"publicNetworkAccess":"Enabled","serverFarmId":"/subscriptions/xxx/resourceGroups/xxx/providers/Microsoft.Web/serverFarms/policytestxxx","siteConfig":{"acrUseManagedIdentityCreds":false,"alwaysOn":true,"autoHealEnabled":false,"ftpsState":"Disabled","http20Enabled":false,"ipSecurityRestrictions":[{"action":"Allow","ipAddress":"1.1.1.1/32","name":"rule","priority":100}],"ipSecurityRestrictionsDefaultAction":"Deny","loadBalancing":"LeastRequests","localMySqlEnabled":false,"managedPipelineMode":"Integrated","minTlsVersion":"1.2","publicNetworkAccess":"Enabled","remoteDebuggingEnabled":false,"scmIpSecurityRestrictionsDefaultAction":"Deny","scmIpSecurityRestrictionsUseMain":false,"scmMinTlsVersion":"1.2","use32BitWorkerProcess":true,"vnetRouteAllEnabled":false,"webSocketsEnabled":false},"vnetRouteAllEnabled":false},"tags":null}: timestamp="xxx"

If I now update the resource (for example change the IP address in the ip_restriction block, the Terraform provider now still performs a PUT request, but omits the ipSecurityRestrictionsDefaultAction properties:

xxx [DEBUG] provider.terraform-provider-azurerm: AzureRM Request: 
PUT /subscriptions/xxx/resourceGroups/xxx/providers/Microsoft.Web/sites/policytestxxx?api-version=2023-01-01 HTTP/1.1
Host: management.azure.com
User-Agent: HashiCorp/go-azure-sdk (Go-http-Client/1.1 webapps/2023-01-01) HashiCorp Terraform/1.7.3 (+https://www.terraform.io) Terraform Plugin SDK/2.10.1 terraform-provider-azurerm/dev pid-xxx
Content-Length: 2673
Content-Type: application/json; charset=utf-8
X-Ms-Correlation-Request-Id: xxx
Accept-Encoding: gzip

{"id":"/subscriptions/xxx/resourceGroups/xxx/providers/Microsoft.Web/sites/policytestxxx","kind":"app,linux","location":"West Europe","name":"policytestxxx","properties":{"availabilityState":"Normal","clientAffinityEnabled":false,"clientCertEnabled":false,"clientCertMode":"Required","containerSize":0,"customDomainVerificationId":"xxx","dailyMemoryTimeQuota":0,"defaultHostName":"policytestxxx.azurewebsites.net","enabled":true,"enabledHostNames":["policytestxxx.azurewebsites.net","policytestxxx.scm.azurewebsites.net"],"httpsOnly":false,"hostNameSslStates":[{"hostType":"Standard","name":"policytestxxx.azurewebsites.net","sslState":"Disabled"},{"hostType":"Repository","name":"policytestxxx.scm.azurewebsites.net","sslState":"Disabled"}],"hostNames":["policytestxxx.azurewebsites.net"],"hostNamesDisabled":false,"hyperV":false,"isXenon":false,"keyVaultReferenceIdentity":"SystemAssigned","lastModifiedTimeUtc":"xxx,"outboundIpAddresses":"4xxx","possibleOutboundIpAddresses":"xxx","publicNetworkAccess":"Enabled","redundancyMode":"None","repositorySiteName":"policytestxxx","reserved":true,"resourceGroup":"xxx","scmSiteAlsoStopped":false,"serverFarmId":"/subscriptions/xxx/resourceGroups/rg-spoke/providers/Microsoft.Web/serverfarms/policytestxxx","siteConfig":{"acrUseManagedIdentityCreds":false,"alwaysOn":true,"autoHealEnabled":false,"functionAppScaleLimit":0,"http20Enabled":false,"ipSecurityRestrictions":[{"action":"Allow","ipAddress":"1.1.1.2/32","name":"rule","priority":100}],"linuxFxVersion":"","localMySqlEnabled":false,"minimumElasticInstanceCount":0,"numberOfWorkers":1,"remoteDebuggingEnabled":false,"scmIpSecurityRestrictionsUseMain":false,"use32BitWorkerProcess":true,"vnetRouteAllEnabled":false,"webSocketsEnabled":false},"state":"Running","storageAccountRequired":false,"usageState":"Normal","vnetContentShareEnabled":false,"vnetImagePullEnabled":false,"vnetRouteAllEnabled":false},"type":"Microsoft.Web/sites"}: timestamp="xxx"

You can see that the Terraform provider fetched all the properties of the resource (even those that have been assigned server-side) and supplies them in the PUT request. Which makes sense, since with a PUT request, the server expects you to provide the whole resource. But the ipSecurityRestrictionsDefaultAction properties are not included which makes it impossible for them to get evaluated by the Azure Policy framework.

My understanding is, if the Terraform provider is only supplying some resource properties to the request, it should perform PATCH requests. But if it performs PUT requests, it needs to supply all properties of the resource for policy evaluatio to work properly.

[jackofallops]

Hi @xiaxyi - As discussed offline, I'm going to close this in favour of a different approach that also covers all the supporting resources in one go.

Thanks again!

[andaryjo]

@jackofallops could you please give some more insights into what that approach is, whether there's already a new pull request or what the rough timeline here is?

After spending so much time and effort discussing this issue, it's really frustrating to me when you guys simply close it without any real explanation. Hope you can understand that.

[andaryjo]

Nevermind, just found the follow-up PR. For future visitors of this thread: #25131

[github-actions]

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.