protobuf: towards unifying PGV, deprecated and unknown field validation.

This is part of envoyproxy#7980; basically, we want to leverage the recursive pass that already exists for the deprecated check. This PR does not implement the recursive behavior yet for unknown fields though, because there is a ton of churn, so this PR just has the mechanical bits. We switch plumbing of validation visitor into places such as anyConvert() and instead pass this to MessageUtil::validate. There are a bunch of future followups planned in additional PRs: * Combine the recursive pass for unknown/deprecated check in MessageUtil::validate(). * Add mitigation for envoyproxy#5965 by copying to a temporary before recursive expansion. * [Future] consider moving deprecated reporting into a message validation visitor handler. Risk level: Low Testing: Some new //test/common/protobuf::utility_test unit test. Signed-off-by: Harvey Tuch <htuch@google.com>
htuch · Aug 22, 2019 · aa4b45c · aa4b45c
1 parent 719245f
commit aa4b45c
Show file tree

Hide file tree

Showing 85 changed files with 246 additions and 185 deletions.
diff --git a/include/envoy/server/BUILD b/include/envoy/server/BUILD
@@ -234,6 +234,7 @@ envoy_cc_library(
         ":resource_monitor_interface",
         "//include/envoy/api:api_interface",
         "//include/envoy/event:dispatcher_interface",
+        "//include/envoy/protobuf:message_validator_interface",
     ],
 )
 

