-
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
Pass full NetworkMode to ParseNetworkNamespace #8638
Pass full NetworkMode to ParseNetworkNamespace #8638
Conversation
This looks more correct on first glance. I think discarding the error is probably wrong, we should at least be logging it. |
Thanks @kwiesmueller LGTM |
This should create the correct namespace for NetworkModes like container:containerid Signed-off-by: Kevin Wiesmueller <[email protected]>
42fbfe3
to
af74d01
Compare
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude, kwiesmueller 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 |
/hold cancel |
When creating a container using a docker client pointed to the compat api, setting the HostConfig.NetworkMode to something like
container:8a16f7abf12aee67342f3e5a9323c6ec33e70ac2e6365c8e4866a45a5b80478d
there is an error returned:After looking at the code, it seems like https://github.com/kwiesmueller/podman/blob/master/pkg/specgen/namespaces.go#L284 checks for the passed in
ns
to have acontainer:
prefix. But theNetworkMode.NetworkName()
method only returnscontainer
.Note that this fix is probably wrong, it solved it for me locally but I'm not familiar enough with what's going on yet and what this might cause.
/hold