feature: capability for both cri manager and container manager #680

YaoZengzeng · 2018-02-01T11:25:57Z

Signed-off-by: YaoZengzeng [email protected]

1.Describe what this PR did

With this PR, we could scope the capability of containers.

2.Does this pull request fix one issue?

3.Describe how you did it

4.Describe how to verify it

root@pouch-ubuntu:/home/monster# pouch run busybox:1.26 brctl addbr foobar
brctl: bridge foobar: Operation not permitted
root@pouch-ubuntu:/home/monster# pouch run --cap-add NET_ADMIN busybox:1.26 brctl addbr foobar
root@pouch-ubuntu:/home/monster#

5.Special notes for reviews

There are some problems with container manager's StartExec method, we couldn't pass the corresponding tests in cri-tools.

I'll fix it ASAP :)

allencloud · 2018-02-02T00:52:24Z

test/cli_create_test.go

+	if err := json.Unmarshal([]byte(output), result); err != nil {
+		c.Errorf("failed to decode inspect output: %v", err)
+	}
+	c.Assert(result.HostConfig.CapAdd, nil)


FAIL: /go/src/github.com/alibaba/pouch/test/cli_create_test.go:170: PouchCreateSuite.TestCreateWithCapability /go/src/github.com/alibaba/pouch/test/cli_create_test.go:183: c.Assert(result.HostConfig.CapAdd, nil) ... Assert(obtained, nil!?, ...): ... Oops.. you've provided a nil checker!

allencloud · 2018-02-02T00:55:01Z

vendor/github.com/syndtr/gocapability/LICENSE

@@ -0,0 +1,24 @@
+Copyright 2013 Suryandaru Triandana <[email protected]>


What kind of this license is it?
It is some kind of different from some usual others.

codecov-io · 2018-02-02T02:51:57Z

Codecov Report

Merging #680 into master will decrease coverage by 0.04%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master    #680      +/-   ##
=========================================
- Coverage   10.01%   9.97%   -0.05%     
=========================================
  Files          92      92              
  Lines        5353    5375      +22     
=========================================
  Hits          536     536              
- Misses       4767    4789      +22     
  Partials       50      50

