[improve][admin] Check if the topic existed before the permission operations#22547
Conversation
cc12979 to
4c81fd9
Compare
RobertIndie
left a comment
There was a problem hiding this comment.
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 |
RobertIndie
left a comment
There was a problem hiding this comment.
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.
b34d6cd to
5a40a1f
Compare
|
@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 |
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. |
|
The breaking change introduced by this PR is being addressed in #23709. |
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
docdoc-requireddoc-not-neededdoc-complete