pgv: validation support for Bootstrap/xDS/filters. (#2146)

This patch lands checking via https://github.com/lyft/protoc-gen-validate in Envoy for bootstrap, xDS ingestion and during filter instantiation. Risk Level: Medium (things might start blowing up on config ingestion that were slipping through before). Testing: Mostly negative tests for bootstrap/xDS/filters, since the happy path is already validated through the existing test suite. Signed-off-by: Harvey Tuch <[email protected]>
envoyproxy · Dec 4, 2017 · 9ed6292 · 9ed6292
1 parent 99baa4e
commit 9ed6292
Show file tree

Hide file tree

Showing 45 changed files with 414 additions and 130 deletions.
diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl
@@ -60,7 +60,7 @@ REPOSITORY_LOCATIONS = dict(
         urls = ["https://github.com/google/protobuf/archive/v3.5.0.tar.gz"],
     ),
     envoy_api = dict(
-        commit = "5055a888929d6aca545bff0159ab937ee40532f3",
+        commit = "ddf6b6206148fd2342172642cf56451316bc870d",
         remote = "https://github.com/envoyproxy/data-plane-api",
     ),
     grpc_httpjson_transcoding = dict(

diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc
@@ -14,6 +14,13 @@ MissingFieldException::MissingFieldException(const std::string& field_name,
     : EnvoyException(
           fmt::format("Field '{}' is missing in: {}", field_name, message.DebugString())) {}
 
+ProtoValidationException::ProtoValidationException(const std::string& validation_error,
+                                                   const Protobuf::Message& message)
+    : EnvoyException(fmt::format("Proto constraint validation failed ({}): {}", validation_error,
+                                 message.DebugString())) {
+  ENVOY_LOG_MISC(debug, "Proto validation error; throwing {}", what());
+}
+
 void MessageUtil::loadFromJson(const std::string& json, Protobuf::Message& message) {
   const auto status = Protobuf::util::JsonStringToMessage(json, &message);
   if (!status.ok()) {

diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h
@@ -10,6 +10,8 @@
 #include "common/json/json_loader.h"
 #include "common/protobuf/protobuf.h"
 
+#include "fmt/format.h"
+
 // Obtain the value of a wrapped field (e.g. google.protobuf.UInt32Value) if set. Otherwise, return
 // the default value.
 #define PROTOBUF_GET_WRAPPED_OR_DEFAULT(message, field_name, default_value)                        \
@@ -88,6 +90,11 @@ class RepeatedPtrUtil {
   }
 };
 
+class ProtoValidationException : public EnvoyException {
+public:
+  ProtoValidationException(const std::string& validation_error, const Protobuf::Message& message);
+};
+
 class MessageUtil {
 public:
   static std::size_t hash(const Protobuf::Message& message) {
@@ -109,6 +116,37 @@ class MessageUtil {
   static void loadFromYaml(const std::string& yaml, Protobuf::Message& message);
   static void loadFromFile(const std::string& path, Protobuf::Message& message);
 
+  /**
+   * Validate protoc-gen-validate constraints on a given protobuf.
+   * @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) {
+    std::string err;
+    if (!Validate(message, &err)) {
+      throw ProtoValidationException(err, message);
+    }
+  }
+
+  template <class MessageType>
+  static void loadFromFileAndValidate(const std::string& path, MessageType& message) {
+    loadFromFile(path, message);
+    validate(message);
+  }
+
+  /**
+   * Downcast and validate protoc-gen-validate constraints on a given protobuf.
+   * @param message const Protobuf::Message& to downcast and validate.
+   * @return const MessageType& the concrete message type downcasted to on success.
+   * @throw ProtoValidationException if the message does not satisfy its type constraints.
+   */
+  template <class MessageType>
+  static const MessageType& downcastAndValidate(const Protobuf::Message& config) {
+    const auto& typed_config = dynamic_cast<MessageType>(config);
+    validate(typed_config);
+    return typed_config;
+  }
+
   /**
    * Convert from google.protobuf.Any to a typed message.
    * @param message source google.protobuf.Any message.

diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc
@@ -9,9 +9,11 @@
 #include "common/config/rds_json.h"
 #include "common/config/subscription_factory.h"
 #include "common/config/utility.h"
+#include "common/protobuf/utility.h"
 #include "common/router/config_impl.h"
 #include "common/router/rds_subscription.h"
 
+#include "api/rds.pb.validate.h"
 #include "fmt/format.h"
 
 namespace Envoy {
@@ -104,6 +106,7 @@ void RdsRouteConfigProviderImpl::onConfigUpdate(const ResourceVector& resources)
     throw EnvoyException(fmt::format("Unexpected RDS resource length: {}", resources.size()));
   }
   const auto& route_config = resources[0];
+  MessageUtil::validate(route_config);
   // TODO(PiotrSikora): Remove this hack once fixed internally.
   if (!(route_config.name() == route_config_name_)) {
     throw EnvoyException(fmt::format("Unexpected RDS configuration (expecting {}): {}",

diff --git a/source/common/upstream/BUILD b/source/common/upstream/BUILD
@@ -23,6 +23,7 @@ envoy_cc_library(
         "//source/common/config:resources_lib",
         "//source/common/config:subscription_factory_lib",
         "//source/common/config:utility_lib",
+        "//source/common/protobuf:utility_lib",
     ],
 )
 
@@ -239,6 +240,7 @@ envoy_cc_library(
         "//source/common/network:address_lib",
         "//source/common/network:resolver_lib",
         "//source/common/network:utility_lib",
+        "//source/common/protobuf:utility_lib",
     ],
 )
 

diff --git a/source/common/upstream/cds_api_impl.cc b/source/common/upstream/cds_api_impl.cc
@@ -6,8 +6,11 @@
 #include "common/config/resources.h"
 #include "common/config/subscription_factory.h"
 #include "common/config/utility.h"
+#include "common/protobuf/utility.h"
 #include "common/upstream/cds_subscription.h"
 
+#include "api/cds.pb.validate.h"
+
 namespace Envoy {
 namespace Upstream {
 
@@ -41,6 +44,9 @@ CdsApiImpl::CdsApiImpl(const envoy::api::v2::ConfigSource& cds_config,
 void CdsApiImpl::onConfigUpdate(const ResourceVector& resources) {
   cm_.adsMux().pause(Config::TypeUrl::get().ClusterLoadAssignment);
   Cleanup eds_resume([this] { cm_.adsMux().resume(Config::TypeUrl::get().ClusterLoadAssignment); });
+  for (const auto& cluster : resources) {
+    MessageUtil::validate(cluster);
+  }
   // We need to keep track of which clusters we might need to remove.
   ClusterManager::ClusterInfoMap clusters_to_remove = cm_.clusters();
   for (auto& cluster : resources) {

diff --git a/source/common/upstream/cds_api_impl.h b/source/common/upstream/cds_api_impl.h
@@ -34,17 +34,17 @@ class CdsApiImpl : public CdsApi,
   }
   const std::string versionInfo() const override { return subscription_->versionInfo(); }
 
+  // Config::SubscriptionCallbacks
+  void onConfigUpdate(const ResourceVector& resources) override;
+  void onConfigUpdateFailed(const EnvoyException* e) override;
+
 private:
   CdsApiImpl(const envoy::api::v2::ConfigSource& cds_config,
              const Optional<envoy::api::v2::ConfigSource>& eds_config, ClusterManager& cm,
              Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random,
              const LocalInfo::LocalInfo& local_info, Stats::Scope& scope);
   void runInitializeCallbackIfAny();
 
-  // Config::SubscriptionCallbacks
-  void onConfigUpdate(const ResourceVector& resources) override;
-  void onConfigUpdateFailed(const EnvoyException* e) override;
-
   ClusterManager& cm_;
   std::unique_ptr<Config::Subscription<envoy::api::v2::Cluster>> subscription_;
   std::function<void()> initialize_callback_;

diff --git a/source/common/upstream/eds.cc b/source/common/upstream/eds.cc
@@ -9,8 +9,10 @@
 #include "common/network/address_impl.h"
 #include "common/network/resolver_impl.h"
 #include "common/network/utility.h"
+#include "common/protobuf/utility.h"
 #include "common/upstream/sds_subscription.h"
 
+#include "api/eds.pb.validate.h"
 #include "fmt/format.h"
 
 namespace Envoy {
@@ -54,6 +56,7 @@ void EdsClusterImpl::onConfigUpdate(const ResourceVector& resources) {
     throw EnvoyException(fmt::format("Unexpected EDS resource length: {}", resources.size()));
   }
   const auto& cluster_load_assignment = resources[0];
+  MessageUtil::validate(cluster_load_assignment);
   // TODO(PiotrSikora): Remove this hack once fixed internally.
   if (!(cluster_load_assignment.cluster_name() == cluster_name_)) {
     throw EnvoyException(fmt::format("Unexpected EDS cluster (expecting {}): {}", cluster_name_,

diff --git a/source/server/BUILD b/source/server/BUILD
@@ -167,6 +167,7 @@ envoy_cc_library(
         "//source/common/config:resources_lib",
         "//source/common/config:subscription_factory_lib",
         "//source/common/config:utility_lib",
+        "//source/common/protobuf:utility_lib",
     ],
 )
 

diff --git a/source/server/config/http/buffer.cc b/source/server/config/http/buffer.cc
@@ -10,6 +10,8 @@
 #include "common/http/filter/buffer_filter.h"
 #include "common/protobuf/utility.h"
 
+#include "api/filter/http/buffer.pb.validate.h"
+
 namespace Envoy {
 namespace Server {
 namespace Configuration {
@@ -42,8 +44,9 @@ HttpFilterFactoryCb
 BufferFilterConfig::createFilterFactoryFromProto(const Protobuf::Message& proto_config,
                                                  const std::string& stats_prefix,
                                                  FactoryContext& context) {
-  return createFilter(dynamic_cast<const envoy::api::v2::filter::http::Buffer&>(proto_config),
-                      stats_prefix, context);
+  return createFilter(
+      MessageUtil::downcastAndValidate<const envoy::api::v2::filter::http::Buffer&>(proto_config),
+      stats_prefix, context);
 }
 
 /**

diff --git a/source/server/config/http/fault.cc b/source/server/config/http/fault.cc
@@ -5,6 +5,8 @@
 #include "common/config/filter_json.h"
 #include "common/http/filter/fault_filter.h"
 
+#include "api/filter/http/fault.pb.validate.h"
+
 namespace Envoy {
 namespace Server {
 namespace Configuration {
@@ -32,8 +34,10 @@ HttpFilterFactoryCb
 FaultFilterConfig::createFilterFactoryFromProto(const Protobuf::Message& proto_config,
                                                 const std::string& stats_prefix,
                                                 FactoryContext& context) {
-  return createFilter(dynamic_cast<const envoy::api::v2::filter::http::HTTPFault&>(proto_config),
-                      stats_prefix, context);
+  return createFilter(
+      MessageUtil::downcastAndValidate<const envoy::api::v2::filter::http::HTTPFault&>(
+          proto_config),
+      stats_prefix, context);
 }
 
 /**

diff --git a/source/server/config/http/grpc_json_transcoder.cc b/source/server/config/http/grpc_json_transcoder.cc
@@ -6,6 +6,8 @@
 #include "common/grpc/json_transcoder_filter.h"
 #include "common/json/config_schemas.h"
 
+#include "api/filter/http/transcoder.pb.validate.h"
+
 namespace Envoy {
 namespace Server {
 namespace Configuration {
@@ -37,7 +39,8 @@ GrpcJsonTranscoderFilterConfig::createFilterFactoryFromProto(const Protobuf::Mes
                                                              const std::string& stat_prefix,
                                                              FactoryContext& context) {
   return createFilter(
-      dynamic_cast<const envoy::api::v2::filter::http::GrpcJsonTranscoder&>(proto_config),
+      MessageUtil::downcastAndValidate<const envoy::api::v2::filter::http::GrpcJsonTranscoder&>(
+          proto_config),
       stat_prefix, context);
 }
 

diff --git a/source/server/config/http/lua.cc b/source/server/config/http/lua.cc
@@ -5,6 +5,8 @@
 #include "common/config/filter_json.h"
 #include "common/http/filter/lua/lua_filter.h"
 
+#include "api/filter/http/lua.pb.validate.h"
+
 namespace Envoy {
 namespace Server {
 namespace Configuration {
@@ -31,8 +33,9 @@ HttpFilterFactoryCb
 LuaFilterConfig::createFilterFactoryFromProto(const Protobuf::Message& proto_config,
                                               const std::string& stat_prefix,
                                               FactoryContext& context) {
-  return createFilter(dynamic_cast<const envoy::api::v2::filter::http::Lua&>(proto_config),
-                      stat_prefix, context);
+  return createFilter(
+      MessageUtil::downcastAndValidate<const envoy::api::v2::filter::http::Lua&>(proto_config),
+      stat_prefix, context);
 }
 
 /**

diff --git a/source/server/config/http/ratelimit.cc b/source/server/config/http/ratelimit.cc
@@ -9,6 +9,8 @@
 #include "common/http/filter/ratelimit.h"
 #include "common/protobuf/utility.h"
 
+#include "api/filter/http/rate_limit.pb.validate.h"
+
 namespace Envoy {
 namespace Server {
 namespace Configuration {
@@ -40,8 +42,10 @@ HttpFilterFactoryCb
 RateLimitFilterConfig::createFilterFactoryFromProto(const Protobuf::Message& proto_config,
                                                     const std::string& stats_prefix,
                                                     FactoryContext& context) {
-  return createFilter(dynamic_cast<const envoy::api::v2::filter::http::RateLimit&>(proto_config),
-                      stats_prefix, context);
+  return createFilter(
+      MessageUtil::downcastAndValidate<const envoy::api::v2::filter::http::RateLimit&>(
+          proto_config),
+      stats_prefix, context);
 }
 
 /**

diff --git a/source/server/config/http/router.cc b/source/server/config/http/router.cc
@@ -9,6 +9,8 @@
 #include "common/router/router.h"
 #include "common/router/shadow_writer_impl.h"
 
+#include "api/filter/http/router.pb.validate.h"
+
 namespace Envoy {
 namespace Server {
 namespace Configuration {
@@ -38,8 +40,9 @@ HttpFilterFactoryCb
 RouterFilterConfig::createFilterFactoryFromProto(const Protobuf::Message& proto_config,
                                                  const std::string& stat_prefix,
                                                  FactoryContext& context) {
-  return createFilter(dynamic_cast<const envoy::api::v2::filter::http::Router&>(proto_config),
-                      stat_prefix, context);
+  return createFilter(
+      MessageUtil::downcastAndValidate<const envoy::api::v2::filter::http::Router&>(proto_config),
+      stat_prefix, context);
 }
 
 /**

diff --git a/source/server/config/network/client_ssl_auth.cc b/source/server/config/network/client_ssl_auth.cc
@@ -8,6 +8,8 @@
 #include "common/config/filter_json.h"
 #include "common/filter/auth/client_ssl.h"
 
+#include "api/filter/network/client_ssl_auth.pb.validate.h"
+
 namespace Envoy {
 namespace Server {
 namespace Configuration {
@@ -38,7 +40,9 @@ NetworkFilterFactoryCb
 ClientSslAuthConfigFactory::createFilterFactoryFromProto(const Protobuf::Message& proto_config,
                                                          FactoryContext& context) {
   return createFilter(
-      dynamic_cast<const envoy::api::v2::filter::network::ClientSSLAuth&>(proto_config), context);
+      MessageUtil::downcastAndValidate<const envoy::api::v2::filter::network::ClientSSLAuth&>(
+          proto_config),
+      context);
 }
 
 /**

diff --git a/source/server/config/network/file_access_log.cc b/source/server/config/network/file_access_log.cc
@@ -9,7 +9,7 @@
 #include "common/config/well_known_names.h"
 #include "common/protobuf/protobuf.h"
 
-#include "api/filter/network/http_connection_manager.pb.h"
+#include "api/filter/accesslog/accesslog.pb.validate.h"
 
 namespace Envoy {
 namespace Server {
@@ -18,7 +18,8 @@ namespace Configuration {
 AccessLog::InstanceSharedPtr FileAccessLogFactory::createAccessLogInstance(
     const Protobuf::Message& config, AccessLog::FilterPtr&& filter, FactoryContext& context) {
   const auto& fal_config =
-      dynamic_cast<const envoy::api::v2::filter::accesslog::FileAccessLog&>(config);
+      MessageUtil::downcastAndValidate<const envoy::api::v2::filter::accesslog::FileAccessLog&>(
+          config);
   AccessLog::FormatterPtr formatter;
   if (fal_config.format().empty()) {
     formatter = AccessLog::AccessLogFormatUtils::defaultAccessLogFormatter();

diff --git a/source/server/config/network/http_connection_manager.cc b/source/server/config/network/http_connection_manager.cc
@@ -23,6 +23,7 @@
 #include "common/protobuf/utility.h"
 #include "common/router/rds_impl.h"
 
+#include "api/filter/network/http_connection_manager.pb.validate.h"
 #include "fmt/format.h"
 
 namespace Envoy {
@@ -78,7 +79,8 @@ HttpConnectionManagerFilterConfigFactory::createFilterFactory(const Json::Object
 NetworkFilterFactoryCb HttpConnectionManagerFilterConfigFactory::createFilterFactoryFromProto(
     const Protobuf::Message& proto_config, FactoryContext& context) {
   return createFilter(
-      dynamic_cast<const envoy::api::v2::filter::network::HttpConnectionManager&>(proto_config),
+      MessageUtil::downcastAndValidate<
+          const envoy::api::v2::filter::network::HttpConnectionManager&>(proto_config),
       context);
 }
 

diff --git a/source/server/config/network/mongo_proxy.cc b/source/server/config/network/mongo_proxy.cc
@@ -8,6 +8,7 @@
 #include "common/config/filter_json.h"
 #include "common/mongo/proxy.h"
 
+#include "api/filter/network/mongo_proxy.pb.validate.h"
 #include "fmt/format.h"
 
 namespace Envoy {
@@ -52,7 +53,9 @@ NetworkFilterFactoryCb
 MongoProxyFilterConfigFactory::createFilterFactoryFromProto(const Protobuf::Message& proto_config,
                                                             FactoryContext& context) {
   return createFilter(
-      dynamic_cast<const envoy::api::v2::filter::network::MongoProxy&>(proto_config), context);
+      MessageUtil::downcastAndValidate<const envoy::api::v2::filter::network::MongoProxy&>(
+          proto_config),
+      context);
 }
 
 /**