-
Notifications
You must be signed in to change notification settings - Fork 617
driver/docker-container: remove conditional UsernsMode=host for userns #3425
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
I see there also was a nestybox / sysbox issue linked to the original PR; in case we need to verify behavior with that somehow; |
| hc.UsernsMode = "host" | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prior to #887 , UsernsMode was rather unconditionally set to "host".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wah, sorry, looks like I missed your comment @AkihiroSuda
So, IIUC, before #561, we unconditionally set it to UsernsMode=host, which was a workaround for moby/moby#43084, because the daemon didn't correctly detect user-ns. And #561 made it a bit more granular so that we wouldn't set UsernsMode=host unconditionally, and only when needed.
But (again, IIUC), with moby/moby#43084 now included in the daemon, that handling wouldn't be needed, as it would no longer be needed to set UsernsMode=host ?
But maybe I mis-interpreted the intent here; what's the best way to verify (other than CI in this repo?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But maybe I mis-interpreted the intent here; what's the best way to verify (other than CI in this repo?)
Probably we should have a CI with userns-remap mode.
If the CI passes, I have no objection to merge this PR.
This special handling was added in 5f8600f, to work around an issue in the daemon. That issue was fixed in [moby@a826ca3], which was backported to docker v20.10.13 in [moby@660b996]. Given that docker 20.10 reached EOL, and any currently supported version of docker would have the fix from [moby@a826ca3], we can remove this special handling. [moby@a826ca3]: moby/moby@a826ca3 [moby@660b996]: moby/moby@660b996 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
fb0801e to
6397d1a
Compare
relates to:
container init caused: write sysctl key net.ipv4.ping_group_range: write /proc/sys/net/ipv4/ping_group_range: invalid argument: unknown) #561SecurityOptandDecodeSecurityOptionsto client mod moby/moby#50825This special handling was added in 5f8600f, to work around an issue in the daemon. That issue was fixed in moby@a826ca3, which was backported to docker v20.10.13 in moby@660b996.
Given that docker 20.10 reached EOL, and any currently supported version of docker would have the fix from moby@a826ca3, we can remove this special handling.