Ext_proc: changing the append default value to be true and protect it with a runtime

changelogs/current.yaml

@@ -13,5 +13,12 @@ removed_config_or_runtime:
 # *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`
 
 new_features:
+- area: ext_proc
+  change: |
+    Added runtime guard ``envoy.reloadable_features.ext_proc_modified_append_default_value``.
+    The default value of the runtime is kept as false to keep the legacy behavior. Stats counters and error logs are added if
+    :ref:`append <envoy_v3_api_field_config.core.v3.HeaderValueOption.append>` in
+    :ref:`external processor service <envoy_v3_api_msg_service.ext_proc.v3.ProcessingResponse>` is not explicitly encoded.
+    These data will give us information whether it is safe to change the default value of ``append`` to be true.
 
 deprecated:

source/common/runtime/runtime_features.cc

@@ -179,6 +179,7 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_use_network_type_socket_option);
 FALSE_RUNTIME_GUARD(envoy_reloadable_features_quic_disable_client_early_data);
 
 FALSE_RUNTIME_GUARD(envoy_reloadable_features_ext_proc_graceful_grpc_close);
+FALSE_RUNTIME_GUARD(envoy_reloadable_features_ext_proc_modified_append_default_value);
 
 // Block of non-boolean flags. Use of int flags is deprecated. Do not add more.
 ABSL_FLAG(uint64_t, re2_max_program_size_error_level, 100, ""); // NOLINT

source/extensions/filters/http/ext_proc/ext_proc.cc

@@ -1540,7 +1540,7 @@ void Filter::sendImmediateResponse(const ImmediateResponse& response) {
     if (response.has_headers()) {
       const absl::Status mut_status = MutationUtils::applyHeaderMutations(
           response.headers(), headers, false, config().mutationChecker(),
-          stats_.rejected_header_mutations_);
+          stats_.rejected_header_mutations_, stats_.invalid_header_append_encoding_);
       if (!mut_status.ok()) {
         ENVOY_LOG_EVERY_POW_2(error, "Immediate response mutations failed with {}",
                               mut_status.message());

source/extensions/filters/http/ext_proc/ext_proc.h

@@ -44,6 +44,7 @@ namespace ExternalProcessing {
   COUNTER(failure_mode_allowed)                                                                    \
   COUNTER(message_timeouts)                                                                        \
   COUNTER(rejected_header_mutations)                                                               \
+  COUNTER(invalid_header_append_encoding)                                                          \
   COUNTER(override_message_timeout_received)                                                       \
   COUNTER(override_message_timeout_ignored)                                                        \
   COUNTER(clear_route_cache_ignored)                                                               \

source/extensions/filters/http/ext_proc/mutation_utils.cc

@@ -106,10 +106,36 @@ absl::Status MutationUtils::headerMutationResultCheck(const Http::HeaderMap& hea
   return absl::OkStatus();
 }
 
+// TODO(yanjunxiang-google): Remove the invalid_header_append_encoding counter when removing the
+// runtime.
+bool MutationUtils::getAppendFromHeaderMutation(
+    const envoy::config::core::v3::HeaderValueOption& set_header,
+    Stats::Counter& invalid_append_encoding) {
+  bool append;
+  if (!Runtime::runtimeFeatureEnabled(
+          "envoy.reloadable_features.ext_proc_modified_append_default_value")) {
+    // This is legacy case. i.e, only "append" is supported, and default "append"" is false.
+    append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(set_header, append, false);
+
+    // As "append" is WKT, it needs to be explicitly encoded based on the
+    // API guide. Log an error and increment the counter in case it is not set.
+    if (!set_header.has_append()) {
+      ENVOY_LOG_EVERY_POW_2(error, "set_headers append value is not explicitly set, this is wrong");
+      invalid_append_encoding.inc();
+    }
+  } else {
+    // This is new behavior with default append as true. And "append_action" support
+    // can be added in this case.
+    append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(set_header, append, true);
+  }
+  return append;
+}
+
 absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation,
                                                  Http::HeaderMap& headers, bool replacing_message,
                                                  const Checker& checker,
                                                  Counter& rejected_mutations,
+                                                 Counter& invalid_append_encoding,
                                                  bool remove_content_length) {
   // Check whether the remove_headers or set_headers size exceed the HTTP connection manager limit.
   // Reject the mutation and return error status if either one does.
@@ -162,7 +188,7 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation,
       return absl::InvalidArgumentError("Invalid character in set_headers mutation.");
     }
     const LowerCaseString header_name(sh.header().key());
-    const bool append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(sh, append, false);
+    const bool append = getAppendFromHeaderMutation(sh, invalid_append_encoding);
     const auto check_op = (append && !headers.get(header_name).empty()) ? CheckOperation::APPEND
                                                                         : CheckOperation::SET;
     auto check_result = checker.check(check_op, header_name, header_value);

source/extensions/filters/http/ext_proc/mutation_utils.h

@@ -33,7 +33,8 @@ class MutationUtils : public Logger::Loggable<Logger::Id::ext_proc> {
   applyHeaderMutations(const envoy::service::ext_proc::v3::HeaderMutation& mutation,
                        Http::HeaderMap& headers, bool replacing_message,
                        const Filters::Common::MutationRules::Checker& rule_checker,
-                       Stats::Counter& rejected_mutations, bool remove_content_length = false);
+                       Stats::Counter& rejected_mutations, Stats::Counter& invalid_append_encoding,
+                       bool remove_content_length = false);
 
   // Modify a buffer based on a set of mutations from a protobuf
   static void applyBodyMutations(const envoy::service::ext_proc::v3::BodyMutation& mutation,
@@ -59,6 +60,9 @@ class MutationUtils : public Logger::Loggable<Logger::Id::ext_proc> {
   // Check whether the header size after mutation is over the HCM size config.
   static absl::Status headerMutationResultCheck(const Http::HeaderMap& headers,
                                                 Stats::Counter& rejected_mutations);
+  static bool
+  getAppendFromHeaderMutation(const envoy::config::core::v3::HeaderValueOption& set_header,
+                              Stats::Counter& invalid_append_encoding);
 };
 
 } // namespace ExternalProcessing

source/extensions/filters/http/ext_proc/processor_state.cc

@@ -114,7 +114,7 @@ absl::Status ProcessorState::processHeaderMutation(const CommonResponse& common_
       common_response.header_mutation(), *headers_,
       common_response.status() == CommonResponse::CONTINUE_AND_REPLACE,
       filter_.config().mutationChecker(), filter_.stats().rejected_header_mutations_,
-      shouldRemoveContentLength());
+      filter_.stats().invalid_header_append_encoding_, shouldRemoveContentLength());
   return mut_status;
 }
 
