-
Notifications
You must be signed in to change notification settings - Fork 590
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
feature(parser): consolidation of multiple HTTPRouteMatch
objects into single kong.Route
#3060
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jrsmroz
force-pushed
the
jrsmroz-issue-2881-v3-rebased
branch
from
October 17, 2022 14:32
13fc2a6
to
aaa153d
Compare
jrsmroz
force-pushed
the
jrsmroz-issue-2881-v3-rebased
branch
from
October 17, 2022 15:09
aaa153d
to
cb35228
Compare
jrsmroz
force-pushed
the
jrsmroz-issue-2881-v3-rebased
branch
from
October 17, 2022 21:14
cb35228
to
fdcb120
Compare
jrsmroz
force-pushed
the
jrsmroz-issue-2881-v3-rebased
branch
from
October 17, 2022 21:23
fdcb120
to
c0cc237
Compare
jrsmroz
force-pushed
the
jrsmroz-issue-2881-v3-rebased
branch
from
October 17, 2022 21:45
c0cc237
to
eb9cf86
Compare
jrsmroz
force-pushed
the
jrsmroz-issue-2881-v3-rebased
branch
from
October 17, 2022 21:49
eb9cf86
to
481eff8
Compare
jrsmroz
force-pushed
the
jrsmroz-issue-2881-v3-rebased
branch
from
October 18, 2022 07:42
481eff8
to
8cbfee8
Compare
jrsmroz
force-pushed
the
jrsmroz-issue-2881-v3-rebased
branch
from
October 18, 2022 07:43
8cbfee8
to
70b3ca9
Compare
jrsmroz
force-pushed
the
jrsmroz-issue-2881-v3-rebased
branch
from
October 18, 2022 08:25
70b3ca9
to
bad4efd
Compare
jrsmroz
force-pushed
the
jrsmroz-issue-2881-v3-rebased
branch
from
October 18, 2022 08:30
bad4efd
to
11a28f2
Compare
pmalek
previously approved these changes
Oct 24, 2022
@pmalek I propose we also wait for Travis to take a look. There is no rush in merging. |
jrsmroz
force-pushed
the
jrsmroz-issue-2881-v3-rebased
branch
from
October 27, 2022 20:50
9ec396d
to
148e525
Compare
rainest
previously approved these changes
Oct 29, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, my comments are just on documentation stuff.
Co-authored-by: Travis Raines <571832+rainest@users.noreply.github.com>
jrsmroz
force-pushed
the
jrsmroz-issue-2881-v3-rebased
branch
from
October 31, 2022 17:17
6c30f5b
to
0167ad7
Compare
@rainest Applied the changes after the review. PTAL. |
rainest
approved these changes
Oct 31, 2022
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
This PR consolidates
HTTPRouteMatch
objects into paths inkong.Route
if they share the route, filters and matching data.The behaviour depending on
CombinedRoutes
being enabled or not:CombinedRoutes
feature is disabled, the parser generates onekong.Service
perHTTPRouteRule
and onekong.Route
perHTTPRouteMatch
pointing to that service.CombinedRoutes
feature is enabled, prior to this PR, multipleHTTPRouteRule
objects can be consolidated into a singlekong.Service
if they share the backend refs. However, singleHTTPRouteMatch
generates a single rule pointing to the generated service.CombinedRoutes
enabled, the multipleHTTPRouteMatch
objects are further consolidated into a singlekong.Route
with multiple paths.The following limitations apply to the consolidation logic:
HTTPRouteMatch
objects cannot be consolidated if they belong to differentHTTPRoute
objects.HTTPRouteMatch
objects cannot be consolidated ifHTTPRouteRule
objects they belong to have different backend refs.HTTPRouteMatch
objects cannot be consolidated ifHTTPRouteRule
objects they belong to have different filters.HTTPRouteMatch
objects cannot be consolidated if they have differentspec
(headers, query params, method).HTTPRouteMatch
objects.This PR changes the code path used for generating
kong.Route
andkong.Service
objects when service based consolidation is enabled, added previously by #3008. It does not change the behaviour of soon to be deprecatedkong.Route
andkong.Service
generation used when service based consolidation is disabled. The PR extracts common functions from this code path.Which issue this PR fixes:
Part of #2881
Special notes for your reviewer:
This feature does not yet provide a benchmark, to measure performance hit, but it's intended to provide benchamarks too.
PR Readiness Checklist:
Complete these before marking the PR as
ready to review
:CHANGELOG.md
release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR