diff --git a/cmd/mesh-controller/plan.go b/cmd/mesh-controller/plan.go index 017f5b8..8877ccf 100644 --- a/cmd/mesh-controller/plan.go +++ b/cmd/mesh-controller/plan.go @@ -1160,13 +1160,19 @@ func portsOn( func composeBusUsers(ctx context.Context, inv *inventory.Inventory, onThisNode []catalogue.Manifest) (string, error) { - _, onNATS, err := broker.OnNATS() - if err != nil || !onNATS { - return "", err - } - // Only for the machine holding the bus. Asked of what this push resolves to rather than of the - // seat's holder mesh-wide: the file is a resource of that module, so the question is whether it - // is here. + // **Not gated on which bus the controller is on, and that was a bug.** It read "compose this only + // once the mesh is on the new bus" — which cannot work, because the server needs its user list + // *before* anything moves onto it. Step 2 of the change is exactly that: the server stands in the + // mesh carrying nothing, on its own ports, while every node is still on the old bus (novox/hq + // ADR 0116). Under the old gating that step could not happen: the module would come up, find no + // accounts file, and its entrypoint would wait for one the controller had decided not to write. + // + // So the question is only whether this machine runs the module that asked for the file. A mesh + // that never moves has written a user list nothing reads, which costs a few hundred bytes on one + // node; the reverse cost a step that cannot be taken. + // + // Asked of what this push resolves to rather than of the seat's holder mesh-wide: the file is a + // resource of that module, so the question is whether it is here. holdsTheBus := false for _, m := range onThisNode { if m.BusUsers != "" && m.ClaimsSeat("mesh-broker") { diff --git a/internal/broker/users_test.go b/internal/broker/users_test.go index 8f40e1b..0e21eaf 100644 --- a/internal/broker/users_test.go +++ b/internal/broker/users_test.go @@ -201,3 +201,41 @@ func TestTheAccountsFileRefusesAUserWithNoPassword(t *testing.T) { t.Fatal("a user with no password hash was written") } } + +// **A user list is composed for a bus the mesh has not moved onto yet**, and that is the whole of +// step 2 (novox/hq ADR 0116): the server stands in the mesh carrying nothing, on its own ports, while +// every node is still on the bus it was on. +// +// Pinned because the first version of the composing step got it backwards — it wrote the list only +// once the controller was already on the new bus, which is a step that cannot be taken: the module +// comes up, finds no accounts file, and waits for one the controller had decided not to write. +func TestAUserListIsComposedBeforeAnythingMovesOntoTheBus(t *testing.T) { + // Exactly the records of a mesh mid-change: everything running, nothing on the new bus. + users, err := Users(Records{ + Nodes: []string{"anchor"}, + Assigned: map[string][]Declared{"anchor": {{Module: "nats"}}}, + }) + if err != nil { + t.Fatal(err) + } + hashes := map[string]string{} + for _, u := range users { + hashes[u.Username()] = "$2a$11$" + strings.Repeat("x", 22) + } + filled, missing := WithPasswords(users, hashes) + if len(missing) != 0 { + t.Fatalf("users with no credential: %v", missing) + } + accounts, err := ComposeAccounts(filled) + if err != nil { + t.Fatal(err) + } + // The controller's own user above all: a file without it is a bus its writer cannot connect to, + // which is what the server would be left holding the moment it starts. + if !strings.Contains(accounts, `user: "controller"`) { + t.Fatalf("the composed list does not contain the controller:\n%s", accounts) + } + if !strings.Contains(accounts, `user: "node.anchor"`) { + t.Errorf("the composed list does not contain the machine running the bus") + } +}