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_network_interface - Support auxiliary_mode, auxiliary_sku #22979

Merged
merged 5 commits into from
Sep 18, 2023

Conversation

ms-zhenhua
Copy link
Contributor

@ms-zhenhua ms-zhenhua commented Aug 16, 2023

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hi @ms-zhenhua

I've taken a look through and left some comments inline, however many of the comments I've raised in this PR have been raised in previous PR's (for example the None issue, which can be found here) - as such would you mind fixing these up and paying particular attention to the None issue going forward?

Thanks!

Comment on lines 135 to 137
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

Comment on lines 149 to 151
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

Comment on lines 515 to 516
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

Comment on lines 424 to 430
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


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

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

@ms-zhenhua
Copy link
Contributor Author

Hi @tombuildsstuff,

Thank you for your review. Sorry for not noticing the latest shema design. I will pay more attention in future. I have updated all comments and rerun the tests. Kindly take another review.

Thanks.

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hi @ms-zhenhua

Thanks for pushing those changes - I've taken a look through and left some comments inline, however would it be possible to explain the fastpathenabled tag?

Thanks!

Comment on lines 142 to 147
ValidateFunc: validation.StringInSlice([]string{
string(networkinterfaces.NetworkInterfaceAuxiliarySkuAEight),
string(networkinterfaces.NetworkInterfaceAuxiliarySkuAFour),
string(networkinterfaces.NetworkInterfaceAuxiliarySkuAOne),
string(networkinterfaces.NetworkInterfaceAuxiliarySkuATwo),
}, false),
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:

Suggested change
ValidateFunc: validation.StringInSlice([]string{
string(networkinterfaces.NetworkInterfaceAuxiliarySkuAEight),
string(networkinterfaces.NetworkInterfaceAuxiliarySkuAFour),
string(networkinterfaces.NetworkInterfaceAuxiliarySkuAOne),
string(networkinterfaces.NetworkInterfaceAuxiliarySkuATwo),
}, false),
ValidateFunc: validation.StringInSlice(networkinterfaces.PossibleValuesForNetworkInterfaceAuxiliarySku(), 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. Thanks.

Comment on lines 133 to 134
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.

Comment on lines +358 to +364
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
}
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

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.

@ms-zhenhua
Copy link
Contributor Author

Hi @tombuildsstuff, thank you for your review. I have resolved the new comments and explained fastpathenabled tag. Could you kindly take another review? Thanks.

Test Result

=== RUN TestAccNetworkInterface_auxiliary
=== PAUSE TestAccNetworkInterface_auxiliary
=== CONT TestAccNetworkInterface_auxiliary
--- PASS: TestAccNetworkInterface_auxiliary (524.86s)
PASS

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

Left a few comments around the documentation, as we should have a succinct explanation of what this does without requiring clicking through to another site, but if we can fix this up then it should otherwise be good to go 👍

@@ -60,6 +60,12 @@ The following arguments are supported:

---

