extensions: strongly prefer type URL lookup #20397

kyessenov · 2022-03-17T19:09:24Z

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

Commit Message: Use type_url to look up extensions. This prevents the undesirable practice of putting invalid protobufs to avoid a type lookup or duplicating the type URL.
Risk Level: medium, affects extensions with duplicated type URLs or no configuration
Testing: yes
Docs Changes: yes, this has been the recommendation for awhile.
Release Notes: yes
Runtime Guard: envoy.reloadable_features.no_extension_lookup_by_name

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

repokitteh-read-only · 2022-03-17T19:09:27Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #20397 was opened by kyessenov.

see: more, trace.

kyessenov · 2022-03-17T22:23:41Z

Lots of issues... Will require a slog to clean up the codebase from duplicate type URL registrations.

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

repokitteh-read-only · 2022-03-18T05:19:18Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #20397 was synchronize by kyessenov.

see: more, trace.

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

source/common/runtime/runtime_features.cc

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

phlax · 2022-03-21T14:21:40Z

@markdroth would you mind reviwing the API changes here

/wait-any

markdroth · 2022-03-21T16:25:27Z

source/common/config/utility.h

-    Factory* factory = Utility::getFactoryByType<Factory>(message.typed_config());
-    if (factory != nullptr) {
-      return factory;
+    if (message.has_typed_config()) {


This seems like a step in the wrong direction. My understanding is that we want to go in the direction that the type_url always defines the extension to use. It should not be optional -- we should reject extensions for which there is no embedded config type. If an extension does not need any config, it should still define its own config message type, which can be empty. That way, we still have a way to uniquely identify it.

Note that gRPC already does the above. It has no concept of Envoy's extension names; the extension name idea is considered legacy and IMHO should be removed from Envoy.

The problem is the legacy definitions that allow empty typed_config (e.g. HTTP filters in listeners). I think the new definitions already require defined typed_config in which case this condition is always true, but we still have instances without any typed_config in the wild and in the tests here. The proposal is basically restrict the usage of extension names only to those remaining instances without any typed_config.

My concern is that this change does nothing to prevent new extensions from being added without a typed_config. And by documenting the fact that this does work, it actually seems to encourage people into doing that in the future, on the incorrect assumption that this is a supported and intended approach.

I would really prefer to lock this down rather than documenting the fact that it works. How about the following instead:

Fix all remaining extensions in the Envoy repo to have typed_configs.

Add a runtime option to reject extensions that do not specify a typed_config. Initially, this will be off by default, but in release N+1, it will be changed to be on by default.

Add a log message for any extension that does not specify a typed_config, warning people that the config will stop working by default in release N+1.

At some point in the future, we can remove the runtime option and support only extensions that specify typed_config.

@mattklein123 or @htuch, thoughts?

@markdroth The specific problem that I'm seeing is that the following pattern happens:

name: <extension_name> typed_config: @type_url: <unrelated proto type URL, e.g. struct>

Requiring typed config does not help with this problem. This PR does help. I agree with the second step to deprecate missing typed_config, but that's a different problem, and we can do it in a followup.

If an extension is being configured with the wrong type_url, that definitely seems like something that we should be rejecting. But I think we could eliminate this through basically the same process I described above. Basically, we would implement the warning message and the runtime option after we fail the type_url lookup, and it would be the same regardless of whether the type_url lookup fails because there was no type_url or because the type_url was invalid.

In other words, the code here would be something like this:

Factory* factory = Utility::getFactoryByType<Factory>(message.typed_config()); if (factory != nullptr) { return factory; } LogWarningMessage(); if (runtime_flag_allows_invalid_type_urls) { return Utility::getFactoryByName<Factory>(message.name()); } return nullptr;

@htuch Is there a way to use a codepath for-by-name lookup only in existing tests? I tried to get a sense of how much work it is to define protos for every test extension and that seems overwhelming.

I'm on a journey to set runtime guard for all legacy tests using by-name lookup. I found internal use for raw_buffer inside listener manager which I fixed, too.

@kyessenov I think we should file a bug for cleaning up the test extensions. It's a good bug for folks who want to make some early contributions I think (once we have an example PR to point them at to show what needs doing, which would be really useful).

#20476. The bulk of the work is done in this PR, we can make a small follow-up to demonstrate the clean-up.

SGTM, thanks, I'm good with this PR.

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

markdroth · 2022-03-22T14:27:54Z

/lgtm api

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

kyessenov · 2022-03-30T21:29:58Z

This PR has a high potential of having merge conflicts with newly introduced test extensions. Do you think we can merge it?

soulxu

thanks for the update, LGTM also

yanavlasov · 2022-03-31T14:45:49Z

This PR has a high potential of having merge conflicts with newly introduced test extensions. Do you think we can merge it?

Does it make sense to merge main first to make sure CI is green. Otherwise it would just be reverted.

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

kyessenov · 2022-03-31T17:21:59Z

Merged main and resolved a conflict.

alyssawilk · 2022-04-07T13:22:37Z

@yanavlasov this has been waiting 7 days. what's the plan here?

PiotrSikora · 2022-04-07T17:28:21Z

This broke //test/config_test:example_configs_test:

test/config_test/config_test.cc:137: Failure
Failed
'/b/f/w/_tmp/0060fce94c7e28a507f30c85c68f79b1/test/config_test/local_ratelimit_ratelimit-envoy.yaml' config failed. Error: Didn't find a registered implementation for 'envoy.filters.http.router' with type URL: ''
Stack trace:
  0x4526c78: Envoy::ConfigTest::ConfigTest::ConfigTest()
  0x451d8ea: Envoy::ConfigTest::run()

test/config_test/config_test.cc:137: Failure
Failed
'' config failed. Error: Didn't find a registered implementation for 'envoy.filters.http.router' with type URL: ''
Stack trace:
  0x4526c78: Envoy::ConfigTest::ConfigTest::ConfigTest()
  0x451ddae: Envoy::ConfigTest::run()

[  FAILED  ] ExampleConfigsTest.All (54314 ms)

mattklein123 · 2022-04-07T17:31:38Z

It's a merge race. Let's just fix it forward. cc @kyessenov

kyessenov · 2022-04-07T18:22:21Z

#20719 should fix this test.

@type

- Updated .bazelversion - Reacted to changes in envoyproxy/envoy#20397. Proto extensions are now looked up by @type rather than name in envoy. Getting onto this standard is too much work for this nighthawk update, so instead: - Added a runtime override for all integration test yaml files - Added a runtime override for fake stats sink in the proccess_test unit test Signed-off-by: Nathan Perry <nbperry@google.com>

howardjohn

I think a large number of docs are still using the outdated method and will no longer work. Some examples are docs/root/intro/arch_overview/http/http_connection_management.rst previous_hosts, a few using envoy.transport_sockets.raw_buffer - just some examples.

Do we have a plan to update these all?

kyessenov · 2022-04-13T17:16:31Z

Yes, those are not using validate code fragments, so it's difficult to identify all case. I can do a sweeping pass over router and raw_buffer since those seem like most common.

moderation · 2022-04-13T21:42:25Z

/cc @phlax as maybe there is a way to scan for non-validated code fragments ?

kyessenov · 2022-04-13T21:47:56Z

#20812 for a quick pass.

Use type_url to look up extensions. This prevents the undesirable practice of putting invalid protobufs to avoid a type lookup or duplicating the type URL. Risk Level: medium, affects extensions with duplicated type URLs or no configuration Testing: yes Docs Changes: yes, this has been the recommendation for awhile. Release Notes: yes Runtime Guard: envoy.reloadable_features.no_extension_lookup_by_name Signed-off-by: Kuat Yessenov <kuat@google.com>

extensions: strongly prefer type URL lookup if typed_config is set

6b78b30

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

kyessenov added 2 commits March 17, 2022 15:24

edits

737e7b1

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

fix tests

0e2443e

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

repokitteh-read-only bot added the api label Mar 18, 2022

repokitteh-read-only bot assigned markdroth Mar 18, 2022

kyessenov marked this pull request as ready for review March 18, 2022 05:19

kyessenov requested review from dio, rojkov, alyssawilk, mattklein123, antoniovicente and adisuissa as code owners March 18, 2022 05:19

kyessenov added 2 commits March 17, 2022 22:22

release note

36792bc

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

typo

cbb0e65

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

zhxie reviewed Mar 18, 2022

View reviewed changes

source/common/runtime/runtime_features.cc Outdated Show resolved Hide resolved

review

bf03443

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

kyessenov mentioned this pull request Mar 18, 2022

docs: add cncf/xds protos #20277

Merged

fix apple test

c8f5aed

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

kyessenov requested a review from yanavlasov as a code owner March 18, 2022 18:28

repokitteh-read-only bot added the waiting:any label Mar 21, 2022

markdroth reviewed Mar 21, 2022

View reviewed changes

repokitteh-read-only bot removed the waiting:any label Mar 21, 2022

detect failures

205a6f6

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

repokitteh-read-only bot removed the api label Mar 22, 2022

phlax assigned htuch Mar 22, 2022

review

02b130f

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

yanavlasov previously approved these changes Mar 28, 2022

View reviewed changes

kyessenov mentioned this pull request Mar 29, 2022

test: creating a test listener filter and moving the xds test over to it #20571

Merged

soulxu previously approved these changes Mar 31, 2022

View reviewed changes

merge fix

80da970

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

kyessenov dismissed stale reviews from soulxu and yanavlasov via 80da970 March 31, 2022 17:21

yanavlasov approved these changes Mar 31, 2022

View reviewed changes

wrowe merged commit 8cb6862 into envoyproxy:main Apr 7, 2022

This was referenced Apr 12, 2022

Update envoy to 90fb3c8 (2022-04-11) envoyproxy/nighthawk#828

Merged

Move nighthawk onto architecture that supports envoy proto extension lookup by type, not name envoyproxy/nighthawk#829

Closed

howardjohn reviewed Apr 13, 2022

View reviewed changes

howardjohn mentioned this pull request Apr 13, 2022

Update proxy binary istio/istio#38372

Merged

sampajano mentioned this pull request Apr 21, 2022

Update envoy version to 1.22 (with config updates) grpc/grpc-web#1222

Merged

valerian-roche mentioned this pull request Apr 22, 2022

test: Update http listener connection-manager configuration to use fully qualified config as required in envoy now. envoyproxy/go-control-plane#561

Merged

alyssawilk mentioned this pull request Oct 20, 2022

envoy_reloadable_features_no_extension_lookup_by_name deprecation #23598

Open

sam-heilbron mentioned this pull request Dec 5, 2022

Update xDS Demo solo-io/hoot#64

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extensions: strongly prefer type URL lookup #20397

extensions: strongly prefer type URL lookup #20397

kyessenov commented Mar 17, 2022 •

edited

Loading

repokitteh-read-only bot commented Mar 17, 2022

kyessenov commented Mar 17, 2022

repokitteh-read-only bot commented Mar 18, 2022

phlax commented Mar 21, 2022

markdroth Mar 21, 2022

kyessenov Mar 21, 2022

markdroth Mar 21, 2022

kyessenov Mar 21, 2022

markdroth Mar 21, 2022

kyessenov Mar 22, 2022 •

edited

Loading

kyessenov Mar 22, 2022

htuch Mar 23, 2022

kyessenov Mar 23, 2022

htuch Mar 25, 2022

markdroth commented Mar 22, 2022

kyessenov commented Mar 30, 2022

soulxu left a comment

yanavlasov commented Mar 31, 2022

kyessenov commented Mar 31, 2022

alyssawilk commented Apr 7, 2022

PiotrSikora commented Apr 7, 2022

mattklein123 commented Apr 7, 2022

kyessenov commented Apr 7, 2022

howardjohn left a comment

kyessenov commented Apr 13, 2022

moderation commented Apr 13, 2022

kyessenov commented Apr 13, 2022

extensions: strongly prefer type URL lookup #20397

extensions: strongly prefer type URL lookup #20397

Conversation

kyessenov commented Mar 17, 2022 • edited Loading

repokitteh-read-only bot commented Mar 17, 2022

kyessenov commented Mar 17, 2022

repokitteh-read-only bot commented Mar 18, 2022

phlax commented Mar 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markdroth commented Mar 22, 2022

kyessenov commented Mar 30, 2022

soulxu left a comment

Choose a reason for hiding this comment

yanavlasov commented Mar 31, 2022

kyessenov commented Mar 31, 2022

alyssawilk commented Apr 7, 2022

PiotrSikora commented Apr 7, 2022

mattklein123 commented Apr 7, 2022

kyessenov commented Apr 7, 2022

howardjohn left a comment

Choose a reason for hiding this comment

kyessenov commented Apr 13, 2022

moderation commented Apr 13, 2022

kyessenov commented Apr 13, 2022

kyessenov commented Mar 17, 2022 •

edited

Loading

kyessenov Mar 22, 2022 •

edited

Loading