[grpc-transcoder] Add option to pack unknown parameters into HttpBody…

… extension (envoyproxy#34999) Commit Message: [grpc-transcoder] Add option to pack unknown parameters into HttpBody extension Additional Description: We've been using this behavior for years, with PR envoyproxy#15338 as a patch. Finally getting around to trying to upstream the behavior to make it available for others, and to make it so I don't have to keep repositioning the patch. Unlike envoyproxy#15338 I'm also adding a configuration option so that no behavior change will occur without a corresponding configuration change. Risk Level: Very low, guarded by a new config field. Testing: Added positive unit tests, added conditions to other tests for the negative case. Docs Changes: Autogen Fixes envoyproxy#14710 --------- Signed-off-by: Raven Black <[email protected]>
makesoftwaresafe · Jul 9, 2024 · f837c48 · f837c48
1 parent e8da3a5
commit f837c48
Show file tree

Hide file tree

Showing 9 changed files with 158 additions and 40 deletions.
diff --git a/api/envoy/extensions/filters/http/grpc_json_transcoder/v3/transcoder.proto b/api/envoy/extensions/filters/http/grpc_json_transcoder/v3/transcoder.proto
@@ -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
@@ -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.
@@ -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;
+}
+
+// ``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;
 }
diff --git a/source/extensions/filters/http/grpc_json_transcoder/BUILD b/source/extensions/filters/http/grpc_json_transcoder/BUILD
@@ -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",
     ],
 )
 

diff --git a/source/extensions/filters/http/grpc_json_transcoder/http_body_utils.cc b/source/extensions/filters/http/grpc_json_transcoder/http_body_utils.cc
@@ -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;
@@ -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.
 
@@ -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) +

diff --git a/source/extensions/filters/http/grpc_json_transcoder/http_body_utils.h b/source/extensions/filters/http/grpc_json_transcoder/http_body_utils.h
@@ -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"
@@ -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

diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc
@@ -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"
@@ -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 {
@@ -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()) {
@@ -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());
@@ -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;
@@ -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());
@@ -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_);

diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h
@@ -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.
@@ -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};
 
@@ -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_;
 

diff --git a/test/extensions/filters/http/grpc_json_transcoder/BUILD b/test/extensions/filters/http/grpc_json_transcoder/BUILD
@@ -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",
     ],
 )
 

diff --git a/test/extensions/filters/http/grpc_json_transcoder/http_body_utils_test.cc b/test/extensions/filters/http/grpc_json_transcoder/http_body_utils_test.cc
@@ -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;
@@ -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));
@@ -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;
@@ -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; });
@@ -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) {