allow switching of port-forward approaches when rootless/using slirp4netns #6324

aleks-mariusz · 2020-05-21T16:53:36Z

This PR is by request against master (the other original PR was against the 1.9 branch as that's what i was asked to base it off of).

As of podman 1.8.0, because of commit da7595a, the default approach of providing port-forwarding in rootless mode has switched (and been hard-coded) to rootlessport, for the purpose of providing super performance. The side-effect of this switch is source within the container to the port-forwarded service always appears to originate from 127.0.0.1 (see issue #5138).

This commit allows a user to specify if they want to revert to the previous approach
of leveraging slirp4netns add_hostfwd() api which, although not as stellar performance,
restores usefulness of seeing incoming traffic origin IP addresses.

The change should be transparent; when not specified, rootlessport will continue to be
used, however if specifying --net slirp4netns:slirplisten the old approach will be used.

Testing performed in rootless mode of relevant functionality:

Default behaviour scenario 1 (nothing specified, so not taking advantage of this commit original behaviour (higher performance rootlessport in use))

[test@test ~]$ podman run -d --name nginx -p 8080:80 docker.io/library/nginx:alpine
c0644bcd50c950a000305d0e548e3b1c40732af5c1f9bcbe1dffba244fc1a048
[test@test ~]$ podman logs -f nginx
127.0.0.1 - - [28/Apr/2020:18:19:47 +0000] "GET / HTTP/1.1" 304 0 "-" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/81.0.4044.122 Safari/537.36" "-"

Default behaviour scenario 2 (specifying just --net slirp4netns, which is default when in rootless mode)

[test@test ~]$ podman run -d --name nginx -p 8080:80 --net slirp4netns docker.io/library/nginx:alpine
82e93b571cb013b05f586097544c081c78335f932a5fae071d27ca0f24014cf0
[test@test ~]$ podman logs -f nginx
127.0.0.1 - - [28/Apr/2020:18:20:10 +0000] "GET / HTTP/1.1" 304 0 "-" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/81.0.4044.122 Safari/537.36" "-"

Restored (old, pre-1.8.0) behaviour, where source IP show true origin of port-forwarded traffic.

[test@test ~]$ podman run -d --name nginx -p 8080:80 --net slirp4netns:slirplisten docker.io/library/nginx:alpine
4c5f517d20df3bfc8767371cfefbe87835eb0a0e4e6c300c24b306c80fd3f59f
[test@test ~]$ podman logs -f nginx
192.168.0.22 - - [28/Apr/2020:18:19:14 +0000] "GET / HTTP/1.1" 304 0 "-" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/81.0.4044.122 Safari/537.36" "-"

Note: the above may imply the restored port-forwarding via slirp4netns is not as performant as the new rootlessport approach, however the figures shared in the original commit that introduced rootlessport are as follows: socat: 5.2 Gbps, slirp4netns: 8.3 Gbps, RootlessKit: 27.3 Gbps, which are more than sufficient for many use cases where the origin of traffic is more important than limits that cannot be reached due to bottlenecks elsewhere.

Signed-off-by: Aleks Mariusz <[email protected]>

openshift-ci-robot · 2020-05-21T16:53:44Z

Hi @aleks-mariusz. Thanks for your PR.

I'm waiting for a containers 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.

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.

aleks-mariusz · 2020-05-21T17:41:36Z

/assign @TomSweeneyRedHat

jwhonce · 2020-05-22T01:48:45Z

/ok-to-test

giuseppe · 2020-05-22T14:16:15Z

LGTM

TomSweeneyRedHat · 2020-05-22T14:19:43Z

LGTM

mheon · 2020-05-22T14:24:58Z

Can the manpages be updated to include the new network modes?

rhatdan · 2020-05-22T14:50:00Z

/approve

pkg/namespaces/namespaces.go

vrothberg · 2020-06-04T12:01:18Z

Friendly ping. Once https://github.com/containers/libpod/pull/6324/files/b9893ce95307629cfc54bd8707859c6c2bb146b6#diff-6018b2b043bbcbdd9ac0f143ac9458be is addressed, we can merge.

rhatdan · 2020-06-05T09:39:09Z

@aleks-mariusz Are you still working on this. Simple change and then we merge.

aleks-mariusz · 2020-06-05T12:17:58Z

Sorry, just re-pushed the commit w/ the change

openshift-ci-robot · 2020-06-05T12:31:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AkihiroSuda, aleks-mariusz, rhatdan

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:

~~OWNERS~~ [rhatdan]

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

giuseppe · 2020-06-08T06:34:04Z

/retest

LGTM

TomSweeneyRedHat · 2020-06-08T14:12:09Z

Tests aren't looking all that hip

rhatdan · 2020-06-09T19:35:38Z

@aleks-mariusz Please fix the tests might need a rebase.

giuseppe · 2020-06-16T09:33:54Z

@aleks-mariusz could you please rebase and push again?

aleks-mariusz · 2020-06-16T10:03:08Z

bare with me, trying to fix this now.. rebasing off master did not seem to be the right move (or i didn't do it correctly)

aleks-mariusz · 2020-06-16T10:39:10Z

@giuseppe can we try again please? i messed up the previously attempt (rebased off a stale master), and now have hand re-applied the patch to a new branch based off latest master, and re-pushed now..

giuseppe · 2020-06-16T12:26:31Z

/retest

giuseppe · 2020-06-22T07:34:10Z

/retest

giuseppe · 2020-06-22T07:36:54Z

@aleks-mariusz I'd like to get this feature merged. Could we try to rebase again? For rebasing, it should be enough to do:

git pull --rebase origin master
git push -f ...

As of podman 1.8.0, because of commit da7595a, the default approach of providing port-forwarding in rootless mode has switched (and been hard-coded) to rootlessport, for the purpose of providing super performance. The side-effect of this switch is source within the container to the port-forwarded service always appears to originate from 127.0.0.1 (see issue containers#5138). This commit allows a user to specify if they want to revert to the previous approach of leveraging slirp4netns add_hostfwd() api which, although not as stellar performance, restores usefulness of seeing incoming traffic origin IP addresses. The change should be transparent; when not specified, rootlessport will continue to be used, however if specifying --net slirp4netns:slirplisten the old approach will be used. Note: the above may imply the restored port-forwarding via slirp4netns is not as performant as the new rootlessport approach, however the figures shared in the original commit that introduced rootlessport are as follows: slirp4netns: 8.3 Gbps, RootlessKit: 27.3 Gbps, which are more than sufficient for many use cases where the origin of traffic is more important than limits that cannot be reached due to bottlenecks elsewhere. Signed-off-by: Aleks Mariusz <[email protected]>

aleks-mariusz · 2020-06-22T09:31:20Z

Commands (adjusted to my setup) and re-ran:

$ git remote -v
origin  [email protected]:aleks-mariusz/libpod.git (fetch)
origin  [email protected]:aleks-mariusz/libpod.git (push)
upstream        https://github.com/containers/libpod.git (fetch)
upstream        https://github.com/containers/libpod.git (push)
$ git pull --rebase upstream master
remote: Enumerating objects: 2021, done.
remote: Counting objects: 100% (2021/2021), done.
remote: Compressing objects: 100% (31/31), done.
remote: Total 3699 (delta 2000), reused 1998 (delta 1988), pack-reused 1678
Receiving objects: 100% (3699/3699), 1.16 MiB | 0 bytes/s, done.
Resolving deltas: 100% (2784/2784), completed with 700 local objects.
From https://github.com/containers/libpod
 * branch            master     -> FETCH_HEAD
First, rewinding head to replay your work on top of it...
Applying: allow switching of port-forward approaches in rootless/using slirp4netns
$ git push -f
Counting objects: 15, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (7/7), done.
Writing objects: 100% (8/8), 3.05 KiB | 0 bytes/s, done.
Total 8 (delta 6), reused 0 (delta 0)
remote: Resolving deltas: 100% (6/6), completed with 6 local objects.
To [email protected]:aleks-mariusz/libpod.git
 + c6cdbcd...3c2cbcb issues/5138-off-master -> issues/5138-off-master (forced update)

Let me know if there's an issue with the above

giuseppe · 2020-06-22T11:53:21Z

thanks, tests are finally green.

LGTM

@mheon do you want to wait for the merge?

mheon · 2020-06-22T13:35:39Z

libpod/container_validate.go

@@ -66,7 +66,7 @@ func (c *Container) validate() error {

 	// Rootless has some requirements, compared to networks.
 	if rootless.IsRootless() {
-		if len(c.config.Networks) > 0 {
+		if !c.config.NetMode.IsSlirp4netns() && len(c.config.Networks) > 0 {


I think CNI networks are still unsupported with Slirp, so this should probably be removed unless I missed something

Unfortunately without this, a any param to --net that contains a : character results in an error about rootless not supporting choosing a network CNI

Hmmm. Let me look a bit, but I suspect this is mostly because of how we're parsing at the command line level.

…RT_DRIVER The default port driver "builtin" might not be always preferrable as it drops src IP information: containers/podman#6324 Now the port driver can be changed to "slirp4netns" via the environment variable `DOCKERD_ROOTLESS_ROOTLESSKIT_PORT_DRIVER`. It is still recommended to use the default "builtin" driver. Signed-off-by: Akihiro Suda <[email protected]>

…RT_DRIVER The default port driver "builtin" might not be always preferrable as it drops src IP information: containers/podman#6324 Now the port driver can be changed to "slirp4netns" via the environment variable `DOCKERD_ROOTLESS_ROOTLESSKIT_PORT_DRIVER`. It is still recommended to use the default "builtin" driver. Signed-off-by: Akihiro Suda <[email protected]> Upstream-commit: 6743320a125e6c351aef13a0772dd6e8cea90482 Component: engine

AkihiroSuda · 2020-07-07T09:35:12Z

What's current status?

giuseppe · 2020-07-14T08:01:00Z

@aleks-mariusz are you still interested in this feature?

giuseppe · 2020-07-14T10:43:06Z

let's follow up here: #6965

openshift-ci-robot requested review from umohnani8 and vrothberg May 21, 2020 16:53

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 21, 2020

aleks-mariusz mentioned this pull request May 21, 2020

allow switching of port-forward approaches when rootless using slirp4netns #6025

Closed

openshift-ci-robot assigned TomSweeneyRedHat May 21, 2020

openshift-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 22, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 22, 2020

AkihiroSuda reviewed May 28, 2020

View reviewed changes

pkg/namespaces/namespaces.go Outdated Show resolved Hide resolved

aleks-mariusz force-pushed the issues/5138-off-master branch from b9893ce to 2821297 Compare June 5, 2020 12:16

AkihiroSuda approved these changes Jun 5, 2020

View reviewed changes

aleks-mariusz force-pushed the issues/5138-off-master branch from 2821297 to 802433b Compare June 16, 2020 10:00

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 16, 2020

aleks-mariusz force-pushed the issues/5138-off-master branch from 802433b to c6cdbcd Compare June 16, 2020 10:37

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 16, 2020

giuseppe linked an issue Jun 22, 2020 that may be closed by this pull request

Source IP should be 127.0.0.1 in rootless Podman 1.4.4 - RHEL 7.7 #6691

Closed

aleks-mariusz force-pushed the issues/5138-off-master branch from c6cdbcd to 3c2cbcb Compare June 22, 2020 09:00

mheon reviewed Jun 22, 2020

View reviewed changes

AkihiroSuda mentioned this pull request Jun 24, 2020

dockerd-rootless.sh: allow specifying DOCKERD_ROOTLESS_ROOTLESSKIT_PORT_DRIVER moby/moby#41146

Merged

giuseppe mentioned this pull request Jul 14, 2020

allow switching of port-forward approaches in rootless/using slirp4netns #6965

Merged

giuseppe closed this Jul 14, 2020

5eraph mentioned this pull request Aug 4, 2020

support outbound-addr #7216

Merged

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow switching of port-forward approaches when rootless/using slirp4netns #6324

allow switching of port-forward approaches when rootless/using slirp4netns #6324

aleks-mariusz commented May 21, 2020

openshift-ci-robot commented May 21, 2020

aleks-mariusz commented May 21, 2020

jwhonce commented May 22, 2020

giuseppe commented May 22, 2020

TomSweeneyRedHat commented May 22, 2020

mheon commented May 22, 2020

rhatdan commented May 22, 2020

vrothberg commented Jun 4, 2020

rhatdan commented Jun 5, 2020

aleks-mariusz commented Jun 5, 2020

openshift-ci-robot commented Jun 5, 2020

giuseppe commented Jun 8, 2020

TomSweeneyRedHat commented Jun 8, 2020

rhatdan commented Jun 9, 2020

giuseppe commented Jun 16, 2020

aleks-mariusz commented Jun 16, 2020

aleks-mariusz commented Jun 16, 2020

giuseppe commented Jun 16, 2020

giuseppe commented Jun 22, 2020

giuseppe commented Jun 22, 2020

aleks-mariusz commented Jun 22, 2020

giuseppe commented Jun 22, 2020

mheon Jun 22, 2020

aleks-mariusz Jun 22, 2020

mheon Jun 22, 2020

AkihiroSuda commented Jul 7, 2020

giuseppe commented Jul 14, 2020

giuseppe commented Jul 14, 2020

allow switching of port-forward approaches when rootless/using slirp4netns #6324

allow switching of port-forward approaches when rootless/using slirp4netns #6324

Conversation

aleks-mariusz commented May 21, 2020

openshift-ci-robot commented May 21, 2020

aleks-mariusz commented May 21, 2020

jwhonce commented May 22, 2020

giuseppe commented May 22, 2020

TomSweeneyRedHat commented May 22, 2020

mheon commented May 22, 2020

rhatdan commented May 22, 2020

vrothberg commented Jun 4, 2020

rhatdan commented Jun 5, 2020

aleks-mariusz commented Jun 5, 2020

openshift-ci-robot commented Jun 5, 2020

giuseppe commented Jun 8, 2020

TomSweeneyRedHat commented Jun 8, 2020

rhatdan commented Jun 9, 2020

giuseppe commented Jun 16, 2020

aleks-mariusz commented Jun 16, 2020

aleks-mariusz commented Jun 16, 2020

giuseppe commented Jun 16, 2020

giuseppe commented Jun 22, 2020

giuseppe commented Jun 22, 2020

aleks-mariusz commented Jun 22, 2020

giuseppe commented Jun 22, 2020

mheon Jun 22, 2020

Choose a reason for hiding this comment

aleks-mariusz Jun 22, 2020

Choose a reason for hiding this comment

mheon Jun 22, 2020

Choose a reason for hiding this comment

AkihiroSuda commented Jul 7, 2020

giuseppe commented Jul 14, 2020

giuseppe commented Jul 14, 2020