Add portforward support. #130

Random-Liu · 2017-08-14T02:47:09Z

Add port forward support. The implementation is the same with the dockershim.

Signed-off-by: Lantao Liu [email protected]

AkihiroSuda · 2017-08-14T02:56:39Z

pkg/server/streaming.go


 func (s *streamRuntime) PortForward(podSandboxID string, port int32, stream io.ReadWriteCloser) error {
-	return errors.New("not implemented")
+	if port < 0 || port > math.MaxUint16 {


port <= 0 ?

The orignal code in kubelet is using port < 0. However, you are right, we should check ==0 as well. Will update.

mikebrow · 2017-08-15T17:00:35Z

pkg/server/sandbox_portforward.go

 	"k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime"
 )

 // PortForward prepares a streaming endpoint to forward ports from a PodSandbox, and returns the address.


Comment does not follow what is happening in the api. Currently it's validating that the sandbox is running then returns the serving URL for specified streaming port.

The comment is the same with dockershim https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/dockershim/docker_streaming.go#L115.
And I think it explains what the function is doing. :)

ok I'm confused .. where does the api "prepare a streaming endpoint to forward ports from a PodSandbox" all I see is ensuring the running state for the sandbox and calling GetPortForward which I presume just does a get?

GetPortForward prepares the streaming endpoint and returns the URL.

mikebrow · 2017-08-15T17:04:10Z

pkg/server/sandbox_portforward.go

+}
+
+// portForward requires `nsenter` and `socat` on the node.
+func (c *criContainerdService) portForward(id string, port int32, stream io.ReadWriteCloser) error {


should this api be here or over in streaming.go

I put attach, portForward and exec in corresponding source file.

Don't understand the answer. There is a PortForward api in streaming.go that calls portForward. The PortForward api in sandbox_portforward.go does not call portForward. Thus my question.

mikebrow · 2017-08-15T17:12:30Z

pkg/server/sandbox_portforward.go

+		return nil, errors.New("sandbox container is not running")
+	}
+	// TODO(random-liu): Verify that ports are exposed.
+	return c.streamServer.GetPortForward(r)


Curious.. why all the above validation on the running status? Are there expected errors if the sandbox isn't running?

Portforward nsenters a the pause container namespace, and running a socat server to do portforwarding.

I believe this requires the pause container is running, because we use the pid to find it's namespace.

We could probably use permanent network namespace to prevent this in the future.

Does the GetPortForward api require we make sure this is the case, or will it return an appropriate error if we have not. IOW are we double checking what the API is going to check anyway?

streamServer doesn't even know CRI, it can't check container status.

not what I meant but ok..

I presumed you wanted to ensure there is a pid and associated namespace before calling.. and also presumed that the streamServer would also check and return an error message if not vs using a static table of some sort. But I'm not looking at it's code so..

I see.. one of "those" apis :-) should probably be called Reserve instead of Get.. now it makes sense.

Just for my understanding :
Even if the task is paused/stopped the namespace is still available to nsenter right?
How is this supposed to work with windows in the future.

Even if the task is paused/stopped the namespace is still available to nsenter right?

Offline talked with @yujuhong. Yeah, we only need network namespace here. I had a concern that we run a pod specific process but only in its network namespace, it means that the process could access other host namespaces. However, socat is on the node, the cluster admin should make sure it's friendly. :)

Actually, there are other issues with the current solution - the socat is not in pod cgroup, so the resource usage will be charged to the system.

How is this supposed to work with windows in the future.

@abhinandanpb Good question. Currently, Kubelet only has limited windows support. Many critical kubelet features doesn't have corresponding windows support, e.g. pod level cgroup, cadvisor monitoring etc. There is an ongoing effort to make kubelet work better on windows.

Actually, we should be able to run a port forward container inside the pod sandbox to do port forward, which makes more sense because:

The resource overhead will be charged to the pod;

If containerd container exec support different platforms, this solution could be platform agnostic.

I'll add a TODO here, we could do this in near future. :)

got it. Thanks for explaining.

mikebrow · 2017-08-15T17:12:58Z

pkg/server/sandbox_portforward.go

+	return c.streamServer.GetPortForward(r)
+}
+
+// portForward requires `nsenter` and `socat` on the node.


maybe some explanation about what the api does.

mikebrow · 2017-08-15T17:13:54Z

pkg/server/sandbox_portforward.go

+		return fmt.Errorf("unable to find socat: %v", err)
+	}
+
+	args := []string{"-t", fmt.Sprintf("%d", pid), "-n", socat,


maybe a comment or pointer about the options being used.

Good idea. Will do.

mikebrow · 2017-08-15T17:28:01Z

pkg/server/sandbox_portforward.go

+	if err := cmd.Run(); err != nil {
+		return fmt.Errorf("nsenter command returns error: %v, stderr: %q", err, stderr.String())
+	}
+


should we use a channel to wait on the go func io.Copy err? Maybe pipe the err in the go func after the in.Close() call and wait on the channel before the final return..?

I think we want the goroutine to keep serving even after the function returns, until the client disconnects.

kk didn't know that was a long running task. Maybe a comment about that..

I'll add some comment to describe what port forward is doing.

Actually, you are correct, we should wait on the go func. Will update.

The implementation is the same with https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/dockershim/docker_streaming.go#L162. And based on the original comment and code analysis, the code should be safe.

I added more logs so that if in the future there are any problem, we could figure it out.

Logging works if we have the right test buckets... hate leaving what might be a dangling pipe thread but at least we'll have an unbalanced lvl 4 log to identify it if it happens.

@mikebrow Run already contains a Wait, and the wait makes sure that the stream will be closed. And there is no context passed in by streaming server, so there is no cancellation, either.

If we don't use Run and a context is passed in, I'd prefer adding a channel and do select. :)

Makes sense.

mikebrow

See comments.

Random-Liu · 2017-08-16T05:36:23Z

@mikebrow Addressed comments.

mikebrow

/LGTM

abhi · 2017-08-16T13:01:28Z

pkg/server/sandbox_portforward.go

+	}
+	id := sandbox.ID
+
+	info, err := c.taskService.Get(ctx, &tasks.GetTaskRequest{ContainerID: id})


Can you please add a TODO to replace this with client API

or TODO to use permanent network namespace ( discussed below)

abhi · 2017-08-16T14:02:44Z

Minor comments, LGTM

Random-Liu · 2017-08-16T18:27:24Z

@abhinandanpb Addressed comments. Thanks for reviewing!

Signed-off-by: Lantao Liu <[email protected]>

Random-Liu · 2017-08-17T00:47:29Z

Just updated this PR to use the containerd client.

Add portforward support.

k8s-ci-robot added the cncf-cla: yes label Aug 14, 2017

Random-Liu assigned mikebrow Aug 14, 2017

Random-Liu force-pushed the add-port-forward branch 2 times, most recently from b0db171 to ae91a27 Compare August 14, 2017 02:55

AkihiroSuda reviewed Aug 14, 2017

View reviewed changes

Random-Liu force-pushed the add-port-forward branch from ae91a27 to b9505ba Compare August 14, 2017 03:01

mikebrow reviewed Aug 15, 2017

View reviewed changes

Random-Liu force-pushed the add-port-forward branch from e414d44 to 40ac337 Compare August 16, 2017 05:50

mikebrow approved these changes Aug 16, 2017

View reviewed changes

mikebrow added the lgtm label Aug 16, 2017

Random-Liu force-pushed the add-port-forward branch from 40ac337 to 0e5404b Compare August 16, 2017 06:46

abhi reviewed Aug 16, 2017

View reviewed changes

Random-Liu force-pushed the add-port-forward branch from 0e5404b to ebe41b1 Compare August 16, 2017 18:26

Add portforward support.

f555bb1

Signed-off-by: Lantao Liu <[email protected]>

Random-Liu force-pushed the add-port-forward branch from ebe41b1 to f555bb1 Compare August 17, 2017 00:13

Random-Liu merged commit 860dac2 into containerd:master Aug 17, 2017

Random-Liu deleted the add-port-forward branch August 17, 2017 00:47

Random-Liu mentioned this pull request Aug 17, 2017

CRI-Containerd Missing Pieces #62

Closed

42 tasks

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

Merge pull request containerd#130 from Random-Liu/add-port-forward

82fd596

Add portforward support.

Add portforward support. #130

Add portforward support. #130

Uh oh!

Conversation

Random-Liu commented Aug 14, 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!

mikebrow Aug 15, 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!

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 Aug 16, 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!

Random-Liu Aug 16, 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!

mikebrow Aug 15, 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!

mikebrow Aug 15, 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!

mikebrow Aug 15, 2017 •

edited

Loading

Random-Liu Aug 16, 2017 •

edited

Loading

Random-Liu Aug 16, 2017 •

edited

Loading

mikebrow Aug 15, 2017 •

edited

Loading

mikebrow Aug 15, 2017 •

edited

Loading

Random-Liu Aug 16, 2017 •

edited

Loading

Random-Liu commented Aug 16, 2017 •

edited

Loading