ext_proc: support HeaderAppendAction in mutation utils

changelogs/current.yaml

@@ -24,6 +24,14 @@ behavior_changes:
     rather than waiting for the original request to complete. This allows shadowing requests larger than the buffer limit,
     but also means shadowing may take place for requests which are canceled mid-stream. This behavior change can be
     temporarily reverted by flipping  ``envoy.reloadable_features.streaming_shadow`` to false.
+- area: ext_proc
+  change: |
+    The default behavior for header mutations in ext_proc changed. Previously, header values would be replaced by default,
+    and setting append to true was required for appending. Now, header values will be appended by default if append is
+    not specified. The old behavior can be temporarily restored by setting runtime guard
+    ``envoy.reloadable_features.ext_proc_legacy_append`` to ``true``. For more control over header mutation behavior,
+    use the :ref:`append_action <envoy_v3_api_field_config.core.v3.HeaderValueOption.append_action>` field which will
+    be the only supported option in the future.
 
 minor_behavior_changes:
 # *Changes that may cause incompatibilities for some users, but should not for most*
@@ -265,6 +273,11 @@ new_features:
   change: |
     Added support in SNI dynamic forward proxy for saving the resolved upstream address in the filter state.
     The state is saved with the key ``envoy.stream.upstream_address``.
+- area: ext_proc
+  change: |
+    Added support for ``HeaderAppendAction`` in ext_proc mutation utils, allowing more granular control over header
+    modifications using ``APPEND_IF_EXISTS_OR_ADD``, ``ADD_IF_ABSENT``, ``OVERWRITE_IF_EXISTS``, and
+    ``OVERWRITE_IF_EXISTS_OR_ADD`` actions. The existing append behavior continues to work as before.
 
 deprecated:
 - area: rbac

source/common/runtime/runtime_features.cc

@@ -113,6 +113,9 @@ RUNTIME_GUARD(envoy_restart_features_use_eds_cache_for_ads);
 
 // Begin false flags. Most of them should come with a TODO to flip true.
 
+// Used to temporarily allow the legacy append behavior on ext_proc header mutations
+FALSE_RUNTIME_GUARD(envoy_reloadable_features_ext_proc_legacy_append);
+
 // Sentinel and test flag.
 FALSE_RUNTIME_GUARD(envoy_reloadable_features_test_feature_false);
 // TODO(adisuissa) reset to true to enable unified mux by default

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

@@ -161,20 +161,66 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation,
       rejected_mutations.inc();
       return absl::InvalidArgumentError("Invalid character in set_headers mutation.");
     }
+
+    auto check_op = CheckOperation::SET;
+    auto should_append = false;
     const LowerCaseString header_name(sh.header().key());
-    const bool append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(sh, append, false);
-    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);
-    if (replacing_message && header_name == Http::Headers::get().Method) {
-      // Special handling to allow changing ":method" when the
-      // CONTINUE_AND_REPLACE option is selected, to stay compatible.
-      check_result = CheckResult::OK;
+    const auto header_exists = !headers.get(header_name).empty();
+
+    if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.ext_proc_legacy_append")) {
+      // Legacy append behavior
+      should_append = PROTOBUF_GET_WRAPPED_OR_DEFAULT(sh, append, false);
+      check_op = (should_append && header_exists) ? CheckOperation::APPEND : CheckOperation::SET;
+    } else {
+      if (sh.has_append()) {
+        // 'append' is set and ensure the 'append_action' value is equal to the default value.
+        if (sh.append_action() !=
+            envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD) {
+          return absl::InvalidArgumentError(
+              "Both append and append_action are set and it's not allowed");
+        }
+
+        // Legacy append behavior
+        should_append = sh.append().value();
+        check_op = (should_append && header_exists) ? CheckOperation::APPEND : CheckOperation::SET;
+      } else {
+        switch (sh.append_action()) {
+          PANIC_ON_PROTO_ENUM_SENTINEL_VALUES;
+        case envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD:
+          should_append = true;
+          check_op = CheckOperation::APPEND;
+          break;
+        case envoy::config::core::v3::HeaderValueOption::ADD_IF_ABSENT:
+          if (header_exists) {
+            continue; // Skip if header exists
+          }
+          should_append = false;
+          check_op = CheckOperation::SET;
+          break;
+        case envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS:
+          if (!header_exists) {
+            continue; // Skip if header doesn't exist
+          }
+          should_append = false;
+          check_op = CheckOperation::SET;
+          break;
+        case envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD:
+          should_append = false;
+          check_op = CheckOperation::SET;
+          break;
+        }
+      }
     }
