From b2e47fc6f5ce711ec68d9b7b36764c01d4a4b839 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 29 Jan 2019 15:40:36 -0500 Subject: [PATCH 1/5] config: logging a warning on use of deprecated fields Signed-off-by: Alyssa Wilk --- source/common/protobuf/utility.h | 24 ++++++++++++++++++++++++ test/common/protobuf/BUILD | 1 + test/common/protobuf/utility_test.cc | 26 ++++++++++++++++++++++++++ test/proto/BUILD | 5 +++++ test/proto/deprecated.proto | 12 ++++++++++++ 5 files changed, 68 insertions(+) create mode 100644 test/proto/deprecated.proto diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index 7dcdddb24c48c..ad57526f213ff 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -6,6 +6,7 @@ #include "envoy/json/json_object.h" #include "envoy/type/percent.pb.h" +#include "common/common/fmt.h" #include "common/common/hash.h" #include "common/common/utility.h" #include "common/json/json_loader.h" @@ -177,6 +178,26 @@ class MessageUtil { static void loadFromYaml(const std::string& yaml, Protobuf::Message& message); static void loadFromFile(const std::string& path, Protobuf::Message& message); + template + static void checkForDeprecation(const MessageType& message, bool fatal) { + const auto* desc = message.GetDescriptor(); + const auto* refl = message.GetReflection(); + for (int i = 0; i < desc->field_count(); ++i) { + const auto* fd = desc->field(i); + if (fd->options().deprecated() && refl->HasField(message, fd)) { + std::string err = fmt::format( + "Using deprecated option '{}'. This configuration will be removed from Envoy soon. " + "Please see https://github.com/envoyproxy/envoy/blob/master/DEPRECATED.md for " + "details.", + fd->name()); + if (fatal) { + throw ProtoValidationException(err, message); + } else { + ENVOY_LOG_MISC(warn, "{}", err); + } + } + } + } /** * Validate protoc-gen-validate constraints on a given protobuf. * Note the corresponding `.pb.validate.h` for the message has to be included in the source file @@ -185,6 +206,9 @@ class MessageUtil { * @throw ProtoValidationException if the message does not satisfy its type constraints. */ template static void validate(const MessageType& message) { + // Do a (non-fatal) deprecation check to log warnings about deprecated field use. + checkForDeprecation(message, false); + std::string err; if (!Validate(message, &err)) { throw ProtoValidationException(err, message); diff --git a/test/common/protobuf/BUILD b/test/common/protobuf/BUILD index 64cca446764ee..0bffb96b992d2 100644 --- a/test/common/protobuf/BUILD +++ b/test/common/protobuf/BUILD @@ -14,6 +14,7 @@ envoy_cc_test( srcs = ["utility_test.cc"], deps = [ "//source/common/protobuf:utility_lib", + "//test/proto:deprecated_proto_cc", "//test/test_common:environment_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/config/bootstrap/v2:bootstrap_cc", diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 15a2b80f482c6..102b0b76e4aaa 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -6,6 +6,7 @@ #include "common/protobuf/protobuf.h" #include "common/protobuf/utility.h" +#include "test/proto/deprecated.pb.h" #include "test/test_common/environment.h" #include "test/test_common/utility.h" @@ -40,6 +41,31 @@ TEST(UtilityTest, DowncastAndValidate) { ProtoValidationException); } +TEST(UtilityTest, ValidateDeprecatedFields) { + { + envoy::test::deprecation_test::Base base; + base.set_not_deprecated("foo"); + // Fatal checks for a non-deprecated field should cause no problem. + MessageUtil::checkForDeprecation(base, true); + } + { + envoy::test::deprecation_test::Base base; + base.set_is_deprecated("foo"); + // Non-fatal checks for a deprecated field shouldn't throw an exception. + MessageUtil::checkForDeprecation(base, false); + // Fatal checks for a deprecated field should result in an exception. + EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, true), ProtoValidationException, + "Using deprecated option 'is_deprecated'."); + } + { + envoy::test::deprecation_test::Base base; + base.mutable_deprecated_message(); + // Fatal checks for a present (unused) deprecated message should result in an exception. + EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, true), ProtoValidationException, + "Using deprecated option 'deprecated_message'."); + } +} + TEST(UtilityTest, LoadBinaryProtoFromFile) { envoy::config::bootstrap::v2::Bootstrap bootstrap; bootstrap.mutable_cluster_manager() diff --git a/test/proto/BUILD b/test/proto/BUILD index 3e339d903b55f..08c8384dee44c 100644 --- a/test/proto/BUILD +++ b/test/proto/BUILD @@ -11,6 +11,11 @@ envoy_package() exports_files(["bookstore.proto"]) +envoy_proto_library( + name = "deprecated_proto", + srcs = [":deprecated.proto"], +) + envoy_proto_library( name = "helloworld_proto", srcs = [":helloworld.proto"], diff --git a/test/proto/deprecated.proto b/test/proto/deprecated.proto new file mode 100644 index 0000000000000..7f60f99195c16 --- /dev/null +++ b/test/proto/deprecated.proto @@ -0,0 +1,12 @@ +syntax = "proto3"; + +package envoy.test.deprecation_test; + +message Base { + string not_deprecated = 1; + string is_deprecated = 2 [deprecated = true]; + message InnerMessage { + string value = 1; + } + InnerMessage deprecated_message = 3 [deprecated = true]; +} From de43d411199b8d8bf26cd52b83f67809f9d3577e Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 29 Jan 2019 17:06:07 -0500 Subject: [PATCH 2/5] bugfix Signed-off-by: Alyssa Wilk --- source/common/protobuf/utility.h | 41 ++++++++++++++++++++++------ test/common/protobuf/BUILD | 1 + test/common/protobuf/utility_test.cc | 39 ++++++++++++++++++++++++-- test/proto/deprecated.proto | 6 +++- 4 files changed, 76 insertions(+), 11 deletions(-) diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index ad57526f213ff..5cfffa946aab6 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -178,26 +178,51 @@ class MessageUtil { static void loadFromYaml(const std::string& yaml, Protobuf::Message& message); static void loadFromFile(const std::string& path, Protobuf::Message& message); - template - static void checkForDeprecation(const MessageType& message, bool fatal) { - const auto* desc = message.GetDescriptor(); - const auto* refl = message.GetReflection(); - for (int i = 0; i < desc->field_count(); ++i) { - const auto* fd = desc->field(i); - if (fd->options().deprecated() && refl->HasField(message, fd)) { + static void checkForDeprecation(const Protobuf::Message& message, bool fatal) { + const Protobuf::Descriptor* descriptor = message.GetDescriptor(); + const Protobuf::Reflection* reflection = message.GetReflection(); + for (int i = 0; i < descriptor->field_count(); ++i) { + const auto* field = descriptor->field(i); + + // If this field is not in use, continue. + if ((field->is_repeated() && reflection->FieldSize(message, field) == 0) || + (!field->is_repeated() && !reflection->HasField(message, field))) { + continue; + } + + // If this field is deprecated, warn or throw an error. + if (field->options().deprecated()) { std::string err = fmt::format( "Using deprecated option '{}'. This configuration will be removed from Envoy soon. " "Please see https://github.com/envoyproxy/envoy/blob/master/DEPRECATED.md for " "details.", - fd->name()); + field->name()); if (fatal) { throw ProtoValidationException(err, message); } else { ENVOY_LOG_MISC(warn, "{}", err); } } + + // If this is a message, recurse to check for deprecated fields in the sub-message. + switch (field->cpp_type()) { + case Protobuf::FieldDescriptor::CPPTYPE_MESSAGE: { + if (field->is_repeated()) { + const int size = reflection->FieldSize(message, field); + for (int j = 0; j < size; ++j) { + checkForDeprecation(reflection->GetRepeatedMessage(message, field, j), fatal); + } + } else { + checkForDeprecation(reflection->GetMessage(message, field), fatal); + } + break; + } + default: + break; + } } } + /** * Validate protoc-gen-validate constraints on a given protobuf. * Note the corresponding `.pb.validate.h` for the message has to be included in the source file diff --git a/test/common/protobuf/BUILD b/test/common/protobuf/BUILD index 0bffb96b992d2..0b0a3534d225f 100644 --- a/test/common/protobuf/BUILD +++ b/test/common/protobuf/BUILD @@ -16,6 +16,7 @@ envoy_cc_test( "//source/common/protobuf:utility_lib", "//test/proto:deprecated_proto_cc", "//test/test_common:environment_lib", + "//test/test_common:logging_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/config/bootstrap/v2:bootstrap_cc", ], diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 102b0b76e4aaa..3005ff37ed8ce 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -8,6 +8,7 @@ #include "test/proto/deprecated.pb.h" #include "test/test_common/environment.h" +#include "test/test_common/logging.h" #include "test/test_common/utility.h" #include "gtest/gtest.h" @@ -51,8 +52,9 @@ TEST(UtilityTest, ValidateDeprecatedFields) { { envoy::test::deprecation_test::Base base; base.set_is_deprecated("foo"); - // Non-fatal checks for a deprecated field shouldn't throw an exception. - MessageUtil::checkForDeprecation(base, false); + // Non-fatal checks for a deprecated field should log rather than throw an exception. + EXPECT_LOG_CONTAINS("warning", "Using deprecated option 'is_deprecated'.", + MessageUtil::checkForDeprecation(base, false)); // Fatal checks for a deprecated field should result in an exception. EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, true), ProtoValidationException, "Using deprecated option 'is_deprecated'."); @@ -64,6 +66,39 @@ TEST(UtilityTest, ValidateDeprecatedFields) { EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, true), ProtoValidationException, "Using deprecated option 'deprecated_message'."); } + + { + envoy::test::deprecation_test::Base base; + base.mutable_not_deprecated_message()->set_inner_not_deprecated("foo"); + // Non-fatal checks for a deprecated field shouldn't throw an exception. + MessageUtil::checkForDeprecation(base, false); + + base.mutable_not_deprecated_message()->set_inner_deprecated("bar"); + // Fatal checks for a deprecated sub-message should result in an exception. + EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, true), ProtoValidationException, + "Using deprecated option 'inner_deprecated'."); + } + + // Check that repeated sub-messages get validated. + { + envoy::test::deprecation_test::Base base; + base.add_repeated_message(); + base.add_repeated_message()->set_inner_deprecated("foo"); + base.add_repeated_message(); + + // Fatal checks for a repeated deprecated sub-message should result in an exception. + EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, true), ProtoValidationException, + "Using deprecated option 'inner_deprecated'."); + } + // Check that deprecated repeated messages trigger + { + envoy::test::deprecation_test::Base base; + base.add_deprecated_repeated_message(); + + // Fatal checks for a repeated deprecated sub-message should result in an exception. + EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, true), ProtoValidationException, + "Using deprecated option 'deprecated_repeated_message'."); + } } TEST(UtilityTest, LoadBinaryProtoFromFile) { diff --git a/test/proto/deprecated.proto b/test/proto/deprecated.proto index 7f60f99195c16..f6d965ae971c7 100644 --- a/test/proto/deprecated.proto +++ b/test/proto/deprecated.proto @@ -6,7 +6,11 @@ message Base { string not_deprecated = 1; string is_deprecated = 2 [deprecated = true]; message InnerMessage { - string value = 1; + string inner_not_deprecated = 1; + string inner_deprecated = 2 [deprecated = true]; } InnerMessage deprecated_message = 3 [deprecated = true]; + InnerMessage not_deprecated_message = 4; + repeated InnerMessage repeated_message = 5; + repeated InnerMessage deprecated_repeated_message = 6 [deprecated = true]; } From 5c6adc74c346b20eb1bccf3d7ce4a49dafc599f9 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 30 Jan 2019 09:12:12 -0500 Subject: [PATCH 3/5] reviewer comments Signed-off-by: Alyssa Wilk --- docs/root/intro/version_history.rst | 1 + source/common/protobuf/utility.cc | 40 ++++++++++++++++++++ source/common/protobuf/utility.h | 56 +++++----------------------- test/common/protobuf/utility_test.cc | 47 ++++++++++++++++------- 4 files changed, 84 insertions(+), 60 deletions(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 67a870458353f..69c0031498ec6 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -7,6 +7,7 @@ Version history * admin: the admin server can now be accessed via HTTP/2 (prior knowledge). * buffer: fix vulnerabilities when allocation fails. * config: added support of using google.protobuf.Any in opaque configs for extensions. +* config: logging warnings when deprecated fields are in use. * config: removed deprecated --v2-config-only from command line config. * config: removed deprecated_v1 sds_config from :ref:`Bootstrap config `. * config: removed REST_LEGACY as a valid :ref:`ApiType `. diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index a264b37cb51bf..2acbe01650d07 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -107,6 +107,46 @@ void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& messa } } +void MessageUtil::checkForDeprecation(const Protobuf::Message& message, bool warn_only) { + const Protobuf::Descriptor* descriptor = message.GetDescriptor(); + const Protobuf::Reflection* reflection = message.GetReflection(); + for (int i = 0; i < descriptor->field_count(); ++i) { + const auto* field = descriptor->field(i); + + // If this field is not in use, continue. + if ((field->is_repeated() && reflection->FieldSize(message, field) == 0) || + (!field->is_repeated() && !reflection->HasField(message, field))) { + continue; + } + + // If this field is deprecated, warn or throw an error. + if (field->options().deprecated()) { + std::string err = fmt::format( + "Using deprecated option '{}'. This configuration will be removed from Envoy soon. " + "Please see https://github.com/envoyproxy/envoy/blob/master/DEPRECATED.md for " + "details.", + field->full_name()); + if (warn_only) { + ENVOY_LOG_MISC(warn, "{}", err); + } else { + throw ProtoValidationException(err, message); + } + } + + // If this is a message, recurse to check for deprecated fields in the sub-message. + if (field->cpp_type() == Protobuf::FieldDescriptor::CPPTYPE_MESSAGE) { + if (field->is_repeated()) { + const int size = reflection->FieldSize(message, field); + for (int j = 0; j < size; ++j) { + checkForDeprecation(reflection->GetRepeatedMessage(message, field, j), warn_only); + } + } else { + checkForDeprecation(reflection->GetMessage(message, field), warn_only); + } + } + } +} + std::string MessageUtil::getJsonStringFromMessage(const Protobuf::Message& message, const bool pretty_print, const bool always_print_primitive_fields) { diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index 5cfffa946aab6..f88ca26dd113a 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -178,50 +178,14 @@ class MessageUtil { static void loadFromYaml(const std::string& yaml, Protobuf::Message& message); static void loadFromFile(const std::string& path, Protobuf::Message& message); - static void checkForDeprecation(const Protobuf::Message& message, bool fatal) { - const Protobuf::Descriptor* descriptor = message.GetDescriptor(); - const Protobuf::Reflection* reflection = message.GetReflection(); - for (int i = 0; i < descriptor->field_count(); ++i) { - const auto* field = descriptor->field(i); - - // If this field is not in use, continue. - if ((field->is_repeated() && reflection->FieldSize(message, field) == 0) || - (!field->is_repeated() && !reflection->HasField(message, field))) { - continue; - } - - // If this field is deprecated, warn or throw an error. - if (field->options().deprecated()) { - std::string err = fmt::format( - "Using deprecated option '{}'. This configuration will be removed from Envoy soon. " - "Please see https://github.com/envoyproxy/envoy/blob/master/DEPRECATED.md for " - "details.", - field->name()); - if (fatal) { - throw ProtoValidationException(err, message); - } else { - ENVOY_LOG_MISC(warn, "{}", err); - } - } - - // If this is a message, recurse to check for deprecated fields in the sub-message. - switch (field->cpp_type()) { - case Protobuf::FieldDescriptor::CPPTYPE_MESSAGE: { - if (field->is_repeated()) { - const int size = reflection->FieldSize(message, field); - for (int j = 0; j < size; ++j) { - checkForDeprecation(reflection->GetRepeatedMessage(message, field, j), fatal); - } - } else { - checkForDeprecation(reflection->GetMessage(message, field), fatal); - } - break; - } - default: - break; - } - } - } + /** + * Checks for use of deprecated fields in message and all sub-messages. + * @param message message to validate. + * @param warn_only if true, logs a warning rather than throwing an exception if deprecated fields + * are in use. + * @throw ProtoValidationException if deprecated fields are used and warn_only is false. + */ + static void checkForDeprecation(const Protobuf::Message& message, bool warn_only); /** * Validate protoc-gen-validate constraints on a given protobuf. @@ -231,8 +195,8 @@ class MessageUtil { * @throw ProtoValidationException if the message does not satisfy its type constraints. */ template static void validate(const MessageType& message) { - // Do a (non-fatal) deprecation check to log warnings about deprecated field use. - checkForDeprecation(message, false); + // Log warnings if deprecated fields are in use. + checkForDeprecation(message, true); std::string err; if (!Validate(message, &err)) { diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 3005ff37ed8ce..8f4e308c38933 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -42,43 +42,57 @@ TEST(UtilityTest, DowncastAndValidate) { ProtoValidationException); } -TEST(UtilityTest, ValidateDeprecatedFields) { +TEST(DeprecatedFields, NoErrorWhenDeprecatedFieldsUnused) { { envoy::test::deprecation_test::Base base; base.set_not_deprecated("foo"); // Fatal checks for a non-deprecated field should cause no problem. MessageUtil::checkForDeprecation(base, true); } +} + +TEST(DeprecatedFields, IndividualFieldDeprecated) { { envoy::test::deprecation_test::Base base; base.set_is_deprecated("foo"); // Non-fatal checks for a deprecated field should log rather than throw an exception. - EXPECT_LOG_CONTAINS("warning", "Using deprecated option 'is_deprecated'.", - MessageUtil::checkForDeprecation(base, false)); + EXPECT_LOG_CONTAINS("warning", + "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated'.", + MessageUtil::checkForDeprecation(base, true)); // Fatal checks for a deprecated field should result in an exception. - EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, true), ProtoValidationException, - "Using deprecated option 'is_deprecated'."); + EXPECT_THROW_WITH_REGEX( + MessageUtil::checkForDeprecation(base, false), ProtoValidationException, + "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated'."); } +} + +TEST(DeprecatedFields, MessageDeprecated) { { envoy::test::deprecation_test::Base base; base.mutable_deprecated_message(); // Fatal checks for a present (unused) deprecated message should result in an exception. - EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, true), ProtoValidationException, - "Using deprecated option 'deprecated_message'."); + EXPECT_THROW_WITH_REGEX( + MessageUtil::checkForDeprecation(base, false), ProtoValidationException, + "Using deprecated option 'envoy.test.deprecation_test.Base.deprecated_message'."); } +} +TEST(DeprecatedFields, InnerMessageDeprecated) { { envoy::test::deprecation_test::Base base; base.mutable_not_deprecated_message()->set_inner_not_deprecated("foo"); // Non-fatal checks for a deprecated field shouldn't throw an exception. - MessageUtil::checkForDeprecation(base, false); + MessageUtil::checkForDeprecation(base, true); base.mutable_not_deprecated_message()->set_inner_deprecated("bar"); // Fatal checks for a deprecated sub-message should result in an exception. - EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, true), ProtoValidationException, - "Using deprecated option 'inner_deprecated'."); + EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, false), ProtoValidationException, + "Using deprecated option " + "'envoy.test.deprecation_test.Base.InnerMessage.inner_deprecated'."); } +} +TEST(DeprecatedFields, SubMessageDeprecated) { // Check that repeated sub-messages get validated. { envoy::test::deprecation_test::Base base; @@ -87,17 +101,22 @@ TEST(UtilityTest, ValidateDeprecatedFields) { base.add_repeated_message(); // Fatal checks for a repeated deprecated sub-message should result in an exception. - EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, true), ProtoValidationException, - "Using deprecated option 'inner_deprecated'."); + EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, false), ProtoValidationException, + "Using deprecated option " + "'envoy.test.deprecation_test.Base.InnerMessage.inner_deprecated'."); } +} + +TEST(DeprecatedFields, RepeatedMessageDeprecated) { // Check that deprecated repeated messages trigger { envoy::test::deprecation_test::Base base; base.add_deprecated_repeated_message(); // Fatal checks for a repeated deprecated sub-message should result in an exception. - EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, true), ProtoValidationException, - "Using deprecated option 'deprecated_repeated_message'."); + EXPECT_THROW_WITH_REGEX( + MessageUtil::checkForDeprecation(base, false), ProtoValidationException, + "Using deprecated option 'envoy.test.deprecation_test.Base.deprecated_repeated_message'."); } } From ad3bcf049554adb46e759486739f7b387c96b29d Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 31 Jan 2019 09:23:37 -0500 Subject: [PATCH 4/5] reviewer comments Signed-off-by: Alyssa Wilk --- source/common/protobuf/utility.cc | 2 +- source/common/protobuf/utility.h | 2 +- test/common/protobuf/utility_test.cc | 106 ++++++++++++--------------- 3 files changed, 49 insertions(+), 61 deletions(-) diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 404abb2e572c2..a16170702b441 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -126,7 +126,7 @@ void MessageUtil::checkForDeprecation(const Protobuf::Message& message, bool war "details.", field->full_name()); if (warn_only) { - ENVOY_LOG_MISC(warn, "{}", err); + ENVOY_LOG(warn, "{}", err); } else { throw ProtoValidationException(err, message); } diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index ed42a769c8287..b398dab0987d7 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -138,7 +138,7 @@ class ProtoValidationException : public EnvoyException { enum class ProtoUnknownFieldsMode { Strict, Allow }; -class MessageUtil { +class MessageUtil : public Logger::Loggable { public: // std::hash std::size_t operator()(const Protobuf::Message& message) const { return hash(message); } diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index ae1fcf6fb1a31..8a1e1f757db6b 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -341,81 +341,69 @@ TEST(DurationUtilTest, OutOfRange) { } TEST(DeprecatedFields, NoErrorWhenDeprecatedFieldsUnused) { - { - envoy::test::deprecation_test::Base base; - base.set_not_deprecated("foo"); - // Fatal checks for a non-deprecated field should cause no problem. - MessageUtil::checkForDeprecation(base, true); - } + envoy::test::deprecation_test::Base base; + base.set_not_deprecated("foo"); + // Fatal checks for a non-deprecated field should cause no problem. + MessageUtil::checkForDeprecation(base, true); } TEST(DeprecatedFields, IndividualFieldDeprecated) { - { - envoy::test::deprecation_test::Base base; - base.set_is_deprecated("foo"); - // Non-fatal checks for a deprecated field should log rather than throw an exception. - EXPECT_LOG_CONTAINS("warning", - "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated'.", - MessageUtil::checkForDeprecation(base, true)); - // Fatal checks for a deprecated field should result in an exception. - EXPECT_THROW_WITH_REGEX( - MessageUtil::checkForDeprecation(base, false), ProtoValidationException, - "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated'."); - } + envoy::test::deprecation_test::Base base; + base.set_is_deprecated("foo"); + // Non-fatal checks for a deprecated field should log rather than throw an exception. + EXPECT_LOG_CONTAINS("warning", + "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated'.", + MessageUtil::checkForDeprecation(base, true)); + // Fatal checks for a deprecated field should result in an exception. + EXPECT_THROW_WITH_REGEX( + MessageUtil::checkForDeprecation(base, false), ProtoValidationException, + "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated'."); } TEST(DeprecatedFields, MessageDeprecated) { - { - envoy::test::deprecation_test::Base base; - base.mutable_deprecated_message(); - // Fatal checks for a present (unused) deprecated message should result in an exception. - EXPECT_THROW_WITH_REGEX( - MessageUtil::checkForDeprecation(base, false), ProtoValidationException, - "Using deprecated option 'envoy.test.deprecation_test.Base.deprecated_message'."); - } + envoy::test::deprecation_test::Base base; + base.mutable_deprecated_message(); + // Fatal checks for a present (unused) deprecated message should result in an exception. + EXPECT_THROW_WITH_REGEX( + MessageUtil::checkForDeprecation(base, false), ProtoValidationException, + "Using deprecated option 'envoy.test.deprecation_test.Base.deprecated_message'."); } TEST(DeprecatedFields, InnerMessageDeprecated) { - { - envoy::test::deprecation_test::Base base; - base.mutable_not_deprecated_message()->set_inner_not_deprecated("foo"); - // Non-fatal checks for a deprecated field shouldn't throw an exception. - MessageUtil::checkForDeprecation(base, true); - - base.mutable_not_deprecated_message()->set_inner_deprecated("bar"); - // Fatal checks for a deprecated sub-message should result in an exception. - EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, false), ProtoValidationException, - "Using deprecated option " - "'envoy.test.deprecation_test.Base.InnerMessage.inner_deprecated'."); - } + envoy::test::deprecation_test::Base base; + base.mutable_not_deprecated_message()->set_inner_not_deprecated("foo"); + // Non-fatal checks for a deprecated field shouldn't throw an exception. + MessageUtil::checkForDeprecation(base, true); + + base.mutable_not_deprecated_message()->set_inner_deprecated("bar"); + // Fatal checks for a deprecated sub-message should result in an exception. + EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, false), ProtoValidationException, + "Using deprecated option " + "'envoy.test.deprecation_test.Base.InnerMessage.inner_deprecated'."); } +// Check that repeated sub-messages get validated. TEST(DeprecatedFields, SubMessageDeprecated) { - // Check that repeated sub-messages get validated. - { - envoy::test::deprecation_test::Base base; - base.add_repeated_message(); - base.add_repeated_message()->set_inner_deprecated("foo"); - base.add_repeated_message(); - - // Fatal checks for a repeated deprecated sub-message should result in an exception. - EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, false), ProtoValidationException, - "Using deprecated option " - "'envoy.test.deprecation_test.Base.InnerMessage.inner_deprecated'."); - } + envoy::test::deprecation_test::Base base; + base.add_repeated_message(); + base.add_repeated_message()->set_inner_deprecated("foo"); + base.add_repeated_message(); + + // Fatal checks for a repeated deprecated sub-message should result in an exception. + EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, false), ProtoValidationException, + "Using deprecated option " + "'envoy.test.deprecation_test.Base.InnerMessage.inner_deprecated'."); } +// Check that deprecated repeated messages trigger TEST(DeprecatedFields, RepeatedMessageDeprecated) { - // Check that deprecated repeated messages trigger - { - envoy::test::deprecation_test::Base base; - base.add_deprecated_repeated_message(); + envoy::test::deprecation_test::Base base; + base.add_deprecated_repeated_message(); - // Fatal checks for a repeated deprecated sub-message should result in an exception. - EXPECT_THROW_WITH_REGEX( - MessageUtil::checkForDeprecation(base, false), ProtoValidationException, - "Using deprecated option 'envoy.test.deprecation_test.Base.deprecated_repeated_message'."); - } + // Fatal checks for a repeated deprecated sub-message should result in an exception. + EXPECT_THROW_WITH_REGEX( + MessageUtil::checkForDeprecation(base, false), ProtoValidationException, + "Using deprecated option 'envoy.test.deprecation_test.Base.deprecated_repeated_message'."); } class TimestampUtilTest : public ::testing::Test, public ::testing::WithParamInterface {}; From 17dfc4796fcc1041ea060ab5edc3d0c2d28abf5e Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 31 Jan 2019 16:18:37 -0500 Subject: [PATCH 5/5] Backing out logging change in the hopes it's not-merge-blocking issue Signed-off-by: Alyssa Wilk --- source/common/protobuf/utility.cc | 2 +- source/common/protobuf/utility.h | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index a16170702b441..404abb2e572c2 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -126,7 +126,7 @@ void MessageUtil::checkForDeprecation(const Protobuf::Message& message, bool war "details.", field->full_name()); if (warn_only) { - ENVOY_LOG(warn, "{}", err); + ENVOY_LOG_MISC(warn, "{}", err); } else { throw ProtoValidationException(err, message); } diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index b398dab0987d7..7496f4aa69edd 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -7,7 +7,6 @@ #include "envoy/json/json_object.h" #include "envoy/type/percent.pb.h" -#include "common/common/fmt.h" #include "common/common/hash.h" #include "common/common/utility.h" #include "common/json/json_loader.h" @@ -138,7 +137,7 @@ class ProtoValidationException : public EnvoyException { enum class ProtoUnknownFieldsMode { Strict, Allow }; -class MessageUtil : public Logger::Loggable { +class MessageUtil { public: // std::hash std::size_t operator()(const Protobuf::Message& message) const { return hash(message); }