conn_pool: making TCP upstreams pluggable by alyssawilk · Pull Request #13548 · envoyproxy/envoy

alyssawilk · 2020-10-13T20:51:37Z

Risk Level: Medium (some refactory)
Testing: existing tests pass
Docs Changes: n/a
Release Notes: n/a
Fixes #13185

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only · 2020-10-13T20:51:44Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #13548 was opened by alyssawilk.

see: more, trace.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

antoniovicente

Nice refactor, thanks for this work to reduce differences between various upstream connection factories.

Please look at CI since there's some doc generation and spellcheck errors.

antoniovicente · 2020-10-14T17:32:23Z

source/extensions/upstreams/tcp/generic/BUILD

+    hdrs = [
+        "config.h",
+    ],
+    security_posture = "robust_to_untrusted_downstream",


How does this extension interact with upstreams? Worth a comment explaining the interaction (if there's any) or saying that there should be no upstream interaction?

I think Enovy in general is considered not robust to untrusted upstreams.
@htuch is this documented somewhere?

Given the work @yanavlasov is doing in #12278, this is about to change shortly. Do you think you think there are untrusted upstream issues in this PR?

htuch: I think part of the issue is that there's currently no way to indicate that an extension is robust to untrusted downstreams and agnostic to upstream input.

If it's agnostic then it's robust.

happy to tag this whatever you folks decide - it's no different from the current TCP proxy code robustness which as I understand today is considered robust to downstream and not to upstream.

@yanavlasov do you want to make a call on this one based on upstream hardening progress?

source/extensions/upstreams/tcp/generic/config.cc

antoniovicente · 2020-10-14T17:56:47Z

test/common/tcp_proxy/tcp_proxy_test.cc

 }

+// Test with an explicitly configured upstream.
+TEST_F(TcpProxyTest, DEPRECATED_FEATURE_TEST(ExplicitFactory)) {


I'm a bit confused. GenericConnectionPoolProto is the proto that you added in this PR, but it seems that this test is for a feature that is being deprecated.

antoniovicente · 2020-10-14T18:01:07Z

include/envoy/tcp/upstream.h

+   * @param cm the cluster manager to get the connection pool from
+   * @param hostname the hostname to use if doing connect tunneling, absl::nullopt otherwise.
+   * @param context the load balancing context for this connection.
+   * @param upstream_callbacks the callbacks to provide to the connection if succesfully created.


Spell check error from CI:

@param upstream_callbacks the callbacks to provide to the connection if succesfully created.

@param upstream_callbacks the callbacks to provide to the connection if successfully created.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

lambdai · 2020-10-16T20:25:27Z

source/common/tcp_proxy/tcp_proxy.cc

+    factory = &Envoy::Config::Utility::getAndCheckFactory<GenericConnPoolFactory>(
+        cluster.info()->upstreamConfig().value());
  } else {
-    auto* cluster = cluster_manager_.get(cluster_name);
-    if (!cluster) {
-      return false;
-    }
-    // TODO(snowp): Ideally we should prevent this from being configured, but that's tricky to get
-    // right since whether a cluster is invalid depends on both the tcp_proxy config + cluster
-    // config.
-    if ((cluster->info()->features() & Upstream::ClusterInfo::Features::HTTP2) == 0) {
-      ENVOY_LOG(error, "Attempted to tunnel over HTTP/1.1, this is not supported. Set "
-                       "http2_protocol_options on the cluster.");
-      return false;
-    }
-
-    generic_conn_pool_ = std::make_unique<HttpConnPool>(cluster_name, cluster_manager_, this,
-                                                        config_->tunnelingConfig()->hostname(),
-                                                        *upstream_callbacks_);
-    if (generic_conn_pool_->valid()) {
-      generic_conn_pool_->newStream(this);
-      return true;
-    } else {
-      generic_conn_pool_.reset();
-    }
+    factory = &Envoy::Config::Utility::getAndCheckFactoryByName<GenericConnPoolFactory>(
+        "envoy.filters.connection_pools.tcp.generic");


getAndCheckFactoryByName may throw exception on invalid type_config

Is such "non-exist" factory config validated by XDS already?

Nice catch. This does look like data plane code with no obvious try/catch blocks in sight. The most concerning one is the getAndCheckFactory on line 447 above which is a function of config.

I think it's safe to lookup by name providing you can guarantee that the filter will be there; it will throw otherwise. The config thing becomes an issue only when you are actually loading config.

I wonder if we can make this more robust going forward by having the factory functions ASSERT that they are in a control plane context, using some main thread singleton. There's probably static annotations etc. we could use if we wanted to get fancy, but I can see how to build this dynamic check.

Also, Quic has this same pattern.

Calling the factory lookup functions in a non control plane setting is not legal. I urge we invest in adding the appropriate ASSERTs.

refactored to make it non-fatal and added a test. PTAL!

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

antoniovicente

Just some remaining nits. Looks good otherwise.

antoniovicente · 2020-10-22T15:54:41Z

source/common/config/utility.h

  }

+  /**
+   * Get a Factory from the registry with a particular name or return nullptr.


nit: worth adding a @return statement to the comment?

antoniovicente · 2020-10-22T15:55:04Z

source/common/tcp_proxy/tcp_proxy.cc

 }

 bool Filter::maybeTunnel(Upstream::ThreadLocalCluster& cluster, const std::string& cluster_name) {
+  std::cerr << " HERE\n";


nit: Remove debug logging.

antoniovicente · 2020-10-22T19:56:40Z

include/envoy/tcp/upstream.h

+   * @param info supplies the stream info object associated with the upstream connection.
+   * @param upstream supplies the generic upstream for the stream.
+   * @param host supplies the description of the host that will carry the request.
+   * @param upstream_local_address supplies the local address of the upstream connection.


nit: documentation and argument names disagree: upstream_local_address vs local_address

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2020-10-26T13:19:59Z

@snowp or @mattklein123 up for second pass?

snowp

Thanks this looks good for the most part, just a few comments

snowp · 2020-10-26T18:20:54Z

include/envoy/tcp/upstream.h

+   *
+   * @param callbacks callbacks to communicate stream failure or creation on.
+   */
+  virtual void newStream(GenericConnectionPoolCallbacks* callbacks) PURE;


Should this take a ref instead of a pointer?

source/common/config/utility.h

snowp · 2020-10-26T18:33:26Z

include/envoy/tcp/upstream.h

+   */
+  virtual GenericConnPoolPtr
+  createGenericConnPool(const std::string& cluster_name, Upstream::ClusterManager& cm,
+                        absl::optional<std::string> hostname,


snowp · 2020-10-26T18:36:16Z

source/common/tcp_proxy/tcp_proxy.cc

+    return false;
  }

+  absl::optional<std::string> hostname =


This does a string copy, which means two string copies here since hostname is passed by value to createGenericConnPool. Might not matter in this context

A higher level comment: should we do the factory lookup + partially populate the arguments during filter factory creation? That way we avoid factory lookup and string copies etc in the data path

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2020-11-02T15:49:11Z

@snowp I think I finally got CI sorted, PTAL!

snowp

Makes sense to me! Just a few small comments

snowp · 2020-11-02T22:45:36Z

include/envoy/tcp/upstream.h

+   *
+   * @param disable true if the stream should be read disabled, false otherwise.
+   * @return returns true if the disable is performed, false otherwise
+             (e.g. if the connection is closed)


nit: missing *

snowp · 2020-11-02T22:48:29Z

include/envoy/tcp/upstream.h

+   * @param config the tunneling config, if doing connect tunneling.
+   * @param context the load balancing context for this connection.
+   * @param upstream_callbacks the callbacks to provide to the connection if successfully created.
+   * @return may be null


when would this be null?

snowp · 2020-11-02T22:50:09Z

source/common/config/utility.h


+  /**
+   * Get a Factory from the registry with a particular name or return nullptr.
+   * @param name string identifier for the particular implementation. Note: this is a proto string


Seems like this is just a regular string?

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

snowp

LGTM!

alyssawilk · 2020-11-04T17:00:08Z

@htuch @mattklein123 @lizan I can has API stamp?

mattklein123

API LGTM

alyssawilk added 2 commits October 13, 2020 15:10

WIP

b028ae2

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

pluggable

5a733ca

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk assigned antoniovicente Oct 13, 2020

repokitteh-read-only bot added the api label Oct 13, 2020

format

dee1343

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk mentioned this pull request Oct 13, 2020

Tunnel TCP over HTTP/2 POST #13185

Closed

ci

e60fb54

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

antoniovicente reviewed Oct 14, 2020

View reviewed changes

comments + CI

a8ef982

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

lambdai reviewed Oct 16, 2020

View reviewed changes

alyssawilk added 3 commits October 20, 2020 16:45

factory fix

6c85bdc

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Merge branch 'master' into tcp_api

831f906

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

tidy

da3a175

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

antoniovicente reviewed Oct 22, 2020

View reviewed changes

antoniovicente added the waiting label Oct 23, 2020

comments

9b965fa

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting label Oct 26, 2020

snowp self-assigned this Oct 26, 2020

antoniovicente previously approved these changes Oct 26, 2020

View reviewed changes

snowp suggested changes Oct 26, 2020

View reviewed changes

comments

310a6d7

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk dismissed antoniovicente’s stale review via 310a6d7 October 26, 2020 20:15

alyssawilk added 2 commits October 28, 2020 13:01

CI?

5bd5fa0

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

FFS

a4a2580

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk mentioned this pull request Oct 29, 2020

Support dynamic host rewrite and Proxy-Authorization Header injection via SDS in TCPProxy tunneling #13809

Closed

test fix

c6eb026

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

snowp suggested changes Nov 3, 2020

View reviewed changes

comments

beaddec

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

snowp approved these changes Nov 4, 2020

View reviewed changes

mattklein123 self-assigned this Nov 5, 2020

mattklein123 approved these changes Nov 5, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Nov 5, 2020

alyssawilk merged commit 6a044a3 into envoyproxy:master Nov 5, 2020

wlhee mentioned this pull request Dec 1, 2020

Support TCP Proxy over HTTP/2 POST (Encap and Decap) #14234

Closed

alyssawilk deleted the tcp_api branch June 10, 2021 13:43

Conversation

alyssawilk commented Oct 13, 2020

Uh oh!

repokitteh-read-only bot commented Oct 13, 2020

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

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!

antoniovicente 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 Oct 26, 2020

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

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!

alyssawilk commented Nov 2, 2020

Uh oh!

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

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Nov 4, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels