listener-filter: refactor tls_inspector so its logic could be shared with custom filters

[ronenschafferibm]

Description:
This PR refactors tls_inspector listener filter so its logic could be shared with custom filters.
Usage example can be found here: #4236

This PR is related to issue #4076

Risk Level:
Low

Testing:
since it is refactoring - no new functionality was added, so no new tests are required

Docs Changes:
N/A

Release Notes:
N/A

[cmluciano]

Should we keep this similar debug log in tcp_proxy

[ronenschafferibm]

I think it can be useful for debug to track that the SNI in both filters

[ggreenway]

Given that you're now trying to share code with an out-of-tree filter (in istio), would it make more sense for you to just copy/paste the relevant bits of implementation? Then we don't have to deal with any problems around this code changing and breaking the out-of-tree filter.

[vadimeisenbergibm]

@ggreenway

Given that you're now trying to share code with an out-of-tree filter (in istio), would it make more sense for you to just copy/paste the relevant bits of implementation? Then we don't have to deal with any problems around this code changing and breaking the out-of-tree filter.

Yes, so the out-of-the-tree filter must duplicate the implementation of the Envoy's filter. Let me summarize the story until now:

1. In tlsContext interferes with filterChainMatch and TLS inspector #4076 I claim that Envoy misses one small piece of functionality. For TLS, the SNI can be logged and Envoy's filters can use the SNI. Once mTLS is applied on top of TLS, the original SNI's availability is gone - Envoy's filters can use the SNI of mTLS but not the original SNI. I need this small piece of functionality for a larger use case in Istio described in tlsContext interferes with filterChainMatch and TLS inspector #4076, though I do not think discussing that use case is really relevant here - we can just add the missing piece to Envoy.

2. @ronenschafferibm and I propose to contribute a small filter [WIP] Network-Level SNI reader filter #4236 (refactoring of the existing filter (tls_inspector) plus 70 lines of the filter's .cc file) that the Envoy's users can use in the case of TLS inside mTLS. No code duplication, only refactoring, a small filter and one field in the ReadFilterCallbacks.

3. Our small filter is rejected - I guess it will add a lot of burden on people who support Envoy. We are proposed to submit the refactoring part only, so the users of Envoy will be able to easily write their own filters for the missing piece of functionality in Envoy.

4. The refactoring of the Envoy's tls_inspector is now rejected too. The users of Envoy who need that missing piece of functionality have to duplicate the code of tls_inspector. Additional issue that I see: suppose I have an existing filter which needs to report the original SNI in case of TLS inside mTLS, in addition to other information it processes. Can I add my custom filter and let it extract SNI and store it somewhere, so my existing filter will be able to extract that value? How the filters can pass data between them? if the filters cannot pass data between them, I have to insert the code of TLS inspector into the code of my existing filter, into multiple places, onData(), etc. The whole complication is due to the rejection of the small filter in [WIP] Network-Level SNI reader filter #4236.

Any advice is welcome.

[ggreenway]

Additional issue that I see: suppose I have an existing filter which needs to report the original SNI in case of TLS inside mTLS, in addition to other information it processes. Can I add my custom filter and let it extract SNI and store it somewhere, so my existing filter will be able to extract that value? How the filters can pass data between them?

I think this would be a useful addition to envoy, but it doesn't exist right now. Perhaps we add a way to set and get key-value pairs on a connection or a request. Any other opinions on this from @envoyproxy/maintainers ?

[alyssawilk]

I'd talked to Matt previously about having an extensible per-connection object just as we have a per-stream object - I think it's a great thing for Envoy to have in the long run. I'd prefer we follow the same model we do per HTTP request, where you can add not just strings but arbitrary structured data as well, both for consistency and anticipated feature requirements on our part.

@curiouserrandy

[ggreenway]

I agree. I was envisioning string-->Any (for some implementation of Any)

[curiouserrandy]

It would be somewhere between trivial and easy to adapt the existing (newly added) per-request state structure to also be per-connection. See #4219 and the design doc @ https://docs.google.com/document/d/1GNawM59Pp09WZ34zqJYiO_qmERjuGbOUeAqF1GG6v9Q/edit#heading=h.khcea34u1f8a (see Accessing Data Produced by One Filter from Another section).

[ggreenway]

@curiouserrandy That looks like it should work. Are you interested in doing the work to adapt it to per-connection state?

[curiouserrandy]

@ggreenway : My apologies, but I don't think I have the cycles at the moment. But it really shouldn't be a lot of work, especially for someone who understands the connection architecture better than I do :-}. Basically, you just need to find the structure that's equivalent to RequestInfo for connections and put a member variable/accessor on it for per-connection-state. That plus whatever tests you and your reviewer consider needed and you're done. Glancing at the source, that might be ConnectionImpl or ClientConnectionImpl, but I don't have enough confidence to throw together a trivial PR for it.

[cmluciano]

@ggreenway I can spare a few cycles to work on this. Can you comment on if there is an eqivalent RequestInfo for connections?

I'm unsure if @alyssawilk 's comment here indicates that this does not exist.

[vadimeisenbergibm]

@cmluciano Thanks! I think it is a very important functionality.

[ggreenway]

@cmluciano No, there is no equivalent of RequestInfo for connections. The closest thing is that TcpProxy has a RequestInfo that it uses for access logging.

I think the common place for filters to get state would be ReadFilterCallbacks. It would be better if it was just FilterCallbacks so that it would apply to WriteFilters as well, but that could be fixed later.

So then the question is whether we should attach a RequestInfo to the ReadFilterCallbacks, or just the FilterState. If we add RequestInfo, we could also have network filters fill in dynamicMetadata for use in load balancing by TcpProxy, which would also be useful. But it's a larger change.

We should probably move this discussion to a new issue, since this is larger than the scope of this PR.

[lizan]

Let's move this discussion to #4100, in short I think we agreed there to add PerConnectionState to connection.

[stale]

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[stale]

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!