config: allow unknown fields flag #4094

kyessenov · 2018-08-08T23:02:16Z

Description: Add a flag allow-unknown-fields that controls whether all fields must be known in the filter configurations.

If allow unknown fields is set to false the following takes effect:

filter configs (represented as proto structs) must only have fields defined in the protos
files loaded from .pb, .yaml, and .json are also checked for known fields

Internal conversions in other parts of the code (upstream transport socket factories, etc) always allow unknown fields.

Proto text always requires known fields (general protobuf constraint).

Protos sent as Any always allow unknown fields. Reasoning here is that if the proto is packed then the schema has already been enforced on the supplier side. I also could not find a way to enforce strict checks in the protobuf API.

Risk Level: Medium. Allow unknown fields is set to false by default, meaning unknown fields will be rejected by envoy.

Testing: Unit tests were updated to have it strict by default. Some wrong filter configs were discovered.

Docs Changes: None

Release Notes:
Add a flag allow-unknown-fields to control validation of filter configurations for unknown fields.

Fixes #4053

/cc @PiotrSikora

Signed-off-by: Kuat Yessenov <kuat@google.com>

mattklein123 · 2018-08-08T23:04:41Z

@kyessenov quick pre-review comment: I don't agree this should default to false. I think it should default to true. The current default is very confusing for new/less sophisticated users. I would prefer that we go back to what we had before your first change and then allow people to opt-in to not strict behavior.

kyessenov · 2018-08-08T23:09:34Z

Sure thing. I'm just concerned about risks of some control planes pushing fields and breaking things. I can change the default to true.

mattklein123 · 2018-08-08T23:11:23Z

Sure thing. I'm just concerned about risks of some control planes pushing fields and breaking things. I can change the default to true.

I think we can make this clear in the docs and we can guide operators to make the right choice for their deployment? I'm mainly trying to optimize here for what I think is the common case that people want and what will lead to the least amount of support burden.

Signed-off-by: Kuat Yessenov <kuat@google.com>

mattklein123 · 2018-08-08T23:13:25Z

Internal conversions in other parts of the code (upstream transport socket factories, etc) always allow unknown fields.

What's the reasoning here?

kyessenov · 2018-08-08T23:16:22Z

@mattklein123 : I have started down that road but then noticed that there is downcasting happening after conversion. Should I keep going to wire this flag through all the places where any config proto conversion happens?

Signed-off-by: Kuat Yessenov <kuat@google.com>

mattklein123 · 2018-08-08T23:18:17Z

@mattklein123 : I have started down that road but then noticed that there is downcasting happening after conversion. Should I keep going to wire this flag through all the places where any config proto conversion happens?

@htuch thoughts? I hate to propose a static/global but I wonder if this is one case in which it's the least bad solution for something like this?

mattklein123 · 2018-08-08T23:18:45Z

(Basically would love to avoid the huge amount of call site changes if possible...)

lizan · 2018-08-08T23:27:07Z

(Basically would love to avoid the huge amount of call site changes if possible...)

Yeah, would making a Singleton to provide this flag to MessageUtil saves the amount of call site change?

htuch · 2018-08-09T02:44:39Z

source/common/config/utility.h

@@ -258,13 +259,14 @@ class Utility {
   */
  template <class Factory>
  static ProtobufTypes::MessagePtr translateToFactoryRouteConfig(const Protobuf::Message& source,
-                                                                 Factory& factory) {
+                                                                 Factory& factory,
+                                                                 bool allowUnknownFields) {


Nit: s/allowUnknownFields/allow_unknown_fields/

htuch

Thanks for taking this on @kyessenov. I think we should make the config a global; my reasoning here is that if we were using gflags, it would be effectively a global, we've managed to live with this CLI flag globals internally in Google.

htuch · 2018-08-09T02:54:37Z

source/extensions/grpc_credentials/file_based_metadata/config.cc

        const Envoy::ProtobufTypes::MessagePtr file_based_metadata_config_message =
            Envoy::Config::Utility::translateToFactoryConfig(
-                credential.from_plugin(), file_based_metadata_credentials_factory);
+                credential.from_plugin(), file_based_metadata_credentials_factory, true);


Maybe I'm brain farting, but can you reiterate why we need to do internal conversions with lenient settings, perhaps more lenient than the external ones? I remember there was some reason, but it would be helpful to know this.

Also, I think that having this boolean is one of those needles/poison needles minefields; people will have trouble reasoning when writing code when to set it to true vs. false. We should probably have two versions of translateToFactoryConfig that have names that reflect their intended use, e.g. internal vs. external or whatever.

I think this needs to be decided on case-by-case basis. If the source is a proto, then being lenient is fine. If the source is a struct, then we should ideally follow the same conventions. I don't have a good model of all the places we need this translation except for filter configs. And it started to get way more invasive to insert this boolean than I wished for.

We will need to update the style guide to make this very clear. There's really multiple risks; most contributors won't understand this distinction clearly and even reviewers are likely to not fully grok this or apply inconsistently. Semantic naming of the conversion utils to reflect the different expected use cases seems safer where possible, or anything we can do to structurally make it harder to reach up and grab the poison tipped needles.

Also, what do we gain by being lenient with proto? Presumably we've already vetted for no unknown fields at the edge of config ingest, so we should not hit any unknown fields during internal conversions?

kyessenov · 2018-08-09T03:15:40Z

Are we all OK using a singleton / global for this flag? I'm not a fan of hidden switches, but it surely helps with a less invasive change.

mattklein123 · 2018-08-09T16:15:53Z

Closing this one for now since I think we are converging on the other PR.

Add a flag allow-unknown-fields that controls whether proto conversion ignores unknown fields. Proto structs are uniformly used for extension configuration. A smaller fix than #4094. The validation is applied in the following places: Any conversion (e.g. xDS payloads) Struct conversion (e.g. filter configs) bootstrap proto binary Risk Level: Medium. Allow unknown fields is set to false by default, meaning unknown fields will be rejected by envoy. A missing check for xDS type checking was discovered. Testing: Unit tests were updated to have it strict by default. Some wrong filter configs were discovered. Docs Changes: None Release Notes: Add a flag allow-unknown-fields to control validation of protobuf configurations for unknown fields. Fixes #4053 Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov added 3 commits August 7, 2018 22:51

wire through unknown field bool

bfbbaef

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix filter configs

0d484e2

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix format

84c2967

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov requested review from lizan and zuercher as code owners August 8, 2018 23:02

mattklein123 self-assigned this Aug 8, 2018

change flag default

9916bb8

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov changed the title ~~config: disallow unknown fields flag~~ config: allow unknown fields flag Aug 8, 2018

fix format

d436dad

Signed-off-by: Kuat Yessenov <kuat@google.com>

mattklein123 assigned htuch Aug 8, 2018

htuch reviewed Aug 9, 2018

View reviewed changes

kyessenov mentioned this pull request Aug 9, 2018

config: allow unknown fields flag (take 2) #4096

Merged

mattklein123 closed this Aug 9, 2018

config: allow unknown fields flag #4094

config: allow unknown fields flag #4094

Uh oh!

Conversation

kyessenov commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Aug 8, 2018

Uh oh!

kyessenov commented Aug 8, 2018

Uh oh!

mattklein123 commented Aug 8, 2018

Uh oh!

mattklein123 commented Aug 8, 2018

Uh oh!

kyessenov commented Aug 8, 2018

Uh oh!

mattklein123 commented Aug 8, 2018

Uh oh!

mattklein123 commented Aug 8, 2018

Uh oh!

lizan commented Aug 8, 2018

Uh oh!

htuch Aug 9, 2018

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch Aug 9, 2018

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Aug 9, 2018

Uh oh!

mattklein123 commented Aug 9, 2018

Uh oh!

Uh oh!

kyessenov commented Aug 8, 2018 •

edited

Loading

htuch Aug 9, 2018 •

edited

Loading

kyessenov Aug 9, 2018 •

edited

Loading

htuch Aug 9, 2018 •

edited

Loading