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

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,10 @@ 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;
+
   for (const auto& sh : mutation.set_headers()) {
     if (!sh.has_header()) {
       continue;
@@ -174,40 +181,114 @@ 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);
+
+    bool append = false;
+    if (sh.has_append()) {
+      append = sh.append().value();
+      const auto check_op = (append && !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);
+      if (checkResult == CheckResult::FAIL) {
+        return absl::InvalidArgumentError(absl::StrCat(
+            "Invalid attempt to modify ", static_cast<absl::string_view>(header_name)));
+      }
+    } else {
+      switch (sh.append_action()) {
+      case HeaderValueOption::APPEND_IF_EXISTS_OR_ADD:
+        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)));
       }
-      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