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.

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

Merged
merged 18 commits into from Apr 20, 2020

Conversation

belyalov
Copy link
Contributor

@belyalov belyalov commented Apr 7, 2020

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.

…ming

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
qiwzhang
qiwzhang previously approved these changes Apr 7, 2020
Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
@belyalov belyalov marked this pull request as ready for review April 7, 2020 02:41
@belyalov belyalov requested a review from lizan as a code owner April 7, 2020 02:41
@belyalov belyalov requested a review from qiwzhang April 7, 2020 02:42
@belyalov belyalov changed the title WIP: grpc/transcoding: Add support for google.api.http_body response streaming grpc/transcoding: Add support for google.api.http_body response streaming Apr 7, 2020
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>
qiwzhang
qiwzhang previously approved these changes Apr 8, 2020
Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
@belyalov
Copy link
Contributor Author

BTW, what is the preferred way to resolve conflicts?
Is it OK to rebase once you've done with review or merge is a way to go?

@euroelessar
Copy link
Contributor

BTW, what is the preferred way to resolve conflicts?
Is it OK to rebase once you've done with review or merge is a way to go?

Please check https://github.com/envoyproxy/envoy/blob/master/CONTRIBUTING.md#submitting-a-pr.
TL;DR Please only merge, do not rebase.

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
@belyalov
Copy link
Contributor Author

All done! :)

@belyalov
Copy link
Contributor Author

@lizan please let me know if something else needs to be done to get this merged.. )

Copy link
Member

@lizan lizan left a 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.

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
lizan
lizan previously approved these changes Apr 16, 2020
Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
@npuichigo
Copy link

it seems that coverage test failed.

@mattklein123
Copy link
Member

Please merge master (this should also fix coverage).

/wait

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>
@belyalov
Copy link
Contributor Author

Please merge master (this should also fix coverage).

/wait

Sure, merged and fixed
All checks are now done!

@npuichigo
Copy link

@lizan ping

@lizan lizan merged commit 557f63f into envoyproxy:master Apr 20, 2020
penguingao pushed a commit to penguingao/envoy that referenced this pull request Apr 22, 2020
…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>
@npuichigo
Copy link

@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?

@qiwzhang
Copy link
Contributor

hmm, this line
should return StopIteration.

You can try it.

@belyalov belyalov deleted the grpc_stream_response branch April 23, 2020 20:44
@belyalov
Copy link
Contributor Author

hmm, this line
should return StopIteration.

You can try it.

Hm, it does return:

headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Json);
// In case of HttpBody in response - content type is unknown at this moment.
// So "Continue" only for regular streaming use case and StopIteration for
// all other cases (non streaming, streaming + httpBody)
if (method_->descriptor_->server_streaming() && !method_->response_type_is_http_body_) {
return Http::FilterHeadersStatus::Continue;
}
return Http::FilterHeadersStatus::StopIteration;

@qiwzhang
Copy link
Contributor

@npuichigo could you debug it with "-l debug" log?

@belyalov
Copy link
Contributor Author

belyalov commented Apr 24, 2020

@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

@npuichigo
Copy link

@belyalov I use the latest docker image of envoy-dev. I will verify that again.

@npuichigo
Copy link

npuichigo commented Apr 24, 2020

@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 static const int kBufferSize = 64 * 1024; in https://github.com/npuichigo/grpc_with_envoy/blob/master/grpc_with_envoy/serving/demo_service_impl.cc to reproduce that.

@qiwzhang
Copy link
Contributor

I see the problem is in this line

https://github.com/envoyproxy/envoy/blob/master/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc#L526

If buildResponseFromHttpBodyOutput() did not receive a full HttpBody data frame, it should not return "continue", it should return Stop to keep buffering the data.

@npuichigo
Copy link

@belyalov

@npuichigo
Copy link

@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?

if (frames.empty()) {
    return true;
}

@belyalov
Copy link
Contributor Author

belyalov commented May 5, 2020

@npuichigo and @qiwzhang
Sorry guys, I missed this thread.

Got the repro and @qiannawang is right - basically what happens where is when Http::FilterHeadersStatus::Continue returned - envoy sends headers, that's why you see application/json as content type.

Going to fix it very soon, thanks for research!

lizan pushed a commit that referenced this pull request May 5, 2020
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support streaming HttpBody proto?
6 participants