[feat: gw api] Add eventhandlers for all the gateway resources #4148
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shraddhabang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
|
|
||
| // GetImpactedGatewaysFromParentRefs identifies Gateways affected by changes in parent references. | ||
| // Returns Gateways that are impacted and managed by the LB controller. | ||
| func GetImpactedGatewaysFromParentRefs(ctx context.Context, k8sClient client.Client, parentRefs []gwv1.ParentReference, resourceNamespace string, gwController string) ([]types.NamespacedName, error) { |
There was a problem hiding this comment.
It looks like the error is always ignored. I think we should either:
1/ Remove the error from the return value.
2/ Continue throwing an error, but logging that an error was encountered.
Good catch. I prefer to log the error to ignore the unknown gateways but continue with the impacted gateways. Fixed now.
| continue | ||
| } | ||
|
|
||
| if IsGatewayManagedByLBController(ctx, k8sClient, gw, gwController) { |
There was a problem hiding this comment.
It would be good to cache this calculation. Once we know a gw is impacted once, we shouldn't need to re-calculate this again. This would also prevent putting duplicate events into the queue.
Yes. I plan to add the caches later in separate dedicated PR to improve performance. That will also include the indexers. I have added these for the foundation phase.
| ) | ||
|
|
||
| // NewEnqueueRequestsForLoadBalancerConfigurationEvent creates handler for LoadBalancerConfiguration resources | ||
| func NewEnqueueRequestsForLoadBalancerConfigurationEvent(gwClassEventChan chan<- event.TypedGenericEvent[*gatewayv1.GatewayClass], |
There was a problem hiding this comment.
I think we need the gateway event chan here too, as a change in the lb config could impact the gateway object. We can let the framework handle de-duping events.
If you look at the logic to enqueueImpactedGateways, you will see that I enqueue all the impacted gwclasses first in its channel and then also fetch all the impacted gateways and then filter out those whose gwclasses are already enqueued to avoid dedupes.
| ctx context.Context, | ||
| svc *corev1.Service, | ||
| ) { | ||
| l4Routes, err := routeutils.ListL4Routes(ctx, h.k8sClient) |
There was a problem hiding this comment.
It would be good to log the error happened here. It might not be safe to use l4Routes if an error happens (e.g. it might be nil)
Good point. The idea behind this was to not block the other routes if failure is coming from other kind of route for eg. HTTPRoutes throwing errors but GRPCs are okay so we should atleast enqueue the GRPC route events. But I think you have a point. I have added the errors logs here but I prefer not to block the other non-impacting routes. The nil will be handled gracefully as its an array.
| // Returns a new slice containing only routes that reference the specified service. | ||
| func FilterRoutesBySvc(routes []preLoadRouteDescriptor, svc *corev1.Service) []preLoadRouteDescriptor { | ||
| if svc == nil || len(routes) == 0 { | ||
| return []preLoadRouteDescriptor{} |
There was a problem hiding this comment.
|
/lgtm /approve |
Description
This PR introduces a new event handling system for the Gateway API controllers to manage and process events from various Kubernetes resources efficiently. The implementation includes handlers for LoadBalancer configurations, Gateway classes, and related resources.
TODO: Implement indexers and caches to efficiently fetch the resources.
Sample logs
Checklist
README.md, or thedocsdirectory)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