Impacted Files	Coverage Δ
daemon/mgr/spec.go	`0% <ø> (ø)`	⬆️
cli/run.go	`0% <0%> (ø)`	⬆️
daemon/mgr/cri_utils.go	`46.95% <0%> (-0.52%)`	⬇️
daemon/mgr/spec_linux.go	`0% <0%> (ø)`	⬆️
cli/create.go	`0% <0%> (ø)`	⬆️
cli/container.go	`25.6% <0%> (-0.32%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b6bb67...e086274. Read the comment docs.

YaoZengzeng · 2018-02-02T02:53:26Z

@allencloud updated.

allencloud · 2018-02-02T05:46:34Z

daemon/mgr/spec_linux.go

+	var err error
+
+	s := spec.s
+	if meta.HostConfig.Privileged {


Could you add this flag in pouch command line as well?
I found that in cli, there is no privileged flag. @YaoZengzeng

Actually we could see meta.HostConfig.Privileged in many places associated with security.

I'll use an independent PR to handle it once the security options are complete.

allencloud · 2018-02-02T05:48:01Z

daemon/mgr/spec_linux.go

+	s := spec.s
+	if meta.HostConfig.Privileged {
+		caplist = caps.GetAllCapabilities()
+	} else {


else if caplist, err = caps.TweakCapabilities(s.Process.Capabilities.Effective, meta.HostConfig.CapAdd, meta.HostConfig.CapDrop); err != nil{ return err }

How about this?

s.Process.Capabilities.Effective is the default capability list. The function here is used to modify it with Capability add-list and drop-list.

I said that maybe we can use else if to reduce an ident. 😄

Oh! Yes, it is more clear.

allencloud · 2018-02-02T05:52:17Z

daemon/mgr/spec_linux.go

+	s.Process.Capabilities.Effective = caplist
+	s.Process.Capabilities.Bounding = caplist
+	s.Process.Capabilities.Permitted = caplist
+	s.Process.Capabilities.Inheritable = caplist


I think in this function, we do not need to always use s.Process.Capabilities. We can use caps := s.Process. Capabilities at the very beginning in the function. Then we can use caps.Effective = caplist very simply. Since Capabilities *LinuxCapabilities is a pointer, then we do not worry about another copy or same address thing. And it will make code more clear. WDYT? @YaoZengzeng

We just wish that the data used to communicate among modules or functions should be minimal, so we could encapsulate date transfer very well.

allencloud · 2018-02-02T05:56:48Z

LGTM generally. One data transferring issue and a privileged flag. If privileged flag is in your plan but in a follow-up, it is totally OK.

Signed-off-by: YaoZengzeng <[email protected]>

YaoZengzeng · 2018-02-02T06:43:12Z

@allencloud updated.

allencloud · 2018-02-02T07:16:52Z

re-LGTM

pouchrobot added kind/feature size/XXL conflict/needs-rebase labels Feb 1, 2018

allencloud reviewed Feb 2, 2018

View reviewed changes

YaoZengzeng force-pushed the capability branch 2 times, most recently from 792c37c to 8d6e754 Compare February 2, 2018 02:00

pouchrobot removed the conflict/needs-rebase label Feb 2, 2018

YaoZengzeng mentioned this pull request Feb 2, 2018

[enhancement request] Advanced configuration for Sandbox and Container in CRI #635

Closed

16 tasks

YaoZengzeng force-pushed the capability branch from 8d6e754 to ce11709 Compare February 2, 2018 02:35

allencloud added this to the v0.2-milestone milestone Feb 2, 2018

allencloud reviewed Feb 2, 2018

View reviewed changes

pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Feb 2, 2018

feature: capability for both cri manager and container manager

e086274

Signed-off-by: YaoZengzeng <[email protected]>

YaoZengzeng force-pushed the capability branch from ce11709 to e086274 Compare February 2, 2018 06:12

allencloud merged commit c3fe994 into AliyunContainerService:master Feb 2, 2018

pouchrobot mentioned this pull request Feb 2, 2018

Weekly Report of Pouch from 2018-01-26 to 2018-02-02 #690

Closed

pouchrobot mentioned this pull request Feb 9, 2018

Weekly Report of Pouch from 2018-02-02 to 2018-02-09 #720

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: capability for both cri manager and container manager #680

feature: capability for both cri manager and container manager #680

YaoZengzeng commented Feb 1, 2018

allencloud Feb 2, 2018

allencloud Feb 2, 2018

codecov-io commented Feb 2, 2018 •

edited

Loading

YaoZengzeng commented Feb 2, 2018

allencloud Feb 2, 2018

YaoZengzeng Feb 2, 2018

allencloud Feb 2, 2018

YaoZengzeng Feb 2, 2018

allencloud Feb 2, 2018

YaoZengzeng Feb 2, 2018

allencloud Feb 2, 2018 •

edited

Loading

YaoZengzeng Feb 2, 2018

allencloud commented Feb 2, 2018

YaoZengzeng commented Feb 2, 2018

allencloud commented Feb 2, 2018

		@@ -0,0 +1,24 @@
		Copyright 2013 Suryandaru Triandana <[email protected]>

feature: capability for both cri manager and container manager #680

feature: capability for both cri manager and container manager #680

Conversation

YaoZengzeng commented Feb 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 2, 2018 • edited Loading

Codecov Report

YaoZengzeng commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud commented Feb 2, 2018

YaoZengzeng commented Feb 2, 2018

allencloud commented Feb 2, 2018

codecov-io commented Feb 2, 2018 •

edited

Loading

allencloud Feb 2, 2018 •

edited

Loading