ext_proc: Honor HeaderValueOption.append_action, keep append option as well for backward compatibility

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

@@ -347,7 +347,13 @@ message HeaderValueOption {
   // Describes the supported actions types for header append action.
   enum HeaderAppendAction {
     // This action will append the specified value to the existing values if the header
-    // already exists. If the header doesn't exist then this will add the header with
+    // already exists and the header is inline header.
+    // set-cookie is special predefined inline header that allows to be multi headers present
+    // rather than concatenation.
+    // Reject append request on single-value header like host, content-length.
+    // You can find more detailed information at RFC 7230 section 3.2.2.
+    //
+    // If the header doesn't exist then this will add the header with
     // specified key and value.
     APPEND_IF_EXISTS_OR_ADD = 0;
 

changelogs/current.yaml

@@ -352,3 +352,8 @@ deprecated:
   change: |
     deprecated runtime key ``overload.global_downstream_max_connections`` in favor of :ref:`downstream connections monitor
     <envoy_v3_api_msg_extensions.resource_monitors.downstream_connections.v3.DownstreamConnectionsConfig>`.
+- area: http ext_proc filter
+  change: |
+    Change to hornor append_action <envoy_v3_api_field_config.core.v3.HeaderValueOption.append_action>.
+    This change can be reverted temporarily by setting the runtime guard
+    ``envoy.reloadable_features.header_value_option_change_action`` to false.

source/common/runtime/runtime_features.cc

@@ -127,6 +127,8 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_enable_universal_header_validator)
 FALSE_RUNTIME_GUARD(envoy_reloadable_features_no_downgrade_to_canonical_name);
 // TODO(pksohn): enable after fixing https://github.com/envoyproxy/envoy/issues/29930
 FALSE_RUNTIME_GUARD(envoy_reloadable_features_quic_defer_logging_to_ack_listener);
+// TODO(ronnie-personal): enable by default after a complete deprecation period.
+FALSE_RUNTIME_GUARD(envoy_reloadable_features_header_value_option_change_action)
 
 // 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/mutation_utils.cc

@@ -24,6 +24,9 @@ using Stats::Counter;
 using envoy::service::ext_proc::v3::BodyMutation;
 using envoy::service::ext_proc::v3::HeaderMutation;
 
+using HeaderAppendAction = envoy::config::core::v3::HeaderValueOption::HeaderAppendAction;
+using HeaderValueOption = envoy::config::core::v3::HeaderValueOption;
+
 bool MutationUtils::headerInMatcher(
     absl::string_view key, const std::vector<Matchers::StringMatcherPtr>& header_matchers) {
   return std::any_of(header_matchers.begin(), header_matchers.end(),
@@ -146,6 +149,11 @@ absl::Status MutationUtils::applyHeaderMutations(const HeaderMutation& mutation,
     }
   }
 
+  bool append_mode_for_append_action;
+  Filters::Common::MutationRules::CheckOperation check_op_for_append_action;
+  Filters::Common::MutationRules::CheckResult checkResult_for_append_action;
+  auto is_duplicate = false;
+
   for (const auto& sh : mutation.set_headers()) {
     if (!sh.has_header()) {
       continue;
@@ -174,40 +182,131 @@ 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 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;
-    }
-    switch (check_result) {
-    case CheckResult::OK:
-      ENVOY_LOG(trace, "Setting header {} append = {}", sh.header().key(), append);
-      if (append) {
-        headers.addCopy(header_name, header_value);
-      } else {
-        headers.setCopy(header_name, header_value);
+
+    if (sh.append_action().has_value()) {
+      switch (sh.append_action()) {
+      case HeaderValueOption::APPEND_IF_EXISTS_OR_ADD:
+        // Check if the header already exists with the same name and value.
+        is_duplicate = false;
+        if (!headers.get(header_name).empty()) {
+          Http::HeaderMap::GetResult result = headers.get(header_name);
+          absl::string_view existing_value;
+          for (size_t i = 0; i < result.size(); ++i) {
+            existing_value = result[i]->value().getStringView();
+
+            // Compare the existing value with your desired header_value
+            if (existing_value == header_value) {
+              is_duplicate = true;
+              break;
+            }
+          }
+        }
+        if (!is_duplicate) {
+          append_mode_for_append_action = true;
+          check_op_for_append_action =
+              (!headers.get(header_name).empty()) ? CheckOperation::APPEND : CheckOperation::SET;
+          checkResult_for_append_action = handleCheckResult(
+              headers, replacing_message, checker, rejected_mutations, check_op_for_append_action,
+              header_name, header_value, append_mode_for_append_action);
+          if (checkResult_for_append_action == CheckResult::FAIL) {
+            return absl::InvalidArgumentError(absl::StrCat(
+                "Invalid attempt to modify ", static_cast<absl::string_view>(header_name)));
+          }
+        }
+        break;
+      case HeaderValueOption::ADD_IF_ABSENT:
+        if (headers.get(header_name).empty()) {
+          append_mode_for_append_action = true;
+          check_op_for_append_action = CheckOperation::SET;
+          checkResult_for_append_action = handleCheckResult(
+              headers, replacing_message, checker, rejected_mutations, check_op_for_append_action,
+              header_name, header_value, append_mode_for_append_action);
+          if (checkResult_for_append_action == CheckResult::FAIL) {
+            return absl::InvalidArgumentError(absl::StrCat(
+                "Invalid attempt to modify ", static_cast<absl::string_view>(header_name)));
+          }
+        }
+        break;
+      case HeaderValueOption::OVERWRITE_IF_EXISTS_OR_ADD:
+        append_mode_for_append_action = false;
+        check_op_for_append_action = CheckOperation::SET;
+        checkResult_for_append_action = handleCheckResult(
+            headers, replacing_message, checker, rejected_mutations, check_op_for_append_action,
+            header_name, header_value, append_mode_for_append_action);
+        if (checkResult_for_append_action == CheckResult::FAIL) {
+          return absl::InvalidArgumentError(absl::StrCat(
+              "Invalid attempt to modify ", static_cast<absl::string_view>(header_name)));
+        }
+        break;
+      case HeaderValueOption::OVERWRITE_IF_EXISTS:
+        if (!headers.get(header_name).empty()) {
+          append_mode_for_append_action = false;
+          check_op_for_append_action = CheckOperation::SET;
+          checkResult_for_append_action = handleCheckResult(
+              headers, replacing_message, checker, rejected_mutations, check_op_for_append_action,
+              header_name, header_value, append_mode_for_append_action);
+          if (checkResult_for_append_action == CheckResult::FAIL) {
+            return absl::InvalidArgumentError(absl::StrCat(
+                "Invalid attempt to modify ", static_cast<absl::string_view>(header_name)));
+          }
+        }
+        break;
+      default:
+        // Handle unknown/invalid append_action value
+        return absl::InvalidArgumentError(absl::StrCat(
+            "Invalid append_action value ", static_cast<absl::string_view>(header_name)));
+      }
+    } else {
+      const bool append_mode = PROTOBUF_GET_WRAPPED_OR_DEFAULT(sh, append, false);
+      const auto check_op = (append_mode && !headers.get(header_name).empty())
+                                ? CheckOperation::APPEND
+                                : CheckOperation::SET;
+      auto checkResult = handleCheckResult(headers, replacing_message, checker, rejected_mutations,
+                                           check_op, header_name, header_value, append_mode);
+      if (checkResult == CheckResult::FAIL) {
+        return absl::InvalidArgumentError(absl::StrCat(
+            "Invalid attempt to modify ", static_cast<absl::string_view>(header_name)));
       }
-      break;
-    case CheckResult::IGNORE:
-      ENVOY_LOG(debug, "Header {} may not be modified per rules", header_name);
-      rejected_mutations.inc();
-      break;
-    case CheckResult::FAIL:
-      ENVOY_LOG(debug, "Header {} may not be modified. Returning error", header_name);
-      rejected_mutations.inc();
-      return absl::InvalidArgumentError(
-          absl::StrCat("Invalid attempt to modify ", static_cast<absl::string_view>(header_name)));
     }
   }
 
   // After header mutation, check the ending headers are not exceeding the HCM limit.
   return headerMutationResultCheck(headers, rejected_mutations);
 }
 
+// Define a common function to handle the check result logic
+Filters::Common::MutationRules::CheckResult MutationUtils::handleCheckResult(
+    Http::HeaderMap& headers, bool replacing_message,
+    const Filters::Common::MutationRules::Checker& checker, Stats::Counter& rejected_mutations,
+    Filters::Common::MutationRules::CheckOperation check_op, Http::LowerCaseString header_name,
+    absl::string_view header_value, bool append_mode) {
+  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;
+  }
+  // Print the value as an integer
+  switch (check_result) {
+  case CheckResult::OK:
+    if (append_mode) {
+      headers.addCopy(header_name, header_value);
+    } else {
+      headers.setCopy(header_name, header_value);
+    }
+    break;
+  case CheckResult::IGNORE:
+    rejected_mutations.inc();
+    break;
+  case CheckResult::FAIL:
+    rejected_mutations.inc();
+    // You can add additional handling for the FAIL case here if needed.
+    break;
+  }
+
+  return check_result;
+}
+
 void MutationUtils::applyBodyMutations(const BodyMutation& mutation, Buffer::Instance& buffer) {
   switch (mutation.mutation_case()) {
   case BodyMutation::MutationCase::kClearBody:

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

@@ -58,6 +58,12 @@ 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 Filters::Common::MutationRules::CheckResult handleCheckResult(
+      Http::HeaderMap& headers, bool replacing_message,
+      const Filters::Common::MutationRules::Checker& checker, Stats::Counter& rejected_mutations,
+      Filters::Common::MutationRules::CheckOperation check_op, Http::LowerCaseString header_name,
+      absl::string_view header_value, bool append_mode);
 };
 
 } // namespace ExternalProcessing

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

@@ -3293,6 +3293,103 @@ TEST_F(HttpFilter2Test, LastEncodeDataCallExceedsStreamBufferLimitWouldJustRaise
   conn_manager_->onData(fake_input, false);
 }
 
+TEST_F(HttpFilterTest, AppendActionTest) {
+  TestScopedRuntime scoped_runtime;
+  scoped_runtime.mergeValues({
+      {"envoy.reloadable_features.send_header_raw_value", "false"},
+      {"envoy.reloadable_features.header_value_option_change_action", "true"},
+  });
+  initialize(R"EOF(
+  grpc_service:
+    envoy_grpc:
+      cluster_name: "ext_proc_server"
+  )EOF");
+
+  // Initialize request headers.
+  request_headers_.addCopy(LowerCaseString("x-original-header"), "original_value");
+
+  EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false));
+
+  // Apply the "append_action" to add a value to an existing header.
+  processRequestHeaders(
+      false, [](const HttpHeaders&, ProcessingResponse&, HeadersResponse& header_resp) {
+        auto headers_mut = header_resp.mutable_response()->mutable_header_mutation();
+        auto s = headers_mut->add_set_headers();
+        s->mutable_header()->set_key("x-original-header");
+        s->mutable_header()->set_value("appended_value");
+        s->set_append_action(::envoy::config::core::v3::HeaderValueOption_HeaderAppendAction::
+                                 HeaderValueOption_HeaderAppendAction_APPEND_IF_EXISTS_OR_ADD);
+        s = headers_mut->add_set_headers();
+        s->mutable_header()->set_key("another-request");
+        s->mutable_header()->set_value("request");
+        s->set_append_action(::envoy::config::core::v3::HeaderValueOption_HeaderAppendAction::
+                                 HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS_OR_ADD);
+      });
+
+  // Check that the header was appended correctly.
+  TestRequestHeaderMapImpl expected{{":path", "/"},
+                                    {":method", "POST"},
+                                    {":scheme", "http"},
+                                    {"host", "host"},
+                                    {"x-original-header", "original_value"},
+                                    {"x-original-header", "appended_value"},
+                                    {"another-request", "request"}};
+  EXPECT_THAT(&request_headers_, HeaderMapEqualIgnoreOrder(&expected));
+
+  // Continue processing data and trailers.
+  Buffer::OwnedImpl req_data("foo");
+  EXPECT_EQ(FilterDataStatus::Continue, filter_->decodeData(req_data, true));
+  EXPECT_EQ(FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers_));
+
+  // Initialize response headers.
+  response_headers_.addCopy(LowerCaseString(":status"), "200");
+  response_headers_.addCopy(LowerCaseString("content-type"), "text/plain");
+  response_headers_.addCopy(LowerCaseString("content-length"), "3");
+
+  EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->encodeHeaders(response_headers_, false));
+
+  // Apply the "append_action" to response headers.
+  processResponseHeaders(false, [](const HttpHeaders& response_headers, ProcessingResponse&,
+                                   HeadersResponse& header_resp) {
+    EXPECT_FALSE(response_headers.end_of_stream());
+    TestRequestHeaderMapImpl expected_response{
+        {":status", "200"}, {"content-type", "text/plain"}, {"content-length", "3"}};
+    EXPECT_THAT(response_headers.headers(), HeaderProtosEqual(expected_response));
+
+    auto* resp_headers_mut = header_resp.mutable_response()->mutable_header_mutation();
+    auto* s = resp_headers_mut->add_set_headers();
+    s->mutable_header()->set_key("x-original-response-header");
+    s->mutable_header()->set_value("appended_response_value");
+    s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction::
+                             HeaderValueOption_HeaderAppendAction_ADD_IF_ABSENT);
+    s = resp_headers_mut->add_set_headers();
+    s->mutable_header()->set_key("x-original-response-header");
+    s->mutable_header()->set_value("second value");
+    s->set_append_action(envoy::config::core::v3::HeaderValueOption_HeaderAppendAction::
+                             HeaderValueOption_HeaderAppendAction_OVERWRITE_IF_EXISTS);
+  });
+
+  // Check that the response header was appended correctly.
+  TestRequestHeaderMapImpl final_expected_response{{":status", "200"},
+                                                   {"content-type", "text/plain"},
+                                                   {"content-length", "3"},
+                                                   {"x-original-response-header", "second value"}};
+  EXPECT_THAT(&response_headers_, HeaderMapEqualIgnoreOrder(&final_expected_response));
+
+  // Continue processing data, encode trailers, and destroy the filter.
+  Buffer::OwnedImpl resp_data("bar");
+  EXPECT_EQ(FilterDataStatus::Continue, filter_->encodeData(resp_data, false));
+  Buffer::OwnedImpl empty_data;
+  EXPECT_EQ(FilterDataStatus::Continue, filter_->encodeData(empty_data, true));
+  EXPECT_EQ(FilterTrailersStatus::Continue, filter_->encodeTrailers(response_trailers_));
+  filter_->onDestroy();
+
+  EXPECT_EQ(1, config_->stats().streams_started_.value());
+  EXPECT_EQ(2, config_->stats().stream_msgs_sent_.value());
+  EXPECT_EQ(2, config_->stats().stream_msgs_received_.value());
+  EXPECT_EQ(1, config_->stats().streams_closed_.value());
+}
+
 } // namespace
 } // namespace ExternalProcessing
 } // namespace HttpFilters