http3: Add support for HTTP/3 METADATA by RyanTheOptimist · Pull Request #32568 · envoyproxy/envoy

RyanTheOptimist · 2024-02-26T00:43:30Z

http3: Add support for HTTP/3 METADATA

Adds a new allow_metadata option to Http3ProtocolOptions.

Risk Level: Low, protected by new config option
Testing: New integration tests
Docs Changes: N/A
Release Notes: Updated

repokitteh-read-only · 2024-02-26T00:43:40Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @abeyad
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #32568 was opened by RyanTheOptimist.

see: more, trace.

abeyad

/lgtm api

RyanTheOptimist · 2024-02-26T01:11:01Z

Sorry! I keep forgetting that Envoy does automatic assignment. I think the code in this PR is basically ready for review, but it depends on some un-commited QUICHE changes that I manually passed in so it won't yet pass CI

Signed-off-by: Ryan Hamilton <rch@google.com> Cleanup Signed-off-by: Ryan Hamilton <rch@google.com> Reduce code duplication Signed-off-by: Ryan Hamilton <rch@google.com> Changelog Signed-off-by: Ryan Hamilton <rch@google.com> Better release notes Signed-off-by: Ryan Hamilton <rch@google.com> More cleanup Signed-off-by: Ryan Hamilton <rch@google.com> Even more cleanup Signed-off-by: Ryan Hamilton <rch@google.com> Cleanup! Cleanup Signed-off-by: Ryan Hamilton <rch@google.com>

Signed-off-by: Ryan Hamilton <rch@google.com>

repokitteh-read-only · 2024-02-29T23:25:38Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @mattklein123

🐱

Caused by: #32568 was synchronize by RyanTheOptimist.

see: more, trace.

RyanTheOptimist · 2024-02-29T23:27:03Z

/assign @danzh2010

Ok, this is ready for review, I hope. There's a QUICHE update in this as well, but I'll land that separately once #32642 lands.

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist · 2024-03-02T15:13:39Z

Now that #32650 has landed, this PR does not change deps, so I'll remove that label.

danzh2010 · 2024-03-06T15:33:58Z

+#ifdef ENVOY_ENABLE_QUIC
+  EXPECT_TRUE(GetQuicReloadableFlag(quic_enable_http3_metadata_decoding));
+#else
+  EXCLUDE_DOWNSTREAM_HTTP3; // The HTTP/3 client has no "bad frame" equivalent.


The comment seems irrelevant.

Whoops. Thanks.

danzh2010 · 2024-03-06T15:44:51Z

+      result = quic_stream_.WriteMemSlices(metadata_frame, /*end_stream=*/false);
+    }
+    // QUIC stream must take all.
+    if (result.bytes_consumed == 0) {


Can this be true if the metadata passed in is empty?

No, because the METADATA frame header will always be present.

Signed-off-by: Ryan Hamilton <rch@google.com>

alyssawilk

Nice addition!
/wait on comments

alyssawilk · 2024-03-06T16:09:38Z

  return dynamic_cast<QuicFilterManagerConnectionImpl*>(session());
 }

