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.

Sign up for GitHub

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

Jump to bottom

Add the option of Rootless CNI networking by default #10447

Merged
merged 1 commit into from
May 26, 2021
Merged

Add the option of Rootless CNI networking by default #10447

merged 1 commit into from
May 26, 2021

Conversation

mheon
Copy link
Member

@mheon mheon commented May 24, 2021

When the containers.conf field "NetNS" is set to "Bridge" and the "RootlessNetworking" field is set to "cni", Podman will now handle rootless in the same way it does root - all containers will be joined to a default CNI network, instead of exclusively using slirp4netns.

If no CNI default network config is present for the user, one will be auto-generated (this also works for root, but it won't be nearly as common there since the package should already ship a config).

I eventually hope to remove the "NetNS=Bridge" bit from containers.conf, but let's get something in for Brent to work with.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 24, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 24, 2021
@Luap99
Copy link
Member

Luap99 commented May 24, 2021

Sorry I'm late, but why do we need two configuration fields? Shouldn't netns=bridge and netns=slirp4netns be enough. I don't see the need for the RootlessNetworking field?!

Luap99
Luap99 reviewed May 24, 2021
View reviewed changes
libpod/runtime.go Outdated
@@ -532,6 +533,13 @@ func makeRuntime(ctx context.Context, runtime *Runtime) (retErr error) {
}
}

// If we need to make a default network - do so now.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should happen before ocicni.InitCNI (line 465).

@mheon mheon force-pushed the rootlesscni_default branch from 59aaa70 to 32a56fc Compare May 24, 2021 22:23
@mheon
Copy link
Member Author

mheon commented May 24, 2021

@Luap99 The idea was to keep NetNS=private in all cases, and just adjust RootlessNetworking to set the default behavior. I'm beginning to doubt that it's necessary, though. Fortunately, I think we can remove the field without adverse effect in the future - TOML doesn't really care about field names it doesn't know about.

@Luap99
Copy link
Member

Luap99 commented May 25, 2021

OK, one last point. When the compat api is used as rootless we default to slirp4netns, see #10261. We could not swap the default to bridge since there was no default cni config. With this patch we could change this. However, the config is only created when rootless_networking="cni" is set in containers.conf. I would argue that we should always create the default cni config since this would allow us to default to bridge in the compat api without forcing the user to change his config file.

@mheon
Copy link
Member Author

mheon commented May 25, 2021

I was waffling on that one, but if we have a good reason for it to be universal, I'm happy to make the change.

@mheon mheon force-pushed the rootlesscni_default branch 2 times, most recently from 43a7af4 to 3a64dbf Compare May 25, 2021 15:28
@mheon
Copy link
Member Author

mheon commented May 25, 2021

@Luap99 Made default network creation by default. I left the RootlessNetworking bits intact for now - mainly because I don't see an easier way of setting the default for rootless if the user sets --net=private or some other default value (or the equivalent in containers.conf). I think this is probably worth further discussion, and we have time to do that since this won't ship in 3.2. Test has been added. This is probably good to merge if it goes green, barring further discussion.

@mheon
Copy link
Member Author

mheon commented May 25, 2021

We also probably want to have a discussion about containers.conf stability - I feel like it's not held up to the same standards of stability as the rest of the API because we can remove config options without much consequence. So even if this did ship, IMO we still have the ability to remove it in the future without a major version bump...

@mheon mheon force-pushed the rootlesscni_default branch from 3a64dbf to c7ce571 Compare May 25, 2021 16:57
Luap99
Luap99 reviewed May 26, 2021
View reviewed changes
cmd/podman/common/create_opts.go Outdated
@@ -248,7 +249,7 @@ func ContainerCreateToContainerCLIOpts(cc handlers.CreateContainerConfig, cgroup
}

// netMode
nsmode, _, err := specgen.ParseNetworkNamespace(string(cc.HostConfig.NetworkMode))
nsmode, _, err := specgen.ParseNetworkNamespace(string(cc.HostConfig.NetworkMode), rtc.Containers.RootlessNetworking == "cni")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

always set this to true for compat api, this should fix #10261

libpod/container.go Outdated
@@ -1168,7 +1167,7 @@ func (c *Container) Networks() ([]string, bool, error) {
func (c *Container) networks() ([]string, bool, error) {
networks, err := c.runtime.state.GetNetworks(c)
if err != nil && errors.Cause(err) == define.ErrNoSuchNetwork {
if len(c.config.Networks) == 0 && !rootless.IsRootless() {
if len(c.config.Networks) == 0 && !c.config.NetMode.IsSlirp4netns() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be IsBridge() to not match net=none

@mheon mheon force-pushed the rootlesscni_default branch from c7ce571 to 0889be9 Compare May 26, 2021 17:49
@Luap99
Copy link
Member

Luap99 commented May 26, 2021

@mheon you have to rebase

@mheon
Add the option of Rootless CNI networking by default
533d88b 
When the containers.conf field "NetNS" is set to "Bridge" and the
"RootlessNetworking" field is set to "cni", Podman will now
handle rootless in the same way it does root - all containers
will be joined to a default CNI network, instead of exclusively
using slirp4netns.

If no CNI default network config is present for the user, one
will be auto-generated (this also works for root, but it won't be
nearly as common there since the package should already ship a
config).

I eventually hope to remove the "NetNS=Bridge" bit from
containers.conf, but let's get something in for Brent to work
with.

Signed-off-by: Matthew Heon <[email protected]>
@mheon mheon force-pushed the rootlesscni_default branch from 0889be9 to 533d88b Compare May 26, 2021 19:03
@mheon
Copy link
Member Author

mheon commented May 26, 2021

Done.

Luap99
Luap99 reviewed May 26, 2021
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@TomSweeneyRedHat
Copy link
Member

LGTM

@rhatdan
Copy link
Member

rhatdan commented May 26, 2021

/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 26, 2021
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 26, 2021
@baude
Copy link
Member

baude commented May 26, 2021

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 26, 2021
@openshift-merge-robot openshift-merge-robot merged commit d9eb126 into containers:master May 26, 2021
@riyad riyad mentioned this pull request Jun 4, 2021
Closed
@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 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Luap99 Luap99 Luap99 left review comments

Assignees

@rhatdan rhatdan

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mheon @Luap99 @TomSweeneyRedHat @rhatdan @baude @openshift-merge-robot