-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Quadlet - make sure the /etc/containers/systemd/users is traversed in rootless #24815
base: main
Are you sure you want to change the base?
Conversation
… rootless Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ygalblum The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I'm not sure how to test this. The problem is that creating folders under |
That's usually done via env vars. Could you use |
I don't think this can help here, it will not trigger the code path in question here as this does not change the root. I think for testing it may be possible to run quadlet in a container where we can have full control over all dirs or just chroot() may be enough? But it still is no clear to me how we could integrate this into the existing e2e test. |
Yes, @Luap99 is correct. When |
Containerized e2e could be a safe way to test, but not trivial (test fixtures would need to be set up outside the test itself). And, sorry, I can't remember if we run rootless containerized. |
Maybe in the e2e test instead of calling quadlet on the host we can wrap it in a container like this:
Where /tmp/test is the source of the quadlet files, that seems to work and we have full control where to place the files. |
I've tried running this containerized, but I still see issues. While I would like to explore the idea, can we still merge this PR (by skipping the new tests requirement) as it does fix a real issue? |
Does this PR introduce a user-facing change?
No
Resolves: #24783