+void EnvoyQuicClientStream::OnMetadataComplete(size_t /*frame_len*/,


in H3, can metadata arrive before headers? I think it's an invariant of Envoy that headers arrive first and I'm not sure if it's an invariant of the quiche libraries.

METADATA is an HTTP/3 frame which is delivered on the QUIC stream, so it's guaranteed to be delivered in the order it was sent. I think this is true for HTTP/2 as well. But if a peer sent a METADATA frame before sending HEADERS I think QUICHE will expose it in that order. I think that's also the same for HTTP/2. But we could make this an error of some here, if you prefer?

I think we should either make it an error, or add an e2e test it doesn't cause problems in Envoy.

Oh! I think we have a test for this already! ProxyMetadataInResponse does:

// Sends metadata before response header. const std::string key = "key"; std::string value = std::string(80 * 1024, '1'); Http::MetadataMap metadata_map = {{key, value}}; Http::MetadataMapPtr metadata_map_ptr = std::make_unique<Http::MetadataMap>(metadata_map); Http::MetadataMapVector metadata_map_vector; metadata_map_vector.push_back(std::move(metadata_map_ptr)); upstream_request_->encodeMetadata(metadata_map_vector); upstream_request_->encodeHeaders(default_response_headers_, false); upstream_request_->encodeData(12, true);

alyssawilk · 2024-03-06T16:11:05Z

+
+void EnvoyQuicStream::encodeMetadata(const Http::MetadataMapVector& metadata_map_vector) {
+  if (!http3_options_.allow_metadata()) {
+    // Metadata Frame is not supported in QUICHE.


update comment and log: not supported by config

Good point. Done.

alyssawilk · 2024-03-06T16:11:55Z

+                               "bytes in send buffer. Current write was rejected.",
+                               quic_stream_.write_side_closed() ? "closed" : "open",
+                               quic_stream_.BufferedDataBytes()));
+      quic_stream_.Reset(quic::QUIC_BAD_APPLICATION_PAYLOAD);


is bad application payload the best? feels like an invariant failure but idk if ther's a better code.

Fair enough. How about QUIC_ERROR_PROCESSING_STREAM which seems to be the closest thing we have.

alyssawilk · 2024-03-06T16:12:55Z


+void EnvoyQuicServerStream::OnMetadataComplete(size_t /*frame_len*/,
+                                               const quic::QuicHeaderList& header_list) {
+  request_decoder_->decodeMetadata(metadataMapFromHeaderList(header_list));


do we need to do any validation steps with or without UHV? I honestly don't recall sorry

I don't see anything like that for HTTP/2. I think the payload of METADATA is "key/value pairs" not "HTTP headers" so normal header validation does not apply, as I understand it.

Ok I didn't know if we had concerns about character validation, since I think Envoy has some assumptions there, but maybe not for metadata.
cc @yanavlasov

@birenroy might also know

No restrictions on metadata character set, since it is opt-in and the strings are length prefixed.

alyssawilk · 2024-03-06T16:14:35Z

  EXPECT_EQ(count * size + added_decoded_data_size * 2, response->body().size());
 }

+class MetadataIntegrationTest : public HttpProtocolIntegrationTest {


can we change Http2MetadataIntegrationTest to be MetadataIntegrationTest and run all the tests for H2/H3?

Signed-off-by: Ryan Hamilton <rch@google.com>

https://github.com/google/quiche/compare/cc9724b90..70bc4dfde ``` $ git log cc9724b90..70bc4dfde --date=short --no-merges --format="%ad %al %s" 2024-03-13 rch Return false from QuicSpdyStream::OnMetadataFrameEnd if the stream has been reset. 2024-03-13 martinduke Update MOQT to draft-03 wire image. Does not add ANNOUNCE_CANCEL because it does not have a code point. 2024-03-13 martinduke Rename kGenericError to kInternalError. ``` Signed-off-by: Ryan Hamilton <rch@google.com>

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist · 2024-03-14T00:31:16Z

@alyssawilk PTAL. The QUICHE update which the size limit tests require will be landed in #32892. In the meantime, I've patched that .bzl file change into this PR so I'll need a main merge once that lands. But it's ready for review, I think!

RyanTheOptimist · 2024-03-14T04:15:41Z

/retest

RyanTheOptimist · 2024-03-14T05:13:01Z

/retest

alyssawilk

looks like test/integration/protocol_integration_test.cc has some Metadata tests as well as http2_flood_integration_test.cc

Given it's hidden and we're the only users I'm OK with a TODO to turn those up and landing to kick off import - your call if that's helpful or if you want to just land the right proper fix :-)

alyssawilk · 2024-03-14T13:45:18Z

  return dynamic_cast<QuicFilterManagerConnectionImpl*>(session());
 }

+void EnvoyQuicClientStream::OnMetadataComplete(size_t /*frame_len*/,


I think we should either make it an error, or add an e2e test it doesn't cause problems in Envoy.

alyssawilk · 2024-03-14T13:47:30Z


+void EnvoyQuicServerStream::OnMetadataComplete(size_t /*frame_len*/,
+                                               const quic::QuicHeaderList& header_list) {
+  request_decoder_->decodeMetadata(metadataMapFromHeaderList(header_list));


Ok I didn't know if we had concerns about character validation, since I think Envoy has some assumptions there, but maybe not for metadata.
cc @yanavlasov

alyssawilk · 2024-03-14T13:50:56Z

@@ -571,13 +588,13 @@ TEST_P(Http2MetadataIntegrationTest, TestResponseMetadata) {
  EXPECT_EQ(4, response->metadataMapsDecodedCount());

  // Upstream responds with headers, 100-continue and data.


optional, switch to adding expect 100continue to default_request_headers_

alyssawilk · 2024-03-14T13:52:15Z

@@ -884,14 +911,14 @@ void Http2MetadataIntegrationTest::runHeaderOnlyTest(bool send_request_body, siz
        Http::TestRequestHeaderMapImpl{{":method", "POST"},


default request headers here and below?

alyssawilk · 2024-03-14T13:53:36Z

        [&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void {
          RELEASE_ASSERT(bootstrap.mutable_static_resources()->clusters_size() >= 1, "");
          ConfigHelper::HttpProtocolOptions protocol_options;
-          protocol_options.mutable_explicit_http_config()


Http2MetadataIntegrationTest -> MetadataIntegraionTest?

Whoops, done!

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist · 2024-03-14T14:31:09Z

looks like test/integration/protocol_integration_test.cc has some Metadata tests as well as http2_flood_integration_test.cc

Given it's hidden and we're the only users I'm OK with a TODO to turn those up and landing to kick off import - your call if that's helpful or if you want to just land the right proper fix :-)

Thanks! Yeah, if it's ok with you, I'd love to address this in a follow up. Hopefully it's straightforward :)

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist

Thanks for the comments! The QUICHE merge landed so this PR no longer includes it.

RyanTheOptimist · 2024-03-14T14:19:50Z

        [&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void {
          RELEASE_ASSERT(bootstrap.mutable_static_resources()->clusters_size() >= 1, "");
          ConfigHelper::HttpProtocolOptions protocol_options;
-          protocol_options.mutable_explicit_http_config()


Whoops, done!

RyanTheOptimist · 2024-03-14T14:23:18Z

@@ -884,14 +911,14 @@ void Http2MetadataIntegrationTest::runHeaderOnlyTest(bool send_request_body, siz
        Http::TestRequestHeaderMapImpl{{":method", "POST"},


RyanTheOptimist · 2024-03-14T14:26:10Z

@@ -571,13 +588,13 @@ TEST_P(Http2MetadataIntegrationTest, TestResponseMetadata) {
  EXPECT_EQ(4, response->metadataMapsDecodedCount());

  // Upstream responds with headers, 100-continue and data.


RyanTheOptimist · 2024-03-14T14:29:08Z

  return dynamic_cast<QuicFilterManagerConnectionImpl*>(session());
 }

+void EnvoyQuicClientStream::OnMetadataComplete(size_t /*frame_len*/,


Oh! I think we have a test for this already! ProxyMetadataInResponse does:

// Sends metadata before response header. const std::string key = "key"; std::string value = std::string(80 * 1024, '1'); Http::MetadataMap metadata_map = {{key, value}}; Http::MetadataMapPtr metadata_map_ptr = std::make_unique<Http::MetadataMap>(metadata_map); Http::MetadataMapVector metadata_map_vector; metadata_map_vector.push_back(std::move(metadata_map_ptr)); upstream_request_->encodeMetadata(metadata_map_vector); upstream_request_->encodeHeaders(default_response_headers_, false); upstream_request_->encodeData(12, true);

RyanTheOptimist · 2024-03-14T14:30:09Z


+void EnvoyQuicServerStream::OnMetadataComplete(size_t /*frame_len*/,
+                                               const quic::QuicHeaderList& header_list) {
+  request_decoder_->decodeMetadata(metadataMapFromHeaderList(header_list));


@birenroy might also know

alyssawilk

mildly prefer an explicit TODO for the rest of tests but realistically I know you're good for it =P

repokitteh-read-only bot added the api label Feb 26, 2024

repokitteh-read-only bot assigned abeyad Feb 26, 2024

abeyad reviewed Feb 26, 2024

View reviewed changes

Comment thread api/envoy/config/core/v3/protocol.proto Outdated

Comment thread api/envoy/config/core/v3/protocol.proto Outdated

RyanTheOptimist marked this pull request as draft February 26, 2024 01:10

abeyad previously approved these changes Feb 26, 2024

View reviewed changes

repokitteh-read-only bot removed the api label Feb 26, 2024

RyanTheOptimist dismissed abeyad’s stale review via ccddb41 February 28, 2024 20:19

RyanTheOptimist force-pushed the h3_metadata branch from e68f58b to ccddb41 Compare February 28, 2024 20:19

RyanTheOptimist added 2 commits February 29, 2024 23:25

Update QUICHE

71c28a9

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist force-pushed the h3_metadata branch from ccddb41 to 71c28a9 Compare February 29, 2024 23:25

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Feb 29, 2024

repokitteh-read-only bot assigned mattklein123 Feb 29, 2024

RyanTheOptimist marked this pull request as ready for review February 29, 2024 23:26

repokitteh-read-only bot assigned danzh2010 Feb 29, 2024

RyanTheOptimist added 8 commits March 1, 2024 16:25

Merge branch 'main' into h3_metadata

6c0a232

Signed-off-by: Ryan Hamilton <rch@google.com>

safeMemcpyUnsafeSrc

5fcca3a

Signed-off-by: Ryan Hamilton <rch@google.com>

Cleanup

71cfd1c

Signed-off-by: Ryan Hamilton <rch@google.com>

format

3a7ab40

Signed-off-by: Ryan Hamilton <rch@google.com>

changelog

94ba4eb

Signed-off-by: Ryan Hamilton <rch@google.com>

Changelog

5b6dcb5

Signed-off-by: Ryan Hamilton <rch@google.com>

compile time options

01a0255

Signed-off-by: Ryan Hamilton <rch@google.com>

Merge branch 'main' into h3_metadata

04f65b6

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist removed the deps Approval required for changes to Envoy's external dependencies label Mar 2, 2024

mattklein123 removed their assignment Mar 3, 2024

repokitteh-read-only bot assigned alyssawilk Mar 6, 2024

danzh2010 reviewed Mar 6, 2024

View reviewed changes

Remove bogus comment

c38b40d

Signed-off-by: Ryan Hamilton <rch@google.com>

danzh2010 approved these changes Mar 6, 2024

View reviewed changes

alyssawilk reviewed Mar 6, 2024

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 6, 2024

address comments

8771c89

Signed-off-by: Ryan Hamilton <rch@google.com>

repokitteh-read-only bot removed the waiting label Mar 6, 2024

alyssawilk added the waiting label Mar 7, 2024

Improve tests

c527858

Signed-off-by: Ryan Hamilton <rch@google.com>

repokitteh-read-only bot removed the waiting label Mar 13, 2024

RyanTheOptimist added 5 commits March 13, 2024 18:24

Merge branch 'main' into h3_metadata

fd0f89b

Signed-off-by: Ryan Hamilton <rch@google.com>

Format

74858e6

Signed-off-by: Ryan Hamilton <rch@google.com>

Merge branch 'main' into h3_metadata

33a9dad

Signed-off-by: Ryan Hamilton <rch@google.com>

Enable size limit tests

756c8a5

Signed-off-by: Ryan Hamilton <rch@google.com>

alyssawilk reviewed Mar 14, 2024

View reviewed changes

alyssawilk added the waiting label Mar 14, 2024

Merge branch 'main' into h3_metadata

fd47793

Signed-off-by: Ryan Hamilton <rch@google.com>

Address comments

e2d1fe2

Signed-off-by: Ryan Hamilton <rch@google.com>

repokitteh-read-only bot removed the waiting label Mar 14, 2024

RyanTheOptimist commented Mar 14, 2024

View reviewed changes

alyssawilk approved these changes Mar 14, 2024

View reviewed changes

RyanTheOptimist enabled auto-merge (squash) March 14, 2024 17:29

alyssawilk disabled auto-merge March 14, 2024 17:30

alyssawilk merged commit 640f016 into envoyproxy:main Mar 14, 2024

		@@ -571,13 +588,13 @@ TEST_P(Http2MetadataIntegrationTest, TestResponseMetadata) {
		EXPECT_EQ(4, response->metadataMapsDecodedCount());

		// Upstream responds with headers, 100-continue and data.

		@@ -884,14 +911,14 @@ void Http2MetadataIntegrationTest::runHeaderOnlyTest(bool send_request_body, siz
		Http::TestRequestHeaderMapImpl{{":method", "POST"},

Conversation

RyanTheOptimist commented Feb 26, 2024

Uh oh!

repokitteh-read-only bot commented Feb 26, 2024

Uh oh!

Uh oh!

Uh oh!

abeyad left a comment

Choose a reason for hiding this comment

Uh oh!

RyanTheOptimist commented Feb 26, 2024

Uh oh!

repokitteh-read-only bot commented Feb 29, 2024

Uh oh!

RyanTheOptimist commented Feb 29, 2024

Uh oh!

RyanTheOptimist commented Mar 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanTheOptimist commented Mar 14, 2024

Uh oh!

RyanTheOptimist commented Mar 14, 2024

Uh oh!

RyanTheOptimist commented Mar 14, 2024

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment