fix: adding cmk to data disks

[Dylan-Prins]

Description

CMK was not added to the Data disks, while the parameter was there.

Pipeline Reference

Pipeline

Type of Change

• Azure Verified Module updates:
  • Bugfix containing backwards-compatible bug fixes, and I have NOT bumped the MAJOR or MINOR version in version.json:
  • Feature update backwards compatible feature updates, and I have bumped the MINOR version in version.json.
  • Breaking changes and I have bumped the MAJOR version in version.json.
  • Update to documentation
• Update to CI Environment or utilities (Non-module affecting changes)

Checklist

• I'm sure there are no other open Pull Requests for the same update/change
• I have run Set-AVMModule locally to generate the supporting module files.
• My corresponding pipelines / checks run clean and green without any errors or warnings
• I have updated the module's CHANGELOG.md file with an entry for the next version

[josunefon]

Hi @Dylan-Prins thanks a lot for your contribution, before we approve it, please take a look to the static validation tests that are failing

[Dylan-Prins]

Hi @Dylan-Prins thanks a lot for your contribution, before we approve it, please take a look to the static validation tests that are failing

Yes, i was a little bit hastu with the PR. I will update the PR when the tests are successfull.

[Dylan-Prins]

@josunefon

ok, the tests are passed! I only have trouble setting up my key vault for passing the additional parameters. I have created a key vault and gave the app reg the permissions. following by adding the GitHub secret with the key vault name.

Is there someone who can help me troubleshoot?

[josunefon]

Hi @Dylan-Prins ,

I cheked the last GH action and I saw that you're having the problem with the backup management service, did you check this requirement for the custom secrets?

On the other hand, does the SPN you're using has permissions at key vault secret level? because on the actions error what I see is that it can't be retrieved

[Dylan-Prins]

@josunefon not going to merged, because of the tests? maybe someone can run them for me?

[AlexanderSehr]

@josunefon not going to merged, because of the tests? maybe someone can run them for me?

Hey @Dylan-Prins,
to chime in here: Yes, pretty much. We have to ensure the tests work before the PR goes in as any bug/failed static test etc. would block any other PR to the module until it is fixed in upstream.
If you don't have an option to validate the change on your end, we can implement a workaround by merging your PR not directly into main, but an intermediate branch in upstream from where we can test. If any test happens to fail, we'd then need to fix it though.

Anyways, it's a great contribution and should definitely go in. 🚀 We just have to make sure it works in one way or another before it hits main :)

[AlexanderSehr]

I guess this should rather be something akin to dataDisk.managedDisk.?type ?? 'EncryptionAtRestWithPlatformKey'?

If defaulting to EncryptionAtRestWithCustomerKey I guess you'd always need to provide a key.

[AlexanderSehr]

I wonder if the entire property should be conditional / if the deployment works if no encryption is specified, yet the parameters are passed in as

encryption: {
  diskEncryptionSetId: null
  type: null
}

No need to change it now, it should just be tested. However in case it may fail we'd need to change it to e.g.

encryption: !empty(dataDisk.managedDisk.?diskEncryptionSetResourceId) ? {
  diskEncryptionSetId: dataDisk.managedDisk.?diskEncryptionSetResourceId
  type: dataDisk.managedDisk.?type
} : null

or

...(!empty(dataDisk.managedDisk.?diskEncryptionSetResourceId)  ? { encryption: {
  diskEncryptionSetId: dataDisk.managedDisk.?diskEncryptionSetResourceId
  type: dataDisk.managedDisk.?type
} : {})

UNLESS, the resource type anyways defaults to

encryption: {
  diskEncryptionSetId: null
  type: 'EncryptionAtRestWithPlatformKey'
}

i.e., MS-managed encryption. In that case, the default for type should just be EncryptionAtRestWithPlatformKey and you're good to go