Use getsubids tool for subid validation if possible#49036
Use getsubids tool for subid validation if possible#49036vvoland merged 1 commit intomoby:masterfrom
getsubids tool for subid validation if possible#49036Conversation
43c93ea to
1dbd22d
Compare
|
@AkihiroSuda I would like to hear your point about the change? 👀 If you have time) |
getsubids tool for subid validationgetsubids tool for subid validation if possible
|
@AkihiroSuda Can you provide any insight on the CI failure? I feel like it was some random network issue that is unrelated to the PR. |
|
I restarted CI; I suspect it may have been an issue on GitHub's side (either rate limiting, or some temporary outage of their caching service) |
|
@thaJeztah Thanks for the help! Do you think there's anything left to make this merged? |
|
Ping for review/merge……? |
thaJeztah
left a comment
There was a problem hiding this comment.
Left some suggestions (not blocking, really nits)
I'm slightly wondering now though;
It supports
NSSmodule in addition to/etc/subuidand/etc/subgidfiles. As the latter is a default, existing users that pass the test shouldn't be affected.
So this PR introduces a utility to read the existing mappings, but our instructions still use manual modifying / creating /etc/subuid and /etc/subgid;
# Add subuid entry for ${USERNAME}
echo "${USERNAME}:100000:65536" >> /etc/subuidAnd now I wonder if that's correct - would that mean we'd be mixing/mashing different ways to configure this? Should the instructions also use the same (e.g. usermod --add-subuids)?
However, I know that the daemon code itself does not use those, as it unconditionally looks for /etc/subuid and /etc/subgid to parse;
Lines 31 to 34 in a72026a
Not sure if that's relevant to this situation, but wondering if there's s potentially other cases where those mappings are defined through other ways than those files (i.e., getsubids is aware of existing mapping not handled through those files, but dockerd is not)?
@AkihiroSuda any ideas?
| # instructions: validate subuid for current user | ||
| if ! command -v "getsubids" > /dev/null 2>&1; then | ||
| grep -q "^$USERNAME_ESCAPED:\|^$(id -u):" /etc/subuid 2> /dev/null | ||
| else | ||
| getsubids "$USERNAME" > /dev/null 2>&1 || getsubids "$(id -u)" > /dev/null 2>&1 | ||
| fi |
There was a problem hiding this comment.
This would be really a nit, but wondering if it's clearer to swap the condition; i.e., "if getsubids is available , then we use it, otherwise we fall back to parsing it ourselves manually with grep"
i.e. (not tested);
# instructions: validate subuid for current user
if command -v "getsubids" > /dev/null 2>&1; then
getsubids "$USERNAME" > /dev/null 2>&1 || getsubids "$(id -u)" > /dev/null 2>&1
else
grep -q "^$USERNAME_ESCAPED:\|^$(id -u):" /etc/subuid 2> /dev/null
fi| # instructions: validate subgid for current user | ||
| if ! command -v "getsubids" > /dev/null 2>&1; then | ||
| grep -q "^$USERNAME_ESCAPED:\|^$(id -u):" /etc/subgid 2> /dev/null | ||
| else | ||
| getsubids -g "$USERNAME" > /dev/null 2>&1 || getsubids -g "$(id -u)" > /dev/null 2>&1 | ||
| fi |
There was a problem hiding this comment.
Same here;
# instructions: validate subgid for current user
if command -v "getsubids" > /dev/null 2>&1; then
getsubids -g "$USERNAME" > /dev/null 2>&1 || getsubids -g "$(id -u)" > /dev/null 2>&1
else
grep -q "^$USERNAME_ESCAPED:\|^$(id -u):" /etc/subgid 2> /dev/null
fi
This is for userns-remap, not for rootless |
|
Oh! You're right; I guess I was wondering if any of that code would still be using these in rootless, but I guess not |
|
Or; more to the point; wondering if for userns mode it should be aware of other ways to configure these besides those files, but I guess that would be a separate "enhancement" to make? |
Co-authored-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: YR Chen <stevapple@icloud.com>
|
Did a quick rebase to get a fresh run of CI, and applied my suggestions from #49036 (comment) |
thaJeztah
left a comment
There was a problem hiding this comment.
Thanks for review, and sorry for the delay
let me add my LGTM as well; we can merge once CI finishes 🎉
- What I did
Use
getsubidstool instead of parsing/etc/subuidand/etc/subgidfor subid ownership validation.- How I did it
getsubidstool is includes withinuidmappackage that is required for running rootless Docker. It supports NSS module in addition to/etc/subuidand/etc/subgidfiles. As the latter is a default, existing users that pass the test shouldn't be affected.- How to verify it
Use an NSS-based or traditional file-based
subidbackend and rundockerd-rootless-setuptools.sh install. It would no longer fail for the NSS case.- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)