config: distinct CLI options for strict/permissive checking of static…

…/dynamic config. As per envoyproxy#6651, this PR plumbs in CLI options to allow independent control over static/dynamic unknown field validation. The defaults are the same for static as today (strict) and for dynamic we are by default permissive. This permits easy rollout of new API minor versions, including those related to security fixes. Fixes a regression that occurred in envoyproxy#7200 where strict/permissive checking CLI options were inverted. Risk level: Low (strictly more permissive by default) Testing: additional unit and integration tests added, exercising both permissive/strict checking over various parts of the API (bootstrap, listeners, clusters, xDS, network filters, etc). Fixes envoyproxy#6651 Signed-off-by: Harvey Tuch <[email protected]>
htuch · Aug 7, 2019 · f923a18 · f923a18
1 parent feb56a1
commit f923a18
Show file tree

Hide file tree

Showing 53 changed files with 732 additions and 174 deletions.
diff --git a/api/envoy/admin/v2alpha/server_info.proto b/api/envoy/admin/v2alpha/server_info.proto
@@ -56,6 +56,9 @@ message CommandLineOptions {
   // See :option:`--allow-unknown-fields` for details.
   bool allow_unknown_fields = 5;
 
+  // See :option:`--reject-unknown-fields-dynamic` for details.
+  bool reject_unknown_fields_dynamic = 26;
+
   // See :option:`--admin-address-path` for details.
   string admin_address_path = 6;
 

diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst
@@ -7,6 +7,9 @@ Version history
 * admin: added ability to configure listener :ref:`socket options <envoy_api_field_config.bootstrap.v2.Admin.socket_options>`.
 * admin: added config dump support for Secret Discovery Service :ref:`SecretConfigDump <envoy_api_msg_admin.v2alpha.SecretsConfigDump>`.
 * config: added access log :ref:`extension filter<envoy_api_field_config.filter.accesslog.v2.AccessLogFilter.extension_filter>`.
+* config: added support for :option:`--reject-unknown-fields-dynamic`, providing independent control
+  over whether unknown fields are rejected in static and dynamic configuration. By default, unknown
+  fields in static configuration are rejected and are allowed in dynamic configuration.
 * config: async data access for local and remote data source.
 * config: changed the default value of :ref:`initial_fetch_timeout <envoy_api_field_core.ConfigSource.initial_fetch_timeout>` from 0s to 15s. This is a change in behaviour in the sense that Envoy will move to the next initialization phase, even if the first config is not delivered in 15s. Refer to :ref:`initialization process <arch_overview_initialization>` for more details.
 * config: added stat :ref:`init_fetch_timeout <config_cluster_manager_cds>`.

diff --git a/docs/root/operations/cli.rst b/docs/root/operations/cli.rst
@@ -228,10 +228,17 @@ following are the command line options that Envoy supports.
 
 .. option:: --allow-unknown-fields
 
-  *(optional)* This flag disables validation of protobuf configurations for unknown fields. By default, the 
+  *(optional)* This flag disables validation of protobuf configurations for unknown fields. By default, the
   validation is enabled. For most deployments, the default should be used which ensures configuration errors
-  are caught upfront and Envoy is configured as intended. However in cases where Envoy needs to accept configuration 
-  produced by newer control planes, effectively ignoring new features it does not know about yet, this can be disabled.
+  are caught upfront and Envoy is configured as intended.
+
+.. option:: --reject-unknown-fields-dynamic
+
+  *(optional)* This flag disables validation of protobuf configuration for unknown fields in
+  dynamic configuration. By default, this flag is set false, disabling validation for fields beyond
+  bootstrap. This allows newer xDS configurations to be delivered to older Envoys. This can be set
+  true for strict dynamic checking when this behavior is not wanted but the default should be
+  desirable for most Envoy deployments.
 
 .. option:: --version
 

diff --git a/include/envoy/protobuf/message_validator.h b/include/envoy/protobuf/message_validator.h
@@ -25,5 +25,20 @@ class ValidationVisitor {
   virtual void onUnknownField(absl::string_view description) PURE;
 };
 
+class ValidationContext {
+public:
+  virtual ~ValidationContext() = default;
+
+  /**
+   * @return ValidationVisitor& the validation visitor for static configuration.
+   */
+  virtual ValidationVisitor& staticValidationVisitor() PURE;
+
+  /**
+   * @return ValidationVisitor& the validation visitor for dynamic configuration.
+   */
+  virtual ValidationVisitor& dynamicValidationVisitor() PURE;
+};
+
 } // namespace ProtobufMessage
 } // namespace Envoy
