config: add path_config_source and watched_directory config

For xDS over the file system, sometimes more control is required over what directory/file is watched for symbolic link swaps. Specifically, in order to deliver xDS over a Kubernetes ConfigMap, this extra configuration is required. Fixes #10979 Signed-off-by: Matt Klein <[email protected]>
envoyproxy · Feb 15, 2022 · 957fc1f · 957fc1f
1 parent 6dd74c6
commit 957fc1f
Show file tree

Hide file tree

Showing 12 changed files with 200 additions and 68 deletions.
diff --git a/api/envoy/config/core/v3/config_source.proto b/api/envoy/config/core/v3/config_source.proto
@@ -2,6 +2,7 @@ syntax = "proto3";
 
 package envoy.config.core.v3;
 
+import "envoy/config/core/v3/base.proto";
 import "envoy/config/core/v3/grpc_service.proto";
 
 import "google/protobuf/duration.proto";
@@ -143,13 +144,48 @@ message RateLimitSettings {
   google.protobuf.DoubleValue fill_rate = 2 [(validate.rules).double = {gt: 0.0}];
 }
 
+// Local filesystem path configuration source.
+message PathConfigSource {
+  // Path on the filesystem to source and watch for configuration updates.
+  // When sourcing configuration for a :ref:`secret <envoy_v3_api_msg_extensions.transport_sockets.tls.v3.Secret>`,
+  // the certificate and key files are also watched for updates.
+  //
+  // .. note::
+  //
+  //  The path to the source must exist at config load time.
+  //
+  // .. note::
+  //
+  //   If `watched_directory` is *not* configured, Envoy will watch the file path for *moves.*
+  //   This is because in general only moves are atomic. The same method of swapping files as is
+  //   demonstrated in the :ref:`runtime documentation <config_runtime_symbolic_link_swap>` can be
+  //   used here also. If `watched_directory` is configured, no watch will be placed directly on
+  //   this path. Instead, the configured `watched_directory` will be used to trigger reloads of
+  //   this path. This is required in certain deployment scenarios. See below for more information.
+  string path = 1 [(validate.rules).string = {min_len: 1}];
+
+  // If configured, this directory will be watched for *moves.* When an entry in this directory is
+  // moved to, the `path` will be reloaded. This is required in certain deployment scenarios.
+  //
+  // Specifically, if trying to load an xDS resource using a Kubernetes ConfigMap, the following
+  // configuration might be used:
+  // 1. Store xds.yaml inside a ConfigMap.
+  // 2. Mount the ConfigMap to `/config_map/xds`
+  // 3. Configure path `/config_map/xds/xds.yaml`
+  // 4. Configure watched directory `/config_map/xds`
+  //
+  // The above configuration will ensure that Envoy watches the owning directory for moves which is
+  // required due to how Kubernetes manages ConfigMap symbolic links during atomic updates.
+  WatchedDirectory watched_directory = 2;
+}
+
 // Configuration for :ref:`listeners <config_listeners>`, :ref:`clusters
 // <config_cluster_manager>`, :ref:`routes
 // <envoy_v3_api_msg_config.route.v3.RouteConfiguration>`, :ref:`endpoints
 // <arch_overview_service_discovery>` etc. may either be sourced from the
 // filesystem or from an xDS API source. Filesystem configs are watched with
 // inotify for updates.
