Skip to content

Commit

Permalink
libpod: always use PostConfigureNetNS option
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@Luap99
Luap99 committed May 8, 2023
1 parent ace3cba commit 8031869
Show file tree
Hide file tree
Showing 16 changed files with 194 additions and 413 deletions.
2 changes: 2 additions & 0 deletions libpod/container_config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand Down
5 changes: 1 addition & 4 deletions libpod/container_freebsd.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
55 changes: 15 additions & 40 deletions libpod/container_internal.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
80 changes: 50 additions & 30 deletions libpod/container_internal_common.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
}
Expand All @@ -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)
}
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
94 changes: 9 additions & 85 deletions libpod/container_internal_freebsd.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand Down
Loading

0 comments on commit 8031869

Please sign in to comment.