feature: namespace options and supplemental groups for cri manager #753
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
YaoZengzeng
force-pushed
the
security-context
branch
2 times, most recently
from
0f5cd78 to
379696e
Compare
Codecov Report
@@ Coverage Diff @@
## master #753 +/- ##
==========================================
- Coverage 14.62% 14.58% -0.05%
==========================================
Files 115 115
Lines 6940 7001 +61
==========================================
+ Hits 1015 1021 +6
- Misses 5839 5893 +54
- Partials 86 87 +1
Continue to review full report at Codecov.
|
|
@allencloud PTAL. |
|
@YaoZengzeng could you also add a test for |
YaoZengzeng
force-pushed
the
security-context
branch
from
379696e to
4aaf513
Compare
YaoZengzeng
force-pushed
the
security-context
branch
from
4aaf513 to
43f4b2f
Compare
|
@allencloud @Letty5411 PTAL. |
allencloud
reviewed
Mar 6, 2018
daemon/mgr/spec_process.go
Outdated
| @@ -62,21 +63,30 @@ func setupProcessTTY(ctx context.Context, c *ContainerMeta, spec *SpecWrapper) e | |||
| } | |||
|
|
|||
| func setupProcessUser(ctx context.Context, c *ContainerMeta, spec *SpecWrapper) (err error) { | |||
| user := specs.User{} | |||
There was a problem hiding this comment.
I would invite @Ace-Tang to take a look at the implementation in setupProcessUser.
Ace-Tang
reviewed
Mar 6, 2018
daemon/mgr/spec_process.go
Outdated
| logrus.Errorf("failed to parse supplemental group id %s: %v", group, err) | ||
| continue | ||
| } | ||
| user.AdditionalGids = append(user.AdditionalGids, uint32(gid)) |
There was a problem hiding this comment.
better to take AdditionalGids implement to package pkg/user/
YaoZengzeng
force-pushed
the
security-context
branch
2 times, most recently
from
9e774b1 to
379696e
Compare
YaoZengzeng
force-pushed
the
security-context
branch
from
379696e to
da53ecd
Compare
YaoZengzeng
force-pushed
the
security-context
branch
3 times, most recently
from
d71ae89 to
7ec47b9
Compare
|
@allencloud Updated |
YaoZengzeng
force-pushed
the
security-context
branch
from
7ec47b9 to
affe4ee
Compare
YaoZengzeng
force-pushed
the
security-context
branch
from
affe4ee to
eef2644
Compare
allencloud
reviewed
Mar 9, 2018
| @@ -119,6 +119,22 @@ func GetIntegerID(user string) (uint32, uint32) { | |||
| return uint32(uid), uint32(gid) | |||
| } | |||
|
|
|||
| // GetAdditionalGids parse supplementary gids from slice groups. | |||
There was a problem hiding this comment.
Could you add a unit test case for this func GetAdditionalGids?
|
Do you have any further thoughts on this? @Ace-Tang |
YaoZengzeng
force-pushed
the
security-context
branch
3 times, most recently
from
1b59dc8 to
3a26193
Compare
Signed-off-by: YaoZengzeng <yaozengzeng@zju.edu.cn>
YaoZengzeng
force-pushed
the
security-context
branch
from
3a26193 to
a456ad0
Compare
|
LGTM |
pouchrobot
added
the
LGTM
16 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: YaoZengzeng yaozengzeng@zju.edu.cn
Ⅰ. Describe what this PR did
Ⅱ. Does this pull request fix one issue?
fixes part of #635
Ⅲ. Describe how you did it
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews
We skip the test of "HostNetworkMode", because CRI manager doesn't support logging now.