azurerm_function_app - support for ip_restriction #5440

shibayan · 2020-01-19T09:31:14Z

Add an ip_restriction definition to the Function App. This PR fixes #4878

katbyte

Thanks for the pr @shibayan, thanks for the PR.

My main concern is that we have copied over the hacks in the old code, i think we should mimic the behavior the API expects: a CIDR ip address or the subnet ID.

katbyte · 2020-01-22T04:42:48Z

azurerm/internal/services/web/resource_arm_function_app.go

+										// TODO we should fix this in 2.0
+										// This attribute was made with the assumption that `ip_address` was the only valid option
+										// but `virtual_network_subnet_id` is being added and doesn't need a `subnet_mask`.
+										// We'll assume a default of "255.255.255.255" in the expand code when `ip_address` is specified
+										// and `subnet_mask` is not.
+										// Default:  "255.255.255.255",


I think we can remove this comment?

katbyte · 2020-01-22T04:43:25Z

website/docs/r/function_app.html.markdown

+
+* `ip_address` - (Optional) The IP Address used for this IP Restriction.
+
+* `subnet_mask` - (Optional) The Subnet mask used for this IP Restriction. Defaults to `255.255.255.255`.


This is not true as the default was commented out?

katbyte · 2020-01-22T04:44:12Z

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

