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.

Sign up for GitHub

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

Jump to bottom

fix(router): always calculate match_t.upstream_uri to avoid wrong cache #12307

Merged
merged 8 commits into from
Jan 12, 2024
Merged

fix(router): always calculate match_t.upstream_uri to avoid wrong cache #12307

merged 8 commits into from
Jan 12, 2024

Conversation

chronolaw
Copy link
Contributor

@chronolaw chronolaw commented Jan 9, 2024 •
edited
Loading

Summary

KAG-3505

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

Fix #[issue number]

@chronolaw chronolaw marked this pull request as ready for review January 9, 2024 04:15
windmgc
windmgc approved these changes Jan 9, 2024
View reviewed changes
Copy link
Member

@windmgc windmgc left a comment

Choose a reason for hiding this comment

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

LGTM

@windmgc
Copy link
Member

windmgc commented Jan 9, 2024

I'm also a bit concerned that after the change, those string manipulations are executed on every request, which may cause additional resource usage and we may need a proper performance test to check the exact influence level.
But it may not be a great one since I don't see any expensive operation in it

@chronolaw
Copy link
Contributor Author

I'm also a bit concerned that after the change, those string manipulations are executed on every request, which may cause additional resource usage and we may need a proper performance test to check the exact influence level. But it may not be a great one since I don't see any expensive operation in it

Sure, because now we do not cache upstream_uri, we have to calculate it in each request.
If we add req_uri in cache_key then we will erase this cost.

@chronolaw chronolaw requested a review from dndx January 9, 2024 07:31
@chronolaw chronolaw added this to the 3.6.0 milestone Jan 9, 2024
dndx
dndx requested changes Jan 11, 2024
View reviewed changes
Copy link
Member

@dndx dndx left a comment

Choose a reason for hiding this comment

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

Since we fixed the upstream path generation, should 86f0a6c#diff-10bd9d1fe08f4a9367308540176f3e7c6067a831dc22cc182057319d28f5f061R884 be removed? It seems that we should not have added the paths to work around the issue in the first place.

@dndx dndx force-pushed the refactor/matching_get_upstream_uri branch from 480865d to 70c2fa9 Compare January 12, 2024 07:43
@dndx
Copy link
Member

dndx commented Jan 12, 2024

@chronolaw Approved, notice that this should be a fix, not refactor.

@chronolaw chronolaw changed the title refactor(router): always calculate match_t.upstream_uri to avoid wrong cache fix(router): always calculate match_t.upstream_uri to avoid wrong cache Jan 12, 2024
@dndx dndx merged commit ce12f68 into master Jan 12, 2024
25 checks passed
@dndx dndx deleted the refactor/matching_get_upstream_uri branch January 12, 2024 08:25
@chronolaw chronolaw added the cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee label Jan 12, 2024
@team-gateway-bot
Copy link
Collaborator

Successfully created cherry-pick PR for master:

chronolaw added a commit that referenced this pull request Jan 23, 2024
@chronolaw
fix(router/atc): always re-calculate match_t.upstream_uri even in c…
80e292b 
…ase of cache hits (#12307)

`upstream_uri` is dependent on request path, and should not be automatically cached inside the route cache due to potential for path matching not being configured inside routes.

KAG-3505
@chronolaw chronolaw mentioned this pull request Jan 23, 2024
Merged
3 tasks
@chronolaw chronolaw mentioned this pull request Jan 23, 2024
Merged
3 tasks
chronolaw added a commit that referenced this pull request Jan 24, 2024
@chronolaw
fix(router/atc): always re-calculate match_t.upstream_uri even in c…
18e6747 
…ase of cache hits (#12307)

`upstream_uri` is dependent on request path, and should not be automatically cached inside the route cache due to potential for path matching not being configured inside routes.

KAG-3505
chronolaw added a commit that referenced this pull request Feb 26, 2024
@chronolaw
fix(router/atc): always re-calculate match_t.upstream_uri even in c…
4aad7d2 
…ase of cache hits (#12307)

`upstream_uri` is dependent on request path, and should not be automatically cached inside the route cache due to potential for path matching not being configured inside routes.

KAG-3505
chronolaw added a commit that referenced this pull request Feb 26, 2024
@chronolaw
fix(router/atc): always re-calculate match_t.upstream_uri even in c…
29243d5 
…ase of cache hits (#12307)

`upstream_uri` is dependent on request path, and should not be automatically cached inside the route cache due to potential for path matching not being configured inside routes.

KAG-3505
chronolaw added a commit that referenced this pull request Mar 4, 2024
@chronolaw
fix(router/atc): always re-calculate match_t.upstream_uri even in c…
a035f50 
…ase of cache hits (#12307)

`upstream_uri` is dependent on request path, and should not be automatically cached inside the route cache due to potential for path matching not being configured inside routes.

KAG-3505
locao pushed a commit that referenced this pull request Mar 5, 2024
@chronolaw @locao
fix(router/atc): always re-calculate match_t.upstream_uri even in c…
8da335a 
…ase of cache hits (#12307)

`upstream_uri` is dependent on request path, and should not be automatically cached inside the route cache due to potential for path matching not being configured inside routes.

KAG-3505
ADD-SP pushed a commit that referenced this pull request Mar 7, 2024
@chronolaw @ADD-SP
fix(router/atc): always re-calculate match_t.upstream_uri even in c…
86078fe 
…ase of cache hits (#12307)

`upstream_uri` is dependent on request path, and should not be automatically cached inside the route cache due to potential for path matching not being configured inside routes.

KAG-3505
ADD-SP pushed a commit that referenced this pull request Mar 7, 2024
@chronolaw @ADD-SP
fix(router/atc): always re-calculate match_t.upstream_uri even in c…
344263e 
…ase of cache hits (#12307)

`upstream_uri` is dependent on request path, and should not be automatically cached inside the route cache due to potential for path matching not being configured inside routes.

KAG-3505
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ADD-SP ADD-SP ADD-SP approved these changes

@windmgc windmgc windmgc approved these changes

@dndx dndx dndx approved these changes

Assignees

@chronolaw chronolaw

Labels
cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee core/router size/M skip-changelog
Projects
None yet
Milestone
3.6.0
Development

Successfully merging this pull request may close these issues.
5 participants
@chronolaw @windmgc @dndx @team-gateway-bot @ADD-SP