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

add support for pasta(1) and slirp4netns options #4877

Merged
merged 3 commits into from Jun 26, 2023

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Jun 23, 2023

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test

/kind feature

/kind flake
/kind other

What this PR does / why we need it:

This uses the same code as podman for slirp4netns, this means

  • ipv6 is enabled by default
  • slirp4netns options are read from contianers.conf
  • slirp4netns options can now be set on the cli. This required some
    small rework on where we parse the network string.

Lastly I updated the --network docs, to document the new slirp4netns
mode. That included fixing up buildah-from and buildah-run pages which
were incomplete in that regard. Now we show the same for all options.

Like podman allow buildah and therefore podman build to use the network
mode pasta. The pasta integration is very simple and we do not even
need a teardown handler for that as pasta will exit on its own when the
netns path is removed.

However right now this is broken, pasta will fail to open
/proc/$pid/ns/net. I send a patch[1] to fix this upstream in pasta.
I assume this will land quickly so I like to get this in now just so we
have this included in podman v4.6. Thus the test is skipped for now.

[1] https://archives.passt.top/passt-dev/20230623082531.25947-2-pholzing@redhat.com/

How to verify it

Which issue(s) this PR fixes:

Fixes #3968

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Buidlah now supports pasta as network mode like podman.
Slirp4netns now uses the options from containers.conf and uses ipv6 by default. 

includes new packages for slirp4netns and pasta

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
@openshift-ci openshift-ci bot added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 23, 2023
This uses the same code as podman for slirp4netns, this means
- ipv6 is enabled by default
- slirp4netns options are read from contianers.conf
- slirp4netns options can now be set on the cli. This required some
small rework on where we parse the network string.

Lastly I updated the --network docs, to document the new slirp4netns
mode. That included fixing up buildah-from and buildah-run pages which
were incomplete in that regard. Now we show the same for all options.

Fixes containers#3968

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Like podman allow buildah and therefore podman build to use the network
mode pasta. The pasta integration is very simple and we do not even
need a teardown handler for that as pasta will exit on its own when the
netns path is removed.

However right now this is broken, pasta will fail to open
/proc/$pid/ns/net. I send a patch[1] to fix this upstream in pasta.
I assume this will land quickly so I like to get this in now just so we
have this included in podman v4.6. Thus the test is skipped for now.

[1] https://archives.passt.top/passt-dev/20230623082531.25947-2-pholzing@redhat.com/

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
bytes, disable NDP, DHCPv6 and DHCP support.
- **pasta:-I,tap0,--ipv4-only,-a,10.0.2.0,-n,24,-g,10.0.2.2,--dns-forward,10.0.2.3,--no-ndp,--no-dhcpv6,--no-dhcp**,
equivalent to default slirp4netns(1) options with Podman overrides: same as
above, but leave the MTU to 65520 bytes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's a lack of tea, but I'm not at all understanding the words after the colon here. I think "same as above" refers to the line pasta:--ipv4-only,-a,10.0.2.0,-n,24,-g,10.0.2.2,--dns-forward,10.0.2.3,-m,1500,--no-ndp,--no-dhcpv6,--no-dhcp, , but I'm not sure. I'd suggest saying something like, " this is the same as the prior --ipv4-onlyexample, but leave the MTU at 65520 bytes" I'm just mostly concerned if someone many months from now slaps another example between the lines and the "same as above" is no longer pertinent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is copied from podman. And both really just show how to make pasta behave more like slirp4netns. I mean sure I can change it but both examples should really stay together IMO.

@TomSweeneyRedHat
Copy link
Member

One small doc comment, otherwise LGTM.
Did you spin up a WIP PR in Podman with this vendored in? Podman might need some man page adjustments.

@Luap99
Copy link
Member Author

Luap99 commented Jun 23, 2023

@nalind @flouthoc @rhatdan PTAL

@rhatdan
Copy link
Member

rhatdan commented Jun 23, 2023

LGTM
@flouthoc @nalind PTAL

@nalind
Copy link
Member

nalind commented Jun 26, 2023

--net=container seems to be undocumented now, otherwise LGTM.

@Luap99
Copy link
Member Author

Luap99 commented Jun 26, 2023

--net=container seems to be undocumented now, otherwise LGTM.

That was only mentioned in the buildah-from man page. I rather have all three pages have the same text as they do the same thing. I don't know why this one still says container but it is a bug to me because (d05957a) removed it. It is only in the code for backwards compat so I don't think it should be documented.

Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM other than comments above.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 26, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, Luap99

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:

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

@TomSweeneyRedHat
Copy link
Member

I'm going to push this one through to get the functionality. If there's more nits to pick on the docs, let's do that in separate PRs.

@TomSweeneyRedHat
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jun 26, 2023
@openshift-merge-robot openshift-merge-robot merged commit 5422557 into containers:main Jun 26, 2023
32 checks passed
@Luap99 Luap99 deleted the pasta-slirp4netns branch June 27, 2023 08:13
Luap99 added a commit to Luap99/libpod that referenced this pull request Jul 13, 2023
Support was added in buildah some weeks ago. [1]

[1] containers/buildah#4877

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
ashley-cui pushed a commit to ashley-cui/podman that referenced this pull request Jul 20, 2023
Support was added in buildah some weeks ago. [1]

[1] containers/buildah#4877

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
ashley-cui pushed a commit to ashley-cui/podman that referenced this pull request Jul 20, 2023
Support was added in buildah some weeks ago. [1]

[1] containers/buildah#4877

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved kind/feature Categorizes issue or PR as related to a new feature. lgtm locked - please file new issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No IPv6 access in podman build, but works in podman run
6 participants