diff --git a/Makefile b/Makefile index d5bf716..1c5f2c2 100644 --- a/Makefile +++ b/Makefile @@ -68,9 +68,18 @@ host: # # The saved image occupies the embed slot for the length of one build and the placeholder goes # back, exactly as `host:` does with the bundle. Nothing large is ever committed. +# +# IMAGE must be a NAME:TAG and not an id. The installer identifies the carried image by its tag, +# because an image id is the digest of the image's configuration and a runtime REWRITES that +# configuration as it loads — so the id in the archive is not the id the receiving machine will +# hold, and the tag is the only name that survives the transfer. Saving by id produces an archive +# with no tags at all, which the installer refuses; caught here instead, in front of the person who +# can fix it. bootstrap: @test -n "$(IMAGE)" || { echo "IMAGE= is required; an installer carrying no control-plane image cannot raise a mesh"; exit 1; } + @case "$(IMAGE)" in sha256:*) echo "IMAGE=$(IMAGE) is an image id. The installer identifies the carried image by its tag, because an id is the digest of a configuration that a runtime rewrites as it loads. Pass a name:tag"; exit 1;; esac @docker image inspect "$(IMAGE)" >/dev/null 2>&1 || { echo "this machine does not hold $(IMAGE) — build it in mesh-control with 'make image'"; exit 1; } + @test -n "$$(docker image inspect --format '{{len .RepoTags}}' "$(IMAGE)" | grep -v '^0$$')" || { echo "$(IMAGE) has no repository tag, so the saved archive would carry no name the installer can ask a runtime about. Tag it first: docker tag $(IMAGE) mesh-control:"; exit 1; } @cp internal/image/control-plane.tar internal/image/control-plane.tar.placeholder @docker save --output internal/image/control-plane.tar "$(IMAGE)" @CGO_ENABLED=0 go build -ldflags="-s -w -X main.version=$(VERSION)" -o mesh-bootstrap ./cmd/mesh-bootstrap; \ diff --git a/internal/bootstrap/bootstrap.go b/internal/bootstrap/bootstrap.go index 1771aee..ae31339 100644 --- a/internal/bootstrap/bootstrap.go +++ b/internal/bootstrap/bootstrap.go @@ -20,6 +20,13 @@ // configuration, which is exact, unforgeable, and needs nothing to have served it (novox/hq // ADR 0006, and `internal/declaration`'s checkImage). Third-party images keep their upstream // `name@sha256:` references and are pulled from the internet like anything else. +// +// **And that id is read back from the machine, never predicted from the archive.** The digest of a +// configuration is not portable: a runtime rewrites the configuration as it loads, so the same +// bytes are held under a different name on the machine that receives them than on the one that +// saved them. The archive is identified by its TAG, which does survive the transfer, and the id +// the bundle names is whatever the runtime answers for that tag afterwards. See Load, which +// carries the measurement. package bootstrap import ( @@ -151,12 +158,22 @@ type Result struct { System string `json:"system"` DryRun bool `json:"dry-run,omitempty"` - // Image is the control plane's image id — what the produced bundle names it by. + // Image is what THIS MACHINE'S RUNTIME holds the control plane as, read back from it — and + // what the produced bundle names it by. Image string `json:"image,omitempty"` - // ImageTags is what that image was called when it was saved. Decoration, for a person. + // ImageArchive is what the carried archive calls the same image. Reported because it is + // routinely a DIFFERENT id: a runtime rewrites an image's configuration as it loads, and an id + // is that configuration's digest. Never what the bundle names. + ImageArchive string `json:"image-in-archive,omitempty"` + // ImageTag is the name the runtime was asked by, which is how the id above was obtained. + ImageTag string `json:"image-tag,omitempty"` + // ImageTags is everything the image was called when it was saved. ImageTags []string `json:"image-tags,omitempty"` // ImageHeld is true when the machine already held it and nothing was loaded. ImageHeld bool `json:"image-already-held,omitempty"` + // ImagePredicted is true when Image is the archive's id because nothing was loaded — a dry run + // only, and the reason a dry run does not claim to know what would be applied. + ImagePredicted bool `json:"image-id-is-a-prediction,omitempty"` // Bundle is where the produced bundle was written, and what was done to produce it. Bundle string `json:"bundle,omitempty"` @@ -276,6 +293,8 @@ func Run(ctx context.Context, o Options, d Deps, say func(string)) (Result, erro return result, failed(StepLoad, err) } result.Image, result.ImageTags, result.ImageHeld = loaded.ID, loaded.Tags, loaded.Held + result.ImageArchive, result.ImageTag = loaded.Archive, loaded.Tag + result.ImagePredicted = loaded.Predicted // ---- 3. bundle ---------------------------------------------------------------------- say("bundle — what this machine will be asked to be") @@ -301,6 +320,10 @@ func Run(ctx context.Context, o Options, d Deps, say func(string)) (Result, erro say(fmt.Sprintf(" its image %s — the template already named it, nothing rewritten", rewritten.Now)) } + if loaded.Predicted { + say(" UNCONFIRMED that id is the archive's and no runtime has been asked. A real " + + "run reads it back.") + } for _, kept := range rewritten.Kept { say(" left alone " + kept) } @@ -330,10 +353,27 @@ func Run(ctx context.Context, o Options, d Deps, say func(string)) (Result, erro } result.Stopped = "dry run: the bundle was produced and checked, and nothing was written, " + "loaded or applied" + if loaded.Predicted { + result.Stopped += ". The image id in it is the archive's own and is not necessarily " + + "the one this machine would hold — a runtime rewrites an image's configuration as " + + "it loads, and the id is that configuration's digest" + } say("\n" + result.Stopped) return result, nil } + // **Nothing predicted is ever written down.** The line above is the only path on which + // `loaded.ID` can be the archive's id, and it returns. Asserted here rather than left to the + // reader, because what would follow is a bundle naming an image this machine does not hold — + // and nothing serves an image named by the digest of its own configuration, so it would fail + // inside a pull that cannot succeed, three steps from the cause. + if loaded.Predicted { + return result, failed(StepBundle, fmt.Errorf( + "the control plane's image id was never confirmed against this machine's runtime, and "+ + "the bundle was about to be written with it. This is a fault in the installer, not "+ + "in the machine")) + } + if err := writeBundleFile(o.Out, rewritten.Bundle); err != nil { return result, failed(StepBundle, err) } diff --git a/internal/bootstrap/load.go b/internal/bootstrap/load.go index a44db70..4e1fc71 100644 --- a/internal/bootstrap/load.go +++ b/internal/bootstrap/load.go @@ -11,26 +11,54 @@ import ( // Loaded is the control plane's image on this machine. type Loaded struct { - // ID is what the bundle will name the image by: sha256 of its own configuration. + // ID is what THIS RUNTIME holds the image as, read back from it after the load. It is what the + // bundle names, and outside a dry run it is never a prediction — see Load. ID string - // Tags is what it was called when it was saved. For a person, never for the bundle. + // Archive is what the carried tar calls the same image. Kept because the two differ in + // practice, and a report showing only one of them cannot say that they did. Never what the + // bundle names. + Archive string + // Tag is the name the runtime is asked by. Load-bearing rather than decoration: it is the one + // name that survives `docker save` and `docker load` unchanged. + Tag string + // Tags is everything the archive was called when it was saved. Tags []string - // Held is true when the machine already had it and nothing was loaded. + // Held is true when the machine already held it and nothing moved. Held bool + // Predicted is true only on a dry run, where nothing was loaded and ID is therefore the + // archive's id — which is not necessarily the one this machine would end up with. + Predicted bool } -// Load puts the carried control-plane image into this machine's container runtime. +// Load puts the carried control-plane image into this machine's container runtime, and reports +// what the runtime decided to call it. // -// **Idempotent by asking first, which is possible because the id is a fact about the file.** The -// image id is read out of the saved tar (see `internal/image`.ID) before the runtime is asked -// anything, so this can ask "do you already hold exactly this image" — and on the second, third -// and tenth run of the installer the answer is yes and nothing is loaded. A load that scraped the -// id out of what `docker load` printed could only know that after loading, so it would load every -// time and report the same thing either way. +// **The digest of a configuration is not portable across runtimes, and that is why the id is read +// back rather than predicted.** An image id is the sha256 of the image's configuration document, +// and a runtime REWRITES that document as it loads: a newer Docker saves in one format, an older +// one stores it in another, and the same layers come out under a different name. Measured on a +// live raise, an image saved as `sha256:b86bb81c…` on a workstation was loaded as +// `sha256:2dc21904…` on the machine it was carried to. +// +// This code used to read the id out of the tar before the runtime was asked anything and use it +// for both idempotence and the bundle. That is right on the machine the image was built on and +// wrong on every machine it is carried to — which is every machine this program exists for. The +// bundle would have named an image the machine does not hold; nothing serves an image named by +// the digest of its own configuration, which is the whole point of naming one that way; and the +// apply would have stopped inside a pull that cannot succeed. The lab hit exactly this. +// +// **So the image is identified by its TAG.** A tag is ordinary metadata the tar carries through +// unchanged, and asking the runtime what a tag resolves to is asking the only party entitled to +// answer. The tag never reaches the bundle — a pinned bundle may not rely on one +// (novox/hq ADR 0006) — it is how the id is obtained, not what is written down. +// +// **Idempotence is decided from what the runtime holds.** The tag is asked before the load and +// again after: the same id either side means nothing moved, which is a fact about this machine +// rather than a guess about the file. A tag that already resolves means the image is already +// held, and nothing is loaded at all. // // It reads back (novox/hq ADR 0018). A load that reported success and left nothing there is a -// failure, not a convergence, and the apply would then meet a bundle naming an image the machine -// does not hold — which fails correctly but two steps too late. +// failure, not a convergence. func Load(ctx context.Context, run Runner, dryRun bool, say func(string)) (Loaded, error) { saved, err := image.Saved() if err != nil { @@ -42,22 +70,50 @@ func Load(ctx context.Context, run Runner, dryRun bool, say func(string)) (Loade // loadImage is Load with the carried bytes handed in, so the whole path can be tested against a // saved image a test builds rather than against whatever a particular build embedded. func loadImage(ctx context.Context, run Runner, saved []byte, dryRun bool, say func(string)) (Loaded, error) { - id, err := image.ID(saved) + archiveID, err := image.ArchiveID(saved) if err != nil { return Loaded{}, err } - loaded := Loaded{ID: id, Tags: image.Tags(saved)} + loaded := Loaded{Archive: archiveID, Tags: image.Tags(saved)} - if held, err := holdsImage(ctx, run, id); err != nil { + // **An untagged archive is a build-time fault, refused here rather than worked around.** + // Without a tag there is no portable name to ask the runtime about, and the only thing left is + // scraping the sentence `docker load` prints for a person — which differs between runtime + // versions and is exactly the kind of guess this whole step exists to stop making. The release + // target tags the image; an installer built without one was built wrong. + loaded.Tag = firstOr(loaded.Tags, "") + if loaded.Tag == "" { + return loaded, fmt.Errorf( + "the carried control-plane image has no tag, so there is no portable name to ask this "+ + "machine's runtime what id it gave it.\n"+ + "An image id is the digest of the image's configuration and a runtime rewrites "+ + "that as it loads, so the id in the archive (%s) is not necessarily the id this "+ + "machine will hold — and a bundle naming the wrong one names an image nothing "+ + "serves. Rebuild the installer with a tagged image: `make bootstrap "+ + "IMAGE=:`", archiveID) + } + + // Already held? Asked of the runtime, by the tag, before anything is written anywhere. + before, err := idOfImage(ctx, run, loaded.Tag) + if err != nil { return loaded, err - } else if held { - loaded.Held = true - say(" already held " + id + " — nothing loaded") + } + if before != "" { + loaded.ID, loaded.Held = before, true + say(" already held " + before + " as " + loaded.Tag + " — nothing loaded") + sayIfDifferent(say, archiveID, before) return loaded, nil } if dryRun { - say(fmt.Sprintf(" would load %s (%d bytes)", id, len(saved))) + // Nothing is loaded, so the runtime has not been asked to decide anything — and what it + // would decide cannot be worked out from here. Said, rather than quietly guessed at. + loaded.ID, loaded.Predicted = archiveID, true + say(fmt.Sprintf(" would load %s (%d bytes) as %s", archiveID, len(saved), loaded.Tag)) + say(" NOT THE FINAL ID a runtime rewrites an image's configuration as it loads, and an") + say(" id is that configuration's digest. The bundle names what the") + say(" runtime answers for " + loaded.Tag + " afterwards, which a dry run") + say(" cannot ask for without loading.") return loaded, nil } @@ -83,40 +139,61 @@ func loadImage(ctx context.Context, run Runner, saved []byte, dryRun bool, say f "the container runtime would not load the carried control-plane image: %w", err) } - // Read back. This is what makes "loaded" a fact rather than an intention. - held, err := holdsImage(ctx, run, id) + // Read back, and THIS is the answer the bundle is rewritten to. + after, err := idOfImage(ctx, run, loaded.Tag) if err != nil { return loaded, err } - if !held { + if after == "" { return loaded, fmt.Errorf( - "the load reported success and this machine does not hold %s.\n"+ - "The bundle names the control plane by that id and nothing serves it, so the "+ - "apply would refuse. Check what `docker load` actually took", id) + "the load reported success and this machine holds nothing called %s.\n"+ + "The bundle names the control plane by the id this runtime assigned, so there is "+ + "nothing to name. Check what `docker load` actually took", loaded.Tag) } - say(" loaded " + id) + loaded.ID = after + say(" loaded " + after + " as " + loaded.Tag) + sayIfDifferent(say, archiveID, after) return loaded, nil } -// holdsImage asks the runtime whether this exact image is present. +// sayIfDifferent reports the archive's own id when the runtime chose another. // -// It asks for the id back rather than reading the exit code, because an image inspected by id and -// an image inspected by a tag that happens to point somewhere else are the same successful -// command. What is wanted is "this one", and the answer says which one. -func holdsImage(ctx context.Context, run Runner, id string) (bool, error) { - out, err := run(ctx, "docker", "image", "inspect", "--format", "{{.Id}}", id) - if err != nil { - // Absent is an answer, not a failure. Every other reason the runtime might refuse looks - // the same from here — which is why preflight proves the runtime answers before this runs, - // rather than this trying to tell the two apart from an exit code. - return false, nil +// Said every time it happens, because it is surprising, it is ordinary, and somebody comparing +// this report against `docker images` on the machine the image was built on would otherwise +// conclude that the wrong image had been carried. +func sayIfDifferent(say func(string), archiveID, held string) { + if archiveID == held { + return } - got := strings.TrimSpace(out) - if got != id { - return false, fmt.Errorf( - "asked for image %s, the runtime answered %q. An image id is the digest of the "+ - "image's own configuration, so these are two different images and the bundle "+ - "would name the wrong one", id, got) + say(" the archive says " + archiveID) + say(" this runtime stored the same image under a different configuration, " + + "which is ordinary — the bundle names what the machine holds") +} + +// idOfImage asks the runtime what it holds under a name, or empty if it holds nothing. +// +// It asks for the id back rather than reading the exit code, because the id is what is wanted and +// an exit code is not it. Absent is an answer and not a failure: every other reason the runtime +// might refuse looks the same from here, which is why preflight proves the runtime answers before +// this runs rather than this trying to tell the two apart from an exit status. +// +// What it will not do is accept an answer that is not an image id. That answer becomes the name +// the bundle applies on a machine with no mesh to check anything against, so it is checked here +// where the refusal can say whose mistake it is. +func idOfImage(ctx context.Context, run Runner, name string) (string, error) { + out, err := run(ctx, "docker", "image", "inspect", "--format", "{{.Id}}", name) + if err != nil { + return "", nil } - return true, nil + got := strings.TrimSpace(firstLineOf(out)) + if got == "" { + return "", nil + } + if !isImageID(got) { + return "", fmt.Errorf( + "asked what this machine holds as %q, the runtime answered %q, which is not an image "+ + "id. The bundle would name the control plane by that answer, and it is refused "+ + "rather than written down", name, got) + } + return got, nil } diff --git a/internal/bootstrap/load_test.go b/internal/bootstrap/load_test.go index 0442875..b37fdc2 100644 --- a/internal/bootstrap/load_test.go +++ b/internal/bootstrap/load_test.go @@ -41,12 +41,17 @@ func (a *asked) ran(fragment string) bool { return false } -func savedImageFixture(t *testing.T, digest string) []byte { +// savedImageFixture builds what `docker save` produces, tagged `mesh-control:test` unless a test +// asks for something else. Pass no tags for an archive saved without one. +func savedImageFixture(t *testing.T, digest string, tags ...string) []byte { t.Helper() + if tags == nil { + tags = []string{"mesh-control:test"} + } entries, err := json.Marshal([]struct { Config string RepoTags []string - }{{Config: digest + ".json", RepoTags: []string{"mesh-control:test"}}}) + }{{Config: digest + ".json", RepoTags: tags}}) if err != nil { t.Fatal(err) } @@ -67,17 +72,83 @@ func savedImageFixture(t *testing.T, digest string) []byte { return buffer.Bytes() } -const fixtureDigest = "3333333333333333333333333333333333333333333333333333333333333333" +// fixtureDigest is what the ARCHIVE calls the image, and runtimeDigest is what a runtime calls it +// after loading the same bytes. They differ on purpose, because they differ in reality: an image +// id is the digest of the image's configuration, and a runtime rewrites that configuration as it +// loads. Measured on a live raise, `mesh-control:development` was `sha256:b86bb81c…` on the +// workstation that saved it and `sha256:2dc21904…` on the machine that loaded it. +const ( + fixtureDigest = "3333333333333333333333333333333333333333333333333333333333333333" + runtimeDigest = "4444444444444444444444444444444444444444444444444444444444444444" +) + +// **The id the bundle is named by comes from the RUNTIME, not from the archive.** +// +// This is the test for the fault that took the lab down. The installer used to read the id out of +// the carried tar and use it for the bundle, which is correct on the machine the image was built +// on and wrong on every machine it is carried to — and a bundle naming an id the machine does not +// hold names an image nothing can serve, because an image named by the digest of its own +// configuration is by definition served by nobody. The apply then stops inside a pull that cannot +// succeed, three steps from the cause. +// +// So the image is identified by its TAG, which survives save and load unchanged, and the runtime +// is asked what that tag resolves to. +func TestTheIdComesFromTheRuntimeAndNotFromTheArchive(t *testing.T) { + runtime := &asked{} + inspected := 0 + runtime.answer = func(_ string, args []string) (string, error) { + switch { + case len(args) > 1 && args[0] == "image" && args[1] == "inspect": + inspected++ + if inspected == 1 { + // Nothing held yet. + return "", errors.New("Error: No such image") + } + // Loaded — and stored under a configuration of the runtime's own making. + return "sha256:" + runtimeDigest + "\n", nil + case len(args) > 0 && args[0] == "load": + return "Loaded image: mesh-control:test\n", nil + } + return "", fmt.Errorf("unexpected command: %v", args) + } + + var said []string + loaded, err := loadImage(context.Background(), runtime.run, + savedImageFixture(t, fixtureDigest), false, func(line string) { said = append(said, line) }) + if err != nil { + t.Fatal(err) + } + if loaded.ID != "sha256:"+runtimeDigest { + t.Errorf("the bundle would name %q; this machine holds sha256:%s", loaded.ID, runtimeDigest) + } + if loaded.Archive != "sha256:"+fixtureDigest { + t.Errorf("the archive's own id is reported as %q", loaded.Archive) + } + if loaded.Predicted { + t.Error("a real run reported its id as a prediction") + } + // The runtime was asked BY THE TAG, which is the only name that survives the transfer. + if !runtime.ran("docker image inspect --format {{.Id}} mesh-control:test") { + t.Errorf("the runtime was never asked what the tag resolves to: %v", runtime.commands) + } + // And the difference is said out loud, or somebody comparing this against `docker images` on + // the build machine concludes the wrong image was carried. + if !strings.Contains(strings.Join(said, "\n"), "the archive says") { + t.Errorf("nothing was said about the two ids differing: %v", said) + } +} // A machine that already holds the image is not loaded again, and says so. // // This is the idempotence the installer's usefulness rests on: it is run over and over while // somebody gets a machine working, and a step that did its work again every time would be -// indistinguishable from one that had never run. +// indistinguishable from one that had never run. **Decided from what the runtime holds under the +// tag, not from what the archive predicts** — a predicted id cannot answer this question at all on +// a machine whose runtime rewrites configurations. func TestAnImageThisMachineAlreadyHoldsIsNotLoadedAgain(t *testing.T) { runtime := &asked{answer: func(_ string, args []string) (string, error) { if len(args) > 1 && args[0] == "image" && args[1] == "inspect" { - return "sha256:" + fixtureDigest + "\n", nil + return "sha256:" + runtimeDigest + "\n", nil } return "", fmt.Errorf("unexpected command: %v", args) }} @@ -92,6 +163,10 @@ func TestAnImageThisMachineAlreadyHoldsIsNotLoadedAgain(t *testing.T) { if !loaded.Held { t.Error("the machine already held the image and the load did not say so") } + if loaded.ID != "sha256:"+runtimeDigest { + t.Errorf("the id reported for an already-held image is %q, and the machine holds sha256:%s", + loaded.ID, runtimeDigest) + } if runtime.ran("docker load") { t.Errorf("the image was loaded again although the machine held it: %v", runtime.commands) } @@ -100,45 +175,6 @@ func TestAnImageThisMachineAlreadyHoldsIsNotLoadedAgain(t *testing.T) { } } -// The id comes out of the file, and the bundle is named by it. -// -// Not scraped from what `docker load` prints — that is a sentence for a person, which reads -// `Loaded image: name:tag` or `Loaded image ID: sha256:…` depending on how the image was saved. -// A program depending on which one a runtime chose would be depending on a runtime version. -func TestTheImageIdComesFromTheCarriedFileNotFromWhatTheRuntimeSays(t *testing.T) { - runtime := &asked{} - inspected := 0 - runtime.answer = func(_ string, args []string) (string, error) { - switch { - case len(args) > 1 && args[0] == "image" && args[1] == "inspect": - inspected++ - if inspected == 1 { - return "", errors.New("Error: No such image") - } - return "sha256:" + fixtureDigest + "\n", nil - case len(args) > 0 && args[0] == "load": - // Deliberately says something else entirely. The id must not come from here. - return "Loaded image: some-other-name:whatever\n", nil - } - return "", fmt.Errorf("unexpected command: %v", args) - } - - loaded, err := loadImage(context.Background(), runtime.run, - savedImageFixture(t, fixtureDigest), false, func(string) {}) - if err != nil { - t.Fatal(err) - } - if loaded.ID != "sha256:"+fixtureDigest { - t.Errorf("the image id is %q, want sha256:%s", loaded.ID, fixtureDigest) - } - if loaded.Held { - t.Error("an image that had to be loaded was reported as already held") - } - if !runtime.ran("docker load") { - t.Errorf("the image was never loaded: %v", runtime.commands) - } -} - // A load that reported success and left nothing there is a failure, not a convergence // (novox/hq ADR 0018). Without the read-back it would surface later as the host refusing a bundle // naming an image nothing serves — a true message about the wrong thing. @@ -155,14 +191,17 @@ func TestALoadThatLeftNothingBehindIsAFailure(t *testing.T) { if err == nil { t.Fatal("a load that left nothing on the machine was reported as success") } - if !strings.Contains(err.Error(), fixtureDigest) { - t.Errorf("the failure does not say which image is missing: %v", err) + // Named by the tag, because that is what was asked about and what is missing. + if !strings.Contains(err.Error(), "mesh-control:test") { + t.Errorf("the failure does not say what this machine holds nothing of: %v", err) } } -// A dry run changes nothing, and still knows the id — because the id is a property of the carried -// file. That is what lets `--dry-run` produce and check the real bundle rather than a guess. -func TestADryRunLearnsTheIdAndLoadsNothing(t *testing.T) { +// **A dry run cannot know the id, and says so rather than pretending.** It loads nothing, so no +// runtime has decided anything, and the id in the archive is a fact about a file rather than a +// prediction about this machine. `--dry-run` still produces and checks the bundle's shape; what it +// cannot promise is the one value that only a load can settle. +func TestADryRunLoadsNothingAndSaysTheIdIsUnconfirmed(t *testing.T) { runtime := &asked{answer: func(_ string, args []string) (string, error) { if len(args) > 1 && args[0] == "image" && args[1] == "inspect" { return "", errors.New("Error: No such image") @@ -170,16 +209,68 @@ func TestADryRunLearnsTheIdAndLoadsNothing(t *testing.T) { return "", fmt.Errorf("a dry run ran %v", args) }} + var said []string + loaded, err := loadImage(context.Background(), runtime.run, + savedImageFixture(t, fixtureDigest), true, func(line string) { said = append(said, line) }) + if err != nil { + t.Fatal(err) + } + if !loaded.Predicted { + t.Error("a dry run reported an id no runtime had confirmed as though it had been") + } + if loaded.ID != "sha256:"+fixtureDigest { + t.Errorf("a dry run reported %q, and the archive says sha256:%s", loaded.ID, fixtureDigest) + } + if runtime.ran("docker load") { + t.Errorf("a dry run loaded an image: %v", runtime.commands) + } + if !strings.Contains(strings.Join(said, "\n"), "NOT THE FINAL ID") { + t.Errorf("a dry run did not say its id is unconfirmed: %v", said) + } +} + +// A dry run on a machine that already holds the image DOES know the id, because the runtime was +// asked and answered. Reading is not changing, so a dry run is entitled to that. +func TestADryRunOnAMachineThatHoldsItKnowsTheRealId(t *testing.T) { + runtime := &asked{answer: func(_ string, args []string) (string, error) { + if len(args) > 1 && args[0] == "image" && args[1] == "inspect" { + return "sha256:" + runtimeDigest + "\n", nil + } + return "", fmt.Errorf("a dry run ran %v", args) + }} + loaded, err := loadImage(context.Background(), runtime.run, savedImageFixture(t, fixtureDigest), true, func(string) {}) if err != nil { t.Fatal(err) } - if loaded.ID != "sha256:"+fixtureDigest { - t.Errorf("a dry run did not work out the image id: %q", loaded.ID) + if loaded.Predicted { + t.Error("an id this machine's runtime supplied was reported as a prediction") } - if runtime.ran("docker load") { - t.Errorf("a dry run loaded an image: %v", runtime.commands) + if loaded.ID != "sha256:"+runtimeDigest { + t.Errorf("the id is %q, and the runtime said sha256:%s", loaded.ID, runtimeDigest) + } +} + +// **An untagged archive is a build-time fault, refused rather than worked around.** Without a tag +// there is no portable name to ask the runtime about, and the only thing left is scraping the +// sentence `docker load` prints for a person — which differs between runtime versions and is +// exactly the kind of guess this whole step exists to stop making. +func TestAnUntaggedArchiveIsRefused(t *testing.T) { + runtime := &asked{answer: func(_ string, args []string) (string, error) { + return "", fmt.Errorf("nothing should have been run: %v", args) + }} + + _, err := loadImage(context.Background(), runtime.run, + savedImageFixture(t, fixtureDigest, []string{}...), false, func(string) {}) + if err == nil { + t.Fatal("an archive with no tag was accepted, and there is no way to ask about it") + } + if !strings.Contains(err.Error(), "make bootstrap") { + t.Errorf("the refusal does not say how to build one that is tagged: %v", err) + } + if len(runtime.commands) != 0 { + t.Errorf("the machine was touched first: %v", runtime.commands) } } @@ -198,14 +289,25 @@ func TestAnInstallerCarryingNoImageSaysSoRatherThanRaisingHalfAMesh(t *testing.T } } -// Two different images cannot share an id, so an answer that is not the id asked about means the -// runtime is talking about something else. Reported rather than believed. -func TestARuntimeAnsweringAboutADifferentImageIsRefused(t *testing.T) { - runtime := &asked{answer: func(_ string, _ []string) (string, error) { - return "sha256:" + strings.Repeat("9", 64) + "\n", nil - }} - if _, err := loadImage(context.Background(), runtime.run, - savedImageFixture(t, fixtureDigest), false, func(string) {}); err == nil { - t.Fatal("the runtime answered about a different image and it was accepted") +// An answer that is not an image id is refused rather than written into a bundle. +// +// **This replaces a test that refused an answer differing from the archive's id.** That test +// encoded the mistake: a differing id is now the expected case, not a fault, because a runtime +// rewrites an image's configuration as it loads. What is still worth refusing is an answer that is +// not an id at all — that value becomes the name a bundle applies on a machine with no mesh to +// check anything against, so it is checked where the refusal can say whose mistake it is. +func TestARuntimeAnsweringSomethingThatIsNotAnImageIdIsRefused(t *testing.T) { + for _, nonsense := range []string{ + "mesh-control:test", + "sha256:" + strings.Repeat("9", 63), + "", + } { + runtime := &asked{answer: func(_ string, _ []string) (string, error) { + return nonsense + "\n", nil + }} + if _, err := loadImage(context.Background(), runtime.run, + savedImageFixture(t, fixtureDigest), false, func(string) {}); err == nil { + t.Errorf("the runtime answered %q and it was accepted as an image id", nonsense) + } } } diff --git a/internal/bootstrap/preflight.go b/internal/bootstrap/preflight.go index 476a530..e13e276 100644 --- a/internal/bootstrap/preflight.go +++ b/internal/bootstrap/preflight.go @@ -36,12 +36,27 @@ func Preflight(ctx context.Context, o Options, d Deps, say func(string)) ([]byte if err != nil { return nil, err } - carriedID, err := image.ID(saved) + // + // The id said here is the ARCHIVE's, and it is reported as such: it is a fact about the file + // and not about this machine. What this runtime will call the image once it holds it is the + // runtime's decision, made at the load, and asked for there (see Load). + carriedID, err := image.ArchiveID(saved) if err != nil { return nil, err } - say(fmt.Sprintf(" control plane %s carried (%s)", - firstOr(image.Tags(saved), "untagged"), carriedID)) + tag := firstOr(image.Tags(saved), "") + if tag == "" { + // Refused here as well as at the load, because preflight's whole job is to find at the + // start what would otherwise be found half way through — and this would be found after an + // image had been written to disk and handed to a container runtime. + return nil, fmt.Errorf( + "the carried control-plane image has no tag, and the installer identifies it by one: "+ + "an image id is the digest of a configuration that a runtime rewrites as it loads, "+ + "so the archive's id (%s) is not necessarily the id this machine would hold.\n"+ + "Rebuild the installer with a tagged image: `make bootstrap IMAGE=:`", + carriedID) + } + say(fmt.Sprintf(" control plane %s carried (the archive calls it %s)", tag, carriedID)) // 2. Is the template there, and is it a substrate? template, err := os.ReadFile(o.Template) diff --git a/internal/image/image.go b/internal/image/image.go index 5e94196..55d77c5 100644 --- a/internal/image/image.go +++ b/internal/image/image.go @@ -82,20 +82,33 @@ type manifestEntry struct { RepoTags []string `json:"RepoTags"` } -// ID is the image id the runtime will give this image once it is loaded, read out of the tar. +// ArchiveID is what this archive calls the image it holds. **It is not necessarily the id the +// runtime will assign when the archive is loaded, and it must never be what a bundle names.** // -// **Read here rather than parsed out of what `docker load` prints.** The load prints a sentence -// for a person — `Loaded image: name:tag` or `Loaded image ID: sha256:…`, depending on whether the -// image was saved with a tag — and a program that scraped it would be depending on which of those -// a particular runtime version chose. The id is a fact about the file, available before the -// runtime is asked anything, which is also what makes the load idempotent: the installer can ask -// whether the machine already holds THIS image before loading it. +// This was called ID, and its comment said it was the id the runtime would give the image once +// loaded. That was wrong, and wrong in the worst available way: right on the machine the image +// was saved on, and wrong on the machine it was carried to. An image id is the sha256 of the +// image's *configuration document*, and a runtime rewrites that document as it loads — a newer +// Docker saves in one format and an older one stores it in another. Same layers, same program, +// different name. Measured on a live raise: // -// An image id is the sha256 of the image's configuration document (novox/hq ADR 0006, and see -// `internal/declaration`'s checkImage). `manifest.json` names that document by its digest — as -// `<64hex>.json` in the older layout and `blobs/sha256/<64hex>` in the OCI one — so both forms -// reduce to the same sixty-four characters. -func ID(saved []byte) (string, error) { +// saved on the workstation sha256:b86bb81ca2f9691f24f4725f50962d1e49c98c5ffe211113241243d42d18ceea +// loaded on the machine sha256:2dc219046c73702fc640317f0342a28ec962ef1e9ef547b2f02861c508ca78fb +// +// A bundle naming this id would then name an image the machine does not hold — and nothing serves +// an image named by the digest of its own configuration, which is the whole point of naming one +// that way, so the apply would stop at a pull that cannot succeed. The id a bundle names is read +// back from the runtime after the load (`internal/bootstrap`.Load), which is the only place it is +// a fact rather than a prediction. +// +// What it is still good for is a statement about the FILE — which build somebody embedded — and +// as a hint printed beside the runtime's answer when the two differ, so a person can see that +// they did. +// +// `manifest.json` names the configuration document by its digest — as `<64hex>.json` in the older +// layout and `blobs/sha256/<64hex>` in the OCI one — so both forms reduce to the same sixty-four +// characters. +func ArchiveID(saved []byte) (string, error) { manifest, err := fileFromTar(saved, "manifest.json") if err != nil { return "", err @@ -125,11 +138,15 @@ func ID(saved []byte) (string, error) { return "sha256:" + digest, nil } -// Tags is what the saved image was called when it was saved, for a person reading a report. +// Tags is what the saved image was called when it was saved. // -// Decoration, and said so: the installer names the image by its id everywhere it matters, because -// a tag is exactly what a pinned bundle may not rely on (novox/hq ADR 0006). This is here so a -// report can say which build somebody embedded, which is otherwise sixty-four characters of hex. +// **Not decoration any more, and this comment used to say it was.** A tag is exactly what a pinned +// bundle may not rely on (novox/hq ADR 0006) and none of this ever reaches a bundle — but the tag +// is how the installer ASKS the runtime what id it assigned, because the id itself is not +// knowable beforehand (see ArchiveID). It is the one name that survives `docker save` and +// `docker load` unchanged, which is precisely what the id does not. +// +// It also still says which build somebody embedded, which is otherwise sixty-four hex characters. func Tags(saved []byte) []string { manifest, err := fileFromTar(saved, "manifest.json") if err != nil { diff --git a/internal/image/image_test.go b/internal/image/image_test.go index 74970db..ef93e6a 100644 --- a/internal/image/image_test.go +++ b/internal/image/image_test.go @@ -39,20 +39,25 @@ func manifest(t *testing.T, config string, tags ...string) string { return string(raw) } -// The image id is read from the FILE, before any runtime is asked anything. +// The archive's own id is read from the FILE, in both layouts `docker save` has used. // -// That is what makes the load idempotent: knowing the id in advance lets the installer ask "do you -// already hold exactly this" instead of loading and then finding out. Scraping it from what -// `docker load` prints would only be possible after loading, so the second run of an installer -// would load again every time and be unable to say it had not. -func TestTheImageIdIsReadFromTheSavedFile(t *testing.T) { +// **This test used to say that reading it here was what made the load idempotent — that knowing +// the id in advance let the installer ask "do you already hold exactly this". That was wrong.** An +// id is the digest of the image's configuration document, and a runtime rewrites that document as +// it loads, so this is a fact about the archive and not a prediction about any machine. What the +// installer asks a runtime by is the TAG, and what a bundle names is the answer the runtime gives +// back (`internal/bootstrap`.Load). +// +// It is still read and still checked, because it is what says which build somebody embedded — and +// because printing it beside the runtime's answer is how a person sees that the two differ. +func TestTheArchivesOwnIdIsReadFromTheSavedFile(t *testing.T) { digest := strings.Repeat("a", 64) // Both layouts `docker save` has used. The older one names the config `.json`; the OCI // one names it `blobs/sha256/`. They carry the same sixty-four characters, and a // reader that understood only one would work until somebody upgraded their runtime. for _, config := range []string{digest + ".json", "blobs/sha256/" + digest} { saved := savedImage(t, map[string]string{"manifest.json": manifest(t, config)}) - id, err := ID(saved) + id, err := ArchiveID(saved) if err != nil { t.Fatalf("config %q: %v", config, err) } @@ -62,7 +67,10 @@ func TestTheImageIdIsReadFromTheSavedFile(t *testing.T) { } } -func TestTheSavedTagsAreReadForAPersonToRecognise(t *testing.T) { +// The tag is read, and it is not decoration: it is the name the installer asks a runtime by, +// because it is the one thing that survives `docker save` and `docker load` unchanged. The id +// does not. +func TestTheSavedTagsAreRead(t *testing.T) { saved := savedImage(t, map[string]string{ "manifest.json": manifest(t, strings.Repeat("b", 64)+".json", "mesh-control:v1"), }) @@ -74,18 +82,18 @@ func TestTheSavedTagsAreReadForAPersonToRecognise(t *testing.T) { // A tar that is not a saved image is refused with what is wrong, not with a nil id. // -// The installer names the control plane by this id in the bundle it writes. An id it could not -// read, treated as empty, would produce a bundle naming nothing — refused by the host two steps -// later, with a message about a declaration rather than about what somebody embedded. +// Nothing here reaches a bundle any more, but this is still the earliest moment somebody can be +// told they embedded the wrong file — and the alternative is finding out at the load, from a +// container runtime, in a sentence about a tar rather than about what was built. func TestSomethingThatIsNotASavedImageIsRefused(t *testing.T) { notAnImage := savedImage(t, map[string]string{"hello": "world"}) - if _, err := ID(notAnImage); err == nil { + if _, err := ArchiveID(notAnImage); err == nil { t.Error("a tar with no manifest.json was accepted as a saved image") } else if !strings.Contains(err.Error(), "docker save") { t.Errorf("the refusal does not say what to embed instead: %v", err) } - if _, err := ID([]byte("this is not a tar at all")); err == nil { + if _, err := ArchiveID([]byte("this is not a tar at all")); err == nil { t.Error("bytes that are not a tar were accepted") } } @@ -101,7 +109,7 @@ func TestATarHoldingSeveralImagesIsRefused(t *testing.T) { t.Fatal(err) } saved := savedImage(t, map[string]string{"manifest.json": string(entries)}) - if _, err := ID(saved); err == nil { + if _, err := ArchiveID(saved); err == nil { t.Error("a tar holding two images was accepted") } } @@ -112,7 +120,7 @@ func TestATarHoldingSeveralImagesIsRefused(t *testing.T) { func TestAConfigThatIsNotADigestIsRefused(t *testing.T) { for _, config := range []string{"config.json", "abc.json", "blobs/sha256/" + strings.Repeat("a", 63)} { saved := savedImage(t, map[string]string{"manifest.json": manifest(t, config)}) - if _, err := ID(saved); err == nil { + if _, err := ArchiveID(saved); err == nil { t.Errorf("config %q was accepted and is not a digest", config) } }