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.

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

[PIP 97][SASL Authentication] Remove Deprecated SASL AuthenticationDataSource#authenticate Implementation #12955

Conversation

michaeljmarshall
Copy link
Member

@michaeljmarshall michaeljmarshall commented Nov 24, 2021

Master Issue: PIP 97 #12105

Motivation

When the SASL Authentication Provider was created in #3821, it introduced an authenticate method into the AuthenticationDataSource. This method is not necessary, and was marked as deprecated in PIP 97: #12105. This PR breaks the SASL Authentication Provider's reliance on the AuthenticationDataSource#authenticate method.

As an added benefit, the AuthenticationDataSource is only a data source now. It is no longer overloaded as a data source and a means of authenticating data.

I plan to submit a subsequent PR to remove the SaslAuthenticationDataSource. This change is valuable by itself because it removes the reliance on AuthenticationDataSource#authenticate.

Note also that SaslAuthenticationState#authenticate is also deprecated in PIP 97. I want to first get this merged before moving to the async variant.

Modifications

  • Update SaslAuthenticationDataSource to remove unnecessary method declarations.
  • Update constructor for SaslAuthenticationState to only take the PulsarSaslServer as a parameter.
  • Update implementation of several methods in SaslAuthenticationState so that they no longer rely on the SaslAuthenticationDataSource.

Verifying this change

This change is internal to the SASL provider itself. It is covered by existing tests.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): no
  • The public API: no
  • The schema: no
  • The default values of configurations: no
  • The wire protocol: no
  • The rest endpoints: no
  • The admin cli options: no
  • Anything that affects deployment: no

Documentation

  • no-need-doc

This is a completely internal update, so no docs need updating.

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 24, 2021
@michaeljmarshall michaeljmarshall changed the title [Authn] Deprecate AuthenticationDataSource#authenticate; Update SASL … [SASL Authentication] Remove Deprecated SASL AuthenticationDataSource#authenticate Implementation Nov 24, 2021
@michaeljmarshall
Copy link
Member Author

@jiazhai, @merlimat, @eolivelli - PTAL

@michaeljmarshall michaeljmarshall force-pushed the update-sasl-auth-provider-implementation branch from bacc743 to f82163c Compare November 24, 2021 05:30
@michaeljmarshall michaeljmarshall changed the title [SASL Authentication] Remove Deprecated SASL AuthenticationDataSource#authenticate Implementation [PIP 97][SASL Authentication] Remove Deprecated SASL AuthenticationDataSource#authenticate Implementation Nov 24, 2021
@michaeljmarshall
Copy link
Member Author

/pulsarbot run-failure-checks

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

+1

@merlimat merlimat merged commit 76bffd1 into apache:master Nov 29, 2021
@merlimat merlimat added this to the 2.10.0 milestone Nov 29, 2021
@merlimat merlimat added the type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use label Nov 29, 2021
@michaeljmarshall michaeljmarshall deleted the update-sasl-auth-provider-implementation branch November 29, 2021 18:45
fxbing pushed a commit to fxbing/pulsar that referenced this pull request Dec 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants