From c67f8361852a78e2442946a0b4f16fda43571df7 Mon Sep 17 00:00:00 2001 From: jochen Date: Tue, 1 Sep 2026 19:31:05 +0200 Subject: [PATCH] The mesh may only move a port it actually publishes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lab caught this: a module declaring a port and running no container had its rule set opened on 20000 while its service sat on 9101. The firewall reported success and blocked the thing it was told to admit, which is the precise failure the filtering comment warns about, arrived at from the other side. Assignment was applied to every declared port. But a container's mapping is the thing that translates, and where there is none the software binds what it binds — the mesh choosing a number does not move the service, it only makes the mesh wrong about where it is. The declaration side already knew this: publishedOn rewrites container ports and nothing else. Filtering did not, so the two disagreed about the same fact. MachineSide is now the one derivation both follow. It also fixes a second case nobody had hit yet: a mapping the manifest wrote itself, like the mail system's 7080:80. That is passed through untouched when composing, so assigning it a machine port would have opened a rule on a port the container does not publish. Either side of such a mapping now names it, and the host side is the answer — a module may read `listens` as what its software binds or as what the machine exposes, and both readings want the same number. Recorded either way, assigned or not: the map means where this module's port is on this machine, and every reader needs that answer regardless of who chose it. Tests bite — making it always assignable reproduces the lab failure. --- cmd/mesh-control/plan.go | 22 +++++++--- internal/catalogue/machineside_test.go | 60 ++++++++++++++++++++++++++ internal/catalogue/manifest.go | 50 +++++++++++++++++++++ 3 files changed, 126 insertions(+), 6 deletions(-) create mode 100644 internal/catalogue/machineside_test.go diff --git a/cmd/mesh-control/plan.go b/cmd/mesh-control/plan.go index be10c90..18843bd 100644 --- a/cmd/mesh-control/plan.go +++ b/cmd/mesh-control/plan.go @@ -266,16 +266,26 @@ func declarationWith(ctx context.Context, open *stores, node string, ports := map[string]map[int]int{} for _, m := range plan.Modules { for _, l := range m.Listens { - at, err := inv.PortFor(ctx, node, m.Module, l.Port, l.Fixed) - if err != nil { - return nil, fmt.Errorf( - "%s needs %d reachable on %s and it could not be assigned: %w", - m.Module, l.Port, node, err) + // **Only a port the module actually publishes is the mesh's to move.** A container's + // mapping is the thing that translates; without one the software binds what it binds, + // and an assignment would not move the service — it would open the wrong number in the + // rule set and leave the real one shut. Recorded either way, because this map means + // *where this module's port is on this machine* and every reader of it needs that + // answer whether or not the mesh was the one who chose it. + where, mayAssign := m.MachineSide(l.Port) + if mayAssign { + at, err := inv.PortFor(ctx, node, m.Module, l.Port, l.Fixed) + if err != nil { + return nil, fmt.Errorf( + "%s needs %d reachable on %s and it could not be assigned: %w", + m.Module, l.Port, node, err) + } + where = at.Machine } if ports[m.Module] == nil { ports[m.Module] = map[int]int{} } - ports[m.Module][l.Port] = at.Machine + ports[m.Module][l.Port] = where } } diff --git a/internal/catalogue/machineside_test.go b/internal/catalogue/machineside_test.go new file mode 100644 index 0000000..553e1a5 --- /dev/null +++ b/internal/catalogue/machineside_test.go @@ -0,0 +1,60 @@ +package catalogue + +import "testing" + +// A module with nothing that publishes binds what it binds, and the mesh may not move it. +// +// This is the case that made novox/hq 04-ISSUES/028's fix wrong on its first pass: a port was +// assigned to every module that declared one, so a service listening directly had the rule set +// opened on a number nothing was listening on, and its real port shut. The firewall reported +// success and blocked the service, which is the exact failure the mechanism exists to prevent. +func TestAPortNothingPublishesIsNotTheMeshsToMove(t *testing.T) { + m := Manifest{Module: "talker", Listens: []Listening{{Port: 9101, From: FromMesh}}} + at, mayAssign := m.MachineSide(9101) + if mayAssign { + t.Fatal("the mesh took a port it cannot move: nothing translates it, so assigning one " + + "opens the wrong number and leaves the service unreachable") + } + if at != 9101 { + t.Fatalf("a port nothing publishes reaches the machine where it binds, not at %d", at) + } +} + +// A container publishing in short form is exactly the case the mesh may choose. +func TestAContainerPublishingShortIsTheMeshsToChoose(t *testing.T) { + m := Manifest{Module: "store", Resources: []map[string]any{ + {"type": "container", "id": "server", "ports": []any{"5432"}}, + }} + if _, mayAssign := m.MachineSide(5432); !mayAssign { + t.Fatal("a container's mapping is what translates a port, so this one is the mesh's to " + + "choose; refusing it puts every module back on a number it guessed") + } +} + +// A manifest that wrote its own mapping already chose, and the machine side is the outer one. +func TestAMappingTheManifestWroteIsNotReassigned(t *testing.T) { + m := Manifest{Module: "mail", Resources: []map[string]any{ + {"type": "container", "id": "front", "ports": []any{"7080:80"}}, + }} + for _, named := range []int{7080, 80} { + at, mayAssign := m.MachineSide(named) + if mayAssign { + t.Fatalf("%d was reassigned though the manifest published it explicitly, which "+ + "would open a rule on a port the container does not publish", named) + } + if at != 7080 { + t.Fatalf("naming %d gave %d; the machine side of 7080:80 is 7080", named, at) + } + } +} + +// A port some other container publishes is not this port. +func TestAPortNotInTheMappingIsNotFound(t *testing.T) { + m := Manifest{Module: "mail", Resources: []map[string]any{ + {"type": "container", "id": "front", "ports": []any{"25", "7080:80"}}, + }} + if at, mayAssign := m.MachineSide(993); mayAssign || at != 993 { + t.Fatalf("993 is published by nothing here, so it binds where it binds: got %d, %v", + at, mayAssign) + } +} diff --git a/internal/catalogue/manifest.go b/internal/catalogue/manifest.go index c151e00..910a09f 100644 --- a/internal/catalogue/manifest.go +++ b/internal/catalogue/manifest.go @@ -11,6 +11,7 @@ import ( "fmt" "regexp" "sort" + "strconv" "strings" ) @@ -711,3 +712,52 @@ func (m Manifest) OffersAt(scope string) []string { sort.Strings(out) return out } + +// MachineSide says where a module's declared port reaches this machine, and whether the mesh is +// free to choose it. +// +// **The mesh may only move a port it actually publishes** (novox/hq ADR 0038). A container's +// mapping is the thing that translates, so where there is one the mesh can put the machine side +// anywhere it likes. Where there is not, the software binds what it binds: assigning a port then +// does not move the service, it just opens the wrong number in the rule set and leaves the real +// one shut — a firewall that reports success and blocks the thing it was asked to admit. +// +// Three cases, and only the first belongs to the mesh: +// +// - a container publishes it in short form — the mesh chooses +// - a container publishes it as host:container — the manifest already chose +// - nothing publishes it — whatever binds it, binds it +// +// Either side of a long mapping counts as naming it, and the host side is what comes back. A +// module may reasonably read `listens` as the port its software uses or as the port the machine +// exposes, and both readings have the same right answer here. +func (m Manifest) MachineSide(port int) (at int, mayAssign bool) { + for _, r := range m.Resources { + if fmt.Sprint(r["type"]) != "container" { + continue + } + listed, ok := r["ports"].([]any) + if !ok { + continue + } + for _, entry := range listed { + written := strings.TrimSpace(fmt.Sprint(entry)) + host, inside, long := strings.Cut(written, ":") + if !long { + if n, err := strconv.Atoi(written); err == nil && n == port { + return port, true + } + continue + } + outer, err := strconv.Atoi(strings.TrimSpace(host)) + if err != nil { + continue + } + inner, err := strconv.Atoi(strings.TrimSpace(inside)) + if err == nil && (outer == port || inner == port) { + return outer, false + } + } + } + return port, false +}