Validate deprecated fields via validation visitor

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

@@ -33,6 +33,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,10 @@
 namespace Envoy {
 namespace ProtobufMessage {
 
-void WarningValidationVisitorImpl::setCounter(Stats::Counter& counter) {
-  ASSERT(counter_ == nullptr);
-  counter_ = &counter;
-  counter.add(prestats_count_);
+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,12 +24,22 @@ 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_;
+  ENVOY_LOG(warn, "Unexpected field: {}", description);
+  if (unknown_counter_ == nullptr) {
+    ++prestats_unknown_count_;
+  } else {
+    unknown_counter_->inc();
+  }
+}
+
+void WarningValidationVisitorImpl::onDeprecatedField(absl::string_view description,
+                                                     bool soft_deprecation) {
+  if (soft_deprecation) {
+    ENVOY_LOG_MISC(warn, "Unexpected field: {}", absl::StrCat(description, deprecation_error));
   } else {
-    counter_->inc();
+    throw DeprecatedProtoFieldException(absl::StrCat(description, deprecation_error));
   }
 }
 
@@ -38,6 +48,15 @@ void StrictValidationVisitorImpl::onUnknownField(absl::string_view description)
       absl::StrCat("Protobuf message (", description, ") has unknown fields"));
 }
 
+void StrictValidationVisitorImpl::onDeprecatedField(absl::string_view description,
+                                                    bool soft_deprecation) {
+  if (soft_deprecation) {
+    ENVOY_LOG_MISC(warn, "Unexpected field: {}", absl::StrCat(description, deprecation_error));
+  } else {
+    throw DeprecatedProtoFieldException(absl::StrCat(description, deprecation_error));
+  }
+}
+
 ValidationVisitor& getNullValidationVisitor() {
   MUTABLE_CONSTRUCT_ON_FIRST_USE(NullValidationVisitorImpl);
 }

source/common/protobuf/message_validator_impl.h

@@ -3,17 +3,23 @@
 #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"
 
 namespace Envoy {
 namespace ProtobufMessage {
 
+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.";
+
 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 +30,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 +43,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 +56,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
@@ -385,11 +381,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;
@@ -412,7 +407,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 {
@@ -428,7 +423,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) ||
@@ -446,7 +442,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

@@ -319,9 +319,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

@@ -486,6 +486,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,27 +23,27 @@ 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());
   // First time around we should log.
-  EXPECT_LOG_CONTAINS("warn", "Unknown field: foo",
+  EXPECT_LOG_CONTAINS("warn", "Unexpected field: foo",
                       warning_validation_visitor.onUnknownField("foo"));
   // Duplicate descriptions don't generate a log the second time around.
-  EXPECT_LOG_NOT_CONTAINS("warn", "Unknown field: foo",
+  EXPECT_LOG_NOT_CONTAINS("warn", "Unexpected field: foo",
                           warning_validation_visitor.onUnknownField("foo"));
   // Unrelated variable increments.
-  EXPECT_LOG_CONTAINS("warn", "Unknown field: bar",
+  EXPECT_LOG_CONTAINS("warn", "Unexpected 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",
+  EXPECT_LOG_CONTAINS("warn", "Unexpected 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

@@ -1468,7 +1468,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'");
 }
 
@@ -1479,7 +1479,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());
@@ -1512,7 +1512,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());
 }
@@ -1527,7 +1527,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'");
       });
 }
@@ -1620,7 +1620,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");
 }
 
@@ -1629,7 +1630,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_; }