Count a unit as found only when the machine runs it or starts it at boot, so a packaged unit nothing ran is not held (hq ADR 0103, found by the adoption bed)

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
@@ -285,7 +322,14 @@ func ApplyKeeping(
 		if o, isOpening := resource.(*declaration.Opening); isOpening {
 			outcome, err = applyOpening(ctx, o, run, fw)
 		} else {
-			outcome, err = applyOne(ctx, sys, resource, run, changed, declares, was, unseal)
+			// A file this host has no record of, under any id, is the machine's until the mesh
+			// writes over it — on any node, adopted or not: its original is kept first.
+			var keepFound Keep
+			if f, isFile := resource.(*declaration.File); isFile && was.ID == "" &&
+				!known.Recorded(string(declaration.TypeFile), f.Path) {
+				keepFound = keep
+			}
+			outcome, err = applyOne(ctx, sys, resource, run, changed, declares, was, unseal, keepFound)
 		}
 		if err != nil {
 			failed := &Error{Resource: resource.Identity(), Err: err, Done: report}
@@ -345,6 +389,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,18 +420,22 @@ 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)
 
 func applyOne(ctx context.Context, sys system.System, r declaration.Resource, run Runner,
 	changed map[string]bool, declares map[string]string, previous store.Applied,
-	unseal Unseal) (Outcome, error) {
+	unseal Unseal, keepFound Keep) (Outcome, error) {
 	switch res := r.(type) {
 	case *declaration.Directory:
 		return applyDirectory(res)
 	case *declaration.File:
-		return applyFile(res, previous, unseal)
+		return applyFile(res, previous, unseal, keepFound)
 	case *declaration.Service:
 		return applyService(ctx, sys, res, run, changed)
 	case *declaration.Package:
@@ -526,7 +580,9 @@ func applyAccess(r *declaration.Access) (Outcome, error) {
 	return out, nil
 }
 
-func applyFile(r *declaration.File, previous store.Applied, unseal Unseal) (Outcome, error) {
+// keepFound, when not nil, is where the original of a file this host has no record of is kept
+// before it is written over (novox/hq ADR 0100): once, never overwritten, and named in the outcome.
+func applyFile(r *declaration.File, previous store.Applied, unseal Unseal, keepFound Keep) (Outcome, error) {
 	if r.Into != "" {
 		return applyInto(r, previous)
 	}
@@ -623,7 +679,15 @@ func applyFile(r *declaration.File, previous store.Applied, unseal Unseal) (Outc
 	drifted := existed && previous.Wrote != "" && digestOf(string(existing)) != previous.Wrote
 	modeSame := existed && beforeMode == mode.Perm()
 
+	kept := ""
 	if !contentSame {
+		if existed && keepFound != nil {
+			// Before anything is written: a keep that fails stops the write, since the
+			// original could not be had back otherwise.
+			if kept, err = keepFound(r.Path, existing, beforeMode); err != nil {
+				return out, fmt.Errorf("keeping the original of %s before writing over it: %w", r.Path, err)
+			}
+		}
 		if err := os.MkdirAll(filepath.Dir(r.Path), 0o755); err != nil {
 			return out, err
 		}
@@ -687,6 +751,12 @@ func applyFile(r *declaration.File, previous store.Applied, unseal Unseal) (Outc
 	default:
 		out.Action = "unchanged"
 	}
+	if kept != "" {
+		if out.Detail != "" {
+			out.Detail += "; "
+		}
+		out.Detail += "the file found here, which the mesh had no record of, was kept at " + kept
+	}
 	return out, nil
 }
 

internal/apply/hold.go

@@ -72,13 +72,39 @@ func lookBefore(ctx context.Context, sys system.System, d *declaration.Declarati
 			if present(res.Path) && !recordedPath(known, res.Path) {
 				seen["path:"+res.Path] = true
 			}
+		case *declaration.Archive:
+			// Unpacking over it, and re-owning it recursively, would change the predecessor's
+			// files.
+			if present(res.Path) && !recordedPath(known, res.Path) {
+				seen["path:"+res.Path] = true
+			}
+		case *declaration.Process:
+			// Its unit would be written over and restarted.
+			if !known.Recorded(string(declaration.TypeProcess), res.Name) &&
+				present(filepath.Join(unitDir, res.Name+".service")) {
+				seen["unit-file:"+res.Name] = true
+			}
+		case *declaration.User:
+			// Its shell and groups would be changed.
+			if !known.Recorded(string(declaration.TypeUser), res.Name) {
+				if _, exists, err := system.LookUpUser(ctx, run, res.Name); err == nil && exists {
+					seen["user:"+res.Name] = true
+				}
+			}
 		case *declaration.Service:
 			if known.Recorded(string(declaration.TypeService), res.Unit) {
 				continue
 			}
-			// A unit the service manager cannot find is not there; one it can read is, whatever
-			// state it is in.
-			if _, err := sys.ServiceState(ctx, run, res.Unit); err == nil {
+			// Found is what the machine runs: a unit that is running or starts at boot. A unit
+			// file a package merely ships — a template instance nothing ever started, say the
+			// private network's own wg-quick@mesh0 — is not a predecessor's service, and holding
+			// it kept the private network from ever coming up (found by the adoption bed).
+			state, err := sys.ServiceState(ctx, run, res.Unit)
+			if err != nil {
+				continue
+			}
+			boot, _ := sys.ServiceBoot(ctx, run, res.Unit)
+			if state == "running" || boot == "enabled" {
 				seen["unit:"+res.Unit] = true
 			}
 		case *declaration.Container:
@@ -90,7 +116,7 @@ func lookBefore(ctx context.Context, sys system.System, d *declaration.Declarati
 				switch {
 				case src == "":
 				case strings.HasPrefix(src, "/"):
-					if present(src) && !recordedPath(known, src) {
+					if !systemPath(src) && present(src) && !recordedPath(known, src) {
 						seen["path:"+src] = true
 					}
 				default:
@@ -127,6 +153,26 @@ func recordedPath(known store.State, path string) bool {
 	return false
 }
 
+// systemPath is whether a bind-mount source is the machine's own plumbing — the runtime's socket,
+// the kernel's filesystems, the devices, the clock — which every machine has and no predecessor's
+// data lives in. Mounting it shares nothing that was found.
+func systemPath(src string) bool {
+	clean := filepath.Clean(src)
+	for _, exact := range []string{"/etc/localtime", "/etc/timezone", "/etc/hosts", "/etc/resolv.conf",
+		"/etc/machine-id", "/etc/passwd", "/etc/group"} {
+		if clean == exact {
+			return true
+		}
+	}
+	for _, under := range []string{"/run", "/var/run", "/sys", "/proc", "/dev", "/usr/share/zoneinfo",
+		"/etc/ssl", "/etc/ca-certificates", "/etc/pki", "/lib/modules", "/usr/lib/modules"} {
+		if clean == under || strings.HasPrefix(clean, under+"/") {
+			return true
+		}
+	}
+	return false
+}
+
 // mountSource is what a volume mapping mounts: a path on the machine, or a named volume. Empty for
 // an anonymous volume, which mounts nothing that could already be there.
 func mountSource(mapping string) string {
@@ -196,6 +242,15 @@ func holdOnAdopted(ctx context.Context, sys system.System, r declaration.Resourc
 		}
 	}
 
+	// A file written into replaces nothing that was found, so it is never held (novox/hq ADR
+	// 0102) — and a hold from when it was declared whole must not keep the mesh's keys out.
+	if f, ok := r.(*declaration.File); ok && f.Into != "" {
+		if already {
+			known.Release(r.Identity())
+		}
+		return false, false, out, nil
+	}
+
 	module, untaken := d.Adoption.UntakenModuleOf(r.Identity())
 	if !untaken {
 		return false, false, out, nil
@@ -239,6 +294,12 @@ func holdOnAdopted(ctx context.Context, sys system.System, r declaration.Resourc
 			}
 		case *declaration.Directory:
 			isFound = before["path:"+res.Path]
+		case *declaration.Archive:
+			isFound = before["path:"+res.Path]
+		case *declaration.Process:
+			isFound = before["unit-file:"+res.Name]
+		case *declaration.User:
+			isFound = before["user:"+res.Name]
 		case *declaration.Service:
 			isFound = before["unit:"+res.Unit]
 		}
@@ -396,6 +457,38 @@ func hold(ctx context.Context, sys system.System, r declaration.Resource, module
 			}
 		}
 		detail = "found on the machine; its mode, owner and contents kept until " + module + " is taken"
+	case *declaration.Archive:
+		if _, err := os.Lstat(res.Path); errors.Is(err, os.ErrNotExist) {
+			if !already {
+				return out, h, fmt.Errorf("%s was found and is gone before it could be held", res.Path)
+			}
+			changed = "gone"
+		} else if err != nil {
+			return out, h, err
+		}
+		detail = "something is already at " + res.Path + "; nothing unpacked over it or re-owned until " +
+			module + " is taken"
+	case *declaration.Process:
+		unit := filepath.Join(unitDir, res.Name+".service")
+		if !present(unit) {
+			if !already {
+				return out, h, fmt.Errorf("%s was found and is gone before it could be held", unit)
+			}
+			changed = "gone"
+		}
+		detail = "its unit " + unit + " was found on the machine; not written over or restarted until " +
+			module + " is taken"
+	case *declaration.User:
+		_, exists, err := system.LookUpUser(ctx, run, res.Name)
+		switch {
+		case err != nil:
+			return out, h, err
+		case !exists && !already:
+			return out, h, fmt.Errorf("the user %s was found and is gone before it could be held", res.Name)
+		case !exists:
+			changed = "gone"
+		}
+		detail = "the user was found on the machine; its shell and groups are kept until " + module + " is taken"
 	case *declaration.Service:
 		state, err := sys.ServiceState(ctx, run, res.Unit)
 		switch {

internal/apply/hold_test.go

@@ -24,6 +24,7 @@ type machine struct {
 	// named volumes.
 	units   map[string]*fakeUnit
 	volumes map[string]bool
+	users   map[string]bool
 }
 
 type fakeUnit struct {
@@ -80,6 +81,12 @@ func (m *machine) run(_ context.Context, name string, args ...string) (string, e
 	if name == "systemctl" {
 		return m.systemctl(args)
 	}
+	if name == "getent" {
+		if m.users[args[len(args)-1]] {
+			return args[len(args)-1] + ":x:1500:1500::/home/" + args[len(args)-1] + ":/bin/bash\n", nil
+		}
+		return "", errors.New("exit status 2")
+	}
 	if name != "docker" {
 		return "", nil
 	}
@@ -465,8 +472,40 @@ func TestAConvergedNodeStillReplacesWhatItFinds(t *testing.T) {
 	if len(state.Held) != 0 || outcomeOf(report, "hello-web.page").Action == "held" {
 		t.Errorf("a converged node held something: %+v", state.Held)
 	}
-	if _, err := os.Stat(filepath.Join(dir, "kept")); !errors.Is(err, os.ErrNotExist) {
-		t.Error("a converged node kept originals")
+	// What it writes over that it has no record of, it keeps first — on any node.
+	o := outcomeOf(report, "hello-web.page")
+	kept := o.Detail[strings.Index(o.Detail, "kept at ")+len("kept at "):]
+	if got, err := os.ReadFile(kept); err != nil || string(got) != "the predecessor's page\n" {
+		t.Errorf("the file written over was not kept, or not named: %q (%s) %v", got, o.Detail, err)
+	}
+}
+
+func TestAFileWrittenOverIsKeptOnceAndOnlyWhenTheHostHasNoRecordOfIt(t *testing.T) {
+	dir := t.TempDir()
+	conf := filepath.Join(dir, "nftables.conf")
+	_ = os.WriteFile(conf, []byte("# the distribution's own\n"), 0o644)
+	decl := func(content string) *declaration.Declaration {
+		return parse(t, `{"declaration":1,"resources":[{"id":"nftables.config","type":"file","path":"`+conf+
+			`","content":"`+content+`"}]}`)
+	}
+	m := &machine{containers: map[string]*fakeContainer{}}
+	report, state := applyAdopted(t, decl("table inet mesh {}\\n"), store.State{}, m, dir)
+	o := outcomeOf(report, "nftables.config")
+	if !strings.Contains(o.Detail, "had no record of, was kept at ") {
+		t.Fatalf("writing over an unrecorded file did not keep it: %+v", o)
+	}
+	kept := o.Detail[strings.Index(o.Detail, "kept at ")+len("kept at "):]
+	if got, _ := os.ReadFile(kept); string(got) != "# the distribution's own\n" {
+		t.Errorf("the kept original is %q", got)
+	}
+
+	// Now the mesh's: a later change keeps nothing more, and the first original stays.
+	report, _ = applyAdopted(t, decl("table inet mesh { }\\n"), state, m, dir)
+	if o := outcomeOf(report, "nftables.config"); o.Action != "updated" || strings.Contains(o.Detail, "kept at") {
+		t.Errorf("a file the mesh wrote was kept again: %+v", o)
+	}
+	if got, _ := os.ReadFile(kept); string(got) != "# the distribution's own\n" {
+		t.Errorf("the first original was overwritten: %q", got)
 	}
 }
 
@@ -531,7 +570,8 @@ func TestAFoundServiceOfAnUntakenModuleIsNeitherStartedNorEnabledNorRestarted(t
 	dir := t.TempDir()
 	conf := filepath.Join(dir, "hello.conf")
 	m := &machine{containers: map[string]*fakeContainer{},
-		units: map[string]*fakeUnit{"hello.service": {active: "inactive", enabled: "disabled"}}}
+		// The predecessor's unit: stopped just now, but it starts at boot, so it is the machine's.
+		units: map[string]*fakeUnit{"hello.service": {active: "inactive", enabled: "enabled"}}}
 	report, state := applyAdopted(t, adopted(t, untaken("hello-web.conf", "hello-web.unit"),
 		`{"id":"hello-web.conf","type":"file","path":"`+conf+`","content":"x\n"},
 		 {"id":"hello-web.unit","type":"service","unit":"hello.service","state":"running","boot":"enabled",
@@ -571,6 +611,27 @@ func TestAHeldServiceIsStillReloadedButNeverRestarted(t *testing.T) {
 	}
 }
 
+func TestAUnitAPackageOnlyShipsIsNotFound(t *testing.T) {
+	// The adoption bed found this: the private network's wg-quick@mesh0 is an instance of a unit
+	// the tunnel package ships. Nothing had ever run it, yet it was held as a predecessor's, and
+	// the private network never came up. Found is what the machine runs.
+	dir := t.TempDir()
+	m := &machine{containers: map[string]*fakeContainer{},
+		units: map[string]*fakeUnit{"wg-quick@mesh0.service": {active: "inactive", enabled: "disabled"}}}
+	report, state := applyAdopted(t, adopted(t, untaken("mesh-wireguard.overlay-up"),
+		`{"id":"mesh-wireguard.overlay-up","type":"service","unit":"wg-quick@mesh0.service","state":"running","boot":"enabled"}`),
+		store.State{}, m, dir)
+	if o := outcomeOf(report, "mesh-wireguard.overlay-up"); o.Action == "held" {
+		t.Fatalf("a unit nothing runs was held as a predecessor's: %+v", o)
+	}
+	if !m.did("systemctl start wg-quick@mesh0.service") || !m.did("systemctl enable wg-quick@mesh0.service") {
+		t.Errorf("the unit was not started and enabled: %v", m.asked)
+	}
+	if len(state.Held) != 0 {
+		t.Errorf("held: %+v", state.Held)
+	}
+}
+
 func TestAServiceWhoseUnitIsNotThereIsAppliedAsUsual(t *testing.T) {
 	// No unit before the apply: nothing of a predecessor's to hold.
 	dir := t.TempDir()
@@ -680,3 +741,87 @@ func TestAnActionInAHeldContainerIsHeldUntilItsModuleIsTaken(t *testing.T) {
 		t.Errorf("holds outlived the take: %+v", state.Held)
 	}
 }
+
+const sixtyFourZeros = "0000000000000000000000000000000000000000000000000000000000000000"
+
+func TestAnArchiveOverSomethingFoundIsNotUnpacked(t *testing.T) {
+	dir := t.TempDir()
+	at := filepath.Join(dir, "site")
+	if err := os.Mkdir(at, 0o750); err != nil {
+		t.Fatal(err)
+	}
+	theirs := filepath.Join(at, "index.html")
+	_ = os.WriteFile(theirs, []byte("the predecessor's site\n"), 0o640)
+	m := &machine{containers: map[string]*fakeContainer{}}
+	// The source is unreachable: fetching it would fail the apply, so a pass means it was not tried.
+	report, state := applyAdopted(t, adopted(t, untaken("hello-web.site"),
+		`{"id":"hello-web.site","type":"archive","source":"http://192.0.2.1/site.tar.gz",
+		  "digest":"sha256:`+sixtyFourZeros+`","path":"`+at+`","owner":"root"}`), store.State{}, m, dir)
+	if o := outcomeOf(report, "hello-web.site"); o.Action != "held" || !strings.Contains(o.Detail, "nothing unpacked") {
+		t.Errorf("an archive over found files was not held: %+v", o)
+	}
+	if got, _ := os.ReadFile(theirs); string(got) != "the predecessor's site\n" {
+		t.Errorf("the found files were touched: %q", got)
+	}
+	if _, ok := state.HeldAt("hello-web.site"); !ok {
+		t.Error("the hold was not recorded")
+	}
+}
+
+func TestAProcessWhoseUnitIsFoundIsNotWrittenOverOrRestarted(t *testing.T) {
+	dir := t.TempDir()
+	was := unitDir
+	unitDir = dir
+	t.Cleanup(func() { unitDir = was })
+	unit := filepath.Join(dir, "hello-daemon.service")
+	_ = os.WriteFile(unit, []byte("[Service]\nExecStart=/opt/predecessor/hello\n"), 0o644)
+	m := &machine{containers: map[string]*fakeContainer{}}
+	report, state := applyAdopted(t, adopted(t, untaken("hello-web.daemon"),
+		`{"id":"hello-web.daemon","type":"process","name":"hello-daemon","source":"http://192.0.2.1/d.tar.gz",
+		  "digest":"sha256:`+sixtyFourZeros+`","run":["hello"]}`), store.State{}, m, dir)
+	if o := outcomeOf(report, "hello-web.daemon"); o.Action != "held" || !strings.Contains(o.Detail, "not written over or restarted") {
+		t.Errorf("a process whose unit was found was not held: %+v", o)
+	}
+	if got, _ := os.ReadFile(unit); string(got) != "[Service]\nExecStart=/opt/predecessor/hello\n" {
+		t.Errorf("the found unit was written over: %q", got)
+	}
+	if m.did("systemctl") {
+		t.Errorf("the found unit was touched: %v", m.asked)
+	}
+	if _, ok := state.HeldAt("hello-web.daemon"); !ok {
+		t.Error("the hold was not recorded")
+	}
+}
+
+func TestAUserFoundOnTheMachineKeepsItsShellAndGroups(t *testing.T) {
+	dir := t.TempDir()
+	m := &machine{containers: map[string]*fakeContainer{}, users: map[string]bool{"hello": true}}
+	report, _ := applyAdopted(t, adopted(t, untaken("hello-web.user"),
+		`{"id":"hello-web.user","type":"user","name":"hello","shell":"/bin/zsh","groups":["docker"]}`),
+		store.State{}, m, dir)
+	if o := outcomeOf(report, "hello-web.user"); o.Action != "held" || !strings.Contains(o.Detail, "shell and groups") {
+		t.Errorf("a found user was not held: %+v", o)
+	}
+	for _, a := range m.asked {
+		if strings.HasPrefix(a, "usermod") || strings.HasPrefix(a, "useradd") {
+			t.Errorf("a found user was changed: %s", a)
+		}
+	}
+}
+
+func TestMountingTheMachinesOwnPlumbingIsNotFoundData(t *testing.T) {
+	// The runtime's socket, the kernel's filesystems and the clock are on every machine; a
+	// container mounting them shares nothing a predecessor kept (novox/hq ADR 0103).
+	dir := t.TempDir()
+	m := &machine{containers: map[string]*fakeContainer{}}
+	report, state := applyAdopted(t, adopted(t, untaken("hello-web.server"),
+		`{"id":"hello-web.server","type":"container","name":"hello-web","image":"`+pinned+`",
+		  "volumes":["/var/run/docker.sock:/var/run/docker.sock","/etc/localtime:/etc/localtime:ro",
+		             "/proc/cpuinfo:/host/cpuinfo:ro","/dev/null:/data/null"]}`), store.State{}, m, dir)
+	if o := outcomeOf(report, "hello-web.server"); o.Action != "created" {
+		t.Errorf("a container mounting only system paths was not created: %+v", o)
+	}
+	if len(state.Held) != 0 {
+		t.Errorf("held: %+v", state.Held)
+	}
+}

internal/apply/into_test.go

@@ -281,3 +281,24 @@ func TestAListTheMeshCreatedGoesWhenEmptied(t *testing.T) {
 		t.Errorf("the key the mesh created was not removed: %v", o)
 	}
 }
+
+func TestAHoldFromAWholeFileDoesNotKeepOutAnIntoWrite(t *testing.T) {
+	// Declared whole before, the runtime's file was held; declared into now, it is written into.
+	path := filepath.Join(t.TempDir(), "daemon.json")
+	_ = os.WriteFile(path, []byte(machinesOwn), 0o644)
+	known := store.State{Held: []store.Held{{ID: "networking.registry-trust", Module: "networking",
+		Kind: "file", Target: path}}}
+	d := adopted(t, `{"taken":[],"untaken":{"networking":["networking.registry-trust"]}}`,
+		fmt.Sprintf(`{"id":"networking.registry-trust","type":"file","path":%q,"into":"json","content":%q}`,
+			path, `{"insecure-registries":["10.42.0.1:5000"]}`))
+	report, state := applyAdopted(t, d, known, &machine{}, t.TempDir())
+	if got := outcomeOf(report, "networking.registry-trust").Action; got != "updated" {
+		t.Errorf("the file was %q, not written into", got)
+	}
+	if len(state.Held) != 0 {
+		t.Errorf("the old hold outlived the into declaration: %+v", state.Held)
+	}
+	if fmt.Sprint(readObject(t, path)["insecure-registries"]) != "[192.0.2.7:5000 10.42.0.1:5000]" {
+		t.Errorf("the mesh's member was not written in: %v", readObject(t, path))
+	}
+}

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:], " ")
@@ -304,3 +329,93 @@ 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)
+	}
+}
+
+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/apply/process.go

@@ -32,8 +32,9 @@ import (
 // owners end up disagreeing about one path.
 const daemonRoot = "/var/lib/mesh/daemons"
 
-// unitDir is where the mesh writes the units it owns.
-const unitDir = "/etc/systemd/system"
+// unitDir is where the mesh writes the units it owns. A variable only so a test can point it at a
+// directory of its own.
+var unitDir = "/etc/systemd/system"
 
 func applyProcess(ctx context.Context, r *declaration.Process, run Runner,
 	changed map[string]bool, previous store.Applied) (Outcome, error) {

internal/bootstrap/adopted.go

@@ -87,8 +87,11 @@ func guardResources(ports []int) []map[string]any {
 	return []map[string]any{
 		{"id": guardID, "type": "file", "path": guardPath, "content": AsGuard(ports), "mode": "0644"},
 		{"id": guardUnitID, "type": "file", "path": guardUnitPath, "content": guardUnitText(), "mode": "0644"},
+		// A changed table is reloaded — the unit's ExecReload loads it in one transaction, so the
+		// ports are never unguarded — and only a changed unit restarts it. As the controller
+		// declares it, so the first push finds nothing different.
 		{"id": guardRunningID, "type": "service", "unit": guardUnit, "state": "running",
-			"boot": "enabled", "restart-on": []any{guardID, guardUnitID}},
+			"boot": "enabled", "reload-on": []any{guardID}, "restart-on": []any{guardUnitID}},
 	}
 }
 

internal/bootstrap/adopted_test.go

@@ -121,7 +121,10 @@ func TestAnAdoptedBundleLoadsNoDroppingTableAndExactlyTheGuard(t *testing.T) {
 		t.Error("nft, which loads the guard, is no longer installed")
 	}
 	unit := r.Declaration.Resources[at[guardRunningID]].(*declaration.Service)
-	if unit.Unit != guardUnit || unit.State != "running" || strings.Join(unit.RestartOn, ",") != guardID+","+guardUnitID {
+	// A changed table is reloaded, never restarted: a restart deletes the table before loading
+	// it again, leaving the ports unguarded in between.
+	if unit.Unit != guardUnit || unit.State != "running" || strings.Join(unit.RestartOn, ",") != guardUnitID ||
+		strings.Join(unit.ReloadOn, ",") != guardID {
 		t.Errorf("the guard's service: %+v", unit)
 	}
 	stop := r.Declaration.Resources[at[guardUnitID]].(*declaration.File).Content

internal/firewall/firewall.go

@@ -479,6 +479,7 @@ func words(rule string) []string {
 type ufwRule struct {
 	route              bool
 	action, in, out    string
+	log                string
 	from, fromPort, to string
 	port, proto, app   string
 	comment            string
@@ -488,8 +489,23 @@ type ufwRule struct {
 // in on mesh0 to any port 5432 proto tcp` — into the fields ufw compares. Not ok for anything it
 // does not recognise, which is then never taken to answer an opening.
 func parseRule(rule string) (ufwRule, bool) {
-	w := words(rule)
 	r := ufwRule{from: "any", to: "any", comment: comment(rule)}
+	// A log type may stand after the action or after the direction; either way it is a property
+	// of the rule, not of where it matches. The word after `comment` is the comment, whatever it
+	// says.
+	var w []string
+	all := words(rule)
+	for i := 0; i < len(all); i++ {
+		switch {
+		case all[i] == "comment" && i+1 < len(all):
+			w = append(w, all[i], all[i+1])
+			i++
+		case all[i] == "log" || all[i] == "log-all":
+			r.log = all[i]
+		default:
+			w = append(w, all[i])
+		}
+	}
 	i := 0
 	if i < len(w) && w[i] == "route" {
 		r.route = true
@@ -585,9 +601,13 @@ func isPorts(s string) bool {
 	return true
 }
 
-// sameAs is whether ufw would take two rules for one — everything but the comment equal.
+// sameAs is whether ufw would take two rules for one: everything equal but the comment, the
+// action and the log type. Adding one beside the other updates it in place — its comment, and its
+// action or log type — rather than adding a second.
 func (r ufwRule) sameAs(o ufwRule) bool {
 	r.comment, o.comment = "", ""
+	r.action, o.action = "", ""
+	r.log, o.log = "", ""
 	return r == o
 }
 
@@ -658,6 +678,7 @@ func Converge(ctx context.Context, run Runner, o *declaration.Opening) (Converge
 	present := false
 	var stale []string
 	satisfiedBy := ""
+	want, _ := parseRule(strings.Join(Rule(o), " "))
 	for _, rule := range rules {
 		c := comment(rule)
 		switch {
@@ -666,7 +687,21 @@ func Converge(ctx context.Context, run Runner, o *declaration.Opening) (Converge
 		case markedFor(c, o.ID):
 			stale = append(stale, rule)
 		default:
-			if parsed, ok := parseRule(rule); ok && satisfiedBy == "" && parsed.admits(o) {
+			parsed, ok := parseRule(rule)
+			if !ok {
+				continue
+			}
+			// ufw would take the mesh's rule for this one and rewrite its action or log type:
+			// an operator's refusal, or a limit, would silently become an allow — and removing the
+			// opening would then delete it. A plain allow answers the opening; anything else is a
+			// conflict the operator decides (novox/hq ADR 0103).
+			if parsed.sameAs(want) && (parsed.action != "allow" || parsed.log != "") {
+				return Converged{}, fmt.Errorf("ufw holds %q, which ufw takes for the same rule as the "+
+					"mesh's opening for %s, differing in what it does; adding the opening would change "+
+					"it, so nothing was added. Change or remove that rule, or have the mesh stop "+
+					"declaring the opening", rule, o.Target())
+			}
+			if satisfiedBy == "" && parsed.admits(o) {
 				satisfiedBy = rule
 			}
 		}
@@ -771,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.
-func Disable(ctx context.Context, run Runner) 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})
-		}
-	}
+// was drop is put back and read back. before is ForwardPolicies as read before the first attempt.
+func Disable(ctx context.Context, run Runner, before map[string]string) error {
 	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 {
-		if f.policy != "DROP" {
+	for _, tool := range []string{"iptables", "ip6tables"} {
+		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)
 	}
 }
@@ -658,7 +658,6 @@ func TestARuleThatDoesNotAnswerTheOpeningLeavesItToBeAdded(t *testing.T) {
 		"allow from 192.0.2.0/24 to any port 5671 proto tcp", // narrower: from one range
 		"allow in on eth0 to any port 5671 proto tcp",        // narrower: one interface
 		"allow 5671/udp",                         // another protocol
-		"deny 5671/tcp",                          // refuses
 		"route allow 5671/tcp",                   // another path
 		"allow to 192.0.2.1 port 5671 proto tcp", // one address
 	} {
@@ -681,3 +680,39 @@ func TestAnOpeningWhoseFoundRuleIsGoneIsAddedAgain(t *testing.T) {
 		t.Fatalf("the opening was not added once nothing answered it: %+v %v", done, err)
 	}
 }
+
+func TestARuleUfwWouldMergeThatDoesOtherThanAllowRefusesTheOpening(t *testing.T) {
+	// ufw takes two rules differing only in action or log type for one, and adding the mesh's
+	// would turn the operator's refusal into an allow (novox/hq ADR 0103).
+	for _, operators := range []string{
+		"deny 5671/tcp",
+		"reject 5671/tcp",
+		"limit 5671/tcp",
+		"allow log 5671/tcp",
+		"allow log-all proto tcp to any port 5671",
+		"deny in log to any port 5671 proto tcp comment 'operator note'",
+	} {
+		f := &fakeUFW{installed: true, active: true, rules: []string{operators}}
+		_, err := Converge(context.Background(), f.run, opening("adoption.bus", 5671, "everywhere", "incoming", 0))
+		if err == nil || !strings.Contains(err.Error(), operators) {
+			t.Errorf("%q: the conflict was not refused naming the rule: %v", operators, err)
+		}
+		if f.added() != 0 || len(f.rules) != 1 || f.rules[0] != operators {
+			t.Errorf("%q: something was added or changed: %v %v", operators, f.asked, f.rules)
+		}
+	}
+}
+
+func TestALogTypeIsReadInEitherPlace(t *testing.T) {
+	for rule, want := range map[string]string{
+		"allow log 22/tcp": "allow log 22 tcp in=",
+		"allow in log-all on mesh0 to any port 5432 proto tcp": "allow log-all 5432 tcp in=mesh0",
+		"route deny log in on mesh0 to any port 80 proto tcp":  "deny log 80 tcp in=mesh0",
+		"allow 22/tcp comment 'log'":                           "allow  22 tcp in=",
+	} {
+		r, ok := parseRule(rule)
+		if got := r.action + " " + r.log + " " + r.port + " " + r.proto + " in=" + r.in; !ok || got != want {
+			t.Errorf("%q read as %q (%v), want %q", rule, got, ok, want)
+		}
+	}
+}

internal/store/store.go

@@ -115,8 +115,12 @@ type FoundFirewall struct {
 	WasActive bool `json:"was_active,omitempty"`
 	// 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"`
-	FoundAt        time.Time `json:"found_at"`
+	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"`
 }
 
 // Held is one thing found on an adopted node — a file, directory or container present at a declared