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.

Sign up for GitHub

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

Jump to bottom

support routing based on jwt claims #34974

Merged
merged 5 commits into from
Oct 26, 2021
Merged

support routing based on jwt claims #34974

merged 5 commits into from
Oct 26, 2021

Conversation

yangminzhu
Copy link
Contributor

@yangminzhu yangminzhu commented Sep 1, 2021 •
edited by istio-policy-bot
Loading

Please provide a description of this PR:
Adds support of the special header x-jwt-claims in virtual service for routing based on JWT claims.

This must be used with RequestAuthentication otherwise the routing will never match (covered in e2e tests), see more details in design doc: https://docs.google.com/document/d/1s8Lykf8-KUZTLiG4YMSCHvxBDwZzHlrrHyZ7cJymc_8/edit#heading=h.6p29w9kcduab

Note: will add release notes to make this feature in a follow PR

@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Sep 1, 2021
@istio-policy-bot istio-policy-bot added area/networking area/security release-notes-none Indicates a PR that does not require release notes. labels Sep 1, 2021
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 1, 2021
@istio-testing
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@google-cla google-cla bot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Sep 1, 2021
@yangminzhu yangminzhu force-pushed the route-jwt branch 6 times, most recently from 66ed50e to 4f4018c Compare September 2, 2021 02:38
@istio istio deleted a comment from istio-testing Sep 2, 2021
@yangminzhu yangminzhu force-pushed the route-jwt branch 3 times, most recently from 1c3044e to b9d3a4e Compare September 9, 2021 01:49
@istio-testing istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-rebase Indicates a PR needs to be rebased before being merged and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 9, 2021
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Sep 9, 2021
@yangminzhu
Copy link
Contributor Author

/test all

1 similar comment
@yangminzhu
Copy link
Contributor Author

/test all

@yangminzhu yangminzhu marked this pull request as ready for review September 9, 2021 23:02
@yangminzhu yangminzhu requested review from a team as code owners September 9, 2021 23:02
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Sep 9, 2021
@yangminzhu
Copy link
Contributor Author

@howardjohn @costinm @mandarjog PTAL, this is ready for review. Note I will have some follow-up PRs for things like code refactor of the authn filter go code generation, the release notes and probably more tests, thanks.

There also seems quite some tests fail due to the Envoy update (see #34188 for the failed envoy update) but the newly added tests for jwt route matching should pass.

howardjohn
howardjohn reviewed Sep 9, 2021
View reviewed changes
@yangminzhu yangminzhu added the do-not-merge/hold Block automatic merging of a PR. label Sep 10, 2021
This was referenced Sep 10, 2021
Closed
Merged
@yangminzhu
Copy link
Contributor Author

@howardjohn @costinm could you review the PR? thanks.

@yangminzhu yangminzhu mentioned this pull request Sep 14, 2021
Merged
10 tasks
howardjohn
howardjohn reviewed Sep 17, 2021
View reviewed changes
Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

lgtm, one small comment

@bianpengyuan bianpengyuan mentioned this pull request Oct 10, 2021
Open
@yangminzhu yangminzhu removed the do-not-merge/hold Block automatic merging of a PR. label Oct 21, 2021
howardjohn
howardjohn approved these changes Oct 26, 2021
View reviewed changes
pilot/pkg/networking/core/v1alpha3/route/route_internal_test.go
@@ -459,3 +461,63 @@ func TestSourceMatchHTTP(t *testing.T) {
})
}
}

func TestTranslateMetadataMatch(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice tests

pilot/pkg/networking/plugin/authn/authentication.go
@@ -67,6 +67,7 @@ func buildFilter(in *plugin.InputParams, mutable *networking.MutableObjects) err
if filter := applier.JwtFilter(); filter != nil {
mutable.FilterChains[i].HTTP = append(mutable.FilterChains[i].HTTP, filter)
}
// TODO(yangminzhu): Set DisableClearRouteCache to true on sidecars.
Copy link
Member

Choose a reason for hiding this comment

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

what is the impact until then? just performance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, and the performance impact should be really small. I will fix this right after this PR, just want to get this in first as it has been out for really long and avoid making the existing PR too large.

@yangminzhu
Copy link
Contributor Author

/test integ-telemetry-istiodremote-tests_istio

@yangminzhu
Copy link
Contributor Author

/test integ-pilot-multicluster-tests_istio

@istio-testing istio-testing merged commit 90e61d2 into istio:master Oct 26, 2021
@yangminzhu yangminzhu deleted the route-jwt branch October 26, 2021 01:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hzxuzhonghu hzxuzhonghu hzxuzhonghu left review comments

@howardjohn howardjohn howardjohn approved these changes

@costinm costinm Awaiting requested review from costinm

@linsun linsun Awaiting requested review from linsun

@nmittler nmittler Awaiting requested review from nmittler

Assignees
No one assigned
Labels
area/networking area/security cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. release-notes-none Indicates a PR that does not require release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yangminzhu @istio-testing @howardjohn @hzxuzhonghu @istio-policy-bot