xds: define base interfaces for filter factories by kyessenov · Pull Request #9391 · envoyproxy/envoy

kyessenov · 2019-12-18T02:20:45Z

Signed-off-by: Kuat Yessenov kuat@google.com

Introduces a typed factory and an untyped factory as base types for all extension factories.
Code refactor to facilitate using the config type as the primary selection mechanism for an extension.
This change should not cause any behavioral change. The actual switch is in a follow-up PR.

Contributes to: #9358

Risk Level: low
Testing: existing tests pass
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: Kuat Yessenov <kuat@google.com>

jmarantz · 2019-12-18T03:20:57Z

@kyessenov can you look at CI? Thanks!

kyessenov · 2019-12-18T03:22:22Z

Sorry, I meant to click draft button. Working on tests.

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2019-12-18T07:34:02Z

/cc @htuch
since this overlaps with some API migration work.
A few observations after going through the code that might need attention:

some filters don't use typed_config field to host Any (e.g. dubbo)
some factories are married to using names to select a factory (UDP, protocol options, quic transport sockets)
we would need an API oracle that doesn't need extension name to migrate protobufs

htuch

@kyessenov we already have an API oracle for this, take a look at https://github.com/envoyproxy/envoy/blob/master/source/common/config/api_type_oracle.h. We leave breadcrumb links across different versions of messages. There is also a more complete API type database, which we don't link in for size reasons.

I think it would be nice to have a solution that works consistently across all forms of extension. I think that what you have uncovered is that we haven't been doing a great job at ensuring factories and extensions adopt consistent conventions, which is something that should be improved going forward.

Do you need to land this before the v3 API migration? I'm thinking it could be better to do after 1.13.0 if there are no dependencies. If there's something that you think we will need in v3 to make this tractable, would be good to know soon.

CC @envoyproxy/api-shepherds @yanavlasov (for factory naming conventions)

htuch · 2019-12-19T04:43:23Z

include/envoy/config/typed_config.h

