refactor: upstream host override struct by rudrakhp · Pull Request #43944 · envoyproxy/envoy

rudrakhp · 2026-03-13T09:01:03Z

Commit Message: refactor: upstream host override struct
Additional Description: Refactor is required for a subsequent change proposed to add a status to return if strict destination is missing.
Risk Level: Low (internal refactor)
Testing: Unit tests
Docs Changes: No
Release Notes: No
Related #42686

repokitteh-read-only · 2026-03-13T09:01:16Z

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

🐱

Caused by: #43944 was opened by rudrakhp.

see: more, trace.

paul-r-gall · 2026-03-17T13:06:53Z

Please resolve merge conflicts; do not rebase, instead do a merge of the conflicts.

adisuissa · 2026-03-17T18:53:40Z

+  // <envoy_v3_api_field_extensions.filters.http.stateful_session.v3.StatefulSession.strict>` is enabled
+  // and the requested destination is unavailable.
+  //
+  // This allows clients to distinguish between transient failures and errors from upstream.


From a high-level view, it doesn't seem that the feature addresses the description here. Specifically the error-code could be originating from the upstream or from Envoy, and the downstream client will not know what was the cause, no?

While the status itself wouldn't be enough to determine whether it was due to no available upstream or status from the upstream itself, it would let users configure it to something that is considered safe to retry. For example, it is safe for the downstream client to retry on 425 irrespective of where the status is coming from unlike 503 which might not be safe to retry due to idempotency as pointed out in the issue.

I think this implies that the downstream client knows what the config knob (strict) value is on the proxy, which seems like a broken abstraction that I'm not sure is needed here.

I prefer removing this, and updating the strict description to say that Envoy will return the error code set by this field.

rudrakhp · 2026-03-20T23:47:04Z

/retest

adisuissa · 2026-03-24T15:35:30Z

+  //
+  //   This field only takes effect when ``strict`` is set to ``true``. In non-strict mode,
+  //   requests are routed according to the load balancer and this status code is not used.
+  type.v3.HttpStatus status_on_missing_destination = 4;


Suggested change

type.v3.HttpStatus status_on_missing_destination = 4;

type.v3.HttpStatus status_on_strict_unavailable_destination = 4;

Will go with status_on_missing_strict_destination in the follow up PR. Didn't want to use the term unavailable as it might be conflated with availability as in non-5XX responses, but we mean the destination not being present for it to connect to.

adisuissa · 2026-03-24T15:40:31Z

+  // <envoy_v3_api_field_extensions.filters.http.stateful_session.v3.StatefulSession.strict>` is enabled
+  // and the requested destination is unavailable.
+  //
+  // This allows clients to distinguish between transient failures and errors from upstream.


I think this implies that the downstream client knows what the config knob (strict) value is on the proxy, which seems like a broken abstraction that I'm not sure is needed here.

I prefer removing this, and updating the strict description to say that Envoy will return the error code set by this field.

adisuissa

Overall refactor LGTM.
Assigning @wbpcode for a senior-maintainer review.
/assign @wbpcode

adisuissa · 2026-03-24T20:30:05Z

  Network::Socket::OptionsSharedPtr upstream_options_ =
      std::make_shared<Network::Socket::Options>();
-  std::pair<std::string, bool> upstream_override_host_;
+  struct UpstreamOverrideHostStorage {


I wonder whether the "storage" (UpstreamOverrideHostStorage) and "non-storage" (UpstreamOverrideHost) should just be a plain object (just like the storage one), and references to the plain object.
cc @wbpcode who wrote the original code and probably has more context.

I agree with @adisuissa. I can use the UpstreamOverrideHost to store the string and also bool.

And we can update the upstreamOverrideHost() method to return a OptRef rather then current absl::optional<Upstream::LoadBalancerContext::OverrideHost>.

wbpcode

LGTM overall with some minor comments. Thanks for the update.

/wait

Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>

rudrakhp · 2026-03-27T04:17:20Z

/retest

wbpcode · 2026-03-27T07:13:02Z

Please don't use force push once the review started. You could merge main rather than rebase which would be more friendly to reviewer. :)

Commit Message: refactor: upstream host override struct Additional Description: Refactor is required for a subsequent change proposed to add a status to return if strict destination is missing. Risk Level: Low (internal refactor) Testing: Unit tests Docs Changes: No Release Notes: No Related envoyproxy#42686 --------- Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com> Signed-off-by: Xuyang Tao <taoxuy@google.com>

Commit Message: refactor: upstream host override struct Additional Description: Refactor is required for a subsequent change proposed to add a status to return if strict destination is missing. Risk Level: Low (internal refactor) Testing: Unit tests Docs Changes: No Release Notes: No Related envoyproxy#42686 --------- Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com> Signed-off-by: Jonathan Wu <jtwu@google.com>

Commit Message: refactor: upstream host override struct Additional Description: Refactor is required for a subsequent change proposed to add a status to return if strict destination is missing. Risk Level: Low (internal refactor) Testing: Unit tests Docs Changes: No Release Notes: No Related envoyproxy#42686 --------- Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com> Signed-off-by: Nick Shipilov <nick.shipilov.n@gmail.com>

Commit Message: refactor: upstream host override struct Additional Description: Refactor is required for a subsequent change proposed to add a status to return if strict destination is missing. Risk Level: Low (internal refactor) Testing: Unit tests Docs Changes: No Release Notes: No Related envoyproxy#42686 --------- Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>

rudrakhp requested review from adisuissa, agrawroh, mathetake, mattklein123 and wbpcode as code owners March 13, 2026 09:01

repokitteh-read-only Bot added the api label Mar 13, 2026

repokitteh-read-only Bot assigned adisuissa Mar 13, 2026

rudrakhp force-pushed the strict_stateful_session_status branch 4 times, most recently from 7894d9b to d5a9919 Compare March 13, 2026 12:12

rudrakhp requested review from botengyao and yanavlasov as code owners March 13, 2026 12:12

rudrakhp force-pushed the strict_stateful_session_status branch 3 times, most recently from aaa0098 to 12b45da Compare March 13, 2026 15:26

adisuissa reviewed Mar 17, 2026

View reviewed changes

rudrakhp requested a review from adisuissa March 18, 2026 21:19

adisuissa reviewed Mar 24, 2026

View reviewed changes

rudrakhp force-pushed the strict_stateful_session_status branch from 713aa72 to e132250 Compare March 24, 2026 17:54

rudrakhp changed the title ~~feat: ability to set status for missing dest in strict stateful session~~ refactor: upstream host override struct Mar 24, 2026

rudrakhp force-pushed the strict_stateful_session_status branch from e132250 to ccc35ea Compare March 24, 2026 18:03

rudrakhp requested a review from adisuissa March 24, 2026 18:03

adisuissa previously approved these changes Mar 24, 2026

View reviewed changes

rudrakhp dismissed adisuissa’s stale review via fea9231 March 26, 2026 04:34

rudrakhp requested review from nezdolik and zuercher as code owners March 26, 2026 04:34

wbpcode reviewed Mar 26, 2026

View reviewed changes

repokitteh-read-only Bot added waiting and removed waiting labels Mar 26, 2026

rudrakhp force-pushed the strict_stateful_session_status branch 3 times, most recently from 22d3729 to 57b41de Compare March 26, 2026 15:33

rudrakhp added 3 commits March 27, 2026 08:34

refactor: upstream host override struct

d8fc620

Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>

use OptRef instead of absl optional

24d3dda

Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>

use strict typing and remove unused constructors

4fb16f9

Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>

rudrakhp force-pushed the strict_stateful_session_status branch from 57b41de to 4fb16f9 Compare March 27, 2026 03:04

rudrakhp requested a review from wbpcode March 27, 2026 06:16

wbpcode approved these changes Mar 27, 2026

View reviewed changes

wbpcode merged commit 9457518 into envoyproxy:main Mar 27, 2026
29 checks passed

rudrakhp mentioned this pull request Apr 1, 2026

feat: ability to set status for destination not found in stateful session #44210

Merged

	type.v3.HttpStatus status_on_missing_destination = 4;
	type.v3.HttpStatus status_on_strict_unavailable_destination = 4;

Conversation

rudrakhp commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only Bot commented Mar 13, 2026

Uh oh!

paul-r-gall commented Mar 17, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rudrakhp commented Mar 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rudrakhp commented Mar 27, 2026

Uh oh!

wbpcode commented Mar 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rudrakhp commented Mar 13, 2026 •

edited

Loading