Record the forward policies before disabling ufw, so a retried retirement restores them (hq ADR 0100)

internal/apply/opening.go

@@ -30,6 +30,7 @@ func foundFirewall(ctx context.Context, d *declaration.Declaration, known *store
 			return "", err
 		}
 		rec.DisabledByMesh = false
+		rec.Forward = nil
 		log("  enabled ufw again: this node is adopted, and the firewall found on it is in force")
 	}
 	kind, name, err := firewall.Detect(ctx, run)
@@ -68,7 +69,12 @@ func retireFirewall(ctx context.Context, d *declaration.Declaration, origin stri
 		rec.DisabledByMesh {
 		return nil
 	}
-	if err := firewall.Disable(ctx, run); err != nil {
+	if rec.Forward == nil {
+		// Recorded before ufw is touched: disabling it opens the forward policy, and a retry
+		// must know what it was (novox/hq ADR 0100).
+		rec.Forward = firewall.ForwardPolicies(ctx, run)
+	}
+	if err := firewall.Disable(ctx, run, rec.Forward); err != nil {
 		return err
 	}
 	rec.DisabledByMesh = true

internal/apply/opening_test.go

@@ -21,6 +21,23 @@ type ufwMachine struct {
 	rules             []string
 	ruleset           string
 	asked             []string
+
+	// forward is iptables' forward policy when set; empty is a machine without iptables. failP
+	// is how many -P calls fail before one succeeds.
+	forward string
+	failP   int
+}
+
+func (u *ufwMachine) iptables(args []string) (string, error) {
+	if len(args) == 3 && args[0] == "-P" {
+		if u.failP > 0 {
+			u.failP--
+			return "", errors.New("iptables: resource temporarily unavailable")
+		}
+		u.forward = args[2]
+		return "", nil
+	}
+	return "-P FORWARD " + u.forward + "\n-A FORWARD -j DOCKER-USER\n", nil
 }
 
 func (u *ufwMachine) run(_ context.Context, name string, args ...string) (string, error) {
@@ -28,6 +45,11 @@ func (u *ufwMachine) run(_ context.Context, name string, args ...string) (string
 	switch name {
 	case "nft":
 		return u.ruleset, nil
+	case "iptables":
+		if u.forward == "" {
+			return "", &exec.Error{Name: name, Err: exec.ErrNotFound}
+		}
+		return u.iptables(args)
 	case "ufw":
 		if !u.installed {
 			return "", &exec.Error{Name: name, Err: exec.ErrNotFound}
@@ -52,6 +74,9 @@ func (u *ufwMachine) run(_ context.Context, name string, args ...string) (string
 		return "", nil
 	case "disable":
 		u.active = false
+		if u.forward != "" {
+			u.forward = "ACCEPT" // as measured: ufw disable opens the forward policy
+		}
 		return "", nil
 	case "delete":
 		want := strings.Join(args[1:], " ")
@@ -370,3 +395,27 @@ func TestAStaleOpeningOnAnAdoptedNodeIsRemovedAsAnyOrphan(t *testing.T) {
 		t.Errorf("the stale opening was not removed before the new one was added: %v", u.asked)
 	}
 }
+
+func TestARetiredFirewallRetriedStillPutsBackTheForwardPolicy(t *testing.T) {
+	// The forward policy is recorded before ufw is disabled, so a retirement that failed after
+	// the disable restores what the machine had, not what the disable left (novox/hq ADR 0100).
+	dir := t.TempDir()
+	u := &ufwMachine{installed: true, active: true, forward: "DROP", failP: 1}
+	_, state, err := applyWith(t, adopted(t, `{"taken":[]}`, withConf(dir)), store.State{}, u.run)
+	if err != nil {
+		t.Fatal(err)
+	}
+	converged := parse(t, `{"declaration":1,"resources":[`+withConf(dir)+`]}`)
+	if _, state, err = applyWith(t, converged, state, u.run); err == nil {
+		t.Fatal("the failed restore was not reported")
+	}
+	if u.active || u.forward != "ACCEPT" || state.Firewall.Forward["iptables"] != "DROP" {
+		t.Fatalf("after the failed attempt: active %v, forward %s, recorded %+v", u.active, u.forward, state.Firewall)
+	}
+	if _, state, err = applyWith(t, converged, state, u.run); err != nil {
+		t.Fatal(err)
+	}
+	if u.forward != "DROP" || !state.Firewall.DisabledByMesh {
+		t.Errorf("the retry did not put the forward policy back: %s, %+v", u.forward, state.Firewall)
+	}
+}

internal/firewall/firewall.go

@@ -806,40 +806,46 @@ func Enable(ctx context.Context, run Runner) error {
 // runtime had set that one to drop when it turned forwarding on, and it does not set it again while
 // forwarding stays on — not even on a restart. Left so, a retired ufw turns the machine into a
 // router for anyone who can reach it. So each family's forward policy is read before, and one that
-// was drop is put back and read back.
+// was drop is put back and read back. before is ForwardPolicies as read before the first attempt.
-func Disable(ctx context.Context, run Runner) error {
+func Disable(ctx context.Context, run Runner, before map[string]string) error {
-	type family struct{ tool, policy string }
-	var before []family
-	for _, tool := range []string{"iptables", "ip6tables"} {
-		if policy, ok := forwardPolicy(ctx, run, tool); ok {
-			before = append(before, family{tool, policy})
-		}
-	}
 	if _, err := run(ctx, "ufw", "disable"); err != nil {
 		return fmt.Errorf("disabling ufw: %w", err)
 	}
 	if err := expectActive(ctx, run, false); err != nil {
 		return err
 	}
-	for _, f := range before {
+	for _, tool := range []string{"iptables", "ip6tables"} {
-		if f.policy != "DROP" {
+		if before[tool] != "DROP" {
 			continue
 		}
-		if now, ok := forwardPolicy(ctx, run, f.tool); ok && now == "DROP" {
+		if now, ok := forwardPolicy(ctx, run, tool); ok && now == "DROP" {
 			continue
 		}
-		if _, err := run(ctx, f.tool, "-P", "FORWARD", "DROP"); err != nil {
+		if _, err := run(ctx, tool, "-P", "FORWARD", "DROP"); err != nil {
 			return fmt.Errorf("ufw is disabled, and %s's forward policy, which was drop, could not be put back: %w",
-				f.tool, err)
+				tool, err)
 		}
-		if now, ok := forwardPolicy(ctx, run, f.tool); !ok || now != "DROP" {
+		if now, ok := forwardPolicy(ctx, run, tool); !ok || now != "DROP" {
 			return fmt.Errorf("ufw is disabled, and %s's forward policy was put back to drop and reads %q",
-				f.tool, now)
+				tool, now)
 		}
 	}
 	return nil
 }
 
+// ForwardPolicies reads each family's forward policy, by the tool that sets it. Read before ufw is
+// disabled and kept by the caller, so a retirement that fails half-way is retried with what the
+// machine had — not with what the half-done disable left.
+func ForwardPolicies(ctx context.Context, run Runner) map[string]string {
+	out := map[string]string{}
+	for _, tool := range []string{"iptables", "ip6tables"} {
+		if policy, ok := forwardPolicy(ctx, run, tool); ok {
+			out[tool] = policy
+		}
+	}
+	return out
+}
+
 // forwardPolicy reads the forward chain's policy the way iptables prints it: "-P FORWARD DROP".
 // Not ok when the tool is absent or says nothing readable.
 func forwardPolicy(ctx context.Context, run Runner, tool string) (string, bool) {

internal/firewall/firewall_test.go

@@ -356,7 +356,7 @@ func TestRemovingAnOpeningRemovesOnlyWhatWasMarkedForIt(t *testing.T) {
 
 func TestEnableAndDisableReadBack(t *testing.T) {
 	f := &fakeUFW{installed: true, active: true}
-	if err := Disable(context.Background(), f.run); err != nil || f.active {
+	if err := Disable(context.Background(), f.run, nil); err != nil || f.active {
 		t.Fatalf("disable: %v", err)
 	}
 	if err := Enable(context.Background(), f.run); err != nil || !f.active {
@@ -493,7 +493,7 @@ func TestRetiringUfwKeepsTheMachineFromRoutingForOthers(t *testing.T) {
 		t.Fatal("the captures no longer show ufw disable opening the forward policy")
 	}
 	f := &fakeUFW{installed: true, active: true, iptablesActive: string(before), iptablesInactive: string(after)}
-	if err := Disable(context.Background(), f.run); err != nil {
+	if err := Disable(context.Background(), f.run, ForwardPolicies(context.Background(), f.run)); err != nil {
 		t.Fatal(err)
 	}
 	if f.active {
@@ -511,7 +511,7 @@ func TestRetiringUfwKeepsTheMachineFromRoutingForOthers(t *testing.T) {
 
 func TestRetiringUfwOnAMachineWithoutIptablesStillRetiresIt(t *testing.T) {
 	f := &fakeUFW{installed: true, active: true}
-	if err := Disable(context.Background(), f.run); err != nil || f.active {
+	if err := Disable(context.Background(), f.run, nil); err != nil || f.active {
 		t.Fatalf("disable: %v, active %v", err, f.active)
 	}
 }

internal/store/store.go

@@ -116,6 +116,10 @@ type FoundFirewall struct {
 	// DisabledByMesh is set when converging retired it, so returning to adopted enables it again
 	// and nothing else ever does.
 	DisabledByMesh bool      `json:"disabled_by_mesh,omitempty"`
+	// Forward is each family's forward policy as it was before the mesh disabled the firewall,
+	// by the tool that sets it — recorded before, so a retirement retried puts back what the
+	// machine had.
+	Forward map[string]string `json:"forward,omitempty"`
 	FoundAt        time.Time `json:"found_at"`
 }