* `auxiliary_mode` - (Optional) Specifies auxiliary mode for enabling [Accelerated Connections](https://learn.microsoft.com/en-us/azure/networking/nva-accelerated-connections) to improve networking performance. Possible values are `AcceleratedConnections` and `Floating`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this description clearer to call out what this actually does / update the tone and style to match other fields too? We shouldn't need to link to a product page to explain what a value does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doc updated. Thanks.

@@ -60,6 +60,12 @@ The following arguments are supported:

---

* `auxiliary_mode` - (Optional) Specifies auxiliary mode for enabling [Accelerated Connections](https://learn.microsoft.com/en-us/azure/networking/nva-accelerated-connections) to improve networking performance. Possible values are `AcceleratedConnections` and `Floating`.

* `auxiliary_sku` - (Optional) Specifies auxiliary SKU for tuning the performance of network connections. Possible values are `A1`, `A2`, `A4` and `A8`.
Copy link
Contributor

Choose a reason for hiding this comment

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

(as above)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doc updated. Thanks.


* `auxiliary_sku` - (Optional) Specifies auxiliary SKU for tuning the performance of network connections. Possible values are `A1`, `A2`, `A4` and `A8`.

-> **Note:** `auxiliary_mode` and `auxiliary_sku` require the preview feature is enabled. See the [Prerequisites](https://learn.microsoft.com/en-us/azure/networking/nva-accelerated-connections#prerequisites) for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

The Preview note should be listed under every preview field, and something along the lines of:

Suggested change
-> **Note:** `auxiliary_mode` and `auxiliary_sku` require the preview feature is enabled. See the [Prerequisites](https://learn.microsoft.com/en-us/azure/networking/nva-accelerated-connections#prerequisites) for more details.
-> **Note:** `auxiliary_mode` is in **Preview** and requires that the preview is enabled - [more information can be found in the Azure documentation](https://learn.microsoft.com/azure/networking/nva-accelerated-connections#prerequisites).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doc updated. Thanks.

@ms-zhenhua
Copy link
Contributor Author

Hi @tombuildsstuff, thank you for another review. I have updated the doc. Kindly help have a check. Thanks.

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks @ms-zhenhua LGTM 💯

@stephybun stephybun merged commit 685f277 into hashicorp:main Sep 18, 2023
26 checks passed
@github-actions github-actions bot added this to the v3.74.0 milestone Sep 18, 2023
stephybun added a commit that referenced this pull request Sep 20, 2023
dduportal pushed a commit to jenkins-infra/azure that referenced this pull request Sep 23, 2023
<Actions>
<action
id="4a39167e811ac038e4a588362092472c27cfbe9e4929ae61d035f708a093a669">
        <h3>Bump Terraform `azurerm` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
            <summary>Update Terraform lock file</summary>
<p>&#34;hashicorp/azurerm&#34; updated from &#34;3.73.0&#34; to
&#34;3.74.0&#34; in file &#34;.terraform.lock.hcl&#34;</p>
            <details>
                <summary>3.74.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.74.0&#xA;NOTES:&#xA;&#xA;*
`azurerm_synapse_sql_pool` - users that have imported
`azurerm_synapse_sql_pool` resources that were created outside of
Terraform using an `LRS` storage account type will need to use
`ignore_changes` to avoid the resource from being destroyed and
recreated.&#xA;&#xA;FEATURES:&#xA;&#xA;* **New Resource**:
`azurerm_arc_resource_bridge_appliance`
([#23108](hashicorp/terraform-provider-azurerm#23108
**New Resource**: `azurerm_data_factory_dataset_azure_sql_table`
([#23264](hashicorp/terraform-provider-azurerm#23264
**New Resource**: `azurerm_function_app_connection`
([#23127](https://github.com/hashicorp/terraform-provider-azurerm/issues/23127))&#xA;&#xA;ENHANCEMENTS:&#xA;&#xA;*
dependencies: updating to `v0.20230918.1115907` of
`github.com/hashicorp/go-azure-sdk`
([#23337](hashicorp/terraform-provider-azurerm#23337
dependencies: downgrading to `v1.12.5` of `github.com/rickb777/date`
([#23296](hashicorp/terraform-provider-azurerm#23296
`mysql`: updating to use API Version `2022-01-01`
([#23320](hashicorp/terraform-provider-azurerm#23320
`azurerm_app_configuration` - support for the `replica` block
([#22452](hashicorp/terraform-provider-azurerm#22452
`azurerm_bot_channel_directline` - support for `user_upload_enabled`,
`endpoint_parameters_enabled`, and `storage_enabled`
([#23149](hashicorp/terraform-provider-azurerm#23149
`azurerm_container_app` - support for scale rules
([#23294](hashicorp/terraform-provider-azurerm#23294
`azurerm_container_app_environment` - support for zone redundancy
([#23313](hashicorp/terraform-provider-azurerm#23313
`azurerm_container_group` - support for the `key_vault_user_identity_id`
property for Customer Managed Keys
([#23332](hashicorp/terraform-provider-azurerm#23332
`azurerm_cosmosdb_account` - support for MongoDB connection strings
([#23331](hashicorp/terraform-provider-azurerm#23331
`azurerm_data_factory_dataset_delimited_text` - support for the
`dynamic_file_system_enabled`, `dynamic_path_enabled`, and
`dynamic_filename_enabled` properties
([#23261](hashicorp/terraform-provider-azurerm#23261
`azurerm_data_factory_dataset_parquet` - support for the
`azure_blob_fs_location` block
([#23261](hashicorp/terraform-provider-azurerm#23261
`azurerm_monitor_diagnostic_setting` - validation to ensure either
`category` or `category_group` are supplied in `enabled_log` and `log`
blocks
([#23308](hashicorp/terraform-provider-azurerm#23308
`azurerm_network_interface` - support for the `auxiliary_mode` and
`auxiliary_sku` properties
([#22979](hashicorp/terraform-provider-azurerm#22979
`azurerm_postgresql_flexible_server` - increased the maximum supported
value for `storage_mb`
([#23277](hashicorp/terraform-provider-azurerm#23277
`azurerm_shared_image_version` - support for the
`replicated_region_deletion_enabled` and
`target_region.exclude_from_latest_enabled` properties
([#23147](hashicorp/terraform-provider-azurerm#23147
`azurerm_storage_account` - support for setting `domain_name` and
`domain_guid` for `AADKERB`
([#22833](hashicorp/terraform-provider-azurerm#22833
`azurerm_storage_account_customer_managed_key` - support for
cross-tenant customer-managed keys with the
`federated_identity_client_id`, and `key_vault_uri` properties
([#20356](hashicorp/terraform-provider-azurerm#20356
`azurerm_web_application_firewall_policy` - support for the
`rate_limit_duration`, `rate_limit_threshold`, `group_rate_limit_by`,
and `request_body_inspect_limit_in_kb` properties
([#23239](https://github.com/hashicorp/terraform-provider-azurerm/issues/23239))&#xA;&#xA;BUG
FIXES:&#xA;&#xA;* Data Source: `azurerm_container_app_environment`: fix
`log_analytics_workspace_name` output to correct value
([#23298](hashicorp/terraform-provider-azurerm#23298
`azurerm_api_management_api` - set the `service_url` property when
importing the resource
([#23011](hashicorp/terraform-provider-azurerm#23011
`azurerm_app_configuration` - prevent crash by nil checking the
encryption configuration
([#23302](hashicorp/terraform-provider-azurerm#23302
`azurerm_app_configuration_feature` - update `percentage_filter_value`
to accept correct type of float
([#23263](hashicorp/terraform-provider-azurerm#23263
`azurerm_container_app` - fix an issue with `commands` and `args` being
overwritten when using multiple containers
([#23338](hashicorp/terraform-provider-azurerm#23338
`azurerm_key_vault_certificate` - fix issue where certificates
couldn&#39;t be recovered anymore
([#23204](hashicorp/terraform-provider-azurerm#23204
`azurerm_key_vault_key` - the ForceNew when `expiration_date` is removed
from the config file
([#23327](hashicorp/terraform-provider-azurerm#23327
`azurerm_linux_function_app` - fix a bug in setting the storage settings
when using Elastic Premium plans
([#21212](hashicorp/terraform-provider-azurerm#21212
`azurerm_linux_web_app` - fix docker app stack update
([#23303](hashicorp/terraform-provider-azurerm#23303
`azurerm_linux_web_app` - fix crash in auto heal expansion
([#21328](hashicorp/terraform-provider-azurerm#21328
`azurerm_linux_web_app_slot` - fix docker app stack update
([#23303](hashicorp/terraform-provider-azurerm#23303
`azurerm_linux_web_app_slot` - fix crash in auto heal expansion
([#21328](hashicorp/terraform-provider-azurerm#21328
`azurerm_log_analytics_solution` - fix bug where the resource wasn&#39;t
handling successful creation on subsequent applies
([#23312](hashicorp/terraform-provider-azurerm#23312
`azurerm_management_group_subscription_association` - fix bug to
correctly mark resource as gone if not found during read
([#23335](hashicorp/terraform-provider-azurerm#23335
`azurerm_mssql_elasticpool` - remove check that prevents `license_type`
from being set for certain skus
([#23262](hashicorp/terraform-provider-azurerm#23262
`azurerm_servicebus_queue` - fixing an issue where `auto_delete_on_idle`
couldn&#39;t be set to `P10675199DT2H48M5.4775807S`
([#23296](hashicorp/terraform-provider-azurerm#23296
`azurerm_servicebus_topic` - fixing an issue where `auto_delete_on_idle`
couldn&#39;t be set to `P10675199DT2H48M5.4775807S`
([#23296](hashicorp/terraform-provider-azurerm#23296
`azurerm_storage_account` - prevent sending unsupported blob properties
in payload for `Storage` account kind
([#23288](hashicorp/terraform-provider-azurerm#23288
`azurerm_synapse_sql_pool` - expose `storage_account_type`
([#23217](hashicorp/terraform-provider-azurerm#23217
`azurerm_windows_function_app` - fix a bug in setting the storage
settings when using Elastic Premium plans
([#21212](hashicorp/terraform-provider-azurerm#21212
`azurerm_windows_web_app` - fix docker app stack update
([#23303](hashicorp/terraform-provider-azurerm#23303
`azurerm_windows_web_app_slot` - fix docker app stack update
([#23303](https://github.com/hashicorp/terraform-provider-azurerm/issues/23303))&#xA;&#xA;DEPRECATIONS:&#xA;&#xA;*
`azurerm_application_gateway` - deprecate `Standard` and `WAF` skus
([#23310](hashicorp/terraform-provider-azurerm#23310
`azurerm_bot_channel_web_chat` - deprecate `site_names` in favour of
`site` block
([#23161](hashicorp/terraform-provider-azurerm#23161
`azurerm_monitor_diagnostic_setting` - deprecate `retention_policy` in
favour of `azurerm_storage_management_policy`
([#23260](https://github.com/hashicorp/terraform-provider-azurerm/issues/23260))&#xA;&#xA;&#xA;</pre>
            </details>
        </details>
    </action>
</Actions>

---

<table>
  <tr>
    <td width="77">
<img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli
logo" width="50" height="50">
    </td>
    <td>
      <p>
Created automatically by <a
href="https://www.updatecli.io/">Updatecli</a>
      </p>
      <details><summary>Options:</summary>
        <br />
<p>Most of Updatecli configuration is done via <a
href="https://www.updatecli.io/docs/prologue/quick-start/">its
manifest(s)</a>.</p>
        <ul>
<li>If you close this pull request, Updatecli will automatically reopen
it, the next time it runs.</li>
<li>If you close this pull request and delete the base branch, Updatecli
will automatically recreate it, erasing all previous commits made.</li>
        </ul>
        <p>
Feel free to report any issues at <a
href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br
/>
If you find this tool useful, do not hesitate to star <a
href="https://github.com/updatecli/updatecli/stargazers">our GitHub
repository</a> as a sign of appreciation, and/or to tell us directly on
our <a
href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>!
        </p>
      </details>
    </td>
  </tr>
</table>

Co-authored-by: Jenkins Infra Bot (updatecli) <[email protected]>
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 14, 2024
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.

3 participants