azurerm_key_vault_key_rotation_policy - New resource

[aristosvo]

Depends on #18576

Fixes #14471
Implemented in #19113 as inline properties.

• AccTests
• Docs

[magodo]

Thanks for this PR!
I've taken a look through and left some comments inline, but this is mostly looking good to me 👍

[magodo]

If each key can only has a single rotation, then we can simply reuse the same format of key resource id as the rotation policy id:

Suggested change

	terraform import azurerm_key_vault_key_rotation_policy.example /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/resGroup1/providers/Microsoft.KeyVault/vaults/vault1/keys/key1/rotationpolicy
	terraform import azurerm_key_vault_key_rotation_policy.example /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/resGroup1/providers/Microsoft.KeyVault/vaults/vault1/keys/key1

This makes the id format align with others (even amount of segments), meanwhile it saves your effort to manually implement the ID parser.

[aristosvo]

This would make it indeed easier.., but as it has its own ID and is implemented in this PR as a separate resource, I'd argue that it makes sense to have its specific ID (and resource ID).

[aristosvo]

Thanks for this PR!

I've taken a look through and left some comments inline, but this is mostly looking good to me 👍

Hi @magodo ! Thanks for the review.

One thing that is still on my mind: should we not implement this in-line of the azurerm_key_vault_key instead? Just picking your brain here:

• it is impossible to really delete this resource
• it is tightly coupled to one key
• only one per key
• checks on key dates make some extra validations possible

This is one of the things I've been deciding beforehand based on gutfeeling, I'm not aware if there are guidelines to help us here :)

[magodo]

@aristosvo That makes sense to me! While I'd prefer listening to other's opinion. @katbyte WDYT?

[katbyte]

@aristosvo,

you make some really good arguments and i think i agree with you. generally a 1:1 relation alone is reason enough to consider add it to the existing resource. but when you add in that it cannot be deleted and it really is just "enabling feature X on resource Y" it does make a lot of sense to be part of the key-vault resource.

i always like to think of the UX when making these sort of choices and what I as a user would expect/want, and is there any real benefit to making it separate rather than just trying to mirror the apis as terraform is a stateful DSL not a set of CRUD api operations.

unfortunately we do not have any real guidelines here yet that we all agree on, and something like this would be great to pr into the contributor docs so we can actually nail down some firm guidelines on when/when not to and discuss on the PR what we all think they should be.

improving those docs is something i'll be looking towards later this year so adding this topic to the list 😃

[aristosvo]

Based on previous comment I'll close this PR for now and try to implement it inline first. These experiences may show it was not a good idea, if that's the case then this one is reopened easily enough.

Edit: new PR -> #19113

[aristosvo]

Reopened and updated to have a good comparison

[github-actions]

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.