From 174c8b5d53736da8c30bf586c2af9fb2b5961bac Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 18:19:22 +0200 Subject: [PATCH 1/8] Judge every path the controller resolves for a directory or file where the plan is composed, so the merge gate refuses a directory in Docker's data (issue 496) --- internal/catalogue/dir_into.go | 5 +- internal/catalogue/placement.go | 79 ++++++++++++++++ internal/catalogue/resolved_path_test.go | 109 +++++++++++++++++++++++ 3 files changed, 192 insertions(+), 1 deletion(-) create mode 100644 internal/catalogue/resolved_path_test.go diff --git a/internal/catalogue/dir_into.go b/internal/catalogue/dir_into.go index 585358e3..3a9a919f 100644 --- a/internal/catalogue/dir_into.go +++ b/internal/catalogue/dir_into.go @@ -257,7 +257,10 @@ func dirInto(resource map[string]any, dirs map[string]string, module string) err } resource["env-file"] = filled } - return nil + // **Placed, then judged** (novox/hq issue 496): the path a directory or file resolves to — the default + // layout, a place, a stated path, one beneath a ${dir:…} — is judged here, where every plan and the merge + // gate compose it, so a module whose directory lands in Docker's data is refused before it reaches a machine. + return judgeResolved(resource, module) } // unknownDirRefs is every ${dir:…} in the definition that names no directory the definition diff --git a/internal/catalogue/placement.go b/internal/catalogue/placement.go index b552ba30..24d1000b 100644 --- a/internal/catalogue/placement.go +++ b/internal/catalogue/placement.go @@ -92,6 +92,81 @@ func systemPath(path string) string { return "" } +// Where no directory or file the controller resolves for a module may be, from any route (novox/hq issue 496). +// +// mesh-catalog #205 gave docker's `state` directory `"place": "."`, which resolves to /docker: /var/lib/docker, +// every container's filesystem. systemPath judged only the `places` and `accesses` settings, so the default layout +// and a definition's own paths reached the merge gate unjudged; it composed every machine and passed, and only the +// node-engine refused the directory, at apply, failing the walk. So every path a directory or file resolves to is +// judged where the plan is composed, which is also where the gate composes. +// +// Not by systemPath's whole list: a module writes the machine's configuration in /etc and /usr by design (units, +// jails, a launcher), keeps run directories in /run, an account's keys in its .ssh, and the mesh places its own files +// for every module in /var/lib/mesh. Those are the node-engine's to judge with what only the machine knows (owners, +// links, where the runtimes really keep their data). Refused here is what no module ever writes, whoever owns it: +// the trees in neverWritten, the node-engine's own trees but by its own module, and a path that is one of the +// machine's roots or holds one. Each is refused by the node-engine too (its placement guard, issue 339), so nothing +// that applies today is refused here. +var ( + neverWritten = []string{"/boot", "/root", "/proc", "/sys", "/dev", "/bin", "/sbin", "/lib", "/lib32", "/lib64", + "/var/spool", "/var/lib/docker", "/var/lib/containers", "/var/lib/containerd", "/opt"} + engineTrees = []string{"/var/lib/mesh-host", "/usr/lib/nox-mesh-host"} + // engineModule is the node-engine's own module, the one that places in engineTrees. + engineModule = "mesh-host" + // usrRoots are the roots below /usr that hold every program and library: never a module's directory itself. + usrRoots = []string{"/usr/bin", "/usr/lib", "/usr/lib64", "/usr/local", "/usr/local/bin", "/usr/local/lib", + "/usr/local/sbin", "/usr/sbin", "/usr/share"} +) + +// resolvedSystemPath says why a path resolved for one of module's directories or files is the machine's own, or "". +func resolvedSystemPath(path, module string) string { + if !strings.HasPrefix(path, "/") { + return "" + } + path = filepath.Clean(path) + in := func(tree string) bool { return path == tree || strings.HasPrefix(path, tree+"/") } + for _, tree := range neverWritten { + if in(tree) { + return path + " is in " + tree + ", which no module writes in" + } + } + if module != engineModule { + for _, tree := range engineTrees { + if in(tree) { + return path + " is in " + tree + ", the node-engine's own, which only its module places in" + } + } + } + for _, roots := range [][]string{systemRoots, systemTrees, usrRoots} { + for _, root := range roots { + if path == root { + return path + " is one of the machine's roots, the parent of everything in it" + } + } + } + for _, tree := range append(append([]string(nil), systemTrees...), neverWritten...) { + if strings.HasPrefix(tree, path+"/") { + return path + " holds " + tree + ", the machine's own or the mesh's state" + } + } + return "" +} + +// judgeResolved refuses a directory or file whose resolved path resolvedSystemPath refuses (novox/hq issue 496). +func judgeResolved(resource map[string]any, module string) error { + kind := fmt.Sprint(resource["type"]) + if kind != "directory" && kind != "file" { + return nil + } + path, _ := resource["path"].(string) + if why := resolvedSystemPath(path, module); why != "" { + return fmt.Errorf("%s's %s %q resolves to %s: %s. The node-engine refuses it at apply, so it is refused "+ + "where the plan is composed, and the merge gate with it (novox/hq issue 496)", + module, kind, fmt.Sprint(resource["id"]), filepath.Clean(path), why) + } + return nil +} + // accessRef is how a module names one of its accesses: ${access:}. var accessRef = regexp.MustCompile(`\$\{access:([a-z0-9][a-z0-9-]*)\}`) @@ -284,6 +359,10 @@ func accessInto(resource map[string]any, accesses map[string]string, module stri if resource["path"], err = fill(path); err != nil { return err } + // Judged again once the access is in it: dirInto judged it while it still named one (novox/hq issue 496). + if err := judgeResolved(resource, module); err != nil { + return err + } } if content, ok := resource["content"].(string); ok { if resource["content"], err = fill(content); err != nil { diff --git a/internal/catalogue/resolved_path_test.go b/internal/catalogue/resolved_path_test.go new file mode 100644 index 00000000..4872bd45 --- /dev/null +++ b/internal/catalogue/resolved_path_test.go @@ -0,0 +1,109 @@ +package catalogue + +// Every path the controller resolves for a module's directories and files is judged where the plan is +// composed (novox/hq issue 496). mesh-catalog #205 gave docker's `state` directory `"place": "."`, which +// resolves to /docker — /var/lib/docker, every container's filesystem. The merge gate composed every +// machine and passed it; only the node-engine refused it, at apply, and failed the walk. The gate composes +// through Declaration, so a refusal here is a refusal at the gate, for every module. + +import ( + "strings" + "testing" +) + +func composedOn(m Manifest, with Rendering) ([]map[string]any, error) { + return Resolution{Node: "anchor", Modules: []Manifest{m}}.Declaration(with) +} + +func pathOf(t *testing.T, resources []map[string]any, id string) string { + t.Helper() + for _, r := range resources { + if r["id"] == id { + return r["path"].(string) + } + } + t.Fatalf("no resource %s in %v", id, resources) + return "" +} + +func TestADirectoryPlacedInDockersDataIsRefusedAtComposition(t *testing.T) { + // The #205 shape, as it was merged. + m := Manifest{Module: "docker", Version: "1", Resources: []map[string]any{ + {"id": "state", "type": "directory", "place": "."}, + }} + _, err := composedOn(m, Rendering{}) + if err == nil { + t.Fatal("a directory resolving to /var/lib/docker composed; the node-engine refuses it at apply") + } + for _, said := range []string{"docker", `"state"`, "/var/lib/docker", "issue 496"} { + if !strings.Contains(err.Error(), said) { + t.Errorf("the refusal names the module, the resource, the path and why; %q is missing from %q", said, err) + } + } +} + +func TestTheMeshsPlaceForDockerComposes(t *testing.T) { + // The fix #205 needed: the mesh's own directory for the module, /mesh/docker. + m := Manifest{Module: "docker", Version: "1", Resources: []map[string]any{ + {"id": "state", "type": "directory", "place": "mesh"}, + {"id": "marker", "type": "file", "path": "${dir:state}/applied", "content": "x"}, + }} + resources, err := composedOn(m, Rendering{}) + if err != nil { + t.Fatalf("place %q is the mesh's tree, which the mesh writes for every module: %v", "mesh", err) + } + if got := pathOf(t, resources, "docker.state"); got != "/var/lib/mesh/docker" { + t.Fatalf("got %q", got) + } +} + +func TestEveryResolvedPathIsJudged(t *testing.T) { + refused := map[string]Manifest{ + "a default directory under a data root inside containerd's data": {Module: "images", Version: "1", + Resources: []map[string]any{{"id": "cache", "type": "directory"}}}, + "a stated directory in docker's data": {Module: "sidecar", Version: "1", + Resources: []map[string]any{{"id": "volumes", "type": "directory", "path": "/var/lib/docker/volumes/x"}}}, + "a file beneath a placed directory that climbs out of it": {Module: "docker", Version: "1", + Resources: []map[string]any{{"id": "state", "type": "directory", "place": "mesh"}, + {"id": "f", "type": "file", "path": "${dir:state}/../../containers/x", "content": "x"}}}, + "a file in /boot": {Module: "grub", Version: "1", + Resources: []map[string]any{{"id": "cfg", "type": "file", "path": "/boot/grub/custom.cfg", "content": "x"}}}, + "a directory that is the data root itself": {Module: "lib", Version: "1", + Resources: []map[string]any{{"id": "all", "type": "directory", "place": "."}}}, + "a directory in the node-engine's own tree, by another module": {Module: "intruder", Version: "1", + Resources: []map[string]any{{"id": "d", "type": "directory", "path": "/var/lib/mesh-host/x"}}}, + } + roots := map[string]Rendering{ + "a default directory under a data root inside containerd's data": {DataRoot: "/var/lib/containerd"}, + "a directory that is the data root itself": {DataRoot: "/var"}, + } + for name, m := range refused { + if _, err := composedOn(m, roots[name]); err == nil || !strings.Contains(err.Error(), "issue 496") { + t.Errorf("%s: composed or refused for another reason: %v", name, err) + } + } + + // What the mesh's own modules place today, on every machine (read from each machine's live plan): the + // machine's configuration, the mesh's tree, the node-engine's own, run directories and an account's keys. + passes := map[string]Manifest{ + "a unit in /etc": {Module: "power", Version: "1", + Resources: []map[string]any{{"id": "d", "type": "directory", "path": "/etc/systemd/system/x.service.d"}, + {"id": "u", "type": "file", "path": "/etc/systemd/system/x.service.d/a.conf", "content": "x"}}}, + "a run directory": {Module: "fail2ban", Version: "1", + Resources: []map[string]any{{"id": "run-dir", "type": "directory", "path": "/var/run/fail2ban"}}}, + "a program in /usr/local": {Module: "claude-code", Version: "1", + Resources: []map[string]any{{"id": "start", "type": "file", "path": "/usr/local/bin/claude-agent", "content": "x"}}}, + "the node-engine's launcher, by the node-engine": {Module: "mesh-host", Version: "1", + Resources: []map[string]any{{"id": "launcher", "type": "file", "path": "/usr/lib/nox-mesh-host/launch", "content": "x"}}}, + "an account's .ssh": {Module: "ssh-client", Version: "1", + Resources: []map[string]any{{"id": "ssh-dir", "type": "directory", "path": "/home/someone/.ssh"}, + {"id": "config", "type": "file", "path": "/home/someone/.ssh/config", "content": "x"}}}, + "a module's own root": {Module: "mailu", Version: "1", + Resources: []map[string]any{{"id": "state", "type": "directory", "place": "."}}}, + } + for name, m := range passes { + if _, err := composedOn(m, Rendering{}); err != nil { + t.Errorf("%s: refused: %v", name, err) + } + } +} From 8115f1ac42bf8291a2c9c05b2086b920df8b6999 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 18:59:07 +0200 Subject: [PATCH 2/8] Judge a definition's resolved paths where the definition is judged, by the node-engine's own rules, so a refusal never freezes a machine (review of #228, issue 496) --- internal/catalogue/dir_into.go | 5 +- internal/catalogue/engine_guard_test.go | 71 +++ internal/catalogue/manifest.go | 1 + internal/catalogue/placement.go | 134 +++-- internal/catalogue/resolved_path_test.go | 168 +++--- testdata/beside/CAPTURED | 8 + .../apply/placement_guard.go.captured | 549 ++++++++++++++++++ 7 files changed, 800 insertions(+), 136 deletions(-) create mode 100644 internal/catalogue/engine_guard_test.go create mode 100644 testdata/beside/mesh-host/internal/apply/placement_guard.go.captured diff --git a/internal/catalogue/dir_into.go b/internal/catalogue/dir_into.go index 3a9a919f..585358e3 100644 --- a/internal/catalogue/dir_into.go +++ b/internal/catalogue/dir_into.go @@ -257,10 +257,7 @@ func dirInto(resource map[string]any, dirs map[string]string, module string) err } resource["env-file"] = filled } - // **Placed, then judged** (novox/hq issue 496): the path a directory or file resolves to — the default - // layout, a place, a stated path, one beneath a ${dir:…} — is judged here, where every plan and the merge - // gate compose it, so a module whose directory lands in Docker's data is refused before it reaches a machine. - return judgeResolved(resource, module) + return nil } // unknownDirRefs is every ${dir:…} in the definition that names no directory the definition diff --git a/internal/catalogue/engine_guard_test.go b/internal/catalogue/engine_guard_test.go new file mode 100644 index 00000000..85b75970 --- /dev/null +++ b/internal/catalogue/engine_guard_test.go @@ -0,0 +1,71 @@ +package catalogue + +import ( + "go/ast" + "go/parser" + "go/token" + "path/filepath" + "slices" + "strconv" + "testing" + + "github.com/novox/mesh-controller/internal/beside" +) + +// The controller refuses a definition's resolved path by the node-engine's own rules, no more (novox/hq issue +// 496): the same lists, read here from mesh-host's internal/apply/placement_guard.go — in a merge check the clone +// beside it at the commit the mesh runs, elsewhere the copy captured in testdata/beside. When the engine's lists +// move, this fails until the controller's move with them, so the gate never refuses what the engine applies nor +// passes what it refuses by path alone. +func TestTheResolvedPathRulesAreTheNodeEnginesOwn(t *testing.T) { + file := filepath.Join(beside.Dir(t, "mesh-host"), "internal", "apply", "placement_guard.go") + parsed, err := parser.ParseFile(token.NewFileSet(), file, nil, 0) + if err != nil { + t.Fatalf("the node-engine's guard does not parse: %v", err) + } + lists := map[string][]string{} + consts := map[string]string{} + ast.Inspect(parsed, func(n ast.Node) bool { + spec, ok := n.(*ast.ValueSpec) + if !ok { + return true + } + for i, name := range spec.Names { + if i >= len(spec.Values) { + continue + } + switch v := spec.Values[i].(type) { + case *ast.CompositeLit: + for _, elt := range v.Elts { + if lit, ok := elt.(*ast.BasicLit); ok && lit.Kind == token.STRING { + s, _ := strconv.Unquote(lit.Value) + lists[name.Name] = append(lists[name.Name], s) + } + } + case *ast.BasicLit: + if v.Kind == token.STRING { + consts[name.Name], _ = strconv.Unquote(v.Value) + } + } + } + return true + }) + for name, ours := range map[string][]string{ + "protectedRoots": protectedRoots, "forbiddenBelow": forbiddenBelow, "engineTrees": engineTrees, + } { + theirs := lists[name] + if len(theirs) == 0 { + t.Errorf("the node-engine's guard names no %s any more; read it and say where its rule went", name) + continue + } + a, b := slices.Clone(ours), slices.Clone(theirs) + slices.Sort(a) + slices.Sort(b) + if !slices.Equal(a, b) { + t.Errorf("%s differs from the node-engine's:\n controller %v\n node-engine %v", name, a, b) + } + } + if consts["engineModule"] != engineModule { + t.Errorf("the node-engine's own module is %q there and %q here", consts["engineModule"], engineModule) + } +} diff --git a/internal/catalogue/manifest.go b/internal/catalogue/manifest.go index be0e5871..e344cb94 100644 --- a/internal/catalogue/manifest.go +++ b/internal/catalogue/manifest.go @@ -2138,6 +2138,7 @@ func ParseManifest(raw []byte) (Manifest, error) { problems = append(problems, m.undeclaredMounts()...) problems = append(problems, m.unknownDirRefs()...) problems = append(problems, m.unknownAccessRefs()...) + problems = append(problems, m.resolvedPathProblems()...) problems = append(problems, m.jailProblems()...) // What a module adds to the account's environment and to the login shell, and the holder's // placeholders for them (novox/hq ADR 0203, ADR 0204) — here, so the catalogue check refuses diff --git a/internal/catalogue/placement.go b/internal/catalogue/placement.go index 24d1000b..400c5b3f 100644 --- a/internal/catalogue/placement.go +++ b/internal/catalogue/placement.go @@ -92,79 +92,103 @@ func systemPath(path string) string { return "" } -// Where no directory or file the controller resolves for a module may be, from any route (novox/hq issue 496). +// Where no directory or file a module's definition resolves to may be (novox/hq issue 496). // -// mesh-catalog #205 gave docker's `state` directory `"place": "."`, which resolves to /docker: /var/lib/docker, -// every container's filesystem. systemPath judged only the `places` and `accesses` settings, so the default layout -// and a definition's own paths reached the merge gate unjudged; it composed every machine and passed, and only the -// node-engine refused the directory, at apply, failing the walk. So every path a directory or file resolves to is -// judged where the plan is composed, which is also where the gate composes. +// mesh-catalog #205 gave docker's `state` directory `"place": "."`, which resolves to /docker: with the +// default root, /var/lib/docker, every container's filesystem. systemPath judged only the `places` and `accesses` +// settings, so the default layout and a definition's own paths reached the merge gate unjudged; it composed every +// machine and passed, and only the node-engine refused the directory, at apply, failing the walk. // -// Not by systemPath's whole list: a module writes the machine's configuration in /etc and /usr by design (units, -// jails, a launcher), keeps run directories in /run, an account's keys in its .ssh, and the mesh places its own files -// for every module in /var/lib/mesh. Those are the node-engine's to judge with what only the machine knows (owners, -// links, where the runtimes really keep their data). Refused here is what no module ever writes, whoever owns it: -// the trees in neverWritten, the node-engine's own trees but by its own module, and a path that is one of the -// machine's roots or holds one. Each is refused by the node-engine too (its placement guard, issue 339), so nothing -// that applies today is refused here. +// **Judged where a definition is judged, never where a machine is composed.** resolvedPathProblems runs in +// ParseManifest, which every route a definition takes into the mesh passes: `module check`, registration of a +// build (the builder's and the registry verbs'), and the merge gate's reading of the changed repository. A +// refusal there stops one definition before it reaches any machine. Composition reads registered manifests +// without ParseManifest, and judges nothing of this: refusing there would fail the whole machine's declaration and +// freeze every module on it for one module's path, where the node-engine fails only that resource. +// +// **The node-engine's rules, no more** (mesh-host's internal/apply/placement_guard.go, with files judged as +// directories are after novox/hq issue 495, rule 6). A path is refused when it is one of protectedRoots or holds +// one, when it is at or below a tree in forbiddenBelow, or at or below one of engineTrees and its module is not +// the node-engine's. What the engine judges with what only the machine knows stays the engine's: where the +// runtimes really keep their data, links, the accounts' homes, a directory's owner below /etc. The lists are the +// engine's own words, and a test (engine_guard_test.go) holds them equal to mesh-host's beside this repository. +// systemPath stays the stricter rule for a setting: a setting is an operator's word about one machine, and the +// trees it lists (/etc, /usr, /run, the mesh's own) are where the mesh's own modules write by design. var ( - neverWritten = []string{"/boot", "/root", "/proc", "/sys", "/dev", "/bin", "/sbin", "/lib", "/lib32", "/lib64", - "/var/spool", "/var/lib/docker", "/var/lib/containers", "/var/lib/containerd", "/opt"} + protectedRoots = []string{"/", "/bin", "/boot", "/dev", "/etc", "/home", "/lib", "/lib32", "/lib64", + "/media", "/mnt", "/opt", "/proc", "/root", "/run", "/sbin", "/srv", "/sys", "/tmp", "/usr", "/usr/bin", + "/usr/lib", "/usr/lib64", "/usr/local", "/usr/local/bin", "/usr/local/lib", "/usr/local/sbin", "/usr/sbin", + "/usr/share", "/var", "/var/cache", "/var/lib", "/var/lib/mesh", "/var/log", "/var/run", "/var/tmp", + "/var/spool"} + forbiddenBelow = []string{"/proc", "/sys", "/dev", "/boot", "/root", "/var/spool", "/opt", "/var/lib/docker", + "/var/lib/containers", "/var/lib/containerd"} engineTrees = []string{"/var/lib/mesh-host", "/usr/lib/nox-mesh-host"} - // engineModule is the node-engine's own module, the one that places in engineTrees. - engineModule = "mesh-host" - // usrRoots are the roots below /usr that hold every program and library: never a module's directory itself. - usrRoots = []string{"/usr/bin", "/usr/lib", "/usr/lib64", "/usr/local", "/usr/local/bin", "/usr/local/lib", - "/usr/local/sbin", "/usr/sbin", "/usr/share"} ) -// resolvedSystemPath says why a path resolved for one of module's directories or files is the machine's own, or "". -func resolvedSystemPath(path, module string) string { - if !strings.HasPrefix(path, "/") { +// engineModule is the node-engine's own module, the one that places in engineTrees. +const engineModule = "mesh-host" + +// enginePath says why the node-engine refuses a directory or file at path for module, or "". +func enginePath(path, module string) string { + path = filepath.Clean(path) + if !filepath.IsAbs(path) { return "" } - path = filepath.Clean(path) - in := func(tree string) bool { return path == tree || strings.HasPrefix(path, tree+"/") } - for _, tree := range neverWritten { - if in(tree) { - return path + " is in " + tree + ", which no module writes in" + atOrBelow := func(tree string) bool { return path == tree || strings.HasPrefix(path, tree+"/") } + for _, root := range protectedRoots { + if path == root || path == "/" || strings.HasPrefix(root, path+"/") { + return root + " is one of the machine's own directories, and owning it is owning everything in it" + } + } + for _, tree := range forbiddenBelow { + if atOrBelow(tree) { + return "nothing is placed in " + tree } } if module != engineModule { for _, tree := range engineTrees { - if in(tree) { - return path + " is in " + tree + ", the node-engine's own, which only its module places in" + if atOrBelow(tree) { + return tree + " is the node-engine's own, placed in by its own module alone" } } } - for _, roots := range [][]string{systemRoots, systemTrees, usrRoots} { - for _, root := range roots { - if path == root { - return path + " is one of the machine's roots, the parent of everything in it" - } - } - } - for _, tree := range append(append([]string(nil), systemTrees...), neverWritten...) { - if strings.HasPrefix(tree, path+"/") { - return path + " holds " + tree + ", the machine's own or the mesh's state" - } - } return "" } -// judgeResolved refuses a directory or file whose resolved path resolvedSystemPath refuses (novox/hq issue 496). -func judgeResolved(resource map[string]any, module string) error { - kind := fmt.Sprint(resource["type"]) - if kind != "directory" && kind != "file" { - return nil +// resolvedPathProblems is every directory and file of the definition whose path, resolved as a node with the +// default root resolves it, the node-engine would refuse (novox/hq issue 496). A path still holding a placeholder +// only a machine fills (a setting, an access the definition gives no default) is the engine's to judge. +func (m Manifest) resolvedPathProblems() []string { + dirs := dirsFor(m, Rendering{}) + accesses := map[string]string{} + for _, a := range m.Accesses { + if a.ID != "" && a.Path != "" { + accesses[a.ID] = a.Path + } } - path, _ := resource["path"].(string) - if why := resolvedSystemPath(path, module); why != "" { - return fmt.Errorf("%s's %s %q resolves to %s: %s. The node-engine refuses it at apply, so it is refused "+ - "where the plan is composed, and the merge gate with it (novox/hq issue 496)", - module, kind, fmt.Sprint(resource["id"]), filepath.Clean(path), why) + var problems []string + for _, r := range m.Resources { + kind := fmt.Sprint(r["type"]) + if kind != "directory" && kind != "file" { + continue + } + id := fmt.Sprint(r["id"]) + path, _ := r["path"].(string) + if kind == "directory" && path == "" { + path = dirs[id] + } + path, _ = dirFill(path, dirs, m.Module) + path, _ = accessFill(path, accesses, m.Module) + if path == "" || strings.Contains(path, "${") { + continue + } + if why := enginePath(path, m.Module); why != "" { + problems = append(problems, fmt.Sprintf("%s's %s %q resolves to %s, which the node-engine refuses: %s. "+ + "A module's directories and files are judged when its definition is, so the merge gate refuses it "+ + "before any machine does (novox/hq issue 496)", m.Module, kind, id, filepath.Clean(path), why)) + } } - return nil + return problems } // accessRef is how a module names one of its accesses: ${access:}. @@ -359,10 +383,6 @@ func accessInto(resource map[string]any, accesses map[string]string, module stri if resource["path"], err = fill(path); err != nil { return err } - // Judged again once the access is in it: dirInto judged it while it still named one (novox/hq issue 496). - if err := judgeResolved(resource, module); err != nil { - return err - } } if content, ok := resource["content"].(string); ok { if resource["content"], err = fill(content); err != nil { diff --git a/internal/catalogue/resolved_path_test.go b/internal/catalogue/resolved_path_test.go index 4872bd45..55b5519e 100644 --- a/internal/catalogue/resolved_path_test.go +++ b/internal/catalogue/resolved_path_test.go @@ -1,39 +1,22 @@ package catalogue -// Every path the controller resolves for a module's directories and files is judged where the plan is -// composed (novox/hq issue 496). mesh-catalog #205 gave docker's `state` directory `"place": "."`, which -// resolves to /docker — /var/lib/docker, every container's filesystem. The merge gate composed every -// machine and passed it; only the node-engine refused it, at apply, and failed the walk. The gate composes -// through Declaration, so a refusal here is a refusal at the gate, for every module. +// A definition's directories and files are judged at their resolved paths when the definition is (novox/hq issue +// 496). mesh-catalog #205 gave docker's `state` directory `"place": "."`, which resolves to /docker — +// /var/lib/docker, every container's filesystem. The merge gate composed every machine and passed it; only the +// node-engine refused it, at apply, and failed the walk. ParseManifest is what `module check`, registration and the +// merge gate's reading of a changed repository all run, so a refusal here is a refusal at each of them. import ( "strings" "testing" ) -func composedOn(m Manifest, with Rendering) ([]map[string]any, error) { - return Resolution{Node: "anchor", Modules: []Manifest{m}}.Declaration(with) -} - -func pathOf(t *testing.T, resources []map[string]any, id string) string { - t.Helper() - for _, r := range resources { - if r["id"] == id { - return r["path"].(string) - } - } - t.Fatalf("no resource %s in %v", id, resources) - return "" -} - -func TestADirectoryPlacedInDockersDataIsRefusedAtComposition(t *testing.T) { +func TestADirectoryPlacedInDockersDataIsRefusedWhereTheDefinitionIsJudged(t *testing.T) { // The #205 shape, as it was merged. - m := Manifest{Module: "docker", Version: "1", Resources: []map[string]any{ - {"id": "state", "type": "directory", "place": "."}, - }} - _, err := composedOn(m, Rendering{}) + _, err := ParseManifest([]byte(`{"module": "docker", "version": "1", + "resources": [{"id": "state", "type": "directory", "place": "."}]}`)) if err == nil { - t.Fatal("a directory resolving to /var/lib/docker composed; the node-engine refuses it at apply") + t.Fatal("a directory resolving to /var/lib/docker was accepted; the node-engine refuses it at apply") } for _, said := range []string{"docker", `"state"`, "/var/lib/docker", "issue 496"} { if !strings.Contains(err.Error(), said) { @@ -42,68 +25,103 @@ func TestADirectoryPlacedInDockersDataIsRefusedAtComposition(t *testing.T) { } } -func TestTheMeshsPlaceForDockerComposes(t *testing.T) { +func TestTheMeshsPlaceForDockerIsAccepted(t *testing.T) { // The fix #205 needed: the mesh's own directory for the module, /mesh/docker. - m := Manifest{Module: "docker", Version: "1", Resources: []map[string]any{ + m, err := ParseManifest([]byte(`{"module": "docker", "version": "1", "resources": [ {"id": "state", "type": "directory", "place": "mesh"}, - {"id": "marker", "type": "file", "path": "${dir:state}/applied", "content": "x"}, - }} - resources, err := composedOn(m, Rendering{}) + {"id": "marker", "type": "file", "path": "${dir:state}/applied", "content": "x"}]}`)) if err != nil { - t.Fatalf("place %q is the mesh's tree, which the mesh writes for every module: %v", "mesh", err) + t.Fatalf("place %q is in the mesh's tree, where the mesh writes for every module: %v", "mesh", err) } - if got := pathOf(t, resources, "docker.state"); got != "/var/lib/mesh/docker" { - t.Fatalf("got %q", got) + if got := dirsFor(m, Rendering{}); got["state"] != "/var/lib/mesh/docker" { + t.Fatalf("got %v", got) } } -func TestEveryResolvedPathIsJudged(t *testing.T) { - refused := map[string]Manifest{ - "a default directory under a data root inside containerd's data": {Module: "images", Version: "1", - Resources: []map[string]any{{"id": "cache", "type": "directory"}}}, - "a stated directory in docker's data": {Module: "sidecar", Version: "1", - Resources: []map[string]any{{"id": "volumes", "type": "directory", "path": "/var/lib/docker/volumes/x"}}}, - "a file beneath a placed directory that climbs out of it": {Module: "docker", Version: "1", - Resources: []map[string]any{{"id": "state", "type": "directory", "place": "mesh"}, - {"id": "f", "type": "file", "path": "${dir:state}/../../containers/x", "content": "x"}}}, - "a file in /boot": {Module: "grub", Version: "1", - Resources: []map[string]any{{"id": "cfg", "type": "file", "path": "/boot/grub/custom.cfg", "content": "x"}}}, - "a directory that is the data root itself": {Module: "lib", Version: "1", - Resources: []map[string]any{{"id": "all", "type": "directory", "place": "."}}}, - "a directory in the node-engine's own tree, by another module": {Module: "intruder", Version: "1", - Resources: []map[string]any{{"id": "d", "type": "directory", "path": "/var/lib/mesh-host/x"}}}, +// The node-engine's rules, for directories and — as the engine judges them since novox/hq issue 495 — files. +func TestADefinitionIsRefusedWhereTheNodeEngineRefusesItsPaths(t *testing.T) { + refused := map[string]string{ + "a stated directory in docker's data": `{"module": "sidecar", "version": "1", "resources": [ + {"id": "volumes", "type": "directory", "path": "/var/lib/docker/volumes/x"}]}`, + "a file in containerd's data": `{"module": "images", "version": "1", "resources": [ + {"id": "f", "type": "file", "path": "/var/lib/containerd/x", "content": "x"}]}`, + "a file beneath a placed directory that climbs out of it": `{"module": "docker", "version": "1", "resources": [ + {"id": "state", "type": "directory", "place": "mesh"}, + {"id": "f", "type": "file", "path": "${dir:state}/../../containers/x", "content": "x"}]}`, + "a file in /boot": `{"module": "grub", "version": "1", "resources": [ + {"id": "cfg", "type": "file", "path": "/boot/grub/custom.cfg", "content": "x"}]}`, + "a directory that is the mesh's whole tree": `{"module": "mesh", "version": "1", "resources": [ + {"id": "all", "type": "directory", "place": "."}]}`, + "a directory that holds /etc": `{"module": "x", "version": "1", "resources": [ + {"id": "d", "type": "directory", "path": "/"}]}`, + "a directory in the node-engine's own tree, by another module": `{"module": "intruder", "version": "1", + "resources": [{"id": "d", "type": "directory", "path": "/var/lib/mesh-host/x"}]}`, } - roots := map[string]Rendering{ - "a default directory under a data root inside containerd's data": {DataRoot: "/var/lib/containerd"}, - "a directory that is the data root itself": {DataRoot: "/var"}, - } - for name, m := range refused { - if _, err := composedOn(m, roots[name]); err == nil || !strings.Contains(err.Error(), "issue 496") { - t.Errorf("%s: composed or refused for another reason: %v", name, err) + for name, raw := range refused { + if _, err := ParseManifest([]byte(raw)); err == nil || !strings.Contains(err.Error(), "issue 496") { + t.Errorf("%s: accepted, or refused for another reason: %v", name, err) } } - // What the mesh's own modules place today, on every machine (read from each machine's live plan): the - // machine's configuration, the mesh's tree, the node-engine's own, run directories and an account's keys. - passes := map[string]Manifest{ - "a unit in /etc": {Module: "power", Version: "1", - Resources: []map[string]any{{"id": "d", "type": "directory", "path": "/etc/systemd/system/x.service.d"}, - {"id": "u", "type": "file", "path": "/etc/systemd/system/x.service.d/a.conf", "content": "x"}}}, - "a run directory": {Module: "fail2ban", Version: "1", - Resources: []map[string]any{{"id": "run-dir", "type": "directory", "path": "/var/run/fail2ban"}}}, - "a program in /usr/local": {Module: "claude-code", Version: "1", - Resources: []map[string]any{{"id": "start", "type": "file", "path": "/usr/local/bin/claude-agent", "content": "x"}}}, - "the node-engine's launcher, by the node-engine": {Module: "mesh-host", Version: "1", - Resources: []map[string]any{{"id": "launcher", "type": "file", "path": "/usr/lib/nox-mesh-host/launch", "content": "x"}}}, - "an account's .ssh": {Module: "ssh-client", Version: "1", - Resources: []map[string]any{{"id": "ssh-dir", "type": "directory", "path": "/home/someone/.ssh"}, - {"id": "config", "type": "file", "path": "/home/someone/.ssh/config", "content": "x"}}}, - "a module's own root": {Module: "mailu", Version: "1", - Resources: []map[string]any{{"id": "state", "type": "directory", "place": "."}}}, + // What the engine applies, the mesh's own modules' paths among them (read from every machine's live plan): + // the machine's configuration, run directories, programs below /usr/local, the node-engine's own trees by its + // own module, an account's keys, a module's own root, and — not on the engine's lists, so not refused here — + // below /lib and at /storage, /data, /services and /var/lock. + accepted := map[string]string{ + "a unit in /etc": `{"module": "power", "version": "1", "resources": [ + {"id": "d", "type": "directory", "path": "/etc/systemd/system/x.service.d"}, + {"id": "u", "type": "file", "path": "/etc/systemd/system/x.service.d/a.conf", "content": "x"}]}`, + "a run directory": `{"module": "fail2ban", "version": "1", "resources": [ + {"id": "run-dir", "type": "directory", "path": "/var/run/fail2ban"}]}`, + "a program in /usr/local/bin": `{"module": "claude-code", "version": "1", "resources": [ + {"id": "start", "type": "file", "path": "/usr/local/bin/claude-agent", "content": "x"}]}`, + "the node-engine's launcher and state, by the node-engine": `{"module": "mesh-host", "version": "1", "resources": [ + {"id": "launcher", "type": "file", "path": "/usr/lib/nox-mesh-host/launch", "content": "x"}, + {"id": "state", "type": "directory", "path": "/var/lib/mesh-host"}]}`, + "an account's .ssh": `{"module": "ssh-client", "version": "1", "resources": [ + {"id": "ssh-dir", "type": "directory", "path": "/home/someone/.ssh"}, + {"id": "config", "type": "file", "path": "/home/someone/.ssh/config", "content": "x"}]}`, + "a module's own root": `{"module": "mailu", "version": "1", "resources": [ + {"id": "state", "type": "directory", "place": "."}]}`, + "a file below /lib": `{"module": "udev", "version": "1", "resources": [ + {"id": "rule", "type": "file", "path": "/lib/udev/rules.d/99-x.rules", "content": "x"}]}`, + "directories at /storage, /data, /services and /var/lock": `{"module": "roots", "version": "1", "resources": [ + {"id": "a", "type": "directory", "path": "/storage"}, {"id": "b", "type": "directory", "path": "/data"}, + {"id": "c", "type": "directory", "path": "/services"}, {"id": "d", "type": "directory", "path": "/var/lock"}]}`, } - for name, m := range passes { - if _, err := composedOn(m, Rendering{}); err != nil { + for name, raw := range accepted { + if _, err := ParseManifest([]byte(raw)); err != nil { t.Errorf("%s: refused: %v", name, err) } } } + +func TestAPathThroughAnAccessIsJudgedWithTheAccessFilledIn(t *testing.T) { + // A file's path may name an access; the access's default path is the definition's, so it is judged with it. + _, err := ParseManifest([]byte(`{"module": "backup", "version": "1", + "accesses": [{"id": "images", "path": "/var/lib/docker/volumes"}], + "resources": [{"id": "marker", "type": "file", "path": "${access:images}/marker", "content": "x"}]}`)) + if err == nil || !strings.Contains(err.Error(), "/var/lib/docker/volumes/marker") || + !strings.Contains(err.Error(), "issue 496") { + t.Fatalf("a file reaching Docker's data through an access's default path was accepted: %v", err) + } + // An access the definition gives no path is placed by a setting, which Places and AccessPlaces judge, and on + // the machine by the node-engine: nothing here to resolve it against, so nothing is refused for it. + if _, err := ParseManifest([]byte(`{"module": "backup", "version": "1", + "accesses": [{"id": "images"}], + "resources": [{"id": "marker", "type": "file", "path": "${access:images}/marker", "content": "x"}]}`)); err != nil { + t.Fatalf("an access placed only by a setting cannot be judged at the definition: %v", err) + } +} + +func TestComposingAMachineIsNeverStoppedByOneModulesPath(t *testing.T) { + // A module registered from outside the catalogue never passes the gate. Refusing its path while a machine's + // declaration is composed would fail the whole declaration and freeze every module on that machine; the + // node-engine fails only the one resource. So composition leaves it to the engine. + m := Manifest{Module: "docker", Version: "1", Resources: []map[string]any{ + {"id": "state", "type": "directory", "place": "."}, + }} + if _, err := (Resolution{Node: "anchor", Modules: []Manifest{m}}).Declaration(Rendering{}); err != nil { + t.Fatalf("the machine's declaration failed for one module's path: %v", err) + } +} diff --git a/testdata/beside/CAPTURED b/testdata/beside/CAPTURED index 17d63986..78e7f73d 100644 --- a/testdata/beside/CAPTURED +++ b/testdata/beside/CAPTURED @@ -25,3 +25,11 @@ and write the commits here. The SDK is captured at the commit go.mod pins for gi rm -rf testdata/beside/mesh-sdk && mkdir -p testdata/beside/mesh-sdk git -C ../mesh-sdk archive conformance/events | tar -x -C testdata/beside/mesh-sdk + +And from mesh-host at the same commit as its line above, the node-engine's placement guard, which +internal/catalogue's engine_guard_test.go holds the controller's resolved-path rules to (novox/hq issue 496), +kept as .captured so no Go tool reads it as this repository's code: + + mkdir -p testdata/beside/mesh-host/internal/apply + git -C ../mesh-host show :internal/apply/placement_guard.go \ + > testdata/beside/mesh-host/internal/apply/placement_guard.go.captured diff --git a/testdata/beside/mesh-host/internal/apply/placement_guard.go.captured b/testdata/beside/mesh-host/internal/apply/placement_guard.go.captured new file mode 100644 index 00000000..49c1d83e --- /dev/null +++ b/testdata/beside/mesh-host/internal/apply/placement_guard.go.captured @@ -0,0 +1,549 @@ +package apply + +// Where the node-engine places nothing and mounts nothing, whoever asks (novox/hq issue 339). +// +// A directory names its path, and the controller resolves part of that path from what was set for the +// module: its `places` setting moves a directory anywhere, with an owner it names, and its `accesses` setting +// says which of the machine's paths are mounted into its container. The engine runs as root, so a path it +// accepts blindly is a path anyone who could change those settings hands to any account: a directory at /etc +// owned by a caller's account gives it /etc, and an access at / mounts the machine's root into a container. +// The controller refuses both where a setting is made; the engine refuses them again where it applies, +// because a guard in one place is a guard one change away from gone. Whatever the declaration says: +// +// 1. **No directory, access or mount source is one of the machine's own roots, or holds one**: /, /etc, +// /usr, /var, /var/lib, /home, /run and the rest of protectedRoots. Modules place directories BELOW /etc +// or /var/lib, never the root itself; owning one is owning everything in it. +// 2. **Nothing is placed in /proc, /sys, /dev, /boot, /root, /var/spool, /opt or the container runtimes' data +// (/var/lib/docker, /var/lib/containers, /var/lib/containerd, and where the runtimes' configuration moves +// them: runtimeDataRoots), nor in the node-engine's +// own trees** (its state, its identity, its installed builds) but by its own module; nothing is mounted from +// those but /proc, /sys and /dev. A mount of a kernel file, a device or the clock is the plumbing +// systemPath names. +// 2a. **An account's .ssh is never an access or a mount**, and is a directory only below its account's home, +// as that account's (rule 4). +// 3. **A directory below /etc, /usr or /run is root's.** The machine's configuration and programs are read +// as root's word; a directory there owned by another account is that account writing root's word. An +// access is never there at all: the operator's data is not the machine's configuration. +// 4. **Below a person's or an agent's home, a directory is that account's.** Root's or another account's +// directory there is one the account does not control in a tree whose every parent it does. A home +// itself may be a module's directory (a backup repository kept as an account's home is one), and as +// every directory the mesh did not make, it is used as found: never chowned or chmodded (applyDirectory). +// 5. **A mount source that is a refused directory or a refused access is refused with it**: the container +// would otherwise bind the very path the engine would not place, and the runtime creates a missing one +// as root. +// +// Each is a failed resource with its reason in the node's report; nothing is touched. Paths are judged as +// declared and again with every link in them resolved, so a link at /srv/x pointing at /etc places nothing. + +import ( + "bufio" + "context" + "encoding/json" + "errors" + "fmt" + "os" + "path/filepath" + "regexp" + "strconv" + "strings" + "sync" + "time" + + "github.com/novox/mesh-host/internal/declaration" +) + +// protectedRoots are paths no directory, access or mount source may be, nor hold. +var protectedRoots = []string{"/", "/bin", "/boot", "/dev", "/etc", "/home", "/lib", "/lib32", "/lib64", + "/media", "/mnt", "/opt", "/proc", "/root", "/run", "/sbin", "/srv", "/sys", "/tmp", "/usr", "/usr/bin", + "/usr/lib", "/usr/lib64", "/usr/local", "/usr/local/bin", "/usr/local/lib", "/usr/local/sbin", "/usr/sbin", + "/usr/share", "/var", "/var/cache", "/var/lib", "/var/lib/mesh", "/var/log", "/var/run", "/var/tmp", + "/var/spool"} + +// forbiddenBelow are trees nothing is placed in or mounted from: the kernel's, the boot loader's and root's +// home. engineTrees are the node-engine's own, which only its own module places in. +var ( + forbiddenBelow = []string{"/proc", "/sys", "/dev", "/boot", "/root", "/var/spool", "/opt", "/var/lib/docker", + "/var/lib/containers", "/var/lib/containerd"} + // forbiddenBelowMount are the trees no container mounts from: a mount of the kernel's files and devices is + // the plumbing a container may need (systemPath); root's home and the boot loader's are no plumbing. + forbiddenBelowMount = []string{"/boot", "/root", "/var/spool", "/opt", "/var/lib/docker", "/var/lib/containers", + "/var/lib/containerd"} + engineTrees = []string{"/var/lib/mesh-host", "/usr/lib/nox-mesh-host"} + // rootsOnly are trees a directory below is root's, and an access is never in. + rootsOnly = []string{"/etc", "/usr", "/run", "/var/run"} +) + +// runtimeFiles are where the container runtimes say where they keep their data: dockerd's daemon.json, its +// unit and the unit's drop-ins (an ExecStart with --data-root, or the older -g/--graph, continued over lines, +// quoted, through Environment= or EnvironmentFile=, or a --config-file naming another daemon.json), and podman's +// storage.conf (graphroot, a basic or a literal string). The running runtimes are asked first. containerd keeps its own under /var/lib/containerd, which forbiddenBelow names; a +// containerd configured elsewhere, and podman's rootless stores under each account's home, are not read: the +// first is no runtime this mesh runs, and the second is below a home, which rule 4 already keeps for its +// account. A variable so a test names its own. +type runtimeFiles struct { + daemonJSON string + units []string + dropInDirs []string + storageConf string +} + +var runtimeConfigs = runtimeFiles{ + daemonJSON: "/etc/docker/daemon.json", + units: []string{"/etc/systemd/system/docker.service", "/usr/lib/systemd/system/docker.service", "/lib/systemd/system/docker.service"}, + dropInDirs: []string{"/etc/systemd/system/docker.service.d", "/run/systemd/system/docker.service.d", "/usr/lib/systemd/system/docker.service.d"}, + storageConf: "/etc/containers/storage.conf", +} + +var ( + dataRootFlag = regexp.MustCompile(`(?:--data-root|--graph|-g)(?:=|\s+)(\S+)`) + configFlag = regexp.MustCompile(`--config-file(?:=|\s+)(\S+)`) + graphRoot = regexp.MustCompile(`(?m)^\s*graphroot\s*=\s*(?:"([^"]+)"|'([^']+)')`) + envVar = regexp.MustCompile(`\$\{([A-Za-z_][A-Za-z0-9_]*)\}|\$([A-Za-z_][A-Za-z0-9_]*)`) +) + +// runtimeRoots is where the container runtimes keep their data, as the last apply read it (readRuntimeRoots); +// nil until an apply has read it, when the guard reads the files itself. +var ( + runtimeRootsMu sync.Mutex + runtimeRoots []string +) + +// AskRuntimes is how the engine asks the running container runtimes where they keep their data: set by the engine +// to run the commands, nil in a test, which then reads the files alone. Its own, never the apply's runner, whose +// commands a test reads back as what the apply did. +var AskRuntimes Runner + +// refreshRuntimeRoots reads where the runtimes keep their data once, at the start of an apply. +func refreshRuntimeRoots(ctx context.Context) { + roots := readRuntimeRoots(ctx, AskRuntimes) + runtimeRootsMu.Lock() + runtimeRoots = roots + runtimeRootsMu.Unlock() +} + +// runtimeDataRoots is every place the container runtimes keep their data, beyond the default trees forbiddenBelow +// names: every container's filesystem is there. +func runtimeDataRoots() []string { + runtimeRootsMu.Lock() + roots := runtimeRoots + runtimeRootsMu.Unlock() + if roots != nil { + return roots + } + return readRuntimeRoots(context.Background(), nil) +} + +// readRuntimeRoots asks the running runtimes where they keep their data, when run is given (`docker info`, +// `podman info`), and reads their configuration besides: an answer from a runtime that is running is what it +// really does, and the files say what it will do when it starts again. Both count. A runtime that is not running +// or not installed answers nothing, which is no error. +func readRuntimeRoots(ctx context.Context, run Runner) []string { + seen := map[string]bool{} + var out []string + add := func(p string) { + p = strings.Trim(strings.TrimSpace(p), `"'`) + if !filepath.IsAbs(p) { + return + } + p = filepath.Clean(p) + if !seen[p] { + seen[p] = true + out = append(out, p) + } + } + if run != nil { + // Each runtime has its own ten seconds: one that hangs costs the other nothing. + for _, q := range [][]string{{"docker", "info", "--format", "{{.DockerRootDir}}"}, + {"podman", "info", "--format", "{{.Store.GraphRoot}}"}} { + ask, cancel := context.WithTimeout(ctx, 10*time.Second) + if root, err := run(ask, q[0], q[1:]...); err == nil { + add(root) + } + cancel() + } + } + daemonJSON := func(path string) { + raw, err := os.ReadFile(path) + if err != nil { + return + } + var c struct { + DataRoot string `json:"data-root"` + Graph string `json:"graph"` + } + if json.Unmarshal(raw, &c) == nil { + add(c.DataRoot) + add(c.Graph) + } + } + daemonJSON(runtimeConfigs.daemonJSON) + units := append([]string(nil), runtimeConfigs.units...) + for _, dir := range runtimeConfigs.dropInDirs { + matches, _ := filepath.Glob(filepath.Join(dir, "*.conf")) + units = append(units, matches...) + } + // The unit and its drop-ins are one unit to systemd: a variable set in one is seen by an ExecStart in another. + env := map[string]string{} + var execs []string + for _, u := range units { + raw, err := os.ReadFile(u) + if err != nil { + continue + } + e, x := unitLines(string(raw)) + for k, v := range e { + env[k] = v + } + execs = append(execs, x...) + } + for _, line := range execs { + line = envVar.ReplaceAllStringFunc(line, func(ref string) string { + m := envVar.FindStringSubmatch(ref) + if v, ok := env[m[1]+m[2]]; ok { + return v + } + return ref + }) + for _, m := range dataRootFlag.FindAllStringSubmatch(line, -1) { + add(m[1]) + } + for _, m := range configFlag.FindAllStringSubmatch(line, -1) { + daemonJSON(strings.Trim(m[1], `"'`)) + } + } + if raw, err := os.ReadFile(runtimeConfigs.storageConf); err == nil { + for _, m := range graphRoot.FindAllStringSubmatch(string(raw), -1) { + add(m[1] + m[2]) + } + } + return out +} + +// unitLines reads a unit file as systemd does for what matters here: a line ending in a backslash continues on the +// next, Environment= sets variables (quoted or not, several to a line), EnvironmentFile= (a leading - says it may +// be missing) reads KEY=value lines, and every ExecStart line is returned whole. +func unitLines(text string) (map[string]string, []string) { + var joined []string + var cur strings.Builder + for _, line := range strings.Split(text, "\n") { + trimmed := strings.TrimRight(line, " \t") + if strings.HasSuffix(trimmed, "\\") { + cur.WriteString(strings.TrimSuffix(trimmed, "\\") + " ") + continue + } + cur.WriteString(line) + joined = append(joined, cur.String()) + cur.Reset() + } + if cur.Len() > 0 { + joined = append(joined, cur.String()) + } + env := map[string]string{} + setPairs := func(s string) { + for _, f := range splitQuoted(s) { + if k, v, ok := strings.Cut(f, "="); ok { + env[strings.TrimSpace(k)] = strings.Trim(strings.TrimSpace(v), `"'`) + } + } + } + var execs []string + for _, line := range joined { + l := strings.TrimSpace(line) + switch { + case strings.HasPrefix(l, "Environment="): + setPairs(strings.TrimPrefix(l, "Environment=")) + case strings.HasPrefix(l, "EnvironmentFile="): + path := strings.TrimPrefix(strings.TrimSpace(strings.TrimPrefix(l, "EnvironmentFile=")), "-") + if raw, err := os.ReadFile(path); err == nil { + for _, kv := range strings.Split(string(raw), "\n") { + kv = strings.TrimSpace(kv) + if kv == "" || strings.HasPrefix(kv, "#") { + continue + } + setPairs(kv) + } + } + case strings.HasPrefix(l, "ExecStart"): + execs = append(execs, l) + } + } + return env, execs +} + +// splitQuoted splits on blanks outside double or single quotes, keeping the quotes' contents whole. +func splitQuoted(s string) []string { + var out []string + var cur strings.Builder + var quote rune + for _, r := range s { + switch { + case quote != 0 && r == quote: + quote = 0 + case quote == 0 && (r == '"' || r == '\''): + quote = r + case quote == 0 && (r == ' ' || r == '\t'): + if cur.Len() > 0 { + out = append(out, cur.String()) + cur.Reset() + } + default: + cur.WriteRune(r) + } + } + if cur.Len() > 0 { + out = append(out, cur.String()) + } + return out +} + +// engineModule is the module whose resources may place in the engine's own trees. +const engineModule = "mesh-host" + +// passwdFile is the user database homes are read from. A variable so a test names its own. +var passwdFile = "/etc/passwd" + +// PlacementRefusedError is a resource the engine will not place, or mount, where it says. +type PlacementRefusedError struct { + Path, Why string +} + +func (e *PlacementRefusedError) Error() string { + return fmt.Sprintf("%s is not placed: %s (novox/hq issue 339); nothing was touched", e.Path, e.Why) +} + +// homeAccount is a person's or an agent's account and its home. +type homeAccount struct { + Name string + UID int +} + +// accountsOfHomes is each person's or agent's home and the account it belongs to, from the user database: an +// account with a uid of 1000 or more, or a home under /home. A variable so a test names its own. +var accountsOfHomes = func() map[string]homeAccount { + f, err := os.Open(passwdFile) + if err != nil { + return nil + } + defer f.Close() + out := map[string]homeAccount{} + sc := bufio.NewScanner(f) + for sc.Scan() { + fields := strings.Split(sc.Text(), ":") + if len(fields) < 6 { + continue + } + uid, err := strconv.Atoi(fields[2]) + if err != nil { + continue + } + home := filepath.Clean(fields[5]) + if home == "/" || home == "." || home == "" || home == "/nonexistent" { + continue + } + if (uid >= 1000 && uid != 65534) || strings.HasPrefix(home, "/home/") { + out[home] = homeAccount{Name: fields[0], UID: uid} + } + } + return out +} + +// below says whether path is strictly below dir. +func below(path, dir string) bool { + if dir == "/" { + return path != "/" + } + return strings.HasPrefix(path, dir+"/") +} + +// atOrBelow says whether path is dir or below it. +func atOrBelow(path, dir string) bool { return path == dir || below(path, dir) } + +// resolved is a path with every link in it followed, as far as the path exists, and the rest as declared. +func resolved(path string) string { + rest := "" + for p := path; ; p = filepath.Dir(p) { + if real, err := filepath.EvalSymlinks(p); err == nil { + return filepath.Clean(filepath.Join(real, rest)) + } + if filepath.Dir(p) == p { + return path + } + rest = filepath.Join(filepath.Base(p), rest) + } +} + +// ownedByAccount says whether a declared owner is that account: by name, or by its uid ("1001", "1001:1001"). +func ownedByAccount(owner string, a homeAccount) bool { + if owner == a.Name { + return true + } + user, _, _ := strings.Cut(owner, ":") + if uid, err := strconv.Atoi(user); err == nil { + return uid == a.UID + } + return false +} + +// rootOwner says whether a declared owner is root: none, "root", or uid 0. +func rootOwner(owner string) bool { + if owner == "" || owner == "root" { + return true + } + user, _, _ := strings.Cut(owner, ":") + return user == "0" +} + +// what a guarded path is, for the words of a refusal. +type placing int + +const ( + placingDirectory placing = iota + placingAccess + placingMount +) + +// refusePath says why a path is not placed or mounted; nil when it may be. module is the resource's module, +// owner a directory's declared owner. +func refusePath(path string, kind placing, module, owner string) error { + clean := filepath.Clean(path) + if !filepath.IsAbs(clean) { + return nil // the declaration refuses a relative path already; a named volume is not a path + } + for _, p := range []string{clean, resolved(clean)} { + if err := refuseOne(p, kind, module, owner); err != nil { + if p != clean { + err.Why = fmt.Sprintf("through a link, it is %s, and %s", p, err.Why) + err.Path = clean + } + return err + } + } + return nil +} + +func refuseOne(path string, kind placing, module, owner string) *PlacementRefusedError { + for _, root := range protectedRoots { + if path == root || below(root, path) { + return &PlacementRefusedError{Path: path, Why: root + " is one of the machine's own directories, " + + "and owning or mounting it would be owning or mounting everything in it"} + } + } + trees := append([]string(nil), forbiddenBelow...) + if kind == placingMount { + // A mount is the machine's plumbing as often as a module's data — the clock, a kernel file, /dev/null + // (systemPath) — and what a setting can mount at all is a directory or an access, refused above it. + trees = append([]string(nil), forbiddenBelowMount...) + } + // The container runtimes' data, wherever the machine keeps it: every container's filesystem is there. + trees = append(trees, runtimeDataRoots()...) + for _, tree := range trees { + if atOrBelow(path, tree) { + return &PlacementRefusedError{Path: path, Why: "nothing is placed in or mounted from " + tree} + } + } + if module != engineModule { + for _, tree := range engineTrees { + if atOrBelow(path, tree) { + return &PlacementRefusedError{Path: path, Why: tree + " is the node-engine's own, placed in by " + + "its own module alone"} + } + } + } + for _, tree := range rootsOnly { + if !below(path, tree) { + continue + } + switch { + case kind == placingAccess: + return &PlacementRefusedError{Path: path, Why: "an access is the operator's data, and " + tree + + " is the machine's own"} + case kind == placingDirectory && !rootOwner(owner): + return &PlacementRefusedError{Path: path, Why: fmt.Sprintf("a directory below %s is root's, and this "+ + "one is declared %s's", tree, owner)} + } + } + ssh := false + for _, part := range strings.Split(path, "/") { + ssh = ssh || part == ".ssh" + } + if ssh && kind != placingDirectory { + return &PlacementRefusedError{Path: path, Why: "it is an account's .ssh, which holds its keys and who may " + + "log in as it, and is never an access or a mount"} + } + if kind != placingDirectory { + return nil + } + homes := accountsOfHomes() + var deepest string + for home := range homes { + if below(path, home) && len(home) > len(deepest) { + deepest = home + } + } + if ssh && deepest == "" { + return &PlacementRefusedError{Path: path, Why: "a .ssh directory is placed only below its account's home, " + + "as that account's"} + } + if deepest != "" { + if a := homes[deepest]; !ownedByAccount(owner, a) { + if owner == "" { + owner = "root" + } + return &PlacementRefusedError{Path: path, Why: fmt.Sprintf("it is below %s's home and declared %s's; "+ + "below a home only that account's directories are placed", a.Name, owner)} + } + } + return nil +} + +// moduleOfID is the module a resource id names, or "". +func moduleOfID(id string) string { + module, _ := moduleOf(id) + return module +} + +// refusedPlaces judges every directory and access of a declaration before anything is applied, and answers +// each refusal by path: what is refused is refused again as a container's mount source. +func refusedPlaces(resources []declaration.Resource) map[string]error { + out := map[string]error{} + for _, r := range resources { + var err error + switch res := r.(type) { + case *declaration.Directory: + err = refusePath(res.Path, placingDirectory, moduleOfID(res.ID), res.Owner) + case *declaration.Access: + err = refusePath(res.Path, placingAccess, moduleOfID(res.ID), "") + default: + continue + } + if err != nil { + out[filepath.Clean(r.Target())] = err + } + } + return out +} + +// refuseMounts says why a container's mounts are not made; nil when they may be. +func refuseMounts(c *declaration.Container, refused map[string]error) error { + for _, v := range c.Volumes { + src := mountSource(v) + if !strings.HasPrefix(src, "/") { + continue // a named volume, which the runtime keeps in its own tree + } + src = filepath.Clean(src) + for path, why := range refused { + if atOrBelow(src, path) { + var refusal *PlacementRefusedError + if errors.As(why, &refusal) { + return &PlacementRefusedError{Path: src, Why: "it is mounted from " + path + + ", which is refused: " + refusal.Why} + } + return why + } + } + if err := refusePath(src, placingMount, moduleOfID(c.ID), ""); err != nil { + return err + } + } + return nil +} From e1367d185ad1b606b2a7f0dc1eb76a83bd84c58f Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 19:01:41 +0200 Subject: [PATCH 3/8] Send the bus's user list alone before a walk's gate, so a new grant is on the bus when the first machine is judged (issue 490) A walk left the machine holding the bus out of a gated send whenever a build waited there for that same gate, so a module's new health tool was refused by the bus on its first machine and the gate could not pass. The grants step sends that machine its declaration with every build kept, before the first machine's send; plans and the change plan say it. --- cmd/mesh-controller/changeplan.go | 24 ++++ cmd/mesh-controller/gate.go | 4 + cmd/mesh-controller/grants_step_test.go | 163 ++++++++++++++++++++++++ cmd/mesh-controller/issue352_test.go | 2 +- cmd/mesh-controller/recorded_kept.go | 42 +++++- cmd/mesh-controller/release.go | 98 ++++++++++++-- cmd/mesh-controller/release_plan.go | 7 +- internal/inventory/plans.go | 12 ++ 8 files changed, 335 insertions(+), 17 deletions(-) create mode 100644 cmd/mesh-controller/grants_step_test.go diff --git a/cmd/mesh-controller/changeplan.go b/cmd/mesh-controller/changeplan.go index 7a983c97..9c84cd19 100644 --- a/cmd/mesh-controller/changeplan.go +++ b/cmd/mesh-controller/changeplan.go @@ -2,6 +2,7 @@ package main import ( "fmt" + "slices" "sort" "strings" @@ -28,6 +29,14 @@ func changePlanOf(repository, base, head string, r mergeReach, entries []invento for _, e := range entries { byName[e.Manifest.Module] = e } + // The machine holding the bus, whose declaration carries every module's grants (novox/hq issue 490). + holder := "" + for _, e := range entries { + if e.Manifest.BusUsers != "" && e.Manifest.ClaimsSeat(catalogue.BrokerSeat) && len(e.On) > 0 { + holder = e.On[0] + } + } + var granting []string machines := map[string]*link.MachinePlan{} machine := func(name string) *link.MachinePlan { if machines[name] == nil { @@ -50,6 +59,10 @@ func changePlanOf(repository, base, head string, r mergeReach, entries []invento machine(on).Receives = append(machine(on).Receives, name) } } + if !waits && holder != "" && !(len(e.On) == 1 && e.On[0] == holder) && len(e.On) > 0 && + !slices.Contains(granting, name) { + granting = append(granting, name) + } switch { case (name == "nats" || catalogue.ProvidesBus(e.Manifest)) && len(e.On) > 0: // Said before the merge (novox/hq issue 336): a new bus build holds every send to the bus's machine @@ -76,6 +89,13 @@ func changePlanOf(repository, base, head string, r mergeReach, entries []invento } } } + if len(granting) > 0 { + // Said before the merge (novox/hq issue 490): what the walk does when the change alters the bus's + // user list. Whether it does is known only once the builds are registered, so it is said as a rule. + p.Steps = append(p.Steps, fmt.Sprintf("%s: if this changes what the bus's user list says (a new tool, "+ + "subject or user), %s is sent that list alone, every build there kept, before %s is judged on its first "+ + "machine", grantsStepWord, holder, strings.Join(granting, ", "))) + } var names []string for name := range machines { names = append(names, name) @@ -88,6 +108,10 @@ func changePlanOf(repository, base, head string, r mergeReach, entries []invento return p } +// grantsStepWord names the grants step (novox/hq issue 490): the bus's user list sent alone to the machine +// holding the bus ahead of a walk's gate. Not the bus step, which replaces the bus itself. +const grantsStepWord = "grants step" + // busUpgradeNeeded is how a change plan says that merging it holds the bus's machine for a person's step. const busUpgradeNeeded = "merging this needs a person's bus upgrade" diff --git a/cmd/mesh-controller/gate.go b/cmd/mesh-controller/gate.go index 36eefc4d..8d3bab69 100644 --- a/cmd/mesh-controller/gate.go +++ b/cmd/mesh-controller/gate.go @@ -1353,6 +1353,10 @@ func gateLine(g *inventory.PlanGate) string { if g.Rollback != "" { line += "; " + g.Rollback } + // Before the send it judges (novox/hq issue 490). + if said := grantsSaid(g.Grants); said != "" { + line += "; " + said + } return line } diff --git a/cmd/mesh-controller/grants_step_test.go b/cmd/mesh-controller/grants_step_test.go new file mode 100644 index 00000000..59458c6d --- /dev/null +++ b/cmd/mesh-controller/grants_step_test.go @@ -0,0 +1,163 @@ +package main + +import ( + "context" + "errors" + "slices" + "strings" + "testing" + + "github.com/novox/mesh-controller/internal/catalogue" + "github.com/novox/mesh-controller/internal/inventory" + "github.com/novox/mesh-controller/internal/link" +) + +// The grants reach the bus before the gate (novox/hq issue 490). +// +// On 2026-10-11 a merge gave restic, assigned to every machine, a new health tool. Its walk sent restic to +// its first machine and judged it there; that machine's node-engine asked the new tool and the bus refused +// it, because the grant was in the bus's user list, which only the machine holding the bus carries — and +// that machine runs restic too, so it was left out of the send while restic's new build waited there for +// the very gate that could not pass. A person pushed it by hand, carrying restic's new build there unjudged. + +// aSend is one send the walk made: to which machines, and whether every build there was kept. +type aSend struct { + names []string + keepsAll bool + judged []string +} + +// sendsRecorded replaces the walk's send and the reading of the bus's user list for one test: the machine +// holding the bus is novox, and its list is behind as behind says. +func sendsRecorded(t *testing.T, holder string, behind bool, refuse error) *[]aSend { + t.Helper() + var sends []aSend + wasSend, wasBehind := sendRollout, brokerBehindOf + t.Cleanup(func() { sendRollout, brokerBehindOf = wasSend, wasBehind }) + brokerBehindOf = func(_ context.Context, _ *stores, names []string) (string, bool, error) { + if slices.Contains(names, holder) { + return holder, false, nil + } + return holder, behind, nil + } + sendRollout = func(ctx context.Context, _ *stores, names []string) ([]string, error) { + s := aSend{names: append([]string(nil), names...), keepsAll: everyBuildKept(ctx)} + for n := range scopeOf(ctx).judged { + s.judged = append(s.judged, n) + } + sends = append(sends, s) + if refuse != nil && s.keepsAll { + return nil, refuse + } + return names, nil + } + return &sends +} + +// The restic shape: a new tool on a module on every machine, its first machine ace, the bus on novox. +func TestAWalkSendsTheGrantsToTheBusBeforeTheFirstMachineIsJudged(t *testing.T) { + sends := sendsRecorded(t, "novox", true, nil) + sent, grants, err := sendJudged(t.Context(), nil, "ace") + if err != nil { + t.Fatal(err) + } + if len(*sends) != 2 { + t.Fatalf("the walk made %d send(s), want the grants step and then the first machine: %+v", len(*sends), *sends) + } + first, then := (*sends)[0], (*sends)[1] + if strings.Join(first.names, ",") != "novox" || !first.keepsAll || len(first.judged) != 0 { + t.Errorf("the first send is %+v, want novox alone, every build there kept, judging nothing", first) + } + if strings.Join(then.names, ",") != "ace" || then.keepsAll || strings.Join(then.judged, ",") != "ace" { + t.Errorf("the second send is %+v, want ace, judged", then) + } + if strings.Join(sent, ",") != "ace" { + t.Errorf("the gate's machines are %v, want ace alone: the bus's machine is not judged", sent) + } + if grants == nil || grants.Node != "novox" || grants.At == nil || grants.Failed != "" { + t.Fatalf("the gate keeps %+v as its grants step", grants) + } + line := gateLine(&inventory.PlanGate{Machines: sent, Grants: grants}) + if !strings.Contains(line, "grants step: novox sent the bus's user list first, its builds kept") { + t.Errorf("plans says the gate as %q", line) + } +} + +// Nothing more when there is nothing to carry: the list unchanged, or the first machine holds the bus. +func TestAWalkTakesNoGrantsStepWhenTheListIsAlreadyThere(t *testing.T) { + for _, c := range []struct { + name string + node string + behind bool + }{{"the list unchanged", "ace", false}, {"the first machine holds the bus", "novox", true}} { + t.Run(c.name, func(t *testing.T) { + sends := sendsRecorded(t, "novox", c.behind, nil) + _, grants, err := sendJudged(t.Context(), nil, c.node) + if err != nil { + t.Fatal(err) + } + if len(*sends) != 1 || (*sends)[0].keepsAll || grants != nil { + t.Errorf("the walk made %+v with grants %+v, want the judged send alone", *sends, grants) + } + }) + } +} + +// A grants step that cannot be sent is said on the gate, and the walk goes on as it did before. +func TestAGrantsStepThatFailsIsSaidAndTheSendGoesOn(t *testing.T) { + sends := sendsRecorded(t, "novox", true, errors.New("a bus upgrade waits for a person")) + sent, grants, err := sendJudged(t.Context(), nil, "ace") + if err != nil { + t.Fatal(err) + } + if strings.Join(sent, ",") != "ace" || len(*sends) != 2 { + t.Errorf("the walk made %+v", *sends) + } + if grants == nil || grants.Failed == "" || grants.At != nil { + t.Fatalf("the gate keeps %+v", grants) + } + if line := gateLine(&inventory.PlanGate{Machines: sent, Grants: grants}); !strings.Contains(line, + "grants step to novox FAILED, sent without it: a bus upgrade waits for a person") { + t.Errorf("plans says the gate as %q", line) + } +} + +// The grants step moves no build, so no gate's refusal holds it back: it is never read for moves. +func TestTheGrantsStepIsNotHeldForTheBuildsItKeeps(t *testing.T) { + names, err := ungatedIn(keepingEveryBuild(t.Context()), nil, []string{"novox"}, "") + if err != nil || strings.Join(names, ",") != "novox" { + t.Errorf("the grants step's send reads %v, %v", names, err) + } +} + +// The delivery plan says the step before the merge. +func TestAChangePlanSaysTheGrantsStep(t *testing.T) { + const catalogue_ = "http://forge.internal:20000/novox/mesh-catalog.git" + entry := func(name string, on ...string) inventory.Entry { + e := fromRepo(name, catalogue_, "modules/"+name) + e.Source.BuiltFrom = "old" + e.On = on + return e + } + nats := entry("nats", "novox") + nats.Manifest.BusUsers = "/etc/nats/users.conf" + nats.Manifest.Claims = []catalogue.Claim{{Name: catalogue.BrokerSeat, Scope: catalogue.ScopeMesh}} + restic := entry("restic", "ace", "novox", "shanks") + only := entry("bus-tools", "novox") + entries := []inventory.Entry{nats, restic, only} + rollsOut := func(string) (inventory.Upgrade, bool) { return inventory.Upgrade{RollOut: true}, true } + plan := func(paths ...string) link.ChangePlan { + m := link.SourceMoved{Owner: "novox", Repo: "mesh-catalog", Base: "main", Commit: "head", Paths: paths} + return changePlanOf("novox/mesh-catalog", "main", "head", reachOfMerge(m, entries, nil, nil), entries, rollsOut) + } + + text := planText(plan("modules/restic/module.json")) + if !strings.Contains(text, "grants step: if this changes what the bus's user list says (a new tool, subject or "+ + "user), novox is sent that list alone, every build there kept, before restic is judged on its first machine") { + t.Errorf("the change plan does not say the grants step:\n%s", text) + } + // A module only on the bus's machine goes there with the list in its own send: no step of its own. + if text := planText(plan("modules/bus-tools/module.json")); strings.Contains(text, "grants step") { + t.Errorf("a module on the bus's machine alone reads:\n%s", text) + } +} diff --git a/cmd/mesh-controller/issue352_test.go b/cmd/mesh-controller/issue352_test.go index 951f8514..10afa237 100644 --- a/cmd/mesh-controller/issue352_test.go +++ b/cmd/mesh-controller/issue352_test.go @@ -69,7 +69,7 @@ func TestAPlansFirstSendWaitsForAReleaseJudgingTheModuleThere(t *testing.T) { b := aBacklog(t) ctx := t.Context() advancePlans(ctx, b.open) // the release judges app c2 on anchor - _, _, err := gatedSend(ctx, b.open, "anchor", []inventory.CarriedMove{{Module: "app", Node: "anchor", From: "c2", To: "c3", Build: "build-app-c3"}}) + _, _, _, err := gatedSend(ctx, b.open, "anchor", []inventory.CarriedMove{{Module: "app", Node: "anchor", From: "c2", To: "c3", Build: "build-app-c3"}}) if !errors.Is(err, errWalkedElsewhere) || !strings.Contains(err.Error(), "release-") { t.Fatalf("a newer build of a judged module was sent under the release's gate: %v", err) } diff --git a/cmd/mesh-controller/recorded_kept.go b/cmd/mesh-controller/recorded_kept.go index bbe6f18f..5c81bec8 100644 --- a/cmd/mesh-controller/recorded_kept.go +++ b/cmd/mesh-controller/recorded_kept.go @@ -61,20 +61,52 @@ func keptExcept(ctx context.Context) (map[string]bool, bool) { return skip, on } +type keepEveryBuildKey struct{} + +// keepingEveryBuild is a context whose sends compose every module — recorded or rolling out — at the build +// its machine was last sent (novox/hq issue 490): the grants step, which sends the machine holding the bus +// its new user list and nothing of any module's new code. That code waits there for a gate like any other +// move; the list must not, or the first machine's gate is judged against a bus that refuses what the change +// newly grants. +func keepingEveryBuild(ctx context.Context) context.Context { + return context.WithValue(ctx, keepEveryBuildKey{}, true) +} + +// everyBuildKept is whether this context keeps every module at the build its machine runs. +func everyBuildKept(ctx context.Context) bool { + on, _ := ctx.Value(keepEveryBuildKey{}).(bool) + return on +} + // recordedKept is, for a send under keepingRecorded, every recorded module the machine was last sent a // build of that the mesh's build is not identical to: module → the commit it keeps. Nil when the context // keeps nothing, or when what the machine was last sent is not known (it is then held whole elsewhere — // ADR 0221). +// +// Under keepingEveryBuild, every module the machine was sent is kept, whatever its policy (novox/hq issue +// 490), and a machine whose last send is not known refuses the send: there is nothing to keep it at, and +// composing the mesh's builds would be the very move the grants step must not make. func recordedKept(ctx context.Context, open *stores, node string) (map[string]string, error) { + every := everyBuildKept(ctx) skip, on := keptExcept(ctx) - if !on { + if !on && !every { return nil, nil } + if every { + skip = nil + } inv := open.inventory sent, known, err := inv.SentBuilds(ctx, node) - if err != nil || !known { + if err != nil { return nil, err } + if !known { + if every { + return nil, fmt.Errorf("what %s was last sent is not known, so the bus's user list cannot be sent "+ + "there alone with every build kept (novox/hq issue 490)", node) + } + return nil, nil + } current, err := inv.CurrentBuilds(ctx) if err != nil { return nil, err @@ -83,7 +115,7 @@ func recordedKept(ctx context.Context, open *stores, node string) (map[string]st out := map[string]string{} for m, was := range sent { now, held := current[m] - if !held || now.RollOut || skip[m] || was == "" || sameCommit(was, now.Commit) { + if !held || (now.RollOut && !every) || skip[m] || was == "" || sameCommit(was, now.Commit) { continue } if f == nil { @@ -118,6 +150,10 @@ func keepRecorded(ctx context.Context, open *stores, node string, shelf map[stri if err != nil { return nil, err } + if !found && everyBuildKept(ctx) { + return nil, fmt.Errorf("%s runs %s's build %s, which the build records no longer hold: the bus's user "+ + "list cannot be sent there alone with it kept (novox/hq issue 490)", node, m, short(kept[m])) + } if !found { return nil, fmt.Errorf("%s records rather than rolls out, and %s runs its build %s, which the build "+ "records no longer hold: this send cannot keep it and does not move it — `push %s` sends the new "+ diff --git a/cmd/mesh-controller/release.go b/cmd/mesh-controller/release.go index 56b702c8..87dcc079 100644 --- a/cmd/mesh-controller/release.go +++ b/cmd/mesh-controller/release.go @@ -239,6 +239,11 @@ var machineMoves = func(ctx context.Context, open *stores, f moveFacts, node str // ungatedIn refuses a send whose machines would move a build no gate has seen, outside its scope: the // machines named, and why; the machine holding the bus, added only for its user list, is left instead. func ungatedIn(ctx context.Context, open *stores, names []string, addedHolder string) ([]string, error) { + // The grants step moves no build: every module is composed at the build its machine runs (novox/hq + // issue 490), so there is nothing here for a gate to judge. + if everyBuildKept(ctx) { + return names, nil + } scope := scopeOf(ctx) if scope.person { return names, nil @@ -283,15 +288,18 @@ func ungatedIn(ctx context.Context, open *stores, names []string, addedHolder st // owns are the moves the send exists for — every module of a plan's tier whose first machine this is, in // one send (novox/hq issue 281): a send carries the machine's whole declaration (ADR 0221), so a send per // module was the same declaration sent again and again, each one setting aside the one before. -func gatedSend(ctx context.Context, open *stores, node string, owns []inventory.CarriedMove) ([]inventory.CarriedMove, []string, error) { +// +// And the grants step first, when the send changes what the bus's user list must say (novox/hq issue 490): +// answered for the gate to keep, nil when there was none. +func gatedSend(ctx context.Context, open *stores, node string, owns []inventory.CarriedMove) ([]inventory.CarriedMove, []string, *inventory.GrantsStep, error) { inv := open.inventory f, err := readMoveFacts(ctx, inv) if err != nil { - return nil, nil, err + return nil, nil, nil, err } moves, err := machineMoves(ctx, open, f, node, true) if err != nil { - return nil, nil, err + return nil, nil, nil, err } own := func(module string) bool { return slices.ContainsFunc(owns, func(o inventory.CarriedMove) bool { return o.Module == module }) @@ -301,7 +309,7 @@ func gatedSend(ctx context.Context, open *stores, node string, owns []inventory. continue } if id := f.walkedBy(mv.Module, node, mv.To); id != "" { - return nil, nil, fmt.Errorf("%w: %s's build %s waits on %s, which %s is walking", errWalkedElsewhere, + return nil, nil, nil, fmt.Errorf("%w: %s's build %s waits on %s, which %s is walking", errWalkedElsewhere, mv.Module, short(mv.To), node, id) } } @@ -309,7 +317,7 @@ func gatedSend(ctx context.Context, open *stores, node string, owns []inventory. // plan, or a release, on this machine is not crossed with a newer build; this send waits for its gate. for _, o := range owns { if id := f.walkedBy(o.Module, node, o.To); id != "" { - return nil, nil, fmt.Errorf("%w: %s's build %s waits on %s, which %s is walking", errWalkedElsewhere, + return nil, nil, nil, fmt.Errorf("%w: %s's build %s waits on %s, which %s is walking", errWalkedElsewhere, o.Module, short(o.To), node, id) } } @@ -323,22 +331,22 @@ func gatedSend(ctx context.Context, open *stores, node string, owns []inventory. } } if len(moves) == 0 && len(owns) == 0 { - return nil, nil, nil + return nil, nil, nil, nil } for i := range moves { if moves[i].Build == "" { if moves[i].Build, err = inv.BuildOf(ctx, moves[i].Module, moves[i].To); err != nil { - return nil, nil, err + return nil, nil, nil, err } } } sort.Slice(moves, func(i, j int) bool { return moves[i].Module < moves[j].Module }) sayRecreations(ctx, open, moves) - sent, err := sendRollout(withScope(ctx, sendScope{judged: map[string]bool{node: true}}), open, []string{node}) + sent, grants, err := sendJudged(ctx, open, node) if err != nil { - return nil, nil, err + return nil, nil, nil, err } - return moves, sent, nil + return moves, sent, grants, nil } // passCarried keeps a pass as the verdict of every build the gate judged beside its own module. @@ -674,7 +682,7 @@ func advanceRelease(ctx context.Context, open *stores, p *inventory.Plan) (bool, r.Next++ continue } - moves, sent, err := gatedSend(ctx, open, node, nil) + moves, sent, grants, err := gatedSend(ctx, open, node, nil) if errors.Is(err, errWalkedElsewhere) { note := fmt.Sprintf("waiting before %s: %v", node, err) changed := p.Note != note @@ -689,8 +697,12 @@ func advanceRelease(ctx context.Context, open *stores, p *inventory.Plan) (bool, r.Next++ continue } - r.Gate = &inventory.PlanGate{Machines: sent, Since: &now, Carried: moves, Sent: sentNow(ctx, open.inventory, sent)} + r.Gate = &inventory.PlanGate{Machines: sent, Since: &now, Carried: moves, Sent: sentNow(ctx, open.inventory, sent), + Grants: grants} p.Note = fmt.Sprintf("sent %s %d build(s) that waited for a gate; judging them there", node, len(moves)) + if said := grantsSaid(grants); said != "" { + p.Note += "; " + said + } if said := recreationsSaid(moves); said != "" { p.Note += "; " + said } @@ -916,3 +928,65 @@ func recreationsSaid(moves []inventory.CarriedMove) string { } return strings.Join(said, "; ") } + +// The grants step (novox/hq issue 490). +// +// The bus's user list — every module's grants, composed mesh-wide from the catalogue — travels only in the +// declaration of the machine holding the bus. A send to any other machine that changed it put that machine +// first (issue 249), unless a build waited there for a gate: then ungatedIn left it out, and said so. On +// 2026-10-11 a merge gave restic a new health tool; its walk sent restic to its first machine and judged it +// there, the machine's node-engine asked the tool and the bus refused it ("this host's grant does not name +// …"), because the machine holding the bus also runs restic, whose new build waited for that very gate. The +// gate could not pass; a person pushed the bus's machine by hand, which carried restic's new build there +// unjudged. +// +// So before a gated send, when the user list composed now is not the one the machine holding the bus was +// last sent, that machine is sent its declaration with **every build kept at the one it runs**: the new +// list, and nothing of any module's new code. Said on the gate (`plans`), and, when it cannot be sent, said +// with why and passed over: the send goes on as it did before, and its gate says what it finds. + +// brokerBehindOf is brokerBehind: a variable so a test of the walk needs no store. +var brokerBehindOf = brokerBehind + +// grantsStep sends the machine holding the bus its user list alone, ahead of a gated send to node, when the +// list changed; answers what it did, or nil when there was nothing to send. +func grantsStep(ctx context.Context, open *stores, node string) *inventory.GrantsStep { + holder, behind, err := brokerBehindOf(ctx, open, []string{node}) + if err != nil { + fmt.Printf("grants step before %s: whether the bus's user list changed could not be read: %v\n", node, err) + return &inventory.GrantsStep{Node: "the machine holding the bus", Failed: err.Error()} + } + if holder == "" || holder == node || !behind { + // No bus with a user list, the gate's own machine holds it (its send carries the list first), or the + // list is the one already sent. + return nil + } + if _, err := sendRollout(keepingEveryBuild(withScope(ctx, sendScope{})), open, []string{holder}); err != nil { + fmt.Printf("grants step: the bus's user list could not be sent to %s ahead of %s, which is sent without "+ + "it — the bus may refuse what the send newly grants: %v\n", holder, node, err) + return &inventory.GrantsStep{Node: holder, Failed: err.Error()} + } + now := time.Now().UTC() + fmt.Printf("grants step: %s sent the bus's user list alone, every build there kept, before %s is judged\n", + holder, node) + return &inventory.GrantsStep{Node: holder, At: &now} +} + +// sendJudged is a gated send's sends: the grants step when the user list changed, then the machine judged. +func sendJudged(ctx context.Context, open *stores, node string) ([]string, *inventory.GrantsStep, error) { + grants := grantsStep(ctx, open, node) + sent, err := sendRollout(withScope(ctx, sendScope{judged: map[string]bool{node: true}}), open, []string{node}) + return sent, grants, err +} + +// grantsSaid is a grants step as `plans` and a walk's note say it. +func grantsSaid(g *inventory.GrantsStep) string { + switch { + case g == nil: + return "" + case g.Failed != "": + return fmt.Sprintf(grantsStepWord+" to %s FAILED, sent without it: %s", g.Node, g.Failed) + default: + return fmt.Sprintf(grantsStepWord+": %s sent the bus's user list first, its builds kept", g.Node) + } +} diff --git a/cmd/mesh-controller/release_plan.go b/cmd/mesh-controller/release_plan.go index 7adc089c..d3a9b260 100644 --- a/cmd/mesh-controller/release_plan.go +++ b/cmd/mesh-controller/release_plan.go @@ -971,7 +971,7 @@ func firstSend(ctx context.Context, open *stores, p *inventory.Plan, node string } // A gated send (ADR 0236): everything waiting on the machine goes with the tier, and the gate judges // all of it there. - carried, sent, err := gatedSend(ctx, open, node, owns) + carried, sent, grants, err := gatedSend(ctx, open, node, owns) if err != nil { return err } @@ -992,6 +992,7 @@ func firstSend(ctx context.Context, open *stores, p *inventory.Plan, node string s.GatedBy = "" if m == lead { s.Gate.Carried = carried + s.Gate.Grants = grants } else { s.GatedBy = lead } @@ -1001,6 +1002,10 @@ func firstSend(ctx context.Context, open *stores, p *inventory.Plan, node string strings.Join(sent, ", ")) fmt.Printf("%s: tier %d built; sent %d module(s) to %s first in one send (%s), the rest once its gate passes\n", p.ID, p.Tier, len(modules), strings.Join(sent, ", "), strings.Join(modules, ", ")) + // The grants step taken before it (novox/hq issue 490), said with it. + if said := grantsSaid(grants); said != "" { + p.Note += "; " + said + } // What the send recreates, said with it (novox/hq ADR 0245). if said := recreationsSaid(carried); said != "" { p.Note += "; " + said diff --git a/internal/inventory/plans.go b/internal/inventory/plans.go index bd03edc5..718b483c 100644 --- a/internal/inventory/plans.go +++ b/internal/inventory/plans.go @@ -305,6 +305,18 @@ type PlanGate struct { // Readings are the judging's readings with their times (novox/hq ADR 0282 decision 6): every reading that // counted a pass, and the first that did not after one that did. At most maxReadings, the newest kept. Readings []GateReading `json:"readings,omitempty"` + // Grants is the grants step taken before this gate's send (novox/hq issue 490): the bus's user list sent + // alone to the machine holding the bus, every build there kept, so what the send newly grants is on the + // bus before the gate judges it. Nil when the list did not change, or the gate's machine holds the bus. + Grants *GrantsStep `json:"grants,omitempty"` +} + +// GrantsStep is the bus's user list sent to the machine holding the bus ahead of a gated send (novox/hq +// issue 490): to which machine, when, and, when it could not be sent, why — the send went on without it. +type GrantsStep struct { + Node string `json:"node"` + At *time.Time `json:"at,omitempty"` + Failed string `json:"failed,omitempty"` } // GateReading is one reading of a first-node gate. From fbeffb608deda67faec5f2223e2e26b5d04d736c Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 19:09:23 +0200 Subject: [PATCH 4/8] Refuse the grants step when its send would change more than the bus's user list (review of #230) Kept builds are not the whole declaration: a new assignment, settings or a provision's logins went out as the mesh holds them now. The step now compares what it would send with the last send's summary and fails, said, when anything but the user list differs; ungatedIn reads the kept builds instead of passing the step unread. --- cmd/mesh-controller/grants_step_store_test.go | 161 ++++++++++++++++++ cmd/mesh-controller/grants_step_test.go | 25 ++- cmd/mesh-controller/release.go | 111 +++++++++++- 3 files changed, 285 insertions(+), 12 deletions(-) create mode 100644 cmd/mesh-controller/grants_step_store_test.go diff --git a/cmd/mesh-controller/grants_step_store_test.go b/cmd/mesh-controller/grants_step_store_test.go new file mode 100644 index 00000000..ecd664ac --- /dev/null +++ b/cmd/mesh-controller/grants_step_store_test.go @@ -0,0 +1,161 @@ +package main + +import ( + "encoding/json" + "strings" + "testing" + "time" + + "github.com/novox/mesh-controller/internal/catalogue" + "github.com/novox/mesh-controller/internal/inventory" + "github.com/novox/mesh-controller/internal/link" +) + +// The grants step on a store (novox/hq issue 490): the machine holding the bus is sent its new user list, +// and nothing else — not the new build of a module it runs, not a module newly assigned there. +// +// The restic shape: anchor holds the bus and runs restic at c1, as does laptop; restic's c2 gives it a +// health tool. The grants step's send to anchor composes restic at c1, carries the user list that grants +// laptop's node-engine c2's tool, and records that anchor still runs c1. A module newly assigned to anchor +// makes the step fail, said, rather than install it unjudged. +func TestTheGrantsStepSendsTheUserListAndNothingElse(t *testing.T) { + open := aMesh(t) + ctx := t.Context() + inv := open.inventory + start := time.Now().Add(-time.Hour) + image := "registry.invalid:5000/restic/backup@sha256:" + strings.Repeat("b", 64) + build := func(module, commit string, at time.Time, manifest map[string]any) link.BuildResult { + manifest["module"], manifest["version"] = module, "1" + raw, _ := json.Marshal(manifest) + return link.BuildResult{ID: link.NewBuildID(at), Repository: "novox/mesh-catalog", Path: "modules/" + module, + On: "anchor", Module: module, Commit: commit, Manifest: raw, + Source: &link.SourceOnSeat{Seat: "git", Repository: "novox/mesh-catalog"}} + } + restic := func(health map[string]any, tools ...string) map[string]any { + r := map[string]any{"id": "backup", "type": "container", "name": "restic-backup", "image": image} + if health != nil { + r["health"] = health + } + m := map[string]any{"resources": []any{r}} + if len(tools) > 0 { + m["tools"] = tools + } + return m + } + nats := map[string]any{"bus-users": "/var/lib/nats-module/conf/accounts.conf", + "claims": []any{map[string]any{"name": catalogue.BrokerSeat, "scope": catalogue.ScopeMesh}}} + for _, b := range []link.BuildResult{ + build("nats", "n1", start, nats), + build("restic", "c1", start.Add(time.Second), restic(nil)), + build("wallpaper", "w1", start.Add(2*time.Second), map[string]any{"resources": []any{ + map[string]any{"id": "paper", "type": "container", "name": "wallpaper", "image": image}}}), + } { + if _, _, err := takeIn(ctx, inv, asTheOperator(t, inv, b)); err != nil { + t.Fatal(err) + } + } + for _, a := range [][2]string{{"anchor", "nats"}, {"anchor", "restic"}, {"laptop", "restic"}} { + if _, err := inv.Assign(ctx, a[0], a[1]); err != nil { + t.Fatal(err) + } + } + for _, n := range []string{"anchor", "laptop"} { + if _, err := inv.MintBusPassword(ctx, inventory.BusUser{Username: "node." + n, Kind: inventory.BusNode, Node: n}); err != nil { + t.Fatal(err) + } + } + + // What anchor was last sent: everything at the builds of before the merge, kept as a send keeps it. + sendAnchor := func() { + t.Helper() + plan, settings, err := planFor(keepingRecorded(ctx), open, "anchor") + if err != nil { + t.Fatal(err) + } + gens, err := generators(ctx, open) + if err != nil { + t.Fatal(err) + } + declared, err := declarationWith(keepingRecorded(ctx), open, "anchor", plan, settings, gens, Allocating) + if err != nil { + t.Fatal(err) + } + body, err := declared.Body() + if err != nil { + t.Fatal(err) + } + if _, err := recordSent(ctx, inv, "anchor", body, declared.Builds, 0); err != nil { + t.Fatal(err) + } + } + sendAnchor() + if only, err := grantsOnly(ctx, open, "anchor"); err != nil || only != "" { + t.Fatalf("with nothing changed, the step reads %q, %v", only, err) + } + + // The merge: restic's c2 gives it a health tool. + if _, _, err := takeIn(ctx, inv, build("restic", "c2", start.Add(time.Minute), + restic(map[string]any{"kind": catalogue.HealthTool, "tool": "backup_health"}, "backup_health"))); err != nil { + t.Fatal(err) + } + + // The step's send to anchor: restic as anchor runs it, c1, and the user list granting c2's tool. + kept := keepingEveryBuild(keepingRecorded(ctx)) + plan, settings, err := planFor(kept, open, "anchor") + if err != nil { + t.Fatal(err) + } + for _, m := range plan.Modules { + if m.Module == "restic" && len(m.Tools) > 0 { + t.Fatalf("the grants step composed restic's new build on anchor: tools %v", m.Tools) + } + } + list, _, err := busUserList(ctx, inv, plan.Modules) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(list, "mesh.mod.restic.tool.backup_health.laptop") { + t.Errorf("the user list the step sends does not grant laptop's node-engine restic's new tool:\n%s", list) + } + gens, err := generators(kept, open) + if err != nil { + t.Fatal(err) + } + declared, err := declarationWith(kept, open, "anchor", plan, settings, gens, Allocating) + if err != nil { + t.Fatal(err) + } + if declared.Builds["restic"] != "c1" { + t.Fatalf("the step's send records it carries restic %q; want c1, which anchor runs", declared.Builds["restic"]) + } + if !strings.Contains(declared.BusUsers, "mesh.mod.restic.tool.backup_health.laptop") { + t.Error("the step's declaration does not carry the new grant") + } + // Recorded as a send records it: anchor still runs restic c1, and its gate is still to come there. + if err := inv.RecordSent(ctx, nodeID(t, open, "anchor"), "d-grants", declared.Builds); err != nil { + t.Fatal(err) + } + if sent, _, err := inv.SentBuilds(ctx, "anchor"); err != nil || sent["restic"] != "c1" { + t.Fatalf("after the step anchor reads as sent restic %q (%v); want c1", sent["restic"], err) + } + // The guard: only the user list differs from what anchor was last sent. + if only, err := grantsOnly(ctx, open, "anchor"); err != nil || only != "" { + t.Fatalf("the step reads as changing more than the user list: %q, %v", only, err) + } + // And the gate's refusal still reads the step's send: it moves nothing there. + if names, err := ungatedIn(kept, open, []string{"anchor"}, ""); err != nil || strings.Join(names, ",") != "anchor" { + t.Fatalf("the step's send is refused as a move: %v, %v", names, err) + } + + // A module newly assigned to anchor: the step would install it, unjudged, so it is not sent. + if _, err := inv.Assign(ctx, "anchor", "wallpaper"); err != nil { + t.Fatal(err) + } + only, err := grantsOnly(ctx, open, "anchor") + if err != nil { + t.Fatal(err) + } + if !strings.Contains(only, "wallpaper newly assigned") || !strings.Contains(only, "wallpaper") { + t.Fatalf("a new assignment on the bus's machine reads %q; want the step refused, naming it", only) + } +} diff --git a/cmd/mesh-controller/grants_step_test.go b/cmd/mesh-controller/grants_step_test.go index 59458c6d..fe128009 100644 --- a/cmd/mesh-controller/grants_step_test.go +++ b/cmd/mesh-controller/grants_step_test.go @@ -32,8 +32,9 @@ type aSend struct { func sendsRecorded(t *testing.T, holder string, behind bool, refuse error) *[]aSend { t.Helper() var sends []aSend - wasSend, wasBehind := sendRollout, brokerBehindOf - t.Cleanup(func() { sendRollout, brokerBehindOf = wasSend, wasBehind }) + wasSend, wasBehind, wasOnly := sendRollout, brokerBehindOf, grantsOnlyOf + t.Cleanup(func() { sendRollout, brokerBehindOf, grantsOnlyOf = wasSend, wasBehind, wasOnly }) + grantsOnlyOf = func(context.Context, *stores, string) (string, error) { return "", nil } brokerBehindOf = func(_ context.Context, _ *stores, names []string) (string, bool, error) { if slices.Contains(names, holder) { return holder, false, nil @@ -122,11 +123,21 @@ func TestAGrantsStepThatFailsIsSaidAndTheSendGoesOn(t *testing.T) { } } -// The grants step moves no build, so no gate's refusal holds it back: it is never read for moves. -func TestTheGrantsStepIsNotHeldForTheBuildsItKeeps(t *testing.T) { - names, err := ungatedIn(keepingEveryBuild(t.Context()), nil, []string{"novox"}, "") - if err != nil || strings.Join(names, ",") != "novox" { - t.Errorf("the grants step's send reads %v, %v", names, err) +// A step whose send would change more than the user list is not sent: it fails, said, and the walk goes on. +func TestAGrantsStepThatWouldCarryMoreIsNotSent(t *testing.T) { + sends := sendsRecorded(t, "novox", true, nil) + grantsOnlyOf = func(context.Context, *stores, string) (string, error) { + return "its send would change more than the bus's user list: -postgres.login-n8n", nil + } + sent, grants, err := sendJudged(t.Context(), nil, "ace") + if err != nil { + t.Fatal(err) + } + if len(*sends) != 1 || (*sends)[0].keepsAll || strings.Join(sent, ",") != "ace" { + t.Errorf("the walk made %+v, want ace's judged send alone", *sends) + } + if grants == nil || grants.At != nil || !strings.Contains(grants.Failed, "-postgres.login-n8n") { + t.Fatalf("the gate keeps %+v", grants) } } diff --git a/cmd/mesh-controller/release.go b/cmd/mesh-controller/release.go index 87dcc079..afc2db23 100644 --- a/cmd/mesh-controller/release.go +++ b/cmd/mesh-controller/release.go @@ -2,6 +2,7 @@ package main import ( "context" + "encoding/json" "errors" "flag" "fmt" @@ -239,11 +240,6 @@ var machineMoves = func(ctx context.Context, open *stores, f moveFacts, node str // ungatedIn refuses a send whose machines would move a build no gate has seen, outside its scope: the // machines named, and why; the machine holding the bus, added only for its user list, is left instead. func ungatedIn(ctx context.Context, open *stores, names []string, addedHolder string) ([]string, error) { - // The grants step moves no build: every module is composed at the build its machine runs (novox/hq - // issue 490), so there is nothing here for a gate to judge. - if everyBuildKept(ctx) { - return names, nil - } scope := scopeOf(ctx) if scope.person { return names, nil @@ -258,8 +254,19 @@ func ungatedIn(ctx context.Context, open *stores, names []string, addedHolder st if err != nil { return nil, err } + // The grants step composes every module at the build its machine was last sent (novox/hq issue 490): + // a move it keeps is not made. Read, not assumed, so a move it could not keep is still refused here. + var keeps map[string]string + if everyBuildKept(ctx) { + if keeps, err = recordedKept(ctx, open, n); err != nil { + return nil, err + } + } var waiting []string for _, mv := range moves { + if was, kept := keeps[mv.Module]; kept && sameCommit(was, mv.From) { + continue + } if !scope.judged[n] && !scope.modules[mv.Module] { waiting = append(waiting, fmt.Sprintf("%s %s → %s", mv.Module, short(mv.From), short(mv.To))) } @@ -961,6 +968,19 @@ func grantsStep(ctx context.Context, open *stores, node string) *inventory.Grant // list is the one already sent. return nil } + // **The user list and nothing else**: checked before it is sent, against what the machine was last sent. + // Kept builds are not the whole of a declaration — a new assignment, a settings change, an assignment + // taken away, a provision's logins are composed as the mesh holds them now — and a step that carried any + // of it would be the unjudged change this step exists to avoid. + if other, err := grantsOnlyOf(ctx, open, holder); err != nil || other != "" { + why := other + if err != nil { + why = err.Error() + } + fmt.Printf("grants step: %s is not sent its user list ahead of %s, which is sent without it: %s\n", + holder, node, why) + return &inventory.GrantsStep{Node: holder, Failed: why} + } if _, err := sendRollout(keepingEveryBuild(withScope(ctx, sendScope{})), open, []string{holder}); err != nil { fmt.Printf("grants step: the bus's user list could not be sent to %s ahead of %s, which is sent without "+ "it — the bus may refuse what the send newly grants: %v\n", holder, node, err) @@ -972,6 +992,87 @@ func grantsStep(ctx context.Context, open *stores, node string) *inventory.Grant return &inventory.GrantsStep{Node: holder, At: &now} } +// grantsOnlyOf is grantsOnly: a variable so a test of the walk needs no store. +var grantsOnlyOf = grantsOnly + +// grantsOnly is what the grants step's send to the machine holding the bus would change there besides its +// user list, against what it was last sent (novox/hq issue 490); empty when nothing else. Composed as the +// step's send composes it, without allocating anything (Foreseeing), and compared resource by resource +// with the summary the last send kept (ADR 0217): exact, rather than a list of the cases that can differ. +// A module there that was never sent is a new assignment, and refused too, whatever its resources. +func grantsOnly(ctx context.Context, open *stores, holder string) (string, error) { + inv := open.inventory + kept := keepingEveryBuild(keepingRecorded(ctx)) + plan, settings, err := planFor(kept, open, holder) + if err != nil { + return "", err + } + sent, known, err := inv.SentBuilds(ctx, holder) + if err != nil { + return "", err + } + if !known { + return fmt.Sprintf("what %s was last sent is not known", holder), nil + } + var other []string + listID := "" + for _, m := range plan.Modules { + if _, was := sent[m.Module]; !was { + other = append(other, m.Module+" newly assigned") + } + if m.BusUsers != "" && m.ClaimsSeat(catalogue.BrokerSeat) { + listID = m.Module + "." + catalogue.BusUsersID() + } + } + gens, err := generators(kept, open) + if err != nil { + return "", err + } + declared, err := declarationWith(kept, open, holder, plan, settings, gens, Foreseeing) + if err != nil { + return "", err + } + for _, f := range declared.foreseen { + other = append(other, f+" would be minted") + } + body, err := declared.Body() + if err != nil { + return "", err + } + after, err := summarize(body) + if err != nil { + return "", err + } + prev, err := inv.SentSummary(ctx, holder) + if err != nil { + return "", err + } + if len(prev) == 0 { + return fmt.Sprintf("what %s was last sent is not kept for comparison", holder), nil + } + var before []sentResource + if err := json.Unmarshal(prev, &before); err != nil { + return "", fmt.Errorf("what %s was last sent is kept in a form this version does not read: %w", holder, err) + } + d := diffSent(before, after) + for _, id := range d.Added { + other = append(other, "+"+id) + } + for _, id := range d.Removed { + other = append(other, "-"+id) + } + for _, c := range d.Changed { + if c.ID == listID { + continue + } + other = append(other, fmt.Sprintf("~%s (%s)", c.ID, strings.Join(c.Fields, ", "))) + } + if len(other) == 0 { + return "", nil + } + return "its send would change more than the bus's user list: " + strings.Join(other, "; "), nil +} + // sendJudged is a gated send's sends: the grants step when the user list changed, then the machine judged. func sendJudged(ctx context.Context, open *stores, node string) ([]string, *inventory.GrantsStep, error) { grants := grantsStep(ctx, open, node) From 78205d7405555874c2be673604c819e39392b7ac Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 19:12:33 +0200 Subject: [PATCH 5/8] Judge the grants step on the declaration it sends, so nothing composed between the check and the send slips through (review of #230) The guard composed its own declaration ahead of the send; now sendToEach judges the one it composed and stamped, and refuses it unsent. --- cmd/mesh-controller/grants_step_store_test.go | 85 +++++++++++-------- cmd/mesh-controller/grants_step_test.go | 19 ++--- cmd/mesh-controller/push.go | 11 +++ cmd/mesh-controller/release.go | 61 ++++++------- 4 files changed, 96 insertions(+), 80 deletions(-) diff --git a/cmd/mesh-controller/grants_step_store_test.go b/cmd/mesh-controller/grants_step_store_test.go index ecd664ac..ae01d6e7 100644 --- a/cmd/mesh-controller/grants_step_store_test.go +++ b/cmd/mesh-controller/grants_step_store_test.go @@ -1,6 +1,7 @@ package main import ( + "context" "encoding/json" "strings" "testing" @@ -65,31 +66,50 @@ func TestTheGrantsStepSendsTheUserListAndNothingElse(t *testing.T) { } } - // What anchor was last sent: everything at the builds of before the merge, kept as a send keeps it. - sendAnchor := func() { + wasEpoch := epochForActs + epochForActs = func(context.Context) (uint64, error) { return 7, nil } + t.Cleanup(func() { epochForActs = wasEpoch }) + + // A send to anchor composed as sendToEach composes it: numbered, planned, declared, stamped. + compose := func(under context.Context) (catalogue.Resolution, sendable) { t.Helper() - plan, settings, err := planFor(keepingRecorded(ctx), open, "anchor") + numbered, err := allot(under, inv, "anchor") if err != nil { t.Fatal(err) } - gens, err := generators(ctx, open) + plan, settings, err := planFor(under, open, "anchor") if err != nil { t.Fatal(err) } - declared, err := declarationWith(keepingRecorded(ctx), open, "anchor", plan, settings, gens, Allocating) + gens, err := generators(under, open) if err != nil { t.Fatal(err) } + declared, err := declarationWith(under, open, "anchor", plan, settings, gens, Allocating) + if err != nil { + t.Fatal(err) + } + numbered.stamp(&declared) + return plan, declared + } + record := func(declared sendable) { + t.Helper() body, err := declared.Body() if err != nil { t.Fatal(err) } - if _, err := recordSent(ctx, inv, "anchor", body, declared.Builds, 0); err != nil { + if _, err := recordSent(ctx, inv, "anchor", body, declared.Builds, declared.Epoch, + sentRecordOf(ctx, declared)); err != nil { t.Fatal(err) } } - sendAnchor() - if only, err := grantsOnly(ctx, open, "anchor"); err != nil || only != "" { + kept := keepingEveryBuild(keepingRecorded(ctx)) + + // What anchor was last sent: everything at the builds of before the merge, as an ordinary send sends it. + _, before := compose(keepingRecorded(ctx)) + record(before) + plan, declared := compose(kept) + if only, err := grantsOnlyIn(ctx, open, "anchor", plan, declared); err != nil || only != "" { t.Fatalf("with nothing changed, the step reads %q, %v", only, err) } @@ -100,62 +120,53 @@ func TestTheGrantsStepSendsTheUserListAndNothingElse(t *testing.T) { } // The step's send to anchor: restic as anchor runs it, c1, and the user list granting c2's tool. - kept := keepingEveryBuild(keepingRecorded(ctx)) - plan, settings, err := planFor(kept, open, "anchor") + generation, err := inv.AssignmentGeneration(ctx) if err != nil { t.Fatal(err) } + plan, declared = compose(kept) for _, m := range plan.Modules { if m.Module == "restic" && len(m.Tools) > 0 { t.Fatalf("the grants step composed restic's new build on anchor: tools %v", m.Tools) } } - list, _, err := busUserList(ctx, inv, plan.Modules) - if err != nil { - t.Fatal(err) - } - if !strings.Contains(list, "mesh.mod.restic.tool.backup_health.laptop") { - t.Errorf("the user list the step sends does not grant laptop's node-engine restic's new tool:\n%s", list) - } - gens, err := generators(kept, open) - if err != nil { - t.Fatal(err) - } - declared, err := declarationWith(kept, open, "anchor", plan, settings, gens, Allocating) - if err != nil { - t.Fatal(err) - } if declared.Builds["restic"] != "c1" { t.Fatalf("the step's send records it carries restic %q; want c1, which anchor runs", declared.Builds["restic"]) } if !strings.Contains(declared.BusUsers, "mesh.mod.restic.tool.backup_health.laptop") { - t.Error("the step's declaration does not carry the new grant") + t.Errorf("the step's declaration does not grant laptop's node-engine restic's new tool:\n%s", declared.BusUsers) } - // Recorded as a send records it: anchor still runs restic c1, and its gate is still to come there. - if err := inv.RecordSent(ctx, nodeID(t, open, "anchor"), "d-grants", declared.Builds); err != nil { - t.Fatal(err) + // The generation it carries is the current one (novox/hq issue 234), and the step does not raise it. + if declared.composedFrom != generation { + t.Errorf("the step's send was composed from generation %d; the mesh is at %d", declared.composedFrom, generation) } - if sent, _, err := inv.SentBuilds(ctx, "anchor"); err != nil || sent["restic"] != "c1" { - t.Fatalf("after the step anchor reads as sent restic %q (%v); want c1", sent["restic"], err) - } - // The guard: only the user list differs from what anchor was last sent. - if only, err := grantsOnly(ctx, open, "anchor"); err != nil || only != "" { + // The guard, on the very declaration the send sends: only the user list differs from the last send. + if only, err := grantsOnlyIn(ctx, open, "anchor", plan, declared); err != nil || only != "" { t.Fatalf("the step reads as changing more than the user list: %q, %v", only, err) } // And the gate's refusal still reads the step's send: it moves nothing there. if names, err := ungatedIn(kept, open, []string{"anchor"}, ""); err != nil || strings.Join(names, ",") != "anchor" { t.Fatalf("the step's send is refused as a move: %v, %v", names, err) } + // Recorded as the send records it: anchor still runs restic c1, its gate still to come there. + record(declared) + if sent, _, err := inv.SentBuilds(ctx, "anchor"); err != nil || sent["restic"] != "c1" { + t.Fatalf("after the step anchor reads as sent restic %q (%v); want c1", sent["restic"], err) + } + if after, err := inv.AssignmentGeneration(ctx); err != nil || after != generation { + t.Fatalf("the step moved the assignment generation from %d to %d (%v)", generation, after, err) + } - // A module newly assigned to anchor: the step would install it, unjudged, so it is not sent. + // A module newly assigned to anchor: the step would install it, unjudged, so it is refused unsent. if _, err := inv.Assign(ctx, "anchor", "wallpaper"); err != nil { t.Fatal(err) } - only, err := grantsOnly(ctx, open, "anchor") + plan, declared = compose(kept) + only, err := grantsOnlyIn(ctx, open, "anchor", plan, declared) if err != nil { t.Fatal(err) } - if !strings.Contains(only, "wallpaper newly assigned") || !strings.Contains(only, "wallpaper") { + if !strings.Contains(only, "wallpaper newly assigned") || !strings.Contains(only, "+wallpaper") { t.Fatalf("a new assignment on the bus's machine reads %q; want the step refused, naming it", only) } } diff --git a/cmd/mesh-controller/grants_step_test.go b/cmd/mesh-controller/grants_step_test.go index fe128009..7592f8f1 100644 --- a/cmd/mesh-controller/grants_step_test.go +++ b/cmd/mesh-controller/grants_step_test.go @@ -3,6 +3,7 @@ package main import ( "context" "errors" + "fmt" "slices" "strings" "testing" @@ -32,9 +33,8 @@ type aSend struct { func sendsRecorded(t *testing.T, holder string, behind bool, refuse error) *[]aSend { t.Helper() var sends []aSend - wasSend, wasBehind, wasOnly := sendRollout, brokerBehindOf, grantsOnlyOf - t.Cleanup(func() { sendRollout, brokerBehindOf, grantsOnlyOf = wasSend, wasBehind, wasOnly }) - grantsOnlyOf = func(context.Context, *stores, string) (string, error) { return "", nil } + wasSend, wasBehind := sendRollout, brokerBehindOf + t.Cleanup(func() { sendRollout, brokerBehindOf = wasSend, wasBehind }) brokerBehindOf = func(_ context.Context, _ *stores, names []string) (string, bool, error) { if slices.Contains(names, holder) { return holder, false, nil @@ -123,18 +123,17 @@ func TestAGrantsStepThatFailsIsSaidAndTheSendGoesOn(t *testing.T) { } } -// A step whose send would change more than the user list is not sent: it fails, said, and the walk goes on. +// A step whose send would change more than the user list is refused unsent by the send (sendToEach): the +// gate says it FAILED with what differed, and the walk goes on. func TestAGrantsStepThatWouldCarryMoreIsNotSent(t *testing.T) { - sends := sendsRecorded(t, "novox", true, nil) - grantsOnlyOf = func(context.Context, *stores, string) (string, error) { - return "its send would change more than the bus's user list: -postgres.login-n8n", nil - } + refusal := fmt.Errorf("%w: its send would change more than the bus's user list: -postgres.login-n8n", errNotGrantsOnly) + sends := sendsRecorded(t, "novox", true, refusal) sent, grants, err := sendJudged(t.Context(), nil, "ace") if err != nil { t.Fatal(err) } - if len(*sends) != 1 || (*sends)[0].keepsAll || strings.Join(sent, ",") != "ace" { - t.Errorf("the walk made %+v, want ace's judged send alone", *sends) + if strings.Join(sent, ",") != "ace" || len(*sends) != 2 { + t.Errorf("the walk made %+v, want the refused step and then ace's judged send", *sends) } if grants == nil || grants.At != nil || !strings.Contains(grants.Failed, "-postgres.login-n8n") { t.Fatalf("the gate keeps %+v", grants) diff --git a/cmd/mesh-controller/push.go b/cmd/mesh-controller/push.go index 9f3c1434..30e3fd84 100644 --- a/cmd/mesh-controller/push.go +++ b/cmd/mesh-controller/push.go @@ -1236,6 +1236,17 @@ func sendToEach(ctx context.Context, open *stores, names []string) ([]string, er continue } numbered.stamp(&declared) + // **The grants step sends the user list and nothing else** (novox/hq issue 490): judged on this very + // declaration, the one sent, so nothing composed between a check and the send can slip through. + if everyBuildKept(ctx) { + other, err := grantsOnlyIn(ctx, open, name, plan, declared) + if err != nil { + return nil, err + } + if other != "" { + return nil, fmt.Errorf("%w: %s", errNotGrantsOnly, other) + } + } reportLeftOut(name, declared) sending = append(sending, readyNode{name, declared}) } diff --git a/cmd/mesh-controller/release.go b/cmd/mesh-controller/release.go index afc2db23..1b872036 100644 --- a/cmd/mesh-controller/release.go +++ b/cmd/mesh-controller/release.go @@ -968,19 +968,6 @@ func grantsStep(ctx context.Context, open *stores, node string) *inventory.Grant // list is the one already sent. return nil } - // **The user list and nothing else**: checked before it is sent, against what the machine was last sent. - // Kept builds are not the whole of a declaration — a new assignment, a settings change, an assignment - // taken away, a provision's logins are composed as the mesh holds them now — and a step that carried any - // of it would be the unjudged change this step exists to avoid. - if other, err := grantsOnlyOf(ctx, open, holder); err != nil || other != "" { - why := other - if err != nil { - why = err.Error() - } - fmt.Printf("grants step: %s is not sent its user list ahead of %s, which is sent without it: %s\n", - holder, node, why) - return &inventory.GrantsStep{Node: holder, Failed: why} - } if _, err := sendRollout(keepingEveryBuild(withScope(ctx, sendScope{})), open, []string{holder}); err != nil { fmt.Printf("grants step: the bus's user list could not be sent to %s ahead of %s, which is sent without "+ "it — the bus may refuse what the send newly grants: %v\n", holder, node, err) @@ -992,21 +979,19 @@ func grantsStep(ctx context.Context, open *stores, node string) *inventory.Grant return &inventory.GrantsStep{Node: holder, At: &now} } -// grantsOnlyOf is grantsOnly: a variable so a test of the walk needs no store. -var grantsOnlyOf = grantsOnly +// errNotGrantsOnly is a grants step refused unsent: its declaration would change more than the user list. +var errNotGrantsOnly = errors.New("the grants step would change more than the bus's user list, and is not sent") -// grantsOnly is what the grants step's send to the machine holding the bus would change there besides its -// user list, against what it was last sent (novox/hq issue 490); empty when nothing else. Composed as the -// step's send composes it, without allocating anything (Foreseeing), and compared resource by resource -// with the summary the last send kept (ADR 0217): exact, rather than a list of the cases that can differ. -// A module there that was never sent is a new assignment, and refused too, whatever its resources. -func grantsOnly(ctx context.Context, open *stores, holder string) (string, error) { +// grantsOnlyIn is what a grants step's declaration, composed for sending, would change on the machine holding +// the bus besides its user list, against what it was last sent (novox/hq issue 490); empty when nothing else. +// Judged by sendToEach on the very declaration it then sends — one composition, judged, then sent or refused +// unsent. Kept builds are not the whole of a declaration: a new assignment, a settings change, an assignment +// taken away, a provision's logins are composed as the mesh holds them now, and a step that carried any of +// it would be the unjudged change this step exists to avoid. Compared resource by resource with the summary +// the last send kept (ADR 0217): exact, rather than a list of the cases that can differ. A module there that +// was never sent is a new assignment, refused whatever its resources. +func grantsOnlyIn(ctx context.Context, open *stores, holder string, plan catalogue.Resolution, declared sendable) (string, error) { inv := open.inventory - kept := keepingEveryBuild(keepingRecorded(ctx)) - plan, settings, err := planFor(kept, open, holder) - if err != nil { - return "", err - } sent, known, err := inv.SentBuilds(ctx, holder) if err != nil { return "", err @@ -1024,13 +1009,22 @@ func grantsOnly(ctx context.Context, open *stores, holder string) (string, error listID = m.Module + "." + catalogue.BusUsersID() } } - gens, err := generators(kept, open) - if err != nil { - return "", err - } - declared, err := declarationWith(kept, open, holder, plan, settings, gens, Foreseeing) - if err != nil { - return "", err + var facts *moveFacts + for m, was := range sent { + now, carried := declared.Builds[m] + if !carried || sameCommit(now, was) { + continue + } + if facts == nil { + f, err := readMoveFacts(ctx, inv) + if err != nil { + return "", err + } + facts = &f + } + if !facts.identical(m, was, now) { + other = append(other, fmt.Sprintf("%s %s → %s", m, short(was), short(now))) + } } for _, f := range declared.foreseen { other = append(other, f+" would be minted") @@ -1067,6 +1061,7 @@ func grantsOnly(ctx context.Context, open *stores, holder string) (string, error } other = append(other, fmt.Sprintf("~%s (%s)", c.ID, strings.Join(c.Fields, ", "))) } + sort.Strings(other) if len(other) == 0 { return "", nil } From 0f853e93fa11aee77f74cab9934d6943fc173338 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 19:17:28 +0200 Subject: [PATCH 6/8] Exempt only the user list's content from the grants step's guard, and test the guard in the send itself (review of #230) The guard let any field of the list's resource move; its path or mode changing is a change the step must not carry. The store test now calls sendToEach, so removing the guard there fails it. --- cmd/mesh-controller/grants_step_store_test.go | 8 ++++++ cmd/mesh-controller/grants_step_test.go | 28 +++++++++++++++++++ cmd/mesh-controller/release.go | 22 ++++++++++----- 3 files changed, 51 insertions(+), 7 deletions(-) diff --git a/cmd/mesh-controller/grants_step_store_test.go b/cmd/mesh-controller/grants_step_store_test.go index ae01d6e7..7f675c07 100644 --- a/cmd/mesh-controller/grants_step_store_test.go +++ b/cmd/mesh-controller/grants_step_store_test.go @@ -3,11 +3,13 @@ package main import ( "context" "encoding/json" + "errors" "strings" "testing" "time" "github.com/novox/mesh-controller/internal/catalogue" + "github.com/novox/mesh-controller/internal/identity" "github.com/novox/mesh-controller/internal/inventory" "github.com/novox/mesh-controller/internal/link" ) @@ -169,4 +171,10 @@ func TestTheGrantsStepSendsTheUserListAndNothingElse(t *testing.T) { if !strings.Contains(only, "wallpaper newly assigned") || !strings.Contains(only, "+wallpaper") { t.Fatalf("a new assignment on the bus's machine reads %q; want the step refused, naming it", only) } + // And the send itself refuses it, unsent: judged where it is composed, before the bus is dialled. + identity.ForTest(t) + if _, err := sendToEach(kept, open, []string{"anchor"}); !errors.Is(err, errNotGrantsOnly) || + !strings.Contains(err.Error(), "wallpaper") { + t.Fatalf("the grants step's send of a new assignment was not refused by the send: %v", err) + } } diff --git a/cmd/mesh-controller/grants_step_test.go b/cmd/mesh-controller/grants_step_test.go index 7592f8f1..2820bb51 100644 --- a/cmd/mesh-controller/grants_step_test.go +++ b/cmd/mesh-controller/grants_step_test.go @@ -2,6 +2,7 @@ package main import ( "context" + "encoding/json" "errors" "fmt" "slices" @@ -140,6 +141,33 @@ func TestAGrantsStepThatWouldCarryMoreIsNotSent(t *testing.T) { } } +// The step's exemption is the list's content alone: its path or mode moving is a change like any other. +func TestOnlyTheUserListsContentIsExempt(t *testing.T) { + list := "nats.bus-users" + file := func(fields map[string]any) sentResource { + r := map[string]any{"id": list, "type": "file", "path": "/conf/accounts.conf", "mode": "0600", "content": "a"} + for k, v := range fields { + r[k] = v + } + body, _ := json.Marshal(map[string]any{"resources": []any{r}}) + s, err := summarize(body) + if err != nil { + t.Fatal(err) + } + return s[0] + } + was := file(nil) + if other := otherThanTheList(diffSent([]sentResource{was}, []sentResource{file(map[string]any{"content": "b"})}), list); len(other) != 0 { + t.Errorf("the list's new content reads as more: %v", other) + } + for _, f := range []map[string]any{{"mode": "0644"}, {"path": "/elsewhere"}, {"content": "b", "owner": "nats"}} { + other := otherThanTheList(diffSent([]sentResource{was}, []sentResource{file(f)}), list) + if len(other) != 1 || !strings.HasPrefix(other[0], "~"+list) { + t.Errorf("the list's resource changed by %v reads %v; want it refused", f, other) + } + } +} + // The delivery plan says the step before the merge. func TestAChangePlanSaysTheGrantsStep(t *testing.T) { const catalogue_ = "http://forge.internal:20000/novox/mesh-catalog.git" diff --git a/cmd/mesh-controller/release.go b/cmd/mesh-controller/release.go index 1b872036..8be20f46 100644 --- a/cmd/mesh-controller/release.go +++ b/cmd/mesh-controller/release.go @@ -1048,7 +1048,19 @@ func grantsOnlyIn(ctx context.Context, open *stores, holder string, plan catalog if err := json.Unmarshal(prev, &before); err != nil { return "", fmt.Errorf("what %s was last sent is kept in a form this version does not read: %w", holder, err) } - d := diffSent(before, after) + other = append(other, otherThanTheList(diffSent(before, after), listID)...) + sort.Strings(other) + if len(other) == 0 { + return "", nil + } + return "its send would change more than the bus's user list: " + strings.Join(other, "; "), nil +} + +// otherThanTheList is what a diff against the last send changes besides the user list's content: every +// resource added, removed or changed, but the list's own resource when its content is all that changed — +// its path, mode and every other field must be as last sent (novox/hq issue 490). +func otherThanTheList(d sentDiff, listID string) []string { + var other []string for _, id := range d.Added { other = append(other, "+"+id) } @@ -1056,16 +1068,12 @@ func grantsOnlyIn(ctx context.Context, open *stores, holder string, plan catalog other = append(other, "-"+id) } for _, c := range d.Changed { - if c.ID == listID { + if c.ID == listID && len(c.Fields) == 1 && c.Fields[0] == "content" { continue } other = append(other, fmt.Sprintf("~%s (%s)", c.ID, strings.Join(c.Fields, ", "))) } - sort.Strings(other) - if len(other) == 0 { - return "", nil - } - return "its send would change more than the bus's user list: " + strings.Join(other, "; "), nil + return other } // sendJudged is a gated send's sends: the grants step when the user list changed, then the machine judged. From 325575b2929c93935218a504b96a03921eac68a0 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 19:30:10 +0200 Subject: [PATCH 7/8] Keep the builds of the grants step's machine alone, not of every machine its composition resolves (repo-check of #230) Composing the bus's machine resolves the others on the same context, and a machine never recorded as sent refused the whole composition. --- cmd/mesh-controller/grants_step_store_test.go | 5 ++- cmd/mesh-controller/grants_step_test.go | 11 ++++++- cmd/mesh-controller/push.go | 2 +- cmd/mesh-controller/recorded_kept.go | 31 +++++++++++-------- cmd/mesh-controller/release.go | 4 +-- 5 files changed, 35 insertions(+), 18 deletions(-) diff --git a/cmd/mesh-controller/grants_step_store_test.go b/cmd/mesh-controller/grants_step_store_test.go index 7f675c07..3248a1b4 100644 --- a/cmd/mesh-controller/grants_step_store_test.go +++ b/cmd/mesh-controller/grants_step_store_test.go @@ -105,7 +105,10 @@ func TestTheGrantsStepSendsTheUserListAndNothingElse(t *testing.T) { t.Fatal(err) } } - kept := keepingEveryBuild(keepingRecorded(ctx)) + // Kept for anchor alone. laptop is never recorded as sent, on purpose: composing anchor resolves laptop + // too (who is on the private network), and a step that kept every machine's builds refused anchor's + // composition for want of laptop's last send (repo-check of #230 at 0f853e93). + kept := keepingEveryBuild(keepingRecorded(ctx), "anchor") // What anchor was last sent: everything at the builds of before the merge, as an ordinary send sends it. _, before := compose(keepingRecorded(ctx)) diff --git a/cmd/mesh-controller/grants_step_test.go b/cmd/mesh-controller/grants_step_test.go index 2820bb51..48b34bb5 100644 --- a/cmd/mesh-controller/grants_step_test.go +++ b/cmd/mesh-controller/grants_step_test.go @@ -43,7 +43,7 @@ func sendsRecorded(t *testing.T, holder string, behind bool, refuse error) *[]aS return holder, behind, nil } sendRollout = func(ctx context.Context, _ *stores, names []string) ([]string, error) { - s := aSend{names: append([]string(nil), names...), keepsAll: everyBuildKept(ctx)} + s := aSend{names: append([]string(nil), names...), keepsAll: len(names) == 1 && everyBuildKept(ctx, names[0])} for n := range scopeOf(ctx).judged { s.judged = append(s.judged, n) } @@ -168,6 +168,15 @@ func TestOnlyTheUserListsContentIsExempt(t *testing.T) { } } +// The step keeps the builds of the machine it sends alone: the others its composition resolves are composed +// as any send composes them. +func TestTheGrantsStepKeepsOneMachinesBuilds(t *testing.T) { + ctx := keepingEveryBuild(t.Context(), "novox") + if !everyBuildKept(ctx, "novox") || everyBuildKept(ctx, "ace") || everyBuildKept(t.Context(), "novox") { + t.Error("the grants step keeps the builds of a machine it does not send") + } +} + // The delivery plan says the step before the merge. func TestAChangePlanSaysTheGrantsStep(t *testing.T) { const catalogue_ = "http://forge.internal:20000/novox/mesh-catalog.git" diff --git a/cmd/mesh-controller/push.go b/cmd/mesh-controller/push.go index 30e3fd84..2e134b15 100644 --- a/cmd/mesh-controller/push.go +++ b/cmd/mesh-controller/push.go @@ -1238,7 +1238,7 @@ func sendToEach(ctx context.Context, open *stores, names []string) ([]string, er numbered.stamp(&declared) // **The grants step sends the user list and nothing else** (novox/hq issue 490): judged on this very // declaration, the one sent, so nothing composed between a check and the send can slip through. - if everyBuildKept(ctx) { + if everyBuildKept(ctx, name) { other, err := grantsOnlyIn(ctx, open, name, plan, declared) if err != nil { return nil, err diff --git a/cmd/mesh-controller/recorded_kept.go b/cmd/mesh-controller/recorded_kept.go index 5c81bec8..a8e8cf5b 100644 --- a/cmd/mesh-controller/recorded_kept.go +++ b/cmd/mesh-controller/recorded_kept.go @@ -63,19 +63,24 @@ func keptExcept(ctx context.Context) (map[string]bool, bool) { type keepEveryBuildKey struct{} -// keepingEveryBuild is a context whose sends compose every module — recorded or rolling out — at the build -// its machine was last sent (novox/hq issue 490): the grants step, which sends the machine holding the bus -// its new user list and nothing of any module's new code. That code waits there for a gate like any other -// move; the list must not, or the first machine's gate is judged against a bus that refuses what the change -// newly grants. -func keepingEveryBuild(ctx context.Context) context.Context { - return context.WithValue(ctx, keepEveryBuildKey{}, true) +// keepingEveryBuild is a context whose sends compose every module of one machine — recorded or rolling out — +// at the build that machine was last sent (novox/hq issue 490): the grants step, which sends the machine +// holding the bus its new user list and nothing of any module's new code. That code waits there for a gate +// like any other move; the list must not, or the first machine's gate is judged against a bus that refuses +// what the change newly grants. +// +// **That machine alone.** Composing one machine resolves the others too — who is on the private network, +// who answers a requirement — on the same context, and those are no part of the step's send: they are +// composed as any send composes them. Kept for every machine, a machine whose last send is not known +// refused the bus's machine's composition. +func keepingEveryBuild(ctx context.Context, node string) context.Context { + return context.WithValue(ctx, keepEveryBuildKey{}, node) } -// everyBuildKept is whether this context keeps every module at the build its machine runs. -func everyBuildKept(ctx context.Context) bool { - on, _ := ctx.Value(keepEveryBuildKey{}).(bool) - return on +// everyBuildKept is whether this context keeps every module of this machine at the build it runs. +func everyBuildKept(ctx context.Context, node string) bool { + on, _ := ctx.Value(keepEveryBuildKey{}).(string) + return on != "" && on == node } // recordedKept is, for a send under keepingRecorded, every recorded module the machine was last sent a @@ -87,7 +92,7 @@ func everyBuildKept(ctx context.Context) bool { // 490), and a machine whose last send is not known refuses the send: there is nothing to keep it at, and // composing the mesh's builds would be the very move the grants step must not make. func recordedKept(ctx context.Context, open *stores, node string) (map[string]string, error) { - every := everyBuildKept(ctx) + every := everyBuildKept(ctx, node) skip, on := keptExcept(ctx) if !on && !every { return nil, nil @@ -150,7 +155,7 @@ func keepRecorded(ctx context.Context, open *stores, node string, shelf map[stri if err != nil { return nil, err } - if !found && everyBuildKept(ctx) { + if !found && everyBuildKept(ctx, node) { return nil, fmt.Errorf("%s runs %s's build %s, which the build records no longer hold: the bus's user "+ "list cannot be sent there alone with it kept (novox/hq issue 490)", node, m, short(kept[m])) } diff --git a/cmd/mesh-controller/release.go b/cmd/mesh-controller/release.go index 8be20f46..555ebeb2 100644 --- a/cmd/mesh-controller/release.go +++ b/cmd/mesh-controller/release.go @@ -257,7 +257,7 @@ func ungatedIn(ctx context.Context, open *stores, names []string, addedHolder st // The grants step composes every module at the build its machine was last sent (novox/hq issue 490): // a move it keeps is not made. Read, not assumed, so a move it could not keep is still refused here. var keeps map[string]string - if everyBuildKept(ctx) { + if everyBuildKept(ctx, n) { if keeps, err = recordedKept(ctx, open, n); err != nil { return nil, err } @@ -968,7 +968,7 @@ func grantsStep(ctx context.Context, open *stores, node string) *inventory.Grant // list is the one already sent. return nil } - if _, err := sendRollout(keepingEveryBuild(withScope(ctx, sendScope{})), open, []string{holder}); err != nil { + if _, err := sendRollout(keepingEveryBuild(withScope(ctx, sendScope{}), holder), open, []string{holder}); err != nil { fmt.Printf("grants step: the bus's user list could not be sent to %s ahead of %s, which is sent without "+ "it — the bus may refuse what the send newly grants: %v\n", holder, node, err) return &inventory.GrantsStep{Node: holder, Failed: err.Error()} From eb8233ac7ca8c953b81a6deea71ef203fe8c182a Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 19:43:50 +0200 Subject: [PATCH 8/8] Give the grants step's restic fixture a check beside its tool, as the module rules require (repo-check of #230) A tool check alone is refused, so the store test's c2 never registered; the fixtures are now parsed by a test that needs no store. --- cmd/mesh-controller/grants_step_store_test.go | 53 ++++++++++++------- cmd/mesh-controller/grants_step_test.go | 16 ++++++ 2 files changed, 50 insertions(+), 19 deletions(-) diff --git a/cmd/mesh-controller/grants_step_store_test.go b/cmd/mesh-controller/grants_step_store_test.go index 3248a1b4..095a84f1 100644 --- a/cmd/mesh-controller/grants_step_store_test.go +++ b/cmd/mesh-controller/grants_step_store_test.go @@ -26,7 +26,6 @@ func TestTheGrantsStepSendsTheUserListAndNothingElse(t *testing.T) { ctx := t.Context() inv := open.inventory start := time.Now().Add(-time.Hour) - image := "registry.invalid:5000/restic/backup@sha256:" + strings.Repeat("b", 64) build := func(module, commit string, at time.Time, manifest map[string]any) link.BuildResult { manifest["module"], manifest["version"] = module, "1" raw, _ := json.Marshal(manifest) @@ -34,24 +33,10 @@ func TestTheGrantsStepSendsTheUserListAndNothingElse(t *testing.T) { On: "anchor", Module: module, Commit: commit, Manifest: raw, Source: &link.SourceOnSeat{Seat: "git", Repository: "novox/mesh-catalog"}} } - restic := func(health map[string]any, tools ...string) map[string]any { - r := map[string]any{"id": "backup", "type": "container", "name": "restic-backup", "image": image} - if health != nil { - r["health"] = health - } - m := map[string]any{"resources": []any{r}} - if len(tools) > 0 { - m["tools"] = tools - } - return m - } - nats := map[string]any{"bus-users": "/var/lib/nats-module/conf/accounts.conf", - "claims": []any{map[string]any{"name": catalogue.BrokerSeat, "scope": catalogue.ScopeMesh}}} for _, b := range []link.BuildResult{ - build("nats", "n1", start, nats), - build("restic", "c1", start.Add(time.Second), restic(nil)), - build("wallpaper", "w1", start.Add(2*time.Second), map[string]any{"resources": []any{ - map[string]any{"id": "paper", "type": "container", "name": "wallpaper", "image": image}}}), + build("nats", "n1", start, natsFixture()), + build("restic", "c1", start.Add(time.Second), resticFixture(false)), + build("wallpaper", "w1", start.Add(2*time.Second), wallpaperFixture()), } { if _, _, err := takeIn(ctx, inv, asTheOperator(t, inv, b)); err != nil { t.Fatal(err) @@ -120,7 +105,7 @@ func TestTheGrantsStepSendsTheUserListAndNothingElse(t *testing.T) { // The merge: restic's c2 gives it a health tool. if _, _, err := takeIn(ctx, inv, build("restic", "c2", start.Add(time.Minute), - restic(map[string]any{"kind": catalogue.HealthTool, "tool": "backup_health"}, "backup_health"))); err != nil { + resticFixture(true))); err != nil { t.Fatal(err) } @@ -181,3 +166,33 @@ func TestTheGrantsStepSendsTheUserListAndNothingElse(t *testing.T) { t.Fatalf("the grants step's send of a new assignment was not refused by the send: %v", err) } } + +// fixtureImage is the image every fixture container of the grants step's tests runs. +var fixtureImage = "registry.invalid:5000/restic/backup@sha256:" + strings.Repeat("b", 64) + +// resticFixture is restic's manifest in the shape of mesh-catalog #210: at c2 (withTool) its backup judged by +// its new tool `backup_health`, beside a check of another kind it does not run itself — a tool check alone +// is refused (ADR 0227 rule 8, ADR 0240 rule 2) — and at c1 neither. +func resticFixture(withTool bool) map[string]any { + backup := map[string]any{"id": "backup", "type": "container", "name": "restic-backup", "image": fixtureImage} + measure := map[string]any{"id": "measure", "type": "container", "name": "restic-measure", "image": fixtureImage} + m := map[string]any{"resources": []any{backup, measure}} + if withTool { + backup["health"] = map[string]any{"kind": catalogue.HealthTool, "tool": "backup_health"} + measure["health"] = map[string]any{"kind": "runtime"} + m["tools"] = []string{"backup_health"} + } + return m +} + +// natsFixture is the bus's module: it holds mesh-broker and is sent the user list. +func natsFixture() map[string]any { + return map[string]any{"bus-users": "/var/lib/nats-module/conf/accounts.conf", + "claims": []any{map[string]any{"name": catalogue.BrokerSeat, "scope": catalogue.ScopeMesh}}} +} + +// wallpaperFixture is a module the bus's machine is newly assigned. +func wallpaperFixture() map[string]any { + return map[string]any{"resources": []any{ + map[string]any{"id": "paper", "type": "container", "name": "wallpaper", "image": fixtureImage}}} +} diff --git a/cmd/mesh-controller/grants_step_test.go b/cmd/mesh-controller/grants_step_test.go index 48b34bb5..70e275b2 100644 --- a/cmd/mesh-controller/grants_step_test.go +++ b/cmd/mesh-controller/grants_step_test.go @@ -177,6 +177,22 @@ func TestTheGrantsStepKeepsOneMachinesBuilds(t *testing.T) { } } +// The store test's manifests are manifests the controller takes in: checked here, where no store is needed, +// so a fixture the module rules refuse fails before the store test is run (repo-check of #230 at 325575b2). +func TestTheGrantsStepFixturesAreManifests(t *testing.T) { + for name, m := range map[string]map[string]any{"nats": natsFixture(), "restic-c1": resticFixture(false), + "restic-c2": resticFixture(true), "wallpaper": wallpaperFixture()} { + m["module"], m["version"] = strings.SplitN(name, "-", 2)[0], "1" + raw, err := json.Marshal(m) + if err != nil { + t.Fatal(err) + } + if _, err := catalogue.ParseManifest(raw); err != nil { + t.Errorf("%s: %v", name, err) + } + } +} + // The delivery plan says the step before the merge. func TestAChangePlanSaysTheGrantsStep(t *testing.T) { const catalogue_ = "http://forge.internal:20000/novox/mesh-catalog.git"