Get correct username in pod when using --userns=keep-id #17174
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
openshift-ci
bot
added
the
approved
|
Looks like a REST test needs some tweaking @rhatdan |
Fixes: containers#17148 Signed-off-by: Daniel J Walsh <[email protected]>
|
/lgtm |
| if is_rootless; then | ||
| user=$(id -u) | ||
| run_podman pod create --userns keep-id | ||
| pid=$output |
There was a problem hiding this comment.
Eek, am I the only one who read this as "process id" and got severely confused for a second or two?
| exec1 := podmanTest.Podman([]string{"exec", ctrName, "cat", "/etc/passwd"}) | ||
| exec1.WaitWithDefaultTimeout() | ||
| Expect(exec1).Should(Exit(0)) | ||
| Expect(exec1.OutputToString()).To(ContainSubstring("container")) | ||
| Expect(exec1.OutputToString()).To(ContainSubstring(u.Name)) |
There was a problem hiding this comment.
Just out of principle, I think a better test would be:
.....{"exec", ctrName, "grep", "^"+u.Name+":", "/etc/passwd"})or even (cleaner)
.....{"exec", ... , "id", u.Name})Basically, to avoid a false match on username against (something else in passwd). Not that I can think of a real-world example, so this is paranoia. Anyone concur/disagree? Should I open an update/cleanup PR?
There was a problem hiding this comment.
we would probably catch it in the CI if the passwd file of any image breaks this test. Said so, nothing against paranoia and fixing it earlier :-)
Ha ha. This was supposed to be a trivial little followup to containers#17174: containers#17174 (comment) (safer username check when --userns=keep-id) It got complicated. TL;DR we need to use User.Username, not User.Name. The latter is GECOS! Tests were working because, on Fedora, GECOS for root is "root". Found and fixed all 'u.Name' instances, but if there are any references with a variable other than 'u', they still need looking into. Signed-off-by: Ed Santiago <[email protected]>
github-actions
bot
added
the
locked - please file new issue/PR
Fixes: #17148
Signed-off-by: Daniel J Walsh [email protected]
Does this PR introduce a user-facing change?