Stops route sorting on HTTPProxy CRD

[davinci26]

Stops route sorting on HTTPProxy CRD

Goal:

Contour generates the Envoy routes for each virtual host in the same
order as they are in the HTTPProxy. This benefits users from having no
suprises between the Envoy actual routing table and what they described in the
CRD.

---

Core changes:

• Change the underlying data structure in dag.go and VirtualHost to be a slice such that we can maintain the order.
• Change the sorting to be conditional on the caller wanting it. This is used to implement sorting for virtual hosts which are "partially" controlled by an Ingress/ApiGateway so we can have conformance.
• Change the implementation of expandPrefixMatches to maintain the order of the original list.

Work Pending

• Add better testing to ensure routes are sorted on the default case

Signed-off-by: Sotiris Nanopoulos sotiris.nanopoulos@reddit.com

[codecov]

Codecov Report

Merging #5772 (6d4a432) into main (6917d85) will decrease coverage by 0.03%.
Report is 6 commits behind head on main.
The diff coverage is 95.89%.

❗ Current head 6d4a432 differs from pull request most recent head 664068e. Consider uploading reports for the commit 664068e to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5772      +/-   ##
==========================================
- Coverage   78.56%   78.54%   -0.03%     
==========================================
  Files         139      139              
  Lines       19614    19619       +5     
==========================================
  Hits        15410    15410              
- Misses       3901     3906       +5     
  Partials      303      303              

Files	Coverage Δ
internal/dag/dag.go	95.09% <100.00%> (-3.61%)	⬇️
internal/dag/gatewayapi_processor.go	93.82% <100.00%> (+0.02%)	⬆️
internal/dag/httpproxy_processor.go	91.88% <100.00%> (-0.03%)	⬇️
internal/dag/ingress_processor.go	97.60% <100.00%> (+0.01%)	⬆️
internal/xdscache/v3/route.go	97.34% <100.00%> (-0.12%)	⬇️
pkg/config/parameters.go	86.06% <ø> (ø)
cmd/contour/serve.go	19.59% <50.00%> (+0.06%)	⬆️
internal/dag/accessors.go	88.23% <0.00%> (+1.56%)	⬆️

... and 1 file with indirect coverage changes

[github-actions]

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:

• 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

[davinci26]

Not stale

[sunjayBhatia]

I would like to try to keep this logic out of the various processors as much as possible and try to only make changes at the dag or xdscache level

e.g. we could have this sort of config on the dag.Builder that would get passed down to dag.DAG.EnsureVirtualHost and eventually dag.VirtualHost.AddRoute to abstract this away from the processors (which tbh should maybe be in their own package at this point)

theoretically the processors should already be adding routes in list-order, and I thought we already did process includes in depth-first order, based on this recursion:

contour/internal/dag/httpproxy_processor.go

Line 726 in 6917d85

routes = append(routes, p.computeRoutes(incValidCond, rootProxy, includedProxy, append(conditions, include.Conditions...), visited, enforceTLS, defaultJWTProvider)...)

[sunjayBhatia]

based on the different dag builder/DAG options, we could maybe keep both a list and map structures for routes on a vhost and only use one of the two based on the passed in config

just off the top of my head thoughts, not fully formed yet

[davinci26]

I am open to the idea, still need to marinate the ramifications on test coverage for this. That being said I am not sure
how this would work because:

EnsureVirtualHost which effectively constructs the vhost is not aware of the caller. And we have 3 cases

• The caller is an HTTPProxy with controller in omitRoute mode: in which case it should set it to false
• The caller is an Ingress with controller in omitRoute mode: in which case it should set it to true
• The caller is an Ingress and an HTTPProxy with controller in omitRoute mode: in which case it should set it to false

So in that case it seems to me that it is the responsibility of the processor to communicate its preference. Basically the processor is aware of the conformance requirements and declares if it needs them for the vhost that it manages

[davinci26]

theoretically the processors should already be adding routes in list-order, and I thought we already did process includes in depth-first order, based on this recursion:

😅 actually they werent because expandPrefixMatches was using a match internally and that re-ordered all routes

[github-actions]

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