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

libnetwork: add rootlessnetns package #1761

Merged
merged 14 commits into from
Dec 6, 2023

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Dec 5, 2023

Add a new rootlessnetns package based on the rootless netns code from
podman. It however makes some significant changes:

  • First it uses a directory in the runroot and not tmpdir.
  • The netns mount is stored in the directoy and not the global netns
    runtime dir to prevent name collisions. The old code used the sha256
    to do that.
  • The teardown and setup logic has been made more robust and now used a
    reference counter to keep track on when to cleanup. The podman
    cleanup logic was racy and tied to running podman containers. Given
    the plan to allow buildah to use this as well we need this.
  • There is no lock for this code, the goal is to have this called
    through the network interface which is already locked so there is no
    need for another lock here.

Future work:

  • add pasta support
  • add port forwarding logic here

see commits for more details

This link was added in kernel 3.17 so it should be safe to use now as we
do not support running on such old kernels anyway.

This makes the code a bit simpler and safes two syscalls.

Signed-off-by: Paul Holzinger <[email protected]>
We never use the origNS other than closing it again so we can just
remove this code as it does nothing useful.

Signed-off-by: Paul Holzinger <[email protected]>
Logging the error and returning it makes no sense, instead add the
context to the error before returning it.

Signed-off-by: Paul Holzinger <[email protected]>
Add new function to create a netns bind mounted to a specific path. This
is useful for the new rootless netns logic were I intend to mount the
netns at a different directory to avoid naming conflicts.

Signed-off-by: Paul Holzinger <[email protected]>
Add a new rootlessnetns package based on the rootless netns code from
podman. It however makes some significant changes:
 - First it uses a directory in the runroot and not tmpdir.
 - The netns mount is stored in the directoy and not the global netns
   runtime dir to prevent name collisions. The old code used the sha256
   to do that.
 - The teardown and setup logic has been made more robust and now used a
   reference counter to keep track on when to cleanup. The podman
   cleanup logic was racy and tied to running podman containers. Given
   the plan to allow buildah to use this as well we need this.
 - There is no lock for this code, the goal is to have this called
   through the network interface which is already locked so there is no
   need for another lock here.

Future work:
 - add pasta support
 - add port forwarding logic here

Signed-off-by: Paul Holzinger <[email protected]>
Integrate the new rootlessnetns package into netavark. This means when
this is vendored into podman we need to drop the rootless netns code
there.

Signed-off-by: Paul Holzinger <[email protected]>
With the new NewNSAtPath function it is possible to create netns files
outside the normal netns directory. We need to make sure they can get
unmounted as well. We can check the the path is not under /proc.

Signed-off-by: Paul Holzinger <[email protected]>
Call directly into the rootlessnetns code.

Signed-off-by: Paul Holzinger <[email protected]>
Copy link
Contributor

openshift-ci bot commented Dec 5, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

In podman we have code to move a process into a new systemd cgroup. This
code lived in the podman utils package. Because the new rootlessnetns
must call into that move this code to c/common.

Instead of dumping this again into a "util" package create a systemd
package which should have a better name. Also move the cgroup code
directly into pkg/cgroup. I am sure we can do some cleanup there in a
followup to prevent duplication.

Signed-off-by: Paul Holzinger <[email protected]>
The old rootlessnetns logic overwrote PATH for the current process to
make sure /usr/sbin (where iptables is normally installed) is in $PATH.

Now instead of adding it for the current process we can just always set
it for the cni/iptables exec only.

Signed-off-by: Paul Holzinger <[email protected]>
Make sure we correctly cleanup the netns if there was an error and the
netns was just created. Also make sure the parent dir for the netns is
always created because a previous cleanup() may have it deleted.

Signed-off-by: Paul Holzinger <[email protected]>
Just pass down the full containers.conf as this is needed by
rootlessnetns code, also remove the now duplicated fields and read the
options directly from the config struct.

Signed-off-by: Paul Holzinger <[email protected]>
The cni code is used by freebsd so this package must build for it as
well. Given the logic is linux specific and not called by freebsd just
return an error.

Signed-off-by: Paul Holzinger <[email protected]>
Just add stubs so podman can still compile.

Signed-off-by: Paul Holzinger <[email protected]>
@Luap99
Copy link
Member Author

Luap99 commented Dec 5, 2023

Podman PR for testing: containers/podman#20772

@Luap99 Luap99 marked this pull request as ready for review December 6, 2023 12:21
@Luap99
Copy link
Member Author

Luap99 commented Dec 6, 2023

This is ready for review, podman PR passes tests: containers/podman#20772
@mheon PTAL

@baude
Copy link
Member

baude commented Dec 6, 2023

thanks for your work @Luap99

LGTM

@rhatdan
Copy link
Member

rhatdan commented Dec 6, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Dec 6, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit b647eb3 into containers:main Dec 6, 2023
7 checks passed
@Luap99 Luap99 deleted the rootlessnetns branch December 6, 2023 14:05
Luap99 added a commit to Luap99/libpod that referenced this pull request Dec 6, 2023
Use the new rootlessnetns logic from c/common, drop the podman code
here and make use of the new much simpler API.

ref: containers/common#1761

[NO NEW TESTS NEEDED]

Signed-off-by: Paul Holzinger <[email protected]>
Luap99 added a commit to Luap99/libpod that referenced this pull request Dec 7, 2023
Use the new rootlessnetns logic from c/common, drop the podman code
here and make use of the new much simpler API.

ref: containers/common#1761

[NO NEW TESTS NEEDED]

Signed-off-by: Paul Holzinger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants