handle Route conflicts with HTTPRoute.Matches

[lubronzhan]

Fix #3608

Now if there is conflicts between HTTPRoute's match, then only the HTTPRoute with

• older creationTimestamp
• alphabetically smaller on namespace/name

, will be chosen as the valid HTTPRoute, other HTTPRoute will be marked as conflict condition

TODO:

• Mark HTTPRoute as conflict condition if needed
• E2E Test

Signed-off-by: lubronzhan [email protected]

[codecov]

Codecov Report

Attention: Patch coverage is 87.71930% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 81.59%. Comparing base (3d85617) to head (2ae98cc).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6188   +/-   ##
=======================================
  Coverage   81.58%   81.59%           
=======================================
  Files         133      133           
  Lines       15815    15861   +46     
=======================================
+ Hits        12902    12941   +39     
- Misses       2618     2624    +6     
- Partials      295      296    +1     

Files	Coverage Δ
internal/dag/dag.go	98.44% <100.00%> (+0.04%)	⬆️
internal/status/routeconditions.go	53.01% <ø> (ø)
internal/dag/gatewayapi_processor.go	93.19% <86.79%> (-0.32%)	⬇️

[lubronzhan]

Only failure is context exceeded. I think it's unrelated to the change since the change that affects all route are sorting routes before compute

--- FAIL: TestGRPC (0.76s)
    --- FAIL: TestGRPC/StreamRoutes (0.15s)
        server_test.go:200: 
            	Error Trace:	/Users/runner/work/contour/contour/internal/xdscache/v3/server_test.go:298
            	            				/Users/runner/work/contour/contour/internal/xdscache/v3/server_test.go:200
            	            				/Users/runner/work/contour/contour/internal/xdscache/v3/server_test.go:276
            	Error:      	Received unexpected error:
            	            	rpc error: code = DeadlineExceeded desc = context deadline exceeded
            	Test:       	TestGRPC/StreamRoutes

All passed now

[lubronzhan]

Hi @projectcontour/maintainers any chance to get this merged?

[skriss]

Thanks @lubronzhan, I think this change makes sense though it would definitely be nice to have an upstream Gateway API conformance test to verify against, maybe we can contribute that separately

[lubronzhan]

Thanks @lubronzhan, I think this change makes sense though it would definitely be nice to have an upstream Gateway API conformance test to verify against, maybe we can contribute that separately

Do you have any reference. Just saw we are referencing the conformance test here

Happy to put it into different PR

[skriss]

Thanks @lubronzhan, I think this change makes sense though it would definitely be nice to have an upstream Gateway API conformance test to verify against, maybe we can contribute that separately

Do you have any reference. Just saw we are referencing the conformance test here

Happy to put it into different PR

Yeah it would be in the Gateway API repo, the conformance tests are here: https://github.com/kubernetes-sigs/gateway-api/tree/main/conformance/tests

You can see some of the past PRs to add tests here: https://github.com/kubernetes-sigs/gateway-api/pulls?q=is%3Apr+label%3Aarea%2Fconformance

Happy to pair/provide further pointers if needed

[skriss]

One case I'm not totally clear on is: what if a rule in a given HTTPRoute has multiple matches defined, and only one of those matches conflicts with another HTTPRoute? Should the entire rule be dropped, or only the match that conflicted? (matches within a rule are logically OR'ed)

https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io%2fv1.RouteConditionType docs for PartiallyInvalid seem to imply that the entire rule should be dropped, but it's not crystal clear from the spec. I think this PR's implementation is good for now (dropping the entire rule), but this is an example of something that could be clarified and encoded in the upstream conformance test.

[lubronzhan]

Sunjay also mentioned here, in upstream, it does indicate that we should drop at the rule level, if one rule is conflict, just drop it and leave other rules

// 1) Drop Rule(s): With this approach, implementations will drop the
// invalid Route Rule(s) until they are fully valid again. The message
// for this condition MUST start with the prefix "Dropped Rule" and
// include information about which Rules have been dropped. In this
// state, the "Accepted" condition MUST be set to "True" with the latest
// generation of the resource.

[lubronzhan]

Thanks @lubronzhan, I think this change makes sense though it would definitely be nice to have an upstream Gateway API conformance test to verify against, maybe we can contribute that separately

Do you have any reference. Just saw we are referencing the conformance test here
Happy to put it into different PR

Yeah it would be in the Gateway API repo, the conformance tests are here: kubernetes-sigs/gateway-api@main/conformance/tests

You can see some of the past PRs to add tests here: kubernetes-sigs/gateway-api/pulls (label:area/conformance)

Happy to pair/provide further pointers if needed

Just found out that I did created an issue for tracking conformance test #6227

Thanks for the pointers, will create one for gateway-api

[skriss]

👍 I'm going to go ahead and merge this now so it doesn't run into any conflicts, can always circle back and address any post-merge feedback if needed before release. Thanks again @lubronzhan!