+`, data.RandomInteger, data.Locations.Primary, data.RandomString, data.RandomInteger, data.RandomInteger)
+}
+
+func testAccAzureRMFunctionApp_zeroedIpRestriction(data acceptance.TestData) string {


Suggested change

func testAccAzureRMFunctionApp_zeroedIpRestriction(data acceptance.TestData) string {

func testAccAzureRMFunctionApp_ipRestrictionRemoved(data acceptance.TestData) string {

katbyte · 2020-01-22T04:44:33Z

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

+	})
+}
+
+func TestAccAzureRMFunctionApp_zeroedIpRestriction(t *testing.T) {


Suggested change

func TestAccAzureRMFunctionApp_zeroedIpRestriction(t *testing.T) {

func TestAccAzureRMFunctionApp_ipRestrictionRemoved(t *testing.T) {

katbyte · 2020-01-22T04:46:03Z

azurerm/internal/services/web/resource_arm_function_app.go

@@ -225,6 +227,36 @@ func resourceArmFunctionApp() *schema.Resource {
 							Optional: true,
 							Default:  false,
 						},
+						"ip_restriction": {
+							Type:       schema.TypeList,


Should this be a TypeSet to prevent duplicates & order won't matter here?

I am trying to change to a TypeSet but it is not working as intended. In my case it will take some time to resolve.

It shoul just be a matter of changing how you cast the objects, if you grant me push permissions i'll happily make the change

I have granted push permission, you can freely add commits to this Pull Request. I tried #5319 for a similar change, but was investigating because the plan no longer worked.

katbyte · 2020-01-22T04:46:59Z

azurerm/internal/services/web/resource_arm_function_app.go

+				// the 2018-02-01 API expects a blank subnet mask and an IP address in CIDR format: a.b.c.d/x
+				// so translate the IP and mask if necessary


Can we make this block behave like the API? ie remove the subnet mask property and expect the IP address to be in the CIDR format?

katbyte · 2020-01-22T04:47:29Z

azurerm/internal/services/web/resource_arm_function_app.go

+				// the 2018-02-01 API uses CIDR format (a.b.c.d/x), so translate that back to IP and mask
+				if strings.Contains(*ip, "/") {
+					ipAddr, ipNet, _ := net.ParseCIDR(*ip)
+					block["ip_address"] = ipAddr.String()
+					mask := net.IP(ipNet.Mask)
+					block["subnet_mask"] = mask.String()
+				} else {
+					block["ip_address"] = *ip
+				}


Could we make the behave like the new API expects, no subnet mask and ip address in cidr format

katbyte · 2020-01-22T04:50:26Z

azurerm/internal/services/web/resource_arm_function_app.go

+										Type:     schema.TypeString,
+										Optional: true,
+									},
+									"virtual_network_subnet_id": {


Also could we name this subnet_id to match other resources?

shibayan · 2020-01-23T01:50:44Z

@katbyte
Thank you for your feedback.
I thought it would be better to have compatibility with azurerm_app_service. Is it OK for the function app to have different properties and behaviors?

katbyte · 2020-01-23T04:44:25Z

Yes @shibayan, app service should probably also be updated to behave this way for 2.0

shibayan · 2020-01-23T07:48:34Z

I understood. I’ll modify the code to get the new behavior.

tombuildsstuff

hey @shibayan

Thanks for pushing those changes - apologies for the delayed re-review here!

Taking a look through besides a couple of minor comments (which I hope you don't mind but so that we can get this merged I'll push a commit to fix) this otherwise LGTM 👍

Thanks!

azurerm/internal/services/web/resource_arm_function_app.go

tombuildsstuff · 2020-02-11T09:52:12Z

Ignoring some expected test failures (which will be resolved outside of this PR) the tests otherwise look good:

dismissing since changes have been pushed

…lds are always set

shibayan · 2020-02-11T10:54:18Z

@tombuildsstuff Thanks for the review and improvement. I'm sure it's better.

ghost · 2020-02-12T16:15:28Z

This has been released in version 1.44.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 = "~> 1.44.0"
}
# ... other configuration ...

ghost · 2020-03-28T15:09:19Z

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!

azurerm_function_app - support for ip_restriction

7b68276

ghost added size/XL documentation labels Jan 19, 2020

Fix code formatting issue

3971b88

katbyte added enhancement service/functions Function Apps labels Jan 22, 2020

katbyte previously requested changes Jan 22, 2020

View reviewed changes

katbyte reviewed Jan 22, 2020

View reviewed changes

katbyte added the waiting-response label Jan 22, 2020

ghost removed the waiting-response label Jan 23, 2020

katbyte added the waiting-response label Jan 23, 2020

shibayan added 6 commits January 26, 2020 02:36

Changed test case method name

9593ff8

Changed virtual_network_subnet_id property name

7df4387

Remove subnet_mask property

1b462a4

Update document

2d4be93

Fix tflint error

c9603dc

Extract ip_restriction expand and flatten methods

7cea183

katbyte added this to the v1.43.0 milestone Jan 31, 2020

tombuildsstuff modified the milestones: v1.43.0, v1.44.0 Feb 4, 2020

tombuildsstuff approved these changes Feb 11, 2020

View reviewed changes

azurerm/internal/services/web/resource_arm_function_app.go Outdated Show resolved Hide resolved

azurerm/internal/services/web/resource_arm_function_app.go Show resolved Hide resolved

tombuildsstuff removed the waiting-response label Feb 11, 2020

r/function_app: ensuring the ip_address and subnet_id address fie…

a6d7227

…lds are always set

tombuildsstuff merged commit 8c1e5dd into hashicorp:master Feb 11, 2020

tombuildsstuff added a commit that referenced this pull request Feb 11, 2020

updating to include #5440

c8c4275

shibayan deleted the function_ip_restriction branch February 11, 2020 10:54

ghost locked and limited conversation to collaborators Mar 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

azurerm_function_app - support for ip_restriction #5440

azurerm_function_app - support for ip_restriction #5440

shibayan commented Jan 19, 2020

katbyte left a comment

katbyte Jan 22, 2020

shibayan Jan 26, 2020

katbyte Jan 22, 2020

katbyte Jan 22, 2020

katbyte Jan 22, 2020

katbyte Jan 22, 2020

shibayan Jan 27, 2020

katbyte Jan 31, 2020

shibayan Feb 1, 2020

katbyte Jan 22, 2020

shibayan Jan 26, 2020

katbyte Jan 22, 2020

shibayan Jan 26, 2020

katbyte Jan 22, 2020

shibayan commented Jan 23, 2020

katbyte commented Jan 23, 2020

shibayan commented Jan 23, 2020

tombuildsstuff left a comment

tombuildsstuff commented Feb 11, 2020

shibayan commented Feb 11, 2020

ghost commented Feb 12, 2020

ghost commented Mar 28, 2020


		* `ip_address` - (Optional) The IP Address used for this IP Restriction.

		* `subnet_mask` - (Optional) The Subnet mask used for this IP Restriction. Defaults to `255.255.255.255`.

	func testAccAzureRMFunctionApp_zeroedIpRestriction(data acceptance.TestData) string {
	func testAccAzureRMFunctionApp_ipRestrictionRemoved(data acceptance.TestData) string {

		// the 2018-02-01 API expects a blank subnet mask and an IP address in CIDR format: a.b.c.d/x
		// so translate the IP and mask if necessary

azurerm_function_app - support for ip_restriction #5440

azurerm_function_app - support for ip_restriction #5440

Conversation

shibayan commented Jan 19, 2020

katbyte 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

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

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

shibayan commented Jan 23, 2020

katbyte commented Jan 23, 2020

shibayan commented Jan 23, 2020

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Feb 11, 2020

shibayan commented Feb 11, 2020

ghost commented Feb 12, 2020

ghost commented Mar 28, 2020