Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

azurerm_app_service added support for ip-restrictions scm/kudu site #6955

Merged
merged 5 commits into from Jun 18, 2020
Merged

azurerm_app_service added support for ip-restrictions scm/kudu site #6955

merged 5 commits into from Jun 18, 2020

Conversation

SebRosander
Copy link
Contributor

@SebRosander SebRosander commented May 15, 2020

Fixes #3685

@SebRosander SebRosander changed the title Added support for scm ip restrictions. azurerm_app_service added support for ip-restrictions scm/kudo site May 16, 2020
@SebRosander
Copy link
Contributor Author

added action - allow or deny to match PR: #6967

@sebader
Copy link
Contributor

sebader commented Jun 3, 2020

@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!

Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy-paste error here?

Suggested change
-> **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`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
-> **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.

Comment on lines 364 to 369
"priority": {
Type: schema.TypeInt,
Optional: true,
Computed: true,
ValidateFunc: validation.IntBetween(1, 2147483647),
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably not be computed, but have a default of 65000. If any restriction is given a priority, then all must have one.

Suggested change
"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.

Comment on lines 344 to 345
* `priority` - (Optional) The priority for this IP Restriction. Restrictions are enforced in priority order. By default, priority is set to 65000 if not specified.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For resources the data.ImportStep() checks these values, so this line can be removed safely.

Comment on lines 717 to 721
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"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, covered by data.ImportStep()

CheckDestroy: testCheckAzureRMAppServiceSlotDestroy,
Steps: []resource.TestStep{
{
// This configuration includes a single explicit ip_restriction
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// This configuration explicitly sets ip_restriction to [] using attribute syntax.
// This configuration explicitly sets scm_ip_restriction to [] using attribute syntax.

Comment on lines 793 to 796
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"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A brief description is probably more appropriate here.

Suggested change
* `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 jackofallops added this to the v2.15.0 milestone Jun 15, 2020
@jackofallops jackofallops changed the title azurerm_app_service added support for ip-restrictions scm/kudo site azurerm_app_service added support for ip-restrictions scm/kudu site Jun 15, 2020
@SebRosander
Copy link
Contributor Author

SebRosander commented Jun 15, 2020

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.

@jackofallops
Copy link
Member

Tests pass:
image
(failures transient and subsequently passed)

@jackofallops jackofallops merged commit 5d672a9 into hashicorp:master Jun 18, 2020
jackofallops added a commit that referenced this pull request Jun 18, 2020
@ghost
Copy link

ghost commented Jun 19, 2020

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 ...

@SebRosander SebRosander deleted the issue-3685 branch June 20, 2020 16:42
@matti-bragge-solita
Copy link

Would it be a lot of work to get the same functionalities also for azurerm_function_app?

I mistakenly thought that this PR would have added that support, even though the title clearly says that it's for azurerm_app_service. It could be because Azure portal uses app service and function app so interchangeably.

@SebRosander
Copy link
Contributor Author

SebRosander commented Jun 22, 2020

@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 😊

@ghost
Copy link

ghost commented Jul 18, 2020

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!

@hashicorp hashicorp locked and limited conversation to collaborators Jul 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support App Service "Access Restrictions" functionality
4 participants