Fixup: Sort paths based on length rather than lexi

[davinci26]

Since Envoy is greedy matching path routes, order is important. Contour decides to sort the routes in a way that is not really intuitive and can lead to suprises.

In particular even tho the comment in the code state that routes are ordered based on legnth the reality is that they are sorted based on string comparison. This PR fixes this.

• I think the current behaviour doesnt make much sense and it is a bit brittle.
• Updating the behaviour has significant update risk since there might be folks that rely on this routing behaviour without really knowing it.
• Should we even merge this PR? I am of two minds and I would like some input:

1. Option (1): Merge it as and make a clear changelog/announcement about the fix
2. Option (2): Create a config flag with a feature-flag e.g. route_sorting_strategy and switch the implementation to not do sorting when the flag is present. That way it allows folks to opt-out from the sorting as they need to.

Longest path based matching kinda makes sense to me now that I know about it, but it is rough edge than needs users to be familiar with contour and it is harder to socialize in larger teams.

[davinci26]

Unclear if the CI will pass, will fix tomorrow tho. Looking for your feedback folks!

[sunjayBhatia]

I'm currently leaning toward marking this as a major changelog and proceeding, since this really is a bug fix to get back to how the intended behavior is described, but would appreciate further thoughts

[sunjayBhatia]

Also @davinci26 if you had an example that caused you to run into the problem we discussed on regex match ordering, would be great to add to the tests if not already there at least for documentation/posterity purposes, can of course change any specific strings from your exact example

[davinci26]

Also @davinci26 if you had an example that caused you to run into the problem we discussed on regex match ordering, would be great to add to the tests if not already there at least for documentation/posterity purposes, can of course change any specific strings from your exact example

I added the test in shorter_test.go

// Note that regex matches sort before prefix matches.
{
	PathMatchCondition: matchRegex("/athis/is/the/longest"),
},
{
	PathMatchCondition: matchRegex(`/foo((\/).*)*`),
},

See: https://go.dev/play/p/nIn3eyd-XIX

[codecov]

Codecov Report

Merging #5752 (b5c76a0) into main (438745a) will increase coverage by 0.02%.
Report is 15 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5752      +/-   ##
==========================================
+ Coverage   78.56%   78.59%   +0.02%     
==========================================
  Files         138      138              
  Lines       19163    19198      +35     
==========================================
+ Hits        15056    15088      +32     
- Misses       3820     3822       +2     
- Partials      287      288       +1     

Files	Coverage Δ
internal/sorter/sorter.go	100.00% <100.00%> (ø)

... and 6 files with indirect coverage changes

[davinci26]

@sunjayBhatia all comments addressed, the CI should go green and it should be ready for merge

[clayton-gonsalves]

lgtm, modulo one question/request

[davinci26]

@sunjayBhatia ptal, this should be good now

[sunjayBhatia]

Will leave open for a bit so @skriss and others can get more thoughts in

But overall I think this looks good, LGTM, thanks for fixing this up!

[skriss]

Otherwise LGTM

[davinci26]

DCO is not enforced with commit suggestions

[davinci26]

Unless there are any flakes @skriss this should be good-to-go