From 999b78570c776f03ccc008b58897bbe2d255eab3 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Wed, 8 Aug 2018 21:48:22 -0700 Subject: [PATCH 1/7] smaller fix Signed-off-by: Kuat Yessenov --- source/common/protobuf/utility.cc | 14 ++++++++++++++ source/common/protobuf/utility.h | 3 +++ source/server/BUILD | 1 + source/server/options_impl.cc | 5 +++++ test/common/protobuf/utility_test.cc | 2 ++ test/common/router/config_impl_test.cc | 5 ++++- test/config/utility.cc | 1 - .../network/thrift_proxy/integration_test.cc | 7 +------ test/integration/http_integration.cc | 1 - tools/protodoc/protodoc.bzl | 2 +- 10 files changed, 31 insertions(+), 10 deletions(-) diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index c9be39db0d0ce..140f8f6216ab2 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -48,7 +48,17 @@ ProtoValidationException::ProtoValidationException(const std::string& validation ENVOY_LOG_MISC(debug, "Proto validation error; throwing {}", what()); } +bool MessageUtil::allow_unknown_fields = false; + void MessageUtil::loadFromJson(const std::string& json, Protobuf::Message& message) { + Protobuf::util::JsonParseOptions options; + const auto status = Protobuf::util::JsonStringToMessage(json, &message, options); + if (!status.ok()) { + throw EnvoyException("Unable to parse JSON as proto (" + status.ToString() + "): " + json); + } +} + +void MessageUtil::loadFromJsonLenient(const std::string& json, Protobuf::Message& message) { Protobuf::util::JsonParseOptions options; options.ignore_unknown_fields = true; const auto status = Protobuf::util::JsonStringToMessage(json, &message, options); @@ -121,6 +131,10 @@ void MessageUtil::jsonConvert(const Protobuf::Message& source, Protobuf::Message throw EnvoyException(fmt::format("Unable to convert protobuf message to JSON string: {} {}", status.ToString(), source.DebugString())); } + if (MessageUtil::allow_unknown_fields) { + MessageUtil::loadFromJsonLenient(json, dest); + return; + } MessageUtil::loadFromJson(json, dest); } diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index 93d098c75d739..dacdad7cc80fb 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -158,7 +158,10 @@ class MessageUtil { return HashUtil::xxHash64(text); } + static bool allow_unknown_fields; + static void loadFromJson(const std::string& json, Protobuf::Message& message); + static void loadFromJsonLenient(const std::string& json, Protobuf::Message& message); static void loadFromYaml(const std::string& yaml, Protobuf::Message& message); static void loadFromFile(const std::string& path, Protobuf::Message& message); diff --git a/source/server/BUILD b/source/server/BUILD index 648c17a4e0f4b..892d31ca1b660 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -156,6 +156,7 @@ envoy_cc_library( "//include/envoy/stats:stats_interface", "//source/common/common:macros", "//source/common/common:version_lib", + "//source/common/protobuf:utility_lib", "//source/common/stats:stats_lib", ], ) diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index 2feda07591711..9fcb6056df46f 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -9,6 +9,7 @@ #include "common/common/logger.h" #include "common/common/macros.h" #include "common/common/version.h" +#include "common/protobuf/utility.h" #include "spdlog/spdlog.h" #include "tclap/CmdLine.h" @@ -64,6 +65,9 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv, TCLAP::SwitchArg allow_v1_config("", "allow-deprecated-v1-api", "allow use of legacy v1 config", cmd, false); + TCLAP::SwitchArg allow_unknown_fields( + "", "allow-unknown-fields", "allow unknown fields in the filter configuration", cmd, false); + TCLAP::ValueArg admin_address_path("", "admin-address-path", "Admin address path", false, "", "string", cmd); TCLAP::ValueArg local_address_ip_version("", "local-address-ip-version", @@ -184,6 +188,7 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv, config_path_ = config_path.getValue(); config_yaml_ = config_yaml.getValue(); v2_config_only_ = !allow_v1_config.getValue(); + MessageUtil::allow_unknown_fields = allow_unknown_fields.getValue(); admin_address_path_ = admin_address_path.getValue(); log_path_ = log_path.getValue(); restart_epoch_ = restart_epoch.getValue(); diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 15dbfa1a55f50..293fdfdf62543 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -221,9 +221,11 @@ TEST(UtilityTest, JsonConvertSuccess) { } TEST(UtilityTest, JsonConvertUnknownFieldSuccess) { + MessageUtil::allow_unknown_fields = true; const ProtobufWkt::Struct obj = MessageUtil::keyValueStruct("test_key", "test_value"); envoy::config::bootstrap::v2::Bootstrap bootstrap; EXPECT_NO_THROW(MessageUtil::jsonConvert(obj, bootstrap)); + MessageUtil::allow_unknown_fields = false; } TEST(UtilityTest, JsonConvertFail) { diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index eed552b9bc55b..a4585ad8d447b 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -4490,6 +4490,9 @@ class PerFilterConfigsTest : public testing::Test { Server::Configuration::FactoryContext&) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } + ProtobufTypes::MessagePtr createEmptyRouteConfigProto() override { + return ProtobufTypes::MessagePtr{new ProtobufWkt::Timestamp()}; + } }; void checkEach(const std::string& yaml, uint32_t expected_entry, uint32_t expected_route, @@ -4563,7 +4566,7 @@ name: foo routes: - match: { prefix: "/" } route: { cluster: baz } - per_filter_config: { test.default.filter: { unknown_key: 123} } + per_filter_config: { test.default.filter: { seconds: 123} } )EOF"; checkNoPerFilterConfig(yaml); diff --git a/test/config/utility.cc b/test/config/utility.cc index f0bcc370fcbf0..2cfdca3098e1f 100644 --- a/test/config/utility.cc +++ b/test/config/utility.cc @@ -89,7 +89,6 @@ const std::string ConfigHelper::DEFAULT_HEALTH_CHECK_FILTER = name: envoy.health_check config: pass_through_mode: false - endpoint: /healthcheck )EOF"; const std::string ConfigHelper::DEFAULT_SQUASH_FILTER = diff --git a/test/extensions/filters/network/thrift_proxy/integration_test.cc b/test/extensions/filters/network/thrift_proxy/integration_test.cc index b15ea8fb4e820..6a9e5753fcb13 100644 --- a/test/extensions/filters/network/thrift_proxy/integration_test.cc +++ b/test/extensions/filters/network/thrift_proxy/integration_test.cc @@ -46,12 +46,7 @@ class ThriftConnManagerIntegrationTest route_config: name: "routes" routes: - - match: - method_name: "execute" - route: - cluster: "cluster_0" - - match: - method_name: "poke" + - match: {} route: cluster: "cluster_0" )EOF"; diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index 3f93b90aed7e4..f8ab7a672bd80 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -409,7 +409,6 @@ void HttpIntegrationTest::testComputedHealthCheck() { name: envoy.health_check config: pass_through_mode: false - endpoint: /healthcheck cluster_min_healthy_percentages: example_cluster_name: { value: 75 } )EOF"); diff --git a/tools/protodoc/protodoc.bzl b/tools/protodoc/protodoc.bzl index c7ab5c8948890..4b11e8ef34573 100644 --- a/tools/protodoc/protodoc.bzl +++ b/tools/protodoc/protodoc.bzl @@ -78,7 +78,6 @@ def _proto_doc_aspect_impl(target, ctx): return [OutputGroupInfo(rst = transitive_outputs)] proto_doc_aspect = aspect( - implementation = _proto_doc_aspect_impl, attr_aspects = ["deps"], attrs = { "_protoc": attr.label( @@ -92,4 +91,5 @@ proto_doc_aspect = aspect( cfg = "host", ), }, + implementation = _proto_doc_aspect_impl, ) From 88c642288e2d3426fa7c1016a2746b012a9d92a5 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Thu, 9 Aug 2018 12:29:19 -0700 Subject: [PATCH 2/7] add tests Signed-off-by: Kuat Yessenov --- source/common/config/utility.h | 9 ++++++++- source/common/protobuf/utility.cc | 5 +++++ source/common/protobuf/utility.h | 5 +++++ test/common/config/utility_test.cc | 10 ++++++++++ test/common/protobuf/utility_test.cc | 30 ++++++++++++++++++++++++++++ 5 files changed, 58 insertions(+), 1 deletion(-) diff --git a/source/common/config/utility.h b/source/common/config/utility.h index 165be6c55c2cf..b213fc780304f 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -52,7 +52,14 @@ class Utility { Protobuf::RepeatedPtrField typed_resources; for (const auto& resource : response.resources()) { auto* typed_resource = typed_resources.Add(); - resource.UnpackTo(typed_resource); + if (!resource.UnpackTo(typed_resource)) { + throw EnvoyException("Unable to unpack " + resource.DebugString()); + } + if (!MessageUtil::allow_unknown_fields && + !typed_resource->GetReflection()->GetUnknownFields(*typed_resource).empty()) { + throw EnvoyException("Protobuf Any (type " + typed_resource->GetTypeName() + + ") has unknown fields"); + } } return typed_resources; } diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 140f8f6216ab2..eb58ea35da79a 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -78,6 +78,11 @@ void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& messa if (StringUtil::endsWith(path, ".pb")) { // Attempt to parse the binary format. if (message.ParseFromString(contents)) { + if (!MessageUtil::allow_unknown_fields && + !message.GetReflection()->GetUnknownFields(message).empty()) { + throw EnvoyException("Protobuf Any (type " + message.GetTypeName() + + ") has unknown fields"); + } return; } throw EnvoyException("Unable to parse file \"" + path + "\" as a binary protobuf (type " + diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index dacdad7cc80fb..0bad850769d86 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -217,6 +217,11 @@ class MessageUtil { if (!message.UnpackTo(&typed_message)) { throw EnvoyException("Unable to unpack " + message.DebugString()); } + if (!MessageUtil::allow_unknown_fields && + !typed_message.GetReflection()->GetUnknownFields(typed_message).empty()) { + throw EnvoyException("Protobuf Any (type " + typed_message.GetTypeName() + + ") has unknown fields"); + } return typed_message; }; diff --git a/test/common/config/utility_test.cc b/test/common/config/utility_test.cc index 4efc476395c47..ecefb84b37db3 100644 --- a/test/common/config/utility_test.cc +++ b/test/common/config/utility_test.cc @@ -46,6 +46,16 @@ TEST(UtilityTest, GetTypedResources) { EXPECT_EQ("1", typed_resources[1].cluster_name()); } +TEST(UtilityTest, GetTypedResourcesWrongType) { + envoy::api::v2::DiscoveryResponse response; + envoy::api::v2::ClusterLoadAssignment load_assignment_0; + load_assignment_0.set_cluster_name("0"); + response.add_resources()->PackFrom(load_assignment_0); + + EXPECT_THROW_WITH_REGEX(Utility::getTypedResources(response), + EnvoyException, "Unable to unpack .*"); +} + TEST(UtilityTest, ComputeHashedVersion) { EXPECT_EQ("hash_2e1472b57af294d1", Utility::computeHashedVersion("{}").first); EXPECT_EQ("hash_33bf00a859c4ba3f", Utility::computeHashedVersion("foo").first); diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 293fdfdf62543..5279361c23f9a 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -54,6 +54,17 @@ TEST(UtilityTest, LoadBinaryProtoFromFile) { EXPECT_TRUE(TestUtility::protoEqual(bootstrap, proto_from_file)); } +TEST(UtilityTest, LoadBinaryProtoUnknownFieldFromFile) { + ProtobufWkt::Duration source_duration; + source_duration.set_seconds(42); + const std::string filename = + TestEnvironment::writeStringToFileForTest("proto.pb", source_duration.SerializeAsString()); + envoy::config::bootstrap::v2::Bootstrap proto_from_file; + EXPECT_THROW_WITH_MESSAGE( + MessageUtil::loadFromFile(filename, proto_from_file), EnvoyException, + "Protobuf Any (type envoy.config.bootstrap.v2.Bootstrap) has unknown fields"); +} + TEST(UtilityTest, LoadTextProtoFromFile) { envoy::config::bootstrap::v2::Bootstrap bootstrap; bootstrap.mutable_cluster_manager() @@ -212,6 +223,25 @@ TEST(UtilityTest, HashedValueStdHash) { EXPECT_NE(set.find(hv3), set.end()); } +TEST(UtilityTest, AnyConvertWrongType) { + ProtobufWkt::Duration source_duration; + source_duration.set_seconds(42); + ProtobufWkt::Any source_any; + source_any.PackFrom(source_duration); + EXPECT_THROW_WITH_REGEX(MessageUtil::anyConvert(source_any), + EnvoyException, "Unable to unpack .*"); +} + +TEST(UtilityTest, AnyConvertWrongFields) { + const ProtobufWkt::Struct obj = MessageUtil::keyValueStruct("test_key", "test_value"); + ProtobufWkt::Any source_any; + source_any.PackFrom(obj); + source_any.set_type_url("type.google.com/google.protobuf.Timestamp"); + EXPECT_THROW_WITH_MESSAGE(MessageUtil::anyConvert(source_any), + EnvoyException, + "Protobuf Any (type google.protobuf.Timestamp) has unknown fields"); +} + TEST(UtilityTest, JsonConvertSuccess) { ProtobufWkt::Duration source_duration; source_duration.set_seconds(42); From 2360b5e2d13121b803b1cf0dfb2e1292ef4d352a Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Thu, 9 Aug 2018 12:34:11 -0700 Subject: [PATCH 3/7] comment fix Signed-off-by: Kuat Yessenov --- source/server/options_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index 9fcb6056df46f..98641f14be472 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -66,7 +66,7 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv, cmd, false); TCLAP::SwitchArg allow_unknown_fields( - "", "allow-unknown-fields", "allow unknown fields in the filter configuration", cmd, false); + "", "allow-unknown-fields", "allow unknown fields in the configuration", cmd, false); TCLAP::ValueArg admin_address_path("", "admin-address-path", "Admin address path", false, "", "string", cmd); From 5bdb7cb1e22f20da337e3882b218c3bbc87e6932 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Thu, 9 Aug 2018 12:42:39 -0700 Subject: [PATCH 4/7] fix format Signed-off-by: Kuat Yessenov --- source/server/options_impl.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index 98641f14be472..3a463989e0adf 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -65,8 +65,8 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv, TCLAP::SwitchArg allow_v1_config("", "allow-deprecated-v1-api", "allow use of legacy v1 config", cmd, false); - TCLAP::SwitchArg allow_unknown_fields( - "", "allow-unknown-fields", "allow unknown fields in the configuration", cmd, false); + TCLAP::SwitchArg allow_unknown_fields("", "allow-unknown-fields", + "allow unknown fields in the configuration", cmd, false); TCLAP::ValueArg admin_address_path("", "admin-address-path", "Admin address path", false, "", "string", cmd); From 995b2782cfb7cae4cbb25cf23d5792f9d85f5bf0 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Thu, 9 Aug 2018 16:57:52 -0700 Subject: [PATCH 5/7] cleanup duplicate code Signed-off-by: Kuat Yessenov --- source/common/config/utility.h | 6 +----- source/common/protobuf/utility.cc | 6 +----- source/common/protobuf/utility.h | 13 ++++++++----- 3 files changed, 10 insertions(+), 15 deletions(-) diff --git a/source/common/config/utility.h b/source/common/config/utility.h index b213fc780304f..293d0b03b9f73 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -55,11 +55,7 @@ class Utility { if (!resource.UnpackTo(typed_resource)) { throw EnvoyException("Unable to unpack " + resource.DebugString()); } - if (!MessageUtil::allow_unknown_fields && - !typed_resource->GetReflection()->GetUnknownFields(*typed_resource).empty()) { - throw EnvoyException("Protobuf Any (type " + typed_resource->GetTypeName() + - ") has unknown fields"); - } + MessageUtil::checkUnknownFields(*typed_resource); } return typed_resources; } diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index eb58ea35da79a..1bd6c95ba8c6e 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -78,11 +78,7 @@ void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& messa if (StringUtil::endsWith(path, ".pb")) { // Attempt to parse the binary format. if (message.ParseFromString(contents)) { - if (!MessageUtil::allow_unknown_fields && - !message.GetReflection()->GetUnknownFields(message).empty()) { - throw EnvoyException("Protobuf Any (type " + message.GetTypeName() + - ") has unknown fields"); - } + MessageUtil::checkUnknownFields(message); return; } throw EnvoyException("Unable to parse file \"" + path + "\" as a binary protobuf (type " + diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index 0bad850769d86..95ea5e428ba96 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -160,6 +160,13 @@ class MessageUtil { static bool allow_unknown_fields; + static void checkUnknownFields(const Protobuf::Message& message) { + if (!MessageUtil::allow_unknown_fields && + !message.GetReflection()->GetUnknownFields(message).empty()) { + throw EnvoyException("Protobuf Any (type " + message.GetTypeName() + ") has unknown fields"); + } + } + static void loadFromJson(const std::string& json, Protobuf::Message& message); static void loadFromJsonLenient(const std::string& json, Protobuf::Message& message); static void loadFromYaml(const std::string& yaml, Protobuf::Message& message); @@ -217,11 +224,7 @@ class MessageUtil { if (!message.UnpackTo(&typed_message)) { throw EnvoyException("Unable to unpack " + message.DebugString()); } - if (!MessageUtil::allow_unknown_fields && - !typed_message.GetReflection()->GetUnknownFields(typed_message).empty()) { - throw EnvoyException("Protobuf Any (type " + typed_message.GetTypeName() + - ") has unknown fields"); - } + checkUnknownFields(typed_message); return typed_message; }; From 7d51f8e75e21351da6ec394a6619f76d407650b0 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Fri, 10 Aug 2018 11:16:32 -0700 Subject: [PATCH 6/7] review Signed-off-by: Kuat Yessenov --- source/common/protobuf/utility.cc | 17 +++++------------ source/common/protobuf/utility.h | 6 ++++-- test/common/protobuf/utility_test.cc | 4 ++-- 3 files changed, 11 insertions(+), 16 deletions(-) diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 1bd6c95ba8c6e..200f004c441fb 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -51,16 +51,13 @@ ProtoValidationException::ProtoValidationException(const std::string& validation bool MessageUtil::allow_unknown_fields = false; void MessageUtil::loadFromJson(const std::string& json, Protobuf::Message& message) { - Protobuf::util::JsonParseOptions options; - const auto status = Protobuf::util::JsonStringToMessage(json, &message, options); - if (!status.ok()) { - throw EnvoyException("Unable to parse JSON as proto (" + status.ToString() + "): " + json); - } + MessageUtil::loadFromJsonCustom(json, message, false); } -void MessageUtil::loadFromJsonLenient(const std::string& json, Protobuf::Message& message) { +void MessageUtil::loadFromJsonCustom(const std::string& json, Protobuf::Message& message, + bool allow_unknown_fields) { Protobuf::util::JsonParseOptions options; - options.ignore_unknown_fields = true; + options.ignore_unknown_fields = allow_unknown_fields; const auto status = Protobuf::util::JsonStringToMessage(json, &message, options); if (!status.ok()) { throw EnvoyException("Unable to parse JSON as proto (" + status.ToString() + "): " + json); @@ -132,11 +129,7 @@ void MessageUtil::jsonConvert(const Protobuf::Message& source, Protobuf::Message throw EnvoyException(fmt::format("Unable to convert protobuf message to JSON string: {} {}", status.ToString(), source.DebugString())); } - if (MessageUtil::allow_unknown_fields) { - MessageUtil::loadFromJsonLenient(json, dest); - return; - } - MessageUtil::loadFromJson(json, dest); + MessageUtil::loadFromJsonCustom(json, dest, MessageUtil::allow_unknown_fields); } ProtobufWkt::Struct MessageUtil::keyValueStruct(const std::string& key, const std::string& value) { diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index 95ea5e428ba96..15fd2360e344e 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -163,12 +163,14 @@ class MessageUtil { static void checkUnknownFields(const Protobuf::Message& message) { if (!MessageUtil::allow_unknown_fields && !message.GetReflection()->GetUnknownFields(message).empty()) { - throw EnvoyException("Protobuf Any (type " + message.GetTypeName() + ") has unknown fields"); + throw EnvoyException("Protobuf message (type " + message.GetTypeName() + + ") has unknown fields"); } } static void loadFromJson(const std::string& json, Protobuf::Message& message); - static void loadFromJsonLenient(const std::string& json, Protobuf::Message& message); + static void loadFromJsonCustom(const std::string& json, Protobuf::Message& message, + bool allow_unknown_fields); static void loadFromYaml(const std::string& yaml, Protobuf::Message& message); static void loadFromFile(const std::string& path, Protobuf::Message& message); diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 5279361c23f9a..7f9ea71b11ff4 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -62,7 +62,7 @@ TEST(UtilityTest, LoadBinaryProtoUnknownFieldFromFile) { envoy::config::bootstrap::v2::Bootstrap proto_from_file; EXPECT_THROW_WITH_MESSAGE( MessageUtil::loadFromFile(filename, proto_from_file), EnvoyException, - "Protobuf Any (type envoy.config.bootstrap.v2.Bootstrap) has unknown fields"); + "Protobuf message (type envoy.config.bootstrap.v2.Bootstrap) has unknown fields"); } TEST(UtilityTest, LoadTextProtoFromFile) { @@ -239,7 +239,7 @@ TEST(UtilityTest, AnyConvertWrongFields) { source_any.set_type_url("type.google.com/google.protobuf.Timestamp"); EXPECT_THROW_WITH_MESSAGE(MessageUtil::anyConvert(source_any), EnvoyException, - "Protobuf Any (type google.protobuf.Timestamp) has unknown fields"); + "Protobuf message (type google.protobuf.Timestamp) has unknown fields"); } TEST(UtilityTest, JsonConvertSuccess) { From 416f490dd2b279fcf42a28e5b9a9d340d2f0a703 Mon Sep 17 00:00:00 2001 From: Kuat Yessenov Date: Fri, 10 Aug 2018 11:49:23 -0700 Subject: [PATCH 7/7] use enum Signed-off-by: Kuat Yessenov --- source/common/protobuf/utility.cc | 14 ++++++++------ source/common/protobuf/utility.h | 10 ++++++---- source/server/options_impl.cc | 4 +++- test/common/protobuf/utility_test.cc | 4 ++-- 4 files changed, 19 insertions(+), 13 deletions(-) diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 200f004c441fb..7ac81a8f66466 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -48,16 +48,18 @@ ProtoValidationException::ProtoValidationException(const std::string& validation ENVOY_LOG_MISC(debug, "Proto validation error; throwing {}", what()); } -bool MessageUtil::allow_unknown_fields = false; +ProtoUnknownFieldsMode MessageUtil::proto_unknown_fields = ProtoUnknownFieldsMode::Strict; void MessageUtil::loadFromJson(const std::string& json, Protobuf::Message& message) { - MessageUtil::loadFromJsonCustom(json, message, false); + MessageUtil::loadFromJsonEx(json, message, ProtoUnknownFieldsMode::Strict); } -void MessageUtil::loadFromJsonCustom(const std::string& json, Protobuf::Message& message, - bool allow_unknown_fields) { +void MessageUtil::loadFromJsonEx(const std::string& json, Protobuf::Message& message, + ProtoUnknownFieldsMode proto_unknown_fields) { Protobuf::util::JsonParseOptions options; - options.ignore_unknown_fields = allow_unknown_fields; + if (proto_unknown_fields == ProtoUnknownFieldsMode::Allow) { + options.ignore_unknown_fields = true; + } const auto status = Protobuf::util::JsonStringToMessage(json, &message, options); if (!status.ok()) { throw EnvoyException("Unable to parse JSON as proto (" + status.ToString() + "): " + json); @@ -129,7 +131,7 @@ void MessageUtil::jsonConvert(const Protobuf::Message& source, Protobuf::Message throw EnvoyException(fmt::format("Unable to convert protobuf message to JSON string: {} {}", status.ToString(), source.DebugString())); } - MessageUtil::loadFromJsonCustom(json, dest, MessageUtil::allow_unknown_fields); + MessageUtil::loadFromJsonEx(json, dest, MessageUtil::proto_unknown_fields); } ProtobufWkt::Struct MessageUtil::keyValueStruct(const std::string& key, const std::string& value) { diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index 15fd2360e344e..5c1bd9e12f3f3 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -133,6 +133,8 @@ class ProtoValidationException : public EnvoyException { ProtoValidationException(const std::string& validation_error, const Protobuf::Message& message); }; +enum class ProtoUnknownFieldsMode { Strict, Allow }; + class MessageUtil { public: // std::hash @@ -158,10 +160,10 @@ class MessageUtil { return HashUtil::xxHash64(text); } - static bool allow_unknown_fields; + static ProtoUnknownFieldsMode proto_unknown_fields; static void checkUnknownFields(const Protobuf::Message& message) { - if (!MessageUtil::allow_unknown_fields && + if (MessageUtil::proto_unknown_fields == ProtoUnknownFieldsMode::Strict && !message.GetReflection()->GetUnknownFields(message).empty()) { throw EnvoyException("Protobuf message (type " + message.GetTypeName() + ") has unknown fields"); @@ -169,8 +171,8 @@ class MessageUtil { } static void loadFromJson(const std::string& json, Protobuf::Message& message); - static void loadFromJsonCustom(const std::string& json, Protobuf::Message& message, - bool allow_unknown_fields); + static void loadFromJsonEx(const std::string& json, Protobuf::Message& message, + ProtoUnknownFieldsMode proto_unknown_fields); static void loadFromYaml(const std::string& yaml, Protobuf::Message& message); static void loadFromFile(const std::string& path, Protobuf::Message& message); diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index 3a463989e0adf..207da55303608 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -188,7 +188,9 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv, config_path_ = config_path.getValue(); config_yaml_ = config_yaml.getValue(); v2_config_only_ = !allow_v1_config.getValue(); - MessageUtil::allow_unknown_fields = allow_unknown_fields.getValue(); + if (allow_unknown_fields.getValue()) { + MessageUtil::proto_unknown_fields = ProtoUnknownFieldsMode::Allow; + } admin_address_path_ = admin_address_path.getValue(); log_path_ = log_path.getValue(); restart_epoch_ = restart_epoch.getValue(); diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 7f9ea71b11ff4..4f41c7560d95c 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -251,11 +251,11 @@ TEST(UtilityTest, JsonConvertSuccess) { } TEST(UtilityTest, JsonConvertUnknownFieldSuccess) { - MessageUtil::allow_unknown_fields = true; + MessageUtil::proto_unknown_fields = ProtoUnknownFieldsMode::Allow; const ProtobufWkt::Struct obj = MessageUtil::keyValueStruct("test_key", "test_value"); envoy::config::bootstrap::v2::Bootstrap bootstrap; EXPECT_NO_THROW(MessageUtil::jsonConvert(obj, bootstrap)); - MessageUtil::allow_unknown_fields = false; + MessageUtil::proto_unknown_fields = ProtoUnknownFieldsMode::Strict; } TEST(UtilityTest, JsonConvertFail) {