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.
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
grpc/transcoding: Add support for google.api.http_body response streaming #10673
Conversation
…ming Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc
Outdated
Show resolved
Hide resolved
test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc
Outdated
Show resolved
Hide resolved
Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
docs/root/configuration/http/http_filters/grpc_json_transcoder_filter.rst
Outdated
Show resolved
Hide resolved
Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc
Outdated
Show resolved
Hide resolved
Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
BTW, what is the preferred way to resolve conflicts? |
Please check https://github.com/envoyproxy/envoy/blob/master/CONTRIBUTING.md#submitting-a-pr. |
Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
All done! :) |
@lizan please let me know if something else needs to be done to get this merged.. ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, just one nit.
source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc
Show resolved
Hide resolved
Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
it seems that coverage test failed. |
Please merge master (this should also fix coverage). /wait |
Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
Sure, merged and fixed |
@lizan ping |
…ming (envoyproxy#10673) Description: Add support for `httpBody` for response streaming use case. Risk Level: Low Testing: New unit / integration test Docs Changes: Added Release Notes: Added Fixes envoyproxy#5540 Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru> Signed-off-by: pengg <pengg@google.com>
@belyalov I tested this pr to transcode a streaming grpc to chunked http response, but the content-type of curl is not correct. curl "localhost:8080/get?filename=testdata/music.mp3" -4 -v > test_chunk_music.mp3
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0* Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 8080 (#0)
> GET /get?filename=testdata/music.mp3 HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.47.0
> Accept: */*
>
< HTTP/1.1 200 OK
< content-type: application/json
< grpc-accept-encoding: identity,deflate,gzip
< accept-encoding: identity,gzip
< x-envoy-upstream-service-time: 0
< date: Thu, 23 Apr 2020 14:15:00 GMT
< server: envoy
< transfer-encoding: chunked
<
{ [14238 bytes data]
100 1999k 0 1999k 0 0 640k 0 --:--:-- 0:00:03 --:--:-- 639k^C If I only send the chunk of data once, the content-type is correct. % Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0* Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 8080 (#0)
> GET /get?filename=testdata/musc.mp3 HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.47.0
> Accept: */*
>
< HTTP/1.1 200 OK
< content-type: audio/mp3
< grpc-accept-encoding: identity,deflate,gzip
< accept-encoding: identity,gzip
< x-envoy-upstream-service-time: 0
< date: Thu, 23 Apr 2020 14:18:19 GMT
< server: envoy
< transfer-encoding: chunked Is that the subsequent response chunk overwrite the content-type set by the first chunk? |
hmm, this line You can try it. |
Hm, it does return: envoy/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc Lines 503 to 511 in 62777e8
|
@npuichigo could you debug it with "-l debug" log? |
@npuichigo could you please verify that you're using latest version from master? I've tried to reproduce it on my test setup, it seems work well: Having golang as gRPC server func (k *KV) GetHttp(in *kv.GetRequest, stream kv.KV_GetHttpServer) error {
log.Printf("get: %s", in.Key)
body := &httpbody.HttpBody{
ContentType: "text/plain",
Data: []byte("data\n"),
}
stream.Send(body)
body.ContentType = "unknown"
stream.Send(body)
return nil
} Envoy configured with default gRPC transcoding: #...
- name: envoy.filters.http.grpc_json_transcoder
typed_config:
"@type": type.googleapis.com/envoy.config.filter.http.transcoder.v2.GrpcJsonTranscoder
proto_descriptor: "kv.pb"
services: ["kv.KV"]
print_options:
add_whitespace: true
always_print_primitive_fields: true
always_print_enums_as_ints: false
preserve_proto_field_names: false
#... Request / Response: $ curl -v http://localhost:8080/getHttp
* Trying ::1...
* TCP_NODELAY set
* Connection failed
* connect to ::1 port 8080 failed: Connection refused
* Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8080 (#0)
> GET /getHttp HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.64.1
> Accept: */*
>
< HTTP/1.1 200 OK
< content-type: text/plain
< x-envoy-upstream-service-time: 1
< date: Fri, 24 Apr 2020 01:17:11 GMT
< server: envoy
< transfer-encoding: chunked
<
data
data
* Connection #0 to host localhost left intact
* Closing connection 0 |
@belyalov I use the latest docker image of envoy-dev. I will verify that again. |
@belyalov @qiwzhang I put an example here to reproduce it (https://github.com/npuichigo/grpc_with_envoy). Here I re-pull the latest docker image of envoy-dev. I found that it's cause by the size of HttpBody response. If the body size is smaller than 16KB, then it works well. Otherwise, the received content type is application/json. So is it by design and is there any setting to configure it? You can change |
I see the problem is in this line If buildResponseFromHttpBodyOutput() did not receive a full HttpBody data frame, it should not return "continue", it should return Stop to keep buffering the data. |
@qiwzhang So do you think buildResponsFromHttpBodyOutput should return a boolean to indicate whether we receive a full data frame? Or do we need to check that only for HttpBody?
|
@npuichigo and @qiwzhang Got the repro and @qiannawang is right - basically what happens where is when Going to fix it very soon, thanks for research! |
Additional Description: As mentioned in #10673 when transcoding of httpBody in streaming mode and grpc frame gets fragmented (e.g. large enough) - http header Content-Type may not be set to correct value from content-type of httpBody. Risk Level: low Testing: new unit/integration test Docs Changes: not changed bcz of this feature was introduced in the same release Release Notes: not added bcz of this feature was introduced in the same release Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
Description: Add support for
httpBody
for response streaming use case.Risk Level: Low
Testing: New unit / integration test
Docs Changes: Added
Release Notes: Added
Fixes #5540
This is my first contribution to Envoy project, so I need your help to ramp up with code base.
Please check inline questions and help resolve them.