Introduce PerConnectionState #4454

rshriram · 2018-09-18T18:30:38Z

Signed-off-by: Shriram Rajagopalan shriramr@vmware.com

Description: Introduce per connection state for state sharing across network filters, similar to HTTP filters
Risk Level: low

rshriram · 2018-09-18T18:36:24Z

This is not complete.. But looking for initial feedback to make sure I am on the right track. cc @ggreenway ..

ggreenway

The direction of this looks right.

We need to decide exactly what data we're storing per-connection, and where to store it. But the codes changes for those decisions should be pretty small.

Also, I'd like to make sure other @envoyproxy/maintainers agree with this direction. Thoughts?

ggreenway · 2018-09-18T20:40:09Z

source/common/network/connection_impl.cc

@@ -49,7 +49,8 @@ ConnectionImpl::ConnectionImpl(Event::Dispatcher& dispatcher, ConnectionSocketPt
      socket_(std::move(socket)), write_buffer_(dispatcher.getWatermarkFactory().create(
                                      [this]() -> void { this->onLowWatermark(); },
                                      [this]() -> void { this->onHighWatermark(); })),
-      dispatcher_(dispatcher), id_(next_global_id_++) {
+      dispatcher_(dispatcher), id_(next_global_id_++),
+      per_connection_state_(Envoy::RequestInfo::FilterStateImpl()) {


This stores a reference to a temporary. I don't think you really want a reference here.

Oops! I hacked up something quickly. Will clean all this up.

ggreenway · 2018-09-18T20:40:35Z

source/common/network/connection_impl.h

@@ -123,6 +125,8 @@ class ConnectionImpl : public virtual Connection,
  TransportSocketPtr transport_socket_;
  FilterManagerImpl filter_manager_;
  ConnectionSocketPtr socket_;
+  Envoy::RequestInfo::FilterStateImpl& per_connection_state_{};


This should probably be a unique_ptr or a value, but not a reference.

ggreenway · 2018-09-18T20:42:22Z

include/envoy/network/connection.h

+   * @return Envoy::RequestInfo::FilterState& the state shared between filters operating
+   * on this connection.
+   */
+  virtual Envoy::RequestInfo::FilterState& perConnectionState() PURE;


Design question: should this be a full RequestInfo, instead of just the FilterState?

Also, should this live on the ReadFilterCallbacks, instead of on the Connection?

ReadFilterCallbacks has connection(), I think it is good to have this in Connection.

RequestInfo does not seem to be related to FilterState, eventhough they are both under the same namespace. Doing so, would require exposing a RequestInfo accessor to get the Filterstate and making sure that the RequestInfo constructors will allocate the filterState as well.

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

source/common/tcp_proxy/tcp_proxy.cc

test/common/tcp_proxy/tcp_proxy_test.cc

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

source/common/network/connection_impl.h

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

htuch

I might well be missing the context here, but I had initially thought the idea was to provide a connection-level variant of #3918. Should we reuse most of the data structures in #3918 for this?

Or, is the idea to provide some connection level properties and share them between filters, similar to RequestInfo?

rshriram · 2018-09-20T21:29:24Z

This is the connection level variant of #3918.. All we need is to store the shared filter state and have an accessor for it in the Connection interface (similar to how RequestInfo has an accessor for perRequestState)

mattklein123

Yup this looks like the right direction to me @rshriram and @ggreenway. Excited to see this.

mattklein123 · 2018-09-20T21:30:36Z

include/envoy/network/connection.h

+   * consumed by many other objects.
+   * @return the per-connection state associated with this connection.
+   */
+  virtual Envoy::RequestInfo::FilterState& perConnectionState() PURE;


nit: It doesn't thrill me that this is in the RequestInfo namespace since this has nothing to do with requests at this point. Can we move to some other sensibly named namespace (or an existing one)?

This all came about because I added access logging to TcpProxy, and reused the existing RequestInfo. So the state of things right now is that RequestInfo is used in both http requests and tcp proxy connections. And some of the logging fields are documented as behaving differently in tcp proxy.

I think we can probably leave the code mostly alone, and just come up with a better name than RequestInfo that encompasses both use cases. But names are hard, and I don't have a good idea.

@htuch @mattklein123 How about we leave this PR as-is and file an issue to rename the class/namespace for this stuff?

OK SGTM. I can't immediately think of a better name right now anyway.

ConnectionInfo? I think it makes clearer the L4 nature if we're not multiplexing. Streams/flows to me conjure up L7 H2.

I think we are find a generic name for both L4/L7? if so StreamInfo SGTM.

Oh, I see. Sure, StreamInfo.

We could always subclass/typedef for the respective Connection/Request variants.

Filed #4500

htuch · 2018-09-20T21:36:30Z

Got it; yeah, if we can avoid referencing RequestInfo, all good.

lizan

LGTM, can you merge master to fix mac CI build?

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

rshriram · 2018-09-21T00:34:10Z

merged with master.. mac build is still failing..

source/common/network/connection_impl.h

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

ggreenway · 2018-09-21T19:24:02Z

asan failed; looks unrelated. I kicked it off again.

rshriram · 2018-09-21T19:24:24Z

Thanks.. was just about to ask about that..

*Description*: Use the SNI value as the upstream cluster name. This is similar to the cluster_header feature in HCM. Leverages the perConnectionState to dynamically control the cluster used by tcp_proxy filter. We plan to use this in Istio, where Pilot would manage two kubernetes setups, such that the envoys will have the same set of clusters, but the non-local clusters will have the IP of a Gateway envoy (edge/front envoy). mTLS traffic arriving at the gateway envoy will be routed to the internal (envoy)clusters based on the SNI value Depends on #4454 *Risk Level*: Low *Testing*: Unit tests *Docs Changes*: yes *Release Notes*: yes Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

ggreenway requested changes Sep 18, 2018

View reviewed changes

rshriram force-pushed the tcp_proxy_apis branch from 425b2f7 to 51520a8 Compare September 19, 2018 16:27

Introducing per-connection state that is shared across filters

be1290c

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

rshriram force-pushed the tcp_proxy_apis branch from 51520a8 to be1290c Compare September 19, 2018 16:31

venilnoronha reviewed Sep 19, 2018

View reviewed changes

source/common/tcp_proxy/tcp_proxy.cc Outdated Show resolved Hide resolved

test/common/tcp_proxy/tcp_proxy_test.cc Outdated Show resolved Hide resolved

Shriram Rajagopalan added 2 commits September 19, 2018 19:30

nits

66e8a06

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

format

30d3233

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

lizan reviewed Sep 19, 2018

View reviewed changes

source/common/network/connection_impl.h Outdated Show resolved Hide resolved

Shriram Rajagopalan added 2 commits September 19, 2018 20:26

compile fixes

ea61b5c

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

one last fix

6d90b35

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

htuch assigned lizan Sep 20, 2018

Shriram Rajagopalan added 4 commits September 20, 2018 08:50

missing mock

d95b22f

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

mocks and oncall checks

1d38caa

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

Merge remote-tracking branch 'upstream/master' into tcp_proxy_apis

b42aade

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

undo TCP proxy changes to keep PR focussed

11e7b30

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

rshriram changed the title ~~[WIP] Introduce PerConnectionState and use it to override tcp_proxy.cluster~~ Introduce PerConnectionState Sep 20, 2018

htuch assigned lizan and htuch and unassigned lizan Sep 20, 2018

update mocks

9642ac1

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

htuch reviewed Sep 20, 2018

View reviewed changes

mattklein123 reviewed Sep 20, 2018

View reviewed changes

rshriram mentioned this pull request Sep 20, 2018

Extensions: Network filter to set upstream cluster from SNI #4489

Merged

lizan reviewed Sep 20, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into tcp_proxy_apis

e137c2f

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

ggreenway requested changes Sep 21, 2018

View reviewed changes

source/common/network/connection_impl.h Outdated Show resolved Hide resolved

Shriram Rajagopalan added 2 commits September 21, 2018 16:43

Merge remote-tracking branch 'upstream/master' into tcp_proxy_apis

077c3ff

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

nits

967d33b

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

ggreenway approved these changes Sep 21, 2018

View reviewed changes

ggreenway merged commit 5a3baab into envoyproxy:master Sep 21, 2018

ggreenway mentioned this pull request Sep 21, 2018

Cleanup: rename RequestInfo #4500

Closed

venilnoronha mentioned this pull request Sep 22, 2018

Rename RequestInfo to StreamInfo #4503

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce PerConnectionState #4454

Introduce PerConnectionState #4454

rshriram commented Sep 18, 2018 •

edited

Loading

rshriram commented Sep 18, 2018

ggreenway left a comment

ggreenway Sep 18, 2018

rshriram Sep 19, 2018

ggreenway Sep 18, 2018

ggreenway Sep 18, 2018

lizan Sep 18, 2018

rshriram Sep 19, 2018

htuch left a comment

rshriram commented Sep 20, 2018

mattklein123 left a comment

mattklein123 Sep 20, 2018

ggreenway Sep 20, 2018

ggreenway Sep 20, 2018 •

edited

Loading

lizan Sep 20, 2018

mattklein123 Sep 20, 2018

htuch Sep 21, 2018

lizan Sep 21, 2018

htuch Sep 21, 2018

htuch Sep 21, 2018

ggreenway Sep 21, 2018

htuch commented Sep 20, 2018

lizan left a comment

rshriram commented Sep 21, 2018

ggreenway commented Sep 21, 2018

rshriram commented Sep 21, 2018

Introduce PerConnectionState #4454

Introduce PerConnectionState #4454

Conversation

rshriram commented Sep 18, 2018 • edited Loading

rshriram commented Sep 18, 2018

ggreenway 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

htuch left a comment

Choose a reason for hiding this comment

rshriram commented Sep 20, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggreenway Sep 20, 2018 • edited Loading

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

htuch commented Sep 20, 2018

lizan left a comment

Choose a reason for hiding this comment

rshriram commented Sep 21, 2018

ggreenway commented Sep 21, 2018

rshriram commented Sep 21, 2018

rshriram commented Sep 18, 2018 •

edited

Loading

ggreenway Sep 20, 2018 •

edited

Loading