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

api/envoy/config/core/v3/base.proto

@@ -461,11 +461,6 @@ message HeaderValueOption {
   // existing values. Otherwise it replaces any existing values.
   // This field is deprecated and please use
   // :ref:`append_action <envoy_v3_api_field_config.core.v3.HeaderValueOption.append_action>` as replacement.
-  //
-  // .. note::
-  //   The :ref:`external authorization service <envoy_v3_api_msg_service.auth.v3.CheckResponse>` and
-  //   :ref:`external processor service <envoy_v3_api_msg_service.ext_proc.v3.ProcessingResponse>` have
-  //   default value (``false``) for this field.
   google.protobuf.BoolValue append = 2
       [deprecated = true, (envoy.annotations.deprecated_at_minor_version) = "3.0"];
 

changelogs/current.yaml

@@ -328,6 +328,14 @@ new_features:
     Added an :ref:`extension
     <envoy_v3_api_msg_extensions.http.ext_proc.response_processors.save_processing_response.v3.SaveProcessingResponse>` to save the
     :ref:`response <envoy_v3_api_msg_service.ext_proc.v3.ProcessingResponse>` from the external processor to filter state.
+- area: ext_proc
+  change: |
+    Added runtime guard ``envoy.reloadable_features.ext_proc_modified_append_default_value`` and keep the default as true.
+    The default value of :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 set to this runtime value.
+    This is a breaking change for those users who have their ext_proc servers not explictly encoding ``append`` to false,
+    and leave it as default. Users with this kind of ext_proc servers should flip the runtime guard to false first to revert to
+    the legacy behavior, at same time start to change the ext_proc server to always explictly encode ``append``.
 - area: http
   change: |
     Made the :ref:`credential injector filter <envoy_v3_api_msg_extensions.filters.http.credential_injector.v3.CredentialInjector>`

source/common/runtime/runtime_features.cc

@@ -42,6 +42,7 @@ RUNTIME_GUARD(envoy_reloadable_features_enable_include_histograms);
 RUNTIME_GUARD(envoy_reloadable_features_enable_new_query_param_present_match_behavior);
 RUNTIME_GUARD(envoy_reloadable_features_enable_udp_proxy_outlier_detection);
 RUNTIME_GUARD(envoy_reloadable_features_explicit_internal_address_config);
+RUNTIME_GUARD(envoy_reloadable_features_ext_proc_modified_append_default_value);
 RUNTIME_GUARD(envoy_reloadable_features_ext_proc_timeout_error);
 RUNTIME_GUARD(envoy_reloadable_features_extend_h3_accept_untrusted);
 RUNTIME_GUARD(envoy_reloadable_features_filter_access_loggers_first);
@@ -179,7 +180,6 @@ 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);
-
 // 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
 ABSL_FLAG(uint64_t, re2_max_program_size_warn_level,            // NOLINT

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

@@ -162,7 +162,10 @@ 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 = PROTOBUF_GET_WRAPPED_OR_DEFAULT(
+        sh, append,
+        Runtime::runtimeFeatureEnabled(
+            "envoy.reloadable_features.ext_proc_modified_append_default_value"));
     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);

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/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