Unknown fields in the configuration and new features policy

[PiotrSikora]

Starting with #3778, Envoy ignores unknown fields in the configuration. This is enabled by default and without the ability to turn it off, which already lead to invalid configuration sneaking into tests (see: #4051).

Presumably, the reasoning behind that change was for proxies to be able to accept configuration produced by newer control planes, effectively ignoring new features they don't know about yet.

Unfortunately, this leads to Envoy silently accepting invalid (e.g. simple typo means that the setting is ignored and feature is not configured) and/or not working (e.g. new feature that's required for the deployment is silently ignored by older proxies, leading to unexpected behavior) configurations.

Having said that, we cannot rely on proxies and control planes always being in sync, and the drift can happen the other way around as well, i.e. having proxies with new features, without the ability to configure or even disable (!) them by older control planes, so we should have a solution for both scenarios.

Ultimately, having new features disabled by default and requiring explicit opt-in, might be the safest and easiest option to solve that. Though, it pushes the responsibility for configuring sane defaults onto control planes.

As for the unknown fields in the configuration, I think that control planes could send a list of fields that are safe to be ignored by the proxies.

Thoughts?

cc @mattklein123 @alyssawilk @htuch @ggreenway @kyessenov @rshriram

[mattklein123]

@kyessenov can you refresh our memory on the rationale behind the original change? FWIW, I saw that change go by and I thought about pushing back, but didn't. I probably should have.

IMO this creates a lot of confusion for configuration, and it's a problem we had a lot of in v1 prior to a strict JSON schema. People think they are setting options, but they have a typo, and then the option is ignored and they get the default. This leads to a lot of support queries.

My general feeling is that we should make the change to ignore unknown fields configurable on the CLI, and default to off. Then operators that want this functionality can turn it on via CLI? I think that is the simplest option.

[htuch]

I think the motivation was having consistency between proto behavior and JSON; unknown fields in proto are not automatically rejected (https://developers.google.com/protocol-buffers/docs/proto#updating).

I generally agree with the sentiment that we should make this more robust for users, but consistent behavior between proto and JSON/YAML config where possible is another consideration to throw in there.

[mattklein123]

I generally agree with the sentiment that we should make this more robust for users, but consistent behavior between proto and JSON/YAML config where possible is another consideration to throw in there.

As I recall I though the issue here was more with how we handle struct/any encoding? @kyessenov? In any case, though I understand the difference between JSON/YAML vs. proto, IMO this creates a lot of extra confusion and I think we should make this configurable if people want both behaviors.

[ramaraochavali]

FWIW, I ran in to this issue today where envoy silently accepted a typo in config and it took some time to figure that out.

[kyessenov]

Sorry, I missed notification for this issue. I tend to agree with @mattklein123 that a CLI option for strict semantics is a good idea since it enforces validation. My position is that we should apply it consistently between protos and proto structs. I think it's reasonable to disallow unknown fields in all protos (bootstrap, and proto Any fields), as long as we match the behavior between filter configs and the rest of the config. As an author of filter config, it is unexpected that envoy rejects the config when a new field is added to the proto, since if we were to use a standard proto representation, this would not happen. It's not immediately obvious that there is this difference between structs and Any.

We can be more fine-grained, and customize unknown field behavior by the type of config if necessary.

[mattklein123]

@kyessenov SGTM. Are you willing to work on this? I would suggest we make the default what it was before with a CLI option to opt-out of strict semantics?

[kyessenov]

Sure, feel free to assign this to me.

…

On Tue, Aug 7, 2018, 8:42 AM Matt Klein ***@***.***> wrote: @kyessenov <https://github.com/kyessenov> SGTM. Are you willing to work on this? I would suggest we make the default what it was before with a CLI option to opt-out of strict semantics? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4053 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJGIxllpw1aAcmdvRzBpFqo05nqTyj7qks5uObV6gaJpZM4Vux_S> .

[htuch]

+1 on the general approach; as long as we have consistency between protobuf and JSON/YAML, I'm all good.

[lizan]

In addition to CLI option, perhaps also make it a flag in runtime configuration, so it can be changed without restarting?

[htuch]

@lizan what's the motivation for runtime support? It seems this is something you want to set once for the lifetime of an Envoy instance. For existing deployments, they will need to pickup the new binary and do a restart to be able to take advantage of it.

[lizan]

The motivation for runtime support is to let envoy accepts unknown fields only during control-plane upgrade. Control planes doesn't always track each Envoy instances version and may emit unknown fields to some of Envoy instances. After the Envoy instances pick up new binary, it can be set back to strict.

[kyessenov]

@lizan if we can selectively enable runtime flag to allow unknown fields, then we can surely teach the control plane to do the same on the server side. I think this is mostly an issue for control planes that do not track versions of the proxies.

[lizan]

@kyessenov I'm not saying you would selectively enable this based on each instances. Flow like this:

1. Initially the runtime flag could be set to strict when you deploy Envoy instances
2. You make all of Envoy instances lenient when you deploy control plane.
3. After deployed control plane, pick up new Envoy binary and deploy with strict.

A runtime flag for 2 without restarting, thoughts?

[lizan]

btw, recalled more of this context from protocolbuffers/protobuf#1704, this is more like proto-text vs protobuf binary. JSON is in the middle, could be both handwritten (which shouldn't be parsed leniently as it may contain typos), or machine produced (which could be parsed leniently for backward compatibility, typos are not there).

A couple more things I can think of:

• Raise warnings when JSON/YAML parsing encountered an unknown field.
  Unfortunately protobuf doesn't have an interface to do this easiliy :(

• Differentiate protobuf binary (machine produced) and JSON/YAML strictly.
  This is an unfortunate side effect of using Struct as an extensible config format, when parsing extensions everything is a JSON/YAML, thus we need a flag to control that. I don't want bikeshedding more on Any vs Struct, but it add a point to do oneof, to differentiate the two aspect.

[kyessenov]

I think the new flag seems to fit fine with the paradigm:

• if the config is machine generated, then run with --allow-unknown-fields which applies lenient parsing;
• if the config is hand-written (including xDS payloads), then don't set the flag; that helps novices and test harnesses;
• a hybrid of human/machine config is not something we want to encourage, I think.

[lizan]

a hybrid of human/machine config is not something we want to encourage, I think.

But certainly we do have a use ;) https://istio.io/docs/reference/config/istio.networking.v1alpha3/#EnvoyFilter

[htuch]

I think we might need to revisit this in light of #6271. If we want stable APIs, in the absence of version negotiation (#5405), we're going to need to allow control planes to set fields unknown to earlier Envoys.