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

add new phase to WasmPlugin #3143

Merged
merged 3 commits into from
Apr 5, 2024
Merged

Conversation

zirain
Copy link
Member

@zirain zirain commented Apr 3, 2024

xref: istio/istio#50012

this will allow user insert WasmPlugin at the start of the filter chain.

@zirain zirain requested a review from a team as a code owner April 3, 2024 04:43
@istio-testing istio-testing added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 3, 2024
@@ -414,6 +414,9 @@ enum PluginPhase {

// Insert plugin before Istio stats filters and after Istio authorization filters.
STATS = 3;

// Insert plugin before Istio metadata filters, This will generally be at the start of the filter chain.
METADATA_EXCHANGE = 4;
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure "METADATA_EXCHANGE" should be encoded in our API. The other 3 are logical universal phases of a proxy. MX is a Istio specific internal implementation detail that will probably be changed.

cc @kyessenov

Copy link
Member Author

Choose a reason for hiding this comment

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

do you mean with MDS, there's no more MX filter?

Copy link
Contributor

Choose a reason for hiding this comment

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

MX filter should not be assumed to exist forever, and moreover, the data it provides cannot be trusted.
Maybe it's better to configure MX filter not to strip the header instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

sg

Copy link
Member Author

Choose a reason for hiding this comment

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

rethink about this, still worth add a new phase enable user insert WasmPlugin at the start of the filter chain?

@istio-testing
Copy link
Collaborator

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

Test name Commit Details Required Rerun command
release-notes_api 5e15af9 link false /test release-notes

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 zirain requested review from kyessenov and howardjohn April 5, 2024 12:25
@istio-testing istio-testing merged commit 21eb088 into istio:master Apr 5, 2024
4 of 5 checks passed
@kyessenov
Copy link
Contributor

We deliberately tried to assign semantic meaning to each phase. I don't know what "initial" actually means.

@howardjohn
Copy link
Member

Sorry didn't realize there was open discussion here before merging. @zirain can you discuss with @kyessenov and make any needed changes/revert before 1.22 if needed

@kyessenov
Copy link
Contributor

The config should be about the intent, not the mechanics. If you can explain what "initial" is supposed to be used for, and rename it, that would be enough.

@ericvn
Copy link

ericvn commented Apr 5, 2024

To further expand on this, what filters are run before the Istio authentication filters? From the issue, one would be the istio.metadata_exchange filter. What phase would include that filter and other filters that one would want to put their filter before? It sounds like calling that phase initial may not be the best. My read was that using Initial just meant that the filters would be put at the front of the line. I don't have a better name in mind.

@zirain zirain deleted the wasmplugin-phase branch April 6, 2024 02:29
@zirain
Copy link
Member Author

zirain commented Apr 6, 2024

INITIAL means Insert plugin at the start of the filter chain., just the other side of UNSPECIFIED_PHASE(This will generally be at the end of the filter chain, right before the Router.)

I think maybe METADATA_EXCHANGE is a better option, but as you said before istio should not assume it will be always there, do you have a better proposal than INITIAL ?

@ramaraochavali
Copy link
Contributor

IMO, AUTHN phase should cover most of the cases that need to run. I do not know what would be the use case for being the filter first? METADATA_EXCHANGE seems like a special case - may be we should the change the implementation of AUTHN Phase to insert before Metadata Exchange ?

@hzxuzhonghu
Copy link
Member

Something like iptabels raw table? Allow any audit, any data preprocessing?

@kyessenov
Copy link
Contributor

kyessenov commented Apr 8, 2024

@hzxuzhonghu Isn't that authentication? If you want to check or assert some security property, that's authentication?

@bleggett
Copy link
Contributor

bleggett commented Apr 10, 2024

April 10 WG decision was to revert this change, and solve the metadata exchange UX issue here without adding a new WasmPlugin stage: https://docs.google.com/document/d/1wsa06GGiq1LEGwhkiPP0FKIZJqdAiue-VeBonWAzAyk/edit#heading=h.6fcb5py1h0hy

zirain added a commit that referenced this pull request Apr 11, 2024
@zirain
Copy link
Member Author

zirain commented Apr 11, 2024

solve the metadata exchange UX

Can you explain how to do that?

@kyessenov
Copy link
Contributor

@zirain Can you read the metadata from the filter state? The details for encoding in either the filter state or the header are subject to change, so either way it's not a fully supported configuration.

istio-testing pushed a commit that referenced this pull request Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants