Skip to content

Conversation

@Shikugawa
Copy link
Member

What this PR does / why we need it:
Draft of AuthN wasm implementation.

Which issue this PR fixes
To resolve istio/istio#15772

Special notes for your reviewer:
We can't build it into wasm because there is no suitable build system. I think that this shouldn't be built with make. Instead of this, we should apply bazelbuild to build wasm filter.

Release note:

@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Apr 13, 2020
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Apr 13, 2020
@istio-testing istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-ok-to-test labels Apr 13, 2020
@istio-testing
Copy link
Collaborator

Hi @Shikugawa. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@rshriram
Copy link
Member

/ok-to-test

@istio-testing istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Apr 13, 2020
@istio-testing
Copy link
Collaborator

istio-testing commented Apr 13, 2020

@Shikugawa: 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
test_proxy f4fb717 link /test test_proxy

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.

@mandarjog
Copy link
Contributor

@yangminzhu @diemtvu is this how you guys want to do it ?

@diemtvu
Copy link
Contributor

diemtvu commented Apr 13, 2020

That's fast. Thanks for the PR.

@yangminzhu, I think we should simplify this filter. At least have a new struct/proto for configuration, instead of reuse (alpha) proto. All the conditional validations and principal binding can also be removed. I guess it's best to have a short design for the new filter. Wdyt?

@yangminzhu
Copy link
Contributor

@yangminzhu, I think we should simplify this filter. At least have a new struct/proto for configuration, instead of reuse (alpha) proto. All the conditional validations and principal binding can also be removed. I guess it's best to have a short design for the new filter. Wdyt?

Yes, one of the major issues in today's istio_authn filter is its API is unnecessary complicated as it was originally designed for the old v1alpha1 APIs, the others are some unnecessary principal/identity transformations in multiple data structures. We should definitely have a new and simpler API if we're moving to a new filter implementation.

+1 for a short design doc on this.

@lizan lizan requested review from diemtvu and yangminzhu April 15, 2020 05:31
@yangminzhu
Copy link
Contributor

@lizan @Shikugawa

Can we have a redesign of the filter API? A one page doc of the new API would be very helpful, the new API could be very simplified as it only needs to support the beta Peer/RequestAuthN policy. Thanks.

@Shikugawa
Copy link
Member Author

@yangminzhu There is no filter API changes in this PR. I will send another PR even if we need to change it. I'd like this PR only to be re-implementation of WASM. Should we re-design it in this?

@yangminzhu
Copy link
Contributor

@yangminzhu There is no filter API changes in this PR. I will send another PR even if we need to change it. I'd like this PR only to be re-implementation of WASM. Should we re-design it in this?

well, that might be okay but the most important benefit of rewriting the authn filter in WASM is it cleans up the internal API and removes the tech-debt, right? If we just have the WASM implementation of the old API, we don't get much benefits with all these efforts (yet still need to test the stability of the WASM). And if we have the new API, much of the code can just be removed/simplified and thus not very worth for a through review for now.

@Shikugawa
Copy link
Member Author

@yangminzhu Right. I see. I will consider the new API. Thanks.

@istio-testing istio-testing added needs-rebase Indicates a PR needs to be rebased before being merged 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 May 25, 2020
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jul 15, 2020
@Shikugawa
Copy link
Member Author

@yangminzhu @bianpengyuan For now, building problem is resolved so that reopened to work this. But, this PR is too large to be hard to review. I prefer split out this large PR and merge incrementally to this. What do you think about it?

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jul 23, 2020
@istio-testing
Copy link
Collaborator

@Shikugawa: PR needs rebase.

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.

@zirain
Copy link
Member

zirain commented Oct 27, 2022

outdated

@zirain zirain closed this Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while needs-rebase Indicates a PR needs to be rebased before being merged ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. 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.

Evaluate authentication filter in WASM