One entry per mapping, under the name the module itself uses

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.
This commit is contained in:
2026-09-23 02:32:28 +02:00
parent 58644fd282
commit 7d6f37af54
4 changed files with 136 additions and 53 deletions
+53 -8
View File
@@ -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)
}