header_mutation: Apply header mutations specified in all route levels (…

…envoyproxy#30220) * header mutation with all route level Signed-off-by: tyxia <tyxia@google.com> * remove dead code Signed-off-by: tyxia <tyxia@google.com> * remove dead code Signed-off-by: tyxia <tyxia@google.com> * utility function Signed-off-by: tyxia <tyxia@google.com> * fix format Signed-off-by: tyxia <tyxia@google.com> * fix format Signed-off-by: tyxia <tyxia@google.com> * fix typo Signed-off-by: tyxia <tyxia@google.com> * update header mutation test to match new impl Signed-off-by: tyxia <tyxia@google.com> * update test Signed-off-by: tyxia <tyxia@google.com> * tweak Signed-off-by: tyxia <tyxia@google.com> * fix format Signed-off-by: tyxia <tyxia@google.com> * fix Signed-off-by: tyxia <tyxia@google.com> * merge fix Signed-off-by: tyxia <tyxia@google.com> * fix format Signed-off-by: tyxia <tyxia@google.com> * add config option for specific Signed-off-by: tyxia <tyxia@google.com> * add release note Signed-off-by: tyxia <tyxia@google.com> * reword the comment Signed-off-by: tyxia <tyxia@google.com> * fix typo Signed-off-by: tyxia <tyxia@google.com> * fix format Signed-off-by: tyxia <tyxia@google.com> * test coverage Signed-off-by: tyxia <tyxia@google.com> * nit Signed-off-by: tyxia <tyxia@google.com> * reword Signed-off-by: tyxia <tyxia@google.com> * fix Signed-off-by: tyxia <tyxia@google.com> * coverage Signed-off-by: tyxia <tyxia@google.com> * comment Signed-off-by: tyxia <tyxia@google.com> * fix typo Signed-off-by: tyxia <tyxia@google.com> * remove Signed-off-by: tyxia <tyxia@google.com> * retrigger CI Signed-off-by: tyxia <tyxia@google.com> * set most_specific_wins false Signed-off-by: tyxia <tyxia@google.com> * tweak comments Signed-off-by: tyxia <tyxia@google.com> * update Signed-off-by: tyxia <tyxia@google.com> * fix format Signed-off-by: tyxia <tyxia@google.com> * update per review Signed-off-by: tyxia <tyxia@google.com> --------- Signed-off-by: tyxia <tyxia@google.com>
ohadvano · Jan 11, 2024 · 5b7bbef · 5b7bbef
1 parent 7bd0c0f
commit 5b7bbef
Show file tree

Hide file tree

Showing 8 changed files with 735 additions and 93 deletions.
diff --git a/api/envoy/extensions/filters/http/header_mutation/v3/header_mutation.proto b/api/envoy/extensions/filters/http/header_mutation/v3/header_mutation.proto
@@ -23,8 +23,7 @@ message Mutations {
   repeated config.common.mutation_rules.v3.HeaderMutation response_mutations = 2;
 }
 
-// Per route configuration for the header mutation filter. If this is configured at multiple levels
-// (route level, virtual host level, and route table level), only the most specific one will be used.
+// Per route configuration for the header mutation filter.
 message HeaderMutationPerRoute {
   Mutations mutations = 1;
 }
@@ -33,4 +32,12 @@ message HeaderMutationPerRoute {
 // always be applied first and then the per-route mutation rules, if both are specified.
 message HeaderMutation {
   Mutations mutations = 1;
+
+  // If per route HeaderMutationPerRoute config is configured at multiple route levels, header mutations
+  // at all specified levels are evaluated. By default, the order is from most specific (i.e. route entry level)
+  // to least specific (i.e. route configuration level). Later header mutations may override earlier mutations.
+  //
+  // This order can be reversed by setting this field to true. In other words, most specific level mutation
+  // is evaluated last.
+  bool most_specific_header_mutations_wins = 2;
 }
diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -84,6 +84,10 @@ minor_behavior_changes:
 - area: generic_proxy
   change: |
     Update the stats prefix of generic proxy from ``<stats_prefix>`` to ``generic_proxy.<stats_prefix>``.
+- area: header_mutation
+  change: |
+    If per route configuration is configured at multiple levels (route, virtual host, and route table), all specified
+    levels' mutations are applied. Default order is from least to most specific level (i.e. most specific level wins).
 
 bug_fixes:
 # *Changes expected to improve the state of the world and are unlikely to have negative effects*

diff --git a/source/common/http/utility.h b/source/common/http/utility.h
@@ -626,6 +626,33 @@ getMergedPerFilterConfig(const Http::StreamFilterCallbacks* callbacks,
   return merged;
 }
 
+/**
+ * Return all the available per route filter configs.
+ *
+ * @param callbacks The stream filter callbacks to check for route configs.
+ *
+ * @return The all available per route config. The returned pointers are guaranteed to be non-null
+ * and their lifetime is the same as the matched route.
+ */
+template <class ConfigType>
+absl::InlinedVector<const ConfigType*, 3>
+getAllPerFilterConfig(const Http::StreamFilterCallbacks* callbacks) {
+  ASSERT(callbacks != nullptr);
+
+  absl::InlinedVector<const ConfigType*, 3> all_configs;
+  callbacks->traversePerFilterConfig([&all_configs](const Router::RouteSpecificFilterConfig& cfg) {
+    const ConfigType* typed_cfg = dynamic_cast<const ConfigType*>(&cfg);
+    if (typed_cfg == nullptr) {
+      ENVOY_LOG_MISC(debug, "Failed to retrieve the correct type of route specific filter config");
+      return;
+    }
+
+    all_configs.push_back(typed_cfg);
+  });
+
+  return all_configs;
+}
+
 struct AuthorityAttributes {
   // whether parsed authority is pure ip address(IPv4/IPv6), if it is true
   // passed that are not FQDN

diff --git a/source/extensions/filters/http/header_mutation/header_mutation.cc b/source/extensions/filters/http/header_mutation/header_mutation.cc
@@ -28,20 +28,32 @@ PerRouteHeaderMutation::PerRouteHeaderMutation(const PerRouteProtoConfig& config
     : mutations_(config.mutations()) {}
 
 HeaderMutationConfig::HeaderMutationConfig(const ProtoConfig& config)
-    : mutations_(config.mutations()) {}
+    : mutations_(config.mutations()),
+      most_specific_header_mutations_wins_(config.most_specific_header_mutations_wins()) {}
 
 Http::FilterHeadersStatus HeaderMutation::decodeHeaders(Http::RequestHeaderMap& headers, bool) {
   Formatter::HttpFormatterContext ctx{&headers};
   config_->mutations().mutateRequestHeaders(headers, ctx, decoder_callbacks_->streamInfo());
 
-  // Only the most specific route config is used.
-  // TODO(wbpcode): It's possible to traverse all the route configs to merge the header mutations
-  // in the future.
-  route_config_ =
-      Http::Utility::resolveMostSpecificPerFilterConfig<PerRouteHeaderMutation>(decoder_callbacks_);
-
-  if (route_config_ != nullptr) {
-    route_config_->mutations().mutateRequestHeaders(headers, ctx, decoder_callbacks_->streamInfo());
+  // Traverse through all route configs to retrieve all available header mutations.
+  // `getAllPerFilterConfig` returns in ascending order of specificity (i.e., route table
+  // first, then virtual host, then per route).
+  route_configs_ = Http::Utility::getAllPerFilterConfig<PerRouteHeaderMutation>(decoder_callbacks_);
+
+  if (!config_->mostSpecificHeaderMutationsWins()) {
+    // most_specific_wins means that most specific level per filter config is evaluated last. In
+    // other words, header mutations are evaluated in ascending order of specificity (same order as
+    // `getAllPerFilterConfig` above returns).
+    // Thus, here we reverse iterate the vector when `most_specific_wins` is false.
+    for (auto it = route_configs_.rbegin(); it != route_configs_.rend(); ++it) {
+      (*it)->mutations().mutateRequestHeaders(headers, ctx, decoder_callbacks_->streamInfo());
+    }
+  } else {
+    for (const auto* route_config : route_configs_) {
+      ASSERT(route_config != nullptr);
+      route_config->mutations().mutateRequestHeaders(headers, ctx,
+                                                     decoder_callbacks_->streamInfo());
+    }
   }
 
   return Http::FilterHeadersStatus::Continue;
@@ -51,15 +63,22 @@ Http::FilterHeadersStatus HeaderMutation::encodeHeaders(Http::ResponseHeaderMap&
   Formatter::HttpFormatterContext ctx{encoder_callbacks_->requestHeaders().ptr(), &headers};
   config_->mutations().mutateResponseHeaders(headers, ctx, encoder_callbacks_->streamInfo());
 
-  if (route_config_ == nullptr) {
-    // If we haven't already resolved the route config, do so now.
-    route_config_ = Http::Utility::resolveMostSpecificPerFilterConfig<PerRouteHeaderMutation>(
-        encoder_callbacks_);
+  // If we haven't already traversed the route configs, do so now.
+  if (route_configs_.empty()) {
+    route_configs_ =
+        Http::Utility::getAllPerFilterConfig<PerRouteHeaderMutation>(encoder_callbacks_);
   }
 
-  if (route_config_ != nullptr) {
-    route_config_->mutations().mutateResponseHeaders(headers, ctx,
-                                                     encoder_callbacks_->streamInfo());
+  if (!config_->mostSpecificHeaderMutationsWins()) {
+    for (auto it = route_configs_.rbegin(); it != route_configs_.rend(); ++it) {
+      (*it)->mutations().mutateResponseHeaders(headers, ctx, encoder_callbacks_->streamInfo());
+    }
+  } else {
+    for (const auto* route_config : route_configs_) {
+      ASSERT(route_config != nullptr);
+      route_config->mutations().mutateResponseHeaders(headers, ctx,
+                                                      encoder_callbacks_->streamInfo());
+    }
   }
 
   return Http::FilterHeadersStatus::Continue;

diff --git a/source/extensions/filters/http/header_mutation/header_mutation.h b/source/extensions/filters/http/header_mutation/header_mutation.h
@@ -56,8 +56,11 @@ class HeaderMutationConfig {
 
   const Mutations& mutations() const { return mutations_; }
 
+  bool mostSpecificHeaderMutationsWins() const { return most_specific_header_mutations_wins_; }
+
 private:
   Mutations mutations_;
+  const bool most_specific_header_mutations_wins_;
 };
 using HeaderMutationConfigSharedPtr = std::shared_ptr<HeaderMutationConfig>;
 
@@ -73,7 +76,8 @@ class HeaderMutation : public Http::PassThroughFilter, public Logger::Loggable<L
 
 private:
   HeaderMutationConfigSharedPtr config_{};
-  const PerRouteHeaderMutation* route_config_{};
+  // The lifetime of route config pointers is same as the matched route.
+  absl::InlinedVector<const PerRouteHeaderMutation*, 3> route_configs_{};
 };
 
 } // namespace HeaderMutation

diff --git a/test/extensions/filters/http/header_mutation/BUILD b/test/extensions/filters/http/header_mutation/BUILD
@@ -51,5 +51,6 @@ envoy_extension_cc_test(
         "//test/mocks/server:instance_mocks",
         "//test/test_common:registry_lib",
         "//test/test_common:utility_lib",
+        "@com_google_absl//absl/strings:str_format",
     ],
 )