Support address_prefixes for multiple prefixes in subnet and virtualnetwork (ipv6) #6148

fabianofranz · 2020-03-17T20:39:06Z

The azure subnet resource supports addressPrefix when one prefix is available and addressPrefixes when two or or more are set (for instance, when configuring an ipv4 and ipv6 dual stack configuration for a network). Update azurerm_subnet to allow either address_prefix or address_prefixes as input, where the latter is a list of string. Check that at least one is set, and mark the older address_prefix as deprecated.

The virtual network resource allows inline subnets, but does not currently support this argument. When the address_prefixes argument is used, address_prefix will be null when terraform refreshes its state causing terraform to panic. This code updates also the virtual network resource to allow
address_prefixes to be used and for address_prefix to be null.

Fixes #5140
Takes over #5139

abhinavdahiya · 2020-03-18T00:10:18Z

maybe commits from #6142 belong with this PR since they are addressing the same subnets with multiple address prefixes

fabianofranz · 2020-03-18T15:35:30Z

@abhinavdahiya makes sense, I added commit #6142 to this PR and closed the other one.

fabianofranz · 2020-03-23T17:02:26Z

@katbyte PTAL

katbyte

Hey @fabianofranz, looks like we're missing doc updates, could we update them? thanks!

katbyte · 2020-03-24T02:16:32Z

azurerm/internal/services/network/data_source_subnet.go

@@ -94,7 +100,18 @@ func dataSourceArmSubnetRead(d *schema.ResourceData, meta interface{}) error {
 	d.Set("virtual_network_name", virtualNetworkName)

 	if props := resp.SubnetPropertiesFormat; props != nil {
-		d.Set("address_prefix", props.AddressPrefix)
+		if props.AddressPrefix != nil {


we don't need a nil check here as d.Set will do it for us

katbyte · 2020-03-25T04:46:18Z

azurerm/internal/services/network/resource_arm_virtual_network.go

+						"address_prefixes": {
+							Type:     schema.TypeList,
+							Optional: true,
+							Elem:     &schema.Schema{Type: schema.TypeString},


could we validate this?

We should do the same validation I mentioned above here

mbfrahry

Hey @fabianofranz, this is off to a good start. I've left some comments on what we can do differently but the bulk of my issues are around testing. I've left some notes on one of the tests but it'd also be great to get a more comprehensive suite of tests since we're deprecating and we should ensure that the old functionality works with the new way of doing things. We'll also want to update the attributes by adding and removing address prefixes. Lastly, we'll need to update the documentation by deprecating the old field there and adding the new ones.

mbfrahry · 2020-03-26T17:54:28Z

azurerm/internal/services/network/resource_arm_subnet.go

+		properties.AddressPrefixes = nil
+	}
+	if !prefixSet {
+		return fmt.Errorf("[ERROR] either address_prefix or address_prefixes is required")


We can remove this check and prefixSet if we swap the ConflictsWith tag in the schema for address_prefix and address_prefixes with ExactlyOneOf: []string{"address_prefix", "address_prefixes"},

mbfrahry · 2020-03-26T17:56:48Z

azurerm/internal/services/network/resource_arm_subnet.go

@@ -306,7 +330,18 @@ func resourceArmSubnetRead(d *schema.ResourceData, meta interface{}) error {
 	d.Set("virtual_network_name", networkName)

 	if props := resp.SubnetPropertiesFormat; props != nil {
-		d.Set("address_prefix", props.AddressPrefix)
+		if props.AddressPrefix != nil {
+			d.Set("address_prefix", props.AddressPrefix)


Similar to Katie's comment above, we don't need to nil check this before setting it

mbfrahry · 2020-03-26T19:49:48Z

azurerm/internal/services/network/tests/data_source_subnet_test.go

@@ -30,6 +30,28 @@ func TestAccDataSourceSubnet_basic(t *testing.T) {
 	})
 }

+func TestAccDataSourceAzureRMSubnet_basic_addressPrefixes(t *testing.T) {


It looks like we're missing a similar test for the actual resource

This test also fails with:

Subnet is not valid because its IP address range is outside the IP address range of virtual network.

It looks like it's due to the ipv6 address that was specified.

Test now passes!

azurerm/internal/services/network/resource_arm_subnet.go

+			d.Set("address_prefix", props.AddressPrefix)
+		}
+		if props.AddressPrefixes == nil {
+			if props.AddressPrefix != nil && len(*props.AddressPrefix) > 0 {


mbfrahry · 2020-03-26T20:26:33Z

azurerm/internal/services/network/tests/resource_arm_virtual_network_test.go

+func testAccAzureRMVirtualNetwork_basic_addressPrefixes(data acceptance.TestData) string {
+	return fmt.Sprintf(`
+resource "azurerm_resource_group" "test" {
+  name     = "acctestRG-%d"


We should append the package initial to the resource group name like so:

Suggested change

name = "acctestRG-%d"

name = "acctestRG-n-%d"

so we can better track the resource groups that are getting hung up on what tests

Since most already existing tests already use that, I'd suggest to handle this for all tests in a separate PR. ;-)

I agree we should do it for the rest but we should do the best practice now for any new features and we can fix up the rest when we can

azurerm/internal/services/network/resource_arm_virtual_network.go

+				properties.AddressPrefix = &(*properties.AddressPrefixes)[0]
+				properties.AddressPrefixes = nil
+			}
+			if prefixSet == 0 {


fabianofranz · 2020-04-04T17:45:08Z

@katbyte @mbfrahry I'm having a test failure with ExactlyOneOf, do you have any clue? Tks!

--- FAIL: TestProvider (0.13s)
    provider_test.go:12: err: 1 error occurred:
        	* resource azurerm_virtual_network: ExactlyOneOf: address_prefix references unknown attribute (address_prefix) at part (address_prefix)

mbfrahry · 2020-04-07T04:46:21Z

Hey @fabianofranz, unfortunately ExactlyOneOf, ConflictsWith, and the others have a limitation in that they don't work quite the way you want them to for Lists and Sets. The error you're seeing is because the ExactlyOneOf logic is looking for address_prefix but it should really be looking for subnet.x.address_prefix where x is the index of the set you want to compare. The tricky bit is that we don't know how many subnets there could be so we can't effectively check that ExactlyOneOf works. Unfortunately, we'll need to do some of this logic manually for now with a note to remove it when we release 3.0 of the provider in the future.

You can use it with List/Set that has a MaxItems: 1 because we know that subnet.0.address_prefix will exist. But we can't use that Validation for any List/Set that we don't know the size of.

Also, I want to apologize for sending you down this rabbit hole. The files changed tab made it tricky to see that those values were in a Set. But the validation should be able to stay with the subnet resource

fabianofranz · 2020-04-14T20:22:14Z

Hey @fabianofranz, unfortunately ExactlyOneOf, ConflictsWith, and the others have a limitation in that they don't work quite the way you want them to for Lists and Sets. The error you're seeing is because the ExactlyOneOf logic is looking for address_prefix but it should really be looking for subnet.x.address_prefix where x is the index of the set you want to compare. The tricky bit is that we don't know how many subnets there could be so we can't effectively check that ExactlyOneOf works. Unfortunately, we'll need to do some of this logic manually for now with a note to remove it when we release 3.0 of the provider in the future.

You can use it with List/Set that has a MaxItems: 1 because we know that subnet.0.address_prefix will exist. But we can't use that Validation for any List/Set that we don't know the size of.

Also, I want to apologize for sending you down this rabbit hole. The files changed tab made it tricky to see that those values were in a Set. But the validation should be able to stay with the subnet resource

Ok @mbfrahry, thanks for the explanation. I updated this PR back to validations in the subnet resource and addressed most comments from you and @katbyte. I think it must be good for another round of reviews, thanks!

mbfrahry

Hey @fabianofranz, thanks for making my requested changes. The tests for the subnet resource/datasource side of things looks good. I just have some minor comments/fixes to address

I am having issues getting the AllowMultipleAddressPrefixesOnSubnet feature for virtual networks registered and so I can't move forward on reviewing that because I can't test it. I've opened a support ticket to see why that's the case but I'm not sure how long that'll take.

We can either wait some unknown amount of time to see what they come back with or we split this PR into two PRs. One for subnets and one for virtual networks and we get the subnets one in quickly. Apologies for the added work while we wait on the Azure side

mbfrahry · 2020-04-15T03:14:38Z

azurerm/internal/services/network/resource_arm_subnet.go

@@ -241,6 +263,11 @@ func resourceArmSubnetUpdate(d *schema.ResourceData, meta interface{}) error {
 		props.AddressPrefix = utils.String(d.Get("address_prefix").(string))
 	}

+	if d.HasChange("address_prefixes") {
+		addressPrefixesRaw := d.Get("address_prefixes").([]interface{})
+		props.AddressPrefixes = utils.ExpandStringSlice(addressPrefixesRaw)


You do logic in the Create method for when address prefixes length is 1. Do we want to mirror that here?

mbfrahry · 2020-04-15T03:23:07Z

azurerm/internal/services/network/resource_arm_subnet.go

+				Type:         schema.TypeList,
+				Optional:     true,
+				Computed:     true,
+				Elem:         &schema.Schema{Type: schema.TypeString},


Can you have an empty address prefix? If not we should definitely enforce that we pass something to the API through

Suggested change

Elem: &schema.Schema{Type: schema.TypeString},

Elem: &schema.Schema{

Type: schema.TypeString,

ValidateFunc: validation.StringIsNotEmpty,

},

mbfrahry · 2020-04-15T03:24:32Z

azurerm/internal/services/network/resource_arm_subnet.go

+				Type:         schema.TypeString,
+				Optional:     true,
+				Computed:     true,
+				Deprecated:   "Use the `address_prefixes` property instead.",


We should add a TODO comment here to remove these deprecations in the next major version. This will help us quickly find and remove all the deprecations when the time comes for the next major version

Suggested change

Deprecated: "Use the `address_prefixes` property instead.",

// TODO Remove this in the next major version release

Deprecated: "Use the `address_prefixes` property instead.",

mbfrahry · 2020-04-15T03:25:44Z

azurerm/internal/services/network/resource_arm_virtual_network.go

+						"address_prefixes": {
+							Type:     schema.TypeList,
+							Optional: true,
+							Elem:     &schema.Schema{Type: schema.TypeString},


We should do the same validation I mentioned above here

mbfrahry · 2020-04-15T03:36:07Z

azurerm/internal/services/network/resource_arm_virtual_network.go

+				properties.AddressPrefixes = nil
+			}
+			if prefixSet == 0 {
+				return nil, fmt.Errorf("[ERROR] either address_prefix or address_prefixes is required")


Suggested change

return nil, fmt.Errorf("[ERROR] either address_prefix or address_prefixes is required")

return nil, fmt.Errorf("one of `address_prefix` or `address_prefixes` must be set")

just cleaning up the language here to be more consistent with similar error messages.

mbfrahry · 2020-04-15T04:48:39Z

website/docs/r/subnet.html.markdown

@@ -101,7 +105,8 @@ The following attributes are exported:
 * `name` - The name of the subnet.
 * `resource_group_name` - The name of the resource group in which the subnet is created in.
 * `virtual_network_name` - The name of the virtual network in which the subnet is created in
-* `address_prefix` - The address prefix for the subnet
+* `address_prefix` - (Deprecated) The address prefix for the subnet
+* `address_prefixes` - The address prefixes for the subnet


We'll want to add something similar over on the datasource docs as well

mbfrahry · 2020-04-15T05:25:57Z

azurerm/internal/services/network/tests/resource_arm_virtual_network_test.go

+func testAccAzureRMVirtualNetwork_basic_addressPrefixes(data acceptance.TestData) string {
+	return fmt.Sprintf(`
+resource "azurerm_resource_group" "test" {
+  name     = "acctestRG-%d"


I agree we should do it for the rest but we should do the best practice now for any new features and we can fix up the rest when we can

mbfrahry · 2020-04-15T05:26:17Z

azurerm/internal/services/network/tests/resource_arm_subnet_test.go

+func testAccAzureRMSubnet_basic_addressPrefixes(data acceptance.TestData) string {
+	return fmt.Sprintf(`
+resource "azurerm_resource_group" "test" {
+  name     = "acctestRG-%d"


Suggested change

name = "acctestRG-%d"

name = "acctestRG-n-%d"

Updating for best practices

mbfrahry · 2020-04-15T05:26:42Z

azurerm/internal/services/network/tests/data_source_virtual_network_test.go

+func testAccDataSourceArmVirtualNetwork_basic_addressPrefixes(data acceptance.TestData) string {
+	return fmt.Sprintf(`
+resource "azurerm_resource_group" "test" {
+  name     = "acctest%d-rg"


Suggested change

name = "acctest%d-rg"

name = "acctest%d-n-rg"

Updating for best practices

mbfrahry · 2020-04-15T05:37:05Z

azurerm/internal/services/network/tests/data_source_subnet_test.go

@@ -30,6 +30,28 @@ func TestAccDataSourceSubnet_basic(t *testing.T) {
 	})
 }

+func TestAccDataSourceAzureRMSubnet_basic_addressPrefixes(t *testing.T) {


Test now passes!

fabianofranz · 2020-04-15T21:31:15Z

@mbfrahry thanks for the review, all latest comments were addressed.

I am having issues getting the AllowMultipleAddressPrefixesOnSubnet feature for virtual networks registered

I'm facing exactly the same issue when trying to test it since yesterday, and I'm pretty sure it wasn't happening until a few days ago. Let's wait and see how your ticket goes. ;-)

So here goes another PR that splits this one. If we're good to go with the subnets part of this, let's have the other PR going and once it lands, I'll rebase this one to focus on the VNet and wait for a solution about the test issue. Tks!

mbfrahry · 2020-04-17T22:00:16Z

Thanks for splitting this out @fabianofranz, I just heard back from support and they said that this feature isn't available for the public use yet and they aren't approving people to use it. I'll move this PR to our Blocked milestone until they release this feature to the public.

The azure subnet resource allows for multiple address prefixes using the address_prefixes argument. The virtual network resource allows inline subnets, but does not currently support this argument. When the address_prefixes argument is used, address_prefix will be null when terraform refreshes its state causing terraform to panic. This code updates the virtual network resource to allow address_prefixes to be used and for address_prefix to be null. Tests have been added to test the multiple prefixes. The magic number in the new tests is the same magic number used in the other VNet tests. I'm not 100% clear on this number, but it appears to be a known ID. It's origin is documented (not very well) here: #1913 This fix addresses several IPv6 related bugs.

abhinavdahiya · 2020-07-07T17:27:54Z

Any updates on this @fabianofranz @mbfrahry

mbfrahry · 2020-07-10T17:20:27Z

Closing this for now as it's not yet available for public use

ghost · 2020-08-10T14:51:12Z

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!

ghost added the size/M label Mar 17, 2020

ghost added size/L and removed size/M labels Mar 18, 2020

fabianofranz mentioned this pull request Mar 18, 2020

virtualnetwork: allow address_prefix to be null #6142

Closed

fabianofranz changed the title ~~subnet: Support address_prefixes for multiple prefixes (ipv6)~~ Support address_prefixes for multiple prefixes in subnet and virtualnetwork (ipv6) Mar 18, 2020

This was referenced Mar 18, 2020

subnet: Support address_prefixes for multiple prefixes (ipv6) #5139

Closed

[release-4.4] Bug 1808969: azure - vendor update to support multiple prefixes openshift/installer#3313

Merged

katbyte requested changes Mar 25, 2020

View reviewed changes

katbyte added service/subnets service/virtual-networks enhancement waiting-response labels Mar 25, 2020

mbfrahry requested changes Mar 26, 2020

View reviewed changes

ghost added size/XL documentation and removed size/L labels Apr 2, 2020

ghost removed the waiting-response label Apr 4, 2020

katbyte assigned mbfrahry Apr 15, 2020

mbfrahry requested changes Apr 15, 2020

View reviewed changes

fabianofranz mentioned this pull request Apr 15, 2020

azurerm_subnet: Support for multiple prefixes with address_prefixes #6493

Merged

mbfrahry added this to the Blocked milestone Apr 17, 2020

ghost added size/L and removed size/XL labels Apr 29, 2020

abhinavdahiya mentioned this pull request May 8, 2020

UPSTREAM 6148: virtualnetwork: allow address_prefix to be null openshift/terraform-provider-azurerm#3

Merged

mbfrahry closed this Jul 10, 2020

hashicorp locked and limited conversation to collaborators Aug 10, 2020

	Deprecated: "Use the `address_prefixes` property instead.",
	// TODO Remove this in the next major version release
	Deprecated: "Use the `address_prefixes` property instead.",

	return nil, fmt.Errorf("[ERROR] either address_prefix or address_prefixes is required")
	return nil, fmt.Errorf("one of `address_prefix` or `address_prefixes` must be set")

Support address_prefixes for multiple prefixes in subnet and virtualnetwork (ipv6) #6148

Support address_prefixes for multiple prefixes in subnet and virtualnetwork (ipv6) #6148

Conversation

fabianofranz commented Mar 17, 2020 • edited

abhinavdahiya commented Mar 18, 2020

fabianofranz commented Mar 18, 2020

fabianofranz commented Mar 23, 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

mbfrahry 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

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

fabianofranz commented Apr 4, 2020

mbfrahry commented Apr 7, 2020 • edited

fabianofranz commented Apr 14, 2020

mbfrahry 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

fabianofranz commented Apr 15, 2020

mbfrahry commented Apr 17, 2020

abhinavdahiya commented Jul 7, 2020

mbfrahry commented Jul 10, 2020

ghost commented Aug 10, 2020

fabianofranz commented Mar 17, 2020 •

edited

mbfrahry commented Apr 7, 2020 •

edited