-
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
Config switch to turn on/off Quic processing #9679
Changes from 45 commits
b526e9f
fbbef39
6a8a161
1377cd5
ed1a399
98148f1
43905d8
b64e40b
6d3eaf8
4c0c2fa
f9cdb78
3e47943
9502c3e
6940388
2f8e4de
ccf4bb1
68c8702
cef470d
80556e1
d77c6a1
132cce0
e3ac7b0
0091d58
fd9adb0
d70fb7d
ed64f3a
d914ed0
dd5b6ed
3b595da
60b7034
d27bc45
e98f673
97691af
019dc49
740d3d2
1b7a7f5
9ae7c42
b25447e
7be1fd4
f6960de
471a56f
5f1e4ad
598082c
1828540
5e0fc6c
b865e60
2a0e388
0bd459c
ae894d9
8c1a26a
76bdeed
c52961e
af9a8aa
6c8176e
8ab8576
e4393dd
de45a8d
bb53249
198c267
8ad4ae6
99d1e92
a328c6b
5c76b99
e055f8a
0737550
a507b8b
6ce2379
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,9 +3,11 @@ | |
#include "envoy/config/listener/v3/quic_config.pb.h" | ||
#include "envoy/network/connection_handler.h" | ||
#include "envoy/network/listener.h" | ||
#include "envoy/runtime/runtime.h" | ||
|
||
#include "common/network/socket_option_impl.h" | ||
#include "common/protobuf/utility.h" | ||
#include "common/runtime/runtime_protos.h" | ||
|
||
#include "server/connection_handler_impl.h" | ||
|
||
|
@@ -25,12 +27,14 @@ class ActiveQuicListener : public Network::UdpListenerCallbacks, | |
|
||
ActiveQuicListener(Event::Dispatcher& dispatcher, Network::ConnectionHandler& parent, | ||
Network::ListenerConfig& listener_config, const quic::QuicConfig& quic_config, | ||
Network::Socket::OptionsSharedPtr options); | ||
Network::Socket::OptionsSharedPtr options, | ||
const envoy::config::core::v3::RuntimeFeatureFlag enabled); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: pass by const ref here and below There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
|
||
ActiveQuicListener(Event::Dispatcher& dispatcher, Network::ConnectionHandler& parent, | ||
Network::SocketSharedPtr listen_socket, | ||
Network::ListenerConfig& listener_config, const quic::QuicConfig& quic_config, | ||
Network::Socket::OptionsSharedPtr options); | ||
Network::Socket::OptionsSharedPtr options, | ||
const envoy::config::core::v3::RuntimeFeatureFlag enabled); | ||
|
||
~ActiveQuicListener() override; | ||
|
||
|
@@ -50,6 +54,8 @@ class ActiveQuicListener : public Network::UdpListenerCallbacks, | |
void resumeListening() override; | ||
void shutdownListener() override; | ||
|
||
bool enabled() { return enabled_.enabled(); } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @danzh2010 alright, just looked up that method in other envoy modules and followed by example. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not the only definition of quic listener being enabled. I would prefer not to add this interface. |
||
|
||
private: | ||
friend class ActiveQuicListenerPeer; | ||
|
||
|
@@ -60,6 +66,7 @@ class ActiveQuicListener : public Network::UdpListenerCallbacks, | |
quic::QuicVersionManager version_manager_; | ||
std::unique_ptr<EnvoyQuicDispatcher> quic_dispatcher_; | ||
Network::Socket& listen_socket_; | ||
Runtime::FeatureFlag enabled_; | ||
}; | ||
|
||
using ActiveQuicListenerPtr = std::unique_ptr<ActiveQuicListener>; | ||
|
@@ -83,6 +90,7 @@ class ActiveQuicListenerFactory : public Network::ActiveUdpListenerFactory, | |
quic::QuicConfig quic_config_; | ||
const uint32_t concurrency_; | ||
absl::once_flag install_bpf_once_; | ||
envoy::config::core::v3::RuntimeFeatureFlag enabled_; | ||
}; | ||
|
||
} // namespace Quic | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,10 @@ class ActiveQuicListenerFactoryPeer { | |
static quic::QuicConfig& quicConfig(ActiveQuicListenerFactory& factory) { | ||
return factory.quic_config_; | ||
} | ||
static envoy::config::core::v3::RuntimeFeatureFlag& | ||
runtimeEnabled(ActiveQuicListenerFactory& factory) { | ||
return factory.enabled_; | ||
} | ||
}; | ||
|
||
TEST(ActiveQuicListenerConfigTest, CreateActiveQuicListenerFactory) { | ||
|
@@ -29,6 +33,9 @@ TEST(ActiveQuicListenerConfigTest, CreateActiveQuicListenerFactory) { | |
idle_timeout: { | ||
seconds: 2 | ||
} | ||
enabled: | ||
default_value: true | ||
runtime_key: foo_key | ||
nezdolik marked this conversation as resolved.
Show resolved
Hide resolved
|
||
)EOF"; | ||
TestUtility::loadFromYaml(yaml, *config); | ||
Network::ActiveUdpListenerFactoryPtr listener_factory = | ||
|
@@ -41,6 +48,35 @@ TEST(ActiveQuicListenerConfigTest, CreateActiveQuicListenerFactory) { | |
EXPECT_EQ(2000u, quic_config.IdleNetworkTimeout().ToMilliseconds()); | ||
// Default value if not present in config. | ||
EXPECT_EQ(20000u, quic_config.max_time_before_crypto_handshake().ToMilliseconds()); | ||
envoy::config::core::v3::RuntimeFeatureFlag& runtime_enabled = | ||
ActiveQuicListenerFactoryPeer::runtimeEnabled( | ||
dynamic_cast<ActiveQuicListenerFactory&>(*listener_factory)); | ||
EXPECT_EQ(true, runtime_enabled.default_value().value()); | ||
EXPECT_EQ("foo_key", runtime_enabled.runtime_key()); | ||
} | ||
|
||
TEST(ActiveQuicListenerConfigTest, QuicListenerFlagNotConfigured) { | ||
nezdolik marked this conversation as resolved.
Show resolved
Hide resolved
|
||
std::string listener_name = QuicListenerName; | ||
auto& config_factory = | ||
Config::Utility::getAndCheckFactoryByName<Server::ActiveUdpListenerConfigFactory>( | ||
listener_name); | ||
ProtobufTypes::MessagePtr config = config_factory.createEmptyConfigProto(); | ||
|
||
std::string yaml = R"EOF( | ||
max_concurrent_streams: 10 | ||
idle_timeout: { | ||
seconds: 2 | ||
} | ||
)EOF"; | ||
TestUtility::loadFromYaml(yaml, *config); | ||
nezdolik marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Network::ActiveUdpListenerFactoryPtr listener_factory = | ||
config_factory.createActiveUdpListenerFactory(*config, /*concurrency=*/1); | ||
EXPECT_NE(nullptr, listener_factory); | ||
envoy::config::core::v3::RuntimeFeatureFlag& runtime_enabled = | ||
ActiveQuicListenerFactoryPeer::runtimeEnabled( | ||
dynamic_cast<ActiveQuicListenerFactory&>(*listener_factory)); | ||
EXPECT_EQ(false, runtime_enabled.default_value().value()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you check runtime_enabled.has_default_value() here instead? If it's false, we know that FeatureFlags will be instantiated with default value true. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
EXPECT_EQ("", runtime_enabled.runtime_key()); | ||
} | ||
|
||
} // namespace Quic | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,9 @@ | |
|
||
#include <memory> | ||
|
||
#include "envoy/config/core/v3/base.pb.h" | ||
#include "envoy/config/core/v3/base.pb.validate.h" | ||
|
||
#include "quiche/quic/core/crypto/crypto_protocol.h" | ||
#include "quiche/quic/test_tools/crypto_test_utils.h" | ||
#include "quiche/quic/test_tools/quic_dispatcher_peer.h" | ||
|
@@ -23,6 +26,7 @@ | |
#include "test/test_common/simulated_time_system.h" | ||
#include "test/test_common/environment.h" | ||
#include "test/mocks/network/mocks.h" | ||
#include "test/mocks/runtime/mocks.h" | ||
#include "test/test_common/utility.h" | ||
#include "test/test_common/network_utility.h" | ||
#include "absl/time/time.h" | ||
|
@@ -57,19 +61,28 @@ class ActiveQuicListenerTest : public testing::TestWithParam<Network::Address::I | |
: version_(GetParam()), api_(Api::createApiForTest(simulated_time_system_)), | ||
dispatcher_(api_->allocateDispatcher("test_thread")), clock_(*dispatcher_), | ||
local_address_(Network::Test::getCanonicalLoopbackAddress(version_)), | ||
connection_handler_(*dispatcher_) {} | ||
connection_handler_(*dispatcher_) { | ||
Runtime::LoaderSingleton::initialize(&runtime_); | ||
} | ||
|
||
void SetUp() override { | ||
listen_socket_ = | ||
std::make_shared<Network::UdpListenSocket>(local_address_, nullptr, /*bind*/ true); | ||
listen_socket_->addOptions(Network::SocketOptionFactory::buildIpPacketInfoOptions()); | ||
listen_socket_->addOptions(Network::SocketOptionFactory::buildRxQueueOverFlowOptions()); | ||
|
||
quic_listener_ = std::make_unique<ActiveQuicListener>( | ||
*dispatcher_, connection_handler_, listen_socket_, listener_config_, quic_config_, nullptr); | ||
quic_listener_ = std::make_unique<ActiveQuicListener>(*dispatcher_, connection_handler_, | ||
listen_socket_, listener_config_, | ||
quic_config_, nullptr, enabledFlag()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overriding enabledFlag() is a bit indirect. Can you add two methods like below:
And use them here:
In ActiveQuicListenerEmptyFlagConfigTest,
|
||
quic_dispatcher_ = ActiveQuicListenerPeer::quic_dispatcher(*quic_listener_); | ||
simulated_time_system_.advanceTimeWait(std::chrono::milliseconds(100)); | ||
} | ||
|
||
void configureQuicRuntimeFlag(bool runtime_enabled) { | ||
EXPECT_CALL(runtime_.snapshot_, getBoolean("quic.enabled", true)) | ||
.WillRepeatedly(Return(runtime_enabled)); | ||
} | ||
|
||
void configureMocks(int connection_count) { | ||
EXPECT_CALL(listener_config_, filterChainManager()) | ||
.Times(connection_count) | ||
|
@@ -116,7 +129,7 @@ class ActiveQuicListenerTest : public testing::TestWithParam<Network::Address::I | |
|
||
// TODO(bencebeky): Factor out parts common with | ||
// EnvoyQuicDispatcherTest::createFullChloPacket() to test_utils. | ||
void SendFullCHLO(quic::QuicConnectionId connection_id) { | ||
void sendFullCHLO(quic::QuicConnectionId connection_id) { | ||
client_sockets_.push_back(std::make_unique<Socket>(local_address_, nullptr, /*bind*/ false)); | ||
quic::CryptoHandshakeMessage chlo = quic::test::crypto_test_utils::GenerateDefaultInchoateCHLO( | ||
&clock_, quic::AllSupportedVersions()[0].transport_version, | ||
|
@@ -185,6 +198,18 @@ class ActiveQuicListenerTest : public testing::TestWithParam<Network::Address::I | |
quic_listener_->onListenerShutdown(); | ||
// Trigger alarm to fire before listener destruction. | ||
dispatcher_->run(Event::Dispatcher::RunType::NonBlock); | ||
Runtime::LoaderSingleton::clear(); | ||
} | ||
|
||
protected: | ||
envoy::config::core::v3::RuntimeFeatureFlag enabledFlag() const { | ||
envoy::config::core::v3::RuntimeFeatureFlag enabled_proto; | ||
std::string yaml(R"EOF( | ||
runtime_key: "quic.enabled" | ||
default_value: true | ||
)EOF"); | ||
TestUtility::loadFromYamlAndValidate(yaml, enabled_proto); | ||
return enabled_proto; | ||
} | ||
|
||
Network::Address::IpVersion version_; | ||
|
@@ -201,6 +226,8 @@ class ActiveQuicListenerTest : public testing::TestWithParam<Network::Address::I | |
quic::QuicConfig quic_config_; | ||
Server::ConnectionHandlerImpl connection_handler_; | ||
std::unique_ptr<ActiveQuicListener> quic_listener_; | ||
EnvoyQuicDispatcher* quic_dispatcher_; | ||
NiceMock<Runtime::MockLoader> runtime_; | ||
|
||
std::list<std::unique_ptr<Socket>> client_sockets_; | ||
std::list<std::shared_ptr<Network::MockReadFilter>> read_filters_; | ||
|
@@ -221,30 +248,33 @@ TEST_P(ActiveQuicListenerTest, FailSocketOptionUponCreation) { | |
.WillOnce(Return(false)); | ||
auto options = std::make_shared<std::vector<Network::Socket::OptionConstSharedPtr>>(); | ||
options->emplace_back(std::move(option)); | ||
EXPECT_THROW_WITH_REGEX(std::make_unique<ActiveQuicListener>(*dispatcher_, connection_handler_, | ||
listen_socket_, listener_config_, | ||
quic_config_, options), | ||
EnvoyException, "Failed to apply socket options."); | ||
EXPECT_THROW_WITH_REGEX( | ||
std::make_unique<ActiveQuicListener>(*dispatcher_, connection_handler_, listen_socket_, | ||
listener_config_, quic_config_, options, enabledFlag()), | ||
EnvoyException, "Failed to apply socket options."); | ||
} | ||
|
||
TEST_P(ActiveQuicListenerTest, ReceiveFullQuicCHLO) { | ||
quic::QuicBufferedPacketStore* const buffered_packets = | ||
quic::test::QuicDispatcherPeer::GetBufferedPackets(quic_dispatcher_); | ||
configureQuicRuntimeFlag(/* runtime_enabled = */ true); | ||
configureMocks(/* connection_count = */ 1); | ||
SendFullCHLO(quic::test::TestConnectionId(1)); | ||
sendFullCHLO(quic::test::TestConnectionId(1)); | ||
dispatcher_->run(Event::Dispatcher::RunType::NonBlock); | ||
EXPECT_FALSE(buffered_packets->HasChlosBuffered()); | ||
EXPECT_FALSE(quic_dispatcher_->session_map().empty()); | ||
ReadFromClientSockets(); | ||
} | ||
|
||
TEST_P(ActiveQuicListenerTest, ProcessBufferedChlos) { | ||
EnvoyQuicDispatcher* const envoy_quic_dispatcher = | ||
ActiveQuicListenerPeer::quic_dispatcher(*quic_listener_); | ||
quic::QuicBufferedPacketStore* const buffered_packets = | ||
quic::test::QuicDispatcherPeer::GetBufferedPackets(envoy_quic_dispatcher); | ||
|
||
quic::test::QuicDispatcherPeer::GetBufferedPackets(quic_dispatcher_); | ||
configureQuicRuntimeFlag(/* runtime_enabled = */ true); | ||
configureMocks(ActiveQuicListener::kNumSessionsToCreatePerLoop + 2); | ||
|
||
// Generate one more CHLO than can be processed immediately. | ||
for (size_t i = 1; i <= ActiveQuicListener::kNumSessionsToCreatePerLoop + 1; ++i) { | ||
SendFullCHLO(quic::test::TestConnectionId(i)); | ||
sendFullCHLO(quic::test::TestConnectionId(i)); | ||
} | ||
dispatcher_->run(Event::Dispatcher::RunType::NonBlock); | ||
|
||
|
@@ -256,9 +286,10 @@ TEST_P(ActiveQuicListenerTest, ProcessBufferedChlos) { | |
EXPECT_TRUE(buffered_packets->HasBufferedPackets( | ||
quic::test::TestConnectionId(ActiveQuicListener::kNumSessionsToCreatePerLoop + 1))); | ||
EXPECT_TRUE(buffered_packets->HasChlosBuffered()); | ||
EXPECT_FALSE(quic_dispatcher_->session_map().empty()); | ||
|
||
// Generate more data to trigger a socket read during the next event loop. | ||
SendFullCHLO(quic::test::TestConnectionId(ActiveQuicListener::kNumSessionsToCreatePerLoop + 2)); | ||
sendFullCHLO(quic::test::TestConnectionId(ActiveQuicListener::kNumSessionsToCreatePerLoop + 2)); | ||
dispatcher_->run(Event::Dispatcher::RunType::NonBlock); | ||
|
||
// The socket read results in processing all CHLOs. | ||
|
@@ -270,5 +301,13 @@ TEST_P(ActiveQuicListenerTest, ProcessBufferedChlos) { | |
ReadFromClientSockets(); | ||
} | ||
|
||
TEST_P(ActiveQuicListenerTest, QuicProcessingDisabled) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test is already tested in QuicProcessingDisabledAndEnabled, right? Maybe remove it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed |
||
configureQuicRuntimeFlag(/* runtime_enabled = */ false); | ||
sendFullCHLO(quic::test::TestConnectionId(1)); | ||
dispatcher_->run(Event::Dispatcher::RunType::NonBlock); | ||
// If listener was enabled, there should have been session created for active connection. | ||
EXPECT_TRUE(quic_dispatcher_->session_map().empty()); | ||
} | ||
|
||
} // namespace Quic | ||
} // namespace Envoy |
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.
what if config.has_enabled() == false?
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.
Just verified the implementation, has_enabled() checks if field is not (either nullptr or is set to default value), but enabled instead always provides value to the caller:
If config is empty, enabled field is set to default instance.
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.
Thanks for looking into this!