config: logging a warning on use of deprecated proto fields by alyssawilk · Pull Request #5760 · envoyproxy/envoy

alyssawilk · 2019-01-29T20:59:24Z

Risk Level: Low (logging only)
Testing: new unit tests
Docs Changes: n/a
Release Notes: not included
Part of #5559

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2019-01-29T21:02:56Z

Some discussion points.

I'd prefer this to be a critical log but I think the forced-flush could work very badly if it were present in a large (order tens of thousands of repeated messages) config, so I'm erring on the side of perf-caution

I think we should probably un-deprecate 2 of the 3 remaining deprecated_v1 fields to avoid user-confusion that they will be removed soon. I believe the removal of bind_to_port and the cds tcp proxy filter are blocked on code, so should probably be undeprecated until code is complete and then de-deprecated following normal Envoy process.
The third one, which is the hashing algorithm, I think we can still kill off in fairly short order but I'm happy to be overriden by the folks at Lyft working on their migration.

ggreenway

This is great!

Do we consistently call validate() on all messages, and specifically on the bits that get passed as a Struct and re-proto-ified in filters?

ggreenway · 2019-01-29T21:01:16Z

source/common/protobuf/utility.h

+    const auto* desc = message.GetDescriptor();
+    const auto* refl = message.GetReflection();
+    for (int i = 0; i < desc->field_count(); ++i) {
+      const auto* fd = desc->field(i);


nit: fd means file descriptor in my brain. Rename to field?

ggreenway · 2019-01-29T21:04:45Z

test/common/protobuf/utility_test.cc

+    envoy::test::deprecation_test::Base base;
+    base.set_is_deprecated("foo");
+    // Non-fatal checks for a deprecated field shouldn't throw an exception.
+    MessageUtil::checkForDeprecation(base, false);


Wrap in EXPECT_LOG_CONTAINS?

lizan · 2019-01-29T21:04:32Z

source/common/protobuf/utility.h

  static void loadFromYaml(const std::string& yaml, Protobuf::Message& message);
  static void loadFromFile(const std::string& path, Protobuf::Message& message);

+  template <class MessageType>


I don't think this need to be templatized, using Protobuf::Message should be just enough.

lizan · 2019-01-29T21:07:09Z

source/common/protobuf/utility.h

+  static void checkForDeprecation(const MessageType& message, bool fatal) {
+    const auto* desc = message.GetDescriptor();
+    const auto* refl = message.GetReflection();
+    for (int i = 0; i < desc->field_count(); ++i) {


This only checks top level field and not about inner fields such as here, Validate does recursive validation internally but MessageUtil::validate isn't called at every level.

Ah my bad, I didn't realize Validate was doing the recursion - will fix and regression test

lizan · 2019-01-29T21:09:23Z

Do we consistently call validate() on all messages, and specifically on the bits that get passed as a Struct and re-proto-ified in filters?

Struct and re-proto-ified ones are validated via downcastAndValidate so it will be checked by this method.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2019-01-29T22:08:33Z

Noting for the record I'm now officially out of my depth w.r.t proto reflection so please to encourage more tests if you see any gaps.

And uploaded without testing beyond the unit test - living dangerously today! :-P

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

lizan · 2019-01-29T22:36:33Z

source/common/protobuf/utility.h

  static void loadFromYaml(const std::string& yaml, Protobuf::Message& message);
  static void loadFromFile(const std::string& path, Protobuf::Message& message);

+  static void checkForDeprecation(const Protobuf::Message& message, bool fatal) {


move the impl to .cc

lizan · 2019-01-29T22:38:50Z

source/common/protobuf/utility.h

+            "Using deprecated option '{}'. This configuration will be removed from Envoy soon. "
+            "Please see https://github.com/envoyproxy/envoy/blob/master/DEPRECATED.md for "
+            "details.",
+            field->name());


full_name() instead of name should be more helpful.

Thanks for the tip - this is going to be even more useful!

lizan · 2019-01-29T22:40:15Z

source/common/protobuf/utility.h

+        if (fatal) {
+          throw ProtoValidationException(err, message);
+        } else {
+          ENVOY_LOG_MISC(warn, "{}", err);


use config logger?

Which logger? The only other log macro in the util.cc used MISC so I didn't think any of the other log macros were suitable.

We could wrap in a class to provide access to the config logger (or maybe there is a more direct way?).

Just make MessageUtil inherit Loggable and it should be good (it only provides static method)

Ah, so you want it to inherit from Logger::LoggableLogger::Id::config

Done, though I think ENVOY_LOG_MISC might make more sense, given that someone could put non-config related proto utilities in source/common/protobuf/utility.h

That said the main uses of proto are config and gRPC which has its own directories, so it's fine for now.

lizan · 2019-01-29T22:43:07Z

source/common/protobuf/utility.h

+      }
+
+      // If this is a message, recurse to check for deprecated fields in the sub-message.
+      switch (field->cpp_type()) {


just if (field->cpp_type == Protobuf::FieldDescriptor::CPPTYPE_MESSAGE)?

htuch · 2019-01-29T22:59:11Z

I think for things like bind_to_port, we should keep it deprecated, since we want to encourage users to migrate away. I think warning is ideal, critical is too high for use of a feature the is deprecated but not dead.

htuch

LGTM modulo comments, nice to see that there is first-class support for the deprecated annotation in reflection.

htuch · 2019-01-29T23:04:32Z

test/common/protobuf/utility_test.cc

+                            "Using deprecated option 'inner_deprecated'.");
+  }
+
+  // Check that repeated sub-messages get validated.


Nit: preference for making these all separate TEST_F, since they don't share state and it's more self-documenting.

htuch · 2019-01-29T23:05:27Z

source/common/protobuf/utility.h

  static void loadFromYaml(const std::string& yaml, Protobuf::Message& message);
  static void loadFromFile(const std::string& path, Protobuf::Message& message);

+  static void checkForDeprecation(const Protobuf::Message& message, bool fatal) {


Nit: prefer warn_only vs. fatal, since it's not fatal, it's raising an exception which could be handled in other ways at a call-site (even though you probably only plan on using it for fatal..).

alyssawilk · 2019-01-30T14:11:54Z

@htuch my only concern there is potentially causing confusion for folks who get the warning that they should migrate but can't migrate yet.

That said on both the deprecated_v1 fields I'm dubious about, I think the only folks who can't move are Istio, so as long as they know the logged warning doesn't change our deprecation plans I guess we're good? Tagging @PiotrSikora for Istio comms to avoid any confusion on their part and I'll add a release note as well.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

htuch · 2019-01-30T22:56:34Z

Also @costinm for Istio viz.

htuch

LGTM modulo nits and unresolved comments from @lizan.

htuch · 2019-01-30T22:58:01Z

test/common/protobuf/utility_test.cc

+    EXPECT_THROW_WITH_REGEX(
+        MessageUtil::checkForDeprecation(base, false), ProtoValidationException,
+        "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated'.");
+  }


Nit: no need for the inner braces in all these TESTs..

htuch · 2019-01-30T22:58:26Z

test/common/protobuf/utility_test.cc

+}
+
+TEST(DeprecatedFields, SubMessageDeprecated) {
+  // Check that repeated sub-messages get validated.


Nit: move these comments about the TEST.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

comments addressed

htuch · 2019-01-31T21:04:48Z

source/common/protobuf/utility.h

 enum class ProtoUnknownFieldsMode { Strict, Allow };

-class MessageUtil {
+class MessageUtil : public Logger::Loggable<Logger::Id::config> {


I agree with @alyssawilk, this is not in a config related file/directory, so would prefer not to log with this. Can we pass logger as a parameter, or maybe subclass MessageUtil in config/ with a logger mixin?

Or go back to MISC_LOG and claim there's precedent :-D

Ok that's fair :)

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

htuch

Thanks!

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2019-02-04T14:52:29Z

Had to sync for merge conflict - I'd appreciate one last LGTM!

…xy#5760) Risk Level: Low (logging only) Testing: new unit tests Docs Changes: n/a Release Notes: not included Part of envoyproxy#5559 Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Fred Douglas <fredlas@google.com>

config: logging a warning on use of deprecated fields

b2e47fc

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk assigned lizan and htuch Jan 29, 2019

ggreenway previously requested changes Jan 29, 2019

View reviewed changes

lizan suggested changes Jan 29, 2019

View reviewed changes

Merge branch 'master' into runtime

3a717fa

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

bugfix

de43d41

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the runtime branch from c0ca394 to de43d41 Compare January 29, 2019 22:10

lizan suggested changes Jan 29, 2019

View reviewed changes

htuch reviewed Jan 29, 2019

View reviewed changes

reviewer comments

5c6adc7

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the runtime branch from 23896b6 to 5c6adc7 Compare January 30, 2019 15:26

Merge branch 'master' into runtime

08175d6

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

htuch reviewed Jan 30, 2019

View reviewed changes

reviewer comments

ad3bcf0

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

htuch reviewed Jan 31, 2019

View reviewed changes

Backing out logging change in the hopes it's not-merge-blocking issue

17dfc47

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the runtime branch from 9b4994a to 17dfc47 Compare January 31, 2019 21:19

htuch previously approved these changes Jan 31, 2019

View reviewed changes

lizan previously approved these changes Jan 31, 2019

View reviewed changes

Merge branch 'master' into runtime

9abec4e

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk dismissed stale reviews from lizan and htuch via 9abec4e February 4, 2019 14:52

htuch approved these changes Feb 4, 2019

View reviewed changes

alyssawilk merged commit bb9735a into envoyproxy:master Feb 4, 2019

alyssawilk deleted the runtime branch May 7, 2019 19:11

Conversation

alyssawilk commented Jan 29, 2019

Uh oh!

alyssawilk commented Jan 29, 2019

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizan commented Jan 29, 2019

Uh oh!

alyssawilk commented Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Jan 29, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Jan 30, 2019

Uh oh!

htuch commented Jan 30, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Feb 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alyssawilk commented Jan 29, 2019 •

edited

Loading