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.

Sign up for GitHub

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

Jump to bottom

azurerm_app_service ip_restriction - support for name & priority #6705

Merged
merged 10 commits into from May 7, 2020
Merged

azurerm_app_service ip_restriction - support for name & priority #6705

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
27e5d73
add name and priority to ip_restriction for app_service
aqche Apr 30, 2020
88f3786
make priority computed
aqche Apr 30, 2020
1270e90
changes for review feedback
aqche May 1, 2020
b3c16f4
update max allowed priority
aqche May 1, 2020
c3da179
make dfault explicit
aqche May 4, 2020
f1caf09
address review
aqche May 6, 2020
581e48e
formatting
aqche May 6, 2020
4c3ce82
formatting
aqche May 6, 2020
fc209f9
formatting
aqche May 6, 2020
a6362e4
fix broken test param for remoteDebugging
jackofallops May 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 35 additions & 0 deletions azurerm/helpers/azure/app_service.go
View file
Open in desktop
Expand Up @@ -316,6 +316,17 @@ func SchemaAppServiceSiteConfig() *schema.Schema {
Optional: true,
ValidateFunc: validation.StringIsNotEmpty,
},
"name": {
Type: schema.TypeString,
Optional: true,
ValidateFunc: validation.StringIsNotEmpty,
},
Copy link
Member

Choose a reason for hiding this comment

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

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

"priority": {
Type: schema.TypeInt,
Optional: true,
Computed: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

So might actually be better so explicitly set it?!

ValidateFunc: validation.IntAtLeast(1),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call

Copy link
Contributor Author

@aqche aqche May 1, 2020 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

},
},
},
},
Expand Down Expand Up @@ -679,6 +690,14 @@ func SchemaAppServiceDataSourceSiteConfig() *schema.Schema {
Type: schema.TypeString,
Computed: true,
},
"name": {
Type: schema.TypeString,
Computed: true,
},
"priority": {
Type: schema.TypeInt,
Computed: true,
},
},
},
},
Expand Down Expand Up @@ -1419,6 +1438,8 @@ func ExpandAppServiceSiteConfig(input interface{}) (*web.SiteConfig, error) {

ipAddress := restriction["ip_address"].(string)
vNetSubnetID := restriction["virtual_network_subnet_id"].(string)
name := restriction["name"].(string)
priority := restriction["priority"].(int)
if vNetSubnetID != "" && ipAddress != "" {
return siteConfig, fmt.Errorf(fmt.Sprintf("only one of `ip_address` or `virtual_network_subnet_id` can be set for `site_config.0.ip_restriction.%d`", i))
}
Expand All @@ -1440,6 +1461,14 @@ func ExpandAppServiceSiteConfig(input interface{}) (*web.SiteConfig, error) {
ipSecurityRestriction.VnetSubnetResourceID = &vNetSubnetID
}

if name != "" {
ipSecurityRestriction.Name = &name
}

if priority != 0 {
ipSecurityRestriction.Priority = utils.Int32(int32(priority))
}

restrictions = append(restrictions, ipSecurityRestriction)
}
siteConfig.IPSecurityRestrictions = &restrictions
Expand Down Expand Up @@ -1564,6 +1593,12 @@ func FlattenAppServiceSiteConfig(input *web.SiteConfig) []interface{} {
if vNetSubnetID := v.VnetSubnetResourceID; vNetSubnetID != nil {
block["virtual_network_subnet_id"] = *vNetSubnetID
}
if name := v.Name; name != nil {
block["name"] = *name
}
if priority := v.Priority; priority != nil {
block["priority"] = *priority
}
restrictions = append(restrictions, block)
}
}
Expand Down
4 changes: 3 additions & 1 deletion azurerm/internal/services/web/tests/data_source_app_service_test.go
View file
Open in desktop
Expand Up @@ -137,6 +137,8 @@ func TestAccDataSourceAzureRMAppService_ipRestriction(t *testing.T) {
Config: testAccDataSourceAppService_ipRestriction(data),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(data.ResourceName, "site_config.0.ip_restriction.0.ip_address", "10.10.10.10/32"),
resource.TestCheckResourceAttr(data.ResourceName, "site_config.0.ip_restriction.0.name", "test-restriction"),
resource.TestCheckResourceAttr(data.ResourceName, "site_config.0.ip_restriction.0.priority", "123"),
),
},
},
Expand Down Expand Up @@ -271,7 +273,7 @@ data "azurerm_app_service" "test" {
}

