fix: #3625 - Added CMK encryption properties and updated RSV module version to latest stable release #3821
Conversation
upgraded the rsv module to the latest non-preview and added parameters for CMK
microsoft-github-policy-service
bot
added
the
Needs: Triage 🔍
microsoft-github-policy-service
bot
added
the
Type: AVM 🅰️ ✌️ Ⓜ️
avm-team-linter
bot
added
the
Needs: Core Team 🧞
|
@ReneHezser updated now |
|
Great contribution @alexanderojala. I guess we could also link the PR to [this] issue: #3625 Added a few comments - with only the CMK one being somewhat more complex if it's the first time. Just let me know if I can be of any help (: |
Co-authored-by: Alexander Sehr <ASehr@hotmail.de>
Co-authored-by: Alexander Sehr <ASehr@hotmail.de>
| useSystemAssignedIdentity: empty(customerManagedKey.?userAssignedIdentityResourceId) | ||
| } | ||
| keyVaultProperties: { | ||
| keyUri: cMKKeyVault.properties.vaultUri |
There was a problem hiding this comment.
This seems to be an interesting case. @alexanderojala correct me if I'm wrong, but I read the docs like the implementation can only do auto-key-rotation and you cannot actually specify a specific key version. If the case, we actually may need a different, simplified, interface. The current one. customerManagedKeyWithAutoRotateType, would seemingly allow a user to disable the auto-key-rotation, even though the code in the module actually cannot handle that case.
@eriqua, thoughts?
If my assumption is correct, we'd just need another interface that would contain the key & key vault reference, as well as the managed identity...
There was a problem hiding this comment.
Not yet solved 😉 Please only resolve comments if they're code suggestions. For any other, ideally the reviewer resolves them to enable a re-review. Otherwise it's a bit hard to tell what and what does not need another look.
Co-authored-by: Alexander Sehr <ASehr@hotmail.de>
Co-authored-by: Alexander Sehr <ASehr@hotmail.de>
|
@AlexanderSehr i have updated the CMK settings and pipeline seems to run fine now. Could you kindly review the changes? |
| @@ -411,6 +437,44 @@ output privateEndpoints array = [ | |||
| // Definitions // | |||
| // =============== // | |||
|
|
|||
| type customerManagedKeyType = { | |||
There was a problem hiding this comment.
Not remotely aligned with the AVM Interface for CMK: https://azure.github.io/Azure-Verified-Modules/specs/bcp/res/interfaces/#customer-managed-keys
We have a few options here. One would be that you try and merry to expectations of the RSV with the AVM interface to use the same. Next would be that I try to suggest a code change from the top of my head as part of this PR which would need to be tested and finished. Another would be that we merge this PR into a temporal branch in upstream, I apply the updates, open a new PR and have you check the changes. Last but not least, you could give me permissions in your fork to create branches and I could do the same there.
In any case, as it is right now, it is unfortunately not aligned and definitely should before the module is published. From the looks of it, there should be a fair change that we could use a variant of the AVM interface and just throw some properties out for this implementation (e.g., the key version).
There was a problem hiding this comment.
Much appreciated for the help, i will add you to my fork.
There was a problem hiding this comment.
Great, thanks @alexanderojala. I'll get to it asap
Description
Pipeline Reference
Type of Change
version.json:version.json.version.json.Checklist
Set-AVMModulelocally to generate the supporting module files.