Config switch to turn on/off Quic processing by nezdolik · Pull Request #9679 · envoyproxy/envoy

nezdolik · 2020-01-14T16:20:58Z

Signed-off-by: Kateryna Nezdolii nezdolik@spotify.com

Description:
Extending quic listener configuration with runtime feature flag for enabling/disabling quic processing. Flag controls onReadReady() and onWriteReady() callback methods and is enabled by default.
Risk Level: Low/Medium
Testing: Unit tests+Integration tests.
Docs Changes: NA?
Release Notes:
Fixes ##9230

repokitteh-read-only · 2020-01-14T16:21:03Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9679 was opened by nezdolik.

see: more, trace.

nezdolik · 2020-01-14T16:22:42Z

cc @alyssawilk @danzh2010 @mattklein123 this is initial draft for config layout.

alyssawilk

Looks good!

Some part of me wants to encourage one big enum (since when we disable QUIC we make the shutdown mode call based on the reason we're shutting down) but honestly this is more clear and will hopefully avoid any decision paralysis when dealing with production fires. And folks can still push a different shut down mode with their disable if their default doesn't match their disasater :-)

mattklein123

Thanks for working on this. Flushing out a few comments.

/wait

mattklein123 · 2020-01-16T17:45:18Z

Sorry I'm a little confused about what is being proposed here. Do you mind adding some more comments?

@mattklein123 @htuch for turning off/on udp/quick processing, my proposal is to have boolean switch on udp listener config + shutdown mode field (for shutdown type). Submitted this draft config to get community agreement on the approach, code will follow.

Why don't we just remove the listener and drain as normal? Trying to grok what is distinct here vs. any other listener that comes to life at some point, starts accepting, and then is removed and drained.

@htuch thanks for suggestion, i did not work much with LDS, so missed that opportunity. Agree that this is cleaner way, however currently listener manager supports only draining (graceful shutdown) of active listeners:
https://github.com/envoyproxy/envoy/blob/master/source/server/listener_manager_impl.cc#L668
In case we go with LDS approach, it will require code change to support multiple draining modes, right?

I guess it depends how we structure shared listeners.
I thought if we had QUIC and QUIC-proxy listening on UDP/443 they'd share a listener, and some higher level filter would determine which packets went where. In that case as said it'd be nice to able to disable them one at a time. We can deal without but it's a nice to have.
If we think we'd have a QUIC/443 listener and a QUIC-proxy/443 listener just sharing incoming traffic via some combination of SO_REUSEPORT and BPF this would be fine.
I'd assumed we were going to do more of the former, at which point a non-listener kill switch has value IMO (though again we can live without it)

My actual assumption (potentially naive) was that QUIC and QUIC-proxy would probably be different deployments of the binary in different auto scaling groups, etc. What is the use case in which you envision the same process doing both?

Generic example is for edge proxies with low-value certs that do transparent proxying for latency gains. QUIC-terminate locally served content, UDP proxy where you'd TCP proxy because DNS SRV doesn't work.
We have two different frontline deployments which do TLS / TCP proxy based on VIP and one of them did QUIC+Udp-proxy for years.
We haven't had an issue where we needed to turn off one and not the other yet, but as tunneling over QUIC picks up steam (and that team has agreed to OSS once we land core QUIC) I think it gets more useful to turn different modes down separately.
We can always start simple and add more later though!

We can always start simple and add more later though!

That would be my feeling. Right now we don't have any filter chain concept for UDP (I would like to fix that as part of a general listener API cleanup), and without that I'm not sure how we would do something like ^ today. IMO right now I think we could just have a per-listener runtime kill switch for UDP listeners? Or even just for the QUIC listener config itself?

Thanks for feedback, it's valuable information to get familiar with. So we agreed to introduce separate config switches for upd and quic listeners.

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik · 2020-01-24T15:29:18Z

 ActiveRawUdpListenerConfigFactory::createActiveUdpListenerFactory(
-    const Protobuf::Message& /*message*/) {
+    const Protobuf::Message& message) {
+  auto& config =


for now this cast will not work, need to reorg udp listener proto config to have one top level message

mattklein123

Thanks for working on this. Flushing out a few more comments.

/wait

mattklein123 · 2020-01-24T21:17:35Z

  google.protobuf.Duration crypto_handshake_timeout = 3;
 }
+
+message ActiveQuicListenerConfig {


I would probably just name this QuicListenerConfig. I think we can drop the Active (having it in the other place is a mistake IMO).

In terms of the shutoff, what I'm proposing is just having a RuntimeFeatureFlag which says whether to process packets or not. I think this is ultimately the kill switch we are looking for? @alyssawilk @danzh2010 WDYT?

Again I think it's not the switch I'm looking for in the long term (because not granular enough) but I'm fine landing whatever controls we think are useful today and we can add the finicky ones once we're running in prod and care more :-)

mattklein123 · 2020-01-24T21:18:22Z

 }

 message ActiveRawUdpListenerConfig {
+  enum UdpShutDownMode {


I'm not completely positive we want to bother with this kill switch for raw UDP right now. Maybe defer that for later? If we do it here, we should de-dup the messages and have a common configuration message of some type.

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

stale · 2020-02-03T15:58:26Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik · 2020-02-04T16:37:48Z

Spent most of the time on task, which may be out of scope for this change, basically trying to propagate Envoy primitives (like Runtime) all the way down from server instance->worker->connection handler->active quic listener factory->active quic listener, which introduced cyclic dependency in bazel modules. As current code is not production ready (and ActiveQuicListenerFactory not being instantiated from conn_handler), change may be fine as it is (adding Runtime field to ActiveQuicListenerFactory), but in the long run code needs to be refactored, possibly introducing UdpListenerFactoryContext that would provide to resources like Dispatcher, Runtime::Loader etc.
Still need to complete tests.
Would appreciate opinions if refactoring change should be part of this task or not.

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

mattklein123 · 2020-02-05T00:08:12Z

Check out Runtime::LoaderSingleton::getExisting() which will probably make this task easier. This is one case where we break our rule about no statics.

nezdolik · 2020-04-30T16:42:51Z

@danzh2010 switched to real Runtime object and added end to end test for empty config. Please take a look, i believe it addresses your review comments.

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

danzh2010

Only a few nits!

danzh2010 · 2020-05-01T17:46:59Z

  quic::QuicBufferedPacketStore* const buffered_packets =
      quic::test::QuicDispatcherPeer::GetBufferedPackets(quic_dispatcher_);
-  configureQuicRuntimeFlag(/* runtime_enabled = */ true);
+  // configureQuicRuntimeFlag(/* runtime_enabled = */ true);


danzh2010 · 2020-05-01T17:47:24Z

        "-Wall",
        "-Wextra",
-        "-Werror",
+        #"-Werror",


what is this for?

@danzh2010 i was getting complaint (gcc) for quiche library code:

bazel-out/k8-fastbuild/bin/external/com_googlesource_quiche/quiche/quic/core/quic_framer.cc: In member function 'bool quic::QuicFramer::AppendIetfAckFrameAndTypeByte(const quic::QuicAckFrame&, quic::QuicDataWriter*)': bazel-out/k8-fastbuild/bin/external/com_googlesource_quiche/quiche/quic/core/quic_framer.cc:5434:40: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare] writer->remaining() - ecn_size < ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^ QuicDataWriter::GetVarInt62Len(gap) + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ QuicDataWriter::GetVarInt62Len(ack_range)) { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1plus: all warnings being treated as errors

I'm fixing this issue in quiche now. It will be updated soon.

danzh2010 · 2020-05-01T17:58:41Z

  void resumeListening() override;
  void shutdownListener() override;

+  bool enabled() { return enabled_.enabled(); }


I don't strongly feel this interface is needed. Can you inline it at call site? If it's only used in test, accessing via a peer class is better.

@danzh2010 alright, just looked up that method in other envoy modules and followed by example.

This is not the only definition of quic listener being enabled. I would prefer not to add this interface.

danzh2010 · 2020-05-01T18:18:24Z

+    quic_listener_ = std::make_unique<ActiveQuicListener>(*dispatcher_, connection_handler_,
+                                                          listen_socket_, listener_config_,
+                                                          quic_config_, nullptr, enabledFlag());


Overriding enabledFlag() is a bit indirect. Can you add two methods like below:

std::make_unique<ActiveQuicListener> createQuicListenerFactory(const std::string& yaml) { std::string listener_name = QuicListenerName; auto& config_factory = Config::Utility::getAndCheckFactoryByName<Server::ActiveUdpListenerConfigFactory>( listener_name); ProtobufTypes::MessagePtr config = config_factory.createEmptyConfigProto(); TestUtility::loadFromYaml(yaml, *config); return config_factory.createActiveUdpListenerFactory(*config, /*concurrency=*/1); virtual std::string yamlForQuicConfig() { return R"EOF( runtime_key: "quic.enabled" default_value: true )EOF"; }

And use them here:

EXPECT_CALL(listener_config_, listenSocketFactory()); EXPECT_CALL(listener_config_. socket_factory_, getListenSocket).WillOnce(Return(listen_socket_)); quic_listener_ = createQuicListenerFactory(yamlForQuicConfig())->createActiveUdpListener(*dispatcher_, connection_handler_, ...);

In ActiveQuicListenerEmptyFlagConfigTest,

std::string yamlForQuicConfig() override { return R"EOF( max_concurrent_streams: 10 )EOF"; }

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

repokitteh-read-only · 2020-05-06T21:26:31Z

CC @envoyproxy/api-watchers: FYI only for changes made to api/.

🐱

Caused by: #9679 was synchronize by nezdolik.

see: more, trace.

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik · 2020-05-06T21:44:05Z


-    quic_listener_ = std::make_unique<ActiveQuicListener>(
-        *dispatcher_, connection_handler_, listen_socket_, listener_config_, quic_config_, nullptr);
+    EXPECT_CALL(listener_config_, listenSocketFactory())


this is needed to wire up mocks, that are used inside listener factory when creating quic listener

For this type of thing it's better to use ON_CALL(...).WillByDefault()

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik · 2020-05-08T17:47:11Z

There is failing coverage: https://343614-65214191-gh.circle-artifacts.com/0/coverage/index.html but none if it is related to this PR. Going to merge master and see if it helps

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

mattklein123

LGTM other than small nits. @danzh2010 can you take a final pass? Thank you!

/wait

mattklein123 · 2020-05-12T21:10:58Z

                     Network::ListenerConfig& listener_config, const quic::QuicConfig& quic_config,
-                     Network::Socket::OptionsSharedPtr options);
+                     Network::Socket::OptionsSharedPtr options,
+                     const envoy::config::core::v3::RuntimeFeatureFlag enabled);


nit: pass by const ref here and below

mattklein123 · 2020-05-12T21:12:23Z


-    quic_listener_ = std::make_unique<ActiveQuicListener>(
-        *dispatcher_, connection_handler_, listen_socket_, listener_config_, quic_config_, nullptr);
+    EXPECT_CALL(listener_config_, listenSocketFactory())


For this type of thing it's better to use ON_CALL(...).WillByDefault()

danzh2010

LGTM overall, just a few nits in test.

danzh2010 · 2020-05-13T15:34:56Z

 ActiveQuicListenerFactory::ActiveQuicListenerFactory(
    const envoy::config::listener::v3::QuicProtocolOptions& config, uint32_t concurrency)
-    : concurrency_(concurrency) {
+    : concurrency_(concurrency), enabled_(config.enabled()) {


Thanks for looking into this!

danzh2010 · 2020-05-13T15:59:43Z

  ReadFromClientSockets();
 }

+TEST_P(ActiveQuicListenerTest, QuicProcessingDisabled) {


This test is already tested in QuicProcessingDisabledAndEnabled, right? Maybe remove it?

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik · 2020-05-14T19:24:21Z

FAILED_PRECONDITION: there are no bots capable of executing.... in ci, need to wait for fix...

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik · 2020-05-15T11:11:35Z

@danzh2010 @mattklein123 applied review comments.

mattklein123

Thanks!

repokitteh-read-only Bot added the api label Jan 14, 2020

nezdolik commented Jan 14, 2020

View reviewed changes

mattklein123 assigned danzh2010 Jan 14, 2020

htuch reviewed Jan 14, 2020

View reviewed changes

Comment thread api/envoy/api/v2/listener/udp_listener_config.proto Outdated

alyssawilk reviewed Jan 15, 2020

View reviewed changes

mattklein123 self-assigned this Jan 16, 2020

mattklein123 requested changes Jan 16, 2020

View reviewed changes

repokitteh-read-only Bot added the waiting label Jan 16, 2020

[WIP] Config switch to turn on/off Quic/Udp processing

b526e9f

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik force-pushed the quic-switch branch from 0c123b5 to b526e9f Compare January 24, 2020 15:26

repokitteh-read-only Bot removed the waiting label Jan 24, 2020

nezdolik commented Jan 24, 2020

View reviewed changes

mattklein123 requested changes Jan 24, 2020

View reviewed changes

repokitteh-read-only Bot added the waiting label Jan 24, 2020

Kateryna Nezdolii added 3 commits January 31, 2020 11:11

apply review comments

fbbef39

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

Merge remote-tracking branch 'origin/master'

6a8a161

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

use runtime flag instead

1377cd5

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

stale Bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 3, 2020

Kateryna Nezdolii added 3 commits February 3, 2020 17:07

more wip

ed1a399

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

more wip

98148f1

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

more wip

43905d8

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

stale Bot removed the stale stalebot believes this issue/PR has not been touched recently label Feb 4, 2020

repokitteh-read-only Bot removed the waiting label Feb 4, 2020

remove dummy changes

b64e40b

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

fix default

6d3eaf8

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

mattklein123 added the waiting label Feb 5, 2020

fix spelling

0bd459c

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

danzh2010 reviewed May 1, 2020

View reviewed changes

Apply review comments

ae894d9

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

Kateryna Nezdolii added 3 commits May 6, 2020 23:28

clean up

8c1a26a

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

Clean up

76bdeed

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

ammend formatting of new lines

c52961e

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik commented May 6, 2020

View reviewed changes

Kateryna Nezdolii added 10 commits May 7, 2020 00:06

fix format

af9a8aa

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

Merge remote-tracking branch 'origin/master'

6c8176e

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

fix proto v4

8ab8576

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

fix v4 protos

e4393dd

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

fix api change

de45a8d

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

fix clang

bb53249

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

fix v4 protos

198c267

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

fix generated api shadow

8ad4ae6

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

fix proto again

99d1e92

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

more clang tidy

a328c6b

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

Merge remote-tracking branch 'origin/master'

5c76b99

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

mattklein123 requested changes May 12, 2020

View reviewed changes

danzh2010 previously approved these changes May 13, 2020

View reviewed changes

Kateryna Nezdolii added 2 commits May 14, 2020 18:02

apply review comments

e055f8a

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nits in test

0737550

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

Kateryna Nezdolii added 2 commits May 15, 2020 10:26

dummy to retrigger ci

a507b8b

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

remove unused import

6ce2379

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

mattklein123 approved these changes May 15, 2020

View reviewed changes

Conversation

nezdolik commented Jan 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only Bot commented Jan 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nezdolik Jan 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale Bot commented Feb 3, 2020

Uh oh!

nezdolik commented Feb 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Feb 5, 2020

Uh oh!

nezdolik commented Apr 30, 2020

Uh oh!

danzh2010 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nezdolik May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nezdolik commented Jan 14, 2020 •

edited

Loading

nezdolik Jan 20, 2020 •

edited

Loading

nezdolik commented Feb 4, 2020 •

edited

Loading

nezdolik May 4, 2020 •

edited

Loading

danzh2010 left a comment •

edited

Loading