-// [#next-free-field: 8]
+// [#next-free-field: 9]
 message ConfigSource {
   option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.core.ConfigSource";
 
@@ -162,20 +198,11 @@ message ConfigSource {
   oneof config_source_specifier {
     option (validate.required) = true;
 
-    // Path on the filesystem to source and watch for configuration updates.
-    // When sourcing configuration for :ref:`secret <envoy_v3_api_msg_extensions.transport_sockets.tls.v3.Secret>`,
-    // the certificate and key files are also watched for updates.
-    //
-    // .. note::
-    //
-    //  The path to the source must exist at config load time.
-    //
-    // .. note::
-    //
-    //   Envoy will only watch the file path for *moves.* This is because in general only moves
-    //   are atomic. The same method of swapping files as is demonstrated in the
-    //   :ref:`runtime documentation <config_runtime_symbolic_link_swap>` can be used here also.
-    string path = 1;
+    // Deprecated in favor of `path_config_source`. Use that field instead.
+    string path = 1 [deprecated = true, (envoy.annotations.deprecated_at_minor_version) = "3.0"];
+
+    // Local filesystem path configuration source.
+    PathConfigSource path_config_source = 8;
 
     // API configuration source.
     ApiConfigSource api_config_source = 2;

diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
@@ -53,6 +53,10 @@ Removed Config or Runtime
 New Features
 ------------
 * access_log: make consistent access_log format fields ``%(DOWN|DIRECT_DOWN|UP)STREAM_(LOCAL|REMOTE)_*%`` to provide all combinations of local & remote addresses for upstream & downstream connections.
+* config: added new file based xDS configuration via :ref:`path_config_source <envoy_v3_api_field_config.core.v3.ConfigSource.path_config_source>`.
+  :ref:`watched_directory <envoy_v3_api_field_config.core.v3.PathConfigSource.watched_directory>` can
+  be used to setup an independent watch for when to reload the file path, for example when using
+  Kubernetes ConfigMaps to deliver configuration. See the linked documentation for more information.
 * cors: add dynamic support for headers ``access-control-allow-methods`` and ``access-control-allow-headers`` in cors.
 * http: added random_value_specifier in :ref:`weighted_clusters <envoy_v3_api_field_config.route.v3.RouteAction.weighted_clusters>` to allow random value to be specified from configuration proto.
 * http: added support for :ref:`proxy_status_config <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.proxy_status_config>` for configuring `Proxy-Status <https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-proxy-status-08>`_ HTTP response header fields.
@@ -64,4 +68,6 @@ New Features
 Deprecated
 ----------
 
+* config: deprecated :ref:`path <envoy_v3_api_field_config.core.v3.ConfigSource.path>` in favor of
+  :ref:`path_config_source <envoy_v3_api_field_config.core.v3.ConfigSource.path_config_source>`
 * http: removing support for long-deprecated old style filter names, e.g. envoy.router, envoy.lua.
diff --git a/envoy/router/router.h b/envoy/router/router.h
@@ -383,7 +383,7 @@ class RetryState {
   /**
    * Determine whether a request should be retried based on the response headers.
    * @param response_headers supplies the response headers.
-   * @param original_request supplies the orignal request headers.
+   * @param original_request supplies the original request headers.
    * @param callback supplies the callback that will be invoked when the retry should take place.
    *                 This is used to add timed backoff, etc. The callback will never be called
    *                 inline.
@@ -401,7 +401,7 @@ class RetryState {
    * the information about whether a response is "good" or not is useful, but a retry should
    * not be attempted for other reasons.
    * @param response_headers supplies the response headers.
-   * @param original_request supplies the orignal request headers.
+   * @param original_request supplies the original request headers.
    * @param retry_as_early_data output argument to tell the caller if a retry should be sent as
    *        early data if it is warranted.
    * @return RetryDecision if a retry would be warranted based on the retry policy and if it would

diff --git a/source/common/config/BUILD b/source/common/config/BUILD
@@ -120,6 +120,7 @@ envoy_cc_library(
     hdrs = ["filesystem_subscription_impl.h"],
     deps = [
         ":decoded_resource_lib",
+        ":watched_directory_lib",
         "//envoy/config:subscription_interface",
         "//envoy/event:dispatcher_interface",
         "//envoy/filesystem:filesystem_interface",
@@ -129,6 +130,7 @@ envoy_cc_library(
         "//source/common/protobuf",
         "//source/common/protobuf:message_validator_lib",
         "//source/common/protobuf:utility_lib",
+        "@envoy_api//envoy/config/core/v3:pkg_cc_proto",
         "@envoy_api//envoy/service/discovery/v3:pkg_cc_proto",
     ],
 )

diff --git a/source/common/config/filesystem_subscription_impl.cc b/source/common/config/filesystem_subscription_impl.cc
@@ -12,18 +12,35 @@
 namespace Envoy {
 namespace Config {
 
+envoy::config::core::v3::PathConfigSource makePathConfigSource(const std::string& path) {
+  envoy::config::core::v3::PathConfigSource path_config_source;
+  path_config_source.set_path(path);
+  return path_config_source;
+}
+
 FilesystemSubscriptionImpl::FilesystemSubscriptionImpl(
-    Event::Dispatcher& dispatcher, absl::string_view path, SubscriptionCallbacks& callbacks,
-    OpaqueResourceDecoder& resource_decoder, SubscriptionStats stats,
-    ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api)
-    : path_(path), watcher_(dispatcher.createFilesystemWatcher()), callbacks_(callbacks),
-      resource_decoder_(resource_decoder), stats_(stats), api_(api),
-      validation_visitor_(validation_visitor) {
-  watcher_->addWatch(path_, Filesystem::Watcher::Events::MovedTo, [this](uint32_t) {
-    if (started_) {
-      refresh();
-    }
-  });
+    Event::Dispatcher& dispatcher,
+    const envoy::config::core::v3::PathConfigSource& path_config_source,
+    SubscriptionCallbacks& callbacks, OpaqueResourceDecoder& resource_decoder,
+    SubscriptionStats stats, ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api)
+    : path_(path_config_source.path()), callbacks_(callbacks), resource_decoder_(resource_decoder),
+      stats_(stats), api_(api), validation_visitor_(validation_visitor) {
+  if (!path_config_source.has_watched_directory()) {
+    file_watcher_ = dispatcher.createFilesystemWatcher();
+    file_watcher_->addWatch(path_, Filesystem::Watcher::Events::MovedTo, [this](uint32_t) {
+      if (started_) {
+        refresh();
+      }
+    });
+  } else {
+    directory_watcher_ =
+        std::make_unique<WatchedDirectory>(path_config_source.watched_directory(), dispatcher);
+    directory_watcher_->setCallback([this]() {
+      if (started_) {
+        refresh();
+      }
+    });
+  }
 }
 
 // Config::Subscription
@@ -80,18 +97,19 @@ void FilesystemSubscriptionImpl::refresh() {
     } else {
       ENVOY_LOG(warn, "Filesystem config update failure: {}", e.what());
       stats_.update_failure_.inc();
-      // This could happen due to filesystem issues or a bad configuration (e.g. proto validation).
-      // Since the latter is more likely, for now we will treat it as rejection.
+      // This could happen due to filesystem issues or a bad configuration (e.g. proto
+      // validation). Since the latter is more likely, for now we will treat it as rejection.
       callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, &e);
     }
   }
 }
 
 FilesystemCollectionSubscriptionImpl::FilesystemCollectionSubscriptionImpl(
-    Event::Dispatcher& dispatcher, absl::string_view path, SubscriptionCallbacks& callbacks,
-    OpaqueResourceDecoder& resource_decoder, SubscriptionStats stats,
-    ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api)
-    : FilesystemSubscriptionImpl(dispatcher, path, callbacks, resource_decoder, stats,
+    Event::Dispatcher& dispatcher,
+    const envoy::config::core::v3::PathConfigSource& path_config_source,
+    SubscriptionCallbacks& callbacks, OpaqueResourceDecoder& resource_decoder,
+    SubscriptionStats stats, ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api)
+    : FilesystemSubscriptionImpl(dispatcher, path_config_source, callbacks, resource_decoder, stats,
                                  validation_visitor, api) {}
 
 std::string

diff --git a/source/common/config/filesystem_subscription_impl.h b/source/common/config/filesystem_subscription_impl.h
@@ -1,16 +1,20 @@
 #pragma once
 
 #include "envoy/api/api.h"
+#include "envoy/config/core/v3/config_source.pb.h"
 #include "envoy/config/subscription.h"
 #include "envoy/event/dispatcher.h"
 #include "envoy/filesystem/filesystem.h"
 #include "envoy/protobuf/message_validator.h"
 
 #include "source/common/common/logger.h"
+#include "source/common/config/watched_directory.h"
 
 namespace Envoy {
 namespace Config {
 
+envoy::config::core::v3::PathConfigSource makePathConfigSource(const std::string& path);
+
 /**
  * Filesystem inotify implementation of the API Subscription interface. This allows the API to be
  * consumed on filesystem changes to files containing the JSON canonical representation of
@@ -19,7 +23,8 @@ namespace Config {
 class FilesystemSubscriptionImpl : public Config::Subscription,
                                    protected Logger::Loggable<Logger::Id::config> {
 public:
-  FilesystemSubscriptionImpl(Event::Dispatcher& dispatcher, absl::string_view path,
+  FilesystemSubscriptionImpl(Event::Dispatcher& dispatcher,
+                             const envoy::config::core::v3::PathConfigSource& path_config_source,
                              SubscriptionCallbacks& callbacks,
                              OpaqueResourceDecoder& resource_decoder, SubscriptionStats stats,
                              ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api);
@@ -40,7 +45,8 @@ class FilesystemSubscriptionImpl : public Config::Subscription,
 
   bool started_{};
   const std::string path_;
-  std::unique_ptr<Filesystem::Watcher> watcher_;
+  std::unique_ptr<Filesystem::Watcher> file_watcher_;
+  WatchedDirectoryPtr directory_watcher_;
   SubscriptionCallbacks& callbacks_;
   OpaqueResourceDecoder& resource_decoder_;
   SubscriptionStats stats_;
@@ -52,12 +58,12 @@ class FilesystemSubscriptionImpl : public Config::Subscription,
 // non-inline collection resources.
 class FilesystemCollectionSubscriptionImpl : public FilesystemSubscriptionImpl {
 public:
-  FilesystemCollectionSubscriptionImpl(Event::Dispatcher& dispatcher, absl::string_view path,
-                                       SubscriptionCallbacks& callbacks,
-                                       OpaqueResourceDecoder& resource_decoder,
-                                       SubscriptionStats stats,
-                                       ProtobufMessage::ValidationVisitor& validation_visitor,
-                                       Api::Api& api);
+  FilesystemCollectionSubscriptionImpl(
+      Event::Dispatcher& dispatcher,
+      const envoy::config::core::v3::PathConfigSource& path_config_source,
+      SubscriptionCallbacks& callbacks, OpaqueResourceDecoder& resource_decoder,
+      SubscriptionStats stats, ProtobufMessage::ValidationVisitor& validation_visitor,
+      Api::Api& api);
 
   std::string refreshInternal(ProtobufTypes::MessagePtr* config_update) override;
 };

diff --git a/source/common/config/subscription_factory_impl.cc b/source/common/config/subscription_factory_impl.cc
@@ -36,7 +36,14 @@ SubscriptionPtr SubscriptionFactoryImpl::subscriptionFromConfigSource(
   case envoy::config::core::v3::ConfigSource::ConfigSourceSpecifierCase::kPath: {
     Utility::checkFilesystemSubscriptionBackingPath(config.path(), api_);
     return std::make_unique<Config::FilesystemSubscriptionImpl>(
-        dispatcher_, config.path(), callbacks, resource_decoder, stats, validation_visitor_, api_);
+        dispatcher_, makePathConfigSource(config.path()), callbacks, resource_decoder, stats,
+        validation_visitor_, api_);
+  }
+  case envoy::config::core::v3::ConfigSource::ConfigSourceSpecifierCase::kPathConfigSource: {
+    Utility::checkFilesystemSubscriptionBackingPath(config.path_config_source().path(), api_);
+    return std::make_unique<Config::FilesystemSubscriptionImpl>(
+        dispatcher_, config.path_config_source(), callbacks, resource_decoder, stats,
+        validation_visitor_, api_);
   }
   case envoy::config::core::v3::ConfigSource::ConfigSourceSpecifierCase::kApiConfigSource: {
     const envoy::config::core::v3::ApiConfigSource& api_config_source = config.api_config_source();
@@ -134,7 +141,8 @@ SubscriptionPtr SubscriptionFactoryImpl::collectionSubscriptionFromUrl(
     const std::string path = Http::Utility::localPathFromFilePath(collection_locator.id());
     Utility::checkFilesystemSubscriptionBackingPath(path, api_);
     return std::make_unique<Config::FilesystemCollectionSubscriptionImpl>(
-        dispatcher_, path, callbacks, resource_decoder, stats, validation_visitor_, api_);
+        dispatcher_, makePathConfigSource(path), callbacks, resource_decoder, stats,
+        validation_visitor_, api_);
   }
   case xds::core::v3::ResourceLocator::XDSTP: {
     if (resource_type != collection_locator.resource_type()) {

diff --git a/test/common/config/BUILD b/test/common/config/BUILD
@@ -123,6 +123,7 @@ envoy_cc_test(
         "//test/mocks/event:event_mocks",
         "//test/mocks/filesystem:filesystem_mocks",
         "//test/test_common:logging_lib",
+        "@envoy_api//envoy/config/core/v3:pkg_cc_proto",
         "@envoy_api//envoy/config/endpoint/v3:pkg_cc_proto",
         "@envoy_api//envoy/config/listener/v3:pkg_cc_proto",
     ],

diff --git a/test/common/config/filesystem_subscription_impl_test.cc b/test/common/config/filesystem_subscription_impl_test.cc
@@ -1,3 +1,4 @@
+#include "envoy/config/core/v3/config_source.pb.h"
 #include "envoy/config/endpoint/v3/endpoint.pb.h"
 #include "envoy/config/listener/v3/listener.pb.h"
 #include "envoy/config/listener/v3/listener.pb.validate.h"
@@ -51,10 +52,10 @@ TEST(MiscFilesystemSubscriptionImplTest, BadWatch) {
   EXPECT_CALL(*watcher, addWatch(_, _, _)).WillOnce(Throw(EnvoyException("bad path")));
   NiceMock<Config::MockSubscriptionCallbacks> callbacks;
   NiceMock<Config::MockOpaqueResourceDecoder> resource_decoder;
-  EXPECT_THROW_WITH_MESSAGE(FilesystemSubscriptionImpl(dispatcher, "##!@/dev/null", callbacks,
-                                                       resource_decoder, stats, validation_visitor,
-                                                       *api),
-                            EnvoyException, "bad path");
+  EXPECT_THROW_WITH_MESSAGE(
+      FilesystemSubscriptionImpl(dispatcher, makePathConfigSource("##!@/dev/null"), callbacks,
+                                 resource_decoder, stats, validation_visitor, *api),
+      EnvoyException, "bad path");
 }
 
 // Validate that the update_time statistic isn't changed when the configuration update gets
@@ -87,18 +88,20 @@ class FilesystemCollectionSubscriptionImplTest : public testing::Test,
                                                  Event::TestUsingSimulatedTime {
 public:
   FilesystemCollectionSubscriptionImplTest()
-      : path_(TestEnvironment::temporaryPath("lds.yaml")),
+      : path_(makePathConfigSource(TestEnvironment::temporaryPath("lds.yaml"))),
         stats_(Utility::generateStats(stats_store_)),
         api_(Api::createApiForTest(stats_store_, simTime())), dispatcher_(setupDispatcher()),
         subscription_(*dispatcher_, path_, callbacks_, resource_decoder_, stats_,
                       ProtobufMessage::getStrictValidationVisitor(), *api_) {}
-  ~FilesystemCollectionSubscriptionImplTest() override { TestEnvironment::removePath(path_); }
+  ~FilesystemCollectionSubscriptionImplTest() override {
+    TestEnvironment::removePath(path_.path());
+  }
 
   Event::DispatcherPtr setupDispatcher() {
     auto dispatcher = std::make_unique<Event::MockDispatcher>();
     EXPECT_CALL(*dispatcher, createFilesystemWatcher_()).WillOnce(InvokeWithoutArgs([this] {
       Filesystem::MockWatcher* mock_watcher = new Filesystem::MockWatcher();
-      EXPECT_CALL(*mock_watcher, addWatch(path_, Filesystem::Watcher::Events::MovedTo, _))
+      EXPECT_CALL(*mock_watcher, addWatch(path_.path(), Filesystem::Watcher::Events::MovedTo, _))
           .WillOnce(Invoke([this](absl::string_view, uint32_t,
                                   Filesystem::Watcher::OnChangedCb cb) { on_changed_cb_ = cb; }));
       return mock_watcher;
@@ -109,7 +112,7 @@ class FilesystemCollectionSubscriptionImplTest : public testing::Test,
   void updateFile(const std::string& yaml) {
     // Write YAML contents to file, rename to path_ and invoke on change callback
     const std::string temp_path = TestEnvironment::writeStringToFileForTest("lds.yaml.tmp", yaml);
-    TestEnvironment::renameFile(temp_path, path_);
+    TestEnvironment::renameFile(temp_path, path_.path());
     on_changed_cb_(Filesystem::Watcher::Events::MovedTo);
   }
 
@@ -143,7 +146,7 @@ class FilesystemCollectionSubscriptionImplTest : public testing::Test,
     return testing::AssertionSuccess();
   }
 
-  const std::string path_;
+  const envoy::config::core::v3::PathConfigSource path_;
   Stats::IsolatedStoreImpl stats_store_;
   SubscriptionStats stats_;
   Api::ApiPtr api_;