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

Issue 8249: disable selinux relabel for backupPod #8250

Merged
merged 1 commit into from
Sep 27, 2024

Conversation

Lyndon-Li
Copy link
Contributor

Fix issue #8249, disable selinux relabel for backupPod of data mover

@Lyndon-Li Lyndon-Li added the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Sep 27, 2024
@Lyndon-Li Lyndon-Li marked this pull request as ready for review September 27, 2024 06:09
Copy link

codecov bot commented Sep 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.21%. Comparing base (025d66d) to head (e42de2b).
Report is 8 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8250   +/-   ##
=======================================
  Coverage   59.20%   59.21%           
=======================================
  Files         367      367           
  Lines       30838    30844    +6     
=======================================
+ Hits        18259    18265    +6     
  Misses      11119    11119           
  Partials     1460     1460           

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

@Lyndon-Li
Copy link
Contributor Author

Lyndon-Li commented Sep 27, 2024

@sseago
We may still have problem even for this PR.
From here https://kubernetes.io/docs/concepts/security/pod-security-standards/ we can see that setting spec.securityContext.seLinuxOptions.type is a restricted operation and spc_t is not an allowed type.
Therefore, if the pod security policy is set to Restricted in a cluster, the pod will not be able to start.

This echos the first question in my comment here #8243 (comment).
Therefore, I am afraid setting spc_t type is still not a safe solution.

Please hold on merging this PR until this question is clear.

@sseago
Copy link
Collaborator

sseago commented Sep 27, 2024

Testing on OpenShift was successful with both filesystem and block volumes.

@sseago sseago merged commit 0ccdc7c into vmware-tanzu:main Sep 27, 2024
50 of 51 checks passed
@sseago
Copy link
Collaborator

sseago commented Sep 27, 2024

@Lyndon-Li oops. Sorry, I already merged just as your comment was being posted.

@sseago
Copy link
Collaborator

sseago commented Sep 27, 2024

Should we revert?

kaovilai added a commit to kaovilai/velero that referenced this pull request Sep 27, 2024
kaovilai added a commit to kaovilai/velero that referenced this pull request Sep 27, 2024
@sseago
Copy link
Collaborator

sseago commented Sep 27, 2024

@Lyndon-Li So I think the only path forward is a configurable option. If selinux is enabled, then we either need spc_t or we need to remove that readOnly=true. Lets add a new installer/node-agent flag to configure this behavior. 3 options:

  1. no change from current behavior -- this will be the default, won't work for selinux envs
  2. Add spc_t selinux option -- preferred behavior for selinux env unless they're running with Restricted pods
  3. Remove spec.volumes readOnly field -- required if selinux is enabled and pods are Restricted, but might break shallow copy.

@sseago
Copy link
Collaborator

sseago commented Sep 27, 2024

also, we'll be reverting this merge with #8253

@Lyndon-Li
Copy link
Contributor Author

Lyndon-Li commented Sep 27, 2024

also, we'll be reverting this merge with #8253

Thanks!

sseago pushed a commit that referenced this pull request Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants