Add validate() for containers

Until now, we've been validating every part of container
configuration through the With... functions that set the options.
This if fine when we are just validating the options to an
individual function, but things get complicated once we need to
validate conflicts between different options. We don't know the
order in which things were passed, so we need the validation on
both of the potential options that can conflict, resulting in
significant code duplication. To solve this, add a validate()
function for containers, and use this to check whether everything
is in a good state.

We can probably move more into this function (there are other
parts of container creation that also do validation of a sort)
but this is a good start to simplifying our options.

Signed-off-by: Matthew Heon <matthew.heon@pm.me>

3 files changed, 107 insertions, 107 deletions

diff --git a/libpod/container_validate.go b/libpod/container_validate.go
new file mode 100644
index 000000000..b7f0aadff
--- /dev/null
+++ b/libpod/container_validate.go
@@ -0,0 +1,99 @@
+package libpod
+
+import (
+	"github.com/containers/libpod/libpod/define"
+	"github.com/containers/libpod/pkg/rootless"
+	spec "github.com/opencontainers/runtime-spec/specs-go"
+	"github.com/pkg/errors"
+)
+
+// Validate that the configuration of a container is valid.
+func (c *Container) validate() error {
+	imageIDSet := c.config.RootfsImageID != ""
+	imageNameSet := c.config.RootfsImageName != ""
+	rootfsSet := c.config.Rootfs != ""
+
+	// If one of RootfsImageIDor RootfsImageName are set, both must be set.
+	if (imageIDSet || imageNameSet) && !(imageIDSet && imageNameSet) {
+		return errors.Wrapf(define.ErrInvalidArg, "both RootfsImageName and RootfsImageID must be set if either is set")
+	}
+
+	// Cannot set RootfsImageID and Rootfs at the same time
+	if imageIDSet && rootfsSet {
+		return errors.Wrapf(define.ErrInvalidArg, "cannot set both an image ID and rootfs for a container")
+	}
+
+	// Must set at least one of RootfsImageID or Rootfs
+	if !(imageIDSet || rootfsSet) {
+		return errors.Wrapf(define.ErrInvalidArg, "must set root filesystem source to either image or rootfs")
+	}
+
+	// Cannot make a network namespace if we are joining another container's
+	// network namespace
+	if c.config.CreateNetNS && c.config.NetNsCtr != "" {
+		return errors.Wrapf(define.ErrInvalidArg, "cannot both create a network namespace and join another container's network namespace")
+	}
+
+	// Not creating cgroups has a number of requirements, mostly related to
+	// the PID namespace.
+	if c.config.NoCgroups || c.config.CgroupsMode == "disabled" {
+		if c.config.PIDNsCtr != "" {
+			return errors.Wrapf(define.ErrInvalidArg, "cannot join another container's PID namespace if not creating cgroups")
+		}
+
+		if c.config.CgroupParent != "" {
+			return errors.Wrapf(define.ErrInvalidArg, "cannot set cgroup parent if not creating cgroups")
+		}
+
+		// Ensure we have a PID namespace
+		if c.config.Spec.Linux == nil {
+			return errors.Wrapf(define.ErrInvalidArg, "must provide Linux namespace configuration in OCI spec when using NoCgroups")
+		}
+		foundPid := false
+		for _, ns := range c.config.Spec.Linux.Namespaces {
+			if ns.Type == spec.PIDNamespace {
+				foundPid = true
+				if ns.Path != "" {
+					return errors.Wrapf(define.ErrInvalidArg, "containers not creating CGroups must create a private PID namespace - cannot use another")
+				}
+				break
+			}
+		}
+		if !foundPid {
+			return errors.Wrapf(define.ErrInvalidArg, "containers not creating CGroups must create a private PID namespace")
+		}
+	}
+
+	// Rootless has some requirements, compared to networks.
+	if rootless.IsRootless() {
+		if len(c.config.Networks) > 0 {
+			return errors.Wrapf(define.ErrInvalidArg, "cannot join CNI networks if running rootless")
+		}
+
+		// TODO: Should we make sure network mode is set to Slirp if set
+		// at all?
+	}
+
+	// Can only set static IP or MAC is creating a network namespace.
+	if !c.config.CreateNetNS && (c.config.StaticIP != nil || c.config.StaticMAC != nil) {
+		return errors.Wrapf(define.ErrInvalidArg, "cannot set static IP or MAC address if not creating a network namespace")
+	}
+
+	// Cannot set static IP or MAC if joining >1 CNI network.
+	if len(c.config.Networks) > 1 && (c.config.StaticIP != nil || c.config.StaticMAC != nil) {
+		return errors.Wrapf(define.ErrInvalidArg, "cannot set static IP or MAC address if joining more than one CNI network")
+	}
+
+	// Using image resolv.conf conflicts with various DNS settings.
+	if c.config.UseImageResolvConf &&
+		(len(c.config.DNSSearch) > 0 || len(c.config.DNSServer) > 0 ||
+			len(c.config.DNSOption) > 0) {
+		return errors.Wrapf(define.ErrInvalidArg, "cannot configure DNS options if using image's resolv.conf")
+	}
+
+	if c.config.UseImageHosts && len(c.config.HostAdd) > 0 {
+		return errors.Wrapf(define.ErrInvalidArg, "cannot add to /etc/hosts if using image's /etc/hosts")
+	}
+
+	return nil
+}
diff --git a/libpod/options.go b/libpod/options.go
index d01e8a85f..98de71af2 100644
--- a/libpod/options.go
+++ b/libpod/options.go
@@ -599,13 +599,6 @@ func WithRootFSFromImage(imageID string, imageName string) CtrCreateOption {
 			return define.ErrCtrFinalized
 		}
 
-		if ctr.config.RootfsImageID != "" || ctr.config.RootfsImageName != "" {
-			return errors.Wrapf(define.ErrInvalidArg, "container already configured with root filesystem")
-		}
-		if ctr.config.Rootfs != "" {
-			return errors.Wrapf(define.ErrInvalidArg, "cannot set both an image ID and a rootfs for a container")
-		}
-
 		ctr.config.RootfsImageID = imageID
 		ctr.config.RootfsImageName = imageName
 
@@ -815,10 +808,6 @@ func WithNetNSFrom(nsCtr *Container) CtrCreateOption {
 			return err
 		}
 
-		if ctr.config.CreateNetNS {
-			return errors.Wrapf(define.ErrInvalidArg, "cannot join another container's net ns as we are making a new net ns")
-		}
-
 		ctr.config.NetNsCtr = nsCtr.ID()
 
 		return nil
@@ -839,10 +828,6 @@ func WithPIDNSFrom(nsCtr *Container) CtrCreateOption {
 			return err
 		}
 
-		if ctr.config.NoCgroups {
-			return errors.Wrapf(define.ErrInvalidArg, "container has disabled creation of CGroups, which is incompatible with sharing a PID namespace")
-		}
-
 		ctr.config.PIDNsCtr = nsCtr.ID()
 
 		return nil
@@ -921,16 +906,8 @@ func WithDependencyCtrs(ctrs []*Container) CtrCreateOption {
 		deps := make([]string, 0, len(ctrs))
 
 		for _, dep := range ctrs {
-			if !dep.valid {
-				return errors.Wrapf(define.ErrCtrRemoved, "container %s is not valid", dep.ID())
-			}
-
-			if dep.ID() == ctr.ID() {
-				return errors.Wrapf(define.ErrInvalidArg, "must specify another container")
-			}
-
-			if ctr.config.Pod != "" && dep.config.Pod != ctr.config.Pod {
-				return errors.Wrapf(define.ErrInvalidArg, "container has joined pod %s and dependency container %s is not a member of the pod", ctr.config.Pod, dep.ID())
+			if err := checkDependencyContainer(dep, ctr); err != nil {
+				return err
 			}
 
 			deps = append(deps, dep.ID())
@@ -952,20 +929,6 @@ func WithNetNS(portMappings []ocicni.PortMapping, postConfigureNetNS bool, netmo
 			return define.ErrCtrFinalized
 		}
 
-		if rootless.IsRootless() {
-			if len(networks) > 0 {
-				return errors.Wrapf(define.ErrInvalidArg, "cannot use CNI networks with rootless containers")
-			}
-		}
-
-		if len(networks) > 1 && (ctr.config.StaticIP != nil || ctr.config.StaticMAC != nil) {
-			return errors.Wrapf(define.ErrInvalidArg, "cannot join more than one CNI network if configuring a static IP or MAC address")
-		}
-
-		if ctr.config.NetNsCtr != "" {
-			return errors.Wrapf(define.ErrInvalidArg, "container is already set to join another container's net ns, cannot create a new net ns")
-		}
-
 		ctr.config.PostConfigureNetNS = postConfigureNetNS
 		ctr.config.NetMode = namespaces.NetworkMode(netmode)
 		ctr.config.CreateNetNS = true
@@ -988,14 +951,6 @@ func WithStaticIP(ip net.IP) CtrCreateOption {
 			return define.ErrCtrFinalized
 		}
 
-		if !ctr.config.CreateNetNS {
-			return errors.Wrapf(define.ErrInvalidArg, "cannot set a static IP if the container is not creating a network namespace")
-		}
-
-		if len(ctr.config.Networks) > 1 {
-			return errors.Wrapf(define.ErrInvalidArg, "cannot set a static IP if joining more than 1 CNI network")
-		}
-
 		ctr.config.StaticIP = ip
 
 		return nil
@@ -1013,14 +968,6 @@ func WithStaticMAC(mac net.HardwareAddr) CtrCreateOption {
 			return define.ErrCtrFinalized
 		}
 
-		if !ctr.config.CreateNetNS {
-			return errors.Wrapf(define.ErrInvalidArg, "cannot set a static MAC if the container is not creating a network namespace")
-		}
-
-		if len(ctr.config.Networks) > 1 {
-			return errors.Wrapf(define.ErrInvalidArg, "cannot set a static MAC if joining more than 1 CNI network")
-		}
-
 		ctr.config.StaticMAC = mac
 
 		return nil
@@ -1114,10 +1061,6 @@ func WithCgroupParent(parent string) CtrCreateOption {
 			return errors.Wrapf(define.ErrInvalidArg, "cgroup parent cannot be empty")
 		}
 
-		if ctr.config.NoCgroups {
-			return errors.Wrapf(define.ErrInvalidArg, "CgroupParent conflicts with NoCgroups")
-		}
-
 		ctr.config.CgroupParent = parent
 
 		return nil
@@ -1130,9 +1073,6 @@ func WithDNSSearch(searchDomains []string) CtrCreateOption {
 		if ctr.valid {
 			return define.ErrCtrFinalized
 		}
-		if ctr.config.UseImageResolvConf {
-			return errors.Wrapf(define.ErrInvalidArg, "cannot add DNS search domains if container will not create /etc/resolv.conf")
-		}
 		ctr.config.DNSSearch = searchDomains
 		return nil
 	}
@@ -1144,9 +1084,6 @@ func WithDNS(dnsServers []string) CtrCreateOption {
 		if ctr.valid {
 			return define.ErrCtrFinalized
 		}
-		if ctr.config.UseImageResolvConf {
-			return errors.Wrapf(define.ErrInvalidArg, "cannot add DNS servers if container will not create /etc/resolv.conf")
-		}
 		var dns []net.IP
 		for _, i := range dnsServers {
 			result := net.ParseIP(i)
@@ -1166,9 +1103,6 @@ func WithDNSOption(dnsOptions []string) CtrCreateOption {
 		if ctr.valid {
 			return define.ErrCtrFinalized
 		}
-		if ctr.config.UseImageResolvConf {
-			return errors.Wrapf(define.ErrInvalidArg, "cannot add DNS options if container will not create /etc/resolv.conf")
-		}
 		ctr.config.DNSOption = dnsOptions
 		return nil
 	}
@@ -1181,10 +1115,6 @@ func WithHosts(hosts []string) CtrCreateOption {
 			return define.ErrCtrFinalized
 		}
 
-		if ctr.config.UseImageHosts {
-			return errors.Wrapf(define.ErrInvalidArg, "cannot add hosts if container will not create /etc/hosts")
-		}
-
 		ctr.config.HostAdd = hosts
 		return nil
 	}
@@ -1282,9 +1212,6 @@ func WithRootFS(rootfs string) CtrCreateOption {
 		if _, err := os.Stat(rootfs); err != nil {
 			return errors.Wrapf(err, "error checking path %q", rootfs)
 		}
-		if ctr.config.RootfsImageID != "" {
-			return errors.Wrapf(define.ErrInvalidArg, "cannot set both an image ID and a rootfs for a container")
-		}
 		ctr.config.Rootfs = rootfs
 		return nil
 	}
@@ -1314,12 +1241,6 @@ func WithUseImageResolvConf() CtrCreateOption {
 			return define.ErrCtrFinalized
 		}
 
-		if len(ctr.config.DNSServer) != 0 ||
-			len(ctr.config.DNSSearch) != 0 ||
-			len(ctr.config.DNSOption) != 0 {
-			return errors.Wrapf(define.ErrInvalidArg, "not creating resolv.conf conflicts with DNS options")
-		}
-
 		ctr.config.UseImageResolvConf = true
 
 		return nil
@@ -1334,10 +1255,6 @@ func WithUseImageHosts() CtrCreateOption {
 			return define.ErrCtrFinalized
 		}
 
-		if len(ctr.config.HostAdd) != 0 {
-			return errors.Wrapf(define.ErrInvalidArg, "not creating /etc/hosts conflicts with adding to the hosts file")
-		}
-
 		ctr.config.UseImageHosts = true
 
 		return nil
diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go
index 39284026c..de93fdce7 100644
--- a/libpod/runtime_ctr.go
+++ b/libpod/runtime_ctr.go
@@ -133,7 +133,12 @@ func (r *Runtime) newContainer(ctx context.Context, rSpec *spec.Spec, options ..
 	return r.setupContainer(ctx, ctr)
 }
 
-func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (c *Container, err error) {
+func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (_ *Container, err error) {
+	// Validate the container
+	if err := ctr.validate(); err != nil {
+		return nil, err
+	}
+
 	// Allocate a lock for the container
 	lock, err := r.lockManager.AllocateLock()
 	if err != nil {
@@ -190,27 +195,6 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (c *Contai
 		ctr.config.Name = name
 	}
 
-	// If CGroups are disabled, we MUST create a PID namespace.
-	// Otherwise, the OCI runtime won't be able to stop our container.
-	if ctr.config.NoCgroups {
-		if ctr.config.Spec.Linux == nil {
-			return nil, errors.Wrapf(define.ErrInvalidArg, "must provide Linux namespace configuration in OCI spec when using NoCgroups")
-		}
-		foundPid := false
-		for _, ns := range ctr.config.Spec.Linux.Namespaces {
-			if ns.Type == spec.PIDNamespace {
-				foundPid = true
-				if ns.Path != "" {
-					return nil, errors.Wrapf(define.ErrInvalidArg, "containers not creating CGroups must create a private PID namespace - cannot use another")
-				}
-				break
-			}
-		}
-		if !foundPid {
-			return nil, errors.Wrapf(define.ErrInvalidArg, "containers not creating CGroups must create a private PID namespace")
-		}
-	}
-
 	// Check CGroup parent sanity, and set it if it was not set.
 	// Only if we're actually configuring CGroups.
 	if !ctr.config.NoCgroups {