remote: fix name and ID collisions of containers and pods

Fix the look up of containers and pods in the remote client.  User input
can refer to both, names or IDs of containers and pods, so there is a
fair chance of collisions (e.g., "c1" name with a "c1...." ID).

Those collisions are well handled (and battle tested) in the local
client which is directly using the libpod backend.  Hence, the remote
client should not attempt to introduce its own logic to prevent bugs and
divergence between the local and the remote clients.  To prevent
collisions such as in #7837, do a container/pod inspect on the
user-provided input to find the corresponding ID and eventually do full
ID comparisons to avoid potential collisions with names.

Note that this has a cost that I am not entirely happy with.  Looking at
issue #7837, the collisions are happening when removing the two
containers.  Remote container removal is now very chatty with the server
as it first queries for all containers, then iterates over the provided
names or IDs and does a remote inspect to figure out the IDs and find a
matching container object.  However, remote removal could just pass the
names and IDs directly to the batch removal endpoint.  Querying for all
containers could be prevented if the batch removal endpoint would remove
all if the slice is empty.

In other words, the bug is fixed but there's room for performance
improvements.

Fixes: #7837
Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

1 files changed, 68 insertions, 24 deletions

diff --git a/pkg/domain/infra/tunnel/helpers.go b/pkg/domain/infra/tunnel/helpers.go
index 5944f855a..7d28f1bd3 100644
--- a/pkg/domain/infra/tunnel/helpers.go
+++ b/pkg/domain/infra/tunnel/helpers.go
@@ -2,74 +2,118 @@ package tunnel
 
 import (
 	"context"
-	"strings"
 
 	"github.com/containers/podman/v2/libpod/define"
 	"github.com/containers/podman/v2/pkg/bindings"
 	"github.com/containers/podman/v2/pkg/bindings/containers"
 	"github.com/containers/podman/v2/pkg/bindings/pods"
 	"github.com/containers/podman/v2/pkg/domain/entities"
-	"github.com/containers/podman/v2/pkg/util"
+	"github.com/containers/podman/v2/pkg/errorhandling"
 	"github.com/pkg/errors"
 )
 
+// FIXME: the `ignore` parameter is very likely wrong here as it should rather
+//        be used on *errors* from operations such as remove.
 func getContainersByContext(contextWithConnection context.Context, all, ignore bool, namesOrIDs []string) ([]entities.ListContainer, error) {
-	var (
-		cons []entities.ListContainer
-	)
 	if all && len(namesOrIDs) > 0 {
 		return nil, errors.New("cannot lookup containers and all")
 	}
-	c, err := containers.List(contextWithConnection, nil, bindings.PTrue, nil, nil, bindings.PTrue)
+
+	allContainers, err := containers.List(contextWithConnection, nil, bindings.PTrue, nil, nil, bindings.PTrue)
 	if err != nil {
 		return nil, err
 	}
 	if all {
-		return c, err
+		return allContainers, err
 	}
-	for _, id := range namesOrIDs {
-		var found bool
-		for _, con := range c {
-			if id == con.ID || strings.HasPrefix(con.ID, id) || util.StringInSlice(id, con.Names) {
-				cons = append(cons, con)
+
+	// Note: it would be nicer if the lists endpoint would support that as
+	// we could use the libpod backend for looking up containers rather
+	// than risking diverging the local and remote lookups.
+	//
+	// A `--filter nameOrId=abc` that can be specified multiple times would
+	// be awesome to have.
+	filtered := []entities.ListContainer{}
+	for _, nameOrID := range namesOrIDs {
+		// First determine if the container exists by doing an inspect.
+		// Inspect takes supports names and IDs and let's us determine
+		// a containers full ID.
+		inspectData, err := containers.Inspect(contextWithConnection, nameOrID, bindings.PFalse)
+		if err != nil {
+			if ignore && errorhandling.Contains(err, define.ErrNoSuchCtr) {
+				continue
+			}
+			return nil, err
+		}
+
+		// Now we can do a full match of the ID to find the right
+		// container. Note that we *really* need a full ID match to
+		// prevent any ambiguities between IDs and names (see #7837).
+		found := false
+		for _, ctr := range allContainers {
+			if ctr.ID == inspectData.ID {
+				filtered = append(filtered, ctr)
 				found = true
 				break
 			}
+
 		}
+
 		if !found && !ignore {
-			return nil, errors.Wrapf(define.ErrNoSuchCtr, "unable to find container %q", id)
+			return nil, errors.Wrapf(define.ErrNoSuchCtr, "unable to find container %q", nameOrID)
 		}
 	}
-	return cons, nil
+	return filtered, nil
 }
 
 func getPodsByContext(contextWithConnection context.Context, all bool, namesOrIDs []string) ([]*entities.ListPodsReport, error) {
-	var (
-		sPods []*entities.ListPodsReport
-	)
 	if all && len(namesOrIDs) > 0 {
 		return nil, errors.New("cannot lookup specific pods and all")
 	}
 
-	fPods, err := pods.List(contextWithConnection, nil)
+	allPods, err := pods.List(contextWithConnection, nil)
 	if err != nil {
 		return nil, err
 	}
 	if all {
-		return fPods, nil
+		return allPods, nil
 	}
+
+	filtered := []*entities.ListPodsReport{}
+	// Note: it would be nicer if the lists endpoint would support that as
+	// we could use the libpod backend for looking up pods rather than
+	// risking diverging the local and remote lookups.
+	//
+	// A `--filter nameOrId=abc` that can be specified multiple times would
+	// be awesome to have.
 	for _, nameOrID := range namesOrIDs {
-		var found bool
-		for _, f := range fPods {
-			if f.Name == nameOrID || strings.HasPrefix(f.Id, nameOrID) {
-				sPods = append(sPods, f)
+		// First determine if the pod exists by doing an inspect.
+		// Inspect takes supports names and IDs and let's us determine
+		// a containers full ID.
+		inspectData, err := pods.Inspect(contextWithConnection, nameOrID)
+		if err != nil {
+			if errorhandling.Contains(err, define.ErrNoSuchPod) {
+				return nil, errors.Wrapf(define.ErrNoSuchPod, "unable to find pod %q", nameOrID)
+			}
+			return nil, err
+		}
+
+		// Now we can do a full match of the ID to find the right pod.
+		// Note that we *really* need a full ID match to prevent any
+		// ambiguities between IDs and names (see #7837).
+		found := false
+		for _, pod := range allPods {
+			if pod.Id == inspectData.ID {
+				filtered = append(filtered, pod)
 				found = true
 				break
 			}
+
 		}
+
 		if !found {
 			return nil, errors.Wrapf(define.ErrNoSuchPod, "unable to find pod %q", nameOrID)
 		}
 	}
-	return sPods, nil
+	return filtered, nil
 }