deprecate google.protobuf.Struct as the configuration type

[kyessenov]

Raised in #9618.

We would like to prohibit the use of Struct as the configuration type for a filter for the same reason that Empty is not allowed. Each filter needs to version its configuration proto to enable conversion and backwards compatibility.

[kyessenov]

Note: struct is used heavily in testing because sharing the config type would cause coverage build to fail (it combines all tests into a single binary leading to double registration). We would need to create a separate proto for each test factory.

[htuch]

After reading through the thread on #9618, I think @mattklein123 and you raise a valid point about the use of non-unique config messages by non-Envoy shipped extensions. I think it might be hard to enforce a unique type given this, since it's possible other WKTs etc. might be used.

WDYT about adopting as our long term policy that we:

1. First look at type URL, if it's unique from a registration perspective, use that to resolve filter.
2. Otherwise, fallback to using filter name to infer type (as we will continue to do for legacy Struct configs).
   ?

This is what you have in your PR essentially already, the idea is to codify it as the long term plan.

This allows us to move forward with having type URL usable by all conforming filters, provides folks an adoption path for decoupling naming/typing and deals with legacy.

CC @alexburnos for visibility, as he has expressed interest in this area in the past.

[kyessenov]

I'm not happy about this idea since it leads to non-trivial bugs. E.g. taking a base envoy and adding filters that overlap in type URLs would cause the config that used to work for base envoy stop working. But if this is something that is temporary, we can do that.

[htuch]

Well, the idea is that the base Envoy filters would use unique per-filter config types, so it would be surprising to see anyone reuse them. Then, the contract is with someone specializing Envoy with extensions that they preserve this property.

[mattklein123]

One thing to note here, is that I just saw a comment go by where @kyessenov changed the double registration from a failure to a warning. If we are serious about this being the future, it has to stay an error and not a warning right? If we leave it a warning I think we must do the more relaxed variant that @htuch proposes?

[kyessenov]

The more relaxed variant is fine with me. Basically, whenever there is a type URL collision, we will not use the type URL and fallback to name. I can drop the warning, but I think it's useful since the end game is that each extension reserves its type URLs.

[mattklein123]

No I would keep the warning no matter what. My point is that if we aren't going for relaxed, I think it has to be an error and not a warning.

[alexburnos]

Curious, what is the downside of being opinionated and mandate that non-Envoy extension developers use unique config types? @htuch said it would be hard to enforce, but if double registration becomes an error - this would be in the best interest of an extension developer to follow it.

[mattklein123]

Curious, what is the downside of being opinionated and mandate that non-Envoy extension developers use unique config types? @htuch said it would be hard to enforce, but if double registration becomes an error - this would be in the best interest of an extension developer to follow it.

There is no downside other than we have to go through the entire deprecation test. We can't break existing extensions without a deprecation period.

[stale]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted". Thank you for your contributions.