http: making upstream ALPN accesible. #18884

alyssawilk · 2021-11-03T16:59:32Z

Risk Level: Low
Testing: new integration test
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

RyanTheOptimist

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.

RyanTheOptimist · 2021-11-03T17:06:21Z

source/common/router/router.cc

-  // 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


nit "scenarios" => "scenarios,"

RyanTheOptimist · 2021-11-03T17:09:01Z

source/common/router/router.cc

-  // 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.


Wow, TIL: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/http/http_routing#arch-overview-http-routing-hedging

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?

alyssawilk · 2021-11-03T17:36:33Z

Ah if we want real ALPN I might as well do that now
/wait

alyssawilk · 2021-11-04T13:56:16Z

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.
cc @ggreenway

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2021-11-04T14:00:10Z

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.
Dan as TODO'd I think we need to look at all these and make sure they work as intended.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123 · 2021-11-04T17:07:10Z

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.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

RyanTheOptimist

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.

RyanTheOptimist · 2021-11-04T20:40:27Z

envoy/ssl/connection.h

+  /**
+   * @return std::string the protocol negotiated via ALPN.
+   **/
+  virtual const std::string& alpn() const PURE;


I'm slightly surprised this doesn't already exist!

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.

RyanTheOptimist · 2021-11-04T20:43:19Z

source/extensions/transport_sockets/tls/connection_info_impl_base.cc

+    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);


LOL. C chars are so annoying.

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

test/integration/filters/stream_info_to_headers_filter.cc

test/integration/multiplexed_upstream_integration_test.cc

RyanTheOptimist · 2021-11-04T20:51:17Z

source/common/quic/quic_filter_manager_connection_impl.h

@@ -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);


nit: Would it make sense to use QuicSslConnectionInfo& here, or is the lifetime problematic?

Shouldn't it be std::shared_ptr&& instead?

I'm not convinced it'd be safe lifetime-wise so shared pointer (and to dan's point &&) seems safer.

danzh2010 · 2021-11-05T16:19:16Z

source/common/quic/quic_filter_manager_connection_impl.h

@@ -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);


Shouldn't it be std::shared_ptr&& instead?

danzh2010 · 2021-11-05T16:27:52Z

source/common/quic/quic_filter_manager_connection_impl.h

+
+  // TODO(alyssawilk, danzh), sort out which of these need to be handled
+  // locally.
+  class ConnectionInfoProviderShim : public Network::ConnectionInfoSetter {


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.

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.

test/integration/multiplexed_upstream_integration_test.cc

danzh2010 · 2021-11-05T16:57:12Z

source/extensions/transport_sockets/tls/connection_info_impl_base.cc

@@ -190,6 +190,18 @@ const std::string& ConnectionInfoImplBase::tlsVersion() const {
  return cached_tls_version_;
 }

+const std::string& ConnectionInfoImplBase::alpn() const {
+  if (alpn_.empty()) {


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?

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>

alyssawilk · 2021-11-08T18:43:34Z

my new assert broke a unit test feel free to
/wait
for the fix

RyanTheOptimist

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)

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

danzh2010 · 2021-11-08T20:41:39Z

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>

RyanTheOptimist

LGTM, redux!

http: adding upstream protocol to stream info

8111956

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk assigned mattklein123 Nov 3, 2021

RyanTheOptimist previously approved these changes Nov 3, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 3, 2021

alyssawilk dismissed RyanTheOptimist’s stale review via c1619e2 November 4, 2021 13:53

alyssawilk requested review from asraa, ggreenway and lizan as code owners November 4, 2021 13:53

repokitteh-read-only bot removed the waiting label Nov 4, 2021

alyssawilk changed the title ~~http: adding upstream protocol to stream info~~ http: making upstream ALPN accesible. Nov 4, 2021

http: making sure upstream ALPN is consistently accessible.

2765421

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the alpn branch from c1619e2 to 2765421 Compare November 4, 2021 13:58

Merge branch 'main' into alpn

26a4c95

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

build

d3d092b

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

RyanTheOptimist previously approved these changes Nov 4, 2021

View reviewed changes

alyssawilk added the waiting label Nov 4, 2021

danzh2010 reviewed Nov 5, 2021

View reviewed changes

much cleaner!

296facf

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk dismissed RyanTheOptimist’s stale review via 296facf November 8, 2021 16:24

repokitteh-read-only bot removed the waiting label Nov 8, 2021

repokitteh-read-only bot added the waiting label Nov 8, 2021

RyanTheOptimist reviewed Nov 8, 2021

View reviewed changes

test fix

1cf8942

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting label Nov 8, 2021

Merge branch 'main' into alpn

2bb138d

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot added the waiting:any label Nov 9, 2021

remove redundant code

60895d0

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting:any label Nov 9, 2021

RyanTheOptimist approved these changes Nov 10, 2021

View reviewed changes

mattklein123 approved these changes Nov 10, 2021

View reviewed changes

alyssawilk merged commit a1487bf into envoyproxy:main Nov 10, 2021

yanavlasov mentioned this pull request Nov 23, 2021

TLS: add plumbing for getting SNI from the TLS socket #18205

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: making upstream ALPN accesible. #18884

http: making upstream ALPN accesible. #18884

alyssawilk commented Nov 3, 2021 •

edited

Loading

RyanTheOptimist left a comment

RyanTheOptimist Nov 3, 2021

RyanTheOptimist Nov 3, 2021

alyssawilk commented Nov 3, 2021

alyssawilk commented Nov 4, 2021

alyssawilk commented Nov 4, 2021

mattklein123 commented Nov 4, 2021

RyanTheOptimist left a comment

RyanTheOptimist Nov 4, 2021

alyssawilk Nov 8, 2021

RyanTheOptimist Nov 4, 2021

mattklein123 Nov 9, 2021

RyanTheOptimist Nov 4, 2021

danzh2010 Nov 5, 2021

alyssawilk Nov 8, 2021

danzh2010 Nov 5, 2021

danzh2010 Nov 5, 2021

alyssawilk Nov 8, 2021

danzh2010 Nov 5, 2021

alyssawilk Nov 8, 2021

alyssawilk commented Nov 8, 2021

RyanTheOptimist left a comment

danzh2010 commented Nov 8, 2021

RyanTheOptimist left a comment

http: making upstream ALPN accesible. #18884

http: making upstream ALPN accesible. #18884

Conversation

alyssawilk commented Nov 3, 2021 • edited Loading

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk commented Nov 3, 2021

alyssawilk commented Nov 4, 2021

alyssawilk commented Nov 4, 2021

mattklein123 commented Nov 4, 2021

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk commented Nov 8, 2021

RyanTheOptimist left a comment

Choose a reason for hiding this comment

danzh2010 commented Nov 8, 2021

RyanTheOptimist left a comment

Choose a reason for hiding this comment

alyssawilk commented Nov 3, 2021 •

edited

Loading