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

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

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

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)