diff --git a/include/envoy/server/filter_config.h b/include/envoy/server/filter_config.h
@@ -269,11 +269,14 @@ class ProtocolOptionsFactory {
    * implementation is unable to produce a factory with the provided parameters, it should throw an
    * EnvoyException.
    * @param config supplies the protobuf configuration for the filter
+   * @param validation_visitor message validation visitor instance.
    * @return Upstream::ProtocolOptionsConfigConstSharedPtr the protocol options
    */
   virtual Upstream::ProtocolOptionsConfigConstSharedPtr
-  createProtocolOptionsConfig(const Protobuf::Message& config) {
+  createProtocolOptionsConfig(const Protobuf::Message& config,
+                              ProtobufMessage::ValidationVisitor& validation_visitor) {
     UNREFERENCED_PARAMETER(config);
+    UNREFERENCED_PARAMETER(validation_visitor);
     return nullptr;
   }
 

diff --git a/include/envoy/server/resource_monitor_config.h b/include/envoy/server/resource_monitor_config.h
@@ -3,6 +3,7 @@
 #include "envoy/api/api.h"
 #include "envoy/common/pure.h"
 #include "envoy/event/dispatcher.h"
+#include "envoy/protobuf/message_validator.h"
 #include "envoy/server/resource_monitor.h"
 
 #include "common/protobuf/protobuf.h"
@@ -25,6 +26,12 @@ class ResourceMonitorFactoryContext {
    * @return reference to the Api object
    */
   virtual Api::Api& api() PURE;
+
+  /**
+   * @return ProtobufMessage::ValidationVisitor& validation visitor for filter configuration
+   *         messages.
+   */
+  virtual ProtobufMessage::ValidationVisitor& messageValidationVisitor() PURE;
 };
 
 /**

diff --git a/include/envoy/upstream/retry.h b/include/envoy/upstream/retry.h
@@ -79,8 +79,10 @@ class RetryPriorityFactory {
 public:
   virtual ~RetryPriorityFactory() = default;
 
-  virtual RetryPrioritySharedPtr createRetryPriority(const Protobuf::Message& config,
-                                                     uint32_t retry_count) PURE;
+  virtual RetryPrioritySharedPtr
+  createRetryPriority(const Protobuf::Message& config,
+                      ProtobufMessage::ValidationVisitor& validation_visitor,
+                      uint32_t retry_count) PURE;
 
   virtual std::string name() const PURE;
 

diff --git a/source/common/access_log/access_log_impl.cc b/source/common/access_log/access_log_impl.cc
@@ -53,7 +53,8 @@ bool ComparisonFilter::compareAgainstValue(uint64_t lhs) {
 
 FilterPtr
 FilterFactory::fromProto(const envoy::config::filter::accesslog::v2::AccessLogFilter& config,
-                         Runtime::Loader& runtime, Runtime::RandomGenerator& random) {
+                         Runtime::Loader& runtime, Runtime::RandomGenerator& random,
+                         ProtobufMessage::ValidationVisitor& validation_visitor) {
   switch (config.filter_specifier_case()) {
   case envoy::config::filter::accesslog::v2::AccessLogFilter::kStatusCodeFilter:
     return FilterPtr{new StatusCodeFilter(config.status_code_filter(), runtime)};
@@ -66,19 +67,19 @@ FilterFactory::fromProto(const envoy::config::filter::accesslog::v2::AccessLogFi
   case envoy::config::filter::accesslog::v2::AccessLogFilter::kRuntimeFilter:
     return FilterPtr{new RuntimeFilter(config.runtime_filter(), runtime, random)};
   case envoy::config::filter::accesslog::v2::AccessLogFilter::kAndFilter:
-    return FilterPtr{new AndFilter(config.and_filter(), runtime, random)};
+    return FilterPtr{new AndFilter(config.and_filter(), runtime, random, validation_visitor)};
   case envoy::config::filter::accesslog::v2::AccessLogFilter::kOrFilter:
-    return FilterPtr{new OrFilter(config.or_filter(), runtime, random)};
+    return FilterPtr{new OrFilter(config.or_filter(), runtime, random, validation_visitor)};
   case envoy::config::filter::accesslog::v2::AccessLogFilter::kHeaderFilter:
     return FilterPtr{new HeaderFilter(config.header_filter())};
   case envoy::config::filter::accesslog::v2::AccessLogFilter::kResponseFlagFilter:
-    MessageUtil::validate(config);
+    MessageUtil::validate(config, validation_visitor);
     return FilterPtr{new ResponseFlagFilter(config.response_flag_filter())};
   case envoy::config::filter::accesslog::v2::AccessLogFilter::kGrpcStatusFilter:
-    MessageUtil::validate(config);
+    MessageUtil::validate(config, validation_visitor);
     return FilterPtr{new GrpcStatusFilter(config.grpc_status_filter())};
   case envoy::config::filter::accesslog::v2::AccessLogFilter::kExtensionFilter:
-    MessageUtil::validate(config);
+    MessageUtil::validate(config, validation_visitor);
     {
       auto& factory = Config::Utility::getAndCheckFactory<ExtensionFilterFactory>(
           config.extension_filter().name());
@@ -140,19 +141,22 @@ bool RuntimeFilter::evaluate(const StreamInfo::StreamInfo&, const Http::HeaderMa
 
 OperatorFilter::OperatorFilter(const Protobuf::RepeatedPtrField<
                                    envoy::config::filter::accesslog::v2::AccessLogFilter>& configs,
-                               Runtime::Loader& runtime, Runtime::RandomGenerator& random) {
+                               Runtime::Loader& runtime, Runtime::RandomGenerator& random,
+                               ProtobufMessage::ValidationVisitor& validation_visitor) {
   for (const auto& config : configs) {
-    filters_.emplace_back(FilterFactory::fromProto(config, runtime, random));
+    filters_.emplace_back(FilterFactory::fromProto(config, runtime, random, validation_visitor));
   }
 }
 
 OrFilter::OrFilter(const envoy::config::filter::accesslog::v2::OrFilter& config,
-                   Runtime::Loader& runtime, Runtime::RandomGenerator& random)
-    : OperatorFilter(config.filters(), runtime, random) {}
+                   Runtime::Loader& runtime, Runtime::RandomGenerator& random,
+                   ProtobufMessage::ValidationVisitor& validation_visitor)
+    : OperatorFilter(config.filters(), runtime, random, validation_visitor) {}
 
 AndFilter::AndFilter(const envoy::config::filter::accesslog::v2::AndFilter& config,
-                     Runtime::Loader& runtime, Runtime::RandomGenerator& random)
-    : OperatorFilter(config.filters(), runtime, random) {}
+                     Runtime::Loader& runtime, Runtime::RandomGenerator& random,
+                     ProtobufMessage::ValidationVisitor& validation_visitor)
+    : OperatorFilter(config.filters(), runtime, random, validation_visitor) {}
 
 bool OrFilter::evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap& request_headers,
                         const Http::HeaderMap& response_headers,
@@ -268,7 +272,8 @@ AccessLogFactory::fromProto(const envoy::config::filter::accesslog::v2::AccessLo
                             Server::Configuration::FactoryContext& context) {
   FilterPtr filter;
   if (config.has_filter()) {
-    filter = FilterFactory::fromProto(config.filter(), context.runtime(), context.random());
+    filter = FilterFactory::fromProto(config.filter(), context.runtime(), context.random(),
+                                      context.messageValidationVisitor());
   }
 
   auto& factory =

diff --git a/source/common/access_log/access_log_impl.h b/source/common/access_log/access_log_impl.h
@@ -28,7 +28,8 @@ class FilterFactory {
    * Read a filter definition from proto and instantiate a concrete filter class.
    */
   static FilterPtr fromProto(const envoy::config::filter::accesslog::v2::AccessLogFilter& config,
-                             Runtime::Loader& runtime, Runtime::RandomGenerator& random);
+                             Runtime::Loader& runtime, Runtime::RandomGenerator& random,
+                             ProtobufMessage::ValidationVisitor& validation_visitor);
 };
 
 /**
@@ -82,7 +83,8 @@ class OperatorFilter : public Filter {
 public:
   OperatorFilter(const Protobuf::RepeatedPtrField<
                      envoy::config::filter::accesslog::v2::AccessLogFilter>& configs,
-                 Runtime::Loader& runtime, Runtime::RandomGenerator& random);
+                 Runtime::Loader& runtime, Runtime::RandomGenerator& random,
+                 ProtobufMessage::ValidationVisitor& validation_visitor);
 
 protected:
   std::vector<FilterPtr> filters_;
@@ -94,7 +96,8 @@ class OperatorFilter : public Filter {
 class AndFilter : public OperatorFilter {
 public:
   AndFilter(const envoy::config::filter::accesslog::v2::AndFilter& config, Runtime::Loader& runtime,
-            Runtime::RandomGenerator& random);
+            Runtime::RandomGenerator& random,
+            ProtobufMessage::ValidationVisitor& validation_visitor);
 
   // AccessLog::Filter
   bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap& request_headers,
@@ -108,7 +111,8 @@ class AndFilter : public OperatorFilter {
 class OrFilter : public OperatorFilter {
 public:
   OrFilter(const envoy::config::filter::accesslog::v2::OrFilter& config, Runtime::Loader& runtime,
-           Runtime::RandomGenerator& random);
+           Runtime::RandomGenerator& random,
+           ProtobufMessage::ValidationVisitor& validation_visitor);
 
   // AccessLog::Filter
   bool evaluate(const StreamInfo::StreamInfo& info, const Http::HeaderMap& request_headers,

diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h
@@ -235,9 +235,12 @@ class MessageUtil {
    * @param message message to validate.
    * @throw ProtoValidationException if the message does not satisfy its type constraints.
    */
-  template <class MessageType> static void validate(const MessageType& message) {
+  template <class MessageType>
+  static void validate(const MessageType& message,
+                       ProtobufMessage::ValidationVisitor& validation_visitor) {
     // Log warnings or throw errors if deprecated fields are in use.
     checkForDeprecation(message);
+    checkUnknownFields(message, validation_visitor);
 
     std::string err;
     if (!Validate(message, &err)) {
@@ -249,14 +252,14 @@ class MessageUtil {
   static void loadFromFileAndValidate(const std::string& path, MessageType& message,
                                       ProtobufMessage::ValidationVisitor& validation_visitor) {
     loadFromFile(path, message, validation_visitor);
-    validate(message);
+    validate(message, validation_visitor);
   }
 
   template <class MessageType>
   static void loadFromYamlAndValidate(const std::string& yaml, MessageType& message,
                                       ProtobufMessage::ValidationVisitor& validation_visitor) {
     loadFromYaml(yaml, message, validation_visitor);
-    validate(message);
+    validate(message, validation_visitor);
   }
 
   /**
@@ -268,9 +271,11 @@ class MessageUtil {
    * @throw ProtoValidationException if the message does not satisfy its type constraints.
    */
   template <class MessageType>
-  static const MessageType& downcastAndValidate(const Protobuf::Message& config) {
+  static const MessageType&
+  downcastAndValidate(const Protobuf::Message& config,
+                      ProtobufMessage::ValidationVisitor& validation_visitor) {
     const auto& typed_config = dynamic_cast<MessageType>(config);
-    validate(typed_config);
+    validate(typed_config, validation_visitor);
     return typed_config;
   }
 
@@ -282,13 +287,11 @@ class MessageUtil {
    * @return MessageType the typed message inside the Any.
    */
   template <class MessageType>
-  static inline MessageType anyConvert(const ProtobufWkt::Any& message,
-                                       ProtobufMessage::ValidationVisitor& validation_visitor) {
+  static inline MessageType anyConvert(const ProtobufWkt::Any& message) {
     MessageType typed_message;
     if (!message.UnpackTo(&typed_message)) {
       throw EnvoyException("Unable to unpack " + message.DebugString());
     }
-    checkUnknownFields(typed_message, validation_visitor);
     return typed_message;
   };
 

diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc
@@ -66,7 +66,8 @@ HedgePolicyImpl::HedgePolicyImpl()
     : initial_requests_(1), additional_request_chance_({}), hedge_on_per_try_timeout_(false) {}
 
 RetryPolicyImpl::RetryPolicyImpl(const envoy::api::v2::route::RetryPolicy& retry_policy,
-                                 ProtobufMessage::ValidationVisitor& validation_visitor) {
+                                 ProtobufMessage::ValidationVisitor& validation_visitor)
+    : validation_visitor_(&validation_visitor) {
   per_try_timeout_ =
       std::chrono::milliseconds(PROTOBUF_GET_MS_OR_DEFAULT(retry_policy, per_try_timeout, 0));
   num_retries_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(retry_policy, num_retries, 1);
@@ -141,7 +142,8 @@ Upstream::RetryPrioritySharedPtr RetryPolicyImpl::retryPriority() const {
   auto& factory = Envoy::Config::Utility::getAndCheckFactory<Upstream::RetryPriorityFactory>(
       retry_priority_config_.first);
 
-  return factory.createRetryPriority(*retry_priority_config_.second, num_retries_);
+  return factory.createRetryPriority(*retry_priority_config_.second, *validation_visitor_,
+                                     num_retries_);
 }
 
 CorsPolicyImpl::CorsPolicyImpl(const envoy::api::v2::route::CorsPolicy& config,

diff --git a/source/common/router/config_impl.h b/source/common/router/config_impl.h
@@ -258,6 +258,7 @@ class RetryPolicyImpl : public RetryPolicy {
   std::vector<uint32_t> retriable_status_codes_;
   absl::optional<std::chrono::milliseconds> base_interval_;
   absl::optional<std::chrono::milliseconds> max_interval_;
+  ProtobufMessage::ValidationVisitor* validation_visitor_{};
 };
 
 /**

diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc
@@ -92,9 +92,8 @@ void RdsRouteConfigSubscription::onConfigUpdate(
   if (!validateUpdateSize(resources.size())) {
     return;
   }
-  auto route_config = MessageUtil::anyConvert<envoy::api::v2::RouteConfiguration>(
-      resources[0], validation_visitor_);
-  MessageUtil::validate(route_config);
+  auto route_config = MessageUtil::anyConvert<envoy::api::v2::RouteConfiguration>(resources[0]);
+  MessageUtil::validate(route_config, validation_visitor_);
   if (route_config.name() != route_config_name_) {
     throw EnvoyException(fmt::format("Unexpected RDS configuration (expecting {}): {}",
                                      route_config_name_, route_config.name()));

diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h
@@ -118,9 +118,7 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks,
   void onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason,
                             const EnvoyException* e) override;
   std::string resourceName(const ProtobufWkt::Any& resource) override {
-    return MessageUtil::anyConvert<envoy::api::v2::RouteConfiguration>(resource,
-                                                                       validation_visitor_)
-        .name();
+    return MessageUtil::anyConvert<envoy::api::v2::RouteConfiguration>(resource).name();
   }
 
   RdsRouteConfigSubscription(

diff --git a/source/common/router/route_config_update_receiver_impl.cc b/source/common/router/route_config_update_receiver_impl.cc
@@ -60,9 +60,8 @@ void RouteConfigUpdateReceiverImpl::updateVhosts(
     const Protobuf::RepeatedPtrField<envoy::api::v2::Resource>& added_resources) {
   for (const auto& resource : added_resources) {
     envoy::api::v2::route::VirtualHost vhost =
-        MessageUtil::anyConvert<envoy::api::v2::route::VirtualHost>(resource.resource(),
-                                                                    validation_visitor_);
-    MessageUtil::validate(vhost);
+        MessageUtil::anyConvert<envoy::api::v2::route::VirtualHost>(resource.resource());
+    MessageUtil::validate(vhost, validation_visitor_);
     auto found = vhosts.find(vhost.name());
     if (found != vhosts.end()) {
       vhosts.erase(found);

diff --git a/source/common/router/scoped_rds.cc b/source/common/router/scoped_rds.cc
@@ -105,8 +105,8 @@ void ScopedRdsConfigSubscription::onConfigUpdate(
     const std::string& version_info) {
   std::vector<envoy::api::v2::ScopedRouteConfiguration> scoped_routes;
   for (const auto& resource_any : resources) {
-    scoped_routes.emplace_back(MessageUtil::anyConvert<envoy::api::v2::ScopedRouteConfiguration>(
-        resource_any, validation_visitor_));
+    scoped_routes.emplace_back(
+        MessageUtil::anyConvert<envoy::api::v2::ScopedRouteConfiguration>(resource_any));
   }
 
   std::unordered_set<std::string> resource_names;
@@ -117,7 +117,7 @@ void ScopedRdsConfigSubscription::onConfigUpdate(
     }
   }
   for (const auto& scoped_route : scoped_routes) {
-    MessageUtil::validate(scoped_route);
+    MessageUtil::validate(scoped_route, validation_visitor_);
   }
 
   // TODO(AndresGuedez): refactor such that it can be shared with other delta APIs (e.g., CDS).

diff --git a/source/common/router/scoped_rds.h b/source/common/router/scoped_rds.h
@@ -115,9 +115,7 @@ class ScopedRdsConfigSubscription : public Envoy::Config::DeltaConfigSubscriptio
     ConfigSubscriptionCommonBase::onConfigUpdateFailed();
   }
   std::string resourceName(const ProtobufWkt::Any& resource) override {
-    return MessageUtil::anyConvert<envoy::api::v2::ScopedRouteConfiguration>(resource,
-                                                                             validation_visitor_)
-        .name();
+    return MessageUtil::anyConvert<envoy::api::v2::ScopedRouteConfiguration>(resource).name();
   }
 
   const std::string name_;

diff --git a/source/common/router/vhds.cc b/source/common/router/vhds.cc
@@ -28,8 +28,7 @@ VhdsSubscription::VhdsSubscription(RouteConfigUpdatePtr& config_update_info,
       scope_(factory_context.scope().createScope(stat_prefix + "vhds." +
                                                  config_update_info_->routeConfigName() + ".")),
       stats_({ALL_VHDS_STATS(POOL_COUNTER(*scope_))}),
-      route_config_providers_(route_config_providers),
-      validation_visitor_(factory_context.messageValidationVisitor()) {
+      route_config_providers_(route_config_providers) {
   const auto& config_source = config_update_info_->routeConfiguration()
                                   .vhds()
                                   .config_source()

diff --git a/source/common/router/vhds.h b/source/common/router/vhds.h
@@ -59,9 +59,7 @@ class VhdsSubscription : Envoy::Config::SubscriptionCallbacks,
   void onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason,
                             const EnvoyException* e) override;
   std::string resourceName(const ProtobufWkt::Any& resource) override {
-    return MessageUtil::anyConvert<envoy::api::v2::route::VirtualHost>(resource,
-                                                                       validation_visitor_)
-        .name();
+    return MessageUtil::anyConvert<envoy::api::v2::route::VirtualHost>(resource).name();
   }
 
   RouteConfigUpdatePtr& config_update_info_;
@@ -70,7 +68,6 @@ class VhdsSubscription : Envoy::Config::SubscriptionCallbacks,
   Stats::ScopePtr scope_;
   VhdsStats stats_;
   std::unordered_set<RouteConfigProvider*>& route_config_providers_;
-  ProtobufMessage::ValidationVisitor& validation_visitor_;
 };
 
 using VhdsSubscriptionPtr = std::unique_ptr<VhdsSubscription>;

diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc
@@ -517,9 +517,8 @@ RtdsSubscription::RtdsSubscription(
 void RtdsSubscription::onConfigUpdate(const Protobuf::RepeatedPtrField<ProtobufWkt::Any>& resources,
                                       const std::string&) {
   validateUpdateSize(resources.size());
-  auto runtime = MessageUtil::anyConvert<envoy::service::discovery::v2::Runtime>(
-      resources[0], validation_visitor_);
-  MessageUtil::validate(runtime);
+  auto runtime = MessageUtil::anyConvert<envoy::service::discovery::v2::Runtime>(resources[0]);
+  MessageUtil::validate(runtime, validation_visitor_);
   if (runtime.name() != resource_name_) {
     throw EnvoyException(
         fmt::format("Unexpected RTDS runtime (expecting {}): {}", resource_name_, runtime.name()));

diff --git a/source/common/runtime/runtime_impl.h b/source/common/runtime/runtime_impl.h
@@ -209,9 +209,7 @@ struct RtdsSubscription : Config::SubscriptionCallbacks, Logger::Loggable<Logger
   void onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason,
                             const EnvoyException* e) override;
   std::string resourceName(const ProtobufWkt::Any& resource) override {
-    return MessageUtil::anyConvert<envoy::service::discovery::v2::Runtime>(resource,
-                                                                           validation_visitor_)
-        .name();
+    return MessageUtil::anyConvert<envoy::service::discovery::v2::Runtime>(resource).name();
   }
 
   void start();

diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc
@@ -30,9 +30,8 @@ SdsApi::SdsApi(envoy::api::v2::core::ConfigSource sds_config, absl::string_view
 void SdsApi::onConfigUpdate(const Protobuf::RepeatedPtrField<ProtobufWkt::Any>& resources,
                             const std::string& version_info) {
   validateUpdateSize(resources.size());
-  auto secret =
-      MessageUtil::anyConvert<envoy::api::v2::auth::Secret>(resources[0], validation_visitor_);
-  MessageUtil::validate(secret);
+  auto secret = MessageUtil::anyConvert<envoy::api::v2::auth::Secret>(resources[0]);
+  MessageUtil::validate(secret, validation_visitor_);
 
   if (secret.name() != sds_config_name_) {
     throw EnvoyException(