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/controller/state/conditions/conditions.go b/internal/controller/state/conditions/conditions.go index 6630e69a28..5098f0522d 100644 --- a/internal/controller/state/conditions/conditions.go +++ b/internal/controller/state/conditions/conditions.go @@ -423,6 +423,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(v1.RouteReasonUnsupportedProtocol), + 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..1f7fce6c37 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,23 @@ func createBackendRef( return backendRef, append(conds, conditions.NewRouteBackendRefUnsupportedValue(err.Error())) } + if svcPort.AppProtocol != nil { + err = validateRouteBackendRefAppProtocol(route.RouteType, *svcPort.AppProtocol, backendTLSPolicy) + if err != nil { + backendRef := BackendRef{ + SvcNsName: svcNsName, + BackendTLSPolicy: backendTLSPolicy, + ServicePort: svcPort, + Weight: weight, + Valid: false, + IsMirrorBackend: ref.MirrorBackendIdx != nil, + InvalidForGateways: invalidForGateways, + } + + return backendRef, append(conds, conditions.NewRouteBackendRefUnsupportedProtocol(err.Error())) + } + } + backendRef := BackendRef{ SvcNsName: svcNsName, BackendTLSPolicy: backendTLSPolicy, @@ -414,6 +437,56 @@ 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, +) 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: + 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 { + return nil + } + + return err + case AppProtocolTypeWSS: + if routeType == RouteTypeHTTP { + if backendTLSPolicy != nil { + return nil + } + + return fmt.Errorf("%w; missing corresponding BackendTLSPolicy", err) + } + + if routeType == RouteTypeTLS { + return nil + } + + return err + } + + return nil +} + 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..751e8a5ddd 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,164 @@ 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: 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/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"), + 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 +862,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 +872,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 +882,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 +898,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 +929,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..0e5f760be1 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. diff --git a/internal/controller/state/graph/route_common_test.go b/internal/controller/state/graph/route_common_test.go index 4b4fd1c2fe..1f8152fb06 100644 --- a/internal/controller/state/graph/route_common_test.go +++ b/internal/controller/state/graph/route_common_test.go @@ -571,8 +571,8 @@ func TestBindRouteToListeners(t *testing.T) { tests := []struct { route *L7Route gateway *Gateway - expectedGatewayListeners []*Listener name string + expectedGatewayListeners []*Listener expectedSectionNameRefs []ParentRef expectedConditions []conditions.Condition }{ diff --git a/internal/controller/state/graph/tlsroute.go b/internal/controller/state/graph/tlsroute.go index 2cec3ffb3d..8d4589ffbd 100644 --- a/internal/controller/state/graph/tlsroute.go +++ b/internal/controller/state/graph/tlsroute.go @@ -119,6 +119,15 @@ func validateBackendRefTLSRoute( return backendRef, []conditions.Condition{conditions.NewRouteBackendRefRefBackendNotFound(err.Error())} } + if svcPort.AppProtocol != nil { + err = validateRouteBackendRefAppProtocol(RouteTypeTLS, *svcPort.AppProtocol, nil) + if err != nil { + backendRef.Valid = false + + return backendRef, []conditions.Condition{conditions.NewRouteBackendRefUnsupportedProtocol(err.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/internal/framework/controller/predicate/service.go b/internal/framework/controller/predicate/service.go index 21e59e6ee0..93c766970e 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, newAppProtocol string + + if oldPorts[i].AppProtocol != nil { + oldAppProtocol = *oldPorts[i].AppProtocol + } + + if newPorts[i].AppProtocol != nil { + newAppProtocol = *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: newAppProtocol, + }] = 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(), diff --git a/tests/Makefile b/tests/Makefile index 6db161b3b2..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 +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.