func testAccDataSourceAppService_ipRestriction(data acceptance.TestData) string {
config := testAccAzureRMAppService_oneIpRestriction(data)
config := testAccAzureRMAppService_completeIpRestriction(data)
return fmt.Sprintf(`
%s

Expand Down
60 changes: 60 additions & 0 deletions azurerm/internal/services/web/tests/resource_arm_app_service_test.go
View file
Open in desktop
Expand Up @@ -514,6 +514,27 @@ func TestAccAzureRMAppService_oneIpRestriction(t *testing.T) {
})
}

func TestAccAzureRMAppService_completeIpRestriction(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

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

data := acceptance.BuildTestData(t, "azurerm_app_service", "test")
resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { acceptance.PreCheck(t) },
Providers: acceptance.SupportedProviders,
CheckDestroy: testCheckAzureRMAppServiceDestroy,
Steps: []resource.TestStep{
{
Config: testAccAzureRMAppService_completeIpRestriction(data),
Check: resource.ComposeTestCheckFunc(
testCheckAzureRMAppServiceExists(data.ResourceName),
resource.TestCheckResourceAttr(data.ResourceName, "site_config.0.ip_restriction.0.ip_address", "10.10.10.10/32"),
resource.TestCheckResourceAttr(data.ResourceName, "site_config.0.ip_restriction.0.name", "test-restriction"),
resource.TestCheckResourceAttr(data.ResourceName, "site_config.0.ip_restriction.0.priority", "123"),
),
},
data.ImportStep(),
},
})
}

func TestAccAzureRMAppService_oneVNetSubnetIpRestriction(t *testing.T) {
data := acceptance.BuildTestData(t, "azurerm_app_service", "test")
resource.ParallelTest(t, resource.TestCase{
Expand Down Expand Up @@ -2582,6 +2603,45 @@ resource "azurerm_app_service" "test" {
`, data.RandomInteger, data.Locations.Primary, data.RandomInteger, data.RandomInteger)
}

func testAccAzureRMAppService_completeIpRestriction(data acceptance.TestData) string {
return fmt.Sprintf(`
provider "azurerm" {
features {}
}

resource "azurerm_resource_group" "test" {
name = "acctestRG-%d"
location = "%s"
}

resource "azurerm_app_service_plan" "test" {
name = "acctestASP-%d"
location = azurerm_resource_group.test.location
resource_group_name = azurerm_resource_group.test.name

sku {
tier = "Standard"
size = "S1"
}
}

resource "azurerm_app_service" "test" {
name = "acctestAS-%d"
location = azurerm_resource_group.test.location
resource_group_name = azurerm_resource_group.test.name
app_service_plan_id = azurerm_app_service_plan.test.id

site_config {
ip_restriction {
ip_address = "10.10.10.10/32"
name = "test-restriction"
priority = 123
}
}
}
`, data.RandomInteger, data.Locations.Primary, data.RandomInteger, data.RandomInteger)
}

func testAccAzureRMAppService_oneVNetSubnetIpRestriction(data acceptance.TestData) string {
return fmt.Sprintf(`
provider "azurerm" {
Expand Down
4 changes: 4 additions & 0 deletions website/docs/d/app_service.html.markdown
View file
Open in desktop
Expand Up @@ -85,6 +85,10 @@ A `ip_restriction` block exports the following:

* `subnet_mask` - The Subnet mask used for this IP Restriction.

* `name` - The name for this IP Restriction.

* `priority` - The priority for this IP Restriction.

---

`site_config` supports the following:
Expand Down
4 changes: 4 additions & 0 deletions website/docs/r/app_service.html.markdown
View file
Open in desktop
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a note that it defaults to 65000

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, thanks for the info


---

A `microsoft` block supports the following:
Expand Down