Get rid of socat for port forwarding by aojea · Pull Request #1470 · containerd/cri

aojea · 2020-05-06T10:14:57Z

use goroutines to copy the data from the stream to the TCP
connection, and viceversa, removing the socat dependency.

Quoting Lantao Liu, the logic is as follow:

When one side (either pod side or user side) of portforward
is closed, we should stop port forwarding.

When one side is closed, the io.Copy use that side as source will close,
but the io.Copy use that side as dest won't.

Fixes: #730

k8s-ci-robot · 2020-05-06T10:15:02Z

Hi @aojea. 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.

aojea · 2020-05-06T10:16:09Z

xref kubernetes-sigs/kind#1560

aojea · 2020-05-06T10:17:21Z

/cc @BenTheElder @Random-Liu @howardjohn

k8s-ci-robot · 2020-05-06T10:17:23Z

@aojea: GitHub didn't allow me to request PR reviews from the following users: howardjohn.

Note that only containerd members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @benthelder @Random-Liu @howardjohn

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.

pkg/server/sandbox_portforward_unix.go

BenTheElder · 2020-05-07T22:21:31Z

cc @dims @mikebrow

dims · 2020-05-07T22:42:44Z

in general looks good to me, i'd like to see if we can raise @Random-Liu to take a look.

mikebrow · 2020-05-07T23:02:23Z

/ok-to-test

mikebrow

see questions and comments

pkg/server/sandbox_portforward_unix.go

dims · 2020-05-08T10:06:28Z

changed my mind. Let's wrap up Mike's comments and get this in to iterate. (no need to wait for Lantao)

pkg/server/sandbox_portforward_unix.go

mikebrow · 2020-05-08T16:45:22Z

sry each commit has to be signed :)

pkg/server/sandbox_portforward_unix.go

mikebrow

looking great.. just a couple small comments on err value and debug output..

pkg/server/sandbox_portforward_unix.go

use goroutines to copy the data from the stream to the TCP connection, and viceversa, removing the socat dependency. Quoting Lantao Liu, the logic is as follow: When one side (either pod side or user side) of portforward is closed, we should stop port forwarding. When one side is closed, the io.Copy use that side as source will close, but the io.Copy use that side as dest won't. Signed-off-by: Antonio Ojea <antonio.ojea.garcia@gmail.com>

We encountered two failing end-to-end tests after the adoption of containerd#1470 in cri-o/cri-o#3749: ``` Summarizing 2 Failures: [Fail] [sig-cli] Kubectl Port forwarding With a server listening on 0.0.0.0 that expects a client request [It] should support a client that connects, sends DATA, and disconnects test/e2e/kubectl/portforward.go:343 [Fail] [sig-cli] Kubectl Port forwarding With a server listening on localhost that expects a client request [It] should support a client that connects , sends DATA, and disconnects test/e2e/kubectl/portforward.go:343 ``` Increasing the timeout to 1s fixes the issue. Signed-off-by: Sascha Grunert <sgrunert@suse.com>

This is the analogous implementation to containerd/cri#1470 to remove the socat runtime dependency from CRI-O. Signed-off-by: Sascha Grunert <sgrunert@suse.com>

saschagrunert · 2020-05-13T07:34:08Z

pkg/server/sandbox_portforward_unix.go

+		// TODO: hardcoded to tcp4 because localhost resolves to ::1 by default if the system has IPv6 enabled.
+		// Theoretically happy eyeballs will try IPv6 first and fallback to IPv4
+		// but resolving localhost doesn't seem to return and IPv4 address, thus failing the connection.
+		conn, err := net.Dial("tcp4", fmt.Sprintf("localhost:%d", port))


@aojea could we do something like this?

Suggested change

conn, err := net.Dial("tcp4", fmt.Sprintf("localhost:%d", port))

conn, err := net.Dial("tcp4", fmt.Sprintf("%s:%d", s.IP, port))

I was just keeping the old behaviour because of my ignorance on how the ports are exposed inside the namespace.

I can explain at the network level the problem and why I think is forcing IPv4 here.
localhost, depending on some factors, may resolve to [::1] or 127.0.0.1, Long history short, commonly it resolves to [::1] if you are more interested in the whole history I explain it in this link https://gist.github.com/aojea/94f6f483173641647c731f582e52f0b0

But typically, the applications use to listen only 127.0.0.1 inside the container 🤷 , that's the part I'm missing.

If we are able to identify correctly the address that the app is listening, then is better using it, so this works for both IPv4 and IPv6, but I prefer having @mikebrow guidance here.

it's hardcoded localhost(or 127.0.0.1):random port for sec. reasons... or at least that's what the decision was way back when.

Any change here would likely need a kep that sets up streaming ip(s) and port(s) at the CRI request and/or response level.

This is the analogous implementation to containerd/cri#1470 to remove the socat runtime dependency from CRI-O. Signed-off-by: Sascha Grunert <sgrunert@suse.com>

We encountered two failing end-to-end tests after the adoption of containerd#1470 in cri-o/cri-o#3749: ``` Summarizing 2 Failures: [Fail] [sig-cli] Kubectl Port forwarding With a server listening on 0.0.0.0 that expects a client request [It] should support a client that connects, sends DATA, and disconnects test/e2e/kubectl/portforward.go:343 [Fail] [sig-cli] Kubectl Port forwarding With a server listening on localhost that expects a client request [It] should support a client that connects , sends DATA, and disconnects test/e2e/kubectl/portforward.go:343 ``` Increasing the timeout to 1s fixes the issue. Signed-off-by: Sascha Grunert <sgrunert@suse.com>

This is the analogous implementation to containerd/cri#1470 to remove the socat runtime dependency from CRI-O. Signed-off-by: Sascha Grunert <sgrunert@suse.com>

This is no longer true as of `containerd` v1.4.0 because [0] removes `socat` dependency for port-forwarding. For lower versions of `containerd`, the error message is descriptive i.e. `"socat": executable file not found in $PATH`. [0] containerd/cri#1470

This is no longer true as of `containerd` v1.4.0 because [0] removes `socat` dependency for port-forwarding. For lower versions of `containerd`, the error message is descriptive i.e. `"socat": executable file not found in $PATH`. [0] containerd/cri#1470 Kubernetes-commit: 64fca6bda7d36b9cc1667aa317896b540d4dcf8c

k8s-ci-robot added needs-ok-to-test size/L labels May 6, 2020

aojea changed the title ~~replace socat for port forwarding~~ Get rid of socat for port forwarding May 6, 2020

k8s-ci-robot requested a review from Random-Liu May 6, 2020 10:17

aojea force-pushed the gocat branch 3 times, most recently from 487af78 to 820c134 Compare May 6, 2020 12:17

saschagrunert reviewed May 6, 2020

View reviewed changes

pkg/server/sandbox_portforward_unix.go Outdated Show resolved Hide resolved

pkg/server/sandbox_portforward_unix.go Outdated Show resolved Hide resolved

aojea force-pushed the gocat branch 3 times, most recently from ea07c1b to 2175e78 Compare May 6, 2020 13:29

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels May 7, 2020

mikebrow reviewed May 7, 2020

View reviewed changes

aojea force-pushed the gocat branch from 2175e78 to cfe1e72 Compare May 8, 2020 08:22

aojea force-pushed the gocat branch from cfe1e72 to 98090e8 Compare May 8, 2020 11:46

fuweid reviewed May 8, 2020

View reviewed changes

pkg/server/sandbox_portforward_unix.go Show resolved Hide resolved

mikebrow reviewed May 8, 2020

View reviewed changes

pkg/server/sandbox_portforward_unix.go Outdated Show resolved Hide resolved

pkg/server/sandbox_portforward_unix.go Show resolved Hide resolved

aojea force-pushed the gocat branch from 5e7b3e4 to b6fd9a6 Compare May 8, 2020 18:04

mikebrow reviewed May 8, 2020

View reviewed changes

pkg/server/sandbox_portforward_unix.go Show resolved Hide resolved

pkg/server/sandbox_portforward_unix.go Show resolved Hide resolved

aojea mentioned this pull request May 12, 2020

[Flaky test] [sig-cli] Kubectl Port forwarding With a server listening on localhost that expects a client request should support a client that connects, sends DATA, and disconnects kubernetes/kubernetes#86355

Closed

saschagrunert reviewed May 13, 2020

View reviewed changes

aojea mentioned this pull request Jun 4, 2020

Generic port forwarding kubernetes/enhancements#1846

Closed

BenTheElder mentioned this pull request Jul 11, 2020

port-forwarding broken in kindest/node:v1.18.2 but works on kindest/node:v1.18.0 kubernetes-sigs/kind#1723

Closed

hdp617 mentioned this pull request Mar 30, 2021

doc: remove description that socat is required kubernetes/kubernetes#100679

Merged

raulcabello mentioned this pull request Oct 8, 2021

Remove socat as it is not needed anymore k3s-io/k3s-root#42

Closed

neoaggelos mentioned this pull request Nov 1, 2023

Cleanup unused snap dependencies canonical/microk8s#4271

Merged

	conn, err := net.Dial("tcp4", fmt.Sprintf("localhost:%d", port))
	conn, err := net.Dial("tcp4", fmt.Sprintf("%s:%d", s.IP, port))

Conversation

aojea commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented May 6, 2020

Uh oh!

aojea commented May 6, 2020

Uh oh!

aojea commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented May 6, 2020

Uh oh!

Uh oh!

Uh oh!

BenTheElder commented May 7, 2020

Uh oh!

dims commented May 7, 2020

Uh oh!

mikebrow commented May 7, 2020

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dims commented May 8, 2020

Uh oh!

Uh oh!

mikebrow commented May 8, 2020

Uh oh!

Uh oh!

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

saschagrunert May 13, 2020

Choose a reason for hiding this comment

Uh oh!

aojea May 13, 2020 • 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!

mikebrow May 13, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

aojea commented May 6, 2020 •

edited

Loading

aojea commented May 6, 2020 •

edited

Loading

aojea May 13, 2020 •

edited

Loading