Merge pull request 'Issue a bus process's account when its module is assigned or gains it, and warn of one still missing instead of composing it silently (issue 1226)' (#427) from fix/1226-issue-process-accounts into main

cmd/mesh-controller/acts.go

@@ -408,12 +408,21 @@ 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 {
+	_, reads := m.OwnSecrets["broker"]
+	if !reads && !runsTheProvisionerClient(m) {
+		// A module whose bus accounts are its processes' alone has none of its own: only the processes that
+		// are not issued yet are issued (issue 1226).
+		return processAccountsOnAssign(ctx, inv, m, node)
+	}
+	if !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 {
+	if _, minted, err := inv.BusUserHash(ctx, user); err != nil {
 		return ""
+	} else if minted {
+		// Its own account stands and is kept; a process it gained is not issued yet (issue 1226).
+		return processAccountsOnAssign(ctx, inv, m, node)
 	}
 	busAddress, err := broker.BusAddress()
 	if err == nil {
@@ -426,6 +435,16 @@ func issueOnAssign(ctx context.Context, open *stores, node, module string) strin
 	return fmt.Sprintf("its bus credential is issued and sealed to %s, and arrives with the push", node)
 }
 
+// processAccountsOnAssign issues the process accounts of an assigned module not issued yet, and says what it did.
+func processAccountsOnAssign(ctx context.Context, inv *inventory.Inventory, m catalogue.Manifest, node string) string {
+	line, err := issueMissingProcessAccounts(ctx, inv, m, node)
+	if err != nil {
+		return fmt.Sprintf("its process accounts are not issued (%v): `module issue %s --node %s` first — "+
+			"`push %s` refuses to send %s until it is", err, m.Module, node, node, m.Module)
+	}
+	return line
+}
+
 // refusedMovingTheController refuses assigning or unassigning the controller's module through a verb (review of
 // novox/hq ADR 0272): the node it is assigned to is the control-node, and the control-node's operator account is
 // the controller's terminal, so whoever moves the module chooses the terminal. At the terminal alone, as every

cmd/mesh-controller/build.go

@@ -768,6 +768,9 @@ func takeIn(ctx context.Context, inv *inventory.Inventory, result link.BuildResu
 		}
 		return manifest, kept, err
 	}
+	// A bus process the new definition gains gets its account now, on each machine assigned the module, so the
+	// next push composes a real account (issue 1226).
+	issueGainedProcessAccounts(ctx, inv, manifest)
 	// Which repository it is registered from, by the forge's own id (novox/hq ADR 0266).
 	if err := inv.SetSourceIdentity(ctx, manifest.Module, repoID); err != nil {
 		return manifest, kept, err

cmd/mesh-controller/compose_perform.go

@@ -233,6 +233,12 @@ func prepareSend(ctx context.Context, open *stores, node string) ([]string, erro
 					if !slices.Contains(in.BusMinted, "provisioner."+node) {
 						continue
 					}
+				default:
+					// A bus process's account nobody issued still gets the stand-in, until the composition leaves
+					// the module off (issue 1753): the warning names the verb (issue 1226).
+					if line := processAccountWarning(m, node, name, in.BusMinted); line != "" {
+						sayWarning(ctx, line)
+					}
 				}
 				if _, err := inv.SecretForModule(ctx, node, m.Module, name); err == nil {
 					did = true

cmd/mesh-controller/process_accounts.go

@@ -0,0 +1,130 @@
+package main
+
+import (
+	"context"
+	"fmt"
+	"io"
+	"os"
+	"strings"
+
+	"git.novox.be/novox/mesh-sdk/go/catalogue"
+
+	"github.com/novox/mesh-controller/internal/broker"
+	"github.com/novox/mesh-controller/internal/inventory"
+)
+
+// processUser is the bus user of one process of a module on a node (novox/hq ADR 0436 §1).
+func processUser(node, module, process string) string {
+	return broker.Principal{Kind: broker.KindProcess, Node: node, Module: module, Process: process}.Username()
+}
+
+// processAccountWarning is the warning line for the own secret broker-<process id> of a process that declares bus
+// when nobody issued its account (novox/hq ADR 0436 §2, issue 1226): "" for any other secret and for an issued
+// account. The send goes on with the stand-in until the composition leaves the module off (issue 1753), so the
+// line is the only loud thing about it.
+func processAccountWarning(m catalogue.Manifest, node, secret string, minted []string) string {
+	id, isProcess := strings.CutPrefix(secret, "broker-")
+	if !isProcess {
+		return ""
+	}
+	if _, declared := m.ProcessBuses()[id]; !declared {
+		return ""
+	}
+	user := processUser(node, m.Module, id)
+	for _, u := range minted {
+		if u == user {
+			return ""
+		}
+	}
+	return fmt.Sprintf("%s/%s process %s: its bus account is not issued — mesh-controller.issue %s %s",
+		node, m.Module, id, node, m.Module)
+}
+
+// sayWarning writes a line where the compose's other warnings go: the act's answer when it carries a writer,
+// else the controller's journal.
+func sayWarning(ctx context.Context, line string) {
+	var w io.Writer = os.Stderr
+	if lw, ok := ctx.Value(leftOutKey{}).(io.Writer); ok {
+		w = lw
+	}
+	fmt.Fprintln(w, line)
+}
+
+// processAccountsMissing is each process of m that declares bus and whose account is not minted yet on node.
+func processAccountsMissing(m catalogue.Manifest, node string, minted []string) []broker.ProcessPrincipal {
+	have := map[string]bool{}
+	for _, u := range minted {
+		have[u] = true
+	}
+	var out []broker.ProcessPrincipal
+	for _, pr := range inventory.ProcessesOf(m) {
+		if !have[processUser(node, m.Module, pr.Process)] {
+			out = append(out, pr)
+		}
+	}
+	return out
+}
+
+// issueMissingProcessAccounts issues the accounts of m's bus processes on node that are not minted, and no
+// other: an account already issued is rotated on purpose, never by a later delivery or assignment (issue 1226).
+// It says what it issued, one line, and nothing when nothing was missing.
+func issueMissingProcessAccounts(ctx context.Context, inv *inventory.Inventory, m catalogue.Manifest, node string) (string, error) {
+	if len(m.ProcessBuses()) == 0 || mayIssue(m) != nil {
+		return "", nil
+	}
+	minted, err := inv.BusMinted(ctx)
+	if err != nil {
+		return "", err
+	}
+	missing := processAccountsMissing(m, node, minted)
+	if len(missing) == 0 {
+		return "", nil
+	}
+	known, err := broker.FromEnvironment()
+	if err != nil {
+		return "", fmt.Errorf("cannot deliver a credential without knowing where the bus is: %w", err)
+	}
+	reachable, err := brokerReachableAt(ctx, inv, known, node)
+	if err != nil {
+		return "", err
+	}
+	busAddress, err := broker.BusAddress()
+	if err != nil {
+		busAddress = ""
+	}
+	var ids []string
+	for _, pr := range missing {
+		if err := issueProcess(ctx, inv, m, pr, node, busAddress, known, reachable); err != nil {
+			return "", err
+		}
+		ids = append(ids, pr.Process)
+	}
+	return fmt.Sprintf("its process account(s) %s issued and sealed to %s, and arrive with the push",
+		strings.Join(ids, ", "), node), nil
+}
+
+// issueGainedProcessAccounts runs after a module's new definition is registered: on each machine assigned it, the
+// accounts of its bus processes not yet minted are issued, so a process a delivery adds is not pushed with a
+// random stand-in (issue 1226). Never fatal: the build worked and the module is registered, so a failure is said,
+// and the push refuses the module until `mesh-controller.issue` is run.
+func issueGainedProcessAccounts(ctx context.Context, inv *inventory.Inventory, m catalogue.Manifest) {
+	if len(m.ProcessBuses()) == 0 {
+		return
+	}
+	nodes, err := inv.Running(ctx, m.Module)
+	if err != nil {
+		fmt.Printf("%s: its machines are not read to issue its process accounts: %v\n", m.Module, err)
+		return
+	}
+	for _, node := range nodes {
+		line, err := issueMissingProcessAccounts(ctx, inv, m, node)
+		if err != nil {
+			fmt.Printf("%s on %s: its process accounts are not issued (%v): `mesh-controller.issue %s %s` first\n",
+				m.Module, node, err, node, m.Module)
+			continue
+		}
+		if line != "" {
+			fmt.Printf("%s on %s: %s\n", m.Module, node, line)
+		}
+	}
+}

cmd/mesh-controller/process_accounts_integration_test.go

@@ -0,0 +1,135 @@
+//go:build integration
+
+package main
+
+import (
+	"bytes"
+	"encoding/json"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"git.novox.be/novox/mesh-sdk/go/catalogue"
+	"git.novox.be/novox/mesh-sdk/go/compose"
+
+	"github.com/novox/mesh-controller/internal/broker"
+	"github.com/novox/mesh-controller/internal/inventory"
+	"github.com/novox/mesh-controller/internal/link"
+	"github.com/novox/mesh-controller/internal/testbus"
+)
+
+func gainedAndPlain(t *testing.T) (plain, gained catalogue.Manifest) {
+	t.Helper()
+	plain, err := catalogue.ParseManifest([]byte(`{"module":"mesh-mail","version":"1",
+		"own-secrets":{"broker-send":{"path":"/var/lib/mesh-mail/broker-send","owner":"mesh-mail-send"}},
+		"resources":[{"id":"account","type":"user","name":"mesh-mail-send"}]}`))
+	if err != nil {
+		t.Fatal(err)
+	}
+	return plain, mailSenderWithProcess()
+}
+
+// 1226/AC2: given an assigned module whose new definition adds a bus process, when the delivery records it
+// (takeIn, the function that records a build result), the process's account is minted; and 1226/AC3: the same
+// build heard again replaces nothing.
+func TestAGainedProcessIsIssuedWhenTheDeliveryRecordsIt(t *testing.T) {
+	open := aMesh(t)
+	ctx := t.Context()
+	inv := open.inventory
+
+	dir := t.TempDir()
+	if err := busCertificate([]string{"--into", dir}); err != nil {
+		t.Fatal(err)
+	}
+	t.Setenv(broker.AddressVar, strings.TrimPrefix(testbus.URL(t), "nats://"))
+	t.Setenv(broker.CertificateVar, filepath.Join(dir, "tls.crt"))
+
+	plain, gained := gainedAndPlain(t)
+	register(t, open, plain)
+	if _, err := assign(ctx, open, "laptop", "mesh-mail"); err != nil {
+		t.Fatal(err)
+	}
+	if _, minted, _ := inv.BusUserHash(ctx, "laptop.mesh-mail.send"); minted {
+		t.Fatal("the account was minted before the module gained the process")
+	}
+
+	manifest, _ := json.Marshal(gained)
+	build := func(id, commit string) {
+		t.Helper()
+		if _, _, err := takeIn(ctx, inv, asTheOperator(t, inv, link.BuildResult{
+			ID: id, Repository: "http://forge.internal:20000/novox/mesh-mail.git", Path: "modules/mesh-mail",
+			Ref: "main", On: "anchor", Commit: commit, Manifest: manifest,
+			Source: &link.SourceOnSeat{Seat: "git", Repository: "novox/mesh-mail"},
+		})); err != nil {
+			t.Fatal(err)
+		}
+	}
+	build("b-1", "aaaaaaa1111")
+	hash, held, err := inv.BusUserHash(ctx, "laptop.mesh-mail.send")
+	if err != nil || !held {
+		t.Fatalf("1226/AC2: recording the gained definition did not mint the process's account: held %v, %v", held, err)
+	}
+	// And the next compose carries the account, with no warning (1226/AC2).
+	var said bytes.Buffer
+	if _, err := prepareSend(leftOutSaidBy(ctx, &said), open, "laptop"); err != nil {
+		t.Fatalf("1226/AC2: the next compose failed: %v", err)
+	}
+	if strings.Contains(said.String(), "bus account is not issued") {
+		t.Fatalf("1226/AC2: the next compose warns of an account just minted: %q", said.String())
+	}
+	// The row the compose reads is the account issueProcess accepted (origin accepted), never a value the mesh
+	// made for it (a stand-in).
+	in, err := inputFor(ctx, open)
+	if err != nil {
+		t.Fatal(err)
+	}
+	var row *compose.OwnSecretRow
+	for i, r := range in.OwnSecrets {
+		if r.Node == "laptop" && r.Module == "mesh-mail" && r.Name == "broker-send" {
+			row = &in.OwnSecrets[i]
+		}
+	}
+	if row == nil {
+		t.Fatal("1226/AC2: the next compose carries no row for the process's account")
+	}
+	if row.Origin != inventory.OriginAccepted {
+		t.Fatalf("1226/AC2: the process's account is a stand-in the mesh made (origin %q), not the issued account", row.Origin)
+	}
+	build("b-2", "bbbbbbb2222")
+	again, _, err := inv.BusUserHash(ctx, "laptop.mesh-mail.send")
+	if err != nil || again != hash {
+		t.Fatalf("1226/AC3: recording again replaced the issued account (%v)", err)
+	}
+}
+
+// 1226/AC1 and AC4 against the store: given two modules on a machine, one with an unissued process account, when
+// the machine is composed, the rest of it is composed and sent, and only the warning in the push's answer names
+// the first, with `mesh-controller.issue <node> <module>`.
+func TestAnUnissuedProcessAccountIsWarnedOfAndTheRestIsComposed(t *testing.T) {
+	open := aMesh(t)
+	ctx := t.Context()
+	_, gained := gainedAndPlain(t)
+	register(t, open, gained)
+	other := catalogue.Manifest{Module: "other", Version: "1",
+		OwnSecrets: catalogue.OwnSecrets{"token": {Path: "/var/lib/other/token"}}}
+	register(t, open, other)
+	for _, m := range []string{"mesh-mail", "other"} {
+		if _, err := assign(ctx, open, "laptop", m); err != nil {
+			t.Fatal(err)
+		}
+	}
+	var said bytes.Buffer
+	if _, err := prepareSend(leftOutSaidBy(ctx, &said), open, "laptop"); err != nil {
+		t.Fatalf("1226/AC4: an unissued process account held the machine: %v", err)
+	}
+	want := "laptop/mesh-mail process send: its bus account is not issued — mesh-controller.issue laptop mesh-mail"
+	if !strings.Contains(said.String(), want) {
+		t.Fatalf("1226/AC1: the push says %q, want %q", said.String(), want)
+	}
+	if strings.Contains(said.String(), "other") {
+		t.Fatalf("1226/AC4: the other module is warned of: %q", said.String())
+	}
+	if _, held, err := open.inventory.ModuleSecretIfIssued(ctx, "laptop", "other", "token"); err != nil || !held {
+		t.Fatalf("1226/AC4: the other module's secret was not composed: held %v, %v", held, err)
+	}
+}

cmd/mesh-controller/process_accounts_test.go

@@ -0,0 +1,36 @@
+package main
+
+import "testing"
+
+// 1226/AC1: given a module whose process declares bus and has no issued account, when a machine is composed, the
+// push's warning names issue, and the machine's other modules are composed (AC4: nothing is refused).
+func TestAProcessAccountNobodyIssuedIsWarnedOfNamingIssue(t *testing.T) {
+	m := mailSenderWithProcess()
+	got := processAccountWarning(m, "laptop", "broker-send", nil)
+	want := "laptop/mesh-mail process send: its bus account is not issued — mesh-controller.issue laptop mesh-mail"
+	if got != want {
+		t.Fatalf("1226/AC1: the warning is %q, want %q", got, want)
+	}
+	// Other names are not this rule's.
+	if processAccountWarning(m, "laptop", "broker", nil) != "" || processAccountWarning(m, "laptop", "broker-other", nil) != "" ||
+		processAccountWarning(m, "laptop", "token", nil) != "" {
+		t.Fatal("1226/AC1: a name that is no declared process's account was warned of")
+	}
+	if got := processAccountWarning(m, "laptop", "broker-send", []string{"laptop.mesh-mail.send"}); got != "" {
+		t.Fatalf("1226/AC3: an issued account was warned of: %q", got)
+	}
+}
+
+// 1226/AC3: an account already issued is not issued again by a later delivery or assignment.
+func TestAnIssuedProcessAccountIsNotIssuedAgain(t *testing.T) {
+	m := mailSenderWithProcess()
+	if got := processAccountsMissing(m, "laptop", nil); len(got) != 1 || got[0].Process != "send" {
+		t.Fatalf("1226/AC3: an account not issued is not missing: %v", got)
+	}
+	if got := processAccountsMissing(m, "laptop", []string{"laptop.mesh-mail.send"}); len(got) != 0 {
+		t.Fatalf("1226/AC3: an issued account would be issued again: %v", got)
+	}
+	if got := processAccountsMissing(m, "ace", []string{"laptop.mesh-mail.send"}); len(got) != 1 {
+		t.Fatalf("1226/AC3: another machine's account stands in for this one's: %v", got)
+	}
+}