From a7e90478dd65554d0c9a65a166dc652bcda69ebf Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Thu, 4 May 2023 15:08:26 -0600 Subject: [PATCH 1/3] Allow empty sectionName in HTTPRoute parentRef Support for empty sectionName in HTTPRoute parentRef. This will bind the route to all listeners with matching hostnames. If multiple listeners match, we'll choose the most specific one (mainly for TLS case to ensure proper certs are used). Either way, we should end up with a single nginx server for the hostname. We could end up with duplicate match rules if multiple listeners match, but nginx/njs will send traffic properly. At some point we'll need to figure out how to de-dupe these. --- docs/gateway-api-compatibility.md | 2 +- internal/nginx/config/servers.go | 2 + internal/state/dataplane/configuration.go | 44 +++++- .../state/dataplane/configuration_test.go | 126 ++++++++++++++++++ internal/state/graph/httproute.go | 76 ++++++----- internal/state/graph/httproute_test.go | 28 ++-- 6 files changed, 232 insertions(+), 46 deletions(-) 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/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..bcd08da0c 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 hostnameMoreSpecific(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,35 @@ func convertPathType(pathType v1beta1.PathMatchType) PathType { panic(fmt.Sprintf("unsupported path type: %s", pathType)) } } + +// 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 wildcard/substring. +// +// For example: +// - *.example.com and foo.example.com (host2 wins) +// - 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 hostnameMoreSpecific(host1, host2 *v1beta1.Hostname) bool { + var host1Str, host2Str string + if host1 == nil { + host1Str = "" + } else { + host1Str = string(*host1) + } + + if host2 == nil { + host2Str = "" + } else { + 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..79840face 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(hostnameMoreSpecific(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..6c62cf452 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 valid bool + if getSectionName(routeRef.SectionName) == "" { + for _, l := range gw.Listeners { + valid = bind(l) || valid + } + } 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 + } + + valid = bind(l) + } + if !valid { 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..6d9c641ff 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,15 +856,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): routeWithEmptySectionName, + } + l.AcceptedHostnames = map[string]struct{}{ + "foo.example.com": {}, + } + }), }, name: "section name is empty", }, From c9ae4280be44c67efaa6eabebc0f8922b458bce4 Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Tue, 9 May 2023 09:12:44 -0600 Subject: [PATCH 2/3] Address code review comments --- internal/state/dataplane/configuration.go | 6 ++--- .../state/dataplane/configuration_test.go | 2 +- internal/state/graph/httproute.go | 8 +++---- internal/state/graph/httproute_test.go | 23 +++++++++++++++++++ 4 files changed, 31 insertions(+), 8 deletions(-) diff --git a/internal/state/dataplane/configuration.go b/internal/state/dataplane/configuration.go index bcd08da0c..bc3eb8378 100644 --- a/internal/state/dataplane/configuration.go +++ b/internal/state/dataplane/configuration.go @@ -232,7 +232,7 @@ func (hpr *hostPathRules) upsertListener(l *graph.Listener) { for _, h := range hostnames { if prevListener, exists := hpr.listenersForHost[h]; exists { // override the previous listener if the new one has a more specific hostname - if hostnameMoreSpecific(l.Source.Hostname, prevListener.Source.Hostname) { + if listenerHostnameMoreSpecific(l.Source.Hostname, prevListener.Source.Hostname) { hpr.listenersForHost[h] = l } } else { @@ -443,7 +443,7 @@ func convertPathType(pathType v1beta1.PathMatchType) PathType { } } -// Returns true if host1 is more specific than host2 (using length). +// 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 @@ -458,7 +458,7 @@ func convertPathType(pathType v1beta1.PathMatchType) PathType { // 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 hostnameMoreSpecific(host1, host2 *v1beta1.Hostname) bool { +func listenerHostnameMoreSpecific(host1, host2 *v1beta1.Hostname) bool { var host1Str, host2Str string if host1 == nil { host1Str = "" diff --git a/internal/state/dataplane/configuration_test.go b/internal/state/dataplane/configuration_test.go index 79840face..4b854c5c2 100644 --- a/internal/state/dataplane/configuration_test.go +++ b/internal/state/dataplane/configuration_test.go @@ -1749,6 +1749,6 @@ func TestHostnameMoreSpecific(t *testing.T) { } for _, tc := range tests { - g.Expect(hostnameMoreSpecific(tc.host1, tc.host2)).To(Equal(tc.host1Wins), tc.msg) + 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 6c62cf452..25156f200 100644 --- a/internal/state/graph/httproute.go +++ b/internal/state/graph/httproute.go @@ -340,10 +340,10 @@ func bindRouteToListeners(r *Route, gw *Gateway) { return true } - var valid bool + var validListener bool if getSectionName(routeRef.SectionName) == "" { for _, l := range gw.Listeners { - valid = bind(l) || valid + validListener = bind(l) || validListener } } else { l, exists := gw.Listeners[string(*routeRef.SectionName)] @@ -354,9 +354,9 @@ func bindRouteToListeners(r *Route, gw *Gateway) { continue } - valid = bind(l) + validListener = bind(l) } - if !valid { + if !validListener { attachment.FailedCondition = conditions.NewRouteInvalidListener() continue } diff --git a/internal/state/graph/httproute_test.go b/internal/state/graph/httproute_test.go index 6d9c641ff..c52fc2946 100644 --- a/internal/state/graph/httproute_test.go +++ b/internal/state/graph/httproute_test.go @@ -872,6 +872,29 @@ func TestBindRouteToListeners(t *testing.T) { }, 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{ From 447c0edb2cb65cd984b08d9ea56045531e16906c Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Tue, 9 May 2023 12:39:14 -0600 Subject: [PATCH 3/3] More code review --- examples/advanced-routing/cafe-routes.yaml | 2 -- internal/state/dataplane/configuration.go | 11 +++-------- 2 files changed, 3 insertions(+), 10 deletions(-) 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/state/dataplane/configuration.go b/internal/state/dataplane/configuration.go index bc3eb8378..81f559da2 100644 --- a/internal/state/dataplane/configuration.go +++ b/internal/state/dataplane/configuration.go @@ -447,10 +447,9 @@ func convertPathType(pathType v1beta1.PathMatchType) PathType { // // 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 wildcard/substring. +// exactly or as a substring. // // For example: -// - *.example.com and foo.example.com (host2 wins) // - foo.example.com and "" (host1 wins) // Non-example: // - foo.example.com and bar.example.com (should not be given to this function) @@ -460,15 +459,11 @@ func convertPathType(pathType v1beta1.PathMatchType) PathType { // 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 = "" - } else { + if host1 != nil { host1Str = string(*host1) } - if host2 == nil { - host2Str = "" - } else { + if host2 != nil { host2Str = string(*host2) }