Pasta networking ignores --network-cmd-path

[PhrozenByte]

Issue Description

Podman doesn't honour the --network-cmd-path CLI option to find the pasta binary when attempting to use Pasta networking.

Steps to reproduce the issue

Without pasta installed globally (i.e. there's no /usr/bin/pasta), but with a locally build pasta binary in the current working dir (i.e. ./pasta exists and works fine):

$ podman run -it --rm --network=pasta --network-cmd-path="$PWD" alpine sh
Error: could not find pasta, the network namespace can't be configured: exec: "pasta": executable file not found in $PATH

Describe the results you received

Podman fails to find the pasta binary in $PWD, because it ignores --network-cmd-path and still tries to find it in $PATH.

Describe the results you expected

Podman should use $PWD/pasta.

podman info output

host:
  arch: amd64
  buildahVersion: 1.30.0
  cgroupControllers:
  - cpu
  - memory
  - pids
  cgroupManager: systemd
  cgroupVersion: v2
  conmon:
    package: conmon-2.1.7-2.fc38.x86_64
    path: /usr/bin/conmon
    version: 'conmon version 2.1.7, commit: '
  cpuUtilization:
    idlePercent: 69.85
    systemPercent: 6.65
    userPercent: 23.5
  cpus: 1
  databaseBackend: boltdb
  distribution:
    distribution: fedora
    version: "38"
  eventLogger: journald
  hostname: ubuntu-2gb-fsn1-1
  idMappings:
    gidmap:
    - container_id: 0
      host_id: 1000
      size: 1
    - container_id: 1
      host_id: 524288
      size: 65536
    uidmap:
    - container_id: 0
      host_id: 1000
      size: 1
    - container_id: 1
      host_id: 524288
      size: 65536
  kernel: 6.2.11-300.fc38.x86_64
  linkmode: dynamic
  logDriver: journald
  memFree: 429338624
  memTotal: 1998495744
  networkBackend: netavark
  ociRuntime:
    name: crun
    package: crun-1.8.4-1.fc38.x86_64
    path: /usr/bin/crun
    version: |-
      crun version 1.8.4
      commit: 5a8fa99a5e41facba2eda4af12fa26313918805b
      rundir: /run/user/1000/crun
      spec: 1.0.0
      +SYSTEMD +SELINUX +APPARMOR +CAP +SECCOMP +EBPF +CRIU +LIBKRUN +WASM:wasmedge +YAJL
  os: linux
  remoteSocket:
    path: /run/user/1000/podman/podman.sock
  security:
    apparmorEnabled: false
    capabilities: CAP_CHOWN,CAP_DAC_OVERRIDE,CAP_FOWNER,CAP_FSETID,CAP_KILL,CAP_NET_BIND_SERVICE,CAP_SETFCAP,CAP_SETGID,CAP_SETPCAP,CAP_SETUID,CAP_SYS_CHROOT
    rootless: true
    seccompEnabled: true
    seccompProfilePath: /usr/share/containers/seccomp.json
    selinuxEnabled: true
  serviceIsRemote: false
  slirp4netns:
    executable: /usr/bin/slirp4netns
    package: slirp4netns-1.2.0-12.fc38.x86_64
    version: |-
      slirp4netns version 1.2.0
      commit: 656041d45cfca7a4176f6b7eed9e4fe6c11e8383
      libslirp: 4.7.0
      SLIRP_CONFIG_VERSION_MAX: 4
      libseccomp: 2.5.3
  swapFree: 0
  swapTotal: 0
  uptime: 0h 28m 27.00s
plugins:
  authorization: null
  log:
  - k8s-file
  - none
  - passthrough
  - journald
  network:
  - bridge
  - macvlan
  - ipvlan
  volume:
  - local
registries:
  search:
  - registry.fedoraproject.org
  - registry.access.redhat.com
  - docker.io
  - quay.io
store:
  configFile: /home/user/.config/containers/storage.conf
  containerStore:
    number: 0
    paused: 0
    running: 0
    stopped: 0
  graphDriverName: overlay
  graphOptions: {}
  graphRoot: /home/user/.local/share/containers/storage
  graphRootAllocated: 20004794368
  graphRootUsed: 2821701632
  graphStatus:
    Backing Filesystem: extfs
    Native Overlay Diff: "true"
    Supports d_type: "true"
    Using metacopy: "false"
  imageCopyTmpDir: /var/tmp
  imageStore:
    number: 1
  runRoot: /run/user/1000/containers
  transientStore: false
  volumePath: /home/user/.local/share/containers/storage/volumes
version:
  APIVersion: 4.5.0
  Built: 1681486942
  BuiltTime: Fri Apr 14 15:42:22 2023
  GitCommit: ""
  GoVersion: go1.20.2
  Os: linux
  OsArch: linux/amd64
  Version: 4.5.0

Podman in a container

No

Privileged Or Rootless

None

Upstream Latest Release

Yes

Additional environment details

n/a

Additional information

Using the CONTAINERS_HELPER_BINARY_DIR env variable works:

$ CONTAINERS_HELPER_BINARY_DIR="$PWD" podman run -it --rm --network=pasta alpine sh

[PhrozenByte]

Here's the related code:

podman/libpod/networking_pasta_linux.go

Lines 25 to 28 in 493aac6

	path, err := r.config.FindHelperBinary("pasta", true)
	if err != nil {
	return fmt.Errorf("could not find pasta, the network namespace can't be configured: %w", err)
	}

In comparison, here's slirp4netns' binary lookup code (plus some duplicate code):

podman/libpod/networking_slirp4netns.go

Lines 216 to 223 in 493aac6

	path := r.config.Engine.NetworkCmdPath
	if path == "" {
	var err error
	path, err = exec.LookPath("slirp4netns")
	if err != nil {
	return fmt.Errorf("could not find slirp4netns, the network namespace can't be configured: %w", err)
	}
	}

According to the manpage, --network-cmd-path is supposed to work for both slirp4netns and pasta:

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

Lines 85 to 86 in 493aac6

	#### **--network-cmd-path**=*path*
	Path to the command binary to use for setting up a network. It is currently only used for setting up a slirp4netns(1) or pasta(1) network. If "" is used then the binary is looked up using the $PATH environment variable.

IMHO both slirp4netns and pasta should first check --network-cmd-path and if it wasn't found, fall back to FindHelperBinary (i.e. to search in $PATH, but still support CONTAINERS_HELPER_BINARY_DIR). However, pasta should check whether the binary actually is pasta, like slirp4netns does (see here).

CC: @sbrivio-rh

[Luap99]

The docs are wrong, this works as designed. We really should not use one conf field for two different things. This would just break people who already use this for slirp4netns.

Do you really need a cli option for this? I think the helper_binaries_dir field in container.conf or the env var as you found s better for this. We have plenty of external dependencies and I think having one option for them is better.

I know we have a lot of older dependencies which all use their own fields but I try to have new things use helper_binaries_dir instead. One option is much easier to work with maintenance wise.

[PhrozenByte]

I'm a bit torn about this... On one hand I absolutely agree, using the same --network-cmd-path option for different things isn't optimal. On the other hand, the helper_binaries_dir config resp. the CONTAINERS_HELPER_BINARY_DIR env variable is basically the same, if not even worse, because it doesn't just affect Pasta or slirp4netns, but anything (Qemu, catatonit, vfkit, …). I just want to specify a custom path for Pasta, not for Qemu...

I can't follow your point that using --network-cmd-path for Pasta too would break the usage for slirp4netns users. Why is that? AFAIK Pasta and slirp4netns are mutually exclusive, i.e. you can't use both at the same time. It's either

$ podman run -it --rm --network=pasta --network-cmd-path=./pasta alpine sh

or

$ podman run -it --rm --network=slirp4netns --network-cmd-path=./slirp4netns alpine sh

The --network-cmd-path option tells Podman the networking binary to use. If you want networking with Pasta, you specify the path to Pasta. If you want networking with slirp4netns, you specify the path to slirp4netns. Using the same option (--network-cmd-path) for two things isn't optimal even if both usages are mutually exclusive, but how is using another option (helper_binaries_dir resp. CONTAINERS_HELPER_BINARY_DIR) for even more things better?

Anyway, I said that I'm torn about it: --network-cmd-path is better than helper_binaries_dir IMHO, but optimal would be to indeed have distinct configs for every single helper binary - just not as CLI option (it's not really a runtime choice), not as env variable (CONTAINERS_HELPER_BINARY_DIR is fine, because it's undocumented and meant for testing), but as separate configs in containers.conf (pasta_path, slirp4netns_path, qemu_path, …).

[Luap99]

Well one field is just much less maintenance overhead. I find it much more simple to only call FindHelperBinary() and then use it vs having to create a PR in another repo to add new config field sthen use it etc...
Also as far as packing is concerned it is also much better for them, they only need to patch or change one config field and all the binaries they install are in the right place.
Defaults for a single field are much more difficult as they would need to change every field. This may be fine when the default is lookup in $PATH but we do not have that for all binaries. Distros have different rules around where to place files, e.g. /usr/lib vs /usr/libexec. It would be impossible to have upstream defaults that work for everyone unless we start lookup a list of default locations.
And this is what helper_binaries_dir really is, a list of directories and you can set as many as you want so just add your pasta directory. We will still lookup qemu, catatonit, in the other locations.

I can't follow your point that using --network-cmd-path for Pasta too would break the usage for slirp4netns users. Why is that? AFAIK Pasta and slirp4netns are mutually exclusive, i.e. you can't use both at the same time. It's either

Well sure for a single podman run but this a config field. We have no idea if is was set on the cli or the file in the network code. And even if we do we still have the podman system service which can very much result in the creation of two containers with pasta and slirp4netns.

[PhrozenByte]

All valid points: I was neither aware that helper_binaries_dir actually is a list of directories (I falsely assumed it was a single directory like CONTAINERS_HELPER_BINARY_DIR), nor did I think about podman system service. Even though the latter indeed only contradicts --network-cmd-path, I can agree that adding configs for each binary is additional maintenance effort that might not be worth it. helper_binaries_dir being a list of directories makes this less of an issue, too.

So yeah, we might leave it with helper_binaries_dir.

Besides the docs being wrong about --network-cmd-path also affecting Pasta, I'd like to point out that - as long as I'm reading the code right - slirp4netns doesn't honour helper_binaries_dir? Therefore I'd like to suggest unifying this: slirp4netns should also honour helper_binaries_dir and --network-cmd-path should be deprecated (I'm not sure about Podman's policy when to remove deprecated stuff).

[Luap99]

Besides the docs being wrong about --network-cmd-path also affecting Pasta, I'd like to point out that - as long as I'm reading the code right - slirp4netns doesn't honour helper_binaries_dir? Therefore I'd like to suggest unifying this: slirp4netns should also honour helper_binaries_dir and --network-cmd-path should be deprecated (I'm not sure about Podman's policy when to remove deprecated stuff).

Yes docs must be fixed, changing slirp4netns over sounds good to me as well. We can deprecate but breaking changes (i.e. removing) can only be done for major versions. And even then I would like to keep breaking changes to a minimum.

[PhrozenByte]

Okay, so we have three tasks here:

• Fix the docs about falsely stating that --network-cmd-path also affects Pasta. I can open a PR for that.
• Marking --network-cmd-path as deprecated. Adding a note to the docs is easy, but how can we keep track of removing it with the next major version (or at least consider removing it when the next major version is due)?
• Updating slirp4netns' code to also honour helper_binaries_dir. I guess opening a separate GitHub issue would be best here (since I've never worked with Go and don't have time to learn it, I can't do this myself)

[Luap99]

Marking --network-cmd-path as deprecated. Adding a note to the docs is easy, but how can we keep track of removing it with the next major version (or at least consider removing it when the next major version is due)?

I added TODO (5.0): comments in the code for this. So if we decide we need a major bump I will go trough them and then we can decide if we want to change it or not.