diff --git a/internal/apply/apply.go b/internal/apply/apply.go index ba07b42..586fa07 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -52,6 +52,9 @@ type Outcome struct { wrote string // into is what a file written into held before the mesh's keys (novox/hq ADR 0102). into *store.Into + // reads is, for a container, the digest of each file it was created reading, by path — so + // the next apply can say which one changed (novox/hq 04-ISSUES/103). + reads map[string]string } // Report is what an apply did, in the order it did it. @@ -266,9 +269,16 @@ func ApplyKeeping( // machine reports success, and what is inside is using a credential the mesh has replaced // (novox/hq 04-ISSUES/045). Folding these into the container's spec makes the comparison a // standing one instead. - declares := map[string]string{} + // + // And what each file a container reads at creation holds — its env-files and what is mounted + // into it — by the digest this host recorded when it wrote the file, read from `known` as it + // stands when the container is reached, so a file rewritten earlier in this same apply is + // already the new one (novox/hq 04-ISSUES/103). That needs the file applied before the + // container, which is the declared order; a container declared ahead of its file sees the + // change one apply late, and never misses it. + in := inputs{declares: map[string]string{}, known: &known} for _, resource := range d.Resources { - declares[resource.Identity()] = declaredDigest(resource) + in.declares[resource.Identity()] = declaredDigest(resource) } // Everything is attempted, and every failure is reported. @@ -340,7 +350,7 @@ func ApplyKeeping( !known.Recorded(string(declaration.TypeFile), f.Path) { keepFound = keep } - outcome, err = applyOne(ctx, sys, resource, run, changed, declares, was, unseal, keepFound) + outcome, err = applyOne(ctx, sys, resource, run, changed, in, was, unseal, keepFound) } if err != nil { failed := &Error{Resource: resource.Identity(), Err: err, Done: report} @@ -386,6 +396,7 @@ func ApplyKeeping( Target: outcome.Target, AppliedAt: time.Now().UTC(), Wrote: outcome.wrote, Into: outcome.into, + Reads: outcome.reads, Holds: holds(resource), }) // Its module has been taken, and what was held for it is now the mesh's. @@ -396,7 +407,14 @@ func ApplyKeeping( report.Outcomes = append(report.Outcomes, outcome) if outcome.Action != "unchanged" { changed[resource.Identity()] = true - log(fmt.Sprintf(" %s %s (%s)", outcome.Action, outcome.ID, outcome.Target)) + // With the detail, when there is one: "updated app" says a container was replaced; + // which file made that happen is what somebody reading the log at the time needs + // (novox/hq 04-ISSUES/103). + line := fmt.Sprintf(" %s %s (%s)", outcome.Action, outcome.ID, outcome.Target) + if outcome.Detail != "" { + line += ": " + outcome.Detail + } + log(line) } } @@ -440,7 +458,7 @@ const guardPrefix = declaration.AdoptionPrefix + "guard" type Unseal func(sealed string) ([]byte, error) func applyOne(ctx context.Context, sys system.System, r declaration.Resource, run Runner, - changed map[string]bool, declares map[string]string, previous store.Applied, + changed map[string]bool, in inputs, previous store.Applied, unseal Unseal, keepFound Keep) (Outcome, error) { switch res := r.(type) { case *declaration.Directory: @@ -452,7 +470,7 @@ func applyOne(ctx context.Context, sys system.System, r declaration.Resource, ru case *declaration.Package: return applyPackage(ctx, sys, res, run) case *declaration.Container: - return applyContainer(ctx, res, run, changed, declares, previous) + return applyContainer(ctx, res, run, changed, in, previous) case *declaration.User: return applyUser(ctx, sys, res, run) case *declaration.Archive: @@ -1129,9 +1147,96 @@ const ( idLabel = "mesh-host.id" ) +// inputs is what a container takes in when it is created beyond its own declaration: what each +// resource it names under restart-on currently declares, and what the files it reads hold. +type inputs struct { + // declares is each resource's declared digest, by id (declaredDigest). + declares map[string]string + // known is the node's state as it stands when the container is reached — so a file applied + // earlier in the same pass is already its new self. Nil where nothing was written: a test, + // or a scheduled fire, which reads no file at creation. + known *store.State +} + +// fileDigest is what a file the container reads holds, by digest. +// +// **What this host wrote when it has a record of writing it, and what is on disk when it has +// not.** The record is preferred because it is what the host means by the file: a seed created +// once digests as the seed, not as whatever the service has grown in it, and a file written into +// digests as the mesh's keys, not the machine's (novox/hq ADR 0102). A file the host never wrote +// — an env-file a predecessor left, the superuser secret genesis writes before any declaration +// names it — is read, so the digest is the same one the host records when it later writes the +// same bytes there, and adopting a running store in place stays a reconcile rather than a +// recreate (bootstrap phase three; genesis writes the value with no line ending for exactly this +// reason). Empty when there is nothing readable there: the runtime refuses an absent env-file +// itself, with a better message than this could give. +func (in inputs) fileDigest(path string) string { + if in.known != nil { + if f, recorded := in.known.At(string(declaration.TypeFile), path); recorded { + return f.Wrote + } + } + info, err := os.Stat(path) + if err != nil || !info.Mode().IsRegular() { + return "" + } + content, err := os.ReadFile(path) + if err != nil { + return "" + } + return digestOf(string(content)) +} + +// reads is every file a running container takes in when it is created, by path and digest +// (novox/hq 04-ISSUES/103). +// +// - every env-file: the runtime reads it once, at create, and `docker restart` hands the +// container the same environment it had. +// - a file bind-mounted into it, DIRECTLY, by its content: a secret, a credential file. +// +// **A directory bind-mounted into it is not looked inside**, not even for the files this host +// wrote there. What a service reads out of a mounted directory, and when, is the service's +// business: the route proxy re-reads its routes file live and would be recreated on every route +// change; a provisioner sidecar polls what it receives every few seconds and would be killed +// mid-reconcile on every grant. A module whose container does read such a file once, at start, +// says so with restart-on — that is what the field is for, and it stays the opt-in. +// +// A step is not here either: a run-once or scheduled container reads its files when it runs, and +// runs fresh each time. Only a container that stays running holds what it read. +func (in inputs) reads(r *declaration.Container) map[string]string { + if r.RunOnce || r.Schedule != "" { + return nil + } + out := map[string]string{} + for _, path := range r.EnvFile { + out[path] = in.fileDigest(path) + } + for _, v := range r.Volumes { + src := mountSource(v) + if !strings.HasPrefix(src, "/") { + continue // a named volume: the runtime's, holding data + } + // fileDigest is empty for a directory, and for anything else that is not a regular file. + if digest := in.fileDigest(src); digest != "" { + out[src] = digest + } + } + if len(out) == 0 { + return nil + } + return out +} + // containerSpec is the identity of a declared container: everything that, if changed, means // the running container is no longer what was asked for. -func containerSpec(r *declaration.Container, declares map[string]string) string { +func containerSpec(r *declaration.Container, in inputs) string { + return containerSpecReading(r, in.declares, in.reads(r)) +} + +// containerSpecReading is containerSpec with what the container reads already read — so an +// applier that also records those digests reads each file once, and the label and the record +// cannot disagree about a file that moved between two reads. +func containerSpecReading(r *declaration.Container, declares, reads map[string]string) string { keys := make([]string, 0, len(r.Env)) for k := range r.Env { keys = append(keys, k) @@ -1171,6 +1276,15 @@ func containerSpec(r *declaration.Container, declares map[string]string) string for _, id := range depends { b.WriteString("reads " + id + "=" + declares[id] + "\n") } + // And what the files it reads at creation hold — not only their paths, which `volume` and the + // env-file arguments already name. The spec named the env-file's path and not its content, + // so the host rewrote two environment files with the store's new port and left both + // containers running with the old one, healthy-looking, until they answered 502 (novox/hq + // 04-ISSUES/103). Added only when there is something read, so a container that reads nothing + // keeps the digest it had. + for _, path := range sortedKeys(reads) { + b.WriteString("file " + path + "=" + reads[path] + "\n") + } return fmt.Sprintf("%x", sha256.Sum256([]byte(b.String()))) } @@ -1236,9 +1350,12 @@ func applyNetwork(ctx context.Context, r *declaration.Network, run Runner) (Outc } func applyContainer(ctx context.Context, r *declaration.Container, run Runner, - changed map[string]bool, declares map[string]string, previous store.Applied) (Outcome, error) { + changed map[string]bool, in inputs, previous store.Applied) (Outcome, error) { out := begin(r) - want := containerSpec(r, declares) + // Read once, so the spec and the record agree on what was read even if a file moves under them. + reads := in.reads(r) + want := containerSpecReading(r, in.declares, reads) + out.reads = reads cri, err := containerRuntime(ctx, run) if err != nil { @@ -1279,8 +1396,42 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner, // already has. reasons := restartedBy(r.RestartOn, changed) + // What this container was created reading, as last recorded. By its declared id first, and by + // its NAME when that id has no record: the bundle's `store` becomes the postgres module's + // `postgres.server`, the same container under a new id, and a file change on the day it is + // adopted is a real change with a real record — under the old id. + wasReading := previous.Reads + if previous.ID == "" && in.known != nil { + if byName, ok := in.known.At(string(declaration.TypeContainer), r.Name); ok { + wasReading = byName.Reads + } + } + + // Which of the files it reads no longer hold what it was created reading. The spec label says + // only that SOMETHING moved; the record of what was read says what — and that is the line a + // person needs when a service went stale without a word (novox/hq 04-ISSUES/103). + var changedFiles []string + for _, path := range sortedKeys(wasReading) { + if now, still := reads[path]; still && now != wasReading[path] { + changedFiles = append(changedFiles, path) + } + } + + // **A container labelled before the host folded in what it reads is accepted, not recreated.** + // + // Its label is the spec without the file lines. Recreating every such container on the first + // apply after the host upgraded would be a restart storm across the mesh in declaration order — + // the store first, under everything that uses it. So a label that matches the spec as it used + // to be computed is taken as current: what it reads is recorded now, and from the next apply + // on a changed file is caught by that record; the label itself is rewritten at the next + // genuine recreate. The trade-off, stated: a container that was ALREADY stale when the host + // upgraded — created against a file that has since changed — is not caught by this, and could + // not be by the alternative either, which recreates it without knowing whether it needed to. + legacy := len(reads) > 0 && before.Spec == containerSpecReading(r, in.declares, nil) && + (wasReading == nil || sameReads(wasReading, reads)) + switch { - case existed && before.Spec == want && before.Running && len(reasons) == 0: + case existed && (before.Spec == want || legacy) && before.Running && len(reasons) == 0: out.Action = "unchanged" return out, nil case existed: @@ -1340,9 +1491,15 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner, out.Action = "created" if existed { out.Action = "updated" - if len(reasons) > 0 { + switch { + case len(changedFiles) > 0: + out.Detail = "recreated: " + strings.Join(changedFiles, ", ") + " changed" + if len(reasons) > 0 { + out.Detail += "; and to pick up " + strings.Join(reasons, ", ") + } + case len(reasons) > 0: out.Detail = "recreated to pick up " + strings.Join(reasons, ", ") - } else { + default: out.Detail = "replaced; a container's configuration is fixed when it is created" } } @@ -1510,6 +1667,20 @@ func restartedBy(restartOn []string, changed map[string]bool) []string { return which } +// sameReads is whether two records of what a container reads name the same files holding the +// same content — a file added, dropped or changed makes them differ. +func sameReads(a, b map[string]string) bool { + if len(a) != len(b) { + return false + } + for path, digest := range a { + if b[path] != digest { + return false + } + } + return true +} + func sortedKeys(m map[string]string) []string { keys := make([]string, 0, len(m)) for k := range m { diff --git a/internal/apply/apply_test.go b/internal/apply/apply_test.go index ba5c5aa..fea616b 100644 --- a/internal/apply/apply_test.go +++ b/internal/apply/apply_test.go @@ -666,7 +666,7 @@ func TestAContainerWhoseDeclarationChangedIsReplaced(t *testing.T) { d := parseTrusted(t, `{"declaration":1,"resources":[ {"id":"store","type":"container","name":"store","image":"`+pinned+`","env":{"PGDATA":"/data"}} ]}`) - want := containerSpec(d.Resources[0].(*declaration.Container), nil) + want := containerSpec(d.Resources[0].(*declaration.Container), inputs{}) var removed, created bool run := func(ctx context.Context, name string, args ...string) (string, error) { @@ -704,7 +704,7 @@ func TestAContainerThatMatchesIsLeftAlone(t *testing.T) { d := parseTrusted(t, `{"declaration":1,"resources":[ {"id":"store","type":"container","name":"store","image":"`+pinned+`","env":{"PGDATA":"/data"}} ]}`) - spec := containerSpec(d.Resources[0].(*declaration.Container), nil) + spec := containerSpec(d.Resources[0].(*declaration.Container), inputs{}) var touched bool run := func(ctx context.Context, name string, args ...string) (string, error) { @@ -748,8 +748,8 @@ func TestAContainerIsRecreatedWhenARestartOnResourceChanged(t *testing.T) { // what a container reads is part of what it is, so the old content yields a different spec. was := map[string]string{"config": declaredDigest(&declaration.File{Content: "{\"token\":\"old\"}\n"})} now := map[string]string{"config": declaredDigest(d.Resources[0].(*declaration.File))} - stale := containerSpec(d.Resources[1].(*declaration.Container), was) - fresh := containerSpec(d.Resources[1].(*declaration.Container), now) + stale := containerSpec(d.Resources[1].(*declaration.Container), inputs{declares: was}) + fresh := containerSpec(d.Resources[1].(*declaration.Container), inputs{declares: now}) var removed, created bool run := func(ctx context.Context, name string, args ...string) (string, error) { @@ -1538,8 +1538,8 @@ func TestAContainerStaleFromAnEarlierApplyIsReplaced(t *testing.T) { // The container was created when the file said something else. was := map[string]string{"env": declaredDigest(&declaration.File{Content: "PASSWORD=old\n"})} now := map[string]string{"env": declaredDigest(d.Resources[0].(*declaration.File))} - stale := containerSpec(d.Resources[1].(*declaration.Container), was) - fresh := containerSpec(d.Resources[1].(*declaration.Container), now) + stale := containerSpec(d.Resources[1].(*declaration.Container), inputs{declares: was}) + fresh := containerSpec(d.Resources[1].(*declaration.Container), inputs{declares: now}) var removed, created bool run := func(ctx context.Context, name string, args ...string) (string, error) { diff --git a/internal/apply/plan.go b/internal/apply/plan.go index ced96cc..07205bd 100644 --- a/internal/apply/plan.go +++ b/internal/apply/plan.go @@ -191,10 +191,68 @@ func planned(r declaration.Resource, d *declaration.Declaration, known store.Sta step.Verb, step.Why = "update", "the declaration changed since this host applied it" return step } + if c, ok := r.(*declaration.Container); ok { + if changed := readsChanged(c, d, known, was.Reads); len(changed) > 0 { + step.Verb = "update" + step.Why = "recreated: " + strings.Join(changed, ", ") + " changed since it was created" + return step + } + } step.Verb, step.Why = "check", "recorded here; corrected if this machine drifted from it" return step } +// readsChanged is which of the files a container was created reading the apply will hand it +// changed — the same comparison applyContainer makes (novox/hq 04-ISSUES/103), settled from the +// declaration and the record alone. +// +// A file's digest is what this apply will record for it: a plain file declared here, by its +// declared content; otherwise what this host last wrote there, under any id. A file neither +// declares nor records — an env-file a predecessor left — is read by the apply from the machine, +// which a plan does not do, so it stays a check. A container with no record of what it read was +// labelled before the host kept that record and is accepted as it is, so it is a check too. +func readsChanged(c *declaration.Container, d *declaration.Declaration, known store.State, + wasReading map[string]string) []string { + if len(wasReading) == 0 { + return nil + } + willWrite := map[string]string{} + for _, r := range d.Resources { + if f, ok := r.(*declaration.File); ok { + if want := wouldWrite(f); want != "" { + willWrite[f.Path] = want + } + } + } + // Only what it still reads: a file it was created reading and no longer names is a changed + // declaration, not a changed file. + stillReads := map[string]bool{} + for _, path := range c.EnvFile { + stillReads[path] = true + } + for _, v := range c.Volumes { + if src := mountSource(v); strings.HasPrefix(src, "/") { + stillReads[src] = true + } + } + var changed []string + for _, path := range sortedKeys(wasReading) { + if !stillReads[path] { + continue + } + now, settled := willWrite[path] + if !settled { + if f, recorded := known.At(string(declaration.TypeFile), path); recorded { + now, settled = f.Wrote, true + } + } + if settled && now != wasReading[path] { + changed = append(changed, path) + } + } + return changed +} + // wouldWrite is the digest a plain file would be recorded under, or empty where only the apply // can know: a sealed file, one with secrets in it, one written into, one carrying bytes. func wouldWrite(r declaration.Resource) string { diff --git a/internal/apply/plan_test.go b/internal/apply/plan_test.go index 768230a..4f8f7ce 100644 --- a/internal/apply/plan_test.go +++ b/internal/apply/plan_test.go @@ -225,3 +225,39 @@ func TestAResourceRunInAHeldContainerIsPlannedAsItIsApplied(t *testing.T) { t.Errorf("planned %q", got) } } + +func TestAPlanSaysAContainerIsRecreatedWhenAFileItReadsChanged(t *testing.T) { + // The apply recreates a container when the content of a file it reads at creation changed + // (novox/hq 04-ISSUES/103); the plan says so from the record alone — what the container was + // created reading, against what this apply will write. And a container recorded before the + // host kept that record is accepted, so it is a check, not an update. + dir := t.TempDir() + env := filepath.Join(dir, "forge.env") + declare := func(port string) *declaration.Declaration { + return trusted(t, `{"declaration":1,"resources":[ + {"id":"forge.env","type":"file","path":"`+env+`","content":"DATABASE_PORT=`+port+`\n"}, + {"id":"forge.server","type":"container","name":"forge","image":"`+pinned+`","env-file":["`+env+`"]}]}`) + } + created := digestOf("DATABASE_PORT=5432\n") + known := store.State{} + known.Record(store.Applied{ID: "forge.env", Type: "file", Target: env, Wrote: created}) + known.Record(store.Applied{ID: "forge.server", Type: "container", Target: "forge", + Reads: map[string]string{env: created}}) + + if got := verbs(Plan(declare("5432"), known, store.OriginCarried)); got != "check forge.env, check forge.server" { + t.Errorf("nothing changed and the plan says %q", got) + } + steps := Plan(declare("5433"), known, store.OriginCarried) + if got := verbs(steps); got != "update forge.env, update forge.server" { + t.Fatalf("the env-file changes and the plan says %q", got) + } + if !strings.Contains(steps[1].Why, env+" changed") { + t.Errorf("the plan does not say which file: %+v", steps[1]) + } + + // No record of what it read: labelled by an earlier host, accepted as it is. + known.Record(store.Applied{ID: "forge.server", Type: "container", Target: "forge"}) + if got := verbs(Plan(declare("5433"), known, store.OriginCarried)); got != "update forge.env, check forge.server" { + t.Errorf("a container with no record of what it read is planned as %q", got) + } +} diff --git a/internal/apply/reads_test.go b/internal/apply/reads_test.go new file mode 100644 index 0000000..be2d8fe --- /dev/null +++ b/internal/apply/reads_test.go @@ -0,0 +1,316 @@ +package apply + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/novox/mesh-host/internal/declaration" + "github.com/novox/mesh-host/internal/store" +) + +// Defends novox/hq 04-ISSUES/103: a container is recreated when the CONTENT of a file it reads at +// creation changes, not only when its path does. A container takes its env-file and its mounted +// files in once, when it is created; `docker restart` hands it the same environment again, so +// only a recreate carries a rewritten file into the process. +// +// The runtime here is the `machine` fake: it keeps the spec label the host gave a container and +// hands it back on inspect, so the comparison under test is the one the host really makes, +// against what it really wrote — not against a spec a test imagined. + +func applyCarried(t *testing.T, d *declaration.Declaration, known store.State, m *machine, + log func(string)) (Report, store.State) { + t.Helper() + report, state, err := Apply(context.Background(), archHost(t), d, known, store.OriginCarried, m.run, log, nil) + if err != nil { + t.Fatalf("apply failed: %v", err) + } + return report, state +} + +func TestAContainerIsRecreatedWhenItsEnvFileChanged(t *testing.T) { + // The night of the issue: the store was given a new port, the host rewrote the forge's + // environment file with it — and left the forge running with the old one. + dir := t.TempDir() + env := filepath.Join(dir, "forge.env") + declare := func(port string) *declaration.Declaration { + return parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"forge.env","type":"file","path":"`+env+`","content":"DATABASE_PORT=`+port+`\n","mode":"0600"}, + {"id":"forge.server","type":"container","name":"forge","image":"`+pinned+`","env-file":["`+env+`"]} + ]}`) + } + m := &machine{containers: map[string]*fakeContainer{}} + var logged []string + log := func(line string) { logged = append(logged, line) } + + report, state := applyCarried(t, declare("5432"), store.State{}, m, log) + if o := outcomeOf(report, "forge.server"); o.Action != "created" { + t.Fatalf("the container was not created: %+v", report.Outcomes) + } + + // The store moved. The file is rewritten in this apply, before the container is reached, and + // the container must follow it in the same pass. + m.asked, logged = nil, nil + report, state = applyCarried(t, declare("5433"), state, m, log) + if !m.removed("forge") || !m.did("docker run") { + t.Fatalf("the container kept running with the old environment after its env-file changed: %v", m.asked) + } + o := outcomeOf(report, "forge.server") + if o.Action != "updated" || o.Detail != "recreated: "+env+" changed" { + t.Errorf("the recreate did not say which file changed: %+v", o) + } + var said bool + for _, line := range logged { + if strings.Contains(line, "updated forge.server") && strings.Contains(line, "recreated: "+env+" changed") { + said = true + } + } + if !said { + t.Errorf("the log did not say which file made the container recreate: %q", logged) + } + + // And with nothing moved, it is left alone: content is part of the identity, not a tripwire. + m.asked = nil + report, _ = applyCarried(t, declare("5433"), state, m, log) + if m.did("docker rm") || m.did("docker run") || report.Changed() { + t.Errorf("a container whose env-file did not change was recreated: %v %+v", m.asked, report.Outcomes) + } +} + +func TestAnEnvFileTheHostDidNotWriteIsStillReadForWhatItHolds(t *testing.T) { + // The host has no record of this file — a predecessor left it, or something else on the + // machine maintains it — and the container still reads it once. Its content is read from the + // disk, so a change is a recreate exactly as for a file the host wrote. + dir := t.TempDir() + env := filepath.Join(dir, "app.env") + if err := os.WriteFile(env, []byte("TOKEN=old\n"), 0o600); err != nil { + t.Fatal(err) + } + d := parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"app.server","type":"container","name":"app","image":"`+pinned+`","env-file":["`+env+`"]} + ]}`) + m := &machine{containers: map[string]*fakeContainer{}} + _, state := applyCarried(t, d, store.State{}, m, nil) + + if err := os.WriteFile(env, []byte("TOKEN=new\n"), 0o600); err != nil { + t.Fatal(err) + } + m.asked = nil + report, _ := applyCarried(t, d, state, m, nil) + if !m.removed("app") || !m.did("docker run") { + t.Fatalf("a container reading an env-file the host did not write was not recreated when it changed: %v", m.asked) + } + if o := outcomeOf(report, "app.server"); o.Detail != "recreated: "+env+" changed" { + t.Errorf("the recreate did not name the file: %+v", o) + } +} + +func TestAContainerIsRecreatedWhenAMountedSecretChanged(t *testing.T) { + // A rotated credential has the same shape as a moved port: the host writes the file the + // container mounts, and the process holds the value it was created with. + dir := t.TempDir() + secret := filepath.Join(dir, "db.secret") + declare := func(value string) *declaration.Declaration { + return parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"app.secret","type":"file","path":"`+secret+`","content":"`+value+`","mode":"0600"}, + {"id":"app.server","type":"container","name":"app","image":"`+pinned+`", + "volumes":["`+secret+`:/run/secrets/db:ro"]} + ]}`) + } + m := &machine{containers: map[string]*fakeContainer{}} + _, state := applyCarried(t, declare("hunter2"), store.State{}, m, nil) + + m.asked = nil + report, _ := applyCarried(t, declare("correct-horse-battery-staple"), state, m, nil) + if !m.removed("app") || !m.did("docker run") { + t.Fatalf("the container kept the secret it was created with after the mounted file changed: %v", m.asked) + } + if o := outcomeOf(report, "app.server"); o.Action != "updated" || o.Detail != "recreated: "+secret+" changed" { + t.Errorf("the recreate did not say which file changed: %+v", o) + } +} + +func TestAMountedDirectoryIsNotLookedInside(t *testing.T) { + // A bind-mounted directory is not part of what a container is — not the data the service + // grows in it, and not the files the host itself writes there either. Whether a service reads + // a file under its directory once at start or watches it live is the service's business: the + // route proxy re-reads its routes live, a provisioner sidecar polls what it receives every few + // seconds, and recreating either for a file the host rewrote would kill them for nothing. A + // module whose container does read such a file once says so with restart-on, which stays the + // opt-in. + dir := t.TempDir() + state := filepath.Join(dir, "state") + config := filepath.Join(state, "config.toml") + declare := func(level, restartOn string) *declaration.Declaration { + return parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"app.state","type":"directory","path":"`+state+`"}, + {"id":"app.config","type":"file","path":"`+config+`","content":"level = \"`+level+`\"\n"}, + {"id":"app.server","type":"container","name":"app","image":"`+pinned+`", + "volumes":["`+state+`:/var/lib/app"]`+restartOn+`} + ]}`) + } + m := &machine{containers: map[string]*fakeContainer{}} + _, known := applyCarried(t, declare("info", ""), store.State{}, m, nil) + + // The service grows its data in the directory it was given. + if err := os.WriteFile(filepath.Join(state, "app.db"), []byte("rows"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(state, "cache"), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(state, "cache", "index"), []byte("entries"), 0o600); err != nil { + t.Fatal(err) + } + m.asked = nil + report, known := applyCarried(t, declare("info", ""), known, m, nil) + if m.did("docker rm") || m.did("docker run") || report.Changed() { + t.Errorf("a container was recreated for data its service wrote in a mounted directory: %v %+v", + m.asked, report.Outcomes) + } + + // The host rewrites its own file under the same directory: still not a reason. The container + // did not name it. + m.asked = nil + report, known = applyCarried(t, declare("debug", ""), known, m, nil) + if m.did("docker rm") || m.did("docker run") { + t.Errorf("a container was recreated for a file under a mounted directory it did not name: %v", m.asked) + } + if o := outcomeOf(report, "app.config"); o.Action != "updated" { + t.Fatalf("the config was not rewritten: %+v", o) + } + + // Naming it is what makes it a reason, as before this change. + m.asked = nil + report, _ = applyCarried(t, declare("trace", `,"restart-on":["app.config"]`), known, m, nil) + if !m.removed("app") || !m.did("docker run") { + t.Fatalf("a container naming a rewritten file under its mount was not recreated: %v", m.asked) + } + if o := outcomeOf(report, "app.server"); !strings.Contains(o.Detail, "app.config") { + t.Errorf("the recreate did not name why: %+v", o) + } +} + +func TestAContainerLabelledBeforeTheHostReadItsFilesIsAcceptedNotRecreated(t *testing.T) { + // The first apply after the host upgrades finds every container carrying a label computed + // without the file lines. Recreating them all would be a restart storm across the mesh in + // declaration order, the store first. A label that matches the spec as it used to be computed + // is accepted: what the container reads is recorded now, and from then on a change is caught. + dir := t.TempDir() + env := filepath.Join(dir, "forge.env") + declare := func(port string) *declaration.Declaration { + return parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"forge.env","type":"file","path":"`+env+`","content":"DATABASE_PORT=`+port+`\n","mode":"0600"}, + {"id":"forge.server","type":"container","name":"forge","image":"`+pinned+`","env-file":["`+env+`"]} + ]}`) + } + // The machine as the previous host left it: the file written and recorded, the container up + // under the label that host computed — the spec with nothing about the file's content. + if err := os.WriteFile(env, []byte("DATABASE_PORT=5432\n"), 0o600); err != nil { + t.Fatal(err) + } + d := declare("5432") + legacy := containerSpecReading(d.Resources[1].(*declaration.Container), nil, nil) + known := store.State{} + known.Record(store.Applied{ID: "forge.env", Type: "file", Origin: store.OriginCarried, Target: env, + Wrote: digestOf("DATABASE_PORT=5432\n")}) + known.Record(store.Applied{ID: "forge.server", Type: "container", Origin: store.OriginCarried, Target: "forge"}) + m := &machine{containers: map[string]*fakeContainer{"forge": {id: "made-by-host", running: true, spec: legacy}}} + + report, known := applyCarried(t, d, known, m, nil) + if m.did("docker rm") || m.did("docker run") || report.Changed() { + t.Fatalf("a container labelled by the previous host was recreated on upgrade: %v %+v", m.asked, report.Outcomes) + } + if got, _ := known.Find("forge.server"); got.Reads[env] != digestOf("DATABASE_PORT=5432\n") { + t.Fatalf("what the accepted container reads was not recorded: %+v", got) + } + // Accepted stays accepted: the next pass with nothing moved is quiet too. + m.asked = nil + report, known = applyCarried(t, d, known, m, nil) + if m.did("docker rm") || m.did("docker run") || report.Changed() { + t.Fatalf("an accepted container was recreated on the pass after: %v", m.asked) + } + + // And a change to the file is caught from the record, and the label is renewed. + m.asked = nil + report, _ = applyCarried(t, declare("5433"), known, m, nil) + if !m.removed("forge") || !m.did("docker run") { + t.Fatalf("an accepted container was not recreated when its env-file changed: %v", m.asked) + } + if o := outcomeOf(report, "forge.server"); o.Detail != "recreated: "+env+" changed" { + t.Errorf("the recreate did not name the file: %+v", o) + } + if m.containers["forge"].spec == legacy { + t.Error("the recreated container still carries the legacy label") + } +} + +func TestAContainerAdoptedUnderANewIdStillSaysWhichFileChanged(t *testing.T) { + // The bundle's `store` becomes the postgres module's `postgres.server`: the same container by + // name, under a new id with no record of its own. A file change on that day is a real change, + // and the record of what it read is under the old id — by name, it is found. + dir := t.TempDir() + env := filepath.Join(dir, "store.env") + m := &machine{containers: map[string]*fakeContainer{}} + raised := parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"env","type":"file","path":"`+env+`","content":"PORT=5432\n","mode":"0600"}, + {"id":"store","type":"container","name":"mesh-store","image":"`+pinned+`","env-file":["`+env+`"]} + ]}`) + _, known := applyCarried(t, raised, store.State{}, m, nil) + + adopted := parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"postgres.env","type":"file","path":"`+env+`","content":"PORT=5433\n","mode":"0600"}, + {"id":"postgres.server","type":"container","name":"mesh-store","image":"`+pinned+`","env-file":["`+env+`"]} + ]}`) + m.asked = nil + report, _, err := Apply(context.Background(), archHost(t), adopted, known, store.OriginDeclared, m.run, nil, nil) + if err != nil { + t.Fatal(err) + } + if o := outcomeOf(report, "postgres.server"); o.Action != "updated" || o.Detail != "recreated: "+env+" changed" { + t.Errorf("a container adopted under a new id did not say which file changed: %+v", o) + } +} + +func TestAHeldContainerIsNotRecreatedByAChangedHeldFile(t *testing.T) { + // On an adopted node the predecessor's container and the file it reads are both held as + // found (novox/hq ADR 0100). The predecessor rewriting its own file is reported on the file + // — and is nothing to recreate the container for: it is not the host's to recreate. + dir := t.TempDir() + env := filepath.Join(dir, "hello.env") + if err := os.WriteFile(env, []byte("PORT=5432\n"), 0o600); err != nil { + t.Fatal(err) + } + m := &machine{containers: map[string]*fakeContainer{ + "hello-web": {id: "predecessor-id", running: true}, + }} + d := adopted(t, untaken("hello-web.env", "hello-web.server"), + `{"id":"hello-web.env","type":"file","path":"`+env+`","content":"PORT=5433\n","mode":"0600"}, + {"id":"hello-web.server","type":"container","name":"hello-web","image":"`+pinned+`","env-file":["`+env+`"]}`) + report, state := applyAdopted(t, d, store.State{}, m, dir) + if outcomeOf(report, "hello-web.env").Action != "held" || outcomeOf(report, "hello-web.server").Action != "held" { + t.Fatalf("the predecessor's file and container were not held: %+v", report.Outcomes) + } + + if err := os.WriteFile(env, []byte("PORT=5434\n"), 0o600); err != nil { + t.Fatal(err) + } + m.asked = nil + report, state = applyAdopted(t, d, state, m, dir) + for _, a := range m.asked { + if strings.HasPrefix(a, "docker run") || strings.HasPrefix(a, "docker rm") { + t.Fatalf("a held container was acted on because a held file changed: %s", a) + } + } + if o := outcomeOf(report, "hello-web.server"); o.Action != "held" { + t.Errorf("the container is no longer held: %+v", o) + } + if h, _ := state.HeldAt("hello-web.env"); h.Changed != "rewritten" { + t.Errorf("the predecessor's rewrite was not reported on the file: %+v", h) + } + if h, _ := state.HeldAt("hello-web.server"); h.Changed != "" { + t.Errorf("a file change was charged to the container: %+v", h) + } +} diff --git a/internal/apply/runonce_test.go b/internal/apply/runonce_test.go index e728e7b..16b3919 100644 --- a/internal/apply/runonce_test.go +++ b/internal/apply/runonce_test.go @@ -69,7 +69,7 @@ func TestARunOnceStepIsRunToCompletionNotLeftRunning(t *testing.T) { if !ok { t.Fatal("a completed run-once step was not recorded") } - if applied.Wrote != containerSpec(d.Resources[0].(*declaration.Container), nil) { + if applied.Wrote != containerSpec(d.Resources[0].(*declaration.Container), inputs{}) { t.Errorf("the run-once record is not the declaration's digest: %q", applied.Wrote) } } @@ -155,7 +155,7 @@ func TestARunOnceStepAlreadyCompletedIsNotReRun(t *testing.T) { d := parseTrusted(t, `{"declaration":1,"resources":[ {"id":"seed","type":"container","name":"seed","image":"`+pinned+`","run-once":true} ]}`) - want := containerSpec(d.Resources[0].(*declaration.Container), nil) + want := containerSpec(d.Resources[0].(*declaration.Container), inputs{}) var ran bool run := func(ctx context.Context, name string, args ...string) (string, error) { @@ -232,7 +232,7 @@ func TestARunOnceStepRunsAgainWhenWhatItReadsChanged(t *testing.T) { was := map[string]string{"env": declaredDigest(&declaration.File{Content: "ACME_ROOTS=https://10.0.0.1/roots.pem\n"})} known := store.State{} known.Record(store.Applied{ID: "trust", Type: "container", Origin: store.OriginCarried, Target: "trust", - Wrote: containerSpec(d.Resources[1].(*declaration.Container), was)}) + Wrote: containerSpec(d.Resources[1].(*declaration.Container), inputs{declares: was})}) var ran bool run := func(ctx context.Context, name string, args ...string) (string, error) { @@ -262,7 +262,7 @@ func TestARunOnceStepRunsAgainWhenWhatItReadsChanged(t *testing.T) { settled := store.State{} settled.Record(store.Applied{ID: "env", Type: "file", Origin: store.OriginCarried, Target: env, Wrote: now["env"]}) settled.Record(store.Applied{ID: "trust", Type: "container", Origin: store.OriginCarried, Target: "trust", - Wrote: containerSpec(d.Resources[1].(*declaration.Container), now)}) + Wrote: containerSpec(d.Resources[1].(*declaration.Container), inputs{declares: now})}) if _, _, err := Apply(context.Background(), archHost(t), d, settled, store.OriginCarried, run, nil, nil); err != nil { t.Fatalf("re-apply failed: %v", err) } @@ -280,7 +280,7 @@ func TestAContainerNamingARunOnceStepIsRecreatedWhenItRan(t *testing.T) { {"id":"server","type":"container","name":"server","image":"`+pinned+`","restart-on":["trust"]} ]}`) declares := map[string]string{"trust": declaredDigest(d.Resources[0].(*declaration.Container))} - spec := containerSpec(d.Resources[1].(*declaration.Container), declares) + spec := containerSpec(d.Resources[1].(*declaration.Container), inputs{declares: declares}) var removed, created bool run := func(ctx context.Context, name string, args ...string) (string, error) { diff --git a/internal/apply/schedule.go b/internal/apply/schedule.go index ccede49..6c80de1 100644 --- a/internal/apply/schedule.go +++ b/internal/apply/schedule.go @@ -122,7 +122,7 @@ func (s *Scheduler) Sync(d *declaration.Declaration, held map[string]bool) { // Nothing to read: a scheduled container may not declare restart-on — it runs to completion // on its cadence rather than staying running to be restarted — so its identity cannot // depend on another resource's content and there is nothing to pass. - spec := containerSpec(c, nil) + spec := containerSpec(c, inputs{}) if existing := s.jobs[c.Identity()]; existing != nil && existing.spec == spec { // Unchanged: keep where it is in its cadence, refresh the declaration pointer only. existing.container = c diff --git a/internal/bootstrap/adopt_in_place_test.go b/internal/bootstrap/adopt_in_place_test.go new file mode 100644 index 0000000..9814cf7 --- /dev/null +++ b/internal/bootstrap/adopt_in_place_test.go @@ -0,0 +1,130 @@ +package bootstrap + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/novox/mesh-host/internal/apply" + "github.com/novox/mesh-host/internal/declaration" + "github.com/novox/mesh-host/internal/store" +) + +// Defends phase three's premise (phase3.go): the store genesis raised is adopted by the postgres +// module IN PLACE — same name, same image, same spec — so the applier reconciles it and never +// recreates the mesh's memory with the temporary control plane connected to it. +// +// The host folds a mounted file's content into the container's spec (novox/hq 04-ISSUES/103), so +// this now depends on a byte: the superuser file genesis writes and mounts must be the same bytes +// the module later declares. The module's value is what `secret accept` took — the operator's +// file with its line ending removed and nothing else (mesh-control, asSupplied). Reproduced before +// it was fixed: genesis wrote `value\n`, the module wrote `value`, and the store was recreated +// during install. + +// labelled is a runtime that keeps the spec label the host gives a container and hands it back. +type labelled struct { + spec map[string]string + created []string + removed []string +} + +func (l *labelled) run(_ context.Context, _ string, args ...string) (string, error) { + switch args[0] { + case "info": + return "27.0\n", nil + case "inspect": + spec, ok := l.spec[args[len(args)-1]] + if !ok { + return "", errors.New("no such container") + } + return "true\t" + spec + "\n", nil + case "rm": + l.removed = append(l.removed, args[len(args)-1]) + delete(l.spec, args[len(args)-1]) + case "run": + var name, spec string + for i, a := range args { + if a == "--name" { + name = args[i+1] + } + if a == "--label" && strings.HasPrefix(args[i+1], "mesh-host.spec=") { + spec = strings.TrimPrefix(args[i+1], "mesh-host.spec=") + } + } + l.spec[name] = spec + l.created = append(l.created, name) + return "made\n", nil + } + return "", nil +} + +func TestTheStoreGenesisRaisedIsAdoptedInPlaceNotRecreated(t *testing.T) { + dir := t.TempDir() + secret := filepath.Join(dir, "superuser.secret") + + // The bytes genesis really writes — the code path, not a fixture that agrees with it. + if _, made, err := keptOrMade(secret, false); err != nil || !made { + t.Fatalf("genesis did not make the superuser secret: made=%v err=%v", made, err) + } + onDisk, err := os.ReadFile(secret) + if err != nil { + t.Fatal(err) + } + + image := "docker.io/library/postgres@sha256:" + strings.Repeat("ab", 32) + mounts := `"volumes":["mesh-store-data:/var/lib/postgresql/data","` + secret + `:` + storeSuperuserMount + `:ro"]` + + // The foundation's store, as the produced bundle raises it (RewriteRoot): the file mounted, + // declared by nothing — genesis wrote it before there was a declaration to name it. + raise, err := declaration.ParseTrusted([]byte(`{"declaration":1,"resources":[ + {"id":"` + StoreID + `","type":"container","name":"mesh-store","image":"` + image + `", + "env":{"POSTGRES_PASSWORD_FILE":"` + storeSuperuserMount + `"},` + mounts + `} + ]}`)) + if err != nil { + t.Fatal(err) + } + runtime := &labelled{spec: map[string]string{}} + _, known, err := apply.Apply(context.Background(), arch(t), raise, store.State{}, store.OriginCarried, + runtime.run, nil, nil) + if err != nil { + t.Fatalf("raising the foundation's store: %v", err) + } + if len(runtime.created) != 1 { + t.Fatalf("the store was not raised once: %v", runtime.created) + } + + // The postgres module's declaration of the same store: the superuser file as `secret accept` + // took it in — its line ending removed and nothing else — then the same container. + accepted := strings.TrimRight(string(onDisk), "\r\n") + adopt, err := declaration.ParseTrusted([]byte(`{"declaration":1,"resources":[ + {"id":"postgres.superuser","type":"file","path":"` + secret + `","content":"` + accepted + `","mode":"0600"}, + {"id":"postgres.server","type":"container","name":"mesh-store","image":"` + image + `", + "env":{"POSTGRES_PASSWORD_FILE":"` + storeSuperuserMount + `"},` + mounts + `} + ]}`)) + if err != nil { + t.Fatal(err) + } + runtime.created, runtime.removed = nil, nil + report, _, err := apply.Apply(context.Background(), arch(t), adopt, known, store.OriginDeclared, + runtime.run, nil, nil) + if err != nil { + t.Fatalf("adopting the store: %v", err) + } + + if len(runtime.removed) > 0 || len(runtime.created) > 0 { + t.Fatalf("the module's declaration recreated the store genesis raised (removed %v, created %v): "+ + "the file genesis mounted and the file the module declares are not the same bytes", + runtime.removed, runtime.created) + } + for _, o := range report.Outcomes { + if o.ID == "postgres.server" && o.Action != "unchanged" { + t.Errorf("the store was not adopted in place: %+v", o) + } + if o.ID == "postgres.superuser" && o.Action != "unchanged" { + t.Errorf("the module rewrote the superuser file genesis wrote: %+v", o) + } + } +} diff --git a/internal/bootstrap/rootsecrets.go b/internal/bootstrap/rootsecrets.go index 06a81f2..4b5ec7c 100644 --- a/internal/bootstrap/rootsecrets.go +++ b/internal/bootstrap/rootsecrets.go @@ -102,8 +102,16 @@ func keptOrMade(path string, dryRun bool) (value string, made bool, err error) { } // Written whole and renamed into place, at 0600, owned by whoever runs the installer — root, // which is also who the host runs as when it later writes the sealed copy here. + // + // **The value alone, no line ending.** The module that adopts the store declares this same + // file, and what it declares is the value as `secret accept` took it — its line ending gone, + // by design. The host folds a mounted file's content into the container's spec (novox/hq + // 04-ISSUES/103), so a genesis that wrote `value\n` here would raise a store whose label + // digests one byte more than the module's file, and phase three would RECREATE the store it + // meant to adopt in place, with the temporary control plane connected to it. readCredentialFile + // tolerates either ending, so a file an earlier genesis wrote still reads. tmp := path + ".genesis" - if err := os.WriteFile(tmp, []byte(value+"\n"), 0o600); err != nil { + if err := os.WriteFile(tmp, []byte(value), 0o600); err != nil { return "", false, err } if err := os.Rename(tmp, path); err != nil { diff --git a/internal/declaration/declaration.go b/internal/declaration/declaration.go index c0bc7af..d7e877d 100644 --- a/internal/declaration/declaration.go +++ b/internal/declaration/declaration.go @@ -782,12 +782,17 @@ type Container struct { // RestartOn names resources whose change means this container must be recreated — the same // field a service has, for the same reason (novox/hq 04-ISSUES/009). A container reads a // mounted file once at start; a changed file leaves the running process holding the old value, - // while every check passes because the file on disk is right. The container's spec — image, - // env, volumes — does not include a mounted file's *content*, so a settings change that - // re-renders that file is invisible to the ordinary spec diff. This closes that: the host - // recreates the container when one of these resources changed this pass, even if the spec - // matches. On a run-once step it means *run again*: a step that fetches a fact from a provider - // names the binding it reads, and is run again when the provider moved (novox/hq ADR 0099). + // while every check passes because the file on disk is right. The host recreates the container + // when one of these resources changed this pass, even if the spec matches. + // + // What a running container reads at creation — its env-files, and a file mounted into it + // directly — is part of its spec by content since novox/hq 04-ISSUES/103, and needs no naming + // here. A directory mounted into it is NOT looked inside, not even for files the host wrote + // there: whether a service reads such a file once or watches it live is the service's, and + // RestartOn is how a module says "once, at start" — a config the host renders under the + // module's state directory, a step whose result it consumes. On a run-once step it means *run + // again*: a step that fetches a fact from a provider names the binding it reads, and is run + // again when the provider moved (novox/hq ADR 0099). RestartOn []string `json:"restart-on,omitempty"` // RunOnce marks a container the host runs to completion rather than leaves running: a step, diff --git a/internal/store/store.go b/internal/store/store.go index 70bec77..eb8264d 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -74,6 +74,14 @@ type Applied struct { // each of the mesh's keys held before it set them, which of them were absent, and whether the // file itself was — so undeclaring it gives the machine back exactly what it had. Into *Into `json:"into,omitempty"` + + // Reads is, for a container, the digest of each file it was created reading — its env-files + // and the files mounted into it — by path (novox/hq 04-ISSUES/103). + // + // A container takes those in once, when it is created, and the digest of the whole is in the + // container's spec label; this is the same information kept per file, so that when the spec + // no longer matches the host can say WHICH file changed rather than only that something did. + Reads map[string]string `json:"reads,omitempty"` } // Into is what a file written into held before the mesh's keys. @@ -200,6 +208,28 @@ func (s State) Recorded(kind, target string) bool { return false } +// At returns what this host has a record of putting at a target of this kind, under any id and of +// any origin — Recorded, with the record. +// +// By target rather than by id because the id a thing was declared under may change while the thing +// does not: the bundle's `store` becomes a module's `postgres.server` for the same container, and +// the file that container reads is the same file under either id. Both ids may then hold a record +// for the one target — the bundle's is never removed by the mesh's declaration — and the most +// recently applied is the one that says what is there now. +func (s State) At(kind, target string) (Applied, bool) { + var latest Applied + found := false + for _, r := range s.Resources { + if r.Type != kind || r.Target != target { + continue + } + if !found || r.AppliedAt.After(latest.AppliedAt) { + latest, found = r, true + } + } + return latest, found +} + // HeldAt returns what is held under a resource id. func (s State) HeldAt(id string) (Held, bool) { for _, h := range s.Held {