From 78205d7405555874c2be673604c819e39392b7ac Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 19:12:33 +0200 Subject: [PATCH] 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 }