stream_info: Add the concept of "life span" to StreamInfo::FilterState by penguingao · Pull Request #9202 · envoyproxy/envoy

penguingao · 2019-12-03T15:32:07Z

Description: Add the concept of "life span" to StreamInfo::FilterState so that we can make some object survive across recreateStream during internal redirect handling. Since internal redirect recreates the entire filter chain, we need a mechanism for router filter to keep track of number of redirects that have been followed in order to support multiple hop redirects. Connection level sharing is also possible with FilterState::LifeSpan::DownstreamConnection.

This makes the filter implementation explicitly set the desire life span of their filter state object, so that the implementation can be internal redirect aware if so desired.

This is part of #9143. I intend to better handle StreamInfo/logging for internal redirect handling in a separate PR.

Risk Level: Medium
Testing: Unit test added
Docs Changes: N/A
Release Notes: N/A

FilterState objects across recreateStream. Signed-off-by: Peng Gao <pengg@google.com>

Signed-off-by: Peng Gao <pengg@google.com>

penguingao · 2019-12-03T15:34:38Z

@alyssawilk

it instead of a pointer to FilterState. Signed-off-by: Peng Gao <pengg@google.com>

alyssawilk

I think this is a simple and elegant way to allow the custom data to persist, regardless of what entity is doing the redirect.
Tagging @mattklein123 for thoughts rather than merging because he may see some gotchas I'm missing.

Signed-off-by: Peng Gao <pengg@google.com>

kyessenov · 2019-12-03T17:13:23Z

Neat design. I wonder if we can add Connection life span that allows copying from a connection stream info to a request stream info. That way a TCP filter can pass data to HTTP filters. We would make Connection life span the lowest and copy FilterChain and above data into HTTP filter stream info.

penguingao · 2019-12-03T17:17:42Z

@alyssawilk Thanks for the review!

@kyessenov I did think about it, and I actually left a comment in filter_state.h saying we can do that in the future. The connection level sharing will be a little trickier than DownstreamRequest scope since there may be multiple active streams at the same time. We need some callback and feedback mechanisms to make it work. That said, I wanted to do it, but changed my mind to defer to when we need such capability.

kyessenov · 2019-12-03T18:33:33Z

@penguingao We do have a need so I can follow up with that. Can you clarify a bit what you mean by callbacks needed to make it work? I would assume it's a matter of copying connection filter state into stream filter state on creation. It might be OK to mutate the connection filter state from multiple HTTP streams (e.g. count number of active streams?), it's a single silo after all.

penguingao · 2019-12-03T18:48:56Z

@kyessenov, ok. Let me prepare another PR to add it.

I think what I meant is that we should better pass objects created by setData that has Connection life span to a connection level data structure during the call, because multiple active streams can call setData.

That way, we don't have to explicitly copy the newly added object to all other active stream's filter state.

kyessenov · 2019-12-03T19:21:51Z

@penguingao I just discussed this with @gargnupur. We have some concerns about the copying that would happen with every request. Have you considered chaining filter states instead? We can change lookup function to traverse a parent as a fallback at a specific life span. Given that the lifetimes are clear in the case of HTTP vs TCP stream infos, the ownership is a non-issue.

penguingao · 2019-12-03T19:23:05Z

@kyessenov that's exactly what I was proposing for my next PR

gargnupur · 2019-12-03T19:47:56Z

@penguingao : This solution is quite neat!

include/envoy/stream_info/filter_state.h

mattklein123

At a high this looks good to me. If we know that we need connection level sharing, can we potentially merge it into this PR? It's not that big and I'm a little concerned that connection level sharing is going to slightly change the design. Also, is it possible to have an internal redirection test that shows this new behavior to verify that aspect of the code? Thank you!

/wait

source/common/stream_info/filter_state_impl.cc

penguingao · 2019-12-03T21:12:06Z

@mattklein123
re: merge connection level sharing into this PR. SGTM. I'll make that change.

re: internal redirect test: I was planning to add that test when I add multiple hop redirects, where the state sharing is actually used. Let me make internal redirect to use FilterState in this PR to make it tested as well.

