Merge pull request 'Multiple fixes: a run-once step may name what it reads (ADR 0099); secret accept refuses an undeclared name (078)' (#40) from multiple-fixes into main

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,6 +253,9 @@ func moduleCommand(ctx context.Context, args []string) error {
 		if !ok {
 			return fmt.Errorf("this mesh knows no module %q; `module add` it first", module)
 		}
+		if err := mayIssue(m); err != nil {
+			return err
+		}
 
 		management, err := broker.ManagementFromEnvironment()
 		if err != nil {
@@ -552,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/catalogue/manifest.go

@@ -985,11 +985,11 @@ func ParseManifest(raw []byte) (Manifest, error) {
 	}
 	// **A run-once container is a step the host runs to completion** (novox/hq ADR 0052). It is a
 	// boolean modifier on the container shape — the host runs the container, requires it to exit 0,
-	// and starts whatever the declaration places after it only once it has. Two things are refused
-	// here rather than only on the machine, for the same near-versus-far reason the action ban
-	// above records: a value that is not a boolean, and the pair run-once + restart-on, which asks
-	// for two contradictory lifecycles — restart-on brings a *running* container back, and a
-	// run-once step does not stay running.
+	// and starts whatever the declaration places after it only once it has. A value that is not a
+	// boolean is refused here rather than only on the machine, for the same near-versus-far reason
+	// the action ban above records. A run-once step may name what it reads under restart-on: for a
+	// step the word means *run again* when one of those changed, which is how a fact fetched from a
+	// provider is fetched again when the provider moved (novox/hq ADR 0099).
 	for _, r := range m.Resources {
 		if fmt.Sprint(r["type"]) != "container" {
 			continue
@@ -1003,14 +1003,6 @@ func ParseManifest(raw []byte) (Manifest, error) {
 					m.Module, r["id"], raw))
 			} else {
 				runOnce = once
-				if once {
-					if _, hasRestart := r["restart-on"]; hasRestart {
-						problems = append(problems, fmt.Sprintf(
-							"%s declares %v as run-once and with restart-on; a run-once step runs to "+
-								"completion rather than staying running to be restarted (novox/hq ADR 0052)",
-							m.Module, r["id"]))
-					}
-				}
 			}
 		}
 		// **A scheduled container runs on a recurring cadence** (novox/hq ADR 0053), the recurring

internal/catalogue/runonce_test.go

@@ -77,17 +77,16 @@ func TestARunOnceMustBeABoolean(t *testing.T) {
 	}
 }
 
-func TestARunOnceContainerCannotAlsoDeclareRestartOn(t *testing.T) {
-	// restart-on brings a running container back; a run-once step does not stay running. The pair
-	// is a contradiction, refused at the manifest rather than surfacing far away on the host.
+func TestARunOnceStepMayNameWhatItReads(t *testing.T) {
+	// For a step, restart-on means *run again* when what it reads changed: a gate that fetches a
+	// provider's root names the binding file it reads, so a provider that moved is fetched again
+	// (novox/hq ADR 0099). Accepted here, and the host's digest does the rest.
 	digest := "@sha256:" + strings.Repeat("a", 64)
-	bad := []byte(`{"module":"m","resources":[
+	good := []byte(`{"module":"m","resources":[
 	  {"id":"conf","type":"file","path":"/x","content":"y"},
 	  {"id":"seed","type":"container","name":"seed","image":"registry.example/x` + digest + `","run-once":true,"restart-on":["conf"]}
 	]}`)
-	if _, err := ParseManifest(bad); err == nil {
-		t.Error("a run-once container that also declared restart-on was accepted")
-	} else if !strings.Contains(err.Error(), "restart-on") {
-		t.Errorf("refused for the wrong reason: %v", err)
+	if _, err := ParseManifest(good); err != nil {
+		t.Errorf("a run-once step naming what it reads was refused: %v", err)
 	}
 }

internal/inventory/forget_test.go

@@ -32,7 +32,7 @@ func TestRegisteringAModuleAgainKeepsWhatTheMeshHoldsForIt(t *testing.T) {
 		t.Fatal(err)
 	}
 	m := catalogue.Manifest{Module: "step-ca", Version: "1",
-		Provides: catalogue.Offers("acme-ca")}
+		Provides: catalogue.Offers("acme-ca"), OwnSecrets: map[string]string{"password": "/run/password"}}
 	if err := inv.RegisterModule(ctx, m, Source{}); err != nil {
 		t.Fatal(err)
 	}
@@ -101,7 +101,7 @@ func TestForgettingAModuleRefusesRatherThanDiscardingWhatTheMeshHolds(t *testing
 	if err := inv.RecordSealingKey(ctx, node.ID, key); err != nil {
 		t.Fatal(err)
 	}
-	if err := inv.RegisterModule(ctx, manifest("step-ca", nil, nil), Source{}); err != nil {
+	if err := inv.RegisterModule(ctx, withOwnSecret(manifest("step-ca", nil, nil), "password"), Source{}); err != nil {
 		t.Fatal(err)
 	}
 	if err := inv.SetSettings(ctx, "anchor", "step-ca", map[string]any{"port": 9000}); err != nil {
@@ -156,7 +156,7 @@ func TestDiscardingAModuleSaysWhatWentWithIt(t *testing.T) {
 	if err := inv.RecordSealingKey(ctx, node.ID, key); err != nil {
 		t.Fatal(err)
 	}
-	if err := inv.RegisterModule(ctx, manifest("step-ca", nil, nil), Source{}); err != nil {
+	if err := inv.RegisterModule(ctx, withOwnSecret(manifest("step-ca", nil, nil), "password"), Source{}); err != nil {
 		t.Fatal(err)
 	}
 	if err := inv.SetSettings(ctx, "", "step-ca", map[string]any{"issuer": "the mesh"}); err != nil {
@@ -206,7 +206,7 @@ func TestAModuleStillAssignedRefusesBeforeAnythingAboutWhatItHolds(t *testing.T)
 	if _, err := inv.AddNode(ctx, "anchor"); err != nil {
 		t.Fatal(err)
 	}
-	if err := inv.RegisterModule(ctx, manifest("step-ca", nil, nil), Source{}); err != nil {
+	if err := inv.RegisterModule(ctx, withOwnSecret(manifest("step-ca", nil, nil), "password"), Source{}); err != nil {
 		t.Fatal(err)
 	}
 	if err := inv.SetSettings(ctx, "anchor", "step-ca", map[string]any{"a": 1}); err != nil {
@@ -224,3 +224,10 @@ func TestAModuleStillAssignedRefusesBeforeAnythingAboutWhatItHolds(t *testing.T)
 		}
 	}
 }
+
+// withOwnSecret gives a fixture manifest an own secret, so a delivery to it is one the module
+// declares (novox/hq 04-ISSUES/078).
+func withOwnSecret(m catalogue.Manifest, name string) catalogue.Manifest {
+	m.OwnSecrets = map[string]string{name: "/run/" + name}
+	return m
+}

internal/inventory/operator_test.go

@@ -13,7 +13,8 @@ import (
 // opens exactly the value the node was given.
 func TestAnOwnSecretIsSealedToTheOperatorToo(t *testing.T) {
 	inv, ctx := twoNodesWithKeys(t)
-	if err := inv.RegisterModule(ctx, catalogue.Manifest{Module: "postgres", Version: "1"}, Source{}); err != nil {
+	if err := inv.RegisterModule(ctx, catalogue.Manifest{Module: "postgres", Version: "1",
+		OwnSecrets: map[string]string{"superuser": "/run/superuser", "replication": "/run/replication"}}, Source{}); err != nil {
 		t.Fatal(err)
 	}
 

internal/inventory/secrets.go

@@ -4,9 +4,13 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"slices"
+	"sort"
+	"strings"
 
 	"github.com/jackc/pgx/v5"
 
+	"github.com/novox/mesh-controller/internal/catalogue"
 	"github.com/novox/mesh-controller/internal/secrets"
 )
 
@@ -137,6 +141,30 @@ func (i *Inventory) SecretFor(ctx context.Context, name, consumer, consumerModul
 // what differs is that both ends of the pair are sealed to, and that the record says `accepted`
 // so a later read never replaces it with a minted one. The plaintext is discarded here.
 func (i *Inventory) AcceptSecretForPair(ctx context.Context, name, consumer, consumerModule, provider, local, value string) error {
+	// Refused for a requirement the module does not have, or a local it does not keep under it
+	// (novox/hq 04-ISSUES/078): the credential would sit in the pair unread.
+	m, err := i.declared(ctx, consumerModule)
+	if err != nil {
+		return err
+	}
+	if !slices.Contains(m.Requires, name) {
+		return fmt.Errorf("%s does not require %q; it requires: %s", consumerModule, name, orNone(m.Requires))
+	}
+	if locals := m.SecretsMany[name]; len(locals) > 0 {
+		if local == "" {
+			return fmt.Errorf("%s keeps several secrets for %q; name one with --local: %s",
+				consumerModule, name, orNone(sortedNames(locals)))
+		}
+		if _, kept := locals[local]; !kept {
+			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)
+	}
 	consumerKey, err := i.SealingKeyOf(ctx, consumer)
 	if err != nil {
 		return err
@@ -362,6 +390,16 @@ func (i *Inventory) SecretForModule(ctx context.Context, node, module, name stri
 // Sealed on the way in and the plaintext discarded, exactly as a generated one is — so the only
 // difference between the two is where the value came from.
 func (i *Inventory) AcceptSecretForModule(ctx context.Context, node, module, name, value string) error {
+	// Refused for a name the module does not declare. A value stored under a name nothing reads
+	// is a delivery that changed nothing and reported success — the shape of failure the mesh
+	// is built to refuse (novox/hq 04-ISSUES/078).
+	m, err := i.declared(ctx, module)
+	if err != nil {
+		return err
+	}
+	if _, own := m.OwnSecrets[name]; !own {
+		return fmt.Errorf("%s does not declare %q as an own secret; %s", module, name, declaresOwn(m))
+	}
 	key, err := i.SealingKeyOf(ctx, node)
 	if err != nil {
 		return err
@@ -448,3 +486,40 @@ func localFlag(local string) string {
 	}
 	return " --local " + local
 }
+
+// declared is the manifest the mesh holds for a module — what a delivered value is checked
+// against, so a delivery for a name the module does not have is refused rather than stored.
+func (i *Inventory) declared(ctx context.Context, module string) (catalogue.Manifest, error) {
+	known, err := i.Catalogue(ctx)
+	if err != nil {
+		return catalogue.Manifest{}, err
+	}
+	m, ok := known[module]
+	if !ok {
+		return catalogue.Manifest{}, fmt.Errorf("%s is not a module the mesh knows; `module add` it first", module)
+	}
+	return m, nil
+}
+
+func declaresOwn(m catalogue.Manifest) string {
+	if len(m.OwnSecrets) == 0 {
+		return "it declares no own secrets"
+	}
+	return "it declares: " + strings.Join(sortedNames(m.OwnSecrets), ", ")
+}
+
+func sortedNames(of map[string]string) []string {
+	names := make([]string, 0, len(of))
+	for name := range of {
+		names = append(names, name)
+	}
+	sort.Strings(names)
+	return names
+}
+
+func orNone(names []string) string {
+	if len(names) == 0 {
+		return "none"
+	}
+	return strings.Join(names, ", ")
+}

internal/inventory/secrets_test.go

@@ -52,8 +52,11 @@ func twoNodesWithKeys(t *testing.T) (*Inventory, context.Context) {
 	// before it can (novox/hq 04-ISSUES/022). Two of them, because "two consumers on one node"
 	// is the case that key exists for.
 	for _, m := range []string{"gitea", "keycloak"} {
-		if err := inv.RegisterModule(ctx, catalogue.Manifest{Module: m, Version: "1"},
-			Source{}); err != nil {
+		// 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", "object-store"},
+			Secrets:  map[string]string{"secret": "/run/secret", "postgres-database": "/run/pg"}}, Source{}); err != nil {
 			t.Fatal(err)
 		}
 	}
@@ -404,8 +407,8 @@ func TestACredentialGoesWhenEitherMachineDoes(t *testing.T) {
 // secret was delivered — which it was.
 func TestASecretTheMeshWasGivenIsNotReinventedWhenTheMachineRejoins(t *testing.T) {
 	inv, ctx := twoNodesWithKeys(t)
-	if err := inv.RegisterModule(ctx, catalogue.Manifest{Module: "builder", Version: "1"},
-		Source{}); err != nil {
+	if err := inv.RegisterModule(ctx, catalogue.Manifest{Module: "builder", Version: "1",
+		OwnSecrets: map[string]string{"broker": "/run/broker"}}, Source{}); err != nil {
 		t.Fatal(err)
 	}
 	const url = "amqps://builder:the-password-the-broker-was-told@broker/"
@@ -436,8 +439,8 @@ func TestASecretTheMeshWasGivenIsNotReinventedWhenTheMachineRejoins(t *testing.T
 // that would make the refusal above useless if it were wrong.
 func TestASecretTheMeshWasGivenSurvivesAnOrdinaryPush(t *testing.T) {
 	inv, ctx := twoNodesWithKeys(t)
-	if err := inv.RegisterModule(ctx, catalogue.Manifest{Module: "builder", Version: "1"},
-		Source{}); err != nil {
+	if err := inv.RegisterModule(ctx, catalogue.Manifest{Module: "builder", Version: "1",
+		OwnSecrets: map[string]string{"broker": "/run/broker"}}, Source{}); err != nil {
 		t.Fatal(err)
 	}
 	if err := inv.AcceptSecretForModule(ctx, "consumer", "builder", "broker",
@@ -715,3 +718,70 @@ func TestTheOperatorRecoversEachLocalNameApart(t *testing.T) {
 		t.Fatalf("the export does not name the local names: %v", kept)
 	}
 }
+
+// **A delivered secret is accepted only under a name the module declares** (novox/hq
+// 04-ISSUES/078). A value stored under a name nothing reads is a delivery that changed nothing
+// and reported success; refused, naming what the module does declare.
+func TestADeliveredSecretIsRefusedUnderANameTheModuleDoesNotDeclare(t *testing.T) {
+	inv, ctx := twoNodesWithKeys(t)
+	if err := inv.RegisterModule(ctx, catalogue.Manifest{Module: "step-ca", Version: "1",
+		OwnSecrets: map[string]string{"password": "/run/password"}}, Source{}); err != nil {
+		t.Fatal(err)
+	}
+	err := inv.AcceptSecretForModule(ctx, "consumer", "step-ca", "root-key", "not-a-key")
+	if err == nil {
+		t.Fatal("a secret delivered under a name the module does not declare was accepted")
+	}
+	for _, want := range []string{"root-key", "password"} {
+		if !strings.Contains(err.Error(), want) {
+			t.Errorf("the refusal does not say %q: %v", want, err)
+		}
+	}
+	if err := inv.AcceptSecretForModule(ctx, "consumer", "step-ca", "password", "hunter2"); err != nil {
+		t.Fatalf("a secret delivered under a declared name was refused: %v", err)
+	}
+	// A module the mesh does not know is said, not stored.
+	err = inv.AcceptSecretForModule(ctx, "consumer", "nobody", "password", "hunter2")
+	if err == nil || !strings.Contains(err.Error(), "module add") {
+		t.Errorf("a delivery to an unknown module was not refused with the remedy: %v", err)
+	}
+}
+
+func TestADeliveredPairCredentialIsRefusedForARequirementTheModuleDoesNotHave(t *testing.T) {
+	inv, ctx := twoNodesWithKeys(t)
+	// gitea requires secret, postgres-database and object-store (the fixture); it keeps a secret
+	// 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"},
+		SecretsMany: map[string]map[string]string{"secret": {"admin": "/run/admin", "api-token": "/run/api-token"}}}
+	if err := inv.RegisterModule(ctx, m, Source{}); err != nil {
+		t.Fatal(err)
+	}
+	err = inv.AcceptSecretForPair(ctx, "secret", "consumer", "mailu", "provider", "", "hunter2")
+	if err == nil || !strings.Contains(err.Error(), "--local") {
+		t.Errorf("a delivery to a module keeping several secrets, with no local named, was not refused: %v", err)
+	}
+	err = inv.AcceptSecretForPair(ctx, "secret", "consumer", "mailu", "provider", "secret-key", "hunter2")
+	if err == nil || !strings.Contains(err.Error(), "api-token") {
+		t.Errorf("a delivery under a local the module does not keep was not refused naming the ones it keeps: %v", err)
+	}
+	if err := inv.AcceptSecretForPair(ctx, "secret", "consumer", "mailu", "provider", "admin", "hunter2"); err != nil {
+		t.Errorf("a delivery under a kept local was refused: %v", err)
+	}
+}