tcp: proxying TCP over HTTP/2 to upstreams#10162
tcp: proxying TCP over HTTP/2 to upstreams#10162alyssawilk merged 7 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
That seems like the right shape to me. |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
ping! |
zuercher
left a comment
There was a problem hiding this comment.
One little thing, but otherwise looks great.
| auto* filter_chain = listener->add_filter_chains(); | ||
| auto* filter = filter_chain->add_filters(); | ||
| filter->mutable_typed_config()->PackFrom(proxy_config); | ||
| filter->set_name("envoy.tcp_proxy"); |
There was a problem hiding this comment.
We should use the new name: envoy.filters.network.tcp_proxy.
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
@mattklein123 can you take a look, esp for API sign off? |
|
Yup will do today. |
mattklein123
left a comment
There was a problem hiding this comment.
Very cool! I just reviewed the API and skimmed the rest of it. Just a few small API comments. Thank you!
/wait
| // Configuration for tunneling TCP over other transports or application layers. | ||
| // Currently, only HTTP/2 is supported. When other options exist, HTTP/2 will | ||
| // remain the default. |
There was a problem hiding this comment.
Can we future proof this by having a oneof that specifies a required protocol type, with the only type currently supported being something like Http2TunnelConfig or something like that? Then hostname can be embedded in that message. This will future proof adding additional protocols in the future if needed.
There was a problem hiding this comment.
HTTP/1.1 and HTTP/3 would both benefit from a configured as well, so I think it's orthogonal of transports for the forseeable future and the current structure makes sense. Happy to move if you have thoughts of other things we want to tunnel over
There was a problem hiding this comment.
It's up to you, but if you think there will be non-HTTP transports in the future, you might still consider the structure I suggested, and just call the current option HttpTunnelConfig.
There was a problem hiding this comment.
I don't know of any, so I'd prefer to avoid the layer of indirection and risk the deprecation dance if you're OK either way. If you have a preference it's not like it'll take more than 2 minutes though, so just let me know :-)
There was a problem hiding this comment.
Well, what I'm suggesting will avoid deprecation in the future, at the expense of a layer of indirection. I don't feel strongly about it so up to you.
| // limited to 1. | ||
| repeated type.HashPolicy hash_policy = 11 [(validate.rules).repeated = {max_items: 1}]; | ||
|
|
||
| TunnelingConfig tunneling_config = 12; |
There was a problem hiding this comment.
Can you add docs here, mainly saying what the default is if nothing is specified?
mattklein123
left a comment
There was a problem hiding this comment.
LGTM unless you want to change the API structure per comments.
The first half of proxying TCP over HTTP/1, sending the TCP over an HTTP connection.
Risk Level: Low (new code config guarded, minor TCP proxy refactors)
Testing: new integration tests, unit tests
Docs Changes: n/a (will land docs when the other half makes it usable)
Release Notes: n/a
Part of #1630