protobuf: allow relaxed opaque message with no properties

[Shikugawa]

Signed-off-by: Shikugawa rei@tetrate.io

Commit Message: I faced a crash while opening config_dump.

[2021-06-24 14:48:48.771][934895][critical][assert] [source/common/protobuf/utility.cc:778] assert failure: type_url_field_descriptor != nullptr && value_field_descriptor != nullptr && reflection->HasField(*message, type_url_field_descriptor).

It is because we passed opaque message without any properties as follows.

- name: envoy.filters.http.router
  typed_config: {}

We can avoid this crash with providing as follows

- name: envoy.filters.http.router
  typed_config:
    "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router

But, initial way should be accepted even if config_dump because Envoy accepts it while bootstrap.
This PR fixes to accept relaxed opaque type and avoid crash with it.

Additional Description:
Risk Level: Low
Testing: Unit
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

[dmitri-d]

Here and the test below: expected gets a copy of actual after it's been (potentially) modified by the call to MessageUtil::redact. The equality comparison on the next line will always return true in this case. You probably want to reverse the order of lines 1006 and 1007? I think the return value of MessageUtil::redact should be verified too.

[htuch]

+1

[Shikugawa]

ah sorry. I overlooked.

[htuch]

Did you consider having Envoy just reject these invalid configs? CC @phlax who I know was looking at this recently (maybe same conversation?)

[phlax]

passing

- name: envoy.filters.http.router
  typed_config: {}

should work as i understand - so if its not working somewhere - like other than bootstrap - then probs a good idea to fix

in lots of config around the repo that previously had this, it has been replaced with just:

- name: envoy.filters.http.router

or similar i think for other fields - which i understood to work the same

[phlax]

Did you consider having Envoy just reject these invalid configs?

i think if we do that we need to deprecate as a ~large chunk of the demo/example/docs configs have had this for quite some time so its likely to have been used in production configs etc

[phlax]

related pr is here #17065

[Shikugawa]

Did you consider having Envoy just reject these invalid configs?

i think if we do that we need to deprecate as a ~large chunk of the demo/example/docs configs have had this for quite some time so its likely to have been used in production configs etc

+1. But I think that we can reject empty value on next major API version during bootstrap.

[htuch]

passing

- name: envoy.filters.http.router
  typed_config: {}

So, there are two things:

1. This is allowed as a legacy mechanism. We are pushing to avoid people relying on special names for filters to convey types and to use type URLs instead. This allows, for example, multiple filters of the same type in a single filter chain.
2. In the interest of not breaking the world, I think we can continue to allow these. I don't see the harm, but would prefer we encourage the new style where possible, and in particular, never add special purpose hacks for advanced use cases around these names (e.g. multiple filters per chain, overrides, etc.)

[htuch]

Is this case covered in test?

[Shikugawa]

added value exists but type_url not exists on typed_struct

[Shikugawa]

@htuch friendly ping

[htuch]

LGTM, thanks!