diff --git a/api/envoy/admin/v2alpha/server_info.proto b/api/envoy/admin/v2alpha/server_info.proto index 0a4506f1676b..7389af5b0860 100644 --- a/api/envoy/admin/v2alpha/server_info.proto +++ b/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; diff --git a/docs/root/configuration/statistics.rst b/docs/root/configuration/statistics.rst index 1e79f39a0bf9..0042051e0acb 100644 --- a/docs/root/configuration/statistics.rst +++ b/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 ----------- diff --git a/docs/root/intro/deprecated.rst b/docs/root/intro/deprecated.rst index c8399d9ab087..6ca0b034b759 100644 --- a/docs/root/intro/deprecated.rst +++ b/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 ` is deprecated, use CLUSTER_PROVIDED policy instead when configuring an :ref:`original destination cluster `. +* The :option:`--allow-unknown-fields` command-line option, use :option:`--allow-unknown-static-fields` instead. Version 1.11.0 (July 11, 2019) ============================== diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 72e4c3b12f0e..ca60b5387630 100644 --- a/docs/root/intro/version_history.rst +++ b/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`. +* 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 ` and :ref:`server.dynamic_unknown_fields + ` statistics. * config: async data access for local and remote data source. * config: changed the default value of :ref:`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 ` for more details. * config: added stat :ref:`init_fetch_timeout `. diff --git a/docs/root/operations/admin.rst b/docs/root/operations/admin.rst index f0d1f1d73bc2..c681deb55951 100644 --- a/docs/root/operations/admin.rst +++ b/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", diff --git a/docs/root/operations/cli.rst b/docs/root/operations/cli.rst index 3f6494e64138..4ebc9bb4b6c7 100644 --- a/docs/root/operations/cli.rst +++ b/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 + ` 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 ` + statistic. .. option:: --version diff --git a/docs/root/start/sandboxes/front_proxy.rst b/docs/root/start/sandboxes/front_proxy.rst index e14d938cadfd..ee607db133dc 100644 --- a/docs/root/start/sandboxes/front_proxy.rst +++ b/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", diff --git a/include/envoy/protobuf/message_validator.h b/include/envoy/protobuf/message_validator.h index 1459aee4b8be..8c2ac4bc8c4e 100644 --- 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 index dbb964eb3df0..d87e72f84482 100644 --- 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 index 8904925f28e9..8ecc14f0c555 100644 --- a/include/envoy/server/options.h +++ b/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. diff --git a/source/common/config/filesystem_subscription_impl.cc b/source/common/config/filesystem_subscription_impl.cc index f14e39533075..1bef8eafe8f6 100644 --- 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 likely, for now we will treat it as rejection. + callbacks_.onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, &e); } } } diff --git a/source/common/protobuf/BUILD b/source/common/protobuf/BUILD index e6fa19e96d0a..4feeec55c982 100644 --- a/source/common/protobuf/BUILD +++ b/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", ], diff --git a/source/common/protobuf/message_validator_impl.cc b/source/common/protobuf/message_validator_impl.cc index ba89b89bdb7b..4e921a0f0215 100644 --- a/source/common/protobuf/message_validator_impl.cc +++ b/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")); } diff --git a/source/common/protobuf/message_validator_impl.h b/source/common/protobuf/message_validator_impl.h index cf42cb4e26a8..2d5b3d41af0f 100644 --- a/source/common/protobuf/message_validator_impl.h +++ b/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 { +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 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,43 @@ 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_; +}; + +class ProdValidationContextImpl : public ValidationContextImpl { +public: + ProdValidationContextImpl(bool allow_unknown_static_fields, bool allow_unknown_dynamic_fields) + : ValidationContextImpl(allow_unknown_static_fields ? static_warning_validation_visitor_ + : getStrictValidationVisitor(), + allow_unknown_dynamic_fields + ? dynamic_warning_validation_visitor_ + : ProtobufMessage::getStrictValidationVisitor()) {} + + ProtobufMessage::WarningValidationVisitorImpl& static_warning_validation_visitor() { + return static_warning_validation_visitor_; + } + + ProtobufMessage::WarningValidationVisitorImpl& dynamic_warning_validation_visitor() { + return dynamic_warning_validation_visitor_; + } + +private: + ProtobufMessage::WarningValidationVisitorImpl static_warning_validation_visitor_; + ProtobufMessage::WarningValidationVisitorImpl dynamic_warning_validation_visitor_; +}; + } // namespace ProtobufMessage } // namespace Envoy diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 69dae7cef8bb..7c4ac14f1bee 100644 --- 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(*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 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 index 055cb9fd2cce..ae0ecb2dbd40 100644 --- 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( 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 index f4b4ce40bd9b..5a7f29955475 100644 --- 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 index 9e32616d7124..f0f63045d604 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -43,7 +43,9 @@ 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_.allowUnknownStaticFields(), + !options.rejectUnknownDynamicFields()), + 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 +77,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 +89,9 @@ void ValidationInstance::initialize(const Options& options, options.serviceNodeName()); Configuration::InitialImpl initial_config(bootstrap); - overload_manager_ = std::make_unique(dispatcher(), stats(), threadLocal(), - bootstrap.overload_manager(), - messageValidationVisitor(), *api_); + overload_manager_ = std::make_unique( + dispatcher(), stats(), threadLocal(), bootstrap.overload_manager(), + messageValidationContext().staticValidationVisitor(), *api_); listener_manager_ = std::make_unique(*this, *this, *this, false); thread_local_.registerThread(*dispatcher_, true); runtime_loader_ = component_factory.createRuntime(*this, initial_config); @@ -97,7 +100,7 @@ void ValidationInstance::initialize(const Options& options, createContextManager(Ssl::ContextManagerFactory::name(), api_->timeSource()); cluster_manager_factory_ = std::make_unique( 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 index 2a710cd23675..bbd350b325f1 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -104,15 +104,15 @@ class ValidationInstance : Logger::Loggable, 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(lds_config, clusterManager(), initManager(), stats(), - listenerManager(), messageValidationVisitor()); + listenerManager(), + messageValidationContext().dynamicValidationVisitor()); } std::vector createNetworkFilterFactoryList( const Protobuf::RepeatedPtrField& filters, @@ -173,6 +173,7 @@ class ValidationInstance : Logger::Loggable, // - There may be active connections referencing it. std::unique_ptr secret_manager_; const Options& options_; + ProtobufMessage::ProdValidationContextImpl validation_context_; Stats::IsolatedStoreImpl& stats_store_; ThreadLocal::InstanceImpl thread_local_; Api::ApiPtr api_; diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index 20c87a41e039..da83272b5392 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -108,7 +108,7 @@ void MainImpl::initializeTracers(const envoy::config::trace::v2::Tracing& config // Now see if there is a factory that will accept the config. auto& factory = Config::Utility::getAndCheckFactory(type); ProtobufTypes::MessagePtr message = Config::Utility::translateToFactoryConfig( - configuration.http(), server.messageValidationVisitor(), factory); + configuration.http(), server.messageValidationContext().staticValidationVisitor(), factory); http_tracer_ = factory.createHttpTracer(*message, server); } @@ -120,7 +120,7 @@ void MainImpl::initializeStatsSinks(const envoy::config::bootstrap::v2::Bootstra // Generate factory and translate stats sink custom config auto& factory = Config::Utility::getAndCheckFactory(sink_object.name()); ProtobufTypes::MessagePtr message = Config::Utility::translateToFactoryConfig( - sink_object, server.messageValidationVisitor(), factory); + sink_object, server.messageValidationContext().staticValidationVisitor(), factory); stats_sinks_.emplace_back(factory.createStatsSink(*message, server)); } diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 6db28bdd0f36..0f2ec74ee62b 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -187,8 +187,9 @@ ProdListenerComponentFactory::createDrainManager(envoy::api::v2::Listener::Drain } ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, const std::string& version_info, - ListenerManagerImpl& parent, const std::string& name, bool modifiable, - bool workers_started, uint64_t hash) + ListenerManagerImpl& parent, const std::string& name, bool added_via_api, + bool workers_started, uint64_t hash, + ProtobufMessage::ValidationVisitor& validation_visitor) : parent_(parent), address_(Network::Address::resolveProtoAddress(config.address())), filter_chain_manager_(address_), socket_type_(Network::Utility::protobufAddressSocketType(config.address())), @@ -200,8 +201,8 @@ ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, const std::st PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, use_original_dst, false)), per_connection_buffer_limit_bytes_( PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, per_connection_buffer_limit_bytes, 1024 * 1024)), - listener_tag_(parent_.factory_.nextListenerTag()), name_(name), modifiable_(modifiable), - workers_started_(workers_started), hash_(hash), + listener_tag_(parent_.factory_.nextListenerTag()), name_(name), added_via_api_(added_via_api), + workers_started_(workers_started), hash_(hash), validation_visitor_(validation_visitor), dynamic_init_manager_(fmt::format("Listener {}", name)), init_watcher_(std::make_unique( "ListenerImpl", [this] { parent_.onListenerWarmed(*this); })), @@ -284,8 +285,7 @@ ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, const std::st parent_.server_.admin(), parent_.server_.sslContextManager(), *listener_scope_, parent_.server_.clusterManager(), parent_.server_.localInfo(), parent_.server_.dispatcher(), parent_.server_.random(), parent_.server_.stats(), parent_.server_.singletonManager(), - parent_.server_.threadLocal(), parent_.server_.messageValidationVisitor(), - parent_.server_.api()); + parent_.server_.threadLocal(), validation_visitor, parent_.server_.api()); factory_context.setInitManager(initManager()); ListenerFilterChainFactoryBuilder builder(*this, factory_context); filter_chain_manager_.addFilterChain(config.filter_chains(), builder); @@ -488,7 +488,7 @@ ListenerManagerStats ListenerManagerImpl::generateStats(Stats::Scope& scope) { } bool ListenerManagerImpl::addOrUpdateListener(const envoy::api::v2::Listener& config, - const std::string& version_info, bool modifiable) { + const std::string& version_info, bool added_via_api) { std::string name; if (!config.name().empty()) { name = config.name(); @@ -511,8 +511,10 @@ bool ListenerManagerImpl::addOrUpdateListener(const envoy::api::v2::Listener& co return false; } - ListenerImplPtr new_listener( - new ListenerImpl(config, version_info, *this, name, modifiable, workers_started_, hash)); + ListenerImplPtr new_listener(new ListenerImpl( + config, version_info, *this, name, added_via_api, workers_started_, hash, + added_via_api ? server_.messageValidationContext().dynamicValidationVisitor() + : server_.messageValidationContext().staticValidationVisitor())); ListenerImpl& new_listener_ref = *new_listener; // We mandate that a listener with the same name must have the same configured address. This diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index 9177a0ec03af..1197ac82a9be 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -60,9 +60,9 @@ class ProdListenerComponentFactory : public ListenerComponentFactory, // Server::ListenerComponentFactory LdsApiPtr createLdsApi(const envoy::api::v2::core::ConfigSource& lds_config) override { - return std::make_unique(lds_config, server_.clusterManager(), server_.initManager(), - server_.stats(), server_.listenerManager(), - server_.messageValidationVisitor()); + return std::make_unique( + lds_config, server_.clusterManager(), server_.initManager(), server_.stats(), + server_.listenerManager(), server_.messageValidationContext().dynamicValidationVisitor()); } std::vector createNetworkFilterFactoryList( const Protobuf::RepeatedPtrField& filters, @@ -126,7 +126,7 @@ class ListenerManagerImpl : public ListenerManager, Logger::Loggable admin_address_path("", "admin-address-path", "Admin address path", false, "", "string", cmd); @@ -181,7 +188,13 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv, config_path_ = config_path.getValue(); config_yaml_ = config_yaml.getValue(); - allow_unknown_fields_ = allow_unknown_fields.getValue(); + if (allow_unknown_fields.getValue()) { + ENVOY_LOG(warn, + "--allow-unknown-fields is deprecated, use --allow-unknown-static-fields instead."); + } + allow_unknown_static_fields_ = + allow_unknown_static_fields.getValue() || allow_unknown_fields.getValue(); + reject_unknown_dynamic_fields_ = reject_unknown_dynamic_fields.getValue(); admin_address_path_ = admin_address_path.getValue(); log_path_ = log_path.getValue(); restart_epoch_ = restart_epoch.getValue(); @@ -241,7 +254,8 @@ Server::CommandLineOptionsPtr OptionsImpl::toCommandLineOptions() const { command_line_options->set_concurrency(concurrency()); command_line_options->set_config_path(configPath()); command_line_options->set_config_yaml(configYaml()); - command_line_options->set_allow_unknown_fields(allow_unknown_fields_); + command_line_options->set_allow_unknown_static_fields(allow_unknown_static_fields_); + command_line_options->set_reject_unknown_dynamic_fields(reject_unknown_dynamic_fields_); command_line_options->set_admin_address_path(adminAddressPath()); command_line_options->set_component_log_level(component_log_level_str_); command_line_options->set_log_level(spdlog::level::to_string_view(logLevel()).data(), diff --git a/source/server/options_impl.h b/source/server/options_impl.h index e9663953aa99..7fea3a2546a1 100644 --- a/source/server/options_impl.h +++ b/source/server/options_impl.h @@ -75,6 +75,12 @@ class OptionsImpl : public Server::Options, protected Logger::Loggable process_context) - : workers_started_(false), shutdown_(false), options_(options), time_source_(time_system), - restarter_(restarter), start_time_(time(nullptr)), original_start_time_(start_time_), - stats_store_(store), thread_local_(tls), + : workers_started_(false), shutdown_(false), options_(options), + validation_context_(options_.allowUnknownStaticFields(), + !options.rejectUnknownDynamicFields()), + time_source_(time_system), restarter_(restarter), start_time_(time(nullptr)), + original_start_time_(start_time_), stats_store_(store), thread_local_(tls), api_(new Api::Impl(thread_factory, store, time_system, file_system)), dispatcher_(api_->allocateDispatcher()), singleton_manager_(new Singleton::ManagerImpl(api_->threadFactory())), @@ -269,7 +271,8 @@ void InstanceImpl::initialize(const Options& options, Buffer::OwnedImpl().usesOldImpl() ? "old (libevent)" : "new"); // Handle configuration that needs to take place prior to the main configuration load. - InstanceUtil::loadBootstrapConfig(bootstrap_, options, messageValidationVisitor(), *api_); + InstanceUtil::loadBootstrapConfig(bootstrap_, options, + messageValidationContext().staticValidationVisitor(), *api_); bootstrap_config_update_time_ = time_source_.systemTime(); // Immediate after the bootstrap has been loaded, override the header prefix, if configured to @@ -289,6 +292,10 @@ void InstanceImpl::initialize(const Options& options, ServerStats{ALL_SERVER_STATS(POOL_COUNTER_PREFIX(stats_store_, server_stats_prefix), POOL_GAUGE_PREFIX(stats_store_, server_stats_prefix), POOL_HISTOGRAM_PREFIX(stats_store_, server_stats_prefix))}); + validation_context_.static_warning_validation_visitor().setCounter( + server_stats_->static_unknown_fields_); + validation_context_.dynamic_warning_validation_visitor().setCounter( + server_stats_->dynamic_unknown_fields_); initialization_timer_ = std::make_unique(server_stats_->initialization_time_ms_, timeSource()); @@ -342,7 +349,7 @@ void InstanceImpl::initialize(const Options& options, // Initialize the overload manager early so other modules can register for actions. overload_manager_ = std::make_unique( *dispatcher_, stats_store_, thread_local_, bootstrap_.overload_manager(), - messageValidationVisitor(), *api_); + messageValidationContext().staticValidationVisitor(), *api_); heap_shrinker_ = std::make_unique(*dispatcher_, *overload_manager_, stats_store_); @@ -375,7 +382,7 @@ void InstanceImpl::initialize(const Options& options, cluster_manager_factory_ = std::make_unique( *admin_, Runtime::LoaderSingleton::get(), stats_store_, thread_local_, *random_generator_, dns_resolver_, *ssl_context_manager_, *dispatcher_, *local_info_, *secret_manager_, - messageValidationVisitor(), *api_, http_context_, access_log_manager_, *singleton_manager_); + messageValidationContext(), *api_, http_context_, access_log_manager_, *singleton_manager_); // Now the configuration gets parsed. The configuration may start setting // thread local data per above. See MainImpl::initialize() for why ConfigImpl @@ -405,8 +412,8 @@ void InstanceImpl::initialize(const Options& options, ->create(), *dispatcher_, Runtime::LoaderSingleton::get(), stats_store_, *ssl_context_manager_, *random_generator_, info_factory_, access_log_manager_, *config_.clusterManager(), - *local_info_, *admin_, *singleton_manager_, thread_local_, messageValidationVisitor(), - *api_); + *local_info_, *admin_, *singleton_manager_, thread_local_, + messageValidationContext().dynamicValidationVisitor(), *api_); } for (Stats::SinkPtr& sink : config_.statsSinks()) { @@ -438,8 +445,8 @@ Runtime::LoaderPtr InstanceUtil::createRuntime(Instance& server, ENVOY_LOG(info, "runtime: {}", MessageUtil::getYamlStringFromMessage(config.runtime())); return std::make_unique( server.dispatcher(), server.threadLocal(), config.runtime(), server.localInfo(), - server.initManager(), server.stats(), server.random(), server.messageValidationVisitor(), - server.api()); + server.initManager(), server.stats(), server.random(), + server.messageValidationContext().dynamicValidationVisitor(), server.api()); } void InstanceImpl::loadServerFlags(const absl::optional& flags_path) { diff --git a/source/server/server.h b/source/server/server.h index 90128e9b2deb..500f743f98a6 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -49,6 +49,8 @@ namespace Server { * All server wide stats. @see stats_macros.h */ #define ALL_SERVER_STATS(COUNTER, GAUGE, HISTOGRAM) \ + COUNTER(static_unknown_fields) \ + COUNTER(dynamic_unknown_fields) \ COUNTER(debug_assertion_failures) \ GAUGE(concurrency, NeverImport) \ GAUGE(days_until_first_cert_expiring, Accumulate) \ @@ -201,9 +203,8 @@ class InstanceImpl : Logger::Loggable, return config_.statsFlushInterval(); } - ProtobufMessage::ValidationVisitor& messageValidationVisitor() override { - return options_.allowUnknownFields() ? ProtobufMessage::getStrictValidationVisitor() - : ProtobufMessage::getNullValidationVisitor(); + ProtobufMessage::ValidationContext& messageValidationContext() override { + return validation_context_; } // ServerLifecycleNotifier @@ -236,6 +237,7 @@ class InstanceImpl : Logger::Loggable, bool workers_started_; bool shutdown_; const Options& options_; + ProtobufMessage::ProdValidationContextImpl validation_context_; TimeSource& time_source_; HotRestart& restarter_; const time_t start_time_; diff --git a/test/common/config/filesystem_subscription_impl_test.cc b/test/common/config/filesystem_subscription_impl_test.cc index 840748a72800..524914346484 100644 --- a/test/common/config/filesystem_subscription_impl_test.cc +++ b/test/common/config/filesystem_subscription_impl_test.cc @@ -20,7 +20,7 @@ TEST_F(FilesystemSubscriptionImplTest, BadJsonRecovery) { startSubscription({"cluster0", "cluster1"}); EXPECT_TRUE(statsAre(1, 0, 0, 0, 0, 0)); EXPECT_CALL(callbacks_, - onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::ConnectionFailure, _)); + onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, _)); updateFile(";!@#badjso n"); EXPECT_TRUE(statsAre(2, 0, 0, 1, 0, 0)); deliverConfigUpdate({"cluster0", "cluster1"}, "0", true); diff --git a/test/common/protobuf/BUILD b/test/common/protobuf/BUILD index 94fbcced56c7..81c30eb6a241 100644 --- a/test/common/protobuf/BUILD +++ b/test/common/protobuf/BUILD @@ -9,6 +9,16 @@ load( envoy_package() +envoy_cc_test( + name = "message_validator_impl_test", + srcs = ["message_validator_impl_test.cc"], + deps = [ + "//source/common/protobuf:message_validator_lib", + "//test/test_common:logging_lib", + "//test/test_common:utility_lib", + ], +) + envoy_cc_test( name = "utility_test", srcs = ["utility_test.cc"], diff --git a/test/common/protobuf/message_validator_impl_test.cc b/test/common/protobuf/message_validator_impl_test.cc new file mode 100644 index 000000000000..a2fa6f4b011a --- /dev/null +++ b/test/common/protobuf/message_validator_impl_test.cc @@ -0,0 +1,54 @@ +#include "envoy/common/exception.h" + +#include "common/protobuf/message_validator_impl.h" +#include "common/stats/isolated_store_impl.h" + +#include "test/test_common/logging.h" +#include "test/test_common/utility.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace ProtobufMessage { +namespace { + +// The null validation visitor doesn't do anything on unknown fields. +TEST(NullValidationVisitorImpl, UnknownField) { + NullValidationVisitorImpl null_validation_visitor; + EXPECT_NO_THROW(null_validation_visitor.onUnknownField("foo")); +} + +// The warning validation visitor logs and bumps stats on unknown fields +TEST(WarningValidationVisitorImpl, UnknownField) { + Stats::IsolatedStoreImpl stats; + Stats::Counter& counter = stats.counter("counter"); + WarningValidationVisitorImpl warning_validation_visitor; + // First time around we should log. + EXPECT_LOG_CONTAINS("warn", "Unknown field: foo", + warning_validation_visitor.onUnknownField("foo")); + // Duplicate descriptions don't generate a log the second time around. + EXPECT_LOG_NOT_CONTAINS("warn", "Unknown field: foo", + warning_validation_visitor.onUnknownField("foo")); + // Unrelated variable increments. + EXPECT_LOG_CONTAINS("warn", "Unknown field: bar", + warning_validation_visitor.onUnknownField("bar")); + // When we set the stats counter, the above increments are transferred. + EXPECT_EQ(0, counter.value()); + warning_validation_visitor.setCounter(counter); + EXPECT_EQ(2, counter.value()); + // A third unknown field is tracked in stats post-initialization. + EXPECT_LOG_CONTAINS("warn", "Unknown field: baz", + warning_validation_visitor.onUnknownField("baz")); + EXPECT_EQ(3, counter.value()); +} + +// The strict validation visitor throws on unknown fields. +TEST(StrictValidationVisitorImpl, UnknownField) { + StrictValidationVisitorImpl strict_validation_visitor; + EXPECT_THROW_WITH_MESSAGE(strict_validation_visitor.onUnknownField("foo"), EnvoyException, + "Protobuf message (foo) has unknown fields"); +} + +} // namespace +} // namespace ProtobufMessage +} // namespace Envoy diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 5ed984b29124..a7ada1599f80 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -162,9 +162,10 @@ class TestClusterManagerImpl : public ClusterManagerImpl { Runtime::RandomGenerator& random, const LocalInfo::LocalInfo& local_info, AccessLog::AccessLogManager& log_manager, Event::Dispatcher& main_thread_dispatcher, Server::Admin& admin, - Api::Api& api, Http::Context& http_context) + ProtobufMessage::ValidationContext& validation_context, Api::Api& api, + Http::Context& http_context) : 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) {} std::map> activeClusters() { std::map> clusters; @@ -173,8 +174,6 @@ class TestClusterManagerImpl : public ClusterManagerImpl { } return clusters; } - - NiceMock validation_visitor_; }; // Override postThreadLocalClusterUpdate so we can test that merged updates calls @@ -186,10 +185,12 @@ class MockedUpdatedClusterManagerImpl : public TestClusterManagerImpl { 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, Api::Api& api, MockLocalClusterUpdate& local_cluster_update, - MockLocalHostsRemoved& local_hosts_removed, Http::Context& http_context) + Server::Admin& admin, ProtobufMessage::ValidationContext& validation_context, Api::Api& api, + MockLocalClusterUpdate& local_cluster_update, MockLocalHostsRemoved& local_hosts_removed, + Http::Context& http_context) : TestClusterManagerImpl(bootstrap, factory, stats, tls, runtime, random, local_info, - log_manager, main_thread_dispatcher, admin, api, http_context), + log_manager, main_thread_dispatcher, admin, validation_context, api, + http_context), local_cluster_update_(local_cluster_update), local_hosts_removed_(local_hosts_removed) {} protected: @@ -225,7 +226,8 @@ class ClusterManagerImplTest : public testing::Test { void create(const envoy::config::bootstrap::v2::Bootstrap& bootstrap) { cluster_manager_ = std::make_unique( bootstrap, factory_, factory_.stats_, factory_.tls_, factory_.runtime_, factory_.random_, - factory_.local_info_, log_manager_, factory_.dispatcher_, admin_, *api_, http_context_); + factory_.local_info_, log_manager_, factory_.dispatcher_, admin_, validation_context_, + *api_, http_context_); } void createWithLocalClusterUpdate(const bool enable_merge_window = true) { @@ -259,8 +261,8 @@ class ClusterManagerImplTest : public testing::Test { cluster_manager_ = std::make_unique( bootstrap, factory_, factory_.stats_, factory_.tls_, factory_.runtime_, factory_.random_, - factory_.local_info_, log_manager_, factory_.dispatcher_, admin_, *api_, - local_cluster_update_, local_hosts_removed_, http_context_); + factory_.local_info_, log_manager_, factory_.dispatcher_, admin_, validation_context_, + *api_, local_cluster_update_, local_hosts_removed_, http_context_); } void checkStats(uint64_t added, uint64_t modified, uint64_t removed, uint64_t active, @@ -303,6 +305,7 @@ class ClusterManagerImplTest : public testing::Test { Event::SimulatedTimeSystem time_system_; Api::ApiPtr api_; NiceMock factory_; + NiceMock validation_context_; std::unique_ptr cluster_manager_; AccessLog::MockAccessLogManager log_manager_; NiceMock admin_; diff --git a/test/config/integration/BUILD b/test/config/integration/BUILD index d28d2d8011fa..a6c4442e3e0b 100644 --- a/test/config/integration/BUILD +++ b/test/config/integration/BUILD @@ -16,9 +16,13 @@ filegroup( name = "server_xds_files", srcs = [ "server_xds.bootstrap.yaml", + "server_xds.cds.with_unknown_field.yaml", "server_xds.cds.yaml", + "server_xds.eds.with_unknown_field.yaml", "server_xds.eds.yaml", + "server_xds.lds.with_unknown_field.yaml", "server_xds.lds.yaml", + "server_xds.rds.with_unknown_field.yaml", "server_xds.rds.yaml", ], ) diff --git a/test/config/integration/server.yaml b/test/config/integration/server.yaml index 455a17bc0592..88d34049619b 100644 --- a/test/config/integration/server.yaml +++ b/test/config/integration/server.yaml @@ -8,10 +8,10 @@ static_resources: - filters: - name: envoy.http_connection_manager config: - drain_timeout_ms: 5000 + drain_timeout: 5s route_config: virtual_hosts: - - require_ssl: all + - require_tls: all routes: - route: { cluster: cluster_1 } match: { prefix: "/" } @@ -22,9 +22,6 @@ static_resources: - match: { prefix: "/" } route: cluster: cluster_1 - runtime: - key: some_key - default: 0 - match: { prefix: "/test/long/url" } route: rate_limits: @@ -42,15 +39,11 @@ static_resources: name: integration codec_type: http1 stat_prefix: router - filters: - - name: health_check + http_filters: + - name: envoy.health_check config: - endpoint: "/healthcheck" pass_through_mode: false - - name: rate_limit - config: - domain: foo - - name: router + - name: envoy.router config: {} access_log: - name: envoy.file_access_log diff --git a/test/config/integration/server_unix_listener.yaml b/test/config/integration/server_unix_listener.yaml index 0ba01442cb6d..bd0c6f090403 100644 --- a/test/config/integration/server_unix_listener.yaml +++ b/test/config/integration/server_unix_listener.yaml @@ -7,12 +7,12 @@ static_resources: - filters: - name: envoy.http_connection_manager config: - filters: - - name: router + http_filters: + - name: envoy.router config: {} codec_type: auto stat_prefix: router - drain_timeout_ms: 5000 + drain_timeout: 5s route_config: virtual_hosts: - domains: diff --git a/test/config/integration/server_xds.cds.with_unknown_field.yaml b/test/config/integration/server_xds.cds.with_unknown_field.yaml new file mode 100644 index 000000000000..01d794ab4032 --- /dev/null +++ b/test/config/integration/server_xds.cds.with_unknown_field.yaml @@ -0,0 +1,15 @@ +version_info: "0" +resources: +- "@type": type.googleapis.com/envoy.api.v2.Cluster + name: cluster_1 + connect_timeout: { seconds: 5 } + type: EDS + eds_cluster_config: + eds_config: { path: {{ eds_json_path }} } + lb_policy: ROUND_ROBIN + http2_protocol_options: {} + extension_protocol_options: + envoy.test.dynamic_validation: + stat_prefix: blah + cluster: blah + foo: bar diff --git a/test/config/integration/server_xds.eds.with_unknown_field.yaml b/test/config/integration/server_xds.eds.with_unknown_field.yaml new file mode 100644 index 000000000000..912be177993d --- /dev/null +++ b/test/config/integration/server_xds.eds.with_unknown_field.yaml @@ -0,0 +1,12 @@ +version_info: "0" +resources: +- "@type": type.googleapis.com/envoy.api.v2.ClusterLoadAssignment + cluster_name: cluster_1 + foo: bar + endpoints: + - lb_endpoints: + - endpoint: + address: + socket_address: + address: {{ ntop_ip_loopback_address }} + port_value: {{ upstream_0 }} diff --git a/test/config/integration/server_xds.lds.with_unknown_field.yaml b/test/config/integration/server_xds.lds.with_unknown_field.yaml new file mode 100644 index 000000000000..e1fe53b7127b --- /dev/null +++ b/test/config/integration/server_xds.lds.with_unknown_field.yaml @@ -0,0 +1,20 @@ +version_info: "0" +resources: +- "@type": type.googleapis.com/envoy.api.v2.Listener + name: listener_0 + address: + socket_address: + address: {{ ntop_ip_loopback_address }} + port_value: 0 + filter_chains: + - filters: + - name: envoy.http_connection_manager + config: + codec_type: HTTP2 + drain_timeout: 5s + stat_prefix: router + rds: + route_config_name: route_config_0 + config_source: { path: {{ rds_json_path }} } + http_filters: [{ name: envoy.router }] + foo: bar diff --git a/test/config/integration/server_xds.rds.with_unknown_field.yaml b/test/config/integration/server_xds.rds.with_unknown_field.yaml new file mode 100644 index 000000000000..0fb40bd6a74a --- /dev/null +++ b/test/config/integration/server_xds.rds.with_unknown_field.yaml @@ -0,0 +1,11 @@ +version_info: "0" +resources: +- "@type": type.googleapis.com/envoy.api.v2.RouteConfiguration + name: route_config_0 + virtual_hosts: + - name: integration + domains: [ "*" ] + routes: + - match: { prefix: "/test/long/url" } + route: { cluster: cluster_1 } + foo: bar diff --git a/test/config_test/config_test.cc b/test/config_test/config_test.cc index b552cb9ec31b..d7311ec8c160 100644 --- a/test/config_test/config_test.cc +++ b/test/config_test/config_test.cc @@ -81,15 +81,15 @@ class ConfigTest { })); envoy::config::bootstrap::v2::Bootstrap bootstrap; - Server::InstanceUtil::loadBootstrapConfig(bootstrap, options_, - server_.messageValidationVisitor(), *api_); + Server::InstanceUtil::loadBootstrapConfig( + bootstrap, options_, server_.messageValidationContext().staticValidationVisitor(), *api_); Server::Configuration::InitialImpl initial_config(bootstrap); Server::Configuration::MainImpl main_config; cluster_manager_factory_ = std::make_unique( server_.admin(), server_.runtime(), server_.stats(), server_.threadLocal(), server_.random(), server_.dnsResolver(), ssl_context_manager_, server_.dispatcher(), - server_.localInfo(), server_.secretManager(), server_.messageValidationVisitor(), *api_, + server_.localInfo(), server_.secretManager(), server_.messageValidationContext(), *api_, server_.httpContext(), server_.accessLogManager(), server_.singletonManager(), time_system_); diff --git a/test/integration/BUILD b/test/integration/BUILD index f100db5066d8..393eadbfa2e8 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -772,6 +772,16 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "dynamic_validation_integration_test", + srcs = ["dynamic_validation_integration_test.cc"], + data = ["//test/config/integration:server_xds_files"], + deps = [ + ":http_integration_lib", + "//source/common/stats:stats_lib", + ], +) + envoy_cc_test( name = "xds_integration_test", srcs = ["xds_integration_test.cc"], diff --git a/test/integration/dynamic_validation_integration_test.cc b/test/integration/dynamic_validation_integration_test.cc new file mode 100644 index 000000000000..b4344312b85a --- /dev/null +++ b/test/integration/dynamic_validation_integration_test.cc @@ -0,0 +1,181 @@ +#include + +#include "envoy/config/filter/network/tcp_proxy/v2/tcp_proxy.pb.validate.h" + +#include "extensions/filters/network/common/factory_base.h" + +#include "test/integration/http_integration.h" +#include "test/test_common/environment.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace { + +// This fake filter is used by CdsProtocolOptionsRejected. +class TestDynamicValidationNetworkFilter : public Network::WriteFilter { +public: + Network::FilterStatus onWrite(Buffer::Instance&, bool) override { + return Network::FilterStatus::Continue; + } +}; + +class TestDynamicValidationNetworkFilterConfigFactory + : public Extensions::NetworkFilters::Common::FactoryBase< + envoy::config::filter::network::tcp_proxy::v2::TcpProxy> { +public: + TestDynamicValidationNetworkFilterConfigFactory() + : Extensions::NetworkFilters::Common::FactoryBase< + envoy::config::filter::network::tcp_proxy::v2::TcpProxy>( + "envoy.test.dynamic_validation") {} + +private: + Network::FilterFactoryCb createFilterFactoryFromProtoTyped( + const envoy::config::filter::network::tcp_proxy::v2::TcpProxy& /*proto_config*/, + Server::Configuration::FactoryContext& /*context*/) override { + return Network::FilterFactoryCb(); + } + + Upstream::ProtocolOptionsConfigConstSharedPtr createProtocolOptionsTyped( + const envoy::config::filter::network::tcp_proxy::v2::TcpProxy&) override { + return nullptr; + } +}; + +REGISTER_FACTORY(TestDynamicValidationNetworkFilterConfigFactory, + Server::Configuration::NamedNetworkFilterConfigFactory); + +// Pretty-printing of parameterized test names. +std::string dynamicValidationTestParamsToString( + const ::testing::TestParamInfo>& params) { + return fmt::format( + "{}_{}", + TestUtility::ipTestParamsToString( + ::testing::TestParamInfo(std::get<0>(params.param), 0)), + std::get<1>(params.param) ? "with_reject_unknown_fields" : "without_reject_unknown_fields"); +} + +// Validate unknown field handling in dynamic configuration. +class DynamicValidationIntegrationTest + : public testing::TestWithParam>, + public HttpIntegrationTest { +public: + DynamicValidationIntegrationTest() + : HttpIntegrationTest(Http::CodecClient::Type::HTTP2, std::get<0>(GetParam())), + reject_unknown_dynamic_fields_(std::get<1>(GetParam())) { + setUpstreamProtocol(FakeHttpConnection::Type::HTTP2); + } + + void createEnvoy() override { + registerPort("upstream_0", fake_upstreams_.back()->localAddress()->ip()->port()); + createApiTestServer(api_filesystem_config_, {"http"}, reject_unknown_dynamic_fields_, + reject_unknown_dynamic_fields_, allow_lds_rejection_); + } + + ApiFilesystemConfig api_filesystem_config_; + const bool reject_unknown_dynamic_fields_; + bool allow_lds_rejection_{}; +}; + +INSTANTIATE_TEST_SUITE_P( + IpVersions, DynamicValidationIntegrationTest, + testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), testing::Bool()), + dynamicValidationTestParamsToString); + +// Protocol options in CDS with unknown fields are rejected if and only if strict. +TEST_P(DynamicValidationIntegrationTest, CdsProtocolOptionsRejected) { + api_filesystem_config_ = { + "test/config/integration/server_xds.bootstrap.yaml", + "test/config/integration/server_xds.cds.with_unknown_field.yaml", + "test/config/integration/server_xds.eds.yaml", + "test/config/integration/server_xds.lds.yaml", + "test/config/integration/server_xds.rds.yaml", + }; + initialize(); + if (reject_unknown_dynamic_fields_) { + EXPECT_EQ(0, test_server_->counter("cluster_manager.cds.update_success")->value()); + // CDS API parsing will reject due to unknown HCM field. + EXPECT_EQ(1, test_server_->counter("cluster_manager.cds.update_rejected")->value()); + EXPECT_EQ(0, test_server_->counter("server.dynamic_unknown_fields")->value()); + } else { + EXPECT_EQ(1, test_server_->counter("cluster_manager.cds.update_success")->value()); + EXPECT_EQ(1, test_server_->counter("server.dynamic_unknown_fields")->value()); + } +} + +// Network filters in LDS with unknown fields are rejected if and only if strict. +TEST_P(DynamicValidationIntegrationTest, LdsFilterRejected) { + allow_lds_rejection_ = true; + api_filesystem_config_ = { + "test/config/integration/server_xds.bootstrap.yaml", + "test/config/integration/server_xds.cds.yaml", + "test/config/integration/server_xds.eds.yaml", + "test/config/integration/server_xds.lds.with_unknown_field.yaml", + "test/config/integration/server_xds.rds.yaml", + }; + initialize(); + if (reject_unknown_dynamic_fields_) { + EXPECT_EQ(0, test_server_->counter("listener_manager.lds.update_success")->value()); + // LDS API parsing will reject due to unknown HCM field. + EXPECT_EQ(1, test_server_->counter("listener_manager.lds.update_rejected")->value()); + EXPECT_EQ(nullptr, test_server_->counter("http.router.rds.route_config_0.update_success")); + EXPECT_EQ(0, test_server_->counter("server.dynamic_unknown_fields")->value()); + } else { + EXPECT_EQ(1, test_server_->counter("listener_manager.lds.update_success")->value()); + EXPECT_EQ(1, test_server_->counter("http.router.rds.route_config_0.update_success")->value()); + EXPECT_EQ(1, test_server_->counter("server.dynamic_unknown_fields")->value()); + } + EXPECT_EQ(1, test_server_->counter("cluster_manager.cds.update_success")->value()); + EXPECT_EQ(1, test_server_->counter("cluster.cluster_1.update_success")->value()); +} + +// Unknown fields in RDS cause config load failure if and only if strict. +TEST_P(DynamicValidationIntegrationTest, RdsFailedBySubscription) { + api_filesystem_config_ = { + "test/config/integration/server_xds.bootstrap.yaml", + "test/config/integration/server_xds.cds.yaml", + "test/config/integration/server_xds.eds.yaml", + "test/config/integration/server_xds.lds.yaml", + "test/config/integration/server_xds.rds.with_unknown_field.yaml", + }; + initialize(); + EXPECT_EQ(1, test_server_->counter("listener_manager.lds.update_success")->value()); + if (reject_unknown_dynamic_fields_) { + EXPECT_EQ(0, test_server_->counter("http.router.rds.route_config_0.update_success")->value()); + // FilesystemSubscriptionImpl will reject early at the ingestion level + EXPECT_EQ(1, test_server_->counter("http.router.rds.route_config_0.update_failure")->value()); + EXPECT_EQ(0, test_server_->counter("server.dynamic_unknown_fields")->value()); + } else { + EXPECT_EQ(1, test_server_->counter("http.router.rds.route_config_0.update_success")->value()); + EXPECT_EQ(1, test_server_->counter("server.dynamic_unknown_fields")->value()); + } + EXPECT_EQ(1, test_server_->counter("cluster_manager.cds.update_success")->value()); + EXPECT_EQ(1, test_server_->counter("cluster.cluster_1.update_success")->value()); +} + +// Unknown fields in EDS cause config load failure if and only if strict. +TEST_P(DynamicValidationIntegrationTest, EdsFailedBySubscription) { + api_filesystem_config_ = { + "test/config/integration/server_xds.bootstrap.yaml", + "test/config/integration/server_xds.cds.yaml", + "test/config/integration/server_xds.eds.with_unknown_field.yaml", + "test/config/integration/server_xds.lds.yaml", + "test/config/integration/server_xds.rds.yaml", + }; + initialize(); + EXPECT_EQ(1, test_server_->counter("listener_manager.lds.update_success")->value()); + EXPECT_EQ(1, test_server_->counter("http.router.rds.route_config_0.update_success")->value()); + EXPECT_EQ(1, test_server_->counter("cluster_manager.cds.update_success")->value()); + if (reject_unknown_dynamic_fields_) { + EXPECT_EQ(0, test_server_->counter("cluster.cluster_1.update_success")->value()); + // FilesystemSubscriptionImpl will reject early at the ingestion level + EXPECT_EQ(1, test_server_->counter("cluster.cluster_1.update_failure")->value()); + EXPECT_EQ(0, test_server_->counter("server.dynamic_unknown_fields")->value()); + } else { + EXPECT_EQ(1, test_server_->counter("cluster.cluster_1.update_success")->value()); + EXPECT_EQ(1, test_server_->counter("server.dynamic_unknown_fields")->value()); + } +} + +} // namespace +} // namespace Envoy diff --git a/test/integration/integration.cc b/test/integration/integration.cc index b7fd1d46696c..e0c124bd3c6b 100644 --- a/test/integration/integration.cc +++ b/test/integration/integration.cc @@ -354,7 +354,7 @@ void BaseIntegrationTest::createEnvoy() { for (int i = 0; i < static_resources.listeners_size(); ++i) { named_ports.push_back(static_resources.listeners(i).name()); } - createGeneratedApiTestServer(bootstrap_path, named_ports); + createGeneratedApiTestServer(bootstrap_path, named_ports, false, true, false); } void BaseIntegrationTest::setUpstreamProtocol(FakeHttpConnection::Type protocol) { @@ -416,10 +416,14 @@ void BaseIntegrationTest::registerTestServerPorts(const std::vector } void BaseIntegrationTest::createGeneratedApiTestServer(const std::string& bootstrap_path, - const std::vector& port_names) { - test_server_ = IntegrationTestServer::create(bootstrap_path, version_, on_server_init_function_, - deterministic_, timeSystem(), *api_, - defer_listener_finalization_, process_object_); + const std::vector& port_names, + bool allow_unknown_static_fields, + bool reject_unknown_dynamic_fields, + bool allow_lds_rejection) { + test_server_ = IntegrationTestServer::create( + bootstrap_path, version_, on_server_init_function_, deterministic_, timeSystem(), *api_, + defer_listener_finalization_, process_object_, allow_unknown_static_fields, + reject_unknown_dynamic_fields); if (config_helper_.bootstrap().static_resources().listeners_size() > 0 && !defer_listener_finalization_) { @@ -427,15 +431,19 @@ void BaseIntegrationTest::createGeneratedApiTestServer(const std::string& bootst // needs to know about the bound listener ports. auto end_time = time_system_.monotonicTime() + TestUtility::DefaultTimeout; const char* success = "listener_manager.listener_create_success"; - const char* failure = "listener_manager.lds.update_rejected"; - while (test_server_->counter(success) == nullptr || - test_server_->counter(success)->value() == 0) { + const char* rejected = "listener_manager.lds.update_rejected"; + while ((test_server_->counter(success) == nullptr || + test_server_->counter(success)->value() == 0) && + (!allow_lds_rejection || test_server_->counter(rejected) == nullptr || + test_server_->counter(rejected)->value() == 0)) { if (time_system_.monotonicTime() >= end_time) { RELEASE_ASSERT(0, "Timed out waiting for listeners."); } - RELEASE_ASSERT(test_server_->counter(failure) == nullptr || - test_server_->counter(failure)->value() == 0, - "Lds update failed"); + if (!allow_lds_rejection) { + RELEASE_ASSERT(test_server_->counter(rejected) == nullptr || + test_server_->counter(rejected)->value() == 0, + "Lds update failed"); + } time_system_.sleep(std::chrono::milliseconds(10)); } @@ -444,7 +452,10 @@ void BaseIntegrationTest::createGeneratedApiTestServer(const std::string& bootst } void BaseIntegrationTest::createApiTestServer(const ApiFilesystemConfig& api_filesystem_config, - const std::vector& port_names) { + const std::vector& port_names, + bool allow_unknown_static_fields, + bool reject_unknown_dynamic_fields, + bool allow_lds_rejection) { const std::string eds_path = TestEnvironment::temporaryFileSubstitute( api_filesystem_config.eds_path_, port_map_, version_); const std::string cds_path = TestEnvironment::temporaryFileSubstitute( @@ -453,11 +464,11 @@ void BaseIntegrationTest::createApiTestServer(const ApiFilesystemConfig& api_fil api_filesystem_config.rds_path_, port_map_, version_); const std::string lds_path = TestEnvironment::temporaryFileSubstitute( api_filesystem_config.lds_path_, {{"rds_json_path", rds_path}}, port_map_, version_); - createGeneratedApiTestServer(TestEnvironment::temporaryFileSubstitute( - api_filesystem_config.bootstrap_path_, - {{"cds_json_path", cds_path}, {"lds_json_path", lds_path}}, - port_map_, version_), - port_names); + createGeneratedApiTestServer( + TestEnvironment::temporaryFileSubstitute( + api_filesystem_config.bootstrap_path_, + {{"cds_json_path", cds_path}, {"lds_json_path", lds_path}}, port_map_, version_), + port_names, allow_unknown_static_fields, reject_unknown_dynamic_fields, allow_lds_rejection); } void BaseIntegrationTest::createTestServer(const std::string& json_path, diff --git a/test/integration/integration.h b/test/integration/integration.h index 4c09b7ecc6fa..db32e8a8cfc6 100644 --- a/test/integration/integration.h +++ b/test/integration/integration.h @@ -193,9 +193,13 @@ class BaseIntegrationTest : Logger::Loggable { void registerTestServerPorts(const std::vector& port_names); void createTestServer(const std::string& json_path, const std::vector& port_names); void createGeneratedApiTestServer(const std::string& bootstrap_path, - const std::vector& port_names); + const std::vector& port_names, + bool allow_unknown_static_fields, + bool reject_unknown_dynamic_fields, bool allow_lds_rejection); void createApiTestServer(const ApiFilesystemConfig& api_filesystem_config, - const std::vector& port_names); + const std::vector& port_names, + bool allow_unknown_static_fields, bool reject_unknown_dynamic_fields, + bool allow_lds_rejection); Event::TestTimeSystem& timeSystem() { return time_system_; } diff --git a/test/integration/server.cc b/test/integration/server.cc index 382332aa80c3..6161eeedf8b4 100644 --- a/test/integration/server.cc +++ b/test/integration/server.cc @@ -29,7 +29,9 @@ namespace Envoy { namespace Server { OptionsImpl createTestOptionsImpl(const std::string& config_path, const std::string& config_yaml, - Network::Address::IpVersion ip_version) { + Network::Address::IpVersion ip_version, + bool allow_unknown_static_fields, + bool reject_unknown_dynamic_fields) { OptionsImpl test_options("cluster_name", "node_name", "zone_name", spdlog::level::info); test_options.setConfigPath(config_path); @@ -38,6 +40,8 @@ OptionsImpl createTestOptionsImpl(const std::string& config_path, const std::str test_options.setFileFlushIntervalMsec(std::chrono::milliseconds(50)); test_options.setDrainTime(std::chrono::seconds(1)); test_options.setParentShutdownTime(std::chrono::seconds(2)); + test_options.setAllowUnkownFields(allow_unknown_static_fields); + test_options.setRejectUnknownFieldsDynamic(reject_unknown_dynamic_fields); return test_options; } @@ -48,11 +52,12 @@ IntegrationTestServerPtr IntegrationTestServer::create( const std::string& config_path, const Network::Address::IpVersion version, std::function on_server_init_function, bool deterministic, Event::TestTimeSystem& time_system, Api::Api& api, bool defer_listener_finalization, - absl::optional> process_object) { + absl::optional> process_object, + bool allow_unknown_static_fields, bool reject_unknown_dynamic_fields) { IntegrationTestServerPtr server{ std::make_unique(time_system, api, config_path)}; server->start(version, on_server_init_function, deterministic, defer_listener_finalization, - process_object); + process_object, allow_unknown_static_fields, reject_unknown_dynamic_fields); return server; } @@ -69,13 +74,16 @@ void IntegrationTestServer::waitUntilListenersReady() { void IntegrationTestServer::start( const Network::Address::IpVersion version, std::function on_server_init_function, bool deterministic, bool defer_listener_finalization, - absl::optional> process_object) { + absl::optional> process_object, + bool allow_unknown_static_fields, bool reject_unknown_dynamic_fields) { ENVOY_LOG(info, "starting integration test server"); ASSERT(!thread_); - thread_ = - api_.threadFactory().createThread([version, deterministic, process_object, this]() -> void { - threadRoutine(version, deterministic, process_object); - }); + thread_ = api_.threadFactory().createThread([version, deterministic, process_object, + allow_unknown_static_fields, + reject_unknown_dynamic_fields, this]() -> void { + threadRoutine(version, deterministic, process_object, allow_unknown_static_fields, + reject_unknown_dynamic_fields); + }); // If any steps need to be done prior to workers starting, do them now. E.g., xDS pre-init. // Note that there is no synchronization guaranteeing this happens either @@ -146,8 +154,10 @@ void IntegrationTestServer::serverReady() { void IntegrationTestServer::threadRoutine( const Network::Address::IpVersion version, bool deterministic, - absl::optional> process_object) { - OptionsImpl options(Server::createTestOptionsImpl(config_path_, "", version)); + absl::optional> process_object, + bool allow_unknown_static_fields, bool reject_unknown_dynamic_fields) { + OptionsImpl options(Server::createTestOptionsImpl( + config_path_, "", version, allow_unknown_static_fields, reject_unknown_dynamic_fields)); Thread::MutexBasicLockable lock; Runtime::RandomGeneratorPtr random_generator; diff --git a/test/integration/server.h b/test/integration/server.h index 55e3c11eab23..4489c436df19 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -32,7 +32,9 @@ namespace Server { // Create OptionsImpl structures suitable for tests. OptionsImpl createTestOptionsImpl(const std::string& config_path, const std::string& config_yaml, - Network::Address::IpVersion ip_version); + Network::Address::IpVersion ip_version, + bool allow_unknown_static_fields = false, + bool reject_unknown_dynamic_fields = false); class TestDrainManager : public DrainManager { public: @@ -233,7 +235,8 @@ class IntegrationTestServer : public Logger::Loggable, std::function on_server_init_function, bool deterministic, Event::TestTimeSystem& time_system, Api::Api& api, bool defer_listener_finalization = false, - absl::optional> process_object = absl::nullopt); + absl::optional> process_object = absl::nullopt, + bool allow_unknown_static_fields = false, bool reject_unknown_dynamic_fields = false); // Note that the derived class is responsible for tearing down the server in its // destructor. ~IntegrationTestServer() override; @@ -252,7 +255,8 @@ class IntegrationTestServer : public Logger::Loggable, void start(const Network::Address::IpVersion version, std::function on_server_init_function, bool deterministic, bool defer_listener_finalization, - absl::optional> process_object); + absl::optional> process_object, + bool allow_unknown_static_fields, bool reject_unknown_dynamic_fields); void waitForCounterEq(const std::string& name, uint64_t value) override { TestUtility::waitForCounterEq(stat_store(), name, value, time_system_); @@ -331,7 +335,8 @@ class IntegrationTestServer : public Logger::Loggable, * Runs the real server on a thread. */ void threadRoutine(const Network::Address::IpVersion version, bool deterministic, - absl::optional> process_object); + absl::optional> process_object, + bool allow_unknown_static_fields, bool reject_unknown_dynamic_fields); Event::TestTimeSystem& time_system_; Api::Api& api_; diff --git a/test/integration/xds_integration_test.cc b/test/integration/xds_integration_test.cc index 218ea948dc78..12e86df53292 100644 --- a/test/integration/xds_integration_test.cc +++ b/test/integration/xds_integration_test.cc @@ -28,7 +28,11 @@ class XdsIntegrationTest : public testing::TestWithParamcounter("listener_manager.lds.update_success")->value()); + EXPECT_EQ(1, test_server_->counter("http.router.rds.route_config_0.update_success")->value()); + EXPECT_EQ(1, test_server_->counter("cluster_manager.cds.update_success")->value()); + EXPECT_EQ(1, test_server_->counter("cluster.cluster_1.update_success")->value()); } }; diff --git a/test/mocks/protobuf/mocks.cc b/test/mocks/protobuf/mocks.cc index 908b08864fab..e425963e71dc 100644 --- a/test/mocks/protobuf/mocks.cc +++ b/test/mocks/protobuf/mocks.cc @@ -1,5 +1,7 @@ #include "test/mocks/protobuf/mocks.h" +using testing::ReturnRef; + namespace Envoy { namespace ProtobufMessage { @@ -7,5 +9,12 @@ MockValidationVisitor::MockValidationVisitor() = default; MockValidationVisitor::~MockValidationVisitor() = default; +MockValidationContext::MockValidationContext() { + ON_CALL(*this, staticValidationVisitor()).WillByDefault(ReturnRef(static_validation_visitor_)); + ON_CALL(*this, dynamicValidationVisitor()).WillByDefault(ReturnRef(dynamic_validation_visitor_)); +} + +MockValidationContext::~MockValidationContext() = default; + } // namespace ProtobufMessage } // namespace Envoy diff --git a/test/mocks/protobuf/mocks.h b/test/mocks/protobuf/mocks.h index 66e2f5d2b9c1..a099571d5e33 100644 --- a/test/mocks/protobuf/mocks.h +++ b/test/mocks/protobuf/mocks.h @@ -15,5 +15,17 @@ class MockValidationVisitor : public ValidationVisitor { MOCK_METHOD1(onUnknownField, void(absl::string_view)); }; +class MockValidationContext : public ValidationContext { +public: + MockValidationContext(); + ~MockValidationContext() override; + + MOCK_METHOD0(staticValidationVisitor, ValidationVisitor&()); + MOCK_METHOD0(dynamicValidationVisitor, ValidationVisitor&()); + + MockValidationVisitor static_validation_visitor_; + MockValidationVisitor dynamic_validation_visitor_; +}; + } // namespace ProtobufMessage } // namespace Envoy diff --git a/test/mocks/server/BUILD b/test/mocks/server/BUILD index 5d368f3c78ba..695f49be6c23 100644 --- a/test/mocks/server/BUILD +++ b/test/mocks/server/BUILD @@ -38,6 +38,7 @@ envoy_cc_mock( "//test/mocks/init:init_mocks", "//test/mocks/local_info:local_info_mocks", "//test/mocks/network:network_mocks", + "//test/mocks/protobuf:protobuf_mocks", "//test/mocks/router:router_mocks", "//test/mocks/runtime:runtime_mocks", "//test/mocks/secret:secret_mocks", diff --git a/test/mocks/server/mocks.cc b/test/mocks/server/mocks.cc index e1006497c8ba..9d2d49f52750 100644 --- a/test/mocks/server/mocks.cc +++ b/test/mocks/server/mocks.cc @@ -23,6 +23,12 @@ MockOptions::MockOptions(const std::string& config_path) : config_path_(config_p ON_CALL(*this, configPath()).WillByDefault(ReturnRef(config_path_)); ON_CALL(*this, configProto()).WillByDefault(ReturnRef(config_proto_)); ON_CALL(*this, configYaml()).WillByDefault(ReturnRef(config_yaml_)); + ON_CALL(*this, allowUnknownStaticFields()).WillByDefault(Invoke([this] { + return allow_unknown_static_fields_; + })); + ON_CALL(*this, rejectUnknownDynamicFields()).WillByDefault(Invoke([this] { + return reject_unknown_dynamic_fields_; + })); ON_CALL(*this, adminAddressPath()).WillByDefault(ReturnRef(admin_address_path_)); ON_CALL(*this, serviceClusterName()).WillByDefault(ReturnRef(service_cluster_name_)); ON_CALL(*this, serviceNodeName()).WillByDefault(ReturnRef(service_node_name_)); @@ -152,8 +158,7 @@ MockInstance::MockInstance() ON_CALL(*this, mutexTracer()).WillByDefault(Return(nullptr)); ON_CALL(*this, singletonManager()).WillByDefault(ReturnRef(*singleton_manager_)); ON_CALL(*this, overloadManager()).WillByDefault(ReturnRef(overload_manager_)); - ON_CALL(*this, messageValidationVisitor()) - .WillByDefault(ReturnRef(ProtobufMessage::getStrictValidationVisitor())); + ON_CALL(*this, messageValidationContext()).WillByDefault(ReturnRef(validation_context_)); } MockInstance::~MockInstance() = default; diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index 99fb3ebc1512..4ba0920f5695 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -7,6 +7,7 @@ #include "envoy/common/mutex_tracer.h" #include "envoy/config/bootstrap/v2/bootstrap.pb.h" +#include "envoy/protobuf/message_validator.h" #include "envoy/server/admin.h" #include "envoy/server/configuration.h" #include "envoy/server/drain_manager.h" @@ -35,6 +36,7 @@ #include "test/mocks/init/mocks.h" #include "test/mocks/local_info/mocks.h" #include "test/mocks/network/mocks.h" +#include "test/mocks/protobuf/mocks.h" #include "test/mocks/router/mocks.h" #include "test/mocks/runtime/mocks.h" #include "test/mocks/secret/mocks.h" @@ -62,7 +64,8 @@ class MockOptions : public Options { MOCK_CONST_METHOD0(configPath, const std::string&()); MOCK_CONST_METHOD0(configProto, const envoy::config::bootstrap::v2::Bootstrap&()); MOCK_CONST_METHOD0(configYaml, const std::string&()); - MOCK_CONST_METHOD0(allowUnknownFields, bool()); + MOCK_CONST_METHOD0(allowUnknownStaticFields, bool()); + MOCK_CONST_METHOD0(rejectUnknownDynamicFields, bool()); MOCK_CONST_METHOD0(adminAddressPath, const std::string&()); MOCK_CONST_METHOD0(localAddressIpVersion, Network::Address::IpVersion()); MOCK_CONST_METHOD0(drainTime, std::chrono::seconds()); @@ -88,6 +91,8 @@ class MockOptions : public Options { std::string config_path_; envoy::config::bootstrap::v2::Bootstrap config_proto_; std::string config_yaml_; + bool allow_unknown_static_fields_{}; + bool reject_unknown_dynamic_fields_{}; std::string admin_address_path_; std::string service_cluster_name_; std::string service_node_name_; @@ -380,7 +385,7 @@ class MockInstance : public Instance { MOCK_METHOD0(threadLocal, ThreadLocal::Instance&()); MOCK_METHOD0(localInfo, const LocalInfo::LocalInfo&()); MOCK_CONST_METHOD0(statsFlushInterval, std::chrono::milliseconds()); - MOCK_METHOD0(messageValidationVisitor, ProtobufMessage::ValidationVisitor&()); + MOCK_METHOD0(messageValidationContext, ProtobufMessage::ValidationContext&()); TimeSource& timeSource() override { return time_system_; } @@ -410,6 +415,7 @@ class MockInstance : public Instance { Singleton::ManagerPtr singleton_manager_; Grpc::ContextImpl grpc_context_; Http::ContextImpl http_context_; + testing::NiceMock validation_context_; }; namespace Configuration { diff --git a/test/server/BUILD b/test/server/BUILD index fcd3adfbd670..6f2893539e87 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -243,6 +243,11 @@ filegroup( srcs = glob(["test_data/runtime/**"]), ) +filegroup( + name = "static_validation_test_data", + srcs = glob(["test_data/static_validation/**"]), +) + envoy_cc_test( name = "server_test", srcs = ["server_test.cc"], @@ -261,6 +266,7 @@ envoy_cc_test( ":node_bootstrap_without_access_log.yaml", ":runtime_bootstrap.yaml", ":runtime_test_data", + ":static_validation_test_data", ":stats_sink_bootstrap.yaml", ":zipkin_tracing.yaml", ], diff --git a/test/server/config_validation/cluster_manager_test.cc b/test/server/config_validation/cluster_manager_test.cc index ce734f717301..0d3639d4860b 100644 --- a/test/server/config_validation/cluster_manager_test.cc +++ b/test/server/config_validation/cluster_manager_test.cc @@ -30,7 +30,7 @@ namespace { TEST(ValidationClusterManagerTest, MockedMethods) { Stats::IsolatedStoreImpl stats_store; Event::SimulatedTimeSystem time_system; - NiceMock validation_visitor; + NiceMock validation_context; Api::ApiPtr api(Api::createApiForTest(stats_store, time_system)); NiceMock runtime; NiceMock tls; @@ -47,7 +47,7 @@ TEST(ValidationClusterManagerTest, MockedMethods) { ValidationClusterManagerFactory factory(admin, runtime, stats_store, tls, random, dns_resolver, ssl_context_manager, dispatcher, local_info, - secret_manager, validation_visitor, *api, http_context, + secret_manager, validation_context, *api, http_context, log_manager, singleton_manager, time_system); const envoy::config::bootstrap::v2::Bootstrap bootstrap; diff --git a/test/server/configuration_impl_test.cc b/test/server/configuration_impl_test.cc index c2c4247b1f87..dbc13a45d189 100644 --- a/test/server/configuration_impl_test.cc +++ b/test/server/configuration_impl_test.cc @@ -59,7 +59,7 @@ class ConfigurationImplTest : public testing::Test { server_.threadLocal(), server_.random(), server_.dnsResolver(), server_.sslContextManager(), server_.dispatcher(), server_.localInfo(), server_.secretManager(), - server_.messageValidationVisitor(), *api_, server_.httpContext(), + server_.messageValidationContext(), *api_, server_.httpContext(), server_.accessLogManager(), server_.singletonManager()) {} Api::ApiPtr api_; diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index e3cdeffa44f1..768468f8c96e 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -37,6 +37,7 @@ #include "gtest/gtest.h" using testing::_; +using testing::AtLeast; using testing::InSequence; using testing::Invoke; using testing::NiceMock; @@ -77,8 +78,15 @@ class ListenerManagerImplTest : public testing::Test { * 4) Creates a mock local drain manager for the listener. */ ListenerHandle* expectListenerCreate( - bool need_init, + bool need_init, bool added_via_api, envoy::api::v2::Listener::DrainType drain_type = envoy::api::v2::Listener_DrainType_DEFAULT) { + if (added_via_api) { + EXPECT_CALL(server_.validation_context_, staticValidationVisitor()).Times(0); + EXPECT_CALL(server_.validation_context_, dynamicValidationVisitor()); + } else { + EXPECT_CALL(server_.validation_context_, staticValidationVisitor()); + EXPECT_CALL(server_.validation_context_, dynamicValidationVisitor()).Times(0); + } ListenerHandle* raw_listener = new ListenerHandle(); EXPECT_CALL(listener_factory_, createDrainManager_(drain_type)) .WillOnce(Return(raw_listener->drain_manager_)); @@ -608,7 +616,7 @@ TEST_F(ListenerManagerImplTest, ModifyOnlyDrainType) { )EOF"; ListenerHandle* listener_foo = - expectListenerCreate(false, envoy::api::v2::Listener_DrainType_MODIFY_ONLY); + expectListenerCreate(false, true, envoy::api::v2::Listener_DrainType_MODIFY_ONLY); EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, true)); EXPECT_TRUE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(listener_foo_yaml), "", true)); checkStats(1, 0, 0, 0, 1, 0); @@ -632,7 +640,7 @@ drain_type: default )EOF"; - ListenerHandle* listener_foo = expectListenerCreate(false); + ListenerHandle* listener_foo = expectListenerCreate(false, true); EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, true)); EXPECT_TRUE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(listener_foo_yaml), "", true)); checkStats(1, 0, 0, 0, 1, 0); @@ -650,7 +658,7 @@ drain_type: modify_only )EOF"; ListenerHandle* listener_foo_different_address = - expectListenerCreate(false, envoy::api::v2::Listener_DrainType_MODIFY_ONLY); + expectListenerCreate(false, true, envoy::api::v2::Listener_DrainType_MODIFY_ONLY); EXPECT_CALL(*listener_foo_different_address, onDestroy()); EXPECT_THROW_WITH_MESSAGE( manager_->addOrUpdateListener(parseListenerFromV2Yaml(listener_foo_different_address_yaml), @@ -682,7 +690,7 @@ name: foo drain_type: default )EOF"; - ListenerHandle* listener_foo = expectListenerCreate(false); + ListenerHandle* listener_foo = expectListenerCreate(false, true); ON_CALL(os_sys_calls, socket(AF_INET, _, 0)).WillByDefault(Return(Api::SysCallIntResult{5, 0})); ON_CALL(os_sys_calls, socket(AF_INET6, _, 0)).WillByDefault(Return(Api::SysCallIntResult{-1, 0})); @@ -714,7 +722,7 @@ name: foo drain_type: default )EOF"; - ListenerHandle* listener_foo = expectListenerCreate(false); + ListenerHandle* listener_foo = expectListenerCreate(false, true); ON_CALL(os_sys_calls, socket(AF_INET, _, 0)).WillByDefault(Return(Api::SysCallIntResult{-1, 0})); ON_CALL(os_sys_calls, socket(AF_INET6, _, 0)).WillByDefault(Return(Api::SysCallIntResult{5, 0})); @@ -726,7 +734,7 @@ drain_type: default EXPECT_CALL(*listener_foo, onDestroy()); } -// Make sure that a listener that is not modifiable cannot be updated or removed. +// Make sure that a listener that is not added_via_api cannot be updated or removed. TEST_F(ListenerManagerImplTest, UpdateRemoveNotModifiableListener) { time_system_.setSystemTime(std::chrono::milliseconds(1001001001001)); @@ -743,7 +751,7 @@ name: foo - filters: [] )EOF"; - ListenerHandle* listener_foo = expectListenerCreate(false); + ListenerHandle* listener_foo = expectListenerCreate(false, false); EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, true)); EXPECT_TRUE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(listener_foo_yaml), "", false)); checkStats(1, 0, 0, 0, 1, 0); @@ -816,7 +824,7 @@ name: "foo" filter_chains: {} )EOF"; - ListenerHandle* listener_foo = expectListenerCreate(false); + ListenerHandle* listener_foo = expectListenerCreate(false, true); EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, true)); EXPECT_TRUE( manager_->addOrUpdateListener(parseListenerFromV2Yaml(listener_foo_yaml), "version1", true)); @@ -858,7 +866,7 @@ per_connection_buffer_limit_bytes: 10 time_system_.setSystemTime(std::chrono::milliseconds(2002002002002)); - ListenerHandle* listener_foo_update1 = expectListenerCreate(false); + ListenerHandle* listener_foo_update1 = expectListenerCreate(false, true); EXPECT_CALL(*listener_foo, onDestroy()); EXPECT_TRUE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(listener_foo_update1_yaml), "version2", true)); @@ -899,7 +907,7 @@ version_info: version2 // Update foo. Should go into warming, have an immediate warming callback, and start immediate // removal. - ListenerHandle* listener_foo_update2 = expectListenerCreate(false); + ListenerHandle* listener_foo_update2 = expectListenerCreate(false, true); EXPECT_CALL(*worker_, addListener(_, _)); EXPECT_CALL(*worker_, stopListener(_)); EXPECT_CALL(*listener_foo_update1->drain_manager_, startDrainSequence(_)); @@ -958,7 +966,7 @@ name: "bar" filter_chains: {} )EOF"; - ListenerHandle* listener_bar = expectListenerCreate(false); + ListenerHandle* listener_bar = expectListenerCreate(false, true); EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, true)); EXPECT_CALL(*worker_, addListener(_, _)); EXPECT_TRUE( @@ -979,7 +987,7 @@ name: "baz" filter_chains: {} )EOF"; - ListenerHandle* listener_baz = expectListenerCreate(true); + ListenerHandle* listener_baz = expectListenerCreate(true, true); EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, true)); EXPECT_CALL(listener_baz->target_, initialize()); EXPECT_TRUE( @@ -1045,7 +1053,7 @@ name: baz config: {} )EOF"; - ListenerHandle* listener_baz_update1 = expectListenerCreate(true); + ListenerHandle* listener_baz_update1 = expectListenerCreate(true, true); EXPECT_CALL(*listener_baz, onDestroy()).WillOnce(Invoke([listener_baz]() -> void { // Call the initialize callback during destruction like RDS will. listener_baz->target_.ready(); @@ -1089,7 +1097,7 @@ name: foo new Network::Address::Ipv4Instance("127.0.0.1", 1234)); ON_CALL(*listener_factory_.socket_, localAddress()).WillByDefault(ReturnRef(local_address)); - ListenerHandle* listener_foo = expectListenerCreate(false); + ListenerHandle* listener_foo = expectListenerCreate(false, true); EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, true)); EXPECT_CALL(*worker_, addListener(_, _)); EXPECT_TRUE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(listener_foo_yaml), "", true)); @@ -1106,7 +1114,7 @@ name: foo checkStats(1, 0, 1, 0, 0, 1); // Add foo again. We should use the socket from draining. - ListenerHandle* listener_foo2 = expectListenerCreate(false); + ListenerHandle* listener_foo2 = expectListenerCreate(false, true); EXPECT_CALL(*worker_, addListener(_, _)); EXPECT_TRUE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(listener_foo_yaml), "", true)); worker_->callAddCompletion(true); @@ -1135,7 +1143,7 @@ name: foo - filters: [] )EOF"; - ListenerHandle* listener_foo = expectListenerCreate(true); + ListenerHandle* listener_foo = expectListenerCreate(true, true); EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, true)) .WillOnce(Throw(EnvoyException("can't bind"))); EXPECT_CALL(*listener_foo, onDestroy()); @@ -1159,7 +1167,7 @@ name: foo - filters: [] )EOF"; - ListenerHandle* listener_foo = expectListenerCreate(false); + ListenerHandle* listener_foo = expectListenerCreate(false, true); EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, true)); EXPECT_CALL(*worker_, addListener(_, _)); EXPECT_TRUE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(listener_foo_yaml), "", true)); @@ -1213,7 +1221,7 @@ name: foo - filters: [] )EOF"; - ListenerHandle* listener_foo = expectListenerCreate(true); + ListenerHandle* listener_foo = expectListenerCreate(true, true); EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, true)); EXPECT_CALL(listener_foo->target_, initialize()); EXPECT_TRUE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(listener_foo_yaml), "", true)); @@ -1227,7 +1235,7 @@ name: foo checkStats(1, 0, 1, 0, 0, 0); // Add foo again and initialize it. - listener_foo = expectListenerCreate(true); + listener_foo = expectListenerCreate(true, true); EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, true)); EXPECT_CALL(listener_foo->target_, initialize()); EXPECT_TRUE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(listener_foo_yaml), "", true)); @@ -1251,7 +1259,7 @@ name: foo config: {} )EOF"; - ListenerHandle* listener_foo_update1 = expectListenerCreate(true); + ListenerHandle* listener_foo_update1 = expectListenerCreate(true, true); EXPECT_CALL(listener_foo_update1->target_, initialize()); EXPECT_TRUE( manager_->addOrUpdateListener(parseListenerFromV2Yaml(listener_foo_update1_yaml), "", true)); @@ -1290,7 +1298,7 @@ name: foo - filters: [] )EOF"; - ListenerHandle* listener_foo = expectListenerCreate(false); + ListenerHandle* listener_foo = expectListenerCreate(false, true); EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, true)); EXPECT_CALL(*worker_, addListener(_, _)); EXPECT_TRUE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(listener_foo_yaml), "", true)); @@ -1343,7 +1351,7 @@ name: foo - filters: [] )EOF"; - ListenerHandle* listener_foo = expectListenerCreate(true); + ListenerHandle* listener_foo = expectListenerCreate(true, true); EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, false)); EXPECT_CALL(listener_foo->target_, initialize()); EXPECT_TRUE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(listener_foo_yaml), "", true)); @@ -1361,7 +1369,7 @@ name: bar - filters: [] )EOF"; - ListenerHandle* listener_bar = expectListenerCreate(true); + ListenerHandle* listener_bar = expectListenerCreate(true, true); EXPECT_CALL(*listener_bar, onDestroy()); EXPECT_THROW_WITH_MESSAGE( manager_->addOrUpdateListener(parseListenerFromV2Yaml(listener_bar_yaml), "", true), @@ -1373,7 +1381,7 @@ name: bar listener_foo->target_.ready(); worker_->callAddCompletion(true); - listener_bar = expectListenerCreate(true); + listener_bar = expectListenerCreate(true, true); EXPECT_CALL(*listener_bar, onDestroy()); EXPECT_THROW_WITH_MESSAGE( manager_->addOrUpdateListener(parseListenerFromV2Yaml(listener_bar_yaml), "", true), diff --git a/test/server/options_impl_test.cc b/test/server/options_impl_test.cc index 78c3a8373429..71662bb36bd6 100644 --- a/test/server/options_impl_test.cc +++ b/test/server/options_impl_test.cc @@ -76,7 +76,8 @@ TEST_F(OptionsImplTest, All) { "--service-cluster cluster --service-node node --service-zone zone " "--file-flush-interval-msec 9000 " "--drain-time-s 60 --log-format [%v] --parent-shutdown-time-s 90 --log-path /foo/bar " - "--disable-hot-restart --cpuset-threads"); + "--disable-hot-restart --cpuset-threads --allow-unknown-static-fields " + "--reject-unknown-dynamic-fields"); EXPECT_EQ(Server::Mode::Validate, options->mode()); EXPECT_EQ(2U, options->concurrency()); EXPECT_EQ("hello", options->configPath()); @@ -93,14 +94,36 @@ TEST_F(OptionsImplTest, All) { EXPECT_EQ(std::chrono::milliseconds(9000), options->fileFlushIntervalMsec()); EXPECT_EQ(std::chrono::seconds(60), options->drainTime()); EXPECT_EQ(std::chrono::seconds(90), options->parentShutdownTime()); - EXPECT_EQ(true, options->hotRestartDisabled()); - EXPECT_EQ(false, options->libeventBufferEnabled()); - EXPECT_EQ(true, options->cpusetThreadsEnabled()); + EXPECT_TRUE(options->hotRestartDisabled()); + EXPECT_FALSE(options->libeventBufferEnabled()); + EXPECT_TRUE(options->cpusetThreadsEnabled()); + EXPECT_TRUE(options->allowUnknownStaticFields()); + EXPECT_TRUE(options->rejectUnknownDynamicFields()); options = createOptionsImpl("envoy --mode init_only"); EXPECT_EQ(Server::Mode::InitOnly, options->mode()); } +// Either variants of allow-unknown-[static-]-fields works. +TEST_F(OptionsImplTest, AllowUnknownFields) { + { + std::unique_ptr options = createOptionsImpl("envoy"); + EXPECT_FALSE(options->allowUnknownStaticFields()); + } + { + std::unique_ptr options; + EXPECT_LOG_CONTAINS( + "warning", + "--allow-unknown-fields is deprecated, use --allow-unknown-static-fields instead.", + options = createOptionsImpl("envoy --allow-unknown-fields")); + EXPECT_TRUE(options->allowUnknownStaticFields()); + } + { + std::unique_ptr options = createOptionsImpl("envoy --allow-unknown-static-fields"); + EXPECT_TRUE(options->allowUnknownStaticFields()); + } +} + TEST_F(OptionsImplTest, SetAll) { std::unique_ptr options = createOptionsImpl("envoy -c hello"); bool hot_restart_disabled = options->hotRestartDisabled(); @@ -130,6 +153,8 @@ TEST_F(OptionsImplTest, SetAll) { options->setHotRestartDisabled(!options->hotRestartDisabled()); options->setSignalHandling(!options->signalHandlingEnabled()); options->setCpusetThreads(!options->cpusetThreadsEnabled()); + options->setAllowUnkownFields(true); + options->setRejectUnknownFieldsDynamic(true); EXPECT_EQ(109876, options->baseId()); EXPECT_EQ(42U, options->concurrency()); @@ -154,6 +179,8 @@ TEST_F(OptionsImplTest, SetAll) { EXPECT_EQ(!hot_restart_disabled, options->hotRestartDisabled()); EXPECT_EQ(!signal_handling_enabled, options->signalHandlingEnabled()); EXPECT_EQ(!cpuset_threads_enabled, options->cpusetThreadsEnabled()); + EXPECT_TRUE(options->allowUnknownStaticFields()); + EXPECT_TRUE(options->rejectUnknownDynamicFields()); // Validate that CommandLineOptions is constructed correctly. Server::CommandLineOptionsPtr command_line_options = options->toCommandLineOptions(); @@ -190,8 +217,8 @@ TEST_F(OptionsImplTest, DefaultParams) { EXPECT_EQ("", options->adminAddressPath()); EXPECT_EQ(Network::Address::IpVersion::v4, options->localAddressIpVersion()); EXPECT_EQ(Server::Mode::Serve, options->mode()); - EXPECT_EQ(false, options->hotRestartDisabled()); - EXPECT_EQ(false, options->cpusetThreadsEnabled()); + EXPECT_FALSE(options->hotRestartDisabled()); + EXPECT_FALSE(options->cpusetThreadsEnabled()); // Validate that CommandLineOptions is constructed correctly with default params. Server::CommandLineOptionsPtr command_line_options = options->toCommandLineOptions(); @@ -202,8 +229,10 @@ TEST_F(OptionsImplTest, DefaultParams) { EXPECT_EQ(envoy::admin::v2alpha::CommandLineOptions::v4, command_line_options->local_address_ip_version()); EXPECT_EQ(envoy::admin::v2alpha::CommandLineOptions::Serve, command_line_options->mode()); - EXPECT_EQ(false, command_line_options->disable_hot_restart()); - EXPECT_EQ(false, command_line_options->cpuset_threads()); + EXPECT_FALSE(command_line_options->disable_hot_restart()); + EXPECT_FALSE(command_line_options->cpuset_threads()); + EXPECT_FALSE(command_line_options->allow_unknown_static_fields()); + EXPECT_FALSE(command_line_options->reject_unknown_dynamic_fields()); } // Validates that the server_info proto is in sync with the options. @@ -212,12 +241,13 @@ TEST_F(OptionsImplTest, OptionsAreInSyncWithProto) { Server::CommandLineOptionsPtr command_line_options = options->toCommandLineOptions(); // Failure of this condition indicates that the server_info proto is not in sync with the options. // If an option is added/removed, please update server_info proto as well to keep it in sync. - // Currently the following 4 options are not defined in proto, hence the count differs by 5. + // Currently the following 5 options are not defined in proto, hence the count differs by 5. // 1. version - default TCLAP argument. // 2. help - default TCLAP argument. // 3. ignore_rest - default TCLAP argument. // 4. use-libevent-buffers - short-term override for rollout of new buffer implementation. - EXPECT_EQ(options->count() - 4, command_line_options->GetDescriptor()->field_count()); + // 5. allow-unknown-fields - deprecated alias of allow-unknown-static-fields. + EXPECT_EQ(options->count() - 5, command_line_options->GetDescriptor()->field_count()); } TEST_F(OptionsImplTest, BadCliOption) { diff --git a/test/server/server_test.cc b/test/server/server_test.cc index 2edb98e09540..ca2a755d26ac 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -165,10 +165,8 @@ class InitializingInstanceImpl : public InstanceImpl { }; // Class creates minimally viable server instance for testing. -class ServerInstanceImplTest : public testing::TestWithParam { +class ServerInstanceImplTestBase { protected: - ServerInstanceImplTest() : version_(GetParam()) {} - void initialize(const std::string& bootstrap_path) { initialize(bootstrap_path, false); } void initialize(const std::string& bootstrap_path, const bool use_intializing_instance) { @@ -258,6 +256,12 @@ class ServerInstanceImplTest : public testing::TestWithParam server_; }; +class ServerInstanceImplTest : public ServerInstanceImplTestBase, + public testing::TestWithParam { +protected: + ServerInstanceImplTest() { version_ = GetParam(); } +}; + // Custom StatsSink that just increments a counter when flush is called. class CustomStatsSink : public Stats::Sink { public: @@ -443,6 +447,66 @@ TEST_P(ServerInstanceImplTest, Stats) { #endif } +// Default validation mode +TEST_P(ServerInstanceImplTest, ValidationDefault) { + options_.service_cluster_name_ = "some_cluster_name"; + options_.service_node_name_ = "some_node_name"; + EXPECT_NO_THROW(initialize("test/server/empty_bootstrap.yaml")); + EXPECT_THAT_THROWS_MESSAGE( + server_->messageValidationContext().staticValidationVisitor().onUnknownField("foo"), + EnvoyException, "Protobuf message (foo) has unknown fields"); + EXPECT_EQ(0, TestUtility::findCounter(stats_store_, "server.static_unknown_fields")->value()); + EXPECT_NO_THROW( + server_->messageValidationContext().dynamicValidationVisitor().onUnknownField("bar")); + EXPECT_EQ(1, TestUtility::findCounter(stats_store_, "server.dynamic_unknown_fields")->value()); +} + +// Validation mode with --allow-unknown-static-fields +TEST_P(ServerInstanceImplTest, ValidationAllowStatic) { + options_.service_cluster_name_ = "some_cluster_name"; + options_.service_node_name_ = "some_node_name"; + options_.allow_unknown_static_fields_ = true; + EXPECT_NO_THROW(initialize("test/server/empty_bootstrap.yaml")); + EXPECT_NO_THROW( + server_->messageValidationContext().staticValidationVisitor().onUnknownField("foo")); + EXPECT_EQ(1, TestUtility::findCounter(stats_store_, "server.static_unknown_fields")->value()); + EXPECT_NO_THROW( + server_->messageValidationContext().dynamicValidationVisitor().onUnknownField("bar")); + EXPECT_EQ(1, TestUtility::findCounter(stats_store_, "server.dynamic_unknown_fields")->value()); +} + +// Validation mode with --reject-unknown-dynamic-fields +TEST_P(ServerInstanceImplTest, ValidationRejectDynamic) { + options_.service_cluster_name_ = "some_cluster_name"; + options_.service_node_name_ = "some_node_name"; + options_.reject_unknown_dynamic_fields_ = true; + EXPECT_NO_THROW(initialize("test/server/empty_bootstrap.yaml")); + EXPECT_THAT_THROWS_MESSAGE( + server_->messageValidationContext().staticValidationVisitor().onUnknownField("foo"), + EnvoyException, "Protobuf message (foo) has unknown fields"); + EXPECT_EQ(0, TestUtility::findCounter(stats_store_, "server.static_unknown_fields")->value()); + EXPECT_THAT_THROWS_MESSAGE( + server_->messageValidationContext().dynamicValidationVisitor().onUnknownField("bar"), + EnvoyException, "Protobuf message (bar) has unknown fields"); + EXPECT_EQ(0, TestUtility::findCounter(stats_store_, "server.dynamic_unknown_fields")->value()); +} + +// Validation mode with --allow-unknown-static-fields --reject-unknown-dynamic-fields +TEST_P(ServerInstanceImplTest, ValidationAllowStaticRejectDynamic) { + options_.service_cluster_name_ = "some_cluster_name"; + options_.service_node_name_ = "some_node_name"; + options_.allow_unknown_static_fields_ = true; + options_.reject_unknown_dynamic_fields_ = true; + EXPECT_NO_THROW(initialize("test/server/empty_bootstrap.yaml")); + EXPECT_NO_THROW( + server_->messageValidationContext().staticValidationVisitor().onUnknownField("foo")); + EXPECT_EQ(1, TestUtility::findCounter(stats_store_, "server.static_unknown_fields")->value()); + EXPECT_THAT_THROWS_MESSAGE( + server_->messageValidationContext().dynamicValidationVisitor().onUnknownField("bar"), + EnvoyException, "Protobuf message (bar) has unknown fields"); + EXPECT_EQ(0, TestUtility::findCounter(stats_store_, "server.dynamic_unknown_fields")->value()); +} + // Validate server localInfo() from bootstrap Node. TEST_P(ServerInstanceImplTest, BootstrapNode) { initialize("test/server/node_bootstrap.yaml"); @@ -781,6 +845,65 @@ TEST_P(ServerInstanceImplTest, WithProcessContext) { EXPECT_FALSE(object_from_context.boolean_flag_); } +// Static configuration validation. We test with both allow/reject settings various aspects of +// configuration from YAML. +class StaticValidationTest + : public ServerInstanceImplTestBase, + public testing::TestWithParam> { +protected: + StaticValidationTest() { + version_ = std::get<0>(GetParam()); + options_.service_cluster_name_ = "some_cluster_name"; + options_.service_node_name_ = "some_node_name"; + options_.allow_unknown_static_fields_ = std::get<1>(GetParam()); + // By inverting the static validation value, we can hopefully catch places we may have confused + // static/dynamic validation. + options_.reject_unknown_dynamic_fields_ = options_.allow_unknown_static_fields_; + } + + AssertionResult validate(absl::string_view yaml_filename) { + const std::string path = + absl::StrCat("test/server/test_data/static_validation/", yaml_filename); + try { + initialize(path); + } catch (EnvoyException&) { + return options_.allow_unknown_static_fields_ ? AssertionFailure() : AssertionSuccess(); + } + return options_.allow_unknown_static_fields_ ? AssertionSuccess() : AssertionFailure(); + } +}; + +std::string staticValidationTestParamsToString( + const ::testing::TestParamInfo>& params) { + return fmt::format( + "{}_{}", + TestUtility::ipTestParamsToString( + ::testing::TestParamInfo(std::get<0>(params.param), 0)), + std::get<1>(params.param) ? "with_allow_unknown_static_fields" + : "without_allow_unknown_static_fields"); +} + +INSTANTIATE_TEST_SUITE_P( + IpVersions, StaticValidationTest, + testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), testing::Bool()), + staticValidationTestParamsToString); + +TEST_P(StaticValidationTest, BootstrapUnknownField) { + EXPECT_TRUE(validate("bootstrap_unknown_field.yaml")); +} + +TEST_P(StaticValidationTest, ListenerUnknownField) { + EXPECT_TRUE(validate("listener_unknown_field.yaml")); +} + +TEST_P(StaticValidationTest, NetworkFilterUnknownField) { + EXPECT_TRUE(validate("network_filter_unknown_field.yaml")); +} + +TEST_P(StaticValidationTest, ClusterUnknownField) { + EXPECT_TRUE(validate("cluster_unknown_field.yaml")); +} + } // namespace } // namespace Server } // namespace Envoy diff --git a/test/server/test_data/static_validation/bootstrap_unknown_field.yaml b/test/server/test_data/static_validation/bootstrap_unknown_field.yaml new file mode 100644 index 000000000000..20e9ff3feaa8 --- /dev/null +++ b/test/server/test_data/static_validation/bootstrap_unknown_field.yaml @@ -0,0 +1 @@ +foo: bar diff --git a/test/server/test_data/static_validation/cluster_unknown_field.yaml b/test/server/test_data/static_validation/cluster_unknown_field.yaml new file mode 100644 index 000000000000..61675f55ee30 --- /dev/null +++ b/test/server/test_data/static_validation/cluster_unknown_field.yaml @@ -0,0 +1,5 @@ +static_resources: + clusters: + name: foo + connect_timeout: { seconds: 5 } + foo: bar diff --git a/test/server/test_data/static_validation/listener_unknown_field.yaml b/test/server/test_data/static_validation/listener_unknown_field.yaml new file mode 100644 index 000000000000..8dcf743e63ee --- /dev/null +++ b/test/server/test_data/static_validation/listener_unknown_field.yaml @@ -0,0 +1,10 @@ +static_resources: + listeners: + name: foo + address: + socket_address: + address: {{ ntop_ip_loopback_address }} + port_value: 0 + foo: bar + filter_chains: + - filters: diff --git a/test/server/test_data/static_validation/network_filter_unknown_field.yaml b/test/server/test_data/static_validation/network_filter_unknown_field.yaml new file mode 100644 index 000000000000..35450ef00657 --- /dev/null +++ b/test/server/test_data/static_validation/network_filter_unknown_field.yaml @@ -0,0 +1,15 @@ +static_resources: + listeners: + name: foo + address: + socket_address: + address: {{ ntop_ip_loopback_address }} + port_value: 0 + filter_chains: + - filters: + - name: envoy.http_connection_manager + config: + codec_type: HTTP2 + stat_prefix: blah + route_config: {} + foo: bar