cli/command/container: prevent panic during stats on empty event Actor.ID

This code was missing a check for the ID field before truncating it to a
shorter length for presentation. This would result in a panic if an event
would either have an empty ID field or a shorter length ID;

    panic: runtime error: slice bounds out of range [:12] with length 0

    goroutine 82 [running]:
    github.com/docker/cli/cli/command/container.RunStats.func2({{0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}, {0x40001fcba0, 0x9}, {0x40001fcba9, 0x5}, ...})
        /go/src/github.com/docker/cli/cli/command/container/stats.go:146 +0x1d0
    created by github.com/docker/cli/cli/command/container.(*eventHandler).watch in goroutine 6
        /go/src/github.com/docker/cli/cli/command/container/stats.go:363 +0x1c8

We need to look at this code in general; the truncated ID is passed to
NewStats, which uses the ID to propagate the `Container` field in the
`StatsEntry` struct. which is not used in the default format used by
`docker stats` and, having the same content as the `ID` field on the
same struct, doesn't make it very useful, other than being able to
present it under a `CONTAINER` column (instead of `CONTAINER ID`);
we should consider deprecating it; there may be some subtle things
to look into here; the `Container` field originally held the container
name. This was changed in [moby@ef915fd], which introduced separate
`ID` and `Name` fields, renaming the old `Name` field to container.

Looking at [`Stats.SetStatistics()`] and related code in [stats_helpers.go],
the `Container` field is used as the "canonical" reference for the stats
record; this allows the stats _data_ to be refreshed when a new stats
sample arrives for the same container (also see [moby@929a77b], which
moved locking to the `Stats` wrapper struct). This construct allows to
account for intermediate states, where a stats sample was incomplete
or could produce an error; in that case, the reference to the container
for which the stats were sampled is kept to allow removing a container
from the list once the container was removed. We should consider removing
`Container` as a formatting option, and moving the `Container` field to
the outer struct; this makes the outer struct responsible for keeping a
reference to the container, allowing the `StatsEntry` as a whole to be
replaced atomically.

This patch only addresses the panic;

- It changes the logic to preserve the container ID verbatim instead
  of truncating. This allows stats samples to be matched against the
  `Actor.ID` as-is.
- Truncating the `Container` is moved to the presentation logic;
  currently this does not take `--no-trunc` into account to keep
  the existing behavior, but we can (should) consider adding this.
- Logging is improved to use structured logs, and an extra check is
  added to prevent empty IDs from being added as watcher.

[`Stats.SetStatistics()`]: https://github.com/docker/cli/blob/82281087e3e186c5a2eafa0d973e849ff84c357d/cli/command/container/formatter_stats.go#L88-L94
[moby@ef915fd]: moby/moby@ef915fd
[moby@929a77b]: moby/moby@929a77b
[stats_helpers.go]: https://github.com/docker/cli/blob/82281087e3e186c5a2eafa0d973e849ff84c357d/cli/command/container/stats_helpers.go#L26-L51

Signed-off-by: Sebastiaan van Stijn <[email protected]>
(cherry picked from commit 9b79e48)
Signed-off-by: Sebastiaan van Stijn <[email protected]>

Author: thaJeztah

cli/command/container/formatter_stats.go

@@ -167,6 +167,7 @@ func (c *statsContext) Container() string {
 }
 
 func (c *statsContext) Name() string {
+	// TODO(thaJeztah): make this explicitly trim the "/" prefix, not just any char.
 	if len(c.s.Name) > 1 {
 		return c.s.Name[1:]
 	}

cli/command/container/stats.go

@@ -139,7 +139,7 @@ func RunStats(ctx context.Context, dockerCLI command.Cli, options *StatsOptions)
 		eh := newEventHandler()
 		if options.All {
 			eh.setHandler(events.ActionCreate, func(e events.Message) {
-				s := NewStats(e.Actor.ID[:12])
+				s := NewStats(e.Actor.ID)
 				if cStats.add(s) {
 					waitFirst.Add(1)
 					go collect(ctx, s, apiClient, !options.NoStream, waitFirst)
@@ -148,7 +148,7 @@ func RunStats(ctx context.Context, dockerCLI command.Cli, options *StatsOptions)
 		}
 
 		eh.setHandler(events.ActionStart, func(e events.Message) {
-			s := NewStats(e.Actor.ID[:12])
+			s := NewStats(e.Actor.ID)
 			if cStats.add(s) {
 				waitFirst.Add(1)
 				go collect(ctx, s, apiClient, !options.NoStream, waitFirst)
@@ -157,7 +157,7 @@ func RunStats(ctx context.Context, dockerCLI command.Cli, options *StatsOptions)
 
 		if !options.All {
 			eh.setHandler(events.ActionDie, func(e events.Message) {
-				cStats.remove(e.Actor.ID[:12])
+				cStats.remove(e.Actor.ID)
 			})
 		}
 
@@ -210,7 +210,7 @@ func RunStats(ctx context.Context, dockerCLI command.Cli, options *StatsOptions)
 			return err
 		}
 		for _, ctr := range cs {
-			s := NewStats(ctr.ID[:12])
+			s := NewStats(ctr.ID)
 			if cStats.add(s) {
 				waitFirst.Add(1)
 				go collect(ctx, s, apiClient, !options.NoStream, waitFirst)
@@ -363,7 +363,12 @@ func (eh *eventHandler) watch(c <-chan events.Message) {
 		if !exists {
 			continue
 		}
-		logrus.Debugf("event handler: received event: %v", e)
+		if e.Actor.ID == "" {
+			logrus.WithField("event", e).Errorf("event handler: received %s event with empty ID", e.Action)
+			continue
+		}
+
+		logrus.WithField("event", e).Debugf("event handler: received %s event for: %s", e.Action, e.Actor.ID)
 		go h(e)
 	}
 }