From 8031869f31b194f27b8e8af926f298106b031c17 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 4 May 2023 16:54:33 +0200 Subject: [PATCH] libpod: always use PostConfigureNetNS option Maintaining two code paths for network setup has been difficult, I had to deal with many bugs because of that. Often the PostConfigureNetNS was not as tested. Overall the code has quite a bit of complexity because of this option. Just look at this commit how much simpler the code now looks. The main advantage of this is that we no longer have to test everything twice, i.e. with userns and without one. The downside is that mount and netns setup is no longer parallel but I think this is fine, it didn't seem to make a measurable difference. To preserve compatibility in case of an downgrade we keep the PostConfigureNetNS bool and set it always to true. This turned out to be much more complicated that thought due to our spaghetti code. The restart=always case is special because we reuse the netns. But the slirp4netns and rootlessport process are bound to the conmon process so they have to be restarted. Given the the network is now setup in completeNetworkSetup() which is always called by init() which is called in handleRestartPolicy(). Therefore we can get rid of the special rootless setup function to restart slirp and rootlessport. Instead we let one single network setup function take care of that which is used in all cases. [NO NEW TESTS NEEDED] Existing test should all pass. Fixes #17965 Signed-off-by: Paul Holzinger --- libpod/container_config.go | 2 + libpod/container_freebsd.go | 5 +- libpod/container_internal.go | 55 +++------- libpod/container_internal_common.go | 80 ++++++++------ libpod/container_internal_freebsd.go | 94 ++--------------- libpod/container_internal_linux.go | 127 ++-------------------- libpod/container_linux.go | 8 +- libpod/networking_common.go | 3 +- libpod/networking_freebsd.go | 4 - libpod/networking_linux.go | 152 ++++++++++++++------------- libpod/networking_slirp4netns.go | 28 +---- libpod/networking_unsupported.go | 4 - libpod/oci_conmon_common.go | 25 ++--- libpod/options.go | 5 +- pkg/specgen/generate/namespaces.go | 7 +- test/system/500-networking.bats | 8 +- 16 files changed, 194 insertions(+), 413 deletions(-) diff --git a/libpod/container_config.go b/libpod/container_config.go index 6aabc817ac..7343c102a4 100644 --- a/libpod/container_config.go +++ b/libpod/container_config.go @@ -384,6 +384,8 @@ type ContainerMiscConfig struct { // PostConfigureNetNS needed when a user namespace is created by an OCI runtime // if the network namespace is created before the user namespace it will be // owned by the wrong user namespace. + // Deprecated: Do not use this anymore. Always set to true for new containers to + // allow compatibility in case of a downgrade. PostConfigureNetNS bool `json:"postConfigureNetNS"` // OCIRuntime used to create the container OCIRuntime string `json:"runtime,omitempty"` diff --git a/libpod/container_freebsd.go b/libpod/container_freebsd.go index cb0a465b23..6b7c2b07b9 100644 --- a/libpod/container_freebsd.go +++ b/libpod/container_freebsd.go @@ -7,8 +7,5 @@ func networkDisabled(c *Container) (bool, error) { if c.config.CreateNetNS { return false, nil } - if !c.config.PostConfigureNetNS { - return c.state.NetNS != "", nil - } - return false, nil + return c.state.NetNS != "", nil } diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 9be8b9397e..4dda60312d 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -17,7 +17,6 @@ import ( "github.com/containers/buildah/pkg/overlay" butil "github.com/containers/buildah/util" "github.com/containers/common/libnetwork/etchosts" - "github.com/containers/common/libnetwork/resolvconf" "github.com/containers/common/pkg/cgroups" "github.com/containers/common/pkg/chown" "github.com/containers/common/pkg/config" @@ -316,11 +315,6 @@ func (c *Container) handleRestartPolicy(ctx context.Context) (_ bool, retErr err return false, err } - // set up slirp4netns again because slirp4netns will die when conmon exits - if err := c.setupRootlessNetwork(); err != nil { - return false, err - } - if c.state.State == define.ContainerStateStopped { // Reinitialize the container if we need to if err := c.reinit(ctx, true); err != nil { @@ -983,52 +977,33 @@ func (c *Container) checkDependenciesRunning() ([]string, error) { return notRunning, nil } -func (c *Container) completeNetworkSetup() error { - var nameservers []string +func (c *Container) completeNetworkSetup(restore bool) error { netDisabled, err := c.NetworkDisabled() if err != nil { return err } - if !c.config.PostConfigureNetNS || netDisabled { - return nil - } - if err := c.syncContainer(); err != nil { - return err - } - if err := c.runtime.setupNetNS(c); err != nil { - return err - } - if err := c.save(); err != nil { - return err + if netDisabled { + // with net=none we still want to set up /etc/hosts + return c.addHosts() } - state := c.state - // collect any dns servers that the network backend tells us to use - for _, status := range c.getNetworkStatus() { - for _, server := range status.DNSServerIPs { - nameservers = append(nameservers, server.String()) - } + if c.config.NetNsCtr != "" { + return nil } - nameservers = c.addSlirp4netnsDNS(nameservers) - - // check if we have a bindmount for /etc/hosts - if hostsBindMount, ok := state.BindMounts[config.DefaultHostsFile]; ok { - entries, err := c.getHostsEntries() - if err != nil { + if c.config.CreateNetNS { + if err := c.runtime.setupNetNS(c, restore); err != nil { return err } - // add new container ips to the hosts file - if err := etchosts.Add(hostsBindMount, entries); err != nil { + if err := c.save(); err != nil { return err } } - // check if we have a bindmount for resolv.conf - resolvBindMount := state.BindMounts[resolvconf.DefaultResolvConf] - if len(nameservers) < 1 || resolvBindMount == "" || len(c.config.NetNsCtr) > 0 { - return nil + // add /etc/hosts entries + if err := c.addHosts(); err != nil { + return err } - // write and return - return resolvconf.Add(resolvBindMount, nameservers) + + return c.addResolvConf() } // Initialize a container, creating it in the runtime @@ -1128,7 +1103,7 @@ func (c *Container) init(ctx context.Context, retainRetries bool) error { } defer c.newContainerEvent(events.Init) - return c.completeNetworkSetup() + return c.completeNetworkSetup(false) } // Clean up a container in the OCI runtime. diff --git a/libpod/container_internal_common.go b/libpod/container_internal_common.go index 1e16a93feb..0068d8c7a9 100644 --- a/libpod/container_internal_common.go +++ b/libpod/container_internal_common.go @@ -1466,6 +1466,12 @@ func (c *Container) restore(ctx context.Context, options ContainerCheckpointOpti g.SetRootPath(c.state.Mountpoint) } + // For restore we have to create the netns before, this makes + // it not work correctly with userns but there is no other way + // as restore will start the container right away, see #18502. + if err := c.completeNetworkSetup(true); err != nil { + return nil, 0, err + } // We want to have the same network namespace as before. if err := c.addNetworkNamespace(&g); err != nil { return nil, 0, err @@ -1857,13 +1863,13 @@ func (c *Container) makeBindMounts() error { } } else { if !c.config.UseImageResolvConf { - if err := c.generateResolvConf(); err != nil { + if err := c.createResolvConf(); err != nil { return fmt.Errorf("creating resolv.conf for container %s: %w", c.ID(), err) } } if !c.config.UseImageHosts { - if err := c.createHosts(); err != nil { + if err := c.createHostsFile(); err != nil { return fmt.Errorf("creating hosts file for container %s: %w", c.ID(), err) } } @@ -1881,7 +1887,7 @@ func (c *Container) makeBindMounts() error { } } } else if !c.config.UseImageHosts && c.state.BindMounts["/etc/hosts"] == "" { - if err := c.createHosts(); err != nil { + if err := c.createHostsFile(); err != nil { return fmt.Errorf("creating hosts file for container %s: %w", c.ID(), err) } } @@ -2016,13 +2022,30 @@ rootless=%d return c.makePlatformBindMounts() } -// generateResolvConf generates a containers resolv.conf -func (c *Container) generateResolvConf() error { +// createResolvConf create the resolv.conf file and bind mount it +func (c *Container) createResolvConf() error { + destPath := filepath.Join(c.state.RunDir, "resolv.conf") + f, err := os.Create(destPath) + if err != nil { + return err + } + f.Close() + return c.bindMountRootFile(destPath, resolvconf.DefaultResolvConf) +} + +// addResolvConf add resolv.conf entries +func (c *Container) addResolvConf() error { var ( networkNameServers []string networkSearchDomains []string ) + destPath, ok := c.state.BindMounts[resolvconf.DefaultResolvConf] + if !ok { + // no resolv.conf mount, do nothing + return nil + } + netStatus := c.getNetworkStatus() for _, status := range netStatus { if status.DNSServerIPs != nil { @@ -2072,9 +2095,7 @@ func (c *Container) generateResolvConf() error { // slirp4netns has a built in DNS forwarder. // If in userns the network is not setup here, instead we need to do that in // c.completeNetworkSetup() which knows the actual slirp dns ip only at that point - if !c.config.PostConfigureNetNS { - nameservers = c.addSlirp4netnsDNS(nameservers) - } + nameservers = c.addSlirp4netnsDNS(nameservers) } // Set DNS search domains @@ -2091,8 +2112,6 @@ func (c *Container) generateResolvConf() error { options = append(options, c.runtime.config.Containers.DNSOptions...) options = append(options, c.config.DNSOption...) - destPath := filepath.Join(c.state.RunDir, "resolv.conf") - var namespaces []spec.LinuxNamespace if c.config.Spec.Linux != nil { namespaces = c.config.Spec.Linux.Namespaces @@ -2109,8 +2128,7 @@ func (c *Container) generateResolvConf() error { }); err != nil { return fmt.Errorf("building resolv.conf for container %s: %w", c.ID(), err) } - - return c.bindMountRootFile(destPath, resolvconf.DefaultResolvConf) + return nil } // Check if a container uses IPv6. @@ -2197,36 +2215,38 @@ func (c *Container) getHostsEntries() (etchosts.HostEntries, error) { return entries, nil } -func (c *Container) createHosts() error { - var containerIPsEntries etchosts.HostEntries - var err error - // if we configure the netns after the container create we should not add - // the hosts here since we have no information about the actual ips - // instead we will add them in c.completeNetworkSetup() - if !c.config.PostConfigureNetNS { - containerIPsEntries, err = c.getHostsEntries() - if err != nil { - return fmt.Errorf("failed to get container ip host entries: %w", err) - } +func (c *Container) createHostsFile() error { + targetFile := filepath.Join(c.state.RunDir, "hosts") + f, err := os.Create(targetFile) + if err != nil { + return err + } + f.Close() + return c.bindMountRootFile(targetFile, config.DefaultHostsFile) +} + +func (c *Container) addHosts() error { + targetFile, ok := c.state.BindMounts[config.DefaultHostsFile] + if !ok { + // no host file nothing to do + return nil + } + containerIPsEntries, err := c.getHostsEntries() + if err != nil { + return fmt.Errorf("failed to get container ip host entries: %w", err) } baseHostFile, err := etchosts.GetBaseHostFile(c.runtime.config.Containers.BaseHostsFile, c.state.Mountpoint) if err != nil { return err } - targetFile := filepath.Join(c.state.RunDir, "hosts") - err = etchosts.New(&etchosts.Params{ + return etchosts.New(&etchosts.Params{ BaseFile: baseHostFile, ExtraHosts: c.config.HostAdd, ContainerIPs: containerIPsEntries, HostContainersInternalIP: etchosts.GetHostContainersInternalIP(c.runtime.config, c.state.NetworkStatus, c.runtime.network), TargetFile: targetFile, }) - if err != nil { - return err - } - - return c.bindMountRootFile(targetFile, config.DefaultHostsFile) } // bindMountRootFile will chown and relabel the source file to make it usable in the container. diff --git a/libpod/container_internal_freebsd.go b/libpod/container_internal_freebsd.go index de3a0f63f6..ebc0b5faed 100644 --- a/libpod/container_internal_freebsd.go +++ b/libpod/container_internal_freebsd.go @@ -8,11 +8,9 @@ import ( "os" "path/filepath" "strings" - "sync" "syscall" "time" - "github.com/containers/common/libnetwork/types" "github.com/containers/podman/v4/pkg/rootless" spec "github.com/opencontainers/runtime-spec/specs-go" "github.com/opencontainers/runtime-tools/generate" @@ -35,93 +33,19 @@ func (c *Container) unmountSHM(path string) error { // prepare mounts the container and sets up other required resources like net // namespaces func (c *Container) prepare() error { - var ( - wg sync.WaitGroup - ctrNS string - networkStatus map[string]types.StatusBlock - createNetNSErr, mountStorageErr error - mountPoint string - tmpStateLock sync.Mutex - ) - - wg.Add(2) - - go func() { - defer wg.Done() - // Set up network namespace if not already set up - noNetNS := c.state.NetNS == "" - if c.config.CreateNetNS && noNetNS && !c.config.PostConfigureNetNS { - ctrNS, networkStatus, createNetNSErr = c.runtime.createNetNS(c) - if createNetNSErr != nil { - return - } - - tmpStateLock.Lock() - defer tmpStateLock.Unlock() - - // Assign NetNS attributes to container - c.state.NetNS = ctrNS - c.state.NetworkStatus = networkStatus - } - }() - // Mount storage if not mounted - go func() { - defer wg.Done() - mountPoint, mountStorageErr = c.mountStorage() - - if mountStorageErr != nil { - return - } - - tmpStateLock.Lock() - defer tmpStateLock.Unlock() - - // Finish up mountStorage - c.state.Mounted = true - c.state.Mountpoint = mountPoint - - logrus.Debugf("Created root filesystem for container %s at %s", c.ID(), c.state.Mountpoint) - }() - - wg.Wait() - - var createErr error - if createNetNSErr != nil { - createErr = createNetNSErr - } - if mountStorageErr != nil { - if createErr != nil { - logrus.Errorf("Preparing container %s: %v", c.ID(), createErr) - } - createErr = mountStorageErr + mountPoint, err := c.mountStorage() + if err != nil { + return err } - // Only trigger storage cleanup if mountStorage was successful. - // Otherwise, we may mess up mount counters. - if createErr != nil { - if mountStorageErr == nil { - if err := c.cleanupStorage(); err != nil { - // createErr is guaranteed non-nil, so print - // unconditionally - logrus.Errorf("Preparing container %s: %v", c.ID(), createErr) - createErr = fmt.Errorf("unmounting storage for container %s after network create failure: %w", c.ID(), err) - } - } - // It's OK to unconditionally trigger network cleanup. If the network - // isn't ready it will do nothing. - if err := c.cleanupNetwork(); err != nil { - logrus.Errorf("Preparing container %s: %v", c.ID(), createErr) - createErr = fmt.Errorf("cleaning up container %s network after setup failure: %w", c.ID(), err) - } - return createErr - } + // Finish up mountStorage + c.state.Mounted = true + c.state.Mountpoint = mountPoint - // Save changes to container state - if err := c.save(); err != nil { - return err - } + logrus.Debugf("Created root filesystem for container %s at %s", c.ID(), c.state.Mountpoint) - return nil + // Save changes to container state + return c.save() } // cleanupNetwork unmounts and cleans up the container's network diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go index 0a4f99a24e..46f7998be7 100644 --- a/libpod/container_internal_linux.go +++ b/libpod/container_internal_linux.go @@ -11,11 +11,9 @@ import ( "path" "path/filepath" "strings" - "sync" "syscall" "time" - "github.com/containers/common/libnetwork/types" "github.com/containers/common/pkg/cgroups" "github.com/containers/common/pkg/config" "github.com/containers/podman/v4/libpod/define" @@ -59,97 +57,19 @@ func (c *Container) unmountSHM(mount string) error { // prepare mounts the container and sets up other required resources like net // namespaces func (c *Container) prepare() error { - var ( - wg sync.WaitGroup - netNS string - networkStatus map[string]types.StatusBlock - createNetNSErr, mountStorageErr error - mountPoint string - tmpStateLock sync.Mutex - ) - - wg.Add(2) - - go func() { - defer wg.Done() - // Set up network namespace if not already set up - noNetNS := c.state.NetNS == "" - if c.config.CreateNetNS && noNetNS && !c.config.PostConfigureNetNS { - netNS, networkStatus, createNetNSErr = c.runtime.createNetNS(c) - if createNetNSErr != nil { - return - } - - tmpStateLock.Lock() - defer tmpStateLock.Unlock() - - // Assign NetNS attributes to container - c.state.NetNS = netNS - c.state.NetworkStatus = networkStatus - } - }() - // Mount storage if not mounted - go func() { - defer wg.Done() - mountPoint, mountStorageErr = c.mountStorage() - - if mountStorageErr != nil { - return - } - - tmpStateLock.Lock() - defer tmpStateLock.Unlock() - - // Finish up mountStorage - c.state.Mounted = true - c.state.Mountpoint = mountPoint - - logrus.Debugf("Created root filesystem for container %s at %s", c.ID(), c.state.Mountpoint) - }() - - wg.Wait() - - var createErr error - if createNetNSErr != nil { - createErr = createNetNSErr - } - if mountStorageErr != nil { - if createErr != nil { - logrus.Errorf("Preparing container %s: %v", c.ID(), createErr) - } - createErr = mountStorageErr - } - - // Only trigger storage cleanup if mountStorage was successful. - // Otherwise, we may mess up mount counters. - if createNetNSErr != nil && mountStorageErr == nil { - if err := c.cleanupStorage(); err != nil { - // createErr is guaranteed non-nil, so print - // unconditionally - logrus.Errorf("Preparing container %s: %v", c.ID(), createErr) - createErr = fmt.Errorf("unmounting storage for container %s after network create failure: %w", c.ID(), err) - } + mountPoint, err := c.mountStorage() + if err != nil { + return err } - // It's OK to unconditionally trigger network cleanup. If the network - // isn't ready it will do nothing. - if createErr != nil { - if err := c.cleanupNetwork(); err != nil { - logrus.Errorf("Preparing container %s: %v", c.ID(), createErr) - createErr = fmt.Errorf("cleaning up container %s network after setup failure: %w", c.ID(), err) - } - } + // Finish up mountStorage + c.state.Mounted = true + c.state.Mountpoint = mountPoint - if createErr != nil { - return createErr - } + logrus.Debugf("Created root filesystem for container %s at %s", c.ID(), c.state.Mountpoint) // Save changes to container state - if err := c.save(); err != nil { - return err - } - - return nil + return c.save() } // cleanupNetwork unmounts and cleans up the container's network @@ -415,42 +335,13 @@ func (c *Container) getOCICgroupPath() (string, error) { } } -// If the container is rootless, set up the slirp4netns network -func (c *Container) setupRootlessNetwork() error { - // set up slirp4netns again because slirp4netns will die when conmon exits - if c.config.NetMode.IsSlirp4netns() { - err := c.runtime.setupSlirp4netns(c, c.state.NetNS) - if err != nil { - return err - } - } - - // set up rootlesskit port forwarder again since it dies when conmon exits - // we use rootlesskit port forwarder only as rootless and when bridge network is used - if rootless.IsRootless() && c.config.NetMode.IsBridge() && len(c.config.PortMappings) > 0 { - err := c.runtime.setupRootlessPortMappingViaRLK(c, c.state.NetNS, c.state.NetworkStatus) - if err != nil { - return err - } - } - return nil -} - func openDirectory(path string) (fd int, err error) { return unix.Open(path, unix.O_RDONLY|unix.O_PATH, 0) } func (c *Container) addNetworkNamespace(g *generate.Generator) error { if c.config.CreateNetNS { - if c.config.PostConfigureNetNS { - if err := g.AddOrReplaceLinuxNamespace(string(spec.NetworkNamespace), ""); err != nil { - return err - } - } else { - if err := g.AddOrReplaceLinuxNamespace(string(spec.NetworkNamespace), c.state.NetNS); err != nil { - return err - } - } + return g.AddOrReplaceLinuxNamespace(string(spec.NetworkNamespace), c.state.NetNS) } return nil } diff --git a/libpod/container_linux.go b/libpod/container_linux.go index a1a64f5dda..a514398adf 100644 --- a/libpod/container_linux.go +++ b/libpod/container_linux.go @@ -11,11 +11,9 @@ func networkDisabled(c *Container) (bool, error) { if c.config.CreateNetNS { return false, nil } - if !c.config.PostConfigureNetNS { - for _, ns := range c.config.Spec.Linux.Namespaces { - if ns.Type == spec.NetworkNamespace { - return ns.Path == "", nil - } + for _, ns := range c.config.Spec.Linux.Namespaces { + if ns.Type == spec.NetworkNamespace { + return ns.Path == "", nil } } return false, nil diff --git a/libpod/networking_common.go b/libpod/networking_common.go index 24165e1598..c6f40c49a6 100644 --- a/libpod/networking_common.go +++ b/libpod/networking_common.go @@ -206,9 +206,8 @@ func (r *Runtime) reloadContainerNetwork(ctr *Container) (map[string]types.Statu } networkOpts[network] = perNetOpts } - ctr.perNetworkOpts = networkOpts - return r.configureNetNS(ctr, ctr.state.NetNS) + return r.setUpNetwork(ctr.state.NetNS, ctr.getNetworkOptions(networkOpts)) } // Produce an InspectNetworkSettings containing information on the container diff --git a/libpod/networking_freebsd.go b/libpod/networking_freebsd.go index 8a96e7e8d1..1b4c14a2ec 100644 --- a/libpod/networking_freebsd.go +++ b/libpod/networking_freebsd.go @@ -267,7 +267,3 @@ func (c *Container) inspectJoinedNetworkNS(networkns string) (q types.StatusBloc func (c *Container) reloadRootlessRLKPortMapping() error { return errors.New("unsupported (*Container).reloadRootlessRLKPortMapping") } - -func (c *Container) setupRootlessNetwork() error { - return nil -} diff --git a/libpod/networking_linux.go b/libpod/networking_linux.go index 13befa0b37..f7e433d50b 100644 --- a/libpod/networking_linux.go +++ b/libpod/networking_linux.go @@ -560,22 +560,31 @@ func (r *Runtime) GetRootlessNetNs(new bool) (*RootlessNetNS, error) { // Create and configure a new network namespace for a container func (r *Runtime) configureNetNS(ctr *Container, ctrNS string) (status map[string]types.StatusBlock, rerr error) { - if err := r.exposeMachinePorts(ctr.config.PortMappings); err != nil { - return nil, err - } - defer func() { - // make sure to unexpose the gvproxy ports when an error happens - if rerr != nil { - if err := r.unexposeMachinePorts(ctr.config.PortMappings); err != nil { - logrus.Errorf("failed to free gvproxy machine ports: %v", err) - } + // When we get restarted via -restart policy we keep the same ns. + // In this case we only need to restart process that are bound to + // the container live cycle and not the netns, slirp and rootlessport + isNewNS := ctr.state.NetNS != ctrNS + + if isNewNS { + if err := r.exposeMachinePorts(ctr.config.PortMappings); err != nil { + return nil, err } - }() + defer func() { + // make sure to unexpose the gvproxy ports when an error happens + if rerr != nil { + if err := r.unexposeMachinePorts(ctr.config.PortMappings); err != nil { + logrus.Errorf("failed to free gvproxy machine ports: %v", err) + } + } + }() + } if ctr.config.NetMode.IsSlirp4netns() { return nil, r.setupSlirp4netns(ctr, ctrNS) } if ctr.config.NetMode.IsPasta() { - return nil, r.setupPasta(ctr, ctrNS) + if isNewNS { + return nil, r.setupPasta(ctr, ctrNS) + } } networks, err := ctr.networks() if err != nil { @@ -587,24 +596,32 @@ func (r *Runtime) configureNetNS(ctr *Container, ctrNS string) (status map[strin return nil, nil } - netOpts := ctr.getNetworkOptions(networks) - netStatus, err := r.setUpNetwork(ctrNS, netOpts) - if err != nil { - return nil, err - } - defer func() { - // do not forget to tear down the netns when a later error happened. - if rerr != nil { - if err := r.teardownNetworkBackend(ctrNS, netOpts); err != nil { - logrus.Warnf("failed to teardown network after failed setup: %v", err) - } + var netStatus map[string]types.StatusBlock + + if isNewNS { + netOpts := ctr.getNetworkOptions(networks) + netStatus, err = r.setUpNetwork(ctrNS, netOpts) + if err != nil { + return nil, err } - }() + defer func() { + // do not forget to tear down the netns when a later error happened. + if rerr != nil { + if err := r.teardownNetworkBackend(ctrNS, netOpts); err != nil { + logrus.Warnf("failed to teardown network after failed setup: %v", err) + } + } + }() + } else { + // if the netns was already setup, i.e. after restart=always, then just use + // the existing status and do the ports below + netStatus = ctr.getNetworkStatus() + } // set up rootless port forwarder when rootless with ports and the network status is empty, // if this is called from network reload the network status will not be empty and we should // not set up port because they are still active - if rootless.IsRootless() && len(ctr.config.PortMappings) > 0 && ctr.getNetworkStatus() == nil { + if rootless.IsRootless() && len(ctr.config.PortMappings) > 0 { // set up port forwarder for rootless netns // TODO: support slirp4netns port forwarder as well // make sure to fix this in container.handleRestartPolicy() as well @@ -615,65 +632,52 @@ func (r *Runtime) configureNetNS(ctr *Container, ctrNS string) (status map[strin return netStatus, err } -// Create and configure a new network namespace for a container -func (r *Runtime) createNetNS(ctr *Container) (n string, q map[string]types.StatusBlock, retErr error) { - ctrNS, err := netns.NewNS() - if err != nil { - return "", nil, fmt.Errorf("creating network namespace for container %s: %w", ctr.ID(), err) - } - defer func() { - if retErr != nil { - if err := netns.UnmountNS(ctrNS.Path()); err != nil { - logrus.Errorf("Unmounting partially created network namespace for container %s: %v", ctr.ID(), err) +// Configure the network namespace using the container process +func (r *Runtime) setupNetNS(ctr *Container, restore bool) error { + netnsPath := ctr.state.NetNS + if netnsPath == "" { + // when we restore we must create the netns before the container so we cannot use the pid + if restore { + ns, err := netns.NewNS() + if err != nil { + return err } - if err := ctrNS.Close(); err != nil { - logrus.Errorf("Closing partially created network namespace for container %s: %v", ctr.ID(), err) + netnsPath = ns.Path() + } else { + netnsPath = fmt.Sprintf("/proc/%d/ns/net", ctr.state.PID) + b := make([]byte, 16) + if _, err := rand.Reader.Read(b); err != nil { + return fmt.Errorf("failed to generate random netns name: %w", err) } - } - }() - - logrus.Debugf("Made network namespace at %s for container %s", ctrNS.Path(), ctr.ID()) - - var networkStatus map[string]types.StatusBlock - networkStatus, err = r.configureNetNS(ctr, ctrNS.Path()) - return ctrNS.Path(), networkStatus, err -} - -// Configure the network namespace using the container process -func (r *Runtime) setupNetNS(ctr *Container) error { - nsProcess := fmt.Sprintf("/proc/%d/ns/net", ctr.state.PID) - - b := make([]byte, 16) - - if _, err := rand.Reader.Read(b); err != nil { - return fmt.Errorf("failed to generate random netns name: %w", err) - } - nsPath, err := netns.GetNSRunDir() - if err != nil { - return err - } - nsPath = filepath.Join(nsPath, fmt.Sprintf("netns-%x-%x-%x-%x-%x", b[0:4], b[4:6], b[6:8], b[8:10], b[10:])) + nsPath, err := netns.GetNSRunDir() + if err != nil { + return err + } + nsPath = filepath.Join(nsPath, fmt.Sprintf("netns-%x-%x-%x-%x-%x", b[0:4], b[4:6], b[6:8], b[8:10], b[10:])) - if err := os.MkdirAll(filepath.Dir(nsPath), 0711); err != nil { - return err - } + if err := os.MkdirAll(filepath.Dir(nsPath), 0711); err != nil { + return err + } - mountPointFd, err := os.Create(nsPath) - if err != nil { - return err - } - if err := mountPointFd.Close(); err != nil { - return err - } + mountPointFd, err := os.Create(nsPath) + if err != nil { + return err + } + if err := mountPointFd.Close(); err != nil { + return err + } - if err := unix.Mount(nsProcess, nsPath, "none", unix.MS_BIND, ""); err != nil { - return fmt.Errorf("cannot mount %s: %w", nsPath, err) + if err := unix.Mount(netnsPath, nsPath, "none", unix.MS_BIND, ""); err != nil { + return fmt.Errorf("cannot mount %s to %s: %w", netnsPath, nsPath, err) + } + netnsPath = nsPath + } } - networkStatus, err := r.configureNetNS(ctr, nsPath) + networkStatus, err := r.configureNetNS(ctr, netnsPath) // Assign NetNS attributes to container - ctr.state.NetNS = nsPath + ctr.state.NetNS = netnsPath ctr.state.NetworkStatus = networkStatus return err } diff --git a/libpod/networking_slirp4netns.go b/libpod/networking_slirp4netns.go index 8e6dc8b901..e103009a61 100644 --- a/libpod/networking_slirp4netns.go +++ b/libpod/networking_slirp4netns.go @@ -212,7 +212,7 @@ func createBasicSlirp4netnsCmdArgs(options *slirp4netnsNetworkOptions, features } // setupSlirp4netns can be called in rootful as well as in rootless -func (r *Runtime) setupSlirp4netns(ctr *Container, netns string) error { +func (r *Runtime) setupSlirp4netns(ctr *Container, netnsPath string) error { path := r.config.Engine.NetworkCmdPath if path == "" { var err error @@ -261,21 +261,10 @@ func (r *Runtime) setupSlirp4netns(ctr *Container, netns string) error { apiSocket = filepath.Join(ctr.runtime.config.Engine.TmpDir, fmt.Sprintf("%s.net", ctr.config.ID)) cmdArgs = append(cmdArgs, "--api-socket", apiSocket) } - netnsPath := "" - if !ctr.config.PostConfigureNetNS { - ctr.rootlessSlirpSyncR, ctr.rootlessSlirpSyncW, err = os.Pipe() - if err != nil { - return fmt.Errorf("failed to create rootless network sync pipe: %w", err) - } - netnsPath = netns - cmdArgs = append(cmdArgs, "--netns-type=path", netnsPath, "tap0") - } else { - defer errorhandling.CloseQuiet(ctr.rootlessSlirpSyncR) - defer errorhandling.CloseQuiet(ctr.rootlessSlirpSyncW) - netnsPath = fmt.Sprintf("/proc/%d/ns/net", ctr.state.PID) - // we don't use --netns-path here (unavailable for slirp4netns < v0.4) - cmdArgs = append(cmdArgs, fmt.Sprintf("%d", ctr.state.PID), "tap0") - } + defer errorhandling.CloseQuiet(ctr.rootlessSlirpSyncR) + defer errorhandling.CloseQuiet(ctr.rootlessSlirpSyncW) + // we don't use --netns-path here (unavailable for slirp4netns < v0.4) + cmdArgs = append(cmdArgs, fmt.Sprintf("%d", ctr.state.PID), "tap0") cmd := exec.Command(path, cmdArgs...) logrus.Debugf("slirp4netns command: %s", strings.Join(cmd.Args, " ")) @@ -522,13 +511,6 @@ func (r *Runtime) setupRootlessPortMappingViaRLK(ctr *Container, netnsPath strin return fmt.Errorf("delete file %s: %w", logPath, err) } - if !ctr.config.PostConfigureNetNS { - ctr.rootlessPortSyncR, ctr.rootlessPortSyncW, err = os.Pipe() - if err != nil { - return fmt.Errorf("failed to create rootless port sync pipe: %w", err) - } - } - childIP := getRootlessPortChildIP(ctr, netStatus) cfg := rootlessport.Config{ Mappings: ctr.convertPortMappings(), diff --git a/libpod/networking_unsupported.go b/libpod/networking_unsupported.go index 7c00d25b3d..76c1602866 100644 --- a/libpod/networking_unsupported.go +++ b/libpod/networking_unsupported.go @@ -28,10 +28,6 @@ func (c *Container) getContainerNetworkInfo() (*define.InspectNetworkSettings, e return nil, errors.New("not implemented (*Container) getContainerNetworkInfo") } -func (c *Container) setupRootlessNetwork() error { - return errors.New("not implemented (*Container) setupRootlessNetwork") -} - func (r *Runtime) setupNetNS(ctr *Container) error { return errors.New("not implemented (*Runtime) setupNetNS") } diff --git a/libpod/oci_conmon_common.go b/libpod/oci_conmon_common.go index d803276b15..8bb2044209 100644 --- a/libpod/oci_conmon_common.go +++ b/libpod/oci_conmon_common.go @@ -1175,26 +1175,17 @@ func (r *ConmonOCIRuntime) createOCIContainer(ctr *Container, restoreOptions *Co } if ctr.config.NetMode.IsSlirp4netns() || rootless.IsRootless() { - if ctr.config.PostConfigureNetNS { - havePortMapping := len(ctr.config.PortMappings) > 0 - if havePortMapping { - ctr.rootlessPortSyncR, ctr.rootlessPortSyncW, err = os.Pipe() - if err != nil { - return 0, fmt.Errorf("failed to create rootless port sync pipe: %w", err) - } - } - ctr.rootlessSlirpSyncR, ctr.rootlessSlirpSyncW, err = os.Pipe() + havePortMapping := len(ctr.config.PortMappings) > 0 + if havePortMapping { + ctr.rootlessPortSyncR, ctr.rootlessPortSyncW, err = os.Pipe() if err != nil { - return 0, fmt.Errorf("failed to create rootless network sync pipe: %w", err) - } - } else { - if ctr.rootlessSlirpSyncR != nil { - defer errorhandling.CloseQuiet(ctr.rootlessSlirpSyncR) - } - if ctr.rootlessSlirpSyncW != nil { - defer errorhandling.CloseQuiet(ctr.rootlessSlirpSyncW) + return 0, fmt.Errorf("failed to create rootless port sync pipe: %w", err) } } + ctr.rootlessSlirpSyncR, ctr.rootlessSlirpSyncW, err = os.Pipe() + if err != nil { + return 0, fmt.Errorf("failed to create rootless network sync pipe: %w", err) + } // Leak one end in conmon, the other one will be leaked into slirp4netns cmd.ExtraFiles = append(cmd.ExtraFiles, ctr.rootlessSlirpSyncW) diff --git a/libpod/options.go b/libpod/options.go index a974caeebf..6220fe8ce3 100644 --- a/libpod/options.go +++ b/libpod/options.go @@ -1094,13 +1094,14 @@ func WithDependencyCtrs(ctrs []*Container) CtrCreateOption { // namespace with a minimal configuration. // An optional array of port mappings can be provided. // Conflicts with WithNetNSFrom(). -func WithNetNS(portMappings []nettypes.PortMapping, exposedPorts map[uint16][]string, postConfigureNetNS bool, netmode string, networks map[string]nettypes.PerNetworkOptions) CtrCreateOption { +func WithNetNS(portMappings []nettypes.PortMapping, exposedPorts map[uint16][]string, netmode string, networks map[string]nettypes.PerNetworkOptions) CtrCreateOption { return func(ctr *Container) error { if ctr.valid { return define.ErrCtrFinalized } - ctr.config.PostConfigureNetNS = postConfigureNetNS + // allow compatibility in case of downgrade by setting this to always true + ctr.config.PostConfigureNetNS = true ctr.config.NetMode = namespaces.NetworkMode(netmode) ctr.config.CreateNetNS = true ctr.config.PortMappings = portMappings diff --git a/pkg/specgen/generate/namespaces.go b/pkg/specgen/generate/namespaces.go index 4a3d451b94..6f4d4758e0 100644 --- a/pkg/specgen/generate/namespaces.go +++ b/pkg/specgen/generate/namespaces.go @@ -293,7 +293,6 @@ func namespaceOptions(s *specgen.SpecGenerator, rt *libpod.Runtime, pod *libpod. toReturn = append(toReturn, libpod.WithCgroupsMode(s.CgroupsMode)) } - postConfigureNetNS := !s.UserNS.IsHost() // when we are rootless we default to slirp4netns if rootless.IsRootless() && (s.NetNS.IsPrivate() || s.NetNS.IsDefault()) { s.NetNS.NSMode = specgen.Slirp @@ -326,14 +325,14 @@ func namespaceOptions(s *specgen.SpecGenerator, rt *libpod.Runtime, pod *libpod. if s.NetNS.Value != "" { val = fmt.Sprintf("slirp4netns:%s", s.NetNS.Value) } - toReturn = append(toReturn, libpod.WithNetNS(portMappings, expose, postConfigureNetNS, val, nil)) + toReturn = append(toReturn, libpod.WithNetNS(portMappings, expose, val, nil)) case specgen.Pasta: portMappings, expose, err := createPortMappings(s, imageData) if err != nil { return nil, err } val := "pasta" - toReturn = append(toReturn, libpod.WithNetNS(portMappings, expose, postConfigureNetNS, val, nil)) + toReturn = append(toReturn, libpod.WithNetNS(portMappings, expose, val, nil)) case specgen.Bridge, specgen.Private, specgen.Default: portMappings, expose, err := createPortMappings(s, imageData) if err != nil { @@ -366,7 +365,7 @@ func namespaceOptions(s *specgen.SpecGenerator, rt *libpod.Runtime, pod *libpod. s.Networks[rtConfig.Network.DefaultNetwork] = opts delete(s.Networks, "default") } - toReturn = append(toReturn, libpod.WithNetNS(portMappings, expose, postConfigureNetNS, "bridge", s.Networks)) + toReturn = append(toReturn, libpod.WithNetNS(portMappings, expose, "bridge", s.Networks)) } if s.UseImageHosts { diff --git a/test/system/500-networking.bats b/test/system/500-networking.bats index b6390f5f3d..69fd516392 100644 --- a/test/system/500-networking.bats +++ b/test/system/500-networking.bats @@ -582,7 +582,13 @@ load helpers.network run_podman network create $netname is "$output" "$netname" "output of 'network create'" - for network in "slirp4netns" "$netname"; do + # pasta only work as rootless + pasta= + if is_rootless; then + pasta=pasta + fi + + for network in "slirp4netns" "$netname" $pasta; do # Start container with the restart always policy run_podman run -d --name myweb -p "$HOST_PORT:80" \ --restart always \