Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support TCPIngress and UDPIngress expression routes #4612

Merged
merged 2 commits into from
Sep 11, 2023

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Sep 5, 2023

What this PR does / why we need it:

Adds expression routing for TCPIngress and UDPIngress.

Adds a unit test for the L4 kongstate.Route translator.

Which issue this PR fixes:

Fix #4541
Fix #4542

Special notes for your reviewer:

#4385 is still setting the traditional protocols field. Is this correct? https://docs.konghq.com/gateway/latest/reference/router-expressions-language/#main indicates there's a net.protocol expression field.

#4385 added a test for TCPRoute specifically, but I don't think we need a per-resource unit test for these. We already test kongstate.Route creation for resources, and the expression translator operates on kongstate.Routes, so I think we can just test it on those directly.

Kong L4 routes support several features that our CRs and GWAPI Routes do not:

  • Routes support both sources and destinations, whereas the Kubernetes resources only support destinations.
  • Sources and destinations support IPs, whereas the Kubernetes resources only support ports.

This is the intended feature set of GWAPI L4 Routes. We never added support for the complete set of Kong route capabilities to TCPIngress and UDPIngress, and there is apparently limited demand for the missing functionality (I don't know of any outstanding requests for it). The TCPIngress and UDPIngress feature set is coincidentally the same as what GWAPI Routes can support. As this additional functionality isn't supported by the resources, I didn't add support in the translator, just a comment noting the gap.

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

@rainest rainest requested a review from a team as a code owner September 5, 2023 22:42
@rainest rainest marked this pull request as draft September 5, 2023 22:42
@codecov
Copy link

codecov bot commented Sep 5, 2023

Codecov Report

Patch coverage: 88.2% and project coverage change: -0.3% ⚠️

Comparison is base (709f0a1) 68.2% compared to head (375f3b9) 68.0%.
Report is 21 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #4612     +/-   ##
=======================================
- Coverage   68.2%   68.0%   -0.3%     
=======================================
  Files        162     163      +1     
  Lines      19008   19091     +83     
=======================================
+ Hits       12973   12983     +10     
- Misses      5274    5339     +65     
- Partials     761     769      +8     
Files Changed Coverage Δ
internal/dataplane/parser/translate_kong_l4.go 52.2% <75.0%> (-3.3%) ⬇️
...ternal/dataplane/parser/translators/l4route_atc.go 100.0% <100.0%> (+6.6%) ⬆️

... and 16 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Add support for the expression router to TCPIngress and UDPIngress.
@rainest rainest marked this pull request as ready for review September 6, 2023 17:19
@rainest
Copy link
Contributor Author

rainest commented Sep 6, 2023

My question about the use of the protocols field instead of the expression predicate has been answered and integration is passing now that it allows the L4 CRD tests in the expression job, so I think this is good to go.

@rainest
Copy link
Contributor Author

rainest commented Sep 8, 2023

tls_passthrough is broken, which is a known issue (apparently on the gateway side?). I expect the fix is the same for both types of passthrough CRD and won't otherwise change TCPIngress-specific code, so keeping that test disabled with a follow-up tacked onto #4540

@rainest rainest requested a review from randmonkey September 8, 2023 21:37
@randmonkey randmonkey merged commit 0e04e18 into main Sep 11, 2023
33 checks passed
@randmonkey randmonkey deleted the feat/l4-expression branch September 11, 2023 03:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support UDPIngress with expression router Support TCPIngress with expression router
2 participants