Add a shim API for port forwarding. #4814

ianlewis · 2020-12-08T00:43:54Z

This allows shims to implement an optional port forwarding service. This
is useful for runtimes that remove the IP address from the veth for a
user space network stack and can't support port forwarding via the
network namespace.

This implements Option 1b described here: kubernetes/enhancements#1846

Summary:

This PR adds a new optional PortForward grpc service that shims can implement. The service has a single PortForward rpc endpoint that accepts a port number and UDS address. Containerd will attempt to use the PortForward endpoint to initiate port forwarding and fall back to existing logic on the container's network namespace if unsuccessful. Containerd will listen on the UDS address and shims/runtimes are expected to establish a single connection on this address to establish bi-directional communication, forwarding data to/from the container's port to the UDS connection.

As networking is out of scope of containerd itself, a new PortForward plugin is introduced to allow the CRI plugin to make calls to task implementations directly. The CRI plugin uses this plugin to call runtime/v2 implementations directly and returns an error if the task is not a runtime/v2 implementation that implements port forwarding. Only runtime/v2 implementations that are shims are expected to implement this interface.

k8s-ci-robot · 2020-12-08T00:44:03Z

Hi @ianlewis. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

theopenlab-ci · 2020-12-08T00:50:52Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 38s (non-voting)

ianlewis · 2020-12-08T01:05:50Z

/cc @mikebrow

theopenlab-ci · 2020-12-08T01:24:10Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 17m 40s (non-voting)

theopenlab-ci · 2021-03-15T06:20:35Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 48s (non-voting)

theopenlab-ci · 2021-04-15T06:00:49Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 7m 01s (non-voting)

theopenlab-ci · 2021-04-26T09:09:19Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 47s (non-voting)

This allows shims to implement an optional port forwarding service. This is useful for runtimes that remove the IP address from the veth for a user space network stack and can't support port forwarding via the network namespace.

theopenlab-ci · 2021-04-26T09:29:12Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 47s (non-voting)

theopenlab-ci · 2021-04-27T02:16:16Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 43s (non-voting)
containerd-test-arm64 : SUCCESS in 5m 34s (non-voting)
containerd-integration-test-arm64 : SUCCESS in 22m 40s (non-voting)

mikebrow

some comments.. request to get engagement with some more core runtime maintainers

mikebrow · 2021-05-10T22:59:12Z

runtime/v2/binary.go

 	"github.com/containerd/containerd/runtime"
 	client "github.com/containerd/containerd/runtime/v2/shim"
-	"github.com/containerd/containerd/runtime/v2/task"
+	shimapi "github.com/containerd/containerd/runtime/v2/task"


maybe taskAPI

I started doing this partly because it was given this name in runtime/v2/shim/shim.go and it seems like this package name is becoming dated given that we may be adding more optional services to the shim.

e.g. https://github.com/containerd/containerd/pull/5396/files#diff-a90594dff4315b8444427fbaf0c64c6febd6a08ed6b2db5fb706b53ff3f97f79

mikebrow · 2021-05-10T23:22:34Z

pkg/cri/cri.go

 	if err != nil {
 		return nil, errors.Wrap(err, "failed to create containerd client")
 	}
+	pf, err := getPortForwardService(ic)


Did we need to figure out a way to make this per runtime?

My intention was to allow shims to implement port forward. I don't think we had a need to implement it per "runtime" (i.e. v1, v2, v2/shim). My intention was for 'pf' to return an error in cases where it wasn't supported and fall back to the previous logic. I do that around line 75 in sandbox_portforward_linux.go.

I'm open to thinking about it but I can't really imagine how it would work unless we push the code in localPortForward in sandbox_portforward_linux.go into runtime/v2 somewhere. I don't think we really want to do that.

mikebrow · 2021-05-10T23:24:44Z

pkg/cri/cri.go

 	}
+	pf, err := getPortForwardService(ic)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to get port forward service")


should we ignore if there is no port forward service to fall back to the prior behavior..

@dmcgowan @crosbymichael we should do a call I think with Ian to go over this

Yeah, I think it might be good to meet again. I can probably speak more intelligently now that I've taken a stab at implementing this.

I sent an invite for Wed. Hopefully that works.

mikebrow · 2021-05-10T23:43:55Z

runtime/v2/shim/shim.go

 	}

-	logrus.Debug("registering ttrpc server")
+	logrus.Debug("registering task service")


maybe registering task service via ttrpc

ianlewis · 2021-05-10T23:49:32Z

pkg/cri/server/sandbox_portforward_linux.go

 func (c *criService) portForward(ctx context.Context, id string, port int32, stream io.ReadWriteCloser) error {
-	s, err := c.sandboxStore.Get(id)
+	// Attempt to start port forwarding via the port forwarding client.
+	// NOTE: Do not use volitile root dir as it amy exceed max length for UDS addr.


"may exceed"

dmcgowan · 2021-05-19T22:45:48Z

runtime/v2/shim.go

-		rtTasks: rt,
+		client:      client,
+		task:        shimapi.NewTaskClient(client),
+		portforward: shimapi.NewPortForwardClient(client),


If the shim could just return the client here, then the port forward client could be completely separated here

stevvooe · 2021-07-20T16:46:16Z

runtime/v2/task/shim.proto

+}
+
+message PortForwardRequest {
+	// The container id.


Suggested change

// The container id.

// ID is the container id.

stevvooe · 2021-07-20T16:47:05Z

runtime/v2/task/shim.proto

+message PortForwardRequest {
+	// The container id.
+	string id = 1;
+	// The port to be forwarded inside the container.


Suggested change

// The port to be forwarded inside the container.

// Port sets the port to be forward inside the container.

stevvooe · 2021-07-20T16:47:23Z

runtime/v2/task/shim.proto

+	string id = 1;
+	// The port to be forwarded inside the container.
+	int32 port = 2;
+	// A Unix domain socket address


Suggested change

// A Unix domain socket address

// Addr is the unix domain socket address.

stevvooe · 2021-07-20T16:47:40Z

runtime/v2/task/shim.proto

+	rpc PortForward(PortForwardRequest) returns (google.protobuf.Empty);
+}
+
+message PortForwardRequest {


Also, make sure to add some documentation to the message type.

AkihiroSuda · 2021-09-08T13:04:29Z

Needs rebase

ianlewis · 2022-11-15T00:36:33Z

Closing this. @zkoopmans can create a new PR using this as a reference.

k8s-ci-robot added the needs-ok-to-test label Dec 8, 2020

ianlewis force-pushed the port-forward branch 2 times, most recently from 1203211 to 92dfc8d Compare December 8, 2020 01:05

k8s-ci-robot requested a review from mikebrow December 8, 2020 01:05

ianlewis force-pushed the port-forward branch from 92dfc8d to f3b6130 Compare March 15, 2021 06:13

ianlewis force-pushed the port-forward branch from f3b6130 to 8bde50b Compare April 15, 2021 05:52

ianlewis force-pushed the port-forward branch from 8bde50b to ae49a7f Compare April 26, 2021 09:03

Add a shim API for port forwarding.

2f4393e

This allows shims to implement an optional port forwarding service. This is useful for runtimes that remove the IP address from the veth for a user space network stack and can't support port forwarding via the network namespace.

ianlewis force-pushed the port-forward branch from ae49a7f to 2f4393e Compare April 26, 2021 09:22

ianlewis changed the title ~~[WIP] Add port forwarding to shim API~~ Add a shim API for port forwarding. Apr 26, 2021

ianlewis added 2 commits April 26, 2021 21:38

Add documentation comments.

f832fa8

Revert next.pb.txt

01d25dd

mikebrow reviewed May 10, 2021

View reviewed changes

ianlewis commented May 10, 2021

View reviewed changes

dmcgowan reviewed May 19, 2021

View reviewed changes

dmcgowan added the status/needs-discussion Needs discussion and decision from maintainers label May 27, 2021

mxpv mentioned this pull request Jul 15, 2021

[Proposal] Shim service #5742

Closed

stevvooe reviewed Jul 20, 2021

View reviewed changes

ianlewis mentioned this pull request Oct 13, 2022

Sandbox API work #7312

Closed

17 tasks

ianlewis mentioned this pull request Nov 14, 2022

Generic port forwarding kubernetes/enhancements#1846

Closed

ianlewis closed this Nov 15, 2022

antonr-p2p mentioned this pull request Mar 5, 2024

Support portforward for kata and gVisor #6638

Open

	// The port to be forwarded inside the container.
	// Port sets the port to be forward inside the container.

	// A Unix domain socket address
	// Addr is the unix domain socket address.

Add a shim API for port forwarding. #4814

Add a shim API for port forwarding. #4814

Uh oh!

Conversation

ianlewis commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Dec 8, 2020

Uh oh!

theopenlab-ci bot commented Dec 8, 2020

Uh oh!

ianlewis commented Dec 8, 2020

Uh oh!

theopenlab-ci bot commented Dec 8, 2020

Uh oh!

theopenlab-ci bot commented Mar 15, 2021

Uh oh!

theopenlab-ci bot commented Apr 15, 2021

Uh oh!

theopenlab-ci bot commented Apr 26, 2021

Uh oh!

theopenlab-ci bot commented Apr 26, 2021

Uh oh!

theopenlab-ci bot commented Apr 27, 2021

Uh oh!

mikebrow left a comment

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!

mikebrow May 10, 2021 • 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!

AkihiroSuda commented Sep 8, 2021

Uh oh!

ianlewis commented Nov 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ianlewis commented Dec 8, 2020 •

edited

Loading

mikebrow May 10, 2021 •

edited

Loading