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.

Sign up for GitHub

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

Jump to bottom

Ceph FS fscrypt support #3460

Merged
merged 12 commits into from
Nov 23, 2022
Merged

Ceph FS fscrypt support #3460

merged 12 commits into from
Nov 23, 2022

Conversation

irq0
Copy link
Contributor

@irq0 irq0 commented Oct 20, 2022

Add Ceph FS fscrypt support. Supports volumes, snapshots and clones.

Follow up to #3310 (adds fscrypt integration) and second in series towards #1563

Fixes: #1563

Usage Requirements

End-to-end Testing

Testing on minikube requires a custom ISO with the latest Ceph FS patches. To test snapshots and clones a main branch build of Ceph is needed as well.

Custom Minikube ISO

Use patches from https://github.com/irq0/minikube/tree/custom-ceph-fscrypt-kernel on top of minikube to build an ISO with the current Ceph FS development kernel.

Build summary:

make
make buildroot-image
make out/minikube-x86_64.iso

Then use minikube.sh with MINIKUBE_ISO_URL="file://path of minikube.iso"

Use Ceph Main Branch Images

Either use quay.io/ceph/daemon:latest-main-devel or build using ceph-containers project.

Build

In https://github.com/ceph/ceph-container.git clone:

eval $(minikube docker-env)  # will build inside minikube VM
make BASEOS_REGISTRY=quay.io/centos BASEOS_TAG=stream8 CEPH_DEVEL=true FLAVORS="main,centos,8" build

Optional: patch image with ceph/ceph#48410

@irq0
Copy link
Contributor Author

irq0 commented Oct 20, 2022

Test last push addresses the linter errors.

The failure in ci/centos/upgrade-tests-cephfs is interesting:

The updated deploy/cephfs/* files now have the ceph-csi-encryption-kms-config configmaps attached as volumes (like their RBD counterparts). My read is that the update tests fail, because they are not present. What is the best course of action here? Add the encryption related configmaps to the update path? Fork the file into regular and encryption enabled?

@nixpanic nixpanic added enhancement New feature or request component/cephfs Issues related to CephFS labels Oct 20, 2022
@Madhu-1
Copy link
Collaborator

Madhu-1 commented Oct 21, 2022

ceph-csi-encryption-kms-config configmaps attached as volumes (like their RBD counterparts). My read is that the update tests fail, because they are not present.

You can set https://github.com/ceph/ceph-csi/blob/devel/e2e/cephfs.go#L73 to true so that it will continue if the file is not present

@irq0
Copy link
Contributor Author

irq0 commented Oct 24, 2022

ceph-csi-encryption-kms-config configmaps attached as volumes (like their RBD counterparts). My read is that the update tests fail, because they are not present.

You can set https://github.com/ceph/ceph-csi/blob/devel/e2e/cephfs.go#L73 to true so that it will continue if the file is not present

Adding deployVault (just like the RBD suites have) to the tests did the trick. It configures the ceph-csi-encryption-kms-config required by the cephfs provisioner / nodeserver

@Madhu-1 Madhu-1 mentioned this pull request Nov 2, 2022
Merged
7 tasks
Madhu-1
Madhu-1 reviewed Nov 7, 2022
View reviewed changes
Copy link
Collaborator

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

Changes look good, with some minor nits. Can you point to some document that can help us to test this feature on local systems (ceph image, kernel version to use), etc?

deploy/cephfs/kubernetes/csi-nodeplugin-rbac.yaml
@@ -3,3 +3,46 @@ apiVersion: v1
kind: ServiceAccount
metadata:
name: cephfs-csi-nodeplugin
namespace: default
Copy link
Collaborator

Choose a reason for hiding this comment

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

use namespace: {{ .Release.Namespace }} here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The RBD analogue doesn't have that construct. It is for use in the helm templates, not?

deploy/cephfs/kubernetes/csi-nodeplugin-rbac.yaml Outdated Show resolved Hide resolved
deploy/cephfs/kubernetes/csi-nodeplugin-rbac.yaml Outdated Show resolved Hide resolved
deploy/cephfs/kubernetes/csi-provisioner-rbac.yaml Outdated Show resolved Hide resolved
internal/cephfs/controllerserver.go Outdated Show resolved Hide resolved
internal/cephfs/nodeserver.go Outdated Show resolved Hide resolved
internal/cephfs/nodeserver.go Outdated Show resolved Hide resolved
internal/cephfs/nodeserver.go Outdated Show resolved Hide resolved
internal/cephfs/nodeserver.go Outdated Show resolved Hide resolved
internal/cephfs/nodeserver.go
@@ -118,6 +119,40 @@ func validateSnapshotBackedVolCapability(volCap *csi.VolumeCapability) error {
return nil
}

// maybeUnlockFileEncryption unlocks fscrypt on stagingTargetPath, iff volOptions enable encryption.
func maybeUnlockFileEncryption(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any specific reason to start with maybe for both functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both functions only do things if encryption is enabled. The 'maybe' tried to convey that the function may not do anything at all.

@irq0
Copy link
Contributor Author

irq0 commented Nov 8, 2022

Can you point to some document that can help us to test this feature on local systems (ceph image, kernel version to use), etc?

I uploaded a minikube ISO with the current development Ceph FS fscrypt kernel that might be helpful: https://github.com/irq0/minikube/releases

You can test most changes by setting the following when calling scripts/minikube.sh

ROOK_CEPH_CLUSTER_IMAGE=quay.io/ceph/daemon:latest-main-devel
MINIKUBE_ISO_URL="https://github.com/irq0/minikube/releases/download/2022-11-08-master%2Bfscrypt-kernel/minikube-amd64.iso"

My helper scripts are in https://github.com/irq0/dev-ceph-csi-fscrypt-config
Calling kube-deploy-cephcsi from there sets up ceph csi and a deployment using an already set up rook.

@irq0
Copy link
Contributor Author

irq0 commented Nov 8, 2022

Push: Rebase to devel and changes for the comments above

nixpanic
nixpanic approved these changes Nov 18, 2022
View reviewed changes
Copy link
Member

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

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

Just a few nits, nothing that requires correcting in my opinion. I'm a little sad I can't easily test this out. As long as it does not break anything, we should be good for now.

Ideally we start running weekly tests with a non-standard kernel config (minkube VM?) and Ceph release that contains support for fscrypt.

e2e/cephfs.go
for kmsID, kmsConf := range kmsToTest {
kmsID := kmsID
kmsConf := kmsConf
By("create a storageclass with pool and an encrypted PVC then bind it to an app with "+kmsID, func() {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not happy with the extreme indention here... But I also don't immediately see how this can be cleanly simplified.

internal/cephfs/store/volumeoptions.go
@@ -210,6 +210,7 @@ func fmtBackingSnapshotOptionMismatch(optName, expected, actual string) error {

// NewVolumeOptions generates a new instance of volumeOptions from the provided
// CSI request parameters.
// nolint:gocyclo,cyclop // TODO: reduce complexity
Copy link
Member

Choose a reason for hiding this comment

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

this probably should be squashed in the commit that added the InitKMS() and/or other calls?

deploy/cephfs/kubernetes/csi-nodeplugin-rbac.yaml
@@ -10,10 +10,6 @@ apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: cephfs-csi-nodeplugin
rules:
Copy link
Member

Choose a reason for hiding this comment

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

This is a nice cleanup, and could have been a PR by itself.

@irq0
Copy link
Contributor Author

irq0 commented Nov 21, 2022

Just a few nits, nothing that requires correcting in my opinion. I'm a little sad I can't easily test this out. As long as it does not break anything, we should be good for now.

Thanks! Me too. It would be awesome to get at least some CI coverage of the Ceph FS fscrypt path.

Ideally we start running weekly tests with a non-standard kernel config (minkube VM?) and Ceph release that contains support for fscrypt.

A minikube VM is probably the best option. Comment #3460 (comment) has some details on how.

I think a custom ISO is the only way to go for the foreseeable future, as minikube kernels tend to be behind and fscrypt kernel support isn't (fully) merged yet.

@irq0 irq0 requested a review from Madhu-1 November 21, 2022 13:28
@Madhu-1 Madhu-1 added the ok-to-test Label to trigger E2E tests label Nov 21, 2022
@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.23

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.24

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.25

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.23

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.25

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.23

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.25

@github-actions
Copy link

/test ci/centos/upgrade-tests-cephfs

@github-actions
Copy link

/test ci/centos/upgrade-tests-rbd

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Nov 21, 2022

/test ci/centos/upgrade-tests-cephfs

@humblec
Copy link
Collaborator

humblec commented Nov 21, 2022

@irq0 thanks for pulling it ! lgtm.. its also good to add a user facing documentation for CephFS Fscrypt. It could be a kind of placeholder doc with disclaimers ( required kernel support..etc) too .

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.25

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.23

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.25

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.23

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.25

@github-actions
Copy link

/test ci/centos/upgrade-tests-cephfs

@github-actions
Copy link

/test ci/centos/upgrade-tests-rbd

@nixpanic
Copy link
Member

/retest ci/centos/mini-e2e-helm/k8s-1.24

@mergify mergify bot removed the ok-to-test Label to trigger E2E tests label Nov 22, 2022
@nixpanic
Copy link
Member

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented Nov 23, 2022

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Nov 23, 2022
@mergify mergify bot merged commit 5da977d into ceph:devel Nov 23, 2022
@mergify mergify bot removed the ok-to-test Label to trigger E2E tests label Nov 23, 2022
@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.23

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.24

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.25

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.23

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.25

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.23

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.25

@github-actions
Copy link

/test ci/centos/upgrade-tests-cephfs

@github-actions
Copy link

/test ci/centos/upgrade-tests-rbd

@irq0 irq0 mentioned this pull request Dec 9, 2022
Merged
@tongpu tongpu mentioned this pull request Mar 1, 2023
Merged
7 tasks
@NymanRobin NymanRobin mentioned this pull request Apr 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nixpanic nixpanic nixpanic approved these changes

@humblec humblec humblec approved these changes

@Madhu-1 Madhu-1 Madhu-1 approved these changes

Assignees
No one assigned
Labels
component/cephfs Issues related to CephFS enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable CephFS encryption for CephFS subvolumes
4 participants
@irq0 @Madhu-1 @humblec @nixpanic