Support privileged #51

heartlock · 2017-05-25T02:47:10Z

When privileged is true:

Enable all capabilities
Add all host devices
Clear readonly on some volumes, e.g. cgroups and /sys volumes

ref: #29 @Random-Liu Ready for review.

Random-Liu · 2017-05-25T06:20:24Z

Will take a look tomorrow, and let's try to get it merged this week.

mikebrow

LGTM

Random-Liu

Still reviewing. Will finish today.

Random-Liu · 2017-06-06T07:10:55Z

pkg/server/container_start.go

 	"github.com/opencontainers/runtime-tools/generate"
+	"github.com/opencontainers/runtime-tools/validate"
+	"github.com/syndtr/gocapability/capability"
+


unnecessary empty line.

Random-Liu · 2017-06-06T07:13:13Z

pkg/server/container_start.go


-	// TODO(random-liu): [P1] Set device mapping.
-	// Ref https://github.com/moby/moby/blob/master/oci/devices_linux.go.
+	if err := addDevices(&g, config.GetDevices(), securityContext.GetPrivileged()); err != nil {


nit: addOCIDevices? Follow the convention.

Random-Liu · 2017-06-06T07:16:52Z

pkg/server/container_start.go

+		for _, device := range devs {
+			dev, err := devices.DeviceFromPath(device.HostPath, device.Permissions)
+			if err != nil {
+				return fmt.Errorf("failed to add device: %v", err)


nit: Directly return error here, or add description for the other error.

Random-Liu · 2017-06-06T07:18:21Z

pkg/server/container_start.go

+		}
+	}
+	return nil
+


unnecessary empty line.

Random-Liu · 2017-06-06T08:21:59Z

pkg/server/container_start.go

 	}

-	addOCIBindMounts(&g, config.GetMounts())
+	securityContext := config.GetLinux().GetSecurityContext()


nit: I feel like it would be better to add a function setOCIPrivileged, use setOCIPrivileged if privileged is set, and use other functions if not.

This groups all privileged logic together, which makes it more clear what privilege a privileged container has.

Maybe add a TODO for now.

Random-Liu · 2017-06-07T07:58:06Z

pkg/server/container_start.go

+				GID:   &dev.Gid,
+			}
+			g.AddDevice(rd)
+			spec.Linux.Resources.Devices = append(spec.Linux.Resources.Devices, runtimespec.LinuxDeviceCgroup{


Not related to this PR. Don't know why runtime-tools doesn't support this, maybe we could fix it in the future. :)

Random-Liu · 2017-06-07T08:05:50Z

pkg/server/container_start.go

-	}
+func setOCICapabilities(g *generate.Generator, capabilities *runtime.Capability, privileged bool) error {
+	if privileged {
+		// Add all capabilities in privileged mode.


Can use runtimetools. SetupPrivileged(true)

This is my original thought. It is strange why runtime-tools support only cap.

if privileged { g.SetupPrivileged(true) return } /* Other code */

Random-Liu · 2017-06-07T08:10:06Z

pkg/server/container_start.go

-		if err := g.AddProcessCapability(c); err != nil {
-			return err
+		for _, c := range capabilities.GetAddCapabilities() {
+			if err := g.AddProcessCapability(c); err != nil {


We need to do fmt.Sprintf("CAP_%s", strings.ToUpper(cap.String())) for the CAP passed via CRI.
We don't have the CAP_ prefix in Kubernetes/CRI.

Add CAP_ prefix.

Random-Liu · 2017-06-07T08:10:11Z

pkg/server/container_start.go

-		if err := g.DropProcessCapability(c); err != nil {
-			return err
+		for _, c := range capabilities.GetDropCapabilities() {
+			if err := g.DropProcessCapability(c); err != nil {


Same with above.

Random-Liu · 2017-06-07T08:19:15Z

@heartlock We need test for privileged mode and device mapping. However, HostDevices and DeviceFromPath all access the host directly, which should not happen in unit test.

We may want to add DeviceFromPath and HostDevices into os interface so that we could unit test this. Let me think about this a little bit more.

Other than this, please see the comments above.

Random-Liu · 2017-06-07T18:56:42Z

pkg/server/container_start.go

+		spec.Process.Capabilities.Effective = finalCapList
+		spec.Process.Capabilities.Inheritable = finalCapList
+		spec.Process.Capabilities.Permitted = finalCapList
+		spec.Process.Capabilities.Ambient = finalCapList


return here, so we don't need the else. Reducing indent is always prefered.

Random-Liu · 2017-06-07T18:57:10Z

pkg/server/container_start.go

 		// TODO(random-liu): [P1] Apply selinux label
 		g.AddBindMount(src, dst, options)
 	}
+	if privileged {


if !privileged{
return
}

why !privileged ?

I mean:

if !privileged { return } // or else /* clearReadyOnly... */

This reduces indent of the "clear read only code", which is a preferred coding style.

Random-Liu · 2017-06-07T18:57:22Z

pkg/server/container_start.go

+				Access: "rwm",
+			},
+		}
+	} else {


Same here, reduce indent.

if privileged { /* set privileged */ return } /* other code */

Random-Liu · 2017-06-07T20:44:51Z

pkg/server/container_start.go

 	}
+	if privileged {
+		spec := g.Spec()
+		// clear readonly for /sys and cgroup


Let's at least add unit test for this behavior, and leave out the others. But please add TODO for adding test for device mapping code.

Random-Liu · 2017-06-09T00:23:06Z

@heartlock Please address comments and rebase your PR. Thanks.

heartlock · 2017-06-09T01:35:54Z

@Random-Liu will do today.

Random-Liu · 2017-06-09T06:59:12Z

pkg/server/container_start.go

+
 	// Add extra mounts first so that CRI specified mounts can override.
-	addOCIBindMounts(&g, append(extraMounts, config.GetMounts()...))
+	addOCIBindMounts(&g, append(extraMounts, config.GetMounts()...), securityContext.GetPrivileged())


Why add bind mounts twice? Seems to be error caused by rebase.

My fault! Forgot to delete previous one.

Random-Liu · 2017-06-09T07:00:31Z

pkg/server/container_start.go

 		// TODO(random-liu): [P1] Apply selinux label
 		g.AddBindMount(src, dst, options)
 	}
+	if privileged {


I mean:

if !privileged { return } // or else /* clearReadyOnly... */

This reduces indent of the "clear read only code", which is a preferred coding style.

Random-Liu · 2017-06-09T07:02:12Z

pkg/server/container_start.go

-	}
+func setOCICapabilities(g *generate.Generator, capabilities *runtime.Capability, privileged bool) error {
+	if privileged {
+		// Add all capabilities in privileged mode.


if privileged { g.SetupPrivileged(true) return } /* Other code */

Random-Liu · 2017-06-09T07:02:29Z

pkg/server/container_start.go

-		if err := g.AddProcessCapability(c); err != nil {
-			return err
+		for _, c := range capabilities.GetAddCapabilities() {
+			if err := g.AddProcessCapability(c); err != nil {


Add CAP_ prefix.

Random-Liu · 2017-06-09T07:03:24Z

pkg/server/container_start.go

+				Access: "rwm",
+			},
+		}
+	} else {


if privileged { /* set privileged */ return } /* other code */

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

Random-Liu · 2017-06-11T00:30:45Z

LGTM except #51 (comment) and #51 (comment).

Will merge this one first and send another PR to address those 2.

Support privileged

add support for container stats on windows

k8s-ci-robot added the cncf-cla: yes label May 25, 2017

mikebrow approved these changes May 26, 2017

View reviewed changes

Random-Liu added this to the v0.1.0-rc2 milestone May 30, 2017

Random-Liu self-assigned this May 31, 2017

Random-Liu mentioned this pull request Jun 2, 2017

CRI-Containerd Missing Pieces #62

Closed

42 tasks

Random-Liu reviewed Jun 6, 2017

View reviewed changes

heartlock closed this Jun 7, 2017

heartlock reopened this Jun 7, 2017

Random-Liu reviewed Jun 7, 2017

View reviewed changes

heartlock force-pushed the support-privileged branch 2 times, most recently from 6a35bf5 to 1227911 Compare June 9, 2017 06:48

Random-Liu reviewed Jun 9, 2017

View reviewed changes

heartlock added 2 commits June 9, 2017 15:42

support privileged

dda03f7

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

update vendor

73fbe90

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

heartlock force-pushed the support-privileged branch from 1227911 to 73fbe90 Compare June 9, 2017 07:55

Random-Liu added the lgtm label Jun 11, 2017

Random-Liu merged commit e9a930b into containerd:master Jun 11, 2017

lanchongyizu pushed a commit to lanchongyizu/cri-containerd that referenced this pull request Sep 3, 2017

Merge pull request containerd#51 from heartlock/support-privileged

f4fd42e

Support privileged

adelina-t pushed a commit to adelina-t/cri that referenced this pull request Nov 1, 2019

Merge pull request containerd#51 from katiewasnothere/container_stats

6c04ac1

add support for container stats on windows

Support privileged #51

Support privileged #51

Uh oh!

Conversation

heartlock commented May 25, 2017

Uh oh!

Random-Liu commented May 25, 2017

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

Random-Liu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu commented Jun 9, 2017

Uh oh!

heartlock commented Jun 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu commented Jun 11, 2017

Uh oh!

Reviewers

Random-Liu Jun 6, 2017 •

edited

Loading

Random-Liu Jun 6, 2017 •

edited

Loading

Random-Liu Jun 6, 2017 •

edited

Loading

Random-Liu commented Jun 7, 2017 •

edited

Loading