diff --git a/internal/apply/apply.go b/internal/apply/apply.go index 8bd30a2..22cdb3b 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -114,7 +114,9 @@ func (e *Error) Unwrap() error { return e.Err } // Removal happens FIRST, and the order is not arbitrary. A resource that leaves a declaration // while another arrives at the same path is an ordinary rename: removing afterwards would // delete the file that had just been written. Removing first risks losing the old state if the -// apply then fails — a recovery concern, where the other is a correctness one. +// apply then fails — a recovery concern, where the other is a correctness one. The one exception +// is what protects an adopted node, the openings and the guard: that goes last, and only when +// everything else applied (novox/hq ADR 0103). func Apply( ctx context.Context, sys system.System, @@ -159,7 +161,7 @@ func ApplyKeeping( return report, known, &Error{Resource: "the firewall found on this machine", Err: err, Done: report} } - for _, orphan := range known.Orphans(declared, origin) { + removeOrphan := func(orphan store.Applied) error { var action, detail string var err error if declaration.Type(orphan.Type) == declaration.TypeOpening { @@ -168,7 +170,7 @@ func ApplyKeeping( action, detail, err = remove(ctx, sys, orphan, run) } if err != nil { - return report, known, &Error{Resource: orphan.ID, Err: err, Done: report} + return &Error{Resource: orphan.ID, Err: err, Done: report} } known.Forget(orphan.ID) report.Outcomes = append(report.Outcomes, Outcome{ @@ -176,6 +178,24 @@ func ApplyKeeping( Action: action, Detail: detail, }) log(fmt.Sprintf(" %s %s (%s)", action, orphan.ID, orphan.Target)) + 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 + for _, orphan := range known.Orphans(declared, origin) { + if strings.HasPrefix(orphan.ID, declaration.AdoptionPrefix) { + protecting = append(protecting, orphan) + continue + } + if err := removeOrphan(orphan); err != nil { + return report, known, err + } } // What moved in this apply, so a service that must reflect a file can be told the file @@ -321,6 +341,11 @@ func ApplyKeeping( if err := retireFirewall(ctx, d, origin, &known, run, log); err != nil { return report, known, &Error{Resource: "the firewall found on this machine", Err: err, Done: report} } + for _, orphan := range protecting { + if err := removeOrphan(orphan); err != nil { + return report, known, err + } + } } if len(failures) > 0 { diff --git a/internal/apply/opening_test.go b/internal/apply/opening_test.go index 7192925..f4dc76d 100644 --- a/internal/apply/opening_test.go +++ b/internal/apply/opening_test.go @@ -151,8 +151,10 @@ func TestConvergingRetiresTheFoundFirewallAndReturningRestoresIt(t *testing.T) { if len(u.rules) != 1 || u.rules[0] != "allow 22/tcp" { t.Errorf("converged: the operator's rules were touched, or the mesh's left: %v", u.rules) } - if del, dis := u.index("ufw delete"), u.index("ufw disable"); del < 0 || dis < del { - t.Errorf("converged: the opening was not removed before ufw was disabled: %v", u.asked) + // What protected the adopted node goes last: after the derived filter applied and ufw was + // retired (novox/hq ADR 0103). + if del, dis := u.index("ufw delete"), u.index("ufw disable"); dis < 0 || del < dis { + t.Errorf("converged: the opening was removed before ufw was retired: %v", u.asked) } for _, a := range u.asked { if strings.Contains(a, "reset") { @@ -224,3 +226,64 @@ func TestACarriedApplyOnAnAdoptedNodeLeavesItsFirewallInForce(t *testing.T) { u.active, state.Firewall, u.asked) } } + +func TestAFlipThatFailsKeepsTheGuardAndTheOpenings(t *testing.T) { + // Converging a node removes its openings and its guard only once everything else applied and + // the found firewall is retired. A flip that fails part-way keeps them, so the store is never + // left unguarded behind a filter that did not load (novox/hq ADR 0103). + dir := t.TempDir() + guard := filepath.Join(dir, "guard.nft") + guardFile := `{"id":"adoption.guard","type":"file","path":"` + guard + `","content":"table inet mesh_guard {}\n"}` + u := &ufwMachine{installed: true, active: true, rules: []string{"allow 22/tcp"}} + _, state, err := applyWith(t, adopted(t, `{"taken":[]}`, busOpening+","+guardFile+","+withConf(dir)), + store.State{}, u.run) + if err != nil { + t.Fatal(err) + } + + // The derived filter cannot be written: its path is under a file. + blocked := filepath.Join(dir, "not-a-directory") + if err := os.WriteFile(blocked, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + filter := `{"id":"nftables.config","type":"file","path":"` + filepath.Join(blocked, "nftables.conf") + `","content":"table inet mesh {}\n"}` + converged := parse(t, `{"declaration":1,"resources":[`+withConf(dir)+`,`+filter+`]}`) + u.asked = nil + _, state, err = applyWith(t, converged, state, u.run) + if err == nil { + t.Fatal("the failing flip reported success") + } + if !u.active || u.index("ufw disable") >= 0 { + t.Errorf("the found firewall was retired by a flip that failed: %v", u.asked) + } + if len(u.rules) != 2 || u.index("ufw delete") >= 0 { + t.Errorf("the opening was removed by a flip that failed: %v", u.rules) + } + if _, statErr := os.Stat(guard); statErr != nil { + t.Errorf("the guard was removed by a flip that failed: %v", statErr) + } + for _, id := range []string{"adoption.guard", "adoption.opening-tcp-5671-incoming"} { + if _, ok := state.Find(id); !ok { + t.Errorf("%s was forgotten, so the next flip would never remove it", id) + } + } + + // Fixed, the next flip completes: filter, retire, and only then the guard and the openings. + if err := os.Remove(blocked); err != nil { + t.Fatal(err) + } + u.asked = nil + _, state, err = applyWith(t, converged, state, u.run) + if err != nil { + t.Fatal(err) + } + if u.active || len(u.rules) != 1 { + t.Errorf("the completed flip left ufw active %v, rules %v", u.active, u.rules) + } + if _, statErr := os.Stat(guard); !errors.Is(statErr, os.ErrNotExist) { + t.Errorf("the guard outlived the completed flip: %v", statErr) + } + if _, ok := state.Find("adoption.guard"); ok { + t.Error("the guard is still recorded after the completed flip") + } +}