diff --git a/internal/apply/apply.go b/internal/apply/apply.go index baba8d4..ecb4d26 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -150,6 +150,20 @@ func Apply( // restarting for it every time would make a steady machine restart its services for ever. changed := map[string]bool{} + // **What each resource currently says, so a container can be identified by its inputs.** + // + // `changed` above is edge-triggered and only within one apply, which is right for "restart it + // because this just moved" and wrong for "is this container running the file that is there + // now". A file written in an earlier apply, or written before the container declared it as a + // dependency, leaves a container holding values nothing will ever re-read: it is up, the + // 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{} + for _, resource := range d.Resources { + declares[resource.Identity()] = declaredDigest(resource) + } + // Everything is attempted, and every failure is reported. // // **It used to stop at the first one**, and that made one broken resource hold the whole @@ -169,7 +183,7 @@ func Apply( var failures []*Error for _, resource := range d.Resources { was, _ := known.Find(resource.Identity()) - outcome, err := applyOne(ctx, sys, resource, run, changed, was, unseal) + outcome, err := applyOne(ctx, sys, resource, run, changed, declares, was, unseal) if err != nil { failed := &Error{Resource: resource.Identity(), Err: err, Done: report} failures = append(failures, failed) @@ -239,7 +253,8 @@ func Apply( type Unseal func(sealed string) ([]byte, error) func applyOne(ctx context.Context, sys system.System, r declaration.Resource, run Runner, - changed map[string]bool, previous store.Applied, unseal Unseal) (Outcome, error) { + changed map[string]bool, declares map[string]string, previous store.Applied, + unseal Unseal) (Outcome, error) { switch res := r.(type) { case *declaration.Directory: return applyDirectory(res) @@ -250,7 +265,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, previous) + return applyContainer(ctx, res, run, changed, declares, previous) case *declaration.User: return applyUser(ctx, sys, res, run) case *declaration.Archive: @@ -853,7 +868,7 @@ const ( // 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) string { +func containerSpec(r *declaration.Container, declares map[string]string) string { keys := make([]string, 0, len(r.Env)) for k := range r.Env { keys = append(keys, k) @@ -880,6 +895,19 @@ func containerSpec(r *declaration.Container) string { if r.Schedule != "" { b.WriteString("schedule " + r.Schedule + "\n") } + // **What this container reads is part of what it is.** + // + // A container takes its environment and its mounted files once, at start, and never looks + // again. Comparing only the fields above meant a container whose configuration had since been + // rewritten compared equal and was left alone — running values the machine no longer holds, + // while every check reported success (novox/hq 04-ISSUES/045). Naming what it depends on here + // makes that comparison standing rather than a tripwire that fires during one apply and never + // again. Sorted, so the digest does not move for a reordering nobody made. + depends := append([]string{}, r.RestartOn...) + sort.Strings(depends) + for _, id := range depends { + b.WriteString("reads " + id + "=" + declares[id] + "\n") + } return fmt.Sprintf("%x", sha256.Sum256([]byte(b.String()))) } @@ -944,9 +972,10 @@ func applyNetwork(ctx context.Context, r *declaration.Network, run Runner) (Outc return out, nil } -func applyContainer(ctx context.Context, r *declaration.Container, run Runner, changed map[string]bool, previous store.Applied) (Outcome, error) { +func applyContainer(ctx context.Context, r *declaration.Container, run Runner, + changed map[string]bool, declares map[string]string, previous store.Applied) (Outcome, error) { out := begin(r) - want := containerSpec(r) + want := containerSpec(r, declares) cri, err := containerRuntime(ctx, run) if err != nil { @@ -1346,3 +1375,25 @@ func holds(resource declaration.Resource) []int { } return out } + +// declaredDigest is what a resource currently says it should be. +// +// **Content, not identity.** It exists so a container can be told apart by what it reads: a file +// whose text changed must produce a different digest, or the container mounting it compares equal +// to one started against the old text. Only the shapes something can read are digested; for +// everything else the identity is enough, because nothing mounts a package. +func declaredDigest(r declaration.Resource) string { + var material string + switch res := r.(type) { + case *declaration.File: + // The content as declared, before any sealing is opened — two machines are given different + // ciphertext for the same secret, and digesting that would make an unchanged file look + // changed on every apply and restart the container reading it for ever. + material = res.Content + case *declaration.Directory: + material = res.Path + "\n" + res.Mode + default: + return "" + } + return fmt.Sprintf("%x", sha256.Sum256([]byte(material))) +} diff --git a/internal/apply/apply_test.go b/internal/apply/apply_test.go index 0990fca..0086be6 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)) + want := containerSpec(d.Resources[0].(*declaration.Container), nil) 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)) + spec := containerSpec(d.Resources[0].(*declaration.Container), nil) var touched bool run := func(ctx context.Context, name string, args ...string) (string, error) { @@ -743,7 +743,13 @@ func TestAContainerIsRecreatedWhenARestartOnResourceChanged(t *testing.T) { {"id":"config","type":"file","path":"`+conf+`","content":"{\"token\":\"new\"}\n"}, {"id":"app","type":"container","name":"app","image":"`+pinned+`","restart-on":["config"]} ]}`) - spec := containerSpec(d.Resources[1].(*declaration.Container)) + // The spec the container was created with, back when the file said something else. Built the + // way the host builds it, so this is the real comparison rather than a hand-written string: + // 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) var removed, created bool run := func(ctx context.Context, name string, args ...string) (string, error) { @@ -751,9 +757,12 @@ func TestAContainerIsRecreatedWhenARestartOnResourceChanged(t *testing.T) { case "info": return "27.0\n", nil case "inspect": - // The container already exists, running, with exactly the spec it is declared with — - // only the mounted file changed. - return "true\t" + spec, nil + // Already there and running, created against the file as it was. After the host + // recreates it, the runtime holds the one it just made — as a real one would. + if created { + return "true\t" + fresh, nil + } + return "true\t" + stale, nil case "rm": removed = true return "", nil @@ -1492,3 +1501,60 @@ func TestAnEmptyDirectoryIsStillRemoved(t *testing.T) { t.Fatal("an empty directory the mesh made was left behind, so nothing is ever cleaned up") } } + +// A container holding values from before is replaced, even when nothing changed this pass. +// +// **This is the fault the restart-on tripwire could not catch** (novox/hq 04-ISSUES/045). That +// mechanism fires while a resource is being changed, so it covers the apply where the file moved +// and nothing afterwards. A file written in an earlier apply — or written before the container +// declared it as a dependency — leaves a process holding a credential the mesh has already +// replaced, and every check passes: the container is up, the spec matched, the machine reported +// success. Making what a container reads part of what it is turns that from a tripwire into a +// standing comparison. +func TestAContainerStaleFromAnEarlierApplyIsReplaced(t *testing.T) { + dir := t.TempDir() + conf := filepath.Join(dir, "db.env") + // Already on disk with the current content, so this apply changes nothing at all. + if err := os.WriteFile(conf, []byte("PASSWORD=new\n"), 0o600); err != nil { + t.Fatal(err) + } + d := parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"env","type":"file","path":"`+conf+`","content":"PASSWORD=new\n","mode":"0600"}, + {"id":"app","type":"container","name":"app","image":"`+pinned+`","restart-on":["env"]} + ]}`) + + // 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) + + var removed, created bool + run := func(ctx context.Context, name string, args ...string) (string, error) { + switch args[0] { + case "info": + return "27.0\n", nil + case "inspect": + if created { + return "true\t" + fresh, nil + } + return "true\t" + stale, nil + case "rm": + removed = true + return "", nil + case "run": + created = true + return "deadbeef\n", nil + } + return "", nil + } + + _, _, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginCarried, run, nil, nil) + if err != nil { + t.Fatalf("apply failed: %v", err) + } + if !removed || !created { + t.Fatalf("a container running values the machine no longer holds was left alone "+ + "(removed=%v created=%v)", removed, created) + } +} diff --git a/internal/apply/runonce_test.go b/internal/apply/runonce_test.go index 264b1d5..106eb44 100644 --- a/internal/apply/runonce_test.go +++ b/internal/apply/runonce_test.go @@ -68,7 +68,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)) { + if applied.Wrote != containerSpec(d.Resources[0].(*declaration.Container), nil) { t.Errorf("the run-once record is not the declaration's digest: %q", applied.Wrote) } } @@ -154,7 +154,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)) + want := containerSpec(d.Resources[0].(*declaration.Container), nil) var ran bool run := func(ctx context.Context, name string, args ...string) (string, error) { @@ -222,7 +222,7 @@ func TestARunOnceContainerCannotAlsoDeclareRestartOn(t *testing.T) { // silently resolving to one. _, err := declaration.Parse([]byte(`{"declaration":1,"resources":[ {"id":"f","type":"file","path":"/tmp/x","content":"y"}, - {"id":"seed","type":"container","name":"seed","image":"`+pinned+`","run-once":true,"restart-on":["f"]} + {"id":"seed","type":"container","name":"seed","image":"` + pinned + `","run-once":true,"restart-on":["f"]} ]}`)) if err == nil { t.Fatal("a run-once container that also declared restart-on was accepted") diff --git a/internal/apply/schedule.go b/internal/apply/schedule.go index 039385a..3057559 100644 --- a/internal/apply/schedule.go +++ b/internal/apply/schedule.go @@ -107,7 +107,10 @@ func (s *Scheduler) Sync(d *declaration.Declaration) { } seen[c.Identity()] = true - spec := containerSpec(c) + // 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) 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/apply/schedule_test.go b/internal/apply/schedule_test.go index bd0813c..fcca78c 100644 --- a/internal/apply/schedule_test.go +++ b/internal/apply/schedule_test.go @@ -39,7 +39,7 @@ func (c *fixedClock) set(t time.Time) { // recordingRun remembers every command it was asked to run, guarded for the goroutines fire() uses. type recordingRun struct { mu sync.Mutex - runs int // how many `docker run` (a fire) happened + runs int // how many `docker run` (a fire) happened all [][]string } @@ -338,8 +338,8 @@ func TestAFailedRunIsRecordedAndDoesNotFailAnything(t *testing.T) { func TestASlowRunSkipsTheNextDueRunRatherThanStacking(t *testing.T) { clock := &fixedClock{now: time.Date(2026, 9, 7, 12, 0, 30, 0, time.UTC)} - started := make(chan struct{}) // fire() signals it entered the runner - release := make(chan struct{}) // the test lets the run finish + started := make(chan struct{}) // fire() signals it entered the runner + release := make(chan struct{}) // the test lets the run finish var enters int var mu sync.Mutex run := func(ctx context.Context, name string, args ...string) (string, error) { diff --git a/internal/system/arch.go b/internal/system/arch.go index 80ce687..178570d 100644 --- a/internal/system/arch.go +++ b/internal/system/arch.go @@ -99,9 +99,10 @@ func staleIndex(out string) bool { // would have had to drop. func (arch) ServiceState(ctx context.Context, run Runner, unit string) (string, error) { out, _ := run(ctx, "systemctl", "show", unit, - "--property=LoadState", "--property=ActiveState") + "--property=LoadState", "--property=ActiveState", "--property=Type", + "--property=RemainAfterExit", "--property=ExecMainStatus") - var load, active string + var load, active, kind, remains, exited string for _, line := range strings.Split(out, "\n") { key, value, found := strings.Cut(strings.TrimSpace(line), "=") if !found { @@ -112,6 +113,12 @@ func (arch) ServiceState(ctx context.Context, run Runner, unit string) (string, load = value case "ActiveState": active = value + case "Type": + kind = value + case "RemainAfterExit": + remains = value + case "ExecMainStatus": + exited = value } } @@ -129,6 +136,26 @@ func (arch) ServiceState(ctx context.Context, run Runner, unit string) (string, return "", fmt.Errorf("%s is installed but its unit file cannot be loaded (%s)", unit, load) } + // **A one-shot that finished is not stopped.** A unit whose whole job is to apply something + // and exit — load a rule set, set a sysctl — is reported inactive the moment it succeeds, and + // unless it is told to linger there is no state in which it is ever "active". Reading that as + // "stopped" makes such a unit permanently unsatisfiable: the host starts it, it does its work, + // it exits, the host reads back "stopped" and reports failure — for ever, on every apply, + // while the thing it configured is in place and working. + // + // That is not hypothetical. It is what the firewall did on every machine it was ever assigned + // to: rules loaded, service reported failed, the mesh reported a machine not doing what it was + // told, and the only visible symptom was a red line about a unit nobody could see anything + // wrong with. + // + // So for that shape, what "running" means is "it ran, and it worked". + if kind == "oneshot" && remains != "yes" && active == "inactive" { + if exited == "0" || exited == "" { + return "running", nil + } + return "stopped", nil + } + switch active { case "active", "activating", "reloading": return "running", nil diff --git a/internal/system/arch_test.go b/internal/system/arch_test.go new file mode 100644 index 0000000..22d6bf2 --- /dev/null +++ b/internal/system/arch_test.go @@ -0,0 +1,51 @@ +package system + +import ( + "context" + "testing" +) + +// A one-shot unit that did its work and exited is satisfied, not stopped. +// +// **This made the firewall permanently unsatisfiable.** Its unit loads a rule set and exits, so it +// is inactive the instant it succeeds — the host started it, it worked, the host read back +// "stopped" and reported the machine as not doing what it was told. On every apply, for ever, with +// the rules correctly in place the whole time. +func TestAOneShotThatFinishedIsRunning(t *testing.T) { + run := func(_ context.Context, _ string, _ ...string) (string, error) { + return "LoadState=loaded\nActiveState=inactive\nType=oneshot\nRemainAfterExit=no\nExecMainStatus=0\n", nil + } + state, err := arch{}.ServiceState(context.Background(), run, "nftables.service") + if err != nil { + t.Fatalf("a one-shot that succeeded was an error: %v", err) + } + if state != "running" { + t.Fatalf("a one-shot that did its work reads as %q, so it can never be satisfied", state) + } +} + +// And one that failed is still stopped, or the host would report success for work that did not +// happen — which is the opposite mistake and the worse one. +func TestAOneShotThatFailedIsStopped(t *testing.T) { + run := func(_ context.Context, _ string, _ ...string) (string, error) { + return "LoadState=loaded\nActiveState=inactive\nType=oneshot\nRemainAfterExit=no\nExecMainStatus=1\n", nil + } + state, err := arch{}.ServiceState(context.Background(), run, "nftables.service") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if state != "stopped" { + t.Fatalf("a one-shot that failed reads as %q, so a broken firewall reports success", state) + } +} + +// A unit that lingers deliberately is unaffected: it says so, and its active state is the answer. +func TestAOneShotThatRemainsIsJudgedByItsActiveState(t *testing.T) { + run := func(_ context.Context, _ string, _ ...string) (string, error) { + return "LoadState=loaded\nActiveState=active\nType=oneshot\nRemainAfterExit=yes\nExecMainStatus=0\n", nil + } + state, _ := arch{}.ServiceState(context.Background(), run, "thing.service") + if state != "running" { + t.Fatalf("a lingering one-shot reads as %q", state) + } +}