From 58644fd282b9c7330438fe3bf4a2e5b6806d6168 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 23 Sep 2026 02:08:35 +0200 Subject: [PATCH 1/2] A node may move a port a module publishes as a mapping's machine side MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A module publishing `2222:22` — the machine's own ssh daemon holds 22, so the module takes 2222 and says so in `listens` — could not be moved. The setting was read against the last segment of each mapping alone, so the number the module uses everywhere else was refused as a port it does not publish, and the node's every push failed for as long as the setting was stored. The one key that was accepted, the container's own port, was then read only when the container's mapping was rewritten: the mapping moved and the ports map, the filter, the adopted node's openings, its guard and what a consumer is told all stayed on the number the software had left. Either end of a mapping now names it, and a given port comes back under both, so every reader finds the same number under the key it holds. Ambiguity is refused where it is real — one number naming two different mappings, or the two ends of one mapping given two different numbers. --- cmd/mesh-controller/sendable_test.go | 79 +++++++ internal/catalogue/adoption_test.go | 199 ++++++++++++++++++ internal/catalogue/declaration.go | 16 +- internal/catalogue/filtering.go | 98 +++++++-- .../catalogue/foundation_manifests_test.go | 53 +++++ 5 files changed, 429 insertions(+), 16 deletions(-) diff --git a/cmd/mesh-controller/sendable_test.go b/cmd/mesh-controller/sendable_test.go index f41f618..749599f 100644 --- a/cmd/mesh-controller/sendable_test.go +++ b/cmd/mesh-controller/sendable_test.go @@ -272,3 +272,82 @@ func TestTheGuardIsSentForTakenModulesOnly(t *testing.T) { } t.Fatal("a taken store is not guarded") } + +// novox/hq ADR 0038 and 0100: a module may publish a port the long way — `2222:22`, because the +// machine's own ssh daemon holds 22 — and say it listens on the machine side of that mapping, +// which is the number anything reaching it dials. A node moves that port by naming it, and the +// number has to reach everything derived from it at once: what the runtime is handed, what an +// adopted node is told to open, what its guard refuses, and what a consumer elsewhere dials. +// +// It reached none of them. The setting was refused outright for naming the machine side — so a +// module's port could not be put back where the machine it replaces had it, and, worse, the +// node's every push failed for as long as the setting existed. +func TestTheMachineSideOfAMappingIsMovedEverywhereTheNumberIsUsed(t *testing.T) { + open := aMesh(t) + ctx := t.Context() + register(t, open, catalogue.Manifest{Module: "forge", Version: "1", + Provides: []catalogue.Offer{{Name: "git-over-ssh", Scope: catalogue.ScopeMesh}}, + Serves: map[string]map[string]any{"git-over-ssh": {"port": 2222}}, + Listens: []catalogue.Listening{{Port: 2222, From: catalogue.FromMesh}}, + Resources: []map[string]any{{"id": "server", "type": "container", "name": "forge", + "ports": []any{"2222:22"}, + "image": "registry.example/forge@sha256:" + strings.Repeat("c", 64)}}}) + register(t, open, catalogue.Manifest{Module: "app", Version: "1", + Requires: []string{"git-over-ssh"}}) + if err := open.inventory.SetAdopted(ctx, "anchor", true); err != nil { + t.Fatal(err) + } + if _, err := assign(ctx, open, "anchor", "forge"); err != nil { + t.Fatal(err) + } + if _, err := assign(ctx, open, "laptop", "app"); err != nil { + t.Fatal(err) + } + if err := open.inventory.Take(ctx, "anchor", "forge"); err != nil { + t.Fatal(err) + } + if err := open.inventory.SetSettings(ctx, "anchor", "forge", + map[string]any{catalogue.PortsSetting: map[string]any{"2222": 222}}); err != nil { + t.Fatal(err) + } + + resources := composed(t, open, "anchor").Resources + var container, opening map[string]any + for _, r := range resources { + switch r["id"] { + case "forge.server": + container = r + case catalogue.OpeningID("tcp", 222, catalogue.PathForwarded): + opening = r + } + if id, _ := r["id"].(string); strings.HasPrefix(id, "adoption.opening-tcp-2222-") { + t.Errorf("the adopted node is told to open the port the forge was moved off: %s", id) + } + } + if container == nil || !reflect.DeepEqual(container["ports"], []any{"222:22"}) { + t.Fatalf("the forge's container publishes %v", container["ports"]) + } + if opening == nil || opening["to"] != 22 || opening["from"] != catalogue.OpeningFromMesh { + t.Fatalf("no opening for the port this node gave the forge: %v", opening) + } + for _, r := range resources { + if r["id"] == catalogue.GuardID() && r["content"] != catalogue.AsGuard([]int{222}) { + t.Fatalf("the guard does not refuse the port the forge is on:\n%s", r["content"]) + } + } + + // And the consumer on the other machine dials the same number. + plan, _, err := planFor(ctx, open, "laptop") + if err != nil { + t.Fatal(err) + } + var told any + for _, n := range plan.Needs { + if n.Name == "git-over-ssh" { + told = n.Serves["port"] + } + } + if told != 222 { + t.Fatalf("the consumer is told the forge answers on %v", told) + } +} diff --git a/internal/catalogue/adoption_test.go b/internal/catalogue/adoption_test.go index 6de58f1..bf4c405 100644 --- a/internal/catalogue/adoption_test.go +++ b/internal/catalogue/adoption_test.go @@ -429,3 +429,202 @@ func TestAGuardedPortOpenedToEveryoneIsNotGuarded(t *testing.T) { t.Fatalf("a port opened to everyone is still guarded:\n%s", guard) } } + +// A module that publishes its ssh port the long way — `2222:22`, because the machine's own daemon +// holds 22 — and says it listens on the machine side of that mapping, which is what anything +// reaching it dials. +func aForge() Manifest { + return Manifest{Module: "forge", + Provides: []Offer{{Name: "git-over-ssh", Scope: ScopeMesh}}, + Serves: map[string]map[string]any{"git-over-ssh": {"port": 2222}}, + Listens: []Listening{{Port: 3000, From: FromMesh}, {Port: 2222, From: FromMesh}}, + Resources: []map[string]any{{"id": "server", "type": "container", "name": "forge", + "ports": []any{"3000", "2222:22"}}}} +} + +// portsAsThePlanWould is where this machine puts each of a module's ports, derived the way +// cmd/mesh-controller/plan.go derives it: a given port first, looked up by the port the module +// says it listens on, and otherwise wherever the manifest's own mapping already put it. Written +// here because everything below — the filter, the openings, the guard, what a consumer is told — +// reads that map, and a given port that the lookup does not find moves the container's mapping +// and nothing else. +func portsAsThePlanWould(m Manifest, given map[int]int) map[int]int { + out := map[int]int{} + for _, l := range m.Listens { + if at, is := given[l.Port]; is { + out[l.Port] = at + continue + } + at, _ := m.MachineSide(l.Port) + out[l.Port] = at + } + return out +} + +// novox/hq ADR 0100 and 0038: the machine side of a long-form mapping is a port the module +// publishes, so a node may move it — and a module may name a mapping by either end. +func TestAGivenPortNamesEitherEndOfWhatTheModulePublishes(t *testing.T) { + forge := aForge() + node := func(v any) []Layer { + return []Layer{{From: "node anchor", Values: map[string]any{PortsSetting: v}}} + } + + // The machine side — the number this module says it listens on, and the one the predecessor + // had somewhere else. Answered under both ends, because the plan looks a given port up by the + // port the module declares and the container's mapping is rewritten by the port inside it. + given, err := GivenPorts(forge, node(map[string]any{"2222": float64(222)})) + if err != nil { + t.Fatalf("the machine side of a mapping cannot be given a port: %v", err) + } + if given[2222] != 222 || given[22] != 222 { + t.Fatalf("the forge was given %v, and its mapping has two ends", given) + } + + // The container's own port names the same mapping and means the same thing. + if inside, err := GivenPorts(forge, node(map[string]any{"22": float64(222)})); err != nil || + inside[2222] != 222 || inside[22] != 222 { + t.Fatalf("the container's end of the mapping was given %v: %v", inside, err) + } + + // A short form is published on the number it names, and is unchanged by any of this. + if short, err := GivenPorts(forge, node(map[string]any{"3000": float64(2999)})); err != nil || + short[3000] != 2999 { + t.Fatalf("the short form was given %v: %v", short, err) + } + + // A port no container publishes is still refused, in the same words. + if _, err := GivenPorts(forge, node(map[string]any{"9000": float64(9100)})); err == nil || + !strings.Contains(err.Error(), "does not publish") { + t.Fatalf("a port the forge does not publish was given: %v", err) + } + + // And the two ends of one mapping given two different numbers is one setting contradicting + // the other: the machine publishes it once. + if _, err := GivenPorts(forge, node(map[string]any{ + "2222": float64(222), "22": float64(300)})); err == nil || + !strings.Contains(err.Error(), "one mapping") { + t.Fatalf("the two ends of one mapping were given different ports: %v", err) + } + // Said at both ends with the same number, it is still said twice, and refused where every + // other repeated machine port is — as the inventory refuses it when the setting is stored, + // which is the layer that sees it first. + if _, err := GivenPorts(forge, node(map[string]any{ + "2222": float64(222), "22": float64(222)})); err == nil || + !strings.Contains(err.Error(), "to both its 22 and its 2222") { + t.Fatalf("one mapping given one machine port at both ends: %v", err) + } + // A number that names two different mappings names neither: which one to move is not said. + twice := aForge() + twice.Resources[0]["ports"] = []any{"22", "2222:22"} + if _, err := GivenPorts(twice, node(map[string]any{"22": float64(222)})); err == nil || + !strings.Contains(err.Error(), "twice") { + t.Fatalf("a number naming two of the module's mappings was accepted: %v", err) + } +} + +// And the number reaches everything derived from it. The fault this is written against moved the +// container's mapping alone: the filter opened the port the software had left, the adopted node's +// opening named it too, the guard refused it, and a consumer was sent to it. +func TestAGivenMachineSideReachesTheFilterTheOpeningAndTheConsumer(t *testing.T) { + forge := aForge() + given, err := GivenPorts(forge, []Layer{{From: "node anchor", + Values: map[string]any{PortsSetting: map[string]any{"2222": float64(222)}}}}) + if err != nil { + t.Fatalf("the machine side of a mapping cannot be given a port: %v", err) + } + r := Resolution{Node: "anchor", Modules: []Manifest{forge}} + with := Rendering{ + Ports: map[string]map[int]int{"forge": portsAsThePlanWould(forge, given)}, + Given: map[string]map[int]int{"forge": given}, + Mesh: []string{"10.77.0.1"}, + Adopted: true, + Taken: map[string]bool{"forge": true}, + } + + // What the runtime is handed: the machine's own port on the outside, the container's within. + composed, err := r.Compose(with) + if err != nil { + t.Fatalf("the forge does not compose: %v", err) + } + got := byID(composed.Resources) + if ports := got["forge.server"]["ports"]; !reflect.DeepEqual(ports, []any{"3000:3000", "222:22"}) { + t.Fatalf("the forge's container publishes %v", ports) + } + + // What the filter would open, were the node converged. + rules, err := r.Rules(with) + if err != nil { + t.Fatal(err) + } + var opened []int + for _, rule := range rules { + opened = append(opened, rule.Port) + } + if !reflect.DeepEqual(opened, []int{222, 3000}) { + t.Fatalf("the filter opens %v, not where the machine puts the forge", opened) + } + + // And what it is declared instead, adopted: an opening on the machine's port, naming the + // container's port on the forwarded path — a published port is forwarded, never received. + opening := got[OpeningID("tcp", 222, PathForwarded)] + if opening == nil || opening["to"] != 22 || opening["from"] != OpeningFromMesh { + t.Fatalf("no opening for the port this node gave the forge: %v", keys(got)) + } + for id := range got { + if strings.HasPrefix(id, "adoption.opening-tcp-2222-") { + t.Errorf("an opening for the port the forge was moved off: %s", id) + } + } + + // The guard refuses it where the machine put it, and nothing where it used to be. + if guard, _ := got[GuardID()]["content"].(string); guard != AsGuard([]int{222, 3000}) { + t.Fatalf("the guard does not follow the given port:\n%s", guard) + } + + // And a consumer is sent to the same number, which is read from what the module serves. + if told := ServedOn(forge, "git-over-ssh", with.Ports["forge"])["port"]; told != 222 { + t.Fatalf("a consumer is told the forge answers on %v", told) + } +} + +// And the mapping itself moves under either name, because Rendering.Given is a map anybody +// composing a declaration hands in: keyed by the machine side, which is what a module declaring +// 2222 calls its port, only the outside moves and the container's own port stays as written. +func TestAMappingIsMovedUnderEitherOfItsNames(t *testing.T) { + for _, c := range []struct { + written string + given map[int]int + want string + }{ + {"2222:22", map[int]int{2222: 222}, "222:22"}, + {"2222:22", map[int]int{22: 222}, "222:22"}, + {"127.0.0.1:15672:15672/tcp", map[int]int{15672: 15673}, "127.0.0.1:15673:15672/tcp"}, + {"2222:22", map[int]int{3000: 2999}, "2222:22"}, + {"2222:22", nil, "2222:22"}, + } { + if got := givenOuter(c.written, c.given); got != c.want { + t.Errorf("%s given %v is published as %s, want %s", c.written, c.given, got, c.want) + } + } +} + +// The same on a node that was given nothing, which is where the derivation was never at fault: +// a long-form mapping is published where the manifest put it, and an adopted node opens that port +// on the forwarded path like any other. What broke it was the number reaching only the mapping. +func TestALongFormPortIsOpenedWhereTheManifestPublishesIt(t *testing.T) { + forge := aForge() + r := Resolution{Node: "anchor", Modules: []Manifest{forge}} + composed, err := r.Compose(Rendering{ + Ports: map[string]map[int]int{"forge": portsAsThePlanWould(forge, nil)}, + Mesh: []string{"10.77.0.1"}, + Adopted: true, + }) + if err != nil { + t.Fatal(err) + } + got := byID(composed.Resources) + opening := got[OpeningID("tcp", 2222, PathForwarded)] + if opening == nil || opening["to"] != 22 { + t.Fatalf("no opening for the forge's published ssh port: %v", keys(got)) + } +} diff --git a/internal/catalogue/declaration.go b/internal/catalogue/declaration.go index b9cfdf7..a9b5549 100644 --- a/internal/catalogue/declaration.go +++ b/internal/catalogue/declaration.go @@ -1289,7 +1289,13 @@ func publishedOn(resource map[string]any, module string, with Rendering) { } // givenOuter rewrites the machine side of a long-form mapping to the port this node was given for -// its software side, when it was given one. +// it, when it was given one. +// +// **Under either of the mapping's names.** A node gives a port by the number the module names it +// by, and a module publishing `"2222:22"` may say it listens on 22 or on 2222 — GivenPorts accepts +// both and answers to both, so looking the machine side up first and the container's port second +// finds the same number either way. Read only by the container's port, this moved nothing for the +// module that declares the machine side, and the setting was refused before it got here. func givenOuter(written string, given map[int]int) string { if len(given) == 0 { return written @@ -1299,11 +1305,19 @@ func givenOuter(written string, given map[int]int) string { mapping, protocol = written[:cut], written[cut:] } parts := strings.Split(mapping, ":") + if len(parts) < 2 { + return written + } inner, err := strconv.Atoi(strings.TrimSpace(parts[len(parts)-1])) if err != nil { return written } at, ok := given[inner] + if outer, err := strconv.Atoi(strings.TrimSpace(parts[len(parts)-2])); err == nil { + if machine, named := given[outer]; named { + at, ok = machine, true + } + } if !ok { return written } diff --git a/internal/catalogue/filtering.go b/internal/catalogue/filtering.go index 674ebe1..994c2b0 100644 --- a/internal/catalogue/filtering.go +++ b/internal/catalogue/filtering.go @@ -483,12 +483,26 @@ const MeshWideLayer = "the mesh" // was configured with, and moving that number would put it in the filter, in the openings and in // what consumers are told while the software still listens on the old one — a port that reads as // moved and is not. +// +// **Either name of a mapping names it.** A short form publishes one number, which is the +// container's port and the machine's at once. A mapping written the long way — `"2222:22"` — has +// two, and a module reasonably declares it listens on either: the port its software uses, or the +// port the machine already serves on. Both are accepted, and both come back, so that whoever +// reads this — the ports map, the filter, the openings, what a consumer is told, and the mapping +// the runtime is handed — finds the same number under the key it happens to hold. Keyed one way +// and read the other, the setting moved the container's mapping and nothing else: a firewall, +// a set of openings and a consumer all pointing at a port the software had left. func GivenPorts(m Manifest, layers []Layer) (map[int]int, error) { - known := map[int]bool{} - for _, p := range containerPorts(m) { - known[p] = true + // Every name a setting may use, and the mapping it names. + names := map[int][]publishing{} + for _, p := range publishedPorts(m) { + names[p.inner] = append(names[p.inner], p) + if p.machine != p.inner { + names[p.machine] = append(names[p.machine], p) + } } - out := map[int]int{} + chose := map[int]int{} // the port a setting named → the machine port it gave it + meant := map[int]publishing{} // and which of the module's mappings that was for _, layer := range layers { raw, ok := layer.Values[PortsSetting] if !ok { @@ -508,11 +522,22 @@ func GivenPorts(m Manifest, layers []Layer) (map[int]int, error) { if err != nil { return nil, fmt.Errorf("%s gives %q a port, which is not a port", m.Module, portText) } - if !known[port] { + publishes, known := names[port] + if !known { return nil, fmt.Errorf("%s gives port %d a machine port, and no container of its "+ "publishes %d — the mesh cannot move a port the module does not publish; the "+ "software would go on listening where it was told to", m.Module, port, port) } + // The same number naming two different mappings — `"22"` beside `"2222:22"`, say. + // Refused rather than picked: moving one of them and leaving the other is a mapping + // the operator did not ask for and cannot see, and the two readings differ. + for _, other := range publishes[1:] { + if other != publishes[0] { + return nil, fmt.Errorf("%s gives port %d a machine port, and its containers "+ + "publish %d twice — as %s and as %s; which one to move is not said", + m.Module, port, port, publishes[0], other) + } + } at, ok := asPort(value) if !ok || at < 1 || at > 65535 { return nil, fmt.Errorf("%s gives port %d the machine port %v, which is not a port", @@ -522,28 +547,58 @@ func GivenPorts(m Manifest, layers []Layer) (map[int]int, error) { return nil, fmt.Errorf("%s gives port %d the machine port %d, which is ssh's — the "+ "one port a machine may never lose", m.Module, port, at) } - out[port] = at + chose[port] = at + meant[port] = publishes[0] } } - // One holder per machine port, within the module too. + // One holder per machine port, within the module too — which also settles a mapping named at + // both ends, because the machine publishes it once whichever end the setting called it. holder := map[int]int{} - for port, at := range out { + for _, port := range sorted(chose) { + at := chose[port] if other, twice := holder[at]; twice { return nil, fmt.Errorf("%s gives machine port %d to both its %d and its %d", m.Module, at, min(port, other), max(port, other)) } holder[at] = port } + // And one machine port per mapping: `{"22": 222, "2222": 300}` is two numbers for the one + // thing the machine publishes, and neither is more right. + out := map[int]int{} + said := map[publishing]int{} + for _, port := range sorted(chose) { + at, mapped := chose[port], meant[port] + if was, twice := said[mapped]; twice { + return nil, fmt.Errorf("%s gives %s the machine ports %d and %d — its %d and its %d "+ + "are the two ends of one mapping, and it is published once", m.Module, mapped, + min(was, at), max(was, at), min(mapped.machine, mapped.inner), + max(mapped.machine, mapped.inner)) + } + said[mapped] = at + out[mapped.inner] = at + out[mapped.machine] = at + } if len(out) == 0 { return nil, nil } return out, nil } -// containerPorts are the software ports a module's containers publish, whichever form they are -// written in. -func containerPorts(m Manifest) []int { - var out []int +// publishing is one mapping a module's container writes: the port the container itself uses, and +// the machine port the manifest put it on — the same number for a short form, which the runtime +// publishes on the port it names. +type publishing struct{ machine, inner int } + +func (p publishing) String() string { + if p.machine == p.inner { + return strconv.Itoa(p.inner) + } + return fmt.Sprintf("%d:%d", p.machine, p.inner) +} + +// publishedPorts is every mapping a module's containers publish, whichever form it is written in. +func publishedPorts(m Manifest) []publishing { + var out []publishing for _, r := range m.Resources { if fmt.Sprint(r["type"]) != "container" { continue @@ -551,14 +606,27 @@ func containerPorts(m Manifest) []int { listed, _ := r["ports"].([]any) for _, entry := range listed { written := strings.TrimSpace(fmt.Sprint(entry)) + if machine, inner, _, ok := mapping(written); ok { + out = append(out, publishing{machine: machine, inner: inner}) + continue + } if cut := strings.LastIndex(written, "/"); cut >= 0 { written = written[:cut] } - parts := strings.Split(written, ":") - if n, err := strconv.Atoi(parts[len(parts)-1]); err == nil { - out = append(out, n) + if n, err := strconv.Atoi(strings.TrimSpace(written)); err == nil { + out = append(out, publishing{machine: n, inner: n}) } } } return out } + +// sorted is a settings map's ports in order, so a refusal reads the same on every run. +func sorted(of map[int]int) []int { + out := make([]int, 0, len(of)) + for port := range of { + out = append(out, port) + } + sort.Ints(out) + return out +} diff --git a/internal/catalogue/foundation_manifests_test.go b/internal/catalogue/foundation_manifests_test.go index 0493abe..ff4ce36 100644 --- a/internal/catalogue/foundation_manifests_test.go +++ b/internal/catalogue/foundation_manifests_test.go @@ -250,3 +250,56 @@ func TestTheForgesOwnAddressFollowsThePortTheNodeGaveIt(t *testing.T) { "whatever reads it dials a dead port", env["MESH_GITEA_URL"]) } } + +// **And the port the forge publishes the long way is the node's too** (novox/hq ADR 0100). +// +// The forge's ssh port is written `2222:22` — the machine's own daemon holds 22, so the module +// takes 2222 and says so in `listens`. A node whose predecessor served git on another number +// cannot be told to leave it there unless the setting may name the machine side of that mapping, +// which is the number the manifest itself uses everywhere else. Composed from the manifest in the +// catalogue beside this checkout, because what the mesh can move is a fact about what the module +// actually writes. +func TestTheForgesSshPortIsGivenByTheNumberTheForgeCallsIt(t *testing.T) { + forge := catalogueManifest(t, "gitea") + given, err := GivenPorts(forge, []Layer{{From: "anchor", + Values: map[string]any{PortsSetting: map[string]any{"2222": float64(222)}}}}) + if err != nil { + t.Fatalf("the forge's ssh port cannot be given on a node: %v", err) + } + // Under the number the module listens on, which is how the plan finds it, and under the + // container's own port, which is how the mapping is rewritten. + if given[2222] != 222 || given[22] != 222 { + t.Fatalf("the forge was given %v", given) + } + + resolved, err := forge.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{resolved}, 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"}, + }} + out, err := r.Declaration(Rendering{ + Needed: map[string]map[string]string{"gitea": {"broker": "sealed-broker"}}, + Ports: map[string]map[int]int{"gitea": {3000: 3000, 2222: 222}}, + Given: map[string]map[int]int{"gitea": given}, + }) + if err != nil { + t.Fatalf("the forge does not compose: %v", err) + } + 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, "222:22") || + strings.Contains(published, "2222:22") { + t.Fatalf("the forge is published on %v, not the port this node gave it", server["ports"]) + } +} -- 2.54.0 From 7d6f37af541ef2cdf3df8b1ce28c3b154042474f Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 23 Sep 2026 02:32:28 +0200 Subject: [PATCH 2/2] One entry per mapping, under the name the module itself uses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the first pass aliased its answer under both ends of a mapping, which is wrong wherever two mappings share a number: the alias lands on a key belonging to another mapping, the later write wins, and the filter and the container then disagree — the very fault this change exists to close. Two reproduced cases: a module publishing 8080:80 beside 9090:8080 had an explicit setting silently overwritten; a module publishing 4001:80 beside 4002:80 composed both containers onto one machine port, where before it was safely refused. Now a mapping's answer is filed once, under the end the module names in its listens — the number the plan, the filter, the openings, the guard and the consumer all ask for — and a key that names two mappings is refused in the same words as a setting that does. Also: the guard assertion in the end-to-end test failed open when the resource was absent; the plan-mirroring helper now says it stands in only where the plan does not allocate, and the assertions it feeds are narrowed to the port under test. --- cmd/mesh-controller/sendable_test.go | 8 +- internal/catalogue/adoption_test.go | 61 ++++++++-- internal/catalogue/filtering.go | 112 ++++++++++++------ .../catalogue/foundation_manifests_test.go | 8 +- 4 files changed, 136 insertions(+), 53 deletions(-) diff --git a/cmd/mesh-controller/sendable_test.go b/cmd/mesh-controller/sendable_test.go index 749599f..f831e34 100644 --- a/cmd/mesh-controller/sendable_test.go +++ b/cmd/mesh-controller/sendable_test.go @@ -330,11 +330,15 @@ func TestTheMachineSideOfAMappingIsMovedEverywhereTheNumberIsUsed(t *testing.T) if opening == nil || opening["to"] != 22 || opening["from"] != catalogue.OpeningFromMesh { t.Fatalf("no opening for the port this node gave the forge: %v", opening) } + var guard map[string]any for _, r := range resources { - if r["id"] == catalogue.GuardID() && r["content"] != catalogue.AsGuard([]int{222}) { - t.Fatalf("the guard does not refuse the port the forge is on:\n%s", r["content"]) + if r["id"] == catalogue.GuardID() { + guard = r } } + if guard == nil || guard["content"] != catalogue.AsGuard([]int{222}) { + t.Fatalf("the guard does not refuse the port the forge is on:\n%v", guard["content"]) + } // And the consumer on the other machine dials the same number. plan, _, err := planFor(ctx, open, "laptop") diff --git a/internal/catalogue/adoption_test.go b/internal/catalogue/adoption_test.go index bf4c405..7f313d6 100644 --- a/internal/catalogue/adoption_test.go +++ b/internal/catalogue/adoption_test.go @@ -3,6 +3,7 @@ package catalogue import ( "encoding/json" "reflect" + "slices" "strings" "testing" ) @@ -448,6 +449,12 @@ func aForge() Manifest { // here because everything below — the filter, the openings, the guard, what a consumer is told — // reads that map, and a given port that the lookup does not find moves the container's mapping // and nothing else. +// +// It stands in for the plan only where the plan does not allocate: a port the manifest already +// placed, or one a node was given. For a short form with no given port the real plan asks the +// inventory for a machine port and may come back with one from the pool, which needs a store and +// is what cmd/mesh-controller's own tests exercise. So an assertion here about such a port asserts +// this helper, not the mesh; keep the assertions to the ports under test. func portsAsThePlanWould(m Manifest, given map[int]int) map[int]int { out := map[int]int{} for _, l := range m.Listens { @@ -470,19 +477,21 @@ func TestAGivenPortNamesEitherEndOfWhatTheModulePublishes(t *testing.T) { } // The machine side — the number this module says it listens on, and the one the predecessor - // had somewhere else. Answered under both ends, because the plan looks a given port up by the - // port the module declares and the container's mapping is rewritten by the port inside it. + // had somewhere else. Answered under 2222, the end the module itself names, which is what + // every reader of this map asks for. One entry, not two: a second key for the same answer is + // an entry another mapping's reader could find instead. given, err := GivenPorts(forge, node(map[string]any{"2222": float64(222)})) if err != nil { t.Fatalf("the machine side of a mapping cannot be given a port: %v", err) } - if given[2222] != 222 || given[22] != 222 { - t.Fatalf("the forge was given %v, and its mapping has two ends", given) + if want := map[int]int{2222: 222}; !reflect.DeepEqual(given, want) { + t.Fatalf("the forge was given %v; the mapping it names is filed under %v", given, want) } - // The container's own port names the same mapping and means the same thing. + // The container's own port names the same mapping and means the same thing — and is filed + // under the same name, because the module's name for it has not changed. if inside, err := GivenPorts(forge, node(map[string]any{"22": float64(222)})); err != nil || - inside[2222] != 222 || inside[22] != 222 { + !reflect.DeepEqual(inside, map[int]int{2222: 222}) { t.Fatalf("the container's end of the mapping was given %v: %v", inside, err) } @@ -520,6 +529,38 @@ func TestAGivenPortNamesEitherEndOfWhatTheModulePublishes(t *testing.T) { !strings.Contains(err.Error(), "twice") { t.Fatalf("a number naming two of the module's mappings was accepted: %v", err) } + + // And the same refusal when the number naming two mappings is not the one the setting used + // but the one the answer would be filed under. Here `80` is the module's own name for a + // mapping, and two mappings wear it; filing an answer there is one container's port standing + // where the other's is read, and both containers then publish it. + shared := Manifest{Module: "gallery", + Listens: []Listening{{Port: 80, From: FromMesh}}, + Resources: []map[string]any{ + {"id": "a", "type": "container", "name": "a", "ports": []any{"4001:80"}}, + {"id": "b", "type": "container", "name": "b", "ports": []any{"4002:80"}}}} + if _, err := GivenPorts(shared, node(map[string]any{"4001": float64(1234)})); err == nil || + !strings.Contains(err.Error(), "twice") { + t.Fatalf("two containers were put on one machine port: %v", err) + } + + // A module whose mappings chain — one's machine side is another's container port — keeps them + // apart, because each is filed under the port the module names it by and neither name is + // shared. Given both, each moves on its own and neither overwrites the other. + chained := Manifest{Module: "chain", + Listens: []Listening{{Port: 80, From: FromMesh}, {Port: 9090, From: FromMesh}}, + Resources: []map[string]any{{"id": "server", "type": "container", "name": "chain", + "ports": []any{"8080:80", "9090:8080"}}}} + both, err := GivenPorts(chained, node(map[string]any{"80": float64(1234), "9090": float64(5678)})) + if err != nil || !reflect.DeepEqual(both, map[int]int{80: 1234, 9090: 5678}) { + t.Fatalf("chained mappings were given %v: %v", both, err) + } + if moved := givenOuter("8080:80", both); moved != "1234:80" { + t.Fatalf("the first mapping moved to %q, and it was given 1234", moved) + } + if moved := givenOuter("9090:8080", both); moved != "5678:8080" { + t.Fatalf("the second mapping moved to %q, and it was given 5678", moved) + } } // And the number reaches everything derived from it. The fault this is written against moved the @@ -560,7 +601,10 @@ func TestAGivenMachineSideReachesTheFilterTheOpeningAndTheConsumer(t *testing.T) for _, rule := range rules { opened = append(opened, rule.Port) } - if !reflect.DeepEqual(opened, []int{222, 3000}) { + // Only the moved port is asserted: the forge's other port is a short form the real plan would + // allocate rather than read off the manifest, so its number here is portsAsThePlanWould's and + // not the mesh's. + if !slices.Contains(opened, 222) || slices.Contains(opened, 2222) { t.Fatalf("the filter opens %v, not where the machine puts the forge", opened) } @@ -577,7 +621,8 @@ func TestAGivenMachineSideReachesTheFilterTheOpeningAndTheConsumer(t *testing.T) } // The guard refuses it where the machine put it, and nothing where it used to be. - if guard, _ := got[GuardID()]["content"].(string); guard != AsGuard([]int{222, 3000}) { + guard, _ := got[GuardID()]["content"].(string) + if !strings.Contains(guard, "222") || strings.Contains(guard, "2222") { t.Fatalf("the guard does not follow the given port:\n%s", guard) } diff --git a/internal/catalogue/filtering.go b/internal/catalogue/filtering.go index 994c2b0..fd3c78f 100644 --- a/internal/catalogue/filtering.go +++ b/internal/catalogue/filtering.go @@ -2,6 +2,7 @@ package catalogue import ( "fmt" + "slices" "sort" "strconv" "strings" @@ -484,14 +485,19 @@ const MeshWideLayer = "the mesh" // what consumers are told while the software still listens on the old one — a port that reads as // moved and is not. // -// **Either name of a mapping names it.** A short form publishes one number, which is the -// container's port and the machine's at once. A mapping written the long way — `"2222:22"` — has -// two, and a module reasonably declares it listens on either: the port its software uses, or the -// port the machine already serves on. Both are accepted, and both come back, so that whoever -// reads this — the ports map, the filter, the openings, what a consumer is told, and the mapping -// the runtime is handed — finds the same number under the key it happens to hold. Keyed one way -// and read the other, the setting moved the container's mapping and nothing else: a firewall, -// a set of openings and a consumer all pointing at a port the software had left. +// **Either name of a mapping names it; the module's own name answers.** A short form publishes one +// number, which is the container's port and the machine's at once. A mapping written the long way +// — `"2222:22"` — has two, and a module reasonably declares it listens on either: the port its +// software uses, or the port the machine already serves on. A setting may name either end, because +// both are true of the same mapping. The answer comes back under the end the **module** names in +// its `listens`, which is the number every reader of this map holds: the ports map, the filter, the +// openings, what a consumer is told, and the mapping the runtime is handed. Keyed one way and read +// the other, the setting moved the container's mapping and nothing else — a firewall, a set of +// openings and a consumer all pointing at a port the software had left. +// +// One entry per mapping, never two. A second key for the same answer is not a convenience: where +// two mappings share a number, it is an entry one of them writes over the other's, and the reader +// that finds the survivor disagrees with the reader that recomputes it. func GivenPorts(m Manifest, layers []Layer) (map[int]int, error) { // Every name a setting may use, and the mapping it names. names := map[int][]publishing{} @@ -501,8 +507,16 @@ func GivenPorts(m Manifest, layers []Layer) (map[int]int, error) { names[p.machine] = append(names[p.machine], p) } } - chose := map[int]int{} // the port a setting named → the machine port it gave it - meant := map[int]publishing{} // and which of the module's mappings that was + // And the names the module itself uses. A mapping's answer is filed under these, because they + // are what every reader asks for; a mapping the module names at neither end is filed under its + // machine side, where nothing looks, which is correct — nothing serves it. + declares := map[int]bool{} + for _, l := range m.Listens { + declares[l.Port] = true + } + chose := map[int]int{} // the port a setting named → the machine port it gave it + out := map[int]int{} // the port the module names → the machine port it is on + by := map[int]int{} // and which of the setting's ports put it there, for the refusal for _, layer := range layers { raw, ok := layer.Values[PortsSetting] if !ok { @@ -528,15 +542,8 @@ func GivenPorts(m Manifest, layers []Layer) (map[int]int, error) { "publishes %d — the mesh cannot move a port the module does not publish; the "+ "software would go on listening where it was told to", m.Module, port, port) } - // The same number naming two different mappings — `"22"` beside `"2222:22"`, say. - // Refused rather than picked: moving one of them and leaving the other is a mapping - // the operator did not ask for and cannot see, and the two readings differ. - for _, other := range publishes[1:] { - if other != publishes[0] { - return nil, fmt.Errorf("%s gives port %d a machine port, and its containers "+ - "publish %d twice — as %s and as %s; which one to move is not said", - m.Module, port, port, publishes[0], other) - } + if err := oneMapping(m.Module, port, publishes); err != nil { + return nil, err } at, ok := asPort(value) if !ok || at < 1 || at > 65535 { @@ -548,13 +555,11 @@ func GivenPorts(m Manifest, layers []Layer) (map[int]int, error) { "one port a machine may never lose", m.Module, port, at) } chose[port] = at - meant[port] = publishes[0] } } - // One holder per machine port, within the module too — which also settles a mapping named at - // both ends, because the machine publishes it once whichever end the setting called it. + // One holder per machine port, within the module too. holder := map[int]int{} - for _, port := range sorted(chose) { + for _, port := range sortedPorts(chose) { at := chose[port] if other, twice := holder[at]; twice { return nil, fmt.Errorf("%s gives machine port %d to both its %d and its %d", m.Module, @@ -562,21 +567,35 @@ func GivenPorts(m Manifest, layers []Layer) (map[int]int, error) { } holder[at] = port } - // And one machine port per mapping: `{"22": 222, "2222": 300}` is two numbers for the one - // thing the machine publishes, and neither is more right. - out := map[int]int{} - said := map[publishing]int{} - for _, port := range sorted(chose) { - at, mapped := chose[port], meant[port] - if was, twice := said[mapped]; twice { - return nil, fmt.Errorf("%s gives %s the machine ports %d and %d — its %d and its %d "+ - "are the two ends of one mapping, and it is published once", m.Module, mapped, - min(was, at), max(was, at), min(mapped.machine, mapped.inner), - max(mapped.machine, mapped.inner)) + // Filed under the module's own names for the mapping — every one it uses, so a module that + // says it listens on both ends is answered at both, and under the machine side when it names + // neither. + for _, port := range sortedPorts(chose) { + at, mapping := chose[port], names[port][0] + keys := []int{} + for _, end := range []int{mapping.machine, mapping.inner} { + if declares[end] && !slices.Contains(keys, end) { + keys = append(keys, end) + } + } + if len(keys) == 0 { + keys = []int{mapping.machine} + } + for _, key := range keys { + // The key must name this mapping and no other, or the entry is one mapping's answer + // standing where another's is read. + if err := oneMapping(m.Module, key, names[key]); err != nil { + return nil, err + } + // And one machine port per mapping: `{"22": 222, "2222": 300}` is two numbers for the + // one thing the machine publishes, and neither is more right. + if was, twice := out[key]; twice && was != at { + return nil, fmt.Errorf("%s gives %s the machine ports %d and %d — its %d and its "+ + "%d are the two ends of one mapping, and it is published once", m.Module, + mapping, min(was, at), max(was, at), min(by[key], port), max(by[key], port)) + } + out[key], by[key] = at, port } - said[mapped] = at - out[mapped.inner] = at - out[mapped.machine] = at } if len(out) == 0 { return nil, nil @@ -584,6 +603,21 @@ func GivenPorts(m Manifest, layers []Layer) (map[int]int, error) { return out, nil } +// oneMapping refuses a number that names two of a module's mappings — `"22"` beside `"2222:22"`, +// say, or `"4001:80"` beside `"4002:80"` read by their shared `80`. Refused rather than picked: +// moving one of them and leaving the other is a mapping the operator did not ask for and cannot +// see, and the two readings differ. +func oneMapping(module string, port int, publishes []publishing) error { + for _, other := range publishes[1:] { + if other != publishes[0] { + return fmt.Errorf("%s gives port %d a machine port, and its containers publish %d "+ + "twice — as %s and as %s; which one to move is not said", module, port, port, + publishes[0], other) + } + } + return nil +} + // publishing is one mapping a module's container writes: the port the container itself uses, and // the machine port the manifest put it on — the same number for a short form, which the runtime // publishes on the port it names. @@ -621,8 +655,8 @@ func publishedPorts(m Manifest) []publishing { return out } -// sorted is a settings map's ports in order, so a refusal reads the same on every run. -func sorted(of map[int]int) []int { +// sortedPorts is a settings map's ports in order, so a refusal reads the same on every run. +func sortedPorts(of map[int]int) []int { out := make([]int, 0, len(of)) for port := range of { out = append(out, port) diff --git a/internal/catalogue/foundation_manifests_test.go b/internal/catalogue/foundation_manifests_test.go index ff4ce36..5f032c6 100644 --- a/internal/catalogue/foundation_manifests_test.go +++ b/internal/catalogue/foundation_manifests_test.go @@ -266,10 +266,10 @@ func TestTheForgesSshPortIsGivenByTheNumberTheForgeCallsIt(t *testing.T) { if err != nil { t.Fatalf("the forge's ssh port cannot be given on a node: %v", err) } - // Under the number the module listens on, which is how the plan finds it, and under the - // container's own port, which is how the mapping is rewritten. - if given[2222] != 222 || given[22] != 222 { - t.Fatalf("the forge was given %v", given) + // Under the number the module listens on — 2222, the machine side of its mapping — which is + // the number the plan, the filter, the openings and the consumer all ask for. One entry. + if want := map[int]int{2222: 222}; !reflect.DeepEqual(given, want) { + t.Fatalf("the forge was given %v, and it names its ssh port %v", given, want) } resolved, err := forge.Resolve([]Built{{ -- 2.54.0