diff --git a/docs/gateway-api-compatibility.md b/docs/gateway-api-compatibility.md index 8346361c4..0e27688a5 100644 --- a/docs/gateway-api-compatibility.md +++ b/docs/gateway-api-compatibility.md @@ -79,7 +79,7 @@ Fields: Fields: * `spec` - * `parentRefs` - partially supported. `sectionName` must always be set. + * `parentRefs` - partially supported. Port not supported. * `hostnames` - partially supported. Wildcard binding is not supported: a hostname like `example.com` will not bind to a listener with the hostname `*.example.com`. However, `example.com` will bind to a listener with the empty hostname. * `rules` * `matches` diff --git a/examples/advanced-routing/cafe-routes.yaml b/examples/advanced-routing/cafe-routes.yaml index 06d0eaa54..accdaa007 100644 --- a/examples/advanced-routing/cafe-routes.yaml +++ b/examples/advanced-routing/cafe-routes.yaml @@ -5,7 +5,6 @@ metadata: spec: parentRefs: - name: gateway - sectionName: http hostnames: - "cafe.example.com" rules: @@ -40,7 +39,6 @@ metadata: spec: parentRefs: - name: gateway - sectionName: http hostnames: - "cafe.example.com" rules: diff --git a/internal/nginx/config/servers.go b/internal/nginx/config/servers.go index cbb28db87..0a46489ce 100644 --- a/internal/nginx/config/servers.go +++ b/internal/nginx/config/servers.go @@ -144,6 +144,8 @@ func createLocations(pathRules []dataplane.PathRule, listenerPort int) []http.Lo } if len(matches) > 0 { + // FIXME(sberman): De-dupe matches and associated locations + // so we don't need nginx/njs to perform unnecessary matching. b, err := json.Marshal(matches) if err != nil { // panic is safe here because we should never fail to marshal the match unless we constructed it incorrectly. diff --git a/internal/state/dataplane/configuration.go b/internal/state/dataplane/configuration.go index 311105484..81f559da2 100644 --- a/internal/state/dataplane/configuration.go +++ b/internal/state/dataplane/configuration.go @@ -230,7 +230,14 @@ func (hpr *hostPathRules) upsertListener(l *graph.Listener) { } for _, h := range hostnames { - hpr.listenersForHost[h] = l + if prevListener, exists := hpr.listenersForHost[h]; exists { + // override the previous listener if the new one has a more specific hostname + if listenerHostnameMoreSpecific(l.Source.Hostname, prevListener.Source.Hostname) { + hpr.listenersForHost[h] = l + } + } else { + hpr.listenersForHost[h] = l + } if _, exist := hpr.rulesPerHost[h]; !exist { hpr.rulesPerHost[h] = make(map[pathAndType]PathRule) @@ -319,7 +326,8 @@ func (hpr *hostPathRules) buildServers() []VirtualServer { for _, l := range hpr.httpsListeners { hostname := getListenerHostname(l.Source.Hostname) - // generate a 404 ssl server block for listeners with no routes or listeners with wildcard (match-all) routes + // Generate a 404 ssl server block for listeners with no routes or listeners with wildcard (match-all) routes. + // This server overrides the default ssl server. // FIXME(kate-osborn): when we support regex hostnames (e.g. *.example.com) // we will have to modify this check to catch regex hostnames. if len(l.Routes) == 0 || hostname == wildcardHostname { @@ -434,3 +442,30 @@ func convertPathType(pathType v1beta1.PathMatchType) PathType { panic(fmt.Sprintf("unsupported path type: %s", pathType)) } } + +// listenerHostnameMoreSpecific returns true if host1 is more specific than host2 (using length). +// +// FIXME(sberman): Since the only caller of this function specifies listener hostnames that are both +// bound to the same route hostname, this function assumes that host1 and host2 match, either +// exactly or as a substring. +// +// For example: +// - foo.example.com and "" (host1 wins) +// Non-example: +// - foo.example.com and bar.example.com (should not be given to this function) +// +// As we add regex support, we should put in the proper +// validation and error handling for this function to ensure that the hostnames are actually matching, +// to avoid the unintended inputs above for the invalid case. +func listenerHostnameMoreSpecific(host1, host2 *v1beta1.Hostname) bool { + var host1Str, host2Str string + if host1 != nil { + host1Str = string(*host1) + } + + if host2 != nil { + host2Str = string(*host2) + } + + return len(host1Str) >= len(host2Str) +} diff --git a/internal/state/dataplane/configuration_test.go b/internal/state/dataplane/configuration_test.go index 96da054a0..4b854c5c2 100644 --- a/internal/state/dataplane/configuration_test.go +++ b/internal/state/dataplane/configuration_test.go @@ -1057,6 +1057,86 @@ func TestBuildConfiguration(t *testing.T) { }, msg: "duplicate paths with different types", }, + { + graph: &graph.Graph{ + GatewayClass: &graph.GatewayClass{ + Source: &v1beta1.GatewayClass{}, + Valid: true, + }, + Gateway: &graph.Gateway{ + Source: &v1beta1.Gateway{}, + Listeners: map[string]*graph.Listener{ + "listener-443-with-hostname": { + Source: listener443WithHostname, + Valid: true, + SecretPath: "secret-path-https-listener-2", + Routes: map[types.NamespacedName]*graph.Route{ + {Namespace: "test", Name: "https-hr-5"}: httpsRouteHR5, + }, + AcceptedHostnames: map[string]struct{}{ + "example.com": {}, + }, + }, + "listener-443-1": { + Source: listener443, + Valid: true, + SecretPath: secretPath, + Routes: map[types.NamespacedName]*graph.Route{ + {Namespace: "test", Name: "https-hr-5"}: httpsRouteHR5, + }, + AcceptedHostnames: map[string]struct{}{ + "example.com": {}, + }, + }, + }, + }, + Routes: map[types.NamespacedName]*graph.Route{ + {Namespace: "test", Name: "https-hr-5"}: httpsRouteHR5, + }, + }, + expConf: Configuration{ + HTTPServers: []VirtualServer{}, + SSLServers: []VirtualServer{ + { + IsDefault: true, + }, + { + Hostname: "example.com", + PathRules: []PathRule{ + { + Path: "/", + PathType: PathTypePrefix, + MatchRules: []MatchRule{ + // duplicate match rules since two listeners both match this route's hostname + { + MatchIdx: 0, + RuleIdx: 0, + BackendGroup: httpsHR5Groups[0], + Source: httpsHR5, + }, + { + MatchIdx: 0, + RuleIdx: 0, + BackendGroup: httpsHR5Groups[0], + Source: httpsHR5, + }, + }, + }, + }, + SSL: &SSL{ + CertificatePath: "secret-path-https-listener-2", + }, + }, + { + Hostname: wildcardHostname, + SSL: &SSL{CertificatePath: secretPath}, + }, + }, + Upstreams: []Upstream{fooUpstream}, + BackendGroups: []graph.BackendGroup{httpsHR5Groups[0]}, + }, + msg: "two https listeners with different hostnames but same route; chooses listener with more specific hostname", + }, } for _, test := range tests { @@ -1626,3 +1706,49 @@ func TestConvertPathType(t *testing.T) { } } } + +func TestHostnameMoreSpecific(t *testing.T) { + g := NewGomegaWithT(t) + + tests := []struct { + host1 *v1beta1.Hostname + host2 *v1beta1.Hostname + msg string + host1Wins bool + }{ + { + host1: nil, + host2: helpers.GetPointer(v1beta1.Hostname("")), + host1Wins: true, + msg: "host1 nil; host2 empty", + }, + { + host1: helpers.GetPointer(v1beta1.Hostname("")), + host2: nil, + host1Wins: true, + msg: "host1 empty; host2 nil", + }, + { + host1: helpers.GetPointer(v1beta1.Hostname("")), + host2: helpers.GetPointer(v1beta1.Hostname("")), + host1Wins: true, + msg: "both hosts empty", + }, + { + host1: helpers.GetPointer(v1beta1.Hostname("example.com")), + host2: helpers.GetPointer(v1beta1.Hostname("")), + host1Wins: true, + msg: "host1 has value; host2 empty", + }, + { + host1: helpers.GetPointer(v1beta1.Hostname("example.com")), + host2: helpers.GetPointer(v1beta1.Hostname("foo.example.com")), + host1Wins: false, + msg: "host2 longer than host1", + }, + } + + for _, tc := range tests { + g.Expect(listenerHostnameMoreSpecific(tc.host1, tc.host2)).To(Equal(tc.host1Wins), tc.msg) + } +} diff --git a/internal/state/graph/httproute.go b/internal/state/graph/httproute.go index 27fbcb825..25156f200 100644 --- a/internal/state/graph/httproute.go +++ b/internal/state/graph/httproute.go @@ -298,12 +298,6 @@ func bindRouteToListeners(r *Route, gw *Gateway) { // Case 1: Attachment is not possible due to unsupported configuration - if routeRef.SectionName == nil || *routeRef.SectionName == "" { - valErr := field.Required(path.Child("sectionName"), "cannot be empty") - attachment.FailedCondition = conditions.NewRouteUnsupportedValue(valErr.Error()) - continue - } - if routeRef.Port != nil { valErr := field.Forbidden(path.Child("port"), "cannot be set") attachment.FailedCondition = conditions.NewRouteUnsupportedValue(valErr.Error()) @@ -326,40 +320,49 @@ func bindRouteToListeners(r *Route, gw *Gateway) { // FIXME(pleshakov) // For now, let's do simple matching. // However, we need to also support wildcard matching. - // More over, we need to handle cases when a Route host matches multiple HTTP listeners on the same port - // when sectionName is empty and only choose one listener. - // For example: - // - Route with host foo.example.com; - // - listener 1 for port 80 with hostname foo.example.com - // - listener 2 for port 80 with hostname *.example.com; - // In this case, the Route host foo.example.com should choose listener 1, as it is a more specific match. - - l, exists := gw.Listeners[string(*routeRef.SectionName)] - if !exists { - // FIXME(pleshakov): Add a proper condition once it is available in the Gateway API. - // https://github.com/nginxinc/nginx-kubernetes-gateway/issues/306 - attachment.FailedCondition = conditions.NewTODO("listener is not found") - continue + + bind := func(l *Listener) (valid bool) { + if !l.Valid { + return false + } + + hostnames := findAcceptedHostnames(l.Source.Hostname, r.Source.Spec.Hostnames) + if len(hostnames) == 0 { + return true // listener is valid, but return without attaching due to no matching hostnames + } + + attachment.Attached = true + for _, h := range hostnames { + l.AcceptedHostnames[h] = struct{}{} + } + l.Routes[client.ObjectKeyFromObject(r.Source)] = r + + return true } - if !l.Valid { + var validListener bool + if getSectionName(routeRef.SectionName) == "" { + for _, l := range gw.Listeners { + validListener = bind(l) || validListener + } + } else { + l, exists := gw.Listeners[string(*routeRef.SectionName)] + if !exists { + // FIXME(pleshakov): Add a proper condition once it is available in the Gateway API. + // https://github.com/nginxinc/nginx-kubernetes-gateway/issues/306 + attachment.FailedCondition = conditions.NewTODO("listener is not found") + continue + } + + validListener = bind(l) + } + if !validListener { attachment.FailedCondition = conditions.NewRouteInvalidListener() continue } - - accepted := findAcceptedHostnames(l.Source.Hostname, r.Source.Spec.Hostnames) - - if len(accepted) == 0 { + if !attachment.Attached { attachment.FailedCondition = conditions.NewRouteNoMatchingListenerHostname() - continue } - - attachment.Attached = true - - for _, h := range accepted { - l.AcceptedHostnames[h] = struct{}{} - } - l.Routes[client.ObjectKeyFromObject(r.Source)] = r } } @@ -391,6 +394,13 @@ func getHostname(h *v1beta1.Hostname) string { return string(*h) } +func getSectionName(s *v1beta1.SectionName) string { + if s == nil { + return "" + } + return string(*s) +} + func validateHostnames(hostnames []v1beta1.Hostname, path *field.Path) error { var allErrs field.ErrorList diff --git a/internal/state/graph/httproute_test.go b/internal/state/graph/httproute_test.go index 8e44082aa..c52fc2946 100644 --- a/internal/state/graph/httproute_test.go +++ b/internal/state/graph/httproute_test.go @@ -827,15 +827,19 @@ func TestBindRouteToListeners(t *testing.T) { Idx: 0, Gateway: client.ObjectKeyFromObject(gw), Attachment: &ParentRefAttachmentStatus{ - Attached: false, - FailedCondition: conditions.NewRouteUnsupportedValue( - `spec.parentRefs[0].sectionName: Required value: cannot be empty`, - ), + Attached: true, }, }, }, expectedGatewayListeners: map[string]*Listener{ - "listener-80-1": createListener(), + "listener-80-1": createModifiedListener(func(l *Listener) { + l.Routes = map[types.NamespacedName]*Route{ + client.ObjectKeyFromObject(hr): routeWithMissingSectionName, + } + l.AcceptedHostnames = map[string]struct{}{ + "foo.example.com": {}, + } + }), }, name: "section name is nil", }, @@ -852,18 +856,45 @@ func TestBindRouteToListeners(t *testing.T) { Idx: 0, Gateway: client.ObjectKeyFromObject(gw), Attachment: &ParentRefAttachmentStatus{ - Attached: false, - FailedCondition: conditions.NewRouteUnsupportedValue( - `spec.parentRefs[0].sectionName: Required value: cannot be empty`, - ), + Attached: true, }, }, }, expectedGatewayListeners: map[string]*Listener{ - "listener-80-1": createListener(), + "listener-80-1": createModifiedListener(func(l *Listener) { + l.Routes = map[types.NamespacedName]*Route{ + client.ObjectKeyFromObject(hr): routeWithEmptySectionName, + } + l.AcceptedHostnames = map[string]struct{}{ + "foo.example.com": {}, + } + }), }, name: "section name is empty", }, + { + route: routeWithEmptySectionName, + gateway: &Gateway{ + Source: gw, + Listeners: map[string]*Listener{ + "listener-80-1": notValidListener, + }, + }, + expectedSectionNameRefs: []ParentRef{ + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + Attachment: &ParentRefAttachmentStatus{ + Attached: false, + FailedCondition: conditions.NewRouteInvalidListener(), + }, + }, + }, + expectedGatewayListeners: map[string]*Listener{ + "listener-80-1": notValidListener, + }, + name: "empty section name with no valid listeners", + }, { route: routeWithPort, gateway: &Gateway{