pulsar

pulsar copied to clipboard

Motivation

related to https://github.com/apache/pulsar/issues/16768

Modifications

1. no need to grant permission for each partition in internalGrantPermissionsOnTopic(), only grant permission on partitionedTopicName
2. do not revoke permission for each partition in internalRevokePermissionsOnTopic(), only revoke permission on partitionedTopicName
3. internalGetPermissionsOnTopic() should get the partitionedTopic permission
4. PulsarAuthorizationProvider#checkPermission() check permission by partitionedTopicName.

If create topic and grant permission before and revoke permission after this pr, the permission of topic partition would remain in zk metadata. This do not effect the authorization. When do delete topic operation, the remained metadata can be cleaned.

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

Check the box below or label this PR directly.

Need to update docs?

• [ ] doc-required (Your PR needs to update docs and you will update later)

• [x] doc-not-needed (Please explain why)

• [ ] doc (Your PR contains doc changes)

• [ ] doc-complete (Docs have been already added)

@nodece @Technoboy- @codelipenghui PTAL, this problem would result in permission can not be revoked

Hi @TakaHiR07, Thanks for your contribution! I think you should consider the compatibility here.

I have changed the proposal and I think it is the better way to solve this problem and meet the compatibility. Please take a look, @nodece @codelipenghui @BewareMyPower

pulsar old version

	partition (e.g., persistent://public/default/mytopic-partition-0)	based topic (e.g., persistent://public/default/mytopic)
grant	only partition	grant all partition and then grant based topic
revoke	only partition	revoke all partition and then revoke based topic (shutdown if throw exception)
get	only partition	only based topic
check	check partition and then check based topic	only based topic

pulsar new version

	partition (e.g., persistent://public/default/mytopic-partition-0)	based topic (e.g., persistent://public/default/mytopic)
grant	only partition	only based topic
revoke	only partition	revoke based topic and then revoke partition (it doesn't matter if revoke partition failed)
get	only partition	only based topic
check	check partition and then check based topic	only based topic

/pulsarbot rerun-failure-checks

This table looks confused, I think you should follow the https://github.com/apache/pulsar/pull/16792?notification_referrer_id=NT_kwDOAPe6cbM0MDcwMTQyMTMyOjE2MjM1MTIx&notifications_query=repo%3Aapache%2Fpulsar#discussion_r938428577, you also need to improve the get action.

It looks like you didn't update the org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider#checkPermission.

It looks like you didn't update the org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider#checkPermission.

Yes, I didn't update this for compatibility.

As you saw, the CI fails.

As you saw, the CI fails.

@nodece As you suggest, I have completely change the implementation of grant/revoke/get/check permission. Therefore many relevant unittest become flaky.

What do you think of this new implementation？ If there are no other problems, I would fix the relevant flaky test.

Thanks for your contribution @TakaHiR07. This is a great start. However, I think we'll need some additional work before it is ready to be merged.

First, I think this work will need to be two PRs.

There will be a PR to update how permissions are revoked. Essentially, we should not fail a request that attempts to remove permission from a partition of a topic that was not already granted permission. We will need to determine how to handle the case that a non-partitioned topic or all of the partitions of a partitioned topic (including the partitioned topic itself) does not have permission. This PR will be backwards compatible and will therefore be able to be cherry picked to existing release branches.

Then there is a PR that will change what the PersistentTopicsBase passes to the AuthorizationService. This PR is an optimization to limit the amount of metadata stored in ZK. Instead of passing each topic partition to the grantPermissionsAsync method, we'll just pass the topic that the user is attempting to grant permission on. This will technically be a breaking change for custom implemenations of the AuthorizationProvider interface, so we cannot cherry pick this change.

In order to be upgrade and downgrade compatible, we'll need to leave the revoke permission logic the same for a multiple minor versions. This work could be tracked in an issue.

Since both PRs will have to do with authorization and what the Pulsar Broker passes to the pluggable AuthorizationProvider, I think we should give a notice on the [email protected] mailing list to get consensus. I expect that this will not be a blocker for the feature/bug fix.

I agree with it. I open another PR to update how permissions are revoked.

This PR remain as the second PR to optimize the implementation of permission. I am not familiar with how to get consensus, so I would wait for update this pr until the implementation is approved.

This PR remain as the second PR to optimize the implementation of permission. I am not familiar with how to get consensus, so I would wait for update this pr until the implementation is approved.

You can do this simply by sending an email to the [email protected] email indicating the change you'd like to make. If you are unable to do this, I can send the email.

Guide: https://pulsar.apache.org/contributing

This PR remain as the second PR to optimize the implementation of permission. I am not familiar with how to get consensus, so I would wait for update this pr until the implementation is approved.

You can do this simply by sending an email to the [email protected] email indicating the change you'd like to make. If you are unable to do this, I can send the email.

Thanks for your guide, I have sent an email to [email protected].

It seems send fail because I can't see my email in dev mail list. I wish if you can help send this email @michaeljmarshall

@TakaHiR07 Any updates?

@TakaHiR07 Any updates?

Haven't updated. I would update the implementation in a few day.

The code has been updated. PTAL. @michaeljmarshall @nodece With #17393, we can simply change the grantPermission() at AuthorizationProvider, and internalGetPermissionsOnTopic() at PersistentTopicsBase

The pr had no activity for 30 days, mark with Stale label.

@TakaHiR07 hi, I move this PR to release/2.9.5, if you have any questions, please ping me. thanks.

The pr had no activity for 30 days, mark with Stale label.

Since we will start the RC version of 3.0.0 on 2023-04-11, I will change the label/milestone of PR who have not been merged.

• The PR of type feature is deferred to 3.1.0
• The PR of type fix is deferred to 3.0.1

So drag this PR to 3.0.1

@TakaHiR07 - I am so sorry for losing track of this PR. Are you interested in continuing your work on it? If not, I am going to pick up the work in a couple weeks. Thank you for your valuable observations and contributions so far!

@TakaHiR07 - I am so sorry for losing track of this PR. Are you interested in continuing your work on it? If not, I am going to pick up the work in a couple weeks. Thank you for your valuable observations and contributions so far!

There is another pr about this work, you can take a look of it. https://github.com/apache/pulsar/pull/18222. I am glad to continue improve this work