Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Per-extension empty message configs #8933

Closed
htuch opened this issue Nov 7, 2019 · 12 comments
Closed

Per-extension empty message configs #8933

htuch opened this issue Nov 7, 2019 · 12 comments
Assignees
Labels
api/v3 Major version release @ end of Q3 2019 no stalebot Disables stalebot from closing an issue
Milestone

Comments

@htuch
Copy link
Member

htuch commented Nov 7, 2019

In v2, we used google.protobuf.Empty for the configuration for simple extensions that didn't require custom configuration. Now that we have Any as the canonical method for specifying extensions, this doesn't work as well, since it makes it difficult to move to a versioned per-extension message without breaking wire compatibility within a major version.

As part of the v3 works, we should give each existing filter that relies on google.protobuf.Empty a custom empty config message in v3 and deprecate the use of WKT here.

@htuch htuch added no stalebot Disables stalebot from closing an issue api/v3 Major version release @ end of Q3 2019 labels Nov 7, 2019
htuch added a commit that referenced this issue Nov 11, 2019
* Add an explicit threat model to the end user facing docs, link to this from SECURITY.md

* Switch all Envoy extensions to use a new macro `envoy_cc_extension`, mandating that extensions declare a security posture. Extensions can also optionally declare `alpha` or `wip` status.

* Tag all documentation sites with their well-known Envoy names.

* Introduce tooling to automagically populate a list of known trusted/untrusted extensions in the threat model docs.

* Generate API docs for extensions that depend on `google.protobuf.Empty`. This pattern is deprecated as per #8933, but we need these for tooling support meanwhile.

This work was motivated by oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=18370

Signed-off-by: Harvey Tuch <[email protected]>
mattklein123 pushed a commit to envoyproxy/data-plane-api that referenced this issue Nov 11, 2019
* Add an explicit threat model to the end user facing docs, link to this from SECURITY.md

* Switch all Envoy extensions to use a new macro `envoy_cc_extension`, mandating that extensions declare a security posture. Extensions can also optionally declare `alpha` or `wip` status.

* Tag all documentation sites with their well-known Envoy names.

* Introduce tooling to automagically populate a list of known trusted/untrusted extensions in the threat model docs.

* Generate API docs for extensions that depend on `google.protobuf.Empty`. This pattern is deprecated as per envoyproxy/envoy#8933, but we need these for tooling support meanwhile.

This work was motivated by oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=18370

Signed-off-by: Harvey Tuch <[email protected]>

Mirrored from https://github.com/envoyproxy/envoy @ 90d1094b32aa017f90cc8efcd379aeb143acabfc
@htuch htuch added this to the 1.13.0 milestone Nov 21, 2019
@htuch htuch added help wanted Needs help! and removed no stalebot Disables stalebot from closing an issue labels Nov 21, 2019
@derekargueta
Copy link
Member

@htuch Mind checking that I'm moving in the right direction? https://github.com/envoyproxy/envoy/compare/master...derekargueta:dereka/empty-proto?expand=1 Pretty straightforward - for every extension that is using Envoy::ProtobufWkt::Empty, create a proto with an empty message and replace it - but want to verify before I go on to HTTP filters

As for defensively preventing new extensions from using Envoy::ProtobufWkt::Empty, I'm thinking a check_format.py check?

@htuch
Copy link
Member Author

htuch commented Dec 2, 2019

@derekargueta looks good! The main thing we need to do in addition to what you have is allow Empty to continue working until we delete v2. When filter config comes in via an Any, the type URL needs to match whatever the filter expects. The way I would tackle this is in places like

if (!typed_config.value().empty()) {
, we have a special case for Empty.

@derekargueta
Copy link
Member

ah right, can't swap atomically or it'd break wire compatibility with v2 xDS servers. Will do some spelunking around envoy/source/common/config/utility.cc, can't say I'm super familiar with this area of the codebase yet, but from what you're saying I'm inferring that I'm clear to continue this approach and that the Empty protobuf can be indirectly handled through these utility routines. In the meantime I'll go ahead and finish out the HTTP filters too.

@htuch
Copy link
Member Author

htuch commented Dec 16, 2019

@derekargueta any update on how this is tracking for EOY?

@kyessenov
Copy link
Contributor

xref: #9358
This issue basically ensures uniqueness of config proto by filter in a category.
@derekargueta I can land a helping hand to push this through if you want.

@derekargueta
Copy link
Member

I'll have this done by Thursday EOD (shooting for tomorrow though). Thanks for offering help @kyessenov, I'll ping you on Slack if I need anything.

@derekargueta
Copy link
Member

Have all the new proto files ready here, the strategy I'm going down to support ProtobufWkt::Empty (somewhat based on the diff @kyessenov started - https://github.com/envoyproxy/envoy/pull/9391/files#diff-17967d56376bf9821edb17b67b8a7e63R180) is adding a usesGoogleProtobufWktEmpty() (or similar) to FactoryRegistry defaulting to false, then in the applicable factories override it to true. This way extensions can be looked up in the registry using the proto type name, but before returning the empty proto we can check if the factory is supposed to use ProtobufWkt::Empty for the wire. An alternative is using a protofbuf option, which I'm less familiar with. The last catch is that when we cut v3, we want to stop using ProtobufWkt::Empty and switch to the empty per-extension message.

Also, since we're coming to a tight deadline I'm fine letting someone else carry this across. I'm a bit resource-constrained at the moment with impending change freeze.

@htuch
Copy link
Member Author

htuch commented Dec 20, 2019

@derekargueta awesome, I like this plan. Thanks a bunch for doing all the ground work here. I think we shouldn't have to do a ton in terms of converting between typed protos, it's mostly about replacing type URLs during Any/TypedStruct conversion; on the wire, ProtobufWkt::Empty and the per-filter empty protos are identical modulo type URL.

@kyessenov if you have cycles to lend a hand taking us towards the ability to express filter type in the v3 APIs via config type, this is the PR :D

@htuch
Copy link
Member Author

htuch commented Jan 2, 2020

@derekargueta @kyessenov do either of you have cycles to land this in the next week (prior to 1.13.0)? Would be great to do this and align with #9391.

@kyessenov
Copy link
Contributor

@derekargueta I can take over the branch if you're OK. We just need to provide empty protos for all filters.

@derekargueta
Copy link
Member

@kyessenov yes feel free, I'm currently trying to fix clang-tidy, and have another internal request to implement 😓

@htuch htuch removed the help wanted Needs help! label Jan 7, 2020
@mattklein123 mattklein123 added the no stalebot Disables stalebot from closing an issue label Jan 7, 2020
@htuch
Copy link
Member Author

htuch commented Jan 8, 2020

Fixed in #9581

@htuch htuch closed this as completed Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api/v3 Major version release @ end of Q3 2019 no stalebot Disables stalebot from closing an issue
Projects
None yet
Development

No branches or pull requests

4 participants