diff --git a/cmd/mesh-controller/acts.go b/cmd/mesh-controller/acts.go index 54703d8e..4f02544c 100644 --- a/cmd/mesh-controller/acts.go +++ b/cmd/mesh-controller/acts.go @@ -391,10 +391,10 @@ func blockedElsewhere(ctx context.Context, open *stores, except string) string { } // issueOnAssign gives a newly assigned module its bus credential, the way `module issue` does, and -// says what it did in one line. Nothing for a module that declares no broker secret; nothing for one -// whose user is already minted (a credential is rotated on purpose, never by re-assigning); and when -// the bus cannot be reached from here, the line names the verb and the push that would refuse the -// module until it is run — never a silent placeholder (novox/hq issue 203). +// says what it did in one line. Nothing for a module that declares no broker secret and runs no +// provisioner client; nothing for one whose user is already minted (a credential is rotated on +// purpose, never by re-assigning); and when the bus cannot be reached from here, the line names the +// verb and the push that would refuse the module until it is run — never a silent placeholder (novox/hq issue 203). func issueOnAssign(ctx context.Context, open *stores, node, module string) string { inv := open.inventory shelf, err := inv.Catalogue(ctx) @@ -406,6 +406,10 @@ func issueOnAssign(ctx context.Context, open *stores, node, module string) strin return "" } user := broker.Principal{Kind: broker.KindModule, Node: node, Module: module}.Username() + if _, reads := m.OwnSecrets["broker"]; !reads { + // A module running the provisioner client alone is minted the client's account (issue 917). + user = broker.Principal{Kind: broker.KindProvisioner, Node: node, Module: module}.Username() + } if _, minted, err := inv.BusUserHash(ctx, user); err != nil || minted { return "" } diff --git a/cmd/mesh-controller/modules.go b/cmd/mesh-controller/modules.go index 80c58cab..754bc044 100644 --- a/cmd/mesh-controller/modules.go +++ b/cmd/mesh-controller/modules.go @@ -923,8 +923,10 @@ func brokerReachableAt(ctx context.Context, inv *inventory.Inventory, known brok // module's own secret named broker; a module that declares none has nothing to read it with, and // an account nothing reads is an orphan on the bus (novox/hq 04-ISSUES/078). Said before the // account is made, not after. +// A module that runs its machine's provisioner client and declares no broker is issued the client's account +// alone (issue 917): the client speaks on the bus only as itself. func mayIssue(m catalogue.Manifest) error { - if _, reads := m.OwnSecrets["broker"]; !reads { + if _, reads := m.OwnSecrets["broker"]; !reads && !runsTheProvisionerClient(m) { return fmt.Errorf("%s declares no own secret named broker, so there is nowhere to deliver "+ "an account; a module that speaks on the bus declares \"own-secrets\": {\"broker\": }", m.Module) } @@ -943,14 +945,6 @@ func mayIssue(m catalogue.Manifest) error { func issueOnTheNewBus(ctx context.Context, inv *inventory.Inventory, m catalogue.Manifest, node, busAddress string) error { - user := broker.Principal{Kind: broker.KindModule, Node: node, Module: m.Module}.Username() - password, err := inv.MintBusPassword(ctx, inventory.BusUser{ - Username: user, Kind: busKindOf(m.Module), Node: node, Module: m.Module, - }) - if err != nil { - return err - } - // Where the module is told to find the bus, and what certificate it must present. The same pair // a node is told, for the same reason: a mesh's bus presents its own certificate, in no public // trust store, so an address alone fails at TLS. @@ -962,8 +956,17 @@ func issueOnTheNewBus(ctx context.Context, inv *inventory.Inventory, m catalogue if err != nil { return err } - if err := issueWith(ctx, inv, m, node, busAddress, known, reachable, user, password); err != nil { - return err + if _, reads := m.OwnSecrets["broker"]; reads { + user := broker.Principal{Kind: broker.KindModule, Node: node, Module: m.Module}.Username() + password, err := inv.MintBusPassword(ctx, inventory.BusUser{ + Username: user, Kind: busKindOf(m.Module), Node: node, Module: m.Module, + }) + if err != nil { + return err + } + if err := issueWith(ctx, inv, m, node, busAddress, known, reachable, user, password); err != nil { + return err + } } if !runsTheProvisionerClient(m) { return nil diff --git a/cmd/mesh-controller/provisioner_account_test.go b/cmd/mesh-controller/provisioner_account_test.go index bf537bff..20dc7b15 100644 --- a/cmd/mesh-controller/provisioner_account_test.go +++ b/cmd/mesh-controller/provisioner_account_test.go @@ -2,6 +2,7 @@ package main import ( "slices" + "strings" "testing" "github.com/novox/mesh-controller/internal/broker" @@ -60,3 +61,32 @@ func TestTheProvisionerClientsAccountIsIssuedForItsServiceAndNeverInvented(t *te t.Fatal("a module with nowhere to read the client's account is read as running it") } } + +// A module that runs the client and declares no broker of its own, as the catalogue's mesh-provisioner-client does, +// may be issued: the client's account alone (issue 917). Before, `module issue` refused it for having no broker, +// while a push refused it for having no account, so no machine could run the client. +func TestAClientWithoutABrokerOfItsOwnMayBeIssued(t *testing.T) { + if err := mayIssue(aClientModule()); err != nil { + t.Fatalf("the client alone is refused an account: %v", err) + } + neither := aBot("bot", false) + if err := mayIssue(neither); err == nil { + t.Fatal("a module with nowhere to read any account may be issued one") + } +} + +// Assigning the client tries to issue the client's account, as assigning any module issues its broker (issue 917): +// without a bus to issue on, the line says so and names the verb, never nothing. +func TestAssigningTheClientIssuesItsAccount(t *testing.T) { + open := aMesh(t) + ctx := t.Context() + client := aClientModule() + register(t, open, client) + out, err := assign(ctx, open, "laptop", client.Module) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(out, "bus credential") { + t.Fatalf("assigning the client said nothing of its account:\n%s", out) + } +} diff --git a/internal/catalogue/delivery_seat.go b/internal/catalogue/delivery_seat.go index 682723e1..19a2d51b 100644 --- a/internal/catalogue/delivery_seat.go +++ b/internal/catalogue/delivery_seat.go @@ -9,7 +9,7 @@ package catalogue // DeliverySeat is the seat mesh-delivery holds. const DeliverySeat = "mesh-delivery" -// deliveryVerbs are the delivery seat's tools: six that read, and the acts of a person and of healer H2. +// deliveryVerbs are the delivery seat's tools: six that read, and the acts of a person, a reviewer and healer H2. func deliveryVerbs() []Verb { return []Verb{ {Name: "deliveries", Description: "Every delivery not final, and those that ended in the last day, one line " + @@ -84,6 +84,15 @@ func deliveryVerbs() []Verb { "dry": "\"true\": what it would retire, changing nothing"}, []string{"why"}, "dry"), // Added after the seat's holder shipped (novox/hq issue 313): optional until mesh-delivery serves it. Optional: true}, + {Name: "mark-ready", Description: "A reviewer's word that a delivery's head is approved: the holder puts the " + + "label mesh/ready on its pull request and then writes \"ready at \", the order the forge counts, " + + "as the mesh's own forge account, naming who reviewed it and why. Refused when the delivery's commit is " + + "no longer its pull request's head. mesh-delivery then merges it once its checks pass (novox/hq ADR 0364).", + Input: schema(map[string]string{"id": "the delivery's id (owner/repository@commit): the head reviewed", + "reviewed-by": "who reviewed it, as who@node", "why": "the review's verdict in a line"}, + []string{"id", "reviewed-by", "why"}), + // Added after the seat's holder shipped (novox/hq issue 913): optional until mesh-delivery serves it. + Optional: true}, } } diff --git a/internal/catalogue/delivery_seat_test.go b/internal/catalogue/delivery_seat_test.go index 5fe8daf2..783ec06e 100644 --- a/internal/catalogue/delivery_seat_test.go +++ b/internal/catalogue/delivery_seat_test.go @@ -133,3 +133,32 @@ func TestTheDeliverySeatPromisesTimesOptionally(t *testing.T) { t.Fatalf("a holder serving times: %v", err) } } + +// The seat marks a reviewed head ready (novox/hq issue 913): `mark-ready`, with the delivery's id, who reviewed it +// and why, so a reviewer marks a pull request through one audited verb, label and comment in the order the forge +// counts, instead of two raw forge calls. Added after the holder shipped, it is optional. +func TestTheDeliverySeatPromisesMarkReadyOptionally(t *testing.T) { + seat, _ := SeatNamed(DeliverySeat) + var mark *Verb + for i := range seat.Serves { + if seat.Serves[i].Name == "mark-ready" { + mark = &seat.Serves[i] + } + } + if mark == nil || !mark.Optional { + t.Fatalf("the delivery seat promises %v, mark-ready optionally", VerbNames(seat.Serves)) + } + if req, _ := mark.Input["required"].([]string); !reflect.DeepEqual(req, []string{"id", "reviewed-by", "why"}) { + t.Errorf("mark-ready requires %v, wanted id, reviewed-by and why", req) + } + var without []string + for _, v := range VerbNames(seat.Serves) { + if v != "mark-ready" { + without = append(without, v) + } + } + m := Manifest{Module: "mesh-delivery", Claims: []Claim{{Name: DeliverySeat, Scope: ScopeMesh, Serves: without}}} + if err := CanHold(m, seat); err != nil { + t.Fatalf("a holder without mark-ready yet: %v", err) + } +} diff --git a/testdata/store-tests b/testdata/store-tests index a0413ae1..9980948a 100644 --- a/testdata/store-tests +++ b/testdata/store-tests @@ -126,3 +126,4 @@ ./cmd/mesh-controller TestTheRecheckVerbAsksMeshDeliveryOnABusOfItsOwn ./cmd/mesh-controller TestAGroupsCompositionIntoAReleaseBranchIsJudgedByItsOwnController ./cmd/mesh-controller TestAGroupsCompositionJudgedWithTheValidatorIntoAReleaseBranchIsJudgedByItsOwnController +./cmd/mesh-controller TestAssigningTheClientIssuesItsAccount