upstream adding QUIC-to-TCP failover #15894

alyssawilk · 2021-04-08T19:23:38Z

Hey Matt, can I get your thoughts on 2 things?

if we're Ok with having a hard-coded something (right now "use_quic") matcher to handle TCP-TLS and QUIC-TLS in the same cluster (or maybe we want to do something to combine the two configs and auto-select based on transport?)
how we want to handle the connection pool being able to select a non-default transportSocket. I have "use_quic" as a placeholder argument but I think if we extend the API the boolean should probably be replaced with an optional metadata. I'd like your thoughts if you have a better solution before I go change the API and all the mocks though.

Because I haven't cleaned up all the mocks so this will not build, but the integration test both provides a proof of concept for failover working, and the annoyance in configuring both TCP-TLS and QUIC-TLS transport sockets.

cc @RyanTheOptimist

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Part of #14829

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

repokitteh-read-only · 2021-04-08T19:23:44Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #15894 was opened by alyssawilk.

see: more, trace.

mattklein123

Exciting stuff. A left some ideas for discussion. Thank you!

/wait-any

test/common/upstream/upstream_impl_test.cc

source/common/upstream/cluster_manager_impl.cc

source/common/upstream/upstream_impl.cc

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

alyssawilk · 2021-04-14T16:50:00Z

Tsan failure was CI failure, not test fail. This should be ready for a pass!

alyssawilk · 2021-04-15T15:07:23Z

@snowp would you have time today to do a first pass? Matt's on call for the release and I was hoping to land this before I take off for a week.

alyssawilk · 2021-04-15T15:30:28Z

ugggh, accidentally pushed to the wrong branch. reverted but CI is of course running again (was green, and barring flakes will be green again)

alyssawilk · 2021-04-15T19:10:37Z

CI is unrelated flake:

2021-04-15T17:51:08.2680556Z Pulling zipkin (openzipkin/zipkin:)...
2021-04-15T17:51:13.3215508Z received unexpected HTTP status: 503 Service Unavailable
2021-04-15T17:51:13.3776159Z ERROR: starting zipkin .
2021-04-15T17:51:13.3797151Z
2021-04-15T17:51:13.3797958Z > [zipkin] Cleanup (.)
2021-04-15T17:51:13.9288406Z Removing network zipkin-tracing_envoymesh
2021-04-15T17:51:14.2006879Z TESTS FAILED:

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

antoniovicente

Generally looks good. There may be a threading issue in the call to maybeCreateFallbackFactory if the factories are not per-worker.

source/common/quic/quic_transport_socket_factory.cc

antoniovicente · 2021-04-15T23:21:16Z

source/common/quic/quic_transport_socket_factory.h

+  Network::TransportSocketPtr
+  createTransportSocket(Network::TransportSocketOptionsSharedPtr options) const override {
+    // TODO(#15649) make createTransportSocket non-const to avoid the const cast.
+    const_cast<QuicClientTransportSocketFactory*>(this)->maybeCreateFallbackFactory();


Are factories per worker or can createTransportSocket be called concurrently by multiple theads? If the later, there's a thread race in this code.

I would consider creating this fallback factory in QuicClientTransportSocketFactory's constructor in order to guarantee that it always exists if createTransportSocket is ever called.

antoniovicente · 2021-04-15T23:26:04Z

source/common/upstream/cluster_manager_impl.cc

+    Envoy::Http::ConnectivityGrid::ConnectivityOptions coptions{protocols};
+    return std::make_unique<Http::ConnectivityGrid>(
+        dispatcher, api_.randomGenerator(), host, priority, options, transport_socket_options,
+        state, source, std::chrono::milliseconds(300), coptions);


Curious, are there plans to make next_attempt_duration configurable in the future?

Yep, I figured I'd make @RyanTheOptimist take that on both as a simple API change, and to reduce bikeshedding on this PR

source/common/upstream/cluster_manager_impl.cc

antoniovicente · 2021-04-15T23:33:54Z

test/common/upstream/upstream_impl_test.cc

+  ASSERT_TRUE(cluster1->info()->idleTimeout().has_value());
+  EXPECT_EQ(std::chrono::hours(1), cluster1->info()->idleTimeout().value());
+
+  const std::string explicit_http3 = R"EOF(


nit: changing "explicit" to "alpn" or "auto" would be slightly clearer

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

alyssawilk

Thanks for the helpful review!
I think default-creating the fallback isn't enough for secrets update, but I'm going to leave that as a TODO and fix when I get back. Assuming I manage to get away =P

alyssawilk · 2021-04-16T01:37:46Z

source/common/upstream/cluster_manager_impl.cc

+    Envoy::Http::ConnectivityGrid::ConnectivityOptions coptions{protocols};
+    return std::make_unique<Http::ConnectivityGrid>(
+        dispatcher, api_.randomGenerator(), host, priority, options, transport_socket_options,
+        state, source, std::chrono::milliseconds(300), coptions);


Yep, I figured I'd make @RyanTheOptimist take that on both as a simple API change, and to reduce bikeshedding on this PR

antoniovicente

Thanks for the change and enjoy your vacation next week!

htuch

/lgtm api

Risk Level: n/a (hidden by default) Testing: e2e tests Docs Changes: n/a Release Notes: n/a Part of envoyproxy#14829 Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Risk Level: n/a (hidden by default) Testing: e2e tests Docs Changes: n/a Release Notes: n/a Part of envoyproxy#14829 Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>

Risk Level: n/a (hidden by default) Testing: e2e tests Docs Changes: n/a Release Notes: n/a Part of envoyproxy#14829 Signed-off-by: Gokul Nair <gnair@twitter.com>

Adding QUIC-to-TCP failover

745132d

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

alyssawilk assigned mattklein123 Apr 8, 2021

repokitteh-read-only bot added the api label Apr 8, 2021

mattklein123 reviewed Apr 8, 2021

View reviewed changes

test/common/upstream/upstream_impl_test.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting:any label Apr 8, 2021

RyanTheOptimist reviewed Apr 8, 2021

View reviewed changes

source/common/upstream/cluster_manager_impl.cc Show resolved Hide resolved

source/common/upstream/upstream_impl.cc Show resolved Hide resolved

repokitteh-read-only bot removed the waiting:any label Apr 8, 2021

mattklein123 added the waiting label Apr 9, 2021

Removing duplicate TLS config

fdede29

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

repokitteh-read-only bot removed the waiting label Apr 13, 2021

alyssawilk marked this pull request as ready for review April 13, 2021 15:43

alyssawilk requested review from asraa and ggreenway as code owners April 13, 2021 15:43

mattklein123 added the waiting label Apr 13, 2021

spelling

91df86b

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

repokitteh-read-only bot removed the waiting label Apr 13, 2021

mattklein123 added the waiting label Apr 13, 2021

fixing ~http3 build

c3f7e0a

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

repokitteh-read-only bot removed the waiting label Apr 14, 2021

alyssawilk force-pushed the h3_api branch from 0687ed1 to c3f7e0a Compare April 15, 2021 15:22

Merge branch 'main' into h3_api

99d30d3

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

antoniovicente self-assigned this Apr 15, 2021

antoniovicente reviewed Apr 16, 2021

View reviewed changes

alyssawilk added 2 commits April 15, 2021 21:29

Merge branch 'main' into h3_api

0cd983b

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

comments

7cdb07c

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

alyssawilk commented Apr 16, 2021

View reviewed changes

antoniovicente approved these changes Apr 16, 2021

View reviewed changes

htuch reviewed Apr 16, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Apr 16, 2021

alyssawilk merged commit ad4919d into envoyproxy:main Apr 16, 2021

alyssawilk deleted the h3_api branch February 28, 2022 21:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upstream adding QUIC-to-TCP failover #15894

upstream adding QUIC-to-TCP failover #15894

alyssawilk commented Apr 8, 2021 •

edited

Loading

repokitteh-read-only bot commented Apr 8, 2021

mattklein123 left a comment

alyssawilk commented Apr 14, 2021

alyssawilk commented Apr 15, 2021

alyssawilk commented Apr 15, 2021

alyssawilk commented Apr 15, 2021

antoniovicente left a comment

antoniovicente Apr 15, 2021

antoniovicente Apr 15, 2021

alyssawilk Apr 16, 2021

antoniovicente Apr 15, 2021

alyssawilk left a comment

alyssawilk Apr 16, 2021

antoniovicente left a comment

htuch left a comment

upstream adding QUIC-to-TCP failover #15894

upstream adding QUIC-to-TCP failover #15894

Conversation

alyssawilk commented Apr 8, 2021 • edited Loading

repokitteh-read-only bot commented Apr 8, 2021

mattklein123 left a comment

Choose a reason for hiding this comment

alyssawilk commented Apr 14, 2021

alyssawilk commented Apr 15, 2021

alyssawilk commented Apr 15, 2021

alyssawilk commented Apr 15, 2021

antoniovicente left a comment

Choose a reason for hiding this comment

antoniovicente Apr 15, 2021

Choose a reason for hiding this comment

antoniovicente Apr 15, 2021

Choose a reason for hiding this comment

alyssawilk Apr 16, 2021

Choose a reason for hiding this comment

antoniovicente Apr 15, 2021

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

alyssawilk Apr 16, 2021

Choose a reason for hiding this comment

antoniovicente left a comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

alyssawilk commented Apr 8, 2021 •

edited

Loading