[Gateway API] Support attaching routes from listeners originating from a ListenerSet#4639
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zac-nixon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| return nameCheck && nsCheck | ||
| } | ||
|
|
||
| func doesResourceAllowNamespace(ctx context.Context, fromNamespaces gwv1.FromNamespaces, labelSelector *metav1.LabelSelector, nsSelector namespaceSelector, resourceNamespace string, gw gwv1.Gateway) (bool, error) { |
There was a problem hiding this comment.
We want to re-use the logic between ListenerSet and Gateway.
| // a route to attach to it. | ||
| type listenerAttachmentHelper interface { | ||
| listenerAllowsAttachment(ctx context.Context, gw gwv1.Gateway, listener gwv1.Listener, route preLoadRouteDescriptor, matchedParentRef *gwv1.ParentReference, hostnamesFromHttpRoutes map[types.NamespacedName][]gwv1.Hostname, hostnamesFromGrpcRoutes map[types.NamespacedName][]gwv1.Hostname) ([]gwv1.Hostname, bool, *RouteData, error) | ||
| listenerAllowsAttachment(ctx context.Context, parentNamespace string, listener gwv1.Listener, route preLoadRouteDescriptor, matchedParentRef gwv1.ParentReference, hostnamesFromHttpRoutes map[int32]sets.Set[gwv1.Hostname], hostnamesFromGrpcRoutes map[int32]sets.Set[gwv1.Hostname]) ([]gwv1.Hostname, *RouteData, error) |
There was a problem hiding this comment.
There was a bug where we only considered conflicting hostnames using listener section name, however we need to use listener port to accurately capture all attached hostnames.
| // check namespace TODO --- Update for ListenerSet, should be ListenerSet namespace. | ||
| namespaceOK, err := attachmentHelper.namespaceCheck(ctx, parentNamespace, listener, route) | ||
| if err != nil { | ||
| return nil, false, nil, err |
There was a problem hiding this comment.
Removed returning false / true for attachment. Users can utilize RouteData being nil to signify successful attachment.
There was a problem hiding this comment.
can it be nil? i remember they need some attributes not empty
There was a problem hiding this comment.
check out the old code path. there was no case where we returned attachment = true with route data being nil.
| if !kindOK { | ||
| rd := GenerateRouteData(false, true, string(gwv1.RouteReasonNotAllowedByListeners), RouteStatusInfoRejectedMessageKindNotMatch, route.GetRouteNamespacedName(), route.GetRouteKind(), route.GetRouteGeneration(), gw, port, sectionName) | ||
| return nil, false, &rd, nil | ||
| return nil, new(GenerateRouteData(false, true, string(gwv1.RouteReasonNotAllowedByListeners), RouteStatusInfoRejectedMessageKindNotMatch, route.GetRouteNamespacedName(), route.GetRouteKind(), route.GetRouteGeneration(), matchedParentRef)), nil |
There was a problem hiding this comment.
This was suggested by my IDE. In go 1.26 we can use new() instead of &Value{}.
There was a problem hiding this comment.
I was able to consolidate this logic into loader.go, we didn't need all this additional logic.
|
/retest |
| Routes: loadedRoute, | ||
| Listeners: gw.Spec.Listeners, | ||
| AttachedRoutesMap: attachedRouteMap, | ||
| AttachedRoutesMap: mapResult.routesPerListener, |
There was a problem hiding this comment.
why do we remove Listeners from loaderResult? is it gonna be added back?
| // check namespace TODO --- Update for ListenerSet, should be ListenerSet namespace. | ||
| namespaceOK, err := attachmentHelper.namespaceCheck(ctx, parentNamespace, listener, route) | ||
| if err != nil { | ||
| return nil, false, nil, err |
There was a problem hiding this comment.
can it be nil? i remember they need some attributes not empty
|
/lgtm |
Description
Refactored the route, listener mapper to hopefully clean the code up a bit. This PR adds the ability for users to attach listeners from a ListenerSet to a Gateway. Things left to do:
1/ Add ListenerSet status updates
2/ Automated E2E test add.
See inline comments for more context.
Checklist
README.md, or thedocsdirectory)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