-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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-json: no buffering at end_stream or non-gRPC response #2363
Conversation
Signed-off-by: Lizan Zhou <zlizan@google.com>
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.
LGTM, small nits. Thanks for fixing!
@@ -70,6 +72,14 @@ class TranscoderImpl : public Transcoder { | |||
std::unique_ptr<TranscoderInputStream> response_stream_; | |||
}; | |||
|
|||
bool isGrpcResponse(const Http::HeaderMap& headers) { | |||
if (strcmp("200", headers.Status()->value().c_str()) != 0) { |
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.
Please use getResponseStatus
here to match similar code elsewhere.
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.
Done.
return false; | ||
} | ||
return headers.ContentType() && | ||
Http::Headers::get().ContentTypeValues.Grpc == headers.ContentType()->value().c_str(); |
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.
I think this will cause a string temporary to be created. If you invert it won't:
headers.ContentType()->value() == Http::Headers::get().ContentTypeValues.Grpc.c_str()
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.
Done.
I'm fine either though I don't think it will make a string temporary, as <string>
defines:
bool operator== (const string& lhs, const char* rhs);
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.
Ah, good to know. Thanks for pointing that out.
Signed-off-by: Lizan Zhou <zlizan@google.com>
if (Http::Utility::getResponseStatus(headers) != 200) { | ||
return false; | ||
} | ||
return headers.ContentType() && |
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.
Should this use https://github.com/envoyproxy/envoy/blob/master/source/common/grpc/common.cc#L25? Maybe this util belongs in Grpc::Common
as well.
Signed-off-by: Lizan Zhou <zlizan@google.com>
Signed-off-by: Lizan Zhou <zlizan@google.com>
source/common/grpc/common.h
Outdated
@@ -28,10 +28,17 @@ class Common { | |||
public: | |||
/** | |||
* @param headers the headers to parse. | |||
* @param bool indicating wether the header is at end_stream. |
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.
? (I think you meant to put this below)
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.
oops, done.
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.
LGTM other than small comment.
Signed-off-by: Lizan Zhou <zlizan@google.com>
Signed-off-by: Lizan Zhou <zlizan@google.com>
Signed-off-by: Lizan Zhou <zlizan@google.com>
Description: Improves security with respect to third-party actions. See: https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions I also read through the recent commits and updated to the latest release, since it contained a few bugfixes. Risk: Low Testing: CI Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>
Description: Improves security with respect to third-party actions. See: https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions I also read through the recent commits and updated to the latest release, since it contained a few bugfixes. Risk: Low Testing: CI Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: Lizan Zhou zlizan@google.com
Description:
Make gRPC-JSON transcoder not buffering response at end_stream or non-gRPC response.
Risk Level: Low
Testing:
unit test
Docs Changes:
N/A
Release Notes:
N/A
Fixes #2275