Skip to content

Windows: Fix quiche test compilation #12898

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
mattklein123 merged 16 commits into from
Sep 25, 2020
Merged

Windows: Fix quiche test compilation #12898

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
14398f0
Windows: Fix quiche test compilation
sunjayBhatia Aug 31, 2020
44430ed
Drop batched write test until PR #13086 makes this platform agnostic
wrowe Sep 21, 2020
e99d52d
Merge remote-tracking branch 'origin/master' into windows-fix-quiche-…
wrowe Sep 21, 2020
bbcc905
Guard #pragma GCC with correct test
wrowe Sep 22, 2020
16b4768
Merge remote-tracking branch 'origin/master' into windows-fix-quiche-…
wrowe Sep 22, 2020
567765b
Fix Format
wrowe Sep 22, 2020
4496617
Fix new quiche breakage on windows
sunjayBhatia Sep 22, 2020
0d13c26
Resolve last build quirk in linux
sunjayBhatia Sep 22, 2020
2df2ef9
Fix format
sunjayBhatia Sep 22, 2020
ba3b31f
Merge remote-tracking branch 'origin/master' into windows-fix-quiche-…
wrowe Sep 22, 2020
761b537
Merge remote-tracking branch 'origin/master' into windows-fix-quiche-…
wrowe Sep 24, 2020
8e19e61
Replace header lost in merge master
wrowe Sep 25, 2020
0403ba7
udp_gso headers are now guarded with #ifdef __linux__
wrowe Sep 25, 2020
f9c1f01
Correct udp_gso header guard
wrowe Sep 25, 2020
ab26231
Enable test to demonstrate windows failures once
wrowe Sep 25, 2020
7672f9b
Changes to pass CI for merge
sunjayBhatia Sep 25, 2020
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
4 changes: 0 additions & 4 deletions source/extensions/quic_listeners/quiche/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,13 +375,9 @@ envoy_cc_extension(
# QUICHE can't build against FIPS BoringSSL until the FIPS build
# is on a new enough version to have QUIC support. Remove it from
# the build until then. Re-enable as part of #7433.
#
# QUICHE doesn't compile on Windows right now. Disable until it
# is fixed.
deps = select({
"//bazel:boringssl_fips": [],
"//bazel:boringssl_disabled": [],
"//bazel:windows_x86_64": [],
"//conditions:default": [
":active_quic_listener_config_lib",
":codec_lib",
Expand Down
3 changes: 2 additions & 1 deletion source/extensions/quic_listeners/quiche/envoy_quic_client_connection.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ void EnvoyQuicClientConnection::setUpConnectionSocket() {
if (connectionSocket()->ioHandle().isOpen()) {
file_event_ = connectionSocket()->ioHandle().createFileEvent(
dispatcher_, [this](uint32_t events) -> void { onFileEvent(events); },
Event::FileTriggerType::Edge, Event::FileReadyType::Read | Event::FileReadyType::Write);
Event::PlatformDefaultTriggerType,
Event::FileReadyType::Read | Event::FileReadyType::Write);

if (!Network::Socket::applyOptions(connectionSocket()->options(), *connectionSocket(),
envoy::config::core::v3::SocketOption::STATE_LISTENING)) {
Expand Down
60 changes: 10 additions & 50 deletions test/extensions/quic_listeners/quiche/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,7 @@ envoy_cc_test(
name = "envoy_quic_writer_test",
srcs = ["envoy_quic_writer_test.cc"],
external_deps = ["quiche_quic_platform"],
# Skipping as quiche quic_stream_send_buffer.cc does not currently compile on Windows
tags = [
"nofips",
"skip_on_windows",
],
tags = ["nofips"],
deps = [
"//source/common/network:io_socket_error_lib",
"//source/common/network:udp_packet_writer_handler_lib",
Expand Down Expand Up @@ -74,11 +70,7 @@ envoy_cc_test(
envoy_cc_test(
name = "envoy_quic_server_stream_test",
srcs = ["envoy_quic_server_stream_test.cc"],
# Skipping as quiche quic_stream_send_buffer.cc does not currently compile on Windows
tags = [
"nofips",
"skip_on_windows",
],
tags = ["nofips"],
deps = [
":quic_test_utils_for_envoy_lib",
":test_utils_lib",
Expand All @@ -99,11 +91,7 @@ envoy_cc_test(
envoy_cc_test(
name = "envoy_quic_client_stream_test",
srcs = ["envoy_quic_client_stream_test.cc"],
# Skipping as quiche quic_stream_send_buffer.cc does not currently compile on Windows
tags = [
"nofips",
"skip_on_windows",
],
tags = ["nofips"],
deps = [
":quic_test_utils_for_envoy_lib",
":test_utils_lib",
Expand All @@ -123,11 +111,7 @@ envoy_cc_test(
envoy_cc_test(
name = "envoy_quic_server_session_test",
srcs = ["envoy_quic_server_session_test.cc"],
# Skipping as quiche quic_stream_send_buffer.cc does not currently compile on Windows
tags = [
"nofips",
"skip_on_windows",
],
tags = ["nofips"],
deps = [
":quic_test_utils_for_envoy_lib",
":test_proof_source_lib",
Expand Down Expand Up @@ -156,11 +140,7 @@ envoy_cc_test(
envoy_cc_test(
name = "envoy_quic_client_session_test",
srcs = ["envoy_quic_client_session_test.cc"],
# Skipping as quiche quic_stream_send_buffer.cc does not currently compile on Windows
tags = [
"nofips",
"skip_on_windows",
],
tags = ["nofips"],
deps = [
":quic_test_utils_for_envoy_lib",
":test_utils_lib",
Expand All @@ -182,11 +162,7 @@ envoy_cc_test(
envoy_cc_test(
name = "active_quic_listener_test",
srcs = ["active_quic_listener_test.cc"],
# Skipping as quiche quic_stream_send_buffer.cc does not currently compile on Windows
tags = [
"nofips",
"skip_on_windows",
],
tags = ["nofips"],
deps = [
":quic_test_utils_for_envoy_lib",
":test_utils_lib",
Expand All @@ -207,11 +183,7 @@ envoy_cc_test(
envoy_cc_test(
name = "envoy_quic_dispatcher_test",
srcs = ["envoy_quic_dispatcher_test.cc"],
# quic_sent_packet_manager.cc does not compile error: warning C4715: 'quic::QuicSentPacketManager::OnRetransmissionTimeout': not all control paths return a value
tags = [
"nofips",
"skip_on_windows",
],
tags = ["nofips"],
deps = [
":quic_test_utils_for_envoy_lib",
":test_proof_source_lib",
Expand Down Expand Up @@ -278,11 +250,7 @@ envoy_cc_test(
envoy_cc_test(
name = "envoy_quic_utils_test",
srcs = ["envoy_quic_utils_test.cc"],
# Skipping as quiche quic_stream_send_buffer.cc does not currently compile on Windows
tags = [
"nofips",
"skip_on_windows",
],
tags = ["nofips"],
deps = [
":quic_test_utils_for_envoy_lib",
"//source/extensions/quic_listeners/quiche:envoy_quic_utils_lib",
Expand All @@ -294,11 +262,7 @@ envoy_cc_test(
envoy_cc_test(
name = "active_quic_listener_config_test",
srcs = ["active_quic_listener_config_test.cc"],
# Skipping as quiche quic_stream_send_buffer.cc does not currently compile on Windows
tags = [
"nofips",
"skip_on_windows",
],
tags = ["nofips"],
deps = [
"//source/common/config:utility_lib",
"//source/extensions/quic_listeners/quiche:active_quic_listener_config_lib",
Expand All @@ -309,11 +273,7 @@ envoy_cc_test(
envoy_cc_test(
name = "envoy_quic_simulated_watermark_buffer_test",
srcs = ["envoy_quic_simulated_watermark_buffer_test.cc"],
# Skipping as quiche quic_stream_send_buffer.cc does not currently compile on Windows
tags = [
"nofips",
"skip_on_windows",
],
tags = ["nofips"],
deps = ["//source/extensions/quic_listeners/quiche:envoy_quic_simulated_watermark_buffer_lib"],
)

Expand Down
12 changes: 11 additions & 1 deletion test/extensions/quic_listeners/quiche/active_quic_listener_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "common/common/logger.h"
#include "common/network/listen_socket_impl.h"
#include "common/network/socket_option_factory.h"
#include "common/network/udp_packet_writer_handler_impl.h"
#include "extensions/quic_listeners/quiche/active_quic_listener.h"
#include "test/extensions/quic_listeners/quiche/test_utils.h"
#include "test/extensions/quic_listeners/quiche/test_proof_source.h"
Expand All @@ -42,8 +43,11 @@
#include "gmock/gmock.h"
#include "extensions/quic_listeners/quiche/active_quic_listener_config.h"
#include "extensions/quic_listeners/quiche/platform/envoy_quic_clock.h"
#include "extensions/quic_listeners/quiche/envoy_quic_packet_writer.h"
#include "extensions/quic_listeners/quiche/envoy_quic_utils.h"
#ifndef WIN32
#include "extensions/quic_listeners/quiche/udp_gso_batch_writer.h"
#endif

@mattklein123 mattklein123 Sep 21, 2020

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.

I thought @ggreenway had fixed this so we don't need ifdef for this anymore, at least in the main code. Is there any way to avoid the ifdef in this test?

@wrowe wrowe Sep 21, 2020

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.

IIUC this specific test does validate udp_gso_batch_writer by name, not the generic wrapper which is windows agnostic, but I could be wrong; @ggreenway ?

@mattklein123 mattklein123 Sep 21, 2020

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.

Optimally we would not have this type of ifdef/dependency in the test. IMO I would just remove it entirely and do whatever you did on Windows. In the interest of moving this forward I would be fine with a TODO and a follow up?

@ggreenway ggreenway Sep 21, 2020

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.

The change that removed the need for this ifdef isn't merged yet. It's #13086

@wrowe wrowe Sep 21, 2020

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.

In an effort to also see that 13086 doesn't add more regressions to work around under windows, lets get it running under CI. Since that PR will simplify this particular test, does the new workaround above satisfy everyone?

@wrowe wrowe Sep 21, 2020

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.

(Modulo an error I've introduced and need to fix)

/wait

@wrowe wrowe Sep 22, 2020

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.

Fix should be in... waiting on CI confirmation

@wrowe wrowe Sep 25, 2020

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.

This is resolved. Note the introduction of new build failures. Because these are batch writer related, we are marking that test failing on windows, for the time being, as it's a regression in the most recent batch writer refactoring. Let's get this merged to avoid future breakage and we'll need to unwind the two broken tests.


using testing::Return;
using testing::ReturnRef;
Expand Down Expand Up @@ -111,15 +115,21 @@ class ActiveQuicListenerTest : public QuicMultiVersionTest {
ON_CALL(listener_config_, listenSocketFactory()).WillByDefault(ReturnRef(socket_factory_));
ON_CALL(socket_factory_, getListenSocket()).WillByDefault(Return(listen_socket_));

// Use UdpGsoBatchWriter to perform non-batched writes for the purpose of this test
ON_CALL(listener_config_, udpPacketWriterFactory())
.WillByDefault(Return(
std::reference_wrapper<Network::UdpPacketWriterFactory>(udp_packet_writer_factory_)));
ON_CALL(udp_packet_writer_factory_, createUdpPacketWriter(_, _))
.WillByDefault(Invoke(
[&](Network::IoHandle& io_handle, Stats::Scope& scope) -> Network::UdpPacketWriterPtr {
#ifndef WIN32
// Use UdpGsoBatchWriter to perform non-batched writes for the purpose of this test
Network::UdpPacketWriterPtr udp_packet_writer =
std::make_unique<Quic::UdpGsoBatchWriter>(io_handle, scope);
#else
// UdpGsoBatchWriter cannot be supported on Windows, use UdpDefaultWriter instead
Network::UdpPacketWriterPtr udp_packet_writer =
std::make_unique<Network::UdpDefaultWriter>(io_handle);

@ggreenway ggreenway Sep 22, 2020

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.

The need for this change will go away after #13086 merges.

@danzh2010 what was the reason for using the GSO batch writer in this test instead of the default writer? Coverage?

@mattklein123 mattklein123 Sep 22, 2020

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.

Let's wait for the other PR to merge and then come back to this. It should merge soon.

/wait

@danzh2010 danzh2010 Sep 23, 2020

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.

The need for this change will go away after #13086 merges.

@danzh2010 what was the reason for using the GSO batch writer in this test instead of the default writer? Coverage?

Just to test more the interaction between the batch writer and QUIC listener because mostly likely the batch writer will be used in production.

#endif
return udp_packet_writer;
}));

Expand Down
2 changes: 1 addition & 1 deletion test/extensions/quic_listeners/quiche/envoy_quic_utils_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ TEST(EnvoyQuicUtilsTest, ConversionBetweenQuicAddressAndEnvoyAddress) {
// Mock out socket() system call to test both V4 and V6 address conversion.
testing::NiceMock<Envoy::Api::MockOsSysCalls> os_sys_calls;
TestThreadsafeSingletonInjector<Envoy::Api::OsSysCallsImpl> os_calls{&os_sys_calls};
ON_CALL(os_sys_calls, socket(_, _, _)).WillByDefault(Return(Api::SysCallIntResult{1, 0}));
ON_CALL(os_sys_calls, socket(_, _, _)).WillByDefault(Return(Api::SysCallSocketResult{1, 0}));
ON_CALL(os_sys_calls, close(_)).WillByDefault(Return(Api::SysCallIntResult{0, 0}));

quic::QuicSocketAddress quic_uninitialized_addr;
Expand Down
2 changes: 1 addition & 1 deletion test/extensions/quic_listeners/quiche/envoy_quic_writer_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class EnvoyQuicWriterTest : public ::testing::Test {
quic::QuicIpAddress peer_ip;
peer_ip.FromString("::1");
peer_address_ = quic::QuicSocketAddress(peer_ip, /*port=*/123);
ON_CALL(os_sys_calls_, socket(_, _, _)).WillByDefault(Return(Api::SysCallIntResult{3, 0}));
ON_CALL(os_sys_calls_, socket(_, _, _)).WillByDefault(Return(Api::SysCallSocketResult{3, 0}));
ON_CALL(os_sys_calls_, close(3)).WillByDefault(Return(Api::SysCallIntResult{0, 0}));
}

Expand Down
3 changes: 1 addition & 2 deletions test/extensions/quic_listeners/quiche/integration/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,9 @@ envoy_cc_test(
size = "medium",
srcs = ["quic_http_integration_test.cc"],
data = ["//test/config/integration/certs"],
# Skipping as quiche quic_stream_send_buffer.cc does not currently compile on Windows
tags = [
"fails_on_windows",

@danzh2010 danzh2010 Sep 21, 2020

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.

I'm curious which test fails on Windows. Can you file an issue about the test failure?

@wrowe wrowe Sep 25, 2020

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.

Yup (actually, now two failures since the last merge) #13273

"nofips",
"skip_on_windows",
],
deps = [
"//source/extensions/filters/http/dynamo:config",
Expand Down
4 changes: 2 additions & 2 deletions test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,8 +344,8 @@ TEST_P(QuicHttpIntegrationTest, TestDelayedConnectionTeardownTimeoutTrigger) {
1);
}

TEST_P(QuicHttpIntegrationTest, MultipleQuicListenersWithBPF) {
#if defined(SO_ATTACH_REUSEPORT_CBPF) && defined(__linux__)
TEST_P(QuicHttpIntegrationTest, MultipleQuicListenersWithBPF) {
concurrency_ = 8;
set_reuse_port_ = true;
initialize();
Expand Down Expand Up @@ -379,8 +379,8 @@ TEST_P(QuicHttpIntegrationTest, MultipleQuicListenersWithBPF) {
for (size_t i = 0; i < concurrency_; ++i) {
codec_clients[i]->close();
}
#endif
}
#endif

#ifndef __APPLE__
TEST_P(QuicHttpIntegrationTest, MultipleQuicListenersNoBPF) {
Expand Down
3 changes: 2 additions & 1 deletion test/server/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,8 @@ envoy_cc_test(
srcs = ["listener_manager_impl_quic_only_test.cc"],
tags = [
"nofips",
# Skipping as quiche quic_stream_send_buffer.cc does not currently compile on Windows
# Skipping as quiche quic_gso_batch_writer.h does not exist on Windows
# required by quic_stream_send_buffer.cc
"skip_on_windows",
],
deps = [
Expand Down