[fix][broker] TokenAuthenticationState: authenticate token only once#19314
Merged
lhotari merged 10 commits intoapache:masterfrom Feb 1, 2023
Merged
[fix][broker] TokenAuthenticationState: authenticate token only once#19314lhotari merged 10 commits intoapache:masterfrom
lhotari merged 10 commits intoapache:masterfrom
Conversation
Member
Author
|
Tests failures are legitimate. I'll look into those later tonight or tomorrow morning. |
Member
Author
|
I expect the tests to pass now. |
lhotari
approved these changes
Jan 27, 2023
Member
|
/pulsarbot rerun-failure-checks |
nodece
approved these changes
Jan 28, 2023
Member
|
There are some tests failures, for example https://github.com/apache/pulsar/actions/runs/4055811136/jobs/6979681080#step:11:943 |
lhotari
approved these changes
Feb 1, 2023
michaeljmarshall
added a commit
to michaeljmarshall/pulsar
that referenced
this pull request
Apr 19, 2023
…pache#19314) Co-authored-by: Lari Hotari <lhotari@apache.org> (cherry picked from commit 0273f31)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PIP: #12105
Builds on #19282 and similar to #19295.
Motivation
Pulsar always calls the
AuthenticationState#authenticatemethod to authenticateauthData. We can remove an unnecessary call toauthenticate, which saves some resources.Modifications
authDatain theTokenAuthenticationStateconstructor.AuthenticationProviderList#newAuthStateimplementation so that it authenticatesAuthDatathat is passed to it. Given the testing, this design might have been the original one. It's also possible that the original creators meant to put all states into the list of potential auth states.testNewHttpAuthStatewithtestAuthenticateHttpRequest. This change is an extension of [feat][broker] Update AuthenticationProvider to simplify HTTP Authn #19197 where we deprecatednewHttpAuthStatein favor ofauthenticateHttpRequest.AuthenticationProviderList#newAuthStateandAuthenticationProviderList#newHttpAuthStateto add all providers that do not throw an exception on the respectivenewAuthStateandnewHttpAuthStatemethod calls. This logic started to fail after I updatedTokenAuthenticationState#newAuthStateto not authenticate the token because the list logic considered the first one to not fail on initialization to be the one to put in the list. As such, the list never had more than one state. Given the logic in theAuthenticationListStateclass, this seems like an unintentional design. The side effect of this change is that authentication will be slightly more expensive because the list provider will use the "good" and the "bad" token provider in the auth state.AuthenticationServiceto only wrapAuthenticationProviderwithAuthenticationProviderListwhen there are multiple authentication providers. This change could be controversial because I ran into an issue whereProxySaslAuthenticationTestfailed because of a change I made in theAuthenticationProviderListlogic, and that test failure improved this PR. However, it seems completely unnecessary to wrap allAuthenticationProviders with theAuthenticationProviderListwhen it is rarely necessary.AuthenticationProvider#authenticateHttpRequestdefault implementation to match theAuthenticationProviderList#authenticateHttpRequesthandling, which caught all exceptions and threw them asAuthenticationException.Verifying this change
New tests are added and old tests also verify correctness.
Does this pull request potentially affect one of the following parts:
In a sense, this breaks an implicit contract that the class had. However, because the
getAuthRole()method will throw an exception if called incorrectly, it is likely that misuse of this class will result in a fail fast behavior.Documentation
doc-not-neededThis change affects third party broker plugins. We do not document these features yet, so the best we can do is add a comment in the release notes.
Matching PR in forked repository
PR in forked repository: michaeljmarshall#19