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

Allow multiple --network flags for podman run/create #8410

Merged

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Nov 19, 2020

We allow a container to be connected to several cni networks
but only if they are listed comma sperated. This is not intuitive
for users especially since the flag parsing allows multiple string
flags but only would take the last value. see: spf13/pflag#72

Also get rid of the extra parsing logic for pods. The invalid options
are already handled by pkg/specgen.

A test is added to prevent a future regression.

@mheon
Copy link
Member

mheon commented Nov 19, 2020

I'm not strictly opposed, but if we do this we need to be very sure the manpage makes it clear that this is only for multiple networks - I'm afraid of users thinking that podman run --net slirp4netns --net host and similar invocations are valid.

@rhatdan
Copy link
Member

rhatdan commented Nov 19, 2020

Could we add a failure mode where the user did do --net=host --net=container:foobar

@TomSweeneyRedHat
Copy link
Member

The code LGTM, but share @mheon's thought on man page updates for this.

@zhangguanzhang
Copy link
Collaborator

Could we add a failure mode where the user did do --net=host --net=container:foobar

agree

@Luap99
Copy link
Member Author

Luap99 commented Nov 20, 2020

Updated. This is now the error.

$ podman run --network host --network foo-a alpine
Error: network conflict between type host and bridge

I also removed the extra pod parsing logic. The invalid options are already handled in pkg/specgen

@mheon
Copy link
Member

mheon commented Nov 20, 2020

/approve
Restarted one test which looked flaked.
LGTM

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, mheon

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 20, 2020
We allow a container to be connected to several cni networks
but only if they are listed comma sperated. This is not intuitive
for users especially since the flag parsing allows multiple string
flags but only would take the last value. see: spf13/pflag#72

Also get rid of the extra parsing logic for pods. The invalid options
are already handled by `pkg/specgen`.

A test is added to prevent a future regression.

Signed-off-by: Paul Holzinger <paul.holzinger@web.de>
@Luap99
Copy link
Member Author

Luap99 commented Nov 20, 2020

Tests are green.
@containers/podman-maintainers PTAL

@TomSweeneyRedHat
Copy link
Member

LGTM
Thanks @Luap99 !

@mheon
Copy link
Member

mheon commented Nov 21, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 21, 2020
@openshift-merge-robot openshift-merge-robot merged commit b30ca11 into containers:master Nov 21, 2020
@github-actions 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 Sep 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants