Add proxy for container streaming in kubelet for streaming auth. #64006

Random-Liu · 2018-05-18T01:43:23Z

For #36666, option 2 of #36666 (comment).

This PR:

Removed the DirectStreamingRuntime, and changed IndirectStreamingRuntime to StreamingRuntime. All DirectStreamingRuntimes, dockertools and rkt, were removed.
Proxy container streaming in kubelet instead of returning redirect to apiserver. This solves the container runtime authentication issue, which is what we agreed on in [CRI] Add authentication to streaming server #36666.

Please note that, this PR replaced the redirect with proxy directly instead of adding a knob to switch between the 2 behaviors. For existing CRI runtimes like containerd and cri-o, they should change to serve container streaming on localhost, so as to make the whole container streaming connection secure.

If a general authentication mechanism proposed in #62747 is ready, we can switch back to redirect, and all code can be found in github history.

Please also note that this added some overhead in kubelet when there are container streaming connections. However, the actual bottleneck is in the apiserver anyway, because it does proxy for all container streaming happens in the cluster. So it seems fine to get security and simplicity with this overhead. @derekwaynecarr @mrunalp Are you ok with this? Or do you prefer a knob?

@yujuhong @timstclair @dchen1107 @mikebrow @feiskyer
/cc @kubernetes/sig-node-pr-reviews
Release note:

Kubelet now proxies container streaming between apiserver and container runtime. The connection between kubelet and apiserver is authenticated. Container runtime should change streaming server to serve on localhost, to make the connection between kubelet and container runtime local.

In this way, the whole container streaming connection is secure. To switch back to the old behavior, set `--redirect-container-streaming=true` flag.

Random-Liu · 2018-05-18T01:57:08Z

With this PR, crictl also works with dockershim now:

$ sudo /usr/local/bin/crictl --runtime-endpoint=unix:///var/run/dockershim.sock exec -i -t 1fe29f12bc656 /bin/sh
# ls
ls
bin  build  cgi-bin  conf  error  htdocs  icons  include  logs	modules
#

mikebrow

See comments / questions.

mikebrow · 2018-05-18T14:38:06Z

pkg/kubelet/server/server.go

-		s.host.StreamingConnectionIdleTimeout(),
-		remotecommandconsts.DefaultStreamCreationTimeout,
-		remotecommandconsts.SupportedStreamingProtocols)
+	handler := proxy.NewUpgradeAwareHandler(url, nil, false, false, &responder{})


// retrive handler for url location using default proxy transport(nil) with wrapTransport and upgradeRequired parameters set to off (false)

Will add comment for each field.

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

pkg/kubelet/server/server.go

-		s.host.StreamingConnectionIdleTimeout(),
-		remotecommandconsts.DefaultStreamCreationTimeout,
-		remotecommandconsts.SupportedStreamingProtocols)
+	handler := proxy.NewUpgradeAwareHandler(url, nil, false, false, &responder{})


Should responder be a proxy.ErrorResponder or nil here?

proxy.ErrorResponder is just an interface. responder implements it.

mikebrow · 2018-05-18T16:12:11Z

pkg/kubelet/server/streaming/server.go

+		return err
+	}
+	// Use the actual address as baseURL host. This handles the "0" port case.
+	s.config.BaseURL.Host = listener.Addr().String()


should probably comment the baseurl structure element on line 74 above so people know it will/may be updated after start() otherwise someone might rval it early...

mikebrow · 2018-05-18T16:17:59Z

pkg/kubelet/container/runtime.go

-}
-
-// IndirectStreamingRuntime is the interface implemented by runtimes that handle the serving of the
+// StreamingRuntime is the interface implemented by runtimes that handle the serving of the


Should make a comment here about DirectStreamingRuntime and IndirectStreamingRuntime being end of life at least for the time being. Maybe with a link to the pr that removed them.

I think this is just kubelet implementation detail. I don't think anyone will come back and cleanup if we put such a comment here.

Like when we remove rkt and dockershim package, we won't leave a comment saying dockershim was removed.

True.. I saw the upper case first char.. and since we may have a future implementation with a switch, I figured a link to the past. But as you say the link is in github.

Random-Liu · 2018-05-18T17:31:26Z

I'll fix the unit test.

Random-Liu · 2018-05-18T23:10:40Z

Fixed the unit test. The unit test was testing DirectStreamingRuntime, which is not supported anymore.
This PR changed the test to test against the proxy mode instead.

Random-Liu · 2018-05-18T23:18:24Z

/test containerd

k8s-ci-robot · 2018-06-01T00:29:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Random-Liu, yujuhong

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cmd/kubelet/OWNERS~~ [Random-Liu,yujuhong]
~~pkg/kubelet/OWNERS~~ [Random-Liu,yujuhong]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tallclair · 2018-06-01T00:44:41Z

/sig auth

k8s-github-robot · 2018-06-01T00:45:20Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@Random-Liu @tallclair @yujuhong

Pull Request Labels

sig/auth sig/cluster-lifecycle sig/node: Pull Request will be escalated to these SIGs if needed.
priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
kind/feature: New functionality.

Help

tallclair · 2018-06-01T00:49:54Z

pkg/kubelet/config/flags.go

+	// and apiserver will access container runtime directly. This approach is more performant,
+	// but less secure because the connection between apiserver and container runtime is not
+	// authenticated.
+	RedirectContainerStreaming bool


Should this be a FeatureGate instead?

This is more an internal refactoring than a "feature".

We didn't even want to add the flag initially, because we don't want user to care about this. But eventually we decided to do so because we don't have performance data yet, and we want to leave a knob for people who care about the overhead.

If the data in #64006 (comment) makes sense, I even want to remove the flag. :P

tallclair · 2018-06-01T00:53:23Z

pkg/kubelet/config/flags.go

@@ -77,6 +86,7 @@ func (s *ContainerRuntimeOptions) AddFlags(fs *pflag.FlagSet) {
 	// General settings.
 	fs.StringVar(&s.ContainerRuntime, "container-runtime", s.ContainerRuntime, "The container runtime to use. Possible values: 'docker', 'remote', 'rkt (deprecated)'.")
 	fs.StringVar(&s.RuntimeCgroups, "runtime-cgroups", s.RuntimeCgroups, "Optional absolute name of cgroups to create and run the runtime in.")
+	fs.BoolVar(&s.RedirectContainerStreaming, "redirect-container-streaming", s.RedirectContainerStreaming, "Enables container streaming redirect. If false, kubelet will proxy container streaming data between apiserver and container runtime; if true, kubelet will return an http redirect to apiserver, and apiserver will access container runtime directly. The proxy approach is more secure, but introduces some overhead. The redirect approach is more performant, but less secure because the connection between apiserver and container runtime is not authenticated.")


nit: "not authenticated" -> "may not be authenticated"

tallclair · 2018-06-01T00:56:52Z

pkg/kubelet/dockershim/docker_service.go

+		}()
+		go func() {
+			if err := ds.streamingServer.Start(true); err != nil && err != http.ErrServerClosed {
+				glog.Fatalf("Failed to start streaming server: %v", err)


nit: server may have started, maybe "Streaming server stopped unexpectedly: %v"

tallclair · 2018-06-01T00:58:50Z

pkg/kubelet/config/flags.go

@@ -77,6 +86,7 @@ func (s *ContainerRuntimeOptions) AddFlags(fs *pflag.FlagSet) {
 	// General settings.
 	fs.StringVar(&s.ContainerRuntime, "container-runtime", s.ContainerRuntime, "The container runtime to use. Possible values: 'docker', 'remote', 'rkt (deprecated)'.")
 	fs.StringVar(&s.RuntimeCgroups, "runtime-cgroups", s.RuntimeCgroups, "Optional absolute name of cgroups to create and run the runtime in.")
+	fs.BoolVar(&s.RedirectContainerStreaming, "redirect-container-streaming", s.RedirectContainerStreaming, "Enables container streaming redirect. If false, kubelet will proxy container streaming data between apiserver and container runtime; if true, kubelet will return an http redirect to apiserver, and apiserver will access container runtime directly. The proxy approach is more secure, but introduces some overhead. The redirect approach is more performant, but less secure because the connection between apiserver and container runtime is not authenticated.")


nit: Should this be proxy-container-streaming so the false value maintains the previous behavior?

We want proxy to be the default behavior, so that:

Container streaming of all CRI container runtime is more secure.

dockershim can work with crictl (it is not the main goal, but is something we do want)

tallclair · 2018-06-01T01:00:59Z

pkg/kubelet/kubelet.go

-	if kubeDeps.TLSOptions != nil {
-		config.TLSConfig = kubeDeps.TLSOptions.Config
+	if !crOptions.RedirectContainerStreaming {
+		config.Addr = net.JoinHostPort("localhost", "0")


Can this use a unix socket instead of localhost? (more secure)

I can add a TODO.

I decided to go with localhost first because it is simpler. For unix socket, we need to parse URL returned by container runtime to figure out whether it is unix socket (container runtime could still return regular URL), and handle unix socket differently for NewUpgradeAwareHandler.

Follow up is fine, but I'm just curious - did you try this? I don't see anything in NewUpgradeAwareHandler that looks like it would have a problem with a unix URL?

tallclair · 2018-06-01T01:17:00Z

pkg/kubelet/server/server.go

-		s.host.StreamingConnectionIdleTimeout(),
-		remotecommandconsts.DefaultStreamCreationTimeout,
-		remotecommandconsts.SupportedStreamingProtocols)
+	handler := proxy.NewUpgradeAwareHandler(url, nil /*transport*/, false /*wrapTransport*/, false /*upgradeRequired*/, &responder{})


set MaxBytesPerSec, or at least put a TODO

tallclair · 2018-06-01T01:17:51Z

pkg/kubelet/server/server.go

-		remotecommandconsts.DefaultStreamCreationTimeout,
-		remotecommandconsts.SupportedStreamingProtocols)
+	handler := proxy.NewUpgradeAwareHandler(url, nil /*transport*/, false /*wrapTransport*/, false /*upgradeRequired*/, &responder{})
+	handler.ServeHTTP(response.ResponseWriter, request.Request)


nit: refactor this to function, so we can be sure to use the same options for exec, attach & pf

tallclair · 2018-06-01T01:20:34Z

pkg/kubelet/server/server.go

-		s.host.StreamingConnectionIdleTimeout(),
-		remotecommandconsts.DefaultStreamCreationTimeout,
-		remotecommandconsts.SupportedStreamingProtocols)
+	handler := proxy.NewUpgradeAwareHandler(url, nil /*transport*/, false /*wrapTransport*/, false /*upgradeRequired*/, &responder{})


It looks like the apiserver sets upgradeRequired for exec, attach & PF requests, so I think we should do the same.

tallclair · 2018-06-01T01:23:25Z

pkg/kubelet/server/streaming/server.go

@@ -233,10 +235,16 @@ func (s *server) Start(stayUp bool) error {
 		return errors.New("stayUp=false is not yet implemented")
 	}

+	listener, err := net.Listen("tcp", s.config.Addr)


As mentioned elsewhere, can we provide a "unix" option?

k8s-ci-robot · 2018-06-01T01:27:18Z

@Random-Liu: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-node-e2e-containerd	a9fe427319d0f01aefe9dca97943cb445f402bf8	link	`/test pull-kubernetes-node-e2e-containerd`
pull-kubernetes-e2e-containerd-gce	a9fe427319d0f01aefe9dca97943cb445f402bf8	link	`/test pull-kubernetes-e2e-containerd-gce`
pull-kubernetes-kubemark-e2e-gce-big	`746c32d`	link	`/test pull-kubernetes-kubemark-e2e-gce-big`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

fejta-bot · 2018-06-01T04:26:18Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

k8s-github-robot · 2018-06-01T05:45:28Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

Random-Liu · 2018-06-01T07:23:28Z

@tallclair The PR was merged... I'll address your comments in a new PR.

Random-Liu · 2018-06-01T08:01:38Z

I did some simple performance benchmark in a local cluster today.

Benchmark:

Create a local cluster with/without --redirect-container-streaming flag;
Create a busybox pod;
Exec into the busybox pod and run while true; do echo a; done;
Use top to monitor CPU and memory usage of kubelet and apiserver (cadvisor port is disabled now, I decided to use top...)

Result:

	kubelet cpu (cores)	kubelet rss (mb)	apiserver cpu (cores)	apiserver rss (mb)
proxy	1.6	134.7	0.65	402.6
redirect	1.16	134.9	0.87	432.3

It seems that with proxy, kubelet has higher cpu usage, because of the extra hop, but no much memory usage difference.
But with redirect, apiserver has higher cpu and memory usage. I don't know the reason yet, but probably because kubelet proxy helps throttle the stream (?).

The benchmark is very simple. Please feel free to reproduce this.

Anyway, this is the data I get. And if it turns out that redirect has higher apiserver resource usage, I prefer proxy more because apiserver is the actual bottleneck.

@yujuhong @tallclair @mrunalp @feiskyer
/cc @kubernetes/sig-node-bugs

Random-Liu · 2018-06-01T21:20:44Z

Here is the result if I run 2 exec at the same time:

	kubelet cpu (cores)	kubelet rss (mb)	apiserver cpu (cores)	apiserver rss (mb)
proxy	1.7	137.8	0.6	391.7
redirect	1.2	138.4	0.98	428.8

Apiserver still have more source usage in the redirect approach. Interesting...

Signed-off-by: Lantao Liu <lantaol@google.com>

@tallclair

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Address comments in #64006. Address comments in #64006 @tallclair @yujuhong @kubernetes/sig-node-pr-reviews Signed-off-by: Lantao Liu <lantaol@google.com> **Release note**: ```release-note none ```

Signed-off-by: Lantao Liu <lantaol@google.com>

What871As · 2022-04-04T18:25:02Z

[plugins.cri]
stream_server_address = "127.0.0.1"
stream_server_port = "0"
enable_tls_streaming = false

Random-Liu added this to the v1.11 milestone May 18, 2018

Random-Liu assigned yujuhong and tallclair May 18, 2018

k8s-ci-robot requested a review from yifan-gu May 18, 2018 01:43

k8s-github-robot added the milestone/incomplete-labels label May 18, 2018

Random-Liu changed the title ~~Streaming auth~~ Add proxy for container streaming in kubelet for streaming auth. May 18, 2018

Random-Liu force-pushed the streaming-auth branch 2 times, most recently from f1d3c57 to c59bff0 Compare May 18, 2018 01:54

Random-Liu added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. kind/feature Categorizes issue or PR as related to a new feature. labels May 18, 2018

k8s-github-robot added milestone/needs-approval and removed milestone/incomplete-labels labels May 18, 2018

Random-Liu added the status/approved-for-milestone label May 18, 2018

k8s-github-robot removed the milestone/needs-approval label May 18, 2018

Random-Liu force-pushed the streaming-auth branch from c59bff0 to 2ca85a4 Compare May 18, 2018 02:09

mikebrow reviewed May 18, 2018

View reviewed changes

Random-Liu force-pushed the streaming-auth branch from 2ca85a4 to 39eaab4 Compare May 18, 2018 23:09

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 18, 2018

Random-Liu force-pushed the streaming-auth branch from 39eaab4 to 3f133f1 Compare May 18, 2018 23:17

Random-Liu force-pushed the streaming-auth branch from 3f133f1 to d42b9a6 Compare May 18, 2018 23:26

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2018

k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Jun 1, 2018

tallclair reviewed Jun 1, 2018

View reviewed changes

k8s-github-robot merged commit 8d10a8f into kubernetes:master Jun 1, 2018

Random-Liu deleted the streaming-auth branch June 1, 2018 07:23

Random-Liu added a commit to Random-Liu/kubernetes that referenced this pull request Jun 2, 2018

Address comments in kubernetes#64006.

9677616

Signed-off-by: Lantao Liu <lantaol@google.com>

Random-Liu mentioned this pull request Jun 2, 2018

Address comments in #64006. #64644

Merged

feiskyer mentioned this pull request Jun 11, 2018

crictl exec and dockershim only work with synchronous execution kubernetes-sigs/cri-tools#283

Closed

wenjiaswe pushed a commit to wenjiaswe/kubernetes that referenced this pull request Jun 19, 2018

Address comments in kubernetes#64006.

42120e4

Signed-off-by: Lantao Liu <lantaol@google.com>

Random-Liu mentioned this pull request Jun 26, 2018

Set stream server to serve on localhost on GCE. containerd/cri#829

Merged

This was referenced Jul 20, 2018

certificate signed by unknown authority when installing with kubeadm containerd/cri#856

Closed

Why does docker-containerd listen on external interface ? containerd/containerd#2483

Closed

runcom mentioned this pull request Jul 31, 2018

server: serve streaming on localhost on a random port cri-o/cri-o#1714

Merged

jakolehm mentioned this pull request Aug 7, 2018

Cri-o should use --redirect-container-streaming=false kontena/pharos-cluster#508

Closed

Random-Liu mentioned this pull request Sep 1, 2018

stream: can use user certificates containerd/cri#886

Merged

Random-Liu mentioned this pull request Sep 17, 2018

Update cri to 9f39e3289533fc228c5e5fcac0a6dbdd60c6047b. containerd/containerd#2656

Merged

yujuhong mentioned this pull request Sep 22, 2018

Add verification to apiserver redirect following #66516

Merged

Add proxy for container streaming in kubelet for streaming auth. #64006

Add proxy for container streaming in kubelet for streaming auth. #64006

Conversation

Random-Liu commented May 18, 2018 • edited

Random-Liu commented May 18, 2018

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

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

Random-Liu commented May 18, 2018

Random-Liu commented May 18, 2018 • edited

Random-Liu commented May 18, 2018

k8s-ci-robot commented Jun 1, 2018

tallclair commented Jun 1, 2018

k8s-github-robot commented Jun 1, 2018

Choose a reason for hiding this comment

Random-Liu Jun 1, 2018 • edited

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

Random-Liu Jun 1, 2018 • edited

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

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

k8s-ci-robot commented Jun 1, 2018 • edited

fejta-bot commented Jun 1, 2018

k8s-github-robot commented Jun 1, 2018

Random-Liu commented Jun 1, 2018

Random-Liu commented Jun 1, 2018 • edited

Random-Liu commented Jun 1, 2018 • edited

What871As commented Apr 4, 2022

Random-Liu commented May 18, 2018 •

edited

Random-Liu commented May 18, 2018 •

edited

Random-Liu Jun 1, 2018 •

edited

Random-Liu Jun 1, 2018 •

edited

k8s-ci-robot commented Jun 1, 2018 •

edited

Random-Liu commented Jun 1, 2018 •

edited

Random-Liu commented Jun 1, 2018 •

edited