Support google.api.HttpBody in gRPC/JSON transcoding by Dogacel · Pull Request #5400 · line/armeria

Dogacel · 2024-01-21T17:05:57Z

Motivation:

Trying to make this work.

Modifications:

Parse media type and content dynamically if method descriptor is a google.api.HttpBody.

Result:

Closes Support sending arbitrary content for gRPC transcoding #5311

Need to do some testing, I couldn't fully understand how to do this. The way I am doing it is a bit tricky bit I guess it should work

github-actions · 2024-01-21T18:06:57Z

🔍 Build Scan® (commit: `100a429`)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	✅	https://ge.armeria.dev/s/5fw5rfzypbxcm
build-self-hosted-unsafe-jdk-8	❌ (failure)	https://ge.armeria.dev/s/gw6j7zg6odlk4
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/you3uut6z3qhu
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/dgzbavoklqhwa
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/d2iljt7ycsk7w
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/qi6agy3bsentq
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/xwzoyajchj6ty
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/hyngunrnduhyy

Dogacel · 2024-01-21T23:43:14Z

+                                      content, responseFuture,
+                                      generateResponseBodyConverter(spec), mediaType);


Here it is trickier, we should pass the media type dynamically before responding. It is going to involve some minor changes to the deframe logic. (Such as determining the content type dynamically after response is generated and falling back if response is not generated).

Dogacel · 2024-01-21T23:43:50Z

+message ArbitraryHttpWrappedRequest {
+  string request_id = 1;
+  google.api.HttpBody body = 2;
+}
+
+message ArbitraryHttpWrappedResponse {
+  string response_id = 1;
+  google.api.HttpBody body = 2;
+}


I am not 100% sure how transcoder should behave when http body is a sub field. There is also extensions field which I did not understand either.

I think it's fine until we hear more feedback on this feature from early adoptors. It won't harm at all anyway if a user is not using this feature, so I'd be happy to merge it even if sub-field and extensions cases are unsupported in this PR. 😄

Sure, let me come back to this PR. I think I have left some gaps therefore this PR was forgotten in the Draft stage.

Do you know if there is anyone waiting for this feature to be released? That would be great to get some feedback.

None that I'm aware of, but I know the colleagues of @hyangtack use HTTP/JSON transcoding, so they might be interested. If we release this and mention it in our release note, someone might start tinkering with it though! 😉

I have updated the code, PTAL at the test cases and LMK what other tests we can run. For now those two seem like fine to me.

trustin · 2024-06-06T14:21:13Z

+
+                        // Media type can only be nullif the request doesn't have a content type.
+                        if (mediaType == null) {
+                            throw new IllegalArgumentException("Unsupported content-type: " +


Maybe should we fall back to octet-stream?

First time hearing about it, mozilla docs also point to the same direction, https://arc.net/l/quote/dteigvyk

Generic binary data (or binary data whose true type is unknown) is application/octet-stream.

I think it makes sense 👍

trustin · 2024-06-06T14:23:13Z

+
+        try {
+            body.put("content_type",  request.contentUtf8());
+            body.put("data", request.content().array());


Out of curiosity, how is a byte array serialized into JSON? How do we deal with a non-UTF8 data?

Let me write some tests, in protobuf spec it should be base64 array.

Looking back to this, I have realized this is still WIP. I'll come back.

I'm still trying to comprehend how transcoding works internally 🙂.

Sure. Please take your time. I appreciate that you continue working on this 💟

I have tested it out and turns out it uses base64. So in that case whether it is UTF8 or not should not matter, right?

So rest of the code should be self-explanatory. We will see.

trustin · 2024-06-06T14:24:48Z

+message ArbitraryHttpWrappedRequest {
+  string request_id = 1;
+  google.api.HttpBody body = 2;
+}
+
+message ArbitraryHttpWrappedResponse {
+  string response_id = 1;
+  google.api.HttpBody body = 2;
+}


I think it's fine until we hear more feedback on this feature from early adoptors. It won't harm at all anyway if a user is not using this feature, so I'd be happy to merge it even if sub-field and extensions cases are unsupported in this PR. 😄

Dogacel · 2024-06-09T02:43:39Z

+                    // Default behavior is to return JSON_UTF_8, the default content type for JSON transcoding.
+                    return MediaType.JSON_UTF_8;


@trustin I think this makes more sense for transcoding perspective. By default responses are json? WDY think?

I'm not sure about the others, but my preference is to just set the content type as-is transparently.

So..

if non-null, then set content-type transparently

if null, we could just not set the content-type. Alternatively, I'm fine with setting a default of text/plain which is the default in armeria.

From my understanding, content-type isn't really mandatory depending on whether the body HttpBody exists. Also, upstream seems to be doing something similar ref: https://github.com/envoyproxy/envoy/blob/94037231eea7150a20b4a07b8c981ae89d1a0a60/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc#L441

What does setting transparently mean? Are you implying we shouldn't add any header at all if it is unset? Kinda makes sense.

What does setting transparently mean?

For a little more background, MediaType is not really a comprehensive list of all content-types out there.
We pretty much copy over guava's often-used list of content-types manually whenever the following test fails. (we also add custom media types when necessary)

armeria/core/src/test/java/com/linecorp/armeria/common/MediaTypeTest.java

Line 598 in 2219de1

public void wellDefinedUpstreamMediaTypes() {

Given that users using HttpBody will be sending over arbitrary data, I wasn't sure if it's worth validating the content-type.

e.g.

final JsonNode contentType = jsonNode.get("contentType"); if (contentType != null && contentType.isTextual()) { builder.set(HttpHeaderNames.CONTENT_TYPE, contentType.textValue()); } else { builder.remove(HttpHeaderNames.CONTENT_TYPE); }

Makes sense, it doesn't have to be MediaType. I.e. application/scim+json wasn't an option when I checked it last time.

Dogacel · 2024-06-09T02:44:16Z

+                    if (charset != null) {
+                        httpBodyBytes = Base64.getDecoder().decode(httpBody);
+                    } else {
+                        httpBodyBytes = Base64.getDecoder().decode(httpBody);
+                    }


Response is Base64 because protobuf representation of body is byte[] and jackson serializes it as a base64 string.

I assume performance is not our first concern here so it seems fine 🙂

jrhee17

Mostly looks good 👍 Left some minor comments

jrhee17 · 2024-06-14T10:43:15Z

+            }
+
+            body.put("content_type", contentType.toString());
+            body.put("data", content.array()); // Base64 encoded binary data


Double checked that we want to use base64 👍
ref: https://protobuf.dev/programming-guides/proto3/#json

jrhee17 · 2024-06-14T10:44:11Z

        return HttpResponse.of(responseFuture);
    }

+    private HttpData convertToArbitraryHttpData(AggregatedHttpRequest request) throws IOException {


optional; naming and static

Suggested change

private HttpData convertToArbitraryHttpData(AggregatedHttpRequest request) throws IOException {

private static HttpData convertToHttpBody(AggregatedHttpRequest request) throws IOException {

jrhee17 · 2024-06-14T10:47:01Z

+                    final byte[] httpBodyBytes;
+                    if (charset != null) {
+                        httpBodyBytes = Base64.getDecoder().decode(httpBody);
+                    } else {
+                        httpBodyBytes = Base64.getDecoder().decode(httpBody);
+                    }


Question) Is there a point in checking the charset? I think it's fine to delegate to the user responsibility for setting the arbitrary data as well as setting the correct charset. (also, the if statement isn't doing anything)

Suggested change

final byte[] httpBodyBytes;

if (charset != null) {

httpBodyBytes = Base64.getDecoder().decode(httpBody);

} else {

httpBodyBytes = Base64.getDecoder().decode(httpBody);

}

final byte[] httpBodyBytes = Base64.getDecoder().decode(httpBody);

Oh, I must have deleted something. let me check this.

jrhee17 · 2024-06-14T11:07:15Z

+                    // Default behavior is to return JSON_UTF_8, the default content type for JSON transcoding.
+                    return MediaType.JSON_UTF_8;


I'm not sure about the others, but my preference is to just set the content type as-is transparently.

So..

if non-null, then set content-type transparently

if null, we could just not set the content-type. Alternatively, I'm fine with setting a default of text/plain which is the default in armeria.

From my understanding, content-type isn't really mandatory depending on whether the body HttpBody exists. Also, upstream seems to be doing something similar ref: https://github.com/envoyproxy/envoy/blob/94037231eea7150a20b4a07b8c981ae89d1a0a60/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc#L441

jrhee17 · 2024-06-14T11:08:36Z

+                try (HttpData data = httpData) {
+                    final byte[] array = data.array();
+                    try {
+                        final JsonNode jsonNode = mapper.readValue(array, JsonNode.class);


Question) Is it possible to refactor so that only one deserialization is done? We seem to be doing this at generateResponseBodyConverter as well.

There is a race condition, response body converter first deserializes the JSON, finds the body parameter and converts the body to the inner body parameter. So we lose content-type data from the original body.

A way to solve this would be returning body + media_type bundled together by updating the generateResponseBodyConverter method to be generateResponseBodyAndMediaTypeConverter.

I am not entirely sure if it is worth the effort the abstract this out. I feel like readability of the code dropped quite a bit when I have done this. I can try introducing a method that takes HttpResponse and emits another HttpResponse instead of the two converters. This method can transform headers & body on the go.

I have used AggregatedHttpResponse because it seems like the response was already aggregated right before we passed it to the converter. LMK if anything looks odd.

jrhee17 · 2024-06-14T11:10:43Z

Oops, approved by mistake 😅

jrhee17

Changes look pretty much good to me 👍 Left some final comments 🙇

jrhee17 · 2024-06-17T02:19:17Z

+                    // Default behavior is to return JSON_UTF_8, the default content type for JSON transcoding.
+                    return MediaType.JSON_UTF_8;


What does setting transparently mean?

For a little more background, MediaType is not really a comprehensive list of all content-types out there.
We pretty much copy over guava's often-used list of content-types manually whenever the following test fails. (we also add custom media types when necessary)

armeria/core/src/test/java/com/linecorp/armeria/common/MediaTypeTest.java

Line 598 in 2219de1

public void wellDefinedUpstreamMediaTypes() {

Given that users using HttpBody will be sending over arbitrary data, I wasn't sure if it's worth validating the content-type.

e.g.

final JsonNode contentType = jsonNode.get("contentType"); if (contentType != null && contentType.isTextual()) { builder.set(HttpHeaderNames.CONTENT_TYPE, contentType.textValue()); } else { builder.remove(HttpHeaderNames.CONTENT_TYPE); }

minwoox

This looks great! Thanks! 👍 👍 👍

trustin

Nice work, @Dogacel! 🙇

jrhee17

Looks good 👍 Left some final comments 🙇

jrhee17 · 2024-06-27T07:07:02Z

            return null;
-        } else {
-            return httpData -> {
-                try (HttpData data = httpData) {


Question) Can we revive the old behavior to close the httpData in case the data is pooled?

jrhee17 · 2024-06-27T07:09:02Z

+                if (jsonNode == null) {
+                    return httpResponse;
+                }
+


Can we also close the httpResponse.content() once we're sure we will be using google.api.httpBody? (Either via try blocks, or by using PooledObjects.close(httpResponse.content()))

I guess I got it, I am not entirely sure how that try(...) works internally or what pooled objects are TBH. Please LMK if it looks right.

I am not entirely sure how that try(...) works internally or what pooled objects are TBH.

Once a gRPC message is written, it is converted to bytes via MessageMarshaller

armeria/grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/AbstractServerCall.java

Line 547 in ebb0888

return responseFramer.writePayload(marshaller.serializeResponse(message));

Here the channel's ByteBufAllocator is used to allocate bytes so that no additional copies are necessary when writing over the wire.

armeria/grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/GrpcMessageMarshaller.java

Line 145 in ebb0888

public ByteBuf serializeResponse(O message) throws IOException {

The response is aggregated with the pooled option, meaning don't copy the bytes

armeria/grpc/src/main/java/com/linecorp/armeria/server/grpc/AbstractUnframedGrpcService.java

Line 172 in ebb0888

grpcResponse.aggregate(AggregationOptions.usePooledObjects(ctx.alloc(), ctx.eventLoop()))

Hence, if we want to use the httpResponse as-is in the early return, we shouldn't close the httpResponse.content().

Can you remove the try block here and modify like the following?

e.g.

return httpResponse -> { final HttpData data = httpResponse.content(); final JsonNode jsonNode = extractHttpBody(data); // Failed to parse the JSON body, return the original response. if (jsonNode == null) { return httpResponse; } PooledObjects.close(data); // The data field is base64 encoded. // https://protobuf.dev/programming-guides/proto3/#json

Okay, I guess I kinda understand. What happens if we don't close the pool? I suspect the memory has to be claimed by the garbage collector which is inefficient? Or maybe there is some reference to this byte buffer left somewhere internally thus it might lead to a memory leak? (Seems unlikely)

Asking just for learning purposes 👼

This is more netty territory, but my understanding is that since direct memory is not cheap to allocate/deallocate netty maintains a pool of java ByteBuffers (may be direct or heap based)

If we don't call release on the ByteBuf, the ByteBuf object itself will be garbage collected but the memory pool will continue to have a strong reference to the underlyingByteBuffer - hence a memory leak. The others may shed more light on this subject though.

jrhee17

Only two nits 👍

jrhee17 · 2024-07-10T09:25:00Z

+                if (jsonNode == null) {
+                    return httpResponse;
+                }
+


I am not entirely sure how that try(...) works internally or what pooled objects are TBH.

Once a gRPC message is written, it is converted to bytes via MessageMarshaller

armeria/grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/AbstractServerCall.java

Line 547 in ebb0888

return responseFramer.writePayload(marshaller.serializeResponse(message));

Here the channel's ByteBufAllocator is used to allocate bytes so that no additional copies are necessary when writing over the wire.

armeria/grpc/src/main/java/com/linecorp/armeria/internal/common/grpc/GrpcMessageMarshaller.java

Line 145 in ebb0888

public ByteBuf serializeResponse(O message) throws IOException {

The response is aggregated with the pooled option, meaning don't copy the bytes

armeria/grpc/src/main/java/com/linecorp/armeria/server/grpc/AbstractUnframedGrpcService.java

Line 172 in ebb0888

grpcResponse.aggregate(AggregationOptions.usePooledObjects(ctx.alloc(), ctx.eventLoop()))

Hence, if we want to use the httpResponse as-is in the early return, we shouldn't close the httpResponse.content().

Can you remove the try block here and modify like the following?

e.g.

return httpResponse -> { final HttpData data = httpResponse.content(); final JsonNode jsonNode = extractHttpBody(data); // Failed to parse the JSON body, return the original response. if (jsonNode == null) { return httpResponse; } PooledObjects.close(data); // The data field is base64 encoded. // https://protobuf.dev/programming-guides/proto3/#json

ikhoon

Nice work, @Dogacel! 👍👍

jrhee17

👍 👍 👍

Dogacel commented Jan 21, 2024

View reviewed changes

github-actions Bot added the Stale label Apr 9, 2024

trustin reviewed Jun 6, 2024

View reviewed changes

github-actions Bot removed the Stale label Jun 8, 2024

Dogacel added 3 commits June 8, 2024 00:25

Support google.api.HttpBody in gRPC/JSON transcoding

0db64be

throw IO and return json node

c13de0f

continue working on supporting arbitrart HTTP data on gRPC

f213f19

Dogacel force-pushed the dogac/support-http-body-in-json-transcoding branch from 259a7b4 to f213f19 Compare June 8, 2024 05:25

fix HttpBody transcoding

295c2a3

Dogacel marked this pull request as ready for review June 9, 2024 02:42

Dogacel requested review from ikhoon, jrhee17 and minwoox as code owners June 9, 2024 02:42

Dogacel commented Jun 9, 2024

View reviewed changes

remove println

a8c5239

ikhoon added the new feature label Jun 10, 2024

Merge branch 'main' into dogac/support-http-body-in-json-transcoding

d549be7

jrhee17 approved these changes Jun 14, 2024

View reviewed changes

jrhee17 self-requested a review June 14, 2024 11:09

comment resolution

9edd198

jrhee17 reviewed Jun 17, 2024

View reviewed changes

comment resolution

cab8d06

Dogacel requested a review from jrhee17 June 17, 2024 14:40

minwoox approved these changes Jun 19, 2024

View reviewed changes

minwoox added this to the 1.30.0 milestone Jun 19, 2024

trustin approved these changes Jun 19, 2024

View reviewed changes

jrhee17 reviewed Jun 27, 2024

View reviewed changes

comment resolution

6478ab3

Dogacel requested a review from jrhee17 June 28, 2024 04:10

jrhee17 reviewed Jul 10, 2024

View reviewed changes

don't close pooled data if returning the original response

100a429

ikhoon approved these changes Jul 16, 2024

View reviewed changes

jrhee17 approved these changes Jul 19, 2024

View reviewed changes

ikhoon merged commit c7b2a44 into line:main Jul 25, 2024

		content, responseFuture,
		generateResponseBodyConverter(spec), mediaType);

		// Default behavior is to return JSON_UTF_8, the default content type for JSON transcoding.
		return MediaType.JSON_UTF_8;

	private HttpData convertToArbitraryHttpData(AggregatedHttpRequest request) throws IOException {
	private static HttpData convertToHttpBody(AggregatedHttpRequest request) throws IOException {

Uh oh!

Conversation

Dogacel commented Jan 21, 2024

Uh oh!

github-actions Bot commented Jan 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔍 Build Scan® (commit: 100a429)

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!

Dogacel Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Choose a reason for hiding this comment

Uh oh!

jrhee17 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!

jrhee17 commented Jun 14, 2024

Uh oh!

github-actions Bot commented Jan 21, 2024 •

edited

Loading

🔍 Build Scan® (commit: `100a429`)

Dogacel Jun 6, 2024 •

edited

Loading