filter state: add a mark for sharing with the upstream by kyessenov · Pull Request #21700 · envoyproxy/envoy

kyessenov · 2022-06-14T17:29:56Z

Commit Message: Adds a new flag for filter state objects that indicates the intent to share with the upstream.
Additional Description: Follow-up to #19809. There have been multiple reports of unexpected lifecycle changes for the filter state objects because they are stored in the transport socket options. This PR addresses this issue by introducing a new mark for filter state that explicitly changes the usage of filter state objects:

marked objects always participate in the connection pool hashing (generalizing and simplifying transport sockets: support passthrough state for internal connections #19435);
marked objects are copied by reference to the upstream info - this allows sharing state between downstream and upstream (and further down the chain, the internal listeners).

Risk Level: medium, revert to the original behavior prior to #19809
Testing: yes
Docs Changes: yes
Release Notes: yes

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only · 2022-06-14T17:30:00Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #21700 was opened by kyessenov.

see: more, trace.

Signed-off-by: Kuat Yessenov <kuat@google.com>

ggreenway

This approach seems good to me.

/wait

ggreenway · 2022-06-16T15:52:03Z

envoy/stream_info/filter_state.h

 class FilterState {
 public:
-  enum class StateType { ReadOnly, Mutable };
+  enum StateType {


I think it's simpler to just leave this as the previous enum, and add another bool for SharedWithUpstreamConnection.

When the upstream L7 filters get done, we'll also need to share with the upstream request I think. I could add another enum for sharing configuration I suppose?

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only · 2022-06-28T05:34:39Z

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

🐱

Caused by: #21700 was synchronize by kyessenov.

see: more, trace.

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2022-06-28T18:38:40Z

@ggreenway This should be ready now. I could simplify the recently merged extension, too, which was the only client in-tree for filter state within transport socket options.

Signed-off-by: Kuat Yessenov <kuat@google.com>

lizan

/lgtm api

breaking API changes approved per exception:
Changes made within 14 days of the introduction of a new API field or message, provided the new field or message has not been included in an Envoy release.

lizan · 2022-06-28T20:21:53Z

envoy/event/dispatcher.h

+      Network::Address::InstanceConstSharedPtr source_address,
+      Network::TransportSocketPtr&& transport_socket,
+      const Network::ConnectionSocket::OptionsSharedPtr& options,
+      const Network::TransportSocketOptionsConstSharedPtr& transport_options = nullptr) PURE;


no default argument for virtual methods per style guide.

Signed-off-by: Kuat Yessenov <kuat@google.com>

ggreenway

/wait

changelogs/current.yaml

ggreenway · 2022-06-29T15:02:51Z

envoy/network/transport_socket.h

+   * that are marked as shared with the upstream connection.
   */
-  virtual const StreamInfo::FilterStateSharedPtr& filterState() const PURE;
+  virtual const StreamInfo::FilterState::Objects& filterStateObjects() const PURE;


I think this function name should be more descriptive, maybe something like downstreamSharedFilterStateObjects().

ggreenway · 2022-06-29T15:04:25Z

envoy/stream_info/filter_state.h

+  // state.
+  //
+  // As a special case, objects that are marked as shared with the upstream
+  // become bound to the upstream connection life span, regardless of the


clarify: the object will live as long as either the downstream or upstream connection is open. The current wording implies that the lifetime is strictly tied to the upstream connection.

Clarified, LMK if it can be improved further.

ggreenway · 2022-06-29T15:05:44Z

envoy/stream_info/filter_state.h

+    std::shared_ptr<Object> data_;
+    StateType state_type_;
+    StreamSharing stream_sharing_{StreamSharing::None};
+    std::string name_{""};


The initialization can just be {} instead of {""}, but empty is the default so it can be omitted entirely.

ggreenway · 2022-06-29T15:17:33Z

source/common/network/transport_socket_options_impl.cc

-  if (!filter_state) {
-    return nullptr;
+  for (const auto& object : options->filterStateObjects()) {
+    if (auto hashable = dynamic_cast<const Hashable*>(object.data_.get()); hashable != nullptr) {


What happens if an object isn't Hashable? Can we end up with bugs due to matching transport socket options hash even though it contains different filter state? Should FilterState::Object always inherit from Hashable? Or maybe make it required when the object opts in to sharing, via an accessor like getHashable()? We mostly try to avoid dynamic_cast in envoy for this type of feature detection.

Hashable is only needed for L7->L4 stream transition. For tcp_proxy, it's already 1-1 so hashing is not necessary. I followed https://www.envoyproxy.io/docs/envoy/latest/api-v3/type/v3/hash_policy.proto#envoy-v3-api-msg-type-v3-hashpolicy-filterstate which does the same dynamic cast test.

ggreenway · 2022-06-29T15:27:43Z

source/extensions/io_socket/user_space/io_handle_impl.cc

+      filter_state.setData(object.name_, object.data_, object.state_type_,
+                           StreamInfo::FilterState::LifeSpan::Connection, object.stream_sharing_);
+    } catch (const EnvoyException& e) {
+      ENVOY_LOG(trace, "Failed to set data for '{}': {}", object.name_, e.what());


Ignoring this error seems unsafe; something important may have been discarded. Are there cases where this is expected to happen? Should this have an ENVOY_BUG()?

I think this never throws because the stream info is new and objects have distinct names. Removed try/catch.

Signed-off-by: Kuat Yessenov <kuat@google.com>

lizan · 2022-06-29T22:51:57Z

envoy/stream_info/filter_state.h


+  struct FilterObject {
+    std::shared_ptr<Object> data_;
+    StateType state_type_;


Suggested change

StateType state_type_;

StateType state_type_{};

default initialize this?

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2022-06-30T17:17:40Z

/retest

repokitteh-read-only · 2022-06-30T17:17:49Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #21700 (comment) was created by @kyessenov.

see: more, trace.

lizan

LGTM, can you resolve conflicts?

Signed-off-by: Kuat Yessenov <kuat@google.com>

ggreenway · 2022-07-05T18:12:04Z

/retest

repokitteh-read-only · 2022-07-05T18:12:08Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #21700 (comment) was created by @ggreenway.

see: more, trace.

ggreenway

LGTM

kyessenov added 2 commits June 13, 2022 16:40

add headers

1c0cf91

Signed-off-by: Kuat Yessenov <kuat@google.com>

draft

52bd5c7

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov mentioned this pull request Jun 14, 2022

FilterState LifeSpan::Connection lifetime lasts longer than the downstream connection #21663

Closed

fix release

83a8469

Signed-off-by: Kuat Yessenov <kuat@google.com>

ggreenway requested changes Jun 16, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 16, 2022

kyessenov added 3 commits June 27, 2022 15:10

merge fix

03318e8

Signed-off-by: Kuat Yessenov <kuat@google.com>

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

7a1b3ee

changes

218f97d

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only bot added api and removed waiting labels Jun 28, 2022

repokitteh-read-only bot assigned lizan Jun 28, 2022

kyessenov added 2 commits June 28, 2022 09:51

wip

eaca9db

Signed-off-by: Kuat Yessenov <kuat@google.com>

tests

0c3b11f

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov marked this pull request as ready for review June 28, 2022 18:37

kyessenov requested review from adisuissa, antoniovicente and lizan as code owners June 28, 2022 18:37

fix

9e5a187

Signed-off-by: Kuat Yessenov <kuat@google.com>

lizan reviewed Jun 28, 2022

View reviewed changes

repokitteh-read-only bot removed the api label Jun 28, 2022

remove default argument

cd1f7c5

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov requested review from KBaichoo and alyssawilk as code owners June 28, 2022 22:11

fix tests

7ff0070

Signed-off-by: Kuat Yessenov <kuat@google.com>

ggreenway self-assigned this Jun 29, 2022

ggreenway requested changes Jun 29, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 29, 2022

review

1948a0a

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only bot removed the waiting label Jun 29, 2022

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

643e24b

lizan reviewed Jun 29, 2022

View reviewed changes

kyessenov added 2 commits June 29, 2022 15:54

default initialize

a16242b

Signed-off-by: Kuat Yessenov <kuat@google.com>

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

4566981

kyessenov mentioned this pull request Jun 30, 2022

Adding direct connect support. #21942

Merged

lizan reviewed Jul 1, 2022

View reviewed changes

kyessenov added 2 commits July 1, 2022 14:25

merge fix

a1fd5df

Signed-off-by: Kuat Yessenov <kuat@google.com>

merge fix

30250ba

Signed-off-by: Kuat Yessenov <kuat@google.com>

ggreenway approved these changes Jul 5, 2022

View reviewed changes

kyessenov merged commit 18212bb into envoyproxy:main Jul 6, 2022

Conversation

kyessenov commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jun 14, 2022

Uh oh!

ggreenway 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!

repokitteh-read-only bot commented Jun 28, 2022

Uh oh!

kyessenov commented Jun 28, 2022

Uh oh!

lizan 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!

ggreenway left a comment

Choose a reason for hiding this comment

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!

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!

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!

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!

kyessenov commented Jun 30, 2022

Uh oh!

repokitteh-read-only bot commented Jun 30, 2022

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

ggreenway commented Jul 5, 2022

Uh oh!

repokitteh-read-only bot commented Jul 5, 2022

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kyessenov commented Jun 14, 2022 •

edited

Loading