From 473de43c33e47251a6148da1333a207d985ad74e Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Fri, 5 Jun 2020 16:34:20 +0200 Subject: [PATCH] Validate deprecated fields via validation visitor (#10853) This change intends to unify field deprecation and unknown field handling behind the validation visitor. So far validation of unknown fields was done through validation visitor and validation of deprecated fields was performed inline. Risk Level:Low Fixes #8092 Signed-off-by: Kateryna Nezdolii Signed-off-by: yashwant121 --- include/envoy/protobuf/BUILD | 4 +- include/envoy/protobuf/message_validator.h | 21 +++++++++- source/common/protobuf/BUILD | 1 + .../common/protobuf/message_validator_impl.cc | 39 +++++++++++++++---- .../common/protobuf/message_validator_impl.h | 14 ++++--- source/common/protobuf/utility.cc | 30 +++++++------- source/server/server.cc | 4 +- .../http/conn_manager_impl_fuzz_test.cc | 3 ++ .../protobuf/message_validator_impl_test.cc | 10 ++--- test/common/protobuf/utility_test.cc | 14 ++++--- test/mocks/protobuf/BUILD | 4 +- test/mocks/protobuf/mocks.h | 1 + test/server/listener_manager_impl_test.h | 5 +++ 13 files changed, 104 insertions(+), 46 deletions(-) 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_;