@@ -511,7 +511,8 @@ absl::Status ProcessorState::handleTrailersResponse(const TrailersResponse& resp
     if (response.has_header_mutation() && trailers_ != nullptr) {
       auto mut_status = MutationUtils::applyHeaderMutations(
           response.header_mutation(), *trailers_, false, filter_.config().mutationChecker(),
-          filter_.stats().rejected_header_mutations_);
+          filter_.stats().rejected_header_mutations_,
+          filter_.stats().invalid_header_append_encoding_);
       if (!mut_status.ok()) {
         filter_.onProcessTrailersResponse(response, mut_status, trafficDirection());
         return mut_status;

test/extensions/filters/http/ext_proc/BUILD

@@ -65,6 +65,7 @@ envoy_extension_cc_test(
         "//test/mocks/server:server_factory_context_mocks",
         "//test/proto:helloworld_proto_cc_proto",
         "//test/test_common:registry_lib",
+        "//test/test_common:test_runtime_lib",
         "//test/test_common:utility_lib",
         "@envoy_api//envoy/config/core/v3:pkg_cc_proto",
         "@envoy_api//envoy/extensions/http/ext_proc/response_processors/save_processing_response/v3:pkg_cc_proto",

test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc

@@ -122,6 +122,8 @@ class ExtProcIntegrationTest : public HttpIntegrationTest,
   void initializeConfig(ConfigOptions config_option = {},
                         const std::vector<std::pair<int, int>>& cluster_endpoints = {{0, 1},
                                                                                      {1, 1}}) {
+    scoped_runtime_.mergeValues(
+        {{"envoy.reloadable_features.ext_proc_modified_append_default_value", "false"}});
     int total_cluster_endpoints = 0;
     std::for_each(
         cluster_endpoints.begin(), cluster_endpoints.end(),

test/extensions/filters/http/ext_proc/filter_test.cc

@@ -36,6 +36,7 @@
 #include "test/mocks/upstream/cluster_manager.h"
 #include "test/test_common/printers.h"
 #include "test/test_common/registry.h"
+#include "test/test_common/test_runtime.h"
 #include "test/test_common/utility.h"
 
 #include "gmock/gmock.h"
@@ -97,6 +98,9 @@ static const std::string filter_config_name = "scooby.dooby.doo";
 class HttpFilterTest : public testing::Test {
 protected:
   void initialize(std::string&& yaml, bool is_upstream_filter = false) {
+    scoped_runtime_.mergeValues(
+        {{"envoy.reloadable_features.ext_proc_modified_append_default_value", "false"}});
+
     client_ = std::make_unique<MockClient>();
     route_ = std::make_shared<NiceMock<Router::MockRoute>>();
     EXPECT_CALL(*client_, start(_, _, _, _)).WillOnce(Invoke(this, &HttpFilterTest::doStart));
@@ -663,6 +667,7 @@ class HttpFilterTest : public testing::Test {
   envoy::config::core::v3::Metadata dynamic_metadata_;
   testing::NiceMock<Network::MockConnection> connection_;
   NiceMock<Server::Configuration::MockServerFactoryContext> factory_context_;
+  TestScopedRuntime scoped_runtime_;
 };
 
 // Using the default configuration, test the filter with a processor that
@@ -821,6 +826,69 @@ TEST_F(HttpFilterTest, PostAndChangeHeaders) {
   EXPECT_EQ(1, config_->stats().streams_closed_.value());
 }
 
+TEST_F(HttpFilterTest, PostAndChangeHeadersAppendDefaulFalse) {
+  initialize(R"EOF(
+  grpc_service:
+    envoy_grpc:
+      cluster_name: "ext_proc_server"
+  )EOF");
+
+  // Set the runtime to false to force append default to be false, which is legacy behavior.
+  scoped_runtime_.mergeValues(
+      {{"envoy.reloadable_features.ext_proc_modified_append_default_value", "false"}});
+
+  request_headers_.addCopy(LowerCaseString("x-some-other-header"), "yes");
+  EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false));
+  processRequestHeaders(
+      false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& header_resp) {
+        auto headers_mut = header_resp.mutable_response()->mutable_header_mutation();
+        auto add1 = headers_mut->add_set_headers();
+        add1->mutable_header()->set_key("x-some-other-header");
+        add1->mutable_header()->set_raw_value("no");
+      });
+
+  // We should now have changed the original header a bit
+  TestRequestHeaderMapImpl expected{{":path", "/"},
+                                    {":method", "POST"},
+                                    {":scheme", "http"},
+                                    {"host", "host"},
+                                    {"x-some-other-header", "no"}};
+  EXPECT_THAT(&request_headers_, HeaderMapEqualIgnoreOrder(&expected));
+  filter_->onDestroy();
+}
+
+TEST_F(HttpFilterTest, PostAndChangeHeadersAppendDefaulTrue) {
+  initialize(R"EOF(
+  grpc_service:
+    envoy_grpc:
+      cluster_name: "ext_proc_server"
+  )EOF");
+
+  // Set the runtime to true to force append default to be true.
+  scoped_runtime_.mergeValues(
+      {{"envoy.reloadable_features.ext_proc_modified_append_default_value", "true"}});
+
+  request_headers_.addCopy(LowerCaseString("x-some-other-header"), "yes");
+  EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false));
+  processRequestHeaders(
+      false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& header_resp) {
+        auto headers_mut = header_resp.mutable_response()->mutable_header_mutation();
+        auto add1 = headers_mut->add_set_headers();
+        add1->mutable_header()->set_key("x-some-other-header");
+        add1->mutable_header()->set_raw_value("no");
+      });
+
+  // We should now have changed the original header a bit
+  TestRequestHeaderMapImpl expected{{":path", "/"},
+                                    {":method", "POST"},
+                                    {":scheme", "http"},
+                                    {"host", "host"},
+                                    {"x-some-other-header", "yes"},
+                                    {"x-some-other-header", "no"}};
+  EXPECT_THAT(&request_headers_, HeaderMapEqualIgnoreOrder(&expected));
+  filter_->onDestroy();
+}
+
 // Using the default configuration, test the filter with a processor that
 // replies to the request_headers message with an "immediate response" message
 // that should result in a response being directly sent downstream with

test/extensions/filters/http/ext_proc/mutation_utils_test.cc

@@ -136,11 +136,13 @@ TEST_F(MutationUtilsTest, TestApplyMutations) {
 
   // Use the default mutation rules
   Checker checker(HeaderMutationRules::default_instance(), regex_engine_);
-  Envoy::Stats::MockCounter rejections;
+  Envoy::Stats::MockCounter rejections, invalid_append;
   EXPECT_CALL(rejections, inc()).Times(10);
+  EXPECT_CALL(invalid_append, inc()).Times(10);
   // There were 10 attempts to change un-changeable headers above.
-  EXPECT_TRUE(
-      MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok());
+  EXPECT_TRUE(MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections,
+                                                  invalid_append)
+                  .ok());
 
   Http::TestRequestHeaderMapImpl expected_headers{
       {":scheme", "https"},
@@ -185,10 +187,12 @@ TEST_F(MutationUtilsTest, TestNonAppendableHeaders) {
   // Use the default mutation rules
   Checker checker(HeaderMutationRules::default_instance(), regex_engine_);
   // There were two invalid attempts above.
-  Envoy::Stats::MockCounter rejections;
+  Envoy::Stats::MockCounter rejections, invalid_append;
   EXPECT_CALL(rejections, inc()).Times(2);
-  EXPECT_TRUE(
-      MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok());
+  EXPECT_CALL(invalid_append, inc());
+  EXPECT_TRUE(MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections,
+                                                  invalid_append)
+                  .ok());
 
   Http::TestRequestHeaderMapImpl expected_headers{
       {":path", "/foo"},
@@ -203,24 +207,26 @@ TEST_F(MutationUtilsTest, TestSetHeaderWithInvalidCharacter) {
       {"host", "localhost:1000"},
   };
   Checker checker(HeaderMutationRules::default_instance(), regex_engine_);
-  Envoy::Stats::MockCounter rejections;
+  Envoy::Stats::MockCounter rejections, invalid_append;
   envoy::service::ext_proc::v3::HeaderMutation mutation;
   auto* s = mutation.add_set_headers();
   // Test header key contains invalid character.
   s->mutable_header()->set_key("x-append-this\n");
   s->mutable_header()->set_raw_value("value");
   EXPECT_CALL(rejections, inc());
-  EXPECT_FALSE(
-      MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok());
+  EXPECT_FALSE(MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections,
+                                                   invalid_append)
+                   .ok());
 
   mutation.Clear();
   s = mutation.add_set_headers();
   // Test header value contains invalid character.
   s->mutable_header()->set_key("x-append-this");
   s->mutable_header()->set_raw_value("value\r");
   EXPECT_CALL(rejections, inc());
-  EXPECT_FALSE(
-      MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok());
+  EXPECT_FALSE(MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections,
+                                                   invalid_append)
+                   .ok());
 }
 
 TEST_F(MutationUtilsTest, TestSetHeaderWithContentLength) {
@@ -232,20 +238,23 @@ TEST_F(MutationUtilsTest, TestSetHeaderWithContentLength) {
   };
   // Use the default mutation rules
   Checker checker(HeaderMutationRules::default_instance(), regex_engine_);
-  Envoy::Stats::MockCounter rejections;
+  Envoy::Stats::MockCounter rejections, invalid_append;
   envoy::service::ext_proc::v3::HeaderMutation mutation;
   auto* s = mutation.add_set_headers();
   // Test header key contains content_length.
   s->mutable_header()->set_key("content-length");
   s->mutable_header()->set_raw_value("10");
 
   EXPECT_TRUE(MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections,
+                                                  invalid_append,
                                                   /*remove_content_length=*/true)
                   .ok());
   // When `remove_content_length` is true, content_length headers is not added.
   EXPECT_EQ(headers.ContentLength(), nullptr);
 
+  EXPECT_CALL(invalid_append, inc());
   EXPECT_TRUE(MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections,
+                                                  invalid_append,
                                                   /*remove_content_length=*/false)
                   .ok());
   // When `remove_content_length` is false, content_length headers is added.
@@ -260,10 +269,11 @@ TEST_F(MutationUtilsTest, TestRemoveHeaderWithInvalidCharacter) {
   envoy::service::ext_proc::v3::HeaderMutation mutation;
   mutation.add_remove_headers("host\n");
   Checker checker(HeaderMutationRules::default_instance(), regex_engine_);
-  Envoy::Stats::MockCounter rejections;
+  Envoy::Stats::MockCounter rejections, invalid_append;
   EXPECT_CALL(rejections, inc());
-  EXPECT_FALSE(
-      MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections).ok());
+  EXPECT_FALSE(MutationUtils::applyHeaderMutations(mutation, headers, false, checker, rejections,
+                                                   invalid_append)
+                   .ok());
 }
 
 // Ensure that we actually replace the body