diff --git a/gateway/perlistener_test.go b/gateway/perlistener_test.go new file mode 100644 index 0000000..db79302 --- /dev/null +++ b/gateway/perlistener_test.go @@ -0,0 +1,66 @@ +package gateway_test + +import ( + "strings" + "testing" + + "github.com/bitcomplete/tsjwt/gateway" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + gwapi "sigs.k8s.io/gateway-api/apis/v1" +) + +// allowedRoutes must be evaluated per matching listener, not per Gateway. On a +// Gateway with an open listener (From=All) and a strict one (From=Same) that own +// different hostnames, a route from another namespace is admitted only by the +// open listener and so may serve only the open listener's hostname — it must not +// borrow the strict listener's hostname. +// +// Regression for adversarial finding B (per-listener allowedRoutes). +func TestAllowedRoutesEvaluatedPerListener(t *testing.T) { + t.Parallel() + all := gwapi.NamespacesFromAll + same := gwapi.NamespacesFromSame + g := &gwapi.Gateway{ObjectMeta: metav1.ObjectMeta{Name: "edge", Namespace: "infra"}} + g.Spec.Listeners = []gwapi.Listener{ + { + Name: "open", Port: 443, Protocol: gwapi.HTTPSProtocolType, + Hostname: ptr(gwapi.Hostname("open.example")), + AllowedRoutes: &gwapi.AllowedRoutes{Namespaces: &gwapi.RouteNamespaces{From: &all}}, + }, + { + Name: "strict", Port: 443, Protocol: gwapi.HTTPSProtocolType, + Hostname: ptr(gwapi.Hostname("strict.example")), + AllowedRoutes: &gwapi.AllowedRoutes{Namespaces: &gwapi.RouteNamespaces{From: &same}}, + }, + } + + // A foreign-namespace route may attach (the open listener is From=All), but + // requesting the strict listener's hostname must not be served. + foreignStrict := route("gstrict", "other", "edge", []string{"strict.example"}, rule("guest-svc", 80, "/")) + foreignStrict.Spec.ParentRefs[0].Namespace = ptr(gwapi.Namespace("infra")) + foreignOpen := route("gopen", "other", "edge", []string{"open.example"}, rule("guest-svc", 80, "/")) + foreignOpen.Spec.ParentRefs[0].Namespace = ptr(gwapi.Namespace("infra")) + localStrict := route("lstrict", "infra", "edge", []string{"strict.example"}, rule("app-svc", 80, "/")) + + svcs := []corev1.Service{svc("guest-svc", "other", 80), svc("app-svc", "infra", 80)} + got, _ := gateway.Translate(g, []gwapi.HTTPRoute{foreignStrict, foreignOpen, localStrict}, svcs) + + var openServed, localServed bool + for _, r := range got { + switch { + case strings.HasPrefix(r.Name, "other/gstrict"): + t.Errorf("foreign-namespace route served strict.example, which only a From=Same listener owns: %s", r.Name) + case strings.HasPrefix(r.Name, "other/gopen"): + openServed = true + case strings.HasPrefix(r.Name, "infra/lstrict"): + localServed = true + } + } + if !openServed { + t.Error("foreign route on the From=All listener's hostname should be served") + } + if !localServed { + t.Error("same-namespace route on the From=Same listener's hostname should be served") + } +} diff --git a/gateway/reconcile.go b/gateway/reconcile.go index 0e057c7..6c174ed 100644 --- a/gateway/reconcile.go +++ b/gateway/reconcile.go @@ -179,6 +179,9 @@ func (r *GatewayReconciler) render(ctx context.Context, gw *gwapi.Gateway) (stri } table, errs := Translate(gw, routes.Items, services.Items) + // Report acceptance back on each attached route's status before rendering + // the table, so a rejected route is visible to its author, not only logged. + r.applyRouteStatus(ctx, gw, routes.Items, errs) // The generation is a usable revision: it changes when the Gateway // does, and the data plane only uses it for logs and readiness. file := routetable.Render(gw.Namespace+"/"+gw.Name, gw.Generation, table) @@ -314,4 +317,61 @@ func (conditionHelpers) setCondition(conds *[]metav1.Condition, c metav1.Conditi *conds = append(*conds, c) } +// applyRouteStatus writes this controller's Accepted condition onto every route +// attached to gw, so an author learns from the route's own status whether it +// was accepted or why it was rejected — not only from the controller log. A +// status write that fails is logged, never fatal to the reconcile. +func (r *GatewayReconciler) applyRouteStatus(ctx context.Context, gw *gwapi.Gateway, routes []gwapi.HTTPRoute, errs []RouteError) { + l := log.FromContext(ctx) + ref := parentRefToGateway(gw) + for i := range routes { + route := &routes[i] + if !attachedTo(gw, route) { + continue + } + if !upsertRouteParentCondition(route, ref, routeAcceptedCondition(route, errs)) { + continue // unchanged; do not churn the status + } + if err := r.Status().Update(ctx, route); err != nil { + l.Error(err, "update HTTPRoute status", "route", route.Namespace+"/"+route.Name) + } + } +} + +// upsertRouteParentCondition merges cond into the RouteParentStatus for this +// controller and ref, and reports whether anything changed. +func upsertRouteParentCondition(route *gwapi.HTTPRoute, ref gwapi.ParentReference, cond metav1.Condition) bool { + for i := range route.Status.Parents { + p := &route.Status.Parents[i] + if string(p.ControllerName) != ControllerName || !parentRefEqual(p.ParentRef, ref) { + continue + } + for j := range p.Conditions { + c := p.Conditions[j] + if c.Type == cond.Type { + if c.Status == cond.Status && c.Reason == cond.Reason && + c.Message == cond.Message && c.ObservedGeneration == cond.ObservedGeneration { + return false // identical; leave it, and its transition time, alone + } + break + } + } + meta.setCondition(&p.Conditions, cond) + return true + } + route.Status.Parents = append(route.Status.Parents, gwapi.RouteParentStatus{ + ParentRef: ref, + ControllerName: gwapi.GatewayController(ControllerName), + Conditions: []metav1.Condition{cond}, + }) + return true +} + +func parentRefEqual(a, b gwapi.ParentReference) bool { + name := a.Name == b.Name + ns := (a.Namespace == nil && b.Namespace == nil) || + (a.Namespace != nil && b.Namespace != nil && *a.Namespace == *b.Namespace) + return name && ns +} + var _ = apierrors.IsNotFound diff --git a/gateway/reconcile_test.go b/gateway/reconcile_test.go index 8cff01e..760b8ac 100644 --- a/gateway/reconcile_test.go +++ b/gateway/reconcile_test.go @@ -51,7 +51,7 @@ func newReconciler(t *testing.T, objs ...client.Object) (*gateway.GatewayReconci c := fake.NewClientBuilder(). WithScheme(reconcileScheme(t)). WithObjects(objs...). - WithStatusSubresource(&gwapi.Gateway{}, &gwapi.GatewayClass{}). + WithStatusSubresource(&gwapi.Gateway{}, &gwapi.GatewayClass{}, &gwapi.HTTPRoute{}). Build() return &gateway.GatewayReconciler{ Client: c, diff --git a/gateway/routes.go b/gateway/routes.go index 67a6456..fcf1d8a 100644 --- a/gateway/routes.go +++ b/gateway/routes.go @@ -275,12 +275,11 @@ func attachedTo(gw *gwapi.Gateway, r *gwapi.HTTPRoute) bool { ns = string(*p.Namespace) } if string(p.Name) == gw.Name && ns == gw.Namespace { - // Naming the Gateway is necessary but not sufficient: the - // Gateway's listeners decide which namespaces may attach, via - // allowedRoutes.namespaces (Gateway API, default From=Same). - // Without this check a route in any namespace could bind to a - // Gateway it does not own and shadow another tenant's routes. - return listenerAllowsNamespace(gw, r.Namespace) + // Naming the Gateway is necessary but not sufficient: at least one + // listener's allowedRoutes.namespaces must admit the route's + // namespace (Gateway API, default From=Same). hostnamesFor then + // restricts what the route may serve to those same listeners. + return len(admittingListeners(gw, r.Namespace)) > 0 } } return false @@ -291,25 +290,37 @@ func attachedTo(gw *gwapi.Gateway, r *gwapi.HTTPRoute) bool { // when unset is From=Same (only the Gateway's own namespace). From=Selector // needs the route namespace's labels, which this controller does not read, so // it fails closed until that lookup is wired in. -func listenerAllowsNamespace(gw *gwapi.Gateway, routeNS string) bool { +func listenerAdmits(l gwapi.Listener, gwNamespace, routeNS string) bool { + from := gwapi.NamespacesFromSame + if l.AllowedRoutes != nil && l.AllowedRoutes.Namespaces != nil && l.AllowedRoutes.Namespaces.From != nil { + from = *l.AllowedRoutes.Namespaces.From + } + switch from { + case gwapi.NamespacesFromAll: + return true + case gwapi.NamespacesFromSame: + return routeNS == gwNamespace + default: + // Selector needs the route namespace's labels, which this controller + // does not list; None admits nothing. Fail closed rather than guess. + return false + } +} + +// admittingListeners returns the Gateway's listeners whose allowedRoutes admit a +// route from routeNS. A route may attach through, and — crucially — may serve +// the hostnames of, only these listeners. Evaluating this per listener rather +// than per Gateway is what stops a route admitted by a permissive listener +// (From=All) from serving a hostname that only a stricter listener (From=Same) +// owns. +func admittingListeners(gw *gwapi.Gateway, routeNS string) []gwapi.Listener { + var out []gwapi.Listener for _, l := range gw.Spec.Listeners { - from := gwapi.NamespacesFromSame - if l.AllowedRoutes != nil && l.AllowedRoutes.Namespaces != nil && l.AllowedRoutes.Namespaces.From != nil { - from = *l.AllowedRoutes.Namespaces.From - } - switch from { - case gwapi.NamespacesFromAll: - return true - case gwapi.NamespacesFromSame: - if routeNS == gw.Namespace { - return true - } - case gwapi.NamespacesFromSelector: - // Selector needs the route namespace's labels, which this - // controller does not list; fail closed rather than guess. + if listenerAdmits(l, gw.Namespace, routeNS) { + out = append(out, l) } } - return false + return out } // hostnamesFor intersects the route's hostnames with the Gateway's listeners, @@ -321,8 +332,11 @@ func listenerAllowsNamespace(gw *gwapi.Gateway, routeNS string) bool { // True with an empty list means neither side named any, so the route answers // for whatever reaches the Gateway. func hostnamesFor(gw *gwapi.Gateway, r *gwapi.HTTPRoute) ([]string, bool) { + // Only listeners that admit this route's namespace count. A route admitted + // through a From=All listener must not borrow the hostname of a From=Same + // listener that would not have admitted it. var listener []string - for _, l := range gw.Spec.Listeners { + for _, l := range admittingListeners(gw, r.Namespace) { if l.Hostname != nil && *l.Hostname != "" { listener = append(listener, string(*l.Hostname)) } @@ -400,6 +414,30 @@ func dedupe(in []string) []string { return out } +// parentRefToGateway builds a ParentReference identifying gw, for a route's +// parent status. +func parentRefToGateway(gw *gwapi.Gateway) gwapi.ParentReference { + group := gwapi.Group("gateway.networking.k8s.io") + kind := gwapi.Kind("Gateway") + ns := gwapi.Namespace(gw.Namespace) + return gwapi.ParentReference{Group: &group, Kind: &kind, Namespace: &ns, Name: gwapi.ObjectName(gw.Name)} +} + +// routeAcceptedCondition reports this controller's Accepted condition for a +// route attached to gw, given the translation errors for the whole table: True +// when every rule translated, or False with the first rejection's reason. It +// lets a route author see acceptance (or why not) on the route's own status, +// as Gateway API expects, rather than only in the controller log. +func routeAcceptedCondition(route *gwapi.HTTPRoute, errs []RouteError) metav1.Condition { + key := route.Namespace + "/" + route.Name + for _, e := range errs { + if e.Route == key || strings.HasPrefix(e.Route, key+"#") { + return routeStatusCondition(route.Generation, false, e.Reason, e.Detail) + } + } + return routeStatusCondition(route.Generation, true, gwapi.RouteReasonAccepted, "accepted by tsjwt") +} + // routeStatusCondition builds the Accepted condition for a route. func routeStatusCondition(gen int64, accepted bool, reason gwapi.RouteConditionReason, msg string) metav1.Condition { status := metav1.ConditionTrue diff --git a/gateway/routestatus_test.go b/gateway/routestatus_test.go new file mode 100644 index 0000000..b7e3af8 --- /dev/null +++ b/gateway/routestatus_test.go @@ -0,0 +1,64 @@ +package gateway_test + +import ( + "context" + "testing" + + "github.com/bitcomplete/tsjwt/gateway" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + gwapi "sigs.k8s.io/gateway-api/apis/v1" +) + +// The controller must report acceptance on each attached route's own status — +// Accepted=True for a route it serves, Accepted=False with the reason for one +// it rejects — so an author sees it without reading controller logs. +// +// Regression for the missing per-HTTPRoute status conditions. +func TestReconcileWritesRouteStatus(t *testing.T) { + t.Parallel() + gc := gatewayClass("tsjwt", gateway.ControllerName) + g := gatewayWithClass("edge", "infra", "tsjwt", "app.ts.net") + + good := route("good", "infra", "edge", []string{"app.ts.net"}, rule("app-svc", 80, "/")) + // A cross-namespace backendRef is rejected with RefNotPermitted. + bad := route("bad", "infra", "edge", []string{"app.ts.net"}, rule("payments", 8080, "/x")) + bad.Spec.Rules[0].BackendRefs[0].Namespace = ptr(gwapi.Namespace("other")) + s := svc("app-svc", "infra", 80) + + r, c := newReconciler(t, gc, g, &good, &bad, &s) + ctx := context.Background() + if _, err := r.Reconcile(ctx, ctrl.Request{ + NamespacedName: types.NamespacedName{Namespace: "infra", Name: "edge"}}); err != nil { + t.Fatalf("reconcile: %v", err) + } + + accepted := func(name string) metav1.Condition { + t.Helper() + var hr gwapi.HTTPRoute + if err := c.Get(ctx, types.NamespacedName{Namespace: "infra", Name: name}, &hr); err != nil { + t.Fatalf("get route %s: %v", name, err) + } + for _, p := range hr.Status.Parents { + if string(p.ControllerName) != gateway.ControllerName { + continue + } + for _, cnd := range p.Conditions { + if cnd.Type == string(gwapi.RouteConditionAccepted) { + return cnd + } + } + } + t.Fatalf("route %s has no Accepted condition from this controller", name) + return metav1.Condition{} + } + + if g := accepted("good"); g.Status != metav1.ConditionTrue { + t.Errorf("good route: Accepted=%s, want True", g.Status) + } + if b := accepted("bad"); b.Status != metav1.ConditionFalse || + b.Reason != string(gwapi.RouteReasonRefNotPermitted) { + t.Errorf("bad route: Accepted=%s reason=%q, want False/RefNotPermitted", b.Status, b.Reason) + } +}