Allow empty sectionName in HTTPRoute parentRef (#626)

* 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.

Author: sjberman

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`

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:

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.

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)
+}

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)
+	}
+}

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