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

feat(expression router) Implement translator of ingresses #3935

Merged
merged 7 commits into from
May 4, 2023

Conversation

randmonkey
Copy link
Contributor

What this PR does / why we need it:

  • add feature gate ExpressionRoutes and flags in related structures (KongClient,Parser,KongState,Route) to process expression based routes in a different way
  • Implement translation of ingresses
  • Add integration tests for expression based router

Which issue this PR fixes:

fixes #3750

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

@randmonkey randmonkey requested a review from a team as a code owner April 26, 2023 09:30
@randmonkey randmonkey marked this pull request as draft April 26, 2023 09:30
@codecov
Copy link

codecov bot commented Apr 26, 2023

Codecov Report

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

Comparison is base (ee30642) 58.7% compared to head (440d449) 59.1%.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #3935     +/-   ##
=======================================
+ Coverage   58.7%   59.1%   +0.4%     
=======================================
  Files        141     142      +1     
  Lines      16252   16521    +269     
=======================================
+ Hits        9545    9772    +227     
- Misses      6055    6085     +30     
- Partials     652     664     +12     
Impacted Files Coverage Δ
internal/dataplane/parser/parser.go 73.4% <0.0%> (+0.3%) ⬆️
internal/manager/utils/kongconfig/root.go 50.3% <46.6%> (+0.3%) ⬆️
internal/dataplane/kong_client.go 65.3% <60.0%> (+3.4%) ⬆️
internal/manager/run.go 37.1% <61.5%> (+1.0%) ⬆️
internal/dataplane/kongstate/route.go 89.1% <76.9%> (-2.5%) ⬇️
internal/dataplane/parser/translate_ingress.go 91.4% <77.4%> (-1.8%) ⬇️
internal/dataplane/deckgen/generate.go 31.4% <77.7%> (ø)
...ternal/dataplane/parser/translators/ingress_atc.go 88.3% <88.3%> (ø)
internal/annotations/annotations.go 97.4% <100.0%> (+<0.1%) ⬆️
internal/dataplane/parser/translators/ingress.go 94.1% <100.0%> (+0.1%) ⬆️
... and 1 more

... and 2 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.

@randmonkey randmonkey force-pushed the feat/translate_ingress_expressions_implement branch from 6907cbe to 564820e Compare April 26, 2023 10:44
@randmonkey randmonkey marked this pull request as ready for review April 27, 2023 07:46
Copy link
Contributor

@czeslavo czeslavo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a lot of good work! 👍

My comments mainly gravitate around my thought that we should be careful when adding such a volume of brand new code by making sure that all of the pieces we add are properly unit tested + we have at least one integration test covering it works with Kong as expected in a sunny day scenario.

internal/dataplane/kong_client.go Outdated Show resolved Hide resolved
internal/manager/utils/kongconfig/root.go Outdated Show resolved Hide resolved
internal/manager/utils/kongconfig/root.go Outdated Show resolved Hide resolved
internal/dataplane/parser/translators/ingress_atc.go Outdated Show resolved Hide resolved
internal/manager/run.go Outdated Show resolved Hide resolved
internal/dataplane/kongstate/route.go Outdated Show resolved Hide resolved
internal/dataplane/deckgen/generate.go Outdated Show resolved Hide resolved
@randmonkey randmonkey force-pushed the feat/translate_ingress_expressions_implement branch from 82fb33f to 8f31796 Compare April 28, 2023 08:33
@randmonkey
Copy link
Contributor Author

That's a lot of good work! +1

My comments mainly gravitate around my thought that we should be careful when adding such a volume of brand new code by making sure that all of the pieces we add are properly unit tested + we have at least one integration test covering it works with Kong as expected in a sunny day scenario.

I added several unit tests fro translating ingresses to expression routes. For integration tests, I added workflow integartion-tests/dbless-expression-router to run integration tests that are valid using expression routes. tests using TCPIngress,UDPIngress,TCPRoute,HTTPRoute,GRPCRoutes and knative.Ingress are skipped. Also, the case testing HTTP redirect is skipped because Kong gateway does not support this when using expression router.

internal/dataplane/kong_client.go Outdated Show resolved Hide resolved
internal/dataplane/kongstate/route.go Show resolved Hide resolved
test/integration/grpcroute_test.go Outdated Show resolved Hide resolved
test/integration/helpers_test.go Outdated Show resolved Hide resolved
test/integration/helpers_test.go Show resolved Hide resolved
internal/manager/run.go Outdated Show resolved Hide resolved
@randmonkey randmonkey force-pushed the feat/translate_ingress_expressions_implement branch from 6098cde to 052ff70 Compare May 4, 2023 03:53
@randmonkey randmonkey requested review from czeslavo and pmalek May 4, 2023 06:08
Copy link
Member

@pmalek pmalek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

Copy link
Contributor

@czeslavo czeslavo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@randmonkey randmonkey merged commit ed05179 into main May 4, 2023
23 checks passed
@randmonkey randmonkey deleted the feat/translate_ingress_expressions_implement branch May 4, 2023 08:56
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.

Translate ingress into expression based routes
3 participants