listener: add support for multiple filter chains. #3217

PiotrSikora · 2018-04-26T00:20:51Z

Risk Level: Medium
Testing: bazel test //test/...
Docs Changes: Added
Release Notes: Added

Fixes #1280, #1843.

Signed-off-by: Piotr Sikora piotrsikora@google.com

PiotrSikora · 2018-04-26T00:24:52Z

This is still WIP (but code complete, and works fine with manual testing), split into 4 commits for easier review.

Sending this early to get feedback on the interface changes (mostly from @mattklein123).
cc @alyssawilk @ggreenway

lizan · 2018-04-26T01:42:34Z

include/envoy/network/filter.h

@@ -10,6 +10,8 @@ namespace Network {

 class Connection;
 class ConnectionSocket;
+class TransportSocket;
+typedef std::unique_ptr<TransportSocket> TransportSocketPtr;


Just include transport socket here? If you use unique_ptr, this is going to force files including this file to include transport_socket as well.

Unfortunately, including transport_socket.h here introduces circular dependency.

Note that if FilterChain would be defined somewhere in include/envoy/server/... (i.e. Server::Configuration:: namespace) instead, then we wouldn't need to declare TransportSocketPtr and NetworkFilterFactoryCb here.

It felt a bit weird defining FilterChain outside of Network:: namespace, so it's here now, but I don't mind moving it into Server::Configuration:: namespace if you think that's better.

I think you can change transport_socket.h a bit to avoid circular dependency by forward declaring Network::Connection there. Since it only use reference to it (which doesn't force includes).

ggreenway

A few comments to start, mostly from a high-level. I still need to spend more time going through the code in detail.

ggreenway · 2018-04-26T16:28:04Z

api/envoy/api/v2/listener/listener.proto

@@ -87,6 +87,9 @@ message FilterChainMatch {
  // listener in determining a filter chain match.
  // [#not-implemented-hide:]
  google.protobuf.UInt32Value destination_port = 8;
+
+  // TODO
+  string transport_protocol = 9;


Should sni_domains (not part of this PR) be renamed to something more generic, so that if we eventually sniff the Host header from plaintext http it can match on that?

Yes! It's been annoying me for a while, but the proto is frozen, and it's not clear whether it's worth adding a new field and deprecating sni_domains just yet.

ggreenway · 2018-04-26T16:39:40Z

source/server/connection_handler_impl.cc

+  if (filter_chain == nullptr) {
+    ENVOY_LOG_TO_LOGGER(parent_.logger_, debug,
+                        "closing connection: no matching filter chain found");
+    socket->close();


Please increment a stat in this case

ggreenway · 2018-04-26T16:45:45Z

source/server/listener_manager_impl.cc

+  }
+
+  // Automatically inject TLS Inspector if it wasn't configured explicitly and it's needed.
+  if (install_tls_inspector) {


This makes the system easier to use, but at the possible cost of flexibility. Envoy has a pluggable filter system, but this is hard-coding that a particular filter is added automatically in this case. Do we care to support someone providing their own filter that provides the same information as tls_inspector? Given that most/all configs are machine-generated, should we just make people manually add the tls_inspector filter to their config? I'm torn on this; what does the group think?

I actually don't like this code too much, and I agree that it should be explicitly configured, so I'm happy to remove it (which is one of the reasons why it was split into separate commit).

The downside of removing it, however, is that transport protocol and SNI-based filter chain matching won't work at all without it, and worse, traffic might be incorrectly routed to a filter chain with empty SNI and/or wrong transport protocol.

What if we log a warning anytime a filterchainmatch specifies a transport protocol, but the detectedTransportProtocol hasn't been set? And then revert the commit that automatically adds in this filter?

With the current implementation of TLS sniffer, we have (and I should have caught this earlier, sorry!):

TLS sniffer: set "tls" or set "raw_buffer".

2nd sniffer: set "dummy" or set "raw_buffer".

However, this clearly doesn't work with more than one sniffer, since 2nd sniffer would overwrite value set by TLS sniffer, therefore we need:

TLS sniffer: set "tls" or do nothing.

2nd sniffer: set "dummy" or do nothing.

listener: set "raw_buffer" if nothing was detected.

See PR #3260.

We could emit warning in 3., but after above change empty detectedTransportProtocol() is a valid state (i.e. no transport protocol was detected).

Also, I'd really like to be able to detect this at config time, and not at runtime, but nothing obvious comes to mind... Perhaps listener filters could indicate which transport protocols they can detect?

(same is true for missing SNI, btw.)

I think my general opinion here is to just inject it, as we do similar things in other places. If in the future someone really wants to not use TLS inspector we can make injection configurable. I would definitely add some type of warning though if we do inject telling people what we did, and make it more obvious if the config then fails to load because TLS inspector was not compiled/linked.

I left the auto-inject, but removed "downstream send first" guard, since it wouldn't allow mixing with filter chains with custom transport sockets.

ggreenway · 2018-04-26T17:02:52Z

source/server/listener_manager_impl.cc

+}
+
+const Network::FilterChainSharedPtr
+ListenerImpl::findFilterChain(const std::string& transport_protocol_name,


The documentation says "The FilterChain with the most specific FilterChainMatch criteria is used on a connection."

I think this is too imprecise to be useful. What exactly does "most specific" mean? Given that every field except sni_domains is marked not-implemented-hide, I think this algorithm is fine. But as soon as we un-hide any of the other fields, we'll have to revisit this, and update the documentation to specify how it works.

FWIW, this is the matching algorithm that I want/need for my use case: hash-table lookup by SNI, as opposed to O(n) walking a list looking for matches.

Yes, we'll need to revisit this sooner rather than later (i.e. I need to add filter chain matching based on the destination IP:port to get ride of the bind_to_port setting), but I think we can iterate on it as we unhide features, instead of blocking this PR on that.

ggreenway · 2018-04-26T17:04:39Z

source/server/listener_manager_impl.h

+                      std::vector<Network::NetworkFilterFactoryCb> filters_factory);
+  static bool isWildcardServerName(const std::string& name);
+
+  std::unordered_map<std::string, std::unordered_map<std::string, Network::FilterChainSharedPtr>>


Please comment on what the two strings are in the map keys. I think it's transport-socket-type and SNI value.

Done.

Also, as I was writing comments and optimizing the code, I've started questioning my previous choice of having 2 separate maps (one for exact server names and one for the wildcard domains), so I merged them together.

ggreenway · 2018-04-30T20:53:47Z

api/envoy/api/v2/listener/listener.proto

@@ -87,6 +87,9 @@ message FilterChainMatch {
  // listener in determining a filter chain match.
  // [#not-implemented-hide:]
  google.protobuf.UInt32Value destination_port = 8;
+
+  // TODO
+  string transport_protocol = 9;


I think this should be an enum, not a string.

I disagree. Enum won't work with custom transport protocols (e.g. Istio's ALTS). cc @lizan

In other places in the config, there is an enum for the builtin types, and then a 2nd string field for custom types. I'm not sure if that was always the desired model, or if that happened via evolution. I don't feel very strongly either way about enum vs string, just trying to aim for consistency. Any maintainers know the "preferred" style for something like this?

I don't have a strong preference here either. We do string for ALPN already so IMO it's not a big deal to do string only.

Ok, I'm fine with leaving it as a string; just document the valid values.

ggreenway · 2018-04-30T20:59:01Z

include/envoy/network/filter.h

   * @return true if filter chain was created successfully. Otherwise
   *   false, e.g. filter chain is empty.
   */
-  virtual bool createNetworkFilterChain(Connection& connection) PURE;
+  virtual bool
+  createNetworkFilterChain(Connection& connection,


This list of params will keep growing as we implement all the filter matches. Should we pass the ConnectionSocket here instead?

I'm fine changing this to ConnectionSocket, but please note that (a) I don't envision more parameters being added here (they would be added to the findFilterChain() function), (b) createNetworkFilterChain() is part of the Network::FilterChainFactory interface that's also implemented by upstreams, and it appears that it's used there (but I cannot find where it's being called from, to be honest, so maybe that's a dead code).

Thoughts? Should I change it here and remove from upstreams or leave it as-is?

Sorry, commented on the wrong line. I meant this comment for findFilterChain().

ggreenway · 2018-04-30T21:11:47Z

source/server/http/admin.cc

+bool AdminImpl::createNetworkFilterChain(
+    Network::Connection& connection,
+    const std::vector<Network::NetworkFilterFactoryCb>& filter_factories) {
+  UNREFERENCED_PARAMETER(filter_factories);


nit: remove UNREFERENCED_PARAMETER, and lave this un-named in the function parameters

TIL, thanks!

ggreenway · 2018-04-30T21:12:22Z

source/server/http/admin.h

+  const Network::FilterChainSharedPtr
+  findFilterChain(const std::string& transport_socket_name,
+                  const std::string& server_name) const override {
+    UNREFERENCED_PARAMETER(transport_socket_name);


nit: remove UNREFERENCED_PARAMETER

ggreenway

I'm pretty happy with the overall direction of this change. I really like how this simplified the ssl_context.

There's still some tests failing, and we'll need to add coverage for the new code.

@envoyproxy/maintainers Anyone else want to comment on the overall direction, structure, architecture of this?

mattklein123 · 2018-04-30T21:40:34Z

@PiotrSikora @ggreenway I will take a look tomorrow.

mattklein123

In general I love the direction of this change. I agree with @ggreenway that it simplifies a lot of stuff and I think makes the overall system a lot more well structured. I didn't really focus on the detailed code that much. I'm happy to take a much more detailed pass through once it's fully ready.

One suggestion, you might want to just do a dedicated PR now with some of the namespace changes. It will make the final PR easier to review.

mattklein123 · 2018-05-01T07:25:53Z

api/envoy/api/v2/listener/listener.proto

@@ -87,6 +87,9 @@ message FilterChainMatch {
  // listener in determining a filter chain match.
  // [#not-implemented-hide:]
  google.protobuf.UInt32Value destination_port = 8;
+
+  // TODO
+  string transport_protocol = 9;


I don't have a strong preference here either. We do string for ALPN already so IMO it's not a big deal to do string only.

mattklein123 · 2018-05-01T07:27:56Z

include/envoy/network/filter.h

+   * TODO
+   */
+  virtual const FilterChainSharedPtr findFilterChain(const std::string& transport_socket_name,
+                                                     const std::string& server_name) const PURE;


What does "server_name" mean here? SNI? Is this related to @ggreenway comment above about potentially allowing sniffing plaintext HTTP host? I think it's probably fine to continue calling this "server_name" but it would be good to add some good docs/comments on example ways this can be populated in the real world.

mattklein123 · 2018-05-01T07:34:52Z

source/server/listener_manager_impl.cc

+  }
+
+  // Automatically inject TLS Inspector if it wasn't configured explicitly and it's needed.
+  if (install_tls_inspector) {


I think my general opinion here is to just inject it, as we do similar things in other places. If in the future someone really wants to not use TLS inspector we can make injection configurable. I would definitely add some type of warning though if we do inject telling people what we did, and make it more obvious if the config then fails to load because TLS inspector was not compiled/linked.

htuch · 2018-05-04T14:40:46Z

api/envoy/api/v2/listener/listener.proto

+  // * "raw_buffer" - default, used when no transport protocol is detected,
+  // * "tls" - set by :ref:`envoy.listener.tls_inspector <config_listener_filters_tls_inspector>`
+  //   when TLS protocol is detected.
+  string transport_protocol = 9;


I think in general, you can imagine listener filters setting arbitrary key/value and wanting to match here. In that sense, {transport_protocol=tls, sni_domains=[foo.com]} is one example. Does it make sense to generalize to a Struct match?

This is a bit more complex than a simple key-value matching, i.e. options covered in this PR, and the one I'm going to do next (destination IP:port), are matching on:

transport protocol,

server names / sni domains,

destination IP:port.

Those are not equal, and we need to filter on destination IP:port first, then on server names (or use catch-all), then on transport protocol (or use catch-all).

...and even that isn't totally obvious, i.e. should {requestedServerName=www.example.com, detectedTransportProtocol=tls} be matched against {sni_domains=[www.example.com],transport_protocol=} or {sni_domains=,transport_protocol=tls}?

I'm in favor of the former, since it better matches "virtual hosts" concept, but I could see the arguments being made for the either of those.

Yeah, I agree that the full logic can't be captured as a simple key/value match. I'm curious though if you think it makes sense to allow listener filters more expressive power than just a string match on protocol here? We could punt this to future work with a TODO if it makes sense.

htuch

At a high-level, I think the direction in this PR is great. I have one API level question and will do a more detailed review next week.

htuch

This is really fantastic, thanks for making this happen @PiotrSikora.

htuch · 2018-05-07T04:05:07Z

source/server/connection_handler_impl.cc

  new_connection->setBufferLimits(config_.perConnectionBufferLimitBytes());
+
+  bool empty_filter_chain = !config_.filterChainFactory().createNetworkFilterChain(


htuch · 2018-05-07T04:06:26Z

source/server/connection_handler_impl.cc

+  // Find matching filter chain.
+  const auto filter_chain = config_.filterChainManager().findFilterChain(socket);
+  if (filter_chain == nullptr) {
+    ENVOY_LOG_TO_LOGGER(parent_.logger_, debug,


Can you fix the build so that we can generate a coverage run?

Sigh, sorry for the delay, I've got stuck trying to figure out why mocking findFilterChain() results in socket being destroyed... Ultimately, I gave up and implemented it in the tests instead.

htuch · 2018-05-07T04:07:17Z

source/server/connection_handler_impl.cc

+  if (filter_chain == nullptr) {
+    ENVOY_LOG_TO_LOGGER(parent_.logger_, debug,
+                        "closing connection: no matching filter chain found");
+    socket->close();


Should we bump a stat or something else that could be monitored? Seems a debug log here is effectively a silent fail in production. I realize this is existing behavior, so a TODO is fine as well.

htuch · 2018-05-07T04:12:43Z

source/server/listener_manager_impl.cc

-    transport_socket_factories_.emplace_back(config_factory.createTransportSocketFactory(
-        name_, sni_domains, skip_context_update, *message, *this));
-    ASSERT(transport_socket_factories_.back() != nullptr);
+    // TODO(PiotrSikora): reject multiple filter chains with the same matching criteria.


Do you think we have adequate documentation on filter chain match precedence? E.g. if we have a CIDR match, do we consider the most specific filter chain the winner, or the first in the list of filter chains?

CIDR match isn't implemented yet, but the idea is that the most specific match wins.

Sure. I think if you can add some comments on what the end goal behavior is in the docs or API it would be great.

htuch · 2018-05-07T04:14:26Z

source/server/listener_manager_impl.cc

-    throw EnvoyException(fmt::format("error adding listener '{}': filter chains with mixed use of "
-                                     "Session Ticket Keys are currently not supported",
-                                     address_->asString()));
+  // Automatically inject TLS Inspector if it wasn't configured explicitly and it's needed.


I wasn't involved in the initial reviews for this feature, but I'm wondering about the tradeoff here between making this explicit as behavior the user should guarantee if they want the feature vs. implicit manipulation of the filter chain?

Well, without TLS inspector, sni_domains and transport_protocol requirements are never going to be matched, so we can either:

Automatically inject TLS Inspector when sni_domains or transport_protocol are specified (this is the behavior right now).

Reject configuration and emit error when TLS Inspector is needed (i.e. when sni_domains or transport_protocol are specified), but isn't configured.

Do nothing and let user figure out why things like SNI don't work.

I've heard the "config is machine generated" argument enough times to not care anymore, so let me know which one you prefer and I'm going to change the code if necessary.

I'm fine with either (1) or (2). Definitely not (3) please. @ggreenway @htuch opinions?

Prefer (2), since it's least surprising. Alternatively, make the TLS inspector impossible to configure by hand and then (1) is fine as well. I would find it surprising as a user if sometimes filters appear by automatic insertion and other times I'm expected or allowed to put them there by hand (or machine generation).

If choosing between (1) and (2) I also have a mild preference for (2) so let's just go with that with a very clear error message on what the user needs to configure.

One thing to note about (2) is that once people start adding custom transport protocols and sniffers, we might end up with config like:

filter_chain_match: - sni_domains: "example.com" - transport_protocol: "bla"

which we would reject without TLS inspector, but the user don't really need it here...

In theory we could limit exception only to cases when transport_protocol: "tls", but then we wouldn't reject configurations without TLS inspector that list only sni_domains as their criteria.

Not sure if we want to deal with this now, though...

I think (2) is better than (1). But to allow custom transport protocols with custom listener filters, we'd need a way for either to filter to say that it provides this connection data, or a way to turn off the error, or something similar.

I think (1) is a bad idea because it's very hard to change in the future, because it could make a previously-working configuration no longer working if we remove it.

With (2), if we change/remove it, we're just being less-strict in validation, so that would be an easier change to make in the future.

(2) done, with a check that should work for custom transport protocols as well.

htuch · 2018-05-07T04:16:12Z

source/server/listener_manager_impl.cc

+    Network::ConnectionSocketPtr& socket) const {
+  const std::string transport_protocol_name(socket->detectedTransportProtocol());
+
+  // Match on exact transport protocol, e.g. "tls".


I think it would be great to document in user facing documentation somewhere what the rules regarding order are here.

Agreed, I'll write something up tomorrow.

htuch

Also, can you add release notes as this is user facing behavior?

mattklein123 · 2018-05-07T14:24:21Z

@PiotrSikora can you ping me after you have gone though @htuch comments? I would like to take a more complete pass also.

PiotrSikora · 2018-05-13T12:01:05Z

This is almost complete (missing docs & release notes), with automatic TLS Inspector injection being something that needs to be decided.

Also, I disabled (#if 0) SNI-based tests in //test/common/ssl:ssl_socket_test, since we got rid of SNI callback in Ssl::Socket. The features there are mostly covered by non-SNI tests and SNI selection is covered by tests in //test/server:listener_manager_impl_test, so perhaps those tests can be completely removed, but didn't fully investigate it yet.

mattklein123 · 2018-05-13T21:41:41Z

Thanks @PiotrSikora will spend some dedicated time on this tomorrow.

PiotrSikora · 2018-05-14T22:31:30Z

I rewrote the old Ssl::Socket SNI-based test, so this is as ready as it will ever be.

The only outstanding question is what should we do about the automatic injection of TLS Inspector, the options are:

Automatically inject TLS Inspector when sni_domains or transport_protocol are specified (this is the behavior right now).
Reject configuration and emit error when TLS Inspector is needed (i.e. when sni_domains or transport_protocol are specified), but isn't configured.
Do nothing and let user figure out why things like SNI don't work.

@ggreenway @htuch @mattklein123 please take a look.

Also, it seems that I broke the DCO bot, because my merge commits are not singed... any way to fix it?

mattklein123

Thanks @PiotrSikora this is totally awesome, really love seeing this come together from an API/code perspective. So many hacks removed. Few small comments. I didn't look at the tests carefully. Hopefully @ggreenway can do that.

mattklein123 · 2018-05-14T22:13:28Z

api/envoy/api/v2/lds.proto

-  // .. attention::
-  //
-  //   In the current version, multiple filter chains are supported **only** so that SNI can be
-  //   configured. See the :ref:`FAQ entry <faq_how_to_setup_sni>` on how to configure SNI for more


Can we still link to the FAQ from somewhere relevant (or multiple places)? This is a super common question and it's still going to confuse people so would like to maximize cross linking.

mattklein123 · 2018-05-14T22:14:20Z

api/envoy/api/v2/listener/listener.proto

+// 2. Transport protocol.
+//
+// For criterias that allow ranges or wildcards, the most specific value in any
+// of the configured filter chains that matches incoming connection is going to


"the incoming connection"

mattklein123 · 2018-05-14T22:14:54Z

api/envoy/api/v2/listener/listener.proto

@@ -87,6 +101,16 @@ message FilterChainMatch {
  // listener in determining a filter chain match.
  // [#not-implemented-hide:]
  google.protobuf.UInt32Value destination_port = 8;
+
+  // If non-empty, a transport protocol to consider when determining a filter chain match.
+  // This value will be compared against transport protocol of a new connection, when it's


"the transport protocol"

mattklein123 · 2018-05-14T22:15:30Z

api/envoy/api/v2/listener/listener.proto

+  // Valid values include:
+  // * "raw_buffer" - default, used when no transport protocol is detected,
+  // * "tls" - set by :ref:`envoy.listener.tls_inspector <config_listener_filters_tls_inspector>`
+  // when TLS protocol is detected.


Please check rendered docs. I think this might not render correctly as a bullet list.

mattklein123 · 2018-05-14T22:17:06Z

include/envoy/network/filter.h

+   * @return const FilterChainSharedPtr filter chain to be used by the new connection,
+   *         nullptr if no matching filter chain was found.
+   */
+  virtual const FilterChainSharedPtr findFilterChain(ConnectionSocketPtr& socket) const PURE;


nit: Could this be ConnectionSocketConstPtr ?

Also, is there a reason to return a shared pointer here? From my read of the code we never actually persistently store the result. Could we just return a const FilterChain* and avoid the shared_ptr inc/dec overhead?

wrt ConnectionSocketConstPtr - do you mean const unique_ptr<ConnectionSocket> (i.e. const ConnectionSocketPtr) or unique_ptr<const ConnectionSocket>?

wrt FilterChainSharedPtr - there is no reason why we couldn't return const FilterChain*, but I thought the whole idea behind unique_ptr / shared_ptr is to avoid passing raw pointers around.

I meant unique_ptr<const ConnectionSocket>

For FilterChainSharedPtr raw pointer is fine in a case like this where we are just referencing constant data that might be nullptr.

Erm, but we cannot safely create ConnectionSocketConstPtr from ConnectionSocketPtr without move... and few lines below, we need ConnectionSocketPtr to pass into createServerConnection to create ConnectionImpl, which means that we would need to move twice.

Per @lizan's suggestion, perhaps we should pass const ConnectionSocket* here instead?

I ended up with const FilterChain* findFilterChain(const ConnectionSocket* socket) const.

mattklein123 · 2018-05-14T22:24:18Z

source/server/http/admin.h

+
+  private:
+    const Network::RawBufferSocketFactory transport_socket_factory_;
+    const std::vector<Network::FilterFactoryCb> empty_network_filter_factory_{};


nit: {} not needed

mattklein123 · 2018-05-14T22:29:55Z

source/server/listener_manager_impl.cc

-  // TODO(htuch): Support multiple filter chains #1280, add constraint to ensure we have at least on
-  // filter chain #1308.
-  ASSERT(config.filter_chains().size() >= 1);
+  if (config.filter_chains().empty()) {


Can we do this via proto constraint? Do we need the check here?

Actually, it looks that it's already enforced by the proto constraint.

I removed the check and test for it.

mattklein123 · 2018-05-14T22:31:33Z

source/server/listener_manager_impl.cc

-    throw EnvoyException(fmt::format("error adding listener '{}': filter chains with mixed use of "
-                                     "Session Ticket Keys are currently not supported",
-                                     address_->asString()));
+  // Automatically inject TLS Inspector if it wasn't configured explicitly and it's needed.


I'm fine with either (1) or (2). Definitely not (3) please. @ggreenway @htuch opinions?

mattklein123 · 2018-05-14T22:36:16Z

@PiotrSikora re: DCO bot, I think it ignores merge commits, so not sure what the problem is. You could do interactive rebase and just fix the offending commits, then force push. Or better, I would just get final sign off from everyone then we can squash/rebace for push and merge.

PiotrSikora · 2018-05-15T04:36:54Z

@mattklein123 I found one "revert" commit without Singed-off-by, so that's probably it. I'll wait for everybody to approve before force-pushing fixed commit.

htuch

Implementation and API docs look really great, a couple of comments. Would be nice to see coverage run when taking a look at tests.

htuch · 2018-05-15T12:57:39Z

api/envoy/api/v2/listener/listener.proto

+// of the configured filter chains that matches the incoming connection is going
+// to be used (e.g. for SNI ``www.example.com`` the most specific match would be
+// ``www.example.com``, then ``*.example.com``, then any filter chain without
+// ``sni_domains`` requirements).


What about when we have CIDR and SNI, what order do we resolve with? In general, it would be helpful to clarify where we want to go in the end state of the API for all the match criteria and precedence.

I've been thinking about this a fair bit. I think the precedence should be configurable. I can think of a few ways that might be useful; I'm not sure yet what the right answer is.

One option is to let the user provide a list for the field precedence order. For instance, [SNI, source-ip, transport protocol]. Another possible matching type is the entire list of matches as an ordered list, with first-match-wins. I'm sure there's other options as well. I would be surprised if we can come up with a one-size-fits-all matching algorithm with this many potential criteria.

I think this can all be solved in a later PR.

Sure, I can see this being a useful thing to add later. So, for now, as long as we have precedence for the fields that are actually implemented, and a TODO for the rest I'd be happy.

As mentioned earlier (I think?), the destination IP would take precedence over SNI, over transport protocol. I didn't consider source IPs.

I could see having filter_chain_match_precedence: ["server_names", "source_ranges", "transport_protocol"] as a way of controlling precedence, but it doesn't seem that this is something required by most of the use cases, so I'm going to let someone that has an use case for it work on this.

As for the "end goal" docs - I'm under the impression that the API and docs document current state, and not future vision, so I don't think the "end goal" should be added here, especially since things change during implementation, and I wouldn't want to promise stuff that won't be eventually implemented.

This isn't quite correct. The user facing API docs should reflect the current state. We do explicit hidden TODOs in various places to signal to developers and those interested in understanding future directions where we want to head, e.g.

envoy/api/envoy/config/accesslog/v2/als.proto

Line 13 in 6ae5612

// [#comment:TODO(mattklein123): Block type in non-tcp proxy cases?]

.

Please do add appropriate TODOs in that style and preferably open an issue to track future work here. That's all that's being asked at this point.

Added TODOs about few upcoming rules and the precedence we discussed here (I'll open separate issue to track it).

htuch · 2018-05-15T13:01:05Z

source/server/connection_handler_impl.cc

+  if (empty_filter_chain) {
+    ENVOY_CONN_LOG_TO_LOGGER(parent_.logger_, debug, "closing connection: no filters",
+                             *new_connection);
+    new_connection->close(Network::ConnectionCloseType::NoFlush);


Also bump stat on this one?

I intentionally didn't add it here, since this can only happen if filter chain is intentionally configured without filters (i.e. blackhole), therefore I don't see a value in stat here.

But feel free to disagree, and I'll add one.

ggreenway · 2018-05-15T16:27:51Z

source/server/listener_manager_impl.cc

@@ -128,11 +128,6 @@ ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, const std::st
      workers_started_(workers_started), hash_(hash),
      local_drain_manager_(parent.factory_.createDrainManager(config.drain_type())),
      config_(config), version_info_(version_info) {
-  if (config.filter_chains().empty()) {


I don't understand why this is removed. Is this handled elsewhere now, or we just want to let the counter for no matching filter chain go up, or is there some different semantic in place now for this case?

This is enforced by proto constraint already, so the check was useless:

repeated listener.FilterChain filter_chains = 3 [(validate.rules).repeated .min_items = 1, (gogoproto.nullable) = false];

mattklein123 · 2018-05-15T16:49:28Z

include/envoy/network/filter.h

+   * @return const FilterChainSharedPtr filter chain to be used by the new connection,
+   *         nullptr if no matching filter chain was found.
+   */
+  virtual const FilterChain* findFilterChain(const ConnectionSocket* socket) const PURE;


nit: Sorry on the param stuff, but can it be const ConnectionSocket& socket? It can't be null right?

mattklein123 · 2018-05-15T16:49:43Z

include/envoy/network/filter.h

+  /**
+   * Find filter chain that's matching metadata from the new connection.
+   * @param socket supplies connection metadata that's going to be used for the filter chain lookup.
+   * @return const FilterChainSharedPtr filter chain to be used by the new connection,


nit: update return param type

mattklein123 · 2018-05-15T16:50:57Z

source/server/connection_handler_impl.cc

-      std::move(socket), config_.transportSocketFactory().createTransportSocket());
+  // Find matching filter chain.
+  const auto filter_chain = config_.filterChainManager().findFilterChain(
+      const_cast<Network::ConnectionSocket*>(socket.get()));


I think you can lose the const_cast here when you switch to const reference passing? (In general const_cast is an anti-pattern)

mattklein123 · 2018-05-15T16:51:41Z

source/server/listener_manager_impl.cc

-    throw EnvoyException(fmt::format("error adding listener '{}': filter chains with mixed use of "
-                                     "Session Ticket Keys are currently not supported",
-                                     address_->asString()));
+  // Automatically inject TLS Inspector if it wasn't configured explicitly and it's needed.


If choosing between (1) and (2) I also have a mild preference for (2) so let's just go with that with a very clear error message on what the user needs to configure.

lizan · 2018-05-15T17:53:05Z

include/envoy/network/filter.h

+public:
+  virtual ~FilterChain() {}
+
+  /**


You're copying two method from TransportSocketFactory, why don't just have an accessor returns a const TransportSocketFactory&, like Cluster does?

To be honest, I don't see much value in exposing implementation details / member classes, since it makes it harder to make changes in the future.

What would be benefit of returning TransportSocketFactory directly here? Calls to those 2 methods are done in separate places, so it wouldn't save any calls.

It is not exposing implementation details / member classes, the TransportSocketFactory is a pure interface that could be returned here. Just to be more consistent with cluster, also avoid copy and paste same interface around.

Fair enough, changed.

lizan · 2018-05-15T17:57:10Z

source/server/listener_manager_impl.cc

+}
+
+bool ListenerImpl::isWildcardServerName(const std::string& name) {
+  return name.size() > 2 && name[0] == '*' && name[1] == '.';


use absl::StartsWith?

PiotrSikora · 2018-05-16T07:10:12Z

FWIW, I have squashed commit ready to push. I'm just waiting to see if @htuch or @lizan have any last comments before wiping review history.

htuch · 2018-05-16T14:42:19Z

@PiotrSikora this is great, thanks, LGTM. If you can just add the TODO and fix DCO, we can ship it.

lizan

LGTM

PiotrSikora · 2018-05-16T22:21:18Z

@ggreenway @mattklein123 @htuch @lizan please take last look at the interface changes in 5256e84. If it's fine with you, I'm going to push squashed commit to fix the DCO bot.

*Risk Level*: Medium *Testing*: bazel test //test/... *Docs Changes*: Added *Release Notes*: Added Fixes envoyproxy#1843. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

ggreenway

LGTM

mattklein123

LGTM. Let's squash/rebase!

PiotrSikora · 2018-05-16T23:22:53Z

Updated description to indicate that it also fixes #1280.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

@htuch

@htuch can't get this PR to open without unicorn, but he approved via slack.

Signed-off-by: Greg Greenway <ggreenway@apple.com>

PiotrSikora · 2018-05-17T17:52:29Z

Thanks for fixing conflicts, @ggreenway! I didn't expect those 2 PRs to be merged in that order.

lizan reviewed Apr 26, 2018

View reviewed changes

PiotrSikora mentioned this pull request Apr 26, 2018

Enable incremental mTLS adoption by using envoy TLS/TCP port multiplexing feature istio/istio#5059

Closed

mattklein123 assigned mattklein123, ggreenway and htuch Apr 26, 2018

ggreenway requested changes Apr 26, 2018

View reviewed changes

PiotrSikora force-pushed the multiple_filter_chains_v2 branch from 48e1ecb to 9a6f285 Compare April 27, 2018 00:49

ggreenway requested changes Apr 30, 2018

View reviewed changes

ggreenway reviewed Apr 30, 2018

View reviewed changes

mattklein123 reviewed May 1, 2018

View reviewed changes

PiotrSikora mentioned this pull request May 1, 2018

cleanup: move filter wrappers into Network and Http namespaces. #3267

Merged

htuch reviewed May 4, 2018

View reviewed changes

htuch suggested changes May 7, 2018

View reviewed changes

andraxylia mentioned this pull request May 7, 2018

SNI sniffing/routing support in Gateway istio/istio#5455

Closed

mattklein123 reviewed May 14, 2018

View reviewed changes

htuch previously requested changes May 15, 2018

View reviewed changes

ggreenway reviewed May 15, 2018

View reviewed changes

mattklein123 reviewed May 15, 2018

View reviewed changes

lizan reviewed May 15, 2018

View reviewed changes

PiotrSikora dismissed ggreenway’s stale review via ee4d5fa May 16, 2018 03:10

lizan reviewed May 16, 2018

View reviewed changes

listener: add support for multiple filter chains.

b24190c

*Risk Level*: Medium *Testing*: bazel test //test/... *Docs Changes*: Added *Release Notes*: Added Fixes envoyproxy#1843. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

ggreenway previously approved these changes May 16, 2018

View reviewed changes

PiotrSikora mentioned this pull request May 16, 2018

Add support for configurable precedence of FilterChainMatch rules. #3411

Closed

mattklein123 previously approved these changes May 16, 2018

View reviewed changes

PiotrSikora dismissed stale reviews from mattklein123 and ggreenway via b24190c May 16, 2018 22:55

PiotrSikora force-pushed the multiple_filter_chains_v2 branch from 5256e84 to b24190c Compare May 16, 2018 22:55

mattklein123 previously approved these changes May 16, 2018

View reviewed changes

PiotrSikora added 2 commits May 16, 2018 21:32

review: remove no longer necessary loop from the test code.

e999693

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: clarify comment about the use of default transport sockets.

104338a

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora dismissed mattklein123’s stale review via 104338a May 17, 2018 05:15

Merge branch 'master' into multiple_filter_chains_v2

50fceeb

ggreenway previously approved these changes May 17, 2018

View reviewed changes

fix one more merge conflict

f87d907

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway dismissed their stale review via f87d907 May 17, 2018 17:37

ggreenway approved these changes May 17, 2018

View reviewed changes

ggreenway merged commit 93780c1 into envoyproxy:master May 17, 2018

This was referenced May 18, 2018

Update Envoy so that SNI is available istio/proxy#1756

Closed

update envoy sha for sni istio/proxy#1757

Closed

PiotrSikora mentioned this pull request May 21, 2018

listener: rename "sni_domains" to "server_names". #3454

Merged

kichristensen mentioned this pull request May 29, 2018

SSL issue with Istio enabled microservice using EgressRules. SSL routines:CONNECT_CR_SRVR_HELLO:wrong version number istio/old_issues_repo#188

Open

PiotrSikora mentioned this pull request May 11, 2019

Provide better error logs for filter chains with overlapping rules #6905

Closed

		new_connection->setBufferLimits(config_.perConnectionBufferLimitBytes());

		bool empty_filter_chain = !config_.filterChainFactory().createNetworkFilterChain(

listener: add support for multiple filter chains. #3217

listener: add support for multiple filter chains. #3217

Conversation

PiotrSikora commented Apr 26, 2018 • edited Loading

PiotrSikora commented Apr 26, 2018

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggreenway left a comment

Choose a reason for hiding this comment

mattklein123 commented Apr 30, 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

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

htuch 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

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

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

mattklein123 commented May 7, 2018

PiotrSikora commented May 13, 2018

mattklein123 commented May 13, 2018

PiotrSikora commented May 14, 2018 • edited Loading

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

PiotrSikora commented Apr 26, 2018 •

edited

Loading

PiotrSikora commented May 14, 2018 •

edited

Loading