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

Use HostToContainer mount propagation instead of /host/proc/mounts #321

Merged
merged 10 commits into from
Dec 20, 2024

Conversation

jiaeenie
Copy link
Contributor

Description of changes:
Currently, we spawn Mountpoint processes on the host using systemd. As a result, the mounts created by Mountpoint are not visible inside the CSI Driver Pod. To work around this, we were mounting /proc/mounts from host and parsing this file to check existing mounts on the host. Mounting /proc/mounts causes problems with Karpenter sometimes and its also its blocked by some SELinux policies. Such as this issue.

This commit instead uses HostToContainer mount propagation on hostPath mount for /var/lib/kubelet. Thanks to HostToContainer, any new mounts created inside /var/lib/kubelet gets automatically propagated to our Pod from the host.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

unexge and others added 3 commits December 10, 2024 15:17
Currently, we spawn Mountpoint processes on the host using systemd. As
a result, the mounts created by Mountpoint are not visible inside the CSI
Driver Pod. To work around this, we were mounting `/proc/mounts` from
host and parsing this file to check existing mounts on the
host. Mounting `/proc/mounts` causes problems with Karpenter sometimes
and its also its blocked by some SELinux policies.

This commit instead uses `Bidirectional` mount propagation on
`hostPath` mount for `/var/lib/kubelet`. Thanks to `Bidirectional`,
any new mounts created inside `/var/lib/kubelet` gets automatically
propagated to our Pod from the host, and also vice versa - which will
be useful once we stop using systemd hack and doing mounts inside the
CSI Driver Pods.

Signed-off-by: Burak Varlı <[email protected]>
Kubernetes only allows using `Bidirectional` mount propagation on
privileged containers. But making a container privileged causes
containerd to reset container's SELinux labels, and that causes
problems with Bottlerocket.

Because we spawn Mountpoint processes on the host by using systemd,
Bottlerocket's SELinux policies by default denies that. In order to
overcome this limitation we're using `super_t` SELinux label in our
containers which then they become "superpowered" containers and
bypasses the limitation.

So, if we make our containers privileged, due to containerd's
behaviour we lose our `super_t` SELinux label and Bottlerocket blocks
our container to spawn systemd processes.

Signed-off-by: Burak Varlı <[email protected]>
@jiaeenie jiaeenie requested a review from unexge December 16, 2024 15:59
@jiaeenie jiaeenie changed the title UseHostToContainer mount propagation instead of /host/proc/mounts Use HostToContainer mount propagation instead of /host/proc/mounts Dec 16, 2024
@jiaeenie jiaeenie requested a review from muddyfish December 16, 2024 16:09
@jiaeenie jiaeenie requested a review from unexge December 16, 2024 17:11
@unexge
Copy link
Contributor

unexge commented Dec 17, 2024

The change looks good to me. One additional thing we should do is to ensure targetPath passed in NodePublishVolume is inside kubelet path. Because we rely on propagation of mounts under kubelet path, any other mount wouldn't be propagated to our Pod. This shouldn't be the case, but it would be good to emit an error just in case. Something like that in S3NodeServer.NodePublishVolume:

const defaultKubeletPath = "/var/lib/kubelet"

var kubeletPath = getKubeletPath()

func getKubeletPath() string {
	path := os.Getenv("KUBELET_PATH")
	if path == "" {
		return defaultKubeletPath
	}
	return path
}

// ...

func (ns *S3NodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublishVolumeRequest) (*csi.NodePublishVolumeResponse, error) {
        // ...

	if !filepath.HasPrefix(target, kubeletPath) {
		klog.Errorf("NodePublishVolume: target path %q is not in kubelet path %q. This might cause mounting issues, please ensure you have correct kubelet path configured.", req.GetTargetPath(), kubeletPath)
	}
}

@jiaeenie jiaeenie requested a review from muddyfish December 18, 2024 11:04
pkg/driver/node/node.go Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@unexge
Copy link
Contributor

unexge commented Dec 19, 2024

The CI failure is unrelated, "Delete cluster" step doesn't handle "NotFound" errors currently and tries to remove some resources for maximum number of retries configured which takes around ~20min and that causes temporary credentials to expire and operations to fail.

I've restarted the failed tasks and will submit a separate PR to fix the CI.

Copy link
Contributor

@unexge unexge left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @jiaeenie!

@unexge unexge merged commit 1dc5e4d into main Dec 20, 2024
21 of 22 checks passed
@unexge unexge deleted the feature/dont-mount-proc-mounts-from-host branch December 20, 2024 11:19
@UnyieldingOrca
Copy link

Can you make a patch release for this fix? Or is there a way to deploy a prerelease version of the csi driver myself?

@unexge
Copy link
Contributor

unexge commented Jan 15, 2025

Hey @UnyieldingOrca, we plan to do a release soon, but we don't have any date we can share at the moment. We'll be sharing updates in this ticket: #284.

Regarding deploying a pre-release version, you'd first need to build our Docker image by using our Makefile, and push it to your registry, and then install our Helm chart from source by specifying your registry.

helm upgrade --install aws-mountpoint-s3-csi-driver \
    --namespace kube-system \
    ... \
    --set image.repository="1234567890.dkr.ecr.eu-north-1.amazonaws.com/your-registry" \
    --set image.tag=your-tag \
   ./charts/aws-mountpoint-s3-csi-driver

Unfortunately, it's not trivial at the moment.

@UnyieldingOrca
Copy link

Thanks for the guidance. I ended up following this and it fixed my issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants