[grpc] Envoy/Googe-Grpc-lib base64 encode "-bin" client init-metadata #39026

stevenzzzz · 2025-04-05T23:54:40Z

Commit Message: Envoy Grpc-lib en/decode base64 on -bin init-metadata
Additional Description: Following https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md spec about "-bin" metadata.

Also fixed a use-after-move bug in the grpc async-client-impl.

Risk Level: LOW
Testing: unit tests, integration tests.
Docs Changes:
Release Notes:
Platform Specific Features:

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

…oogle grpc "-bin" initial metadata Signed-off-by: Xin Zhuang <stevenzzz@google.com>

…ocessing Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

…ponse for Envoy-grpc (less test breakages) Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

stevenzzzz · 2025-04-09T02:47:59Z

/assign @yanjunxiang-google

Hey Yanjun, could you give it a pass?

stevenzzzz · 2025-04-09T02:48:17Z

@kyessenov FYI

stevenzzzz · 2025-04-09T14:17:49Z

/assign @yanavlasov

stevenzzzz · 2025-04-09T14:18:41Z

/retest

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

kyessenov · 2025-04-09T15:15:50Z

source/common/grpc/google_async_client_impl.cc

    Http::HeaderMap& header_map) {
  // More painful copying, this time due to the mismatch in header
  // representation data structures in Envoy and Google gRPC.
+  // Note that grpc-lib has handled the "-bin" metadata decoding.


Does it do it at send, or when set? I am worried about a case some module using gRPC SDK gets this header corrupted.

It's in the gprc hpack lib I think.
check out the integration test. I am pretty sure on the wire it should be based-encoded. at least, we should not encode it on the receiving path.
(I will show you an internal integration test as example).

stevenzzzz · 2025-04-09T16:16:11Z

/assign @htuch

yanjunxiang-google · 2025-04-09T18:06:31Z

source/common/grpc/async_client_impl.cc

+    if (absl::EndsWith(header.key().getStringView(), "-bin")) {
+      bin_metadata.emplace(header.key().getStringView(),
+                           absl::Base64Escape(header.value().getStringView()));
+    }


Can we directly remove the original header/value pair, and adds the new header/base-64-value pair here? That way, we can avoid the below for loop.

I can not find a method from headermap to in-place replace the value of a key, let me know if you know one.

I am wondering can you use setCopy() to replace the value inline, like something as below:

headers.iterate([&bin_metadata](const Http::HeaderEntry& header) {
if (absl::EndsWith(header.key().getStringView(), "-bin")) {
headers.setCopy(header.key().getStringView(), absl::Base64Escape(header.value().getStringView()));
}
}

setCopy is basically remove + addCopy under the hood. this iterate() method is a "const" method, I don't fell like mutate the headermap inside it.
Let's use the somewhat ugly two loops to fix the issue.

yanjunxiang-google · 2025-04-09T18:09:36Z

source/common/grpc/async_client_impl.cc

+      std::string value;
+      absl::Base64Unescape(header.value().getStringView(), &value);
+      bin_metadata[header.key().getStringView()] = std::move(value);
+    }


pls see the other comment.

this method is removed in later commit.

yanjunxiang-google · 2025-04-09T18:12:08Z

source/common/grpc/async_client_impl.cc


 // TODO(htuch): match Google gRPC base64 encoding behavior for *-bin headers, see
 // https://github.com/envoyproxy/envoy/pull/2444#discussion_r163914459.
 void AsyncStreamImpl::onHeaders(Http::ResponseHeaderMapPtr&& headers, bool end_stream) {


Should we remove this TODO comments?

per recent discussion, we want to drop the base64 decoding of server side -bin metadata until #39054 resolves.

yanjunxiang-google · 2025-04-09T18:30:45Z

source/common/grpc/async_client_impl.cc

+  }
+  // Normal response headers/Server initial metadata.
+  if (!waiting_to_delete_on_remote_close_) {
+    base64UnescapeBinHeaders(*headers);


Can we just add this line base64UnescapeBinHeaders(*headers) at the original place, i.e, line 194, and keep other places in this function untouched? What's the benefit to move it to the end, and spread the logic before each return? Also, why not having if (!waiting_to_delete_on_remote_close_) check in other places? Is it a logic change?

stevenzzzz · 2025-04-09T19:12:48Z

Per discussion, let's not decode on the receiving path for -bin headers. This means encoding the Google-gprc-client "-bin" headers. #39054

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

stevenzzzz

Thanks.

Reverted the "decoding" on receiving path. adjusted tests, PTAL.

stevenzzzz · 2025-04-09T20:20:16Z

source/common/grpc/async_client_impl.cc


 // TODO(htuch): match Google gRPC base64 encoding behavior for *-bin headers, see
 // https://github.com/envoyproxy/envoy/pull/2444#discussion_r163914459.
 void AsyncStreamImpl::onHeaders(Http::ResponseHeaderMapPtr&& headers, bool end_stream) {


per recent discussion, we want to drop the base64 decoding of server side -bin metadata until #39054 resolves.

yanjunxiang-google · 2025-04-10T18:04:11Z

LGTM

stevenzzzz · 2025-04-10T23:48:27Z

/retest

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

stevenzzzz · 2025-04-11T12:56:59Z

Hey @yanavlasov, could you pls give it a maintainer bless?

I'd hope someone could take care of #39054,

Right now wasm filter is the only consuming server metadatas, but I am not sure if there is any meaningful application, but still good to have.

…envoyproxy#39026) Envoy Grpc-lib en/decode base64 on -bin init-metadata Following https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md spec about "-bin" metadata. Also fixed a use-after-move bug in the grpc async-client-impl. Risk Level: LOW Testing: unit tests, integration tests. Docs Changes: Release Notes: Platform Specific Features: --------- Signed-off-by: Lizan Zhou <lizan@tetrate.io> Signed-off-by: Xin Zhuang <stevenzzz@google.com> Co-authored-by: Lizan Zhou <lizan@tetrate.io> Signed-off-by: Kyle O'Connor <oconnorkyle@google.com>

lizan and others added 18 commits March 2, 2020 14:27

doc: fix SNI FAQ link (envoyproxy#10227)

a9b9f8b

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

Merge branch 'master' of https://github.com/envoyproxy/envoy

ff341df

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy

963e42e

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy

60953f8

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy

8991be3

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy

9ed6dee

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy

3944e77

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy

dbbc4a0

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into master

dd8972f

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into master

34408d9

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy

7c2c46c

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy

2de342d

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Merge branch 'main' of https://github.com/envoyproxy/envoy

9b2e552

Merge branch 'main' of https://github.com/envoyproxy/envoy

bb5d27e

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Merge branch 'envoyproxy:main' into master

ab02e20

Merge branch 'envoyproxy:main' into master

9d4f511

base64 encode on request path and decode on response path for Envoy G…

6a704dd

…oogle grpc "-bin" initial metadata Signed-off-by: Xin Zhuang <stevenzzz@google.com>

also resolve one move-after-free issue around server-init-metadata pr…

9f44df0

…ocessing Signed-off-by: Xin Zhuang <stevenzzz@google.com>

stevenzzzz marked this pull request as draft April 5, 2025 23:56

stevenzzzz changed the title ~~Envoy Grpc-lib en/decode base64 on -bin init-metadata~~ [grpc] Envoy Grpc-lib en/decode base64 on -bin init-metadata Apr 7, 2025

stevenzzzz added 4 commits April 8, 2025 00:53

add more code

67c39c1

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

add code changes and tests

36edb29

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

let's follow what's been done, add init-metadata on trailers-only res…

bb536f1

…ponse for Envoy-grpc (less test breakages) Signed-off-by: Xin Zhuang <stevenzzz@google.com>

fix one more test

9c48633

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

stevenzzzz changed the title ~~[grpc] Envoy Grpc-lib en/decode base64 on -bin init-metadata~~ [grpc] Envoy/Googe-Grpc-lib en/decode base64 on -bin init-metadata Apr 9, 2025

stevenzzzz marked this pull request as ready for review April 9, 2025 02:47

repokitteh-read-only bot assigned yanjunxiang-google Apr 9, 2025

repokitteh-read-only bot assigned yanavlasov Apr 9, 2025

ahh, envoy repo not c++20 mode

361b1b5

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

kyessenov reviewed Apr 9, 2025

View reviewed changes

repokitteh-read-only bot assigned htuch Apr 9, 2025

yanjunxiang-google reviewed Apr 9, 2025

View reviewed changes

stevenzzzz added 2 commits April 9, 2025 20:13

revert the decoding on receiving path for bin metadata

e25f86f

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

add some more comments

61eab38

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

stevenzzzz commented Apr 9, 2025

View reviewed changes

stevenzzzz changed the title ~~[grpc] Envoy/Googe-Grpc-lib en/decode base64 on -bin init-metadata~~ [grpc] Envoy/Googe-Grpc-lib base64 encode "-bin" client init-metadata Apr 11, 2025

stevenzzzz added 2 commits April 11, 2025 12:43

Heh, Envoy fomatter doesn't like Heh

926ed08

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Merge branch 'main' into grpc-encode-base64

eabe8ca

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

yanavlasov approved these changes Apr 14, 2025

View reviewed changes

yanavlasov merged commit fcb83af into envoyproxy:main Apr 14, 2025
25 checks passed

stevenzzzz mentioned this pull request May 10, 2025

[envoy_grpc] base64 en/decoding on -bin metadata entries. #39034

Closed

[grpc] Envoy/Googe-Grpc-lib base64 encode "-bin" client init-metadata #39026

[grpc] Envoy/Googe-Grpc-lib base64 encode "-bin" client init-metadata #39026

Uh oh!

Conversation

stevenzzzz commented Apr 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevenzzzz commented Apr 9, 2025

Uh oh!

stevenzzzz commented Apr 9, 2025

Uh oh!

stevenzzzz commented Apr 9, 2025

Uh oh!

stevenzzzz commented Apr 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenzzzz commented Apr 9, 2025

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!

stevenzzzz commented Apr 9, 2025

Uh oh!

stevenzzzz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanjunxiang-google commented Apr 10, 2025

Uh oh!

stevenzzzz commented Apr 10, 2025

Uh oh!

stevenzzzz commented Apr 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

stevenzzzz commented Apr 5, 2025 •

edited

Loading