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

delete forward sni, cluster rewrite, and sni verifier filters #4958

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

kyessenov
Copy link
Contributor

@kyessenov kyessenov commented Sep 18, 2023

No longer used in Istio, and should be rewritten as Wasm if still needed.

Deletes some unused bazel dependencies.

Issue: envoyproxy/envoy#29681

Signed-off-by: Kuat Yessenov <kuat@google.com>
@kyessenov kyessenov requested a review from a team as a code owner September 18, 2023 19:34
@istio-testing istio-testing added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 18, 2023
@kyessenov kyessenov changed the title delete cluster rewrite filter delete cluster rewrite filter and sni verifier Sep 18, 2023
@kyessenov kyessenov changed the title delete cluster rewrite filter and sni verifier delete forward sni, cluster rewrite, and sni verifier filters Sep 18, 2023
Signed-off-by: Kuat Yessenov <kuat@google.com>
@istio-testing istio-testing added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 18, 2023
Copy link
Contributor

@lei-tang lei-tang left a comment

Choose a reason for hiding this comment

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

LGTM as Istio no longer uses these three filters. Beside Istio, are there other projects/products that use Istio proxy and these filters? Consider providing an advance notice (e.g., in Istio release notes, or on istio.io) about removing these filters to avoid breaking their users.

@kyessenov
Copy link
Contributor Author

The only way to use those is via EnvoyFilter, and there's no integration test, no doc, and no reference in the istio code base to them. We'll post a release note, but I think it's OK to try to remove them for now, and if there's a strong complaint we can get them back.

@istio-testing istio-testing merged commit e2ced9c into istio:master Sep 18, 2023
@frankbu
Copy link

frankbu commented Oct 5, 2023

@kyessenov Removing forward_downstream_sni will break IBM (and presumably other) production clusters that are still using the old egress wildcard pattern. We are going to switch to an alternative approach, but I think advance notice of this change is needed, before deleting it, because it was previously documented.

@kyessenov
Copy link
Contributor Author

@frankbu Ack, I checked with @howardjohn first, but didn't see EnvoyFilter in the docs. I think we can get envoyproxy/envoy#29844 from upstream merged by the release of 1.20, and it's going to be functionally equivalent. If not, we'll bring it back. I hope there's at least some testing somewhere in istio.

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.

4 participants