proxy
proxy copied to clipboard
istio
authn: wasm implementation
What this PR does / why we need it: Draft of AuthN wasm implementation.
Which issue this PR fixes To resolve https://github.com/istio/istio/issues/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:
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.
@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 | f4fb7175d5249a6d5a958b8920ca5d257eade173 | 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.
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, 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 @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.
@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 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.
🧭 This issue or pull request has been automatically marked as stale because it has not had activity from an Istio team member since 2020-05-13. It will be closed on 2020-06-27 unless an Istio team member takes action. Please see this wiki page for more information. Thank you for your contributions.
Created by the issue and PR lifecycle manager.
@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?
@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.
