xds: keep a reference to old config in the filter config provider

source/common/filter/http/filter_config_discovery_impl.cc

@@ -54,15 +54,21 @@ void DynamicFilterConfigProviderImpl::validateConfig(
 void DynamicFilterConfigProviderImpl::onConfigUpdate(Envoy::Http::FilterFactoryCb config,
                                                      const std::string&,
                                                      Config::ConfigAppliedCb cb) {
-  tls_->runOnAllThreads([config, cb](ThreadLocal::ThreadLocalObjectSharedPtr previous)
-                            -> ThreadLocal::ThreadLocalObjectSharedPtr {
-    auto prev_config = std::dynamic_pointer_cast<ThreadLocalConfig>(previous);
-    prev_config->config_ = config;
-    if (cb) {
-      cb();
-    }
-    return previous;
-  });
+  tls_->runOnAllThreads(
+      [config, cb](ThreadLocal::ThreadLocalObjectSharedPtr previous)
+          -> ThreadLocal::ThreadLocalObjectSharedPtr {
+        auto prev_config = std::dynamic_pointer_cast<ThreadLocalConfig>(previous);
+        prev_config->config_ = config;
+        if (cb) {
+          cb();
+        }
+        return previous;
+      },
+      [this, config]() {
+        // This happens after all workers have discarded the previous config so it can be safely
+        // deleted on the main thread by an update with the new config.
+        this->current_config_ = config;
+      });
 }
 
 FilterConfigSubscription::FilterConfigSubscription(

source/common/filter/http/filter_config_discovery_impl.h

@@ -52,6 +52,9 @@ class DynamicFilterConfigProviderImpl : public FilterConfigProvider {
 
   FilterConfigSubscriptionSharedPtr subscription_;
   const std::set<std::string> require_type_urls_;
+  // Currently applied configuration to ensure that the main thread deletes the last reference to
+  // it.
+  absl::optional<Envoy::Http::FilterFactoryCb> current_config_{absl::nullopt};
   ThreadLocal::SlotPtr tls_;
 
   // Local initialization target to ensure that the subscription starts in

test/integration/BUILD

@@ -883,10 +883,10 @@ envoy_cc_test(
     tags = ["fails_on_windows"],
     deps = [
         ":http_integration_lib",
-        "//source/extensions/filters/http/rbac:config",
         "//test/common/grpc:grpc_client_integration_lib",
+        "//test/integration/filters:set_response_code_filter_config_proto_cc_proto",
+        "//test/integration/filters:set_response_code_filter_lib",
         "//test/test_common:utility_lib",
-        "@envoy_api//envoy/extensions/filters/http/rbac/v3:pkg_cc_proto",
         "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto",
         "@envoy_api//envoy/service/extension/v3:pkg_cc_proto",
     ],

test/integration/extension_discovery_integration_test.cc

@@ -1,8 +1,8 @@
-#include "envoy/extensions/filters/http/rbac/v3/rbac.pb.h"
 #include "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.pb.h"
 #include "envoy/service/extension/v3/config_discovery.pb.h"
 
 #include "test/common/grpc/grpc_client_integration.h"
+#include "test/integration/filters/set_response_code_filter_config.pb.h"
 #include "test/integration/http_integration.h"
 #include "test/test_common/utility.h"
 
@@ -13,38 +13,14 @@ namespace {
 
 std::string denyPrivateConfig() {
   return R"EOF(
-      rules:
-        action: DENY
-        policies:
-          "test":
-            permissions:
-              - url_path: { path: { prefix: "/private" } }
-            principals:
-              - any: true
+    prefix: "/private"
+    code: 403
 )EOF";
 }
 
-std::string allowAllConfig() {
-  return R"EOF(
-      rules:
-        action: ALLOW
-        policies:
-          "test":
-            permissions:
-              - any: true
-            principals:
-              - any: true
-)EOF";
-}
+std::string allowAllConfig() { return "code: 200"; }
 
-std::string invalidConfig() {
-  return R"EOF(
-      rules:
-        action: DENY
-        policies:
-          "test": {}
-)EOF";
-}
+std::string invalidConfig() { return "code: 90"; }
 
 class ExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegrationParamTest,
                                           public HttpIntegrationTest {
@@ -62,20 +38,12 @@ class ExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegrationPara
           filter->set_name(name);
           auto* discovery = filter->mutable_config_discovery();
           discovery->add_type_urls(
-              "type.googleapis.com/envoy.extensions.filters.http.rbac.v3.RBAC");
+              "type.googleapis.com/test.integration.filters.SetResponseCodeFilterConfig");
           if (set_default_config) {
-            const auto rbac_configuration =
-                TestUtility::parseYaml<envoy::extensions::filters::http::rbac::v3::RBAC>(R"EOF(
-                rules:
-                  action: DENY
-                  policies:
-                    "test":
-                      permissions:
-                        - any: true
-                      principals:
-                        - any: true
-              )EOF");
-            discovery->mutable_default_config()->PackFrom(rbac_configuration);
+            const auto default_configuration =
+                TestUtility::parseYaml<test::integration::filters::SetResponseCodeFilterConfig>(
+                    "code: 403");
+            discovery->mutable_default_config()->PackFrom(default_configuration);
           }
           discovery->set_apply_default_config_without_warming(apply_without_warming);
           auto* api_config_source = discovery->mutable_config_source()->mutable_api_config_source();
@@ -145,15 +113,16 @@ class ExtensionDiscoveryIntegrationTest : public Grpc::GrpcClientIntegrationPara
   }
 
   void sendXdsResponse(const std::string& name, const std::string& version,
-                       const std::string& rbac_config) {
+                       const std::string& yaml_config) {
     envoy::service::discovery::v3::DiscoveryResponse response;
     response.set_version_info(version);
     response.set_type_url("type.googleapis.com/envoy.config.core.v3.TypedExtensionConfig");
-    const auto rbac_configuration =
-        TestUtility::parseYaml<envoy::extensions::filters::http::rbac::v3::RBAC>(rbac_config);
+    const auto configuration =
+        TestUtility::parseYaml<test::integration::filters::SetResponseCodeFilterConfig>(
+            yaml_config);
     envoy::config::core::v3::TypedExtensionConfig typed_config;
     typed_config.set_name(name);
-    typed_config.mutable_typed_config()->PackFrom(rbac_configuration);
+    typed_config.mutable_typed_config()->PackFrom(configuration);
     response.add_resources()->PackFrom(typed_config);
     ecds_stream_->sendGrpcMessage(response);
   }

test/integration/filters/BUILD

@@ -2,6 +2,7 @@ load(
     "//bazel:envoy_build_system.bzl",
     "envoy_cc_test_library",
     "envoy_package",
+    "envoy_proto_library",
 )
 
 licenses(["notice"])  # Apache 2
@@ -172,6 +173,25 @@ envoy_cc_test_library(
     ],
 )
 
+envoy_cc_test_library(
+    name = "set_response_code_filter_lib",
+    srcs = [
+        "set_response_code_filter.cc",
+    ],
+    deps = [
+        ":set_response_code_filter_config_proto_cc_proto",
+        "//include/envoy/http:filter_interface",
+        "//include/envoy/registry",
+        "//source/extensions/filters/http/common:factory_base_lib",
+        "//source/extensions/filters/http/common:pass_through_filter_lib",
+    ],
+)
+
+envoy_proto_library(
+    name = "set_response_code_filter_config_proto",
+    srcs = [":set_response_code_filter_config.proto"],
+)
+
 envoy_cc_test_library(
     name = "stop_iteration_and_continue",
     srcs = [

test/integration/filters/set_response_code_filter.cc

@@ -0,0 +1,64 @@
+#include <string>
+
+#include "envoy/http/filter.h"
+#include "envoy/registry/registry.h"
+
+#include "extensions/filters/http/common/factory_base.h"
+#include "extensions/filters/http/common/pass_through_filter.h"
+
+#include "test/integration/filters/set_response_code_filter_config.pb.h"
+#include "test/integration/filters/set_response_code_filter_config.pb.validate.h"
+
+#include "absl/strings/match.h"
+
+namespace Envoy {
+
+// A test filter that responds directly with a code on a prefix match.
+class SetResponseCodeFilterConfig {
+public:
+  SetResponseCodeFilterConfig(const std::string& prefix, uint32_t code,
+                              Server::Configuration::FactoryContext& context)
+      : prefix_(prefix), code_(code), tls_slot_(context.threadLocal().allocateSlot()) {}
+
+  const std::string prefix_;
+  const uint32_t code_;
+  // Allocate a slot to validate that it is destroyed on a main thread only.
+  ThreadLocal::SlotPtr tls_slot_;
+};
+
+class SetResponseCodeFilter : public Http::PassThroughFilter {
+public:
+  SetResponseCodeFilter(std::shared_ptr<SetResponseCodeFilterConfig> config) : config_(config) {}
+
+  Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& headers, bool) override {
+    if (absl::StartsWith(headers.Path()->value().getStringView(), config_->prefix_)) {
+      decoder_callbacks_->sendLocalReply(static_cast<Http::Code>(config_->code_), "", nullptr,
+                                         absl::nullopt, "");
+      return Http::FilterHeadersStatus::StopIteration;
+    }
+    return Http::FilterHeadersStatus::Continue;
+  }
+
+private:
+  const std::shared_ptr<SetResponseCodeFilterConfig> config_;
+};
+
+class SetResponseCodeFilterFactory : public Extensions::HttpFilters::Common::FactoryBase<
+                                         test::integration::filters::SetResponseCodeFilterConfig> {
+public:
+  SetResponseCodeFilterFactory() : FactoryBase("set-response-code-filter") {}
+
+private:
+  Http::FilterFactoryCb createFilterFactoryFromProtoTyped(
+      const test::integration::filters::SetResponseCodeFilterConfig& proto_config,
+      const std::string&, Server::Configuration::FactoryContext& context) override {
+    auto filter_config = std::make_shared<SetResponseCodeFilterConfig>(
+        proto_config.prefix(), proto_config.code(), context);
+    return [filter_config](Http::FilterChainFactoryCallbacks& callbacks) -> void {
+      callbacks.addStreamFilter(std::make_shared<SetResponseCodeFilter>(filter_config));
+    };
+  }
+};
+
+REGISTER_FACTORY(SetResponseCodeFilterFactory, Server::Configuration::NamedHttpFilterConfigFactory);
+} // namespace Envoy

test/integration/filters/set_response_code_filter_config.proto

@@ -0,0 +1,10 @@
+syntax = "proto3";
+
+package test.integration.filters;
+
+import "validate/validate.proto";
+
+message SetResponseCodeFilterConfig {
+  string prefix = 1;
+  uint32 code = 2 [(validate.rules).uint32 = {lt: 600 gte: 200}];
+}