Validate deprecated fields via validation visitor

include/envoy/protobuf/BUILD

@@ -11,5 +11,10 @@ envoy_package()
 envoy_cc_library(
     name = "message_validator_interface",
     hdrs = ["message_validator.h"],
-    deps = ["//source/common/protobuf"],
+    deps = [
+        "//source/common/common:documentation_url_lib",
+        "//source/common/common:logger_lib",
+        "//source/common/common:macros",
+        "//source/common/protobuf",
+    ],
 )

include/envoy/protobuf/message_validator.h

@@ -3,22 +3,13 @@
 #include "envoy/common/exception.h"
 #include "envoy/common/pure.h"
 
+#include "common/common/documentation_url.h"
 #include "common/protobuf/protobuf.h"
 
 #include "absl/strings/string_view.h"
 
 namespace Envoy {
 namespace ProtobufMessage {
-
-/**
- * Exception class for reporting validation errors due to the presence of unknown
- * fields in a protobuf
- */
-class UnknownProtoFieldException : public EnvoyException {
-public:
-  UnknownProtoFieldException(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 +21,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 +30,15 @@ class ValidationVisitor {
    * possible.
    **/
   virtual bool skipValidation() PURE;
+
+  /**
+   * Invoked when deprecated field is encountered.
+   * @param description human readable description of the field.
+   */
+  virtual void onDeprecatedField(absl::string_view description, bool soft_deprecation) PURE;
+
+protected:
+  void onDeprecatedFieldDefault(absl::string_view description, bool soft_deprecation);
 };
 
 class ValidationContext {

source/common/config/filesystem_subscription_impl.cc

@@ -57,7 +57,7 @@ void FilesystemSubscriptionImpl::refresh() {
     stats_.version_text_.set(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.cc

@@ -0,0 +1,23 @@
+#include "envoy/common/exception.h"
+#include "envoy/protobuf/message_validator.h"
+
+#include "common/common/logger.h"
+
+#include "absl/strings/str_cat.h"
+
+namespace Envoy {
+namespace ProtobufMessage {
+
+void ValidationVisitor::onDeprecatedFieldDefault(absl::string_view description,
+                                                 bool soft_deprecation) {
+  if (soft_deprecation) {
+    ENVOY_LOG_MISC(warn, "Unexpected field: {}",
+                   absl::StrCat(description, ValidationError::deprecation_error));
+  } else {
+    throw ValidationError::DeprecatedProtoFieldException(
+        absl::StrCat(description, ValidationError::deprecation_error));
+  }
+}
+
+} // namespace ProtobufMessage
+} // namespace Envoy

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,20 +24,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_;
+  ENVOY_LOG(warn, "Unexpected field: {}", description);
+  if (unknown_counter_ == nullptr) {
+    ++prestats_unknown_count_;
   } else {
-    counter_->inc();
+    unknown_counter_->inc();
   }
 }
 
+void WarningValidationVisitorImpl::onDeprecatedField(absl::string_view description,
+                                                     bool soft_deprecation) {
+  onDeprecatedFieldDefault(description, soft_deprecation);
+}
+
 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,
+                                                    bool soft_deprecation) {
+  onDeprecatedFieldDefault(description, soft_deprecation);
+}
+
 ValidationVisitor& getNullValidationVisitor() {
   MUTABLE_CONSTRUCT_ON_FIRST_USE(NullValidationVisitorImpl);
 }

source/common/protobuf/message_validator_impl.h

@@ -10,10 +10,36 @@
 namespace Envoy {
 namespace ProtobufMessage {
 
+namespace ValidationError {
+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.";
+
+/**
+ * Exception class for reporting validation errors due to the presence of unknown
+ * 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
+
 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 +50,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 +63,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 +76,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/protobuf/message_validator_impl_test.cc

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