Validate deprecated fields via validation visitor#10853
Validate deprecated fields via validation visitor#10853htuch merged 29 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>
Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>
Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>
6120ce4 to
0b0b046
Compare
bazel/envoy_internal.bzl
Outdated
| "-Wall", | ||
| "-Wextra", | ||
| "-Werror", | ||
| # "-Werror", |
|
still need to update/add more tests |
htuch
left a comment
There was a problem hiding this comment.
Looks great, with one caveat (see comment).
|
/retest |
|
🔨 rebuilding |
| // 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.
adding extra counter for deprecated fields may not be needed, as there is stats counter in Runtime already: deprecated_feature_use
There was a problem hiding this comment.
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.
Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>
|
/retest |
|
🐴 hold your horses - no failures detected, yet. |
source/common/protobuf/utility.cc
Outdated
| "#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); |
There was a problem hiding this comment.
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 (warn_only), maybe call it soft_deprecation?
| // 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.
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.
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
keepalive ping, i was busy with another pr |
Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>
Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>
Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>
Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>
Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>
|
🐴 hold your horses - no failures detected, yet. |
Not sure how to fix this |
Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>
Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>
Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>
htuch
left a comment
There was a problem hiding this comment.
Other than the nits around file organization, LGTM. Thanks!
| namespace Envoy { | ||
| namespace ProtobufMessage { | ||
|
|
||
| void ValidationVisitor::onDeprecatedFieldDefault(absl::string_view description, |
There was a problem hiding this comment.
We tend to prefer that even default behavior lives in source/ vs. include/, the latter of which is generally just pure virtual functions and classes.
Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>
|
Thanks for review @htuch. I will need to fix few tests that use MockValdationVisitors |
Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>
|
@htuch this PR is ready for review |
htuch
left a comment
There was a problem hiding this comment.
Looks good, but not sure why of a few changes in the last updates.
/wait
| } else { | ||
| counter_->inc(); | ||
| throw DeprecatedProtoFieldException(absl::StrCat(description, deprecation_error)); | ||
| } |
There was a problem hiding this comment.
How come the common method is inline now?
There was a problem hiding this comment.
@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.
Yeah, but looking at the two implementations, I'm not seeing a stats increment in either, and still unclear why the warnings are different.
Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>
Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>
Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>
Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>
|
/retest |
|
🤷♀️ nothing to rebuild. |
htuch
left a comment
There was a problem hiding this comment.
LGTM, thanks! Appreciate the patience while iterating on this.
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 envoyproxy#8092 Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>
Description: Duplicate of #10307
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
Testing: TBD
Docs Changes:NA
Release Notes:TBD
Fixes #8092