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

new resource azurerm_elastic_san_volume_group #24166

Merged
merged 9 commits into from
Jan 23, 2024

Conversation

teowa
Copy link
Contributor

@teowa teowa commented Dec 8, 2023

@teowa
Copy link
Contributor Author

teowa commented Dec 19, 2023

image

Copy link
Contributor

@ms-zhenhua ms-zhenhua left a comment

Choose a reason for hiding this comment

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

Hey @teowa,
Thanks for this PR - I've taken a look through and left some comments inline. If we can fix those up, this should be good to go 👍

Thanks!

Comment on lines 72 to 77
"elastic_san_id": {
Type: pluginsdk.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: volumegroups.ValidateElasticSanID,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"elastic_san_id": {
Type: pluginsdk.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: volumegroups.ValidateElasticSanID,
},
"san_id": commonschema.ResourceIDReferenceForceNew(volumegroups.ElasticSanID{}),


* `elastic_san_id` - (Required) Specifies the Elastic SAN ID within which this Elastic SAN Volume Group should exist. Changing this forces a new resource to be created.

* `encryption_type` - (Optional) Type of encryption. Possible values are `EncryptionAtRestWithCustomerManagedKey` and `EncryptionAtRestWithPlatformKey`. Defaults to `EncryptionAtRestWithPlatformKey`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `encryption_type` - (Optional) Type of encryption. Possible values are `EncryptionAtRestWithCustomerManagedKey` and `EncryptionAtRestWithPlatformKey`. Defaults to `EncryptionAtRestWithPlatformKey`.
* `encryption_type` - (Optional) Specifies the type of the key used to encrypt the data of the disk. Possible values are `EncryptionAtRestWithCustomerManagedKey` and `EncryptionAtRestWithPlatformKey`. Defaults to `EncryptionAtRestWithPlatformKey`.

Comment on lines 92 to 101
"user_assigned_identity_id": {
Optional: true,
Type: pluginsdk.TypeString,
ValidateFunc: commonids.ValidateUserAssignedIdentityID,
},
"key_vault_key_id": {
Required: true,
Type: pluginsdk.TypeString,
ValidateFunc: keyVaultValidate.NestedItemIdWithOptionalVersion,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

put required ahead of optional

Suggested change
"user_assigned_identity_id": {
Optional: true,
Type: pluginsdk.TypeString,
ValidateFunc: commonids.ValidateUserAssignedIdentityID,
},
"key_vault_key_id": {
Required: true,
Type: pluginsdk.TypeString,
ValidateFunc: keyVaultValidate.NestedItemIdWithOptionalVersion,
},
"key_vault_key_id": {
Required: true,
Type: pluginsdk.TypeString,
ValidateFunc: keyVaultValidate.NestedItemIdWithOptionalVersion,
},
"user_assigned_identity_id": {
Optional: true,
Type: pluginsdk.TypeString,
ValidateFunc: commonids.ValidateUserAssignedIdentityID,
},


An `identity` block exports the following arguments:

* `principal_id` - The Principal ID for the System-Assigned Managed Identity assigned to this Elastic SAN Volume Group.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `principal_id` - The Principal ID for the System-Assigned Managed Identity assigned to this Elastic SAN Volume Group.
* `principal_id` - The Principal ID associated with the Managed Service Identity assigned to this Elastic SAN Volume Group.


* `principal_id` - The Principal ID for the System-Assigned Managed Identity assigned to this Elastic SAN Volume Group.

* `tenant_id` - The Tenant ID for the System-Assigned Managed Identity assigned to this Elastic SAN Volume Group.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `tenant_id` - The Tenant ID for the System-Assigned Managed Identity assigned to this Elastic SAN Volume Group.
* `tenant_id` - The Tenant ID associated with this Managed Service Identity assigned to this Elastic SAN Volume Group.


* `current_versioned_key_expiration_timestamp` - Current Versioned Key Expiration Timestamp of the Elastic SAN Volume Group.

* `current_versioned_key_id` - Current Versioned Key ID.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `current_versioned_key_id` - Current Versioned Key ID.
* `current_versioned_key_id` - The ID of the current versioned Key Vault Key in use.


* `current_versioned_key_id` - Current Versioned Key ID.

* `last_key_rotation_timestamp` - Last Key Rotation Timestamp.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `last_key_rotation_timestamp` - Last Key Rotation Timestamp.
* `last_key_rotation_timestamp` - The timestamp of the last rotation of the Key Vault Key.


An `encryption` block exports the following arguments:

* `current_versioned_key_expiration_timestamp` - Current Versioned Key Expiration Timestamp of the Elastic SAN Volume Group.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `current_versioned_key_expiration_timestamp` - Current Versioned Key Expiration Timestamp of the Elastic SAN Volume Group.
* `current_versioned_key_expiration_timestamp` - The timestamp of the expiration time for the current version of the customer managed key.

Comment on lines 72 to 77
"san_id": {
Type: pluginsdk.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: volumegroups.ValidateElasticSanID,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"san_id": {
Type: pluginsdk.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: volumegroups.ValidateElasticSanID,
},
"san_id": commonschema.ResourceIDReferenceForceNew(volumegroups.ElasticSanID{}),

@teowa
Copy link
Contributor Author

teowa commented Dec 27, 2023

image

@@ -140,7 +135,7 @@ func (r ElasticSANVolumeGroupResource) Arguments() map[string]*pluginsdk.Schema
Type: pluginsdk.TypeString,
Optional: true,
ValidateFunc: validation.StringInSlice([]string{
// None is not exposed
// None is not exposed, Iscsi is only allowed protocolType to connect to volume group
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// None is not exposed, Iscsi is only allowed protocolType to connect to volume group
// None is not a valid value and service team will consider removing it in future versions.

@@ -139,7 +139,7 @@ The following arguments are supported:

* `network_rule` - (Optional) One or more `network_rule` blocks as defined below.

* `protocol_type` - (Optional) Specifies the type of the storage target. The only possible value is `Iscsi`. Defaults to `Iscsi`.
* `protocol_type` - (Optional) Specifies the type of the storage target. The only possible values are `Iscsi`. Defaults to `Iscsi`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `protocol_type` - (Optional) Specifies the type of the storage target. The only possible values are `Iscsi`. Defaults to `Iscsi`.
* `protocol_type` - (Optional) Specifies the type of the storage target. The only possible value is `Iscsi`. Defaults to `Iscsi`.

@teowa teowa marked this pull request as ready for review December 28, 2023 05:44
return []ElasticSANVolumeGroupResourceNetworkRuleModel{}
}

var networkRules []ElasticSANVolumeGroupResourceNetworkRuleModel
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var networkRules []ElasticSANVolumeGroupResourceNetworkRuleModel
networkRules := make([]ElasticSANVolumeGroupResourceNetworkRuleModel,0)

}

func (r ElasticSANVolumeGroupTestResource) template(data acceptance.TestData) string {
// some of the features are supported in limited regions, see https://learn.microsoft.com/en-us/azure/storage/elastic-san/elastic-san-networking-concepts#regional-availability
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// some of the features are supported in limited regions, see https://learn.microsoft.com/en-us/azure/storage/elastic-san/elastic-san-networking-concepts#regional-availability
// some of the features are supported in limited regions, see https://learn.microsoft.com/azure/storage/elastic-san/elastic-san-networking-concepts#regional-availability

ValidateFunc: validate.ElasticSanVolumnGroupName,
},

"san_id": commonschema.ResourceIDReferenceRequiredForceNew(volumegroups.ElasticSanId{}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"san_id": commonschema.ResourceIDReferenceRequiredForceNew(volumegroups.ElasticSanId{}),
"elastic_san_id": commonschema.ResourceIDReferenceRequiredForceNew(volumegroups.ElasticSanId{}),

KeyVaultKeyId: keyVaultKeyId,
UserAssignedIdentityId: userAssignedIdentityId,
CurrentVersionedKeyExpirationTimestamp: pointer.From(input.KeyVaultProperties.CurrentVersionedKeyExpirationTimestamp),
CurrentVersionedKeyId: pointer.From(input.KeyVaultProperties.CurrentVersionedKeyIdentifier),
Copy link
Contributor

Choose a reason for hiding this comment

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

what if input.KeyVaultProperties == nil ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nil check added.


func ExpandVolumeGroupEncryption(input []ElasticSANVolumeGroupResourceEncryptionModel) (*volumegroups.EncryptionProperties, error) {
if len(input) == 0 {
return nil, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

according to the protocol(https://www.rfc-editor.org/rfc/rfc7386.html#page-3), nil properties will be ignored by PATCH which means Encryption cannot be removed. Why TestAccElasticSANVolumeGroup_update can pass?

Copy link
Contributor Author

@teowa teowa Jan 17, 2024

Choose a reason for hiding this comment

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

By my test, if encryptionType is updated from EncryptionAtRestWithCustomerManagedKey to EncryptionAtRestWithPlatformKey, then encryption block will be ignored at backend. In customDiff, we have limited the encryption block can only set with encryptionType=EncryptionAtRestWithCustomerManagedKey

"regexp"
)

func ElasticSanVolumnGroupName(i interface{}, k string) (warnings []string, errors []error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func ElasticSanVolumnGroupName(i interface{}, k string) (warnings []string, errors []error) {
func ElasticSanVolumeGroupName(i interface{}, k string) (warnings []string, errors []error) {


* `name` - (Required) Specifies the name of this Elastic SAN Volume Group. Changing this forces a new resource to be created.

* `san_id` - (Required) Specifies the Elastic SAN ID within which this Elastic SAN Volume Group should exist. Changing this forces a new resource to be created.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `san_id` - (Required) Specifies the Elastic SAN ID within which this Elastic SAN Volume Group should exist. Changing this forces a new resource to be created.
* `elastic_san_id` - (Required) Specifies the Elastic SAN ID within which this Elastic SAN Volume Group should exist. Changing this forces a new resource to be created.


* `encryption` - (Optional) An `encryption` block as defined below.

**NOTE:** The `encryption` block can only be set when `encryption_type` is set to `EncryptionAtRestWithCustomerManagedKey`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
**NOTE:** The `encryption` block can only be set when `encryption_type` is set to `EncryptionAtRestWithCustomerManagedKey`.
-> **NOTE:** The `encryption` block can only be set when `encryption_type` is set to `EncryptionAtRestWithCustomerManagedKey`.


var keyVaultKeyId string
if kv := input.KeyVaultProperties; kv != nil {
id, err := keyVaultParse.NewNestedItemID(pointer.From(kv.KeyVaultUri), keyVaultParse.NestedItemTypeKey, pointer.From(kv.KeyName), pointer.From(kv.KeyVersion))
Copy link
Contributor

Choose a reason for hiding this comment

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

if the key was rotated, which value will be returned by the service end for keyVaultKeyId? The original configured key? Or current versioned key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From doc, If key_vault_id has no version, the backend will periodically retrieve the latest version, if version specified, we need to manually update version if key is rotated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it mean no matter KeyVersion is set or not, the service end will not change its value in Get API if the keyvault was rotated before? Could you confirm it with the service team or by your test?

Copy link
Contributor Author

@teowa teowa Jan 17, 2024

Choose a reason for hiding this comment

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

Yes, by my test, KeyVersion will not be changed, the versioned Key ID in use is exported through currentVersionedKeyId field of REST API.

return nil, err
}

result := volumegroups.EncryptionProperties{
Copy link
Contributor

Choose a reason for hiding this comment

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

if the key was rotated and we still take the original configured value to the service end, how will the service end handle it? Will the service end use the configured key to replace the current versioned key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If version is specified, specific versioned key will be used;
if version is not specified,

Elastic SAN checks the key vault daily for a new version of the key.

@teowa
Copy link
Contributor Author

teowa commented Jan 17, 2024

image

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 @teowa and @ms-zhenhua! LGTM 🦀

@stephybun stephybun merged commit 377692e into hashicorp:main Jan 23, 2024
38 checks passed
@github-actions github-actions bot added this to the v3.89.0 milestone Jan 23, 2024
Copy link

This functionality has been released in v3.89.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

dduportal added a commit to jenkins-infra/azure that referenced this pull request Jan 25, 2024
<Actions>
<action
id="f410411e63aff4bb73a81c2aec1d373cf8a903e63b30dee2006b0030d8a94cc8">
        <h3>Bump Terraform `azurerm` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
            <summary>Update Terraform lock file</summary>
<p>changes detected:&#xA;&#x9;&#34;hashicorp/azurerm&#34; updated from
&#34;3.88.0&#34; to &#34;3.89.0&#34; in file
&#34;.terraform.lock.hcl&#34;</p>
            <details>
                <summary>3.89.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.89.0&#xA;FEATURES:&#xA;&#xA;*
New Data Source: `azurerm_data_factory_trigger_schedule`
([#24572](hashicorp/terraform-provider-azurerm#24572
New Data Source: `azurerm_data_factory_trigger_schedules`
([#24572](hashicorp/terraform-provider-azurerm#24572
New Data Source: `azurerm_ip_groups`
([#24540](hashicorp/terraform-provider-azurerm#24540
New Data Source: `azurerm_nginx_certificate`
([#24577](hashicorp/terraform-provider-azurerm#24577
New Resource: `azurerm_chaos_studio_target`
([#24580](hashicorp/terraform-provider-azurerm#24580
New Resource: `azurerm_elastic_san_volume_group`
([#24166](hashicorp/terraform-provider-azurerm#24166
New Resource: `azurerm_netapp_account_encryption`
([#23733](hashicorp/terraform-provider-azurerm#23733
New Resource: `azurerm_redhat_openshift_cluster`
([#24375](https://github.com/hashicorp/terraform-provider-azurerm/issues/24375))&#xA;&#xA;ENHANCEMENTS:&#xA;&#xA;*
dependencies: updating to `v0.66.1` of
`github.com/hashicorp/go-azure-helpers`
([#24561](hashicorp/terraform-provider-azurerm#24561
dependencies: updating to `v0.20240124.1115501` of
`github.com/hashicorp/go-azure-sdk`
([#24619](hashicorp/terraform-provider-azurerm#24619
`bot`: updating to API Version `2021-05-01-preview`
([#24555](hashicorp/terraform-provider-azurerm#24555
`containerservice`: the SDK Clients now support logging
([#24564](hashicorp/terraform-provider-azurerm#24564
`cosmosdb`: updating to API Version `2023-04-15`
([#24541](hashicorp/terraform-provider-azurerm#24541
`loadtestservice`: updating to use the base layer from
`hashicorp/go-azure-sdk` rather than `Azure/go-autorest` (and support
logging)
([#24578](hashicorp/terraform-provider-azurerm#24578
`managedidentity`: updating to use the base layer from
`hashicorp/go-azure-sdk` rather than `Azure/go-autorest` (and support
logging)
([#24578](hashicorp/terraform-provider-azurerm#24578
`azurerm_api_management_api` - change the `id` format so specific
`revision`s can be managed by Terraform
([#23031](hashicorp/terraform-provider-azurerm#23031
`azurerm_data_protection_backup_vault` - the `redundancy` propety can
now be set to `ZoneRedundant`
([#24556](hashicorp/terraform-provider-azurerm#24556
`azurerm_data_factory_integration_runtime_azure_ssis` - support for the
`credential_name` property
([#24458](hashicorp/terraform-provider-azurerm#24458
`azurerm_orchestrated_virtual_machine_scale_set` - support
`2022-datacenter-azure-edition-hotpatch` and
`2022-datacenter-azure-edition-hotpatch-smalldisk` hotpatching images
([#23500](hashicorp/terraform-provider-azurerm#23500
`azurerm_stream_analytics_job` - support for the `sku_name` property
([#24554](https://github.com/hashicorp/terraform-provider-azurerm/issues/24554))&#xA;&#xA;BUG
FIXES:&#xA;&#xA;* Data Source: `azurerm_app_service` - parsing the API
Response for `app_service_plan_id` case-insensitively
([#24626](hashicorp/terraform-provider-azurerm#24626
Data Source: `azurerm_function_app` - parsing the API Response for
`app_service_plan_id` case-insensitively
([#24626](hashicorp/terraform-provider-azurerm#24626
`azurerm_app_configuration_key` - the value for the `value` property can
now be removed/emptied
([#24582](https://github.com/hashicorp/terraform-provider-azurerm/issues/24582))&#xA;&#xA;*
`azurerm_app_service` - parsing the API Response for
`app_service_plan_id` case-insensitively
([#24626](hashicorp/terraform-provider-azurerm#24626
`azurerm_app_service_plan` - fix casing in `serverFarms` due to ID
update
([#24562](hashicorp/terraform-provider-azurerm#24562
`azurerm_app_service_slot` - parsing the API Response for
`app_service_plan_id` case-insensitively
([#24626](hashicorp/terraform-provider-azurerm#24626
`azurerm_automation_schedule` - only one `monthly_occurence` block can
now be specified
([#24614](hashicorp/terraform-provider-azurerm#24614
`azurerm_cognitive_deployment` - the `model.version` property is no
longer required
([#24264](hashicorp/terraform-provider-azurerm#24264
`azurerm_container_app` - multiple `custom_scale_rule` can not be
updated
([#24509](hashicorp/terraform-provider-azurerm#24509
`azurerm_container_registry_task_schedule_run_now` - prevent issue where
the incorrect scheduled run in tracked if there have been multiple
([#24592](hashicorp/terraform-provider-azurerm#24592
`azurerm_function_app` - parsing the API Response for
`app_service_plan_id` case-insensitively
([#24626](hashicorp/terraform-provider-azurerm#24626
`azurerm_function_app_slot` - parsing the API Response for
`app_service_plan_id` case-insensitively
([#24626](hashicorp/terraform-provider-azurerm#24626
`azurerm_logic_app_standard` - now will parse the app service ID
insensitively
([#24562](hashicorp/terraform-provider-azurerm#24562
`azurerm_logic_app_workflow` - the `workflow_parameters` will now
correctly handle information specified by `$connections`
([#24141](hashicorp/terraform-provider-azurerm#24141
`azurerm_mssql_managed_instance_security_alert_policy` - can not update
empty storage attributes
([#24553](hashicorp/terraform-provider-azurerm#24553
`azurerm_network_interface` - the `ip_configuration` properties are no
longer added to a Load Balancer Backend if one of those
`ip_configurations` is associated with a backend
([#24470](https://github.com/hashicorp/terraform-provider-azurerm/issues/24470))&#xA;&#xA;&#xA;</pre>
            </details>
        </details>
<a
href="https://infra.ci.jenkins.io/job/terraform-jobs/job/azure/job/main/1052/">Jenkins
pipeline link</a>
    </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) <60776566+jenkins-infra-bot@users.noreply.github.com>
Co-authored-by: Damien Duportal <damien.duportal@gmail.com>
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 Apr 27, 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