Skip to content

quiche: update QUICHE to 4f552f349b8df000af24bc6cfa0b78fdc2467fef #19013

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
alyssawilk merged 4 commits into from
Nov 17, 2021
Merged

quiche: update QUICHE to 4f552f349b8df000af24bc6cfa0b78fdc2467fef #19013

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
aa209b9
Update QUICHE from 0b75841d5 to 4f552f349
danzh1989 Nov 15, 2021
9c91a4b
fix build
danzh1989 Nov 16, 2021
6926d61
bring back patch
danzh1989 Nov 16, 2021
89678b5
Merge branch 'master' into updatetar19
danzh1989 Nov 16, 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: 13 additions & 0 deletions bazel/external/quiche.BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,18 @@ envoy_cc_test_library(
deps = [":http2_platform"],
)

envoy_cc_library(
name = "http2_event_forwarder",
srcs = ["quiche/http2/adapter/event_forwarder.cc"],
hdrs = ["quiche/http2/adapter/event_forwarder.h"],
copts = quiche_copts,
repository = "@envoy",
deps = [
":quiche_common_platform_export",
":spdy_core_http2_deframer_lib",
],
)

envoy_cc_library(
name = "http2_adapter",
srcs = [
Expand Down Expand Up @@ -129,6 +141,7 @@ envoy_cc_library(
deps = [
":http2_core_http2_trace_logging_lib",
":http2_core_priority_write_scheduler_lib",
":http2_event_forwarder",
":spdy_core_framer_lib",
":spdy_core_header_block_lib",
":spdy_core_http2_deframer_lib",
Expand Down
42 changes: 1 addition & 41 deletions bazel/external/quiche.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,31 +67,14 @@
#include "absl/types/span.h"
#include "http2/adapter/data_source.h"
#include "http2/adapter/http2_protocol.h"
#include "http2/adapter/http2_visitor_interface.h"
Copy link
Copy Markdown
Contributor

@RyanTheOptimist RyanTheOptimist Nov 16, 2021

Choose a reason for hiding this comment

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

We're planning to back-port these patches to QUICHE, right?

Copy link
Copy Markdown
Contributor Author

@danzh2010 danzh2010 Nov 16, 2021

Choose a reason for hiding this comment

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

This was added in QUICHE so I needed to adjust the patch lines here.

Copy link
Copy Markdown
Contributor

@RyanTheOptimist RyanTheOptimist Nov 16, 2021

Choose a reason for hiding this comment

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

nod but I mean all of the diffs in this file. For example, are we planning to back-port the ssize_t typdef, the NO_ERROR and the default returns?

Copy link
Copy Markdown
Contributor

@DavidSchinazi DavidSchinazi Nov 16, 2021

Choose a reason for hiding this comment

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

+1, why not simply ban use of ssize_t in QUICHE via a presubmit?

Copy link
Copy Markdown
Contributor

@RyanTheOptimist RyanTheOptimist Nov 16, 2021

Choose a reason for hiding this comment

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

IIRC, it's used by nghttp somewhere that oghttp has to overload (or some such).

Copy link
Copy Markdown
Contributor

@birenroy birenroy Nov 16, 2021

Choose a reason for hiding this comment

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

Yeah, nghttp2 uses ssize_t. Envoy actually has a similar workaround in envoy/common/platform.h.

Copy link
Copy Markdown
Contributor

@birenroy birenroy Nov 16, 2021

Choose a reason for hiding this comment

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

I'm working on back-porting the fixes in the patch file. Some of these patches will be unnecessary in the next QUICHE update.

Copy link
Copy Markdown
Contributor

@RyanTheOptimist RyanTheOptimist Nov 16, 2021

Choose a reason for hiding this comment

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

Awesome, that's great to hear!

+
+// Required to build on Windows.
+typedef ptrdiff_t ssize_t;
+
#include "third_party/nghttp2/src/lib/includes/nghttp2/nghttp2.h"
#include "spdy/core/spdy_header_block.h"

--- http2/adapter/oghttp2_session.cc
+++ http2/adapter/oghttp2_session.cc
@@ -131,6 +131,7 @@ absl::string_view TracePerspectiveAsStri
case Perspective::kServer:
return "OGHTTP2_SERVER";
}
+ return "OGHTTP2_SERVER";
}

class RunOnExit {
@@ -186,6 +187,7 @@ Http2ErrorCode GetHttp2ErrorCode(SpdyFra
case SpdyFramerError::LAST_ERROR:
return Http2ErrorCode::INTERNAL_ERROR;
}
+ return Http2ErrorCode::INTERNAL_ERROR;
}

} // namespace
--- http2/adapter/http2_util.cc
+++ http2/adapter/http2_util.cc
@@ -1,5 +1,7 @@
Expand All @@ -102,30 +85,7 @@
namespace http2 {
namespace adapter {
namespace {
@@ -42,6 +44,7 @@ spdy::SpdyErrorCode TranslateErrorCode(H
case Http2ErrorCode::HTTP_1_1_REQUIRED:
return spdy::ERROR_CODE_HTTP_1_1_REQUIRED;
}
+ return spdy::ERROR_CODE_INTERNAL_ERROR;
}

Http2ErrorCode TranslateErrorCode(spdy::SpdyErrorCode code) {
@@ -76,6 +77,7 @@ Http2ErrorCode TranslateErrorCode(spdy::
case spdy::ERROR_CODE_HTTP_1_1_REQUIRED:
return Http2ErrorCode::HTTP_1_1_REQUIRED;
}
+ return Http2ErrorCode::INTERNAL_ERROR;
}

absl::string_view ConnectionErrorToString(ConnectionError error) {
@@ -95,6 +96,7 @@ absl::string_view ConnectionErrorToStrin
case ConnectionError::kInvalidPushPromise:
return "InvalidPushPromise";
}
+ return "UnknownConnectionError";
}

} // namespace adapter
--- http2/adapter/oghttp2_session.cc
+++ http2/adapter/oghttp2_session.cc
@@ -11,6 +11,8 @@
Expand Down
6 changes: 3 additions & 3 deletions bazel/repository_locations.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -838,12 +838,12 @@ REPOSITORY_LOCATIONS_SPEC = dict(
project_name = "QUICHE",
project_desc = "QUICHE (QUIC, HTTP/2, Etc) is Google‘s implementation of QUIC and related protocols",
project_url = "https://github.com/google/quiche",
version = "0b75841d5b5844c53f4399a41d64de7250c204d8",
sha256 = "a00b0671180fc79952baf754148e65364bfca9d35b988710594752fb7f9bf6a1",
version = "4f552f349b8df000af24bc6cfa0b78fdc2467fef",
sha256 = "355c80803698d2a44363ed304d250059cc5d7712281481803717f8d779229bd8",
urls = ["https://github.com/google/quiche/archive/{version}.tar.gz"],
strip_prefix = "quiche-{version}",
use_category = ["dataplane_core"],
release_date = "2021-11-02",
release_date = "2021-11-15",
cpe = "N/A",
),
com_googlesource_googleurl = dict(
Expand Down
4 changes: 2 additions & 2 deletions test/common/quic/envoy_quic_dispatcher_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ class EnvoyQuicDispatcherTest : public testing::TestWithParam<Network::Address::
EXPECT_TRUE(session->IsEncryptionEstablished());
EXPECT_EQ(1u, connection_handler_.numConnections());
auto envoy_connection = static_cast<const EnvoyQuicServerSession*>(session);
EXPECT_EQ("test.example.org", envoy_connection->requestedServerName());
EXPECT_EQ("test.example.com", envoy_connection->requestedServerName());
EXPECT_EQ(peer_addr, envoyIpAddressToQuicSocketAddress(
envoy_connection->connectionInfoProvider().remoteAddress()->ip()));
ASSERT(envoy_connection->connectionInfoProvider().localAddress() != nullptr);
Expand Down Expand Up @@ -190,7 +190,7 @@ class EnvoyQuicDispatcherTest : public testing::TestWithParam<Network::Address::
EXPECT_CALL(filter_chain_manager, findFilterChain(_))
.WillOnce(Invoke([this](const Network::ConnectionSocket& socket) {
EXPECT_EQ("h3", socket.requestedApplicationProtocols()[0]);
EXPECT_EQ("test.example.org", socket.requestedServerName());
EXPECT_EQ("test.example.com", socket.requestedServerName());
return &proof_source_->filterChain();
}));
Network::MockTransportSocketFactory transport_socket_factory;
Expand Down
7 changes: 4 additions & 3 deletions test/common/quic/envoy_quic_session_cache_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,10 @@ std::vector<uint8_t> createFakeStatelessResetToken() {
std::unique_ptr<quic::TransportParameters> makeFakeTransportParams() {
auto params = std::make_unique<quic::TransportParameters>();
params->perspective = quic::Perspective::IS_CLIENT;
params->version = FakeVersionLabel;
params->supported_versions.push_back(FakeVersionLabel);
params->supported_versions.push_back(FakeVersionLabel2);
params->legacy_version_information = quic::TransportParameters::LegacyVersionInformation();
Copy link
Copy Markdown
Contributor

@RyanTheOptimist RyanTheOptimist Nov 16, 2021

Choose a reason for hiding this comment

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

Oh, interesting. I'm slightly surprised that this is required because I thought it was optional. But I guess it's going to be optional eventually, just not yet?

Copy link
Copy Markdown
Contributor Author

@danzh2010 danzh2010 Nov 16, 2021

Choose a reason for hiding this comment

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

It is optional indeed. But this test uses this field.

Copy link
Copy Markdown
Contributor

@DavidSchinazi DavidSchinazi Nov 16, 2021

Choose a reason for hiding this comment

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

legacy_version_information is absl::optional but our code (currently) requires it to be present - we'll remove it at a later date. @danzh2010's change is the correct one here.

params->legacy_version_information->version = FakeVersionLabel;
params->legacy_version_information->supported_versions.push_back(FakeVersionLabel);
params->legacy_version_information->supported_versions.push_back(FakeVersionLabel2);
params->max_idle_timeout_ms.set_value(FakeIdleTimeoutMilliseconds);
params->stateless_reset_token = createFakeStatelessResetToken();
params->max_udp_payload_size.set_value(FakeMaxPacketSize);
Expand Down