New Resource: azurerm_cloud_hardware_security_module_cluster#30293
New Resource: azurerm_cloud_hardware_security_module_cluster#30293wuxu92 wants to merge 28 commits intohashicorp:mainfrom
azurerm_cloud_hardware_security_module_cluster#30293Conversation
ms-zhenhua
left a comment
There was a problem hiding this comment.
Hi @wuxu92 ,
Thanks for the PR. I've taken a look through and left some comments inline. If we can fix those up, this should be good to continue 👍
internal/services/hsm/cloud_hardware_security_module_cluster_resource.go
Outdated
Show resolved
Hide resolved
internal/services/hsm/cloud_hardware_security_module_cluster_resource.go
Outdated
Show resolved
Hide resolved
internal/services/hsm/cloud_hardware_security_module_cluster_resource_test.go
Outdated
Show resolved
Hide resolved
| provider "azurerm" { | ||
| features {} | ||
| } |
There was a problem hiding this comment.
The provider block should be put in each testcase.
There was a problem hiding this comment.
sorry, I didn't get this comment, this is in a template method so it applies to each testcase, what change is required here?
There was a problem hiding this comment.
Use this as an example, could you put provider definition in each testcase of basic, complete, update...?
There was a problem hiding this comment.
Is this kind of new guideline? I see there are lots of tests put the provider definition in the template instead of each case.
There was a problem hiding this comment.
It's not new, you may refer to the example of the guidance.
internal/services/hsm/validate/cloud_hardware_security_module_name_valite.go
Show resolved
Hide resolved
internal/services/hsm/cloud_hardware_security_module_cluster_resource.go
Show resolved
Hide resolved
internal/services/hsm/cloud_hardware_security_module_cluster_resource.go
Outdated
Show resolved
Hide resolved
internal/services/hsm/cloud_hardware_security_module_cluster_resource.go
Show resolved
Hide resolved
internal/services/hsm/cloud_hardware_security_module_cluster_resource.go
Outdated
Show resolved
Hide resolved
internal/services/hsm/cloud_hardware_security_module_cluster_resource.go
Outdated
Show resolved
Hide resolved
| provider "azurerm" { | ||
| features {} | ||
| } |
There was a problem hiding this comment.
Use this as an example, could you put provider definition in each testcase of basic, complete, update...?
internal/services/hsm/custompollers/cloud_hsm_cluster_state_poller.go
Outdated
Show resolved
Hide resolved
internal/services/hsm/validate/cloud_hardware_security_module_name_test.go
Outdated
Show resolved
Hide resolved
internal/services/hsm/cloud_hardware_security_module_cluster_resource.go
Outdated
Show resolved
Hide resolved
internal/services/hsm/cloud_hardware_security_module_cluster_resource_test.go
Outdated
Show resolved
Hide resolved
Co-authored-by: Zhenhua Hu <zhhu@microsoft.com>
magodo
left a comment
There was a problem hiding this comment.
Thanks for this PR!
I've taken a look through and left some comments inline, but this is mostly looking good to me 👍
internal/services/hsm/validate/cloud_hardware_security_module_name.go
Outdated
Show resolved
Hide resolved
| Computed: true, | ||
| }, | ||
|
|
||
| "hsms": { |
There was a problem hiding this comment.
Will there be more than one hsm's information returned?
There was a problem hiding this comment.
yes, there are 3 hsms by default, and the current API doesn't support customize. I'm renaming it to hsm.
There was a problem hiding this comment.
If there are 3, then we can keep it to be plural as in the flattening code you are still populating all of them?
There was a problem hiding this comment.
Oh, I mixed up the plural rule and have reverted back to using "hsms". I'm wondering if it's acceptable to use "hsms" here, or if I should use the full name "hardware_security_modules" instead of the abbreviation(there were already "hsm_xx" properties in the provider).
There was a problem hiding this comment.
I'm not sure, let's keep hsms first...
internal/services/hsm/cloud_hardware_security_module_cluster_resource.go
Outdated
Show resolved
Hide resolved
internal/services/hsm/cloud_hardware_security_module_cluster_resource.go
Outdated
Show resolved
Hide resolved
internal/services/hsm/cloud_hardware_security_module_cluster_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/hsm/cloud_hardware_security_module_cluster_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/hsm/cloud_hardware_security_module_cluster_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/hsm/cloud_hardware_security_module_cluster_resource_test.go
Outdated
Show resolved
Hide resolved
| resp, err := client.Delete(ctx, *id) | ||
| if err != nil { | ||
| if response.WasConflict(resp.HttpResponse) { | ||
| return pluginsdk.RetryableError(err) |
There was a problem hiding this comment.
How come this is a retryable error? A retry on a Cloud HSM whose PE is deleted beforehead will succeed?
There was a problem hiding this comment.
Yes a retry is required here, as the CloudHSM will under updating status when and a while after the deletion of the linked PE. so when run terraform destroy, the PE is deleted firstly then to delete the Cloud HSM, and the conflict error responded and a retry may succeed. this is confirmed by service team: cloud hsm should be in-progress for a max of 5 mins after deleting a private endpoint
There was a problem hiding this comment.
This also seems to be like a service issue. Did the service team mention they have any plan to improve this?
There was a problem hiding this comment.
Yes it's a service issue. I know other resources linked to a PE don't encounter such issue and HSM team is aware of it. I need to confirm with them again about the fix as they once indicated a way to improve the deletion in next version.

Community Note
Description
Documentation: https://learn.microsoft.com/en-us/azure/cloud-hsm/
The current version only support fixed Sku, so this resource intentionally doesn't expose SKU properties. it also only support useridentity while the swagger definition is SystemAndUserAssignedIdentity, and PublicNetworkAccess only support Disabled, so does not expose either.
PR Checklist
For example: “
resource_name_here- description of change e.g. adding propertynew_property_name_here”Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_cloud_hardware_security_module_clusterThis is a (please select all that apply):
Rollback Plan
If a change needs to be reverted, we will publish an updated version of the provider.
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
Note
If this PR changes meaningfully during the course of review please update the title and description as required.