refactor attaching listeners

Author: sarthyparty

internal/controller/state/graph/route_common.go

@@ -307,37 +307,24 @@ func buildSectionNameRefs(
 			gwNsName: gwNsName,
 		}
 
-		// If there is no section name, handle based on whether port is specified
-		// FIXME(sarthyparty): this logic seems to be duplicated in findAttachableListeners so we should refactor this,
-		// either here or in findAttachableListeners
+		// If there is no section name, expand to all listeners (port filtering will be done in findAttachableListeners)
 		if p.SectionName == nil {
-			// If port is specified, preserve the port-only nature for proper validation
-			if p.Port != nil {
+			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: nil, // Keep as nil to preserve port-only semantics
+					SectionName: &l.Source.Name,
 					Port:        p.Port,
 				})
-			} else {
-				// If no port and no sectionName, expand to all listeners
-				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,
-						Port:        nil,
-					})
-				}
 			}
 
 			continue
@@ -538,14 +525,14 @@ func removeHostnames(hostnames []string, toRemove map[string]struct{}) []string
 func validateParentRef(
 	ref *ParentRef,
 	gw *Gateway,
-) (status *ParentRefAttachmentStatus, attachableListeners []*Listener) {
+) (status *ParentRefAttachmentStatus, attachableListener *Listener) {
 	attachment := &ParentRefAttachmentStatus{
 		AcceptedHostnames: make(map[string][]string),
 	}
 
 	ref.Attachment = attachment
 
-	attachableListeners, listenerExists := findAttachableListeners(
+	attachableListener, listenerExists := findAttachableListener(
 		ref,
 		gw.Listeners,
 	)
@@ -561,10 +548,10 @@ func validateParentRef(
 
 	if !gw.Valid {
 		attachment.FailedConditions = append(attachment.FailedConditions, conditions.NewRouteInvalidGateway())
-		return attachment, attachableListeners
+		return attachment, attachableListener
 	}
 
-	return attachment, attachableListeners
+	return attachment, attachableListener
 }
 
 func bindL4RouteToListeners(
@@ -589,7 +576,7 @@ func bindL4RouteToListeners(
 			continue
 		}
 
-		attachment, attachableListeners := validateParentRef(ref, gw)
+		attachment, attachableListener := validateParentRef(ref, gw)
 
 		if len(attachment.FailedConditions) > 0 {
 			continue
@@ -599,11 +586,10 @@ func bindL4RouteToListeners(
 			attachment.FailedConditions = append(attachment.FailedConditions, cond)
 		}
 
-		// Try to attach Route to all matching listeners
-
-		cond, attached := tryToAttachL4RouteToListeners(
+		// Try to attach Route to the matching listener
+		cond, attached := tryToAttachL4RouteToListener(
 			ref.Attachment,
-			attachableListeners,
+			attachableListener,
 			route,
 			gw,
 			namespaces,
@@ -621,56 +607,29 @@ func bindL4RouteToListeners(
 	}
 }
 
-// tryToAttachL4RouteToListeners tries to attach the L4Route to listeners that match the parentRef and the hostnames.
-// There are two cases:
-// (1) If it succeeds in attaching at least one listener it will return true. The returned condition will be empty if
-// at least one of the listeners is valid. Otherwise, it will return the failure condition.
-// (2) If it fails to attach the route, it will return false and the failure condition.
-func tryToAttachL4RouteToListeners(
+// tryToAttachL4RouteToListener attempts to attach a L4 route to the given listener.
+// Returns a condition and whether the attachment was successful.
+func tryToAttachL4RouteToListener(
 	refStatus *ParentRefAttachmentStatus,
-	attachableListeners []*Listener,
+	attachableListener *Listener,
 	route *L4Route,
 	gw *Gateway,
 	namespaces map[types.NamespacedName]*apiv1.Namespace,
 	portHostnamesMap map[string]struct{},
 ) (conditions.Condition, bool) {
-	if len(attachableListeners) == 0 {
+	if attachableListener == nil {
 		return conditions.NewRouteInvalidListener(), false
 	}
 
-	var (
-		attachedToAtLeastOneValidListener  bool
-		allowed, attached, hostnamesUnique bool
+	allowed, attached, hostnamesUnique := bindToListenerL4(
+		attachableListener,
+		route,
+		gw,
+		namespaces,
+		portHostnamesMap,
+		refStatus,
 	)
 
-	// Sorting the listeners from most specific hostname to the least specific hostname
-	sort.Slice(attachableListeners, func(i, j int) bool {
-		h1 := ""
-		h2 := ""
-		if attachableListeners[i].Source.Hostname != nil {
-			h1 = string(*attachableListeners[i].Source.Hostname)
-		}
-		if attachableListeners[j].Source.Hostname != nil {
-			h2 = string(*attachableListeners[j].Source.Hostname)
-		}
-		return h1 == GetMoreSpecificHostname(h1, h2)
-	})
-
-	for _, l := range attachableListeners {
-		routeAllowed, routeAttached, routeHostnamesUnique := bindToListenerL4(
-			l,
-			route,
-			gw,
-			namespaces,
-			portHostnamesMap,
-			refStatus,
-		)
-		allowed = allowed || routeAllowed
-		attached = attached || routeAttached
-		hostnamesUnique = hostnamesUnique || routeHostnamesUnique
-		attachedToAtLeastOneValidListener = attachedToAtLeastOneValidListener || (routeAttached && l.Valid)
-	}
-
 	if !attached {
 		if !allowed {
 			return conditions.NewRouteNotAllowedByListeners(), false
@@ -681,7 +640,7 @@ func tryToAttachL4RouteToListeners(
 		return conditions.NewRouteNoMatchingListenerHostname(), false
 	}
 
-	if !attachedToAtLeastOneValidListener {
+	if !attachableListener.Valid {
 		return conditions.NewRouteInvalidListener(), true
 	}
 
@@ -754,7 +713,7 @@ func bindL7RouteToListeners(
 			continue
 		}
 
-		attachment, attachableListeners := validateParentRef(ref, gw)
+		attachment, attachableListener := validateParentRef(ref, gw)
 
 		if route.RouteType == RouteTypeGRPC && isHTTP2Disabled(gw.EffectiveNginxProxy) {
 			msg := "HTTP2 is disabled - cannot configure GRPCRoutes"
@@ -775,11 +734,10 @@ func bindL7RouteToListeners(
 			}
 		}
 
-		// Try to attach Route to all matching listeners
-
-		cond, attached := tryToAttachL7RouteToListeners(
+		// Try to attach Route to the matching listener
+		cond, attached := tryToAttachL7RouteToListener(
 			ref.Attachment,
-			attachableListeners,
+			attachableListener,
 			route,
 			gw,
 			namespaces,
@@ -808,19 +766,16 @@ func isHTTP2Disabled(npCfg *EffectiveNginxProxy) bool {
 	return *npCfg.DisableHTTP2
 }
 
-// tryToAttachRouteToListeners tries to attach the route to the listeners that match the parentRef and the hostnames.
-// There are two cases:
-// (1) If it succeeds in attaching at least one listener it will return true. The returned condition will be empty if
-// at least one of the listeners is valid. Otherwise, it will return the failure condition.
-// (2) If it fails to attach the route, it will return false and the failure condition.
-func tryToAttachL7RouteToListeners(
+// tryToAttachL7RouteToListener attempts to attach a L7 route to the given listener.
+// Returns a condition and whether the attachment was successful.
+func tryToAttachL7RouteToListener(
 	refStatus *ParentRefAttachmentStatus,
-	attachableListeners []*Listener,
+	attachableListener *Listener,
 	route *L7Route,
 	gw *Gateway,
 	namespaces map[types.NamespacedName]*apiv1.Namespace,
 ) (conditions.Condition, bool) {
-	if len(attachableListeners) == 0 {
+	if attachableListener == nil {
 		return conditions.NewRouteInvalidListener(), false
 	}
 
@@ -848,15 +803,7 @@ func tryToAttachL7RouteToListeners(
 		return true, true
 	}
 
-	var attachedToAtLeastOneValidListener bool
-
-	var allowed, attached bool
-	for _, l := range attachableListeners {
-		routeAllowed, routeAttached := bind(l)
-		allowed = allowed || routeAllowed
-		attached = attached || routeAttached
-		attachedToAtLeastOneValidListener = attachedToAtLeastOneValidListener || (routeAttached && l.Valid)
-	}
+	allowed, attached := bind(attachableListener)
 
 	if !attached {
 		if !allowed {
@@ -865,57 +812,31 @@ func tryToAttachL7RouteToListeners(
 		return conditions.NewRouteNoMatchingListenerHostname(), false
 	}
 
-	if !attachedToAtLeastOneValidListener {
+	if !attachableListener.Valid {
 		return conditions.NewRouteInvalidListener(), true
 	}
 
 	return conditions.Condition{}, true
 }
 
-// findAttachableListeners returns a list of attachable listeners and whether the listener exists for a non-empty
-// sectionName.
-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 && (ref.Port == nil || l.Source.Port == *ref.Port) {
-					return []*Listener{l}, true
-				}
-				if ref.Port != nil && l.Source.Port != *ref.Port {
-					return nil, false
-				}
-				return nil, true
-			}
-		}
-		return nil, false
-	}
-
-	// 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)
-				}
-			}
-		}
-		return attachableListeners, foundListener
-	}
+// findAttachableListener returns the attachable listener that matches the sectionName and port.
+// Since buildSectionNameRefs always expands to concrete sectionName+port combinations,
+// we only need to handle the sectionName case.
+func findAttachableListener(ref *ParentRef, listeners []*Listener) (*Listener, bool) {
+	sectionName := string(*ref.SectionName)
 
-	// 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)
+		if l.Name == sectionName {
+			if l.Attachable && (ref.Port == nil || l.Source.Port == *ref.Port) {
+				return l, true
+			}
+			if ref.Port != nil && l.Source.Port != *ref.Port {
+				return nil, false
+			}
+			return nil, true
 		}
 	}
-	return attachableListeners, len(listeners) > 0
+	return nil, false
 }
 
 func findAcceptedHostnames(listenerHostname *v1.Hostname, routeHostnames []v1.Hostname) []string {
@@ -1057,13 +978,6 @@ func getHostname(h *v1.Hostname) string {
 	return string(*h)
 }
 
-func getSectionName(s *v1.SectionName) string {
-	if s == nil {
-		return ""
-	}
-	return string(*s)
-}
-
 func validateHostnames(hostnames []v1.Hostname, path *field.Path) error {
 	var allErrs field.ErrorList