Merge pull request #9163 from mheon/backports_rc2

Backports for v3.0 RC2

8 files changed, 315 insertions, 40 deletions

diff --git a/libpod/container_api.go b/libpod/container_api.go
index 0d62a2dd7..951227a4f 100644
--- a/libpod/container_api.go
+++ b/libpod/container_api.go
@@ -776,3 +776,32 @@ func (c *Container) ShouldRestart(ctx context.Context) bool {
 	}
 	return c.shouldRestart()
 }
+
+// ResolvePath resolves the specified path on the root for the container.  The
+// root must either be the mounted image of the container or the already
+// mounted container storage.
+//
+// It returns the resolved root and the resolved path.  Note that the path may
+// resolve to the container's mount point or to a volume or bind mount.
+func (c *Container) ResolvePath(ctx context.Context, root string, path string) (string, string, error) {
+	logrus.Debugf("Resolving path %q (root %q) on container %s", path, root, c.ID())
+
+	// Minimal sanity checks.
+	if len(root)*len(path) == 0 {
+		return "", "", errors.Wrapf(define.ErrInternal, "ResolvePath: root (%q) and path (%q) must be non empty", root, path)
+	}
+	if _, err := os.Stat(root); err != nil {
+		return "", "", errors.Wrapf(err, "cannot locate root to resolve path on container %s", c.ID())
+	}
+
+	if !c.batched {
+		c.lock.Lock()
+		defer c.lock.Unlock()
+
+		if err := c.syncContainer(); err != nil {
+			return "", "", err
+		}
+	}
+
+	return c.resolvePath(root, path)
+}
diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go
index 0553cc59c..9c5989d23 100644
--- a/libpod/container_internal_linux.go
+++ b/libpod/container_internal_linux.go
@@ -174,25 +174,60 @@ func (c *Container) prepare() error {
 		return err
 	}
 
-	// Ensure container entrypoint is created (if required)
-	if c.config.CreateWorkingDir {
-		workdir, err := securejoin.SecureJoin(c.state.Mountpoint, c.WorkingDir())
-		if err != nil {
-			return errors.Wrapf(err, "error creating path to container %s working dir", c.ID())
-		}
-		rootUID := c.RootUID()
-		rootGID := c.RootGID()
+	// Make sure the workdir exists
+	if err := c.resolveWorkDir(); err != nil {
+		return err
+	}
 
-		if err := os.MkdirAll(workdir, 0755); err != nil {
-			if os.IsExist(err) {
-				return nil
+	return nil
+}
+
+// resolveWorkDir resolves the container's workdir and, depending on the
+// configuration, will create it, or error out if it does not exist.
+// Note that the container must be mounted before.
+func (c *Container) resolveWorkDir() error {
+	workdir := c.WorkingDir()
+
+	// If the specified workdir is a subdir of a volume or mount,
+	// we don't need to do anything.  The runtime is taking care of
+	// that.
+	if isPathOnVolume(c, workdir) || isPathOnBindMount(c, workdir) {
+		logrus.Debugf("Workdir %q resolved to a volume or mount", workdir)
+		return nil
+	}
+
+	_, resolvedWorkdir, err := c.resolvePath(c.state.Mountpoint, workdir)
+	if err != nil {
+		return err
+	}
+	logrus.Debugf("Workdir %q resolved to host path %q", workdir, resolvedWorkdir)
+
+	// No need to create it (e.g., `--workdir=/foo`), so let's make sure
+	// the path exists on the container.
+	if !c.config.CreateWorkingDir {
+		if _, err := os.Stat(resolvedWorkdir); err != nil {
+			if os.IsNotExist(err) {
+				return errors.Errorf("workdir %q does not exist on container %s", workdir, c.ID())
 			}
-			return errors.Wrapf(err, "error creating container %s working dir", c.ID())
+			// This might be a serious error (e.g., permission), so
+			// we need to return the full error.
+			return errors.Wrapf(err, "error detecting workdir %q on container %s", workdir, c.ID())
 		}
+	}
+
+	// Ensure container entrypoint is created (if required).
+	rootUID := c.RootUID()
+	rootGID := c.RootGID()
 
-		if err := os.Chown(workdir, rootUID, rootGID); err != nil {
-			return errors.Wrapf(err, "error chowning container %s working directory to container root", c.ID())
+	if err := os.MkdirAll(resolvedWorkdir, 0755); err != nil {
+		if os.IsExist(err) {
+			return nil
 		}
+		return errors.Wrapf(err, "error creating container %s workdir", c.ID())
+	}
+
+	if err := os.Chown(resolvedWorkdir, rootUID, rootGID); err != nil {
+		return errors.Wrapf(err, "error chowning container %s workdir to container root", c.ID())
 	}
 
 	return nil
diff --git a/libpod/container_path_resolution.go b/libpod/container_path_resolution.go
new file mode 100644
index 000000000..805b3b947
--- /dev/null
+++ b/libpod/container_path_resolution.go
@@ -0,0 +1,166 @@
+package libpod
+
+import (
+	"path/filepath"
+	"strings"
+
+	securejoin "github.com/cyphar/filepath-securejoin"
+	"github.com/opencontainers/runtime-spec/specs-go"
+	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
+)
+
+// resolveContainerPaths resolves the container's mount point and the container
+// path as specified by the user.  Both may resolve to paths outside of the
+// container's mount point when the container path hits a volume or bind mount.
+//
+// It returns a bool, indicating whether containerPath resolves outside of
+// mountPoint (e.g., via a mount or volume), the resolved root (e.g., container
+// mount, bind mount or volume) and the resolved path on the root (absolute to
+// the host).
+func (container *Container) resolvePath(mountPoint string, containerPath string) (string, string, error) {
+	// Let's first make sure we have a path relative to the mount point.
+	pathRelativeToContainerMountPoint := containerPath
+	if !filepath.IsAbs(containerPath) {
+		// If the containerPath is not absolute, it's relative to the
+		// container's working dir.  To be extra careful, let's first
+		// join the working dir with "/", and the add the containerPath
+		// to it.
+		pathRelativeToContainerMountPoint = filepath.Join(filepath.Join("/", container.WorkingDir()), containerPath)
+	}
+	resolvedPathOnTheContainerMountPoint := filepath.Join(mountPoint, pathRelativeToContainerMountPoint)
+	pathRelativeToContainerMountPoint = strings.TrimPrefix(pathRelativeToContainerMountPoint, mountPoint)
+	pathRelativeToContainerMountPoint = filepath.Join("/", pathRelativeToContainerMountPoint)
+
+	// Now we have an "absolute container Path" but not yet resolved on the
+	// host (e.g., "/foo/bar/file.txt").  As mentioned above, we need to
+	// check if "/foo/bar/file.txt" is on a volume or bind mount.  To do
+	// that, we need to walk *down* the paths to the root.  Assuming
+	// volume-1 is mounted to "/foo" and volume-2 is mounted to "/foo/bar",
+	// we must select "/foo/bar".  Once selected, we need to rebase the
+	// remainder (i.e, "/file.txt") on the volume's mount point on the
+	// host.  Same applies to bind mounts.
+
+	searchPath := pathRelativeToContainerMountPoint
+	for {
+		volume, err := findVolume(container, searchPath)
+		if err != nil {
+			return "", "", err
+		}
+		if volume != nil {
+			logrus.Debugf("Container path %q resolved to volume %q on path %q", containerPath, volume.Name(), searchPath)
+
+			// TODO: We really need to force the volume to mount
+			// before doing this, but that API is not exposed
+			// externally right now and doing so is beyond the scope
+			// of this commit.
+			mountPoint, err := volume.MountPoint()
+			if err != nil {
+				return "", "", err
+			}
+			if mountPoint == "" {
+				return "", "", errors.Errorf("volume %s is not mounted, cannot copy into it", volume.Name())
+			}
+
+			// We found a matching volume for searchPath.  We now
+			// need to first find the relative path of our input
+			// path to the searchPath, and then join it with the
+			// volume's mount point.
+			pathRelativeToVolume := strings.TrimPrefix(pathRelativeToContainerMountPoint, searchPath)
+			absolutePathOnTheVolumeMount, err := securejoin.SecureJoin(mountPoint, pathRelativeToVolume)
+			if err != nil {
+				return "", "", err
+			}
+			return mountPoint, absolutePathOnTheVolumeMount, nil
+		}
+
+		if mount := findBindMount(container, searchPath); mount != nil {
+			logrus.Debugf("Container path %q resolved to bind mount %q:%q on path %q", containerPath, mount.Source, mount.Destination, searchPath)
+			// We found a matching bind mount for searchPath.  We
+			// now need to first find the relative path of our
+			// input path to the searchPath, and then join it with
+			// the source of the bind mount.
+			pathRelativeToBindMount := strings.TrimPrefix(pathRelativeToContainerMountPoint, searchPath)
+			absolutePathOnTheBindMount, err := securejoin.SecureJoin(mount.Source, pathRelativeToBindMount)
+			if err != nil {
+				return "", "", err
+			}
+			return mount.Source, absolutePathOnTheBindMount, nil
+
+		}
+
+		if searchPath == "/" {
+			// Cannot go beyond "/", so we're done.
+			break
+		}
+
+		// Walk *down* the path (e.g., "/foo/bar/x" -> "/foo/bar").
+		searchPath = filepath.Dir(searchPath)
+	}
+
+	// No volume, no bind mount but just a normal path on the container.
+	return mountPoint, resolvedPathOnTheContainerMountPoint, nil
+}
+
+// findVolume checks if the specified containerPath matches the destination
+// path of a Volume.  Returns a matching Volume or nil.
+func findVolume(c *Container, containerPath string) (*Volume, error) {
+	runtime := c.Runtime()
+	cleanedContainerPath := filepath.Clean(containerPath)
+	for _, vol := range c.Config().NamedVolumes {
+		if cleanedContainerPath == filepath.Clean(vol.Dest) {
+			return runtime.GetVolume(vol.Name)
+		}
+	}
+	return nil, nil
+}
+
+// isPathOnVolume returns true if the specified containerPath is a subdir of any
+// Volume's destination.
+func isPathOnVolume(c *Container, containerPath string) bool {
+	cleanedContainerPath := filepath.Clean(containerPath)
+	for _, vol := range c.Config().NamedVolumes {
+		if cleanedContainerPath == filepath.Clean(vol.Dest) {
+			return true
+		}
+		for dest := vol.Dest; dest != "/"; dest = filepath.Dir(dest) {
+			if cleanedContainerPath == dest {
+				return true
+			}
+		}
+	}
+	return false
+}
+
+// findBindMounts checks if the specified containerPath matches the destination
+// path of a Mount.  Returns a matching Mount or nil.
+func findBindMount(c *Container, containerPath string) *specs.Mount {
+	cleanedPath := filepath.Clean(containerPath)
+	for _, m := range c.Config().Spec.Mounts {
+		if m.Type != "bind" {
+			continue
+		}
+		if cleanedPath == filepath.Clean(m.Destination) {
+			mount := m
+			return &mount
+		}
+	}
+	return nil
+}
+
+/// isPathOnBindMount returns true if the specified containerPath is a subdir of any
+// Mount's destination.
+func isPathOnBindMount(c *Container, containerPath string) bool {
+	cleanedContainerPath := filepath.Clean(containerPath)
+	for _, m := range c.Config().Spec.Mounts {
+		if cleanedContainerPath == filepath.Clean(m.Destination) {
+			return true
+		}
+		for dest := m.Destination; dest != "/"; dest = filepath.Dir(dest) {
+			if cleanedContainerPath == dest {
+				return true
+			}
+		}
+	}
+	return false
+}
diff --git a/libpod/networking_linux.go b/libpod/networking_linux.go
index addf1814c..94b8563fe 100644
--- a/libpod/networking_linux.go
+++ b/libpod/networking_linux.go
@@ -955,7 +955,7 @@ func (c *Container) getContainerNetworkInfo() (*define.InspectNetworkSettings, e
 	if c.state.NetNS == nil {
 		// We still want to make dummy configurations for each CNI net
 		// the container joined.
-		if len(networks) > 0 && !isDefault {
+		if len(networks) > 0 {
 			settings.Networks = make(map[string]*define.InspectAdditionalNetwork, len(networks))
 			for _, net := range networks {
 				cniNet := new(define.InspectAdditionalNetwork)
@@ -976,7 +976,7 @@ func (c *Container) getContainerNetworkInfo() (*define.InspectNetworkSettings, e
 	}
 
 	// If we have CNI networks - handle that here
-	if len(networks) > 0 && !isDefault {
+	if len(networks) > 0 {
 		if len(networks) != len(c.state.NetworkStatus) {
 			return nil, errors.Wrapf(define.ErrInternal, "network inspection mismatch: asked to join %d CNI network(s) %v, but have information on %d network(s)", len(networks), networks, len(c.state.NetworkStatus))
 		}
@@ -1006,7 +1006,9 @@ func (c *Container) getContainerNetworkInfo() (*define.InspectNetworkSettings, e
 			settings.Networks[name] = addedNet
 		}
 
-		return settings, nil
+		if !isDefault {
+			return settings, nil
+		}
 	}
 
 	// If not joining networks, we should have at most 1 result
diff --git a/libpod/oci_conmon_exec_linux.go b/libpod/oci_conmon_exec_linux.go
index dc5dd03df..faf86ea5b 100644
--- a/libpod/oci_conmon_exec_linux.go
+++ b/libpod/oci_conmon_exec_linux.go
@@ -126,20 +126,25 @@ func (r *ConmonOCIRuntime) ExecContainerHTTP(ctr *Container, sessionID string, o
 	}()
 
 	attachChan := make(chan error)
+	conmonPipeDataChan := make(chan conmonPipeData)
 	go func() {
 		// attachToExec is responsible for closing pipes
-		attachChan <- attachExecHTTP(ctr, sessionID, req, w, streams, pipes, detachKeys, options.Terminal, cancel, hijackDone, holdConnOpen)
+		attachChan <- attachExecHTTP(ctr, sessionID, req, w, streams, pipes, detachKeys, options.Terminal, cancel, hijackDone, holdConnOpen, execCmd, conmonPipeDataChan, ociLog)
 		close(attachChan)
 	}()
 
-	// Wait for conmon to succeed, when return.
-	if err := execCmd.Wait(); err != nil {
-		return -1, nil, errors.Wrapf(err, "cannot run conmon")
-	}
+	// NOTE: the channel is needed to communicate conmon's data.  In case
+	// of an error, the error will be written on the hijacked http
+	// connection such that remote clients will receive the error.
+	pipeData := <-conmonPipeDataChan
 
-	pid, err := readConmonPipeData(pipes.syncPipe, ociLog)
+	return pipeData.pid, attachChan, pipeData.err
+}
 
-	return pid, attachChan, err
+// conmonPipeData contains the data when reading from conmon's pipe.
+type conmonPipeData struct {
+	pid int
+	err error
 }
 
 // ExecContainerDetached executes a command in a running container, but does
@@ -488,9 +493,16 @@ func (r *ConmonOCIRuntime) startExec(c *Container, sessionID string, options *Ex
 }
 
 // Attach to a container over HTTP
-func attachExecHTTP(c *Container, sessionID string, r *http.Request, w http.ResponseWriter, streams *HTTPAttachStreams, pipes *execPipes, detachKeys []byte, isTerminal bool, cancel <-chan bool, hijackDone chan<- bool, holdConnOpen <-chan bool) (deferredErr error) {
+func attachExecHTTP(c *Container, sessionID string, r *http.Request, w http.ResponseWriter, streams *HTTPAttachStreams, pipes *execPipes, detachKeys []byte, isTerminal bool, cancel <-chan bool, hijackDone chan<- bool, holdConnOpen <-chan bool, execCmd *exec.Cmd, conmonPipeDataChan chan<- conmonPipeData, ociLog string) (deferredErr error) {
+	// NOTE: As you may notice, the attach code is quite complex.
+	// Many things happen concurrently and yet are interdependent.
+	// If you ever change this function, make sure to write to the
+	// conmonPipeDataChan in case of an error.
+
 	if pipes == nil || pipes.startPipe == nil || pipes.attachPipe == nil {
-		return errors.Wrapf(define.ErrInvalidArg, "must provide a start and attach pipe to finish an exec attach")
+		err := errors.Wrapf(define.ErrInvalidArg, "must provide a start and attach pipe to finish an exec attach")
+		conmonPipeDataChan <- conmonPipeData{-1, err}
+		return err
 	}
 
 	defer func() {
@@ -509,17 +521,20 @@ func attachExecHTTP(c *Container, sessionID string, r *http.Request, w http.Resp
 	// set up the socket path, such that it is the correct length and location for exec
 	sockPath, err := c.execAttachSocketPath(sessionID)
 	if err != nil {
+		conmonPipeDataChan <- conmonPipeData{-1, err}
 		return err
 	}
 
 	// 2: read from attachFd that the parent process has set up the console socket
 	if _, err := readConmonPipeData(pipes.attachPipe, ""); err != nil {
+		conmonPipeDataChan <- conmonPipeData{-1, err}
 		return err
 	}
 
 	// 2: then attach
 	conn, err := openUnixSocket(sockPath)
 	if err != nil {
+		conmonPipeDataChan <- conmonPipeData{-1, err}
 		return errors.Wrapf(err, "failed to connect to container's attach socket: %v", sockPath)
 	}
 	defer func() {
@@ -540,11 +555,13 @@ func attachExecHTTP(c *Container, sessionID string, r *http.Request, w http.Resp
 	// Perform hijack
 	hijacker, ok := w.(http.Hijacker)
 	if !ok {
+		conmonPipeDataChan <- conmonPipeData{-1, err}
 		return errors.Errorf("unable to hijack connection")
 	}
 
 	httpCon, httpBuf, err := hijacker.Hijack()
 	if err != nil {
+		conmonPipeDataChan <- conmonPipeData{-1, err}
 		return errors.Wrapf(err, "error hijacking connection")
 	}
 
@@ -555,10 +572,23 @@ func attachExecHTTP(c *Container, sessionID string, r *http.Request, w http.Resp
 
 	// Force a flush after the header is written.
 	if err := httpBuf.Flush(); err != nil {
+		conmonPipeDataChan <- conmonPipeData{-1, err}
 		return errors.Wrapf(err, "error flushing HTTP hijack header")
 	}
 
 	go func() {
+		// Wait for conmon to succeed, when return.
+		if err := execCmd.Wait(); err != nil {
+			conmonPipeDataChan <- conmonPipeData{-1, err}
+		} else {
+			pid, err := readConmonPipeData(pipes.syncPipe, ociLog)
+			if err != nil {
+				hijackWriteError(err, c.ID(), isTerminal, httpBuf)
+				conmonPipeDataChan <- conmonPipeData{pid, err}
+			} else {
+				conmonPipeDataChan <- conmonPipeData{pid, err}
+			}
+		}
 		// We need to hold the connection open until the complete exec
 		// function has finished. This channel will be closed in a defer
 		// in that function, so we can wait for it here.
diff --git a/libpod/runtime.go b/libpod/runtime.go
index 34c737a67..0dc220b52 100644
--- a/libpod/runtime.go
+++ b/libpod/runtime.go
@@ -180,6 +180,13 @@ func newRuntimeFromConfig(ctx context.Context, conf *config.Config, options ...R
 		}
 	}
 
+	if err := shutdown.Register("libpod", func(sig os.Signal) error {
+		os.Exit(1)
+		return nil
+	}); err != nil && errors.Cause(err) != shutdown.ErrHandlerExists {
+		logrus.Errorf("Error registering shutdown handler for libpod: %v", err)
+	}
+
 	if err := shutdown.Start(); err != nil {
 		return nil, errors.Wrapf(err, "error starting shutdown signal handler")
 	}
@@ -188,13 +195,6 @@ func newRuntimeFromConfig(ctx context.Context, conf *config.Config, options ...R
 		return nil, err
 	}
 
-	if err := shutdown.Register("libpod", func(sig os.Signal) error {
-		os.Exit(1)
-		return nil
-	}); err != nil && errors.Cause(err) != shutdown.ErrHandlerExists {
-		logrus.Errorf("Error registering shutdown handler for libpod: %v", err)
-	}
-
 	return runtime, nil
 }
 
diff --git a/libpod/shutdown/handler.go b/libpod/shutdown/handler.go
index f0f228b19..ac1d33910 100644
--- a/libpod/shutdown/handler.go
+++ b/libpod/shutdown/handler.go
@@ -18,6 +18,8 @@ var (
 	stopped    bool
 	sigChan    chan os.Signal
 	cancelChan chan bool
+	// Syncronize accesses to the map
+	handlerLock sync.Mutex
 	// Definitions of all on-shutdown handlers
 	handlers map[string]func(os.Signal) error
 	// Ordering that on-shutdown handlers will be invoked.
@@ -50,6 +52,7 @@ func Start() error {
 		case sig := <-sigChan:
 			logrus.Infof("Received shutdown signal %v, terminating!", sig)
 			shutdownInhibit.Lock()
+			handlerLock.Lock()
 			for _, name := range handlerOrder {
 				handler, ok := handlers[name]
 				if !ok {
@@ -61,6 +64,7 @@ func Start() error {
 					logrus.Errorf("Error running shutdown handler %s: %v", name, err)
 				}
 			}
+			handlerLock.Unlock()
 			shutdownInhibit.Unlock()
 			return
 		}
@@ -97,6 +101,9 @@ func Uninhibit() {
 // by a signal. Handlers are invoked LIFO - the last handler registered is the
 // first run.
 func Register(name string, handler func(os.Signal) error) error {
+	handlerLock.Lock()
+	defer handlerLock.Unlock()
+
 	if handlers == nil {
 		handlers = make(map[string]func(os.Signal) error)
 	}
@@ -113,6 +120,9 @@ func Register(name string, handler func(os.Signal) error) error {
 
 // Unregister un-registers a given shutdown handler.
 func Unregister(name string) error {
+	handlerLock.Lock()
+	defer handlerLock.Unlock()
+
 	if handlers == nil {
 		return nil
 	}
diff --git a/libpod/util.go b/libpod/util.go
index bf9bf2542..391208fb9 100644
--- a/libpod/util.go
+++ b/libpod/util.go
@@ -235,20 +235,16 @@ func checkDependencyContainer(depCtr, ctr *Container) error {
 	return nil
 }
 
-// hijackWriteErrorAndClose writes an error to a hijacked HTTP session and
-// closes it. Intended to HTTPAttach function.
-// If error is nil, it will not be written; we'll only close the connection.
-func hijackWriteErrorAndClose(toWrite error, cid string, terminal bool, httpCon io.Closer, httpBuf *bufio.ReadWriter) {
+// hijackWriteError writes an error to a hijacked HTTP session.
+func hijackWriteError(toWrite error, cid string, terminal bool, httpBuf *bufio.ReadWriter) {
 	if toWrite != nil {
-		errString := []byte(fmt.Sprintf("%v\n", toWrite))
+		errString := []byte(fmt.Sprintf("Error: %v\n", toWrite))
 		if !terminal {
 			// We need a header.
 			header := makeHTTPAttachHeader(2, uint32(len(errString)))
 			if _, err := httpBuf.Write(header); err != nil {
 				logrus.Errorf("Error writing header for container %s attach connection error: %v", cid, err)
 			}
-			// TODO: May want to return immediately here to avoid
-			// writing garbage to the socket?
 		}
 		if _, err := httpBuf.Write(errString); err != nil {
 			logrus.Errorf("Error writing error to container %s HTTP attach connection: %v", cid, err)
@@ -257,6 +253,13 @@ func hijackWriteErrorAndClose(toWrite error, cid string, terminal bool, httpCon
 			logrus.Errorf("Error flushing HTTP buffer for container %s HTTP attach connection: %v", cid, err)
 		}
 	}
+}
+
+// hijackWriteErrorAndClose writes an error to a hijacked HTTP session and
+// closes it. Intended to HTTPAttach function.
+// If error is nil, it will not be written; we'll only close the connection.
+func hijackWriteErrorAndClose(toWrite error, cid string, terminal bool, httpCon io.Closer, httpBuf *bufio.ReadWriter) {
+	hijackWriteError(toWrite, cid, terminal, httpBuf)
 
 	if err := httpCon.Close(); err != nil {
 		logrus.Errorf("Error closing container %s HTTP attach connection: %v", cid, err)