Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,27 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
// gRPC-JSON transcoder :ref:`configuration overview <config_http_filters_grpc_json_transcoder>`.
// [#extension: envoy.filters.http.grpc_json_transcoder]

// [#next-free-field: 10]
// [#next-free-field: 11]
message GrpcJsonTranscoder {
option (udpa.annotations.versioning).previous_message_type =
"envoy.config.filter.http.transcoder.v2.GrpcJsonTranscoder";

enum UrlUnescapeSpec {
// URL path parameters will not decode RFC 6570 reserved characters.
// For example, segment `%2f%23/%20%2523` is unescaped to `%2f%23/ %23`.
ALL_CHARACTERS_EXCEPT_RESERVED = 0;

// URL path parameters will be fully URI-decoded except in
// cases of single segment matches in reserved expansion, where "%2F" will be
// left encoded.
// For example, segment `%2f%23/%20%2523` is unescaped to `%2f#/ %23`.
ALL_CHARACTERS_EXCEPT_SLASH = 1;

// URL path parameters will be fully URI-decoded.
// For example, segment `%2f%23/%20%2523` is unescaped to `/#/ %23`.
Copy link
Member

Choose a reason for hiding this comment

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

How does this interact with Envoy's existing path normalization controls?

// Should paths be normalized according to RFC 3986 before any processing of

Keep in mind that we'e planning on making some changes here in the future #6589

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UrlUnescapeSpec controls decoding logic only, it does not try to normalize path. The result of this decoding is never used in envoy itself to reconstruct back to the :path header.
So said, this logic is applied after envoy's path normalization and does honor it.

Copy link
Member

Choose a reason for hiding this comment

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

I think there may be overlap in the future with path normalization, but I can see how these would be used distinctly in a transcoder.

ALL_CHARACTERS = 2;
}

message PrintOptions {
option (udpa.annotations.versioning).previous_message_type =
"envoy.config.filter.http.transcoder.v2.GrpcJsonTranscoder.PrintOptions";
Expand Down Expand Up @@ -160,4 +176,11 @@ message GrpcJsonTranscoder {
// the ``google/rpc/error_details.proto`` should be included in the configured
// :ref:`proto descriptor set <config_grpc_json_generate_proto_descriptor_set>`.
bool convert_grpc_status = 9;

// URL unescaping policy.
// This spec is only applied when extracting variable with multiple segments.
// For example, in case of `/foo/{x=*}/bar/{y=prefix/*}/{z=**}` `x` variable is single segment and `y` and `z` are multiple segments.
// For a path with `/foo/first/bar/prefix/second/third/fourth`, `x=first`, `y=prefix/second`, `z=third/fourth`.
// If this setting is not specified, the value defaults to :ref:`ALL_CHARACTERS_EXCEPT_RESERVED<envoy_api_enum_value_extensions.filters.http.grpc_json_transcoder.v3.GrpcJsonTranscoder.UrlUnescapeSpec.ALL_CHARACTERS_EXCEPT_RESERVED>`.
UrlUnescapeSpec url_unescape_spec = 10 [(validate.rules).enum = {defined_only: true}];
}
6 changes: 3 additions & 3 deletions bazel/repository_locations.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -507,13 +507,13 @@ REPOSITORY_LOCATIONS_SPEC = dict(
project_name = "grpc-httpjson-transcoding",
project_desc = "Library that supports transcoding so that HTTP/JSON can be converted to gRPC",
project_url = "https://github.com/grpc-ecosystem/grpc-httpjson-transcoding",
version = "b48d8aa15b3825e146168146755475ab918e95b7",
sha256 = "4147e992ec239fb78c435fdd9f68e8d93d89106f67278bf2995f3672dddba52b",
version = "4d095f048889d4fc3b8d4579aa80ca4290319802",
sha256 = "7af66e0674340932683ab4f04ea6f03e2550849a54741738d94310b84d396a2c",
strip_prefix = "grpc-httpjson-transcoding-{version}",
urls = ["https://github.com/grpc-ecosystem/grpc-httpjson-transcoding/archive/{version}.tar.gz"],
use_category = ["dataplane_ext"],
extensions = ["envoy.filters.http.grpc_json_transcoder"],
release_date = "2020-11-05",
release_date = "2020-11-12",
cpe = "N/A",
),
io_bazel_rules_go = dict(
Expand Down
1 change: 1 addition & 0 deletions docs/root/version_history/current.rst
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ New Features
------------
* config: added new runtime feature `envoy.features.enable_all_deprecated_features` that allows the use of all deprecated features.
* grpc: implemented header value syntax support when defining :ref:`initial metadata <envoy_v3_api_field_config.core.v3.GrpcService.initial_metadata>` for gRPC-based `ext_authz` :ref:`HTTP <envoy_v3_api_field_extensions.filters.http.ext_authz.v3.ExtAuthz.grpc_service>` and :ref:`network <envoy_v3_api_field_extensions.filters.network.ext_authz.v3.ExtAuthz.grpc_service>` filters, and :ref:`ratelimit <envoy_v3_api_field_config.ratelimit.v3.RateLimitServiceConfig.grpc_service>` filters.
* grpc-json: added support for configuring :ref:`unescaping behavior <envoy_v3_api_field_extensions.filters.http.grpc_json_transcoder.v3.GrpcJsonTranscoder.url_unescape_spec>` for path components.
* hds: added support for delta updates in the :ref:`HealthCheckSpecifier <envoy_v3_api_msg_service.health.v3.HealthCheckSpecifier>`, making only the Endpoints and Health Checkers that changed be reconstructed on receiving a new message, rather than the entire HDS.
* health_check: added option to use :ref:`no_traffic_healthy_interval <envoy_v3_api_field_config.core.v3.HealthCheck.no_traffic_healthy_interval>` which allows a different no traffic interval when the host is healthy.
* http: added HCM :ref:`timeout config field <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.request_headers_timeout>` to control how long a downstream has to finish sending headers before the stream is cancelled.
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,24 @@ JsonTranscoderConfig::JsonTranscoderConfig(
}
}

switch (proto_config.url_unescape_spec()) {
case envoy::extensions::filters::http::grpc_json_transcoder::v3::GrpcJsonTranscoder::
ALL_CHARACTERS_EXCEPT_RESERVED:
pmb.SetUrlUnescapeSpec(
google::grpc::transcoding::UrlUnescapeSpec::kAllCharactersExceptReserved);
break;
case envoy::extensions::filters::http::grpc_json_transcoder::v3::GrpcJsonTranscoder::
ALL_CHARACTERS_EXCEPT_SLASH:
pmb.SetUrlUnescapeSpec(google::grpc::transcoding::UrlUnescapeSpec::kAllCharactersExceptSlash);
break;
case envoy::extensions::filters::http::grpc_json_transcoder::v3::GrpcJsonTranscoder::
ALL_CHARACTERS:
pmb.SetUrlUnescapeSpec(google::grpc::transcoding::UrlUnescapeSpec::kAllCharacters);
break;
default:
NOT_REACHED_GCOVR_EXCL_LINE;
}

path_matcher_ = pmb.Build();

const auto& print_config = proto_config.print_options();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1270,6 +1270,78 @@ INSTANTIATE_TEST_SUITE_P(
})",
R"({"id":"101","gender":"MALE","last_name":"Shakespeare"})"}));

struct GrpcJsonTranscoderFilterUnescapeTestParam {
std::string config_json_;
std::string expected_arg_;
};

class GrpcJsonTranscoderFilterUnescapeTest
: public testing::TestWithParam<GrpcJsonTranscoderFilterUnescapeTestParam>,
public GrpcJsonTranscoderFilterTestBase {
protected:
GrpcJsonTranscoderFilterUnescapeTest() {
envoy::extensions::filters::http::grpc_json_transcoder::v3::GrpcJsonTranscoder proto_config;
TestUtility::loadFromJson(TestEnvironment::substitute(GetParam().config_json_), proto_config);
config_ = std::make_unique<JsonTranscoderConfig>(proto_config, *api_);
filter_ = std::make_unique<JsonTranscoderFilter>(*config_);
filter_->setDecoderFilterCallbacks(decoder_callbacks_);
filter_->setEncoderFilterCallbacks(encoder_callbacks_);
}

std::unique_ptr<JsonTranscoderConfig> config_;
std::unique_ptr<JsonTranscoderFilter> filter_;
NiceMock<Http::MockStreamDecoderFilterCallbacks> decoder_callbacks_;
NiceMock<Http::MockStreamEncoderFilterCallbacks> encoder_callbacks_;
};

TEST_P(GrpcJsonTranscoderFilterUnescapeTest, UnescapeSpec) {
Http::TestRequestHeaderMapImpl request_headers{
{"content-type", "text/plain"}, {":method", "POST"}, {":path", "/wildcard/%2f%23/%20%2523"}};
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false));

