Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix slirp4netns port forwarding with ranges #13646

Merged
merged 1 commit into from
Mar 30, 2022

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Mar 24, 2022

The slirp4netns port forwarder was not updated to make use of the new
port format. This results in a problem when port ranges are used since
it does not read the range field from the port.

Update the logic to iterate through all ports with the range and
protocols. Also added a system test for port ranges with slirp4netns,
rootlesskit and the bridge network mode.

Fixes #13643

@mheon
Copy link
Member

mheon commented Mar 24, 2022

LGTM, would love to get this merged and backported for 4.0.3. @TomSweeneyRedHat This seems fairly significant, possibly worth an exception?

@mheon
Copy link
Member

mheon commented Mar 24, 2022

Actually, nevermind, it's exclusively slirp4netns, not rootlessport - less serious than I thought

@Luap99
Copy link
Member Author

Luap99 commented Mar 24, 2022

Yes this is only slirp4netns port forwarder, the default configuration with rootlessport will work.

@rhatdan
Copy link
Member

rhatdan commented Mar 24, 2022

/approve
/lgtm
/hold

@mheon
Copy link
Member

mheon commented Mar 24, 2022

Github appears to have frozen. @Luap99 Can you force-push?

@mheon
Copy link
Member

mheon commented Mar 24, 2022

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 24, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, mheon, 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 [Luap99,mheon,rhatdan]

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 24, 2022
@rhatdan
Copy link
Member

rhatdan commented Mar 25, 2022

@Luap99 could you force push to see if this triggers the CI?

@Luap99 Luap99 force-pushed the slirp4netns-portrange branch from 2fc46f7 to d7c3a68 Compare March 25, 2022 10:08
@baude
Copy link
Member

baude commented Mar 26, 2022

I reran these ubuntu tests several times. I'm thinking the failure is legit? Old slirp ?

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 28, 2022
@Luap99 Luap99 force-pushed the slirp4netns-portrange branch from d7c3a68 to 556c5bf Compare March 28, 2022 10:48
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 28, 2022
@Luap99 Luap99 force-pushed the slirp4netns-portrange branch from 556c5bf to c59246a Compare March 28, 2022 15:59
@Luap99
Copy link
Member Author

Luap99 commented Mar 29, 2022

The problem is in the test, ubuntus ncat doesn't close the connection when it gets EOF so it will just hang.
Man I really hate that every distro uses a different ncat implementation. Somehow they managed that they all behave differently and use different cli flags :(

@Luap99 Luap99 force-pushed the slirp4netns-portrange branch from c59246a to 9f95350 Compare March 29, 2022 15:12
The slirp4netns port forwarder was not updated to make use of the new
port format. This results in a problem when port ranges are used since
it does not read the range field from the port.

Update the logic to iterate through all ports with the range and
protocols. Also added a system test for port ranges with slirp4netns,
rootlesskit and the bridge network mode.

Fixes containers#13643

Signed-off-by: Paul Holzinger <[email protected]>
@Luap99 Luap99 force-pushed the slirp4netns-portrange branch from 9f95350 to eedaaf3 Compare March 29, 2022 17:19
@edsantiago
Copy link
Member

Doesn't work on my laptop (f35, root):

   #|     FAIL: ncat got data back (netmode=bridge port=5211)
   #| expected: 'TEZD7qnccS'
   #|   actual: 'Ncat: TIMEOUT.'

...but it works in CI, so LGTM I guess?

@Luap99
Copy link
Member Author

Luap99 commented Mar 29, 2022

@edsantiago Going for a long shot here but I think you have a cni/netavark conflict on you system. Assuming you run upgrade test before they will leave the cni-podman0 interface around which will conflict with the netavark interface.

@edsantiago
Copy link
Member

That was my first thought, so I ran podman system reset, then confirmed that podman info|grep neta shows netavarkl.

Now that I look further, most of the 500-network tests fail. So it's clearly something broken on my end.

@Luap99
Copy link
Member Author

Luap99 commented Mar 29, 2022

@edsantiago check ip addr | grep cni and remove the cni interface.

@edsantiago
Copy link
Member

Thank you! ip link del cni-podman0 and now all tests pass. I've added that to my troubleshooting reference.

@Luap99
Copy link
Member Author

Luap99 commented Mar 30, 2022

merge me

@mheon
Copy link
Member

mheon commented Mar 30, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 30, 2022
@openshift-merge-robot openshift-merge-robot merged commit fbbf5e8 into containers:main Mar 30, 2022
@Luap99 Luap99 deleted the slirp4netns-portrange branch March 30, 2022 17:58
@mheon
Copy link
Member

mheon commented Mar 30, 2022

FYI, did not backport cleanly, so not in 4.0.3. I don't think this is a big deal.

@Luap99
Copy link
Member Author

Luap99 commented Mar 30, 2022

I can backport manually if you want?
Or you need to grab #13660 before this commit

@mheon
Copy link
Member

mheon commented Mar 30, 2022

Backported

@github-actions 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 github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

port_handler=slirp4netns not publishing multiple ports anymore
6 participants