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

jwt: support nested claims #3045

Merged
merged 4 commits into from
Oct 13, 2020
Merged

jwt: support nested claims #3045

merged 4 commits into from
Oct 13, 2020

Conversation

yangminzhu
Copy link
Contributor

What this PR does / why we need it:
istio/istio#21340

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

@yangminzhu yangminzhu requested a review from a team October 10, 2020 05:20
@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 Oct 10, 2020
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 10, 2020
@yangminzhu yangminzhu added this to the 1.8 milestone Oct 10, 2020
@yangminzhu
Copy link
Contributor Author

/test test-asan_proxy

@yangminzhu yangminzhu force-pushed the jwt-claims branch 3 times, most recently from b132da6 to 1e2674f Compare October 12, 2020 20:55
@bianpengyuan
Copy link
Contributor

Looks like there is memory leak in implementation

@yangminzhu
Copy link
Contributor Author

Looks like there is memory leak in implementation

@bianpengyuan I think it's due to the change of using string_view. I'm not sure why the tsan/asan still failed even after I move the claim.key() to be above the JsonValueAs, it seems it still get destroyed too early.

liminw
liminw reviewed Oct 12, 2020
View reviewed changes
Copy link

@liminw liminw left a comment

Choose a reason for hiding this comment

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

Looks good.

src/envoy/http/authn/authn_utils_test.cc Show resolved Hide resolved
@yangminzhu yangminzhu mentioned this pull request Oct 13, 2020
Merged
@istio-testing
Copy link
Collaborator

@yangminzhu: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
release-centos-test_proxy 0b13e76 link /test release-centos-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.

@istio-testing istio-testing merged commit b908a15 into istio:master Oct 13, 2020
@yangminzhu yangminzhu deleted the jwt-claims branch October 13, 2020 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bianpengyuan bianpengyuan bianpengyuan approved these changes

@liminw liminw liminw approved these changes

Assignees
No one assigned
Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
1.8
Development

Successfully merging this pull request may close these issues.
4 participants
@yangminzhu @bianpengyuan @istio-testing @liminw