diff --git a/source/common/config/utility.h b/source/common/config/utility.h index 165be6c55c2cf..293d0b03b9f73 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -52,7 +52,10 @@ 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()); + } + MessageUtil::checkUnknownFields(*typed_resource); } return typed_resources; } diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index c9be39db0d0ce..7ac81a8f66466 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -48,9 +48,18 @@ ProtoValidationException::ProtoValidationException(const std::string& validation ENVOY_LOG_MISC(debug, "Proto validation error; throwing {}", what()); } +ProtoUnknownFieldsMode MessageUtil::proto_unknown_fields = ProtoUnknownFieldsMode::Strict; + void MessageUtil::loadFromJson(const std::string& json, Protobuf::Message& message) { + MessageUtil::loadFromJsonEx(json, message, ProtoUnknownFieldsMode::Strict); +} + +void MessageUtil::loadFromJsonEx(const std::string& json, Protobuf::Message& message, + ProtoUnknownFieldsMode proto_unknown_fields) { Protobuf::util::JsonParseOptions options; - options.ignore_unknown_fields = true; + 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); @@ -68,6 +77,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)) { + MessageUtil::checkUnknownFields(message); return; } throw EnvoyException("Unable to parse file \"" + path + "\" as a binary protobuf (type " + @@ -121,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::loadFromJson(json, dest); + 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 93d098c75d739..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,7 +160,19 @@ class MessageUtil { return HashUtil::xxHash64(text); } + static ProtoUnknownFieldsMode proto_unknown_fields; + + static void checkUnknownFields(const Protobuf::Message& message) { + if (MessageUtil::proto_unknown_fields == ProtoUnknownFieldsMode::Strict && + !message.GetReflection()->GetUnknownFields(message).empty()) { + throw EnvoyException("Protobuf message (type " + message.GetTypeName() + + ") has unknown fields"); + } + } + static void loadFromJson(const std::string& json, Protobuf::Message& message); + 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); @@ -214,6 +228,7 @@ class MessageUtil { if (!message.UnpackTo(&typed_message)) { throw EnvoyException("Unable to unpack " + message.DebugString()); } + checkUnknownFields(typed_message); return typed_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..207da55303608 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 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,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(); + 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/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 15dbfa1a55f50..4f41c7560d95c 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 message (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 message (type google.protobuf.Timestamp) has unknown fields"); +} + TEST(UtilityTest, JsonConvertSuccess) { ProtobufWkt::Duration source_duration; source_duration.set_seconds(42); @@ -221,9 +251,11 @@ TEST(UtilityTest, JsonConvertSuccess) { } TEST(UtilityTest, JsonConvertUnknownFieldSuccess) { + 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::proto_unknown_fields = ProtoUnknownFieldsMode::Strict; } 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, )