-    switch (check_result) {
+
+    const auto check_result = checker.check(check_op, header_name, header_value);
+    const auto final_result = replacing_message && header_name == Http::Headers::get().Method
+                                  ? CheckResult::OK
+                                  : check_result;
+
+    switch (final_result) {
     case CheckResult::OK:
-      ENVOY_LOG(trace, "Setting header {} append = {}", sh.header().key(), append);
-      if (append) {
+      ENVOY_LOG(trace, "Setting header {} append = {}", sh.header().key(), should_append);
+      if (should_append) {
         headers.addCopy(header_name, header_value);
       } else {
         headers.setCopy(header_name, header_value);

test/extensions/filters/http/ext_proc/BUILD

@@ -151,6 +151,7 @@ envoy_extension_cc_test(
         "//source/extensions/filters/http/ext_proc:mutation_utils_lib",
         "//test/mocks/server:server_factory_context_mocks",
         "//test/mocks/stats:stats_mocks",
+        "//test/test_common:test_runtime_lib",
         "//test/test_common:utility_lib",
         "@envoy_api//envoy/config/common/mutation_rules/v3:pkg_cc_proto",
     ],

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

@@ -1131,6 +1131,8 @@ TEST_P(ExtProcIntegrationTest, SetHostHeaderRoutingSucceeded) {
 
         // Set host header to match the domain of virtual host in routing configuration.
         auto* mut = response_header_mutation->add_set_headers();
+        mut->set_append_action(
+            envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD);
         mut->mutable_header()->set_key(":authority");
         mut->mutable_header()->set_raw_value(vhost_domain);
 
@@ -1222,18 +1224,26 @@ TEST_P(ExtProcIntegrationTest, GetAndSetPathHeader) {
 
         auto response_header_mutation = headers_resp.mutable_response()->mutable_header_mutation();
         auto* mut1 = response_header_mutation->add_set_headers();
+        mut1->set_append_action(
+            envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD);
         mut1->mutable_header()->set_key(":path");
         mut1->mutable_header()->set_raw_value("/mutated_path/bluh");
 
         auto* mut2 = response_header_mutation->add_set_headers();
+        mut2->set_append_action(
+            envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD);
         mut2->mutable_header()->set_key(":scheme");
         mut2->mutable_header()->set_raw_value("https");
 
         auto* mut3 = response_header_mutation->add_set_headers();
+        mut3->set_append_action(
+            envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD);
         mut3->mutable_header()->set_key(":authority");
         mut3->mutable_header()->set_raw_value("new_host");
 
         auto* mut4 = response_header_mutation->add_set_headers();
+        mut4->set_append_action(
+            envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD);
         mut4->mutable_header()->set_key(":method");
         mut4->mutable_header()->set_raw_value("POST");
         return true;
@@ -1606,9 +1616,13 @@ TEST_P(ExtProcIntegrationTest, GetAndSetHeadersOnResponse) {
       *grpc_upstreams_[0], false, [](const HttpHeaders&, HeadersResponse& headers_resp) {
         auto* response_mutation = headers_resp.mutable_response()->mutable_header_mutation();
         auto* add1 = response_mutation->add_set_headers();
+        add1->set_append_action(
+            envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD);
         add1->mutable_header()->set_key("x-response-processed");
         add1->mutable_header()->set_raw_value("1");
         auto* add2 = response_mutation->add_set_headers();
+        add2->set_append_action(
+            envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD);
         add2->mutable_header()->set_key(":status");
         add2->mutable_header()->set_raw_value("201");
         return true;
@@ -1661,9 +1675,13 @@ TEST_P(ExtProcIntegrationTest, GetAndSetHeadersOnResponseTwoStatuses) {
       *grpc_upstreams_[0], false, [](const HttpHeaders&, HeadersResponse& headers_resp) {
         auto* response_mutation = headers_resp.mutable_response()->mutable_header_mutation();
         auto* add1 = response_mutation->add_set_headers();
+        add1->set_append_action(
+            envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD);
         add1->mutable_header()->set_key("x-response-processed");
         add1->mutable_header()->set_raw_value("1");
         auto* add2 = response_mutation->add_set_headers();
+        add2->set_append_action(
+            envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD);
         add2->mutable_header()->set_key(":status");
         add2->mutable_header()->set_raw_value("201");
         auto* add3 = response_mutation->add_set_headers();
@@ -2362,6 +2380,10 @@ TEST_P(ExtProcIntegrationTest, GetAndIncorrectlyModifyHeaderOnBodyPartialBuffer)
 // Test the ability of the filter to turn a GET into a POST by adding a body
 // and changing the method.
 TEST_P(ExtProcIntegrationTest, ConvertGetToPost) {
+  // NOTE: The use of `append` has been deprecated in favor of `append_action`. This is a temporary
+  // provision for users who have enabled legacy behavior, which will be phased out entirely in the
+  // future.
+  scoped_runtime_.mergeValues({{"envoy.reloadable_features.ext_proc_legacy_append", "true"}});
   initializeConfig();
   HttpIntegrationTest::initialize();
 
@@ -3033,9 +3055,13 @@ TEST_P(ExtProcIntegrationTest, PerRouteGrpcService) {
       *grpc_upstreams_[1], false, [](const HttpHeaders&, HeadersResponse& headers_resp) {
         auto* response_mutation = headers_resp.mutable_response()->mutable_header_mutation();
         auto* add1 = response_mutation->add_set_headers();
+        add1->set_append_action(
+            envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD);
         add1->mutable_header()->set_key("x-response-processed");
         add1->mutable_header()->set_raw_value("1");
         auto* add2 = response_mutation->add_set_headers();
+        add2->set_append_action(
+            envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD);
         add2->mutable_header()->set_key(":status");
         add2->mutable_header()->set_raw_value("201");
         return true;
@@ -5244,4 +5270,161 @@ TEST_P(ExtProcIntegrationTest, ModeOverrideDisallowed) {
   verifyDownstreamResponse(*response, 200);
 }
 
+TEST_P(ExtProcIntegrationTest, HeaderAppendAction) {
+  initializeConfig();
+  HttpIntegrationTest::initialize();
+
+  auto response = sendDownstreamRequest([](Http::HeaderMap& headers) {
+    headers.addCopy(LowerCaseString("x-append-test"), "original");
+    headers.addCopy(LowerCaseString("x-overwrite-test"), "original");
+  });
+
+  // Test different append actions
+  processRequestHeadersMessage(
+      *grpc_upstreams_[0], true, [](const HttpHeaders& /*headers*/, HeadersResponse& headers_resp) {
+        auto* response_mutation = headers_resp.mutable_response()->mutable_header_mutation();
+
+        // Append to existing header
+        auto* mut1 = response_mutation->add_set_headers();
+        mut1->set_append_action(
+            envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD);
+        mut1->mutable_header()->set_key("x-append-test");
+        mut1->mutable_header()->set_raw_value("appended");
+
+        // Add new header
+        auto* mut2 = response_mutation->add_set_headers();
+        mut2->set_append_action(envoy::config::core::v3::HeaderValueOption::ADD_IF_ABSENT);
+        mut2->mutable_header()->set_key("x-new-absent");
+        mut2->mutable_header()->set_raw_value("new");
+
+        // Overwrite existing header
+        auto* mut3 = response_mutation->add_set_headers();
+        mut3->set_append_action(envoy::config::core::v3::HeaderValueOption::OVERWRITE_IF_EXISTS);
+        mut3->mutable_header()->set_key("x-overwrite-test");
+        mut3->mutable_header()->set_raw_value("overwritten");
+
+        return true;
+      });
+
+  ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_));
+  ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_));
+  ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_));
+
+  // Get a copy of upstream headers for comparison
+  Http::TestRequestHeaderMapImpl expected_request_headers{
+      {":scheme", "http"},
+      {":method", "GET"},
+      {":path", "/"},
+      {":authority", "host"},
+      {"x-append-test", "original"},
+      {"x-append-test", "appended"},
+      {"x-new-absent", "new"},
+      {"x-overwrite-test", "overwritten"},
+      {"x-envoy-expected-rq-timeout-ms", "15000"},
+      {"x-forwarded-proto", "http"}};
+
+  Http::TestRequestHeaderMapImpl upstream_headers;
+  const auto& headers = upstream_request_->headers();
+  headers.iterate([&upstream_headers](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate {
+    // Skip x-request-id header
+    if (header.key().getStringView() != "x-request-id") {
+      upstream_headers.addCopy(Http::LowerCaseString(std::string(header.key().getStringView())),
+                               std::string(header.value().getStringView()));
+    }
+    return Http::HeaderMap::Iterate::Continue;
+  });
+
+  EXPECT_THAT(&upstream_headers, HeaderMapEqualIgnoreOrder(&expected_request_headers));
+
+  // Complete the request
+  upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, true);
+  verifyDownstreamResponse(*response, 504);
+}
+
+TEST_P(ExtProcIntegrationTest, HeaderAppendActionWithResponseHeaders) {
+  initializeConfig();
+  HttpIntegrationTest::initialize();
+
+  auto response = sendDownstreamRequest(absl::nullopt);
+  processRequestHeadersMessage(*grpc_upstreams_[0], true, absl::nullopt);
+  handleUpstreamRequest();
+
+  processResponseHeadersMessage(
+      *grpc_upstreams_[0], false,
+      [](const HttpHeaders& /*headers*/, HeadersResponse& headers_resp) {
+        auto* response_mutation = headers_resp.mutable_response()->mutable_header_mutation();
+
+        auto* mut1 = response_mutation->add_set_headers();
+        mut1->set_append_action(
+            envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD);
+        mut1->mutable_header()->set_key("x-multiple-test");
+        mut1->mutable_header()->set_raw_value("first");
+
+        auto* mut2 = response_mutation->add_set_headers();
+        mut2->set_append_action(
+            envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD);
+        mut2->mutable_header()->set_key("x-multiple-test");
+        mut2->mutable_header()->set_raw_value("second");
+
+        return true;
+      });
+
+  ASSERT_TRUE(response->waitForEndStream());
+  ASSERT_TRUE(response->complete());
+
+  const Http::ResponseHeaderMap& response_headers = response->headers();
+  EXPECT_EQ(
+      response_headers.get(Http::LowerCaseString("x-multiple-test"))[0]->value().getStringView(),
+      "first");
+  EXPECT_EQ(
+      response_headers.get(Http::LowerCaseString("x-multiple-test"))[1]->value().getStringView(),
+      "second");
+  EXPECT_EQ(response->headers().getStatusValue(), "200");
+}
+
+TEST_P(ExtProcIntegrationTest, HeaderAppendActionWithTrailers) {
+  proto_config_.mutable_processing_mode()->set_request_header_mode(ProcessingMode::SEND);
+  proto_config_.mutable_processing_mode()->set_request_trailer_mode(ProcessingMode::SEND);
+  initializeConfig();
+  HttpIntegrationTest::initialize();
+
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+  Http::TestRequestHeaderMapImpl headers;
+  HttpTestUtility::addDefaultHeaders(headers);
+  auto encoder_decoder = codec_client_->startRequest(headers);
+  request_encoder_ = &encoder_decoder.first;
+  auto response = std::move(encoder_decoder.second);
+
+  Http::TestRequestTrailerMapImpl request_trailers{
+      {"x-trailer-test", "original"},
+  };
+  codec_client_->sendTrailers(*request_encoder_, request_trailers);
+
+  processRequestHeadersMessage(*grpc_upstreams_[0], true, absl::nullopt);
+
+  processRequestTrailersMessage(
+      *grpc_upstreams_[0], false, [](const HttpTrailers& /*trailers*/, TrailersResponse& resp) {
+        auto* trailer_mut = resp.mutable_header_mutation();
+        auto* mut = trailer_mut->add_set_headers();
+        mut->set_append_action(envoy::config::core::v3::HeaderValueOption::APPEND_IF_EXISTS_OR_ADD);
+        mut->mutable_header()->set_key("x-trailer-test");
+        mut->mutable_header()->set_raw_value("appended");
+        return true;
+      });
+
+  ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_));
+  ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_));
+  ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_));
+
+  ASSERT_TRUE(upstream_request_->trailers());
+  const auto& trailer_values =
+      upstream_request_->trailers()->get(Http::LowerCaseString("x-trailer-test"));
+  ASSERT_EQ(trailer_values.size(), 2);
+  EXPECT_EQ(trailer_values[0]->value().getStringView(), "original");
+  EXPECT_EQ(trailer_values[1]->value().getStringView(), "appended");
+
+  upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, true);
+  verifyDownstreamResponse(*response, 504);
+}
+
 } // namespace Envoy