Skip to content
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

quic: improve coverage #16569

Merged
merged 3 commits into from
May 20, 2021
Merged

quic: improve coverage #16569

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7468a30
improve coverage
alyssawilk May 18, 2021
caad86a
Merge branch 'main' into quic_coverage
alyssawilk May 19, 2021
1ea2aa6
Merge branch 'main' into quic_coverage
alyssawilk May 19, 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
1 change: 0 additions & 1 deletion source/common/quic/client_connection_factory_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ getContext(Network::TransportSocketFactory& transport_socket_factory) {
auto* quic_socket_factory =
dynamic_cast<QuicClientTransportSocketFactory*>(&transport_socket_factory);
ASSERT(quic_socket_factory != nullptr);
ASSERT(quic_socket_factory->sslCtx() != nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

If this ASSERT were to fail the following code would crash. If we remove the ASSERT, we should also fix this code so it doesn't crash.

envoy/source/common/quic/envoy_quic_proof_verifier.cc

Line 35 in 1ea2aa6

bool success = static_cast<Extensions::TransportSockets::Tls::ClientContextImpl*>(context_.get())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is already removed over here #16462
just hadn't merged it in.
That PR came with tests and error handling for SDS lazy loading of secrets :-)

Copy link
Contributor

@antoniovicente antoniovicente May 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

nit: Could we add ASSERT(context_ != nullptr) to the EnvoyQuicProofVerifier constructor as a sanity check? I'm fine with it being in this PR or a followup.

https://github.com/envoyproxy/envoy/blob/main/source/common/quic/envoy_quic_proof_verifier.h#L15

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in #16466 thanks

alyssawilk marked this conversation as resolved.
Show resolved Hide resolved
return quic_socket_factory->sslCtx();
}

