-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Validate deprecated fields via validation visitor #10853
Changes from 5 commits
6158514
74f21d0
b7e47b6
303494c
0b0b046
fcf1e98
bc67e6e
237f48a
fb9329a
0a13ab9
0eebfc6
23cabf6
66e0551
a6277d5
9f1b788
b6109d4
097cc86
93fc3b5
27a05bf
42d1a76
5c4863f
07b47ae
494931b
f6e7c3d
51ba2d6
7a27798
0e0e577
3e94b20
6334f35
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,33 +11,62 @@ | |
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); | ||
} | ||
|
||
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); | ||
nezdolik marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (counter != nullptr) { | ||
counter->inc(); | ||
} else { | ||
counter_->inc(); | ||
switch (validation_type) { | ||
case UnknownFields: | ||
++prestats_unknown_count_; | ||
break; | ||
case DeprecatedFields: | ||
++prestats_deprecated_count_; | ||
break; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How come the common method is inline now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @htuch if i understand your question correctly, you mean previous version of PR where both visitors shared common method for incrementing stats and logging message? i've changed impl, there is no longer common method. There is little benefit to have common method as deprecated field validation does not increment counters and log messages are different. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, but looking at the two implementations, I'm not seeing a stats increment in either, and still unclear why the warnings are different. |
||
} | ||
|
||
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); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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_{}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. adding extra counter for deprecated fields may not be needed, as there is stats counter in Runtime already: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, that's fair enough. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, it's a bit strange that we bump the stat in runtime rather than in the calling code, but I don't think we can undo that at this point as folks are likely relying on it. So, yeah, we can simplify the code here quite a bit as a result. |
||
}; | ||
|
||
class StrictValidationVisitorImpl : public ValidationVisitor { | ||
public: | ||
// Envoy::ProtobufMessage::ValidationVisitor | ||
void onUnknownField(absl::string_view description) override; | ||
void onDeprecatedField(absl::string_view description) override; | ||
}; | ||
|
||
ValidationVisitor& getStrictValidationVisitor(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
nezdolik marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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); | ||
nezdolik marked this conversation as resolved.
Show resolved
Hide resolved
|
||
validation_visitor.onDeprecatedField("type " + message.GetTypeName() + " " + with_overridden); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should always be called, even if warning, but we can add a parameter to control whether it's expected to log or throw an exception ( |
||
} | ||
} | ||
|
||
|
@@ -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; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops