Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: Allow specify application protocol in cluster upstream transport socket #8197

Closed
lambdai opened this issue Sep 10, 2019 · 13 comments
Labels
design proposal Needs design doc/proposal before implementation stale stalebot believes this issue/PR has not been touched recently

Comments

@lambdai
Copy link
Contributor

lambdai commented Sep 10, 2019

Istio is endeavor to reduce the burden of config the mesh by specifying service port and protocol. It is great to have a cluster for all upstream protocol.

Actually envoy cluster can be used to create tcp upstream connection, http1 and http2. However the limitation is that the cluster api can only specify one application protocol for those 3 connections.

If envoy provide the api of specifying application protocol for http1/2/tcp, we are getting close to the generic cluster. Also xds server can avoid providing 1 cluster for http1, 1 cluster for http2 and 1 cluster for tcp.

My proposed cluster api

message Http1ProtocolOptions {
  string application_protocol  =  4;         // ADD. See usage below
}
message Http2ProtocolOptions {
  string application_protocol  = 13;        // ADD. see usage bellow
}

a yaml cluster message

  http_protocol_options:
    application_protocol: "http/1.1"          //   for http1 upstream connection
  http2_protocol_options:
    application_protocol: "h2"                   //  for http2 upstream connection
  tls_context:                                            // Below doesn't change. List as an example
    common_tls_context: 
      alpn_protocols:
         - A_TCP_PROTOCOL                   

Alternative attempt made
Envoy has tls_inspector and http_inspector to sniff the protocol of incoming traffic. However, http_inspector can do nothing if it is ssl traffic since http_inspector relies on encrypted stream.

@lambdai
Copy link
Contributor Author

lambdai commented Sep 10, 2019

@envoyproxy/api-shepherds Could you take a look?

@mattklein123
Copy link
Member

@lambdai I don't fully understand what you are after. Can you explain at a high level what you want Envoy to do with this information?

@mattklein123 mattklein123 added the design proposal Needs design doc/proposal before implementation label Sep 10, 2019
@lambdai
Copy link
Contributor Author

lambdai commented Sep 10, 2019

The idea is to allow HCM and tcp_proxy specify the same cluster.
tcp_proxy would obtain a ssl connection with application protocol "TCP" while
HCM would obtain a ssl connection with application protocol "HTTP".

You may ask what protocol the endpoint would server: http or non-http. The answer is we don't know. We use sniffer to choose between HCM filter chain and tcp filter chain and embed the protocol to envoy upstream.


client --envoyA sniffer -- HCM    --  envoyA clusterX --envoyB filterchain -- alpn HTTP   ---  HTTP filter
                        \ _Tcpproxy-- envoyA clusterY _/                                   \_ alpn TCP  ---  TCP   filter

Currently clusterX and clusterY diffs only at alpn_protocols. It would be nice we can merge the two cluster X and Y into one

 http_protocol_options:
    application_protocol: "ALPN_H1"          //   for http1 upstream connection
  http2_protocol_options:
    application_protocol: "ALPN_H2"                   //  for http2 upstream connection
  tls_context:                                            // Below doesn't change. List as an example
    common_tls_context: 
      alpn_protocols:
         - ALPN_TCP   

@mattklein123
Copy link
Member

But what are you actually after? Just setting ALPN? Or something else?

@lambdai
Copy link
Contributor Author

lambdai commented Sep 10, 2019

I would say that's a override/added cluster.http_protocol_options.application_protocol other than the default one in cluster.tls_context.common_tls_context.alpn_protocols. Yes, for the envoy cluster, it add the alpn in the ssl handshake.
Is there anything else I could do?

In the end to end story is, downstream envoy could sniff the plain text protocol, encrypt the traffic between downstream envoy and upstream envoy, and deliver the protocol to upstream envoy.

Comparing to the attempt to exchange more than protocol, this cluster impl reuse the mechanism built in TLS.

@mattklein123
Copy link
Member

I discussed this offline with @lambdai and my general feeling here is we should try to build on the existing original src/dst socket option support in both TCP/HTTP to also allow transport socket options to be altered by filters, and then fed into the conn pool creation code (with appropriate hashing). In this way, ALPN could then be modified by filters and everything would "just work"

I like this option as it would not require any new config and would also be a nice new extension point for other filters.

@lizan WDYT?

@lizan
Copy link
Member

lizan commented Sep 11, 2019

Adding TransportSocketOptions to override ALPN sounds reasonable to me.

@lambdai
Copy link
Contributor Author

lambdai commented Sep 11, 2019

Steps:

  1. embed transport_socket_option into the upstreamSocketOption, eventually affect select or create ConnPoolImpl
  2. ConnPoolImpl should extract the transport_socket_option and pass it to createTransportSocket() if a connection need to be created
  3. Since transport_socket_option matters, this newssl need to utilized the provided option https://github.com/envoyproxy/envoy/blob/master/source/extensions/transport_sockets/tls/context_impl.cc#L448

@lizan
Copy link
Member

lizan commented Sep 11, 2019

@lambdai I think TransportSocketOptions are already affect select or create ConnPool, no?

@lambdai
Copy link
Contributor Author

lambdai commented Sep 12, 2019

@lizan TransportSocketOption has hash_key affecting conn pool

@rshriram
Copy link
Member

More context behind the problem can be found in this comment: istio/istio#17002 (comment)

lizan pushed a commit that referenced this issue Oct 4, 2019
Description: Override ALPN in transport socket options
Risk Level: Low
Testing: Unit test
Docs Changes: N/A
Release Notes: N/A
Fixes #Issue: Part of #8197

Signed-off-by: crazyxy <[email protected]>
danzh2010 pushed a commit to danzh2010/envoy that referenced this issue Oct 4, 2019
Description: Override ALPN in transport socket options
Risk Level: Low
Testing: Unit test
Docs Changes: N/A
Release Notes: N/A
Fixes #Issue: Part of envoyproxy#8197

Signed-off-by: crazyxy <[email protected]>
@stale
Copy link

stale bot commented Oct 12, 2019

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Oct 12, 2019
nandu-vinodan pushed a commit to nandu-vinodan/envoy that referenced this issue Oct 17, 2019
Description: Override ALPN in transport socket options
Risk Level: Low
Testing: Unit test
Docs Changes: N/A
Release Notes: N/A
Fixes #Issue: Part of envoyproxy#8197

Signed-off-by: crazyxy <[email protected]>
@stale
Copy link

stale bot commented Oct 19, 2019

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted". Thank you for your contributions.

@stale stale bot closed this as completed Oct 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design proposal Needs design doc/proposal before implementation stale stalebot believes this issue/PR has not been touched recently
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants