From d791ac94d486811f573563e88c60951fbfd0b8e6 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Thu, 1 Oct 2020 14:17:02 +0200 Subject: [PATCH] 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 --- pkg/domain/infra/tunnel/helpers.go | 92 ++++++++++++++++++++++-------- 1 file changed, 68 insertions(+), 24 deletions(-) diff --git a/pkg/domain/infra/tunnel/helpers.go b/pkg/domain/infra/tunnel/helpers.go index 5944f855a0..7d28f1bd3d 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 }