Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions controllers/storagecluster/cephcluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ func newCephCluster(sc *ocsv1.StorageCluster, cephImage string, kmsConfigMap *co
CSI: rookCephv1.CSIDriverSpec{
ReadAffinity: getReadAffinityOptions(sc),
CephFS: rookCephv1.CSICephFSSpec{
KernelMountOptions: getCephFSKernelMountOptions(sc),
KernelMountOptions: statusutil.GetCephFSKernelMountOptions(sc),
},
},
SkipUpgradeChecks: sc.Spec.ManagedResources.CephCluster.SkipUpgradeChecks,
Expand Down Expand Up @@ -666,7 +666,7 @@ func newExternalCephCluster(sc *ocsv1.StorageCluster, monitoringIP, monitoringPo
CSI: rookCephv1.CSIDriverSpec{
ReadAffinity: getReadAffinityOptions(sc),
CephFS: rookCephv1.CSICephFSSpec{
KernelMountOptions: getCephFSKernelMountOptions(sc),
KernelMountOptions: statusutil.GetCephFSKernelMountOptions(sc),
},
},
},
Expand Down
27 changes: 0 additions & 27 deletions controllers/storagecluster/csi_options.go
Copy link
Contributor

Choose a reason for hiding this comment

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

#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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
Expand Up @@ -5,33 +5,6 @@ import (
rookCephv1 "github.com/rook/rook/pkg/apis/ceph.rook.io/v1"
)

// getCephFSKernelMountOptions returns the kernel mount options for CephFS based on the spec on the StorageCluster
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"
}

// getReadAffinityyOptions returns the read affinity options based on the spec on the StorageCluster.
func getReadAffinityOptions(sc *ocsv1.StorageCluster) rookCephv1.ReadAffinitySpec {
if sc.Spec.CSI != nil && sc.Spec.CSI.ReadAffinity != nil {
Expand Down
13 changes: 13 additions & 0 deletions controllers/util/clusters.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,3 +157,16 @@ func (c *Clusters) HasMultipleStorageClustersWithSameName(name string) bool {

return count > 1
}

func GetStorageClustersInNamespace(ctx context.Context, cli client.Client, namespace string) (ocsv1.StorageClusterList, error) {
storageClusters := ocsv1.StorageClusterList{}
listOpts := []client.ListOption{
client.InNamespace(namespace),
}
err := cli.List(ctx, &storageClusters, listOpts...)
if err != nil || len(storageClusters.Items) == 0 {
return storageClusters, err
}

return storageClusters, nil
}
32 changes: 32 additions & 0 deletions controllers/util/csi_options.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package util

import (
ocsv1 "github.com/red-hat-storage/ocs-operator/api/v4/v1"
)

// GetCephFSKernelMountOptions returns the kernel mount options for CephFS based on the spec on the StorageCluster
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"
}
7 changes: 7 additions & 0 deletions deploy/ocs-operator/manifests/provider-role.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,10 @@ rules:
verbs:
- get
- list
- apiGroups:
- ocs.openshift.io
resources:
- storageclusters
verbs:
- get
- list
7 changes: 7 additions & 0 deletions rbac/provider-role.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,10 @@ rules:
verbs:
- get
- list
- apiGroups:
- ocs.openshift.io
resources:
- storageclusters
verbs:
- get
- list
20 changes: 18 additions & 2 deletions services/provider/server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (

"github.com/blang/semver/v4"
quotav1 "github.com/openshift/api/quota/v1"
ocsv1 "github.com/red-hat-storage/ocs-operator/api/v4/v1"
"github.com/red-hat-storage/ocs-operator/api/v4/v1alpha1"
ocsv1alpha1 "github.com/red-hat-storage/ocs-operator/api/v4/v1alpha1"
controllers "github.com/red-hat-storage/ocs-operator/v4/controllers/storageconsumer"
Expand Down Expand Up @@ -55,8 +56,10 @@ const (
)

const (
monConfigMap = "rook-ceph-mon-endpoints"
monSecret = "rook-ceph-mon"
monConfigMap = "rook-ceph-mon-endpoints"
monSecret = "rook-ceph-mon"
kernelMountOptionsKey = "kernelmountoptions"
kernelMountOptionSecure = "ms_mode=secure"
)

type OCSProviderServer struct {
Expand Down Expand Up @@ -238,6 +241,10 @@ func newClient() (client.Client, error) {
if err != nil {
return nil, fmt.Errorf("failed to add operatorsv1alpha1 to scheme. %v", err)
}
err = ocsv1.AddToScheme(scheme)
if err != nil {
return nil, fmt.Errorf("failed to add ocsv1 to scheme. %v", err)
}

config, err := config.GetConfig()
if err != nil {
Expand Down Expand Up @@ -687,6 +694,15 @@ func (s *OCSProviderServer) GetStorageClaimConfig(ctx context.Context, req *pb.S
"csi.storage.k8s.io/controller-expand-secret-name": provisionerSecretName,
}

storageClusters, err := util.GetStorageClustersInNamespace(ctx, s.client, s.namespace)
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])
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if kernelMountOptions == kernelMountOptionSecure {
cephfsStorageClassData[kernelMountOptionsKey] = kernelMountOptionSecure
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

extR = append(extR,
&pb.ExternalResource{
Name: "cephfs",
Expand Down
9 changes: 9 additions & 0 deletions services/provider/server/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"testing"

quotav1 "github.com/openshift/api/quota/v1"
ocsv1 "github.com/red-hat-storage/ocs-operator/api/v4/v1"
ocsv1alpha1 "github.com/red-hat-storage/ocs-operator/api/v4/v1alpha1"
controllers "github.com/red-hat-storage/ocs-operator/v4/controllers/storageconsumer"
pb "github.com/red-hat-storage/ocs-operator/v4/services/provider/pb"
Expand Down Expand Up @@ -757,6 +758,13 @@ func TestOCSProviderServerGetStorageClaimConfig(t *testing.T) {
Phase: ocsv1alpha1.StorageRequestFailed,
},
}
storageClusterResourceName = "mock-storage-cluster"
storageClustersResource = &ocsv1.StorageCluster{
ObjectMeta: metav1.ObjectMeta{
Name: storageClusterResourceName,
Namespace: serverNamespace,
},
}
)

ctx := context.TODO()
Expand All @@ -767,6 +775,7 @@ func TestOCSProviderServerGetStorageClaimConfig(t *testing.T) {
claimResourceInitializing,
claimResourceCreating,
claimResourceFailed,
storageClustersResource,
}

// Create a fake client to mock API calls.
Expand Down
Loading