-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
libct: we should set envs after we are in the jail of the container #4693
Conversation
2c1ab94
to
409b8fa
Compare
43d6f21
to
6869606
Compare
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.
Left some comments.
However, I think it might be simpler if we sync on slack. Let's chat there, it might be faster :)
tests/integration/run.bats
Outdated
|
||
# https://github.com/opencontainers/runc/issues/4688 | ||
@test "runc run check default home" { | ||
requires root |
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.
Does it require root?
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.
IMHO it does not; instead, it should use
[ $EUID -ne 0 ] && requires rootless_idmap
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.
Thanks!
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.
LGTM overall (except for the test case nits), and thanks!
PS it makes sense to add test that runc exec
also sets $HOME properly.
6869606
to
d8bdf54
Compare
d8bdf54
to
0823f14
Compare
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.
Still LGTM. Thanks for adding the exec test case.
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.
LGTM. Thanks again for the quick fix!
If you can add that comment in the test, it would be great. Feel free to merge after adding that comment (if you agree it makes sense), don't need to review and wait for that again :)
0823f14
to
dabe9cb
Compare
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.
LGTM, feel free to merge when tests are green :)
Signed-off-by: lifubang <[email protected]>
Because we have to set a default HOME env for the current container user, so we should set it after we are in the jail of the container, or else we'll use host's `/etc/passwd` to get a wrong HOME value. Please see: opencontainers#4688. Signed-off-by: lifubang <[email protected]>
dabe9cb
to
bf38646
Compare
@lifubang Can you open a backport to 1.3? |
1.3 backport: #4706 |
Because we have to set a default HOME env for the current container user, so we should set it after we are in the jail of the container, or else we'll use host's
/etc/passwd
to get a wrong HOME value.Fixes #4688.