Validate deprecated fields via validation visitor #10853

nezdolik · 2020-04-20T15:12:23Z

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

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik · 2020-04-20T16:11:53Z

bazel/envoy_internal.bzl

@@ -7,7 +7,7 @@ def envoy_copts(repository, test = False):
    posix_options = [
        "-Wall",
        "-Wextra",
-        "-Werror",
+        #        "-Werror",


nezdolik · 2020-04-20T16:18:59Z

still need to update/add more tests

htuch

Looks great, with one caveat (see comment).

source/common/protobuf/utility.cc

nezdolik · 2020-04-23T10:08:12Z

/retest

repokitteh-read-only · 2020-04-23T10:08:17Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #10853 (comment) was created by @nezdolik.

see: more, trace.

nezdolik · 2020-04-24T11:19:23Z

source/common/protobuf/message_validator_impl.h

+  // which occurs prior to the initialization of the stats subsystem.
+  Stats::Counter* deprecated_counter_{};
+  uint64_t prestats_unknown_count_{};
+  uint64_t prestats_deprecated_count_{};


adding extra counter for deprecated fields may not be needed, as there is stats counter in Runtime already: deprecated_feature_use

Yep, that's fair enough.

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>

nezdolik · 2020-04-24T13:43:34Z

/retest

repokitteh-read-only · 2020-04-24T13:43:37Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #10853 (comment) was created by @nezdolik.

see: more, trace.

htuch · 2020-04-27T02:14:38Z

source/common/protobuf/utility.cc

-        "#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);


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?

htuch · 2020-04-27T02:15:51Z

source/common/protobuf/message_validator_impl.h

+  // which occurs prior to the initialization of the stats subsystem.
+  Stats::Counter* deprecated_counter_{};
+  uint64_t prestats_unknown_count_{};
+  uint64_t prestats_deprecated_count_{};


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.

stale · 2020-05-04T03:15:08Z

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!

nezdolik · 2020-05-06T20:37:32Z

keepalive ping, i was busy with another pr

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

repokitteh-read-only · 2020-05-14T17:02:24Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #10853 (comment) was created by @nezdolik.

see: more, trace.

nezdolik · 2020-05-14T17:12:17Z

Caused by: io.grpc.StatusRuntimeException: FAILED_PRECONDITION: there are no bots capable of executing the action, requested action properties: OSFamily = Linux, concurrency = exclusive (Added by RBE; concurrent actions are incompatible with the following requested properties: dockerSiblingContainers = true)

Not sure how to fix this

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

test/mocks/protobuf/mocks.h

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

htuch

Other than the nits around file organization, LGTM. Thanks!

htuch · 2020-05-15T23:00:42Z

include/envoy/protobuf/message_validator.cc

+namespace Envoy {
+namespace ProtobufMessage {
+
+void ValidationVisitor::onDeprecatedFieldDefault(absl::string_view description,


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.

include/envoy/protobuf/message_validator.h

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

htuch

LGTM, build needs fixing.

include/envoy/protobuf/BUILD

source/common/protobuf/message_validator_impl.cc

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik · 2020-05-19T18:10:37Z

Thanks for review @htuch. I will need to fix few tests that use MockValdationVisitors

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik · 2020-06-02T12:12:35Z

@htuch this PR is ready for review

htuch

Looks good, but not sure why of a few changes in the last updates.
/wait

source/common/protobuf/message_validator_impl.cc

htuch · 2020-06-02T18:31:34Z

source/common/protobuf/message_validator_impl.cc

  } else {
-    counter_->inc();
+    throw DeprecatedProtoFieldException(absl::StrCat(description, deprecation_error));
  }


How come the common method is inline now?

@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.

Yeah, but looking at the two implementations, I'm not seeing a stats increment in either, and still unclear why the warnings are different.

source/common/protobuf/utility.cc

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik · 2020-06-05T07:44:52Z

/retest

repokitteh-read-only · 2020-06-05T07:44:55Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #10853 (comment) was created by @nezdolik.

see: more, trace.

htuch

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>

Kateryna Nezdolii added 4 commits March 9, 2020 17:29

Validate deprecated fields via validation visitor

6158514

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

Merge remote-tracking branch 'origin/master'

74f21d0

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

wip

b7e47b6

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

more vip

303494c

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

htuch self-requested a review April 20, 2020 16:04

htuch self-assigned this Apr 20, 2020

Merge remote-tracking branch 'origin/master'

0b0b046

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik force-pushed the proto-validator branch from 6120ce4 to 0b0b046 Compare April 20, 2020 16:11

nezdolik commented Apr 20, 2020

View reviewed changes

htuch reviewed Apr 21, 2020

View reviewed changes

source/common/protobuf/utility.cc Show resolved Hide resolved

nezdolik commented Apr 24, 2020

View reviewed changes

Kateryna Nezdolii added 2 commits April 24, 2020 14:39

appy review comments

fcf1e98

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

clean up

bc67e6e

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

htuch reviewed Apr 27, 2020

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label May 4, 2020

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label May 6, 2020

Apply fixes

237f48a

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik changed the title ~~Validate deprecated fields via validation visitor~~ [wip] Validate deprecated fields via validation visitor May 11, 2020

Kateryna Nezdolii added 5 commits May 11, 2020 18:06

more fixes

fb9329a

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

more fixes

0a13ab9

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

Merge remote-tracking branch 'origin/master'

0eebfc6

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

refactor

23cabf6

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

Merge remote-tracking branch 'origin/master'

66e0551

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

Kateryna Nezdolii added 2 commits May 15, 2020 11:09

dummy commit to retrigger ci

93fc3b5

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

fix format

27a05bf

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik commented May 15, 2020

View reviewed changes

test/mocks/protobuf/mocks.h Outdated Show resolved Hide resolved

fix merge mistake

42d1a76

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

htuch reviewed May 15, 2020

View reviewed changes

Kateryna Nezdolii added 2 commits May 18, 2020 11:24

Move out impl from include

5c4863f

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

fix build

07b47ae

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

htuch reviewed May 19, 2020

View reviewed changes

include/envoy/protobuf/BUILD Outdated Show resolved Hide resolved

source/common/protobuf/message_validator_impl.cc Outdated Show resolved Hide resolved

Kateryna Nezdolii added 2 commits May 19, 2020 14:25

refactor

494931b

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nits

f6e7c3d

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

fix fuzz test

51ba2d6

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

htuch suggested changes Jun 2, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 2, 2020

nezdolik commented Jun 3, 2020

View reviewed changes

source/common/protobuf/utility.cc Show resolved Hide resolved

apply review comments

7a27798

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

repokitteh-read-only bot removed the waiting label Jun 3, 2020

Kateryna Nezdolii added 3 commits June 4, 2020 19:56

reduuce code dupl

0e0e577

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

rename function

3e94b20

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

Merge remote-tracking branch 'origin/master'

6334f35

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

htuch approved these changes Jun 5, 2020

View reviewed changes

htuch merged commit 67376d9 into envoyproxy:master Jun 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate deprecated fields via validation visitor #10853

Validate deprecated fields via validation visitor #10853

nezdolik commented Apr 20, 2020 •

edited

Loading

nezdolik Apr 20, 2020

nezdolik commented Apr 20, 2020

htuch left a comment

nezdolik commented Apr 23, 2020

repokitteh-read-only bot commented Apr 23, 2020

nezdolik Apr 24, 2020

htuch Apr 27, 2020

htuch Apr 27, 2020

nezdolik commented Apr 24, 2020

repokitteh-read-only bot commented Apr 24, 2020

htuch Apr 27, 2020

htuch Apr 27, 2020

stale bot commented May 4, 2020

nezdolik commented May 6, 2020

repokitteh-read-only bot commented May 14, 2020

nezdolik commented May 14, 2020

htuch left a comment

htuch May 15, 2020

htuch left a comment

nezdolik commented May 19, 2020

nezdolik commented Jun 2, 2020

htuch left a comment

htuch Jun 2, 2020

nezdolik Jun 3, 2020

htuch Jun 4, 2020

nezdolik commented Jun 5, 2020

repokitteh-read-only bot commented Jun 5, 2020

htuch left a comment

Validate deprecated fields via validation visitor #10853

Validate deprecated fields via validation visitor #10853

Conversation

nezdolik commented Apr 20, 2020 • edited Loading

Choose a reason for hiding this comment

nezdolik commented Apr 20, 2020

htuch left a comment

Choose a reason for hiding this comment

nezdolik commented Apr 23, 2020

repokitteh-read-only bot commented Apr 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nezdolik commented Apr 24, 2020

repokitteh-read-only bot commented Apr 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented May 4, 2020

nezdolik commented May 6, 2020

repokitteh-read-only bot commented May 14, 2020

nezdolik commented May 14, 2020

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

nezdolik commented May 19, 2020

nezdolik commented Jun 2, 2020

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nezdolik commented Jun 5, 2020

repokitteh-read-only bot commented Jun 5, 2020

htuch left a comment

Choose a reason for hiding this comment

nezdolik commented Apr 20, 2020 •

edited

Loading