From 8c9a24059715a04627557586a227f1bfabbc5407 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 8 Jul 2022 11:12:49 +0200 Subject: [PATCH 1/6] container: use const for null-terminator Also using `bytes.TrimSuffix()`, which is slightly more readable, and makes sure we're only stripping the null terminator. Signed-off-by: Sebastiaan van Stijn --- container/view.go | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/container/view.go b/container/view.go index 3c48644946..4c9a48ea8a 100644 --- a/container/view.go +++ b/container/view.go @@ -1,6 +1,7 @@ package container // import "github.com/docker/docker/container" import ( + "bytes" "errors" "fmt" "strings" @@ -452,6 +453,9 @@ func (v *View) transform(container *Container) *Snapshot { // memdb.StringFieldIndex can not be used since ID is a field from an embedded struct. type containerByIDIndexer struct{} +// terminator is the null character, used as a terminator. +const terminator = "\x00" + // FromObject implements the memdb.SingleIndexer interface for Container objects func (e *containerByIDIndexer) FromObject(obj interface{}) (bool, []byte, error) { c, ok := obj.(*Container) @@ -459,8 +463,7 @@ func (e *containerByIDIndexer) FromObject(obj interface{}) (bool, []byte, error) return false, nil, fmt.Errorf("%T is not a Container", obj) } // Add the null character as a terminator - v := c.ID + "\x00" - return true, []byte(v), nil + return true, []byte(c.ID + terminator), nil } // FromArgs implements the memdb.Indexer interface @@ -473,8 +476,7 @@ func (e *containerByIDIndexer) FromArgs(args ...interface{}) ([]byte, error) { return nil, fmt.Errorf("argument must be a string: %#v", args[0]) } // Add the null character as a terminator - arg += "\x00" - return []byte(arg), nil + return []byte(arg + terminator), nil } func (e *containerByIDIndexer) PrefixFromArgs(args ...interface{}) ([]byte, error) { @@ -484,11 +486,7 @@ func (e *containerByIDIndexer) PrefixFromArgs(args ...interface{}) ([]byte, erro } // Strip the null terminator, the rest is a prefix - n := len(val) - if n > 0 { - return val[:n-1], nil - } - return val, nil + return bytes.TrimSuffix(val, []byte(terminator)), nil } // namesByNameIndexer is used to index container name associations by name. @@ -501,7 +499,7 @@ func (e *namesByNameIndexer) FromObject(obj interface{}) (bool, []byte, error) { } // Add the null character as a terminator - return true, []byte(n.name + "\x00"), nil + return true, []byte(n.name + terminator), nil } func (e *namesByNameIndexer) FromArgs(args ...interface{}) ([]byte, error) { @@ -513,8 +511,7 @@ func (e *namesByNameIndexer) FromArgs(args ...interface{}) ([]byte, error) { return nil, fmt.Errorf("argument must be a string: %#v", args[0]) } // Add the null character as a terminator - arg += "\x00" - return []byte(arg), nil + return []byte(arg + terminator), nil } // namesByContainerIDIndexer is used to index container names by container ID. @@ -527,7 +524,7 @@ func (e *namesByContainerIDIndexer) FromObject(obj interface{}) (bool, []byte, e } // Add the null character as a terminator - return true, []byte(n.containerID + "\x00"), nil + return true, []byte(n.containerID + terminator), nil } func (e *namesByContainerIDIndexer) FromArgs(args ...interface{}) ([]byte, error) { @@ -539,6 +536,5 @@ func (e *namesByContainerIDIndexer) FromArgs(args ...interface{}) ([]byte, error return nil, fmt.Errorf("argument must be a string: %#v", args[0]) } // Add the null character as a terminator - arg += "\x00" - return []byte(arg), nil + return []byte(arg + terminator), nil } From ddaba6d57650bb85a59c43a0343aa22a084d303a Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 8 Jul 2022 13:07:19 +0200 Subject: [PATCH 2/6] daemon: filterByNameIDMatches() fix error handling Probably not critical, but we shouldn't use the data that's returned if there's an error. Signed-off-by: Sebastiaan van Stijn --- daemon/list.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/daemon/list.go b/daemon/list.go index 584acf572c..b85f8d6b48 100644 --- a/daemon/list.go +++ b/daemon/list.go @@ -119,8 +119,11 @@ func (daemon *Daemon) filterByNameIDMatches(view *container.View, filter *listCo // standard behavior of walking the entire container // list from the daemon's in-memory store all, err := view.All() + if err != nil { + return nil, err + } sort.Sort(byCreatedDescending(all)) - return all, err + return all, nil } // idSearch will determine if we limit name matching to the IDs From da4d627e7910bb5310b039f79d84b49eda8bff09 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 8 Jul 2022 13:12:30 +0200 Subject: [PATCH 3/6] container: ViewDB: use errdefs for non-existing containers Signed-off-by: Sebastiaan van Stijn --- container/view.go | 19 +++---------------- daemon/container.go | 11 +++++------ daemon/list.go | 15 +++++++-------- 3 files changed, 15 insertions(+), 30 deletions(-) diff --git a/container/view.go b/container/view.go index 4c9a48ea8a..9b63c74eef 100644 --- a/container/view.go +++ b/container/view.go @@ -9,6 +9,7 @@ import ( "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/network" + "github.com/docker/docker/errdefs" "github.com/docker/go-connections/nat" memdb "github.com/hashicorp/go-memdb" "github.com/sirupsen/logrus" @@ -32,9 +33,6 @@ var ( var ( // ErrEmptyPrefix is an error returned if the prefix was empty. ErrEmptyPrefix = errors.New("Prefix can't be empty") - - // ErrNotExist is returned when ID or its prefix not found in index. - ErrNotExist = errors.New("ID does not exist") ) // ErrAmbiguousPrefix is returned if the prefix was ambiguous @@ -113,17 +111,6 @@ type ViewDB struct { store *memdb.MemDB } -// NoSuchContainerError indicates that the container wasn't found in the -// database. -type NoSuchContainerError struct { - id string -} - -// Error satisfies the error interface. -func (e NoSuchContainerError) Error() string { - return "no such container " + e.id -} - // NewViewDB provides the default implementation, with the default schema func NewViewDB() (*ViewDB, error) { store, err := memdb.NewMemDB(schema) @@ -164,7 +151,7 @@ func (db *ViewDB) GetByPrefix(s string) (string, error) { return id, nil } - return "", ErrNotExist + return "", errdefs.NotFound(errors.New("No such container: " + s)) } // Snapshot provides a consistent read-only view of the database. @@ -268,7 +255,7 @@ func (v *View) Get(id string) (*Snapshot, error) { return nil, err } if s == nil { - return nil, NoSuchContainerError{id: id} + return nil, errdefs.NotFound(errors.New("No such container: " + id)) } return v.transform(s.(*Container)), nil } diff --git a/daemon/container.go b/daemon/container.go index d21b3386e1..75d6c48a26 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -48,13 +48,12 @@ func (daemon *Daemon) GetContainer(prefixOrName string) (*container.Container, e return containerByName, nil } - containerID, indexError := daemon.containersReplica.GetByPrefix(prefixOrName) - if indexError != nil { - // When truncindex defines an error type, use that instead - if indexError == container.ErrNotExist { - return nil, containerNotFound(prefixOrName) + containerID, err := daemon.containersReplica.GetByPrefix(prefixOrName) + if err != nil { + if errdefs.IsNotFound(err) { + return nil, err } - return nil, errdefs.System(indexError) + return nil, errdefs.System(err) } return daemon.containers.Get(containerID), nil } diff --git a/daemon/list.go b/daemon/list.go index b85f8d6b48..c7776816ee 100644 --- a/daemon/list.go +++ b/daemon/list.go @@ -162,14 +162,14 @@ func (daemon *Daemon) filterByNameIDMatches(view *container.View, filter *listCo cntrs := make([]container.Snapshot, 0, len(matches)) for id := range matches { c, err := view.Get(id) - switch err.(type) { - case nil: - cntrs = append(cntrs, *c) - case container.NoSuchContainerError: - // ignore error - default: + if err != nil { + if errdefs.IsNotFound(err) { + // ignore error + continue + } return nil, err } + cntrs = append(cntrs, *c) } // Restore sort-order after filtering @@ -367,8 +367,7 @@ func (daemon *Daemon) foldFilter(ctx context.Context, view *container.View, conf func idOrNameFilter(view *container.View, value string) (*container.Snapshot, error) { filter, err := view.Get(value) - switch err.(type) { - case container.NoSuchContainerError: + if err != nil && errdefs.IsNotFound(err) { // Try name search instead found := "" for id, idNames := range view.GetAllNames() { From 94dea2018e6e9b4836af610b68293f81bb5adb62 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 8 Jul 2022 13:23:01 +0200 Subject: [PATCH 4/6] container: ViewDB: GetByPrefix() return typed errors Signed-off-by: Sebastiaan van Stijn --- container/view.go | 21 +++------------------ daemon/container.go | 5 +---- 2 files changed, 4 insertions(+), 22 deletions(-) diff --git a/container/view.go b/container/view.go index 9b63c74eef..837345d3e7 100644 --- a/container/view.go +++ b/container/view.go @@ -30,21 +30,6 @@ var ( ErrNameNotReserved = errors.New("name is not reserved") ) -var ( - // ErrEmptyPrefix is an error returned if the prefix was empty. - ErrEmptyPrefix = errors.New("Prefix can't be empty") -) - -// ErrAmbiguousPrefix is returned if the prefix was ambiguous -// (multiple ids for the prefix). -type ErrAmbiguousPrefix struct { - prefix string -} - -func (e ErrAmbiguousPrefix) Error() string { - return fmt.Sprintf("Multiple IDs found with provided prefix: %s", e.prefix) -} - // Snapshot is a read only view for Containers. It holds all information necessary to serve container queries in a // versioned ACID in-memory store. type Snapshot struct { @@ -124,12 +109,12 @@ func NewViewDB() (*ViewDB, error) { // error if an empty prefix was given or if multiple containers match the prefix. func (db *ViewDB) GetByPrefix(s string) (string, error) { if s == "" { - return "", ErrEmptyPrefix + return "", errdefs.InvalidParameter(errors.New("prefix can't be empty")) } txn := db.store.Txn(false) iter, err := txn.Get(memdbContainersTable, memdbIDIndexPrefix, s) if err != nil { - return "", err + return "", errdefs.System(err) } var ( @@ -142,7 +127,7 @@ func (db *ViewDB) GetByPrefix(s string) (string, error) { break } if id != "" { - return "", ErrAmbiguousPrefix{prefix: s} + return "", errdefs.InvalidParameter(errors.New("multiple IDs found with provided prefix: " + s)) } id = item.(*Container).ID } diff --git a/daemon/container.go b/daemon/container.go index 75d6c48a26..3cfa0107c2 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -50,10 +50,7 @@ func (daemon *Daemon) GetContainer(prefixOrName string) (*container.Container, e containerID, err := daemon.containersReplica.GetByPrefix(prefixOrName) if err != nil { - if errdefs.IsNotFound(err) { - return nil, err - } - return nil, errdefs.System(err) + return nil, err } return daemon.containers.Get(containerID), nil } From 6549a270e962dba0e24033367ec328b586444c92 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 8 Jul 2022 13:29:56 +0200 Subject: [PATCH 5/6] container: ViewDB: return typed system errors Signed-off-by: Sebastiaan van Stijn --- container/view.go | 12 ++++++------ daemon/daemon.go | 2 +- daemon/names.go | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/container/view.go b/container/view.go index 837345d3e7..5c23f7c539 100644 --- a/container/view.go +++ b/container/view.go @@ -100,7 +100,7 @@ type ViewDB struct { func NewViewDB() (*ViewDB, error) { store, err := memdb.NewMemDB(schema) if err != nil { - return nil, err + return nil, errdefs.System(err) } return &ViewDB{store: store}, nil } @@ -151,7 +151,7 @@ func (db *ViewDB) withTxn(cb func(*memdb.Txn) error) error { err := cb(txn) if err != nil { txn.Abort() - return err + return errdefs.System(err) } txn.Commit() return nil @@ -190,7 +190,7 @@ func (db *ViewDB) ReserveName(name, containerID string) error { return db.withTxn(func(txn *memdb.Txn) error { s, err := txn.First(memdbNamesTable, memdbIDIndex, name) if err != nil { - return err + return errdefs.System(err) } if s != nil { if s.(nameAssociation).containerID != containerID { @@ -220,7 +220,7 @@ func (v *View) All() ([]Snapshot, error) { var all []Snapshot iter, err := v.txn.Get(memdbContainersTable, memdbIDIndex) if err != nil { - return nil, err + return nil, errdefs.System(err) } for { item := iter.Next() @@ -237,7 +237,7 @@ func (v *View) All() ([]Snapshot, error) { func (v *View) Get(id string) (*Snapshot, error) { s, err := v.txn.First(memdbContainersTable, memdbIDIndex, id) if err != nil { - return nil, err + return nil, errdefs.System(err) } if s == nil { return nil, errdefs.NotFound(errors.New("No such container: " + id)) @@ -268,7 +268,7 @@ func (v *View) getNames(containerID string) []string { func (v *View) GetID(name string) (string, error) { s, err := v.txn.First(memdbNamesTable, memdbIDIndex, name) if err != nil { - return "", err + return "", errdefs.System(err) } if s == nil { return "", ErrNameNotReserved diff --git a/daemon/daemon.go b/daemon/daemon.go index 729679bfac..3f221d1322 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -647,7 +647,7 @@ func (daemon *Daemon) parents(c *container.Container) map[string]*container.Cont func (daemon *Daemon) registerLink(parent, child *container.Container, alias string) error { fullName := path.Join(parent.Name, alias) if err := daemon.containersReplica.ReserveName(fullName, child.ID); err != nil { - if err == container.ErrNameReserved { + if errors.Is(err, container.ErrNameReserved) { logrus.Warnf("error registering link for %s, to %s, as alias %s, ignoring: %v", parent.ID, child.ID, alias, err) return nil } diff --git a/daemon/names.go b/daemon/names.go index 4fa39af2ee..b6d6e4da84 100644 --- a/daemon/names.go +++ b/daemon/names.go @@ -64,7 +64,7 @@ func (daemon *Daemon) reserveName(id, name string) (string, error) { } if err := daemon.containersReplica.ReserveName(name, id); err != nil { - if err == container.ErrNameReserved { + if errors.Is(err, container.ErrNameReserved) { id, err := daemon.containersReplica.Snapshot().GetID(name) if err != nil { logrus.Errorf("got unexpected error while looking up reserved name: %v", err) @@ -90,7 +90,7 @@ func (daemon *Daemon) generateNewName(id string) (string, error) { } if err := daemon.containersReplica.ReserveName(name, id); err != nil { - if err == container.ErrNameReserved { + if errors.Is(err, container.ErrNameReserved) { continue } return "", err From 28382c58ec2078d17ae3a3d6a2f262733a1e779d Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 8 Dec 2022 14:35:13 +0100 Subject: [PATCH 6/6] container: ViewDB: use logrus.WithError() Signed-off-by: Sebastiaan van Stijn --- container/view.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/container/view.go b/container/view.go index 5c23f7c539..62f7b1b95b 100644 --- a/container/view.go +++ b/container/view.go @@ -391,7 +391,7 @@ func (v *View) transform(container *Container) *Snapshot { for port, bindings := range container.NetworkSettings.Ports { p, err := nat.ParsePort(port.Port()) if err != nil { - logrus.Warnf("invalid port map %+v", err) + logrus.WithError(err).Warn("invalid port map") continue } if len(bindings) == 0 { @@ -404,7 +404,7 @@ func (v *View) transform(container *Container) *Snapshot { for _, binding := range bindings { h, err := nat.ParsePort(binding.HostPort) if err != nil { - logrus.Warnf("invalid host port map %+v", err) + logrus.WithError(err).Warn("invalid host port map") continue } snapshot.Ports = append(snapshot.Ports, types.Port{