Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proto: use anyConvertAndValidate where applicable #9726

Merged
merged 3 commits into from
Jan 19, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions source/common/router/rds_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
Expand Down
4 changes: 2 additions & 2 deletions source/common/router/route_config_update_receiver_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
9 changes: 4 additions & 5 deletions source/common/router/scoped_rds.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions source/common/runtime/runtime_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
Expand Down
4 changes: 2 additions & 2 deletions source/common/secret/sds_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
5 changes: 3 additions & 2 deletions source/common/upstream/cds_api_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down Expand Up @@ -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()));
Expand Down
4 changes: 2 additions & 2 deletions source/common/upstream/eds.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
Expand Down
6 changes: 3 additions & 3 deletions source/server/lds_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
Expand Down Expand Up @@ -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);
Expand Down
12 changes: 12 additions & 0 deletions test/common/protobuf/utility_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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) {
junr03 marked this conversation as resolved.
Show resolved Hide resolved
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;
Expand Down