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

giuseppe · 2020-07-14T10:42:47Z

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.

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
Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

Follow up for #6324

aleks-mariusz · 2020-07-14T12:26:30Z

Hi @giuseppe to answer the question from the other thread (where i thought @AkihiroSuda was waiting for a @mheon as he last posted the review), i am interested in this feature still, however i'd be more interested if this was config file driven rather than command line parameter.. someone offered to look into doing this after the command line support was in, as it's beyond my famliarity with the code base (not to mention this is the first golang code i've ever touched).

mheon · 2020-07-14T13:21:50Z

libpod/container_validate.go

@@ -70,7 +70,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() {


I'm still against merging with this here - there's probably something on the frontend that's erroneously detecting slirp4netns:rootlesskit as a CNI network and adding it.

understood, and i'm open to ideas what/where else may need to be adjusted to not need this "enabling" snippet in container_validate.go..

i don't however quite understand what is wrong with the logic applied here, why would the length of networks given ever be a non-empty array, networks shouldn't be able to be specified when rootless.. the fixing of the erroneous detection of that parameter seems like a separate bug that needs to be fixed/handled in a different PR, no ?

@mheon sorry my mistake. I had fixed the detection but forgot to switch the condition here. Pushed a new version with if len(c.config.Networks) > 0.

I am currently experimenting running CNI plugins inside of the rootless environment. So I cannot promise this line will stay around for long :-)

Fair enough - I'd be extremely interested to see proper rootless CNI!

it will still be limited. We first need to create a network namespace managed by slirp, and from that network namespace create new ones using CNI. I am still unsure how to handle its UX nicely

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> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe · 2020-07-15T16:21:38Z

tests are green

mheon · 2020-07-15T17:44:05Z

I'm still a little iffy on the throwing options in the namespace itself. We have other requests to allow setting arbitrary options on networks (#6912) - what would you think about having a separate entry (map[string][]string - map of network name (slirp4netns, bridge, etc) to options) in the database for network options? We can keep the existing CLI, but split on the : and anything after that is split on , and turns into a network option?

giuseppe · 2020-07-15T19:15:49Z

@mheon is it something we can add to NetOptions?

mheon · 2020-07-15T19:43:58Z

@giuseppe That would work for me - I basically want a way to handle the next request after this one without duplicating all the plumbing.

giuseppe · 2020-07-16T10:33:38Z

@giuseppe That would work for me - I basically want a way to handle the next request after this one without duplicating all the plumbing.

thanks. That was indeed a great idea. It looks much cleaner and it was easy to add a new option on top of it.

docs/source/markdown/podman-create.1.md

docs/source/markdown/podman-run.1.md

AkihiroSuda · 2020-07-16T10:39:27Z

LGTM but a nit in the man page. Thanks!

docs/source/markdown/podman-run.1.md

openshift-ci-robot · 2020-07-16T11:45:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AkihiroSuda, giuseppe

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~~ [giuseppe]

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

rhatdan · 2020-07-16T13:50:40Z

@mheon PTAL

mheon · 2020-07-16T14:00:20Z

cmd/podman/common/netflags.go

@@ -164,11 +165,17 @@ func NetFlagsToNetOptions(cmd *cobra.Command) (*entities.NetOptions, error) {
 			return nil, err
 		}

+		parts := strings.SplitN(network, ":", 2)


Recommendation: If length on this is 2, we should probably throw away cniNets below (options should only be given if you're not specifying more than 1 seems reasonable)

mheon · 2020-07-16T14:01:38Z

One comment, and it would be nice to have a test; otherwise LGTM

giuseppe · 2020-07-16T14:43:43Z

One comment, and it would be nice to have a test; otherwise LGTM

took care of the comment and added tests

do not pass network specific options through the network namespace. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Closes: containers#6912 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

rhatdan · 2020-07-16T21:13:30Z

/lgtm

mheon · 2020-07-16T22:23:27Z

/hold
One flake

giuseppe · 2020-07-17T08:40:52Z

tests are green now

/hold cancel

aleks-mariusz · 2020-07-17T12:01:38Z

@giuseppe thanks so much for driving this forward and finally getting it all preped, perfected, and merged!

quick follow up question, did this implement config-file handling as well? if so what params would need to go into which conf file to fix the original source-ip=127.0.0.1 issue that spawned this?

giuseppe · 2020-07-17T14:36:10Z

quick follow up question, did this implement config-file handling as well? if so what params would need to go into which conf file to fix the original source-ip=127.0.0.1 issue that spawned this?

no, that part is not implemented yet, it is possible to specify the new option only through the CLI

mheon · 2020-07-17T14:40:23Z

Yes - we'll need a separate field for this in containers.conf

andrewgdunn · 2020-07-26T14:55:34Z

Did this make it into 2.0.3?

mheon · 2020-07-26T16:43:03Z

Negative - 2.0.3 is bugfixes only, this is a new feature.

I think we'll probably start cutting 2.1.0 release candidates soon, though - this week or next.

DavidePrincipi · 2020-12-15T09:02:22Z

Thanks for this PR!

The jvb component of Jitsi is an UDP server that needs IP addresses are preserved.

On podman 2.2.1 I can run

$ podman pod create --network=slirp4netns:port_handler=slirp4netns --publish 2086:80 --publish 10000:10000/udp --name=jitsi ...

https://gist.github.com/DavidePrincipi/6a45a7cfaf367115931eff3f50d1a681

aleks-mariusz · 2020-12-15T12:50:50Z

Negative - 2.0.3 is bugfixes only, this is a new feature.

I think we'll probably start cutting 2.1.0 release candidates soon, though - this week or next.

@mheon does this imply the conf support is available as well? if so can you please point to the param needed and which file it needs to go into? this is a follow up to my previous ask (in July) to @giuseppe

mheon · 2020-12-15T16:36:56Z

The netns field in containers.conf should be what you want - setting that to slirp4netns:port_handler=slirp4netns should enable this by default

barosl · 2021-01-01T12:41:40Z

The netns field in containers.conf should be what you want - setting that to slirp4netns:port_handler=slirp4netns should enable this by default

This doesn't seem to work. I tried the following:

[containers]
netns = 'slirp4netns:port_handler=slirp4netns'

rktjmp · 2022-01-17T02:04:35Z

Edit: Sorry for the necro, got my years mixed up thinking the last post was only a few days ago, not ... a year ago...

The netns field in containers.conf should be what you want - setting that to slirp4netns:port_handler=slirp4netns should enable this by default

This doesn't seem to work. I tried the following:
[containers]
netns = 'slirp4netns:port_handler=slirp4netns'

I believe you want to use

[engine]
network_cmd_options=["port_handler=slirp4netns"]

Which sets Default options to pass to the slirp4netns binary.

The manpage also shows network_cmd_options["enable_ipv6=true",], for the option, which implies to me it's the default value, but the actual description for enable_ipv6 says the default is false, so... not sure. I guess just be aware that the option exists if something breaks.

Also be aware that unfortunately you cant mix the port_handler with user-defined networks, so the option might not apply in your case:

• port_handler=slirp4netns:
Use  the  slirp4netns  port forwarding, it is slower than
rootlesskit but preserves the correct source IP address.
This port handler cannot be used for user-defined networks.

openshift-ci-robot requested review from baude and mheon July 14, 2020 10:42

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

giuseppe mentioned this pull request Jul 14, 2020

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

Closed

mheon reviewed Jul 14, 2020

View reviewed changes

giuseppe force-pushed the followup-pr6324 branch from 5dcb6ba to f683586 Compare July 14, 2020 18:56

giuseppe force-pushed the followup-pr6324 branch from f683586 to 8d12f19 Compare July 15, 2020 06:04

giuseppe mentioned this pull request Jul 15, 2020

rootless: support launching slirp4netns without --disable-host-loopback #6912

Closed

giuseppe force-pushed the followup-pr6324 branch from edfc28e to 03d4f62 Compare July 16, 2020 10:33

AkihiroSuda reviewed Jul 16, 2020

View reviewed changes

docs/source/markdown/podman-create.1.md Outdated Show resolved Hide resolved

AkihiroSuda reviewed Jul 16, 2020

View reviewed changes

docs/source/markdown/podman-run.1.md Outdated Show resolved Hide resolved

giuseppe force-pushed the followup-pr6324 branch from 03d4f62 to 155ffbd Compare July 16, 2020 10:45

AkihiroSuda reviewed Jul 16, 2020

View reviewed changes

docs/source/markdown/podman-run.1.md Outdated Show resolved Hide resolved

AkihiroSuda approved these changes Jul 16, 2020

View reviewed changes

giuseppe force-pushed the followup-pr6324 branch from 155ffbd to d6aeee8 Compare July 16, 2020 11:42

AkihiroSuda approved these changes Jul 16, 2020

View reviewed changes

mheon reviewed Jul 16, 2020

View reviewed changes

giuseppe force-pushed the followup-pr6324 branch from d6aeee8 to 98dd447 Compare July 16, 2020 14:43

giuseppe added 3 commits July 16, 2020 22:37

libpod: pass down network options

9be7029

do not pass network specific options through the network namespace. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

network, slirp4netns: add option to allow host loopback

7722b58

Closes: containers#6912 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

docs: document the new slirp4netns options

d35d812

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the followup-pr6324 branch from 98dd447 to d35d812 Compare July 16, 2020 20:39

openshift-ci-robot assigned rhatdan Jul 16, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 16, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 17, 2020

openshift-merge-robot merged commit d86bae2 into containers:master Jul 17, 2020

This was referenced Jul 17, 2020

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

Closed

Source IP always 127.0.0.1 in rootless Podman 1.8.0 #5138

Closed

andrewgdunn mentioned this pull request Jul 25, 2020

port-forwarded service always appears to originate from 127.0.0.1 rootless-containers/rootlesskit#155

Closed

5eraph mentioned this pull request Aug 4, 2020

support outbound-addr #7216

Merged

usury mentioned this pull request Oct 29, 2020

Alternate port_handler that keeps the source ip for user-defined rootless networks #8193

Open

usury mentioned this pull request Mar 13, 2023

Rootless Nginx Wrong "remote_addr" while Rootful Nginx has correct "remote_addr" #17765

Closed

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 21, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

giuseppe commented Jul 14, 2020

aleks-mariusz commented Jul 14, 2020

mheon Jul 14, 2020

aleks-mariusz Jul 14, 2020 •

edited

Loading

giuseppe Jul 14, 2020

mheon Jul 14, 2020

giuseppe Jul 15, 2020

giuseppe commented Jul 15, 2020

mheon commented Jul 15, 2020

giuseppe commented Jul 15, 2020

mheon commented Jul 15, 2020

giuseppe commented Jul 16, 2020

AkihiroSuda commented Jul 16, 2020

openshift-ci-robot commented Jul 16, 2020

rhatdan commented Jul 16, 2020

mheon Jul 16, 2020

giuseppe Jul 16, 2020

mheon commented Jul 16, 2020

giuseppe commented Jul 16, 2020

rhatdan commented Jul 16, 2020

mheon commented Jul 16, 2020

giuseppe commented Jul 17, 2020

aleks-mariusz commented Jul 17, 2020

giuseppe commented Jul 17, 2020

mheon commented Jul 17, 2020

andrewgdunn commented Jul 26, 2020

mheon commented Jul 26, 2020

DavidePrincipi commented Dec 15, 2020

aleks-mariusz commented Dec 15, 2020 •

edited

Loading

mheon commented Dec 15, 2020

barosl commented Jan 1, 2021

rktjmp commented Jan 17, 2022 •

edited

Loading

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

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

Conversation

giuseppe commented Jul 14, 2020

aleks-mariusz commented Jul 14, 2020

mheon Jul 14, 2020

Choose a reason for hiding this comment

aleks-mariusz Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

giuseppe Jul 14, 2020

Choose a reason for hiding this comment

mheon Jul 14, 2020

Choose a reason for hiding this comment

giuseppe Jul 15, 2020

Choose a reason for hiding this comment

giuseppe commented Jul 15, 2020

mheon commented Jul 15, 2020

giuseppe commented Jul 15, 2020

mheon commented Jul 15, 2020

giuseppe commented Jul 16, 2020

AkihiroSuda commented Jul 16, 2020

openshift-ci-robot commented Jul 16, 2020

rhatdan commented Jul 16, 2020

mheon Jul 16, 2020

Choose a reason for hiding this comment

giuseppe Jul 16, 2020

Choose a reason for hiding this comment

mheon commented Jul 16, 2020

giuseppe commented Jul 16, 2020

rhatdan commented Jul 16, 2020

mheon commented Jul 16, 2020

giuseppe commented Jul 17, 2020

aleks-mariusz commented Jul 17, 2020

giuseppe commented Jul 17, 2020

mheon commented Jul 17, 2020

andrewgdunn commented Jul 26, 2020

mheon commented Jul 26, 2020

DavidePrincipi commented Dec 15, 2020

aleks-mariusz commented Dec 15, 2020 • edited Loading

mheon commented Dec 15, 2020

barosl commented Jan 1, 2021

rktjmp commented Jan 17, 2022 • edited Loading

aleks-mariusz Jul 14, 2020 •

edited

Loading

aleks-mariusz commented Dec 15, 2020 •

edited

Loading

rktjmp commented Jan 17, 2022 •

edited

Loading