azurerm_app_service added support for ip-restrictions scm/kudu site
#6955
Conversation
SebRosander
changed the title
azurerm_app_service added support for ip-restrictions scm/kudo site
|
added action - allow or deny to match PR: #6967 |
|
@katbyte @jackofallops is there anything blocking this PR? we are waiting for this feature for a while and would be great to get it out :) thanks! |
There was a problem hiding this comment.
Hi @SebRosander
Thanks for this PR, it's looking pretty good. I've put some comments and changes below. If you can address those, we'll look to get this run through testing and merged asap.
Thanks again!
|
|
||
| * `scm_ip_restriction` - (Optional) A [List of objects](/docs/configuration/attr-as-blocks.html) representing ip restrictions as defined below. | ||
|
|
||
| -> **NOTE** User has to explicitly set `ip_restriction` to empty slice (`[]`) to remove it. |
There was a problem hiding this comment.
Copy-paste error here?
| -> **NOTE** User has to explicitly set `ip_restriction` to empty slice (`[]`) to remove it. | |
| -> **NOTE** User has to explicitly set `scm_ip_restriction` to empty slice (`[]`) to remove it. |
| @@ -197,6 +197,14 @@ A `site_config` block supports the following: | |||
|
|
|||
| -> **NOTE** User has to explicitly set `ip_restriction` to empty slice (`[]`) to remove it. | |||
|
|
|||
| * `scm_use_main_ip_restriction` - (Optional) IP security restrictions for scm to use main. Defaults to false. | |||
|
|
|||
| -> **NOTE** Can't be use together with `scm_ip_restriction`. | |||
There was a problem hiding this comment.
Neither the API nor the resource prevent these both being set, and anything configured is both accepted and returned by the API, though any restrictions are ignored by the service. It might be better to have:
| -> **NOTE** Can't be use together with `scm_ip_restriction`. | |
| -> **NOTE** Any `scm_ip_restriction` blocks configured are ignored by the service when `scm_use_main_ip_restriction` is set to `true`. Any scm restrictions will become active if this is subsequently set to `false` or removed. |
azurerm/helpers/azure/app_service.go
Outdated
| "priority": { | ||
| Type: schema.TypeInt, | ||
| Optional: true, | ||
| Computed: true, | ||
| ValidateFunc: validation.IntBetween(1, 2147483647), | ||
| }, |
There was a problem hiding this comment.
This should probably not be computed, but have a default of 65000. If any restriction is given a priority, then all must have one.
| "priority": { | |
| Type: schema.TypeInt, | |
| Optional: true, | |
| Computed: true, | |
| ValidateFunc: validation.IntBetween(1, 2147483647), | |
| }, | |
| "priority": { | |
| Type: schema.TypeInt, | |
| Optional: true, | |
| Default: 65000, | |
| ValidateFunc: validation.IntBetween(1, 2147483647), | |
| }, |
This is also the case for the "main" ip_restriction, that can be addressed separately (it's on my todo list...) to keep the scope of this PR focused.
| * `priority` - (Optional) The priority for this IP Restriction. Restrictions are enforced in priority order. By default, priority is set to 65000 if not specified. | ||
|
|
There was a problem hiding this comment.
The service does not set default priority for restrictions if any restriction item has a priority explicitly set, this can be addressed with the schema change suggestion above.
| Config: testAccAzureRMAppService_scmUseMainIPRestriction(data), | ||
| Check: resource.ComposeTestCheckFunc( | ||
| testCheckAzureRMAppServiceExists(data.ResourceName), | ||
| resource.TestCheckResourceAttr(data.ResourceName, "site_config.0.scm_use_main_ip_restriction", "true"), |
There was a problem hiding this comment.
For resources the data.ImportStep() checks these values, so this line can be removed safely.
| resource.TestCheckResourceAttr(data.ResourceName, "site_config.0.scm_ip_restriction.#", "1"), | ||
| resource.TestCheckResourceAttr(data.ResourceName, "site_config.0.scm_ip_restriction.0.ip_address", "10.10.10.10/32"), | ||
| resource.TestCheckResourceAttr(data.ResourceName, "site_config.0.scm_ip_restriction.0.name", "test-restriction"), | ||
| resource.TestCheckResourceAttr(data.ResourceName, "site_config.0.scm_ip_restriction.0.priority", "123"), | ||
| resource.TestCheckResourceAttr(data.ResourceName, "site_config.0.scm_ip_restriction.0.action", "Allow"), |
There was a problem hiding this comment.
As above, covered by data.ImportStep()
| CheckDestroy: testCheckAzureRMAppServiceSlotDestroy, | ||
| Steps: []resource.TestStep{ | ||
| { | ||
| // This configuration includes a single explicit ip_restriction |
There was a problem hiding this comment.
| // This configuration includes a single explicit ip_restriction | |
| // This configuration includes a single explicit scm_ip_restriction |
| ), | ||
| }, | ||
| { | ||
| // This configuration explicitly sets ip_restriction to [] using attribute syntax. |
There was a problem hiding this comment.
| // This configuration explicitly sets ip_restriction to [] using attribute syntax. | |
| // This configuration explicitly sets scm_ip_restriction to [] using attribute syntax. |
| resource.TestCheckResourceAttr(data.ResourceName, "site_config.0.scm_ip_restriction.0.ip_address", "10.10.10.10/32"), | ||
| resource.TestCheckResourceAttr(data.ResourceName, "site_config.0.scm_ip_restriction.1.ip_address", "20.20.20.0/24"), | ||
| resource.TestCheckResourceAttr(data.ResourceName, "site_config.0.scm_ip_restriction.2.ip_address", "30.30.0.0/16"), | ||
| resource.TestCheckResourceAttr(data.ResourceName, "site_config.0.scm_ip_restriction.3.ip_address", "192.168.1.2/24"), |
There was a problem hiding this comment.
As above, covered by data.ImportStep()
|
|
||
| * `name` - The name for this IP Restriction. | ||
|
|
||
| * `priority` - The priority for this IP Restriction. Restrictions are enforced in priority order. By default, priority is set to 65000 if not specified. |
There was a problem hiding this comment.
A brief description is probably more appropriate here.
| * `priority` - The priority for this IP Restriction. Restrictions are enforced in priority order. By default, priority is set to 65000 if not specified. | |
| * `priority` - The priority for this IP Restriction. |
jackofallops
changed the title
azurerm_app_service added support for ip-restrictions scm/kudo siteazurerm_app_service added support for ip-restrictions scm/kudu site
|
Hi @jackofallops ! Thanks for your review & comments :) Have updated accordingly. Let me know if there is something else needed and I'll fix it ASAP. |
|
Tests pass: |
|
This has been released in version 2.15.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azurerm" {
version = "~> 2.15.0"
}
# ... other configuration ... |
|
Would it be a lot of work to get the same functionalities also for I mistakenly thought that this PR would have added that support, even though the title clearly says that it's for |
|
@matti-bragge-solita Yeah, my idea was to get that started as soon as this PR got accepted. More or less copy pasta mode for the function app. Mind opening a issue for this? And I’ll have a look when I get some spare time 😊 |
|
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 👉 [email protected]. Thanks! |
ghost
locked and limited conversation to collaborators
Fixes #3685