diff --git a/include/envoy/server/instance.h b/include/envoy/server/instance.h
@@ -220,10 +220,10 @@ class Instance {
   virtual std::chrono::milliseconds statsFlushInterval() const PURE;
 
   /**
-   * @return ProtobufMessage::ValidationVisitor& validation visitor for configuration
+   * @return ProtobufMessage::ValidationContext& validation context for configuration
    *         messages.
    */
-  virtual ProtobufMessage::ValidationVisitor& messageValidationVisitor() PURE;
+  virtual ProtobufMessage::ValidationContext& messageValidationContext() PURE;
 };
 
 } // namespace Server

diff --git a/include/envoy/server/options.h b/include/envoy/server/options.h
@@ -86,10 +86,15 @@ class Options {
   virtual const envoy::config::bootstrap::v2::Bootstrap& configProto() const PURE;
 
   /**
-   * @return bool allow unknown fields in the configuration?
+   * @return bool allow unknown fields in the static configuration?
    */
   virtual bool allowUnknownFields() const PURE;
 
+  /**
+   * @return bool allow unknown fields in the dynamic configuration?
+   */
+  virtual bool rejectUnknownFieldsDynamic() const PURE;
+
   /**
    * @return const std::string& the admin address output file.
    */

diff --git a/source/common/config/filesystem_subscription_impl.cc b/source/common/config/filesystem_subscription_impl.cc
@@ -53,10 +53,9 @@ void FilesystemSubscriptionImpl::refresh() {
     } else {
       ENVOY_LOG(warn, "Filesystem config update failure: {}", e.what());
       stats_.update_failure_.inc();
-      // ConnectionFailure is not a meaningful error code for file system but it has been chosen so
-      // that the behaviour is uniform across all subscription types.
-      callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure,
-                                      &e);
+      // This could happen due to filesystem issues or a bad configuration (e.g. proto validation).
+      // Since the latter is more likel, for now we will treat it as rejection.
+      callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, &e);
     }
   }
 }

diff --git a/source/common/protobuf/message_validator_impl.h b/source/common/protobuf/message_validator_impl.h
@@ -21,5 +21,21 @@ class StrictValidationVisitorImpl : public ValidationVisitor {
 
 ValidationVisitor& getStrictValidationVisitor();
 
+class ValidationContextImpl : public ValidationContext {
+public:
+  ValidationContextImpl(ValidationVisitor& static_validation_visitor,
+                        ValidationVisitor& dynamic_validation_visitor)
+      : static_validation_visitor_(static_validation_visitor),
+        dynamic_validation_visitor_(dynamic_validation_visitor) {}
+
+  // Envoy::ProtobufMessage::ValidationContext
+  ValidationVisitor& staticValidationVisitor() override { return static_validation_visitor_; }
+  ValidationVisitor& dynamicValidationVisitor() override { return dynamic_validation_visitor_; }
+
+private:
+  ValidationVisitor& static_validation_visitor_;
+  ValidationVisitor& dynamic_validation_visitor_;
+};
+
 } // namespace ProtobufMessage
 } // namespace Envoy
diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc
@@ -183,7 +183,7 @@ ClusterManagerImpl::ClusterManagerImpl(
     Stats::Store& stats, ThreadLocal::Instance& tls, Runtime::Loader& runtime,
     Runtime::RandomGenerator& random, const LocalInfo::LocalInfo& local_info,
     AccessLog::AccessLogManager& log_manager, Event::Dispatcher& main_thread_dispatcher,
-    Server::Admin& admin, ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api,
+    Server::Admin& admin, ProtobufMessage::ValidationContext& validation_context, Api::Api& api,
     Http::Context& http_context)
     : factory_(factory), runtime_(runtime), stats_(stats), tls_(tls.allocateSlot()),
       random_(random), bind_config_(bootstrap.cluster_manager().upstream_bind_config()),
@@ -192,8 +192,9 @@ ClusterManagerImpl::ClusterManagerImpl(
       config_tracker_entry_(
           admin.getConfigTracker().add("clusters", [this] { return dumpClusterConfigs(); })),
       time_source_(main_thread_dispatcher.timeSource()), dispatcher_(main_thread_dispatcher),
-      http_context_(http_context), subscription_factory_(local_info, main_thread_dispatcher, *this,
-                                                         random, validation_visitor, api) {
+      http_context_(http_context),
+      subscription_factory_(local_info, main_thread_dispatcher, *this, random,
+                            validation_context.dynamicValidationVisitor(), api) {
   async_client_manager_ =
       std::make_unique<Grpc::AsyncClientManagerImpl>(*this, tls, time_source_, api);
   const auto& cm_config = bootstrap.cluster_manager();
@@ -1236,7 +1237,7 @@ ClusterManagerPtr ProdClusterManagerFactory::clusterManagerFromProto(
     const envoy::config::bootstrap::v2::Bootstrap& bootstrap) {
   return ClusterManagerPtr{new ClusterManagerImpl(
       bootstrap, *this, stats_, tls_, runtime_, random_, local_info_, log_manager_,
-      main_thread_dispatcher_, admin_, validation_visitor_, api_, http_context_)};
+      main_thread_dispatcher_, admin_, validation_context_, api_, http_context_)};
 }
 
 Http::ConnectionPool::InstancePtr ProdClusterManagerFactory::allocateConnPool(
@@ -1266,13 +1267,16 @@ std::pair<ClusterSharedPtr, ThreadAwareLoadBalancerPtr> ProdClusterManagerFactor
   return ClusterFactoryImplBase::create(
       cluster, cm, stats_, tls_, dns_resolver_, ssl_context_manager_, runtime_, random_,
       main_thread_dispatcher_, log_manager_, local_info_, admin_, singleton_manager_,
-      outlier_event_logger, added_via_api, validation_visitor_, api_);
+      outlier_event_logger, added_via_api,
+      added_via_api ? validation_context_.dynamicValidationVisitor()
+                    : validation_context_.staticValidationVisitor(),
+      api_);
 }
 
 CdsApiPtr ProdClusterManagerFactory::createCds(const envoy::api::v2::core::ConfigSource& cds_config,
                                                ClusterManager& cm) {
   // TODO(htuch): Differentiate static vs. dynamic validation visitors.
-  return CdsApiImpl::create(cds_config, cm, stats_, validation_visitor_);
+  return CdsApiImpl::create(cds_config, cm, stats_, validation_context_.dynamicValidationVisitor());
 }
 
 } // namespace Upstream

diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h
@@ -43,10 +43,10 @@ class ProdClusterManagerFactory : public ClusterManagerFactory {
                             Event::Dispatcher& main_thread_dispatcher,
                             const LocalInfo::LocalInfo& local_info,
                             Secret::SecretManager& secret_manager,
-                            ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api,
+                            ProtobufMessage::ValidationContext& validation_context, Api::Api& api,
                             Http::Context& http_context, AccessLog::AccessLogManager& log_manager,
                             Singleton::Manager& singleton_manager)
-      : main_thread_dispatcher_(main_thread_dispatcher), validation_visitor_(validation_visitor),
+      : main_thread_dispatcher_(main_thread_dispatcher), validation_context_(validation_context),
         api_(api), http_context_(http_context), admin_(admin), runtime_(runtime), stats_(stats),
         tls_(tls), random_(random), dns_resolver_(dns_resolver),
         ssl_context_manager_(ssl_context_manager), local_info_(local_info),
@@ -74,7 +74,7 @@ class ProdClusterManagerFactory : public ClusterManagerFactory {
 
 protected:
   Event::Dispatcher& main_thread_dispatcher_;
-  ProtobufMessage::ValidationVisitor& validation_visitor_;
+  ProtobufMessage::ValidationContext& validation_context_;
   Api::Api& api_;
   Http::Context& http_context_;
   Server::Admin& admin_;
@@ -173,7 +173,7 @@ class ClusterManagerImpl : public ClusterManager, Logger::Loggable<Logger::Id::u
                      Runtime::RandomGenerator& random, const LocalInfo::LocalInfo& local_info,
                      AccessLog::AccessLogManager& log_manager,
                      Event::Dispatcher& main_thread_dispatcher, Server::Admin& admin,
-                     ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api,
+                     ProtobufMessage::ValidationContext& validation_context, Api::Api& api,
                      Http::Context& http_context);
 
   // Upstream::ClusterManager

