http-route-controller: watch ReferencePolicy lifecycle #156
Conversation
…HTTPRouteController
…Routes in From Namespace
create separate namespace for route directly in test
| if condition.Type == "Accepted" || | ||
| condition.Status == "True" { |
There was a problem hiding this comment.
Refactor uncovered that this should've been &&, so this check was erroneously passing on any condition with Status: "True", and was being called in a few places where conditionReady should have been called instead.
Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>
| // TODO: search by from.Group and from.Kind instead of assuming HTTPRoute | ||
| routes, err := r.Client.GetHTTPRoutesInNamespace(r.Context, string(from.Namespace)) |
There was a problem hiding this comment.
@sarahalsmiller could you look into how best to do this?
There was a problem hiding this comment.
After a few hours of digging through the dynamic and unstructured packages, I don't think it's strictly possible to search for multiple kinds in a single API call with the go-client (would love to be proven wrong here though). The most generic thing I can figure out how to do is potentially use a mapper to search for the routes that are available on the cluster and list each of those found group version kinds and apply our logic from there, but I'm not sure that even makes sense at the moment when we're only supporting two kinds.
There was a problem hiding this comment.
I'm thinking it's not worth more digging vs. just having r.Client.GetTCPRoutesInNamespace( ... ). Could wait and dig more if we wind up hitting rule of three
There was a problem hiding this comment.
👍 to revisiting this when we get to implementing TLSRoute or UDPRoute - something more generic might be possible with unstructured.Unstructured struct and scheme.Runtime using reflection to convert from kind strings to types as described in https://iximiuz.com/en/posts/kubernetes-api-go-types-and-common-machinery/ (and likely adding a RouteController interface), but definitely not straightforward.
Implemented as GetTCPRoutesInNamespace in #162
There was a problem hiding this comment.
Rather than doing things with strings and unstructured, if you really want you could just make a generic List/ListInNamespace function on the interface and pass in the object you want retrieved directly. The k8s client uses reflection for retrieving lists/objects anyway. It'd handle something like client.ListInNamespace(ctx, &gateway.HTTPRouteList{}, namespace). Which is a lot closer to the vanilla k8s client List method. TCPRoute would just then be client.ListInNamespace(ctx, &gateway.TCPRouteList{}, namespace). The function signature would just specify a runtimeclient.ObjectList.
The main two reasons you'll still want to wrap the k8s client call, even if it's a really thin wrapper over List are for:
K8sErrorwrapping for our retry middleware- Easy mocking for error injection for tests
I don't necessarily have much of a preference but think the per-type signatures are kind of nice in that you can just having the initialized structures passed back to you. Per-type also follow more of the pattern throughout most of the gatewayclient interface. But either way is fine with me. If you do change it though, I'd do that in a follow-up PR at this point, no need to re-block.
mikemorris
force-pushed
the
reference-policy-lifecycle
branch
from
e8bb3e5
to
dc8e1cd
Compare
| // TODO: search by from.Group and from.Kind instead of assuming HTTPRoute | ||
| routes, err := r.Client.GetHTTPRoutesInNamespace(r.Context, string(from.Namespace)) |
There was a problem hiding this comment.
Rather than doing things with strings and unstructured, if you really want you could just make a generic List/ListInNamespace function on the interface and pass in the object you want retrieved directly. The k8s client uses reflection for retrieving lists/objects anyway. It'd handle something like client.ListInNamespace(ctx, &gateway.HTTPRouteList{}, namespace). Which is a lot closer to the vanilla k8s client List method. TCPRoute would just then be client.ListInNamespace(ctx, &gateway.TCPRouteList{}, namespace). The function signature would just specify a runtimeclient.ObjectList.
The main two reasons you'll still want to wrap the k8s client call, even if it's a really thin wrapper over List are for:
K8sErrorwrapping for our retry middleware- Easy mocking for error injection for tests
I don't necessarily have much of a preference but think the per-type signatures are kind of nice in that you can just having the initialized structures passed back to you. Per-type also follow more of the pattern throughout most of the gatewayclient interface. But either way is fine with me. If you do change it though, I'd do that in a follow-up PR at this point, no need to re-block.
Changes proposed in this PR:
Adds a
Watchesclause to the HTTPRouteReconciler to watch for changes to ReferencePolicy objects and trigger reconciliation and re-validation for any HTTPRoutes that may be affected by changes.(Please don't view by commit, it's too messy to bother trying to interactive rebase cleanly 🙈)
How I've tested this PR:
GatewayClient.GetHTTPRoutesInNamespaceHTTPRouteReconciler.referencePolicyToRouteRequestsHow I expect reviewers to test this PR:
Reference Documentation:
For unimplemented GroupKind matching on ReferencePolicyFrom instead of assuming HTTPRoute, and BackendRef filtering on ReferencePolicyTo:
Checklist: