Skip to content

quic: test fast-fail if secrets not loaded when create quic connection #18705

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.

Sign up for GitHub

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

Merged
ggreenway merged 4 commits into from
Nov 4, 2021
Merged

quic: test fast-fail if secrets not loaded when create quic connection #18705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0ea6649
quic: test fast-fail if secrets not loaded when create quic connection
YaoZengzeng Oct 21, 2021
32a8cc2
MockQuicClientTransportFactory
YaoZengzeng Oct 30, 2021
b4d5695
output log if create connection failed
YaoZengzeng Oct 30, 2021
22feb33
ENVOY_LOG_TO_LOGGER to ENVOY_LOG_EVERY_POW_2_TO_LOGGER
YaoZengzeng Nov 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions source/common/http/http3/conn_pool.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,10 @@ allocateConnPool(Event::Dispatcher& dispatcher, Random::RandomGenerator& random_
auto factory = &pool->host()->transportSocketFactory();
ASSERT(dynamic_cast<Quic::QuicClientTransportSocketFactory*>(factory) != nullptr);
if (static_cast<Quic::QuicClientTransportSocketFactory*>(factory)->sslCtx() == nullptr) {
ENVOY_LOG_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::pool), warn,
"Failed to create Http/3 client. Transport socket "
"factory is not configured correctly.");
ENVOY_LOG_EVERY_POW_2_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::pool),
warn,
"Failed to create Http/3 client. Transport socket "
"factory is not configured correctly.");
return nullptr;
}
Http3ConnPoolImpl* h3_pool = reinterpret_cast<Http3ConnPoolImpl*>(pool);
Expand All @@ -105,6 +106,12 @@ allocateConnPool(Event::Dispatcher& dispatcher, Random::RandomGenerator& random_
data.connection_ =
Quic::createQuicNetworkConnection(h3_pool->quicInfo(), pool->dispatcher(), host_address,
source_address, quic_stat_names, scope);
if (data.connection_ == nullptr) {
ENVOY_LOG_EVERY_POW_2_TO_LOGGER(
Envoy::Logger::Registry::getLog(Envoy::Logger::Id::pool), warn,

@ggreenway ggreenway Nov 2, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is warn going to be too much output in the log? I presume if this happens once, it'll probably happen for every connection attempt. Maybe use ENVOY_LOG_PERIODIC_TO_LOGGER or ENVOY_LOG_EVERY_POW_2_TO_LOGGER?

Is there any failure metric that increases in this case?

@YaoZengzeng YaoZengzeng Nov 3, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ggreenway updated to ENVOY_LOG_EVERY_POW_2_TO_LOGGER.

As for failure metric, there seems to be no metric to indicate the failure of quic connection creation. From my research, even for HTTP 1/2, there is only a metric bind_errors_ which is used to indicate the bind error when failed to create client network connection.

"Failed to create Http/3 client. Failed to create quic network connection.");
return nullptr;
}
// Store a handle to connection as it will be moved during client construction.
Network::Connection& connection = *data.connection_;
auto client = std::make_unique<ActiveClient>(*pool, data);
Expand Down
2 changes: 1 addition & 1 deletion source/common/quic/quic_transport_socket_factory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ class QuicClientTransportSocketFactory : public QuicTransportSocketFactoryBase {
return fallback_factory_->createTransportSocket(options);
}

Envoy::Ssl::ClientContextSharedPtr sslCtx() { return fallback_factory_->sslCtx(); }
virtual Envoy::Ssl::ClientContextSharedPtr sslCtx() { return fallback_factory_->sslCtx(); }

const Ssl::ClientContextConfig& clientContextConfig() const {
return fallback_factory_->config();
Expand Down
54 changes: 54 additions & 0 deletions test/common/http/http3/conn_pool_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,60 @@ class Http3ConnPoolImplTest : public Event::TestUsingSimulatedTime, public testi
ConnectionPool::InstancePtr pool_;
};

class MockQuicClientTransportSocketFactory : public Quic::QuicClientTransportSocketFactory {
public:
MockQuicClientTransportSocketFactory(
Ssl::ClientContextConfigPtr config,
Server::Configuration::TransportSocketFactoryContext& factory_context)
: Quic::QuicClientTransportSocketFactory(move(config), factory_context) {}

MOCK_METHOD(Envoy::Ssl::ClientContextSharedPtr, sslCtx, ());
};

TEST_F(Http3ConnPoolImplTest, FastFailWithoutSecretsLoaded) {
MockQuicClientTransportSocketFactory factory{
std::unique_ptr<Envoy::Ssl::ClientContextConfig>(new NiceMock<Ssl::MockClientContextConfig>),
context_};

EXPECT_CALL(factory, sslCtx()).WillRepeatedly(Return(nullptr));

EXPECT_CALL(mockHost(), address()).WillRepeatedly(Return(test_address_));
EXPECT_CALL(mockHost(), transportSocketFactory()).WillRepeatedly(testing::ReturnRef(factory));
// The unique pointer of this object will be returned in createSchedulableCallback_ of
// dispatcher_, so there is no risk of object leak.
new Event::MockSchedulableCallback(&dispatcher_);

@RyanTheOptimist RyanTheOptimist Oct 28, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this object leak, or does the dispatcher end up returning it at some point? If the latter, please add a short comment which says that.

Network::ConnectionSocket::OptionsSharedPtr options;
Network::TransportSocketOptionsConstSharedPtr transport_options;
ConnectionPool::InstancePtr pool =
allocateConnPool(dispatcher_, random_, host_, Upstream::ResourcePriority::Default, options,
transport_options, state_, simTime(), quic_stat_names_, store_);

EXPECT_EQ(static_cast<Http3ConnPoolImpl*>(pool.get())->instantiateActiveClient(), nullptr);
}

TEST_F(Http3ConnPoolImplTest, FailWithSecretsBecomeEmpty) {
MockQuicClientTransportSocketFactory factory{
std::unique_ptr<Envoy::Ssl::ClientContextConfig>(new NiceMock<Ssl::MockClientContextConfig>),
context_};

Ssl::ClientContextSharedPtr ssl_context(new Ssl::MockClientContext());
EXPECT_CALL(factory, sslCtx())
.WillOnce(Return(ssl_context))
.WillOnce(Return(nullptr))
.WillRepeatedly(Return(ssl_context));

EXPECT_CALL(mockHost(), address()).WillRepeatedly(Return(test_address_));
EXPECT_CALL(mockHost(), transportSocketFactory()).WillRepeatedly(testing::ReturnRef(factory));
new Event::MockSchedulableCallback(&dispatcher_);
Network::ConnectionSocket::OptionsSharedPtr options;
Network::TransportSocketOptionsConstSharedPtr transport_options;
ConnectionPool::InstancePtr pool =
allocateConnPool(dispatcher_, random_, host_, Upstream::ResourcePriority::Default, options,
transport_options, state_, simTime(), quic_stat_names_, store_);

EXPECT_EQ(static_cast<Http3ConnPoolImpl*>(pool.get())->instantiateActiveClient(), nullptr);
}

TEST_F(Http3ConnPoolImplTest, CreationWithBufferLimits) {
EXPECT_CALL(mockHost().cluster_, perConnectionBufferLimitBytes);
initialize();
Expand Down