[improve][broker] AuthenticationState authenticate data once#19280
Closed
michaeljmarshall wants to merge 2 commits intoapache:masterfrom
Closed
[improve][broker] AuthenticationState authenticate data once#19280michaeljmarshall wants to merge 2 commits intoapache:masterfrom
michaeljmarshall wants to merge 2 commits intoapache:masterfrom
Conversation
Member
Author
|
After thinking about this more, I think we should probably take a different direction. I'll follow up with another PR. |
Member
Author
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 97: #12105
Motivation
In order to implement PIP 97, it is essential that we remove all blocking calls to
provider.authenticate(authData). Two places where we currently call the synchronousauthenticatemethod are in theOneStageAuthenticationStateand theTokenAuthenticationStateclass constructors. Sinceauthenticatewill be replaced withauthenticateAsync, we won't be able to rely on authenticating theauthDatain the constructor. Further, it is inefficient to verify authentication data twice, which currently happens forTokenAuthenticationStatein theProxyConnectionand theServerCnx.This PR seeks to make explicit a paradigm that is already followed in the Pulsar code base. Specifically, that the contract for using an
AuthenticationStateclass is as follows:When PIP 97 is complete, step 2 will replace
authenticatewithauthenticateAsync.Modifications
Add
AuthenticationProvidermethod:newAuthState(SocketAddress remoteAddress, SSLSession sslSession). This method does not takeAuthDatato make it a bit clearer that class constructors are not meant to perform authentication. This method's default definition isOneStageAuthenticationState. Users that want a customAuthenticationProviderto use a differentAuthenticationStateimplementation must override this method.Add
@Deprecatedannotation tonewAuthState(AuthData authData, SocketAddress remoteAddress, SSLSession sslSession)and to several class constructors that were used by that method. Note: leave the implementations the same for broker extensions that use this method. This is the only backwards compatible portion of this PR.Update
ProxyConnectionandServerCnxto use the newnewAuthStatemethod. This change could break customAuthenticationStateimplementations as described in the first bullet point.Update
TokenAuthenticationStateandOneStageAuthenticationStateto only authenticate tokens in theauthenticatemethod.Verifying this change
There are new tests and there is already some test coverage.
Does this pull request potentially affect one of the following parts:
This change affects the
AuthenticationProviderinterface.Documentation
doc-not-neededWe document the authentication interfaces in the code, and I've added docs there.
Matching PR in forked repository
PR in forked repository: michaeljmarshall#14