transport socket: refactor to capture the filter state by kyessenov · Pull Request #19809 · envoyproxy/envoy

kyessenov · 2022-02-03T21:14:14Z

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

Commit Message: Capture the filter state in the transport socket options to be exposed to the internal listener transport socket. The goal is to enable propagation the filter state from the downstream request/connection to the upstream internal listener. The assumption is that the propagation is consistent with the hash key: if multiple downstream HTTP requests share the internal upstream connection, then their filter state objects are "equivalent".

Risk Level: low, refactor (shared pointer does not seem to prolong the lifetime)
Testing: unit
Docs Changes: none
Release Notes: none
Issue: #19274

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

kyessenov · 2022-02-03T21:25:32Z

@alyssawilk This is a cleaner attempt to wire the filter state across internal loopback. The plan is to add a "hash" method to filter state objects, and then in the internal upstream transport socket, hash all the exported objects.

alyssawilk

sorry for the lag - somehow it got lost in my inbox!
/wait

alyssawilk · 2022-02-08T13:55:33Z

envoy/network/transport_socket.h

  virtual absl::optional<Network::ProxyProtocolData> proxyProtocolOptions() const PURE;

+  /**
+   * @return filter state from the downstream request or connection.


What in the implementation makes this downstream-specific?
and aren't transport sockets per-connection?

It specifies which filter state to capture when creating transport socket options. That determines whether downstream requests/connections map to the same upstream connection once we use the filter state in hashKey. This is basically a generalization of ProxyProtocolData but for arbitrary filter state objects.

alyssawilk · 2022-02-08T14:00:43Z

source/common/network/transport_socket_options_impl.cc

  std::vector<std::string> alpn_fallback;
  absl::optional<Network::ProxyProtocolData> proxy_protocol_options;

-  bool needs_transport_socket_options = false;


I could easily be misremembering but I thought previously this being null vs non-null meant that connections would or would not be hashed together in connection pools. have you made sure it's safe to always return this?
cc @lizan who wrote this code

Good point. I assumed this is just some optimization but if nullptr is actually special, then I'll have to track its usage throughout the code base.

I looked over and I think there is no risk in returning an empty options struct vs nullptr. This is because the common hash iterates over the optionals and vectors to add to hash, and if they are empty, then it is effectively a no-op.

PS. I added some additional refactor to make it more clear the goal of this PR. I want to swap out hashKey definition site to the transport socket factory so that the factory can change the hash (rather that the options checking whether a factory is special).

source/common/tcp_proxy/tcp_proxy.cc

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

alyssawilk

LGTM (with minor test question) but I'd definitely appreciate a second pass, throwing over to Greg

alyssawilk · 2022-02-09T19:43:43Z

test/common/network/raw_buffer_socket_test.cc

 TEST(RawBufferSocketFactory, RawBufferSocketFactory) {
  TransportSocketFactoryPtr factory = Envoy::Network::Test::createRawBufferSocketFactory();
-  EXPECT_FALSE(factory->usesProxyProtocolOptions());
+  EXPECT_FALSE(factory->implementsSecureTransport());


shouldn't this be swapped for testing the hash key method?

I can add that, too. I added something to recover coverage since the entire file has just one test.

kyessenov · 2022-02-09T19:57:41Z

source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc

+  // Proxy protocol options should only be included in the hash if the upstream
+  // socket intends to use them.
+  if (options) {
+    const auto& proxy_protocol_options = options->proxyProtocolOptions();


Minor comment: As I was writing this, I realized that we don't have to capture proxy data by copy in the options for proxy protocol. We could just look it up in the filter state here.

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

alyssawilk · 2022-02-15T15:53:33Z

@ggreenway ping?

ggreenway · 2022-02-15T21:41:41Z

Given that this is for use between custom upstream/downstream transport sockets for the internal listener, would it make more sense to send the relevant data in-band, eg somehow stick it in the payload that the upstream writes to the internal-listener connection? That would prevent lots of implementation details from leaking out and affecting the rest of the system. If we validate that the special upstream transport-socket is connecting to an internal listener with the special downstream transport-socket, it could even be a pointer still (not a copy/serialization of the data).

/wait-any

kyessenov · 2022-02-16T01:29:03Z

I understand the need to keep it more like proxy protocol handling. I think the cost of passing data on the wire is not worth it. Serialization seems hard to justify since this all happens within the same process. Passing a raw pointer from a shared pointer that is filter state seems problematic to me since we buffer data in the internal listener.

Besides the above, this PR doesn't actually make use of the stored filter state (that's #19435). Whether we pass it on the wire or via a side channel, we have to expose the filter state in the transport socket context. This PR just makes it possible to reference other filter state objects besides the proxy protocol one and some other well-known ones.

kyessenov · 2022-02-18T15:49:20Z

@ggreenway Thoughts?

ggreenway

This makes sense to me. I like that proxy-protocol isn't special-cased in the interface anymore. Access to this data makes sense regardless of how it is passed through the internal-listener implementation.

/wait

ggreenway · 2022-02-21T19:50:27Z

source/extensions/transport_sockets/common/passthrough.cc


+void PassthroughFactory::hashKey(std::vector<uint8_t>& key,
+                                 Network::TransportSocketOptionsConstSharedPtr options) const {
+  return Network::TransportSocketOptionsUtility::commonHashKey(key, options);


Should this implementation go into a base class, since it is used in almost every transport socket?

Good idea. Thanks for the feedback.

Implemented a common class.

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

kyessenov · 2022-02-23T17:13:07Z

/retest

repokitteh-read-only · 2022-02-23T17:13:13Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

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

see: more, trace.

ggreenway

LGTM

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

transport socket: capture filter state

feca4fc

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

kyessenov force-pushed the socket_filter_state branch from c7584d6 to feca4fc Compare February 3, 2022 21:15

snowp assigned alyssawilk Feb 4, 2022

alyssawilk reviewed Feb 8, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 8, 2022

kyessenov added 2 commits February 8, 2022 10:10

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

120dd52

refactor interface

1966cdb

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

kyessenov requested review from adisuissa, antoniovicente, ggreenway and lizan as code owners February 8, 2022 20:45

repokitteh-read-only bot removed the waiting label Feb 8, 2022

coverage

43179d3

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

alyssawilk previously approved these changes Feb 9, 2022

View reviewed changes

alyssawilk assigned ggreenway Feb 9, 2022

kyessenov commented Feb 9, 2022

View reviewed changes

add test

bfd25ab

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

kyessenov dismissed alyssawilk’s stale review via bfd25ab February 9, 2022 20:58

kyessenov added 2 commits February 10, 2022 10:42

merge fix

a242a23

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

format

88ff73a

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

repokitteh-read-only bot added the waiting:any label Feb 15, 2022

repokitteh-read-only bot removed the waiting:any label Feb 16, 2022

ggreenway reviewed Feb 21, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 21, 2022

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

5108c78

review

82ba840

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

repokitteh-read-only bot removed the waiting label Feb 23, 2022

kyessenov added 2 commits February 22, 2022 21:04

fix build

8aec921

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

fix test

8f849ae

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

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

7121ce6

ggreenway approved these changes Feb 23, 2022

View reviewed changes

ggreenway merged commit 3e6aee6 into envoyproxy:main Feb 23, 2022

kyessenov mentioned this pull request Jun 14, 2022

filter state: add a mark for sharing with the upstream #21700

Merged

lambdai mentioned this pull request Aug 16, 2022

[feature request] mullti-certs tls upsteram transport socket #22726

Closed

Conversation

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

Uh oh!

Uh oh!

kyessenov commented Feb 3, 2022

Uh oh!

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

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!

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

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Feb 15, 2022

Uh oh!

ggreenway commented Feb 15, 2022

Uh oh!

kyessenov commented Feb 16, 2022

Uh oh!

kyessenov commented Feb 18, 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!

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Feb 23, 2022

Uh oh!

repokitteh-read-only bot commented Feb 23, 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 Feb 3, 2022 •

edited

Loading