transport sockets: split downstream and upstream transport socket factory#21648
Conversation
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
kyessenov
requested review from
adisuissa,
alyssawilk,
antoniovicente,
ggreenway,
lizan and
mattklein123
as code owners
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
| virtual void initialize() PURE; | ||
|
|
||
| // Network::TransportSocketFactory | ||
| // Network::UpstreamTransportSocketFactory |
There was a problem hiding this comment.
move this to QuicClientTransportSocketFactory?
|
/retest |
|
Retrying Azure Pipelines: |
…_transport_socket
Signed-off-by: Kuat Yessenov <kuat@google.com>
This reverts commit 87fa464. Signed-off-by: Kuat Yessenov <kuat@google.com>
|
/retest |
|
Retrying Azure Pipelines: |
|
@lizan Could you re-approve? Added just one more test to recover coverage. |
Signed-off-by: JP Simard <jp@jpsim.com>
| } | ||
| stream_info->setFilterChainName(filter_chain->name()); | ||
| auto transport_socket = filter_chain->transportSocketFactory().createTransportSocket(nullptr); | ||
| auto transport_socket = filter_chain->transportSocketFactory().createDownstreamTransportSocket(); |
There was a problem hiding this comment.
The intention of this PR is to split createTransportSocket() into client method and server method. But is it intentional that the transport socket option param is removed from the server transport socket creation interface? We don't have config knob for server transport socket options right now, but if we want to do mutual TLS and server transport socket will need to verify cert, would server transport socket also take some transport socket options?
cc @yanavlasov
There was a problem hiding this comment.
Socket options mostly serve as a way to create multiple upstream transport sockets for distinct groups of downstream requests, one per each variation. It's not about configuring the transport socket itself, that's done through the factory.
There was a problem hiding this comment.
TransportSocketOptions::verifySubjectAltNameListOverride() is used in cert verification:
Could this be used by the server to match SAN values in a client cert? Or allowing per-connection override is non-sense? @RyanTheOptimist
There was a problem hiding this comment.
I think the listener filter chain match is used to match SAN values. I can imagine a more flexible transport socket that can adapt during negotiation, but Envoy doesn't have that yet.
…tory (envoyproxy#21648) Addresses the unfortunate pattern of using `createTransportSocket(nullptr)` to indicate a downstream transport socket, by making the two interfaces explicit. In theory, this should permit adding more context to the calls (such as host for upstream). Description: Risk Level: low, refactor Testing: WIP Docs Changes: none Release Notes: none Signed-off-by: Kuat Yessenov <kuat@google.com> Signed-off-by: Amila Senadheera <amila.15@cse.mrt.ac.lk>
3 participants
Signed-off-by: Kuat Yessenov kuat@google.com
Description: Addresses the unfortunate pattern of using
createTransportSocket(nullptr)to indicate a downstream transport socket, by making the two interfaces explicit. In theory, this should permit adding more context to the calls (such as host for upstream).Description:
Risk Level: low, refactor
Testing: WIP
Docs Changes: none
Release Notes: none
/assign @lizan