Merge pull request 'A container's environment follows a moved port, as a file already does (hq issue 088)' (#46) from feat/forge-address into main

internal/catalogue/foundation_manifests_test.go

@@ -196,3 +196,57 @@ func TestTheBuildersPackageBindingKeepsItsIdentity(t *testing.T) {
 		}
 	}
 }
+
+// **And the forge's own address follows it**, composed from the manifest in the catalogue beside
+// this checkout (novox/hq 04-ISSUES/088).
+//
+// The forge is reached a third way that neither test above covers: by its own sidecar, over the
+// machine's loopback, told where to go in its environment. The `2999:3000` mapping that lets the
+// forge go on binding 3000 does nothing for a caller dialling the machine — so a literal there is
+// wrong on every node whose assignment differs, and wrong for a second reason on a node given the
+// port (ADR 0100). Composed through the whole path, because what proves the placeholder resolves
+// in an `env` at all is a declaration, not a manifest.
+func TestTheForgesOwnAddressFollowsThePortTheNodeGaveIt(t *testing.T) {
+	forge, err := catalogueManifest(t, "gitea").Resolve([]Built{{
+		Name: "runtime", Kind: ArtifactImage,
+		Reference: "registry.example/gitea-runtime@sha256:" + strings.Repeat("a", 64),
+	}})
+	if err != nil {
+		t.Fatalf("the forge's manifest does not resolve against its own build: %v", err)
+	}
+	r := Resolution{Node: "anchor", Modules: []Manifest{forge}, Needs: []Needed{
+		{Name: "postgres-database", For: "gitea", From: "anchor", At: "127.0.0.1",
+			Serves: map[string]any{"port": float64(5432)}, Sealed: "sealed-db"},
+		{Name: "route", For: "gitea", From: "anchor"},
+		{Name: "secret", For: "gitea", From: "anchor", Local: "internal-token", Sealed: "sealed-token"},
+		{Name: "secret", For: "gitea", From: "anchor", Local: "admin", Sealed: "sealed-admin"},
+	}}
+
+	// The number this node was given for the forge — the one the machine it is about to run on
+	// already publishes.
+	out, err := r.Declaration(Rendering{
+		Needed: map[string]map[string]string{"gitea": {"broker": "sealed-broker"}},
+		Given:  map[string]map[int]int{"gitea": {3000: 2999}},
+	})
+	if err != nil {
+		t.Fatalf("the forge does not compose: %v", err)
+	}
+
+	// What the machine publishes, and what the forge's sidecar is told to dial: one number.
+	server := fileNamed(out, "gitea.server")
+	if server == nil {
+		t.Fatalf("the forge's own container is not in the declaration: %v", out)
+	}
+	if published := fmt.Sprint(server["ports"]); !strings.Contains(published, "2999:3000") {
+		t.Fatalf("the forge is not published on the port this node gave it: %v", server["ports"])
+	}
+	runtime := fileNamed(out, "gitea.runtime")
+	if runtime == nil {
+		t.Fatalf("the forge's sidecar is not in the declaration: %v", out)
+	}
+	env, _ := runtime["env"].(map[string]any)
+	if env["MESH_GITEA_URL"] != "http://127.0.0.1:2999" {
+		t.Fatalf("the forge's sidecar dials %v while the machine publishes the forge on 2999 — "+
+			"whatever reads it dials a dead port", env["MESH_GITEA_URL"])
+	}
+}

internal/catalogue/port_into.go

@@ -0,0 +1,164 @@
+package catalogue
+
+import (
+	"fmt"
+	"regexp"
+	"sort"
+	"strconv"
+	"strings"
+)
+
+// Telling a module which port it was given.
+//
+// **The mesh assigns the machine-side port and a module does not choose one**
+// ([ADR 0038](../../02-DECISIONS/0038-the-mesh-assigns-the-port.md)); on a node given one for a
+// module it is the operator's number rather than the mesh's
+// ([ADR 0100](../../02-DECISIONS/0100-a-node-in-use-is-adopted-before-it-is-converged.md)). For a
+// container's own listening socket that is invisible: the mesh rewrites `ports` into
+// `assigned:wanted`, the software inside binds the number it has always bound, and the machine
+// publishes a different one.
+//
+// **Two kinds of resource have no such layer.**
+//
+//   - **A process** runs on the machine, there is nothing to rewrite, and it binds whatever its
+//     configuration says — so without this, every process binds the number written in its own
+//     config, two modules declaring the same one collide, and the mesh's whole reason for
+//     assigning ports is defeated by the resource kind that most needs it.
+//   - **A container that DIALS the machine** — a module's own sidecar reaching the service beside
+//     it over the machine's loopback — is told that address in its environment, and the mapping
+//     that saves the listener does nothing for the caller: what it must dial is the machine-side
+//     number, which is exactly the one the module cannot know (novox/hq 04-ISSUES/088).
+//
+// So a module asks. `${port:8080}` is "the machine-side port you gave me for the 8080 I said I
+// listen on", and the module writes that where it would otherwise have written a literal — in a
+// file's content, or in a value of a container's `env`.
+//
+// **The environment is filled by the control plane, exactly as a bound value is.** A port is not
+// secret — the mesh holds it in the clear — so there is nothing for the host to be the only
+// witness of, and the host learns no new field. That is what separates this from
+// [ADR 0086](../../02-DECISIONS/0086-a-secret-reaches-a-process-as-a-file.md), which refuses a
+// `${secret:…}` in an `env` outright: the objection there is to the value being in an environment
+// at all, not to who fills it in.
+//
+// **It answers with the machine's number, wherever it is written.** A container reaching a sibling
+// over the runtime's own network reaches it on the port inside that container and goes on writing
+// that number literally — it is a number the module does control. This is for the machine side,
+// which is the side nobody but the mesh can know.
+
+// ofPort is where a module asks which port it was given: ${port:<the port its software uses>}.
+var ofPort = regexp.MustCompile(`\$\{port:([0-9]+)\}`)
+
+// portsUsed are the ports a written value asks about, first appearance first.
+func portsUsed(content string) []int {
+	var used []int
+	seen := map[int]bool{}
+	for _, m := range ofPort.FindAllStringSubmatch(content, -1) {
+		n, err := strconv.Atoi(m[1])
+		if err != nil || seen[n] {
+			continue
+		}
+		seen[n] = true
+		used = append(used, n)
+	}
+	return used
+}
+
+// portInto replaces a resource's ${port:…} placeholders with what this machine assigned — in a
+// file's content, and in a value of a container's environment.
+//
+// A port the module did not say it listens on is refused, for the same reason a binding's unknown
+// key is: the module is asking about something it never declared, and the answer would be a guess.
+// Left alone, the literal would be written into a configuration file, or handed to a process as
+// its environment, and read as a port number.
+func portInto(resource map[string]any, module string, listens []Listening, with Rendering) error {
+	switch fmt.Sprint(resource["type"]) {
+	case "file":
+		content, ok := resource["content"].(string)
+		if !ok {
+			return nil
+		}
+		filled, err := portsFilledInto(content,
+			fmt.Sprintf("%s has a file that", module), module, listens, with)
+		if err != nil {
+			return err
+		}
+		resource["content"] = filled
+
+	case "container":
+		env, ok := resource["env"].(map[string]any)
+		if !ok {
+			return nil
+		}
+		// In a stated order, so a container with two bad values always refuses on the same one.
+		named := make([]string, 0, len(env))
+		for key := range env {
+			named = append(named, key)
+		}
+		sort.Strings(named)
+
+		// **A fresh map, and only when something changes.** This map came out of the module's
+		// manifest and the resource around it is a shallow copy, so filling a value in place would
+		// change what the catalogue holds for every other machine running the module — the trap
+		// withMeshNames is written to avoid, one field along.
+		var filled map[string]any
+		for _, key := range named {
+			written, ok := env[key].(string)
+			if !ok || len(portsUsed(written)) == 0 {
+				continue
+			}
+			value, err := portsFilledInto(written,
+				fmt.Sprintf("%s's container %s sets %s to something that",
+					module, resource["name"], key), module, listens, with)
+			if err != nil {
+				return err
+			}
+			if filled == nil {
+				filled = map[string]any{}
+				for k, v := range env {
+					filled[k] = v
+				}
+			}
+			filled[key] = value
+		}
+		if filled != nil {
+			resource["env"] = filled
+		}
+	}
+	return nil
+}
+
+// portsFilledInto answers every ${port:…} in one written value, or refuses. `where` names the
+// place it was written, so a refusal is one edit from right whichever kind of resource it came
+// out of.
+func portsFilledInto(written, where, module string, listens []Listening, with Rendering) (
+	string, error) {
+	for _, wanted := range portsUsed(written) {
+		var declared bool
+		for _, l := range listens {
+			if l.Port == wanted {
+				declared = true
+			}
+		}
+		if !declared {
+			return "", fmt.Errorf(
+				"%s says ${port:%d}, and %s does not say it listens on %d. A module is told the "+
+					"port it was given for something it declared, and %s",
+				where, wanted, module, wanted, orNoListens(listens))
+		}
+		written = strings.ReplaceAll(written, fmt.Sprintf("${port:%d}", wanted),
+			strconv.Itoa(with.machinePort(module, wanted)))
+	}
+	return written, nil
+}
+
+// orNoListens says what would have worked, so a refusal is one edit from right.
+func orNoListens(listens []Listening) string {
+	if len(listens) == 0 {
+		return "it declares no ports at all"
+	}
+	said := make([]string, 0, len(listens))
+	for _, l := range listens {
+		said = append(said, strconv.Itoa(l.Port))
+	}
+	return "it declares " + strings.Join(said, ", ")
+}

