Fix permission on secrets directory #12813
Conversation
openshift-ci
bot
added
the
approved
|
@ashley-cui @containers/podman-maintainers PTAL |
test/system/170-run-userns.bats
Outdated
| run_podman secret create ${test_name} ${secret_file} | ||
| run_podman run --secret=${test_name} --userns=auto:size=1000 $IMAGE cat /run/secrets/${test_name} | ||
| is ${output} ${secret_content} "Secrets should work with user namespace" | ||
| run_podman rm --all --force |
There was a problem hiding this comment.
Why not just run --rm in line 93?
test/system/170-run-userns.bats
Outdated
| egrep -q "^containers:" /etc/subuid || skip "no IDs allocated for user 'containers'" | ||
| fi | ||
| test_name="test_$(random_string 12)" | ||
| secret_file="/tmp/secret$(random_string 12)" |
There was a problem hiding this comment.
If you use secret_file=$PODMAN_TMPDIR/secret$(random_string 12) you don't have to rm -f $secret_file at the end.
| egrep -q "^$(id -un):" /etc/subuid || skip "no IDs allocated for current user" | ||
| else | ||
| egrep -q "^containers:" /etc/subuid || skip "no IDs allocated for user 'containers'" | ||
| fi |
There was a problem hiding this comment.
Very minor duplication-avoiding suggestion:
ns_user="containers"
if is_rootless then
ns_user=$(id -un)
fi
egrep -q "$ns_user:" /etc/subuid || skip "no IDs allocated for user '$ns_user'"
test/system/170-run-userns.bats
Outdated
| if is_rootless; then | ||
| ns_user=$(id -un) | ||
| fi | ||
| egrep -q "ns_user:" /etc/subuid || skip "no IDs allocated for current user" |
There was a problem hiding this comment.
Missing dollar sign, should be $ns_user
There was a problem hiding this comment.
Oh, and can you change current user to user '$ns_user'?
test/system/170-run-userns.bats
Outdated
| secret_content=$(random_string) | ||
| echo ${secret_content} > ${secret_file} | ||
| run_podman secret create ${test_name} ${secret_file} | ||
| run_podman run -rm --secret=${test_name} --userns=auto:size=1000 $IMAGE cat /run/secrets/${test_name} |
There was a problem hiding this comment.
One more: it should be --rm (two dashes, not one)
This directory needs to be world searchable so users can access it from different user namespaces. Fixes: containers#12779 Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
|
Changes LGTM, but the test failure doesn't look to be a flake. |
|
Rebase needed to fix buildah-bud tests; see #12818 for details. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, flouthoc, 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 |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, flouthoc, 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 |
github-actions
bot
added
the
locked - please file new issue/PR
This directory needs to be world searchable so users can access it from
different user namespaces.
Fixes: #12779
Signed-off-by: Daniel J Walsh dwalsh@redhat.com