Fix parsing of paths for unmask #25660
Conversation
There was a problem hiding this comment.
Thanks, please add a test for it.
Overall I hate such string.Split() calls for paths because it means all users will be unable to specify paths with a colon in them. Given it is is already done with mask I guess it is good to be consistent so I am fine with it.
|
I'll try, but I'm not exactly sure what's going on. The tests currently test for this and that does work, both paths are masked by default. But when I try adding paths that are read-only by default, it doesn't effect them. I don't understand how the parsing changes that in any way. I don't understand this behavior, I guess there could be two tests, one for masked paths and one for read-only paths, but it's strange. I'll double check this later when I have more time. |
|
I still don't know why some paths worked, but I've added a simple test with one that I can reproduce doesn't.
Agree, it's not pretty, I did it this way for consistency and also because I know absolutely no GO :) |
test/e2e/run_test.go
Outdated
| @@ -423,6 +423,9 @@ var _ = Describe("Podman run", func() { | |||
| session.WaitWithDefaultTimeout() | |||
| Expect(session.OutputToString()).To(Not(BeEmpty())) | |||
| Expect(session).Should(ExitCleanly()) | |||
| session = podmanTest.Podman([]string{"exec", "maskCtr2", "mkdir", "/sys/fs/cgroup/testcgroup"}) | |||
There was a problem hiding this comment.
this won't work with rootless and cgroupfs because the user doesn't get a delegated cgroup.
It would be better to run something like grep /sys/fs/cgroup /proc/self/mountinfo and make sure there is rw in the mount flags.
|
LGTM once comment is addressed |
|
A friendly reminder that this PR had no activity for 30 days. |
|
@ver4a would you be willing to push this over the finish line? much appreciated if so. |
This fixes an issue where multiple paths separated by a colon were treated as a single path, contrary to what docs say and unlike how mask option works. Test was updated with a case that fails without this commit. Signed-off-by: Šimon Škoda <ver4a@uncontrol.me>
|
Sorry for the delay, I got distracted with other stuff. Could you please take another look at this @giuseppe? I've signed-off the commit with my legal name and made a new test based on your suggestion. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, ver4a 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 |
openshift-ci
bot
added
the
approved
This fixes an issue where multiple paths separated by a colon were treated as a single path, contrary to what docs say and unlike how mask option works.
Does this PR introduce a user-facing change?