Signed-off-by: Peng Gao <pengg@google.com>

callers explicitly specify what they want. Fixing tests as well. Signed-off-by: Peng Gao <pengg@google.com>

Signed-off-by: Peng Gao <pengg@google.com>

penguingao · 2019-12-04T15:20:26Z

/wait

Signed-off-by: Peng Gao <pengg@google.com>

penguingao · 2019-12-09T13:53:13Z

@alyssawilk @mattklein123 PTAL when you have a chance.

This has gotten a bit more complicated than I imagined to support the connection level sharing, but at least I made the FilterStateImpl self contained to support lazy init of parent state - only HCM needs to care about the slightly complex constructor with a reference to a shared_ptr. Let me know what you think.

Thanks!

in the constructor. Signed-off-by: Peng Gao <pengg@google.com>

alyssawilk

Well this got more complicated since last I looked. Also far more useful - thanks! :-)

include/envoy/stream_info/filter_state.h

source/common/http/conn_manager_impl.cc

source/common/stream_info/filter_state_impl.cc

Update FilterState top level comment. Add comment in recreateStream for on-demand copy. Rename FilterState(Impl)::hasDataAtOrAboveLifeSpan. Signed-off-by: Peng Gao <pengg@google.com>

Signed-off-by: Peng Gao <pengg@google.com>

mattklein123

Thanks this is really awesome work! Just a bunch of random small comments. Can you also check the code coverage report in CI to make sure you have 100% coverage for all of the new branching states you added? Thank you!

/wait

mattklein123 · 2019-12-10T20:56:09Z

include/envoy/stream_info/filter_state.h

+  // FilterStateImpl at the same life span. This interface is supposed to be accessed at the leaf
+  // level (FilterChain) for objects with any desired longer life span.
+  //
+  // - FilterChain has the shortest life span, which is as long as the http filter chain lives.


nit: would remove http here as this equally applies to network filters.

mattklein123 · 2019-12-10T20:57:03Z

include/envoy/stream_info/filter_state.h

+  //
+  // - DownstreamRequest is longer than FilterChain. When internal redirect is enabled, one
+  //   downstream request may create multiple filter chains. DownstreamRequest allows an Object to
+  //   survived across filter chains for book needs.


survive across filter chains for bookkeeping needs.

mattklein123 · 2019-12-10T20:59:20Z

include/envoy/stream_info/filter_state.h

+   */
+  virtual LifeSpan lifeSpan() const PURE;
+
+  /**


* @return the pointer of the parent FilterState that has longer life span. nullptr means this is * either the top LifeSpan or the parent is not yet created.

mattklein123 · 2019-12-10T21:05:55Z

source/common/http/conn_manager_impl.h


  TimeSource& timeSource() { return time_source_; }

+  std::shared_ptr<StreamInfo::FilterState>& filterState() { return filter_state_; }


nit: can you add a small comment on why we return this by reference?

mattklein123 · 2019-12-10T21:07:02Z

source/common/stream_info/filter_state_impl.cc

+      throw EnvoyException(
+          "FilterState::setData<T> called twice with conflicting life_span on the same data_name.");
+    }
+    maybeCreateParent(/* read_only = */ false);


nit: if you find yourself wanting to have these comments here, I would recommend making this function take a dual state enum that will provide more clarity. TIOLO.

mattklein123 · 2019-12-10T21:14:21Z

source/common/stream_info/stream_info_impl.h

