Allow empty sectionName in HTTPRoute parentRef

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