fix(gateway): tolerate optional SectionName in access-points status (#1134)

* fix(gateway): tolerate optional SectionName in access-points status

Since `cc8a8e1` [1], BuildGatewayAccessPointsStatus treats a nil
ParentReference.SectionName as a fatal error via
fetchGatewayByListener, which aborts the whole TCP status update.

Such situation leads to spamming of "missing sectionName" error lines
on every reconcile. Even though nothing is functionally impacted the
reporting of .status.kubernetesResources.gateway and
status.addons.konnectivity.gateway isn't done, so any future logic
relying on it would be broken.

The Gateway API spec makes SectionName optional [2] ("When unspecified
(empty string), this will reference the entire resource"), and clients
like the Kamaji CAPI control-plane provider do not currently emit it
(at least until [3] is merged) so valid Route attachments end up
blocking status progression.

Consequently relaxed the behavior by introducing
resolveMatchingListeners:
  - SectionName set -> existing listener-name indexer fast path;
  - SectionName nil/empty -> fetch the Gateway by namespace/name and
    emit one access point per listener, optionally filtered by
    ParentRef.Port;
  - missing or unprogrammed Gateways are skipped silently so a single
    unresolvable parentRef cannot abort the whole status update.

FindMatchingListener stays strict and keeps backing the indexer path.

Tests cover the unset, port-filter, missing-gateway, and not-programmed
cases.

[1] https://github.com/clastix/kamaji/pull/1074
[2] https://gateway-api.sigs.k8s.io/reference/spec/#parentreference
[3] https://github.com/clastix/cluster-api-control-plane-provider-kamaji/pull/338

Signed-off-by: Pierre Cheynier <p.cheynier@criteo.com>

* fixup! fix(gateway): tolerate optional SectionName in access-points

Address review feedbacks on clastix/kamaji#1134:

* discriminate "no gateway with that listener" from real failures.
  fetchGatewayByListener now returns a typed errGatewayListenerNotFound
  in that case and propagates every other error.

* in the "unset SectionName" branch, restrict listeners to those using
  Protocol == TLS via a new isEligibleListener helper, in which the port
  matching test is also moved.

Tests cover non-TLS listener filtering and error propagation from
BuildGatewayAccessPointsStatus.

Signed-off-by: Pierre Cheynier <p.cheynier@criteo.com>

* fixup! fix(gateway): tolerate optional SectionName in access-points

Address new review feedback on clastix/kamaji#1134:
* apply isEligibleListener on the SectionName fast path too, so a buggy
  Gateway controller cannot leave us emitting https://... access points
  for non-TLS listeners or for listeners using a port distinct from
  parentRef.Port.

Tests cover protocol-mismatch and port-mismatch fast-path cases.

Signed-off-by: Pierre Cheynier <p.cheynier@criteo.com>

---------

Signed-off-by: Pierre Cheynier <p.cheynier@criteo.com>

internal/resources/k8s_gateway_resource_test.go

@@ -32,6 +32,7 @@ var _ = BeforeSuite(func() {
 	runtimeScheme = runtime.NewScheme()
 	Expect(scheme.AddToScheme(runtimeScheme)).To(Succeed())
 	Expect(kamajiv1alpha1.AddToScheme(runtimeScheme)).To(Succeed())
+	Expect(gatewayv1.Install(runtimeScheme)).To(Succeed())
 	Expect(gatewayv1alpha2.Install(runtimeScheme)).To(Succeed())
 })
 
@@ -267,4 +268,242 @@ var _ = Describe("KubernetesGatewayResource", func() {
 			Expect(listener.Port).To(Equal(gatewayv1.PortNumber(80)))
 		})
 	})
+
+	Describe("BuildGatewayAccessPointsStatus", func() {
+		var (
+			gwNamespace = "gateway-system"
+			gwName      = "test-gateway"
+			gateway     *gatewayv1.Gateway
+			route       *gatewayv1alpha2.TLSRoute
+			fakeClient  client.Client
+		)
+
+		// Builds a RouteStatus with a single Accepted parent using the
+		// supplied ParentReference.
+		buildRouteStatus := func(ref gatewayv1.ParentReference) gatewayv1alpha2.RouteStatus {
+			return gatewayv1alpha2.RouteStatus{
+				Parents: []gatewayv1.RouteParentStatus{{
+					ParentRef: ref,
+					Conditions: []metav1.Condition{{
+						Type:               string(gatewayv1.RouteConditionAccepted),
+						Status:             metav1.ConditionTrue,
+						Reason:             "Accepted",
+						LastTransitionTime: metav1.Now(),
+					}},
+				}},
+			}
+		}
+
+		BeforeEach(func() {
+			gateway = &gatewayv1.Gateway{
+				ObjectMeta: metav1.ObjectMeta{Name: gwName, Namespace: gwNamespace},
+				Spec: gatewayv1.GatewaySpec{
+					Listeners: []gatewayv1.Listener{
+						{Name: "kube-apiserver", Port: 31443, Protocol: gatewayv1.TLSProtocolType},
+						{Name: "konnectivity-server", Port: 32132, Protocol: gatewayv1.TLSProtocolType},
+						// Non-TLS listener on the same Gateway: it must not be turned
+						// into a TLSRoute access point.
+						{Name: "http-noise", Port: 8080, Protocol: gatewayv1.HTTPProtocolType},
+					},
+				},
+				Status: gatewayv1.GatewayStatus{
+					Conditions: []metav1.Condition{{
+						Type:               string(gatewayv1.GatewayConditionProgrammed),
+						Status:             metav1.ConditionTrue,
+						Reason:             "Programmed",
+						LastTransitionTime: metav1.Now(),
+					}},
+				},
+			}
+
+			route = &gatewayv1alpha2.TLSRoute{
+				ObjectMeta: metav1.ObjectMeta{Name: "tcp", Namespace: "tenant-ns"},
+				Spec: gatewayv1alpha2.TLSRouteSpec{
+					Hostnames: []gatewayv1.Hostname{"tcp.example.com"},
+				},
+			}
+
+			fakeClient = fake.NewClientBuilder().
+				WithScheme(runtimeScheme).
+				WithObjects(gateway).
+				WithIndex(&gatewayv1.Gateway{}, kamajiv1alpha1.GatewayListenerNameKey, (&kamajiv1alpha1.GatewayListener{}).ExtractValue()).
+				Build()
+		})
+
+		It("builds a single access point when the parentRef specifies a sectionName", func() {
+			section := gatewayv1.SectionName("konnectivity-server")
+			ns := gatewayv1.Namespace(gwNamespace)
+
+			statuses := buildRouteStatus(gatewayv1.ParentReference{
+				Name:        gatewayv1.ObjectName(gwName),
+				Namespace:   &ns,
+				SectionName: &section,
+			})
+
+			aps, err := resources.BuildGatewayAccessPointsStatus(ctx, fakeClient, route, statuses)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(aps).To(HaveLen(1))
+			Expect(aps[0].Port).To(Equal(gatewayv1.PortNumber(32132)))
+			Expect(aps[0].Value).To(Equal("https://tcp.example.com:32132"))
+		})
+
+		It("ignores a non-TLS listener even when explicitly selected via sectionName", func() {
+			section := gatewayv1.SectionName("http-noise")
+			ns := gatewayv1.Namespace(gwNamespace)
+
+			statuses := buildRouteStatus(gatewayv1.ParentReference{
+				Name:        gatewayv1.ObjectName(gwName),
+				Namespace:   &ns,
+				SectionName: &section,
+			})
+
+			aps, err := resources.BuildGatewayAccessPointsStatus(ctx, fakeClient, route, statuses)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(aps).To(BeEmpty())
+		})
+
+		It("ignores a TLS listener whose port disagrees with parentRef.Port", func() {
+			section := gatewayv1.SectionName("kube-apiserver")
+			ns := gatewayv1.Namespace(gwNamespace)
+			mismatch := gatewayv1.PortNumber(9999)
+
+			statuses := buildRouteStatus(gatewayv1.ParentReference{
+				Name:        gatewayv1.ObjectName(gwName),
+				Namespace:   &ns,
+				SectionName: &section,
+				Port:        &mismatch,
+			})
+
+			aps, err := resources.BuildGatewayAccessPointsStatus(ctx, fakeClient, route, statuses)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(aps).To(BeEmpty())
+		})
+
+		It("builds one access point per listener when sectionName is unset", func() {
+			ns := gatewayv1.Namespace(gwNamespace)
+
+			statuses := buildRouteStatus(gatewayv1.ParentReference{
+				Name:      gatewayv1.ObjectName(gwName),
+				Namespace: &ns,
+			})
+
+			aps, err := resources.BuildGatewayAccessPointsStatus(ctx, fakeClient, route, statuses)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(aps).To(HaveLen(2))
+			ports := []gatewayv1.PortNumber{aps[0].Port, aps[1].Port}
+			Expect(ports).To(ConsistOf(gatewayv1.PortNumber(31443), gatewayv1.PortNumber(32132)))
+		})
+
+		It("filters listeners by port when sectionName is unset but port is specified", func() {
+			ns := gatewayv1.Namespace(gwNamespace)
+			port := gatewayv1.PortNumber(31443)
+
+			statuses := buildRouteStatus(gatewayv1.ParentReference{
+				Name:      gatewayv1.ObjectName(gwName),
+				Namespace: &ns,
+				Port:      &port,
+			})
+
+			aps, err := resources.BuildGatewayAccessPointsStatus(ctx, fakeClient, route, statuses)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(aps).To(HaveLen(1))
+			Expect(aps[0].Port).To(Equal(port))
+		})
+
+		It("defaults the parent namespace to the route namespace when unset", func() {
+			// Gateway is in the route's namespace so the nil-namespace default kicks in.
+			colocated := gateway.DeepCopy()
+			colocated.Namespace = route.Namespace
+
+			c := fake.NewClientBuilder().
+				WithScheme(runtimeScheme).
+				WithObjects(colocated).
+				WithIndex(&gatewayv1.Gateway{}, kamajiv1alpha1.GatewayListenerNameKey, (&kamajiv1alpha1.GatewayListener{}).ExtractValue()).
+				Build()
+
+			statuses := buildRouteStatus(gatewayv1.ParentReference{
+				Name: gatewayv1.ObjectName(gwName),
+			})
+
+			aps, err := resources.BuildGatewayAccessPointsStatus(ctx, c, route, statuses)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(aps).To(HaveLen(2))
+		})
+
+		It("skips silently when the referenced Gateway is missing", func() {
+			ns := gatewayv1.Namespace("does-not-exist")
+
+			statuses := buildRouteStatus(gatewayv1.ParentReference{
+				Name:      gatewayv1.ObjectName("ghost-gateway"),
+				Namespace: &ns,
+			})
+
+			aps, err := resources.BuildGatewayAccessPointsStatus(ctx, fakeClient, route, statuses)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(aps).To(BeEmpty())
+		})
+
+		It("skips Gateways that are not Programmed", func() {
+			notProgrammed := gateway.DeepCopy()
+			notProgrammed.Status.Conditions = nil
+
+			c := fake.NewClientBuilder().
+				WithScheme(runtimeScheme).
+				WithObjects(notProgrammed).
+				WithIndex(&gatewayv1.Gateway{}, kamajiv1alpha1.GatewayListenerNameKey, (&kamajiv1alpha1.GatewayListener{}).ExtractValue()).
+				Build()
+
+			ns := gatewayv1.Namespace(gwNamespace)
+
+			statuses := buildRouteStatus(gatewayv1.ParentReference{
+				Name:      gatewayv1.ObjectName(gwName),
+				Namespace: &ns,
+			})
+
+			aps, err := resources.BuildGatewayAccessPointsStatus(ctx, c, route, statuses)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(aps).To(BeEmpty())
+		})
+
+		It("ignores non-TLS listeners when sectionName is unset", func() {
+			ns := gatewayv1.Namespace(gwNamespace)
+
+			statuses := buildRouteStatus(gatewayv1.ParentReference{
+				Name:      gatewayv1.ObjectName(gwName),
+				Namespace: &ns,
+			})
+
+			aps, err := resources.BuildGatewayAccessPointsStatus(ctx, fakeClient, route, statuses)
+			Expect(err).NotTo(HaveOccurred())
+			// Only the two TLS listeners may produce https:// access points.
+			Expect(aps).To(HaveLen(2))
+			ports := []gatewayv1.PortNumber{aps[0].Port, aps[1].Port}
+			Expect(ports).To(ConsistOf(gatewayv1.PortNumber(31443), gatewayv1.PortNumber(32132)))
+			Expect(ports).NotTo(ContainElement(gatewayv1.PortNumber(8080)))
+		})
+
+		It("propagates indexer failures from the sectionName fast path", func() {
+			// Fault-injection; build a client *without* the listener-name, i.e.
+			// WithIndex(&gatewayv1.Gateway{}, GatewayListenerNameKey, …).
+			// An error is expected to be propagated
+			c := fake.NewClientBuilder().
+				WithScheme(runtimeScheme).
+				WithObjects(gateway).
+				Build()
+
+			section := gatewayv1.SectionName("konnectivity-server")
+			ns := gatewayv1.Namespace(gwNamespace)
+
+			statuses := buildRouteStatus(gatewayv1.ParentReference{
+				Name:        gatewayv1.ObjectName(gwName),
+				Namespace:   &ns,
+				SectionName: &section,
+			})
+
+			_, err := resources.BuildGatewayAccessPointsStatus(ctx, c, route, statuses)
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("could not resolve gateway listeners for parentRef"))
+			Expect(err.Error()).To(ContainSubstring("failed to fetch gateway for listener"))
+		})
+	})
 })

internal/resources/k8s_gateway_utils.go

@@ -5,6 +5,7 @@ package resources
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/url"
 
@@ -12,6 +13,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/fields"
+	k8stypes "k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	gatewayv1 "sigs.k8s.io/gateway-api/apis/v1"
 	gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
@@ -19,6 +21,10 @@ import (
 	kamajiv1alpha1 "github.com/clastix/kamaji/api/v1alpha1"
 )
 
+// errGatewayListenerNotFound is returned when the indexer yields no Gateway
+// for the requested (namespace/name/listener) tuple.
+var errGatewayListenerNotFound = errors.New("no gateway found for listener")
+
 // fetchGatewayByListener uses the indexer to efficiently find a gateway with a specific listener.
 // This avoids the need to iterate through all listeners in a gateway.
 func fetchGatewayByListener(ctx context.Context, c client.Client, ref gatewayv1.ParentReference) (*gatewayv1.Gateway, error) {
@@ -38,7 +44,7 @@ func fetchGatewayByListener(ctx context.Context, c client.Client, ref gatewayv1.
 	}
 
 	if len(gatewayList.Items) == 0 {
-		return nil, fmt.Errorf("no gateway found with listener '%s'", *ref.SectionName)
+		return nil, fmt.Errorf("%w: %s", errGatewayListenerNotFound, *ref.SectionName)
 	}
 
 	// Since we're using a composite key with namespace/name/listener, we should get exactly one result
@@ -167,6 +173,20 @@ func CleanupTLSRoute(ctx context.Context, c client.Client, routeName, routeNames
 }
 
 // BuildGatewayAccessPointsStatus builds access points from route statuses.
+//
+// Per the Gateway API specification, ParentReference.SectionName is optional:
+// when unset (or empty), the Route attaches to every listener of the referenced
+// Gateway that accepts it (typically via port/protocol matching). We support
+// both cases:
+//
+//   - SectionName is set: resolve the Gateway via the listener-name indexer and
+//     build a single access point for that listener.
+//   - SectionName is nil/empty: resolve the Gateway by namespace/name and build
+//     an access point for each listener, optionally filtered by ParentRef.Port.
+//
+// Unresolvable or unprogrammed Gateways are skipped rather than returned as
+// errors, so that a single mis-attached parentRef does not block the whole
+// status update for the Route.
 func BuildGatewayAccessPointsStatus(ctx context.Context, c client.Client, route *gatewayv1alpha2.TLSRoute, routeStatuses gatewayv1alpha2.RouteStatus) ([]kamajiv1alpha1.GatewayAccessPoint, error) {
 	accessPoints := []kamajiv1alpha1.GatewayAccessPoint{}
 	routeNamespace := gatewayv1.Namespace(route.Namespace)
@@ -185,44 +205,123 @@ func BuildGatewayAccessPointsStatus(ctx context.Context, c client.Client, route
 			routeStatus.ParentRef.Namespace = &routeNamespace
 		}
 
-		// Use the indexer to efficiently find the gateway with the specific listener
-		gateway, err := fetchGatewayByListener(ctx, c, routeStatus.ParentRef)
+		listeners, err := resolveMatchingListeners(ctx, c, routeStatus.ParentRef)
 		if err != nil {
-			return nil, fmt.Errorf("could not fetch gateway with listener '%v': %w",
-				routeStatus.ParentRef.SectionName, err)
-		}
-		gatewayProgrammed := meta.IsStatusConditionTrue(
-			gateway.Status.Conditions,
-			string(gatewayv1.GatewayConditionProgrammed),
-		)
-		if !gatewayProgrammed {
-			continue
+			return nil, fmt.Errorf("could not resolve gateway listeners for parentRef '%s/%s': %w",
+				*routeStatus.ParentRef.Namespace, routeStatus.ParentRef.Name, err)
 		}
 
-		// Since we fetched the gateway using the indexer, we know the listener exists
-		// but we still need to get its details from the gateway spec
-		listener, err := FindMatchingListener(
-			gateway.Spec.Listeners, routeStatus.ParentRef,
-		)
-		if err != nil {
-			return nil, fmt.Errorf("failed to match listener: %w", err)
-		}
+		for _, listener := range listeners {
+			for _, hostname := range route.Spec.Hostnames {
+				rawURL := fmt.Sprintf("https://%s:%d", hostname, listener.Port)
+				parsedURL, err := url.Parse(rawURL)
+				if err != nil {
+					return nil, fmt.Errorf("invalid url: %w", err)
+				}
 
-		for _, hostname := range route.Spec.Hostnames {
-			rawURL := fmt.Sprintf("https://%s:%d", hostname, listener.Port)
-			parsedURL, err := url.Parse(rawURL)
-			if err != nil {
-				return nil, fmt.Errorf("invalid url: %w", err)
+				hostnameAddressType := gatewayv1.HostnameAddressType
+				accessPoints = append(accessPoints, kamajiv1alpha1.GatewayAccessPoint{
+					Type:  &hostnameAddressType,
+					Value: parsedURL.String(),
+					Port:  listener.Port,
+				})
 			}
-
-			hostnameAddressType := gatewayv1.HostnameAddressType
-			accessPoints = append(accessPoints, kamajiv1alpha1.GatewayAccessPoint{
-				Type:  &hostnameAddressType,
-				Value: parsedURL.String(),
-				Port:  listener.Port,
-			})
 		}
 	}
 
 	return accessPoints, nil
 }
+
+// resolveMatchingListeners returns the listeners of the Gateway referenced by
+// ref that should contribute access points for the enclosing Route.
+//
+// When ref.SectionName is set, exactly one listener is returned (looked up via
+// the name indexer for efficiency). When SectionName is nil or empty, every
+// listener of the Gateway is considered and, if ref.Port is set, further
+// filtered to listeners exposing that port.
+//
+// If the referenced Gateway is not found, is not Programmed, or has no
+// matching listener, an empty slice is returned with a nil error: a single
+// unresolvable parentRef must not fail the whole status update.
+func resolveMatchingListeners(ctx context.Context, c client.Client, ref gatewayv1.ParentReference) ([]gatewayv1.Listener, error) {
+	hasSectionName := ref.SectionName != nil && *ref.SectionName != ""
+
+	// Fast path: look the Gateway up by its specific listener name.
+	if hasSectionName {
+		gateway, err := fetchGatewayByListener(ctx, c, ref)
+		if err != nil {
+			// Only the "no gateway with that listener" case is benign and
+			// must not block the status update. Failures are propagated.
+			if errors.Is(err, errGatewayListenerNotFound) {
+				return nil, nil
+			}
+
+			return nil, fmt.Errorf("failed to fetch gateway for listener: %w", err)
+		}
+
+		if !isGatewayProgrammed(gateway) {
+			return nil, nil
+		}
+
+		listener, err := FindMatchingListener(gateway.Spec.Listeners, ref)
+		if err != nil {
+			return nil, fmt.Errorf("failed to match listener: %w", err)
+		}
+
+		if !isEligibleListener(listener, ref) {
+			return nil, nil
+		}
+
+		return []gatewayv1.Listener{listener}, nil
+	}
+
+	// SectionName unset: resolve the Gateway by namespace/name.
+	gateway := &gatewayv1.Gateway{}
+	key := k8stypes.NamespacedName{Namespace: string(*ref.Namespace), Name: string(ref.Name)}
+	if err := c.Get(ctx, key, gateway); err != nil {
+		if k8serrors.IsNotFound(err) {
+			return nil, nil
+		}
+
+		return nil, fmt.Errorf("failed to fetch gateway %s: %w", key, err)
+	}
+
+	if !isGatewayProgrammed(gateway) {
+		return nil, nil
+	}
+
+	matching := make([]gatewayv1.Listener, 0, len(gateway.Spec.Listeners))
+	for _, listener := range gateway.Spec.Listeners {
+		if !isEligibleListener(listener, ref) {
+			continue
+		}
+
+		matching = append(matching, listener)
+	}
+
+	return matching, nil
+}
+
+func isGatewayProgrammed(gateway *gatewayv1.Gateway) bool {
+	return meta.IsStatusConditionTrue(
+		gateway.Status.Conditions,
+		string(gatewayv1.GatewayConditionProgrammed),
+	)
+}
+
+// isEligibleListener reports whether a Gateway listener can contribute an
+// access point for the TLSRoute attachment, ie.:
+//   - its protocol is TLS (BuildGatewayAccessPointsStatus is invoked for
+//     TLSRoutes only, so non-TLS listeners can never legally attach);
+//   - and, if ref.Port is set, its port matches ref.Port.
+func isEligibleListener(listener gatewayv1.Listener, ref gatewayv1.ParentReference) bool {
+	if listener.Protocol != gatewayv1.TLSProtocolType {
+		return false
+	}
+
+	if ref.Port != nil && listener.Port != *ref.Port {
+		return false
+	}
+
+	return true
+}