provider-server: send info of kernelMountOptions for cephfs to client when encryption in transit is enabled #2707

rohan47 · 2024-07-20T22:35:44Z

add kernel mount option ms_mode=secure to cephfs storageclass data when encryption in transit is enabled

openshift-ci · 2024-07-20T22:35:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rohan47
Once this PR has been reviewed and has the lgtm label, please assign jarrpa 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

openshift-ci · 2024-07-20T22:35:55Z

Hi @rohan47. Thanks for your PR.

I'm waiting for a red-hat-storage 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.

Madhu-1 · 2024-07-22T14:28:24Z

services/provider/server/server.go

@@ -769,3 +776,25 @@ func (s *OCSProviderServer) getOCSSubscriptionChannel(ctx context.Context) (stri
 	}
 	return subscription.Spec.Channel, nil
 }
+
+func (s *OCSProviderServer) getCephfsKernelMountOptions(ctx context.Context) (string, error) {


we already have

ocs-operator/controllers/storagecluster/csi_options.go

Lines 9 to 33 in 9200d55

func getCephFSKernelMountOptions(sc *ocsv1.StorageCluster) string {

// If Encryption is enabled, Always use secure mode

if sc.Spec.Network != nil && sc.Spec.Network.Connections != nil &&

sc.Spec.Network.Connections.Encryption != nil && sc.Spec.Network.Connections.Encryption.Enabled {

return "ms_mode=secure"

}

// If Encryption is not enabled, but Compression or RequireMsgr2 is enabled, use prefer-crc mode

if sc.Spec.Network != nil && sc.Spec.Network.Connections != nil &&

((sc.Spec.Network.Connections.Compression != nil && sc.Spec.Network.Connections.Compression.Enabled) ||

sc.Spec.Network.Connections.RequireMsgr2) {

return "ms_mode=prefer-crc"

}

// Network spec always has higher precedence even in the External or Provider cluster. so they are checked first above

// None of Encryption, Compression, RequireMsgr2 are enabled on the StorageCluster

// If it's an External or Provider cluster, We don't require msgr2 by default so no mount options are needed

if sc.Spec.ExternalStorage.Enable || sc.Spec.AllowRemoteStorageConsumers {

return "ms_mode=legacy"

}

// If none of the above cases apply, We set RequireMsgr2 true by default on the cephcluster

// so we need to set the mount options to prefer-crc

return "ms_mode=prefer-crc"

}

just adjust it as per the requirement?

moved getCephFSKernelMountOptions function from storagecluster package to util package Signed-off-by: Rohan Gupta <rohgupta@redhat.com>

rohan47 · 2024-07-22T16:44:18Z

Testing the code changes

add kernel mount option ms_mode=secure to cephfs storageclass data when encryption in transit is enabled Signed-off-by: Rohan Gupta <rohgupta@redhat.com>

rohan47 · 2024-07-22T20:29:55Z

Tested the code changes

Csi config after the changes

oc get cm ceph-csi-configs -o yaml
apiVersion: v1
data:
  config.json: '[{"clusterID":"81fd58def37a35d1b4d4bee7ba35c3c3","storageClientID":"fb3e4f11-a38b-4859-ad18-1e8d579eb72b",
"monitors":["10.0.16.90:3300","10.0.61.203:3300","10.0.59.136:3300"],"rbd":{"radosNamespace":"cephradosnamespace-f6e94668817e4eefeb19b7a402fd947e"}},{"clusterID":"fe09c94a50c72d033da43478c21b8314",
"storageClientID":"fb3e4f11-a38b-4859-ad18-1e8d579eb72b","monitors":["10.0.16.90:3300","10.0.61.203:3300","10.0.59.136:3300"],
"cephFS":{"subvolumeGroup":"cephfilesystemsubvolumegroup-38beca789f5bc10e556b9832fd1070f6",
"kernelMountOptions":"ms_mode=secure"}}]'
kind: ConfigMap
metadata:
  creationTimestamp: "2024-07-22T19:24:10Z"
  name: ceph-csi-configs
  namespace: openshift-storage
  ownerReferences:
  - apiVersion: v1
    blockOwnerDeletion: true
    controller: true
    kind: ConfigMap
    name: ocs-client-operator-config
    uid: 664e804f-fc77-4aef-9387-a3c39dcb0b00
  resourceVersion: "3135818"
  uid: 7dee1733-974c-48f3-9cc7-7dbab17aee62

nb-ohad · 2024-07-22T20:48:07Z

services/provider/server/server.go

+			}
+			kernelMountOptions := util.GetCephFSKernelMountOptions(&storageClusters.Items[0])
+			if kernelMountOptions == kernelMountOptionSecure {
+				cephfsStorageClassData[kernelMountOptionsKey] = kernelMountOptionSecure


We should not piggyback csi config info onto the storage class resource.
CSI config needs to be returned as its own resource

leelavg

In the generated ceph config, shouldn't it also contain mons w/ 6789 port for ceph client to communicate w/o encryption?

leelavg · 2024-07-23T03:04:18Z

services/provider/server/server.go

+			if err != nil || len(storageClusters.Items) == 0 {
+				return nil, status.Errorf(codes.Internal, "failed to get storage cluster %v", err)
+			}
+			kernelMountOptions := util.GetCephFSKernelMountOptions(&storageClusters.Items[0])


how are you getting ms_mode=secure as mentioned in the test comment as the function will return ms_mode=legacy if remote consumers are allowed? Is it due to sc.Spec.Network.Connections.Encryption.Enabled?

Yes if sc.Spec.Network.Connections.Encryption.Enabled then it will return ms_mode=secure

agarwal-mudit · 2024-07-23T07:59:48Z

/ok-to-test

openshift-ci · 2024-07-23T09:45:19Z

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

Test name	Commit	Details	Required	Rerun command
ci/prow/ocs-operator-bundle-e2e-aws	`8ae40d6`	link	true	`/test ocs-operator-bundle-e2e-aws`

Full PR test history. Your PR dashboard.

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.

rohan47 · 2024-07-23T10:08:30Z

In the generated ceph config, shouldn't it also contain mons w/ 6789 port for ceph client to communicate w/o encryption?

In Case EiT is enabled on storagecluster all the clients are supposed to use 3300 right as mons will only listen on secure port?

leelavg · 2024-07-23T10:11:37Z

controllers/storagecluster/csi_options.go

#2707 (comment)

continuing the discussion on PR rather than as a comment. So, does the kernel client on client cluster continues to offer services for PVCs which were created before enabling EiT?

for 4.17 deployments the decision to use EiT will be taken when the provider is deployed right and we don't have to worry about clients connecting on unsecure port

Well, rather than asking the specifics, let me put it this way, w/ this PR does the PVCs created on 4.16 still work?

No, It won't work
4.16-based deployments will not support EiT even after they upgrade to 4.17 right?

leelavg · 2024-08-09T09:51:13Z

included in #2713 pls check & confirm.

openshift-merge-robot · 2024-08-12T16:42:36Z

PR needs rebase.

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.

openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 20, 2024

rohan47 marked this pull request as draft July 20, 2024 23:00

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 Jul 20, 2024

rohan47 mentioned this pull request Jul 22, 2024

controllers: kernel mount options for cephfs Encryption in transit red-hat-storage/ocs-client-operator#185

Closed

rohan47 force-pushed the enc_in_transit_provider branch from 3e165cc to 9f21e7a Compare July 22, 2024 14:11

Madhu-1 reviewed Jul 22, 2024

View reviewed changes

rohan47 force-pushed the enc_in_transit_provider branch 5 times, most recently from f46aaba to 43eb71d Compare July 22, 2024 16:29

moved getCephFSKernelMountOptions from storagecluster to util

4f1ee30

moved getCephFSKernelMountOptions function from storagecluster package to util package Signed-off-by: Rohan Gupta <rohgupta@redhat.com>

rohan47 force-pushed the enc_in_transit_provider branch 2 times, most recently from 3c608a8 to c4b98ec Compare July 22, 2024 16:42

rohan47 marked this pull request as ready for review July 22, 2024 16:43

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 Jul 22, 2024

provider-server: send info of kernelMountOptions for cephfs to client

8ae40d6

add kernel mount option ms_mode=secure to cephfs storageclass data when encryption in transit is enabled Signed-off-by: Rohan Gupta <rohgupta@redhat.com>

rohan47 force-pushed the enc_in_transit_provider branch from c4b98ec to 8ae40d6 Compare July 22, 2024 20:13

nb-ohad requested changes Jul 22, 2024

View reviewed changes

openshift-ci bot assigned nb-ohad Jul 22, 2024

leelavg reviewed Jul 23, 2024

View reviewed changes

openshift-ci bot 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 Jul 23, 2024

leelavg reviewed Jul 23, 2024

View reviewed changes

rohan47 marked this pull request as draft July 24, 2024 14:12

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 Jul 24, 2024

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider-server: send info of kernelMountOptions for cephfs to client when encryption in transit is enabled #2707

provider-server: send info of kernelMountOptions for cephfs to client when encryption in transit is enabled #2707

rohan47 commented Jul 20, 2024

openshift-ci bot commented Jul 20, 2024

openshift-ci bot commented Jul 20, 2024

Madhu-1 Jul 22, 2024

rohan47 commented Jul 22, 2024

rohan47 commented Jul 22, 2024

nb-ohad Jul 22, 2024

leelavg left a comment

leelavg Jul 23, 2024

rohan47 Jul 23, 2024

agarwal-mudit commented Jul 23, 2024

openshift-ci bot commented Jul 23, 2024

rohan47 commented Jul 23, 2024

leelavg Jul 23, 2024

rohan47 Jul 23, 2024

leelavg Jul 23, 2024

rohan47 Jul 23, 2024

leelavg commented Aug 9, 2024

openshift-merge-robot commented Aug 12, 2024

	func getCephFSKernelMountOptions(sc *ocsv1.StorageCluster) string {
	// If Encryption is enabled, Always use secure mode
	if sc.Spec.Network != nil && sc.Spec.Network.Connections != nil &&
	sc.Spec.Network.Connections.Encryption != nil && sc.Spec.Network.Connections.Encryption.Enabled {
	return "ms_mode=secure"
	}

	// If Encryption is not enabled, but Compression or RequireMsgr2 is enabled, use prefer-crc mode
	if sc.Spec.Network != nil && sc.Spec.Network.Connections != nil &&
	((sc.Spec.Network.Connections.Compression != nil && sc.Spec.Network.Connections.Compression.Enabled) \|\|
	sc.Spec.Network.Connections.RequireMsgr2) {
	return "ms_mode=prefer-crc"
	}

	// Network spec always has higher precedence even in the External or Provider cluster. so they are checked first above

	// None of Encryption, Compression, RequireMsgr2 are enabled on the StorageCluster
	// If it's an External or Provider cluster, We don't require msgr2 by default so no mount options are needed
	if sc.Spec.ExternalStorage.Enable \|\| sc.Spec.AllowRemoteStorageConsumers {
	return "ms_mode=legacy"
	}
	// If none of the above cases apply, We set RequireMsgr2 true by default on the cephcluster
	// so we need to set the mount options to prefer-crc
	return "ms_mode=prefer-crc"
	}

provider-server: send info of kernelMountOptions for cephfs to client when encryption in transit is enabled #2707

Are you sure you want to change the base?

provider-server: send info of kernelMountOptions for cephfs to client when encryption in transit is enabled #2707

Conversation

rohan47 commented Jul 20, 2024

openshift-ci bot commented Jul 20, 2024

openshift-ci bot commented Jul 20, 2024

Choose a reason for hiding this comment

rohan47 commented Jul 22, 2024

rohan47 commented Jul 22, 2024

Choose a reason for hiding this comment

leelavg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agarwal-mudit commented Jul 23, 2024

openshift-ci bot commented Jul 23, 2024

rohan47 commented Jul 23, 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

leelavg commented Aug 9, 2024

openshift-merge-robot commented Aug 12, 2024