implement assigning priorities to Kong routes translated from HTTPRoute

[randmonkey]

What this PR does / why we need it:

Assign priorities to Kong routes translated from HTTPRoutes.

Which issue this PR fixes:

fixes #4249

Special notes for your reviewer:

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

• the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

[pmalek]

Some minor comments. I might not be an expert here but I believe splitted shouldn't be used and instead we should use split as past participle of split.

[rainest]

Are we able to pre-process HTTPRoutes for validity and register errors regardless of whether we're using expression routes or not?

It's not quite obvious from the diff but it looks like this will introduce a separate validation path for expression and traditional mode, even though the issues aren't exclusive to one mode (e.g. ErrRouteValidationNoMatchRulesOrHostnamesSpecified and the results of validateHTTPRoute() depend only on the contents of the original resource, not which type of Kong route we're translating to).

If we have those checks separately in the mode translators, we're likely to miss checks in one or the other (we ran into this with CombinedRoutes) and need more test coverage, so if we can validate before translating, we should.

[randmonkey]

Are we able to pre-process HTTPRoutes for validity and register errors regardless of whether we're using expression routes or not?

It's not quite obvious from the diff but it looks like this will introduce a separate validation path for expression and traditional mode, even though the issues aren't exclusive to one mode (e.g. ErrRouteValidationNoMatchRulesOrHostnamesSpecified and the results of validateHTTPRoute() depend only on the contents of the original resource, not which type of Kong route we're translating to).

If we have those checks separately in the mode translators, we're likely to miss checks in one or the other (we ran into this with CombinedRoutes) and need more test coverage, so if we can validate before translating, we should.

Sounds good, but if we extract the validation code, it will affect the original translation code in multiple places. This would make the PR to be even larger and harder for a detailed review. I suggest to finish it in another PR. @rainest would you please create another issue to extract the validation and translation failure code (and add it to 2.11 milestone)?

@rainest's edit: logged it in #4321

[codecov]

Codecov Report

Patch coverage: 88.5% and project coverage change: +0.4 🎉

Comparison is base (c8843de) 64.9% compared to head (7709eaa) 65.4%.

❗ Current head 7709eaa differs from pull request most recent head 9ac8af7. Consider uploading reports for the commit 9ac8af7 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #4296     +/-   ##
=======================================
+ Coverage   64.9%   65.4%   +0.4%     
=======================================
  Files        154     154             
  Lines      17384   17722    +338     
=======================================
+ Hits       11297   11603    +306     
- Misses      5367    5388     +21     
- Partials     720     731     +11     

Impacted Files	Coverage Δ
internal/dataplane/parser/translate_httproute.go	78.3% <68.5%> (-2.2%)	⬇️
...rnal/dataplane/parser/translators/httproute_atc.go	96.0% <94.4%> (-2.2%)	⬇️

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[mlavacca]

It looks good @randmonkey! Can you please re-enable the header matching conformance test to verify we are actually fixing that issue?

kubernetes-ingress-controller/test/conformance/gateway_conformance_test.go

Lines 85 to 86 in 2b13580

	tests.HTTPRouteHeaderMatching.ShortName,

[randmonkey]

It looks good @randmonkey! Can you please re-enable the header matching conformance test to verify we are actually fixing that issue?

kubernetes-ingress-controller/test/conformance/gateway_conformance_test.go

Lines 85 to 86 in 2b13580

	tests.HTTPRouteHeaderMatching.ShortName,

I think this conformance should be re-enabled when KIC can run with L4 proxy with expression router: #4312 because the TCPRoute, TLSRoute, UDPRoute cases will fail when L4 proxy cannot be supported.
Or we could run 2 sets of conformance tests for different router flavors. WDYT @mlavacca ?

[mlavacca]

I think this conformance should be re-enabled when KIC can run with L4 proxy with expression router: #4312 because the TCPRoute, TLSRoute, UDPRoute cases will fail when L4 proxy cannot be supported.
Or we could run 2 sets of conformance tests for different router flavors. WDYT @mlavacca ?

There are no conformance tests for L4 routes so far. There are L7-only conformance tests upstream, so I don't think it is a problem 🤔

[randmonkey]

I think this conformance should be re-enabled when KIC can run with L4 proxy with expression router: #4312 because the TCPRoute, TLSRoute, UDPRoute cases will fail when L4 proxy cannot be supported.
Or we could run 2 sets of conformance tests for different router flavors. WDYT @mlavacca ?

There are no conformance tests for L4 routes so far. There are L7-only conformance tests upstream, so I don't think it is a problem thinking

There is ONE conformance test: tests.TLSRouteSimpleSameNamespace.ShortName depending on TLSRoute. But it is OK to re-enable the header match case, because we skip the TLSRoute case now.

[czeslavo]

🚀

[mlavacca]

It looks like the conformance test is broken. No idea why it was passing 🤔