Merge pull request 'inspect by kind, not the ambiguous bare form — a same-named network stops a container from ever being found' (#25) from fix/inspect-by-kind-not-the-ambiguous-bare-form into main

internal/apply/apply.go

@@ -1347,11 +1347,15 @@ func containerSpecReading(r *declaration.Container, declares, reads map[string]s
 
 // containerState reports whether a container is running and which spec made it.
 // The error means the container does not exist.
+//
+// `container inspect`, not the bare form: a name is not unique across object kinds (a network and
+// its container are routinely named alike), and the bare form can resolve to the wrong kind
+// instead of reporting absence — see inspectFound in hold.go for the failure this produces.
 func containerState(ctx context.Context, name string, run Runner) (state struct {
 	Running bool
 	Spec    string
 }, err error) {
-	out, err := run(ctx, "docker", "inspect", "--format",
+	out, err := run(ctx, "docker", "container", "inspect", "--format",
 		"{{.State.Running}}\t{{index .Config.Labels \""+specLabel+"\"}}", name)
 	if err != nil {
 		return state, fmt.Errorf("no container named %s", name)

internal/apply/apply_test.go

@@ -635,7 +635,7 @@ func TestAContainerThatExitsImmediatelyFailsTheApply(t *testing.T) {
 		switch {
 		case args[0] == "info":
 			return "27.0\n", nil
-		case args[0] == "inspect":
+		case args[0] == "container":
 			return "false\t" + "", nil // exists, not running
 		case args[0] == "run":
 			return "deadbeef\n", nil
@@ -673,7 +673,7 @@ func TestAContainerWhoseDeclarationChangedIsReplaced(t *testing.T) {
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil
-		case "inspect":
+		case "container":
 			if created {
 				return "true\t" + want, nil
 			}
@@ -711,7 +711,7 @@ func TestAContainerThatMatchesIsLeftAlone(t *testing.T) {
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil
-		case "inspect":
+		case "container":
 			return "true\t" + spec, nil
 		}
 		touched = true
@@ -756,7 +756,7 @@ func TestAContainerIsRecreatedWhenARestartOnResourceChanged(t *testing.T) {
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil
-		case "inspect":
+		case "container":
 			// Already there and running, created against the file as it was. After the host
 			// recreates it, the runtime holds the one it just made — as a real one would.
 			if created {
@@ -1000,7 +1000,7 @@ func TestAContainerUsesTheRuntimeTheMachineHas(t *testing.T) {
 		switch args[0] {
 		case "info":
 			return "6.1.0\n", nil
-		case "inspect":
+		case "container":
 			return "false\t\n", errors.New("no such container")
 		case "run":
 			return "deadbeef\n", nil
@@ -1326,7 +1326,7 @@ func TestAContainerIsGivenTheMeshsNames(t *testing.T) {
 		switch args[0] {
 		case "info":
 			return "29.0.0\n", nil
-		case "inspect":
+		case "container":
 			return "false\t\n", errors.New("no such container")
 		case "run":
 			ran = args
@@ -1361,7 +1361,7 @@ func TestAContainerGivenNoNamesIsRunAsBefore(t *testing.T) {
 		switch args[0] {
 		case "info":
 			return "29.0.0\n", nil
-		case "inspect":
+		case "container":
 			return "false\t\n", errors.New("no such container")
 		case "run":
 			ran = args
@@ -1546,7 +1546,7 @@ func TestAContainerStaleFromAnEarlierApplyIsReplaced(t *testing.T) {
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil
-		case "inspect":
+		case "container":
 			if created {
 				return "true\t" + fresh, nil
 			}

internal/apply/hold.go

@@ -424,12 +424,21 @@ type foundContainer struct {
 
 // inspectFound reads a container by name the way a hold needs it: its id, whether it runs, and
 // whether a host made it.
+//
+// **`container inspect`, not the bare form.** A name is not unique across object kinds — a
+// module regularly names a network the same as the container that joins it (`keycloak` names
+// both, and it is ordinary). The bare form resolves across every kind and returns whichever it
+// finds, so a container that does not exist yet but a same-named network does answers with the
+// network's JSON — no `.State` field at all — and the template below fails to execute rather
+// than failing to find anything. That reads as "the runtime could not say", which this function's
+// caller correctly refuses to build on (novox/hq ADR 0100) — but there was something to say, a
+// question of kind, not of ambiguity that should have stopped anything.
 func inspectFound(ctx context.Context, name string, run Runner) (foundContainer, bool, error) {
 	cri, err := containerRuntime(ctx, run)
 	if err != nil {
 		return foundContainer{}, false, fmt.Errorf("%w, so nothing can be said about %q", err, name)
 	}
-	out, err := run(ctx, cri, "inspect", "--format",
+	out, err := run(ctx, cri, "container", "inspect", "--format",
 		"{{.Id}}\t{{.State.Running}}\t{{index .Config.Labels \""+specLabel+"\"}}", name)
 	if err != nil {
 		if absent(err) {

internal/apply/hold_test.go

@@ -120,7 +120,10 @@ func (m *machine) run(_ context.Context, name string, args ...string) (string, e
 		return "", errors.New("no such volume")
 	case "info":
 		return "27.0\n", nil
-	case "inspect":
+	case "container":
+		if args[1] != "inspect" {
+			return "", errors.New("unexpected docker container command")
+		}
 		c, ok := m.containers[args[len(args)-1]]
 		if !ok {
 			return "", errors.New("no such container")
@@ -129,7 +132,7 @@ func (m *machine) run(_ context.Context, name string, args ...string) (string, e
 		if c.running {
 			running = "true"
 		}
-		if strings.HasPrefix(args[2], "{{.Id}}") {
+		if strings.HasPrefix(args[3], "{{.Id}}") {
 			return c.id + "\t" + running + "\t" + c.spec + "\n", nil
 		}
 		return running + "\t" + c.spec + "\n", nil
@@ -916,7 +919,7 @@ func TestAVolumeTheRuntimeCannotBeAskedAboutStopsTheContainer(t *testing.T) {
 			return "27.0\n", nil
 		case name == "docker" && args[0] == "volume":
 			return "", errors.New("docker exited 1: Cannot connect to the Docker daemon")
-		case name == "docker" && args[0] == "inspect":
+		case name == "docker" && args[0] == "container":
 			return "", errors.New("Error: No such object: hello-web")
 		case name == "docker":
 			return "", errors.New("docker run must not happen")

internal/apply/inspect_scope_test.go

@@ -0,0 +1,66 @@
+package apply
+
+import (
+	"context"
+	"errors"
+	"testing"
+)
+
+// A name is not unique across object kinds: a module regularly names a network the same as the
+// container that joins it (keycloak does this today, ordinarily). `docker inspect <name>`, unlike
+// `docker container inspect <name>`, resolves across every kind — so when the container does not
+// exist yet but a same-named network does, the bare form answers with the network's JSON instead
+// of reporting the container absent. containerState and inspectFound must ask by kind, or a
+// same-named network makes them unable to tell "not here yet" from "the runtime is broken"
+// (novox/hq ADR 0100's refusal, tripped by nothing wrong).
+//
+// dockerLikeByKind is Docker's real behaviour, not the bug: `container inspect` only ever
+// answers from the container namespace. A fake that also answered the bare, unscoped form would
+// not catch a regression back to it — this one refuses to, on purpose.
+func dockerLikeByKind(containers map[string]bool) func(context.Context, string, ...string) (string, error) {
+	return func(_ context.Context, name string, args ...string) (string, error) {
+		if name != "docker" {
+			return "", errors.New("unexpected program: " + name)
+		}
+		if len(args) > 0 && args[0] == "info" {
+			// containerRuntime's probe, answered so inspectFound gets past it to the check under
+			// test.
+			return "27.0\n", nil
+		}
+		if len(args) < 2 || args[0] != "container" || args[1] != "inspect" {
+			return "", errors.New("unexpected command: only `docker container inspect` is modelled here")
+		}
+		target := args[len(args)-1]
+		if !containers[target] {
+			return "", errors.New("Error: No such container: " + target)
+		}
+		return "false\t\n", nil
+	}
+}
+
+func TestContainerStateAsksTheContainerNamespaceNotTheBareForm(t *testing.T) {
+	// "minio" exists only as a network in this scenario — never in `containers` — matching the
+	// live failure this guards: a module's network and its container share a name, and the
+	// container does not exist yet.
+	run := dockerLikeByKind(map[string]bool{"keycloak": true})
+
+	if _, err := containerState(context.Background(), "minio", run); err == nil {
+		t.Fatal("a container that does not exist should report absent, not be mistaken for found")
+	}
+	if state, err := containerState(context.Background(), "keycloak", run); err != nil {
+		t.Fatalf("a container that does exist should be found: %v", err)
+	} else if state.Running {
+		t.Errorf("the fake said not running; containerState disagreed: %+v", state)
+	}
+}
+
+func TestInspectFoundAsksTheContainerNamespaceNotTheBareForm(t *testing.T) {
+	run := dockerLikeByKind(map[string]bool{"keycloak": true})
+
+	if _, exists, err := inspectFound(context.Background(), "minio", run); err != nil || exists {
+		t.Fatalf("a container that does not exist should be reported absent cleanly, not refused: exists=%v err=%v", exists, err)
+	}
+	if _, exists, err := inspectFound(context.Background(), "keycloak", run); err != nil || !exists {
+		t.Fatalf("a container that does exist should be found: exists=%v err=%v", exists, err)
+	}
+}

internal/apply/runonce_test.go

@@ -113,7 +113,7 @@ func TestAFailedRunOnceStepGatesWhatFollows(t *testing.T) {
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil
-		case "inspect":
+		case "container":
 			return "false\t\n", errors.New("no such container")
 		case "run":
 			startedNames = append(startedNames, nameOf(args))
@@ -287,7 +287,7 @@ func TestAContainerNamingARunOnceStepIsRecreatedWhenItRan(t *testing.T) {
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil
-		case "inspect":
+		case "container":
 			// The server is up, made from exactly this spec — nothing but the step's run says
 			// it must be replaced.
 			return "true\t" + spec, nil

internal/apply/schedule_test.go

@@ -208,7 +208,7 @@ func TestAScheduledStepDoesNotGateWhatFollows(t *testing.T) {
 		switch args[0] {
 		case "info":
 			return "27.0\n", nil
-		case "inspect":
+		case "container":
 			// The container name is the last argument to `docker inspect --format ... <name>`.
 			target := args[len(args)-1]
 			if spec, up := specs[target]; up {

internal/apply/vocabulary_test.go

@@ -376,7 +376,7 @@ func TestAContainerIsGivenItsEnvironmentFiles(t *testing.T) {
 	var ran []string
 	run := func(_ context.Context, name string, args ...string) (string, error) {
 		ran = append(ran, name+" "+strings.Join(args, " "))
-		if len(args) > 0 && args[0] == "inspect" {
+		if len(args) > 0 && args[0] == "container" {
 			return "", fmt.Errorf("no such container")
 		}
 		return "", nil

internal/bootstrap/adopt_in_place_test.go

@@ -35,7 +35,7 @@ func (l *labelled) run(_ context.Context, _ string, args ...string) (string, err
 	switch args[0] {
 	case "info":
 		return "27.0\n", nil
-	case "inspect":
+	case "container":
 		spec, ok := l.spec[args[len(args)-1]]
 		if !ok {
 			return "", errors.New("no such container")

internal/bootstrap/phase_packages.go

@@ -230,8 +230,9 @@ func raiseGiteaServer(ctx context.Context, run Runner, timeout time.Duration, db
 	defer cancel()
 
 	// Already there: a re-run does not raise a second one. `docker start` is a no-op on a running
-	// container and revives a stopped one.
-	if out, _ := run(asking, "docker", "inspect", "--format", "{{.Id}}", giteaBootstrap); strings.TrimSpace(out) != "" {
+	// container and revives a stopped one. `container inspect`, not the bare form: a name is not
+	// unique across object kinds, and `.Id` resolves on a network or volume too.
+	if out, _ := run(asking, "docker", "container", "inspect", "--format", "{{.Id}}", giteaBootstrap); strings.TrimSpace(out) != "" {
 		_, _ = run(asking, "docker", "start", giteaBootstrap)
 		return nil
 	}

internal/bootstrap/ports.go

@@ -431,7 +431,9 @@ func NamesFree(ctx context.Context, run Runner, names []string, known store.Stat
 	sorted := append([]string{}, names...)
 	sort.Strings(sorted)
 	for _, name := range sorted {
-		out, err := run(ctx, "docker", "inspect", "--format",
+		// `container inspect`: a name is not unique across object kinds, and the bare form can
+		// resolve to a same-named network or volume instead of reporting the container absent.
+		out, err := run(ctx, "docker", "container", "inspect", "--format",
 			"{{index .Config.Labels \"mesh-host.spec\"}}", name)
 		if err != nil {
 			continue // no such container

internal/bootstrap/ports_test.go

@@ -145,7 +145,7 @@ func (m machineRunner) run(_ context.Context, name string, args ...string) (stri
 		return m.ss, nil
 	case name == "docker" && args[0] == "ps":
 		return m.ps, nil
-	case name == "docker" && args[0] == "inspect":
+	case name == "docker" && args[0] == "container":
 		n := args[len(args)-1]
 		if m.labelled[n] {
 			return "abc\n", nil

internal/bootstrap/publish.go

@@ -150,7 +150,7 @@ func digestOf(ctx context.Context, o Options, d Deps, remote string) (string, er
 
 	// The registry has it. What digest, according to the runtime that pushed it.
 	reading, cancel := context.WithTimeout(ctx, o.Timeout)
-	out, err := d.Run(reading, "docker", "inspect", "--format", "{{json .RepoDigests}}",
+	out, err := d.Run(reading, "docker", "image", "inspect", "--format", "{{json .RepoDigests}}",
 		remote+":"+genesisTag)
 	cancel()
 	if err != nil {

internal/bootstrap/publish_test.go

@@ -50,7 +50,7 @@ func TestAnImageNoRegistryHasEverHeldIsPushed(t *testing.T) {
 			case "push":
 				pushed = true
 				return "", nil
-			case "inspect":
+			case "image":
 				return `["127.0.0.1:5000/mesh-controller@sha256:` + strings.Repeat("a", 64) + `"]`, nil
 			}
 			return "", fmt.Errorf("unexpected: %v", args)
@@ -80,7 +80,7 @@ func TestAnImageTheRegistryAlreadyServesIsNotPushedAgain(t *testing.T) {
 			return http.StatusOK, `{"name":"mesh-controller","tags":["genesis"]}`, nil
 		},
 		func(_ string, args []string) (string, error) {
-			if args[0] == "inspect" {
+			if args[0] == "image" {
 				return `["127.0.0.1:5000/mesh-controller@sha256:` + strings.Repeat("b", 64) + `"]`, nil
 			}
 			return "", fmt.Errorf("unexpected: %v", args)
@@ -111,7 +111,7 @@ func TestTheDigestComesFromThisMeshsOwnRegistry(t *testing.T) {
 			return http.StatusOK, `{"tags":["genesis"]}`, nil
 		},
 		func(_ string, args []string) (string, error) {
-			if args[0] == "inspect" {
+			if args[0] == "image" {
 				return `["` + elsewhere + `","` + ours + `"]`, nil
 			}
 			return "", fmt.Errorf("unexpected: %v", args)
@@ -166,7 +166,7 @@ func TestATagIsNotAPin(t *testing.T) {
 			return http.StatusOK, `{"tags":["genesis"]}`, nil
 		},
 		func(_ string, args []string) (string, error) {
-			if args[0] == "inspect" {
+			if args[0] == "image" {
 				return `["127.0.0.1:5000/mesh-controller:genesis"]`, nil
 			}
 			return "", nil

internal/bootstrap/registry_test.go

@@ -62,7 +62,7 @@ func aMeshThatAgrees(answers map[string]string) func(string, []string) (string,
 			return "", fmt.Errorf("unexpected program %q", name)
 		case args[0] == "cp":
 			return "", nil
-		case args[0] == "inspect":
+		case args[0] == "container":
 			return "true running\n", nil
 		case args[0] == "exec":
 			return "", nil

internal/bootstrap/verify.go

@@ -129,8 +129,9 @@ func containerRunning(ctx context.Context, run Runner, probe time.Duration, name
 	defer cancel()
 
 	// Both facts in one answer, so a container that is not running is reported with what it IS
-	// rather than with the absence of what it should be.
-	out, err := run(asking, "docker", "inspect", "--format", "{{.State.Running}} {{.State.Status}}", name)
+	// rather than with the absence of what it should be. `container inspect`, not the bare form:
+	// a same-named network or volume would otherwise answer in the container's place.
+	out, err := run(asking, "docker", "container", "inspect", "--format", "{{.State.Running}} {{.State.Status}}", name)
 	if err != nil {
 		return containerState{}, fmt.Errorf(
 			"the container %q is not there at all, and the apply reported it applied: %w", name, err)

internal/bootstrap/verify_test.go

@@ -30,7 +30,7 @@ func TestAContainerThatIsUpIsNotAControlPlaneThatReplies(t *testing.T) {
 
 	runtime := &asked{answer: func(_ string, args []string) (string, error) {
 		switch args[0] {
-		case "inspect":
+		case "container":
 			return "true running\n", nil
 		case "exec":
 			// Up, and saying nothing. The program inside is not answering.
@@ -59,7 +59,7 @@ func TestAControlPlaneThatSaysNothingHasNotAnswered(t *testing.T) {
 	defer func() { answerEvery = previous }()
 
 	runtime := &asked{answer: func(_ string, args []string) (string, error) {
-		if args[0] == "inspect" {
+		if args[0] == "container" {
 			return "true running\n", nil
 		}
 		return "   \n", nil
@@ -74,7 +74,7 @@ func TestAControlPlaneThatSaysNothingHasNotAnswered(t *testing.T) {
 // The foundation answering is the whole point, and what it said is reported rather than asserted.
 func TestAFoundationThatIsUpAndAnsweringIsAccepted(t *testing.T) {
 	runtime := &asked{answer: func(_ string, args []string) (string, error) {
-		if args[0] == "inspect" {
+		if args[0] == "container" {
 			return "true running\n", nil
 		}
 		return "1 node, 0 waiting\n", nil
@@ -112,7 +112,7 @@ func TestAControlPlaneThatIsStillStartingIsWaitedFor(t *testing.T) {
 
 	attempts := 0
 	runtime := &asked{answer: func(_ string, args []string) (string, error) {
-		if args[0] == "inspect" {
+		if args[0] == "container" {
 			return "true running\n", nil
 		}
 		attempts++
@@ -132,10 +132,10 @@ func TestAControlPlaneThatIsStillStartingIsWaitedFor(t *testing.T) {
 // than being told only that something is not what it should be.
 func TestAContainerThatExitedIsNamedWithItsState(t *testing.T) {
 	runtime := &asked{answer: func(_ string, args []string) (string, error) {
-		if args[0] == "inspect" && args[len(args)-1] == "mesh-broker" {
+		if args[0] == "container" && args[len(args)-1] == "mesh-broker" {
 			return "false exited\n", nil
 		}
-		if args[0] == "inspect" {
+		if args[0] == "container" {
 			return "true running\n", nil
 		}
 		return "", fmt.Errorf("unexpected command: %v", args)
@@ -155,7 +155,7 @@ func TestAContainerThatExitedIsNamedWithItsState(t *testing.T) {
 // `FROM scratch` and has no shell for a command line to be interpreted by.
 func TestTheControlPlaneIsAskedByRunningItsOwnBinary(t *testing.T) {
 	runtime := &asked{answer: func(_ string, args []string) (string, error) {
-		if args[0] == "inspect" {
+		if args[0] == "container" {
 			return "true running\n", nil
 		}
 		return "1 node\n", nil