Skip to content
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

Add support for umask when exec container #3661

Merged
merged 1 commit into from
Feb 28, 2023

Conversation

Wang-squirrel
Copy link
Contributor

@Wang-squirrel Wang-squirrel commented Nov 11, 2022

Signed-off-by: Wang-squirrel wang.xiaosong1@zte.com.cn

@ningmingxiao
Copy link

this will support update umask when exec container @kolyshkin

@kolyshkin
Copy link
Contributor

this will support update umask when exec container @kolyshkin

Thanks! Need to fix a linter warning (please use gofumpt), and add a test case.

For the test case, something like this should work:

diff --git a/tests/integration/umask.bats b/tests/integration/umask.bats
index fe9f9d63..38e8f621 100644
--- a/tests/integration/umask.bats
+++ b/tests/integration/umask.bats
@@ -21,4 +21,9 @@ function teardown() {
        [ "$status" -eq 0 ]
        # umask 63 decimal = umask 77 octal
        [[ "${output}" == *"77"* ]]
+
+       runc exec test_busybox grep '^Umask:' "/proc/self/status"
+       [ "$status" -eq 0 ]
+       # umask 63 decimal = umask 77 octal
+       [[ "${output}" == *"77"* ]]

@Wang-squirrel
Copy link
Contributor Author

@kolyshkin done

@Wang-squirrel
Copy link
Contributor Author

Please check it, thanks @kolyshkin @AkihiroSuda

@AkihiroSuda
Copy link
Member

@Wang-squirrel
Copy link
Contributor Author

kolyshkin
kolyshkin previously approved these changes Dec 15, 2022
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kolyshkin kolyshkin added this to the 1.2.0 milestone Dec 15, 2022
AkihiroSuda
AkihiroSuda previously approved these changes Dec 24, 2022
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but slightly fear this may potentially break some compatibility

@kolyshkin
Copy link
Contributor

LGTM but slightly fear this may potentially break some compatibility

I guess we can afford it with 1.2.0. I checked and it looks like crun is supporting umask for exec from the very beginning.

@kolyshkin
Copy link
Contributor

@Wang-squirrel can you please rebase?

Signed-off-by: WangXiaoSong <wang.xiaosong1@zte.com.cn>
@Wang-squirrel
Copy link
Contributor Author

@Wang-squirrel can you please rebase?

Done

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants