Skip to content

Conversation

@rolandmkunkel
Copy link
Contributor

@rolandmkunkel rolandmkunkel commented Nov 1, 2021

Which issue this PR addresses:

Second part of:
https://msazure.visualstudio.com/AzureRedHatOpenShift/_workitems/edit/7404076

What this PR does / why we need it:

This PR adds tests for end-to-end disk encryption. When creating a cluster in CI or via local deployment, all data and OS disks are encrypted with customer-owned keys, which in case of our testing environment is either a shared key from the shared keyvault, or a generated one in case of CI. The tests in this PR make sure that the default storage class uses disk encryption, and that all OS disks are encrypted.

Is there any documentation that needs to be updated for this PR?

no, everything works under the hood

@github-actions
Copy link

github-actions bot commented Nov 2, 2021

Please rebase pull request.

@github-actions github-actions bot added the needs-rebase branch needs a rebase label Nov 2, 2021
@rolandmkunkel rolandmkunkel force-pushed the disk-encryption-e2e-tests-ci-cluster-creation-and-tests branch from d4cbb97 to cf4a7d8 Compare November 2, 2021 10:23
@github-actions github-actions bot removed the needs-rebase branch needs a rebase label Nov 2, 2021
@rolandmkunkel rolandmkunkel force-pushed the disk-encryption-e2e-tests-ci-cluster-creation-and-tests branch from cf4a7d8 to 5b63c3d Compare November 2, 2021 11:54
@rolandmkunkel rolandmkunkel marked this pull request as ready for review November 2, 2021 14:07
Comment on lines +62 to +65
diskEncryptionSet, err := clients.DiskEncryptionSets.Get(ctx, vnetResourceGroup, fmt.Sprintf("%s-disk-encryption-set", vnetResourceGroup))
Expect(err).NotTo(HaveOccurred())
expectedResourceIDs = append(expectedResourceIDs, strings.ToLower(*diskEncryptionSet.ID))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solves work item 12050816. I assigned it to you.

Copy link
Contributor

@m1kola m1kola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this work?

I see that in CI we create KV, key, disk encrpyiont set and they get deleted together with vnetResourceGroup.

When not in CI:

  1. What is the experience with go run ./hack/cluster create?

    which in case of our testing environment is either a shared key from the shared keyvault, or a generated one in case of CI.

    Do I undersntad correctly that we use resources from the shared environemnt and only add Microsoft.KeyVault/vaults/accessPolicies? If so, I think there is mismatch in names between this PR and #1793. For example, key vault name is concat(take(resourceGroup().name,15), '-sharedKV') vs fmt.Sprintf("%s%s", vnetResourceGroup, sharedKeyVaultNameSuffix) in this PR.

  2. What is the experience with go run ./hack/cluster delete?
    Do I understand correctly that we don't have to delete anything when not in CI because we use shared kv, key and disk encryption set?

@rolandmkunkel rolandmkunkel force-pushed the disk-encryption-e2e-tests-ci-cluster-creation-and-tests branch 3 times, most recently from 3c19a43 to 2569226 Compare November 10, 2021 13:25
@rolandmkunkel
Copy link
Contributor Author

Regarding the questions:

  1. You are correct, except for that access policies are not required to be set when using the shared environment. Access policies are only required on the disk encryption set for keys, which is taken care of while creating the environment. I fixed the name mismatch for the shared vault name.
  2. Correct.

Copy link
Contributor

@m1kola m1kola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only have one question about condition for KV access policy (diskEncryptionKeyVaultAccessPolicy). Just double checking. If we actually don't need it - I'm happy to merge it as is.

Rest of the comments - let's create a small PR for them. These are 5 minutes changes and I don't want to hold this PR any longer becuase of them.

@rolandmkunkel rolandmkunkel force-pushed the disk-encryption-e2e-tests-ci-cluster-creation-and-tests branch from 2569226 to 8499c97 Compare November 16, 2021 11:45
Copy link
Contributor

@m1kola m1kola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! I think this is ready to be merged once e2es are green 🎉

Reminder to myself: squash on merge.

@m1kola m1kola merged commit ef412b2 into Azure:master Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants