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

azurerm_network_interface - Support auxiliary_mode, auxiliary_sku #22979

Merged
merged 5 commits into from
Sep 18, 2023
Merged

azurerm_network_interface - Support auxiliary_mode, auxiliary_sku #22979

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c5b5d13
add code
ms-zhenhua Jul 26, 2023
decfbcb
resolve comments 0817
ms-zhenhua Aug 17, 2023
3d23252
resolve comments 0818
ms-zhenhua Aug 18, 2023
401ed5a
resolve comments 0906
ms-zhenhua Sep 6, 2023
e553b9b
resolve comments 0914
ms-zhenhua Sep 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions internal/services/network/network_interface_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,32 @@ func resourceNetworkInterface() *pluginsdk.Resource {
},

// Optional
"auxiliary_mode": {
Type: pluginsdk.TypeString,
Optional: true,
ValidateFunc: validation.StringInSlice([]string{
string(networkinterfaces.NetworkInterfaceAuxiliaryModeAcceleratedConnections),
string(networkinterfaces.NetworkInterfaceAuxiliaryModeFloating),
Copy link
Contributor

Choose a reason for hiding this comment

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

we can use possiblevaluesfor here fwiw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

code updated. Thanks.

string(networkinterfaces.NetworkInterfaceAuxiliaryModeNone),
}, false),
Default: string(networkinterfaces.NetworkInterfaceAuxiliaryModeNone),
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need to expose None here, since this is already available in the provider by omitting the value for this field (null) - so can we remove this:

Suggested change
string(networkinterfaces.NetworkInterfaceAuxiliaryModeNone),
}, false),
Default: string(networkinterfaces.NetworkInterfaceAuxiliaryModeNone),
}, false),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

code updated

RequiredWith: []string{"auxiliary_sku"},
},

"auxiliary_sku": {
Type: pluginsdk.TypeString,
Optional: true,
ValidateFunc: validation.StringInSlice([]string{
string(networkinterfaces.NetworkInterfaceAuxiliarySkuAEight),
string(networkinterfaces.NetworkInterfaceAuxiliarySkuAFour),
string(networkinterfaces.NetworkInterfaceAuxiliarySkuAOne),
string(networkinterfaces.NetworkInterfaceAuxiliarySkuATwo),
string(networkinterfaces.NetworkInterfaceAuxiliarySkuNone),
}, false),
Default: string(networkinterfaces.NetworkInterfaceAuxiliarySkuNone),
Copy link
Contributor

Choose a reason for hiding this comment

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

(same here)

Suggested change
string(networkinterfaces.NetworkInterfaceAuxiliarySkuNone),
}, false),
Default: string(networkinterfaces.NetworkInterfaceAuxiliarySkuNone),
}, false),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

code updated

RequiredWith: []string{"auxiliary_mode"},
},

"dns_servers": {
Type: pluginsdk.TypeList,
Optional: true,
Expand Down Expand Up @@ -232,6 +258,14 @@ func resourceNetworkInterfaceCreate(d *pluginsdk.ResourceData, meta interface{})
locks.ByName(id.NetworkInterfaceName, networkInterfaceResourceName)
defer locks.UnlockByName(id.NetworkInterfaceName, networkInterfaceResourceName)

if auxiliaryMode, hasAuxiliaryMode := d.GetOk("auxiliary_mode"); hasAuxiliaryMode {
properties.AuxiliaryMode = pointer.To(networkinterfaces.NetworkInterfaceAuxiliaryMode(auxiliaryMode.(string)))
}

if auxiliarySku, hasAuxiliarySku := d.GetOk("auxiliary_sku"); hasAuxiliarySku {
properties.AuxiliarySku = pointer.To(networkinterfaces.NetworkInterfaceAuxiliarySku(auxiliarySku.(string)))
}

dns, hasDns := d.GetOk("dns_servers")
nameLabel, hasNameLabel := d.GetOk("internal_dns_name_label")
if hasDns || hasNameLabel {
Expand Down Expand Up @@ -325,6 +359,22 @@ func resourceNetworkInterfaceUpdate(d *pluginsdk.ResourceData, meta interface{})
},
}

if d.HasChange("auxiliary_mode") {
if auxiliaryMode, hasAuxiliaryMode := d.GetOk("auxiliary_mode"); hasAuxiliaryMode {
update.Properties.AuxiliaryMode = pointer.To(networkinterfaces.NetworkInterfaceAuxiliaryMode(auxiliaryMode.(string)))
}
} else {
update.Properties.AuxiliaryMode = existing.Model.Properties.AuxiliaryMode
}
Comment on lines +350 to +356
Copy link
Contributor

Choose a reason for hiding this comment

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

how do users unset this property?

Copy link
Contributor Author

@ms-zhenhua ms-zhenhua Sep 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If users remove this property from the .tf, the code will enter HasChange branch. Since hasAuxiliaryMode = false, update.Properties.AuxiliaryMode will not be assigned a value and use the default value nil, then Azure will unset this property.


if d.HasChange("auxiliary_sku") {
if auxiliarySku, hasAuxiliarySku := d.GetOk("auxiliary_sku"); hasAuxiliarySku {
update.Properties.AuxiliarySku = pointer.To(networkinterfaces.NetworkInterfaceAuxiliarySku(auxiliarySku.(string)))
}
} else {
update.Properties.AuxiliarySku = existing.Model.Properties.AuxiliarySku
}
tombuildsstuff marked this conversation as resolved.
Show resolved Hide resolved

if d.HasChange("dns_servers") {
dnsServersRaw := d.Get("dns_servers").([]interface{})
dnsServers := expandNetworkInterfaceDnsServers(dnsServersRaw)
Expand Down Expand Up @@ -462,6 +512,8 @@ func resourceNetworkInterfaceRead(d *pluginsdk.ResourceData, meta interface{}) e
return fmt.Errorf("setting `applied_dns_servers`: %+v", err)
}

