contour

contour copied to clipboard

Contour is currently failing the main conformance test defined for route sorting: https://github.com/kubernetes-sigs/gateway-api/pull/1370

(Note, there is an issue filed to ensure that the spec is unambiguous: https://github.com/kubernetes-sigs/gateway-api/issues/1381)

We need to track the outcome of this and ensure Contour implements the correct behavior.

Tentatively adding to 1.23 for tracking.

@sunjayBhatia's PR to clarify this upstream has been merged: https://github.com/kubernetes-sigs/gateway-api/pull/1401

So looks like now we need to fix Contour to pass the conformance test!

For context, this comment/function is useful: https://github.com/projectcontour/contour/blob/5af0a4263d73bd24bb2dabf163c7c0e39814de4a/internal/xdscache/v3/route.go#L136-L154

also route sorter: https://github.com/projectcontour/contour/blob/5af0a4263d73bd24bb2dabf163c7c0e39814de4a/internal/sorter/sorter.go#L147

route sorter helper for comparing header + query matches: https://github.com/projectcontour/contour/blob/5af0a4263d73bd24bb2dabf163c7c0e39814de4a/internal/sorter/sorter.go#L113

we first sort the header matches within each dag.Route (should probably do the same for query matches)

then we sort the dag.Routes, which has a precedence first for the path match type and length of path, then orders them based on number of header and query matches

header matches here are then compared between routes, if theyre the same length the longer one wins, if the same, theyre compared w/ the header name one-by-one alphabetically

in the failing conformance test, we have two route matches that could apply to the request, both with the same path and number of header matches so the first one in the list should win

however, in contour currently the one with the header name that sorts first wins instead

a couple ideas come to mind to fix this:

• add an option to dag.Route, used only in routes generated by HTTPRoute processing, to signify we will not sort on header name alphabetical order to retain passed in order
• just never sort routes based on header name alphabetical order, and retain the passed in order from the HTTPRoute/HTTPProxy etc.
• add an option to dag.Route that would add a priority field, could sort on path match+length, then header length, then query length, then priority, and so-on

there is also a question of how to sort things that may overlap between different HTTPRoutes but that is another issue, to do with resource precedence (age, etc.)

e.g. just commenting out these lines makes the conformance tests pass: https://github.com/projectcontour/contour/blob/5af0a4263d73bd24bb2dabf163c7c0e39814de4a/internal/sorter/sorter.go#L119-L129

Hmm yeah I'm dubious of the value of comparing headers alphabetically, hard to say that that's a logical way to order things (other than just making it deterministic), but you know if we change the default behavior we'll probably break someone.

Will think about the other options & can discuss some more tomorrow.

I think options 1 or 3 from your list above are reasonable. 1 is probably simplest and could be refactored into 3 down the road if needed. I think it's probably best not to change HTTPProxy sorting logic, at least for now, especially given that the order you specify routes in the CRD spec has never really mattered for HTTPProxy.

I did find https://github.com/projectcontour/contour/issues/1579#issuecomment-540856962 which indicates that the alpha sorting by header name is more about maintaining stable ordering than actually indicating a priority between them.

yeah, that makes a lot of sense, since the dag list of routes won't always be added to in the same order

since we store dag.Routes as a map (and then iterate the map to retrieve them as a slice to sort them), doing option 1 or 2 can produce instability in the sorted routes, since the order we iterate over them isn't guaranteed (so preserving the order we get can mean the order of things changes)

I think option 3 directly will alleviate that since we have something we are actively sorting on, rather than trying to maintain the order of the slice that came from the dag.Routes map