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

aleks-mariusz · 2020-04-28T18:49:50Z

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 <m.k@alek.cx>

openshift-ci-robot · 2020-04-28T18:49:59Z

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.

openshift-ci-robot · 2020-04-28T18:50:06Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aleks-mariusz
To complete the pull request process, please assign tomsweeneyredhat
You can assign the PR to them by writing /assign @tomsweeneyredhat in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

aleks-mariusz · 2020-04-28T18:52:49Z

/assign @TomSweeneyRedHat

mheon · 2020-04-28T18:57:40Z

libpod/container_validate.go

@@ -66,12 +66,9 @@ 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 {


Why are we changing this? CNI networks will never be usable with rootless Podman's current network stack.

I changed this because otherwise i cannot use the ":" character as a parameter to --net (when using rootless mode), this validation thinks i'm trying to join CNI networks which this validation prohibits when rootless.

If there's a better way to enable this functionality, please let me know.

rhatdan · 2020-04-28T18:58:50Z

@aleks-mariusz Thanks for the PR, You need to sign your commit. BTW Did this already get merged upstream in the master branch.
@giuseppe PTAL

mheon · 2020-04-28T18:59:11Z

pkg/namespaces/namespaces.go

+			return true // default type
+		} else { // below here, also contains(":") == true
+			parts := strings.SplitN(string(n), ":", 2)
+			return parts[1] == rlkFwdType


Need to verify that len(parts) == 2 else this could segfault

just for my clarification, should i be checking if it's exactly length 2? or is the fact that this statement falls on the other side of the if statement else clause, where it definitely has to contain a ":" not sufficient?

aleks-mariusz · 2020-04-28T19:33:12Z

@aleks-mariusz Thanks for the PR, You need to sign your commit.

I updated the PR to add a sign-off at the end of the description, is that enough or do i need to amend the comment itself to include a sign-off as well?

BTW Did this already get merged upstream in the master branch.

I was asked to base this off v1.9 branch, so i create the PR against that as the source/target branch

AkihiroSuda · 2020-04-28T19:35:52Z

pkg/namespaces/namespaces.go

@@ -17,7 +17,9 @@ const (
 	nsType        = "ns"
 	podType       = "pod"
 	privateType   = "private"
+	rlkFwdType    = "rootlessport"


rootlessport -> rootlesskit?

this was the example parameter that @giuseppe used.. can change it if you want the parameter to look different

AkihiroSuda · 2020-04-28T19:36:55Z

FYI this package has same interface as rootlessport and can be used for simplifying code https://github.com/rootless-containers/rootlesskit/blob/master/pkg/port/slirp4netns/slirp4netns.go

aleks-mariusz · 2020-04-28T21:37:42Z

thanks, however i simply aimed to use the code that was removed in da7595a on advice in #5138 as it was the easiest way to restore source-ip functionality (and it works).

AkihiroSuda · 2020-05-04T18:53:51Z

Is this going to be --network-opt?
#6064 (comment)

mheon · 2020-05-04T21:15:38Z

I think it's a cleaner approach than trying to add further parsing to --network.

--network slirp4netns --network-opt port_forward=slirp4netns and --network slirp4netns --network-opt port_forward=rootlesskit seem pretty reasonable.

We'll need a new field (probably map[string]string) in Container configuration to hold it, and we can check it during network setup; if not set, we just use the default or rootlesskit.

giuseppe · 2020-05-05T10:10:47Z

I think it's a cleaner approach than trying to add further parsing to --network.

--network slirp4netns --network-opt port_forward=slirp4netns and --network slirp4netns --network-opt port_forward=rootlesskit seem pretty reasonable.

would it be possible in future to have multiple networks? With separate flags it will be more difficult, while having a single --network=TYPE:OPTIONS would make it simpler.

mheon · 2020-05-05T17:04:35Z

I don't know - I don't think we have any ability to do that right now (--network can only be passed once, and we'd have to rework how we store it in the DB if we wanted to change that)

5eraph · 2020-05-05T22:13:09Z

Well what about both:

keep --network as is
--network-opt will optionally accept network scope (global scope by default)

So sample parameter of slirp4nets outbound address would be
--network-opt=slirp4netns:OUTBOUND_ADDR=127.0.0.1
This should be backward compatible with current --network and we can transition without significant changes later.

giuseppe · 2020-05-06T09:17:19Z

I don't know - I don't think we have any ability to do that right now (--network can only be passed once, and we'd have to rework how we store it in the DB if we wanted to change that)

true but that will be an internal change, while now we are introducing a CLI change that will be difficult to change later.

Well what about both:
* keep `--network` as is

* `--network-opt` will optionally accept network scope (global scope by default)
So sample parameter of slirp4nets outbound address would be
--network-opt=slirp4netns:OUTBOUND_ADDR=127.0.0.1
This should be backward compatible with current --network and we can transition without significant changes later.

it is possible to add multiple slirp4netns networks to the same container

aleks-mariusz · 2020-05-07T12:37:15Z

Would it be of any value to anyone else beside me if we perhaps also had an environment variable be handled in order to make this change take effect?

I'm asking because the way i am launching podman, when/where i really need this source-ip preserved functionality is via podman-compose, which does not currently support any --network parameter being read from the docker-compose.yml nor ever passed (not to mention it defines a pod and adds the container, so i'm not sure how the above support i added fits into that), perhaps an environment variable would be better suited to that purpose?

AkihiroSuda · 2020-05-07T12:44:27Z

or libpod.conf?

aleks-mariusz · 2020-05-07T13:57:59Z

the config file approach is an interesting idea, i didn't even consider that.. but won't that mean that every container will in-effect be impacted/affected by the change if it's a global (or per user) level? is that desirable?

TBH, i'm not a huge fan of the environment variable idea (it's a bit too easy to lose hours off debugging something where it turns out you set (or didn't set) an environment variable and forgot), but it was the most immediate thing that came into my mind that worked around pod definitions not supporting --network

giuseppe · 2020-05-07T13:59:40Z

the config file approach is an interesting idea, i didn't even consider that.. but won't that mean that every container will in-effect be impacted/affected by the change if it's a global (or per user) level? is that desirable?

that can be used exactly in the case you want to change the default. For a single container, --network should be enough. I think we shouldn't be abusing env variables more than we already do :-)

rhatdan · 2020-05-07T18:55:39Z

/usr/share/containers/containers.conf and /etc/containers/containers.conf are global.
~/.config/containers/containers.conf is local.

aleks-mariusz · 2020-05-07T19:50:17Z

i can take a stab at adding the ability to choose between legacy slirp4netns and the new rootlesskit in containers.conf to this PR also, if folks are happy with what i have in here so far looks good..

also what should i name the parameter (presuming it'd go under the [network] section) in the containers.conf? i am thinking rootless_portforward_mechanism but i tend to be a bit overly verbose :-)

mheon · 2020-05-07T20:02:11Z

Maybe rootless_port_handler?

giuseppe · 2020-05-08T08:58:05Z

Maybe rootless_port_handler?

could it be port_handler or slirp_port_handler? In theory slirp can be used from root as well

giuseppe

I'd prefer if the modes are an enum, so we can also check for invalid inputs:

But that is not blocking, we can either fix in a follow up PR, or I can take a look at it.

What is more important now is that we pick the correct flags. I suggest we have:

--network=slirp4netns:slirp_port_handler=slirplisten
--network=slirp4netns:slirp_port_handler=rootlesskit

the config name must be the same as we agree for the config file.

You can just hardcode the slirp_port_handler= part inside of the rlkFwdType and slirpFwdType variables for now. What do you think? Does it make sense?

giuseppe · 2020-05-08T08:59:48Z

pkg/namespaces/namespaces.go

+func (n NetworkMode) IsPortForwardViaRootlessKit() bool {
+	if !n.IsSlirp4netns() {
+		return false
+	} else { // below here, implied IsSlirp4netns() == true


you can drop the else since there is an early return in the first condition and move everything one level back.

giuseppe · 2020-05-08T08:59:53Z

pkg/namespaces/namespaces.go

+func (n NetworkMode) IsPortForwardViaSlirpHostFwd() bool {
+	if !n.IsSlirp4netns() {
+		return false
+	} else { // below here, implied IsSlirp4netns() == true


AkihiroSuda · 2020-05-08T09:09:47Z

s/slirp_port_handler/port_handler/ because rootlesskit isn't really slirp. Also, future version of rootless Podman may support non-slirp network via some privileged helper like lxc-user-nic.

rhatdan · 2020-05-09T09:13:20Z

@aleks-mariusz ping

aleks-mariusz · 2020-05-09T10:18:08Z

@rhatdan Will try to look at giuseppe's review this weekend and maybe put the config file handling in as well

aleks-mariusz · 2020-05-10T11:01:39Z

@giuseppe i've added your review feedback to this PR now..

I'm going to hold off from trying to tackle the config file handling as i'm not quite sure how to this is handled (and may not be handled in this project but in containers/common )

Can you (or anyone) add the config file handling to this PR please (or guide me how to)

giuseppe · 2020-05-11T06:45:33Z

Can you (or anyone) add the config file handling to this PR please (or guide me how to)

I think we can tackle that separately as another PR.

The new code LGTM, could you just fix the golint issues reported here: https://github.com/containers/libpod/pull/6025/checks?check_run_id=660593109 ?

giuseppe · 2020-05-12T10:19:40Z

I think the next two patches change code introduced by the first one, can you please squash the patches in one? After that, I think it is ready to go

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

…netns 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 <m.k@alek.cx>

disaster123 · 2020-05-19T18:14:43Z

my fault - works fine thanks!

aleks-mariusz · 2020-05-20T00:12:32Z

@giuseppe is there anything else needed to merge this? i would really like the config file support to be written as well as even this is a bit cumbersome to have to use and being able to use a config file would be a preferable/more global change please

giuseppe · 2020-05-20T08:15:45Z

it looks fine, thanks.

Can you also open a PR for master? I'd like that the feature goes first in master, and then in 1.9

aleks-mariusz · 2020-05-21T16:54:09Z

it looks fine, thanks.

Can you also open a PR for master? I'd like that the feature goes first in master, and then in 1.9

@giuseppe PR #6324 created

github-actions · 2020-06-21T00:09:19Z

A friendly reminder that this PR had no activity for 30 days.

aleks-mariusz · 2020-07-17T11:49:03Z

implemented w/ Giuseppe's help via #6965

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 Apr 28, 2020

openshift-ci-robot requested review from jwhonce and mheon April 28, 2020 18:50

aleks-mariusz mentioned this pull request Apr 28, 2020

Source IP always 127.0.0.1 in rootless Podman 1.8.0 #5138

Closed

openshift-ci-robot assigned TomSweeneyRedHat Apr 28, 2020

mheon reviewed Apr 28, 2020

View reviewed changes

aleks-mariusz force-pushed the issues/5138 branch from 876314a to 8674925 Compare April 28, 2020 19:24

AkihiroSuda reviewed Apr 28, 2020

View reviewed changes

sujil02 force-pushed the v1.9 branch 2 times, most recently from 2f2d053 to be44305 Compare April 28, 2020 23:19

mheon force-pushed the v1.9 branch from be44305 to d985723 Compare April 28, 2020 23:23

aleks-mariusz force-pushed the issues/5138 branch from 3a15821 to 1b75c78 Compare April 29, 2020 22:17

5eraph mentioned this pull request May 5, 2020

Allowing rootless containers to be binded to a specific IP #6064

Closed

aleks-mariusz mentioned this pull request May 7, 2020

adding IPv6 section to 87-podman-bridge.conflist breaks host's ipv6 network access #6114

Closed

giuseppe reviewed May 8, 2020

View reviewed changes

aleks-mariusz force-pushed the issues/5138 branch from 1b75c78 to c5cd8ff Compare May 10, 2020 10:57

aleks-mariusz force-pushed the issues/5138 branch 2 times, most recently from d4c36fd to f3974be Compare May 11, 2020 18:10

rhatdan and others added 2 commits May 12, 2020 11:22

Don't check configuration until user input is applied

1b679b0

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

aleks-mariusz force-pushed the issues/5138 branch from f3974be to 41044b2 Compare May 12, 2020 10:23

aleks-mariusz mentioned this pull request May 21, 2020

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

Closed

github-actions bot added the stale-pr label Jun 21, 2020

aleks-mariusz closed this Jul 17, 2020

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

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

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

Conversation

aleks-mariusz commented Apr 28, 2020 • edited Loading

openshift-ci-robot commented Apr 28, 2020

openshift-ci-robot commented Apr 28, 2020

aleks-mariusz commented Apr 28, 2020

mheon Apr 28, 2020

Choose a reason for hiding this comment

aleks-mariusz Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

rhatdan commented Apr 28, 2020

mheon Apr 28, 2020

Choose a reason for hiding this comment

aleks-mariusz Apr 28, 2020

Choose a reason for hiding this comment

aleks-mariusz commented Apr 28, 2020

AkihiroSuda Apr 28, 2020

Choose a reason for hiding this comment

aleks-mariusz Apr 28, 2020

Choose a reason for hiding this comment

AkihiroSuda commented Apr 28, 2020

aleks-mariusz commented Apr 28, 2020 • edited Loading

AkihiroSuda commented May 4, 2020

mheon commented May 4, 2020

giuseppe commented May 5, 2020

mheon commented May 5, 2020

5eraph commented May 5, 2020

giuseppe commented May 6, 2020

aleks-mariusz commented May 7, 2020 • edited Loading

AkihiroSuda commented May 7, 2020

aleks-mariusz commented May 7, 2020 • edited Loading

giuseppe commented May 7, 2020

rhatdan commented May 7, 2020

aleks-mariusz commented May 7, 2020 • edited Loading

mheon commented May 7, 2020

giuseppe commented May 8, 2020

giuseppe left a comment • edited Loading

Choose a reason for hiding this comment

giuseppe May 8, 2020

Choose a reason for hiding this comment

giuseppe May 8, 2020

Choose a reason for hiding this comment

AkihiroSuda commented May 8, 2020

rhatdan commented May 9, 2020

aleks-mariusz commented May 9, 2020

aleks-mariusz commented May 10, 2020

giuseppe commented May 11, 2020

giuseppe commented May 12, 2020

disaster123 commented May 19, 2020 • edited Loading

aleks-mariusz commented May 20, 2020

giuseppe commented May 20, 2020

aleks-mariusz commented May 21, 2020

github-actions bot commented Jun 21, 2020

aleks-mariusz commented Jul 17, 2020

aleks-mariusz commented Apr 28, 2020 •

edited

Loading

aleks-mariusz Apr 28, 2020 •

edited

Loading

aleks-mariusz commented Apr 28, 2020 •

edited

Loading

aleks-mariusz commented May 7, 2020 •

edited

Loading

aleks-mariusz commented May 7, 2020 •

edited

Loading

aleks-mariusz commented May 7, 2020 •

edited

Loading

giuseppe left a comment •

edited

Loading

disaster123 commented May 19, 2020 •

edited

Loading