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).

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.

Please increment a stat in this case

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!):

1. TLS sniffer: set "tls" or set "raw_buffer".
2. 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:

1. TLS sniffer: set "tls" or do nothing.
2. 2nd sniffer: set "dummy" or do nothing.
3. 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.

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.

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.

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.

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().

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

TIL, thanks!

nit: remove UNREFERENCED_PARAMETER

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.

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.

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 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.

Nit: const

Done.

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.

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.

Done.

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.

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:

1. Automatically inject TLS Inspector when sni_domains or transport_protocol are specified (this is the behavior right now).
2. Reject configuration and emit error when TLS Inspector is needed (i.e. when sni_domains or transport_protocol are specified), but isn't configured.
3. 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.

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.

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.

Re-added.

"the incoming connection"