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

api/envoy/admin/v2alpha/server_info.proto

@@ -53,8 +53,11 @@ message CommandLineOptions {
   // See :option:`--config-yaml` for details.
   string config_yaml = 4;
 
-  // See :option:`--allow-unknown-fields` for details.
-  bool allow_unknown_fields = 5;
+  // See :option:`--allow-unknown-static-fields` for details.
+  bool allow_unknown_static_fields = 5;
+
+  // See :option:`--reject-unknown-dynamic-fields` for details.
+  bool reject_unknown_dynamic_fields = 26;
 
   // See :option:`--admin-address-path` for details.
   string admin_address_path = 6;

docs/root/configuration/statistics.rst

@@ -3,6 +3,8 @@
 Statistics
 ==========
 
+.. _server_statistics:
+
 Server
 ------
 
@@ -25,6 +27,8 @@ Server related statistics are rooted at *server.* with following statistics:
   hot_restart_epoch, Gauge, Current hot restart epoch
   initialization_time_ms, Histogram, Total time taken for Envoy initialization in milliseconds. This is the time from server start-up until the worker threads are ready to accept new connections
   debug_assertion_failures, Counter, Number of debug assertion failures detected in a release build if compiled with `--define log_debug_assert_in_release=enabled` or zero otherwise
+  static_unknown_fields, Counter, Number of messages in static configuration with unknown fields
+  dynamic_unknown_fields, Counter, Number of messages in dynamic configuration with unknown fields
 
 File system
 -----------

docs/root/intro/deprecated.rst

@@ -13,6 +13,7 @@ Deprecated items below are listed in chronological order.
 Version 1.12.0 (pending)
 ========================
 * The ORIGINAL_DST_LB :ref:`load balancing policy <envoy_api_field_Cluster.lb_policy>` is deprecated, use CLUSTER_PROVIDED policy instead when configuring an :ref:`original destination cluster <envoy_api_field_Cluster.type>`.
+* The :option:`--allow-unknown-fields` command-line option, use :option:`--allow-unknown-static-fields` instead.
 
 Version 1.11.0 (July 11, 2019)
 ==============================

docs/root/intro/version_history.rst

@@ -10,6 +10,12 @@ Version history
 * config: enforcing that terminal filters (e.g. HttpConnectionManager for L4, router for L7) be the last in their respective filter chains.
 * buffer filter: the buffer filter populates content-length header if not present, behavior can be disabled using the runtime feature `envoy.reloadable_features.buffer_filter_populate_content_length`.
 * config: added access log :ref:`extension filter<envoy_api_field_config.filter.accesslog.v2.AccessLogFilter.extension_filter>`.
+* config: added support for :option:`--reject-unknown-dynamic-fields`, 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. Warnings
+  are logged for the first use of any unknown field and these occurrences are counted in the
+  :ref:`server.static_unknown_fields <server_statistics>` and :ref:`server.dynamic_unknown_fields
+  <server_statistics>` statistics.
 * 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>`.

docs/root/operations/admin.rst

@@ -219,7 +219,7 @@ modify different aspects of the server:
         "concurrency": 8,
         "config_path": "config.yaml",
         "config_yaml": "",
-        "allow_unknown_fields": false,
+        "allow_unknown_static_fields": false,
         "admin_address_path": "",
         "local_address_ip_version": "v4",
         "log_level": "info",

docs/root/operations/cli.rst

@@ -228,10 +228,25 @@ 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)* Deprecated alias for :option:`--allow-unknown-static-fields`.
+
+.. option:: --allow-unknown-static-fields
+
+  *(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. Warnings are logged for the first use of
+  any unknown field and these occurrences are counted in the :ref:`server.static_unknown_fields
+  <server_statistics>` statistic.
+
+.. option:: --reject-unknown-dynamic-fields
+
+  *(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. Warnings are logged for the first use of any unknown field
+  and these occurrences are counted in the :ref:`server.dynamic_unknown_fields <server_statistics>`
+  statistic.
 
 .. option:: --version
 

docs/root/start/sandboxes/front_proxy.rst

@@ -204,7 +204,7 @@ statistics. For example inside ``frontenvoy`` we can get::
       "concurrency": 4,
       "config_path": "/etc/front-envoy.yaml",
       "config_yaml": "",
-      "allow_unknown_fields": false,
+      "allow_unknown_static_fields": false,
       "admin_address_path": "",
       "local_address_ip_version": "v4",
       "log_level": "info",

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

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

include/envoy/server/options.h

@@ -86,9 +86,14 @@ 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;
+  virtual bool allowUnknownStaticFields() const PURE;
+
+  /**
+   * @return bool allow unknown fields in the dynamic configuration?
+   */
+  virtual bool rejectUnknownDynamicFields() const PURE;
 
   /**
    * @return const std::string& the admin address output file.

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 likely, for now we will treat it as rejection.
+      callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, &e);
     }
   }
 }

source/common/protobuf/BUILD

@@ -31,6 +31,8 @@ envoy_cc_library(
     external_deps = ["protobuf"],
     deps = [
         "//include/envoy/protobuf:message_validator_interface",
+        "//include/envoy/stats:stats_interface",
+        "//source/common/common:hash_lib",
         "//source/common/common:logger_lib",
         "//source/common/common:macros",
     ],

source/common/protobuf/message_validator_impl.cc

@@ -2,6 +2,8 @@
 
 #include "envoy/common/exception.h"
 
+#include "common/common/assert.h"
+#include "common/common/hash.h"
 #include "common/common/logger.h"
 #include "common/common/macros.h"
 
@@ -10,6 +12,28 @@
 namespace Envoy {
 namespace ProtobufMessage {
 
+void WarningValidationVisitorImpl::setCounter(Stats::Counter& counter) {
+  ASSERT(counter_ == nullptr, "");
+  counter_ = &counter;
+  counter.add(prestats_count_);
+}
+
+void WarningValidationVisitorImpl::onUnknownField(absl::string_view description) {
+  const uint64_t hash = HashUtil::xxHash64(description);
+  auto it = descriptions_.insert(hash);
+  // If we've seen this before, skip.
+  if (!it.second) {
+    return;
+  }
+  // It's a new field, log and bump stat.
+  ENVOY_LOG(warn, "Unknown field: {}", description);
+  if (counter_ == nullptr) {
+    ++prestats_count_;
+  } else {
+    counter_->inc();
+  }
+}
+
 void StrictValidationVisitorImpl::onUnknownField(absl::string_view description) {
   throw EnvoyException(absl::StrCat("Protobuf message (", description, ") has unknown fields"));
 }

source/common/protobuf/message_validator_impl.h

@@ -1,6 +1,9 @@
 #pragma once
 
 #include "envoy/protobuf/message_validator.h"
+#include "envoy/stats/stats.h"
+
+#include "absl/container/flat_hash_set.h"
 
 namespace Envoy {
 namespace ProtobufMessage {
@@ -13,6 +16,24 @@ class NullValidationVisitorImpl : public ValidationVisitor {
 
 ValidationVisitor& getNullValidationVisitor();
 
+class WarningValidationVisitorImpl : public ValidationVisitor,
+                                     public Logger::Loggable<Logger::Id::config> {
+public:
+  void setCounter(Stats::Counter& counter);
+
+  // Envoy::ProtobufMessage::ValidationVisitor
+  void onUnknownField(absl::string_view description) override;
+
+private:
+  // Track hashes of descriptions we've seen, to avoid log spam. A hash is used here to avoid
+  // wasting memory with unused strings.
+  absl::flat_hash_set<uint64_t> descriptions_;
+  // This can be late initialized via setCounter(), enabling the server bootstrap loading which
+  // occurs prior to the initialization of the stats subsystem.
+  Stats::Counter* counter_{};
+  uint64_t prestats_count_{};
+};
+
 class StrictValidationVisitorImpl : public ValidationVisitor {
 public:
   // Envoy::ProtobufMessage::ValidationVisitor
@@ -21,5 +42,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

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();
@@ -1231,7 +1232,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(
@@ -1261,13 +1262,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

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

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*

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,