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

[improve][admin] Check if the topic existed before the permission operations #22547

Merged
merged 5 commits into from
Apr 26, 2024

Conversation

Technoboy-
Copy link
Contributor

@Technoboy- Technoboy- commented Apr 22, 2024

Motivation

The get/grant/revoke permissions to a non-existed topic will be ok which may confuse the user. So it's better to check the topic existed before the grant permissions.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@Technoboy- Technoboy- self-assigned this Apr 22, 2024
@Technoboy- Technoboy- added this to the 3.3.0 milestone Apr 22, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 22, 2024
@Technoboy- Technoboy- force-pushed the improve-rest-msg branch 2 times, most recently from cc12979 to 4c81fd9 Compare April 24, 2024 03:00
@Technoboy- Technoboy- requested a review from RobertIndie April 24, 2024 09:01
Copy link
Member

@RobertIndie RobertIndie left a comment

Choose a reason for hiding this comment

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

I think this should be a breaking change and we need a PIP for it. Users need to explicitly create a topic before granting the permission after this change. This doesn't seem to make sense in some cases. The admin would simply want to grant users permissions without requiring that the topic has already been created.

@Technoboy-
Copy link
Contributor Author

I think this should be a breaking change and we need a PIP for it. Users need to explicitly create a topic before granting the permission after this change. This doesn't seem to make sense in some cases. The admin would simply want to grant users permissions without requiring that the topic has already been created.

not a break change. because new grant topic permissions need to create topic first. old ones not impact

Copy link
Member

@RobertIndie RobertIndie left a comment

Choose a reason for hiding this comment

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

After checking the API doc: https://pulsar.apache.org/admin-rest-api/?version=2.10.5#operation/grantPermissionsOnTopic, it appears that a 404 error should occur if the topic does not exist when granting permission. Thus, this should be recognized as a bug. +1 for this change. Thanks.

@Technoboy- Technoboy- merged commit 69a600e into apache:master Apr 26, 2024
49 of 50 checks passed
@lhotari
Copy link
Member

lhotari commented May 14, 2024

@Technoboy- AuthorizationTest doesn't pass in branch-3.2 because this PR has been cherry-picked to branch-3.2 . This blocks 3.2.3 release. Would you be able to fix the problem? Thanks

@Technoboy-
Copy link
Contributor Author

@Technoboy- AuthorizationTest doesn't pass in branch-3.2 because this PR has been cherry-picked to branch-3.2 . This blocks 3.2.3 release. Would you be able to fix the problem? Thanks

Fixed. forgot to cherry-pick #22550

@lhotari
Copy link
Member

lhotari commented Jun 2, 2024

I think this should be a breaking change and we need a PIP for it. Users need to explicitly create a topic before granting the permission after this change. This doesn't seem to make sense in some cases. The admin would simply want to grant users permissions without requiring that the topic has already been created.

I agree, this is a breaking change. There are reports from Pulsar users that this breaks their Pulsar solutions.

@Technoboy- We need to restore the old behavior and possibly have a toggle for this new behavior.

@lhotari
Copy link
Member

lhotari commented Dec 11, 2024

The breaking change introduced by this PR is being addressed in #23709.

hanmz pushed a commit to hanmz/pulsar that referenced this pull request Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants