Encryption of block volumes using EncryptionClasses #3106

nikolay-andreev · 2024-11-14T07:46:09Z

What this PR does / why we need it:
Implement encryption of block volumes using EncryptionClass associated with PVC

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Testing done:
GINKGO_FOCUS=encryption make test-e2e

Verify block volume is encrypted with a specific KeyID/Provider when PVC is associated with EncryptionClass.
Verify block volume is encrypted with the default KeyID/Provider when PVC is not associated with EncryptionClass but has encrypted StorageClass.
Verify block volume is recrypted the changing EncryptionClass associated with PVCs.
Verify block volume is recrypted when the EncryptionClass associated with PVCs has its key rotated.
Verify PVC creation fails when associated with EncryptionClass and StorageClass not supporting encryption.

SUCCESS! -- 5 Passed | 0 Failed | 0 Pending | 905 Skipped
PASS

Special notes for your reviewer:

Release note:
-->

Signed-off-by: Nikolay Andreev <[email protected]>

linux-foundation-easycla · 2024-11-14T07:46:15Z

❌ - login: @nikolay-andreev / name: Nikolay Andreev . The commit (27acae3) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

k8s-ci-robot · 2024-11-14T07:46:15Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nikolay-andreev
Once this PR has been reviewed and has the lgtm label, please assign divyenpatel for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2024-11-14T07:46:18Z

Welcome @nikolay-andreev!

It looks like this is your first PR to kubernetes-sigs/vsphere-csi-driver 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/vsphere-csi-driver has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-11-14T07:46:18Z

Hi @nikolay-andreev. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

divyenpatel · 2024-11-14T19:44:03Z

/ok-to-test

k8s-ci-robot · 2024-11-14T19:58:27Z

@nikolay-andreev: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-vsphere-csi-driver-verify-golangci-lint	`27acae3`	link	true	`/test pull-vsphere-csi-driver-verify-golangci-lint`
pull-vsphere-csi-driver-verify-staticcheck	`27acae3`	link	true	`/test pull-vsphere-csi-driver-verify-staticcheck`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

adikul30 · 2024-11-14T20:11:36Z

pkg/csi/service/common/constants.go

@@ -431,6 +431,8 @@ const (
 	WorkloadDomainIsolationFSS = "workload-domain-isolation"
 	// VPCCapabilitySupervisor is a supervisor capability indicating if VPC FSS is enabled
 	VPCCapabilitySupervisor = "VPC_Supported"
+	// TODO: WCP_VMService_BYOK_FSS enables Bring Your Own Key (BYOK) capabilities.
+	BYOK_FSS = "byok"


Can you confirm if this capability name exists in the required repository on GitLab.

divyenpatel · 2024-11-14T19:50:07Z

cmd/syncer/main.go

@@ -358,6 +359,17 @@ func initSyncerComponents(ctx context.Context, clusterFlavor cnstypes.CnsCluster
 				os.Exit(0)
 			}
 		}()
