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

Jump to bottom

New Resource: `azurerm_key_vault_managed_hardware_security_module_key` #25069

Closed

wuxu92 wants to merge 10 commits into hashicorp:main from wuxu92:mhsm/keys

Contributor

wuxu92 commented Feb 28, 2024 •

edited

Loading

fixes: #13654.

--- PASS: TestAccKeyVaultManagedHardwareSecurityModule (2054.45s)
    --- PASS: TestAccKeyVaultManagedHardwareSecurityModule/resource (2054.45s)
        --- PASS: TestAccKeyVaultManagedHardwareSecurityModule/resource/key (2054.45s)
PASS


          add managed hsm key resource

2fa6680

github-actions bot added documentation service/managed-hsm size/XL waiting-response labels


          update acc test

f3e4db5

github-actions bot added waiting-response and removed waiting-response labels

tombuildsstuff reviewed

View reviewed changes

internal/services/managedhsm/parse/nested_item.go Outdated

Comment on lines 15 to 21

+              // NestedItemObjectType enumerates the type of the "NestedItemType" value (e.g."keys", "secrets" or "certificates").
+              type NestedItemObjectType string
+              const (
+              	// KeyVaultObjectType Keys...
+              	NestedItemTypeKey NestedItemObjectType = "keys"
+              )

Member

tombuildsstuff Feb 28, 2024

We're intending to move away from the NestedItemId in favour of Resource ID types specific to the type of Nested Item (e.g. a ManagedHSMKey etc) - as such can we update this Resource ID type to be one for each type?

Contributor Author

wuxu92 Feb 29, 2024

updated.

Member

tombuildsstuff Mar 12, 2024

This is still pending FWIW

Contributor Author

wuxu92 Mar 12, 2024

this ID is replaced with ManagedHSMKeyID in ./parse/managed_hsm_key_id.go. is there other updates needed to ressolve this comment?

Contributor Author

wuxu92 Mar 13, 2024

Should we use the KeyID of internal/services/managedhsm/parse/key.go as the resource ID instead of the NestedItemID format?


          update managed hsm key id instead of nested item id

151cc5a

github-actions bot added waiting-response and removed waiting-response labels

katbyte mentioned this pull request

azurerm_storage_account: Support managed HSMs for CMKs #25088

Merged


          fix parse hsm key id

e9b39fe

github-actions bot added waiting-response and removed waiting-response labels


          remove unused unit test

a564987

github-actions bot added waiting-response and removed waiting-response labels


          add exception for hsm keys read tiemout unit test

fdb34a1

github-actions bot removed the waiting-response label

katbyte requested changes

View reviewed changes

website/docs/r/key_vault_managed_hardware_security_module_key.html.markdown Outdated


		* `key_opts` - (Required) A list of JSON web key operations. Possible values are `decrypt`, `encrypt`, `import`, `export`, `sign`, `unwrapKey`, `verify` and `wrapKey`. Please note these values are case sensitive.

		* `not_before_date` - (Optional) Key not usable before the provided UTC datetime (Y-m-d'T'H:M:S'Z').

Collaborator

katbyte Mar 12, 2024

would this be better as

Suggested change

      
            * `not_before_date` - (Optional) Key not usable before the provided UTC datetime (Y-m-d'T'H:M:S'Z').
          
            * `not_usable_before_date` - (Optional) Key not usable before the provided UTC datetime (Y-m-d'T'H:M:S'Z').

website/docs/r/key_vault_managed_hardware_security_module_key.html.markdown Outdated


		* `curve` - (Optional) Specifies the curve to use when creating an `EC` key. Possible values are `P-256`, `P-256K`, `P-384`, and `P-521`. This field will be required in a future release if `key_type` is `EC-HSM`. The API will default to `P-256` if nothing is specified. Changing this forces a new resource to be created.

		* `key_opts` - (Required) A list of JSON web key operations. Possible values are `decrypt`, `encrypt`, `import`, `export`, `sign`, `unwrapKey`, `verify` and `wrapKey`. Please note these values are case sensitive.

Collaborator

katbyte Mar 12, 2024

Suggested change

      
            * `key_opts` - (Required) A list of JSON web key operations. Possible values are `decrypt`, `encrypt`, `import`, `export`, `sign`, `unwrapKey`, `verify` and `wrapKey`. Please note these values are case sensitive.
          
            * `key_options` - (Required) A list of JSON web key operations. Possible values are `decrypt`, `encrypt`, `import`, `export`, `sign`, `unwrapKey`, `verify` and `wrapKey`. Please note these values are case sensitive.

website/docs/r/key_vault_managed_hardware_security_module_key.html.markdown Outdated


		* `key_type` - (Required) Specifies the Key Type to use for this Managed Hardware Security Module Key. Possible values are `EC-HSM`, `RSA-HSM` and `oct-HSM`. Changing this forces a new resource to be created.

		* `key_size` - (Optional) Specifies the Size of the RSA key to create in bytes. For example, 1024 or 2048. Note: This field is required if `key_type` is `RSA-HSM`. Changing this forces a new resource to be created.

Collaborator

katbyte Mar 12, 2024

Suggested change

      
            * `key_size` - (Optional) Specifies the Size of the RSA key to create in bytes. For example, 1024 or 2048. *Note*: This field is required if `key_type` is `RSA-HSM`. Changing this forces a new resource to be created.
          
            * `key_size` - (Optional) Specifies the Size of the RSA key to create in bytes. For example, `1024` or `2048`. *Note*: This field is required if `key_type` is `RSA-HSM`. Changing this forces a new resource to be created.

website/docs/r/key_vault_managed_hardware_security_module_key.html.markdown Outdated


		An `automatic` block supports the following:

		* `time_after_creation` - (Optional) Rotate automatically at a duration after create as an [ISO 8601 duration](https://en.wikipedia.org/wiki/ISO_8601#Durations).

Collaborator

katbyte Mar 12, 2024

it seems like these should be called durations not times? ie

Suggested change

      
            * `time_after_creation` - (Optional) Rotate automatically at a duration after create as an [ISO 8601 duration](https://en.wikipedia.org/wiki/ISO_8601#Durations).
          
            * `duration_after_creation` - (Optional) Rotate automatically at a duration after create as an [ISO 8601 duration](https://en.wikipedia.org/wiki/ISO_8601#Durations).

github-actions bot added the waiting-response label


          rename fields

7031d70

github-actions bot added waiting-response and removed waiting-response labels


          fix typo

081f292

github-actions bot added waiting-response and removed waiting-response labels

katbyte requested changes

View reviewed changes

website/docs/r/key_vault_managed_hardware_security_module_key.html.markdown Outdated


		* `duration_after_creation` - (Optional) Rotate automatically at a duration after create as an [ISO 8601 duration](https://en.wikipedia.org/wiki/ISO_8601#Durations).

		* `time_before_expiry` - (Optional) Rotate automatically at a duration before expiry as an [ISO 8601 duration](https://en.wikipedia.org/wiki/ISO_8601#Durations).

Collaborator

katbyte Mar 12, 2024

this is also a duration? and hsould be duration_before_expiry

website/docs/r/key_vault_managed_hardware_security_module_key.html.markdown Outdated


		A `rotation_policy` block supports the following:

		* `expire_after` - (Optional) Expire a Managed Hardware Security Module Key after given duration as an [ISO 8601 duration](https://en.wikipedia.org/wiki/ISO_8601#Durations).

Collaborator

katbyte Mar 12, 2024

and here

Suggested change

      
            * `expire_after` - (Optional) Expire a Managed Hardware Security Module Key after given duration as an [ISO 8601 duration](https://en.wikipedia.org/wiki/ISO_8601#Durations).
          
            * `expire_after_duration` - (Optional) Expire a Managed Hardware Security Module Key after given duration as an [ISO 8601 duration](https://en.wikipedia.org/wiki/ISO_8601#Durations).


          rename fields

4e88fee

github-actions bot added waiting-response and removed waiting-response labels

katbyte requested changes

View reviewed changes

internal/services/managedhsm/parse/managed_hsm_key_id.go Outdated

+              // ParseNestedItemID parses a managed HSM Nested Item ID (such as a Certificate, Key or Secret)
+              // containing a version into a NestedItemId object
+              func ParseNestedItemID(input string) (*ManagedHSMKeyID, error) {

Collaborator

katbyte Mar 14, 2024

this name is wrong and hsould have managed HSM in it?

internal/services/managedhsm/parse/mhsm_nested_item.go Outdated

@@ @@ -22,14 +22,14 @@ const ( @@
               	RoleAssignmentType MHSMResourceType = "RoleAssignment"
               )
-              type NestedItemId struct {
+              type RoleNestedItemId struct {

Collaborator

katbyte Mar 14, 2024

So instead of making a generic tyoe trying to do multiple things can we split this into three specific IDs with specific opininated types with their own set of parsers and validators? id managed hsm nestedKeyId, managed hsm role definition id, managed hsm role assingment? this will make things far more clear as what the intented suppoted types are

Contributor Author

wuxu92 Mar 19, 2024

reverted this changed. will create separate PR to move to sepcific IDs for role related IDs.

internal/services/managedhsm/client/helpers.go

Comment on lines +22 to +31

+              type localCache struct {
+              	mux        sync.Locker
+              	nameToItem map[string]cacheItem
+              }
+              var defaultCache = &localCache{
+              	mux:        &sync.Mutex{},
+              	nameToItem: map[string]cacheItem{},
+              }

Collaborator

katbyte Mar 15, 2024

why are there two caches??

Contributor Author

wuxu92 Mar 15, 2024

I just defined a struct and an instance of the struct to managed the cache, instead of a global map and lock variables which used in keyvault helpers. I think this way is more readable and easy to manage the lock operations.

internal/services/managedhsm/client/helpers.go

		@@ -0,0 +1,177 @@
		// Copyright (c) HashiCorp, Inc.

Collaborator

katbyte Mar 15, 2024

actually - why is this so different this https://github.com/hashicorp/terraform-provider-azurerm/blob/main/internal/services/keyvault/client/helpers.go ?

Contributor Author

wuxu92 Mar 19, 2024

I implemented a new cache logic that differs from keyvault's helper functions. It may appear different, but it should work properly. The locks in the keyvault helpers are complex and may not function as expected actually.

internal/services/managedhsm/internal.go Outdated

Comment on lines 20 to 23

+              	DeleteNestedItem(ctx context.Context) (autorest.Response, error)
+              	NestedItemHasBeenDeleted(ctx context.Context) (autorest.Response, error)
+              	PurgeNestedItem(ctx context.Context) (autorest.Response, error)
+              	NestedItemHasBeenPurged(ctx context.Context) (autorest.Response, error)

Collaborator

katbyte Mar 15, 2024

we should be removing all references to autorest, so instead can we return at the very least an http response instead?

internal/services/managedhsm/parse/managed_hsm_key_id.go

		@@ -0,0 +1,135 @@
		// Copyright (c) HashiCorp, Inc.

Collaborator

katbyte Mar 15, 2024

to simplify this can we move all the ID parser/id changes into a seperate PR so it can be more easily reviewed & unblock other PRs & work waiting on it?

Contributor Author

wuxu92 Mar 15, 2024

the managed hsm key id is a new file which shoud be included in this PR. I'll create another pull request to move the role definitions into specific IDs.

Member

tombuildsstuff Mar 15, 2024

@wuxu92 can we also include the ManagedHSMNestedKey in the other PR

Contributor Author

wuxu92 Mar 18, 2024

@tombuildsstuff do you mean the KeyID or ManagedHSMKeyID? there is no ManagedHSMNestedKey in this PR.

Member

tombuildsstuff Mar 19, 2024 •

edited

Loading

KeyId should become ManagedHSMNestedKeyIdWithVersion
KeyVersionlessId should become ManagedHSMNestedKeyIdVersionless
ManagedHSMKeyID can be removed

-> Note: whilst this differs from the Key Vault Nested Item parsers - those will change to this approach in the future - since we should not be using these generic Resource ID parser methods going forward.

However as @katbyte has mentioned, can we split this out to a separate PR, since these Resource ID parsers need to be used elsewhere?


          rever role nested item ids and remove direct autorest dependency

cfbcfbe

github-actions bot added waiting-response and removed waiting-response labels

This was referenced Mar 20, 2024

managed_hsm_*: add managed hsm nested key ids #25324

Closed

azurerm_key_vault_managed_hardware_security_module_role_definition got error when run terraform apply "StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="BadParameter" Message="Invalid role definition properties" #24790

Closed

Member

tombuildsstuff commented Apr 19, 2024

Thanks for this PR.

Since this PR requires some rework based on the changes from #25601 - given the criticality of this resource and the learnings from #25601 - we're wanting to take a deeper look into this resource to ensure the behaviour ships the way it needs to.

As such whilst I'd like to thank you for this contribution, unfortunately I'm going to close this PR for the moment - but I've asked @manicminer to take a look into this one next once #25601 has been merged.

Thanks!

tombuildsstuff closed this

github-actions bot commented May 20, 2024

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 bot locked as resolved and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.