From 491e04fb8f39a8e6c3d53237cfd1cff85ed21787 Mon Sep 17 00:00:00 2001 From: jochen Date: Tue, 22 Sep 2026 18:30:13 +0200 Subject: [PATCH] Load the guard before removing the derived filter when a node returns to adopted, and defer the adoption's orphans only on the flip (hq ADR 0103) --- internal/apply/apply.go | 69 ++++++++++++++++++++++++++++------ internal/apply/opening_test.go | 66 ++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 11 deletions(-) diff --git a/internal/apply/apply.go b/internal/apply/apply.go index f9e8d96..873e484 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -185,21 +185,53 @@ func ApplyKeeping( return nil } - // **What protects an adopted node goes last** (novox/hq ADR 0103). The openings and the guard - // are what keep the mesh reachable through the found firewall and the store unreachable from - // outside. When a node is converged they leave the declaration, and removing them first would - // leave the store open from the moment the guard stops until the derived filter loads — and - // for ever, if the filter then fails. So they are removed only once everything else applied - // and the found firewall is retired; if anything failed, they stay, recorded, for the next try. - var protecting []store.Applied + // **What protects an adopted node goes last on the flip, and first on the way back** (novox/hq + // ADR 0103). The openings and the guard are what keep the mesh reachable through the found + // firewall and the store unreachable from outside. + // + // When a node is converged they leave the declaration, and removing them first would leave the + // store open from the moment the guard stops until the derived filter loads — and for ever, if + // the filter then fails. So on a converged declaration they are removed only once everything + // else applied and the found firewall is retired; if anything failed, they stay, recorded, for + // the next try. + // + // Returned to adopted, it is the mirror image: removing the derived filter first would leave + // the store open until the guard loads. So the guard's own resources are applied before any + // orphan is removed, and if a removal then fails the guard is already up. A stale opening on an + // adopted node is removed as any orphan is. + var protecting, orphans []store.Applied for _, orphan := range known.Orphans(declared, origin) { - if strings.HasPrefix(orphan.ID, declaration.AdoptionPrefix) { + if d.Adoption == nil && strings.HasPrefix(orphan.ID, declaration.AdoptionPrefix) { protecting = append(protecting, orphan) continue } - if err := removeOrphan(orphan); err != nil { - return report, known, err + orphans = append(orphans, orphan) + } + ordered := d.Resources + guardFirst := 0 + if d.Adoption != nil { + ordered = nil + for _, r := range d.Resources { + if strings.HasPrefix(r.Identity(), guardPrefix) { + ordered = append(ordered, r) + } } + guardFirst = len(ordered) + for _, r := range d.Resources { + if !strings.HasPrefix(r.Identity(), guardPrefix) { + ordered = append(ordered, r) + } + } + } + orphansRemoved := false + removeOrphans := func() error { + orphansRemoved = true + for _, orphan := range orphans { + if err := removeOrphan(orphan); err != nil { + return err + } + } + return nil } // **A hold whose resource is no longer declared is let go, and nothing on disk is touched.** @@ -256,7 +288,12 @@ func ApplyKeeping( // is a different thing — one is "this machine could not do it", the other is "this was never // a declaration", and they are fixed in different places. var failures []*Error - for _, resource := range d.Resources { + for i, resource := range ordered { + if !orphansRemoved && i == guardFirst { + if err := removeOrphans(); err != nil { + return report, known, err + } + } // **On an adopted node, what is found is kept until its module is taken** (novox/hq ADR // 0100, ADR 0103). Before anything is applied: whatever of a module not yet taken is // present with no record of this host making it — or would reach what is — is held as it @@ -345,6 +382,12 @@ func ApplyKeeping( } } + if !orphansRemoved { + if err := removeOrphans(); err != nil { + return report, known, err + } + } + // A converged node whose found firewall was in force retires it only now, once everything — // the mesh's derived filter among it — applied cleanly (novox/hq ADR 0100). if len(failures) == 0 { @@ -370,6 +413,10 @@ func ApplyKeeping( return report, known, nil } +// guardPrefix is the ids of the mesh's guard on an adopted node: its package, table, unit and +// service (novox/hq ADR 0100). +const guardPrefix = declaration.AdoptionPrefix + "guard" + // Unseal opens a value the mesh sealed to this node. Nil when the node has no sealing key, which // makes every sealed file an error rather than a silently skipped one. type Unseal func(sealed string) ([]byte, error) diff --git a/internal/apply/opening_test.go b/internal/apply/opening_test.go index c02aa79..ccdf79d 100644 --- a/internal/apply/opening_test.go +++ b/internal/apply/opening_test.go @@ -304,3 +304,69 @@ func TestAnOpeningAFoundRuleAnswersIsReportedSatisfied(t *testing.T) { t.Errorf("a rule was added beside the found one: %v", u.rules) } } + +// Defends novox/hq ADR 0103: returned to adopted, the guard is up before the derived filter's +// orphans go, and stays up if removing them fails. +func TestReturningToAdoptedLoadsTheGuardBeforeRemovingTheFilter(t *testing.T) { + dir := t.TempDir() + guard := filepath.Join(dir, "guard.nft") + guardFile := `{"id":"adoption.guard","type":"file","path":"` + guard + `","content":"table inet mesh_guard {}\n"}` + for _, stopFails := range []bool{false, true} { + _ = os.Remove(guard) + guardUpAtStop := false + run := func(_ context.Context, name string, args ...string) (string, error) { + if name != "systemctl" { + return "", &exec.Error{Name: name, Err: exec.ErrNotFound} + } + switch args[0] { + case "show": + return "LoadState=loaded\nActiveState=active\nType=oneshot\nRemainAfterExit=yes\n", nil + case "stop": + _, err := os.Stat(guard) + guardUpAtStop = err == nil + if stopFails { + return "", errors.New("the filter would not stop") + } + } + return "", nil + } + converged := store.State{Resources: []store.Applied{ + {ID: "nftables.load", Type: "service", Target: "mesh-filter.service", Origin: store.OriginDeclared}}} + _, state, err := applyWith(t, adopted(t, `{"taken":[]}`, withConf(dir)+","+guardFile), converged, run) + if !guardUpAtStop { + t.Errorf("stop fails %v: the derived filter was stopped before the guard was written", stopFails) + } + if stopFails { + if err == nil { + t.Error("a failed removal was not reported") + } + if _, statErr := os.Stat(guard); statErr != nil { + t.Error("the guard is not up after the filter's removal failed") + } + if _, ok := state.Find("adoption.guard"); !ok { + t.Error("the guard applied before the failure was not recorded") + } + } else if err != nil { + t.Fatal(err) + } + } +} + +func TestAStaleOpeningOnAnAdoptedNodeIsRemovedAsAnyOrphan(t *testing.T) { + // Only the flip defers the adoption's own orphans; an adopted node drops a stale opening at + // once, before what replaces it is applied. + dir := t.TempDir() + u := &ufwMachine{installed: true, active: true, rules: []string{"allow 22/tcp"}} + _, state, err := applyWith(t, adopted(t, `{"taken":[]}`, busOpening+","+withConf(dir)), store.State{}, u.run) + if err != nil { + t.Fatal(err) + } + u.asked = nil + other := `{"id":"adoption.opening-tcp-5000-incoming","type":"opening","port":5000,"protocol":"tcp","from":"everywhere","path":"incoming"}` + if _, _, err = applyWith(t, adopted(t, `{"taken":[]}`, other+","+withConf(dir)), state, u.run); err != nil { + t.Fatal(err) + } + if del, add := u.index("ufw delete"), u.index("ufw allow"); del < 0 || add < 0 || del > add { + t.Errorf("the stale opening was not removed before the new one was added: %v", u.asked) + } +}