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

Support specifying netns (and userns?) file path as well as PID #84

Closed
AkihiroSuda opened this issue Apr 11, 2019 · 17 comments · Fixed by #86
Closed

Support specifying netns (and userns?) file path as well as PID #84

AkihiroSuda opened this issue Apr 11, 2019 · 17 comments · Fixed by #86
Labels
help wanted Extra attention is needed

Comments

@AkihiroSuda
Copy link
Member

proposed in containers/podman#2897

@sboeuf
Copy link

sboeuf commented Apr 11, 2019

@AkihiroSuda thanks for opening this issue!
I'd like to clarify the way to implement this. Currently the way slirp4netns works, it expects the process id of the container to be provided to know which network namespace needs to be joined.
Here we are talking about adding some flags/options in order to join specific network namespace based on a path and not a process ID.

The question I have is about the way to do this. Do you want to keep backward compatibility about providing a process ID that would be superseded by --netns-path if it is there? Or instead should we simply remove the expectation about the process ID, providing --netns-path and --pid?

@sboeuf
Copy link

sboeuf commented Apr 11, 2019

/cc @amshinde @gabibeyer

@AkihiroSuda
Copy link
Member Author

We need to keep compatibility.

The PID portion should be an empty string when --netns-path is present?

@AkihiroSuda
Copy link
Member Author

cc @giuseppe

@amshinde
Copy link

@AkihiroSuda We would need to pass a user namespace path and a mount namespace path as well, if I am not mistaken.

@amshinde
Copy link

Just adding details of what was discussed with @giuseppe, from the podman point of view, when running in rootless mode, podman creates a user and mount namespace and calls slirp4netns from the same user+mount namespace. So what podman would need to provide to slirp4netns is the network namespace path.

@gabibeyer
Copy link
Contributor

@AkihiroSuda @giuseppe Would you rather an empty string for PID when --netns-path is passed, or don't allow the PID arg altogether, and only the single TAPNAME argument to be passed?

slirp4netns --netns-path=<PATH> "" <TAPNAME>
slirp4netns --netns-path=<PATH> <TAPNAME>

I'm working on a PR to solve this issue, thank you for the help !

@AkihiroSuda
Copy link
Member Author

I prefer the former one.
cc @giuseppe WDYT?

I think we should also have --userns-path for consistency with PID mode even though it is not needed for Podman.

@giuseppe
Copy link
Collaborator

could we reuse the same argument? We can consider it a path if it starts with /

slirp4netns /path/to/the/ns
slirp4netns 1234

what do you think?

@AkihiroSuda
Copy link
Member Author

We can consider it a path if it starts with /

No support for relative path? How to support specifying userns path?

@giuseppe
Copy link
Collaborator

No support for relative path? How to support specifying userns path?

I'd expect the path to be mostly specified as /proc/PID/ns/net or /proc/self/ns/net. If we want support for relative path then we could probably use something like:

slirp4netns --netns-path <PATH|PID>

if --netns-path is specified then the first argument is a path, otherwise a pid, or alternatively:

slirp4netns --netns-type=path <PATH|PID>

so we don't have to deal with an empty argument

@rhatdan
Copy link

rhatdan commented Apr 12, 2019

slirp4netns --netns-type=path <PATH|PID>
slirp4netns --netns-type=pid <PATH|PID>
With pid as default works for me.
I would avoid the ambiguity.

@AkihiroSuda
Copy link
Member Author

--netns-type SGTM, but wondering how we can specify userns path, though it is not needed for Podman. maybe just --userns-path=/foo/bar

@sboeuf
Copy link

sboeuf commented Apr 12, 2019

I have one proposal if you really want to make this generic. We could introduce --netns, --userns, and --mntns flags, but also keep the PID through the command line.
By default, slirp4netns would work as it is working right now, which means it would pick all namespaces from the PID, but if one of the additional option is used, it would override the namespace specified by the flag. WDYT?

@amshinde
Copy link

I like @sboeuf's proposal to keep the existing pid option and introduce two new options --netns and --userns. These would be used to override, if pid is provided as well, if neither the pid nor the namespace paths are provided, then slirp4netns would error out.

@gabibeyer
Copy link
Contributor

@sboeuf I feel like the only problem with that is what the PID argument would be if --netns-path is passed.

How about a hybrid approach, a 'no-argument' option --use-netns-path that will allow the pid arg to be a path, otherwise it will expect a PID value. The current logic of getting the mount namespace and user namespace from the PID will stay in place. Then an addition of --userns-path=PATH and --mntns-path=PATH can be used to override what the PID finds, as previously suggested.

so something like

slirp4netns --use-netns-path --mntns-path=<PATH> --userns-path=<PATH><PATH> tap0
slirp4netns --mntns-path=<PATH><PID> tap0

if a user doesn't specify a userns-path, or a mntns-path with the --use-netns-path, and they are required within the code, it can error out then.

@AkihiroSuda
Copy link
Member Author

I dont have a strong opinion - UX is out of scope of this repo, but IIUC we dont need the mntns stuff

gabibeyer pushed a commit to gabibeyer/slirp4netns that referenced this issue Apr 16, 2019
Allow a network namespace path to be passed instead of a PID by
using the --use-netns-path argument, and passing a path as the
first argument. An optional --userns-path=PATH argument can be
passed that will be supersede the default user namespace path.

Fixes rootless-containers#84

Signed-off-by: Gabi Beyer <[email protected]>
gabibeyer pushed a commit to gabibeyer/slirp4netns that referenced this issue Apr 19, 2019
Allow a network namespace path to be passed instead of a PID by
using the --use-netns-path argument, and passing a path as the
first argument. An optional --userns-path=PATH argument can be
passed that will be supersede the default user namespace path.

Fixes rootless-containers#84

Signed-off-by: Gabi Beyer <[email protected]>
gabibeyer pushed a commit to gabibeyer/slirp4netns that referenced this issue Apr 21, 2019
Allow a network namespace path to be passed instead of a PID by
using the --use-netns-path argument, and passing a path as the
first argument. An optional --userns-path=PATH argument can be
passed that will be supersede the default user namespace path.

Fixes rootless-containers#84

Signed-off-by: Gabi Beyer <[email protected]>
gabibeyer pushed a commit to gabibeyer/slirp4netns that referenced this issue Apr 21, 2019
Allow a network namespace path to be passed instead of a PID by
using the --use-netns-path argument, and passing a path as the
first argument. An optional --userns-path=PATH argument can be
passed that will be supersede the default user namespace path.

Fixes rootless-containers#84

Signed-off-by: Gabi Beyer <[email protected]>
gabibeyer pushed a commit to gabibeyer/slirp4netns that referenced this issue Apr 22, 2019
Allow a network namespace path to be passed instead of a PID by
using the --use-netns-path argument, and passing a path as the
first argument. An optional --userns-path=PATH argument can be
passed that will be supersede the default user namespace path.

Fixes rootless-containers#84

Signed-off-by: Gabi Beyer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants