-
Notifications
You must be signed in to change notification settings - Fork 604
feat: add basic sticky-sessions support #7466
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7466 +/- ##
=======================================
- Coverage 77.3% 77.3% -0.1%
=======================================
Files 220 220
Lines 25372 25407 +35
=======================================
+ Hits 19623 19643 +20
- Misses 4747 4767 +20
+ Partials 1002 997 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
76311b9
to
0be0414
Compare
examples/sticky-sessions.yaml
Outdated
algorithm: consistent-hashing | ||
slots: 100 | ||
|
||
# Configure session-based sticky routing using cookies | ||
hashOn: | ||
cookie: session-id # Name of the session cookie | ||
cookiePath: / # Path for the session cookie |
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.
This section is currently a placeholder, I need to make modifications based on the specific implementation of the Gateway.
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.
Does this still require more work or can we move forward with currently set value here?
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.
@pmalek In the current gateway implementation, the algorithm name is set to "sticky-sessions"
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.
Ok, can you be more specific what does that mean for us? What work is still required? What changes still need (if any) to be done?
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.
This means we need to change the manifest according to the implementation in the gateway. I think the other parts do not need to be adjusted.
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.
according to the implementation in the gateway
That means specifically what exactly? We need to change the hashOn
settings? We need to change the session-script
ConfigMap
? I think that this needs to be written down somewhere.
I've reopened #7418 to cover this work (as I understand there is still something to be done for this feature). LMYK if that works for you.
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.
I guess I can submit a PR when I'm available (in a few hours)
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.
Was this update made ? OR is it simply I need to update the alorithm in the manofest to be sticky-sessions ?
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.
The updated manifest can be found in #7495
1eda300
to
1de5abf
Compare
internal/dataplane/translator/translate_upstreams_sticky_sessions_test.go
Outdated
Show resolved
Hide resolved
internal/dataplane/translator/translate_upstreams_sticky_sessions_test.go
Outdated
Show resolved
Hide resolved
bab8ad7
to
f54f96a
Compare
Signed-off-by: Jintao Zhang <[email protected]>
Signed-off-by: Jintao Zhang <[email protected]>
Signed-off-by: Jintao Zhang <[email protected]>
Signed-off-by: Jintao Zhang <[email protected]>
Signed-off-by: Jintao Zhang <[email protected]>
Signed-off-by: Jintao Zhang <[email protected]>
Signed-off-by: Jintao Zhang <[email protected]>
Signed-off-by: Jintao Zhang <[email protected]>
Signed-off-by: Jintao Zhang <[email protected]>
f54f96a
to
5e1b0c3
Compare
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
internal/dataplane/translator/translate_upstreams_sticky_sessions_test.go
Outdated
Show resolved
Hide resolved
internal/dataplane/translator/translate_upstreams_sticky_sessions_test.go
Outdated
Show resolved
Hide resolved
internal/dataplane/translator/translate_upstreams_sticky_sessions_test.go
Outdated
Show resolved
Hide resolved
internal/dataplane/translator/translate_upstreams_sticky_sessions_test.go
Outdated
Show resolved
Hide resolved
…ons_test.go -> internal/dataplane/translator/translate_upstreams_drain_support_test.go
fae08a9
to
502f01a
Compare
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.
I'm sorry it's just now that I realized this but I believe we should remove this test:
-
This test shouldn't really be placed in this directory.
This is not an integration test, it doesn't use the cluster in any way of nor it interacts with it using any of the clients that we typically use in integration tests.
-
Secondly and more importantly, this doesn't really test anything. Note that the only interaction that this test has with KIC's packages is
util.Endpoint
which it creates in the test body itself, not through any of the functions or types added in this PR.
Having said that I believe we should remove this test and ideally add a proper one that verifies the added functionality.
What this PR does / why we need it:
Which issue this PR fixes:
part of #7418
Special notes for your reviewer:
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