controllers: Add logic to Create cephfs encrypted storageclass #2605

iamniting · 2024-05-13T04:59:59Z

Signed-off-by: Nitin Goyal nigoyal@redhat.com

We also need changes in the Ceph and kernel to support this feature. Will add hold until those changes are present in the DS.

openshift-ci · 2024-05-13T05:00:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iamniting

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [iamniting]

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

iamniting · 2024-05-13T05:28:27Z

/retest

malayparida2000 · 2024-05-13T05:42:22Z

controllers/storagecluster/storageclasses.go

+	encryptedStorageClassConfig := newCephFilesystemStorageClassConfiguration(initData)
+	encryptedStorageClassConfig.storageClass.ObjectMeta.Name = generateNameForEncryptedCephFileSystemSC(initData)
+	// adding a annotation to support smart cloning across namespace for encrypted volume
+	encryptedStorageClassConfig.storageClass.ObjectMeta.Annotations["cdi.kubevirt.io/clone-strategy"] = "copy"


Should confirm with kubevirt team once if they want to support cephfs volumes for this as well before adding the annotation.

@nixpanic Can you pls confirm this if you have an idea?

i dont think we use cephfs for kubevirt. do we have any cephfs storageclass with this annotation in ocs-operator?

No only encrypted rbd is the one where it was added.

I removed the annotation.

KubeVirt can use CephFS, but it is not recommended. Users have better performance and more options when using RBD with block-mode, that is the preferred storage backend when using Ceph.

SanjalKatiyar · 2024-05-14T07:53:30Z

controllers/storagecluster/reconcile.go

@@ -853,6 +853,12 @@ func validateCustomStorageClassNames(sc *ocsv1.StorageCluster) error {
 		}
 		scMap[sc.Spec.Encryption.StorageClassName] = true
 	}
+	if sc.Spec.Encryption.CephFs.StorageClass && sc.Spec.Encryption.KeyManagementService.Enable && sc.Spec.Encryption.CephFs.StorageClassName != "" {
+		if _, ok := scMap[sc.Spec.Encryption.CephFs.StorageClassName]; ok {
+			duplicateNames = append(duplicateNames, "Encryption")


how about this ?
(and for RBD, append "RBD Encryption"... instead of just using "Encryption" in both cases)

Suggested change

duplicateNames = append(duplicateNames, "Encryption")

duplicateNames = append(duplicateNames, "CephFS Encryption")

yeah correct, changed it

Madhu-1 · 2024-05-14T08:50:04Z

api/v1/storagecluster_types.go

+	StorageClassName string `json:"storageClassName,omitempty"`
+	// Configure the default CephFS encrypted storage class
+	// +optional
+	CephFs               DefaultStorageClassSpec  `json:"cephfs,omitempty"`


Suggested change

CephFs DefaultStorageClassSpec `json:"cephfs,omitempty"`

CephFS DefaultStorageClassSpec `json:"cephFS,omitempty"`

Madhu-1 · 2024-05-14T08:51:44Z

controllers/storagecluster/storageclasses.go

+	encryptedStorageClassConfig := newCephFilesystemStorageClassConfiguration(initData)
+	encryptedStorageClassConfig.storageClass.ObjectMeta.Name = generateNameForEncryptedCephFileSystemSC(initData)
+	// adding a annotation to support smart cloning across namespace for encrypted volume
+	encryptedStorageClassConfig.storageClass.ObjectMeta.Annotations["cdi.kubevirt.io/clone-strategy"] = "copy"


i dont think we use cephfs for kubevirt. do we have any cephfs storageclass with this annotation in ocs-operator?

Madhu-1 · 2024-05-14T08:53:30Z

api/v1/storagecluster_types.go

+	StorageClassName string `json:"storageClassName,omitempty"`
+	// Configure the default CephFS encrypted storage class
+	// +optional
+	CephFs               DefaultStorageClassSpec  `json:"cephfs,omitempty"`


@iamniting Did we decide that we are going with this approach?

Yes we did that.

Madhu-1 · 2024-05-14T08:55:04Z

api/v1/storagecluster_types.go

 	KeyManagementService KeyManagementServiceSpec `json:"kms,omitempty"`
 	// KeyRotation defines options for Key Rotation.
 	// +optional
 	KeyRotation KeyRotationSpec `json:"keyRotation,omitempty"`
 }

+type DefaultStorageClassSpec struct {


Suggested change

type DefaultStorageClassSpec struct {

type StorageClassSpec struct {

}

// or

type CSIStorageClassSpec struct {

}

default doesnt suit as its a shared struct for cephfs/rbd/nfs

Changed it to StorageClassSpec

Madhu-1 · 2024-05-14T08:55:38Z

api/v1/storagecluster_types.go

 	KeyManagementService KeyManagementServiceSpec `json:"kms,omitempty"`
 	// KeyRotation defines options for Key Rotation.
 	// +optional
 	KeyRotation KeyRotationSpec `json:"keyRotation,omitempty"`
 }

+type DefaultStorageClassSpec struct {
+	// Enable Default StorageClass


Suggested change

// Enable Default StorageClass

Create StorageClass

Madhu-1 · 2024-05-14T08:57:30Z

api/v1/storagecluster_types.go

@@ -456,18 +456,33 @@ type EncryptionSpec struct {
 	Enable bool `json:"enable,omitempty"`
 	// +optional
 	ClusterWide bool `json:"clusterWide,omitempty"`
+	// Configure the default rbd encrypted storage class


can we remove the word default as we are not creating defaults with this one? use RBD

iamniting · 2024-05-14T14:09:00Z

/hold

Madhu-1 · 2024-05-14T15:24:47Z

api/v1/storagecluster_types.go

 	// +optional
 	StorageClass bool `json:"storageClass,omitempty"`
 	// StorageClassName specifies the name of the storage class created for ceph encrypted block pools
 	// +kubebuilder:validation:MaxLength=253
 	// +kubebuilder:validation:Pattern=^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
-	StorageClassName     string                   `json:"storageClassName,omitempty"`
+	StorageClassName string `json:"storageClassName,omitempty"`
+	// Configure the default CephFS encrypted storage class


Suggested change

// Configure the default CephFS encrypted storage class

// Configure the CephFS encrypted storage class

Madhu-1 · 2024-05-14T15:25:26Z

api/v1/storagecluster_types.go

+type StorageClassSpec struct {
+	// Create StorageClass
+	// +optional
+	StorageClass bool `json:"storageClass,omitempty"`


Do we need this to be pointer to bool or just bool

anything can be fine, I kept it as bool to align with the old flag.

Madhu-1 · 2024-05-14T15:25:57Z

api/v1/storagecluster_types.go

 	KeyManagementService KeyManagementServiceSpec `json:"kms,omitempty"`
 	// KeyRotation defines options for Key Rotation.
 	// +optional
 	KeyRotation KeyRotationSpec `json:"keyRotation,omitempty"`
 }

+type StorageClassSpec struct {
+	// Create StorageClass


Suggested change

// Create StorageClass

// Create Storage class

lets be constant with name in all the places storage class

Signed-off-by: Nitin Goyal <nigoyal@redhat.com>

Madhu-1

LGTM, lets keep it on hold until its fully tested with Rook+cephCSI

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 13, 2024

iamniting force-pushed the cephfs-encryption branch from ccc76e7 to a2985f1 Compare May 13, 2024 05:10

iamniting marked this pull request as draft May 13, 2024 05:14

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 13, 2024

malayparida2000 reviewed May 13, 2024

View reviewed changes

SanjalKatiyar reviewed May 14, 2024

View reviewed changes

Madhu-1 reviewed May 14, 2024

View reviewed changes

iamniting force-pushed the cephfs-encryption branch 2 times, most recently from f35322b to 1967d06 Compare May 14, 2024 13:53

iamniting requested review from Madhu-1, malayparida2000 and SanjalKatiyar May 14, 2024 13:54

iamniting marked this pull request as ready for review May 14, 2024 13:55

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 14, 2024

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 14, 2024

Madhu-1 reviewed May 14, 2024

View reviewed changes

iamniting added 2 commits May 15, 2024 09:42

api: Add new feild for supporting the cephfs encrypted storageclass

5909f89

Signed-off-by: Nitin Goyal <nigoyal@redhat.com>

controllers: Add logic to Create cephfs encrypted storageclass

1c68588

Signed-off-by: Nitin Goyal <nigoyal@redhat.com>

iamniting force-pushed the cephfs-encryption branch from 1967d06 to 1c68588 Compare May 15, 2024 04:13

iamniting requested a review from Madhu-1 May 15, 2024 04:20

Madhu-1 reviewed May 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

controllers: Add logic to Create cephfs encrypted storageclass #2605

controllers: Add logic to Create cephfs encrypted storageclass #2605

iamniting commented May 13, 2024 •

edited

openshift-ci bot commented May 13, 2024

iamniting commented May 13, 2024

malayparida2000 May 13, 2024

iamniting May 13, 2024

Madhu-1 May 14, 2024

iamniting May 14, 2024

iamniting May 14, 2024

nixpanic May 17, 2024

SanjalKatiyar May 14, 2024

iamniting May 14, 2024

Madhu-1 May 14, 2024

iamniting May 14, 2024

Madhu-1 May 14, 2024

Madhu-1 May 14, 2024

iamniting May 14, 2024

Madhu-1 May 14, 2024

Madhu-1 May 14, 2024

iamniting May 14, 2024

Madhu-1 May 14, 2024

iamniting May 14, 2024

Madhu-1 May 14, 2024

iamniting May 14, 2024

iamniting commented May 14, 2024

Madhu-1 May 14, 2024

iamniting May 15, 2024

Madhu-1 May 14, 2024

iamniting May 15, 2024

Madhu-1 May 14, 2024

iamniting May 15, 2024

Madhu-1 left a comment

	duplicateNames = append(duplicateNames, "Encryption")
	duplicateNames = append(duplicateNames, "CephFS Encryption")

	CephFs DefaultStorageClassSpec `json:"cephfs,omitempty"`
	CephFS DefaultStorageClassSpec `json:"cephFS,omitempty"`

	// Configure the default CephFS encrypted storage class
	// Configure the CephFS encrypted storage class

controllers: Add logic to Create cephfs encrypted storageclass #2605

Are you sure you want to change the base?

controllers: Add logic to Create cephfs encrypted storageclass #2605

Conversation

iamniting commented May 13, 2024 • edited

openshift-ci bot commented May 13, 2024

iamniting commented May 13, 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

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

iamniting commented May 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

Madhu-1 left a comment

Choose a reason for hiding this comment

iamniting commented May 13, 2024 •

edited