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

feature(parser): consolidation of multiple HTTPRouteMatch objects into single kong.Route #3060

Merged
merged 2 commits into from
Oct 31, 2022

Conversation

jrsmroz
Copy link
Contributor

@jrsmroz jrsmroz commented Oct 17, 2022

What this PR does / why we need it:

This PR consolidates HTTPRouteMatch objects into paths in kong.Route if they share the route, filters and matching data.

The behaviour depending on CombinedRoutes being enabled or not:

  • When the CombinedRoutes feature is disabled, the parser generates one kong.Service per HTTPRouteRule and one kong.Route per HTTPRouteMatch pointing to that service.
  • When theCombinedRoutes feature is enabled, prior to this PR, multiple HTTPRouteRule objects can be consolidated into a single kong.Service if they share the backend refs. However, single HTTPRouteMatch generates a single rule pointing to the generated service.
  • With this PR and the CombinedRoutes enabled, the multiple HTTPRouteMatch objects are further consolidated into a single kong.Route with multiple paths.

The following limitations apply to the consolidation logic:

  • HTTPRouteMatch objects cannot be consolidated if they belong to different HTTPRoute objects.
  • HTTPRouteMatch objects cannot be consolidated if HTTPRouteRule objects they belong to have different backend refs.
  • HTTPRouteMatch objects cannot be consolidated if HTTPRouteRule objects they belong to have different filters.
  • HTTPRouteMatch objects cannot be consolidated if they have different spec (headers, query params, method).
  • The different paths does not prevent consolidation of HTTPRouteMatch objects.

This PR changes the code path used for generating kong.Route and kong.Service objects when service based consolidation is enabled, added previously by #3008. It does not change the behaviour of soon to be deprecated kong.Route and kong.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:

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

@jrsmroz jrsmroz temporarily deployed to Configure ci October 17, 2022 13:57 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 17, 2022 14:20 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 17, 2022 14:20 Inactive
@jrsmroz jrsmroz force-pushed the jrsmroz-issue-2881-v3-rebased branch from 13fc2a6 to aaa153d Compare October 17, 2022 14:32
@jrsmroz jrsmroz temporarily deployed to Configure ci October 17, 2022 14:32 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 17, 2022 14:57 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 17, 2022 14:57 Inactive
@jrsmroz jrsmroz force-pushed the jrsmroz-issue-2881-v3-rebased branch from aaa153d to cb35228 Compare October 17, 2022 15:09
@jrsmroz jrsmroz temporarily deployed to Configure ci October 17, 2022 15:09 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 17, 2022 15:37 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 17, 2022 15:37 Inactive
@jrsmroz jrsmroz force-pushed the jrsmroz-issue-2881-v3-rebased branch from cb35228 to fdcb120 Compare October 17, 2022 21:14
@jrsmroz jrsmroz temporarily deployed to Configure ci October 17, 2022 21:14 Inactive
@jrsmroz jrsmroz force-pushed the jrsmroz-issue-2881-v3-rebased branch from fdcb120 to c0cc237 Compare October 17, 2022 21:23
@jrsmroz jrsmroz temporarily deployed to Configure ci October 17, 2022 21:23 Inactive
@jrsmroz jrsmroz force-pushed the jrsmroz-issue-2881-v3-rebased branch from c0cc237 to eb9cf86 Compare October 17, 2022 21:45
@jrsmroz jrsmroz temporarily deployed to Configure ci October 17, 2022 21:45 Inactive
@jrsmroz jrsmroz force-pushed the jrsmroz-issue-2881-v3-rebased branch from eb9cf86 to 481eff8 Compare October 17, 2022 21:49
@jrsmroz jrsmroz temporarily deployed to Configure ci October 17, 2022 21:49 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 17, 2022 22:13 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 17, 2022 22:13 Inactive
@jrsmroz jrsmroz force-pushed the jrsmroz-issue-2881-v3-rebased branch from 481eff8 to 8cbfee8 Compare October 18, 2022 07:42
@jrsmroz jrsmroz temporarily deployed to Configure ci October 18, 2022 07:42 Inactive
@jrsmroz jrsmroz force-pushed the jrsmroz-issue-2881-v3-rebased branch from 8cbfee8 to 70b3ca9 Compare October 18, 2022 07:43
@jrsmroz jrsmroz temporarily deployed to Configure ci October 18, 2022 07:43 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 18, 2022 08:08 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 18, 2022 08:08 Inactive
@jrsmroz jrsmroz force-pushed the jrsmroz-issue-2881-v3-rebased branch from 70b3ca9 to bad4efd Compare October 18, 2022 08:25
@jrsmroz jrsmroz temporarily deployed to Configure ci October 18, 2022 08:25 Inactive
@jrsmroz jrsmroz force-pushed the jrsmroz-issue-2881-v3-rebased branch from bad4efd to 11a28f2 Compare October 18, 2022 08:30
@jrsmroz jrsmroz temporarily deployed to Configure ci October 24, 2022 13:20 Inactive
pmalek
pmalek previously approved these changes Oct 24, 2022
@jrsmroz
Copy link
Contributor Author

jrsmroz commented Oct 24, 2022

@pmalek I propose we also wait for Travis to take a look. There is no rush in merging.

@jrsmroz jrsmroz temporarily deployed to Configure ci October 24, 2022 13:53 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 24, 2022 13:53 Inactive
@jrsmroz jrsmroz force-pushed the jrsmroz-issue-2881-v3-rebased branch from 9ec396d to 148e525 Compare October 27, 2022 20:50
@jrsmroz jrsmroz temporarily deployed to Configure ci October 27, 2022 20:50 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 27, 2022 21:15 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 27, 2022 21:15 Inactive
rainest
rainest previously approved these changes Oct 29, 2022
Copy link
Contributor

@rainest rainest left a 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.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
internal/dataplane/parser/translators/httproute.go Outdated Show resolved Hide resolved
internal/dataplane/parser/translators/httproute.go Outdated Show resolved Hide resolved
internal/dataplane/parser/translators/httproute.go Outdated Show resolved Hide resolved
Co-authored-by: Travis Raines <571832+rainest@users.noreply.github.com>
@jrsmroz jrsmroz force-pushed the jrsmroz-issue-2881-v3-rebased branch from 6c30f5b to 0167ad7 Compare October 31, 2022 17:17
@jrsmroz jrsmroz temporarily deployed to Configure ci October 31, 2022 17:17 Inactive
@jrsmroz
Copy link
Contributor Author

jrsmroz commented Oct 31, 2022

@rainest Applied the changes after the review. PTAL.

@jrsmroz jrsmroz temporarily deployed to Configure ci October 31, 2022 17:44 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 31, 2022 18:53 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 31, 2022 19:16 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 31, 2022 19:16 Inactive
@rainest rainest self-requested a review October 31, 2022 19:36
@jrsmroz jrsmroz temporarily deployed to Configure ci October 31, 2022 19:58 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 31, 2022 19:58 Inactive
@jrsmroz jrsmroz temporarily deployed to Configure ci October 31, 2022 20:04 Inactive
@jrsmroz jrsmroz merged commit 9ef0375 into main Oct 31, 2022
@jrsmroz jrsmroz deleted the jrsmroz-issue-2881-v3-rebased branch October 31, 2022 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/feature New feature or request size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants