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.

Sign up for GitHub

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

skip parsing HttpBody field and pass query params as-is #13679

Merged
merged 3 commits into from
Dec 3, 2020
Merged

skip parsing HttpBody field and pass query params as-is #13679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0a07b27
fix reset and signoff
CurryX Oct 28, 2020
7e317b8
Merge remote-tracking branch 'upstream/master' into curry
CurryX Oct 28, 2020
44411ac
fix format check
CurryX Oct 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,9 +328,16 @@ ProtobufUtil::Status JsonTranscoderConfig::createTranscoder(
return status;
}

resolved_binding.value = binding.value;

request_info.variable_bindings.emplace_back(std::move(resolved_binding));
// HttpBody fields should be passed as-is and not be parsed as JSON.
const bool is_http_body = method_info->request_type_is_http_body_;
const bool is_inside_http_body =
is_http_body && absl::c_equal(absl::MakeSpan(resolved_binding.field_path)
.subspan(0, method_info->request_body_field_path.size()),
method_info->request_body_field_path);
if (!is_inside_http_body) {
resolved_binding.value = binding.value;
request_info.variable_bindings.emplace_back(std::move(resolved_binding));
}
}

RequestMessageTranslatorPtr request_translator;
Expand Down
45 changes: 45 additions & 0 deletions test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -860,6 +860,51 @@ TEST_F(GrpcJsonTranscoderFilterTest, TranscodingUnaryPostWithHttpBody) {
EXPECT_THAT(request, ProtoEq(expected_request));
}

TEST_F(GrpcJsonTranscoderFilterTest, TranscodingUnaryPostWithNestedHttpBody) {
const std::string path = "/echoNestedBody?nested2.body.data=aGkh";
Http::TestRequestHeaderMapImpl request_headers{
{":method", "POST"}, {":path", path}, {"content-type", "text/plain"}};
EXPECT_CALL(decoder_callbacks_, clearRouteCache());
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, true));
EXPECT_EQ("application/grpc", request_headers.get_("content-type"));
EXPECT_EQ(path, request_headers.get_("x-envoy-original-path"));
EXPECT_EQ("POST", request_headers.get_("x-envoy-original-method"));
EXPECT_EQ("/bookstore.Bookstore/EchoNestedBody", request_headers.get_(":path"));
EXPECT_EQ("trailers", request_headers.get_("te"));

const std::string path2 = "/echoNestedBody?nested2.body.data=oops%7b";
request_headers = {{":method", "POST"}, {":path", path2}, {"content-type", "text/plain"}};
EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_.decodeHeaders(request_headers, true));
}

TEST_F(GrpcJsonTranscoderFilterTest, TranscodingUnaryPostWithNestedHttpBodys) {
const std::string path = "/echoNestedBody?nested1.body.data=oops%7b";
Http::TestRequestHeaderMapImpl request_headers{
{":method", "POST"}, {":path", path}, {"content-type", "text/plain"}};
EXPECT_CALL(decoder_callbacks_, clearRouteCache());

EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false));
EXPECT_EQ("application/grpc", request_headers.get_("content-type"));
EXPECT_EQ(path, request_headers.get_("x-envoy-original-path"));
EXPECT_EQ("POST", request_headers.get_("x-envoy-original-method"));
EXPECT_EQ("/bookstore.Bookstore/EchoNestedBody", request_headers.get_(":path"));
EXPECT_EQ("trailers", request_headers.get_("te"));
}

TEST_F(GrpcJsonTranscoderFilterTest, TranscodingUnaryGetWithHttpBody) {
const std::string path = "/echoRawBody?data=oops%7b";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite understand the intention of setting data parameter here? Does it want transcoder to set its value to the HttpBody.data field?

I don't see the code to copy it to the HttpBody.data field.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code shouldn't write data field in to HttpBody.data because HttpBody field shouldn't be override by query param. This test is to test when there is a query param is named with data, and endpoint wants a raw httpbody request info, transcoder should handle this correctly.

Http::TestRequestHeaderMapImpl request_headers{{":method", "GET"}, {":path", path}};

EXPECT_CALL(decoder_callbacks_, clearRouteCache());

EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, true));
EXPECT_EQ("application/grpc", request_headers.get_("content-type"));
EXPECT_EQ(path, request_headers.get_("x-envoy-original-path"));
EXPECT_EQ("GET", request_headers.get_("x-envoy-original-method"));
EXPECT_EQ("/bookstore.Bookstore/EchoRawBody", request_headers.get_(":path"));
EXPECT_EQ("trailers", request_headers.get_("te"));
}

TEST_F(GrpcJsonTranscoderFilterTest, TranscodingStreamPostWithHttpBody) {
Http::TestRequestHeaderMapImpl request_headers{
{":method", "POST"}, {":path", "/streamBody?arg=hi"}, {"content-type", "text/plain"}};
Expand Down
22 changes: 22 additions & 0 deletions test/proto/bookstore.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,20 @@ service Bookstore {
body: "nested.content"
};
}

rpc EchoRawBody(google.api.HttpBody) returns (google.api.HttpBody) {
option (google.api.http) = {
get: "/echoRawBody"
additional_bindings: {post: "/echoRawBody"}
};
}

rpc EchoNestedBody(EchoNestedRequest) returns (google.protobuf.Empty) {
option (google.api.http) = {
post: "/echoNestedBody"
body: "nested1.body"
};
}
rpc EchoResponseBodyPath(google.protobuf.Empty) returns (EchoBodyRequest) {
option (google.api.http) = {
get: "/echoResponseBodyPath"
Expand Down Expand Up @@ -274,6 +288,14 @@ message EchoBodyRequest {
Nested nested = 3;
}

message EchoNestedRequest {
message Nested {
google.api.HttpBody body = 1;
}
Nested nested1 = 1;
Nested nested2 = 2;
}

// Request and Response message for EchoStructReqResp method.
message EchoStructReqResp {
// The content of request.
Expand Down