diff --git a/source/server/config_validation/cluster_manager.cc b/source/server/config_validation/cluster_manager.cc
@@ -9,7 +9,7 @@ ClusterManagerPtr ValidationClusterManagerFactory::clusterManagerFromProto(
     const envoy::config::bootstrap::v2::Bootstrap& bootstrap) {
   return std::make_unique<ValidationClusterManager>(
       bootstrap, *this, stats_, tls_, runtime_, random_, local_info_, log_manager_,
-      main_thread_dispatcher_, admin_, validation_visitor_, api_, http_context_, time_system_);
+      main_thread_dispatcher_, admin_, validation_context_, api_, http_context_, time_system_);
 }
 
 CdsApiPtr
@@ -26,10 +26,10 @@ ValidationClusterManager::ValidationClusterManager(
     Stats::Store& stats, ThreadLocal::Instance& tls, Runtime::Loader& runtime,
     Runtime::RandomGenerator& random, const LocalInfo::LocalInfo& local_info,
     AccessLog::AccessLogManager& log_manager, Event::Dispatcher& main_thread_dispatcher,
-    Server::Admin& admin, ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api,
+    Server::Admin& admin, ProtobufMessage::ValidationContext& validation_context, Api::Api& api,
     Http::Context& http_context, Event::TimeSystem& time_system)
     : ClusterManagerImpl(bootstrap, factory, stats, tls, runtime, random, local_info, log_manager,
-                         main_thread_dispatcher, admin, validation_visitor, api, http_context),
+                         main_thread_dispatcher, admin, validation_context, api, http_context),
       async_client_(api, time_system) {}
 
 Http::ConnectionPool::Instance*

diff --git a/source/server/config_validation/cluster_manager.h b/source/server/config_validation/cluster_manager.h
@@ -24,12 +24,12 @@ class ValidationClusterManagerFactory : public ProdClusterManagerFactory {
       ThreadLocal::Instance& tls, Runtime::RandomGenerator& random,
       Network::DnsResolverSharedPtr dns_resolver, Ssl::ContextManager& ssl_context_manager,
       Event::Dispatcher& main_thread_dispatcher, const LocalInfo::LocalInfo& local_info,
-      Secret::SecretManager& secret_manager, ProtobufMessage::ValidationVisitor& validation_visitor,
+      Secret::SecretManager& secret_manager, ProtobufMessage::ValidationContext& validation_context,
       Api::Api& api, Http::Context& http_context, AccessLog::AccessLogManager& log_manager,
       Singleton::Manager& singleton_manager, Event::TimeSystem& time_system)
       : ProdClusterManagerFactory(admin, runtime, stats, tls, random, dns_resolver,
                                   ssl_context_manager, main_thread_dispatcher, local_info,
-                                  secret_manager, validation_visitor, api, http_context,
+                                  secret_manager, validation_context, api, http_context,
                                   log_manager, singleton_manager),
         time_system_(time_system) {}
 
@@ -56,7 +56,7 @@ class ValidationClusterManager : public ClusterManagerImpl {
                            Runtime::RandomGenerator& random, const LocalInfo::LocalInfo& local_info,
                            AccessLog::AccessLogManager& log_manager, Event::Dispatcher& dispatcher,
                            Server::Admin& admin,
-                           ProtobufMessage::ValidationVisitor& validation_visitor, Api::Api& api,
+                           ProtobufMessage::ValidationContext& validation_context, Api::Api& api,
                            Http::Context& http_context, Event::TimeSystem& time_system);
 
   Http::ConnectionPool::Instance* httpConnPoolForCluster(const std::string&, ResourcePriority,

diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc
@@ -43,7 +43,13 @@ ValidationInstance::ValidationInstance(const Options& options, Event::TimeSystem
                                        ComponentFactory& component_factory,
                                        Thread::ThreadFactory& thread_factory,
                                        Filesystem::Instance& file_system)
-    : options_(options), stats_store_(store),
+    : options_(options), validation_context_(options_.allowUnknownFields()
+                                                 ? ProtobufMessage::getNullValidationVisitor()
+                                                 : ProtobufMessage::getStrictValidationVisitor(),
+                                             !options.rejectUnknownFieldsDynamic()
+                                                 ? ProtobufMessage::getNullValidationVisitor()
+                                                 : ProtobufMessage::getStrictValidationVisitor()),
+      stats_store_(store),
       api_(new Api::ValidationImpl(thread_factory, store, time_system, file_system)),
       dispatcher_(api_->allocateDispatcher()),
       singleton_manager_(new Singleton::ManagerImpl(api_->threadFactory())),
@@ -75,7 +81,8 @@ void ValidationInstance::initialize(const Options& options,
   // be ready to serve, then the config has passed validation.
   // Handle configuration that needs to take place prior to the main configuration load.
   envoy::config::bootstrap::v2::Bootstrap bootstrap;
-  InstanceUtil::loadBootstrapConfig(bootstrap, options, messageValidationVisitor(), *api_);
+  InstanceUtil::loadBootstrapConfig(bootstrap, options,
+                                    messageValidationContext().staticValidationVisitor(), *api_);
 
   Config::Utility::createTagProducer(bootstrap);
 
@@ -86,9 +93,9 @@ void ValidationInstance::initialize(const Options& options,
       options.serviceNodeName());
 
   Configuration::InitialImpl initial_config(bootstrap);
-  overload_manager_ = std::make_unique<OverloadManagerImpl>(dispatcher(), stats(), threadLocal(),
-                                                            bootstrap.overload_manager(),
-                                                            messageValidationVisitor(), *api_);
+  overload_manager_ = std::make_unique<OverloadManagerImpl>(
+      dispatcher(), stats(), threadLocal(), bootstrap.overload_manager(),
+      messageValidationContext().staticValidationVisitor(), *api_);
   listener_manager_ = std::make_unique<ListenerManagerImpl>(*this, *this, *this, false);
   thread_local_.registerThread(*dispatcher_, true);
   runtime_loader_ = component_factory.createRuntime(*this, initial_config);
@@ -97,7 +104,7 @@ void ValidationInstance::initialize(const Options& options,
       createContextManager(Ssl::ContextManagerFactory::name(), api_->timeSource());
   cluster_manager_factory_ = std::make_unique<Upstream::ValidationClusterManagerFactory>(
       admin(), runtime(), stats(), threadLocal(), random(), dnsResolver(), sslContextManager(),
-      dispatcher(), localInfo(), *secret_manager_, messageValidationVisitor(), *api_, http_context_,
+      dispatcher(), localInfo(), *secret_manager_, messageValidationContext(), *api_, http_context_,
       accessLogManager(), singletonManager(), time_system_);
   config_.initialize(bootstrap, *this, *cluster_manager_factory_);
   http_context_.setTracer(config_.httpTracer());

diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h
@@ -104,15 +104,15 @@ class ValidationInstance : Logger::Loggable<Logger::Id::main>,
     return config_.statsFlushInterval();
   }
 
-  ProtobufMessage::ValidationVisitor& messageValidationVisitor() override {
-    return options_.allowUnknownFields() ? ProtobufMessage::getStrictValidationVisitor()
-                                         : ProtobufMessage::getNullValidationVisitor();
+  ProtobufMessage::ValidationContext& messageValidationContext() override {
+    return validation_context_;
   }
 
   // Server::ListenerComponentFactory
   LdsApiPtr createLdsApi(const envoy::api::v2::core::ConfigSource& lds_config) override {
     return std::make_unique<LdsApiImpl>(lds_config, clusterManager(), initManager(), stats(),
-                                        listenerManager(), messageValidationVisitor());
+                                        listenerManager(),
+                                        messageValidationContext().dynamicValidationVisitor());
   }
   std::vector<Network::FilterFactoryCb> createNetworkFilterFactoryList(
       const Protobuf::RepeatedPtrField<envoy::api::v2::listener::Filter>& filters,
@@ -173,6 +173,7 @@ class ValidationInstance : Logger::Loggable<Logger::Id::main>,
   // - There may be active connections referencing it.
   std::unique_ptr<Secret::SecretManager> secret_manager_;
   const Options& options_;
+  ProtobufMessage::ValidationContextImpl validation_context_;
   Stats::IsolatedStoreImpl& stats_store_;
   ThreadLocal::InstanceImpl thread_local_;
   Api::ApiPtr api_;