-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
http: making upstream ALPN accesible. #18884
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I totally agree that this "incrementally adding upstreamFoo" approach is annoying and for Envoy Mobile, it's likely to be really annoying. I tend to agree that having access to the StreamInfo from the upstream stream is a better path forward.
cc @RyanTheOptimist I'm also planning on faking "negotiated ALPN" from upstream protocol (so if you negotiate something non-standard it'd not be reported. LMK if you think that's a problem
I think this sounds fine for now, but we should probably wire up the actual TLS ALPN at some point.
source/common/router/router.cc
Outdated
// be the connection ID of the upstream connection that ended up receiving upstream headers. Thus | ||
// reset the upstream connection ID here with the ID of the connection that ultimately was the | ||
// transport for the final upstream request. | ||
// In upstream request hedging scenarios properties set in onPoolReady might not be the same |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit "scenarios" => "scenarios,"
source/common/router/router.cc
Outdated
// transport for the final upstream request. | ||
// In upstream request hedging scenarios properties set in onPoolReady might not be the same | ||
// as the properties set on the final tream, thus we reset fields based on the | ||
// final upstream request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that the request is actually sent multiple times? (Not just that multiple streams were requested?) I think that's what the hedging policy docs say. Neat! Do you know what this means, "The implementation ensures that the same upstream request is not retried twice."? Is that saying that we won't issue the request twice on the same upstream connection?
Ah if we want real ALPN I might as well do that now |
OK, switching this over to real ALPN didn't work becasue of a series of refactors over in #17168 where we made the socket the entity for a bunch of non-address things like the ssl info and the connection id. This may work just fine for HTTP/1 and HTTP/2 but I'm not sure it's the correct API choice for HTTP/3 support. I'm working around it for now with a shim and a TODO but I find the shim pretty ugly and I'm wondering if we need to look at changing the APIs again. |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Arguably we would have had to shim anyway for HTTP/3 since with we may have one socket and many addresses, so maybe it's not actually more problematic? cc @danzh2010 and @RyanTheOptimist for thoughts as well. |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
At a high level these seems reasonable to me. Happy to figure out better APIs also if needed. For posterity the other API changes were done to fix some fairly serious state bugs in which we didn't have state tracked correctly across the connection, stream info, etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than some annoyance at classes outside the scope of this PR, this looks good to me. I think the shim seems totally reasonable to me. We're going to need something along these lines and this seems fine.
/** | ||
* @return std::string the protocol negotiated via ALPN. | ||
**/ | ||
virtual const std::string& alpn() const PURE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm slightly surprised this doesn't already exist!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's nextProtocol in one of the other classes, but it's not accessible where we need it, and I think it makes more sense here.
unsigned int proto_len; | ||
SSL_get0_alpn_selected(ssl(), &proto, &proto_len); | ||
if (proto != nullptr) { | ||
alpn_ = std::string(reinterpret_cast<const char*>(proto), proto_len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOL. C chars are so annoying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry quick question: this code must exist somewhere else, right? Like in TLS inspector or somewhere else? Should we factor it out into some utility function?
/wait-any
@@ -31,7 +31,8 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase, | |||
public: | |||
QuicFilterManagerConnectionImpl(QuicNetworkConnection& connection, | |||
const quic::QuicConnectionId& connection_id, | |||
Event::Dispatcher& dispatcher, uint32_t send_buffer_limit); | |||
Event::Dispatcher& dispatcher, uint32_t send_buffer_limit, | |||
std::shared_ptr<QuicSslConnectionInfo> info); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Would it make sense to use QuicSslConnectionInfo& here, or is the lifetime problematic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be std::shared_ptr&& instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced it'd be safe lifetime-wise so shared pointer (and to dan's point &&) seems safer.
@@ -31,7 +31,8 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase, | |||
public: | |||
QuicFilterManagerConnectionImpl(QuicNetworkConnection& connection, | |||
const quic::QuicConnectionId& connection_id, | |||
Event::Dispatcher& dispatcher, uint32_t send_buffer_limit); | |||
Event::Dispatcher& dispatcher, uint32_t send_buffer_limit, | |||
std::shared_ptr<QuicSslConnectionInfo> info); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be std::shared_ptr&& instead?
|
||
// TODO(alyssawilk, danzh), sort out which of these need to be handled | ||
// locally. | ||
class ConnectionInfoProviderShim : public Network::ConnectionInfoSetter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I wasn't able to figure out why we need this shim. Doesn't network_connection_->connectionSocket()->connectionInfoProvider().setSslConnection(quic_ssl_info_)
work? If not, a comment about this wrapper class will be very appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I thought quic connections shared the socket object along with the kernel socket. I feel much better about this PR if I can get rid of this mess.
@@ -190,6 +190,18 @@ const std::string& ConnectionInfoImplBase::tlsVersion() const { | |||
return cached_tls_version_; | |||
} | |||
|
|||
const std::string& ConnectionInfoImplBase::alpn() const { | |||
if (alpn_.empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we are hard-coding "h3" in server session right now. Would this cause inconsistency between the ALPN returned via Ssl::ConnectionInfo and the one returned via ConnectionSocket::requestedApplicationProtocols()? requestedApplicationProtocols() is only used in server side, so I guess the upstream connection's connection socket doesn't have ALPN set there, either will it be used. But the changes in this PR applies to downstream as well, hence HCM or the filters can use both the new interface and requestedApplicationProtocols() to get the downstream ALPN and they can be different. Maybe overriding this implementation in QuicSslConnectionInfo according to perspective?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aren't we hard-coding h3 as the thing to negotiate? I think returning what boring returns is correct here.
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
my new assert broke a unit test feel free to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, much cleaner! Other than the question about "h3" hardcoding, this LGTM. (I wonder what it would take to stop hard coding "h3" in server session)
It needs a QUICHE change: #18935 |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, redux!
Part of envoyproxy/envoy-mobile#1546
Risk Level: Low
Testing: new integration test
Docs Changes: n/a
Release Notes: n/a