xds: define empty config protos #9581
Conversation
Signed-off-by: Derek Argueta <dereka@pinterest.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
kyessenov
requested review from
alyssawilk,
htuch,
lizan and
snowp
as code owners
|
I am wondering why there is a fallback in |
|
@kyessenov can you give an example of how the unsafe conversion could be an issue? I think usually we should expect that in an opaque config, the intent is to describe the type that belongs to the filter. If it's untyped config, it's fine to JSON convert. For typed config, we probably could be a bit stricter on the fallback; only allow the existing or previous types of a specific message to be converted, but that seems a nice-to-have safety improvement, not a particularly dangerous thing as is. |
There was a problem hiding this comment.
This is great, thanks so much for taking this on (and @derekargueta for kicking off this initiative).
|
@htuch It's odd because going through JSON loses type information, so if you have two Anys with different types for the same field, it's unclear what happens. It also means we cannot use the type as the primary way to identify the extension (what if you use one extension type as Any for another extension). |
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
| // Fail in an obvious way if a plugin does not return a proto. | ||
| RELEASE_ASSERT(config != nullptr, ""); | ||
|
|
||
| // Check that the config type is not google.protobuf.Empty | ||
| RELEASE_ASSERT(config->GetDescriptor()->full_name() != "google.protobuf.Empty", ""); |
There was a problem hiding this comment.
Nice-to-have expect-death test for this..
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
api/BUILD
Outdated
| @@ -89,13 +99,16 @@ proto_library( | |||
| "//envoy/config/rbac/v3alpha:pkg", | |||
| "//envoy/config/resource_monitor/fixed_heap/v2alpha:pkg", | |||
| "//envoy/config/resource_monitor/injected_resource/v2alpha:pkg", | |||
| "//envoy/config/retry/omit_canary_hosts:pkg", | |||
| "//envoy/config/retry/previous_hosts:pkg", | |||
There was a problem hiding this comment.
Can you switch these to be in versioned packages? I'm working on a PR to fix previous_priorities as a versionless special case, trying to avoid any new ones creeping in.
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Description: define empty config protos for all filters expecting google::protobuf::Empty
Risk Level: medium (change of config type)
Testing: unit
Docs Changes: done
Release Notes: define config protos for all extensions