+		if commonco.ContainerOrchestratorUtility.IsFSSEnabled(ctx, common.BYOK_FSS) &&
+			clusterFlavor == cnstypes.CnsClusterFlavorWorkload {


Can we move condition clusterFlavor == cnstypes.CnsClusterFlavorWorkload first, so FSS related log does not get printed for other flavors.

divyenpatel · 2024-11-14T19:53:31Z

cmd/syncer/main.go

+
+	defer func() {
+		log.Info("Cleaning up vc sessions cns operator")
+		if r := recover(); r != nil {


if you are sharing the same VC session used by other services, we do not need to handle recover here to logout sessions. We are already doing this in the main - https://github.com/kubernetes-sigs/vsphere-csi-driver/blob/master/cmd/syncer/main.go#L349-L354

divyenpatel · 2024-11-14T19:56:40Z

go.mod

@@ -22,7 +22,8 @@ require (
 	github.com/prometheus/client_golang v1.18.0
 	github.com/stretchr/testify v1.9.0
 	github.com/vmware-tanzu/vm-operator/api v1.8.2
-	github.com/vmware/govmomi v0.46.0
+	github.com/vmware-tanzu/vm-operator/external/byok v0.0.0-20241108223224-20f977201370


we need to make sure we have release tag for github.com/vmware-tanzu/vm-operator having this dependency. Do we have a plan to cut release for vm-operator for this change?

divyenpatel · 2024-11-14T19:57:13Z

go.mod

@@ -22,7 +22,8 @@ require (
 	github.com/prometheus/client_golang v1.18.0
 	github.com/stretchr/testify v1.9.0
 	github.com/vmware-tanzu/vm-operator/api v1.8.2
-	github.com/vmware/govmomi v0.46.0
+	github.com/vmware-tanzu/vm-operator/external/byok v0.0.0-20241108223224-20f977201370
+	github.com/vmware/govmomi v0.47.0-alpha.0.0.20241111085955-321b6f0907a4


we need to make sure to use govmomi release tag

Is that a repo rule? In VM Op we link to commits all the time. There is not always a release. It does not really matter from a technical standpoint as a tag is just a pointer to a ref. So I'm guessing it is a repo requirement?

at least before release we make sure we do not use any user commit from the repo.
this is repo specific requirement we are following.

we need to make sure to use govmomi release tag

Created a v0.46.1 release with just @nikolay-andreev 's changes and a small vcsim fix: vmware/govmomi@v0.46.0...v0.46.1

divyenpatel · 2024-11-14T20:04:15Z

pkg/common/cns-lib/crypto/client.go

@@ -0,0 +1,247 @@
+package crypto


we need to add Copyright header in new files.

divyenpatel · 2024-11-14T21:52:59Z

pkg/syncer/byokoperator/vcenter/provider.go

+)
+
+type Provider interface {
+	DoesProfileSupportEncryption(ctx context.Context, profileID string) (bool, error)


add method docs

divyenpatel · 2024-11-14T22:08:34Z

tests/e2e/crypto.go

@@ -0,0 +1,294 @@
+/*
+Copyright 2021 The Kubernetes Authors.


divyenpatel · 2024-11-14T22:11:25Z

pkg/syncer/byokoperator/vcenter/provider.go

+	p.reinitialize = true
+}
+
+func (p *defaultProvider) getVcClient(ctx context.Context) (*cnsvsphere.VirtualCenter, error) {


Can we use existing util file we have to get VC Client?
https://github.com/kubernetes-sigs/vsphere-csi-driver/blob/master/pkg/csi/service/common/util.go#L51-L68

divyenpatel · 2024-11-14T22:14:34Z

pkg/syncer/byokoperator/vcenter/provider.go

+	lock         sync.RWMutex
+}
+
+func (p *defaultProvider) DoesProfileSupportEncryption(ctx context.Context, profileID string) (bool, error) {


Can you move this function to common pbm.go file
https://github.com/kubernetes-sigs/vsphere-csi-driver/blob/master/pkg/common/cns-lib/vsphere/pbm.go

divyenpatel · 2024-11-14T22:17:22Z

pkg/csi/service/common/constants.go

@@ -431,6 +431,8 @@ const (
 	WorkloadDomainIsolationFSS = "workload-domain-isolation"
 	// VPCCapabilitySupervisor is a supervisor capability indicating if VPC FSS is enabled
 	VPCCapabilitySupervisor = "VPC_Supported"
+	// TODO: WCP_VMService_BYOK_FSS enables Bring Your Own Key (BYOK) capabilities.
+	BYOK_FSS = "byok"


it is better to manage this with wcp capability so both vm service and csi can make use of common source of truth to determine if fss needs to be enabled or disabled.

divyenpatel · 2024-11-14T22:18:00Z

@nikolay-andreev can you also fix linter issues on the PR?

Encryption of block volumes using EncryptionClasses

27acae3

Signed-off-by: Nikolay Andreev <[email protected]>

k8s-ci-robot requested review from kavyashree-r and SandeepPissay November 14, 2024 07:46

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Nov 14, 2024

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 14, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 14, 2024

adikul30 reviewed Nov 14, 2024

View reviewed changes

divyenpatel requested changes Nov 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encryption of block volumes using EncryptionClasses #3106

Encryption of block volumes using EncryptionClasses #3106

nikolay-andreev commented Nov 14, 2024

linux-foundation-easycla bot commented Nov 14, 2024

k8s-ci-robot commented Nov 14, 2024

k8s-ci-robot commented Nov 14, 2024

k8s-ci-robot commented Nov 14, 2024

divyenpatel commented Nov 14, 2024

k8s-ci-robot commented Nov 14, 2024

adikul30 Nov 14, 2024

divyenpatel Nov 14, 2024

divyenpatel Nov 14, 2024

divyenpatel Nov 14, 2024

divyenpatel Nov 14, 2024

akutz Nov 14, 2024

divyenpatel Nov 14, 2024

dougm Nov 15, 2024

divyenpatel Nov 14, 2024

divyenpatel Nov 14, 2024

divyenpatel Nov 14, 2024

divyenpatel Nov 14, 2024

divyenpatel Nov 14, 2024

divyenpatel Nov 14, 2024

divyenpatel commented Nov 14, 2024

Encryption of block volumes using EncryptionClasses #3106

Are you sure you want to change the base?

Encryption of block volumes using EncryptionClasses #3106

Conversation

nikolay-andreev commented Nov 14, 2024

linux-foundation-easycla bot commented Nov 14, 2024

k8s-ci-robot commented Nov 14, 2024

k8s-ci-robot commented Nov 14, 2024

k8s-ci-robot commented Nov 14, 2024

divyenpatel commented Nov 14, 2024

k8s-ci-robot commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divyenpatel commented Nov 14, 2024