extension: Add stateful session extension #18207

wbpcode · 2021-09-22T03:26:04Z

Commit Message: "extension: new extensions for stateful session sticky"
Additional Description:

In the #17290, we added new cross priority host map for fast host searching.
In the #17848, we extend LoadBalancerContext interface to provide override host and select upstream host by the override host.

Finally, in this PR, we expanded a new API to allow users to extract the state of the session from the request, and change the result of load balancing by setting the override host value.

Related doc: https://docs.google.com/document/d/1IU4b76AgOXijNa4sew1gfBfSiOMbZNiEt5Dhis8QpYg/edit?usp=sharing

Risk Level: Mid.
Testing: Added.
Docs Changes: Waiting.
Release Notes: Waiting.
Platform Specific Features: N/A.

Signed-off-by: wbpcode <wbphub@live.com>

repokitteh-read-only · 2021-09-22T03:26:10Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #18207 was opened by wbpcode.

see: more, trace.

wbpcode · 2021-09-22T03:26:55Z

cc @mattklein123 @snowp

wbpcode · 2021-09-22T03:31:03Z

Some problems to be solved:

🤔 Should this new API route level or filter level ? In the current implementation, the new API is at the filter level, which is part of the router filter API. But should it be part of the route?

🤔 I added a new extension to support different types of session state, and provide cookie based session state as a basic implementation, which requires the support of maintainer.

zuercher

Thanks for taking this on.

I wonder if we'd be better off putting most of the stateful session implementation in an http filter? Certainly the cookie lookup and generation could be done there. We'd need a way for the session filter to pass an OverrideHost (or equivalent) to the LoadBalancerContext implementation in the router filter. We already have FilterState and dynamic metadata available in StreamInfo or we could introduce a method in Stream[Encoder]FilterCallbacks.

The advantage to using a separate filter would be to reuse the existing implementation of the route's typed_per_filter_config to allow stateful sessions to be enabled/disabled per route (and perhaps even to allow more advanced config difference across routes, like altering the allowed health or even the cookie name).

@snowp do you have any opinions on this?

(I added some grammar-related review comments below, but those are absolutely lower priority than this discussion. I just didn't want to lose track of them.)

api/envoy/extensions/http/stateful_session/cookie/v3/cookie.proto

source/extensions/extensions_build_config.bzl

wbpcode · 2021-09-23T02:46:26Z

The advantage to using a separate filter would be to reuse the existing implementation of the route's typed_per_filter_config to allow stateful sessions to be enabled/disabled per route (and perhaps even to allow more advanced config difference across routes, like altering the allowed health or even the cookie name).

Sounds great.

wbpcode · 2021-09-23T16:01:48Z

I agree with @zuercher 's opinion. But still waiting for @snowp 's feedback before revising this PR.

Also, it looks like the commit from the code review won't pass the DCO, so I might need a force push afterwards.

snowp · 2021-09-28T18:50:19Z

+1 from me if we can easily implement this as a filter + FS, I think that would make it easier to use this feature in various creative ways vs it being specifically for stateful load balancing

wbpcode · 2021-09-29T01:41:40Z

+1 from me if we can easily implement this as a filter + FS, I think that would make it easier to use this feature in various creative ways vs it being specifically for stateful load balancing

@snowp

Yes, based on this function, we can do more hacks on load balancing. If we hope that override host can have more various creative ways, will the filter + new StreamDecoderFilterCallbacks interface be better? In this way, our Router does not need to perceive the FS key and other developers can develop other filters according to their needs.

update comments Co-authored-by: Stephan Zuercher <zuercher@gmail.com> Signed-off-by: wbpcode <wbphub@live.com>

Signed-off-by: wbpcode <wbphub@live.com>

…on-sticky

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode · 2021-10-04T15:27:38Z

@zuercher Hi, I completed the refactoring of this PR. Now, I use a http filter to implement stateful session which gives us two new extensions.
Although they all are simple extensions, according to the requirements of the new extension, we should still need at least one maintainer to support it. Do you have extra free time to help me improve these extensions?

zuercher

Overall I think this looks pretty good. I took a first pass of comments below.

Unortunately, I don't have the bandwidth to take on maintainer-ship of this feature.

api/envoy/extensions/filters/http/stateful_session/v3/stateful_session.proto

envoy/http/stateful_session.h

source/extensions/filters/http/stateful_session/stateful_session.cc

source/extensions/http/stateful_session/cookie/cookie.h

source/common/upstream/load_balancer_impl.cc

api/envoy/extensions/http/stateful_session/cookie/v3/cookie.proto

wbpcode · 2021-10-06T01:33:44Z

@zuercher Thanks for your detailed code review 🌹 . I will find a maintainer to help me sponsor this feature.

htuch · 2021-10-20T03:20:09Z

@wbpcode any progress here on finding an appropriate maintainer sponsor? Wondering if we should close this out or move forward with review.

wbpcode · 2021-10-20T08:24:14Z

@wbpcode any progress here on finding an appropriate maintainer sponsor? Wondering if we should close this out or move forward with review.

There is currently no clear progress. May be we can close this temporarily. If there is result later, I will reopen this PR. I still want to land this feature eventually.

dio · 2021-10-20T22:21:01Z

@htuch I would like to help (sponsoring) with this if that is not a problem. @wbpcode, let's do this!

wbpcode · 2021-10-21T01:45:17Z

@dio Thanks, dio, thanks 🌷. I will update this PR this Friday.

Signed-off-by: wbpcode <wbphub@live.com>

htuch · 2021-12-30T05:01:59Z

@zuercher since you've already been looking at this, assigning your way for senior maintainer approval/merge. Thanks.

docs/root/configuration/http/http_filters/stateful_session_filter.rst

source/common/upstream/load_balancer_impl.cc

CODEOWNERS

source/extensions/filters/http/stateful_session/stateful_session.h

source/extensions/http/stateful_session/cookie/cookie.cc

zuercher · 2022-01-03T18:26:45Z

docs/root/configuration/http/http_filters/stateful_session_filter.rst

@@ -0,0 +1,80 @@
+.. _config_http_filters_stateful_session:


I wonder if we should include a note in this documentation that this type of sticky session is an anti-pattern and that operators should avoid using this feature when possible?

I think this function is not that special, and there are similar functions among other proxies. But I can add a note to let users use this feature with caution.

Similar function in other proxy: https://docs.nginx.com/nginx/admin-guide/load-balancer/http-load-balancer/#enabling-session-persistence

Let's add it. Something like:

.. note:: Stateful sessions can result in imbalanced load across upstreams and allow external actors to direct requests to specific upstream hosts. Operators should carefully consider the security and reliability implications of stateful sessions before enabling this feature.

wbpcode · 2022-01-04T06:05:42Z

Thanks for your detailed review. @zuercher

Signed-off-by: wbpcode <wbphub@live.com>

zuercher

Thanks. I think the code is good -- just some details to finish off.

zuercher · 2022-01-04T18:16:30Z

CODEOWNERS

@@ -195,6 +197,8 @@ extensions/filters/http/oauth2 @rgs1 @derekargueta @snowp
 /*/extensions/matching/input_matchers/ip @aguinet @snowp
 # Key Value store
 /*/extensions/key_value @alyssawilk @ryantheoptimist
+# Stateful session
+/*/extensions/http/stateful_session/cookie @wbpcode @dio @zuercher


Sorry, this one as well.

Suggested change

/*/extensions/http/stateful_session/cookie @wbpcode @dio @zuercher

/*/extensions/http/stateful_session/cookie @wbpcode @dio

sorry, it's my negligence.

zuercher · 2022-01-04T18:41:44Z

docs/root/configuration/http/http_filters/stateful_session_filter.rst

@@ -0,0 +1,80 @@
+.. _config_http_filters_stateful_session:


Let's add it. Something like:

.. note:: Stateful sessions can result in imbalanced load across upstreams and allow external actors to direct requests to specific upstream hosts. Operators should carefully consider the security and reliability implications of stateful sessions before enabling this feature.

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode · 2022-01-06T17:36:56Z

So may be this can be merged now ? cc @dio cc @zuercher cc @htuch

I think these were all addressed.

zuercher · 2022-01-06T18:05:28Z

@dio can you look at what's changed since your approval and if you're happy, go ahead and merge.

wbpcode · 2022-01-10T08:12:55Z

Hi, @dio , can you take a look again when you have free time? Thanks. 🌷

dio

Looks good! Thanks, @zuercher, @wbpcode!

Introduce a new stateful session extension. This is a change after envoyproxy#17848 envoyproxy#17290. In envoyproxy#17290, we added a new cross-priority host map for fast host searching. In envoyproxy#17848, we extend `LoadBalancerContext` interface to provide an override host and to select the upstream host by the override host. Finally, in this change, we expand a new API to allow users to extract the state of the session from the request and change the result of load balancing by setting the override host value. Related doc: https://docs.google.com/document/d/1IU4b76AgOXijNa4sew1gfBfSiOMbZNiEt5Dhis8QpYg/edit?usp=sharing. Risk Level: Medium Testing: Added Docs Changes: Added Release Notes: Added Platform-Specific Features: N/A. Signed-off-by: wbpcode <wbphub@live.com> Signed-off-by: Josh Perry <josh.perry@mx.com>

@htuch

Stateful session will try to parse upstream address from downstream request directly and override the result of load balancing algorithm by the LoadBalancerContext::overrideHostToSelect API. To avoid the load balancer selecting hosts that in unexpected statuses, specifying some expected statuses are necessary. In the previous design, we will provide expected statuses of override host by the LoadBalancerContext::overrideHostToSelect API. And in the PR #18207, after some discussion with @htuch, we found may be cluster-level config may be more reasonable design and implementation. Ref some more details: #18207 (comment) So this PR try to close previous discussion in the #18207: Refactoring LoadBalancerContext::overrideHostToSelect API to remove expected statuses for the return value. Add new common lb config override_host_status and related implementation. Risk Level: Mid. Testing: N/A. Docs Changes: N/A. Release Notes: N/A. Platform Specific Features: N/A. @wbpcode Signed-off-by: wbpcode <wbphub@live.com>

@htuch

…roxy#19665) Stateful session will try to parse upstream address from downstream request directly and override the result of load balancing algorithm by the LoadBalancerContext::overrideHostToSelect API. To avoid the load balancer selecting hosts that in unexpected statuses, specifying some expected statuses are necessary. In the previous design, we will provide expected statuses of override host by the LoadBalancerContext::overrideHostToSelect API. And in the PR envoyproxy#18207, after some discussion with @htuch, we found may be cluster-level config may be more reasonable design and implementation. Ref some more details: envoyproxy#18207 (comment) So this PR try to close previous discussion in the envoyproxy#18207: Refactoring LoadBalancerContext::overrideHostToSelect API to remove expected statuses for the return value. Add new common lb config override_host_status and related implementation. Risk Level: Mid. Testing: N/A. Docs Changes: N/A. Release Notes: N/A. Platform Specific Features: N/A. @wbpcode Signed-off-by: wbpcode <wbphub@live.com> Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>

wbpcode added 2 commits September 15, 2021 13:01

new api for session sticky

320f403

Signed-off-by: wbpcode <wbphub@live.com>

extension: new extensions for stateful session sticky

9342111

Signed-off-by: wbpcode <wbphub@live.com>

repokitteh-read-only bot added the api label Sep 22, 2021

repokitteh-read-only bot assigned adisuissa Sep 22, 2021

alyssawilk assigned zuercher Sep 22, 2021

zuercher reviewed Sep 22, 2021

View reviewed changes

Apply suggestions from code review

a084c5d

update comments Co-authored-by: Stephan Zuercher <zuercher@gmail.com> Signed-off-by: wbpcode <wbphub@live.com>

wbpcode force-pushed the session-sticky branch from 9a9b387 to a084c5d Compare October 2, 2021 06:37

wbpcode added 3 commits October 4, 2021 11:41

refactor: refactor stateful session with http filter

ed30909

Signed-off-by: wbpcode <wbphub@live.com>

Merge branch 'main' of https://github.com/envoyproxy/envoy into sessi…

8968f3c

…on-sticky

fix docs

007e5c3

Signed-off-by: wbpcode <wbphub@live.com>

zuercher reviewed Oct 5, 2021

View reviewed changes

zuercher added the waiting:any label Oct 8, 2021

repokitteh-read-only bot removed waiting:any labels Oct 11, 2021

wbpcode added 2 commits October 21, 2021 15:40

add maintainers to the codeowner list

b5c28fd

Signed-off-by: wbpcode <wbphub@live.com>

update build

fd2e635

Signed-off-by: wbpcode <wbphub@live.com>

htuch assigned zuercher Dec 30, 2021

zuercher reviewed Jan 3, 2022

View reviewed changes

minor update to address reviewer's comments

23a800e

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode dismissed dio’s stale review via 23a800e January 4, 2022 07:29

fix test

958b209

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode requested a review from zuercher January 4, 2022 11:13

zuercher reviewed Jan 4, 2022

View reviewed changes

minor update

5ab21ee

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode requested a review from zuercher January 5, 2022 05:49

zuercher approved these changes Jan 5, 2022

View reviewed changes

wbpcode requested a review from dio January 12, 2022 09:24

dio approved these changes Jan 12, 2022

View reviewed changes

dio changed the title ~~extension: new extensions for stateful session sticky~~ extension: Add stateful session extension Jan 12, 2022

dio merged commit fb2aad0 into envoyproxy:main Jan 12, 2022

wbpcode mentioned this pull request Jan 24, 2022

support override host status restriction for stateful session #19665

Merged

pcj mentioned this pull request Feb 1, 2022

Traffic shifting with envoy.extensions.filters.http.stateful_session.v3.StatefulSession #19776

Closed

This was referenced Jul 1, 2022

Stateful session affinity for stateful services grpc/grpc#30171

Closed

Implement stateful session affinity to a backend istio/istio#39740

Closed

sanjaypujare mentioned this pull request Jan 3, 2023

Support stateful session affinity for weighted clusters to support use-cases like canary deployment #24741

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extension: Add stateful session extension #18207

extension: Add stateful session extension #18207

wbpcode commented Sep 22, 2021

repokitteh-read-only bot commented Sep 22, 2021

wbpcode commented Sep 22, 2021

wbpcode commented Sep 22, 2021 •

edited

Loading

zuercher left a comment

wbpcode commented Sep 23, 2021

wbpcode commented Sep 23, 2021

snowp commented Sep 28, 2021

wbpcode commented Sep 29, 2021 •

edited

Loading

wbpcode commented Oct 4, 2021

zuercher left a comment

wbpcode commented Oct 6, 2021

htuch commented Oct 20, 2021

wbpcode commented Oct 20, 2021

dio commented Oct 20, 2021 •

edited

Loading

wbpcode commented Oct 21, 2021

htuch commented Dec 30, 2021

zuercher Jan 3, 2022

wbpcode Jan 4, 2022

zuercher Jan 4, 2022

wbpcode commented Jan 4, 2022

zuercher left a comment

zuercher Jan 4, 2022

wbpcode Jan 5, 2022 •

edited

Loading

zuercher Jan 4, 2022

wbpcode commented Jan 6, 2022

zuercher commented Jan 6, 2022

wbpcode commented Jan 10, 2022

dio left a comment

	/*/extensions/http/stateful_session/cookie @wbpcode @dio @zuercher
	/*/extensions/http/stateful_session/cookie @wbpcode @dio

extension: Add stateful session extension #18207

extension: Add stateful session extension #18207

Conversation

wbpcode commented Sep 22, 2021

repokitteh-read-only bot commented Sep 22, 2021

wbpcode commented Sep 22, 2021

wbpcode commented Sep 22, 2021 • edited Loading

zuercher left a comment

Choose a reason for hiding this comment

wbpcode commented Sep 23, 2021

wbpcode commented Sep 23, 2021

snowp commented Sep 28, 2021

wbpcode commented Sep 29, 2021 • edited Loading

wbpcode commented Oct 4, 2021

zuercher left a comment

Choose a reason for hiding this comment

wbpcode commented Oct 6, 2021

htuch commented Oct 20, 2021

wbpcode commented Oct 20, 2021

dio commented Oct 20, 2021 • edited Loading

wbpcode commented Oct 21, 2021

htuch commented Dec 30, 2021

zuercher Jan 3, 2022

Choose a reason for hiding this comment

wbpcode Jan 4, 2022

Choose a reason for hiding this comment

zuercher Jan 4, 2022

Choose a reason for hiding this comment

wbpcode commented Jan 4, 2022

zuercher left a comment

Choose a reason for hiding this comment

zuercher Jan 4, 2022

Choose a reason for hiding this comment

wbpcode Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

zuercher Jan 4, 2022

Choose a reason for hiding this comment

wbpcode commented Jan 6, 2022

zuercher commented Jan 6, 2022

wbpcode commented Jan 10, 2022

dio left a comment

Choose a reason for hiding this comment

wbpcode commented Sep 22, 2021 •

edited

Loading

wbpcode commented Sep 29, 2021 •

edited

Loading

dio commented Oct 20, 2021 •

edited

Loading

wbpcode Jan 5, 2022 •

edited

Loading