diff --git a/internal/controller/state/graph/policies.go b/internal/controller/state/graph/policies.go index c4b135672b..f790f478e0 100644 --- a/internal/controller/state/graph/policies.go +++ b/internal/controller/state/graph/policies.go @@ -457,6 +457,9 @@ func checkForRouteOverlap(route *L7Route, hostPortPaths map[string]string) *cond for _, parentRef := range route.ParentRefs { if parentRef.Attachment != nil { port := parentRef.Attachment.ListenerPort + // FIXME(sarthyparty): https://github.com/nginx/nginx-gateway-fabric/issues/3811 + // Need to merge listener hostnames with route hostnames so wildcards are handled correctly + // Also the AcceptedHostnames is a map of slices of strings, so we need to flatten it for _, hostname := range parentRef.Attachment.AcceptedHostnames { for _, rule := range route.Spec.Rules { for _, match := range rule.Matches { diff --git a/internal/controller/state/graph/route_common.go b/internal/controller/state/graph/route_common.go index d097e72cbb..c156ca738a 100644 --- a/internal/controller/state/graph/route_common.go +++ b/internal/controller/state/graph/route_common.go @@ -46,6 +46,8 @@ type ParentRefAttachmentStatus struct { // still attach. The backendRef condition would be displayed here. FailedConditions []conditions.Condition // ListenerPort is the port on the Listener that the Route is attached to. + // FIXME(sarthyparty): https://github.com/nginx/nginx-gateway-fabric/issues/3811 + // Needs to be a map of to port number ListenerPort v1.PortNumber // Attached indicates if the ParentRef is attached to the Gateway. Attached bool @@ -307,24 +309,37 @@ func buildSectionNameRefs( gwNsName: gwNsName, } - // If there is no section name, we create ParentRefs for each listener in the gateway + // If there is no section name, handle based on whether port is specified + // FIXME(sarthyparty): https://github.com/nginx/nginx-gateway-fabric/issues/3811 + // this logic seems to be duplicated in findAttachableListeners so we should refactor this, + // either here or in findAttachableListeners if p.SectionName == nil { - for _, l := range gw.Listeners { - k.sectionName = string(l.Source.Name) - - if err := checkUniqueSections(k); err != nil { - return nil, err - } - + // If port is specified, preserve the port-only nature for proper validation + if p.Port != nil { sectionNameRefs = append(sectionNameRefs, ParentRef{ - // if the ParentRefs we create are for each listener in the same gateway, we keep the - // parentRefIndex the same so when we look at a route's parentRef's we can see - // if the parentRef is a unique parentRef or one we created internally Idx: i, Gateway: CreateParentRefGateway(gw), - SectionName: &l.Source.Name, + SectionName: nil, // Keep as nil to preserve port-only semantics Port: p.Port, }) + } else { + // If there is no port and section name, we create ParentRefs for each listener in the gateway + for _, l := range gw.Listeners { + k.sectionName = string(l.Source.Name) + + if err := checkUniqueSections(k); err != nil { + return nil, err + } + + sectionNameRefs = append(sectionNameRefs, ParentRef{ + // if the ParentRefs we create are for each listener in the same gateway, we keep the + // parentRefIndex the same so when we look at a route's parentRef's we can see + // if the parentRef is a unique parentRef or one we created internally + Idx: i, + Gateway: CreateParentRefGateway(gw), + SectionName: &l.Source.Name, + }) + } } continue @@ -532,10 +547,8 @@ func validateParentRef( ref.Attachment = attachment - path := field.NewPath("spec").Child("parentRefs").Index(ref.Idx) - attachableListeners, listenerExists := findAttachableListeners( - getSectionName(ref.SectionName), + ref, gw.Listeners, ) @@ -546,17 +559,7 @@ func validateParentRef( return attachment, nil } - // Case 2: Attachment is not possible due to unsupported configuration. - - if ref.Port != nil { - valErr := field.Forbidden(path.Child("port"), "cannot be set") - attachment.FailedConditions = append( - attachment.FailedConditions, conditions.NewRouteUnsupportedValue(valErr.Error()), - ) - return attachment, attachableListeners - } - - // Case 3: Attachment is not possible because Gateway is invalid + // Case 2: Attachment is not possible because Gateway is invalid if !gw.Valid { attachment.FailedConditions = append(attachment.FailedConditions, conditions.NewRouteInvalidGateway()) @@ -873,29 +876,50 @@ func tryToAttachL7RouteToListeners( // findAttachableListeners returns a list of attachable listeners and whether the listener exists for a non-empty // sectionName. -func findAttachableListeners(sectionName string, listeners []*Listener) ([]*Listener, bool) { +func findAttachableListeners(ref *ParentRef, listeners []*Listener) ([]*Listener, bool) { + sectionName := getSectionName(ref.SectionName) + + // Case 1: sectionName is specified - look for that specific listener if sectionName != "" { for _, l := range listeners { if l.Name == sectionName { - if l.Attachable { + if l.Attachable && (ref.Port == nil || l.Source.Port == *ref.Port) { return []*Listener{l}, true } + // We return false because we didn't find a listener that matches the port + if ref.Port != nil && l.Source.Port != *ref.Port { + return nil, false + } + // Return true because we found a listener that matches the sectionName and port but is not attachable return nil, true } } return nil, false } - attachableListeners := make([]*Listener, 0, len(listeners)) - for _, l := range listeners { - if !l.Attachable { - continue + // Case 2: Only port is specified - find all attachable listeners matching that port + if ref.Port != nil { + var attachableListeners []*Listener + var foundListener bool + for _, l := range listeners { + if l.Source.Port == *ref.Port { + foundListener = true + if l.Attachable { + attachableListeners = append(attachableListeners, l) + } + } } - - attachableListeners = append(attachableListeners, l) + return attachableListeners, foundListener } - return attachableListeners, true + // Case 3: Neither sectionName nor port specified - return all attachable listeners + var attachableListeners []*Listener + for _, l := range listeners { + if l.Attachable { + attachableListeners = append(attachableListeners, l) + } + } + return attachableListeners, len(listeners) > 0 } func findAcceptedHostnames(listenerHostname *v1.Hostname, routeHostnames []v1.Hostname) []string { diff --git a/internal/controller/state/graph/route_common_test.go b/internal/controller/state/graph/route_common_test.go index 0e5d28f0e4..6ded299d73 100644 --- a/internal/controller/state/graph/route_common_test.go +++ b/internal/controller/state/graph/route_common_test.go @@ -720,7 +720,9 @@ func TestBindRouteToListeners(t *testing.T) { Source: gw, Valid: true, Listeners: []*Listener{ - createListener("listener-80-1"), + createModifiedListener("listener-80-1", func(l *Listener) { + l.Source.Port = 80 + }), }, }, expectedSectionNameRefs: []ParentRef{ @@ -729,19 +731,24 @@ func TestBindRouteToListeners(t *testing.T) { Gateway: &ParentRefGateway{NamespacedName: client.ObjectKeyFromObject(gw)}, SectionName: hrWithPort.Spec.ParentRefs[0].SectionName, Attachment: &ParentRefAttachmentStatus{ - Attached: false, - FailedConditions: []conditions.Condition{ - conditions.NewRouteUnsupportedValue( - `spec.parentRefs[0].port: Forbidden: cannot be set`, - ), + Attached: true, + FailedConditions: nil, + AcceptedHostnames: map[string][]string{ + "test/gateway/listener-80-1": {"foo.example.com"}, }, - AcceptedHostnames: map[string][]string{}, + ListenerPort: 80, }, Port: hrWithPort.Spec.ParentRefs[0].Port, }, }, expectedGatewayListeners: []*Listener{ - createListener("listener-80-1"), + func() *Listener { + l := createModifiedListener("listener-80-1", func(l *Listener) { + l.Source.Port = 80 + }) + l.Routes[CreateRouteKey(hrWithPort)] = routeWithPort + return l + }(), }, name: "port is configured", }, @@ -1904,7 +1911,9 @@ func TestBindL4RouteToListeners(t *testing.T) { Name: "gateway", }, Listeners: []*Listener{ - createListener("listener-443"), + createModifiedListener("listener-443", func(l *Listener) { + l.Source.Port = 443 + }), }, }, expectedSectionNameRefs: []ParentRef{ @@ -1912,9 +1921,7 @@ func TestBindL4RouteToListeners(t *testing.T) { Attachment: &ParentRefAttachmentStatus{ AcceptedHostnames: map[string][]string{}, FailedConditions: []conditions.Condition{ - conditions.NewRouteUnsupportedValue( - `spec.parentRefs[0].port: Forbidden: cannot be set`, - ), + conditions.NewRouteNoMatchingParent(), }, Attached: false, }, @@ -1925,7 +1932,9 @@ func TestBindL4RouteToListeners(t *testing.T) { }, }, expectedGatewayListeners: []*Listener{ - createListener("listener-443"), + createModifiedListener("listener-443", func(l *Listener) { + l.Source.Port = 443 + }), }, name: "port is not nil", }, @@ -3720,3 +3729,141 @@ func TestBindRoutesToListeners(t *testing.T) { bindRoutesToListeners(nil, nil, nil, nil) }).ToNot(Panic()) } + +func TestFindAttachableListenersWithPort(t *testing.T) { + t.Parallel() + + port80 := gatewayv1.PortNumber(80) + port443 := gatewayv1.PortNumber(443) + port8080 := gatewayv1.PortNumber(8080) + + httpListener := &Listener{ + Name: "http-80", + Attachable: true, + Source: gatewayv1.Listener{ + Name: "http-80", + Port: port80, + }, + } + + httpsListener := &Listener{ + Name: "https-443", + Attachable: true, + Source: gatewayv1.Listener{ + Name: "https-443", + Port: port443, + }, + } + + nonAttachableListener := &Listener{ + Name: "http-8080", + Attachable: false, // not attachable + Source: gatewayv1.Listener{ + Name: "http-8080", + Port: port8080, + }, + } + + tests := []struct { + name string + parentRef *ParentRef + expectedListeners []*Listener + expectedListenerExists bool + }{ + { + name: "port 80 filter returns only port 80 listener", + parentRef: &ParentRef{ + Port: &port80, + }, + expectedListeners: []*Listener{httpListener}, + expectedListenerExists: true, + }, + { + name: "port 443 filter returns only port 443 listener", + parentRef: &ParentRef{ + Port: &port443, + }, + expectedListeners: []*Listener{httpsListener}, + expectedListenerExists: true, + }, + { + name: "port 8080 filter returns empty because listener is not attachable", + parentRef: &ParentRef{ + Port: &port8080, + }, + expectedListeners: []*Listener{}, + expectedListenerExists: true, + }, + { + name: "port 9999 filter returns empty because no listener has that port", + parentRef: &ParentRef{ + Port: helpers.GetPointer(gatewayv1.PortNumber(9999)), + }, + expectedListeners: []*Listener{}, + expectedListenerExists: false, + }, + { + name: "no port specified returns all attachable listeners", + parentRef: &ParentRef{ + Port: nil, + }, + expectedListeners: []*Listener{httpListener, httpsListener}, + expectedListenerExists: true, + }, + { + name: "sectionName with matching port returns that specific listener", + parentRef: &ParentRef{ + SectionName: helpers.GetPointer(gatewayv1.SectionName("http-80")), + Port: &port80, + }, + expectedListeners: []*Listener{httpListener}, + expectedListenerExists: true, + }, + { + name: "sectionName with non-matching port returns empty", + parentRef: &ParentRef{ + SectionName: helpers.GetPointer(gatewayv1.SectionName("http-80")), + Port: &port443, // wrong port for http-80 listener + }, + expectedListeners: []*Listener{}, + expectedListenerExists: false, + }, + { + name: "sectionName that doesn't exist returns empty with false", + parentRef: &ParentRef{ + SectionName: helpers.GetPointer(gatewayv1.SectionName("nonexistent")), + Port: &port80, + }, + expectedListeners: []*Listener{}, + expectedListenerExists: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + attachableListeners, listenerExists := findAttachableListeners( + tt.parentRef, + []*Listener{httpListener, httpsListener, nonAttachableListener}, + ) + + g.Expect(listenerExists).To(Equal(tt.expectedListenerExists)) + g.Expect(attachableListeners).To(HaveLen(len(tt.expectedListeners))) + + // Compare listeners by name since they're the same instances + expectedNames := make([]string, len(tt.expectedListeners)) + for i, l := range tt.expectedListeners { + expectedNames[i] = l.Name + } + + actualNames := make([]string, len(attachableListeners)) + for i, l := range attachableListeners { + actualNames[i] = l.Name + } + + g.Expect(actualNames).To(ConsistOf(expectedNames)) + }) + } +} diff --git a/tests/Makefile b/tests/Makefile index 379e3b88b5..f1ccfdd3c5 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,GatewayAddressEmpty,HTTPRouteResponseHeaderModification,HTTPRoutePathRedirect,GatewayHTTPListenerIsolation,GatewayInfrastructurePropagation,HTTPRouteRequestMirror,HTTPRouteRequestMultipleMirrors,HTTPRouteRequestPercentageMirror,HTTPRouteBackendProtocolWebSocket +SUPPORTED_EXTENDED_FEATURES = HTTPRouteQueryParamMatching,HTTPRouteMethodMatching,HTTPRoutePortRedirect,HTTPRouteSchemeRedirect,HTTPRouteHostRewrite,HTTPRoutePathRewrite,GatewayPort8080,GatewayAddressEmpty,HTTPRouteResponseHeaderModification,HTTPRoutePathRedirect,GatewayHTTPListenerIsolation,GatewayInfrastructurePropagation,HTTPRouteRequestMirror,HTTPRouteRequestMultipleMirrors,HTTPRouteRequestPercentageMirror,HTTPRouteBackendProtocolWebSocket,HTTPRouteParentRefPort,HTTPRouteDestinationPortMatching 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.