jwt_authn: change allow_missing implementation under RequiresAny#13839
jwt_authn: change allow_missing implementation under RequiresAny#13839mattklein123 merged 5 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
|
@yangminzhu Could you help to review this change? I believe Istio is the only project using this |
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
yangminzhu
left a comment
There was a problem hiding this comment.
Does this change the existing behavior of the API (allow_missing or allow_missing_or_failed)? From the description it looks like it's doing some internal optimization. But the other sentences seems implying it's going to change the API?
The code is really complicated and I will need some more time to take a closed look.
|
It doesn't change the API. It only changes internal optimization. And it only impacts the case where API functionalities are guarded by many unit-tests; group_verifier_test and all_verifier_test. |
|
@yangminzhu friending ping..... |
yangminzhu
left a comment
There was a problem hiding this comment.
Sorry for the late reply, this looks good to me, only a few small comments.
thanks for the confirm, just want to make sure we don't accidentally break the API behavior, also found the tests covering the Istio use cases in |
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
|
Thanks @yangminzhu for your review. Updated the PR. PTAL |
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
This is to fix: #13458
The current implementation of allow_missing or allow_missing_or_failed under RequiresAny is buggy. This is how it implemented currently. If you have following JwtRequirement:
We create following 3 verifiers:
The 3) veirifier causes two problems:
This PR will make following changes:
allow_missingorallow_failedflag in AnyVerifier. Use the flag in the final status aggregation.require_any. It is not needed any more.Risk Level: Low. It only impacts
allow_missingorallow_missing_or_failedflag underrequire_any. This is a rarely used feature.Testing: unit-tested
Docs Changes: None
Release Notes: None