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{{