From 222f034a2fa9c1d22af8f3f04efa78819f7ffc91 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Fri, 13 Jun 2025 12:18:28 -0700 Subject: [PATCH 1/8] Add support for backendref service appProtocol --- .../controller/state/conditions/conditions.go | 15 ++ .../controller/state/graph/backend_refs.go | 51 ++++ .../state/graph/backend_refs_test.go | 237 ++++++++++++++++-- .../controller/state/graph/route_common.go | 21 ++ .../state/graph/route_common_test.go | 111 ++++++++ internal/controller/state/graph/tlsroute.go | 16 ++ .../controller/state/graph/tlsroute_test.go | 98 ++++++++ tests/Makefile | 2 +- 8 files changed, 532 insertions(+), 19 deletions(-) diff --git a/internal/controller/state/conditions/conditions.go b/internal/controller/state/conditions/conditions.go index 6630e69a28..35f6509afb 100644 --- a/internal/controller/state/conditions/conditions.go +++ b/internal/controller/state/conditions/conditions.go @@ -33,6 +33,10 @@ const ( // Route rules has a backendRef with an unsupported value. RouteReasonBackendRefUnsupportedValue v1.RouteConditionReason = "UnsupportedValue" + // RouteReasonBackendRefUnsupportedProtocol is used with the "ResolvedRefs" condition when one of the + // Route rules has a backendRef with an unsupported protocol. + RouteReasonBackendRefUnsupportedProtocol v1.RouteConditionReason = "UnsupportedProtocol" + // RouteReasonInvalidGateway is used with the "Accepted" (false) condition when the Gateway the Route // references is invalid. RouteReasonInvalidGateway v1.RouteConditionReason = "InvalidGateway" @@ -423,6 +427,17 @@ func NewRouteBackendRefUnsupportedValue(msg string) Condition { } } +// NewRouteBackendRefUnsupportedProtocol returns a Condition that indicates that the Route has a backendRef with +// an unsupported protocol. +func NewRouteBackendRefUnsupportedProtocol(msg string) Condition { + return Condition{ + Type: string(v1.RouteConditionResolvedRefs), + Status: metav1.ConditionFalse, + Reason: string(RouteReasonBackendRefUnsupportedValue), + Message: msg, + } +} + // NewRouteInvalidGateway returns a Condition that indicates that the Route is not Accepted because the Gateway it // references is invalid. func NewRouteInvalidGateway() Condition { diff --git a/internal/controller/state/graph/backend_refs.go b/internal/controller/state/graph/backend_refs.go index 8f01340b59..97e3c2b6f3 100644 --- a/internal/controller/state/graph/backend_refs.go +++ b/internal/controller/state/graph/backend_refs.go @@ -17,6 +17,12 @@ import ( "github.com/nginx/nginx-gateway-fabric/internal/framework/helpers" ) +const ( + AppProtocolTypeH2C string = "kubernetes.io/h2c" + AppProtocolTypeWS string = "kubernetes.io/ws" + AppProtocolTypeWSS string = "kubernetes.io/wss" +) + // BackendRef is an internal representation of a backendRef in an HTTP/GRPC/TLSRoute. type BackendRef struct { // BackendTLSPolicy is the BackendTLSPolicy of the Service which is referenced by the backendRef. @@ -200,6 +206,33 @@ func createBackendRef( return backendRef, append(conds, conditions.NewRouteBackendRefUnsupportedValue(err.Error())) } + if svcPort.AppProtocol != nil { + valid = validateRouteBackendRefAppProtocol(route.RouteType, *svcPort.AppProtocol, backendTLSPolicy) + if !valid { + backendRef := BackendRef{ + SvcNsName: svcNsName, + BackendTLSPolicy: backendTLSPolicy, + ServicePort: svcPort, + Weight: weight, + Valid: false, + IsMirrorBackend: ref.MirrorBackendIdx != nil, + InvalidForGateways: invalidForGateways, + } + + err := fmt.Errorf( + "route type %s does not support service port appProtocol %s", + route.RouteType, + *svcPort.AppProtocol, + ).Error() + + if route.RouteType == RouteTypeHTTP && *svcPort.AppProtocol == AppProtocolTypeWSS && backendTLSPolicy == nil { + err += "; missing corresponding BackendTLSPolicy" + } + + return backendRef, append(conds, conditions.NewRouteBackendRefUnsupportedProtocol(err)) + } + } + backendRef := BackendRef{ SvcNsName: svcNsName, BackendTLSPolicy: backendTLSPolicy, @@ -414,6 +447,24 @@ func validateBackendRef( return true, conditions.Condition{} } +func validateRouteBackendRefAppProtocol( + routeType RouteType, + appProtocol string, + backendTLSPolicy *BackendTLSPolicy, +) (valid bool) { + // Currently we only support recognition of the Kubernetes Standard Application Protocols defined in KEP-3726. + switch appProtocol { + case AppProtocolTypeH2C: + return routeType == RouteTypeHTTP || routeType == RouteTypeGRPC + case AppProtocolTypeWS: + return routeType == RouteTypeHTTP + case AppProtocolTypeWSS: + return (routeType == RouteTypeHTTP && backendTLSPolicy != nil) || routeType == RouteTypeTLS + } + + return true +} + func validateWeight(weight int32) error { const ( minWeight = 0 diff --git a/internal/controller/state/graph/backend_refs_test.go b/internal/controller/state/graph/backend_refs_test.go index cb42f45daa..9fbbaac7cd 100644 --- a/internal/controller/state/graph/backend_refs_test.go +++ b/internal/controller/state/graph/backend_refs_test.go @@ -390,18 +390,19 @@ func TestAddBackendRefsToRules(t *testing.T) { } createRoute := func( name string, + routeType RouteType, kind gatewayv1.Kind, refsPerBackend int, serviceNames ...string, ) *L7Route { - hr := &L7Route{ + route := &L7Route{ Source: &gatewayv1.HTTPRoute{ ObjectMeta: metav1.ObjectMeta{ Namespace: "test", Name: name, }, }, - RouteType: RouteTypeHTTP, + RouteType: routeType, ParentRefs: sectionNameRefs, Valid: true, } @@ -420,7 +421,7 @@ func TestAddBackendRefsToRules(t *testing.T) { } } - hr.Spec.Rules = make([]RouteRule, len(serviceNames)) + route.Spec.Rules = make([]RouteRule, len(serviceNames)) for idx, svcName := range serviceNames { refs := []RouteBackendRef{ @@ -433,7 +434,7 @@ func TestAddBackendRefsToRules(t *testing.T) { panic("invalid refsPerBackend") } - hr.Spec.Rules[idx] = RouteRule{ + route.Spec.Rules[idx] = RouteRule{ RouteBackendRefs: refs, ValidMatches: true, Filters: RouteRuleFilters{ @@ -442,7 +443,7 @@ func TestAddBackendRefsToRules(t *testing.T) { }, } } - return hr + return route } modRoute := func(route *L7Route, mod func(*L7Route) *L7Route) *L7Route { @@ -467,6 +468,24 @@ func TestAddBackendRefsToRules(t *testing.T) { }, } } + + getSvcWithAppProtocol := func(name, appProtocol string) *v1.Service { + return &v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: name, + }, + Spec: v1.ServiceSpec{ + Ports: []v1.ServicePort{ + { + Port: 80, + AppProtocol: &appProtocol, + }, + }, + }, + } + } + svc1 := getSvc("svc1") svc1NsName := types.NamespacedName{ Namespace: "test", @@ -479,9 +498,37 @@ func TestAddBackendRefsToRules(t *testing.T) { Name: "svc2", } + svcH2c := getSvcWithAppProtocol("svcH2c", AppProtocolTypeH2C) + svcH2cNsName := types.NamespacedName{ + Namespace: "test", + Name: "svcH2c", + } + + svcWS := getSvcWithAppProtocol("svcWS", AppProtocolTypeWS) + svcWSNsName := types.NamespacedName{ + Namespace: "test", + Name: "svcWS", + } + + svcWSS := getSvcWithAppProtocol("svcWSS", AppProtocolTypeWSS) + svcWSSNsName := types.NamespacedName{ + Namespace: "test", + Name: "svcWSS", + } + + svcGRPC := getSvcWithAppProtocol("svcGRPC", "grpc") + svcGRPCNsName := types.NamespacedName{ + Namespace: "test", + Name: "svcGRPC", + } + services := map[types.NamespacedName]*v1.Service{ - {Namespace: "test", Name: "svc1"}: svc1, - {Namespace: "test", Name: "svc2"}: svc2, + {Namespace: "test", Name: "svc1"}: svc1, + {Namespace: "test", Name: "svc2"}: svc2, + {Namespace: "test", Name: "svcH2c"}: svcH2c, + {Namespace: "test", Name: "svcWS"}: svcWS, + {Namespace: "test", Name: "svcWSS"}: svcWSS, + {Namespace: "test", Name: "svcGRPC"}: svcGRPC, } emptyPolicies := map[types.NamespacedName]*BackendTLSPolicy{} @@ -519,8 +566,9 @@ func TestAddBackendRefsToRules(t *testing.T) { } policiesMatching := map[types.NamespacedName]*BackendTLSPolicy{ - {Namespace: "test", Name: "btp1"}: getPolicy("btp1", "svc1", "test"), - {Namespace: "test", Name: "btp2"}: getPolicy("btp2", "svc2", "test"), + {Namespace: "test", Name: "btp1"}: getPolicy("btp1", "svc1", "test"), + {Namespace: "test", Name: "btp2"}: getPolicy("btp2", "svc2", "test"), + {Namespace: "test", Name: "btpWSS"}: getPolicy("btpWSS", "svcWSS", "test"), } policiesNotMatching := map[types.NamespacedName]*BackendTLSPolicy{ {Namespace: "test", Name: "btp1"}: getPolicy("btp1", "svc1", "test1"), @@ -576,6 +624,7 @@ func TestAddBackendRefsToRules(t *testing.T) { Message: "Policy is accepted", }, ) + btpWSS := getBtp("btpWSS", "svcWSS", "test") tests := []struct { route *L7Route @@ -585,7 +634,7 @@ func TestAddBackendRefsToRules(t *testing.T) { expectedConditions []conditions.Condition }{ { - route: createRoute("hr1", "Service", 1, "svc1"), + route: createRoute("hr1", RouteTypeHTTP, "Service", 1, "svc1"), expectedBackendRefs: []BackendRef{ { SvcNsName: svc1NsName, @@ -600,7 +649,7 @@ func TestAddBackendRefsToRules(t *testing.T) { name: "normal case with one rule with one backend", }, { - route: createRoute("hr2", "Service", 2, "svc1"), + route: createRoute("hr2", RouteTypeHTTP, "Service", 2, "svc1"), expectedBackendRefs: []BackendRef{ { SvcNsName: svc1NsName, @@ -622,7 +671,7 @@ func TestAddBackendRefsToRules(t *testing.T) { name: "normal case with one rule with two backends", }, { - route: createRoute("hr2", "Service", 2, "svc1"), + route: createRoute("hr2", RouteTypeHTTP, "Service", 2, "svc1"), expectedBackendRefs: []BackendRef{ { SvcNsName: svc1NsName, @@ -646,7 +695,159 @@ func TestAddBackendRefsToRules(t *testing.T) { name: "normal case with one rule with two backends and matching policies", }, { - route: modRoute(createRoute("hr1", "Service", 1, "svc1"), func(route *L7Route) *L7Route { + route: createRoute("hr1", RouteTypeHTTP, "Service", 1, "svcH2c"), + expectedBackendRefs: []BackendRef{ + { + SvcNsName: svcH2cNsName, + ServicePort: svcH2c.Spec.Ports[0], + Valid: true, + Weight: 1, + InvalidForGateways: map[types.NamespacedName]conditions.Condition{}, + }, + }, + expectedConditions: nil, + policies: emptyPolicies, + name: "valid backendRef with service port appProtocol h2c and route type http", + }, + { + route: createRoute("hr1", RouteTypeHTTP, "Service", 1, "svcWS"), + expectedBackendRefs: []BackendRef{ + { + SvcNsName: svcWSNsName, + ServicePort: svcWS.Spec.Ports[0], + Valid: true, + Weight: 1, + InvalidForGateways: map[types.NamespacedName]conditions.Condition{}, + }, + }, + expectedConditions: nil, + policies: emptyPolicies, + name: "valid backendRef with service port appProtocol ws and route type http", + }, + { + route: createRoute("hr1", RouteTypeHTTP, "Service", 1, "svcWSS"), + expectedBackendRefs: []BackendRef{ + { + SvcNsName: svcWSSNsName, + ServicePort: svcWSS.Spec.Ports[0], + Valid: true, + Weight: 1, + BackendTLSPolicy: btpWSS, + InvalidForGateways: map[types.NamespacedName]conditions.Condition{}, + }, + }, + expectedConditions: nil, + policies: policiesMatching, + name: "valid backendRef with service port appProtocol wss," + + " route type http, and corresponding BackendTLSPolicy", + }, + { + route: createRoute("hr1", RouteTypeHTTP, "Service", 1, "svcWSS"), + expectedBackendRefs: []BackendRef{ + { + SvcNsName: svcWSSNsName, + ServicePort: svcWSS.Spec.Ports[0], + Valid: false, + Weight: 1, + InvalidForGateways: map[types.NamespacedName]conditions.Condition{}, + }, + }, + expectedConditions: []conditions.Condition{ + conditions.NewRouteBackendRefUnsupportedProtocol( + "route type http does not support service port appProtocol kubernetes.io/wss;" + + " missing corresponding BackendTLSPolicy", + ), + }, + policies: emptyPolicies, + name: "invalid backendRef with service port appProtocol wss, route type http, but missing BackendTLSPolicy", + }, + { + route: createRoute("gr1", RouteTypeGRPC, "Service", 1, "svcH2c"), + expectedBackendRefs: []BackendRef{ + { + SvcNsName: svcH2cNsName, + ServicePort: svcH2c.Spec.Ports[0], + Valid: true, + Weight: 1, + InvalidForGateways: map[types.NamespacedName]conditions.Condition{}, + }, + }, + expectedConditions: nil, + policies: emptyPolicies, + name: "valid backendRef with service port appProtocol h2c and route type grpc", + }, + { + route: createRoute("gr1", RouteTypeGRPC, "Service", 1, "svcWS"), + expectedBackendRefs: []BackendRef{ + { + SvcNsName: svcWSNsName, + ServicePort: svcWS.Spec.Ports[0], + Valid: false, + Weight: 1, + InvalidForGateways: map[types.NamespacedName]conditions.Condition{}, + }, + }, + expectedConditions: []conditions.Condition{ + conditions.NewRouteBackendRefUnsupportedProtocol( + "route type grpc does not support service port appProtocol kubernetes.io/ws", + ), + }, + policies: emptyPolicies, + name: "invalid backendRef with service port appProtocol ws and route type grpc", + }, + { + route: createRoute("gr1", RouteTypeGRPC, "Service", 1, "svcWSS"), + expectedBackendRefs: []BackendRef{ + { + SvcNsName: svcWSSNsName, + ServicePort: svcWSS.Spec.Ports[0], + Valid: false, + Weight: 1, + InvalidForGateways: map[types.NamespacedName]conditions.Condition{}, + }, + }, + expectedConditions: []conditions.Condition{ + conditions.NewRouteBackendRefUnsupportedProtocol( + "route type grpc does not support service port appProtocol kubernetes.io/wss", + ), + }, + policies: emptyPolicies, + name: "invalid backendRef with service port appProtocol wss and route type grpc", + }, + { + route: createRoute("hr1", RouteTypeHTTP, "Service", 1, "svcGRPC"), + expectedBackendRefs: []BackendRef{ + { + SvcNsName: svcGRPCNsName, + ServicePort: svcGRPC.Spec.Ports[0], + Valid: true, + Weight: 1, + InvalidForGateways: map[types.NamespacedName]conditions.Condition{}, + }, + }, + expectedConditions: nil, + policies: emptyPolicies, + name: "valid backendRef with non-Kubernetes Standard Application Protocol" + + " service port appProtocol and route type http", + }, + { + route: createRoute("gr1", RouteTypeGRPC, "Service", 1, "svcGRPC"), + expectedBackendRefs: []BackendRef{ + { + SvcNsName: svcGRPCNsName, + ServicePort: svcGRPC.Spec.Ports[0], + Valid: true, + Weight: 1, + InvalidForGateways: map[types.NamespacedName]conditions.Condition{}, + }, + }, + expectedConditions: nil, + policies: emptyPolicies, + name: "valid backendRef with non-Kubernetes Standard Application Protocol" + + " service port appProtocol and route type grpc", + }, + { + route: modRoute(createRoute("hr1", RouteTypeHTTP, "Service", 1, "svc1"), func(route *L7Route) *L7Route { route.Valid = false return route }), @@ -656,7 +857,7 @@ func TestAddBackendRefsToRules(t *testing.T) { name: "invalid route", }, { - route: modRoute(createRoute("hr1", "Service", 1, "svc1"), func(route *L7Route) *L7Route { + route: modRoute(createRoute("hr1", RouteTypeHTTP, "Service", 1, "svc1"), func(route *L7Route) *L7Route { route.Spec.Rules[0].ValidMatches = false return route }), @@ -666,7 +867,7 @@ func TestAddBackendRefsToRules(t *testing.T) { name: "invalid matches", }, { - route: modRoute(createRoute("hr1", "Service", 1, "svc1"), func(route *L7Route) *L7Route { + route: modRoute(createRoute("hr1", RouteTypeHTTP, "Service", 1, "svc1"), func(route *L7Route) *L7Route { route.Spec.Rules[0].Filters = RouteRuleFilters{Valid: false} return route }), @@ -676,7 +877,7 @@ func TestAddBackendRefsToRules(t *testing.T) { name: "invalid filters", }, { - route: createRoute("hr3", "NotService", 1, "svc1"), + route: createRoute("hr3", RouteTypeHTTP, "NotService", 1, "svc1"), expectedBackendRefs: []BackendRef{ { Weight: 1, @@ -692,7 +893,7 @@ func TestAddBackendRefsToRules(t *testing.T) { name: "invalid backendRef", }, { - route: modRoute(createRoute("hr2", "Service", 2, "svc1"), func(route *L7Route) *L7Route { + route: modRoute(createRoute("hr2", RouteTypeHTTP, "Service", 2, "svc1"), func(route *L7Route) *L7Route { route.Spec.Rules[0].RouteBackendRefs[1].Name = "svc2" return route }), @@ -723,7 +924,7 @@ func TestAddBackendRefsToRules(t *testing.T) { name: "invalid backendRef - backend TLS policies do not match for all backends", }, { - route: modRoute(createRoute("hr4", "Service", 1, "svc1"), func(route *L7Route) *L7Route { + route: modRoute(createRoute("hr4", RouteTypeHTTP, "Service", 1, "svc1"), func(route *L7Route) *L7Route { route.Spec.Rules[0].RouteBackendRefs = nil return route }), diff --git a/internal/controller/state/graph/route_common.go b/internal/controller/state/graph/route_common.go index 1cbb2fc480..86e65bfb1c 100644 --- a/internal/controller/state/graph/route_common.go +++ b/internal/controller/state/graph/route_common.go @@ -72,6 +72,8 @@ const ( RouteTypeHTTP RouteType = "http" // RouteTypeGRPC indicates that the RouteType of the L7Route is gRPC. RouteTypeGRPC RouteType = "grpc" + // RouteTypeTLS indicates that the RouteType of the L4Route is TLS. + RouteTypeTLS RouteType = "tls" ) // L4RouteKey is the unique identifier for a L4Route. @@ -769,6 +771,8 @@ func bindL7RouteToListeners( if cond, ok := backendRef.InvalidForGateways[gwNsName]; ok { attachment.FailedConditions = append(attachment.FailedConditions, cond) } + + checkAppProtocolH2CConditional(backendRef, gw.EffectiveNginxProxy, route) } } @@ -793,6 +797,23 @@ func bindL7RouteToListeners( } } +func checkAppProtocolH2CConditional(backendRef BackendRef, npCfg *EffectiveNginxProxy, route *L7Route) { + // For all backendRefs referring to a Service with appProtocol h2c, we need to also check if http2 + // is enabled. Don't need to check for RouteTypeGRPC since there are checks before this which fail the + // route from connecting to the Gateway. + if backendRef.ServicePort.AppProtocol != nil && + *backendRef.ServicePort.AppProtocol == AppProtocolTypeH2C && + route.RouteType == RouteTypeHTTP && + isHTTP2Disabled(npCfg) { + backendRef.Valid = false + route.Conditions = append(route.Conditions, conditions.NewRouteBackendRefUnsupportedProtocol( + fmt.Errorf("HTTP2 is disabled - cannot support appProtocol h2c on route type %s", + route.RouteType, + ).Error(), + )) + } +} + func isHTTP2Disabled(npCfg *EffectiveNginxProxy) bool { if npCfg == nil { return false diff --git a/internal/controller/state/graph/route_common_test.go b/internal/controller/state/graph/route_common_test.go index 4b4fd1c2fe..9684e5e96e 100644 --- a/internal/controller/state/graph/route_common_test.go +++ b/internal/controller/state/graph/route_common_test.go @@ -394,6 +394,34 @@ func TestBindRouteToListeners(t *testing.T) { return normalHTTPRoute } + normalHTTPRouteWithH2CBackendRef := &L7Route{ + RouteType: RouteTypeHTTP, + Source: hr, + Spec: L7RouteSpec{ + Hostnames: hr.Spec.Hostnames, + Rules: []RouteRule{ + { + BackendRefs: []BackendRef{ + { + ServicePort: v1.ServicePort{ + AppProtocol: helpers.GetPointer(AppProtocolTypeH2C), + }, + }, + }, + }, + }, + }, + Valid: true, + Attachable: true, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: &ParentRefGateway{NamespacedName: client.ObjectKeyFromObject(gw)}, + SectionName: hr.Spec.ParentRefs[0].SectionName, + }, + }, + } + getLastNormalHTTPRoute := func() *L7Route { return normalHTTPRoute } @@ -610,6 +638,89 @@ func TestBindRouteToListeners(t *testing.T) { }, name: "normal case", }, + { + route: normalHTTPRouteWithH2CBackendRef, + gateway: &Gateway{ + Source: gw, + Valid: true, + Listeners: []*Listener{ + createListener("listener-80-1"), + }, + EffectiveNginxProxy: &EffectiveNginxProxy{ + DisableHTTP2: helpers.GetPointer(false), + }, + }, + expectedSectionNameRefs: []ParentRef{ + { + Idx: 0, + Gateway: &ParentRefGateway{NamespacedName: client.ObjectKeyFromObject(gw)}, + SectionName: hr.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + Attached: true, + AcceptedHostnames: map[string][]string{ + CreateGatewayListenerKey( + client.ObjectKeyFromObject(gw), + "listener-80-1", + ): {"foo.example.com"}, + }, + }, + }, + }, + expectedGatewayListeners: []*Listener{ + createModifiedListener("listener-80-1", func(l *Listener) { + l.Routes = map[RouteKey]*L7Route{ + CreateRouteKey(hr): normalHTTPRouteWithH2CBackendRef, + } + }), + }, + name: "httpRoute with h2c service port protocol in backend and h2c is enabled", + }, + { + route: normalHTTPRouteWithH2CBackendRef, + gateway: &Gateway{ + Source: gw, + Valid: true, + Listeners: []*Listener{ + createListener("listener-80-1"), + }, + EffectiveNginxProxy: &EffectiveNginxProxy{ + DisableHTTP2: helpers.GetPointer(true), + }, + }, + expectedSectionNameRefs: []ParentRef{ + { + Idx: 0, + Gateway: &ParentRefGateway{NamespacedName: client.ObjectKeyFromObject(gw)}, + SectionName: hr.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + Attached: true, + AcceptedHostnames: map[string][]string{ + CreateGatewayListenerKey( + client.ObjectKeyFromObject(gw), + "listener-80-1", + ): {"foo.example.com"}, + }, + }, + }, + }, + expectedGatewayListeners: []*Listener{ + createModifiedListener("listener-80-1", func(l *Listener) { + route := *normalHTTPRouteWithH2CBackendRef + route.Conditions = []conditions.Condition{ + conditions.NewRouteBackendRefUnsupportedProtocol( + "HTTP2 is disabled - cannot support appProtocol h2c on route type http"), + } + l.Routes = map[RouteKey]*L7Route{ + CreateRouteKey(hr): &route, + } + }), + }, + expectedConditions: []conditions.Condition{ + conditions.NewRouteBackendRefUnsupportedProtocol( + "HTTP2 is disabled - cannot support appProtocol h2c on route type http"), + }, + name: "httpRoute with h2c service port protocol in backend and h2c is disabled", + }, { route: routeWithMissingSectionName, gateway: &Gateway{ diff --git a/internal/controller/state/graph/tlsroute.go b/internal/controller/state/graph/tlsroute.go index 2cec3ffb3d..ee58f36de0 100644 --- a/internal/controller/state/graph/tlsroute.go +++ b/internal/controller/state/graph/tlsroute.go @@ -1,6 +1,8 @@ package graph import ( + "fmt" + apiv1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation/field" @@ -119,6 +121,20 @@ func validateBackendRefTLSRoute( return backendRef, []conditions.Condition{conditions.NewRouteBackendRefRefBackendNotFound(err.Error())} } + if svcPort.AppProtocol != nil { + valid := validateRouteBackendRefAppProtocol(RouteTypeTLS, *svcPort.AppProtocol, nil) + if !valid { + backendRef.Valid = false + + return backendRef, []conditions.Condition{conditions.NewRouteBackendRefUnsupportedProtocol( + fmt.Errorf( + "route type %s does not support service port appProtocol %s", + RouteTypeTLS, + *svcPort.AppProtocol, + ).Error())} + } + } + var conds []conditions.Condition for _, parentRef := range parentRefs { if err := verifyIPFamily(parentRef.Gateway.EffectiveNginxProxy, svcIPFamily); err != nil { diff --git a/internal/controller/state/graph/tlsroute_test.go b/internal/controller/state/graph/tlsroute_test.go index aaeb5a3698..4a3e7b450e 100644 --- a/internal/controller/state/graph/tlsroute_test.go +++ b/internal/controller/state/graph/tlsroute_test.go @@ -254,6 +254,20 @@ func TestBuildTLSRoute(t *testing.T) { } } + createSvcWithAppProtocol := func(name, appProtocol string, port int32) *apiv1.Service { + return &apiv1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: name, + }, + Spec: apiv1.ServiceSpec{ + Ports: []apiv1.ServicePort{ + {Port: port, AppProtocol: &appProtocol}, + }, + }, + } + } + diffNsSvc := &apiv1.Service{ ObjectMeta: metav1.ObjectMeta{ Namespace: "diff", @@ -350,6 +364,64 @@ func TestBuildTLSRoute(t *testing.T) { resolver: alwaysTrueRefGrantResolver, name: "invalid rule", }, + { + gtr: validRefSameNs, + expected: &L4Route{ + Source: validRefSameNs, + ParentRefs: []ParentRef{parentRefGraph}, + Spec: L4RouteSpec{ + Hostnames: []gatewayv1.Hostname{ + "app.example.com", + }, + BackendRef: BackendRef{ + SvcNsName: svcNsName, + ServicePort: apiv1.ServicePort{Port: 80, AppProtocol: helpers.GetPointer(AppProtocolTypeH2C)}, + Valid: false, + InvalidForGateways: map[types.NamespacedName]conditions.Condition{}, + }, + }, + Attachable: true, + Valid: true, + Conditions: []conditions.Condition{conditions.NewRouteBackendRefUnsupportedProtocol( + "route type tls does not support service port appProtocol kubernetes.io/h2c", + )}, + }, + gateway: createGateway(), + services: map[types.NamespacedName]*apiv1.Service{ + svcNsName: createSvcWithAppProtocol("hi", AppProtocolTypeH2C, 80), + }, + resolver: alwaysTrueRefGrantResolver, + name: "invalid service port appProtocol h2c", + }, + { + gtr: validRefSameNs, + expected: &L4Route{ + Source: validRefSameNs, + ParentRefs: []ParentRef{parentRefGraph}, + Spec: L4RouteSpec{ + Hostnames: []gatewayv1.Hostname{ + "app.example.com", + }, + BackendRef: BackendRef{ + SvcNsName: svcNsName, + ServicePort: apiv1.ServicePort{Port: 80, AppProtocol: helpers.GetPointer(AppProtocolTypeWS)}, + Valid: false, + InvalidForGateways: map[types.NamespacedName]conditions.Condition{}, + }, + }, + Attachable: true, + Valid: true, + Conditions: []conditions.Condition{conditions.NewRouteBackendRefUnsupportedProtocol( + "route type tls does not support service port appProtocol kubernetes.io/ws", + )}, + }, + gateway: createGateway(), + services: map[types.NamespacedName]*apiv1.Service{ + svcNsName: createSvcWithAppProtocol("hi", AppProtocolTypeWS, 80), + }, + resolver: alwaysTrueRefGrantResolver, + name: "invalid service port appProtocol WS", + }, { gtr: backedRefDNEGtr, expected: &L4Route{ @@ -586,6 +658,32 @@ func TestBuildTLSRoute(t *testing.T) { resolver: alwaysTrueRefGrantResolver, name: "valid; same namespace", }, + { + gtr: validRefSameNs, + expected: &L4Route{ + Source: validRefSameNs, + ParentRefs: []ParentRef{parentRefGraph}, + Spec: L4RouteSpec{ + Hostnames: []gatewayv1.Hostname{ + "app.example.com", + }, + BackendRef: BackendRef{ + SvcNsName: svcNsName, + ServicePort: apiv1.ServicePort{Port: 80, AppProtocol: helpers.GetPointer(AppProtocolTypeWSS)}, + Valid: true, + InvalidForGateways: map[types.NamespacedName]conditions.Condition{}, + }, + }, + Attachable: true, + Valid: true, + }, + gateway: createGateway(), + services: map[types.NamespacedName]*apiv1.Service{ + svcNsName: createSvcWithAppProtocol("hi", AppProtocolTypeWSS, 80), + }, + resolver: alwaysTrueRefGrantResolver, + name: "valid; same namespace, valid appProtocol", + }, } for _, test := range tests { diff --git a/tests/Makefile b/tests/Makefile index 6db161b3b2..6b1f4615ec 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -12,7 +12,7 @@ GW_SERVICE_TYPE = NodePort## Service type to use for the gateway NGF_VERSION ?= edge## NGF version to be tested PULL_POLICY = Never## Pull policy for the images NGINX_CONF_DIR = internal/controller/nginx/conf -SUPPORTED_EXTENDED_FEATURES = HTTPRouteQueryParamMatching,HTTPRouteMethodMatching,HTTPRoutePortRedirect,HTTPRouteSchemeRedirect,HTTPRouteHostRewrite,HTTPRoutePathRewrite,GatewayPort8080,HTTPRouteResponseHeaderModification,HTTPRoutePathRedirect,GatewayHTTPListenerIsolation,GatewayInfrastructurePropagation,HTTPRouteRequestMirror,HTTPRouteRequestMultipleMirrors +SUPPORTED_EXTENDED_FEATURES = HTTPRouteQueryParamMatching,HTTPRouteMethodMatching,HTTPRoutePortRedirect,HTTPRouteSchemeRedirect,HTTPRouteHostRewrite,HTTPRoutePathRewrite,GatewayPort8080,HTTPRouteResponseHeaderModification,HTTPRoutePathRedirect,GatewayHTTPListenerIsolation,GatewayInfrastructurePropagation,HTTPRouteRequestMirror,HTTPRouteRequestMultipleMirrors,HTTPRouteBackendProtocolWebSocket,HTTPRouteBackendProtocolH2C STANDARD_CONFORMANCE_PROFILES = GATEWAY-HTTP,GATEWAY-GRPC EXPERIMENTAL_CONFORMANCE_PROFILES = GATEWAY-TLS CONFORMANCE_PROFILES = $(STANDARD_CONFORMANCE_PROFILES) # by default we use the standard conformance profiles. If experimental is enabled we override this and add the experimental profiles. From 1e713ab8aadbc878b3cd150166429fce0837c772 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Fri, 13 Jun 2025 12:46:36 -0700 Subject: [PATCH 2/8] Fix condition --- internal/controller/state/conditions/conditions.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/state/conditions/conditions.go b/internal/controller/state/conditions/conditions.go index 35f6509afb..afa55b69a0 100644 --- a/internal/controller/state/conditions/conditions.go +++ b/internal/controller/state/conditions/conditions.go @@ -433,7 +433,7 @@ func NewRouteBackendRefUnsupportedProtocol(msg string) Condition { return Condition{ Type: string(v1.RouteConditionResolvedRefs), Status: metav1.ConditionFalse, - Reason: string(RouteReasonBackendRefUnsupportedValue), + Reason: string(RouteReasonBackendRefUnsupportedProtocol), Message: msg, } } From 7687c18e08936921b2698a581953abf03b614fe7 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Mon, 16 Jun 2025 10:42:41 -0700 Subject: [PATCH 3/8] Update service changed predicate --- internal/controller/manager.go | 2 +- .../framework/controller/predicate/service.go | 41 +++-- .../controller/predicate/service_test.go | 144 +++++++++++++++++- .../framework/controller/register_test.go | 2 +- 4 files changed, 173 insertions(+), 16 deletions(-) diff --git a/internal/controller/manager.go b/internal/controller/manager.go index f7bc0a68e5..a8a1afe99b 100644 --- a/internal/controller/manager.go +++ b/internal/controller/manager.go @@ -440,7 +440,7 @@ func registerControllers( objectType: &apiv1.Service{}, name: "user-service", // unique controller names are needed and we have multiple Service ctlrs options: []controller.Option{ - controller.WithK8sPredicate(predicate.ServicePortsChangedPredicate{}), + controller.WithK8sPredicate(predicate.ServiceChangedPredicate{}), }, }, { diff --git a/internal/framework/controller/predicate/service.go b/internal/framework/controller/predicate/service.go index 21e59e6ee0..8795706da9 100644 --- a/internal/framework/controller/predicate/service.go +++ b/internal/framework/controller/predicate/service.go @@ -7,20 +7,21 @@ import ( "sigs.k8s.io/controller-runtime/pkg/predicate" ) -// ServicePortsChangedPredicate implements an update predicate function based on the Ports of a Service. -// This predicate will skip update events that have no change in the Service Ports and TargetPorts. -type ServicePortsChangedPredicate struct { +// ServiceChangedPredicate implements an update predicate function for a Service. +// This predicate will skip update events that have no change in a Service's Ports, TargetPorts, or AppProtocols. +type ServiceChangedPredicate struct { predicate.Funcs } -// ports contains the ports that the Gateway cares about. -type ports struct { +// portInfo contains the information that the Gateway cares about. +type portInfo struct { targetPort intstr.IntOrString + appProtocol string servicePort int32 } -// Update implements default UpdateEvent filter for validating Service port changes. -func (ServicePortsChangedPredicate) Update(e event.UpdateEvent) bool { +// Update implements default UpdateEvent filter for validating Service port information changes. +func (ServiceChangedPredicate) Update(e event.UpdateEvent) bool { if e.ObjectOld == nil { return false } @@ -45,12 +46,30 @@ func (ServicePortsChangedPredicate) Update(e event.UpdateEvent) bool { return true } - oldPortSet := make(map[ports]struct{}) - newPortSet := make(map[ports]struct{}) + oldPortSet := make(map[portInfo]struct{}) + newPortSet := make(map[portInfo]struct{}) for i := range len(oldSvc.Spec.Ports) { - oldPortSet[ports{servicePort: oldPorts[i].Port, targetPort: oldPorts[i].TargetPort}] = struct{}{} - newPortSet[ports{servicePort: newPorts[i].Port, targetPort: newPorts[i].TargetPort}] = struct{}{} + var oldAppProtocol, newAppProtocl string + + if oldPorts[i].AppProtocol != nil { + oldAppProtocol = *oldPorts[i].AppProtocol + } + + if newPorts[i].AppProtocol != nil { + newAppProtocl = *newPorts[i].AppProtocol + } + + oldPortSet[portInfo{ + servicePort: oldPorts[i].Port, + targetPort: oldPorts[i].TargetPort, + appProtocol: oldAppProtocol, + }] = struct{}{} + newPortSet[portInfo{ + servicePort: newPorts[i].Port, + targetPort: newPorts[i].TargetPort, + appProtocol: newAppProtocl, + }] = struct{}{} } for pd := range oldPortSet { diff --git a/internal/framework/controller/predicate/service_test.go b/internal/framework/controller/predicate/service_test.go index fcb4aa694f..abb07c27c5 100644 --- a/internal/framework/controller/predicate/service_test.go +++ b/internal/framework/controller/predicate/service_test.go @@ -8,9 +8,11 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/event" + + "github.com/nginx/nginx-gateway-fabric/internal/framework/helpers" ) -func TestServicePortsChangedPredicate_Update(t *testing.T) { +func TestServiceChangedPredicate_Update(t *testing.T) { t.Parallel() testcases := []struct { objectOld client.Object @@ -221,9 +223,145 @@ func TestServicePortsChangedPredicate_Update(t *testing.T) { }, expUpdate: false, }, + { + msg: "appProtocol changed", + objectOld: &v1.Service{ + Spec: v1.ServiceSpec{ + Ports: []v1.ServicePort{ + { + AppProtocol: helpers.GetPointer("oldAppProtocol"), + }, + }, + }, + }, + objectNew: &v1.Service{ + Spec: v1.ServiceSpec{ + Ports: []v1.ServicePort{ + { + AppProtocol: helpers.GetPointer("newAppProtocol"), + }, + }, + }, + }, + expUpdate: true, + }, + { + msg: "appProtocol stayed the same", + objectOld: &v1.Service{ + Spec: v1.ServiceSpec{ + Ports: []v1.ServicePort{ + { + AppProtocol: helpers.GetPointer("sameAppProtocol"), + }, + }, + }, + }, + objectNew: &v1.Service{ + Spec: v1.ServiceSpec{ + Ports: []v1.ServicePort{ + { + AppProtocol: helpers.GetPointer("sameAppProtocol"), + }, + }, + }, + }, + expUpdate: false, + }, + { + msg: "multiple appProtocols stayed the same", + objectOld: &v1.Service{ + Spec: v1.ServiceSpec{ + Ports: []v1.ServicePort{ + { + Port: 80, + TargetPort: intstr.FromInt(80), + AppProtocol: helpers.GetPointer("sameAppProtocol80"), + }, + { + Port: 81, + TargetPort: intstr.FromInt(81), + AppProtocol: helpers.GetPointer("sameAppProtocol81"), + }, + { + Port: 82, + TargetPort: intstr.FromInt(82), + AppProtocol: helpers.GetPointer("sameAppProtocol82"), + }, + }, + }, + }, + objectNew: &v1.Service{ + Spec: v1.ServiceSpec{ + Ports: []v1.ServicePort{ + { + Port: 80, + TargetPort: intstr.FromInt(80), + AppProtocol: helpers.GetPointer("sameAppProtocol80"), + }, + { + Port: 81, + TargetPort: intstr.FromInt(81), + AppProtocol: helpers.GetPointer("sameAppProtocol81"), + }, + { + Port: 82, + TargetPort: intstr.FromInt(82), + AppProtocol: helpers.GetPointer("sameAppProtocol82"), + }, + }, + }, + }, + expUpdate: false, + }, + { + msg: "multiple appProtocols with one changing", + objectOld: &v1.Service{ + Spec: v1.ServiceSpec{ + Ports: []v1.ServicePort{ + { + Port: 80, + TargetPort: intstr.FromInt(80), + AppProtocol: helpers.GetPointer("sameAppProtocol80"), + }, + { + Port: 81, + TargetPort: intstr.FromInt(81), + AppProtocol: helpers.GetPointer("sameAppProtocol81"), + }, + { + Port: 82, + TargetPort: intstr.FromInt(82), + AppProtocol: helpers.GetPointer("sameAppProtocol82"), + }, + }, + }, + }, + objectNew: &v1.Service{ + Spec: v1.ServiceSpec{ + Ports: []v1.ServicePort{ + { + Port: 80, + TargetPort: intstr.FromInt(80), + AppProtocol: helpers.GetPointer("sameAppProtocol80"), + }, + { + Port: 81, + TargetPort: intstr.FromInt(81), + AppProtocol: helpers.GetPointer("sameAppProtocol81"), + }, + { + Port: 82, + TargetPort: intstr.FromInt(82), + AppProtocol: helpers.GetPointer("differentAppProtocol"), + }, + }, + }, + }, + expUpdate: true, + }, } - p := ServicePortsChangedPredicate{} + p := ServiceChangedPredicate{} for _, tc := range testcases { t.Run(tc.msg, func(t *testing.T) { @@ -243,7 +381,7 @@ func TestServicePortsChangedPredicate(t *testing.T) { t.Parallel() g := NewWithT(t) - p := ServicePortsChangedPredicate{} + p := ServiceChangedPredicate{} g.Expect(p.Delete(event.DeleteEvent{Object: &v1.Service{}})).To(BeTrue()) g.Expect(p.Create(event.CreateEvent{Object: &v1.Service{}})).To(BeTrue()) diff --git a/internal/framework/controller/register_test.go b/internal/framework/controller/register_test.go index 115b737737..e8a20efe3c 100644 --- a/internal/framework/controller/register_test.go +++ b/internal/framework/controller/register_test.go @@ -142,7 +142,7 @@ func TestRegister(t *testing.T) { test.fakes.mgr, eventCh, controller.WithNamespacedNameFilter(nsNameFilter), - controller.WithK8sPredicate(predicate.ServicePortsChangedPredicate{}), + controller.WithK8sPredicate(predicate.ServiceChangedPredicate{}), controller.WithFieldIndices(fieldIndexes), controller.WithNewReconciler(newReconciler), controller.WithOnlyMetadata(), From 5fac23461960f900849c7bab4efa9ccfca65e85f Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Mon, 16 Jun 2025 16:46:53 -0700 Subject: [PATCH 4/8] Correct h2c conditional check --- internal/controller/state/graph/route_common.go | 16 +++++++++++----- .../controller/state/graph/route_common_test.go | 16 ++++++++++++++-- tests/Makefile | 2 +- 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/internal/controller/state/graph/route_common.go b/internal/controller/state/graph/route_common.go index 86e65bfb1c..15392f18dc 100644 --- a/internal/controller/state/graph/route_common.go +++ b/internal/controller/state/graph/route_common.go @@ -766,13 +766,13 @@ func bindL7RouteToListeners( continue } - for _, rule := range route.Spec.Rules { - for _, backendRef := range rule.BackendRefs { + for ruleIdx, rule := range route.Spec.Rules { + for backendRefIdx, backendRef := range rule.BackendRefs { if cond, ok := backendRef.InvalidForGateways[gwNsName]; ok { attachment.FailedConditions = append(attachment.FailedConditions, cond) } - checkAppProtocolH2CConditional(backendRef, gw.EffectiveNginxProxy, route) + checkAppProtocolH2CConditional(backendRef, gw.EffectiveNginxProxy, route, ruleIdx, backendRefIdx) } } @@ -797,7 +797,13 @@ func bindL7RouteToListeners( } } -func checkAppProtocolH2CConditional(backendRef BackendRef, npCfg *EffectiveNginxProxy, route *L7Route) { +func checkAppProtocolH2CConditional( + backendRef BackendRef, + npCfg *EffectiveNginxProxy, + route *L7Route, + ruleIdx, + backendRefIdx int, +) { // For all backendRefs referring to a Service with appProtocol h2c, we need to also check if http2 // is enabled. Don't need to check for RouteTypeGRPC since there are checks before this which fail the // route from connecting to the Gateway. @@ -805,7 +811,7 @@ func checkAppProtocolH2CConditional(backendRef BackendRef, npCfg *EffectiveNginx *backendRef.ServicePort.AppProtocol == AppProtocolTypeH2C && route.RouteType == RouteTypeHTTP && isHTTP2Disabled(npCfg) { - backendRef.Valid = false + route.Spec.Rules[ruleIdx].BackendRefs[backendRefIdx].Valid = false route.Conditions = append(route.Conditions, conditions.NewRouteBackendRefUnsupportedProtocol( fmt.Errorf("HTTP2 is disabled - cannot support appProtocol h2c on route type %s", route.RouteType, diff --git a/internal/controller/state/graph/route_common_test.go b/internal/controller/state/graph/route_common_test.go index 9684e5e96e..955a326ab2 100644 --- a/internal/controller/state/graph/route_common_test.go +++ b/internal/controller/state/graph/route_common_test.go @@ -403,6 +403,7 @@ func TestBindRouteToListeners(t *testing.T) { { BackendRefs: []BackendRef{ { + Valid: true, ServicePort: v1.ServicePort{ AppProtocol: helpers.GetPointer(AppProtocolTypeH2C), }, @@ -422,6 +423,9 @@ func TestBindRouteToListeners(t *testing.T) { }, } + invalidBackendRefH2c := *normalHTTPRouteWithH2CBackendRef + invalidBackendRefH2c.Spec.Rules[0].BackendRefs[0].Valid = false + getLastNormalHTTPRoute := func() *L7Route { return normalHTTPRoute } @@ -599,8 +603,9 @@ func TestBindRouteToListeners(t *testing.T) { tests := []struct { route *L7Route gateway *Gateway - expectedGatewayListeners []*Listener + expectedModifiedRoute *L7Route name string + expectedGatewayListeners []*Listener expectedSectionNameRefs []ParentRef expectedConditions []conditions.Condition }{ @@ -719,7 +724,8 @@ func TestBindRouteToListeners(t *testing.T) { conditions.NewRouteBackendRefUnsupportedProtocol( "HTTP2 is disabled - cannot support appProtocol h2c on route type http"), }, - name: "httpRoute with h2c service port protocol in backend and h2c is disabled", + expectedModifiedRoute: &invalidBackendRefH2c, + name: "httpRoute with h2c service port protocol in backend and h2c is disabled", }, { route: routeWithMissingSectionName, @@ -1509,6 +1515,12 @@ func TestBindRouteToListeners(t *testing.T) { g.Expect(test.route.ParentRefs).To(Equal(test.expectedSectionNameRefs)) g.Expect(helpers.Diff(test.gateway.Listeners, test.expectedGatewayListeners)).To(BeEmpty()) g.Expect(helpers.Diff(test.route.Conditions, test.expectedConditions)).To(BeEmpty()) + + // in situations where bindRouteToListeners modifies the route spec, for instance marking a backendRef + // as invalid + if test.expectedModifiedRoute != nil { + g.Expect(helpers.Diff(test.route.Spec, test.expectedModifiedRoute.Spec)).To(BeEmpty()) + } }) } } diff --git a/tests/Makefile b/tests/Makefile index 6b1f4615ec..ce03c4bd2a 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -12,7 +12,7 @@ GW_SERVICE_TYPE = NodePort## Service type to use for the gateway NGF_VERSION ?= edge## NGF version to be tested PULL_POLICY = Never## Pull policy for the images NGINX_CONF_DIR = internal/controller/nginx/conf -SUPPORTED_EXTENDED_FEATURES = HTTPRouteQueryParamMatching,HTTPRouteMethodMatching,HTTPRoutePortRedirect,HTTPRouteSchemeRedirect,HTTPRouteHostRewrite,HTTPRoutePathRewrite,GatewayPort8080,HTTPRouteResponseHeaderModification,HTTPRoutePathRedirect,GatewayHTTPListenerIsolation,GatewayInfrastructurePropagation,HTTPRouteRequestMirror,HTTPRouteRequestMultipleMirrors,HTTPRouteBackendProtocolWebSocket,HTTPRouteBackendProtocolH2C +SUPPORTED_EXTENDED_FEATURES = HTTPRouteQueryParamMatching,HTTPRouteMethodMatching,HTTPRoutePortRedirect,HTTPRouteSchemeRedirect,HTTPRouteHostRewrite,HTTPRoutePathRewrite,GatewayPort8080,HTTPRouteResponseHeaderModification,HTTPRoutePathRedirect,GatewayHTTPListenerIsolation,GatewayInfrastructurePropagation,HTTPRouteRequestMirror,HTTPRouteRequestMultipleMirrors,HTTPRouteBackendProtocolWebSocket STANDARD_CONFORMANCE_PROFILES = GATEWAY-HTTP,GATEWAY-GRPC EXPERIMENTAL_CONFORMANCE_PROFILES = GATEWAY-TLS CONFORMANCE_PROFILES = $(STANDARD_CONFORMANCE_PROFILES) # by default we use the standard conformance profiles. If experimental is enabled we override this and add the experimental profiles. From d68de0f972acf01c971cdae2e75e4362e1b091de Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 17 Jun 2025 10:51:19 -0700 Subject: [PATCH 5/8] Use gateway api defined route condition reason --- internal/controller/state/conditions/conditions.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/internal/controller/state/conditions/conditions.go b/internal/controller/state/conditions/conditions.go index afa55b69a0..5098f0522d 100644 --- a/internal/controller/state/conditions/conditions.go +++ b/internal/controller/state/conditions/conditions.go @@ -33,10 +33,6 @@ const ( // Route rules has a backendRef with an unsupported value. RouteReasonBackendRefUnsupportedValue v1.RouteConditionReason = "UnsupportedValue" - // RouteReasonBackendRefUnsupportedProtocol is used with the "ResolvedRefs" condition when one of the - // Route rules has a backendRef with an unsupported protocol. - RouteReasonBackendRefUnsupportedProtocol v1.RouteConditionReason = "UnsupportedProtocol" - // RouteReasonInvalidGateway is used with the "Accepted" (false) condition when the Gateway the Route // references is invalid. RouteReasonInvalidGateway v1.RouteConditionReason = "InvalidGateway" @@ -433,7 +429,7 @@ func NewRouteBackendRefUnsupportedProtocol(msg string) Condition { return Condition{ Type: string(v1.RouteConditionResolvedRefs), Status: metav1.ConditionFalse, - Reason: string(RouteReasonBackendRefUnsupportedProtocol), + Reason: string(v1.RouteReasonUnsupportedProtocol), Message: msg, } } From 516e89b6173362158a135ec7d043732bde0e64b3 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 17 Jun 2025 10:55:08 -0700 Subject: [PATCH 6/8] Fix typo --- internal/framework/controller/predicate/service.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/framework/controller/predicate/service.go b/internal/framework/controller/predicate/service.go index 8795706da9..93c766970e 100644 --- a/internal/framework/controller/predicate/service.go +++ b/internal/framework/controller/predicate/service.go @@ -50,14 +50,14 @@ func (ServiceChangedPredicate) Update(e event.UpdateEvent) bool { newPortSet := make(map[portInfo]struct{}) for i := range len(oldSvc.Spec.Ports) { - var oldAppProtocol, newAppProtocl string + var oldAppProtocol, newAppProtocol string if oldPorts[i].AppProtocol != nil { oldAppProtocol = *oldPorts[i].AppProtocol } if newPorts[i].AppProtocol != nil { - newAppProtocl = *newPorts[i].AppProtocol + newAppProtocol = *newPorts[i].AppProtocol } oldPortSet[portInfo{ @@ -68,7 +68,7 @@ func (ServiceChangedPredicate) Update(e event.UpdateEvent) bool { newPortSet[portInfo{ servicePort: newPorts[i].Port, targetPort: newPorts[i].TargetPort, - appProtocol: newAppProtocl, + appProtocol: newAppProtocol, }] = struct{}{} } From 31a3ebb96ca8a0fb5e5dcfbbb41e3a093c1aeaed Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 17 Jun 2025 11:10:56 -0700 Subject: [PATCH 7/8] Refactor validation function --- .../controller/state/graph/backend_refs.go | 54 ++++++++++++------- internal/controller/state/graph/tlsroute.go | 13 ++--- 2 files changed, 39 insertions(+), 28 deletions(-) diff --git a/internal/controller/state/graph/backend_refs.go b/internal/controller/state/graph/backend_refs.go index 97e3c2b6f3..cb846cc511 100644 --- a/internal/controller/state/graph/backend_refs.go +++ b/internal/controller/state/graph/backend_refs.go @@ -207,8 +207,8 @@ func createBackendRef( } if svcPort.AppProtocol != nil { - valid = validateRouteBackendRefAppProtocol(route.RouteType, *svcPort.AppProtocol, backendTLSPolicy) - if !valid { + err = validateRouteBackendRefAppProtocol(route.RouteType, *svcPort.AppProtocol, backendTLSPolicy) + if err != nil { backendRef := BackendRef{ SvcNsName: svcNsName, BackendTLSPolicy: backendTLSPolicy, @@ -219,17 +219,7 @@ func createBackendRef( InvalidForGateways: invalidForGateways, } - err := fmt.Errorf( - "route type %s does not support service port appProtocol %s", - route.RouteType, - *svcPort.AppProtocol, - ).Error() - - if route.RouteType == RouteTypeHTTP && *svcPort.AppProtocol == AppProtocolTypeWSS && backendTLSPolicy == nil { - err += "; missing corresponding BackendTLSPolicy" - } - - return backendRef, append(conds, conditions.NewRouteBackendRefUnsupportedProtocol(err)) + return backendRef, append(conds, conditions.NewRouteBackendRefUnsupportedProtocol(err.Error())) } } @@ -447,22 +437,50 @@ func validateBackendRef( return true, conditions.Condition{} } +// validateRouteBackendRefAppProtocol checks if a given RouteType supports sending traffic to a service AppProtocol. +// Returns nil if true or AppProtocol is not a Kubernetes Standard Application Protocol. func validateRouteBackendRefAppProtocol( routeType RouteType, appProtocol string, backendTLSPolicy *BackendTLSPolicy, -) (valid bool) { +) error { + err := fmt.Errorf( + "route type %s does not support service port appProtocol %s", + routeType, + appProtocol, + ) + // Currently we only support recognition of the Kubernetes Standard Application Protocols defined in KEP-3726. switch appProtocol { case AppProtocolTypeH2C: - return routeType == RouteTypeHTTP || routeType == RouteTypeGRPC + if routeType == RouteTypeHTTP || routeType == RouteTypeGRPC { + return nil + } + + return err case AppProtocolTypeWS: - return routeType == RouteTypeHTTP + if routeType == RouteTypeHTTP { + return nil + } + + return err case AppProtocolTypeWSS: - return (routeType == RouteTypeHTTP && backendTLSPolicy != nil) || routeType == RouteTypeTLS + if routeType == RouteTypeHTTP { + if backendTLSPolicy != nil { + return nil + } + + return fmt.Errorf("%w; missing corresponding BackendTLSPolicy", err) + } + + if routeType == RouteTypeTLS { + return nil + } + + return err } - return true + return nil } func validateWeight(weight int32) error { diff --git a/internal/controller/state/graph/tlsroute.go b/internal/controller/state/graph/tlsroute.go index ee58f36de0..8d4589ffbd 100644 --- a/internal/controller/state/graph/tlsroute.go +++ b/internal/controller/state/graph/tlsroute.go @@ -1,8 +1,6 @@ package graph import ( - "fmt" - apiv1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation/field" @@ -122,16 +120,11 @@ func validateBackendRefTLSRoute( } if svcPort.AppProtocol != nil { - valid := validateRouteBackendRefAppProtocol(RouteTypeTLS, *svcPort.AppProtocol, nil) - if !valid { + err = validateRouteBackendRefAppProtocol(RouteTypeTLS, *svcPort.AppProtocol, nil) + if err != nil { backendRef.Valid = false - return backendRef, []conditions.Condition{conditions.NewRouteBackendRefUnsupportedProtocol( - fmt.Errorf( - "route type %s does not support service port appProtocol %s", - RouteTypeTLS, - *svcPort.AppProtocol, - ).Error())} + return backendRef, []conditions.Condition{conditions.NewRouteBackendRefUnsupportedProtocol(err.Error())} } } From e4251ca777fa785dd82647fab9c04e43fa828195 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Mon, 30 Jun 2025 14:37:03 -0700 Subject: [PATCH 8/8] Invalidate h2c support for http route --- .../controller/state/graph/backend_refs.go | 6 +- .../state/graph/backend_refs_test.go | 13 +- .../controller/state/graph/route_common.go | 29 +---- .../state/graph/route_common_test.go | 123 ------------------ 4 files changed, 16 insertions(+), 155 deletions(-) diff --git a/internal/controller/state/graph/backend_refs.go b/internal/controller/state/graph/backend_refs.go index cb846cc511..1f7fce6c37 100644 --- a/internal/controller/state/graph/backend_refs.go +++ b/internal/controller/state/graph/backend_refs.go @@ -453,10 +453,14 @@ func validateRouteBackendRefAppProtocol( // Currently we only support recognition of the Kubernetes Standard Application Protocols defined in KEP-3726. switch appProtocol { case AppProtocolTypeH2C: - if routeType == RouteTypeHTTP || routeType == RouteTypeGRPC { + if routeType == RouteTypeGRPC { return nil } + if routeType == RouteTypeHTTP { + return fmt.Errorf("%w; nginx does not support proxying to upstreams with http2 or h2c", err) + } + return err case AppProtocolTypeWS: if routeType == RouteTypeHTTP { diff --git a/internal/controller/state/graph/backend_refs_test.go b/internal/controller/state/graph/backend_refs_test.go index 9fbbaac7cd..751e8a5ddd 100644 --- a/internal/controller/state/graph/backend_refs_test.go +++ b/internal/controller/state/graph/backend_refs_test.go @@ -700,14 +700,19 @@ func TestAddBackendRefsToRules(t *testing.T) { { SvcNsName: svcH2cNsName, ServicePort: svcH2c.Spec.Ports[0], - Valid: true, + Valid: false, Weight: 1, InvalidForGateways: map[types.NamespacedName]conditions.Condition{}, }, }, - expectedConditions: nil, - policies: emptyPolicies, - name: "valid backendRef with service port appProtocol h2c and route type http", + expectedConditions: []conditions.Condition{ + conditions.NewRouteBackendRefUnsupportedProtocol( + "route type http does not support service port appProtocol kubernetes.io/h2c;" + + " nginx does not support proxying to upstreams with http2 or h2c", + ), + }, + policies: emptyPolicies, + name: "invalid backendRef with service port appProtocol h2c and route type http", }, { route: createRoute("hr1", RouteTypeHTTP, "Service", 1, "svcWS"), diff --git a/internal/controller/state/graph/route_common.go b/internal/controller/state/graph/route_common.go index 15392f18dc..0e5f760be1 100644 --- a/internal/controller/state/graph/route_common.go +++ b/internal/controller/state/graph/route_common.go @@ -766,13 +766,11 @@ func bindL7RouteToListeners( continue } - for ruleIdx, rule := range route.Spec.Rules { - for backendRefIdx, backendRef := range rule.BackendRefs { + for _, rule := range route.Spec.Rules { + for _, backendRef := range rule.BackendRefs { if cond, ok := backendRef.InvalidForGateways[gwNsName]; ok { attachment.FailedConditions = append(attachment.FailedConditions, cond) } - - checkAppProtocolH2CConditional(backendRef, gw.EffectiveNginxProxy, route, ruleIdx, backendRefIdx) } } @@ -797,29 +795,6 @@ func bindL7RouteToListeners( } } -func checkAppProtocolH2CConditional( - backendRef BackendRef, - npCfg *EffectiveNginxProxy, - route *L7Route, - ruleIdx, - backendRefIdx int, -) { - // For all backendRefs referring to a Service with appProtocol h2c, we need to also check if http2 - // is enabled. Don't need to check for RouteTypeGRPC since there are checks before this which fail the - // route from connecting to the Gateway. - if backendRef.ServicePort.AppProtocol != nil && - *backendRef.ServicePort.AppProtocol == AppProtocolTypeH2C && - route.RouteType == RouteTypeHTTP && - isHTTP2Disabled(npCfg) { - route.Spec.Rules[ruleIdx].BackendRefs[backendRefIdx].Valid = false - route.Conditions = append(route.Conditions, conditions.NewRouteBackendRefUnsupportedProtocol( - fmt.Errorf("HTTP2 is disabled - cannot support appProtocol h2c on route type %s", - route.RouteType, - ).Error(), - )) - } -} - func isHTTP2Disabled(npCfg *EffectiveNginxProxy) bool { if npCfg == nil { return false diff --git a/internal/controller/state/graph/route_common_test.go b/internal/controller/state/graph/route_common_test.go index 955a326ab2..1f8152fb06 100644 --- a/internal/controller/state/graph/route_common_test.go +++ b/internal/controller/state/graph/route_common_test.go @@ -394,38 +394,6 @@ func TestBindRouteToListeners(t *testing.T) { return normalHTTPRoute } - normalHTTPRouteWithH2CBackendRef := &L7Route{ - RouteType: RouteTypeHTTP, - Source: hr, - Spec: L7RouteSpec{ - Hostnames: hr.Spec.Hostnames, - Rules: []RouteRule{ - { - BackendRefs: []BackendRef{ - { - Valid: true, - ServicePort: v1.ServicePort{ - AppProtocol: helpers.GetPointer(AppProtocolTypeH2C), - }, - }, - }, - }, - }, - }, - Valid: true, - Attachable: true, - ParentRefs: []ParentRef{ - { - Idx: 0, - Gateway: &ParentRefGateway{NamespacedName: client.ObjectKeyFromObject(gw)}, - SectionName: hr.Spec.ParentRefs[0].SectionName, - }, - }, - } - - invalidBackendRefH2c := *normalHTTPRouteWithH2CBackendRef - invalidBackendRefH2c.Spec.Rules[0].BackendRefs[0].Valid = false - getLastNormalHTTPRoute := func() *L7Route { return normalHTTPRoute } @@ -603,7 +571,6 @@ func TestBindRouteToListeners(t *testing.T) { tests := []struct { route *L7Route gateway *Gateway - expectedModifiedRoute *L7Route name string expectedGatewayListeners []*Listener expectedSectionNameRefs []ParentRef @@ -643,90 +610,6 @@ func TestBindRouteToListeners(t *testing.T) { }, name: "normal case", }, - { - route: normalHTTPRouteWithH2CBackendRef, - gateway: &Gateway{ - Source: gw, - Valid: true, - Listeners: []*Listener{ - createListener("listener-80-1"), - }, - EffectiveNginxProxy: &EffectiveNginxProxy{ - DisableHTTP2: helpers.GetPointer(false), - }, - }, - expectedSectionNameRefs: []ParentRef{ - { - Idx: 0, - Gateway: &ParentRefGateway{NamespacedName: client.ObjectKeyFromObject(gw)}, - SectionName: hr.Spec.ParentRefs[0].SectionName, - Attachment: &ParentRefAttachmentStatus{ - Attached: true, - AcceptedHostnames: map[string][]string{ - CreateGatewayListenerKey( - client.ObjectKeyFromObject(gw), - "listener-80-1", - ): {"foo.example.com"}, - }, - }, - }, - }, - expectedGatewayListeners: []*Listener{ - createModifiedListener("listener-80-1", func(l *Listener) { - l.Routes = map[RouteKey]*L7Route{ - CreateRouteKey(hr): normalHTTPRouteWithH2CBackendRef, - } - }), - }, - name: "httpRoute with h2c service port protocol in backend and h2c is enabled", - }, - { - route: normalHTTPRouteWithH2CBackendRef, - gateway: &Gateway{ - Source: gw, - Valid: true, - Listeners: []*Listener{ - createListener("listener-80-1"), - }, - EffectiveNginxProxy: &EffectiveNginxProxy{ - DisableHTTP2: helpers.GetPointer(true), - }, - }, - expectedSectionNameRefs: []ParentRef{ - { - Idx: 0, - Gateway: &ParentRefGateway{NamespacedName: client.ObjectKeyFromObject(gw)}, - SectionName: hr.Spec.ParentRefs[0].SectionName, - Attachment: &ParentRefAttachmentStatus{ - Attached: true, - AcceptedHostnames: map[string][]string{ - CreateGatewayListenerKey( - client.ObjectKeyFromObject(gw), - "listener-80-1", - ): {"foo.example.com"}, - }, - }, - }, - }, - expectedGatewayListeners: []*Listener{ - createModifiedListener("listener-80-1", func(l *Listener) { - route := *normalHTTPRouteWithH2CBackendRef - route.Conditions = []conditions.Condition{ - conditions.NewRouteBackendRefUnsupportedProtocol( - "HTTP2 is disabled - cannot support appProtocol h2c on route type http"), - } - l.Routes = map[RouteKey]*L7Route{ - CreateRouteKey(hr): &route, - } - }), - }, - expectedConditions: []conditions.Condition{ - conditions.NewRouteBackendRefUnsupportedProtocol( - "HTTP2 is disabled - cannot support appProtocol h2c on route type http"), - }, - expectedModifiedRoute: &invalidBackendRefH2c, - name: "httpRoute with h2c service port protocol in backend and h2c is disabled", - }, { route: routeWithMissingSectionName, gateway: &Gateway{ @@ -1515,12 +1398,6 @@ func TestBindRouteToListeners(t *testing.T) { g.Expect(test.route.ParentRefs).To(Equal(test.expectedSectionNameRefs)) g.Expect(helpers.Diff(test.gateway.Listeners, test.expectedGatewayListeners)).To(BeEmpty()) g.Expect(helpers.Diff(test.route.Conditions, test.expectedConditions)).To(BeEmpty()) - - // in situations where bindRouteToListeners modifies the route spec, for instance marking a backendRef - // as invalid - if test.expectedModifiedRoute != nil { - g.Expect(helpers.Diff(test.route.Spec, test.expectedModifiedRoute.Spec)).To(BeEmpty()) - } }) } }