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

util: moving more cases to non-throwing unpackTo #32775

Merged
merged 2 commits into from
Mar 27, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ class KeyValueStoreXdsDelegateIntegrationTest
// Expect at least the "client_cert" dynamic secret.
ASSERT_GE(config_dump.configs_size(), 1);
envoy::admin::v3::SecretsConfigDump::DynamicSecret dynamic_secret;
ASSERT_OK(MessageUtil::unpackToNoThrow(config_dump.configs(0), dynamic_secret));
ASSERT_OK(MessageUtil::unpackTo(config_dump.configs(0), dynamic_secret));
EXPECT_EQ(secret_name, dynamic_secret.name());
EXPECT_EQ(version_info, dynamic_secret.version_info());
}
Expand Down
2 changes: 1 addition & 1 deletion contrib/config/test/kv_store_xds_delegate_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ class KeyValueStoreXdsDelegateTest : public testing::Test {
EXPECT_EQ(expected_resources.size(), retrieved_resources.size());
for (size_t i = 0; i < expected_resources.size(); ++i) {
Resource unpacked_resource;
MessageUtil::unpackTo(retrieved_resources[i].resource(), unpacked_resource);
THROW_IF_NOT_OK(MessageUtil::unpackTo(retrieved_resources[i].resource(), unpacked_resource));
TestUtility::protoEqual(expected_resources[i].get().resource(), unpacked_resource);
}
}
Expand Down
2 changes: 1 addition & 1 deletion contrib/dlb/source/connection_balancer_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ DlbConnectionBalanceFactory::createConnectionBalancerFromProto(
const auto& typed_config =
dynamic_cast<const envoy::config::core::v3::TypedExtensionConfig&>(config);
envoy::extensions::network::connection_balance::dlb::v3alpha::Dlb dlb_config;
auto status = Envoy::MessageUtil::unpackToNoThrow(typed_config.typed_config(), dlb_config);
auto status = Envoy::MessageUtil::unpackTo(typed_config.typed_config(), dlb_config);
if (!status.ok()) {
return fallback(fmt::format("unexpected dlb config: {}", typed_config.DebugString()));
}
Expand Down
2 changes: 1 addition & 1 deletion contrib/dlb/test/config_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class DlbConnectionBalanceFactoryTest : public testing::Test {
EXPECT_EQ(typed_config.typed_config().type_url(),
"type.googleapis.com/"
"envoy.extensions.network.connection_balance.dlb.v3alpha.Dlb");
ASSERT_OK(MessageUtil::unpackToNoThrow(typed_config.typed_config(), dlb));
ASSERT_OK(MessageUtil::unpackTo(typed_config.typed_config(), dlb));
}
};

Expand Down
2 changes: 1 addition & 1 deletion source/common/config/decoded_resource_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class DecodedResourceImpl : public DecodedResource {
const std::string& version) {
if (resource.Is<envoy::service::discovery::v3::Resource>()) {
envoy::service::discovery::v3::Resource r;
MessageUtil::unpackTo(resource, r);
MessageUtil::unpackToOrThrow(resource, r);

r.set_version(version);

Expand Down
8 changes: 4 additions & 4 deletions source/common/config/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ void Utility::translateOpaqueConfig(const ProtobufWkt::Any& typed_config,

if (type == typed_struct_type) {
xds::type::v3::TypedStruct typed_struct;
MessageUtil::unpackTo(typed_config, typed_struct);
MessageUtil::unpackToOrThrow(typed_config, typed_struct);
// if out_proto is expecting Struct, return directly
if (out_proto.GetTypeName() == struct_type) {
out_proto.CheckTypeAndMergeFrom(typed_struct.value());
Expand All @@ -250,7 +250,7 @@ void Utility::translateOpaqueConfig(const ProtobufWkt::Any& typed_config,
}
} else if (type == legacy_typed_struct_type) {
udpa::type::v1::TypedStruct typed_struct;
MessageUtil::unpackTo(typed_config, typed_struct);
MessageUtil::unpackToOrThrow(typed_config, typed_struct);
// if out_proto is expecting Struct, return directly
if (out_proto.GetTypeName() == struct_type) {
out_proto.CheckTypeAndMergeFrom(typed_struct.value());
Expand All @@ -266,11 +266,11 @@ void Utility::translateOpaqueConfig(const ProtobufWkt::Any& typed_config,
}
} // out_proto is expecting Struct, unpack directly
else if (type != struct_type || out_proto.GetTypeName() == struct_type) {
MessageUtil::unpackTo(typed_config, out_proto);
MessageUtil::unpackToOrThrow(typed_config, out_proto);
} else {
#ifdef ENVOY_ENABLE_YAML
ProtobufWkt::Struct struct_config;
MessageUtil::unpackTo(typed_config, struct_config);
MessageUtil::unpackToOrThrow(typed_config, struct_config);
MessageUtil::jsonConvert(struct_config, validation_visitor, out_proto);
#else
IS_ENVOY_BUG("Attempting to use JSON structs with JSON compiled out");
Expand Down
4 changes: 2 additions & 2 deletions source/common/config/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -308,12 +308,12 @@ class Utility {
auto type = std::string(TypeUtil::typeUrlToDescriptorFullName(typed_config.type_url()));
if (type == typed_struct_type) {
xds::type::v3::TypedStruct typed_struct;
MessageUtil::unpackTo(typed_config, typed_struct);
MessageUtil::unpackToOrThrow(typed_config, typed_struct);
// Not handling nested structs or typed structs in typed structs
return std::string(TypeUtil::typeUrlToDescriptorFullName(typed_struct.type_url()));
} else if (type == legacy_typed_struct_type) {
udpa::type::v1::TypedStruct typed_struct;
MessageUtil::unpackTo(typed_config, typed_struct);
MessageUtil::unpackToOrThrow(typed_config, typed_struct);
// Not handling nested structs or typed structs in typed structs
return std::string(TypeUtil::typeUrlToDescriptorFullName(typed_struct.type_url()));
}
Expand Down
6 changes: 3 additions & 3 deletions source/common/protobuf/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ void MessageUtil::packFrom(ProtobufWkt::Any& any_message, const Protobuf::Messag
#endif
}

void MessageUtil::unpackTo(const ProtobufWkt::Any& any_message, Protobuf::Message& message) {
void MessageUtil::unpackToOrThrow(const ProtobufWkt::Any& any_message, Protobuf::Message& message) {
#if defined(ENVOY_ENABLE_FULL_PROTOS)
if (!any_message.UnpackTo(&message)) {
throwEnvoyExceptionOrPanic(fmt::format("Unable to unpack as {}: {}",
Expand All @@ -354,8 +354,8 @@ void MessageUtil::unpackTo(const ProtobufWkt::Any& any_message, Protobuf::Messag
}
}

absl::Status MessageUtil::unpackToNoThrow(const ProtobufWkt::Any& any_message,
Protobuf::Message& message) {
absl::Status MessageUtil::unpackTo(const ProtobufWkt::Any& any_message,
Protobuf::Message& message) {
#if defined(ENVOY_ENABLE_FULL_PROTOS)
if (!any_message.UnpackTo(&message)) {
return absl::InternalError(absl::StrCat("Unable to unpack as ",
Expand Down
11 changes: 5 additions & 6 deletions source/common/protobuf/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ class MessageUtil {
*
* @throw EnvoyException if the message does not unpack.
*/
static void unpackTo(const ProtobufWkt::Any& any_message, Protobuf::Message& message);
static void unpackToOrThrow(const ProtobufWkt::Any& any_message, Protobuf::Message& message);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to get rid of this call altogether and use the THROW_IF_NOT_OK macto everywhere the throw is needed? It may be easier to search for throw callsites in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm 100% going to, but I'm trying to do it in a way that doesn't add THROW to more files because if I add one call site, others can add more. If I fix excluded files one by one there will be fewer "leaks" I have to clean up.


/**
* Convert from google.protobuf.Any to a typed message. This should be used
Expand All @@ -378,8 +378,7 @@ class MessageUtil {
*
* @return absl::Status
*/
static absl::Status unpackToNoThrow(const ProtobufWkt::Any& any_message,
Protobuf::Message& message);
static absl::Status unpackTo(const ProtobufWkt::Any& any_message, Protobuf::Message& message);

/**
* Convert from google.protobuf.Any to bytes as std::string
Expand All @@ -390,12 +389,12 @@ class MessageUtil {
static std::string anyToBytes(const ProtobufWkt::Any& any) {
if (any.Is<ProtobufWkt::StringValue>()) {
ProtobufWkt::StringValue s;
MessageUtil::unpackTo(any, s);
MessageUtil::unpackToOrThrow(any, s);
return s.value();
}
if (any.Is<ProtobufWkt::BytesValue>()) {
Protobuf::BytesValue b;
MessageUtil::unpackTo(any, b);
MessageUtil::unpackToOrThrow(any, b);
return b.value();
}
return any.value();
Expand All @@ -409,7 +408,7 @@ class MessageUtil {
*/
template <class MessageType>
static inline void anyConvert(const ProtobufWkt::Any& message, MessageType& typed_message) {
unpackTo(message, typed_message);
unpackToOrThrow(message, typed_message);
};

template <class MessageType>
Expand Down
2 changes: 1 addition & 1 deletion source/common/protobuf/visitor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ void traverseMessageWorker(ConstProtoVisitor& visitor, const Protobuf::Message&
target_type_url = any_message->type_url();
// inner_message must be valid as parsing would have already failed to load if there was an
// invalid type_url.
MessageUtil::unpackTo(*any_message, *inner_message);
THROW_IF_NOT_OK(MessageUtil::unpackTo(*any_message, *inner_message));
} else if (message.GetTypeName() == "xds.type.v3.TypedStruct") {
std::tie(inner_message, target_type_url) =
Helper::convertTypedStruct<xds::type::v3::TypedStruct>(message);
Expand Down
2 changes: 1 addition & 1 deletion source/common/upstream/upstream_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1493,7 +1493,7 @@ bool validateTransportSocketSupportsQuic(
}
envoy::extensions::transport_sockets::http_11_proxy::v3::Http11ProxyUpstreamTransport
http11_socket;
MessageUtil::unpackTo(transport_socket.typed_config(), http11_socket);
THROW_IF_NOT_OK(MessageUtil::unpackTo(transport_socket.typed_config(), http11_socket));
return http11_socket.transport_socket().name() == "envoy.transport_sockets.quic";
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ FilesystemCollectionSubscriptionImpl::refreshInternal(ProtobufTypes::MessagePtr*
Protobuf::DynamicMessageFactory dmf;
ProtobufTypes::MessagePtr collection_message;
collection_message.reset(dmf.GetPrototype(collection_descriptor)->New());
MessageUtil::unpackTo(resource_message.resource(), *collection_message);
THROW_IF_NOT_OK(MessageUtil::unpackTo(resource_message.resource(), *collection_message));
const auto* collection_entries_field_descriptor = collection_descriptor->field(0);
// Verify collection message type structure.
if (collection_entries_field_descriptor == nullptr ||
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ Http::FilterHeadersStatus GcpAuthnFilter::decodeHeaders(Http::RequestHeaderMap&
const auto filter_it = filter_metadata.find(std::string(FilterName));
if (filter_it != filter_metadata.end()) {
envoy::extensions::filters::http::gcp_authn::v3::Audience audience;
MessageUtil::unpackTo(filter_it->second, audience);
THROW_IF_NOT_OK(MessageUtil::unpackTo(filter_it->second, audience));
audience_str_ = audience.url();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ class FileSystemHttpCacheFactory : public HttpCacheFactory {
getCache(const envoy::extensions::filters::http::cache::v3::CacheConfig& filter_config,
Server::Configuration::FactoryContext& context) override {
ConfigProto config;
MessageUtil::unpackTo(filter_config.typed_config(), config);
THROW_IF_NOT_OK(MessageUtil::unpackTo(filter_config.typed_config(), config));
std::shared_ptr<CacheSingleton> caches =
context.serverFactoryContext().singletonManager().getTyped<CacheSingleton>(
SINGLETON_MANAGER_REGISTERED_NAME(file_system_http_cache_singleton), [&context] {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ class AppleDnsResolverFactory : public DnsResolverFactory {
typed_dns_resolver_config) const override {
ASSERT(dispatcher.isThreadSafe());
envoy::extensions::network::dns_resolver::apple::v3::AppleDnsResolverConfig apple;
Envoy::MessageUtil::unpackTo(typed_dns_resolver_config.typed_config(), apple);
THROW_IF_NOT_OK(Envoy::MessageUtil::unpackTo(typed_dns_resolver_config.typed_config(), apple));
return std::make_shared<Network::AppleDnsResolverImpl>(apple, dispatcher, api.rootScope());
}
};
Expand Down
2 changes: 1 addition & 1 deletion source/extensions/network/dns_resolver/cares/dns_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -550,7 +550,7 @@ class CaresDnsResolverFactory : public DnsResolverFactory,
ASSERT(dispatcher.isThreadSafe());
// Only c-ares DNS factory will call into this function.
// Directly unpack the typed config to a c-ares object.
Envoy::MessageUtil::unpackTo(typed_dns_resolver_config.typed_config(), cares);
THROW_IF_NOT_OK(Envoy::MessageUtil::unpackTo(typed_dns_resolver_config.typed_config(), cares));
if (!cares.resolvers().empty()) {
const auto& resolver_addrs = cares.resolvers();
resolvers.reserve(resolver_addrs.size());
Expand Down
4 changes: 3 additions & 1 deletion source/server/admin/config_dump_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,9 @@ bool trimResourceMessage(const Protobuf::FieldMask& field_mask, Protobuf::Messag
ASSERT(inner_descriptor != nullptr);
std::unique_ptr<Protobuf::Message> inner_message;
inner_message.reset(dmf.GetPrototype(inner_descriptor)->New());
MessageUtil::unpackTo(any_message, *inner_message);
if (!MessageUtil::unpackTo(any_message, *inner_message).ok()) {
return false;
}
// Trim message.
if (!checkFieldMaskAndTrimMessage(inner_field_mask, *inner_message)) {
return false;
Expand Down
18 changes: 9 additions & 9 deletions test/common/protobuf/utility_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1468,27 +1468,27 @@ TEST_F(ProtobufUtilityTest, AnyConvertAndValidateFailedValidation) {
ProtoValidationException);
}

// MessageUtility::unpackTo() with the wrong type throws.
// MessageUtility::unpackToOrThrow() with the wrong type throws.
TEST_F(ProtobufUtilityTest, UnpackToWrongType) {
ProtobufWkt::Duration source_duration;
source_duration.set_seconds(42);
ProtobufWkt::Any source_any;
source_any.PackFrom(source_duration);
ProtobufWkt::Timestamp dst;
EXPECT_THROW_WITH_REGEX(
MessageUtil::unpackTo(source_any, dst), EnvoyException,
MessageUtil::unpackToOrThrow(source_any, dst), EnvoyException,
R"(Unable to unpack as google.protobuf.Timestamp: \[type.googleapis.com/google.protobuf.Duration\] .*)");
}

// MessageUtility::unpackTo() with API message works at same version.
// MessageUtility::unpackToOrThrow() with API message works at same version.
TEST_F(ProtobufUtilityTest, UnpackToSameVersion) {
{
API_NO_BOOST(envoy::api::v2::Cluster) source;
source.set_drain_connections_on_host_removal(true);
ProtobufWkt::Any source_any;
source_any.PackFrom(source);
API_NO_BOOST(envoy::api::v2::Cluster) dst;
MessageUtil::unpackTo(source_any, dst);
MessageUtil::unpackToOrThrow(source_any, dst);
EXPECT_TRUE(dst.drain_connections_on_host_removal());
}
{
Expand All @@ -1497,31 +1497,31 @@ TEST_F(ProtobufUtilityTest, UnpackToSameVersion) {
ProtobufWkt::Any source_any;
source_any.PackFrom(source);
API_NO_BOOST(envoy::config::cluster::v3::Cluster) dst;
MessageUtil::unpackTo(source_any, dst);
MessageUtil::unpackToOrThrow(source_any, dst);
EXPECT_TRUE(dst.ignore_health_on_host_removal());
}
}

// MessageUtility::unpackToNoThrow() with the right type.
// MessageUtility::unpackTo() with the right type.
TEST_F(ProtobufUtilityTest, UnpackToNoThrowRightType) {
ProtobufWkt::Duration src_duration;
src_duration.set_seconds(42);
ProtobufWkt::Any source_any;
source_any.PackFrom(src_duration);
ProtobufWkt::Duration dst_duration;
EXPECT_OK(MessageUtil::unpackToNoThrow(source_any, dst_duration));
EXPECT_OK(MessageUtil::unpackTo(source_any, dst_duration));
// Source and destination are expected to be equal.
EXPECT_EQ(src_duration, dst_duration);
}

// MessageUtility::unpackToNoThrow() with the wrong type.
// MessageUtility::unpackTo() with the wrong type.
TEST_F(ProtobufUtilityTest, UnpackToNoThrowWrongType) {
ProtobufWkt::Duration source_duration;
source_duration.set_seconds(42);
ProtobufWkt::Any source_any;
source_any.PackFrom(source_duration);
ProtobufWkt::Timestamp dst;
auto status = MessageUtil::unpackToNoThrow(source_any, dst);
auto status = MessageUtil::unpackTo(source_any, dst);
EXPECT_TRUE(absl::IsInternal(status));
EXPECT_THAT(std::string(status.message()),
testing::ContainsRegex("Unable to unpack as google.protobuf.Timestamp: "
Expand Down
3 changes: 2 additions & 1 deletion test/common/router/route_fuzz_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ bool validateOnMatchConfig(const xds::type::matcher::v3::Matcher::OnMatch& on_ma
return false;
}
envoy::config::route::v3::Route on_match_route_action_config;
MessageUtil::unpackTo(on_match.action().typed_config(), on_match_route_action_config);
THROW_IF_NOT_OK(
MessageUtil::unpackTo(on_match.action().typed_config(), on_match_route_action_config));
ENVOY_LOG_MISC(trace, "typed_config of on_match.action is: {}",
on_match_route_action_config.DebugString());
return !isUnsupportedRouteConfig(on_match_route_action_config);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ class FileSystemCacheTestContext {
envoy::extensions::filters::http::cache::v3::CacheConfig cache_config;
TestUtility::loadFromYaml(std::string(yaml_config), cache_config);
ConfigProto cfg;
MessageUtil::unpackTo(cache_config.typed_config(), cfg);
EXPECT_TRUE(MessageUtil::unpackTo(cache_config.typed_config(), cfg).ok());
cfg.set_cache_path(cache_path_);
return cfg;
}
Expand Down
2 changes: 1 addition & 1 deletion test/fuzz/mutable_visitor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ void traverseMessageWorkerExt(ProtoVisitor& visitor, Protobuf::Message& message,
inner_message = Helper::typeUrlToMessage(any_message->type_url());
target_type_url = any_message->type_url();
if (inner_message) {
MessageUtil::unpackTo(*any_message, *inner_message);
THROW_IF_NOT_OK(MessageUtil::unpackTo(*any_message, *inner_message));
}
} else if (message.GetDescriptor()->full_name() == "xds.type.v3.TypedStruct") {
std::tie(inner_message, target_type_url) =
Expand Down
6 changes: 2 additions & 4 deletions test/integration/xds_config_tracker_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,7 @@ class TestXdsConfigTracker : public Config::XdsConfigTracker {
const auto& config_typed_metadata = resource->metadata()->typed_filter_metadata();
if (const auto& metadata_it = config_typed_metadata.find(kTestKey);
metadata_it != config_typed_metadata.end()) {
const auto status =
Envoy::MessageUtil::unpackToNoThrow(metadata_it->second, test_metadata);
const auto status = Envoy::MessageUtil::unpackTo(metadata_it->second, test_metadata);
if (!status.ok()) {
continue;
}
Expand All @@ -80,8 +79,7 @@ class TestXdsConfigTracker : public Config::XdsConfigTracker {
const auto& config_typed_metadata = resource.metadata().typed_filter_metadata();
if (const auto& metadata_it = config_typed_metadata.find(kTestKey);
metadata_it != config_typed_metadata.end()) {
const auto status =
Envoy::MessageUtil::unpackToNoThrow(metadata_it->second, test_metadata);
const auto status = Envoy::MessageUtil::unpackTo(metadata_it->second, test_metadata);
if (!status.ok()) {
continue;
}
Expand Down
4 changes: 2 additions & 2 deletions test/integration/xds_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -977,10 +977,10 @@ TEST_P(XdsSotwMultipleAuthoritiesTest, SameResourceNameAndTypeFromMultipleAuthor
// Two xDS resources with the same name and same type.
ASSERT_EQ(config_dump.configs_size(), 2);
envoy::admin::v3::SecretsConfigDump::DynamicSecret dynamic_secret;
ASSERT_OK(MessageUtil::unpackToNoThrow(config_dump.configs(0), dynamic_secret));
ASSERT_OK(MessageUtil::unpackTo(config_dump.configs(0), dynamic_secret));
EXPECT_EQ(cert_name, dynamic_secret.name());
EXPECT_EQ("1", dynamic_secret.version_info());
ASSERT_OK(MessageUtil::unpackToNoThrow(config_dump.configs(1), dynamic_secret));
ASSERT_OK(MessageUtil::unpackTo(config_dump.configs(1), dynamic_secret));
EXPECT_EQ(cert_name, dynamic_secret.name());
EXPECT_EQ("1", dynamic_secret.version_info());
}
Expand Down
Loading