Merge pull request 'Reach asks for names on a routed endpoint' (#137) from fix/reach-names-a-route-not-a-port into main

internal/catalogue/declaration.go

@@ -905,7 +905,18 @@ func (r Resolution) Rules(with Rendering) ([]Rule, error) {
 		if err != nil {
 			return nil, err
 		}
+		// **Only for an endpoint the proxy does not serve.** A routed endpoint's port is how the
+		// proxy reaches it and nothing else (ADR 0045), so `public` there asks for a public name and
+		// says nothing about the port — opening it to the world as well would undo the arrangement
+		// the proxy exists for, and would silently reopen a port an operator had narrowed.
+		//
+		// Found by trying to express a real module: one whose routed name must be public and whose
+		// machine-side port must not be. Under one value for both, there was no way to say it.
+		routed := RoutedPorts(m)
 		for port, reach := range reaches {
+			if routed[port] {
+				continue
+			}
 			source, ok := FilterSource(reach)
 			if !ok {
 				return nil, fmt.Errorf("%s: %q is not a reach the filter can read", m.Module, reach)

internal/catalogue/filtering.go

@@ -740,6 +740,31 @@ const (
 // reaches is every value, in the order a refusal lists them.
 var reaches = []string{ReachMachine, ReachInternal, ReachPublic, ReachBoth}
 
+// RoutedPorts are the ports a module serves through a proxy, taken from its route contributions.
+//
+// **A routed endpoint's port is how the proxy reaches it, and nothing else.** That is ADR 0045's
+// decision and it is older than reach: a public service listens `from: mesh`, only the proxy reaches
+// it, and it is exposed by name. So `public` on a routed endpoint asks for a public *name*; opening
+// that port to the world as well would undo the arrangement the proxy exists for.
+//
+// Measured before this was written, not reasoned: a module's routed name answered from the internet
+// over TLS while its machine-side port was refused from the same place. The port is not the path.
+func RoutedPorts(m Manifest) map[int]bool {
+	out := map[int]bool{}
+	note := func(values map[string]any) {
+		if port, ok := asPort(values["port"]); ok {
+			out[port] = true
+		}
+	}
+	if values, ok := m.Contributes["route"]; ok {
+		note(values)
+	}
+	for _, values := range m.ContributesMany["route"] {
+		note(values)
+	}
+	return out
+}
+
 // FilterSource is the source a reach means to the packet filter.
 //
 // `public` and `both` are the same here. A reach that opened a port to the mesh and not to the world

internal/catalogue/reach_test.go

@@ -81,16 +81,23 @@ func TestBothComposesBothNames(t *testing.T) {
 	}
 }
 
-// **The filter reads the same value.** One statement, and the rule it produces is the one the reach
+// **The filter reads the same value — for an endpoint the proxy does not serve.**
-// means — which is the whole claim of ADR 0138 and the reason reach is not two settings.
+//
-func TestTheFilterFollowsTheSameReach(t *testing.T) {
+// A routed endpoint's port is how the proxy reaches it and nothing else (ADR 0045): a public service
+// listens from the mesh, only the proxy reaches it, and it is exposed by name. So on a routed
+// endpoint the reach asks for a name and the port keeps what the manifest said.
+func TestAnUnroutedEndpointsPortFollowsItsReach(t *testing.T) {
+	// The same module with its route taken away: now the port is the only way in, so reach governs it.
+	bare := aRoutedWeb()
+	bare.Contributes = nil
+
 	for _, c := range []struct{ reach, want string }{
 		{ReachInternal, FromMesh},
 		{ReachPublic, FromEverywhere},
 		{ReachBoth, FromEverywhere},
 		{ReachMachine, FromMachine},
 	} {
-		r := Resolution{Node: "anchor", Modules: []Manifest{aRoutedWeb()}}
+		r := Resolution{Node: "anchor", Modules: []Manifest{bare}}
 		rules, err := r.Rules(Rendering{Settings: reachSet(c.reach)})
 		if err != nil {
 			t.Fatalf("%s: rules: %v", c.reach, err)
@@ -110,6 +117,30 @@ func TestTheFilterFollowsTheSameReach(t *testing.T) {
 	}
 }
 
+// **A public name does not open the machine's port**, which is the case that found this.
+//
+// A module whose routed name must be public and whose machine-side port must not be had no way to say
+// so while one value drove both. Under one value it could not be expressed; the port would reopen.
+func TestAPublicNameLeavesARoutedPortAsTheManifestSaid(t *testing.T) {
+	r := Resolution{Node: "anchor", Modules: []Manifest{aRoutedWeb()},
+		PublicDomain: "example.test", At: "anchor.internal"}
+	rules, err := r.Rules(Rendering{Settings: reachSet(ReachPublic)})
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, rule := range rules {
+		if rule.Port == 3000 && rule.From != FromMesh {
+			t.Fatalf("a public reach opened a routed port to %q; the proxy is how it is reached",
+				rule.From)
+		}
+	}
+	// And the name it asked for is there, so the reach was not simply ignored.
+	public, internal := namesFor(t, aRoutedWeb(), reachSet(ReachPublic))
+	if public != "app.example.test" || internal != "" {
+		t.Fatalf("names are %q and %q, want the public one only", public, internal)
+	}
+}
+
 // A reach for a port the module does not listen on reaches nothing, and is refused where it is
 // written rather than accepted and ignored.
 func TestAReachForAPortTheModuleDoesNotListenOnIsRefused(t *testing.T) {