Skip to content

auto update extensions_build_config.bzl#4234

Closed
zirain wants to merge 3 commits intoistio:masterfrom
zirain:sync-extensions-bzl
Closed

auto update extensions_build_config.bzl#4234
zirain wants to merge 3 commits intoistio:masterfrom
zirain:sync-extensions-bzl

Conversation

@zirain
Copy link
Copy Markdown
Member

@zirain zirain commented Nov 21, 2022

avoid issue like #4231

cc @kyessenov @lei-tang

@zirain zirain requested a review from a team November 21, 2022 15:54
@istio-testing istio-testing added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 21, 2022
Copy link
Copy Markdown
Contributor

@kyessenov kyessenov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zirain What's the API for these extensions? I have no problem adding extensions that are used by Istio but I don't think EnvoyFilter is sustainable as an API. If users are de-facto relying on EnvoyFilter then we have to provide security fixes in the extensions you added. Are we comfortable providing support for all these extensions? I'd prefer if we opted-in the things we support rather than have the kitchen sink.

@istio-testing
Copy link
Copy Markdown
Collaborator

@zirain: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
test_proxy 6f8d83e link true /test test
test-asan_proxy 6f8d83e link true /test test-asan
test-tsan_proxy 6f8d83e link true /test test-tsan
Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@zirain
Copy link
Copy Markdown
Member Author

zirain commented Nov 22, 2022

the original thought is preventing manual sync this configuration with upstream, like istio/istio#42080
#4216 #4191 #4200

@kyessenov
Copy link
Copy Markdown
Contributor

@zirain That doesn't happen that often. The cluster work was to reduce the binary size for envoy mobile, which is a one-time thing. I think that's a good question for TOC, whether we auto-import new extensions in Envoy into Istio. I'm mostly concerned about the support, some extensions are experimental and not very stable and may later be removed from Envoy.

@zirain
Copy link
Copy Markdown
Member Author

zirain commented Nov 22, 2022

@zirain That doesn't happen that often. The cluster work was to reduce the binary size for envoy mobile, which is a one-time thing. I think that's a good question for TOC, whether we auto-import new extensions in Envoy into Istio. I'm mostly concerned about the support, some extensions are experimental and not very stable and may later be removed from Envoy.

another question is why some extensions have a different name with upstream?

if we do not want to support some extensions, we should add them to ISTIO_DISABLED_EXTENSIONS ?

@zirain zirain closed this Nov 23, 2022
@zirain zirain deleted the sync-extensions-bzl branch April 23, 2024 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants