Merge pull request 'The broker credential resolves the seat, not the hub (issue 059)' (#30) from fix/broker-address-resolves-the-seat into main

cmd/mesh-controller/modules.go

@@ -467,36 +467,88 @@ func pinCommand(ctx context.Context, args []string, setting bool) error {
 	return nil
 }
 
+// theBrokerSeat is the mesh-scoped seat the broker module claims (novox/hq ADR 0079: a
+// foundation seat is named after the server it guards).
+const theBrokerSeat = "mesh-broker"
+
 // brokerReachableAt is the broker's address as the given node can reach it.
 //
-// The genesis address (MESH_BROKER_ADDRESS) is the broker's public endpoint — reachable from the
-// control-node itself, but not routed to another node, whose firewall admits only the overlay
-// (from:mesh). The foundation, and so the broker, sits on the control-node, which is the overlay
-// hub; a node that is on the overlay reaches the broker by the hub's `.internal` name, which the
-// firewall admits and every node resolves. A node not yet on the overlay — at genesis, before any
-// `overlay place`, which is when the builder's account is issued — keeps the genesis address it
-// was given, so nothing about bring-up changes. This is issue 055.
+// The genesis address (MESH_BROKER_ADDRESS) is the broker's public endpoint — right for a machine
+// that can dial it, wrong across the mesh, where the path is the overlay. A node that is ON the
+// overlay is given the overlay name of the node HOLDING the broker — the one assigned a module
+// claiming the mesh-broker seat — because that is where the broker is, whatever else the topology
+// says. Only when nothing holds the seat yet (genesis raised the broker as plumbing and no module
+// has adopted it) does the hub stand in, which is where the foundation is by convention.
+//
+// "On the overlay" is what `whereEveryoneIs` answers — a machine that RESOLVED the networking
+// module — not "has an address", which is true of every placed machine and says nothing about
+// whether anything can reach it (novox/hq issue 059). A node not on the overlay — at genesis,
+// before any `overlay place`, which is when the builder's account is issued — keeps the genesis
+// address, so nothing about bring-up changes. This is issue 055, corrected by 059.
 func brokerReachableAt(ctx context.Context, inv *inventory.Inventory, known broker.Broker, node string) (string, error) {
-	overlays, err := inv.Overlays(ctx)
+	shelf, err := inv.Catalogue(ctx)
 	if err != nil {
 		return "", err
 	}
-	var hub string
-	onOverlay := false
-	for _, o := range overlays {
-		if o.Hub && o.Address != "" {
-			hub = o.Name
-		}
-		if o.Name == node && o.Address != "" {
-			onOverlay = true
-		}
-	}
-	if hub == "" || !onOverlay {
+	onNetwork, err := whereEveryoneIs(ctx, inv, shelf)
+	if err != nil {
+		// No catalogue yet is genesis, and at genesis the genesis address is the right one.
 		return known.Address, nil
 	}
+	if onNetwork[node] == "" {
+		return known.Address, nil
+	}
+
+	// Where the broker is: the node whose assigned set includes a module claiming the seat.
+	holders := map[string]bool{}
+	for name, m := range shelf {
+		for _, c := range m.Claims {
+			if c.Name == theBrokerSeat && c.At() == catalogue.ScopeMesh {
+				holders[name] = true
+			}
+		}
+	}
+	brokerAt := ""
+	if len(holders) > 0 {
+		overlays, err := inv.Overlays(ctx)
+		if err != nil {
+			return "", err
+		}
+		for _, o := range overlays {
+			assigned, err := inv.Assigned(ctx, o.Name)
+			if err != nil {
+				continue
+			}
+			for _, a := range assigned {
+				if holders[a] && onNetwork[o.Name] != "" {
+					brokerAt = onNetwork[o.Name]
+				}
+			}
+		}
+	}
+	if brokerAt == "" {
+		// Nothing holds the seat (or its node is not on the overlay): the hub, where the
+		// foundation is by convention — and only a hub that is itself on the overlay, or the
+		// name we hand out routes nowhere.
+		overlays, err := inv.Overlays(ctx)
+		if err != nil {
+			return "", err
+		}
+		for _, o := range overlays {
+			if o.Hub && onNetwork[o.Name] != "" {
+				brokerAt = onNetwork[o.Name]
+			}
+		}
+	}
+	if brokerAt == "" {
+		return known.Address, nil
+	}
+	// A portless genesis address is a working configuration (amqps defaults to 5671), and
+	// silently keeping the public address would disable this whole path — so the port defaults
+	// rather than the fix dissolving (novox/hq issue 059).
 	_, port, err := net.SplitHostPort(known.Address)
 	if err != nil {
-		return known.Address, nil
+		port = "5671"
 	}
-	return net.JoinHostPort(overlay.InternalName(hub), port), nil
+	return net.JoinHostPort(brokerAt, port), nil
 }

cmd/mesh-controller/network.go

@@ -93,6 +93,23 @@ func overlayPlace(ctx context.Context, inv *inventory.Inventory, args []string)
 			"and the mesh will not choose between them", node)
 	}
 
+	// One hub per mesh, refused rather than last-write-wins: with two flagged, which one the
+	// graph and the broker address pick is order-dependent — the silent-election fault ADR 0007
+	// exists to avoid, one flag over (novox/hq issue 059). Moving the hub is explicit: re-place
+	// the old one without --hub first.
+	if *hub {
+		placed, err := inv.Overlays(ctx)
+		if err != nil {
+			return err
+		}
+		for _, p := range placed {
+			if p.Hub && p.Name != node {
+				return fmt.Errorf("%s is already the hub; a mesh has one. Re-place %s without "+
+					"--hub first if the hub is moving", p.Name, p.Name)
+			}
+		}
+	}
+
 	// Declared, all three. The address is evidence of reachability and is not the fact, and hub
 	// election by address prefix fails silently (novox/hq ADR 0007).
 	if err := inv.SetPlace(ctx, node, *endpoint, *site, *hub, ""); err != nil {
@@ -108,6 +125,8 @@ func overlayPlace(ctx context.Context, inv *inventory.Inventory, args []string)
 	}
 
 	fmt.Printf("%s is at %s on the overlay\n", node, address)
+	fmt.Println("  credentials issued for it before this placement keep their old broker address —" +
+		" `module issue` them again and push (novox/hq issue 059)")
 	switch {
 	case *hub:
 		fmt.Println("  the hub — every node not sharing a site routes through it")

internal/catalogue/resolve_test.go

@@ -2,6 +2,8 @@ package catalogue
 
 import (
 	"fmt"
+	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 )
@@ -484,3 +486,24 @@ func TestSomethingOrdinaryAnsweredHereNeedsNothing(t *testing.T) {
 		t.Fatalf("a shell answered on this machine produced %d need(s)", len(got.Needs))
 	}
 }
+
+func TestTheControllersOwnManifestClaimsItsSeat(t *testing.T) {
+	// The seat test above fabricates manifests, so deleting the claim from the real module.json
+	// would fail nothing (novox/hq issue 059's review). This binds the one manifest this
+	// repository owns: the controller claims the mesh-scoped seat named after its server
+	// (ADR 0079), or the one-controller property is convention again.
+	raw, err := os.ReadFile(filepath.Join("..", "..", "module.json"))
+	if err != nil {
+		t.Fatalf("the controller's own manifest is unreadable: %v", err)
+	}
+	m, err := ParseManifest(raw)
+	if err != nil {
+		t.Fatalf("the controller's own manifest does not parse: %v", err)
+	}
+	for _, c := range m.Claims {
+		if c.Name == "mesh-controller" && c.At() == ScopeMesh {
+			return
+		}
+	}
+	t.Fatalf("module.json no longer claims the mesh-scoped mesh-controller seat: %+v", m.Claims)
+}