Turn off 'noexec' option by default for named volumes #6280
Conversation
openshift-ci-robot
added
the
approved
|
LGTM |
|
@BenTheElder PTAL |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, mheon, rhatdan 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 |
|
/lgtm |
openshift-ci-robot
added
the
do-not-merge/hold
|
Apparently we have a test verifying that |
|
Looks like the exec/noexec test is still failing. |
|
LGTM assuming happy tests |
|
ACK thanks, small note: In our case we were trying to use anonymous volumes (they're just scratch space, bound to the container lifecycle) and swap docker/podman. Switching to creating volumes and specifying these explicitly in the podman-specific logic and improving detection of podman-docker vs docker is working well for us and should also let us bypass #4276 on older podman versions. |
|
Hit the system tests as well; @edsantiago mind taking a look to make sure this looks OK? |
test/system/160-volumes.bats
Outdated
| @@ -125,13 +126,17 @@ EOF | |||
| expect_msg='.* exec user process caused.*permission denied' | |||
| fi | |||
|
|
|||
| run_podman ${expect_rc} run --rm --volume $myvolume:/vol:z $IMAGE /vol/myscript | |||
| run_podman ${expect_rc} run --rm --volume $myvolume:/vol:noexecz $IMAGE /vol/myscript | |||
There was a problem hiding this comment.
"noexecz"? Should there be a comma or something?
There was a problem hiding this comment.
This is what happens when I push before the tests finish running. Oops.
mheon
force-pushed
the
switch_off_noexec
branch
2 times, most recently
from
a99b01b to
81d2a48
Compare
test/system/160-volumes.bats
Outdated
| # With exec, it should pass | ||
| run_podman run --rm -v $myvolume:/vol:z,exec $IMAGE /vol/myscript | ||
| is "$output" "got here -$rand-" "script in volume is runnable with exec" | ||
|
|
There was a problem hiding this comment.
Sorry for brusqueness, I just wasted too much time typing, then having my comments lost because of outdated diffs, then typing again, then losing them AGAIN DAMN YOU GITHUB. Didn't want to do that again.
Anyhow, my point was, system tests aren't intended to cover every possible situation - they're just a last-minute check for basic functionality. Testing the default is a good enough check for exec; testing noexec, likewise. We don't IMHO need a full test just for parsing 'exec'.
|
"Failed to stop: readHandshakeRecord" in Cirrus ?! |
We previously enforced this for security reasons, but as Dan has explained on several occasions, it's not very valuable there (it's trivially easy to bypass) and it does seriously annoy folks trying to use named volumes. Flip the default from 'on' to 'off'. Signed-off-by: Matthew Heon <[email protected]>
|
oh wow.. this one was obnoxious to track down and not super obvious. All of our CI started breaking. Pretty much compiling anything within a named volume requires exec (autoconf does a bunch of gcc compiler checks which execute test binaries) |
|
/lgtm |
|
@mheon I think this should be back ported to podman 1.9.3 |
|
Sure, will do. Removing hold, things are green |
mheon
removed
the
do-not-merge/hold
github-actions
bot
added
the
locked - please file new issue/PR
We previously enforced this for security reasons, but as Dan has explained on several occasions, it's not very valuable there (it's trivially easy to bypass) and it does seriously annoy folks trying to use named volumes. Flip the default from 'on' to 'off'.