linkerd / linkerd2

Ultralight, security-first service mesh for Kubernetes. Main repo for Linkerd 2.x.
https://linkerd.io
Apache License 2.0
10.61k stars 1.27k forks source link

policy(feat): `GrpcRoute` index support #12584

Closed the-wondersmith closed 4 months ago

the-wondersmith commented 5 months ago

Subject

Enable handling of Gateway GRPCRoute resources in linkerd-policy-controller-k8s-index.

Problem

Coming Soon™

Solution

Validation

Coming Soon™

Fixes Partially Addresses

adleong commented 4 months ago

I've reviewed the outbound side of this and I think this is on the right track. I have a suggestion for how to tighten up the internal types so that we can guarantee we don't accidentally mingle grpc matches with http routes and vice versa.

Keeping only HTTPRoutes when there are multiple route types attached to service sounds like a good first pass. Something to think about going forward we refine this: Should we be signalling that the dropped GrpcRoutes in this case are not accepted by their resource status?

adleong commented 4 months ago

According to the docs, this should be "first come first served": https://gateway-api.sigs.k8s.io/api-types/grpcroute/#cross-serving where the first route type should be accepted but any different route types added after should be rejected.

the-wondersmith commented 4 months ago

According to the docs, this should be "first come first served": https://gateway-api.sigs.k8s.io/api-types/grpcroute/#cross-serving where the first route type should be accepted but any different route types added after should be rejected.

I'd seen that, but had also seen a few comments sprinkled through that call out a similar concern around differences in the way that potentially cross-namespace parent refs are handled in linkerd-policy-controller-k8s-index vs linkerd-policy-controller-k8s-status. Also IIRC Oliver said that he was pretty sure the current implementation would allow both a linkerd and a gateway HTTPRoute to be attached to the same service without a fuss. Long-thought-short I wasn't sure if (or where) we're currently enforcing the "first come first served" rule, so it was on my list of i's to dot and t's to cross here.

... Something to think about going forward we refine this: Should we be signaling that the dropped {routes} in this case are not accepted by their resource status?

I say "absolutely, yes". I'd even float the idea of emitting Events for dropped routes to help ease user experience vis-a-vis monitoring and debugging.

For example:

screenshot

the-wondersmith commented 4 months ago

Just commenting for historical context purposes -

PR was opened to facilitate code review, but its overall size and content doesn't mesh with internal change management strategy. I'm closing this PR now that code review is complete and will be breaking the changes out into smaller chunks of mergeable changes.