Review: module issue's pre-check factored and tested; a pair delivery for a requirement the module keeps no secret for is refused; builder issue's usage says the module comes first

cmd/mesh-controller/main.go

@@ -168,7 +168,8 @@ func usage() {
   build <repository> [--ref R]   have a build machine build it, and record what came out
   build --behind                 build every module the mesh holds older than its source
   builds [<module>]              what has been built lately, and what came of it
-  builder issue <name>           a broker account for a build machine, scoped to build work
+  builder issue <name>           a broker account for a build machine, scoped to build work,
+                                 delivered as the builder module's broker secret (module add it first)
   licence add|list|use|key       model access, under the name a person calls it
   licence manager <name> <node>  the node that holds a refreshable licence's refresh token
   licence refresh <name>         mint a new access token and seal it to every holder

cmd/mesh-controller/modules.go

@@ -253,12 +253,8 @@ func moduleCommand(ctx context.Context, args []string) error {
 		if !ok {
 			return fmt.Errorf("this mesh knows no module %q; `module add` it first", module)
 		}
-		// The account is delivered as the module's own secret named broker; a module that declares
+		if err := mayIssue(m); err != nil {
-		// none has nothing to read it with, and an account nothing reads is an orphan on the bus
+			return err
-		// (novox/hq 04-ISSUES/078). Said before the account is made, not after.
-		if _, reads := m.OwnSecrets["broker"]; !reads {
-			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\": <path>}", module)
 		}
 
 		management, err := broker.ManagementFromEnvironment()
@@ -559,3 +555,15 @@ func brokerReachableAt(ctx context.Context, inv *inventory.Inventory, known brok
 	}
 	return net.JoinHostPort(brokerAt, port), nil
 }
+
+// mayIssue says whether a module can be given a broker account. The account is delivered as the
+// 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.
+func mayIssue(m catalogue.Manifest) error {
+	if _, reads := m.OwnSecrets["broker"]; !reads {
+		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\": <path>}", m.Module)
+	}
+	return nil
+}

cmd/mesh-controller/modules_test.go

@@ -0,0 +1,26 @@
+package main
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/novox/mesh-controller/internal/catalogue"
+)
+
+// `module issue` makes a broker account and delivers it as the module's own secret named broker.
+// A module that declares none is refused before the account exists, so the bus never carries an
+// account nothing reads (novox/hq 04-ISSUES/078).
+func TestAModuleWithNoBrokerSecretCannotBeIssued(t *testing.T) {
+	err := mayIssue(catalogue.Manifest{Module: "step-ca", OwnSecrets: map[string]string{"password": "/run/password"}})
+	if err == nil {
+		t.Fatal("a module with no broker own secret was issued an account")
+	}
+	for _, want := range []string{"step-ca", "broker", "own-secrets"} {
+		if !strings.Contains(err.Error(), want) {
+			t.Errorf("the refusal does not say %q: %v", want, err)
+		}
+	}
+	if err := mayIssue(catalogue.Manifest{Module: "redis", OwnSecrets: map[string]string{"broker": "/run/broker"}}); err != nil {
+		t.Errorf("a module declaring its broker secret was refused: %v", err)
+	}
+}

internal/inventory/secrets.go

@@ -159,6 +159,9 @@ func (i *Inventory) AcceptSecretForPair(ctx context.Context, name, consumer, con
 			return fmt.Errorf("%s does not keep %q for %q; it keeps: %s",
 				consumerModule, local, name, orNone(sortedNames(locals)))
 		}
+	} else if m.Secrets[name] == "" {
+		return fmt.Errorf("%s requires %q but keeps no secret for it, so a delivered value would sit unread; "+
+			"it keeps secrets for: %s", consumerModule, name, orNone(sortedNames(m.Secrets)))
 	} else if local != "" {
 		return fmt.Errorf("%s keeps one secret for %q, not several; drop --local", consumerModule, name)
 	}

internal/inventory/secrets_test.go

@@ -55,7 +55,8 @@ func twoNodesWithKeys(t *testing.T) (*Inventory, context.Context) {
 		// Each requires what a test delivers to it: a pair credential is refused for a
 		// requirement the module does not have (novox/hq 04-ISSUES/078).
 		if err := inv.RegisterModule(ctx, catalogue.Manifest{Module: m, Version: "1",
-			Requires: []string{"secret", "postgres-database"}}, Source{}); err != nil {
+			Requires: []string{"secret", "postgres-database", "object-store"},
+			Secrets:  map[string]string{"secret": "/run/secret", "postgres-database": "/run/pg"}}, Source{}); err != nil {
 			t.Fatal(err)
 		}
 	}
@@ -748,11 +749,23 @@ func TestADeliveredSecretIsRefusedUnderANameTheModuleDoesNotDeclare(t *testing.T
 
 func TestADeliveredPairCredentialIsRefusedForARequirementTheModuleDoesNotHave(t *testing.T) {
 	inv, ctx := twoNodesWithKeys(t)
-	// gitea requires secret and postgres-database (the fixture); not an object store.
+	// gitea requires secret, postgres-database and object-store (the fixture); it keeps a secret
-	err := inv.AcceptSecretForPair(ctx, "object-store", "consumer", "gitea", "provider", "", "hunter2")
+	// for the first two only, and requires no cache at all.
+	err := inv.AcceptSecretForPair(ctx, "redis-cache", "consumer", "gitea", "provider", "", "hunter2")
 	if err == nil || !strings.Contains(err.Error(), "postgres-database") {
 		t.Fatalf("a pair credential for a requirement the module does not have was not refused naming what it requires: %v", err)
 	}
+	err = inv.AcceptSecretForPair(ctx, "object-store", "consumer", "gitea", "provider", "", "hunter2")
+	if err == nil || !strings.Contains(err.Error(), "keeps no secret") {
+		t.Fatalf("a pair credential for a requirement the module keeps no secret for was not refused: %v", err)
+	}
+	err = inv.AcceptSecretForPair(ctx, "secret", "consumer", "gitea", "provider", "extra", "hunter2")
+	if err == nil || !strings.Contains(err.Error(), "drop --local") {
+		t.Fatalf("a local named where the module keeps one secret was not refused: %v", err)
+	}
+	if err := inv.AcceptSecretForPair(ctx, "secret", "consumer", "gitea", "provider", "", "hunter2"); err != nil {
+		t.Fatalf("a delivery for a requirement the module keeps one secret for was refused: %v", err)
+	}
 	// A module keeping several secrets for one requirement (ADR 0094) takes a delivery only
 	// under one of its locals.
 	m := catalogue.Manifest{Module: "mailu", Version: "1", Requires: []string{"secret"},