Extensions: Network filter to set upstream cluster from SNI by rshriram · Pull Request #4489 · envoyproxy/envoy

rshriram · 2018-09-20T21:36:10Z

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

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

rshriram · 2018-09-20T21:36:31Z

cc @htuch this should provide context for #4454

rshriram · 2018-09-20T21:37:09Z

fyi @louiscryan @PiotrSikora @lizan

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

ggreenway · 2018-09-22T00:01:16Z

+/**
+ * Per-connection TCP Proxy Filter configuration.
+ */
+class PerConnectionTcpProxyConfig : public Envoy::RequestInfo::FilterState::Object {


I'd name this PerConnectionCluster. It's already in namespace TcpProxy, and we don't want this to grow to include more stuff. If there's more stuff to set on TcpProxy it should be set as separate per-connection values, so that it doesn't all have to be set by the same filter.

But can't filters retrieve and modify the same object? Having the settings in the same object does not preclude multiple filters from using it does it?

It makes the interface a little stranger. Instead of just setting a thing, you have to test if it exists first, and if it does take one path, else create a new one and insert it.

Fair point, but this is based on the assumption that multiple filters will be setting independent things. Now, even in that scenario, given that any other filter could have set the variable before mine, to prevent an exception, I would have to do the check-if-it-exists and then set the thing. [The interface throws an error if I try to set an object that already exists].

Make sense? the check before update will be part of the normal pattern in a scenario where there could be multiple filters. Given that, I feel having a standard check if the umbrella object exists and then do setters/getters is cleaner than check if my object exists and then update..

I don't have strong feelings about this as I don't have data to support either side. So, if you feel like per conn cluster is better, I will happily change it.. We can always consolidate if need be

ggreenway · 2018-09-22T00:05:08Z

Also, we should clearly document this behavior (overriding the cluster), but I'm not sure where. Possibilities:

https://www.envoyproxy.io/docs/envoy/latest/configuration/network_filters/tcp_proxy_filter#config-network-filters-tcp-proxy
https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/tcp_proxy#arch-overview-tcp-proxy

rshriram · 2018-09-22T01:58:37Z

I added a doc file describing the filter and it’s purpose. It should be in the PR. Does that suffice ?

rshriram · 2018-09-24T19:25:58Z

gentle ping :)

lizan

Can you also figure out who will be the sponsor per extension policy and add a line to CODEOWNERS?

lizan · 2018-09-24T19:48:45Z

+/**
+ * Per-connection TCP Proxy Filter configuration.
+ */
+class PerConnectionTcpProxyConfig : public Envoy::RequestInfo::FilterState::Object {


nit: No need Envoy:: prefix. +1 to name @ggreenway suggested.

lizan · 2018-09-24T19:49:27Z

+  static const std::string CLUSTER_KEY;
+
+private:
+  std::string cluster_;


This is set by the filter isn't it? And it could be changed by other filters before hitting tcp proxy.

The design of the perConnectionState() requires that only a single filter set the given field, and accessors only get a const reference. So no later filter may modify the value.

Oh stupid me. That explains why you wanted PerConnectionCluster. Does it make sense to change the design to return non-const reference to the ConnectionState object ?

I think that would be outside the scope of this PR.

lizan · 2018-09-24T19:49:47Z

+}
+
+ProtobufTypes::MessagePtr SniClusterNetworkFilterConfigFactory::createEmptyConfigProto() {
+  return ProtobufTypes::MessagePtr{new Envoy::ProtobufWkt::Empty()};


nit: no need Envoy::.

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

lizan · 2018-09-25T18:09:08Z


 const std::string& Config::getRegularRouteFromEntries(Network::Connection& connection) {
+  // First check if the per-connection state to see if we need to route to a pre-selected cluster
+  if (connection.perConnectionState().hasData<PerConnectionCluster>(PerConnectionCluster::KEY)) {


not directly with this PR, but seems this pattern (hasData, then getData) would be common, is there any way to avoid double lookup?

I had this thought too, but was thinking we should fix it in a separate PR. Probably just make the accessor return a const pointer instead of const ref, so that it can be nullptr when it isn't found. And maybe throw if the item is found, but has an incompatible type (I think that always indicates a programmer error).

lizan · 2018-09-25T18:11:18Z

+SniClusterNetworkFilterConfigFactory::createFilterFactory(const Json::Object&,
+                                                          Server::Configuration::FactoryContext&) {
+  // Only used in v1 filters.
+  NOT_IMPLEMENTED_GCOVR_EXCL_LINE;


Since v1 will be completely removed soon, should we mark this method non-PURE and in NamedNetworkFilterConfigFactory and make it NOT_IMPLEMENTED?

I was waiting for the 1.8 cycle. after that, we could remove these methods entirely from everywhere.

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

ggreenway

LGTM. @lizan any other comments/feedback?

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

Shriram Rajagopalan added 6 commits September 19, 2018 21:47

Cluster name from SNI

dcc38b8

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

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

683285d

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

tests and more

f10f6b4

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

fixes

c783a18

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

tests and fixes

9bba070

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

streamline

b430ae7

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

htuch requested a review from ggreenway September 20, 2018 21:37

htuch assigned ggreenway Sep 20, 2018

Shriram Rajagopalan added 6 commits September 21, 2018 00:35

never use auto

ac73c43

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

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

e9680b3

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

docs, release notes

7a6a37c

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

two files

03dca75

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

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

0408373

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

docs and formatting

2e30879

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

ggreenway requested changes Sep 22, 2018

View reviewed changes

lizan reviewed Sep 24, 2018

View reviewed changes

Shriram Rajagopalan added 5 commits September 25, 2018 02:27

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

cb8cd6e

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

formatting and renaming

f63b194

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

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

377c741

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

codeowners

f99d175

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

assorted fixes

8dd96ef

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

lizan reviewed Sep 25, 2018

View reviewed changes

lizan self-assigned this Sep 25, 2018

Shriram Rajagopalan added 2 commits September 25, 2018 18:21

nits

e49e40d

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

std::unique

e2254c0

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

ggreenway previously approved these changes Sep 25, 2018

View reviewed changes

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

39ba79a

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

ggreenway mentioned this pull request Sep 25, 2018

Generic attribute bag shared by all filters #4528

Closed

rshriram dismissed ggreenway’s stale review via 39ba79a September 25, 2018 22:02

ggreenway approved these changes Sep 25, 2018

View reviewed changes

lizan approved these changes Sep 26, 2018

View reviewed changes

lizan merged commit a637506 into envoyproxy:master Sep 26, 2018

lizan mentioned this pull request Oct 15, 2018

Extensions: Network filter to forward SNI from the downstream TLS connection to the upstream TLS connection #4334

Closed

Conversation

rshriram commented Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rshriram commented Sep 20, 2018

Uh oh!

rshriram commented Sep 20, 2018

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!

ggreenway commented Sep 22, 2018

Uh oh!

rshriram commented Sep 22, 2018

Uh oh!

rshriram commented Sep 24, 2018

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!

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!

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

rshriram commented Sep 20, 2018 •

edited

Loading