Expand Down
3 changes: 2 additions & 1 deletion source/common/quic/quic_filter_manager_connection_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase,
absl::optional<Network::Connection::UnixDomainSocketPeerCredentials>
unixSocketPeerCredentials() const override {
// Unix domain socket is not supported.
NOT_REACHED_GCOVR_EXCL_LINE;
return absl::nullopt;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having trouble finding non-test uses of unixSocketPeerCredentials().

What am I missing? Who depends on this API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. I'd be happy to remove as a follow-up if we think it's doable.
I wonder if it's used internally by whoever @snowp worked for at the time?

}
void setConnectionStats(const Network::Connection::ConnectionStats& stats) override {
// TODO(danzh): populate stats.
Expand Down Expand Up @@ -113,6 +113,7 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase,
const StreamInfo::StreamInfo& streamInfo() const override { return stream_info_; }
absl::string_view transportFailureReason() const override { return transport_failure_reason_; }
bool startSecureTransport() override { return false; }
// TODO(#2557) Implement this.
absl::optional<std::chrono::milliseconds> lastRoundTripTime() const override { return {}; }

// Network::FilterManagerConnection
Expand Down
12 changes: 4 additions & 8 deletions source/common/quic/udp_gso_batch_writer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ namespace Quic {
namespace {
Api::IoCallUint64Result convertQuicWriteResult(quic::WriteResult quic_result, size_t payload_len) {
switch (quic_result.status) {
case quic::WRITE_STATUS_OK: {
case quic::WRITE_STATUS_OK:
if (quic_result.bytes_written == 0) {
ENVOY_LOG_MISC(trace, "sendmsg successful, message buffered to send");
} else {
Expand All @@ -18,23 +18,20 @@ Api::IoCallUint64Result convertQuicWriteResult(quic::WriteResult quic_result, si
return Api::IoCallUint64Result(
/*rc=*/payload_len,
/*err=*/Api::IoErrorPtr(nullptr, Network::IoSocketError::deleteIoError));
}
case quic::WRITE_STATUS_BLOCKED_DATA_BUFFERED: {
case quic::WRITE_STATUS_BLOCKED_DATA_BUFFERED:
// Data was buffered, Return payload_len as rc & nullptr as error
ENVOY_LOG_MISC(trace, "sendmsg blocked, message buffered to send");
return Api::IoCallUint64Result(
/*rc=*/payload_len,
/*err=*/Api::IoErrorPtr(nullptr, Network::IoSocketError::deleteIoError));
}
case quic::WRITE_STATUS_BLOCKED: {
case quic::WRITE_STATUS_BLOCKED:
// Writer blocked, return error
ENVOY_LOG_MISC(trace, "sendmsg blocked, message not buffered");
return Api::IoCallUint64Result(
/*rc=*/0,
/*err=*/Api::IoErrorPtr(Network::IoSocketError::getIoSocketEagainInstance(),
Network::IoSocketError::deleteIoError));
}
default: {
default:
// Write Failed, return {0 and error_code}
ENVOY_LOG_MISC(trace, "sendmsg failed with error code {}",
static_cast<int>(quic_result.error_code));
Expand All @@ -43,7 +40,6 @@ Api::IoCallUint64Result convertQuicWriteResult(quic::WriteResult quic_result, si
/*err=*/Api::IoErrorPtr(new Network::IoSocketError(quic_result.error_code),
Network::IoSocketError::deleteIoError));
}
}
}

} // namespace
Expand Down
1 change: 1 addition & 0 deletions test/common/http/conn_pool_grid_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,7 @@ TEST_F(ConnectivityGridTest, RealGrid) {
Envoy::Ssl::ClientContextConfigPtr config(new NiceMock<Ssl::MockClientContextConfig>());
auto factory = std::make_unique<Quic::QuicClientTransportSocketFactory>(std::move(config));
factory->initialize();
ASSERT_FALSE(factory->usesProxyProtocolOptions());
auto& matcher =
static_cast<Upstream::MockTransportSocketMatcher&>(*cluster_->transport_socket_matcher_);
EXPECT_CALL(matcher, resolve(_))
Expand Down
24 changes: 24 additions & 0 deletions test/common/quic/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,13 +249,37 @@ envoy_cc_test_library(
],
)

envoy_cc_test(
name = "client_connection_factory_impl_test",
srcs = ["client_connection_factory_impl_test.cc"],
tags = ["nofips"],
deps = [
"//source/common/event:dispatcher_lib",
"//source/common/http/http3:conn_pool_lib",
"//source/common/network:utility_lib",
"//source/common/upstream:upstream_includes",
"//source/common/upstream:upstream_lib",
"//test/common/http:common_lib",
"//test/common/upstream:utility_lib",
"//test/mocks/event:event_mocks",
"//test/mocks/http:http_mocks",
"//test/mocks/network:network_mocks",
"//test/mocks/runtime:runtime_mocks",
"//test/mocks/server:transport_socket_factory_context_mocks",
"//test/mocks/upstream:cluster_info_mocks",
"//test/mocks/upstream:transport_socket_match_mocks",
"//test/test_common:test_runtime_lib",
],
)

envoy_cc_test(
name = "quic_io_handle_wrapper_test",
srcs = ["quic_io_handle_wrapper_test.cc"],
tags = ["nofips"],
deps = [
"//source/common/quic:quic_io_handle_wrapper_lib",
"//test/mocks/api:api_mocks",
"//test/mocks/network:io_handle_mocks",
"//test/mocks/network:network_mocks",
"//test/test_common:threadsafe_singleton_injector_lib",
],
Expand Down
47 changes: 47 additions & 0 deletions test/common/quic/client_connection_factory_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
#include "common/quic/client_connection_factory_impl.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding these tests!

#include "common/quic/quic_transport_socket_factory.h"

#include "test/common/upstream/utility.h"
#include "test/mocks/common.h"
#include "test/mocks/event/mocks.h"
#include "test/mocks/server/transport_socket_factory_context.h"
#include "test/mocks/ssl/mocks.h"
#include "test/mocks/upstream/cluster_info.h"
#include "test/mocks/upstream/host.h"
#include "test/test_common/simulated_time_system.h"

namespace Envoy {
namespace Quic {

class QuicNetworkConnectionTest : public Event::TestUsingSimulatedTime, public testing::Test {
protected:
NiceMock<Event::MockDispatcher> dispatcher_;
std::shared_ptr<Upstream::MockClusterInfo> cluster_{new NiceMock<Upstream::MockClusterInfo>()};
Upstream::HostSharedPtr host_{new NiceMock<Upstream::MockHost>};
NiceMock<Random::MockRandomGenerator> random_;
Upstream::ClusterConnectivityState state_;
Network::Address::InstanceConstSharedPtr test_address_ =
Network::Utility::resolveUrl("tcp://127.0.0.1:3000");
NiceMock<Server::Configuration::MockTransportSocketFactoryContext> context_;
Quic::QuicClientTransportSocketFactory factory_{
std::unique_ptr<Envoy::Ssl::ClientContextConfig>(new NiceMock<Ssl::MockClientContextConfig>),
context_};
};

TEST_F(QuicNetworkConnectionTest, BufferLimits) {
PersistentQuicInfoImpl info{dispatcher_, factory_, simTime(), test_address_};

std::unique_ptr<Network::ClientConnection> client_connection =
createQuicNetworkConnection(info, dispatcher_, test_address_, test_address_);
EnvoyQuicClientSession* session = static_cast<EnvoyQuicClientSession*>(client_connection.get());
session->Initialize();
client_connection->connect();
EXPECT_TRUE(client_connection->connecting());
ASSERT(session != nullptr);
EXPECT_EQ(absl::nullopt, session->unixSocketPeerCredentials());
EXPECT_EQ(absl::nullopt, session->lastRoundTripTime());
client_connection->close(Network::ConnectionCloseType::NoFlush);
}

} // namespace Quic
} // namespace Envoy
81 changes: 81 additions & 0 deletions test/common/quic/quic_io_handle_wrapper_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,14 @@
#include "common/quic/quic_io_handle_wrapper.h"

#include "test/mocks/api/mocks.h"
#include "test/mocks/network/io_handle.h"
#include "test/mocks/network/mocks.h"
#include "test/test_common/threadsafe_singleton_injector.h"

#include "gmock/gmock.h"
#include "gtest/gtest.h"

using testing::ByMove;
using testing::Return;

namespace Envoy {
Expand Down Expand Up @@ -119,5 +121,84 @@ TEST_F(QuicIoHandleWrapperTest, DelegateIoHandleCalls) {
wrapper_->supportsMmsg();
}

TEST(QuicIoHandleWrapper, DelegateWithMocks) {
Network::MockIoHandle mock_io;
QuicIoHandleWrapper wrapper(mock_io);
Buffer::OwnedImpl buffer;
Event::MockDispatcher dispatcher;
Event::FileReadyCb cb;
Event::FileTriggerType trigger = Event::PlatformDefaultTriggerType;

{
EXPECT_CALL(mock_io, fdDoNotUse());
wrapper.fdDoNotUse();

EXPECT_CALL(mock_io, read(_, _))
.WillOnce(testing::Return(ByMove(Api::ioCallUint64ResultNoError())));
wrapper.read(buffer, 5);

EXPECT_CALL(mock_io, write(_)).WillOnce(Return(ByMove(Api::ioCallUint64ResultNoError())));
wrapper.write(buffer);

EXPECT_CALL(mock_io, recv(_, _, _)).WillOnce(Return(ByMove(Api::ioCallUint64ResultNoError())));
wrapper.recv(nullptr, 10, 0);

EXPECT_CALL(mock_io, bind(_)).WillOnce(Return(Api::SysCallIntResult{0, 0}));
wrapper.bind(nullptr);

EXPECT_CALL(mock_io, listen(_)).WillOnce(Return(Api::SysCallIntResult{0, 0}));
wrapper.listen(0);

EXPECT_CALL(mock_io, accept(_, _));
wrapper.accept(nullptr, nullptr);

EXPECT_CALL(mock_io, connect(_)).WillOnce(Return(Api::SysCallIntResult{0, 0}));
wrapper.connect(nullptr);

EXPECT_CALL(mock_io, setOption(_, _, _, _)).WillOnce(Return(Api::SysCallIntResult{0, 0}));
wrapper.setOption(0, 0, nullptr, 0);

EXPECT_CALL(mock_io, ioctl(_, _, _, _, _, _)).WillOnce(Return(Api::SysCallIntResult{0, 0}));
wrapper.ioctl(0, nullptr, 0, nullptr, 0, nullptr);

EXPECT_CALL(mock_io, setBlocking(_)).WillOnce(Return(Api::SysCallIntResult{0, 0}));
wrapper.setBlocking(false);

EXPECT_CALL(mock_io, createFileEvent_(_, _, _, _));
wrapper.initializeFileEvent(dispatcher, cb, trigger, 0);

EXPECT_CALL(mock_io, duplicate);
wrapper.duplicate();

EXPECT_CALL(mock_io, activateFileEvents(_));
wrapper.activateFileEvents(0);

EXPECT_CALL(mock_io, enableFileEvents(_));
wrapper.enableFileEvents(0);

EXPECT_CALL(mock_io, resetFileEvents());
wrapper.resetFileEvents();

EXPECT_CALL(mock_io, shutdown(_));
wrapper.shutdown(0);

EXPECT_CALL(mock_io, lastRoundTripTime()).Times(0);
wrapper.lastRoundTripTime();
}

wrapper.close();

{
EXPECT_CALL(mock_io, read(_, _)).Times(0);
wrapper.read(buffer, 5);

EXPECT_CALL(mock_io, write(_)).Times(0);
wrapper.write(buffer);

EXPECT_CALL(mock_io, recv(_, _, _)).Times(0);
ASSERT_DEBUG_DEATH(wrapper.recv(nullptr, 10, 0), "recv called after close");
}
}

} // namespace Quic
} // namespace Envoy