internal/catalogue/port_into_files.go

@@ -1,87 +0,0 @@
-package catalogue
-
-import (
-	"fmt"
-	"regexp"
-	"strconv"
-	"strings"
-)
-
-// Telling a module which port it was given.
-//
-// **The mesh assigns the machine-side port and a module does not choose one**
-// ([ADR 0038](../../02-DECISIONS/0038-the-mesh-assigns-the-port.md)). For a container that is
-// invisible: the mesh rewrites `ports` into `assigned:wanted`, the software inside binds the number
-// it has always bound, and the machine publishes a different one.
-//
-// **A process has no such layer.** It runs on the machine, there is nothing to rewrite, and it
-// binds whatever its configuration says — so without this, every process binds the number written
-// in its own config, two modules declaring the same one collide, and the mesh's whole reason for
-// assigning ports is defeated by the resource kind that most needs it.
-//
-// So a module asks. `${port:8080}` is "the machine-side port you gave me for the 8080 I said I
-// listen on", and the module writes that into its own configuration exactly as it writes an
-// address it was bound to.
-
-// ofPort is where a module asks which port it was given: ${port:<the port its software uses>}.
-var ofPort = regexp.MustCompile(`\$\{port:([0-9]+)\}`)
-
-// portsUsed are the ports a file's content asks about, first appearance first.
-func portsUsed(content string) []int {
-	var used []int
-	seen := map[int]bool{}
-	for _, m := range ofPort.FindAllStringSubmatch(content, -1) {
-		n, err := strconv.Atoi(m[1])
-		if err != nil || seen[n] {
-			continue
-		}
-		seen[n] = true
-		used = append(used, n)
-	}
-	return used
-}
-
-// portInto replaces a file's ${port:…} placeholders with what this machine assigned.
-//
-// A port the module did not say it listens on is refused, for the same reason a binding's unknown
-// key is: the module is asking about something it never declared, and the answer would be a guess.
-// Left alone, the literal would be written into a configuration file and read as a port number.
-func portInto(resource map[string]any, module string, listens []Listening, with Rendering) error {
-	if fmt.Sprint(resource["type"]) != "file" {
-		return nil
-	}
-	content, ok := resource["content"].(string)
-	if !ok {
-		return nil
-	}
-	for _, wanted := range portsUsed(content) {
-		var declared bool
-		for _, l := range listens {
-			if l.Port == wanted {
-				declared = true
-			}
-		}
-		if !declared {
-			return fmt.Errorf(
-				"%s has a file that says ${port:%d}, and %s does not say it listens on %d. A "+
-					"module is told the port it was given for something it declared, and %s",
-				module, wanted, module, wanted, orNoListens(listens))
-		}
-		content = strings.ReplaceAll(content, fmt.Sprintf("${port:%d}", wanted),
-			strconv.Itoa(with.machinePort(module, wanted)))
-		resource["content"] = content
-	}
-	return nil
-}
-
-// orNoListens says what would have worked, so a refusal is one edit from right.
-func orNoListens(listens []Listening) string {
-	if len(listens) == 0 {
-		return "it declares no ports at all"
-	}
-	said := make([]string, 0, len(listens))
-	for _, l := range listens {
-		said = append(said, strconv.Itoa(l.Port))
-	}
-	return "it declares " + strings.Join(said, ", ")
-}

internal/catalogue/port_into_files_test.go

@@ -1,66 +0,0 @@
-package catalogue
-
-import (
-	"strings"
-	"testing"
-)
-
-// **A process binds the port the mesh gave it, not the one it wrote down.**
-//
-// A container never needed this: the mesh rewrites its `ports` into assigned:wanted, so the
-// software binds the number it always bound and the machine publishes another. A process runs on
-// the machine with nothing to rewrite, so without a way to ask, every process binds the number in
-// its own configuration and two modules declaring the same one collide — which is the whole
-// problem ADR 0038 exists to prevent, reintroduced by the resource kind that most needs it.
-func TestAModuleIsToldWhichPortItWasGiven(t *testing.T) {
-	file := map[string]any{
-		"type": "file", "id": "settings",
-		"content": "LISTEN=${port:8080}\n",
-	}
-	listens := []Listening{{Port: 8080, From: FromMesh}}
-	with := Rendering{Ports: map[string]map[int]int{"showcase": {8080: 21000}}}
-
-	if err := portInto(file, "showcase", listens, with); err != nil {
-		t.Fatal(err)
-	}
-	if got := file["content"].(string); got != "LISTEN=21000\n" {
-		t.Fatalf("the module was not told its assigned port: %q", got)
-	}
-}
-
-// With nothing assigned yet, it is told the port it asked about — so a mesh that has not made an
-// assignment still composes something coherent rather than writing a zero.
-func TestWithNoAssignmentAModuleIsToldWhatItAskedFor(t *testing.T) {
-	file := map[string]any{"type": "file", "content": "LISTEN=${port:8080}\n"}
-	if err := portInto(file, "showcase", []Listening{{Port: 8080}}, Rendering{}); err != nil {
-		t.Fatal(err)
-	}
-	if got := file["content"].(string); got != "LISTEN=8080\n" {
-		t.Fatalf("an unassigned port did not fall back to what was declared: %q", got)
-	}
-}
-
-// **Asking about a port it never declared is refused**, and the refusal says what it did declare.
-// The module is asking about something the mesh has no opinion on, and answering would be a guess
-// written into a configuration file as a port number.
-func TestAskingAboutAnUndeclaredPortIsRefused(t *testing.T) {
-	file := map[string]any{"type": "file", "content": "LISTEN=${port:9999}\n"}
-	err := portInto(file, "showcase", []Listening{{Port: 8080}}, Rendering{})
-	if err == nil {
-		t.Fatal("a module was told a port it never said it listens on")
-	}
-	if !strings.Contains(err.Error(), "8080") {
-		t.Fatalf("the refusal does not say what would have worked: %v", err)
-	}
-}
-
-// And a file mentioning no port is left exactly as it was.
-func TestAFileWithNoPortIsUntouched(t *testing.T) {
-	file := map[string]any{"type": "file", "content": "GREETING=hello\n"}
-	if err := portInto(file, "showcase", nil, Rendering{}); err != nil {
-		t.Fatal(err)
-	}
-	if got := file["content"].(string); got != "GREETING=hello\n" {
-		t.Fatalf("a file with no port was changed: %q", got)
-	}
-}

internal/catalogue/port_into_test.go

@@ -0,0 +1,170 @@
+package catalogue
+
+import (
+	"reflect"
+	"strconv"
+	"strings"
+	"testing"
+)
+
+// **A process binds the port the mesh gave it, not the one it wrote down.**
+//
+// A container never needed this: the mesh rewrites its `ports` into assigned:wanted, so the
+// software binds the number it always bound and the machine publishes another. A process runs on
+// the machine with nothing to rewrite, so without a way to ask, every process binds the number in
+// its own configuration and two modules declaring the same one collide — which is the whole
+// problem ADR 0038 exists to prevent, reintroduced by the resource kind that most needs it.
+func TestAModuleIsToldWhichPortItWasGiven(t *testing.T) {
+	file := map[string]any{
+		"type": "file", "id": "settings",
+		"content": "LISTEN=${port:8080}\n",
+	}
+	listens := []Listening{{Port: 8080, From: FromMesh}}
+	with := Rendering{Ports: map[string]map[int]int{"showcase": {8080: 21000}}}
+
+	if err := portInto(file, "showcase", listens, with); err != nil {
+		t.Fatal(err)
+	}
+	if got := file["content"].(string); got != "LISTEN=21000\n" {
+		t.Fatalf("the module was not told its assigned port: %q", got)
+	}
+}
+
+// With nothing assigned yet, it is told the port it asked about — so a mesh that has not made an
+// assignment still composes something coherent rather than writing a zero.
+func TestWithNoAssignmentAModuleIsToldWhatItAskedFor(t *testing.T) {
+	file := map[string]any{"type": "file", "content": "LISTEN=${port:8080}\n"}
+	if err := portInto(file, "showcase", []Listening{{Port: 8080}}, Rendering{}); err != nil {
+		t.Fatal(err)
+	}
+	if got := file["content"].(string); got != "LISTEN=8080\n" {
+		t.Fatalf("an unassigned port did not fall back to what was declared: %q", got)
+	}
+}
+
+// **Asking about a port it never declared is refused**, and the refusal says what it did declare.
+// The module is asking about something the mesh has no opinion on, and answering would be a guess
+// written into a configuration file as a port number.
+func TestAskingAboutAnUndeclaredPortIsRefused(t *testing.T) {
+	file := map[string]any{"type": "file", "content": "LISTEN=${port:9999}\n"}
+	err := portInto(file, "showcase", []Listening{{Port: 8080}}, Rendering{})
+	if err == nil {
+		t.Fatal("a module was told a port it never said it listens on")
+	}
+	if !strings.Contains(err.Error(), "8080") {
+		t.Fatalf("the refusal does not say what would have worked: %v", err)
+	}
+}
+
+// And a file mentioning no port is left exactly as it was.
+func TestAFileWithNoPortIsUntouched(t *testing.T) {
+	file := map[string]any{"type": "file", "content": "GREETING=hello\n"}
+	if err := portInto(file, "showcase", nil, Rendering{}); err != nil {
+		t.Fatal(err)
+	}
+	if got := file["content"].(string); got != "GREETING=hello\n" {
+		t.Fatalf("a file with no port was changed: %q", got)
+	}
+}
+
+// **A container that dials the machine is told the same thing, in its environment.**
+//
+// The forge's own sidecar reaches the forge over the machine's loopback (novox/hq 04-ISSUES/088).
+// The `assigned:wanted` mapping that lets the forge itself go on binding 3000 does nothing for the
+// caller: the caller dials the machine, so it must be given the machine's number — here one the
+// node was given rather than one the mesh assigned, which is the case that makes the literal wrong
+// even on a mesh that never allocates (ADR 0100).
+func TestAContainerIsToldWhichPortItWasGiven(t *testing.T) {
+	sidecar := map[string]any{
+		"type": "container", "id": "runtime", "name": "mesh-gitea",
+		"env": map[string]any{
+			"MESH_GITEA_URL":  "http://127.0.0.1:${port:3000}",
+			"MESH_ADMIN_USER": "mesh-admin",
+		},
+	}
+	listens := []Listening{{Port: 3000, From: FromMesh}}
+	with := Rendering{Given: map[string]map[int]int{"gitea": {3000: 2999}}}
+
+	if err := portInto(sidecar, "gitea", listens, with); err != nil {
+		t.Fatal(err)
+	}
+	env := sidecar["env"].(map[string]any)
+	if env["MESH_GITEA_URL"] != "http://127.0.0.1:2999" {
+		t.Fatalf("the sidecar dials %v, not the port this machine puts the forge on",
+			env["MESH_GITEA_URL"])
+	}
+	if env["MESH_ADMIN_USER"] != "mesh-admin" {
+		t.Fatalf("filling one value changed another: %v", env)
+	}
+}
+
+// **And the manifest it came out of is left alone.**
+//
+// An `env` map is the catalogue's, shared by every node running the module, and the resource
+// around it is a shallow copy. Filled in place, the first node composed would write its own port
+// into the catalogue and every node composed after it would be told that one — a fault that is
+// invisible in a single composition and wrong in every mesh with two machines.
+func TestFillingAContainersEnvironmentLeavesTheManifestAlone(t *testing.T) {
+	env := map[string]any{"MESH_GITEA_URL": "http://127.0.0.1:${port:3000}"}
+	manifest := map[string]any{"type": "container", "id": "runtime", "name": "mesh-gitea", "env": env}
+
+	for _, at := range []int{2999, 3100} {
+		copied := map[string]any{}
+		for k, v := range manifest {
+			copied[k] = v
+		}
+		with := Rendering{Given: map[string]map[int]int{"gitea": {3000: at}}}
+		if err := portInto(copied, "gitea", []Listening{{Port: 3000}}, with); err != nil {
+			t.Fatal(err)
+		}
+		want := "http://127.0.0.1:" + strconv.Itoa(at)
+		if got := copied["env"].(map[string]any)["MESH_GITEA_URL"]; got != want {
+			t.Fatalf("the second node was told %v, not %v — the first composition wrote its own "+
+				"port into the catalogue", got, want)
+		}
+	}
+	if env["MESH_GITEA_URL"] != "http://127.0.0.1:${port:3000}" {
+		t.Fatalf("the module's own manifest was edited: %v", env)
+	}
+}
+
+// Asking in an environment about a port it never declared is refused exactly as a file's is, and
+// the refusal names the container and the variable — there is no line number to go on.
+func TestAContainerAskingAboutAnUndeclaredPortIsRefused(t *testing.T) {
+	sidecar := map[string]any{
+		"type": "container", "id": "runtime", "name": "mesh-gitea",
+		"env": map[string]any{"MESH_GITEA_URL": "http://127.0.0.1:${port:9999}"},
+	}
+	err := portInto(sidecar, "gitea", []Listening{{Port: 3000}}, Rendering{})
+	if err == nil {
+		t.Fatal("a container was told a port its module never said it listens on")
+	}
+	for _, said := range []string{"mesh-gitea", "MESH_GITEA_URL", "3000"} {
+		if !strings.Contains(err.Error(), said) {
+			t.Errorf("the refusal does not say %q: %v", said, err)
+		}
+	}
+}
+
+// And a container naming no port keeps the environment it was written with — the same map, so
+// nothing is copied and no other node's composition is disturbed.
+func TestAContainerWithNoPortPlaceholderKeepsItsEnvironment(t *testing.T) {
+	env := map[string]any{"MESH_GITEA_URL": "http://gitea:3000", "PORT": 3000}
+	sidecar := map[string]any{"type": "container", "name": "mesh-gitea", "env": env}
+	if err := portInto(sidecar, "gitea", []Listening{{Port: 3000}}, Rendering{
+		Given: map[string]map[int]int{"gitea": {3000: 2999}},
+	}); err != nil {
+		t.Fatal(err)
+	}
+	// A sibling reached over the runtime's own network is reached on the port INSIDE it, which the
+	// module does control: a literal there is right, and moving it would break the one address the
+	// mapping does not touch.
+	if got := sidecar["env"].(map[string]any)["MESH_GITEA_URL"]; got != "http://gitea:3000" {
+		t.Fatalf("an address on the runtime's own network was moved to the machine's port: %v", got)
+	}
+	// The same map, not a copy of it: a container that asks for nothing is left alone, and
+	// comparing the contents would say yes even to a rebuilt map.
+	if reflect.ValueOf(sidecar["env"]).Pointer() != reflect.ValueOf(env).Pointer() {
+		t.Fatalf("a container that asked for nothing had its environment rebuilt: %v", sidecar["env"])
+	}
+}