proto: use anyConvertAndValidate where applicable (#9726)

Signed-off-by: Jose Nino <jnino@lyft.com>
envoyproxy · Jan 19, 2020 · 334724c · 334724c
1 parent 9745341
commit 334724c
Show file tree

Hide file tree

Showing 9 changed files with 32 additions and 20 deletions.
diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc
@@ -102,8 +102,8 @@ void RdsRouteConfigSubscription::onConfigUpdate(
     return;
   }
   auto route_config =
-      MessageUtil::anyConvert<envoy::config::route::v3::RouteConfiguration>(resources[0]);
-  MessageUtil::validate(route_config, validator_);
+      MessageUtil::anyConvertAndValidate<envoy::config::route::v3::RouteConfiguration>(resources[0],
+                                                                                       validator_);
   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/route_config_update_receiver_impl.cc b/source/common/router/route_config_update_receiver_impl.cc
@@ -93,8 +93,8 @@ bool RouteConfigUpdateReceiverImpl::updateVhosts(
       continue;
     }
     envoy::config::route::v3::VirtualHost vhost =
-        MessageUtil::anyConvert<envoy::config::route::v3::VirtualHost>(resource.resource());
-    MessageUtil::validate(vhost, validation_visitor_);
+        MessageUtil::anyConvertAndValidate<envoy::config::route::v3::VirtualHost>(
+            resource.resource(), 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
@@ -142,9 +142,8 @@ bool ScopedRdsConfigSubscription::addOrUpdateScopes(
     envoy::config::route::v3::ScopedRouteConfiguration scoped_route_config;
     try {
       scoped_route_config =
-          MessageUtil::anyConvert<envoy::config::route::v3::ScopedRouteConfiguration>(
-              resource.resource());
-      MessageUtil::validate(scoped_route_config, validation_visitor_);
+          MessageUtil::anyConvertAndValidate<envoy::config::route::v3::ScopedRouteConfiguration>(
+              resource.resource(), validation_visitor_);
       const std::string scope_name = scoped_route_config.name();
       if (!unique_resource_names.insert(scope_name).second) {
         throw EnvoyException(
@@ -313,8 +312,8 @@ void ScopedRdsConfigSubscription::onConfigUpdate(
   for (const auto& resource_any : resources) {
     // Throws (thus rejects all) on any error.
     auto scoped_route =
-        MessageUtil::anyConvert<envoy::config::route::v3::ScopedRouteConfiguration>(resource_any);
-    MessageUtil::validate(scoped_route, validation_visitor_);
+        MessageUtil::anyConvertAndValidate<envoy::config::route::v3::ScopedRouteConfiguration>(
+            resource_any, validation_visitor_);
     const std::string scope_name = scoped_route.name();
     auto scope_config_inserted = scoped_routes.try_emplace(scope_name, std::move(scoped_route));
     if (!scope_config_inserted.second) {

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

diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc
@@ -37,8 +37,8 @@ void SdsApi::onConfigUpdate(const Protobuf::RepeatedPtrField<ProtobufWkt::Any>&
                             const std::string& version_info) {
   validateUpdateSize(resources.size());
   auto secret =
-      MessageUtil::anyConvert<envoy::extensions::transport_sockets::tls::v3::Secret>(resources[0]);
-  MessageUtil::validate(secret, validation_visitor_);
+      MessageUtil::anyConvertAndValidate<envoy::extensions::transport_sockets::tls::v3::Secret>(
+          resources[0], validation_visitor_);
 
   if (secret.name() != sds_config_name_) {
     throw EnvoyException(

diff --git a/source/common/upstream/cds_api_impl.cc b/source/common/upstream/cds_api_impl.cc
@@ -41,6 +41,7 @@ void CdsApiImpl::onConfigUpdate(const Protobuf::RepeatedPtrField<ProtobufWkt::An
   ClusterManager::ClusterInfoMap clusters_to_remove = cm_.clusters();
   std::vector<envoy::config::cluster::v3::Cluster> clusters;
   for (const auto& cluster_blob : resources) {
+    // No validation needed here the overloaded call to onConfigUpdate validates.
     clusters.push_back(MessageUtil::anyConvert<envoy::config::cluster::v3::Cluster>(cluster_blob));
     clusters_to_remove.erase(clusters.back().name());
   }
@@ -78,8 +79,8 @@ void CdsApiImpl::onConfigUpdate(
   for (const auto& resource : added_resources) {
     envoy::config::cluster::v3::Cluster cluster;
     try {
-      cluster = MessageUtil::anyConvert<envoy::config::cluster::v3::Cluster>(resource.resource());
-      MessageUtil::validate(cluster, validation_visitor_);
+      cluster = MessageUtil::anyConvertAndValidate<envoy::config::cluster::v3::Cluster>(
+          resource.resource(), validation_visitor_);
       if (!cluster_names.insert(cluster.name()).second) {
         // NOTE: at this point, the first of these duplicates has already been successfully applied.
         throw EnvoyException(fmt::format("duplicate cluster {} found", cluster.name()));

diff --git a/source/common/upstream/eds.cc b/source/common/upstream/eds.cc
@@ -115,8 +115,8 @@ void EdsClusterImpl::onConfigUpdate(const Protobuf::RepeatedPtrField<ProtobufWkt
     return;
   }
   auto cluster_load_assignment =
-      MessageUtil::anyConvert<envoy::config::endpoint::v3::ClusterLoadAssignment>(resources[0]);
-  MessageUtil::validate(cluster_load_assignment, validation_visitor_);
+      MessageUtil::anyConvertAndValidate<envoy::config::endpoint::v3::ClusterLoadAssignment>(
+          resources[0], validation_visitor_);
   if (cluster_load_assignment.cluster_name() != cluster_name_) {
     throw EnvoyException(fmt::format("Unexpected EDS cluster (expecting {}): {}", cluster_name_,
                                      cluster_load_assignment.cluster_name()));

diff --git a/source/server/lds_api.cc b/source/server/lds_api.cc
@@ -63,9 +63,8 @@ void LdsApiImpl::onConfigUpdate(
   for (const auto& resource : added_resources) {
     envoy::config::listener::v3::Listener listener;
     try {
-      listener =
-          MessageUtil::anyConvert<envoy::config::listener::v3::Listener>(resource.resource());
-      MessageUtil::validate(listener, validation_visitor_);
+      listener = MessageUtil::anyConvertAndValidate<envoy::config::listener::v3::Listener>(
+          resource.resource(), validation_visitor_);
       if (!listener_names.insert(listener.name()).second) {
         // NOTE: at this point, the first of these duplicates has already been successfully applied.
         throw EnvoyException(fmt::format("duplicate listener {} found", listener.name()));
@@ -108,6 +107,7 @@ void LdsApiImpl::onConfigUpdate(const Protobuf::RepeatedPtrField<ProtobufWkt::An
   for (const auto& listener_blob : resources) {
     // Add this resource to our delta added/updated pile...
     envoy::service::discovery::v3::Resource* to_add = to_add_repeated.Add();
+    // No validation needed here the overloaded call to onConfigUpdate validates.
     const std::string listener_name =
         MessageUtil::anyConvert<envoy::config::listener::v3::Listener>(listener_blob).name();
     to_add->set_name(listener_name);

diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc
@@ -6,6 +6,8 @@
 #include "envoy/config/bootstrap/v3/bootstrap.pb.validate.h"
 #include "envoy/config/cluster/v3/cluster.pb.h"
 #include "envoy/config/cluster/v3/cluster.pb.validate.h"
+#include "envoy/config/cluster/v3/filter.pb.h"
+#include "envoy/config/cluster/v3/filter.pb.validate.h"
 #include "envoy/type/v3/percent.pb.h"
 
 #include "common/common/base64.h"
@@ -1107,6 +1109,16 @@ TEST_F(ProtobufUtilityTest, AnyConvertWrongType) {
       R"(Unable to unpack as google.protobuf.Timestamp: \[type.googleapis.com/google.protobuf.Duration\] .*)");
 }
 
+// Validated exception thrown when anyConvertAndValidate observes a PGV failures.
+TEST_F(ProtobufUtilityTest, AnyConvertAndValidateFailedValidation) {
+  envoy::config::cluster::v3::Filter filter;
+  ProtobufWkt::Any source_any;
+  source_any.PackFrom(filter);
+  EXPECT_THROW(MessageUtil::anyConvertAndValidate<envoy::config::cluster::v3::Filter>(
+                   source_any, ProtobufMessage::getStrictValidationVisitor()),
+               ProtoValidationException);
+}
+
 // MessageUtility::unpackTo() with the wrong type throws.
 TEST_F(ProtobufUtilityTest, UnpackToWrongType) {
   ProtobufWkt::Duration source_duration;