+      : time_source_(time_source), start_time_(time_source.systemTime()),
+        start_time_monotonic_(time_source.monotonicTime()),
+        filter_state_(std::make_shared<FilterStateImpl>(FilterState::LifeSpan::FilterChain)) {
+    protocol_ = protocol;


nit: while you are in here, initialize protocol_ in the initializer list, same below.

- remove http from FilterState::LifeSpan::FilterChain comment. - correctly spell survive and bookkeeping. - point -> pointer and grammar - comment why return reference from ConnectionManagerImpl::filterState(). - make FilterStateImpl::maybeCreateParent take a two state enum to avoid comment for boolean value. - Init protocol_ in the constructors of StreamInfoImpl. Signed-off-by: Peng Gao <pengg@google.com>

mattklein123

LGTM, thanks. @alyssawilk any further comments? @penguingao did you check the coverage report also?

penguingao · 2019-12-10T23:46:09Z

@mattklein123 for some reason, my ci run is saying this: "Coverage report will not be uploaded for this build."

I am running the coverage on my workstation to see if I can get a read on the coverage report.

mattklein123 · 2019-12-11T00:19:58Z

@penguingao that is probably just a script bug. I see the artifacts in the circle CI UI: https://305966-65214191-gh.circle-artifacts.com/0/coverage/index.html

penguingao · 2019-12-11T00:25:57Z

@mattklein123 Ah, thanks. I am not yet familiar with CI. Should have looked deeper.

I checked files I modified, all new branches are covered.

penguingao · 2019-12-11T00:56:12Z

/azp run envoy-windows

azure-pipelines · 2019-12-11T00:56:17Z

Commenter does not have sufficient privileges for PR 9202 in repo envoyproxy/envoy

penguingao · 2019-12-11T00:58:32Z

/retest

repokitteh-read-only · 2019-12-11T00:58:37Z

🙀 Error while processing event:

evaluation error
error: function error error: user: combined status is failure, but no failed builds.

🐱

Caused by: a #9202 (comment) was created by @penguingao.

see: more, trace.

include/envoy/stream_info/filter_state.h

source/extensions/filters/network/sni_cluster/sni_cluster.cc

- Object -> object - sni_cluster is a network filter so call FilterState with the correct life span Signed-off-by: Peng Gao <pengg@google.com>

…etwork filters. Signed-off-by: Peng Gao <pengg@google.com>

penguingao · 2019-12-11T20:10:32Z

@alyssawilk I've made change to address your comments. Tests are running.

envoyproxy#9202) Signed-off-by: Peng Gao <pengg@google.com> Signed-off-by: Prakhar <prakhar_au@yahoo.com>

penguingao added 4 commits December 3, 2019 09:01

Add "life span" concept to FilterState and use it for sharing

a95f443

FilterState objects across recreateStream. Signed-off-by: Peng Gao <pengg@google.com>

Add test for FilterState::CopyInto

6323439

Signed-off-by: Peng Gao <pengg@google.com>

Fix format and comment.

33bc63d

Signed-off-by: Peng Gao <pengg@google.com>

Fix comment and format.

4999df4

Signed-off-by: Peng Gao <pengg@google.com>

Add default life_span value to FilterStateImpl as well since tests use

125cc54

it instead of a pointer to FilterState. Signed-off-by: Peng Gao <pengg@google.com>

alyssawilk previously approved these changes Dec 3, 2019

View reviewed changes

alyssawilk assigned mattklein123 and alyssawilk Dec 3, 2019

Kick CI

4683ed5

Signed-off-by: Peng Gao <pengg@google.com>

mattklein123 reviewed Dec 3, 2019

View reviewed changes

include/envoy/stream_info/filter_state.h Show resolved Hide resolved

mattklein123 requested changes Dec 3, 2019

View reviewed changes

source/common/stream_info/filter_state_impl.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Dec 3, 2019

penguingao added 3 commits December 4, 2019 08:19

data_ is a shared_ptr.

ce7306c

Signed-off-by: Peng Gao <pengg@google.com>

Remove default parameter for life_span in FilterState::setData to let

f76d532

callers explicitly specify what they want. Fixing tests as well. Signed-off-by: Peng Gao <pengg@google.com>

Fix more test.

84ea166

Signed-off-by: Peng Gao <pengg@google.com>

penguingao dismissed alyssawilk’s stale review via 84ea166 December 4, 2019 14:58

penguingao requested a review from lizan as a code owner December 4, 2019 14:58

repokitteh-read-only bot removed the waiting label Dec 4, 2019

repokitteh-read-only bot added the waiting label Dec 4, 2019

Revert to use default parameters

08f1ed0

Signed-off-by: Peng Gao <pengg@google.com>

Fix format.

afcbaa7

Signed-off-by: Peng Gao <pengg@google.com>

Fix some comment and make filter_state_ in conn_manager_impl not created

4c10770

in the constructor. Signed-off-by: Peng Gao <pengg@google.com>

alyssawilk reviewed Dec 9, 2019

View reviewed changes

include/envoy/stream_info/filter_state.h Outdated Show resolved Hide resolved

source/common/http/conn_manager_impl.cc Outdated Show resolved Hide resolved

source/common/stream_info/filter_state_impl.cc Outdated Show resolved Hide resolved

penguingao added 2 commits December 9, 2019 14:38

Address comments:

9282aea

Update FilterState top level comment. Add comment in recreateStream for on-demand copy. Rename FilterState(Impl)::hasDataAtOrAboveLifeSpan. Signed-off-by: Peng Gao <pengg@google.com>

Kick CI

3a590a1

Signed-off-by: Peng Gao <pengg@google.com>

penguingao requested a review from mattklein123 December 9, 2019 22:21

mattklein123 requested changes Dec 10, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 10, 2019

repokitteh-read-only bot removed the waiting label Dec 10, 2019

Merge remote-tracking branch 'upstream/master' into filter-state

868ea9e

mattklein123 previously approved these changes Dec 10, 2019

View reviewed changes

alyssawilk reviewed Dec 11, 2019

View reviewed changes

include/envoy/stream_info/filter_state.h Outdated Show resolved Hide resolved

source/extensions/filters/network/sni_cluster/sni_cluster.cc Outdated Show resolved Hide resolved

Address comments:

1987d25

- Object -> object - sni_cluster is a network filter so call FilterState with the correct life span Signed-off-by: Peng Gao <pengg@google.com>

penguingao dismissed mattklein123’s stale review via 1987d25 December 11, 2019 19:11

Make tests also call FilterState::setData with correct LifeSpan for n…

3f85eb7

…etwork filters. Signed-off-by: Peng Gao <pengg@google.com>

alyssawilk approved these changes Dec 11, 2019

View reviewed changes

mattklein123 merged commit ed29591 into envoyproxy:master Dec 11, 2019

penguingao deleted the filter-state branch December 11, 2019 23:52

prakhag1 pushed a commit to prakhag1/envoy that referenced this pull request Jan 3, 2020

stream_info: Add the concept of "life span" to StreamInfo::FilterState (

ca14c2a

envoyproxy#9202) Signed-off-by: Peng Gao <pengg@google.com> Signed-off-by: Prakhar <prakhar_au@yahoo.com>


		TimeSource& timeSource() { return time_source_; }

		std::shared_ptr<StreamInfo::FilterState>& filterState() { return filter_state_; }

Conversation

penguingao commented Dec 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

penguingao commented Dec 3, 2019

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Dec 3, 2019

Uh oh!

penguingao commented Dec 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kyessenov commented Dec 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

penguingao commented Dec 3, 2019

Uh oh!

kyessenov commented Dec 3, 2019

Uh oh!

penguingao commented Dec 3, 2019

Uh oh!

gargnupur commented Dec 3, 2019

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

penguingao commented Dec 3, 2019

Uh oh!

penguingao commented Dec 4, 2019

Uh oh!

penguingao commented Dec 9, 2019

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

penguingao commented Dec 10, 2019

Uh oh!

mattklein123 commented Dec 11, 2019

Uh oh!

penguingao commented Dec 11, 2019

Uh oh!

penguingao commented Dec 11, 2019

Uh oh!

azure-pipelines bot commented Dec 11, 2019

Uh oh!

penguingao commented Dec 11, 2019

Uh oh!

repokitteh-read-only bot commented Dec 11, 2019

Uh oh!

penguingao commented Dec 3, 2019 •

edited

Loading

penguingao commented Dec 3, 2019 •

edited

Loading

kyessenov commented Dec 3, 2019 •

edited

Loading