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

Jump to bottom

mobile fetch client appears to function differently with oghttp2 vs nghttp2 #34693

Closed
alyssawilk opened this issue Jun 11, 2024 · 3 comments · Fixed by #34801
Closed

mobile fetch client appears to function differently with oghttp2 vs nghttp2 #34693

alyssawilk opened this issue Jun 11, 2024 · 3 comments · Fixed by #34801
Assignees
@birenroy
Labels
bug

Comments

@alyssawilk
Copy link
Contributor

cd mobile
blaze test -c dbg //test/cc/unit:fetch_client_test
Will send logs internally
h/t @fredyw
@alyssawilk alyssawilk added the bug label Jun 11, 2024
@fredyw
Copy link
Member

fredyw commented Jun 12, 2024 •
edited

To reproduce

bazel test -c dbg --test_filter=FetchClientTest.Http2 //test/cc/unit:fetch_client_test

With nghttp2 (passed):
Diff:

diff --git a/mobile/examples/cc/fetch_client/fetch_client.cc b/mobile/examples/cc/fetch_client/fetch_client.cc
index b81f6dc38f..3ec6e2aca3 100644
--- a/mobile/examples/cc/fetch_client/fetch_client.cc
+++ b/mobile/examples/cc/fetch_client/fetch_client.cc
@@ -117,6 +117,7 @@ envoy_status_t Fetch::sendRequest(absl::string_view url_string) {
 void Fetch::runEngine(absl::Notification& engine_running,
                       const std::vector<absl::string_view>& quic_hints) {
   Platform::EngineBuilder engine_builder;
+  engine_builder.setRuntimeGuard("http2_use_oghttp2", false);
   engine_builder.setLogLevel(Logger::Logger::debug);
   engine_builder.setOnEngineRunning([&engine_running]() { engine_running.Notify(); });
   if (!quic_hints.empty()) {
diff --git a/mobile/test/cc/unit/fetch_client_test.cc b/mobile/test/cc/unit/fetch_client_test.cc
index 6baa5abdd6..621b3ccca4 100644
--- a/mobile/test/cc/unit/fetch_client_test.cc
+++ b/mobile/test/cc/unit/fetch_client_test.cc
@@ -14,7 +14,7 @@ namespace {
 TEST(FetchClientTest, Http2) {
   Envoy::Fetch client;
   // TODO(fredyw): Connecting to www.google.com is currently broken with oghttp2.
-  ASSERT_EQ(client.fetch({"https://www.example.com/"}), ENVOY_SUCCESS);
+  ASSERT_EQ(client.fetch({"https://www.google.com/"}), ENVOY_SUCCESS);
 }
 
 TEST(FetchClientTest, Http3) {

With oghttp2 (failed):
Diff:

diff --git a/mobile/examples/cc/fetch_client/fetch_client.cc b/mobile/examples/cc/fetch_client/fetch_client.cc
index b81f6dc38f..327abd61b7 100644
--- a/mobile/examples/cc/fetch_client/fetch_client.cc
+++ b/mobile/examples/cc/fetch_client/fetch_client.cc
@@ -117,6 +117,7 @@ envoy_status_t Fetch::sendRequest(absl::string_view url_string) {
 void Fetch::runEngine(absl::Notification& engine_running,
                       const std::vector<absl::string_view>& quic_hints) {
   Platform::EngineBuilder engine_builder;
+  engine_builder.setRuntimeGuard("http2_use_oghttp2", true);
   engine_builder.setLogLevel(Logger::Logger::debug);
   engine_builder.setOnEngineRunning([&engine_running]() { engine_running.Notify(); });
   if (!quic_hints.empty()) {
diff --git a/mobile/test/cc/unit/fetch_client_test.cc b/mobile/test/cc/unit/fetch_client_test.cc
index 6baa5abdd6..621b3ccca4 100644
--- a/mobile/test/cc/unit/fetch_client_test.cc
+++ b/mobile/test/cc/unit/fetch_client_test.cc
@@ -14,7 +14,7 @@ namespace {
 TEST(FetchClientTest, Http2) {
   Envoy::Fetch client;
   // TODO(fredyw): Connecting to www.google.com is currently broken with oghttp2.
-  ASSERT_EQ(client.fetch({"https://www.example.com/"}), ENVOY_SUCCESS);
+  ASSERT_EQ(client.fetch({"https://www.google.com/"}), ENVOY_SUCCESS);
 }
 
 TEST(FetchClientTest, Http3) {

copybara-service bot pushed a commit to google/quiche that referenced this issue Jun 12, 2024
@birenroy
Adds test cases for OgHttp2Adapter as a client receiving a response w…
4421cfd 
…ith Content-Length and padding.

Demonstrates the problem seen in envoyproxy/envoy#34693.

PiperOrigin-RevId: 642720946
copybara-service bot pushed a commit to google/quiche that referenced this issue Jun 12, 2024
@birenroy
Fixes handling for DATA frames with padding in combination with Conte…
5274ee8 
…nt-Length.

Resolves the issue seen in envoyproxy/envoy#34693.

PiperOrigin-RevId: 642728807
@birenroy
Copy link
Contributor

This should be resolved when Envoy imports google/quiche@5274ee8.

@birenroy
Copy link
Contributor

Verified with this patch:

$ git diff bazel/repository_locations.bzl
diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl
index a675c33859..0999094d84 100644
--- a/bazel/repository_locations.bzl
+++ b/bazel/repository_locations.bzl
@@ -1192,8 +1192,8 @@ 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 = "c5d1ed730f6062c6647aebe130d78f088028e52f",
-        sha256 = "d4d61c3189d989d7e51323823ce6f9de6970a09803624e80283e9f8135c9fe4b",
+        version = "5274ee87f052cabcc5f94390dc9c7a7dd127b946",
+        sha256 = "17cd93837ed20c62096fc9839fbcb943febc6a8411392754981a84d6e3ff061e",
         urls = ["https://github.com/google/quiche/archive/{version}.tar.gz"],
         strip_prefix = "quiche-{version}",
         use_category = ["controlplane", "dataplane_core"],

Ran the test:

$ baze test --test_filter='*Http2*' //test/cc/unit:fetch_client_test --test_arg=-- --test_arg="-l trace"
Starting local Bazel server and connecting to it...
INFO: Invocation ID: 8565fd7f-6c04-461b-aaf7-7f1d82acea42
INFO: Analyzed target //test/cc/unit:fetch_client_test (584 packages loaded, 32125 targets configured).
INFO: Found 1 test target...
Target //test/cc/unit:fetch_client_test up-to-date:
  bazel-bin/test/cc/unit/fetch_client_test
INFO: Elapsed time: 77.306s, Critical Path: 56.68s
INFO: 222 processes: 1 internal, 221 remote.
INFO: Build completed successfully, 222 total actions

Executed 1 out of 1 test: 1 test passes.

@birenroy birenroy mentioned this issue Jun 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@birenroy birenroy

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Update QUICHE from c5d1ed730 to 134dcbea6 birenroy/envoy
3 participants
@fredyw @birenroy @alyssawilk