diff --git a/internal/catalogue/declaration.go b/internal/catalogue/declaration.go index 7536ec6..d071434 100644 --- a/internal/catalogue/declaration.go +++ b/internal/catalogue/declaration.go @@ -1098,7 +1098,7 @@ func (r Resolution) contributions(settings SettingsBy, grants []Grant, if err != nil { return nil, fmt.Errorf("%s contributing to %s: %w", m.Module, to, err) } - composeName(values, r.PublicDomain, r.At, reaches) + composeName(values, r.PublicDomain, r.At, reaches, endpointPorts(m)) out[to] = append(out[to], Contribution{From: m.Module, Values: values}) } // Several contributions to one requirement (ADR 0094's sibling for `contributes`): an @@ -1116,7 +1116,7 @@ func (r Resolution) contributions(settings SettingsBy, grants []Grant, if err != nil { return nil, fmt.Errorf("%s contributing %s to %s: %w", m.Module, local, to, err) } - composeName(values, r.PublicDomain, r.At, reaches) + composeName(values, r.PublicDomain, r.At, reaches, endpointPorts(m)) out[to] = append(out[to], Contribution{From: m.Module, Values: values}) } } @@ -1147,7 +1147,8 @@ func (r Resolution) contributions(settings SettingsBy, grants []Grant, // the running mesh keeps serving the full names it has. And a labelled contribution on a node with // no public domain composes nothing — there is nothing to join it to — which reads downstream as a // route that named no host, the same as it would have before this existed. -func composeName(values map[string]any, publicDomain, internalDomain string, reaches map[int]string) { +func composeName(values map[string]any, publicDomain, internalDomain string, reaches map[int]string, + ports map[string]int) { if values == nil { return } @@ -1164,7 +1165,7 @@ func composeName(values map[string]any, publicDomain, internalDomain string, rea // Nothing said is both names, as before. That is what keeps every mesh already running identical // until an assignment speaks. wantPublic, wantInternal := true, true - if port, ok := asPort(values["port"]); ok { + if port, ok := endpointPortOf(values, ports); ok { if reach, said := reaches[port]; said { wantPublic, wantInternal = WantsPublicName(reach), WantsInternalName(reach) } @@ -1794,3 +1795,28 @@ func prepared(from map[string]any) map[string]any { delete(step, "reload-on") return step } + +// endpointPortOf is the port the endpoint a route serves listens on: looked up by the name the route +// gives, or read from the port it repeats (novox/hq ADR 0138). +// +// `ports` maps this module's endpoint names to their ports, computed once per module rather than +// re-scanned per contribution. +func endpointPortOf(values map[string]any, ports map[string]int) (int, bool) { + if name, ok := values[RouteEndpoint].(string); ok { + if port, found := ports[strings.TrimSpace(name)]; found { + return port, true + } + } + return asPort(values["port"]) +} + +// endpointPorts is a module's endpoint names against the ports they listen on. +func endpointPorts(m Manifest) map[string]int { + out := map[string]int{} + for _, l := range m.Listens { + if name := strings.TrimSpace(l.Name); name != "" { + out[name] = l.Port + } + } + return out +} diff --git a/internal/catalogue/endpoint_test.go b/internal/catalogue/endpoint_test.go new file mode 100644 index 0000000..de37ee0 --- /dev/null +++ b/internal/catalogue/endpoint_test.go @@ -0,0 +1,141 @@ +package catalogue + +import ( + "strings" + "testing" +) + +// aMediaServer is the shape one port number per key cannot express: two endpoints of different kinds. +// A web surface a proxy serves under a subdomain, and a protocol port clients dial directly because +// the client expects that number. +func aMediaServer() Manifest { + return Manifest{ + Module: "media", + Listens: []Listening{ + {Name: "web", Port: 80, From: FromMesh, Why: "the app, behind the proxy"}, + {Name: "stream", Port: 32400, From: FromEverywhere, Fixed: true, + Why: "the client dials this number; the protocol chose it"}, + }, + Contributes: map[string]map[string]any{ + "route": {"label": "media", RouteEndpoint: "web"}, + }, + } +} + +// **A route names the endpoint it serves.** A route and a listen both carried a port and nothing said +// they were the same thing; now one of them says so. +func TestARouteNamesTheEndpointItServes(t *testing.T) { + m := aMediaServer() + if port, ok := EndpointPort(m, "web"); !ok || port != 80 { + t.Fatalf("the web endpoint resolves to %d (%v), want 80", port, ok) + } + if port, ok := EndpointPort(m, "stream"); !ok || port != 32400 { + t.Fatalf("the stream endpoint resolves to %d (%v), want 32400", port, ok) + } + if _, ok := EndpointPort(m, "absent"); ok { + t.Fatal("an endpoint the module does not declare resolved to a port") + } +} + +// And the routed set is read through the name, so the endpoint the proxy serves is known without a +// reader joining two numbers. +func TestTheRoutedEndpointIsFoundByName(t *testing.T) { + routed := RoutedPorts(aMediaServer()) + if !routed[80] { + t.Fatalf("the routed endpoint was not found by name: %v", routed) + } + // And the directly-dialled one is not routed, which is what lets its reach govern its port. + if routed[32400] { + t.Fatalf("the endpoint clients dial directly reads as routed: %v", routed) + } +} + +// **Two endpoints of different shapes, configured as themselves.** The web endpoint's reach asks for +// names and leaves its port to the proxy; the stream endpoint's reach governs its port, because +// clients dial it and there is no name. +func TestTwoEndpointsOfDifferentShapesAreConfiguredSeparately(t *testing.T) { + m := aMediaServer() + settings := SettingsBy{"media": {{From: "node anchor", Values: map[string]any{ + ReachSetting: map[string]any{"80": ReachBoth, "32400": ReachPublic}, + }}}} + + r := Resolution{Node: "anchor", Modules: []Manifest{m}, + PublicDomain: "example.test", At: "anchor.internal"} + rules, err := r.Rules(Rendering{Settings: settings}) + if err != nil { + t.Fatal(err) + } + for _, rule := range rules { + switch rule.Port { + case 80: + if rule.From != FromMesh { + t.Fatalf("the routed endpoint's port opened to %q; the proxy is how it is reached", + rule.From) + } + case 32400: + if rule.From != FromEverywhere { + t.Fatalf("the directly-dialled endpoint's port is %q, want anywhere", rule.From) + } + } + } + + // And the routed one carries both names, asked for by the same statement. + given, err := r.contributions(settings, nil, nil) + if err != nil { + t.Fatal(err) + } + var public, internal string + for _, c := range given["route"] { + public, _ = c.Values["name"].(string) + internal, _ = c.Values["internal-name"].(string) + } + if public != "media.example.test" || internal != "media.anchor.internal" { + t.Fatalf("names are %q and %q, want both", public, internal) + } +} + +// A route naming an endpoint the module does not declare reaches nothing, and is refused where it is +// written rather than resolving to no port and serving nothing. +func TestARouteNamingAnEndpointTheModuleLacksIsRefused(t *testing.T) { + m := aMediaServer() + m.Contributes["route"][RouteEndpoint] = "absent" + got := strings.Join(RouteProblems(m), "\n") + if !strings.Contains(got, "does not declare") { + t.Fatalf("a route naming an absent endpoint was accepted:\n%s", got) + } +} + +// **Two endpoints called the same would make an assignment configure whichever was read last.** The +// point of a name is that it identifies one thing. +func TestTwoEndpointsWithOneNameAreRefused(t *testing.T) { + m := Manifest{Module: "twice", Listens: []Listening{ + {Name: "web", Port: 80, From: FromMesh}, + {Name: "web", Port: 8080, From: FromMesh}, + }} + got := strings.Join(endpointNameProblems(m), "\n") + if !strings.Contains(got, "could mean either") { + t.Fatalf("two endpoints with one name were accepted:\n%s", got) + } +} + +// A name that is not a name is refused where it is written: it ends up in something a person types. +func TestAnEndpointNameIsHeldToItsShape(t *testing.T) { + for _, wrong := range []string{"Web", "web port", "3000", "-web", "web_surface"} { + m := Manifest{Module: "odd", Listens: []Listening{{Name: wrong, Port: 80, From: FromMesh}}} + if got := strings.Join(endpointNameProblems(m), "\n"); !strings.Contains(got, "a name is lowercase") { + t.Fatalf("%q was accepted as an endpoint name:\n%s", wrong, got) + } + } +} + +// **Every endpoint in the catalogue is unnamed today, and must stay valid.** The word ships one +// release before anything uses it. +func TestAnUnnamedEndpointIsStillValid(t *testing.T) { + m := Manifest{Module: "ordinary", Listens: []Listening{{Port: 443, From: FromEverywhere}}} + if got := endpointNameProblems(m); len(got) != 0 { + t.Fatalf("an unnamed endpoint was refused: %v", got) + } + if got := RouteProblems(m); len(got) != 0 { + t.Fatalf("a module with no route was refused: %v", got) + } +} diff --git a/internal/catalogue/filtering.go b/internal/catalogue/filtering.go index 2cb363a..94b77e7 100644 --- a/internal/catalogue/filtering.go +++ b/internal/catalogue/filtering.go @@ -752,6 +752,15 @@ var reaches = []string{ReachMachine, ReachInternal, ReachPublic, ReachBoth} func RoutedPorts(m Manifest) map[int]bool { out := map[int]bool{} note := func(values map[string]any) { + // **The endpoint it serves, by name where it says one.** A route repeating a port number is + // the older shape and still read: 35 of the catalogue's 36 route entries name a port their + // module declares a listen on (novox/hq ADR 0138). + if name, ok := values[RouteEndpoint].(string); ok { + if port, found := EndpointPort(m, name); found { + out[port] = true + return + } + } if port, ok := asPort(values["port"]); ok { out[port] = true } @@ -852,3 +861,38 @@ func Reaches(m Manifest, layers []Layer) (map[int]string, error) { } return out, nil } + +// RouteEndpoint is the key a route contribution names the endpoint it serves with, instead of +// repeating that endpoint's port (novox/hq ADR 0138). +// +// **A route and a listen both carried a port, and nothing said they were the same thing.** They +// always were — a route serves one of the module's own endpoints — but a reader had to join two +// numbers, and an assignment configuring "the web endpoint" had to know which number that was. A +// route that names the endpoint says what it means, and the mesh looks the port up. +const RouteEndpoint = "endpoint" + +// RouteProblems holds a module's route contributions to naming an endpoint it actually has. +// +// A route naming an endpoint the module does not declare reaches nothing, and is refused where it is +// written rather than resolving to no port and serving nothing — the fault this repository names most +// often, a declaration that reads as though it did something. +func RouteProblems(m Manifest) []string { + var problems []string + check := func(where string, values map[string]any) { + name, ok := values[RouteEndpoint].(string) + if !ok || strings.TrimSpace(name) == "" { + return + } + if _, found := EndpointPort(m, name); !found { + problems = append(problems, fmt.Sprintf( + "%s routes %s to the endpoint %q, which it does not declare", m.Module, where, name)) + } + } + if values, ok := m.Contributes["route"]; ok { + check("a name", values) + } + for local, values := range m.ContributesMany["route"] { + check(local, values) + } + return problems +} diff --git a/internal/catalogue/manifest.go b/internal/catalogue/manifest.go index 3d0276c..27c80ac 100644 --- a/internal/catalogue/manifest.go +++ b/internal/catalogue/manifest.go @@ -657,8 +657,23 @@ const ( // on is a fact, and it should be written once. const ArtifactStoreProvision = "artifact-store" -// Listening is one port a module accepts connections on. +// Listening is one endpoint a module serves: a port it accepts connections on, and what may be said +// about that port from outside the module. type Listening struct { + // Name is what this endpoint is called, so an assignment and a route can refer to it as one thing + // (novox/hq ADR 0138). + // + // **Because a port number is not a name.** Three facts have to be said about an endpoint when a + // module is assigned — which machine port it lands on, the subdomain a proxy serves it under, and + // how far it reaches — and they were said in three places keyed by the port. A module with two + // endpoints of different shapes, a web surface behind a proxy and a protocol port clients dial + // directly, cannot be configured that way without a reader joining numbers by hand. + // + // The module's to choose, like the route's label: it names its own parts. Lowercase, and unique + // within the module, so a reference to it is unambiguous. Empty is allowed and means an endpoint + // nothing refers to by name, which is every endpoint in the catalogue until they are named. + Name string `json:"name,omitempty"` + Port int `json:"port"` // Protocol is "tcp" or "udp". Absent means tcp, which is what almost everything is — and a // field that had to be written every time would be written wrongly some of the time. @@ -1265,6 +1280,8 @@ func ParseManifest(raw []byte) (Manifest, error) { "%s listens on %d over %q, which is tcp or udp", m.Module, l.Port, p)) } } + problems = append(problems, endpointNameProblems(m)...) + problems = append(problems, RouteProblems(m)...) for _, port := range m.Guards { if port < 1 || port > 65535 { problems = append(problems, fmt.Sprintf( @@ -1689,3 +1706,53 @@ func (m Manifest) undeclaredMounts() []string { } return problems } + +// endpointName is what an endpoint may be called: lowercase letters, digits and dashes, starting +// with a letter. The same shape a label has, because both end up in something a person types. +var endpointName = regexp.MustCompile(`^[a-z][a-z0-9-]*$`) + +// endpointNameProblems holds a module's endpoint names to being usable as references (novox/hq ADR +// 0138). +// +// **Unique, because the point of a name is that it identifies one thing.** Two endpoints called the +// same would make an assignment that configures one silently configure whichever the mesh read last +// — the shape of fault this repository keeps finding, where a declaration appears to say something +// and says something else. +func endpointNameProblems(m Manifest) []string { + var problems []string + seen := map[string]int{} + for _, l := range m.Listens { + name := strings.TrimSpace(l.Name) + if name == "" { + continue + } + if !endpointName.MatchString(name) { + problems = append(problems, fmt.Sprintf( + "%s calls the endpoint on port %d %q; a name is lowercase letters, digits and "+ + "dashes, starting with a letter", m.Module, l.Port, l.Name)) + continue + } + if before, already := seen[name]; already { + problems = append(problems, fmt.Sprintf( + "%s calls both port %d and port %d %q, so anything naming that endpoint could mean "+ + "either", m.Module, before, l.Port, name)) + continue + } + seen[name] = l.Port + } + return problems +} + +// EndpointPort is the port of the endpoint a module calls this, and whether it has one. +func EndpointPort(m Manifest, name string) (int, bool) { + want := strings.TrimSpace(name) + if want == "" { + return 0, false + } + for _, l := range m.Listens { + if strings.TrimSpace(l.Name) == want { + return l.Port, true + } + } + return 0, false +}