xds: use filter type URL as the primary way to discover extensions #9618

kyessenov · 2020-01-09T02:52:01Z

Signed-off-by: Kuat Yessenov [email protected]

Description: use the type URL in typed_config to select an extension instead of name whenever it uniquely identifies it
Risk Level: medium (might affect invalid config with one extension proto used for another extension)
Testing: unit
Docs Changes: updated configuration overview section
Release Notes: use the config type URL for extension discovery
Fixes: #9587

Signed-off-by: Kuat Yessenov <[email protected]>

htuch · 2020-01-09T04:02:48Z

@kyessenov QQ for context: do we want to land this prior to 1.13.0 (given all the v3 disruption already happening) or wait until after? What's the thinking from Istio-side?

mattklein123 · 2020-01-09T04:16:42Z

Can someone describe simply how this effects existing configurations of filters that are either using the deprecated config field or have no config at all? Meaning, how is this backwards compatible until configuration is potentially updated? It's not acceptable to have any breaking changes here that require a lockstep config change.

mattklein123 · 2020-01-09T04:23:11Z

(And yes this is going to need some detailed documentation somewhere in the configuration guide including an overview of Any/Typed Struct/etc. This is all getting super confusing. I would recommend a new section somewhere in here: https://www.envoyproxy.io/docs/envoy/latest/configuration/overview/v2_overview (that page needs to be split up into multiple pages but that's a different issue which I can potentially tackle and will open an issue on)

kyessenov · 2020-01-09T04:55:45Z

@htuch We don't need it immediately, but given that it takes awhile for changes to propagate, it would be good to get in soon so that we can use it in the extension API. It doesn't need to be in 1.13.

@mattklein123 Any config that is not typed (deprecated struct, or struct inside typed_config) falls back to using name just like before. If the config is typed (e.g. any new empty type extension), then the type is used to look up an extension. The risk is that when the config proto and extension name don't match, Envoy silently accepts it by doing struct merge, but will stop working after this PR.

mattklein123 · 2020-01-09T05:00:01Z

@kyessenov and if someone is still using the deprecated raw config field (not typed config) it uses name also, right? If so this makes sense to me and I don't see any strong reason to wait till after 1.13.0 for this, other than more regression risk (which might be a good enough reason to wait).

kyessenov · 2020-01-09T05:01:50Z

Yes, basically any struct (old or extensions that don't want to have proto config type) is filtered both in the type registry and in lookup. So it will only kick-in if there is some non-struct type_url specified either in TypedStruct or directly in typed_config.

mattklein123 · 2020-01-09T05:03:26Z

Yes, basically any struct (old or extensions that don't want to have proto config type) is filtered both in the type registry and in lookup. So it will only kick-in if there is some non-struct type_url specified either in TypedStruct or directly in typed_config.

Awesome, thanks. Will review in detail tomorrow when I am fresh.

htuch

Looks good, the main thing I'm wondering about is the factories that still take Struct, this seems to be the same problem as Empty..
/wait

htuch · 2020-01-09T19:10:40Z

include/envoy/registry/registry.h

+          continue;
+        }
+
+        // skip untyped factories and factories that consume Struct


Which factories consume Struct?

Only test factories AFAICT.

It feels kind of bad that we need to have prod code here that exists just for test, any way we can improve that, e.g. switch tests to use some canonical TestConfigMessage?

I didn't think deeply about filters that use struct as config type. I'd out-right ban Struct but that requires deprecation and will to force everyone to create their own proto types. Can this be done separately? What we did with Empty was easier since it was mostly used in Envoy code base.

People are definitely using Empty outside of Envoy (we have filters at Lyft that have no config), but the Empty case should "just work" as there is no config needed. I agree with @kyessenov that we can't ban struct without a deprecation cycle as we don't know if anyone is using Struct for their filter configs. We could deprecate it though, but that would require a warning/stat/etc. and the full dance. Can you file an issue and TODO a follow up here?

source/common/config/utility.h

Signed-off-by: Kuat Yessenov <[email protected]>

kyessenov · 2020-01-10T00:00:23Z

/retest

repokitteh-read-only · 2020-01-10T00:00:28Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #9618 (comment) was created by @kyessenov.

see: more, trace.

Signed-off-by: Kuat Yessenov <[email protected]>

mattklein123

In general this LGTM but definitely needs docs per previous discussion. Thank you!

/wait

include/envoy/registry/registry.h

mattklein123 · 2020-01-10T00:56:21Z

include/envoy/registry/registry.h

+          continue;
+        }
+
+        // skip untyped factories and factories that consume Struct


People are definitely using Empty outside of Envoy (we have filters at Lyft that have no config), but the Empty case should "just work" as there is no config needed. I agree with @kyessenov that we can't ban struct without a deprecation cycle as we don't know if anyone is using Struct for their filter configs. We could deprecate it though, but that would require a warning/stat/etc. and the full dance. Can you file an issue and TODO a follow up here?

include/envoy/registry/registry.h

Signed-off-by: Kuat Yessenov <[email protected]>

kyessenov · 2020-01-10T06:45:08Z

I'm not sure how to debug the coverage test failure -- the failing test appear to pass locally with and without ASAN. Any clue would be appreciated.

kyessenov · 2020-01-22T05:14:44Z

/retest

repokitteh-read-only · 2020-01-22T05:14:50Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #9618 (comment) was created by @kyessenov.

see: more, trace.

kyessenov · 2020-01-22T06:14:36Z

/retest

repokitteh-read-only · 2020-01-22T06:14:40Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #9618 (comment) was created by @kyessenov.

see: more, trace.

mattklein123 · 2020-01-22T18:47:18Z

The reason for coverage giving me so much grief is because there are RegisterFactory calls inside test bodies in some tests that are picked up by other tests once they are combined into a single binary. We should probably ban global registration inside tests.

IIRC there are test hooks that should allow for both registering and unregistering, but yeah, this will need some cleaning up.

kyessenov · 2020-01-22T21:12:20Z

Now, it's ADS integration test and the error makes no sense. It says the Any type does not match the requested resource type. Same test passes alone. Is there some test that overrides something in protobuf globally?

Signed-off-by: Kuat Yessenov <[email protected]>

kyessenov · 2020-01-23T04:59:06Z

Found a second build bug: there are two TestFilters defined in router_test and router_upstream_log_test. They are almost the same except one has an extra field. Due to some randomness, the shorter wins sometimes and leads to undefined behavior.

mattklein123

Great work!

Signed-off-by: Jose Nino [email protected] Description: envoyproxy/envoy#9618 broke the iOS build due to missing symbols. envoyproxy/envoy#9875 fixes. However, in order to expedite a clean master branch this PR moves the Envoy ref back to a stable place. Note that Android logging is reverted. Also note that CI for iOS was not testing for liveliness, which is how the breakage went through in the first place. This PR also fixes that. Risk Level: low Testing: CI Fixes #646 Signed-off-by: Jose Nino <[email protected]>

Signed-off-by: Jose Nino [email protected] Description: #9618 broke the iOS build due to missing symbols. #9875 fixes. However, in order to expedite a clean master branch this PR moves the Envoy ref back to a stable place. Note that Android logging is reverted. Also note that CI for iOS was not testing for liveliness, which is how the breakage went through in the first place. This PR also fixes that. Risk Level: low Testing: CI Fixes #646 Signed-off-by: Jose Nino <[email protected]> Signed-off-by: JP Simard <[email protected]>

kyessenov added 2 commits January 8, 2020 18:50

xds: use filter type URL as the primary way to discover extensions

7442509

Signed-off-by: Kuat Yessenov <[email protected]>

fix test registry

aad3af2

Signed-off-by: Kuat Yessenov <[email protected]>

kyessenov marked this pull request as ready for review January 9, 2020 03:08

kyessenov added 2 commits January 8, 2020 19:10

merge fix

ed020ad

Signed-off-by: Kuat Yessenov <[email protected]>

merge fix

151f640

Signed-off-by: Kuat Yessenov <[email protected]>

htuch self-assigned this Jan 9, 2020

mattklein123 self-assigned this Jan 9, 2020

mattklein123 mentioned this pull request Jan 9, 2020

docs: split up configuration overview page into multiple pages #9622

Closed

htuch suggested changes Jan 9, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 9, 2020

add unit tests

f76c893

Signed-off-by: Kuat Yessenov <[email protected]>

repokitteh-read-only bot removed the waiting label Jan 9, 2020

fix asan

224d5cf

Signed-off-by: Kuat Yessenov <[email protected]>

mattklein123 requested changes Jan 10, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 10, 2020

kyessenov mentioned this pull request Jan 10, 2020

deprecate google.protobuf.Struct as the configuration type #9643

Closed

review

2e7c1fa

Signed-off-by: Kuat Yessenov <[email protected]>

repokitteh-read-only bot removed the waiting label Jan 10, 2020

kyessenov added 2 commits January 9, 2020 21:02

asan fix

65113cb

Signed-off-by: Kuat Yessenov <[email protected]>

hopefully fix coverage

6312737

Signed-off-by: Kuat Yessenov <[email protected]>

mattklein123 mentioned this pull request Jan 22, 2020

filter: Add an incomplete pluggable HTTP caching filter (CacheFilter). #7198

Closed

mattklein123 added the waiting label Jan 22, 2020

Merge remote-tracking branch 'upstream/master' into free-the-name

df32987

repokitteh-read-only bot removed the waiting label Jan 22, 2020

Merge remote-tracking branch 'upstream/master' into free-the-name

858dfed

kyessenov added 2 commits January 22, 2020 13:17

trying to debug on CI since my local build passes

0b875dd

Signed-off-by: Kuat Yessenov <[email protected]>

trying to debug on CI since my local build passes

a338edf

Signed-off-by: Kuat Yessenov <[email protected]>

mattklein123 added the waiting label Jan 22, 2020

kyessenov added 2 commits January 22, 2020 20:56

ODR violation

9f398d3

Signed-off-by: Kuat Yessenov <[email protected]>

Merge remote-tracking branch 'upstream/master' into free-the-name

1083c22

repokitteh-read-only bot removed the waiting label Jan 23, 2020

mattklein123 approved these changes Jan 23, 2020

View reviewed changes

mattklein123 merged commit 08ec4dd into envoyproxy:master Jan 23, 2020

junr03 mentioned this pull request Jan 30, 2020

envoy: move ref back to stable envoyproxy/envoy-mobile#648

Merged

kyessenov mentioned this pull request Feb 4, 2020

access_loggers: use new-style names #9921

Merged

mpuncel mentioned this pull request Jul 24, 2020

allow typed_per_filter_config to refer to "custom" HTTP filter names #12274

Closed

markdroth mentioned this pull request Oct 21, 2024

Refactor dns cluster api #36353

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xds: use filter type URL as the primary way to discover extensions #9618

xds: use filter type URL as the primary way to discover extensions #9618

kyessenov commented Jan 9, 2020 •

edited

Loading

htuch commented Jan 9, 2020

mattklein123 commented Jan 9, 2020

mattklein123 commented Jan 9, 2020

kyessenov commented Jan 9, 2020 •

edited

Loading

mattklein123 commented Jan 9, 2020

kyessenov commented Jan 9, 2020

mattklein123 commented Jan 9, 2020

htuch left a comment

htuch Jan 9, 2020

kyessenov Jan 9, 2020

htuch Jan 9, 2020

kyessenov Jan 9, 2020

mattklein123 Jan 10, 2020

kyessenov Jan 10, 2020

kyessenov commented Jan 10, 2020

repokitteh-read-only bot commented Jan 10, 2020

mattklein123 left a comment

mattklein123 Jan 10, 2020

kyessenov commented Jan 10, 2020

kyessenov commented Jan 22, 2020

repokitteh-read-only bot commented Jan 22, 2020

kyessenov commented Jan 22, 2020

repokitteh-read-only bot commented Jan 22, 2020

mattklein123 commented Jan 22, 2020

kyessenov commented Jan 22, 2020

kyessenov commented Jan 23, 2020 •

edited

Loading

mattklein123 left a comment

xds: use filter type URL as the primary way to discover extensions #9618

xds: use filter type URL as the primary way to discover extensions #9618

Conversation

kyessenov commented Jan 9, 2020 • edited Loading

htuch commented Jan 9, 2020

mattklein123 commented Jan 9, 2020

mattklein123 commented Jan 9, 2020

kyessenov commented Jan 9, 2020 • edited Loading

mattklein123 commented Jan 9, 2020

kyessenov commented Jan 9, 2020

mattklein123 commented Jan 9, 2020

htuch left a comment

Choose a reason for hiding this comment

htuch Jan 9, 2020

Choose a reason for hiding this comment

kyessenov Jan 9, 2020

Choose a reason for hiding this comment

htuch Jan 9, 2020

Choose a reason for hiding this comment

kyessenov Jan 9, 2020

Choose a reason for hiding this comment

mattklein123 Jan 10, 2020

Choose a reason for hiding this comment

kyessenov Jan 10, 2020

Choose a reason for hiding this comment

kyessenov commented Jan 10, 2020

repokitteh-read-only bot commented Jan 10, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Jan 10, 2020

Choose a reason for hiding this comment

kyessenov commented Jan 10, 2020

kyessenov commented Jan 22, 2020

repokitteh-read-only bot commented Jan 22, 2020

kyessenov commented Jan 22, 2020

repokitteh-read-only bot commented Jan 22, 2020

mattklein123 commented Jan 22, 2020

kyessenov commented Jan 22, 2020

kyessenov commented Jan 23, 2020 • edited Loading

mattklein123 left a comment

Choose a reason for hiding this comment

kyessenov commented Jan 9, 2020 •

edited

Loading

kyessenov commented Jan 9, 2020 •

edited

Loading

kyessenov commented Jan 23, 2020 •

edited

Loading