-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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_key_vault_managed_hardware_security_module_key_rotation_policy
#27306
Conversation
|
||
The following arguments are supported: | ||
|
||
* `expire_after` - (Required) Specify the expiration time of the 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.
is this a date? a duration? what format?
* `expire_after` - (Required) Specify the expiration time of the key. | |
* `expire_after_date` - (Required) Specify the expiration time of the key. |
* `expire_after` - (Required) Specify the expiration time of the key. | |
* `expire_after_duration` - (Required) Specify the expiration time of the 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.
please detail what this actually means, is it related to the lifecycle actions/trigger not the 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.
Updated the description and kept the name unchanged to align with the key vault rotation policy schema.
|
||
--- | ||
|
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.
why is this sperator here?
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.
removed
_, err = client.GetKey(ctx, keyID.BaseUri(), keyID.KeyName, "") | ||
if err != nil { |
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.
these lines can be merged
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.
updated
...rvices/managedhsm/key_vault_managed_hardware_security_module_key_rotation_policy_resource.go
Show resolved
Hide resolved
func flattenKeyRotationPolicy(p keyvault.KeyRotationPolicy) MHSMKeyRotationPolicyResourceSchema { | ||
var res MHSMKeyRotationPolicyResourceSchema | ||
if p.Attributes != nil { | ||
res.ExipreAfter = pointer.From(p.Attributes.ExpiryTime) |
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.
what does this expire? what happens ?
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.
as documentation specified this is to set the key expiry after create/rotation.
website/docs/r/key_vault_managed_hardware_security_module_key_rotation_policy.html.markdown
Outdated
Show resolved
Hide resolved
website/docs/r/key_vault_managed_hardware_security_module_key_rotation_policy.html.markdown
Outdated
Show resolved
Hide resolved
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.
|
||
var expiryTime *string // = nil // needs to be set to nil if not set | ||
if policy.ExipreAfter != "" { | ||
expiryTime = utils.String(policy.ExipreAfter) |
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.
Can we not use the utils
package here as we're deprecating these functions, and instead use the pointer
package as you have done in the flatten below?
expiryTime = utils.String(policy.ExipreAfter) | |
expiryTime = pointer.To(policy.ExipreAfter) |
type MHSMKeyRotationPolicyTestResource struct{} | ||
|
||
func testAccMHSMKeyRotationPolicy_all(t *testing.T) { | ||
data := acceptance.BuildTestData(t, "azurerm_key_vault_managed_hardware_security_module_key", "test") |
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.
This is the wrong resource here?
data := acceptance.BuildTestData(t, "azurerm_key_vault_managed_hardware_security_module_key", "test") | |
data := acceptance.BuildTestData(t, "azurerm_key_vault_managed_hardware_security_module_key_rotation_policy", "test") |
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.
udpated.
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 the updates @wuxu92 - Spotted a couple typos and whitespace items if you can take a look, then I think we should be good to run the test.
return err | ||
} | ||
|
||
// settign a blank policy to delete the existing policy |
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.
// settign a blank policy to delete the existing policy | |
// setting a blank policy to delete the existing policy |
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.
fixed
|
||
type MHSMKeyRotationPolicyResourceSchema struct { | ||
ManagedHSMKeyID string `tfschema:"managed_hsm_key_id"` | ||
ExipreAfter string `tfschema:"expire_after"` |
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.
Typo here
ExipreAfter string `tfschema:"expire_after"` | |
ExpireAfter string `tfschema:"expire_after"` |
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.
fixed.
|
||
|
||
|
||
|
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.
unnecessary whitespace here?
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.
format issues of acc test fixed. can we add this to the terrafmt linter?
} | ||
resource "azurerm_key_vault_managed_hardware_security_module" "test" { |
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.
} | |
resource "azurerm_key_vault_managed_hardware_security_module" "test" { | |
} | |
resource "azurerm_key_vault_managed_hardware_security_module" "test" { |
|
||
|
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.
unnecessary whitespace here?
} | ||
resource "azurerm_key_vault_certificate" "cert" { |
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.
} | |
resource "azurerm_key_vault_certificate" "cert" { | |
} | |
resource "azurerm_key_vault_certificate" "cert" { |
Test passed:
|
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 @wuxu92 - LGTM 🛋️
website/docs/r/key_vault_managed_hardware_security_module_key_rotation_policy.html.markdown
Outdated
Show resolved
Hide resolved
website/docs/r/key_vault_managed_hardware_security_module_key_rotation_policy.html.markdown
Outdated
Show resolved
Hide resolved
…rotation_policy.html.markdown
…rotation_policy.html.markdown
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. |
Community Note
Description
Add key rotation policy support for Managed HSM Keys. Since this based on a Data plane API, I created a New resource for it instead of integrating into the Managed HSM Key resource. Based on my test, notification is not supported in MHSM Key. There is only one rotate policy for each key and there is no separation resource/id for it, so the new resource just resuse the managed hsm key id as its resource id.
https://learn.microsoft.com/en-us/azure/key-vault/managed-hsm/key-rotation
Given that creating a Managed HSM resource is a heavy operation, I created only one AccTest with both basic and update included. The creation/update of the rotation policy is fast.
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_key_vault_managed_hardware_security_module_key_rotation_policy
- New resource for Managed HSM Key rotation policyThis is a (please select all that apply):
Related Issue(s)
Note
If this PR changes meaningfully during the course of review please update the title and description as required.