Add RuntimeHandler support #891

tallclair · 2018-09-04T23:14:41Z

Implement the changes proposed in #879:

Deprecate plugins.cri.containerd.untrusted_workload_runtime. If it is provided, give it the implicit configuration name of untrusted.
Introduce a new ContainerdConfig field: Runtimes map[string]Runtime
Modify criService.getSandboxRuntime to take the runtime handler as a parameter, and map it to the correct runtime config

Integration tests to follow

/cc @Random-Liu

Random-Liu · 2018-09-04T23:21:31Z

/ok-to-test

Random-Liu · 2018-09-05T01:13:28Z

@tallclair Please use git commit --amend -s to sign your commits.

Signed-off-by: Tim Allclair <tallclair@google.com>

tallclair · 2018-09-05T03:06:08Z

Done.

Random-Liu

LGTM other than comments.

Random-Liu · 2018-09-05T05:45:00Z

cri.go

@@ -104,6 +108,18 @@ func initCRIService(ic *plugin.InitContext) (interface{}, error) {
 	return s, nil
 }

+// validateConfig validates that the given configuration.


nit: remove that?

Random-Liu · 2018-09-05T05:45:32Z

pkg/config/config.go

+
+const (
+	// RuntimeUntrusted is the implicit runtime defined for ContainerdConfig.UntrustedWorkloadRuntime
+	RuntimeUntrusted = "unstrusted"


s/unstrusted/untrusted

oh, good catch. Done.

Random-Liu · 2018-09-05T05:53:04Z

pkg/server/sandbox_run_test.go

+			defaultRuntime: defaultRuntime,
+			runtimes:       map[string]criconfig.Runtime{"foo": fooRuntime},
+			expectErr:      true,
+		},


missing test case: should return error if both annotation and runtime handler are specified.

mikebrow

see comments

mikebrow · 2018-09-05T14:42:13Z

pkg/config/config.go

 	DefaultRuntime Runtime `toml:"default_runtime" json:"defaultRuntime"`
 	// UntrustedWorkloadRuntime is a runtime to run untrusted workloads on it.
+	// DEPRECATED: use Runtimes instead. If provided, this runtime is mapped to the runtime handler
+	//     named 'untrusted'. It is a configuration error to provide both.


How about this: It is a configuration error to provide both the (now deprecated) UntrustedWorkloadRuntime and a handler in the Runtimes handler map (below) for 'untrusted' workloads at the same time. Please provide one or the other.

mikebrow · 2018-09-05T14:45:46Z

pkg/config/config.go

 	UntrustedWorkloadRuntime Runtime `toml:"untrusted_workload_runtime" json:"untrustedWorkloadRuntime"`
+	// Runtimes maps a CRI RuntimeHandler string to a runtime configuration.


How about this: Runtimes is a map from CRI RuntimeHandler strings, that specify types of workloads, to runtimes for handling those types of workloads.

It's not entirely accurate that the RuntimeHandler "specifies types of workloads". For example, one user might want their ngnix workload to be sandboxed, while another is comfortable running it with native isolation. It's not that those are different workloads, necessarily, just different threat models.

Could it be any type of classification. Development, Test, Production; or High, low, no threat model; or multi-tenant vs. uni-tenant; or ... Is it any type system or class dreamed up by the user mapped to particular runtimes chosen by the user?

At any rate the amount of threat secured by the runtime will be dependent on other factors outside just the version of the runtime selected. If we are saying these are threat model classes who's to say which runtime is best for which type of threat model?

ok I like your text as it's vague enough here I suppose. Just needs some explanation elsewhere for patterns of use.

mikebrow · 2018-09-05T15:15:18Z

pkg/config/config.go

@@ -33,10 +33,15 @@ type Runtime struct {
 type ContainerdConfig struct {
 	// Snapshotter is the snapshotter used by containerd.
 	Snapshotter string `toml:"snapshotter" json:"snapshotter"`
-	// DefaultRuntime is the runtime to use in containerd.
+	// DefaultRuntime is the default runtime to use in containerd.


The documentation for these fields has to be cut and pasted to here: https://github.com/containerd/cri/blame/master/docs/config.md#L52

Thanks! I missed that.

Done. Add a clarification about the example to the runtimes entry.

mikebrow · 2018-09-05T15:21:06Z

pkg/server/sandbox_run.go

 	if untrustedWorkload(config) {
+		// If the untrusted annotation is provided, runtimeHandler MUST be empty.
+		if runtimeHandler != "" {
+			return criconfig.Runtime{}, errors.New("untrusted workload with explicit runtime handler is not allowed")


Why can't we match untrusted workloads with a runtime handler for untrusted workloads? I must be missing something.

Suppose I add the untrusted annotation, and ask for the 'native' runtime? Those are conflictiing. Originally, I was thinking RuntimeHandler should take precedence, but that would be "failing open" in this case, and potentially present a security risk.

However, to smooth roll out / upgrade, I decided we should allow setting the untrusted annotation and setting the 'untrusted' runtimeHandler.

tallclair

Thanks for the review!

tallclair · 2018-09-05T18:16:30Z

cri.go

@@ -104,6 +108,18 @@ func initCRIService(ic *plugin.InitContext) (interface{}, error) {
 	return s, nil
 }

+// validateConfig validates that the given configuration.


tallclair · 2018-09-05T18:18:42Z

pkg/config/config.go

+
+const (
+	// RuntimeUntrusted is the implicit runtime defined for ContainerdConfig.UntrustedWorkloadRuntime
+	RuntimeUntrusted = "unstrusted"


oh, good catch. Done.

tallclair · 2018-09-05T18:24:32Z

pkg/server/sandbox_run.go

 	if untrustedWorkload(config) {
+		// If the untrusted annotation is provided, runtimeHandler MUST be empty.
+		if runtimeHandler != "" {
+			return criconfig.Runtime{}, errors.New("untrusted workload with explicit runtime handler is not allowed")


Suppose I add the untrusted annotation, and ask for the 'native' runtime? Those are conflictiing. Originally, I was thinking RuntimeHandler should take precedence, but that would be "failing open" in this case, and potentially present a security risk.

However, to smooth roll out / upgrade, I decided we should allow setting the untrusted annotation and setting the 'untrusted' runtimeHandler.

tallclair · 2018-09-05T18:29:37Z

pkg/config/config.go

 	UntrustedWorkloadRuntime Runtime `toml:"untrusted_workload_runtime" json:"untrustedWorkloadRuntime"`
+	// Runtimes maps a CRI RuntimeHandler string to a runtime configuration.


It's not entirely accurate that the RuntimeHandler "specifies types of workloads". For example, one user might want their ngnix workload to be sandboxed, while another is comfortable running it with native isolation. It's not that those are different workloads, necessarily, just different threat models.

tallclair · 2018-09-05T18:29:42Z

pkg/config/config.go

 	DefaultRuntime Runtime `toml:"default_runtime" json:"defaultRuntime"`
 	// UntrustedWorkloadRuntime is a runtime to run untrusted workloads on it.
+	// DEPRECATED: use Runtimes instead. If provided, this runtime is mapped to the runtime handler
+	//     named 'untrusted'. It is a configuration error to provide both.


tallclair · 2018-09-05T18:33:23Z

pkg/config/config.go

@@ -33,10 +33,15 @@ type Runtime struct {
 type ContainerdConfig struct {
 	// Snapshotter is the snapshotter used by containerd.
 	Snapshotter string `toml:"snapshotter" json:"snapshotter"`
-	// DefaultRuntime is the runtime to use in containerd.
+	// DefaultRuntime is the default runtime to use in containerd.


Done. Add a clarification about the example to the runtimes entry.

tallclair · 2018-09-05T18:35:02Z

pkg/server/sandbox_run_test.go

+			defaultRuntime: defaultRuntime,
+			runtimes:       map[string]criconfig.Runtime{"foo": fooRuntime},
+			expectErr:      true,
+		},


mikebrow

/LGTM with one small nit..

Cheers!

mikebrow · 2018-09-05T19:37:36Z

cri.go

+	// It is an error to provide both an UntrustedWorkloadRuntime & define an 'untrusted' runtime.
+	if _, ok := c.ContainerdConfig.Runtimes[criconfig.RuntimeUntrusted]; ok {
+		if c.ContainerdConfig.UntrustedWorkloadRuntime.Type != "" {
+			return errors.New("conflicting untrusted runtimes defined")


maybe: "duplicate/conflicting definitions, configuration includes untrusted_workload_runtime and runtimes[untrusted]"

mikebrow · 2018-09-05T19:44:35Z

pkg/config/config.go

+
+const (
+	// RuntimeUntrusted is the implicit runtime defined for ContainerdConfig.UntrustedWorkloadRuntime
+	RuntimeUntrusted = "untrusted"


Would be nice if we could move this string over to CRI maybe a TODO?

This isn't an "official" concept, even though most (all?) runtimes have implemented it. We've talked about something like that with RuntimeClass, but for now we decided to keep it open ended. I just used this string as a transitional thing while migrating to the runtime handler based configuration. Once the annotation is no longer used, the runtime can be called anything.

true.. yet it would be beneficial to have example patterns and reserved names that have more meaning than .. anything :-)

Signed-off-by: Tim Allclair <tallclair@google.com>

tallclair

Addressed comments & squashed commits.

tallclair · 2018-09-06T00:28:06Z

cri.go

+	// It is an error to provide both an UntrustedWorkloadRuntime & define an 'untrusted' runtime.
+	if _, ok := c.ContainerdConfig.Runtimes[criconfig.RuntimeUntrusted]; ok {
+		if c.ContainerdConfig.UntrustedWorkloadRuntime.Type != "" {
+			return errors.New("conflicting untrusted runtimes defined")


Random-Liu · 2018-09-06T22:23:53Z

The e2e test is completely broken now, probably caused by #895, but I need to verify.

Before that I'll avoid merging any new PRs.

Random-Liu · 2018-09-10T17:09:29Z

/lgtm

k8s-ci-robot requested a review from Random-Liu September 4, 2018 23:14

k8s-ci-robot added size/XXL needs-ok-to-test labels Sep 4, 2018

k8s-ci-robot removed the needs-ok-to-test label Sep 4, 2018

Random-Liu self-assigned this Sep 4, 2018

mikebrow self-assigned this Sep 4, 2018

Random-Liu added this to the v1.12 milestone Sep 5, 2018

tallclair force-pushed the runtimehandler branch from 0a4e04e to 18bbb92 Compare September 5, 2018 03:04

Vendor latest CRI API

89b5b3c

Signed-off-by: Tim Allclair <tallclair@google.com>

tallclair force-pushed the runtimehandler branch from 18bbb92 to 27d36ad Compare September 5, 2018 03:05

Random-Liu reviewed Sep 5, 2018

View reviewed changes

Random-Liu mentioned this pull request Sep 5, 2018

Deprecate UntrustedWorkloadRuntime Option #893

Closed

3 tasks

mikebrow reviewed Sep 5, 2018

View reviewed changes

tallclair commented Sep 5, 2018

View reviewed changes

mikebrow approved these changes Sep 5, 2018

View reviewed changes

Add RuntimeHandler support

e7189a2

Signed-off-by: Tim Allclair <tallclair@google.com>

tallclair force-pushed the runtimehandler branch from f3d4e72 to e7189a2 Compare September 6, 2018 00:27

tallclair commented Sep 6, 2018

View reviewed changes

k8s-ci-robot added the lgtm label Sep 10, 2018

Random-Liu merged commit cfdf872 into containerd:master Sep 10, 2018

Huang-Wei mentioned this pull request Dec 29, 2018

runtime-aware scheduling kubernetes/kubernetes#72413

Closed

Random-Liu mentioned this pull request Apr 24, 2019

Add default runtime name #1125

Merged

		UntrustedWorkloadRuntime Runtime `toml:"untrusted_workload_runtime" json:"untrustedWorkloadRuntime"`
		// Runtimes maps a CRI RuntimeHandler string to a runtime configuration.

Add RuntimeHandler support #891

Add RuntimeHandler support #891

Conversation

tallclair commented Sep 4, 2018

Random-Liu commented Sep 4, 2018

Random-Liu commented Sep 5, 2018

tallclair commented Sep 5, 2018

Random-Liu left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebrow left a 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

mikebrow Sep 5, 2018 • edited Loading

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

Choose a reason for hiding this comment

tallclair left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebrow left a comment

Choose a reason for hiding this comment

mikebrow Sep 5, 2018 • edited Loading

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

tallclair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented Sep 6, 2018 • edited Loading

Random-Liu commented Sep 10, 2018

mikebrow Sep 5, 2018 •

edited

Loading

mikebrow Sep 5, 2018 •

edited

Loading

Random-Liu commented Sep 6, 2018 •

edited

Loading