+class TypedConfig {
+public:
+  /**
+   * @return ProtobufTypes::MessagePtr create empty config proto message for v2. The config, which


Given that folks are going to be starting to move to v3 in early 2020, do we want to do a lot of work to make this supported in v2?

We need some bridge to associate a protobuf type with a factory. We probably could invert this, and just use type to instantiate a proto. Either way works, I'm aiming for minimal disruption in this PR.

What do you think about getting #8933 landed? In this world, all extensions are typed..

Yes, that would be wonderful. I was going to special case empty config but would be better without that.

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2019-12-19T21:29:04Z

I don't have a strong need to land this before v3 migration. I think it would be good to make the transition smooth though, so it might be a good idea to relax the spec without breaking compatibility first. I find API-first method of detecting extensions is much better than using extension names. We could probably start a UDPA spec for a generic extension config in parallel:

message ExtensionConfig {
  // Resource name for the config (to be referred in the filter config discovery service)
  string name = 1;
  // Opaque config. The type URL is used to select an extension and upgrade the config if necessary.
  google.protobuf.Any typed_config = 2;
  // Transparent metadata for control plane annotations.
  core.Metadata metadata = 3;
}

yanavlasov · 2019-12-20T00:35:21Z

To make sure I understand. The type() method of extension factory provides URL string of corresponding proto message for configuring the extensions. In this way cofig consumer can find and instantiate filters of correct type, right?
Also, is this related to #9358 in any way?

htuch · 2019-12-20T02:49:57Z

@yanavlasov I tagged you for this comment of @kyessenov:

some factories are married to using names to select a factory (UDP, protocol options, quic transport sockets)

I'm generally in agreement with @kyessenov on API native conventions for conveying type. I'm hoping this naturally is a thing in v3 once we get rid of empty proto extensions.

mattklein123 · 2019-12-20T21:12:18Z

include/envoy/config/typed_metadata.h

  parse(const ProtobufWkt::Struct& data) const PURE;

  static std::string category() { return "typed_metadata"; }
+  static std::string type() { return ""; }


qq from a skim: what does type() mean and why is it set to empty string everywhere?

It's set to "" for factories that don't have protos defined.
type() means the protobuf message name for the config of a factory.

Is there some way to make this more obvious and less boilerplate either via templating, base class with a default, etc.?

the happy case is handled by TypedConfig base class. I was thinking we may want to revisit the unhappy cases and fix them, so wasn't optimizing for that. we can provide a default base class UntypedConfig for that.

Yeah something as simple as UntypedConfig IMO would make it a lot more clear, but up to you.

Would it make sense to have a common base class for all factories at this point? I think there might be a critical mass of attributes common to all factories.

Yeah, category, name, and type are sufficient for a base class.

yanavlasov · 2019-12-23T19:10:33Z

include/envoy/config/typed_config.h

+  /**
+   * @return config proto full name
+   */
+  virtual std::string type() {


Would it make sense to name it config_type_url()?

Maybe config_type? I didn't want to call it URL since it's not really a type URL as in Any. Calling message name is also confusing (unless you're used to protobuf lingua).

If we're going to bike shed, what about we replace the whole category and config type thing with kind, as in https://en.wikipedia.org/wiki/Kind_(type_theory).

Kind might confuse a k8s user, since in k8s the API group and version are separated from the kind. Kind means an "object kind" in that object-oriented world.

Why not use the type URL as in Any?

Type URL is ambiguous. It looks like proto_registry/proto_name where proto_registry is usually type.googleapis.com but it doesn't have to be. It's some sort of indicator about where to fetch the descriptor, although I haven't seen anyone using it that way.

Ok, if proto descriptor is less ambiguous than type URL then I should change corresponding field name in the Extension proto in PR #9301

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2019-12-24T01:08:53Z

Updated the change to remove any behavioral change, since refactoring got quite large. Please take a look.

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2019-12-26T18:16:09Z

There is an issue with registering factories based on proto objects. The factory registration call races ahead of the protobuf registration, which causes an issue with incomplete proto objects. There are two possible way to solve this:

make type to factory index be lazily constructed.
change the implementation to return a textual proto string; this would require some macro to convert a parametric C++ proto type to a proto type.

I moved the code that causes it out of this PR, so it'd be easier to tackle. Let me know if this PR is good enough.

kyessenov · 2019-12-26T18:27:25Z

Sent envoyproxy/envoy-filter-example#109 to fix the build error in a sister repo.

mattklein123

In general this approach LGTM.

mattklein123 · 2019-12-26T23:52:36Z

include/envoy/config/typed_config.h

+    if (ptr == nullptr) {
+      return "";
+    }


Should this ever return nullptr? Should this be an ASSERT/RELEASE_ASSERT?

Updated to use ASSERT. Seems reasonable since this interface requires a proto config.

htuch · 2019-12-27T04:23:50Z

include/envoy/config/typed_config.h

+   * arrives in an opaque google.protobuf.Struct message, will be converted to JSON and then parsed
+   * into this empty proto.
+   */
+  virtual ProtobufTypes::MessagePtr createEmptyConfigProto() PURE;


One thing we could do, and it doesn't have to be this PR, is move away from having each individual filter return empty protos. Instead, they could either return a type name or descriptor, and we could have more general machinery for assembling the empty proto. Might not be a big win, but just thought I'd throw it out there.

Agree. It also helps with the registration initialization fiasco, since protobuf initializer may run after factory initializers.

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2019-12-30T18:45:03Z

@htuch can you help merging the linked filter example PR to make tests pass here: envoyproxy/envoy-filter-example#109

Signed-off-by: Kuat Yessenov <kuat@google.com>

htuch

LGTM, thanks!

htuch · 2020-01-03T21:21:29Z

@mattklein123 any final thoughts on this one?

mattklein123 · 2020-01-03T21:47:37Z

Nope ship it! Nice work.

lizan · 2020-01-04T04:20:33Z

@kyessenov the CI failure is real because envoy-filter-example doesn't build with this change. Can you raise a PR there to accommodate this?

kyessenov · 2020-01-04T04:23:37Z

@lizan I already did that and it got merged. How are you supposed to update both repos in tandem?

lizan · 2020-01-04T04:58:13Z

@kyessenov https://github.com/envoyproxy/envoy/blob/master/ci/build_setup.sh#L104 update the SHA here.

Signed-off-by: Kuat Yessenov <kuat@google.com>

ci/build_setup.sh

xds: dis-associate filter name from filter factory

20a864f

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov requested review from alyssawilk, lizan, mattklein123 and zuercher as code owners December 18, 2019 02:20

jmarantz assigned junr03 Dec 18, 2019

fix some tests

b2aee3e

Signed-off-by: Kuat Yessenov <kuat@google.com>

htuch reviewed Dec 19, 2019

View reviewed changes

testing

e249c87

Signed-off-by: Kuat Yessenov <kuat@google.com>

junr03 assigned htuch Dec 19, 2019

yanavlasov mentioned this pull request Dec 20, 2019

config: Consolidate factory category names into common header file #9425

Closed

mattklein123 reviewed Dec 20, 2019

View reviewed changes

yanavlasov reviewed Dec 23, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into relax_filter_name

48905de

This was referenced Dec 24, 2019

Differentiate a Filter's "name" and a "type". #9466

Closed

Proxy-WASM configuration protos. #9256

Merged

make smaller change

217512e

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov requested review from jmarantz and snowp as code owners December 24, 2019 01:06

kyessenov added 2 commits December 23, 2019 17:12

stale comment

1fc3c60

Signed-off-by: Kuat Yessenov <kuat@google.com>

unresolved initialization fiasco

6edc214

Signed-off-by: Kuat Yessenov <kuat@google.com>

yanavlasov mentioned this pull request Dec 26, 2019

API: Add extension information the Node message. #9301

Merged

mattklein123 reviewed Dec 26, 2019

View reviewed changes

htuch reviewed Dec 27, 2019

View reviewed changes

kyessenov added 2 commits December 27, 2019 11:11

change to assert

144a5f1

Signed-off-by: Kuat Yessenov <kuat@google.com>

Merge remote-tracking branch 'upstream/master' into relax_filter_name

c8e424c

kyessenov added 2 commits December 30, 2019 14:07

fix signatures

b3a65bc

Signed-off-by: Kuat Yessenov <kuat@google.com>

Merge remote-tracking branch 'upstream/master' into relax_filter_name

a011cfc

htuch mentioned this pull request Jan 2, 2020

Per-extension empty message configs #8933

Closed

This was referenced Jan 2, 2020

update filter name() method envoyproxy/envoy-filter-example#109

Merged

sketch of an extension manifest istio/api#1221

Closed

kyessenov added 2 commits January 2, 2020 16:33

Merge remote-tracking branch 'upstream/master' into relax_filter_name

338865e

Merge remote-tracking branch 'upstream/master' into relax_filter_name

7f2cf51

kyessenov changed the title ~~xds: dis-associate filter name from filter factory~~ xds: define base interfaces for filter factories Jan 3, 2020

htuch previously approved these changes Jan 3, 2020

View reviewed changes

update filter-example git SHA

b0e7ee5

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov dismissed htuch’s stale review via b0e7ee5 January 4, 2020 05:11

kyessenov added 4 commits January 3, 2020 21:19

update filter-example git SHA

ffb764a

Signed-off-by: Kuat Yessenov <kuat@google.com>

merge fix

29d3448

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix format

afff1d5

Signed-off-by: Kuat Yessenov <kuat@google.com>

try removing the symlink

7406cef

Signed-off-by: Kuat Yessenov <kuat@google.com>

htuch reviewed Jan 7, 2020

View reviewed changes

ci/build_setup.sh Show resolved Hide resolved

lizan approved these changes Jan 7, 2020

View reviewed changes

lizan merged commit 154207e into envoyproxy:master Jan 7, 2020

Conversation

kyessenov commented Dec 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz commented Dec 18, 2019

Uh oh!

kyessenov commented Dec 18, 2019

Uh oh!

kyessenov commented Dec 18, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Dec 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanavlasov commented Dec 20, 2019

Uh oh!

htuch commented Dec 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov Dec 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov Dec 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanavlasov Dec 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Dec 24, 2019

Uh oh!

kyessenov commented Dec 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kyessenov commented Dec 26, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Dec 18, 2019 •

edited

Loading

kyessenov commented Dec 19, 2019 •

edited

Loading

kyessenov Dec 23, 2019 •

edited

Loading

kyessenov Dec 26, 2019 •

edited

Loading

yanavlasov Dec 26, 2019 •

edited

Loading

kyessenov commented Dec 26, 2019 •

edited

Loading