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

feat(csidriver): declare seLinuxMount support #598

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

cjorge-graphops
Copy link

Pull Request template

Please, go through these steps before you submit a PR.

Why is this PR required? What issue does it fix?:

Allows zfs-localpv to support efficient SELinux relabeling (https://kubernetes.io/blog/2023/04/18/kubernetes-1-27-efficient-selinux-relabeling-beta/)

What this PR does?:

Adds seLinuxMount: true to the CSIDriver spec, by default.

Does this PR require any upgrade changes?:

No

If the changes in this PR are manually verified, list down the scenarios covered::

I've validated that when all the required conditions are met, kubelet passes an additional mount option for selinux context, which is supported by ZFS with no issues, effectively avoiding the relabeling step on preparing the PVC.

Any additional information for your reviewer? :

Related to #577

Checklist:

  • Fixes Support mounting with SeLinux mount options #577
  • PR Title follows the convention of <type>(<scope>): <subject>
  • Has the change log section been updated?
  • Commit has unit tests
  • Commit has integration tests
  • (Optional) Are upgrade changes included in this PR? If not, mention the issue/PR to track:
  • (Optional) If documentation changes are required, which issue on https://github.com/openebs/website is used to track them:

@sinhaashish sinhaashish self-requested a review November 4, 2024 05:43
@@ -8,3 +8,4 @@ spec:
attachRequired: false
podInfoOnMount: false
storageCapacity: {{ .Values.feature.storageCapacity }}
seLinuxMount: true
Copy link
Member

Choose a reason for hiding this comment

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

Maybe set this via helm variable?

Copy link
Member

Choose a reason for hiding this comment

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

moreover if this is supported in Kubernetes version ≥ 1.27 then why are we setting it to true as default

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to be accepted from 1.25.x, so I suggest this:

Suggested change
seLinuxMount: true
{{- if semverCompare ">= 1.25.x" .Capabilities.KubeVersion.Version }}
seLinuxMount: true
{{- end }}

Comment on lines +1 to +7
---
title: SELinux Mount Support for LocalPV-ZFS
authors:
- "@cjorge-graphops"
creation-date: 2024-11-01
last-updated: 2024-11-01
---
Copy link
Member

Choose a reason for hiding this comment

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

@tiagolobocastro Should this be here?

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean vs the root openebs? Seems we already have pv-migration here in this repo as well.
So basically, going forward do we want to keep designs per repo or on umbrella repo? CC @avishnu

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just keep it here for now.
Let's discuss on the next maintainers call on how to organize these better.

@codecov-commenter
Copy link

codecov-commenter commented Nov 4, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.99%. Comparing base (e3d55d7) to head (36620f7).
Report is 13 commits behind head on develop.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #598      +/-   ##
===========================================
- Coverage    96.37%   95.99%   -0.38%     
===========================================
  Files            1        1              
  Lines          496      574      +78     
===========================================
+ Hits           478      551      +73     
- Misses          14       19       +5     
  Partials         4        4              
Flag Coverage Δ
bddtests 95.99% <ø> (-0.38%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- Pod must specify SELinux level
- PV must be RWOP.

Note: I'm unsure this is necessary or makes sense to document, thoughts?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I think it's good to document the requirements.

@@ -2787,3 +2787,4 @@ spec:
attachRequired: false
podInfoOnMount: false
storageCapacity: true
seLinuxMount: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
seLinuxMount: true
{{- if semverCompare ">= 1.25.x" .Capabilities.KubeVersion.Version }}
seLinuxMount: true
{{- end }}

@@ -8,3 +8,4 @@ spec:
attachRequired: false
podInfoOnMount: false
storageCapacity: {{ .Values.feature.storageCapacity }}
seLinuxMount: true
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to be accepted from 1.25.x, so I suggest this:

Suggested change
seLinuxMount: true
{{- if semverCompare ">= 1.25.x" .Capabilities.KubeVersion.Version }}
seLinuxMount: true
{{- end }}

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.

Support mounting with SeLinux mount options
5 participants