contour icon indicating copy to clipboard operation
contour copied to clipboard

handle Route conflicts with HTTPRoute.Matches

Open lubronzhan opened this issue 1 year ago • 3 comments

Fix https://github.com/projectcontour/contour/issues/3608

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

  • older creationTimestamp
  • alphabetically smaller on namespace/name

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

TODO:

  • [x] Mark HTTPProxy as conflict condition if needed
  • [x] E2E Test

Signed-off-by: lubronzhan [email protected]

lubronzhan avatar Feb 13 '24 07:02 lubronzhan

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

Impacted file tree graph

@@           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%) :arrow_up:
internal/status/routeconditions.go 53.01% <ø> (ø)
internal/dag/gatewayapi_processor.go 93.19% <86.79%> (-0.32%) :arrow_down:

codecov[bot] avatar Feb 14 '24 06:02 codecov[bot]

~~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 avatar Feb 16 '24 07:02 lubronzhan

hi @sunjayBhatia could you take a look

lubronzhan avatar Feb 27 '24 18:02 lubronzhan

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 14d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

  • Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
  • Mark this PR as fresh by commenting or pushing a commit
  • Close this PR
  • Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

github-actions[bot] avatar Apr 08 '24 00:04 github-actions[bot]

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

lubronzhan avatar Apr 11 '24 04:04 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

lubronzhan avatar Apr 16 '24 01:04 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: 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 avatar Apr 16 '24 16:04 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: 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 https://github.com/projectcontour/contour/issues/6227

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

lubronzhan avatar Apr 16 '24 22:04 lubronzhan

:+1: 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!

skriss avatar Apr 17 '24 19:04 skriss