diff --git a/include/envoy/protobuf/BUILD b/include/envoy/protobuf/BUILD index 6510c566d103..76eff507352d 100644 --- a/include/envoy/protobuf/BUILD +++ b/include/envoy/protobuf/BUILD @@ -11,5 +11,7 @@ envoy_package() envoy_cc_library( name = "message_validator_interface", hdrs = ["message_validator.h"], - deps = ["//source/common/protobuf"], + deps = [ + "//source/common/protobuf", + ], ) diff --git a/include/envoy/protobuf/message_validator.h b/include/envoy/protobuf/message_validator.h index ddd6d14d3da2..8ec4dccb46e3 100644 --- a/include/envoy/protobuf/message_validator.h +++ b/include/envoy/protobuf/message_validator.h @@ -12,13 +12,22 @@ namespace ProtobufMessage { /** * Exception class for reporting validation errors due to the presence of unknown - * fields in a protobuf + * fields in a protobuf. */ class UnknownProtoFieldException : public EnvoyException { public: UnknownProtoFieldException(const std::string& message) : EnvoyException(message) {} }; +/** + * Exception class for reporting validation errors due to the presence of deprecated + * fields in a protobuf. + */ +class DeprecatedProtoFieldException : public EnvoyException { +public: + DeprecatedProtoFieldException(const std::string& message) : EnvoyException(message) {} +}; + /** * Visitor interface for a Protobuf::Message. The methods of ValidationVisitor are invoked to * perform validation based on events encountered during or after the parsing of proto binary @@ -30,7 +39,7 @@ class ValidationVisitor { /** * Invoked when an unknown field is encountered. - * @param description human readable description of the field + * @param description human readable description of the field. */ virtual void onUnknownField(absl::string_view description) PURE; @@ -39,6 +48,14 @@ class ValidationVisitor { * possible. **/ virtual bool skipValidation() PURE; + + /** + * Invoked when deprecated field is encountered. + * @param description human readable description of the field. + * @param soft_deprecation is set to true, visitor would log a warning message, otherwise would + * throw an exception. + */ + virtual void onDeprecatedField(absl::string_view description, bool soft_deprecation) PURE; }; class ValidationContext { diff --git a/source/common/protobuf/BUILD b/source/common/protobuf/BUILD index 20ead1f753c6..f505161b810f 100644 --- a/source/common/protobuf/BUILD +++ b/source/common/protobuf/BUILD @@ -34,6 +34,7 @@ envoy_cc_library( deps = [ "//include/envoy/protobuf:message_validator_interface", "//include/envoy/stats:stats_interface", + "//source/common/common:documentation_url_lib", "//source/common/common:hash_lib", "//source/common/common:logger_lib", "//source/common/common:macros", diff --git a/source/common/protobuf/message_validator_impl.cc b/source/common/protobuf/message_validator_impl.cc index 9b164a925da5..c486f9d0d4ec 100644 --- a/source/common/protobuf/message_validator_impl.cc +++ b/source/common/protobuf/message_validator_impl.cc @@ -11,10 +11,24 @@ namespace Envoy { namespace ProtobufMessage { -void WarningValidationVisitorImpl::setCounter(Stats::Counter& counter) { - ASSERT(counter_ == nullptr); - counter_ = &counter; - counter.add(prestats_count_); +namespace { +const char deprecation_error[] = " If continued use of this field is absolutely necessary, " + "see " ENVOY_DOC_URL_RUNTIME_OVERRIDE_DEPRECATED " for " + "how to apply a temporary and highly discouraged override."; + +void onDeprecatedFieldCommon(absl::string_view description, bool soft_deprecation) { + if (soft_deprecation) { + ENVOY_LOG_MISC(warn, "Deprecated field: {}", absl::StrCat(description, deprecation_error)); + } else { + throw DeprecatedProtoFieldException(absl::StrCat(description, deprecation_error)); + } +} +} // namespace + +void WarningValidationVisitorImpl::setUnknownCounter(Stats::Counter& counter) { + ASSERT(unknown_counter_ == nullptr); + unknown_counter_ = &counter; + counter.add(prestats_unknown_count_); } void WarningValidationVisitorImpl::onUnknownField(absl::string_view description) { @@ -24,20 +38,31 @@ void WarningValidationVisitorImpl::onUnknownField(absl::string_view description) if (!it.second) { return; } + // It's a new field, log and bump stat. ENVOY_LOG(warn, "Unknown field: {}", description); - if (counter_ == nullptr) { - ++prestats_count_; + if (unknown_counter_ == nullptr) { + ++prestats_unknown_count_; } else { - counter_->inc(); + unknown_counter_->inc(); } } +void WarningValidationVisitorImpl::onDeprecatedField(absl::string_view description, + bool soft_deprecation) { + onDeprecatedFieldCommon(description, soft_deprecation); +} + void StrictValidationVisitorImpl::onUnknownField(absl::string_view description) { throw UnknownProtoFieldException( absl::StrCat("Protobuf message (", description, ") has unknown fields")); } +void StrictValidationVisitorImpl::onDeprecatedField(absl::string_view description, + bool soft_deprecation) { + onDeprecatedFieldCommon(description, soft_deprecation); +} + ValidationVisitor& getNullValidationVisitor() { MUTABLE_CONSTRUCT_ON_FIRST_USE(NullValidationVisitorImpl); } diff --git a/source/common/protobuf/message_validator_impl.h b/source/common/protobuf/message_validator_impl.h index 0ba98161ec5d..e2f49ce9dec7 100644 --- a/source/common/protobuf/message_validator_impl.h +++ b/source/common/protobuf/message_validator_impl.h @@ -3,6 +3,7 @@ #include "envoy/protobuf/message_validator.h" #include "envoy/stats/stats.h" +#include "common/common/documentation_url.h" #include "common/common/logger.h" #include "absl/container/flat_hash_set.h" @@ -14,6 +15,7 @@ class NullValidationVisitorImpl : public ValidationVisitor { public: // Envoy::ProtobufMessage::ValidationVisitor void onUnknownField(absl::string_view) override {} + void onDeprecatedField(absl::string_view, bool) override {} // Envoy::ProtobufMessage::ValidationVisitor bool skipValidation() override { return true; } @@ -24,10 +26,11 @@ ValidationVisitor& getNullValidationVisitor(); class WarningValidationVisitorImpl : public ValidationVisitor, public Logger::Loggable { public: - void setCounter(Stats::Counter& counter); + void setUnknownCounter(Stats::Counter& counter); // Envoy::ProtobufMessage::ValidationVisitor void onUnknownField(absl::string_view description) override; + void onDeprecatedField(absl::string_view description, bool soft_deprecation) override; // Envoy::ProtobufMessage::ValidationVisitor bool skipValidation() override { return false; } @@ -36,10 +39,10 @@ class WarningValidationVisitorImpl : public ValidationVisitor, // Track hashes of descriptions we've seen, to avoid log spam. A hash is used here to avoid // wasting memory with unused strings. absl::flat_hash_set descriptions_; - // This can be late initialized via setCounter(), enabling the server bootstrap loading which - // occurs prior to the initialization of the stats subsystem. - Stats::Counter* counter_{}; - uint64_t prestats_count_{}; + // This can be late initialized via setUnknownCounter(), enabling the server bootstrap loading + // which occurs prior to the initialization of the stats subsystem. + Stats::Counter* unknown_counter_{}; + uint64_t prestats_unknown_count_{}; }; class StrictValidationVisitorImpl : public ValidationVisitor { @@ -49,6 +52,7 @@ class StrictValidationVisitorImpl : public ValidationVisitor { // Envoy::ProtobufMessage::ValidationVisitor bool skipValidation() override { return false; } + void onDeprecatedField(absl::string_view description, bool soft_deprecation) override; }; ValidationVisitor& getStrictValidationVisitor(); diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 048da4098382..8bd7ed67c41e 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -165,7 +165,8 @@ void tryWithApiBoosting(MessageXformFn f, Protobuf::Message& message) { // otherwise fatal field. Throws a warning on use of a fatal by default field. void deprecatedFieldHelper(Runtime::Loader* runtime, bool proto_annotated_as_deprecated, bool proto_annotated_as_disallowed, const std::string& feature_name, - std::string error, const Protobuf::Message& message) { + std::string error, const Protobuf::Message& message, + ProtobufMessage::ValidationVisitor& validation_visitor) { // This option is for Envoy builds with --define deprecated_features=disabled // The build options CI then verifies that as Envoy developers deprecate fields, // that they update canonical configs and unit tests to not use those deprecated @@ -196,14 +197,9 @@ void deprecatedFieldHelper(Runtime::Loader* runtime, bool proto_annotated_as_dep std::string with_overridden = fmt::format( error, (runtime_overridden ? "runtime overrides to continue using now fatal-by-default " : "")); - if (warn_only) { - ENVOY_LOG_MISC(warn, "{}", with_overridden); - } else { - const char fatal_error[] = " If continued use of this field is absolutely necessary, " - "see " ENVOY_DOC_URL_RUNTIME_OVERRIDE_DEPRECATED " for how " - "to apply a temporary and highly discouraged override."; - throw ProtoValidationException(with_overridden + fatal_error, message); - } + + validation_visitor.onDeprecatedField("type " + message.GetTypeName() + " " + with_overridden, + warn_only); } } // namespace @@ -386,11 +382,10 @@ void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& messa namespace { -void checkForDeprecatedNonRepeatedEnumValue(const Protobuf::Message& message, - absl::string_view filename, - const Protobuf::FieldDescriptor* field, - const Protobuf::Reflection* reflection, - Runtime::Loader* runtime) { +void checkForDeprecatedNonRepeatedEnumValue( + const Protobuf::Message& message, absl::string_view filename, + const Protobuf::FieldDescriptor* field, const Protobuf::Reflection* reflection, + Runtime::Loader* runtime, ProtobufMessage::ValidationVisitor& validation_visitor) { // Repeated fields will be handled by recursion in checkForUnexpectedFields. if (field->is_repeated() || field->cpp_type() != Protobuf::FieldDescriptor::CPPTYPE_ENUM) { return; @@ -413,7 +408,7 @@ void checkForDeprecatedNonRepeatedEnumValue(const Protobuf::Message& message, runtime, true /*deprecated*/, enum_value_descriptor->options().GetExtension(envoy::annotations::disallowed_by_default_enum), absl::StrCat("envoy.deprecated_features:", enum_value_descriptor->full_name()), error, - message); + message, validation_visitor); } class UnexpectedFieldProtoVisitor : public ProtobufMessage::ConstProtoVisitor { @@ -429,7 +424,8 @@ class UnexpectedFieldProtoVisitor : public ProtobufMessage::ConstProtoVisitor { // Before we check to see if the field is in use, see if there's a // deprecated default enum value. - checkForDeprecatedNonRepeatedEnumValue(message, filename, &field, reflection, runtime_); + checkForDeprecatedNonRepeatedEnumValue(message, filename, &field, reflection, runtime_, + validation_visitor_); // If this field is not in use, continue. if ((field.is_repeated() && reflection->FieldSize(message, &field) == 0) || @@ -447,7 +443,7 @@ class UnexpectedFieldProtoVisitor : public ProtobufMessage::ConstProtoVisitor { deprecatedFieldHelper(runtime_, true /*deprecated*/, field.options().GetExtension(envoy::annotations::disallowed_by_default), absl::StrCat("envoy.deprecated_features:", field.full_name()), warning, - message); + message, validation_visitor_); } return nullptr; } diff --git a/source/server/server.cc b/source/server/server.cc index c3f0c35e683f..246aac405ffe 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -320,9 +320,9 @@ void InstanceImpl::initialize(const Options& options, ServerStats{ALL_SERVER_STATS(POOL_COUNTER_PREFIX(stats_store_, server_stats_prefix), POOL_GAUGE_PREFIX(stats_store_, server_stats_prefix), POOL_HISTOGRAM_PREFIX(stats_store_, server_stats_prefix))}); - validation_context_.static_warning_validation_visitor().setCounter( + validation_context_.static_warning_validation_visitor().setUnknownCounter( server_stats_->static_unknown_fields_); - validation_context_.dynamic_warning_validation_visitor().setCounter( + validation_context_.dynamic_warning_validation_visitor().setUnknownCounter( server_stats_->dynamic_unknown_fields_); initialization_timer_ = std::make_unique( diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index 61e496f85c54..37079704bb85 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -490,6 +490,9 @@ DEFINE_PROTO_FUZZER(const test::common::http::ConnManagerImplTestCase& input) { } catch (const ProtoValidationException& e) { ENVOY_LOG_MISC(debug, "ProtoValidationException: {}", e.what()); return; + } catch (const Envoy::ProtobufMessage::DeprecatedProtoFieldException& e) { + ENVOY_LOG_MISC(debug, "DeprecatedProtoFieldException: {}", e.what()); + return; } FuzzConfig config; diff --git a/test/common/protobuf/message_validator_impl_test.cc b/test/common/protobuf/message_validator_impl_test.cc index 7110a1432537..d558799cc440 100644 --- a/test/common/protobuf/message_validator_impl_test.cc +++ b/test/common/protobuf/message_validator_impl_test.cc @@ -23,7 +23,7 @@ TEST(NullValidationVisitorImpl, UnknownField) { // The warning validation visitor logs and bumps stats on unknown fields TEST(WarningValidationVisitorImpl, UnknownField) { Stats::TestUtil::TestStore stats; - Stats::Counter& counter = stats.counter("counter"); + Stats::Counter& unknown_counter = stats.counter("counter"); WarningValidationVisitorImpl warning_validation_visitor; // we want to be executed. EXPECT_FALSE(warning_validation_visitor.skipValidation()); @@ -37,13 +37,13 @@ TEST(WarningValidationVisitorImpl, UnknownField) { EXPECT_LOG_CONTAINS("warn", "Unknown field: bar", warning_validation_visitor.onUnknownField("bar")); // When we set the stats counter, the above increments are transferred. - EXPECT_EQ(0, counter.value()); - warning_validation_visitor.setCounter(counter); - EXPECT_EQ(2, counter.value()); + EXPECT_EQ(0, unknown_counter.value()); + warning_validation_visitor.setUnknownCounter(unknown_counter); + EXPECT_EQ(2, unknown_counter.value()); // A third unknown field is tracked in stats post-initialization. EXPECT_LOG_CONTAINS("warn", "Unknown field: baz", warning_validation_visitor.onUnknownField("baz")); - EXPECT_EQ(3, counter.value()); + EXPECT_EQ(3, unknown_counter.value()); } // The strict validation visitor throws on unknown fields. diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index ae832053dcab..f7cee00ca4a4 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -1498,7 +1498,7 @@ TEST_P(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(IndividualFieldDisallowed)) envoy::test::deprecation_test::Base base; base.set_is_deprecated_fatal("foo"); EXPECT_THROW_WITH_REGEX( - checkForDeprecation(base), ProtoValidationException, + checkForDeprecation(base), Envoy::ProtobufMessage::DeprecatedProtoFieldException, "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated_fatal'"); } @@ -1509,7 +1509,7 @@ TEST_P(DeprecatedFieldsTest, // Make sure this is set up right. EXPECT_THROW_WITH_REGEX( - checkForDeprecation(base), ProtoValidationException, + checkForDeprecation(base), Envoy::ProtobufMessage::DeprecatedProtoFieldException, "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated_fatal'"); // The config will be rejected, so the feature will not be used. EXPECT_EQ(0, runtime_deprecated_feature_use_.value()); @@ -1542,7 +1542,7 @@ TEST_P(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(DisallowViaRuntime)) { {{"envoy.deprecated_features:envoy.test.deprecation_test.Base.is_deprecated", " false"}}); EXPECT_THROW_WITH_REGEX( - checkForDeprecation(base), ProtoValidationException, + checkForDeprecation(base), Envoy::ProtobufMessage::DeprecatedProtoFieldException, "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated'"); EXPECT_EQ(1, runtime_deprecated_feature_use_.value()); } @@ -1557,7 +1557,7 @@ TEST_P(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(MixOfFatalAndWarnings)) { EXPECT_LOG_CONTAINS( "warning", "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated'", { EXPECT_THROW_WITH_REGEX( - checkForDeprecation(base), ProtoValidationException, + checkForDeprecation(base), Envoy::ProtobufMessage::DeprecatedProtoFieldException, "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated_fatal'"); }); } @@ -1650,7 +1650,8 @@ TEST_P(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(RuntimeOverrideEnumDefault) {{"envoy.deprecated_features:envoy.test.deprecation_test.Base.DEPRECATED_DEFAULT", "false"}}); // Make sure this is set up right. - EXPECT_THROW_WITH_REGEX(checkForDeprecation(base), ProtoValidationException, + EXPECT_THROW_WITH_REGEX(checkForDeprecation(base), + Envoy::ProtobufMessage::DeprecatedProtoFieldException, "Using the default now-deprecated value DEPRECATED_DEFAULT"); } @@ -1659,7 +1660,8 @@ TEST_P(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(FatalEnum)) { envoy::test::deprecation_test::Base base; base.mutable_enum_container()->set_deprecated_enum( envoy::test::deprecation_test::Base::DEPRECATED_FATAL); - EXPECT_THROW_WITH_REGEX(checkForDeprecation(base), ProtoValidationException, + EXPECT_THROW_WITH_REGEX(checkForDeprecation(base), + Envoy::ProtobufMessage::DeprecatedProtoFieldException, "Using deprecated value DEPRECATED_FATAL"); Runtime::LoaderSingleton::getExisting()->mergeValues( diff --git a/test/mocks/protobuf/BUILD b/test/mocks/protobuf/BUILD index 2db80c5e3173..67b4c15cd644 100644 --- a/test/mocks/protobuf/BUILD +++ b/test/mocks/protobuf/BUILD @@ -12,5 +12,7 @@ envoy_cc_mock( name = "protobuf_mocks", srcs = ["mocks.cc"], hdrs = ["mocks.h"], - deps = ["//include/envoy/protobuf:message_validator_interface"], + deps = [ + "//include/envoy/protobuf:message_validator_interface", + ], ) diff --git a/test/mocks/protobuf/mocks.h b/test/mocks/protobuf/mocks.h index 5170f1ba1228..3e61b31fed12 100644 --- a/test/mocks/protobuf/mocks.h +++ b/test/mocks/protobuf/mocks.h @@ -13,6 +13,7 @@ class MockValidationVisitor : public ValidationVisitor { ~MockValidationVisitor() override; MOCK_METHOD(void, onUnknownField, (absl::string_view)); + MOCK_METHOD(void, onDeprecatedField, (absl::string_view, bool)); bool skipValidation() override { return skip_validation_; } diff --git a/test/server/listener_manager_impl_test.h b/test/server/listener_manager_impl_test.h index 0790b8faa4f7..bd979c98ed14 100644 --- a/test/server/listener_manager_impl_test.h +++ b/test/server/listener_manager_impl_test.h @@ -53,6 +53,10 @@ class ListenerManagerImplTest : public testing::Test { void SetUp() override { ON_CALL(server_, api()).WillByDefault(ReturnRef(*api_)); EXPECT_CALL(worker_factory_, createWorker_()).WillOnce(Return(worker_)); + ON_CALL(server_.validation_context_, staticValidationVisitor()) + .WillByDefault(ReturnRef(validation_visitor)); + ON_CALL(server_.validation_context_, dynamicValidationVisitor()) + .WillByDefault(ReturnRef(validation_visitor)); manager_ = std::make_unique(server_, listener_factory_, worker_factory_, enable_dispatcher_stats_); @@ -276,6 +280,7 @@ class ListenerManagerImplTest : public testing::Test { Api::OsSysCallsImpl os_sys_calls_actual_; NiceMock server_; NiceMock listener_factory_; + NiceMock validation_visitor; MockWorker* worker_ = new MockWorker(); NiceMock worker_factory_; std::unique_ptr manager_;