-
Notifications
You must be signed in to change notification settings - Fork 631
Add support for GEP-1713: ListenerSets #11255
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| ) | ||
|
|
||
| // Trigger an event when the gateway changes. This can even be a change in listener sets attached to the gateway | ||
| go func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be in a goroutine ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, will create a goroutine under the hood
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we need to call Register only after krt has started. @stevenctl do you know if that issue was fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it's letting me respond here. For posterity: I believe the only issue is if we need the initial state, and luckily in this case we do not.
Signed-off-by: David Jumani <[email protected]>
| type ListenerSetReporter interface { | ||
| Listener(listener *gwv1.Listener) ListenerReporter | ||
| ListenerName(listenerName string) ListenerReporter | ||
| SetCondition(condition GatewayCondition) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided to keep this the same as the GatewayReporter since they are pretty much the same thing. This avoids too many changes in the code, especially around status reporting
| Name: gw.GetName(), | ||
| Namespace: gw.GetNamespace(), | ||
| } | ||
| irGW := d.inputs.CommonCollections.GatewayIndex.Gateways.GetKey(gwKey.ResourceName()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it might be possible that we miss the krt lookup and only have the api.Gateway here if we hit this path before translation finishes; we're guaranteed to eventually get it though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this should work because the controller should start only after krt is warm, IIRC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the WaitUntilSynced above in an attempt to fix that
| return h | ||
| } | ||
|
|
||
| func allowedListenerSet(gw *gwv1.Gateway, listenerSet *gwxv1a1.XListenerSet, namespaces krt.Collection[NamespaceMetadata]) (func(krt.HandlerContext, string) bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nit: alias the returned func or give it's args names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, added!
| Name: "example-gateway", | ||
| }, | ||
| }), | ||
| // TODO: Add this once istio adds support for listener sets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should eventually make our test translator handle getting istio-unknown types into krt clients so we can test for our own CRs too..
Signed-off-by: David Jumani <[email protected]>
| }) | ||
| } | ||
|
|
||
| func ResolveNamespace(namespace *apiv1.Namespace) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this exported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to remove it in the cleanup. Fixed
| Name: gw.GetName(), | ||
| Namespace: gw.GetNamespace(), | ||
| } | ||
| irGW := d.inputs.CommonCollections.GatewayIndex.Gateways.GetKey(gwKey.ResourceName()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this should work because the controller should start only after krt is warm, IIRC.
| ) | ||
|
|
||
| // Trigger an event when the gateway changes. This can even be a change in listener sets attached to the gateway | ||
| go func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we need to call Register only after krt has started. @stevenctl do you know if that issue was fixed?
Signed-off-by: David Jumani <[email protected]>
Signed-off-by: David Jumani <[email protected]>
Signed-off-by: David Jumani <[email protected]>
Signed-off-by: David Jumani <[email protected]>
Signed-off-by: David Jumani <[email protected]>
Signed-off-by: David Jumani <[email protected]>
| // Trigger an event when the gateway changes. This can even be a change in listener sets attached to the gateway | ||
| c.cfg.CommonCollections.GatewayIndex.Gateways.Register(func(o krt.Event[ir.Gateway]) { | ||
| gw := o.Latest() | ||
| c.reconciler.customEvents <- event.TypedGenericEvent[ir.Gateway]{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider adding select with default clause; so if the channel is full it won't get stuck.
i think the channel would then just need capacity of 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would that lead to missed events when a gateway is modified ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KRT handles a slow/blocking callback internally; it won't make other Register/dependent collections get stuck.
While we can do something like go func() { events <- ev }() here, I think the current code is fine.
Signed-off-by: David Jumani <[email protected]>
Signed-off-by: David Jumani <[email protected]>
Signed-off-by: David Jumani <[email protected]>
| listenerSetPolicies := h.policies.getTargetingPolicies(kctx, extensionsplug.GatewayAttachmentPoint, lsIR.ObjectSource, "", ls.GetLabels()) | ||
|
|
||
| for _, l := range ls.Spec.Listeners { | ||
| listenerSpecificPolicies := h.policies.getTargetingPolicies(kctx, extensionsplug.RouteAttachmentPoint, lsIR.ObjectSource, string(l.Name), ls.GetLabels()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we getting route attached policies here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is similar to fetching policies for listeners in the gateway
Ref:
| AttachedPolicies: toAttachedPolicies(h.policies.getTargetingPolicies(kctx, extensionsplug.RouteAttachmentPoint, out.ObjectSource, string(l.Name), i.GetLabels())), |
Signed-off-by: David Jumani <[email protected]>
Signed-off-by: David Jumani <[email protected]>
Description
Adds support for ListenerSets laid out in https://gateway-api.sigs.k8s.io/geps/gep-1713.
This works around the restriction of a maximum of 64 listeners defined on a gateway. Users can now define multiple listenersets and map them to the appropriate parent gateway
Fixes #10662
This PR also adds policy support for listener sets
Added tests for the same
Change Type
/kind new_feature
Changelog