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 <nezdolik@spotify.com>

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",
+    ],
 )

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 {

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",

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);
 }

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<Logger::Id::config> {
 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<uint64_t> 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();

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;
   }

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<Stats::HistogramCompletableTimespanImpl>(

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;

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.

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(

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",
+    ],
 )

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_; }
 

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<ListenerManagerImpl>(server_, listener_factory_, worker_factory_,
                                                      enable_dispatcher_stats_);
 
@@ -276,6 +280,7 @@ class ListenerManagerImplTest : public testing::Test {
   Api::OsSysCallsImpl os_sys_calls_actual_;
   NiceMock<MockInstance> server_;
   NiceMock<MockListenerComponentFactory> listener_factory_;
+  NiceMock<ProtobufMessage::MockValidationVisitor> validation_visitor;
   MockWorker* worker_ = new MockWorker();
   NiceMock<MockWorkerFactory> worker_factory_;
   std::unique_ptr<ListenerManagerImpl> manager_;