Buffer::OwnedImpl request_data{"{}"};
EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(request_data, true));

Grpc::Decoder decoder;
std::vector<Grpc::Frame> frames;
decoder.decode(request_data, frames);

EXPECT_EQ(1, frames.size());

bookstore::EchoBodyRequest expected_request;
expected_request.set_arg(GetParam().expected_arg_);

bookstore::EchoBodyRequest request;
request.ParseFromString(frames[0].data_->toString());

EXPECT_EQ(expected_request.ByteSize(), frames[0].length_);
EXPECT_TRUE(MessageDifferencer::Equals(expected_request, request));
}

INSTANTIATE_TEST_SUITE_P(GrpcJsonTranscoderFilterUnescapeOptions,
GrpcJsonTranscoderFilterUnescapeTest,
::testing::Values(
GrpcJsonTranscoderFilterUnescapeTestParam{
R"({
"proto_descriptor": "{{ test_rundir }}/test/proto/bookstore.descriptor",
"services": ["bookstore.Bookstore"]
})",
"%2f%23/ %23"},
GrpcJsonTranscoderFilterUnescapeTestParam{
R"({
"proto_descriptor": "{{ test_rundir }}/test/proto/bookstore.descriptor",
"services": ["bookstore.Bookstore"],
"url_unescape_spec": "ALL_CHARACTERS_EXCEPT_SLASH"
})",
"%2f#/ %23"},
GrpcJsonTranscoderFilterUnescapeTestParam{
R"({
"proto_descriptor": "{{ test_rundir }}/test/proto/bookstore.descriptor",
"services": ["bookstore.Bookstore"],
"url_unescape_spec": "ALL_CHARACTERS"
})",
"/#/ %23"}));

} // namespace
} // namespace GrpcJsonTranscoder
} // namespace HttpFilters
Expand Down
5 changes: 5 additions & 0 deletions test/proto/bookstore.proto
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,11 @@ service Bookstore {
get: "/bigbook"
};
}
rpc PostWildcard(EchoBodyRequest) returns (google.protobuf.Empty) {
option (google.api.http) = {
post: "/wildcard/{arg=**}"
};
Copy link
Contributor

@qiwzhang qiwzhang Nov 13, 2020

Choose a reason for hiding this comment

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

I am not sure if fully_decode_reserved_expansion defined here can be specified as option in the proto file. If it could, I like to support it. Could you help to hook it up?

The logic will be if UrlUnescape_spec is default, check such option in the either api or at method level. if it is true, use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can't be, google.api.http extension has google.api.HttpRule type, but fully_decode_reserved_expansion is a field of google.api.Http.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess the intention of Http message is to define collective list of all rules, e.g. in external yaml specification or similar.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Thanks.

}
}

service ServiceWithResponseBody {
Expand Down