fix(route-http-match): adjust route matching to more closely align to…

[anthoturc]

… gateway api httproute spec

also address edge case around ties between exact and prefix match

[adleong]

Looking at how route precedence ordering happens, it seems like this logic is actually spread across a few different places and the result may not be what we want.

In the policy controller, routes are sorted by timestamp then by name: https://github.com/linkerd/linkerd2/blob/main/policy-controller/grpc/src/outbound.rs#L677 with older timestamps getting sorted to the front of the list. This is intended to give HTTPRoutes with older timestamps higher priority.

Then in the proxy, we define a cmp which compares based on match length (and match type, with your change).

Finally, the proxy uses that cmp to pick the maximum match (https://github.com/linkerd/linkerd2-proxy/blob/main/linkerd/http/route/src/lib.rs#L93). But if there are multiple maximal matches then max picks the last one. According to the sorting from the policy controller, this would be the one with the latest timestamp rather than oldest.

So our priority matching may be wrong in multiple ways. I'd recommend the following actions:

1. verify this priority matching behavior manually or with an integration test
2. as a short-term solution, we can update the match picking logic in the proxy to prefer earlier matches in the list when there's a tie for maximum instead of preferring the last one
3. as a long-term solution, it's confusing that the priority logic is split across two different locations (timestamp/name in one place, match length in another). it would be better to do all the priority sorting in one place rather than splitting it up. the full ordering rules are defined in https://gateway-api.sigs.k8s.io/reference/api-spec/main/spec/#httprouterule

[anthoturc]

I agree that it's a little confusing for the matching to be split this way. I'm interested in the longer term fix here. I'll take some time to refactor and see where I land. I did add some unit tests for the precedence ordering but I can add an inter test for more complete verification.

I think in my ideal world, the config and spec are completely handled in the data plane.

edit: I'll create a separate task for doing the long term work. For now, I will go with the short-term approach and the integration test for verification.

[adleong]

Something to keep in mind is that I don't think we send the resource timestamps to the proxy so the data plane may not have all the necessary information to determine the full priority order. In my opinion, the control plane is likely a better place to do this sorting.

I'm not sure why we originally decided to do match length sorting in the data plane but it might have been because we were calculating the match length based on the length of the matched segment in the URI, which is only known at request time by the proxy. But it looks like the spec https://gateway-api.sigs.k8s.io/reference/api-spec/main/spec/#httprouterule doesn't say we have to do this and leaves the priority order of Regex matches up to the implementation.

[anthoturc]

the latest commit adds verification of the gateway htttproute spec via integration tests. also I did introduce a find_max that will propagate the "first" best match (as opposed to the last) in case of a tie

[cratelyn]

i see that i have been requested as a reviewer for this.

Something to keep in mind is that I don't think we send the resource timestamps to the proxy so the data plane may not have all the necessary information to determine the full priority order. In my opinion, the control plane is likely a better place to do this sorting.

i'd like to defer to @adleong, who has context for this already. if her advice is that the control plane would be a better place to do this sorting, particularly if the proxy does not have a complete picture of the information to determine priority order, perhaps we should investigate a fix there instead?

2. as a short-term solution, we can update the match picking logic in the proxy to prefer earlier matches in the list when there's a tie for maximum instead of preferring the last one

my understanding is that this branch currently performs option 2 suggested above. if so, i would not block on this, but would want to see a ticket filed tracking the long-term solution described in step 3.

[anthoturc]

@cratelyn, added you as a reviewer in case you had some cycles

the PR does implement option 2. i created this ticket for the long-term work in 3 and will start working on it as a fast follow to this

[anthoturc]

fwiw, here is the follow up: linkerd/linkerd2#15614