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

feat: support session persistence in HTTPRouteRule #3841

Merged
merged 20 commits into from
Aug 10, 2024
Merged

feat: support session persistence in HTTPRouteRule #3841

merged 20 commits into from
Aug 10, 2024

Conversation

sanposhiho
Copy link
Contributor

What type of PR is this?

What this PR does / why we need it:

support session persistence in HTTPRouteRule.

Which issue(s) this PR fixes:

Fixes #3315
Ref #2624 (comment)
GEP: https://gateway-api.sigs.k8s.io/geps/gep-1619/

@sanposhiho sanposhiho requested a review from a team as a code owner July 14, 2024 12:54
@codecov Codecov
Copy link

codecov bot commented Jul 14, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 58.40708% with 47 lines in your changes missing coverage. Please review.

Project coverage is 67.58%. Comparing base (975b1e8) to head (c5947c5).

Files Patch % Lines
internal/gatewayapi/route.go 12.50% 27 Missing and 1 partial ⚠️
internal/xds/translator/session_persistence.go 75.64% 10 Missing and 9 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3841      +/-   ##
==========================================
- Coverage   67.61%   67.58%   -0.03%     
==========================================
  Files         185      186       +1     
  Lines       22578    22689     +111     
==========================================
+ Hits        15265    15335      +70     
- Misses       6217     6251      +34     
- Partials     1096     1103       +7

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

@sanposhiho sanposhiho force-pushed the session-persistence branch 5 times, most recently from d33c8f3 to 53cb615 Compare July 16, 2024 03:40
shawnh2
shawnh2 reviewed Jul 16, 2024
View reviewed changes
internal/ir/xds.go Show resolved Hide resolved
internal/xds/translator/session_persistence_test.go Outdated Show resolved Hide resolved
@sanposhiho sanposhiho requested a review from shawnh2 July 18, 2024 10:41
@sanposhiho sanposhiho force-pushed the session-persistence branch from 04752ea to 0f45c69 Compare July 18, 2024 10:44
zhaohuabing
zhaohuabing reviewed Jul 22, 2024
View reviewed changes
internal/ir/xds.go Outdated Show resolved Hide resolved
@zhaohuabing
Copy link
Member

Can we add an e2e test for this?

@sanposhiho sanposhiho force-pushed the session-persistence branch 2 times, most recently from d3527e8 to 9e8b85c Compare July 26, 2024 15:38
@sanposhiho
Copy link
Contributor Author

Done. Addressed the points from @zhaohuabing and added the e2e test.

@sanposhiho sanposhiho force-pushed the session-persistence branch 6 times, most recently from 8936443 to 0eed511 Compare July 27, 2024 04:29
@sanposhiho
Copy link
Contributor Author

/retest

@sanposhiho sanposhiho requested a review from zhaohuabing July 29, 2024 15:58
zhaohuabing
zhaohuabing previously approved these changes Jul 30, 2024
View reviewed changes
Copy link
Member

@zhaohuabing zhaohuabing left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! Thanks for picking this up!

sanposhiho added 12 commits August 7, 2024 17:02
@sanposhiho
chore: move SessionName field within ir
fb0ce5b 
Signed-off-by: Kensei Nakada <handbomusic@gmail.com>
@sanposhiho
fix: move stateful session filter to per-route
163aad1 
Signed-off-by: Kensei Nakada <handbomusic@gmail.com>
@sanposhiho
fix: remove incorrect sessionPersistence declare
55956be 
Signed-off-by: Kensei Nakada <handbomusic@gmail.com>
@sanposhiho
fix: configure stateful_session config correctly
6f18d3e 
Signed-off-by: Kensei Nakada <handbomusic@gmail.com>
@sanposhiho
test: add an e2e test for session persistence
5357db7 
Signed-off-by: Kensei Nakada <handbomusic@gmail.com>
@sanposhiho
chore: move the filter name to api level
ca4ea55 
Signed-off-by: Kensei Nakada <handbomusic@gmail.com>
@sanposhiho
address lint issue
8613b1d 
Signed-off-by: Kensei Nakada <handbomusic@gmail.com>
@sanposhiho
fix: skip adding filter if already existing
9725ec3 
Signed-off-by: Kensei Nakada <handbomusic@gmail.com>
@sanposhiho
fix: generate the session name when it's empty
d2f2e84 
Signed-off-by: Kensei Nakada <handbomusic@gmail.com>
@sanposhiho
chore: ignore TTL when it's session cookie
f7d5b01 
Signed-off-by: Kensei Nakada <handbomusic@gmail.com>
@sanposhiho
chore: update the test case per the upstream update
075819d 
Signed-off-by: Kensei Nakada <handbomusic@gmail.com>
@sanposhiho
fix: update the test case
128817b 
Signed-off-by: Kensei Nakada <handbomusic@gmail.com>
@sanposhiho sanposhiho force-pushed the session-persistence branch from 45c2a89 to baf0973 Compare August 7, 2024 08:18
@sanposhiho
fix: unique session name per route
c91cfb6 
Signed-off-by: Kensei Nakada <handbomusic@gmail.com>
@sanposhiho sanposhiho force-pushed the session-persistence branch from baf0973 to c91cfb6 Compare August 7, 2024 08:19
@sanposhiho
Copy link
Contributor Author

@arkodg Addressed your point + rebased for a conflict.

@zirain
Copy link
Contributor

zirain commented Aug 7, 2024

/retest

@sanposhiho
chore: update wordings in test cases
bb5160a 
Signed-off-by: Kensei Nakada <handbomusic@gmail.com>
@sanposhiho
Copy link
Contributor Author

@zirain Updated based on your points.

arkodg
arkodg approved these changes Aug 7, 2024
View reviewed changes
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

@arkodg arkodg requested review from a team August 7, 2024 20:19
@arkodg arkodg requested a review from zirain August 8, 2024 23:16
zirain
zirain approved these changes Aug 10, 2024
View reviewed changes
Copy link
Contributor

@zirain zirain left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@zirain zirain merged commit bd26a41 into envoyproxy:main Aug 10, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arkodg arkodg arkodg approved these changes

@zirain zirain zirain approved these changes

@shawnh2 shawnh2 Awaiting requested review from shawnh2

@Xunzhuo Xunzhuo Awaiting requested review from Xunzhuo

@zhaohuabing zhaohuabing Awaiting requested review from zhaohuabing

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Session Persistence
6 participants
@sanposhiho @zhaohuabing @arkodg @zirain @shawnh2 @Xunzhuo