From 7352c846dd05daa6ab9f82bb10ee3150bcb88490 Mon Sep 17 00:00:00 2001 From: jochen Date: Tue, 22 Sep 2026 22:36:28 +0200 Subject: [PATCH] A module is told its port in a container's environment too (hq issue 088) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ${port:…} answered only inside a file's content, and the one place a module routinely writes its own address is a container's `env` — where the literal is wrong on every node whose assignment differs from the manifest's number, and wrong again on a node given that port as a setting (ADR 0100). Nothing checked it: the value is a string like any other, and it fails at runtime, on one node. Filled by the control plane, like a bound value: a port is not secret, so there is nothing for the host to be the only witness of and it learns no new field. That is the line ADR 0086 draws — its objection is to a secret being in an environment at all, not to who fills one in — so a port crosses it and a credential still does not. Same guard as before: a port the module never said it listens on is refused, now naming the container and the variable. The env map is the catalogue's, shared by every node running the module, and the resource around it is a shallow copy, so a filled value goes into a fresh map — otherwise the first node composed writes its own port into the manifest and every node after it is told that one. Inert on the catalogue as it stands: ${port:…} is written in one other place in it, a file. Renamed off _files, which this no longer is. --- .../catalogue/foundation_manifests_test.go | 54 ++++++ internal/catalogue/port_into.go | 164 +++++++++++++++++ internal/catalogue/port_into_files.go | 87 --------- internal/catalogue/port_into_files_test.go | 66 ------- internal/catalogue/port_into_test.go | 168 ++++++++++++++++++ 5 files changed, 386 insertions(+), 153 deletions(-) create mode 100644 internal/catalogue/port_into.go delete mode 100644 internal/catalogue/port_into_files.go delete mode 100644 internal/catalogue/port_into_files_test.go create mode 100644 internal/catalogue/port_into_test.go diff --git a/internal/catalogue/foundation_manifests_test.go b/internal/catalogue/foundation_manifests_test.go index c07d5f4..0493abe 100644 --- a/internal/catalogue/foundation_manifests_test.go +++ b/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"]) + } +} diff --git a/internal/catalogue/port_into.go b/internal/catalogue/port_into.go new file mode 100644 index 0000000..8cc5866 --- /dev/null +++ b/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:}. +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, ", ") +} diff --git a/internal/catalogue/port_into_files.go b/internal/catalogue/port_into_files.go deleted file mode 100644 index f694cd9..0000000 --- a/internal/catalogue/port_into_files.go +++ /dev/null @@ -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:}. -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, ", ") -} diff --git a/internal/catalogue/port_into_files_test.go b/internal/catalogue/port_into_files_test.go deleted file mode 100644 index e8bcba7..0000000 --- a/internal/catalogue/port_into_files_test.go +++ /dev/null @@ -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) - } -} diff --git a/internal/catalogue/port_into_test.go b/internal/catalogue/port_into_test.go new file mode 100644 index 0000000..8f3c2f4 --- /dev/null +++ b/internal/catalogue/port_into_test.go @@ -0,0 +1,168 @@ +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) + } + if !reflect.DeepEqual(sidecar["env"], env) { + t.Fatalf("a container that asked for nothing had its environment rebuilt: %v", sidecar["env"]) + } +}