auto-merge envoyproxy/envoy[main] into envoyproxy/envoy-openssl[main]

* upstream/main: route: use reference wrapper for get all filter config (#36079)
envoyproxy · Sep 17, 2024 · 30304fd · 30304fd
2 parents 6a1f8d7 + a4ad9c2
commit 30304fd
Show file tree

Hide file tree

Showing 10 changed files with 84 additions and 95 deletions.
diff --git a/contrib/golang/filters/http/source/golang_filter.cc b/contrib/golang/filters/http/source/golang_filter.cc
@@ -1377,16 +1377,13 @@ void Filter::deferredDeleteRequest(HttpRequestInternal* req) {
 uint64_t Filter::getMergedConfigId() {
   Http::StreamFilterCallbacks* callbacks = decoding_state_.getFilterCallbacks();
 
+  auto id = config_->getConfigId();
+
   // get all of the per route config
   auto route_config_list = Http::Utility::getAllPerFilterConfig<FilterConfigPerRoute>(callbacks);
-
   ENVOY_LOG(debug, "golang filter route config list length: {}.", route_config_list.size());
-
-  auto id = config_->getConfigId();
-  for (auto it : route_config_list) {
-    ASSERT(it != nullptr, "route config should not be null");
-    auto route_config = *it;
-    id = route_config.getPluginConfigId(id, config_->pluginName());
+  for (const FilterConfigPerRoute& typed_config : route_config_list) {
+    id = typed_config.getPluginConfigId(id, config_->pluginName());
   }
 
   return id;

diff --git a/source/common/http/utility.h b/source/common/http/utility.h
@@ -2,6 +2,7 @@
 
 #include <chrono>
 #include <cstdint>
+#include <functional>
 #include <memory>
 #include <string>
 #include <vector>
@@ -572,19 +573,19 @@ const ConfigType* resolveMostSpecificPerFilterConfig(const Http::StreamFilterCal
  * and their lifetime is the same as the matched route.
  */
 template <class ConfigType>
-absl::InlinedVector<const ConfigType*, 4>
+absl::InlinedVector<std::reference_wrapper<const ConfigType>, 4>
 getAllPerFilterConfig(const Http::StreamFilterCallbacks* callbacks) {
   ASSERT(callbacks != nullptr);
 
-  absl::InlinedVector<const ConfigType*, 4> all_configs;
+  absl::InlinedVector<std::reference_wrapper<const ConfigType>, 4> all_configs;
 
   for (const auto* config : callbacks->perFilterConfigs()) {
     const ConfigType* typed_config = dynamic_cast<const ConfigType*>(config);
     if (typed_config == nullptr) {
       ENVOY_LOG_MISC(debug, "Failed to retrieve the correct type of route specific filter config");
       continue;
     }
-    all_configs.push_back(typed_config);
+    all_configs.push_back(*typed_config);
   }
 
   return all_configs;

diff --git a/source/extensions/filters/http/cors/cors_filter.cc b/source/extensions/filters/http/cors/cors_filter.cc
@@ -66,10 +66,17 @@ void CorsFilter::initializeCorsPolicies() {
   // If no cors policy is configured in the per filter config, then the cors policy fields in the
   // route configuration will be ignored.
   if (policies_.empty()) {
-    policies_ = {
-        decoder_callbacks_->route()->routeEntry()->corsPolicy(),
-        decoder_callbacks_->route()->virtualHost().corsPolicy(),
-    };
+    const auto route = decoder_callbacks_->route();
+    ASSERT(route != nullptr);
+    ASSERT(route->routeEntry() != nullptr);
+
+    if (auto* typed_cfg = route->routeEntry()->corsPolicy(); typed_cfg != nullptr) {
+      policies_.push_back(*typed_cfg);
+    }
+
+    if (auto* typed_cfg = route->virtualHost().corsPolicy(); typed_cfg != nullptr) {
+      policies_.push_back(*typed_cfg);
+    }
   }
 }
 
@@ -204,104 +211,96 @@ void CorsFilter::setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& c
 }
 
 bool CorsFilter::isOriginAllowed(const Http::HeaderString& origin) {
-  const auto allow_origins = allowOrigins();
-  if (allow_origins == nullptr) {
-    return false;
-  }
-  for (const auto& allow_origin : *allow_origins) {
+  for (const auto& allow_origin : allowOrigins()) {
     if (allow_origin->match("*") || allow_origin->match(origin.getStringView())) {
       return true;
     }
   }
   return false;
 }
 
-const std::vector<Matchers::StringMatcherPtr>* CorsFilter::allowOrigins() {
-  for (const auto policy : policies_) {
-    if (policy && !policy->allowOrigins().empty()) {
-      return &policy->allowOrigins();
+absl::Span<const Matchers::StringMatcherPtr> CorsFilter::allowOrigins() {
+  for (const Router::CorsPolicy& policy : policies_) {
+    if (!policy.allowOrigins().empty()) {
+      return policy.allowOrigins();
     }
   }
-  return nullptr;
+  return {};
 }
 
 bool CorsFilter::forwardNotMatchingPreflights() {
-  for (const auto policy : policies_) {
-    if (policy && policy->forwardNotMatchingPreflights()) {
-      return policy->forwardNotMatchingPreflights().value();
+  for (const Router::CorsPolicy& policy : policies_) {
+    if (policy.forwardNotMatchingPreflights()) {
+      return policy.forwardNotMatchingPreflights().value();
     }
   }
   return true;
 }
 
-const std::string& CorsFilter::allowMethods() {
-  for (const auto policy : policies_) {
-    if (policy && !policy->allowMethods().empty()) {
-      return policy->allowMethods();
+absl::string_view CorsFilter::allowMethods() {
+  for (const Router::CorsPolicy& policy : policies_) {
+    if (!policy.allowMethods().empty()) {
+      return policy.allowMethods();
     }
   }
   return EMPTY_STRING;
 }
 
-const std::string& CorsFilter::allowHeaders() {
-  for (const auto policy : policies_) {
-    if (policy && !policy->allowHeaders().empty()) {
-      return policy->allowHeaders();
+absl::string_view CorsFilter::allowHeaders() {
+  for (const Router::CorsPolicy& policy : policies_) {
+    if (!policy.allowHeaders().empty()) {
+      return policy.allowHeaders();
     }
   }
   return EMPTY_STRING;
 }
 
-const std::string& CorsFilter::exposeHeaders() {
-  for (const auto policy : policies_) {
-    if (policy && !policy->exposeHeaders().empty()) {
-      return policy->exposeHeaders();
+absl::string_view CorsFilter::exposeHeaders() {
+  for (const Router::CorsPolicy& policy : policies_) {
+    if (!policy.exposeHeaders().empty()) {
+      return policy.exposeHeaders();
     }
   }
   return EMPTY_STRING;
 }
 
-const std::string& CorsFilter::maxAge() {
-  for (const auto policy : policies_) {
-    if (policy && !policy->maxAge().empty()) {
-      return policy->maxAge();
+absl::string_view CorsFilter::maxAge() {
+  for (const Router::CorsPolicy& policy : policies_) {
+    if (!policy.maxAge().empty()) {
+      return policy.maxAge();
     }
   }
   return EMPTY_STRING;
 }
 
 bool CorsFilter::allowCredentials() {
-  for (const auto policy : policies_) {
-    if (policy && policy->allowCredentials()) {
-      return policy->allowCredentials().value();
+  for (const Router::CorsPolicy& policy : policies_) {
+    if (policy.allowCredentials()) {
+      return policy.allowCredentials().value();
     }
   }
   return false;
 }
 
 bool CorsFilter::allowPrivateNetworkAccess() {
-  for (const auto policy : policies_) {
-    if (policy && policy->allowPrivateNetworkAccess()) {
-      return policy->allowPrivateNetworkAccess().value();
+  for (const Router::CorsPolicy& policy : policies_) {
+    if (policy.allowPrivateNetworkAccess()) {
+      return policy.allowPrivateNetworkAccess().value();
     }
   }
   return false;
 }
 
 bool CorsFilter::shadowEnabled() {
-  for (const auto policy : policies_) {
-    if (policy) {
-      return policy->shadowEnabled();
-    }
+  for (const Router::CorsPolicy& policy : policies_) {
+    return policy.shadowEnabled();
   }
   return false;
 }
 
 bool CorsFilter::enabled() {
-  for (const auto policy : policies_) {
-    if (policy) {
-      return policy->enabled();
-    }
+  for (const Router::CorsPolicy& policy : policies_) {
+    return policy.enabled();
   }
   return false;
 }

diff --git a/source/extensions/filters/http/cors/cors_filter.h b/source/extensions/filters/http/cors/cors_filter.h
@@ -88,11 +88,11 @@ class CorsFilter : public Http::StreamFilter {
 private:
   friend class CorsFilterTest;
 
-  const std::vector<Matchers::StringMatcherPtr>* allowOrigins();
-  const std::string& allowMethods();
-  const std::string& allowHeaders();
-  const std::string& exposeHeaders();
-  const std::string& maxAge();
+  absl::Span<const Matchers::StringMatcherPtr> allowOrigins();
+  absl::string_view allowMethods();
+  absl::string_view allowHeaders();
+  absl::string_view exposeHeaders();
+  absl::string_view maxAge();
   bool allowCredentials();
   bool allowPrivateNetworkAccess();
   bool shadowEnabled();
@@ -102,7 +102,7 @@ class CorsFilter : public Http::StreamFilter {
 
   Http::StreamDecoderFilterCallbacks* decoder_callbacks_{};
   Http::StreamEncoderFilterCallbacks* encoder_callbacks_{};
-  absl::InlinedVector<const Envoy::Router::CorsPolicy*, 4> policies_;
+  absl::InlinedVector<std::reference_wrapper<const Envoy::Router::CorsPolicy>, 4> policies_;
   bool is_cors_request_{};
   std::string latched_origin_;
 

diff --git a/source/extensions/filters/http/custom_response/custom_response_filter.cc b/source/extensions/filters/http/custom_response/custom_response_filter.cc
@@ -48,13 +48,12 @@ Http::FilterHeadersStatus CustomResponseFilter::encodeHeaders(Http::ResponseHead
   // policy. Note that since the traversal is least to most specific, we can't
   // return early when a match is found.
   PolicySharedPtr policy;
-  for (const auto* typed_config :
+  for (const FilterConfig& typed_config :
        Http::Utility::getAllPerFilterConfig<FilterConfig>(encoder_callbacks_)) {
-    ASSERT(typed_config != nullptr);
 
     // Check if a match is found first to avoid overwriting policy with an
     // empty shared_ptr.
-    auto maybe_policy = typed_config->getPolicy(headers, encoder_callbacks_->streamInfo());
+    auto maybe_policy = typed_config.getPolicy(headers, encoder_callbacks_->streamInfo());
     if (maybe_policy) {
       policy = maybe_policy;
     }

diff --git a/source/extensions/filters/http/ext_authz/ext_authz.cc b/source/extensions/filters/http/ext_authz/ext_authz.cc
@@ -192,13 +192,12 @@ void Filter::initiateCall(const Http::RequestHeaderMap& headers) {
   }
 
   absl::optional<FilterConfigPerRoute> maybe_merged_per_route_config;
-  for (const auto* cfg :
+  for (const FilterConfigPerRoute& cfg :
        Http::Utility::getAllPerFilterConfig<FilterConfigPerRoute>(decoder_callbacks_)) {
-    ASSERT(cfg != nullptr);
     if (maybe_merged_per_route_config.has_value()) {
-      maybe_merged_per_route_config.value().merge(*cfg);
+      maybe_merged_per_route_config.value().merge(cfg);
     } else {
-      maybe_merged_per_route_config = *cfg;
+      maybe_merged_per_route_config = cfg;
     }
   }
 

diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc
@@ -1279,13 +1279,12 @@ void Filter::mergePerRouteConfig() {
   route_config_merged_ = true;
 
   absl::optional<FilterConfigPerRoute> merged_config;
-  for (const auto* typed_cfg :
+  for (const FilterConfigPerRoute& typed_cfg :
        Http::Utility::getAllPerFilterConfig<FilterConfigPerRoute>(decoder_callbacks_)) {
-    ASSERT(typed_cfg != nullptr);
     if (!merged_config.has_value()) {
-      merged_config.emplace(*typed_cfg);
+      merged_config.emplace(typed_cfg);
     } else {
-      merged_config.emplace(FilterConfigPerRoute(merged_config.value(), *typed_cfg));
+      merged_config.emplace(FilterConfigPerRoute(merged_config.value(), typed_cfg));
     }
   }
 

diff --git a/source/extensions/filters/http/header_mutation/header_mutation.cc b/source/extensions/filters/http/header_mutation/header_mutation.cc
@@ -46,12 +46,11 @@ Http::FilterHeadersStatus HeaderMutation::decodeHeaders(Http::RequestHeaderMap&
     // `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());
+      (*it).get().mutations().mutateRequestHeaders(headers, ctx, decoder_callbacks_->streamInfo());
     }
   } else {
-    for (const auto* route_config : route_configs_) {
-      route_config->mutations().mutateRequestHeaders(headers, ctx,
-                                                     decoder_callbacks_->streamInfo());
+    for (const PerRouteHeaderMutation& route_config : route_configs_) {
+      route_config.mutations().mutateRequestHeaders(headers, ctx, decoder_callbacks_->streamInfo());
     }
   }
 
@@ -70,12 +69,12 @@ Http::FilterHeadersStatus HeaderMutation::encodeHeaders(Http::ResponseHeaderMap&
 
   if (!config_->mostSpecificHeaderMutationsWins()) {
     for (auto it = route_configs_.rbegin(); it != route_configs_.rend(); ++it) {
-      (*it)->mutations().mutateResponseHeaders(headers, ctx, encoder_callbacks_->streamInfo());
+      (*it).get().mutations().mutateResponseHeaders(headers, ctx, encoder_callbacks_->streamInfo());
     }
   } else {
-    for (const auto* route_config : route_configs_) {
-      route_config->mutations().mutateResponseHeaders(headers, ctx,
-                                                      encoder_callbacks_->streamInfo());
+    for (const PerRouteHeaderMutation& route_config : route_configs_) {
+      route_config.mutations().mutateResponseHeaders(headers, ctx,
+                                                     encoder_callbacks_->streamInfo());
     }
   }
 

diff --git a/source/extensions/filters/http/header_mutation/header_mutation.h b/source/extensions/filters/http/header_mutation/header_mutation.h
@@ -82,7 +82,7 @@ class HeaderMutation : public Http::PassThroughFilter, public Logger::Loggable<L
 private:
   HeaderMutationConfigSharedPtr config_{};
   // The lifetime of route config pointers is same as the matched route.
-  absl::InlinedVector<const PerRouteHeaderMutation*, 4> route_configs_{};
+  absl::InlinedVector<std::reference_wrapper<const PerRouteHeaderMutation>, 4> route_configs_{};
 };
 
 } // namespace HeaderMutation

diff --git a/test/extensions/filters/http/cors/cors_filter_test.cc b/test/extensions/filters/http/cors/cors_filter_test.cc
@@ -87,8 +87,8 @@ TEST_F(CorsFilterTest, InitializeCorsPoliciesTest) {
     EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, true));
     EXPECT_EQ(false, isCorsRequest());
     EXPECT_EQ(2, filter_.policiesForTest().size());
-    EXPECT_EQ(cors_policy_.get(), filter_.policiesForTest().at(0));
-    EXPECT_EQ(cors_policy_.get(), filter_.policiesForTest().at(1));
+    EXPECT_EQ(cors_policy_.get(), &filter_.policiesForTest().at(0).get());
+    EXPECT_EQ(cors_policy_.get(), &filter_.policiesForTest().at(1).get());
   }
 
   // Only 'typed_per_filter_config' of virtual host has cors policy.
@@ -104,7 +104,7 @@ TEST_F(CorsFilterTest, InitializeCorsPoliciesTest) {
     EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, true));
     EXPECT_EQ(false, isCorsRequest());
     EXPECT_EQ(1, filter_.policiesForTest().size());
-    EXPECT_EQ(cors_policy_.get(), filter_.policiesForTest().at(0));
+    EXPECT_EQ(cors_policy_.get(), &filter_.policiesForTest().at(0).get());
   }
 
   // No cors policy in the 'typed_per_filter_config'.
@@ -121,9 +121,7 @@ TEST_F(CorsFilterTest, InitializeCorsPoliciesTest) {
 
     EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, true));
     EXPECT_EQ(false, isCorsRequest());
-    EXPECT_EQ(2, filter_.policiesForTest().size());
-    EXPECT_EQ(nullptr, filter_.policiesForTest().at(0));
-    EXPECT_EQ(nullptr, filter_.policiesForTest().at(1));
+    EXPECT_EQ(0, filter_.policiesForTest().size());
   }
   {
     filter_ = CorsFilter(config_);
@@ -139,9 +137,8 @@ TEST_F(CorsFilterTest, InitializeCorsPoliciesTest) {
 
     EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, true));
     EXPECT_EQ(false, isCorsRequest());
-    EXPECT_EQ(2, filter_.policiesForTest().size());
-    EXPECT_EQ(cors_policy_.get(), filter_.policiesForTest().at(0));
-    EXPECT_EQ(nullptr, filter_.policiesForTest().at(1));
+    EXPECT_EQ(1, filter_.policiesForTest().size());
+    EXPECT_EQ(cors_policy_.get(), &filter_.policiesForTest().at(0).get());
   }
   {
     filter_ = CorsFilter(config_);
@@ -157,9 +154,8 @@ TEST_F(CorsFilterTest, InitializeCorsPoliciesTest) {
 
     EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false));
     EXPECT_EQ(false, isCorsRequest());
-    EXPECT_EQ(2, filter_.policiesForTest().size());
-    EXPECT_EQ(nullptr, filter_.policiesForTest().at(0));
-    EXPECT_EQ(cors_policy_.get(), filter_.policiesForTest().at(1));
+    EXPECT_EQ(1, filter_.policiesForTest().size());
+    EXPECT_EQ(cors_policy_.get(), &filter_.policiesForTest().at(0).get());
   }
 }