diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index cc3cb90f95107..e07bd5b2abe46 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 6c372fc915041..404abb2e572c2 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -106,6 +106,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 b7b804f45c5f8..7496f4aa69edd 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -178,6 +178,15 @@ class MessageUtil { static void loadFromYaml(const std::string& yaml, Protobuf::Message& message); static void loadFromFile(const std::string& path, Protobuf::Message& message, Api::Api& api); + /** + * 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. * Note the corresponding `.pb.validate.h` for the message has to be included in the source file @@ -186,6 +195,9 @@ class MessageUtil { * @throw ProtoValidationException if the message does not satisfy its type constraints. */ template static void validate(const MessageType& message) { + // Log warnings if deprecated fields are in use. + checkForDeprecation(message, true); + std::string err; if (!Validate(message, &err)) { throw ProtoValidationException(err, message); diff --git a/test/common/protobuf/BUILD b/test/common/protobuf/BUILD index 9a82a8b547896..54e44139d97c7 100644 --- a/test/common/protobuf/BUILD +++ b/test/common/protobuf/BUILD @@ -15,7 +15,9 @@ envoy_cc_test( deps = [ "//source/common/protobuf:utility_lib", "//source/common/stats:isolated_store_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 58d4c00d2d12c..1352ecd17910b 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -7,7 +7,9 @@ #include "common/protobuf/utility.h" #include "common/stats/isolated_store_impl.h" +#include "test/proto/deprecated.pb.h" #include "test/test_common/environment.h" +#include "test/test_common/logging.h" #include "test/test_common/test_base.h" #include "test/test_common/utility.h" @@ -337,6 +339,72 @@ 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); +} + +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'."); +} + +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'."); +} + +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'."); +} + +// Check that repeated sub-messages get validated. +TEST(DeprecatedFields, SubMessageDeprecated) { + 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) { + 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'."); +} + class TimestampUtilTest : public TestBase, public ::testing::WithParamInterface {}; TEST_P(TimestampUtilTest, SystemClockToTimestampTest) { 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..f6d965ae971c7 --- /dev/null +++ b/test/proto/deprecated.proto @@ -0,0 +1,16 @@ +syntax = "proto3"; + +package envoy.test.deprecation_test; + +message Base { + string not_deprecated = 1; + string is_deprecated = 2 [deprecated = true]; + message InnerMessage { + 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]; +}