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

Jump to bottom

[grpc-transcoder] Add option to pack unknown parameters into HttpBody extension #34999

Merged
merged 5 commits into from
Jul 9, 2024
Merged

[grpc-transcoder] Add option to pack unknown parameters into HttpBody extension #34999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
77ec6f2
[grpc-transcoder] Add option to pack unknown parameters into HttpBody…
ravenblackx Jul 1, 2024
f9ddcf3
Rename to UnknownQueryParams, allow multiple values
ravenblackx Jul 8, 2024
a179294
Add a dot-param to the test
ravenblackx Jul 8, 2024
aeb9991
Format
ravenblackx Jul 8, 2024
8625adb
Mimic existing protodoc
ravenblackx Jul 9, 2024
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
21 changes: 19 additions & 2 deletions api/envoy/extensions/filters/http/grpc_json_transcoder/v3/transcoder.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ 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: 17]
// [#next-free-field: 18]
// GrpcJsonTranscoder filter configuration.
// The filter itself can be used per route / per virtual host or on the general level. The most
// specific one is being used for a given route. If the list of services is empty - filter
Expand Down Expand Up @@ -88,7 +88,8 @@ message GrpcJsonTranscoder {
// When set to true, the request will be rejected with a ``HTTP 400 Bad Request``.
//
// The fields
// :ref:`ignore_unknown_query_parameters <envoy_v3_api_field_extensions.filters.http.grpc_json_transcoder.v3.GrpcJsonTranscoder.ignore_unknown_query_parameters>`
// :ref:`ignore_unknown_query_parameters <envoy_v3_api_field_extensions.filters.http.grpc_json_transcoder.v3.GrpcJsonTranscoder.ignore_unknown_query_parameters>`,
// :ref:`capture_unknown_query_parameters <envoy_v3_api_field_extensions.filters.http.grpc_json_transcoder.v3.GrpcJsonTranscoder.capture_unknown_query_parameters>`,
// and
// :ref:`ignored_query_parameters <envoy_v3_api_field_extensions.filters.http.grpc_json_transcoder.v3.GrpcJsonTranscoder.ignored_query_parameters>`
// have priority over this strict validation behavior.
Expand Down Expand Up @@ -288,4 +289,20 @@ message GrpcJsonTranscoder {
//
// If unset, the current stream buffer size is used.
google.protobuf.UInt32Value max_response_body_size = 16 [(validate.rules).uint32 = {gt: 0}];

// If true, query parameters that cannot be mapped to a corresponding
// protobuf field are captured in an HttpBody extension of UnknownQueryParams.
bool capture_unknown_query_parameters = 17;
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO rather than override we should reject a config which suggests doing 2 different things to unknown config params.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes a transition to this new behavior difficult for us, because config and binary can be pushed separately and we're currently operating on a patch that does the new behavior with the old config.

The transition plan for me is to push a new config that specifies both the things, then push the new binary, thereby rolling onto the non-patch implementation of the behavior.

I'd be much more comfortable if I could make this change in a followup if that would be okay?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think API changes which make previously valid config invalid are disallowed unfortunately.
Options would be to have a runtime guard default-false for the allow behavior, and you could flip it internally, or just allow it and I will put up with it with mild crankiness as I have in the myriad other instances I've asked folks to not allow for confusing behavior and the harsh realities of production have made it impossible =P

As a compromise how about at least logging a warning below if the prior config knob is ignored?

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 started to do that, then realized it doesn't make sense - "warning, your ignore directive is being ignored in favor of another setting that does still ignore it in the way you meant".

So instead I made the protodoc less confusing about what overrides what; capture doesn't really override ignore, it just also does ignore whether you set both or not. The only thing it really overrides is the same thing ignore also overrides, and that wasn't documented on ignore, it was only documented on the overridden field, so I've made the documentation for the new field match that.

Copy link
Contributor

Choose a reason for hiding this comment

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

well in that case there should be a warning for both ignore and send iff strict but there's prior art for not warning about that so I'm just going to declare myself an api grinch and LGTM this.

}

// ``UnknownQueryParams`` is added as an extension field in ``HttpBody`` if
// ``GrpcJsonTranscoder::capture_unknown_query_parameters`` is true and unknown query
// parameters were present in the request.
message UnknownQueryParams {
message Values {
repeated string values = 1;
}

// A map from unrecognized query parameter keys, to the values associated with those keys.
map<string, Values> key = 1;
}
1 change: 1 addition & 0 deletions source/extensions/filters/http/grpc_json_transcoder/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ envoy_cc_library(
deps = [
"//source/common/grpc:codec_lib",
"//source/common/protobuf",
"@envoy_api//envoy/extensions/filters/http/grpc_json_transcoder/v3:pkg_cc_proto",
],
)

Expand Down
10 changes: 8 additions & 2 deletions source/extensions/filters/http/grpc_json_transcoder/http_body_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
#include "source/extensions/filters/http/grpc_json_transcoder/http_body_utils.h"

#include "envoy/extensions/filters/http/grpc_json_transcoder/v3/transcoder.pb.h"

#include "google/api/httpbody.pb.h"

using envoy::extensions::filters::http::grpc_json_transcoder::v3::UnknownQueryParams;
using Envoy::Protobuf::io::CodedInputStream;
using Envoy::Protobuf::io::CodedOutputStream;
using Envoy::Protobuf::io::ZeroCopyInputStream;
Expand Down Expand Up @@ -60,8 +63,8 @@ bool HttpBodyUtils::parseMessageByFieldPath(
}

void HttpBodyUtils::appendHttpBodyEnvelope(
Buffer::Instance& output, const std::vector<const ProtobufWkt::Field*>& request_body_field_path,
std::string content_type, uint64_t content_length) {
Buffer::Instance& output, const std::vector<const Protobuf::Field*>& request_body_field_path,
std::string content_type, uint64_t content_length, const UnknownQueryParams& unknown_params) {
// Manually encode the protobuf envelope for the body.
// See https://developers.google.com/protocol-buffers/docs/encoding#embedded for wire format.

Expand All @@ -75,6 +78,9 @@ void HttpBodyUtils::appendHttpBodyEnvelope(

::google::api::HttpBody body;
body.set_content_type(std::move(content_type));
if (!unknown_params.key().empty()) {
body.add_extensions()->PackFrom(unknown_params);
}

uint64_t envelope_size = body.ByteSizeLong() +
CodedOutputStream::VarintSize32(http_body_field_number) +
Expand Down
10 changes: 6 additions & 4 deletions source/extensions/filters/http/grpc_json_transcoder/http_body_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#pragma once

#include "envoy/buffer/buffer.h"
#include "envoy/extensions/filters/http/grpc_json_transcoder/v3/transcoder.pb.h"

#include "source/common/buffer/buffer_impl.h"
#include "source/common/grpc/codec.h"
Expand All @@ -16,10 +17,11 @@ class HttpBodyUtils {
static bool parseMessageByFieldPath(Protobuf::io::ZeroCopyInputStream* stream,
const std::vector<const ProtobufWkt::Field*>& field_path,
Protobuf::Message* message);
static void
appendHttpBodyEnvelope(Buffer::Instance& output,
const std::vector<const ProtobufWkt::Field*>& request_body_field_path,
std::string content_type, uint64_t content_length);
static void appendHttpBodyEnvelope(
Buffer::Instance& output, const std::vector<const Protobuf::Field*>& request_body_field_path,
std::string content_type, uint64_t content_length,
const envoy::extensions::filters::http::grpc_json_transcoder::v3::UnknownQueryParams&
unknown_params);
};

} // namespace GrpcJsonTranscoder
Expand Down
20 changes: 15 additions & 5 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 @@ -18,6 +18,7 @@
#include "source/common/runtime/runtime_features.h"
#include "source/extensions/filters/http/grpc_json_transcoder/http_body_utils.h"

#include "absl/strings/str_join.h"
#include "google/api/annotations.pb.h"
#include "google/api/http.pb.h"
#include "google/api/httpbody.pb.h"
Expand All @@ -43,6 +44,7 @@ using google::grpc::transcoding::Transcoder;
using TranscoderPtr = std::unique_ptr<Transcoder>;
using google::grpc::transcoding::TranscoderInputStream;
using TranscoderInputStreamPtr = std::unique_ptr<TranscoderInputStream>;
using envoy::extensions::filters::http::grpc_json_transcoder::v3::UnknownQueryParams;

namespace Envoy {
namespace Extensions {
Expand Down Expand Up @@ -229,6 +231,7 @@ JsonTranscoderConfig::JsonTranscoderConfig(

match_incoming_request_route_ = proto_config.match_incoming_request_route();
ignore_unknown_query_parameters_ = proto_config.ignore_unknown_query_parameters();
capture_unknown_query_parameters_ = proto_config.capture_unknown_query_parameters();
request_validation_options_ = proto_config.request_validation_options();
case_insensitive_enum_parsing_ = proto_config.case_insensitive_enum_parsing();
if (proto_config.has_max_request_body_size()) {
Expand Down Expand Up @@ -327,7 +330,8 @@ bool JsonTranscoderConfig::convertGrpcStatus() const { return convert_grpc_statu
absl::Status JsonTranscoderConfig::createTranscoder(
const Http::RequestHeaderMap& headers, ZeroCopyInputStream& request_input,
google::grpc::transcoding::TranscoderInputStream& response_input,
std::unique_ptr<Transcoder>& transcoder, MethodInfoSharedPtr& method_info) const {
std::unique_ptr<Transcoder>& transcoder, MethodInfoSharedPtr& method_info,
UnknownQueryParams& unknown_params) const {

ASSERT(!disabled_);
const std::string method(headers.getMethodValue());
Expand Down Expand Up @@ -361,7 +365,11 @@ absl::Status JsonTranscoderConfig::createTranscoder(
status = type_helper_->ResolveFieldPath(*request_info.message_type, binding.field_path,
&resolved_binding.field_path);
if (!status.ok()) {
if (ignore_unknown_query_parameters_) {
if (capture_unknown_query_parameters_) {
auto binding_key = absl::StrJoin(binding.field_path, ".");
(*unknown_params.mutable_key())[binding_key].add_values(binding.value);
continue;
} else if (ignore_unknown_query_parameters_) {
continue;
}
return status;
Expand Down Expand Up @@ -464,8 +472,9 @@ Http::FilterHeadersStatus JsonTranscoderFilter::decodeHeaders(Http::RequestHeade
return Http::FilterHeadersStatus::Continue;
}

const auto status =
per_route_config_->createTranscoder(headers, request_in_, response_in_, transcoder_, method_);
const auto status = per_route_config_->createTranscoder(headers, request_in_, response_in_,
transcoder_, method_, unknown_params_);

if (!status.ok()) {
ENVOY_STREAM_LOG(debug, "Failed to transcode request headers: {}", *decoder_callbacks_,
status.message());
Expand Down Expand Up @@ -904,7 +913,8 @@ void JsonTranscoderFilter::maybeSendHttpBodyRequestMessage(Buffer::Instance* dat
stats_->transcoder_request_buffer_bytes_.sub(initial_request_data_.length());
message_payload.move(initial_request_data_);
HttpBodyUtils::appendHttpBodyEnvelope(message_payload, method_->request_body_field_path,
std::move(content_type_), request_data_.length());
std::move(content_type_), request_data_.length(),
unknown_params_);
content_type_.clear();
stats_->transcoder_request_buffer_bytes_.sub(request_data_.length());
message_payload.move(request_data_);
Expand Down
15 changes: 10 additions & 5 deletions source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,14 @@ class JsonTranscoderConfig : public Logger::Loggable<Logger::Id::config>,
* is not found, status with Code::NOT_FOUND is returned. If the method is found, but
* fields cannot be resolved, status with Code::INVALID_ARGUMENT is returned.
*/
absl::Status createTranscoder(const Http::RequestHeaderMap& headers,
Protobuf::io::ZeroCopyInputStream& request_input,
google::grpc::transcoding::TranscoderInputStream& response_input,
std::unique_ptr<google::grpc::transcoding::Transcoder>& transcoder,
MethodInfoSharedPtr& method_info) const;
absl::Status
createTranscoder(const Http::RequestHeaderMap& headers,
Protobuf::io::ZeroCopyInputStream& request_input,
google::grpc::transcoding::TranscoderInputStream& response_input,
std::unique_ptr<google::grpc::transcoding::Transcoder>& transcoder,
MethodInfoSharedPtr& method_info,
envoy::extensions::filters::http::grpc_json_transcoder::v3::UnknownQueryParams&
unknown_params) const;

/**
* Converts an arbitrary protobuf message to JSON.
Expand Down Expand Up @@ -135,6 +138,7 @@ class JsonTranscoderConfig : public Logger::Loggable<Logger::Id::config>,

bool match_incoming_request_route_{false};
bool ignore_unknown_query_parameters_{false};
bool capture_unknown_query_parameters_{false};
bool convert_grpc_status_{false};
bool case_insensitive_enum_parsing_{false};

Expand Down Expand Up @@ -217,6 +221,7 @@ class JsonTranscoderFilter : public Http::StreamFilter, public Logger::Loggable<
Http::StreamDecoderFilterCallbacks* decoder_callbacks_{};
Http::StreamEncoderFilterCallbacks* encoder_callbacks_{};
MethodInfoSharedPtr method_;
envoy::extensions::filters::http::grpc_json_transcoder::v3::UnknownQueryParams unknown_params_;
Http::ResponseHeaderMap* response_headers_{};
Grpc::Decoder decoder_;

Expand Down
1 change: 1 addition & 0 deletions test/extensions/filters/http/grpc_json_transcoder/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ envoy_extension_cc_test(
"//source/common/buffer:zero_copy_input_stream_lib",
"//source/extensions/filters/http/grpc_json_transcoder:http_body_utils_lib",
"//test/proto:bookstore_proto_cc_proto",
"@envoy_api//envoy/extensions/filters/http/grpc_json_transcoder/v3:pkg_cc_proto",
],
)

Expand Down
41 changes: 39 additions & 2 deletions test/extensions/filters/http/grpc_json_transcoder/http_body_utils_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,25 @@
#include "envoy/extensions/filters/http/grpc_json_transcoder/v3/transcoder.pb.h"

#include "source/common/buffer/buffer_impl.h"
#include "source/common/buffer/zero_copy_input_stream_impl.h"
#include "source/extensions/filters/http/grpc_json_transcoder/http_body_utils.h"

#include "test/proto/bookstore.pb.h"

#include "gmock/gmock.h"
#include "google/api/httpbody.pb.h"
#include "gtest/gtest.h"

using testing::ElementsAre;

namespace Envoy {
namespace Extensions {
namespace HttpFilters {
namespace GrpcJsonTranscoder {
namespace {

using envoy::extensions::filters::http::grpc_json_transcoder::v3::UnknownQueryParams;

class HttpBodyUtilsTest : public testing::Test {
public:
HttpBodyUtilsTest() = default;
Expand All @@ -38,7 +45,7 @@ class HttpBodyUtilsTest : public testing::Test {
{
Buffer::InstancePtr message_buffer = std::make_unique<Buffer::OwnedImpl>();
HttpBodyUtils::appendHttpBodyEnvelope(*message_buffer, body_field_path_, content_type,
content.length());
content.length(), UnknownQueryParams{});
message_buffer->add(content);

Buffer::ZeroCopyInputStreamImpl stream(std::move(message_buffer));
Expand All @@ -55,7 +62,7 @@ class HttpBodyUtilsTest : public testing::Test {
{
Buffer::InstancePtr message_buffer = std::make_unique<Buffer::OwnedImpl>();
HttpBodyUtils::appendHttpBodyEnvelope(*message_buffer, body_field_path_, content_type,
content.length());
content.length(), UnknownQueryParams{});
message_buffer->add(content);

google::api::HttpBody http_body;
Expand All @@ -79,6 +86,35 @@ class HttpBodyUtilsTest : public testing::Test {
std::vector<const ProtobufWkt::Field*> body_field_path_;
};

TEST_F(HttpBodyUtilsTest, UnknownQueryParamsAppearInExtension) {
Buffer::InstancePtr message_buffer = std::make_unique<Buffer::OwnedImpl>();
UnknownQueryParams unknown_params;
auto& foo_key = (*unknown_params.mutable_key())["foo"];
foo_key.add_values("bar");
foo_key.add_values("baz");
const std::string content = "abcd";
const std::string content_type = "text/plain";
HttpBodyUtils::appendHttpBodyEnvelope(*message_buffer, {}, content_type, content.length(),
unknown_params);
message_buffer->add(content);

Buffer::ZeroCopyInputStreamImpl stream(std::move(message_buffer));

google::api::HttpBody http_body;
http_body.ParseFromZeroCopyStream(&stream);

EXPECT_EQ(http_body.content_type(), content_type);
EXPECT_EQ(http_body.data(), content);
ASSERT_EQ(http_body.extensions_size(), 1);
ASSERT_EQ(http_body.extensions(0).type_url(),
"type.googleapis.com/"
"envoy.extensions.filters.http.grpc_json_transcoder.v3.UnknownQueryParams");
UnknownQueryParams unknown_vars_out;
http_body.extensions(0).UnpackTo(&unknown_vars_out);
ASSERT_TRUE(unknown_vars_out.key().contains("foo"));
EXPECT_THAT(unknown_vars_out.key().at("foo").values(), ElementsAre("bar", "baz"));
}

TEST_F(HttpBodyUtilsTest, EmptyFieldsList) {
basicTest<google::api::HttpBody>("abcd", "text/plain", {},
[](google::api::HttpBody http_body) { return http_body; });
Expand Down Expand Up @@ -125,6 +161,7 @@ TEST_F(HttpBodyUtilsTest, SkipUnknownFields) {
EXPECT_TRUE(HttpBodyUtils::parseMessageByFieldPath(&stream, body_field_path_, &http_body));
EXPECT_EQ(http_body.content_type(), "text/nested");
EXPECT_EQ(http_body.data(), "abcd");
EXPECT_TRUE(http_body.extensions().empty());
}

TEST_F(HttpBodyUtilsTest, FailInvalidLength) {
Expand Down
Loading