d.Set("auxiliary_mode", pointer.From(props.AuxiliaryMode))
d.Set("auxiliary_sku", pointer.From(props.AuxiliarySku))
Copy link
Contributor

Choose a reason for hiding this comment

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

these'll both need to be normalized on the way back, since this won't be returned for older NICs - therefore None ("") should be the default value, and we should only be overriding that if the value is != nil && !strings.EqualFold(*value, "None"))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

code updated

d.Set("enable_ip_forwarding", props.EnableIPForwarding)
d.Set("enable_accelerated_networking", props.EnableAcceleratedNetworking)
d.Set("internal_dns_name_label", internalDnsNameLabel)
Expand Down
65 changes: 65 additions & 0 deletions internal/services/network/network_interface_resource_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,34 @@ func TestAccNetworkInterface_disappears(t *testing.T) {
})
}

func TestAccNetworkInterface_auxiliary(t *testing.T) {
data := acceptance.BuildTestData(t, "azurerm_network_interface", "test")
r := NetworkInterfaceResource{}
data.ResourceTest(t, r, []acceptance.TestStep{
{
Config: r.auxiliary(data, "", ""),
Check: acceptance.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
),
},
data.ImportStep(),
{
Config: r.auxiliary(data, "AcceleratedConnections", "A2"),
Check: acceptance.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
),
},
data.ImportStep(),
{
Config: r.auxiliary(data, "", ""),
Check: acceptance.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
),
},
data.ImportStep(),
})
}

func TestAccNetworkInterface_dnsServers(t *testing.T) {
data := acceptance.BuildTestData(t, "azurerm_network_interface", "test")
r := NetworkInterfaceResource{}
Expand Down Expand Up @@ -388,6 +416,43 @@ resource "azurerm_network_interface" "test" {
`, r.template(data), data.RandomInteger)
}

func (r NetworkInterfaceResource) auxiliary(data acceptance.TestData, mode string, sku string) string {
// Auxiliary Mode Nic is enabled in specific regions (https://learn.microsoft.com/en-us/azure/networking/nva-accelerated-connections#supported-regions) for now
// To not affect other testcases of `Network`, hard-code to that for now
data.Locations.Primary = "westus"

if mode != "" {
mode = fmt.Sprintf(`auxiliary_mode = "%s"`, mode)
}

if sku != "" {
sku = fmt.Sprintf(`auxiliary_sku = "%s"`, sku)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

it'd be easier/clearer to use separate test templates here - so can we update this to be one test template per type?

In addition, since these two fields must be specified together, this conditional logic would lead to an invalid configuration - another reason to use a separate test template here to make that clearer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

code updated


return fmt.Sprintf(`
%s

resource "azurerm_network_interface" "test" {
name = "acctestni-%d"
location = "%s"
resource_group_name = azurerm_resource_group.test.name
%s
%s
enable_accelerated_networking = true

ip_configuration {
name = "primary"
subnet_id = azurerm_subnet.test.id
private_ip_address_allocation = "Dynamic"
}

tags = {
fastpathenabled = "true"
tombuildsstuff marked this conversation as resolved.
Show resolved Hide resolved
}
}
`, r.template(data), data.RandomInteger, data.Locations.Primary, mode, sku)
}

func (r NetworkInterfaceResource) withMultipleParameters(data acceptance.TestData) string {
return fmt.Sprintf(`
%s
Expand Down
6 changes: 6 additions & 0 deletions website/docs/r/network_interface.html.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ The following arguments are supported:

---

* `auxiliary_mode` - (Optional) The Auxiliary mode of the Network Interface. Possible values are `AcceleratedConnections`, `Floating` and `None`. Defaults to `None`.
Copy link
Contributor

Choose a reason for hiding this comment

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

firstly:

Suggested change
* `auxiliary_mode` - (Optional) The Auxiliary mode of the Network Interface. Possible values are `AcceleratedConnections`, `Floating` and `None`. Defaults to `None`.
* `auxiliary_mode` - (Optional) The Auxiliary mode of the Network Interface. Possible values are `AcceleratedConnections` and `Floating`.

secondly, what does this do? this description isn't clear/descriptive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

firstly: doc updated.
secondly: it is used for tuning the performance of network connections. I have updated the doc


-> **Note:** This field requires the preview feature is enabled. See the [Prerequisites](https://learn.microsoft.com/en-us/azure/networking/nva-accelerated-connections#prerequisites) for more details.

* `auxiliary_sku` - (Optional) The Auxiliary SKU of the Network Interface. Possible values are `A1`, `A2`, `A4`, `A8` and `None`. Defaults to `None`.
Copy link
Contributor

Choose a reason for hiding this comment

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

firstly: does this require the preview feature too?

secondly:

Suggested change
* `auxiliary_sku` - (Optional) The Auxiliary SKU of the Network Interface. Possible values are `A1`, `A2`, `A4`, `A8` and `None`. Defaults to `None`.
* `auxiliary_sku` - (Optional) The Auxiliary SKU of the Network Interface. Possible values are `A1`, `A2`, `A4`, `A8`.

thirdly: what does this do? this description isn't clear/descriptive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

firstly: Yes, I updated the doc for both of them
secondly: doc updated.
thirdly: It is used for enabling Accelerated Connections to improve networking performance. I have updated the doc.


* `dns_servers` - (Optional) A list of IP Addresses defining the DNS Servers which should be used for this Network Interface.

-> **Note:** Configuring DNS Servers on the Network Interface will override the DNS Servers defined on the Virtual Network.
Expand Down