-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
slirp4netns should honour the helper_binaries_dir
config
#18568
Labels
Good First Issue
This issue would be a good issue for a first time contributor to undertake.
kind/feature
Categorizes issue or PR as related to a new feature.
locked - please file new issue/PR
Assist humans wanting to comment on an old issue or PR with locked comments.
Comments
PhrozenByte
added
the
kind/feature
Categorizes issue or PR as related to a new feature.
label
May 15, 2023
openshift-merge-robot
pushed a commit
that referenced
this issue
May 15, 2023
The `--network-cmd-path` CLI option only affects rootless networks using `slirp4netns(1)`, not `pasta(1)`. Following #18568 Podman should rather use the more generic `r.config.FindHelperBinary()` method (and therefore honour the `helper_binaries_dir` config) to find the path to the `slirp4netns` binary and deprecate the misleading `--network-cmd-path` CLI option. However, since this wasn't implemented yet we can't deprecate `--network-cmd-path` as of now. Adding a note anyway. Fixes #18560 Signed-off-by: Daniel Rudolf <[email protected]>
Interested in opening a PR? |
rhatdan
added
the
Good First Issue
This issue would be a good issue for a first time contributor to undertake.
label
May 16, 2023
Even though I'd love to learn Go, time's too limited for that, sorry 😒 |
I want work for this, if you don't mind, i will finish this issue. |
SGTM |
HirazawaUi
added a commit
to HirazawaUi/podman
that referenced
this issue
May 18, 2023
[NO NEW TESTS NEEDED] Fixes: containers#18568 Signed-off-by: binghongtao <[email protected]>
HirazawaUi
added a commit
to HirazawaUi/podman
that referenced
this issue
May 19, 2023
[NO NEW TESTS NEEDED] Fixes: containers#18568 Signed-off-by: binghongtao <[email protected]>
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
Aug 23, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
Good First Issue
This issue would be a good issue for a first time contributor to undertake.
kind/feature
Categorizes issue or PR as related to a new feature.
locked - please file new issue/PR
Assist humans wanting to comment on an old issue or PR with locked comments.
Feature request description
Podman's slirp4netns implementation currently uses a custom logic to find the
slirp4netns
binary. Seepodman/libpod/networking_slirp4netns.go
Lines 216 to 223 in 493aac6
For some time now Podman chose to rely on
r.config.FindHelperBinary()
to find helper binaries. This method uses thehelper_binaries_dir
config instorage.conf
(or the purposely undocumentedCONTAINERS_HELPER_BINARY_DIR
env variable for testing) and falls back to$PATH
otherwise.slirp4netns instead uses the custom single-purpose CLI option
--network-cmd-path
with a fallback to$PATH
. slirp4netns doesn't honour thehelper_binaries_dir
config right now. Setting a path for slirp4netns is unnecessary hard because of this; due to the CLI option's generic name and a false documentation it even caused some confusion in the past (see #18560 and #18569 to fix it).Suggest potential solution
Unify how Podman finds helper binaries: Podman should use
r.config.FindHelperBinary()
to find theslirp4netns
binary and deprecate the--network-cmd-path
CLI option.For backwards compatibility reasons we must keep the
--network-cmd-path
CLI option for now (it should take precedence for now), but deprecate it by adding a note to thepodman(1)
manpage. We should then consider removing the option with Podman's next major release.Have you considered any alternatives?
Previous discussion and reasoning for this approach see #18560
Additional context
Also see #18569 for the docs fixes
Cc: @Luap99
The text was updated successfully, but these errors were encountered: