stateful_session: added strict mode to header based stateful session #30573

cpakulski · 2023-10-27T18:57:40Z

Commit Message:
Added strict mode to header based stateful session
Additional Description:
Risk Level: Low
Testing: Extended existing tests and added new tests to cover this change
Docs Changes: Yes
Release Notes: Yes
Platform Specific Features: No
Fixes #30475

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

…th strict mode. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

repokitteh-read-only · 2023-10-27T18:57:50Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #30573 was opened by cpakulski.

see: more, trace.

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

wbpcode

This look great. Only some minor comments.

wbpcode · 2023-10-30T04:46:27Z

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

+  // If set to True, the HTTP request must be routed to the requested destination.
+  // If the requested destination is not available, Envoy returns 503. Defaults to False.


nit: If the request destination is not available, Envoy will treat that as no healthy upstream and response with 503 status will be returned.

A high-level question: is this valid only for header, or will this also be used for other types of stateful-session (e.g., cookie). If the latter, then I propose creating a "common" proto, that they all share, and include that in each of them.
Also, consider clarifying what does "not available" means (i.e., non-existent because it was removed, or unhealthy because of active/passive health-check).

cc @adisuissa here is previous response from @cpakulski to this question in another issue. #30475 (comment)

@wbpcode That comment seems to be an argument why this should be configured in the filter instead of in the cluster, and I agree that doing it at the filter level makes sense. But I don't think that answers the question @adisuissa raised about whether this is specific to header-based SSA or could also apply to cookie-based SSA. It seems like it should apply the same to either mechanism -- regardless of whether the affinity information is stored in a header or a cookie, we may want to say that if the specified host is not available, we fail the request instead of picking a different host.

Perhaps this field should be moved to the stateful session filter config itself, so that it applies regardless of what session state extension is used?

I believe that strict setting does not make sense for cookie-based SSA. Let's imagine that there is strict setting for cookie-based SSA. Envoy receives a request, determines that the desired host is not not available and returns 503. A client must send another request WITHOUT cookie. Envoy selects next available upstream host, generates cookie and attaches it to the response. From now on, the client attaches that cookie to each subsequent request to get stickiness. Currently, when upstream host encoded in the cookie is not available, Envoy selects next available upstream host and encodes it in the cookie return in the response. So the final result is the same.
The difference between cookie-based and header-based stateful sessions is that in header-based, encoding format of the header is documented and the client sort of takes over load balancing logic. In cookie-based stateful session, the format and encoding of the cookie are not documented and should not be used by user.
IMHO strict settings only makes sense when user encodes the desired destination and says "I am only interested in that one destination" and such situation takes place only in header-based stateful session.

Thanks for creating that tracking issue, @wbpcode! We can follow up on this discussion over there.

How shall we proceed with this PR? Is strict mode still useful while the question of the session state being a public API is still undecided? If not, then I guess we should hold off on this PR until we resolve that issue. But if it is still useful, then how about we move this new field to the StatefulSession proto, so that it applies equally to both session state mechanisms?

IMO, this strict mode and the format could be completely independent parts and could be implemented in different PR. So, I prefer to proceed with this PR.

But if it is still useful, then how about we move this new field to the StatefulSession proto, so that it applies equally to both session state mechanisms?

Yeah, I agree.

It makes sense to make it a part of StatefulSession proto.

I agree that strict mode is separate and independent from the format. Just need to think about the use case when strict is enabled and the received cookie is expired. I guess that in that case, the stateful filter should immediately return 503 without selecting a new host.

As I mentioned above, I would argue that we should not have tried to solve #26347 on the data plane side in the first place. I think cookie expiration should be strictly an issue for the application to deal with, and the data plane shouldn't even know about the expiration time. I think if the cookie is sent, the data plane should honor it, and strict mode should apply, just as it would be expected to if the cookie were not expired.

@markdroth I think that in this case managing cookie's expiration time falls into grey area. I think that issuer of the cookie should verify expiration time. Normally it is a server.
So, here - on one side, it should not be part of data plane processing, but on the other side Envoy issued the cookie so Envoy should verify it's content.

wbpcode · 2023-10-30T04:48:33Z

envoy/http/filter.h

@@ -757,13 +757,14 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks,
   * host list of the routed cluster, the host should be selected first.
   * @param host The override host address.
   */
-  virtual void setUpstreamOverrideHost(absl::string_view host) PURE;
+  virtual void setUpstreamOverrideHost(absl::string_view host, bool strict) PURE;


virtual void setUpstreamOverrideHost(Upstream::LoadBalancerContext::OverrideHost host) PURE;

I actually left that intensionally in that form. The reason is that info about the desired upstream host is assembled from two distinct pieces: config option strict and IP address from decoded header. There must be a point in the code where those two pieces are assembled into Upstream::LoadBalancerContext::OverrideHost, which is a pair. I thought that it would be cleaner to assemble them in the setUpstreamOverrideHost method, but maybe it would be better to assemble them into a pair before calling that method. Let me experiment.

Refactored.

…eader based stateful sessions. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski · 2023-11-21T19:02:47Z

Moved 'strict' option to main section of API, so it applies to cookie-based and header-based stateful sessions.
I think that this feature is independent from the internal format of the header/cookie.
Let's continue discussion of the format in #30678.

markdroth

/lgtm api

markdroth · 2023-11-27T20:33:00Z

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

@@ -23,6 +23,10 @@ message StatefulSession {
  //
  // [#extension-category: envoy.http.stateful_session]
  config.core.v3.TypedExtensionConfig session_state = 1;
+
+  // If set to True, the HTTP request must be routed to the requested destination.
+  // If the requested destination is not available, Envoy returns 503. Defaults to False.


Just FYI, note that if/when we implement this feature in gRPC, we would return status UNAVAILABLE instead of an HTTP status code. But HTTP 503 maps to UNAVAILABLE anyway, so we don't necessarily need to explicitly note this here.

Thanks for that info and reviewing API.
So, it would be HTTP 200 with grpc-status header code different than zero, correct?

In gRPC, the SSA feature operates on the client side, not the server side, so there is no HTTP code involved; it would simply report to the application that the RPC failed with status UNAVAILABLE.

If someone used gRPC with an Envoy sidecar proxy and the SSA feature was used in Envoy instead of in gRPC, then Envoy would return HTTP status 503, which the gRPC client would map to status UNAVAILABLE. So the client application would see the same result in both cases.

Thanks for explaining!

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski · 2023-12-04T19:49:13Z

/retest

wbpcode

LGTM with only one minor comment. Thanks for your such great contribution. 🌷

wbpcode · 2023-12-05T02:00:36Z

envoy/http/filter.h

-  virtual void setUpstreamOverrideHost(absl::string_view host) PURE;
+  virtual void setUpstreamOverrideHost(Upstream::LoadBalancerContext::OverrideHost&&) PURE;


does this move semantics make sense? The override host just a pair of string view and bool.

I think you are right. Move will most likely apply only to std::pair internals. string_view and bool will just copy their values. I will change it to pass by value. Thanks!

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

wbpcode

LGTM. Thanks for your great work!!!

cpakulski added 5 commits October 23, 2023 19:44

Implemented API and added code to handle strict routing to a host.

3f549f7

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Adjusted existing tests.

fdf21ed

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Added unit and integration tests for header-based stateful session wi…

2fd4080

…th strict mode. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Merge remote-tracking branch 'upstream/main' into strict_stateful

9bab360

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Added release notes.

95c1b33

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski requested a review from wbpcode as a code owner October 27, 2023 18:57

repokitteh-read-only bot added the api label Oct 27, 2023

repokitteh-read-only bot assigned mattklein123 Oct 27, 2023

cpakulski added 4 commits October 27, 2023 19:44

Fixed proto format.

bed1e38

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Fixed router's test.

056e522

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Fixed cluster manager test.

9cca4b1

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Merge remote-tracking branch 'upstream/main' into strict_stateful

b811358

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

wbpcode reviewed Oct 30, 2023

View reviewed changes

mattklein123 added the waiting label Oct 30, 2023

mattklein123 assigned wbpcode and unassigned mattklein123 Oct 30, 2023

markdroth mentioned this pull request Nov 2, 2023

unified format API of stateful session header/cookie value #30678

Open

cpakulski added 2 commits November 21, 2023 16:10

Moved strict mode to stateful session API. It applies to cookie and h…

963c49b

…eader based stateful sessions. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Merge remote-tracking branch 'upstream/main' into strict_stateful

0f03e40

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

repokitteh-read-only bot removed the waiting label Nov 21, 2023

markdroth reviewed Nov 27, 2023

View reviewed changes

repokitteh-read-only bot removed the api label Nov 27, 2023

cpakulski added 2 commits December 4, 2023 17:03

refactored setUpstreamOverrideHost method.

d7242cd

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Merge remote-tracking branch 'upstream/main' into strict_stateful

993f925

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski requested a review from wbpcode December 4, 2023 20:29

wbpcode reviewed Dec 5, 2023

View reviewed changes

Pass parameter to setUpstreamOverrideHost as value.

2148718

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski added 2 commits December 5, 2023 19:23

Merge remote-tracking branch 'upstream/main' into strict_stateful

de8bc31

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Formatting.

1b7e75a

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski requested a review from wbpcode December 6, 2023 16:48

wbpcode approved these changes Dec 7, 2023

View reviewed changes

wbpcode merged commit 62f4a14 into envoyproxy:main Dec 7, 2023
106 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stateful_session: added strict mode to header based stateful session #30573

stateful_session: added strict mode to header based stateful session #30573

cpakulski commented Oct 27, 2023

repokitteh-read-only bot commented Oct 27, 2023

wbpcode left a comment

wbpcode Oct 30, 2023

adisuissa Oct 30, 2023

wbpcode Oct 31, 2023

markdroth Oct 31, 2023

cpakulski Oct 31, 2023

markdroth Nov 3, 2023

wbpcode Nov 3, 2023

cpakulski Nov 10, 2023

markdroth Nov 10, 2023

cpakulski Nov 21, 2023

wbpcode Oct 30, 2023

cpakulski Oct 31, 2023

cpakulski Dec 4, 2023

cpakulski commented Nov 21, 2023

markdroth left a comment

markdroth Nov 27, 2023

cpakulski Nov 29, 2023

markdroth Nov 29, 2023

cpakulski Nov 30, 2023

cpakulski commented Dec 4, 2023

wbpcode left a comment •

edited

Loading

wbpcode Dec 5, 2023

cpakulski Dec 5, 2023

wbpcode left a comment

		// If set to True, the HTTP request must be routed to the requested destination.
		// If the requested destination is not available, Envoy returns 503. Defaults to False.

		virtual void setUpstreamOverrideHost(absl::string_view host) PURE;
		virtual void setUpstreamOverrideHost(Upstream::LoadBalancerContext::OverrideHost&&) PURE;

stateful_session: added strict mode to header based stateful session #30573

stateful_session: added strict mode to header based stateful session #30573

Conversation

cpakulski commented Oct 27, 2023

repokitteh-read-only bot commented Oct 27, 2023

wbpcode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpakulski commented Nov 21, 2023

markdroth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpakulski commented Dec 4, 2023

wbpcode left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode left a comment

Choose a reason for hiding this comment

wbpcode left a comment •

edited

Loading