Validate deprecated fields via validation visitor

include/envoy/protobuf/message_validator.h

@@ -10,15 +10,38 @@
 namespace Envoy {
 namespace ProtobufMessage {
 
+namespace ValidationError {
+const char deprecation_error[] =
+    " If continued use of this field is absolutely necessary, see "
+    "https://www.envoyproxy.io/docs/envoy/latest/configuration/operations/runtime"
+    "#using-runtime-overrides-for-deprecated-features for how to apply a temporary and "
+    "highly discouraged override.";
+
 /**
  * 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) {}
+};
+
+} // namespace ValidationError
+
+enum ValidationType {
+  UnknownFields,
+  DeprecatedFields,
+};
+
 /**
  * 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
@@ -33,6 +56,12 @@ class ValidationVisitor {
    * @param description human readable description of the field
    */
   virtual void onUnknownField(absl::string_view description) PURE;
+
+  /**
+   * Invoked when deprecated field is encountered.
+   * @param description human readable description of the field
+   */
+  virtual void onDeprecatedField(absl::string_view description) PURE;
 };
 
 class ValidationContext {

source/common/config/filesystem_subscription_impl.cc

@@ -56,7 +56,7 @@ void FilesystemSubscriptionImpl::refresh() {
     stats_.version_.set(HashUtil::xxHash64(message.version_info()));
     stats_.update_success_.inc();
     ENVOY_LOG(debug, "Filesystem config update accepted for {}: {}", path_, message.DebugString());
-  } catch (const ProtobufMessage::UnknownProtoFieldException& e) {
+  } catch (const ProtobufMessage::ValidationError::UnknownProtoFieldException& e) {
     configRejected(e, message.DebugString());
   } catch (const EnvoyException& e) {
     if (config_update_available) {

source/common/protobuf/message_validator_impl.cc

@@ -11,33 +11,64 @@
 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::setDeprecatedCounter(Stats::Counter& counter) {
+  ASSERT(deprecated_counter_ == nullptr);
+  deprecated_counter_ = &counter;
+  counter.add(prestats_deprecated_count_);
 }
 
 void WarningValidationVisitorImpl::onUnknownField(absl::string_view description) {
+  onUnexpectedField(description, unknown_counter_, ValidationType::UnknownFields);
+}
+
+void WarningValidationVisitorImpl::onDeprecatedField(absl::string_view description) {
+  std::string message = absl::StrCat(description, ValidationError::deprecation_error);
+  onUnexpectedField(description, deprecated_counter_, ValidationType::DeprecatedFields);
+  throw ValidationError::DeprecatedProtoFieldException(
+      absl::StrCat(description, ValidationError::deprecation_error));
+}
+
+void WarningValidationVisitorImpl::onUnexpectedField(absl::string_view description,
+                                                     Stats::Counter* counter,
+                                                     const ValidationType& validation_type) {
   const uint64_t hash = HashUtil::xxHash64(description);
   auto it = descriptions_.insert(hash);
   // If we've seen this before, skip.
   if (!it.second) {
     return;
   }
-  // It's a new field, log and bump stat.
-  ENVOY_LOG(warn, "Unknown field: {}", description);
-  if (counter_ == nullptr) {
-    ++prestats_count_;
+  // It's a new/deprecated field, log and bump stat.
+  ENVOY_LOG(warn, "Unexpected field: {}", description);
+  if (counter != nullptr) {
+    counter->inc();
   } else {
-    counter_->inc();
+    switch (validation_type) {
+    case UnknownFields:
+      ++prestats_unknown_count_;
+      break;
+    case DeprecatedFields:
+      ++prestats_deprecated_count_;
+      break;
+    }
   }
 }
 
 void StrictValidationVisitorImpl::onUnknownField(absl::string_view description) {
-  throw UnknownProtoFieldException(
+  throw ValidationError::UnknownProtoFieldException(
       absl::StrCat("Protobuf message (", description, ") has unknown fields"));
 }
 
+void StrictValidationVisitorImpl::onDeprecatedField(absl::string_view description) {
+  throw ValidationError::DeprecatedProtoFieldException(
+      absl::StrCat(description, ValidationError::deprecation_error));
+}
+
 ValidationVisitor& getNullValidationVisitor() {
   MUTABLE_CONSTRUCT_ON_FIRST_USE(NullValidationVisitorImpl);
 }

source/common/protobuf/message_validator_impl.h

@@ -14,32 +14,42 @@ class NullValidationVisitorImpl : public ValidationVisitor {
 public:
   // Envoy::ProtobufMessage::ValidationVisitor
   void onUnknownField(absl::string_view) override {}
+  void onDeprecatedField(absl::string_view) override {}
 };
 
 ValidationVisitor& getNullValidationVisitor();
 
 class WarningValidationVisitorImpl : public ValidationVisitor,
                                      public Logger::Loggable<Logger::Id::config> {
 public:
-  void setCounter(Stats::Counter& counter);
+  void setUnknownCounter(Stats::Counter& counter);
+  void setDeprecatedCounter(Stats::Counter& counter);
 
   // Envoy::ProtobufMessage::ValidationVisitor
   void onUnknownField(absl::string_view description) override;
+  void onDeprecatedField(absl::string_view description) override;
 
 private:
+  void onUnexpectedField(absl::string_view description, Stats::Counter* counter,
+                         const ValidationType& validation_type);
   // 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_{};
+  // This can be late initialized via setDeprecatedCounter(), enabling the server bootstrap loading
+  // which occurs prior to the initialization of the stats subsystem.
+  Stats::Counter* deprecated_counter_{};
+  uint64_t prestats_unknown_count_{};
+  uint64_t prestats_deprecated_count_{};
 };
 
 class StrictValidationVisitorImpl : public ValidationVisitor {
 public:
   // Envoy::ProtobufMessage::ValidationVisitor
   void onUnknownField(absl::string_view description) override;
+  void onDeprecatedField(absl::string_view description) override;
 };
 
 ValidationVisitor& getStrictValidationVisitor();

source/common/protobuf/utility.cc

@@ -163,7 +163,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
@@ -197,12 +198,7 @@ void deprecatedFieldHelper(Runtime::Loader* runtime, bool proto_annotated_as_dep
   if (warn_only) {
     ENVOY_LOG_MISC(warn, "{}", with_overridden);
   } else {
-    const char fatal_error[] =
-        " If continued use of this field is absolutely necessary, see "
-        "https://www.envoyproxy.io/docs/envoy/latest/configuration/operations/runtime"
-        "#using-runtime-overrides-for-deprecated-features 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);
   }
 }
 
@@ -373,11 +369,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;
@@ -401,7 +396,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 {
@@ -417,7 +412,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) ||
@@ -436,7 +432,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

@@ -281,10 +281,14 @@ 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_.static_warning_validation_visitor().setDeprecatedCounter(
+      server_stats_->static_deprecated_fields_);
+  validation_context_.dynamic_warning_validation_visitor().setUnknownCounter(
       server_stats_->dynamic_unknown_fields_);
+  validation_context_.dynamic_warning_validation_visitor().setDeprecatedCounter(
+      server_stats_->dynamic_deprecated_fields_);
 
   initialization_timer_ = std::make_unique<Stats::HistogramCompletableTimespanImpl>(
       server_stats_->initialization_time_ms_, timeSource());

source/server/server.h

@@ -53,7 +53,9 @@ namespace Server {
  */
 #define ALL_SERVER_STATS(COUNTER, GAUGE, HISTOGRAM)                                                \
   COUNTER(debug_assertion_failures)                                                                \
+  COUNTER(dynamic_deprecated_fields)                                                               \
   COUNTER(dynamic_unknown_fields)                                                                  \
+  COUNTER(static_deprecated_fields)                                                                \
   COUNTER(static_unknown_fields)                                                                   \
   GAUGE(concurrency, NeverImport)                                                                  \
   GAUGE(days_until_first_cert_expiring, Accumulate)                                                \

test/common/protobuf/message_validator_impl_test.cc

@@ -22,32 +22,32 @@ 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;
   // 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(StrictValidationVisitorImpl, UnknownField) {
   StrictValidationVisitorImpl strict_validation_visitor;
   EXPECT_THROW_WITH_MESSAGE(strict_validation_visitor.onUnknownField("foo"),
-                            UnknownProtoFieldException,
+                            ValidationError::UnknownProtoFieldException,
                             "Protobuf message (foo) has unknown fields");
 }
 

test/common/protobuf/utility_test.cc

@@ -1437,7 +1437,8 @@ 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::ValidationError::DeprecatedProtoFieldException,
       "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated_fatal'");
 }
 
@@ -1448,7 +1449,8 @@ TEST_P(DeprecatedFieldsTest,
 
   // Make sure this is set up right.
   EXPECT_THROW_WITH_REGEX(
-      checkForDeprecation(base), ProtoValidationException,
+      checkForDeprecation(base),
+      Envoy::ProtobufMessage::ValidationError::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());
@@ -1481,7 +1483,8 @@ 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::ValidationError::DeprecatedProtoFieldException,
       "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated'");
   EXPECT_EQ(1, runtime_deprecated_feature_use_.value());
 }
@@ -1496,7 +1499,8 @@ 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::ValidationError::DeprecatedProtoFieldException,
             "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated_fatal'");
       });
 }
@@ -1589,7 +1593,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::ValidationError::DeprecatedProtoFieldException,
                           "Using the default now-deprecated value DEPRECATED_DEFAULT");
 }
 
@@ -1598,7 +1603,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::ValidationError::DeprecatedProtoFieldException,
                           "Using deprecated value DEPRECATED_FATAL");
 
   Runtime::LoaderSingleton::getExisting()->mergeValues(

test/mocks/protobuf/BUILD

@@ -12,5 +12,9 @@ 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",
+        "//include/envoy/stats:stats_interface",
+        "//include/envoy/stats:stats_macros",
+    ],
 )