diff --git a/cmd/mesh-controller/acts.go b/cmd/mesh-controller/acts.go index 1eab1bbc..354e83cc 100644 --- a/cmd/mesh-controller/acts.go +++ b/cmd/mesh-controller/acts.go @@ -292,6 +292,22 @@ func unassign(ctx context.Context, open *stores, node string, modules ...string) answer += "\n " + strings.TrimSpace(line) } } + // Its bus accounts on the node go with it once it no longer resolves there (a module another requires keeps running + // and keeps them), so an assignment again issues them afresh (issue 1945). The node's own provisioner account stays. + if after.err == nil { + for _, module := range modules { + if slices.Contains(after.modules, module) { + continue + } + forgot, err := open.inventory.ForgetBusAccountsOf(ctx, node, module, false) + switch { + case err != nil: + answer += fmt.Sprintf("\n its bus accounts on %s are not forgotten: %v", node, err) + case len(forgot) > 0: + answer += fmt.Sprintf("\n its bus accounts on %s forgotten: %s", node, strings.Join(forgot, ", ")) + } + } + } // What it leaves behind that is irreplaceable is kept and retired, never removed (novox/hq ADR 0233). for _, line := range keptOnUnassign(ctx, open.inventory, node, modules) { answer += "\n " + line diff --git a/cmd/mesh-controller/assignment_accounts.go b/cmd/mesh-controller/assignment_accounts.go index 1005718e..242853de 100644 --- a/cmd/mesh-controller/assignment_accounts.go +++ b/cmd/mesh-controller/assignment_accounts.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "maps" "slices" "strings" "sync" @@ -40,6 +41,9 @@ import ( type assignmentAccountSteps struct { // holds says this controller holds the lease: only its holder issues. holds func(ctx context.Context) bool + // forgetStale forgets every module's accounts on a node where the module no longer resolves, and answers a line for + // each (issue 1945: rows of an unassignment before this release, marked sealed by migration 0108). + forgetStale func(ctx context.Context) ([]string, error) // catalogue reads the catalogue once, for every assignment a look issues for. catalogue func(ctx context.Context) (map[string]catalogue.Manifest, error) // issue issues the module's accounts on the node that are not minted, from the catalogue read; issued says it @@ -86,6 +90,17 @@ func (s assignmentAccountSteps) settle(ctx context.Context, node, module string) // it issued (an assignment that came to stand while no controller heard it may have its accounts and still lack its // place in the bus's user list or its grant). It answers what it did. func (s assignmentAccountSteps) look(ctx context.Context, standing []chain.Assignment) (string, error) { + var forgot []string + if len(standing) > 0 && s.forgetStale != nil { + // Only from a reading of the assignments that holds some: an empty or failed one would forget every account. + if !s.holds(ctx) { + return "", errNotHolding + } + var err error + if forgot, err = s.forgetStale(ctx); err != nil { + return "", fmt.Errorf("the accounts of modules no longer on their nodes are not forgotten: %w", err) + } + } var looked []chain.Assignment for _, a := range standing { if a.Module != controllerModule { @@ -93,7 +108,7 @@ func (s assignmentAccountSteps) look(ctx context.Context, standing []chain.Assig } } if len(looked) == 0 { - return "", nil + return strings.Join(forgot, "\n"), nil } if !s.holds(ctx) { return "", errNotHolding @@ -114,7 +129,7 @@ func (s assignmentAccountSteps) look(ctx context.Context, standing []chain.Assig } } said, err := s.cover(ctx) - lines = append(lines, fmt.Sprintf("%d standing assignment(s) looked at; %s", len(looked), said)) + lines = append(append(forgot, lines...), fmt.Sprintf("%d standing assignment(s) looked at; %s", len(looked), said)) return strings.Join(lines, "\n"), err } @@ -247,7 +262,8 @@ func liveAssignmentAccountSteps(open *stores) assignmentAccountSteps { _, err := theLease.epoch(ctx) return err == nil }, - catalogue: func(ctx context.Context) (map[string]catalogue.Manifest, error) { return inv.Catalogue(ctx) }, + forgetStale: func(ctx context.Context) ([]string, error) { return forgetStaleAccounts(ctx, open) }, + catalogue: func(ctx context.Context) (map[string]catalogue.Manifest, error) { return inv.Catalogue(ctx) }, issue: func(ctx context.Context, shelf map[string]catalogue.Manifest, node, module string) (bool, string, error) { return issueAssignmentAccounts(ctx, inv, shelf, node, module) }, @@ -403,3 +419,71 @@ func watchAssignmentsOnce(ctx context.Context, watch func(context.Context) (<-ch } } } + +// staleAccounts is, by node, each module some account of which stands on a node where the module no longer resolves: +// the accounts of a module's own, its processes' and its provisioner client's. A node whose resolution is unknown is +// left alone. +func staleAccounts(users map[string]inventory.BusUser, resolved map[string]map[string]bool) map[string][]string { + seen := map[string]map[string]bool{} + for _, u := range users { + switch u.Kind { + case inventory.BusModule, inventory.BusNodeTools, inventory.BusProcess, inventory.BusProvisioner: + default: + continue + } + on, known := resolved[u.Node] + if u.Node == "" || u.Module == "" || !known || on[u.Module] { + continue + } + if seen[u.Node] == nil { + seen[u.Node] = map[string]bool{} + } + seen[u.Node][u.Module] = true + } + out := map[string][]string{} + for node, modules := range seen { + out[node] = slices.Sorted(maps.Keys(modules)) + } + return out +} + +// forgetStaleAccounts forgets the accounts of every module on a node where it no longer resolves (assigned or +// required), from each node's own resolution; a node that does not resolve is left alone. +func forgetStaleAccounts(ctx context.Context, open *stores) ([]string, error) { + inv := open.inventory + users, err := inv.BusUsers(ctx) + if err != nil { + return nil, err + } + ctx = withInputCache(ctx) // one reading of the mesh for every node resolved + resolved := map[string]map[string]bool{} + for _, u := range users { + if _, done := resolved[u.Node]; done || u.Node == "" { + continue + } + plan, _, err := resolveNode(ctx, open, u.Node) + if err != nil { + continue // unknown: nothing of this node is forgotten + } + on := map[string]bool{} + for _, m := range plan.Modules { + on[m.Module] = true + } + resolved[u.Node] = on + } + var lines []string + stale := staleAccounts(users, resolved) + for _, node := range slices.Sorted(maps.Keys(stale)) { + for _, module := range stale[node] { + forgot, err := inv.ForgetBusAccountsOf(ctx, node, module, true) + if err != nil { + return lines, err + } + if len(forgot) > 0 { + lines = append(lines, fmt.Sprintf("%s no longer runs on %s: its bus accounts forgotten: %s", module, node, + strings.Join(forgot, ", "))) + } + } + } + return lines, nil +} diff --git a/cmd/mesh-controller/assignment_accounts_test.go b/cmd/mesh-controller/assignment_accounts_test.go index b7f1d67b..0dd9cbc2 100644 --- a/cmd/mesh-controller/assignment_accounts_test.go +++ b/cmd/mesh-controller/assignment_accounts_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "reflect" "strings" "testing" "time" @@ -194,6 +195,7 @@ func TestSettlingAndTheStartsLook(t *testing.T) { wantComposed []string wantGrants bool wantErr string + stale []string // what forgetting the accounts of modules no longer on their nodes says }{ {name: "an account not minted", module: "mesh-build-node", issued: true, holder: "novox", wantIssues: 1, wantComposed: []string{"novox"}, wantGrants: true}, @@ -214,13 +216,21 @@ func TestSettlingAndTheStartsLook(t *testing.T) { look: []chain.Assignment{ace("mesh-build-node"), ace("mesh-build-go")}, holder: "novox", wantIssues: 2, wantComposed: []string{"novox"}, wantGrants: true}, {name: "the start's look at nothing does nothing", look: []chain.Assignment{ace(controllerModule)}, holder: "novox"}, + {name: "the start's look forgets the accounts of modules no longer on their nodes first", + look: []chain.Assignment{ace("mesh-build-node")}, holder: "novox", wantIssues: 1, wantComposed: []string{"novox"}, + wantGrants: true, stale: []string{"mesh-build-go no longer runs on ace: its bus accounts forgotten: ace.mesh-build-go"}}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { var composed []string issues, catalogues, grants := 0, 0, 0 + forgets := 0 steps := assignmentAccountSteps{ holds: func(context.Context) bool { return !c.notHolding }, + forgetStale: func(context.Context) ([]string, error) { + forgets++ + return c.stale, nil + }, catalogue: func(context.Context) (map[string]catalogue.Manifest, error) { catalogues++ return map[string]catalogue.Manifest{}, nil @@ -262,6 +272,14 @@ func TestSettlingAndTheStartsLook(t *testing.T) { if want := map[bool]int{true: 1}[c.wantGrants]; grants != want { t.Fatalf("grants written %d time(s), want %d", grants, want) } + if want := map[bool]int{true: 1}[len(c.look) > 0 && !c.notHolding]; forgets != want { + t.Fatalf("forgot stale accounts %d time(s), want %d (only from a reading that holds assignments)", forgets, want) + } + for _, line := range c.stale { + if !strings.Contains(said, line) { + t.Fatalf("said %q, want it to say %q", said, line) + } + } if c.wantGrants && c.wantErr == "" && !strings.Contains(said, "the grants written") { t.Fatalf("said %q, want it to name the grants", said) } @@ -367,6 +385,26 @@ func TestTheAccountsAModuleSpeaksWithOnANode(t *testing.T) { }) } + // The accounts of a module that no longer resolves on its node are stale, its provisioner client's among them; a + // module still resolved (assigned or required), an account of another kind, and a node whose resolution is unknown + // are left alone (issue 1945: ace's mesh-build-go rows from 17:19, marked sealed by migration 0108). + t.Run("stale accounts", func(t *testing.T) { + u := func(kind, node, module string) inventory.BusUser { + return inventory.BusUser{Username: node + "." + module + "." + kind, Kind: kind, Node: node, Module: module} + } + users := map[string]inventory.BusUser{} + for _, b := range []inventory.BusUser{u(inventory.BusModule, "ace", "mesh-build-go"), + u(inventory.BusProcess, "ace", "mesh-build-go"), u(inventory.BusProvisioner, "ace", "mesh-provisioner"), + u(inventory.BusModule, "ace", "mesh-build-node"), u(inventory.BusNode, "ace", ""), + u(inventory.BusModule, "g14", "mesh-build-go")} { + users[b.Username] = b + } + got := staleAccounts(users, map[string]map[string]bool{"ace": {"mesh-build-node": true}}) + if want := map[string][]string{"ace": {"mesh-build-go", "mesh-provisioner"}}; !reflect.DeepEqual(got, want) { + t.Fatalf("stale %v, want %v", got, want) + } + }) + t.Run("a process may be issued only with its own broker secret", func(t *testing.T) { // mesh-mail's real shape: no `broker` of its own, its process declaring bus (review round 1 of issue 1181). if err := mayIssue(mailSenderWithProcess()); err != nil { diff --git a/cmd/mesh-controller/grants.go b/cmd/mesh-controller/grants.go index 07759f39..4151aa0c 100644 --- a/cmd/mesh-controller/grants.go +++ b/cmd/mesh-controller/grants.go @@ -23,10 +23,13 @@ import ( // writes `.` then, naming the accounts it composed for the module, never removed. A module one // of whose accounts has no password yet (the list leaves it out) has no grant until a later send carries it. -// grantsFrom is the grant of every module whose accounts all have a password in hashes, at the fingerprint current names -// for it; a module whose current build names no fingerprint of the new form has none. principals are the bus's users -// as broker.Users composes them. -func grantsFrom(principals []broker.Principal, hashes map[string]string, current map[string]string) map[string]sdkdelivery.ControllerGrant { +// grantsFrom is the grant of every module at the fingerprint current names for it, naming its accounts that have a +// sealed password in hashes; a module whose current build names no fingerprint of the new form has none. A module one +// of whose accounts has none is granted its sealed accounts alone, and when it has none, its grant is dropped (drop): +// an older grant naming a node whose account is gone would let the assigner want it (issue 1945, rule 6 per node), and +// an empty grant names every node. principals are the bus's users as broker.Users composes them. +func grantsFrom(principals []broker.Principal, hashes map[string]string, current map[string]string) ( + map[string]sdkdelivery.ControllerGrant, []string) { accounts, missing := map[string][]string{}, map[string]bool{} for _, p := range principals { if p.Module == "" || (p.Kind != broker.KindModule && p.Kind != broker.KindProcess) { @@ -39,8 +42,9 @@ func grantsFrom(principals []broker.Principal, hashes map[string]string, current accounts[p.Module] = append(accounts[p.Module], p.Username()) } out := map[string]sdkdelivery.ControllerGrant{} + var drop []string for module, fp := range current { - if missing[module] || !strings.HasPrefix(fp, sdkdelivery.SourcePrefix2) { + if !strings.HasPrefix(fp, sdkdelivery.SourcePrefix2) { continue } key, err := sdkdelivery.ControllerGrantKey(module, fp) @@ -48,10 +52,15 @@ func grantsFrom(principals []broker.Principal, hashes map[string]string, current continue } names := accounts[module] + if missing[module] && len(names) == 0 { + drop = append(drop, key) + continue + } sort.Strings(names) out[key] = sdkdelivery.ControllerGrant{Accounts: names} } - return out + sort.Strings(drop) + return out, drop } // currentFingerprints is, by module, the fingerprint of the build each registered module is registered at: its @@ -258,7 +267,17 @@ func grantsIssued(ctx context.Context, inv *inventory.Inventory, js jetstream.Je if err != nil { return fmt.Errorf("the grants issued are not written: %w", err) } - wrote, err := keepGrants(ctx, kv, grantsFrom(principals, hashes, current)) + grants, drop := grantsFrom(principals, hashes, current) + wrote, err := keepGrants(ctx, kv, grants) + for _, key := range drop { + if _, gerr := kv.Get(ctx, key); gerr == nil { + if derr := kv.Delete(ctx, key); derr != nil { + err = errors.Join(err, fmt.Errorf("%s: %w", key, derr)) + } else { + fmt.Printf("%sthe grant %s dropped: none of its accounts is sealed to its node\n", indent, key) + } + } + } if wrote > 0 { fmt.Printf("%s%d grant(s) issued written for the assigner\n", indent, wrote) } diff --git a/cmd/mesh-controller/grants_test.go b/cmd/mesh-controller/grants_test.go index b14715ef..9bbe0395 100644 --- a/cmd/mesh-controller/grants_test.go +++ b/cmd/mesh-controller/grants_test.go @@ -23,12 +23,25 @@ func TestTheGrantsAListIssues(t *testing.T) { y := broker.Principal{Kind: broker.KindModule, Node: "ace", Module: "mesh-y"} node := broker.Principal{Kind: broker.KindNode, Node: "novox"} hashes := map[string]string{x.Username(): "h1", xp.Username(): "h2", node.Username(): "h3"} - got := grantsFrom([]broker.Principal{x, xp, y, node}, hashes, - map[string]string{"mesh-x": grantFP('a'), "mesh-y": grantFP('b'), "web": grantFP('c'), "old": "src1:abc"}) + // mesh-z is sealed on novox and not on ace (issue 1945: an older grant must not name ace): its grant names novox alone. + zn := broker.Principal{Kind: broker.KindModule, Node: "novox", Module: "mesh-z"} + za := broker.Principal{Kind: broker.KindModule, Node: "ace", Module: "mesh-z"} + hashes[zn.Username()] = "h4" + got, drop := grantsFrom([]broker.Principal{x, xp, y, node, zn, za}, hashes, + map[string]string{"mesh-x": grantFP('a'), "mesh-y": grantFP('b'), "web": grantFP('c'), "old": "src1:abc", + "mesh-z": grantFP('d')}) xKey, _ := sdkdelivery.ControllerGrantKey("mesh-x", grantFP('a')) + yKey, _ := sdkdelivery.ControllerGrantKey("mesh-y", grantFP('b')) + zKey, _ := sdkdelivery.ControllerGrantKey("mesh-z", grantFP('d')) webKey, _ := sdkdelivery.ControllerGrantKey("web", grantFP('c')) - if len(got) != 2 { - t.Fatalf("grants = %v, want mesh-x's and web's", got) + if len(got) != 3 { + t.Fatalf("grants = %v, want mesh-x's, mesh-z's and web's", got) + } + if g := got[zKey]; len(g.Accounts) != 1 || g.Accounts[0] != zn.Username() { + t.Errorf("mesh-z's grant = %+v, want its sealed account alone", g) + } + if len(drop) != 1 || drop[0] != yKey { + t.Errorf("dropped %v, want mesh-y's grant, none of whose accounts is sealed", drop) } if g := got[xKey]; len(g.Accounts) != 2 || g.Accounts[0] != x.Username() && g.Accounts[1] != x.Username() { t.Errorf("mesh-x's grant = %+v, want both its accounts", g) diff --git a/cmd/mesh-controller/modules.go b/cmd/mesh-controller/modules.go index 9e9f175c..1b2a5140 100644 --- a/cmd/mesh-controller/modules.go +++ b/cmd/mesh-controller/modules.go @@ -1008,6 +1008,10 @@ type accountIssuer interface { MarkBusAccountSealed(ctx context.Context, username string) error } +// issueLockedFor is the longest an issue holds an account's lock: a seal cut by it leaves the account unsealed, issued +// again by the next issue. +const issueLockedFor = 2 * time.Minute + // issueAccount mints u's password and seals it to its node with seal. By default it replaces whatever stands (a // rotation: `module issue`). Under onlyIfAbsent it issues only an account that is not issued, a row with no sealed // credential counting as none (issue 1945, live on 2026-10-25 at 20:04: a row whose credential the node no longer held @@ -1025,6 +1029,10 @@ func issueAccount(ctx context.Context, accounts accountIssuer, u inventory.BusUs return err } } else { + // A deadline on the locked section: it holds a connection of the store's pool across the vault and the bus. + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, issueLockedFor) + defer cancel() release, err := accounts.LockBusAccount(ctx, u.Username) if err != nil { return err diff --git a/internal/inventory/bususers.go b/internal/inventory/bususers.go index f879e4c6..08374b35 100644 --- a/internal/inventory/bususers.go +++ b/internal/inventory/bususers.go @@ -141,30 +141,58 @@ func (i *Inventory) LockBusAccount(ctx context.Context, username string) (func() } // ForgetBusAccountsOf forgets the bus accounts of module on node, and the credentials sealed to the node for them (its -// `broker`, each process's `broker-`, its provisioner client's): an unassigned module keeps no account, so an -// assignment again mints and seals afresh, never trusting a row whose credential the node no longer holds (issue 1945, -// live on 2026-10-25 at 20:04: ace's mesh-build-go rows from 17:19 survived its unassignment, nothing was minted or -// sealed again, and the module found no credential). -func (i *Inventory) ForgetBusAccountsOf(ctx context.Context, node, module string) error { +// `broker`, each process's `broker-`, and with nodeOwned its provisioner client's): a module that no longer resolves +// on a node keeps no account there, so an assignment again mints and seals afresh, never trusting a row whose credential +// the node no longer holds (issue 1945, live on 2026-10-25 at 20:04: ace's mesh-build-go rows from 17:19 outlived its +// unassignment, nothing was issued again, and the module found no credential). The provisioner client's account is the +// node's (`provisioner.`), forgotten only with nodeOwned. Each account's lock is held across its delete, so an issue +// in flight never writes a row back after it. It answers the accounts forgotten. +func (i *Inventory) ForgetBusAccountsOf(ctx context.Context, node, module string, nodeOwned bool) ([]string, error) { if node == "" || module == "" { - return errors.New("forgetting the bus accounts of no node or no module would forget others' accounts") + return nil, errors.New("forgetting the bus accounts of no node or no module would forget others' accounts") + } + rows, err := i.store.Pool().Query(ctx, + `select username from bus_user where node = $1 and module = $2 and kind <> $3 and ($4 or kind <> $5) + order by username`, node, module, BusController, nodeOwned, BusProvisioner) + if err != nil { + return nil, err + } + var names []string + for rows.Next() { + var n string + if err := rows.Scan(&n); err != nil { + rows.Close() + return nil, err + } + names = append(names, n) + } + rows.Close() + if err := rows.Err(); err != nil { + return nil, err + } + for _, n := range names { // in order, one lock at a time per issuer: no two hold each other's + release, err := i.LockBusAccount(ctx, n) + if err != nil { + return nil, err + } + defer release() } tx, err := i.store.Pool().Begin(ctx) if err != nil { - return err + return nil, err } defer func() { _ = tx.Rollback(context.WithoutCancel(ctx)) }() - if _, err := tx.Exec(ctx, `delete from bus_user where node = $1 and module = $2 and kind <> $3`, - node, module, BusController); err != nil { - return fmt.Errorf("the bus accounts of %s on %s are not forgotten: %w", module, node, err) + if _, err := tx.Exec(ctx, `delete from bus_user where username = any($1)`, names); err != nil { + return nil, fmt.Errorf("the bus accounts of %s on %s are not forgotten: %w", module, node, err) } if _, err := tx.Exec(ctx, `delete from module_secret s using node n where s.node = n.id and n.name = $1 and s.module = $2 - and (s.name = 'broker' or s.name like 'broker-%' or s.name = 'provisioner-broker')`, node, module); err != nil { - return fmt.Errorf("the bus credentials of %s on %s are not forgotten: %w", module, node, err) + and (s.name = 'broker' or s.name like 'broker-%' or ($3 and s.name = 'provisioner-broker'))`, + node, module, nodeOwned); err != nil { + return nil, fmt.Errorf("the bus credentials of %s on %s are not forgotten: %w", module, node, err) } - return tx.Commit(ctx) + return names, tx.Commit(ctx) } // MintBusPasswordUnsealed mints u's password, replacing whatever stands, with no seal yet: the account is not issued diff --git a/internal/inventory/bususers_integration_test.go b/internal/inventory/bususers_integration_test.go index 229978a0..8b185f80 100644 --- a/internal/inventory/bususers_integration_test.go +++ b/internal/inventory/bususers_integration_test.go @@ -105,7 +105,7 @@ func TestMintingAgainRotatesAndMintingIfAbsentHasOneWinner(t *testing.T) { } release() <-waiting - if err := inv.ForgetBusAccountsOf(ctx, "one", "racer"); err != nil { + if _, err := inv.ForgetBusAccountsOf(ctx, "one", "racer", true); err != nil { t.Fatal(err) } if minted, _, _ := inv.BusAccountIssued(ctx, "one.racer"); minted { diff --git a/internal/inventory/catalogue.go b/internal/inventory/catalogue.go index c7677741..cebcd655 100644 --- a/internal/inventory/catalogue.go +++ b/internal/inventory/catalogue.go @@ -670,10 +670,6 @@ func (i *Inventory) Unassign(ctx context.Context, nodeName, module string) error if err := i.chainUnassign(ctx, c, node, module); err != nil { return err } - // Its bus accounts on the node go with it: an assignment again issues them afresh (issue 1945). - if err := i.ForgetBusAccountsOf(ctx, nodeName, module); err != nil { - return err - } // The seat it held falls back to derivation rather than pointing at nothing: what the store's foreign key did. if _, err := i.store.Pool().Exec(ctx, `delete from seat_holding where node = $1 and module = $2`, node.ID, module); err != nil { return err