[improve][broker] AuthenticationState authenticate data once #19280
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 synchronousauthenticate
method are in theOneStageAuthenticationState
and theTokenAuthenticationState
class constructors. Sinceauthenticate
will be replaced withauthenticateAsync
, we won't be able to rely on authenticating theauthData
in the constructor. Further, it is inefficient to verify authentication data twice, which currently happens forTokenAuthenticationState
in theProxyConnection
and 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
AuthenticationState
class is as follows:When PIP 97 is complete, step 2 will replace
authenticate
withauthenticateAsync
.Modifications
Add
AuthenticationProvider
method:newAuthState(SocketAddress remoteAddress, SSLSession sslSession)
. This method does not takeAuthData
to make it a bit clearer that class constructors are not meant to perform authentication. This method's default definition isOneStageAuthenticationState
. Users that want a customAuthenticationProvider
to use a differentAuthenticationState
implementation must override this method.Add
@Deprecated
annotation 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
ProxyConnection
andServerCnx
to use the newnewAuthState
method. This change could break customAuthenticationState
implementations as described in the first bullet point.Update
TokenAuthenticationState
andOneStageAuthenticationState
to only authenticate tokens in theauthenticate
method.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
AuthenticationProvider
interface.Documentation
doc-not-needed
We document the authentication interfaces in the code, and I've added docs there.
Matching PR in forked repository
PR in forked repository: michaeljmarshall#14