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

libpod: always use PostConfigureNetNS option #18468

Closed
wants to merge 1 commit into from
Closed

libpod: always use PostConfigureNetNS option #18468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions libpod/container_config.go
View file
Open in desktop
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"`
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth leaving this in the struct and setting it unconditionally to true, to preserve compatibility in a downgrade case? Ensuring the bool is still in the DB and set to true guarantees containers created in 4.6 still behave the same if downgraded to 4.5

Copy link
Member Author

Choose a reason for hiding this comment

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

How much do we care about that? I get the point and will add it back. The reason I am asking because I pay zero attention on this when reviewing PRs.

Copy link
Member

Choose a reason for hiding this comment

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

We have not cared about downgrade in the past.

// 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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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