From e7f94e040292584a85b8db6d20ff6a0891b8e3c4 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 14 Sep 2026 16:51:57 +0200 Subject: [PATCH] A one-shot service that finished is not stopped, and a container is what it reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two faults that both reported success while being wrong, found while proving the firewall module actually delivers. A unit whose job is to apply something and exit — load a rule set, set a sysctl — is inactive the instant it succeeds. Reading that as stopped made it permanently unsatisfiable: 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. That is what the firewall has been doing on every machine it was assigned to, and why the four-machine bed was red. And a container took its identity from its own fields, not from the files it reads. A file written in an earlier apply — or before the container declared it as a dependency — left a process holding a credential the mesh had already replaced, with everything reporting success (novox/hq 04-ISSUES/045). What a container reads is now part of what it is, so the comparison is a standing one rather than a tripwire that fires during one apply and never again. --- internal/apply/apply.go | 63 +++++++++++++++++++++++--- internal/apply/apply_test.go | 78 ++++++++++++++++++++++++++++++--- internal/apply/runonce_test.go | 6 +-- internal/apply/schedule.go | 5 ++- internal/apply/schedule_test.go | 6 +-- internal/system/arch.go | 31 ++++++++++++- internal/system/arch_test.go | 51 +++++++++++++++++++++ 7 files changed, 219 insertions(+), 21 deletions(-) create mode 100644 internal/system/arch_test.go 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) + } +}