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

@@ -185,21 +185,53 @@ func ApplyKeeping(
 		return nil
 	}
 
-	// **What protects an adopted node goes last** (novox/hq ADR 0103). The openings and the guard
+	// **What protects an adopted node goes last on the flip, and first on the way back** (novox/hq
-	// are what keep the mesh reachable through the found firewall and the store unreachable from
+	// ADR 0103). The openings and the guard are what keep the mesh reachable through the found
-	// outside. When a node is converged they leave the declaration, and removing them first would
+	// firewall and the store unreachable from outside.
-	// 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
+	// When a node is converged they leave the declaration, and removing them first would leave the
-	// and the found firewall is retired; if anything failed, they stay, recorded, for the next try.
+	// store open from the moment the guard stops until the derived filter loads — and for ever, if
-	var protecting []store.Applied
+	// 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 {
+		orphans = append(orphans, orphan)
-			return report, known, err
+	}
+	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)

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)
+	}
+}