`azurerm_app_service` `ip_restriction` - support for `name` & `priority` #6705

aqche · 2020-04-30T02:02:50Z

Adds the name and priority options to the azurerm_app_service resource's ip_restriction option.

=== RUN   TestAccAzureRMAppService_completeIpRestriction
=== PAUSE TestAccAzureRMAppService_completeIpRestriction
=== CONT  TestAccAzureRMAppService_completeIpRestriction
--- PASS: TestAccAzureRMAppService_completeIpRestriction (143.55s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/web/tests   143.564s

=== RUN   TestAccDataSourceAzureRMAppService_ipRestriction
=== PAUSE TestAccDataSourceAzureRMAppService_ipRestriction
=== CONT  TestAccDataSourceAzureRMAppService_ipRestriction
--- PASS: TestAccDataSourceAzureRMAppService_ipRestriction (147.63s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/web/tests   147.645s

sebader · 2020-05-01T12:57:09Z

azurerm/helpers/azure/app_service.go

+								Type:         schema.TypeInt,
+								Optional:     true,
+								Computed:     true,
+								ValidateFunc: validation.IntAtLeast(1),


Should you check here as well if priority is maximum 65000?

are you sure about the max value being 65000? after some testing, it looks like the max value I can set for priority is 2147483647. If I go above that, priority will be set to a negative value.

good question actually. I just tried to set a rule via az powershell:
Add-AzWebAppAccessRestrictionRule: Cannot validate argument on parameter 'Priority'. The 65001 argument is greater than the maximum allowed range of 65000. Supply an argument that is less than or equal to 65000 and then try the command again.

The portal lets you also set something larger than 65000 and enforces the limit at 2147483647. So I guess you are right, that should be it.

sebader · 2020-05-01T12:59:05Z

website/docs/r/app_service.html.markdown

@@ -309,6 +309,10 @@ A `ip_restriction` block supports the following:

 -> **NOTE:** One of either `ip_address` or `virtual_network_subnet_id` must be specified

+* `name` - (Optional) The name for this IP Restriction.
+
+* `priority` - (Optional) The priority for this IP Restriction. Restrictions are enforced in priority order.


Maybe add a note that it defaults to 65000

sure, thanks for the info

aqche · 2020-05-01T22:46:49Z

azurerm/helpers/azure/app_service.go

+							"priority": {
+								Type:         schema.TypeInt,
+								Optional:     true,
+								Computed:     true,


as @sebader noted, priority defaults to 65000 when not provided. Should we make that an explicit default or let it be computed?

I'm not sure where the default of 65000 is injected in this case. Might be in the Go SDK? Since in CLI, Powershell and the portal the parameter must be manually specified, there is no default there.

So might actually be better so explicitly set it?!

sebader · 2020-05-04T07:38:22Z

nice, looks good to me know @aqche Thank you!

jackofallops

Hi @aqche
Thanks for this PR. Just a few minor changes needed and I think this should be good to merge.

Ste

jackofallops · 2020-05-05T15:01:13Z

azurerm/helpers/azure/app_service.go

+							"name": {
+								Type:         schema.TypeString,
+								Optional:     true,
+								ValidateFunc: validation.StringIsNotEmpty,
+							},


I think we'll want Computed: true, on here.

jackofallops · 2020-05-05T15:03:36Z

azurerm/helpers/azure/app_service.go

+							"priority": {
+								Type:         schema.TypeInt,
+								Optional:     true,
+								Default:      65000,
+								ValidateFunc: validation.IntBetween(1, 2147483647),
+							},


I think this should be Computed: true, rather than setting a default. If more than one IP Restriction is used this could cause a conflict. The "default" value is assigned by the platform, rather than it being in the SDK, so can safely be read back from the API if not explicitly set.

jackofallops · 2020-05-05T15:33:49Z

azurerm/internal/services/web/tests/resource_arm_app_service_test.go

@@ -514,6 +514,27 @@ func TestAccAzureRMAppService_oneIpRestriction(t *testing.T) {
 	})
 }

+func TestAccAzureRMAppService_completeIpRestriction(t *testing.T) {


Could we have an additional test that updates an app_service by adding, then removing, a 2nd ip_restriction? (or extend this one)

aqche · 2020-05-06T22:27:05Z

@jackofallops thanks for the review! made the updates you suggested. let me know what you think!

=== RUN   TestAccAzureRMAppService_completeIpRestriction
=== PAUSE TestAccAzureRMAppService_completeIpRestriction
=== CONT  TestAccAzureRMAppService_completeIpRestriction
--- PASS: TestAccAzureRMAppService_completeIpRestriction (266.53s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/web/tests   266.555s

jackofallops · 2020-05-07T13:25:40Z

Failures are transient at Azure side.

bentterp · 2020-05-08T11:02:14Z

When I compare with the configuration options in the Azure portal, it seems that the action "allow/deny" is missing.

If there were no deny rules, then there would be no need for assigning priorities - similar to NSG rules.

ghost · 2020-06-06T14:50:18Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

add name and priority to ip_restriction for app_service

27e5d73

ghost added size/M documentation labels Apr 30, 2020

make priority computed

88f3786

sebader reviewed May 1, 2020

View reviewed changes

sebader mentioned this pull request May 1, 2020

Problem with IP Restriction rules in azurerm_app_service resource #2488

Closed

changes for review feedback

1270e90

aqche commented May 1, 2020

View reviewed changes

aqche added 2 commits May 1, 2020 15:47

update max allowed priority

b3c16f4

make dfault explicit

c3da179

jackofallops requested changes May 5, 2020

View reviewed changes

katbyte added enhancement service/app-service waiting-response labels May 6, 2020

address review

f1caf09

ghost added size/L and removed size/M labels May 6, 2020

ghost removed the waiting-response label May 6, 2020

aqche and others added 4 commits May 6, 2020 16:16

formatting

581e48e

formatting

4c3ce82

formatting

fc209f9

fix broken test param for remoteDebugging

a6362e4

jackofallops approved these changes May 7, 2020

View reviewed changes

jackofallops merged commit 59f2ddd into hashicorp:master May 7, 2020

jackofallops added a commit that referenced this pull request May 7, 2020

Update for #6705

d1c5abb

sebader mentioned this pull request May 12, 2020

App Service IpRestriction - sometimes fails with error about priority setting #6876

Closed

jjperry69 mentioned this pull request May 13, 2020

Support for Action of Allow or Deny in ip_restriction of azurerm_app_service #6900

Closed

aqche deleted the app_service_ip_restriction_name_and_priority branch May 21, 2020 23:51

hashicorp locked and limited conversation to collaborators Jun 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_app_service` `ip_restriction` - support for `name` & `priority` #6705

`azurerm_app_service` `ip_restriction` - support for `name` & `priority` #6705

aqche commented Apr 30, 2020

sebader May 1, 2020

aqche May 1, 2020

aqche May 1, 2020 •

edited

sebader May 2, 2020

sebader May 1, 2020

aqche May 1, 2020

aqche May 1, 2020

sebader May 2, 2020

sebader May 2, 2020

sebader commented May 4, 2020

jackofallops left a comment

jackofallops May 5, 2020

jackofallops May 5, 2020

jackofallops May 5, 2020

aqche commented May 6, 2020 •

edited

jackofallops commented May 7, 2020

bentterp commented May 8, 2020

ghost commented Jun 6, 2020

azurerm_app_service ip_restriction - support for name & priority #6705

azurerm_app_service ip_restriction - support for name & priority #6705

Conversation

aqche commented Apr 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqche May 1, 2020 • edited

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

sebader commented May 4, 2020

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

aqche commented May 6, 2020 • edited

jackofallops commented May 7, 2020

bentterp commented May 8, 2020

ghost commented Jun 6, 2020

`azurerm_app_service` `ip_restriction` - support for `name` & `priority` #6705

`azurerm_app_service` `ip_restriction` - support for `name` & `priority` #6705

aqche May 1, 2020 •

edited

aqche commented May 6, 2020 •

edited