Skip to content

[Authorization] Revert new AuthorizationProvider method - #13133

Merged
eolivelli merged 1 commit into
apache:masterfrom
michaeljmarshall:remove-unnecessary-method
Dec 6, 2021
Merged

[Authorization] Revert new AuthorizationProvider method#13133
eolivelli merged 1 commit into
apache:masterfrom
michaeljmarshall:remove-unnecessary-method

Conversation

@michaeljmarshall

@michaeljmarshall michaeljmarshall commented Dec 4, 2021

Copy link
Copy Markdown
Member

Motivation

In PR #12600, we added a method to the AuthorizationProvider that is not used outside of implementations of the AuthorizationProvider itself. Since this interface is public, we should make sure that all methods are necessary. Further, since we cherry picked the commit for #12600 to branch-2.9 and branch-2.8, we introduced a potentially breaking change for third party implementations of the AuthorizationProvider interface. While the commit itself isn't breaking, it does imply that the interface has a method that could be used by the authorization service or some other part of Pulsar. Since third party implementations might not have this method implemented, they could break if we started to use the method. Since #12600 includes an important bug fix, we should not couple an expansion of the API with a bug fix.

For added context, it is likely that this method was added because there are other similar methods in the interface. For example, the interface has the following methods: allowSinkOpsAsync, allowSourceOpsAsync, and allowFunctionOpsAsync. These methods are much older and are called by the AuthenticationService.

Note: this was only released in 2.9.0. It hasn't been in any other releases yet.
Note: the new method has not yet been published in any releases. It is set to go in 2.8.2 and 2.9.1.

Modifications

  • Remove the AuthorizationProvider#allowConsumeOpsAsync method from the interface and from all implementations of the interface.

Verifying this change

This is a trivial change. No underlying logic is changed by this commit.

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

This fix addresses a potentially breaking change.

Documentation

  • no-need-doc

@lhotari lhotari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good catch. LGTM

@michaeljmarshall

Copy link
Copy Markdown
Member Author

/pulsarbot run-failure-checks

@michaeljmarshall

Copy link
Copy Markdown
Member Author

/pulsarbot run-failure-checks

1 similar comment
@michaeljmarshall

Copy link
Copy Markdown
Member Author

/pulsarbot run-failure-checks

@codelipenghui

Copy link
Copy Markdown
Contributor

Note: the new method has not yet been published in any releases

I think the 2.9.0 already contains #12600? do you need to also revert the 2.9.0 release?

@michaeljmarshall

Copy link
Copy Markdown
Member Author

Note: the new method has not yet been published in any releases

I think the 2.9.0 already contains #12600? do you need to also revert the 2.9.0 release?

@codelipenghui - you're right, it was included in the 2.9.0 release. I thought it had missed the release because it has the label release/2.9.1. I'm not sure what the right choice is for branch-2.9. The release is not good to use in production, so perhaps it is okay to remove it? If it is not okay to remove it, we could annotate the method as deprecated. What do you think we should do?

@freeznet freeznet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@codelipenghui

Copy link
Copy Markdown
Contributor

@michaeljmarshall I think it's ok to remove it in 2.9.1 since we will not introduce breaking changes to 2.9.1, just a user implemented method will not be used. So we only have a breaking change in 2.9.0. /cc @merlimat Please help check.

@eolivelli eolivelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lgtm

@eolivelli
eolivelli merged commit d1156ca into apache:master Dec 6, 2021
@michaeljmarshall
michaeljmarshall deleted the remove-unnecessary-method branch December 6, 2021 16:44
michaeljmarshall added a commit that referenced this pull request Dec 6, 2021
@michaeljmarshall michaeljmarshall added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Dec 6, 2021
michaeljmarshall added a commit that referenced this pull request Dec 6, 2021
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

area/security cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life doc-not-needed Your PR changes do not impact docs release/2.8.2 release/2.9.1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants