upstream adding QUIC-to-TCP failover by alyssawilk · Pull Request #15894 · envoyproxy/envoy

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

Conversation

alyssawilk commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Apr 8, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alyssawilk commented Apr 14, 2021

Uh oh!

alyssawilk commented Apr 15, 2021

Uh oh!

alyssawilk commented Apr 15, 2021

Uh oh!

alyssawilk commented Apr 15, 2021

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

antoniovicente Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

antoniovicente Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

alyssawilk Apr 16, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

antoniovicente Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk Apr 16, 2021

Choose a reason for hiding this comment

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

alyssawilk commented Apr 8, 2021 •

edited

Loading