-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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 Resources: Support create key for managed hardware security modules #20930
Conversation
@@ -62,7 +63,7 @@ func resourceKeyVaultKey() *pluginsdk.Resource { | |||
Type: pluginsdk.TypeString, | |||
Required: true, | |||
ForceNew: true, | |||
ValidateFunc: keyVaultValidate.VaultID, | |||
ValidateFunc: validation.Any(keyVaultValidate.VaultID, keyVaultValidate.ManagedHSMID), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately as discussed in #20150 - we can't shoe-horn this into the existing resource since the two API's are already diverging, see: https://github.com/Azure/azure-sdk-for-go/pull/20296/files/a5b8068678e35dfe5e4941294897335e88cf3b8e#diff-14f54f17b4ccdfeb282de9756ca15ec92e75d473d8b583a1bc54439d62ddc6b7R3
Instead as mentioned in #20150, we'll need to introduce a new resource specifically for a Managed HSM Key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only the RBAC API for Managed HSM is diverged from keyvault if my understanding right? according to the #uses-same-api-and-management-interfaces-as-key-vault, the key management API is the same.
it makes sense to use a separate resource type (but with the same under client instance) to manage keys for Managed HSM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That may be true for some of the API's now, however per the above comment this is already beginning to diverge - as such we should make these separate resources, even if that means using the same API behind the scenes - else we'll have problems later and need to introduce a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for your explanation! i wonder if I should just register a new resource type with the existing key_vault_key_source.go
, or register a new resource with copy-then-modify the key_vault_key resource.
both implements should not introduce breaking change in the future even if the API diverged eventually. What do you think about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copying the existing resource but updating it to be specific to Managed HSM should be fine - although I'd suggest doing that in a separate PR since this one's also adding the role definition/assignment resources?
Close for splitting to different PRs |
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. |
To support creating keys for Manged HSM, we have to activate the HSM first which is supported in #20855 . and then this PR can create keys. resolves #13654.
And we need local RBAC for HSM as described here managed-hsm/role-management. this PR introcued two terraform resources to support role management:
azurerm_key_vault_role_definition
andazurerm_key_vault_role_assignment
. this also resolves #20112.The PR reuse the existing
azurerm_key_vaule_key
resource to create keys for Managed HSM with a bunch of changes to make keyvaultkey CURD methods support Managed HSM.