[improve][broker] enhance grant permission module to reduce zk metadata #16792

TakaHiR07 · 2022-07-26T07:08:30Z

Motivation

related to #16768

Modifications

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	based topic	only based topic
revoke	revoke all partition granted in previous version then revoke based topic	revoke all partition granted in previous version then revoke based topic
get	get this partition and then get based	only based topic
check	check partition and then check based topic	only based topic

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)
doc-not-needed
(Please explain why)
doc
(Your PR contains doc changes)
doc-complete
(Docs have been already added)

TakaHiR07 · 2022-08-03T08:19:28Z

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

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

...common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java

nodece · 2022-08-04T08:51:17Z

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

TakaHiR07 · 2022-08-18T07:06:56Z

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

nodece · 2022-08-22T07:18:06Z

/pulsarbot rerun-failure-checks

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

nodece · 2022-08-22T08:12:50Z

This table looks confused, I think you should follow the #16792, you also need to improve the get action.

michaeljmarshall

Thanks for your updates @TakaHiR07. I left some comments, let me know what you think.

michaeljmarshall · 2022-10-17T04:55:58Z

...common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java

@@ -247,7 +247,8 @@ public CompletableFuture<Void> grantPermissionAsync(TopicName topicName, Set<Aut
                }
                throw new IllegalStateException("policies are in readonly mode");
            }
-            String topicUri = topicName.toString();
+            // Enhancement: only grant permission on based topic
+            String topicUri = topicName.getPartitionedTopicName();


I think this might have been a misunderstanding of my review. The optimization is simply that we do not recursively add topic names when granting permission in the PersistentTopicsBase#internalGrantPermissionsOnTopic method. I think it is valid to grant a role permission to produce to a single partition of a partitioned topic, so we shouldn't make this update here.

Good catch! All partitions should have the same permissions, is that right?

michaeljmarshall · 2022-10-17T05:00:00Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

-                      if (numPartitions > 0) {
-                          for (int i = 0; i < numPartitions; i++) {
-                              TopicName topicNamePartition = topicName.getPartition(i);
-                              future = future.thenCompose(unused -> grantPermissionsAsync(topicNamePartition, role,
-                                      actions));
-                          }
-                      }


This is all that is required for the optimization (regarding my other comment).

michaeljmarshall · 2022-10-17T05:03:22Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

@@ -273,6 +273,24 @@ protected CompletableFuture<Map<String, Set<AuthAction>>> internalGetPermissions
                        }
                    }
                }
+
+                // If topic is partitioned, add based topic permission


I do not think we should grant permission based on the partition's base topic. If the call is about a specific permission, we should respond with the result for that partition.

@michaeljmarshall Is your idea the same as I described in #16792 (comment) ?? That's what I thought at the beginning, marked as plan 1.

plan 2 is described in #16792 (comment), this one is designed based on the idea of @nodece .

Both of the two plan can enhance. I think plan 1 would have better compatibility with previous version, while plan 2 would have cleaner permission record.

congbobo184 · 2022-11-17T12:10:40Z

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

github-actions · 2023-01-06T02:03:04Z

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

poorbarcode · 2023-04-10T08:25:52Z

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

michaeljmarshall · 2023-06-09T06:04:06Z

@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 · 2023-06-09T06:20:33Z

@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. #18222. I am glad to continue improve this work

TakaHiR07 changed the title ~~Enhance grant permission on topic~~ fix can not revoke permission after update topic partition Aug 3, 2022

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Aug 3, 2022

nodece reviewed Aug 4, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Outdated Show resolved Hide resolved

nodece reviewed Aug 4, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Outdated Show resolved Hide resolved

nodece reviewed Aug 4, 2022

View reviewed changes

...common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java Outdated Show resolved Hide resolved

nodece added release/2.8.4 release/2.10.2 release/2.9.4 component/authorization labels Aug 4, 2022

nodece requested review from codelipenghui, lhotari and michaeljmarshall August 4, 2022 10:29

BewareMyPower added release/2.8.5 and removed release/2.8.4 labels Aug 5, 2022

codelipenghui assigned TakaHiR07 Aug 8, 2022

codelipenghui added this to the 2.12.0 milestone Aug 8, 2022

codelipenghui added the release/2.11.1 label Aug 8, 2022

TakaHiR07 force-pushed the enhance_grant_permission_on_topic branch from d4584a1 to 1ce5cd6 Compare August 18, 2022 06:54

TakaHiR07 closed this Aug 18, 2022

TakaHiR07 reopened this Aug 18, 2022

nodece reviewed Aug 22, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Outdated Show resolved Hide resolved

nodece reviewed Aug 22, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Outdated Show resolved Hide resolved

Jason918 added release/2.10.3 and removed release/2.10.2 labels Aug 27, 2022

TakaHiR07 force-pushed the enhance_grant_permission_on_topic branch from 1ce5cd6 to 0e5003a Compare August 31, 2022 07:37

nodece requested review from merlimat and Technoboy- October 14, 2022 04:02

michaeljmarshall requested changes Oct 17, 2022

View reviewed changes

TakaHiR07 mentioned this pull request Oct 27, 2022

[improve][broker] do not grant permission for each partition to reduce unnecessary zk metadata #18222

Merged

4 tasks

congbobo184 added release/2.9.5 and removed release/2.9.4 labels Nov 17, 2022

github-actions bot removed the Stale label Nov 18, 2022

tisonkun added area/authn and removed component/authorization labels Dec 6, 2022

github-actions bot added the Stale label Jan 6, 2023

liangyepianzhou added release/2.10.4 and removed release/2.10.3 labels Feb 1, 2023

coderzc added release/2.9.6 and removed release/2.9.5 labels Mar 6, 2023

poorbarcode added release/3.0.1 and removed release/2.8.5 release/2.9.6 labels Apr 10, 2023

poorbarcode modified the milestones: 3.0.0, 3.1.0 Apr 10, 2023

github-actions bot removed the Stale label Apr 12, 2023

RobertIndie added release/3.0.2 and removed release/3.0.1 labels Jun 9, 2023

Technoboy- modified the milestones: 3.1.0, 3.2.0 Jul 31, 2023

TakaHiR07 closed this Sep 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improve][broker] enhance grant permission module to reduce zk metadata #16792

[improve][broker] enhance grant permission module to reduce zk metadata #16792

TakaHiR07 commented Jul 26, 2022 •

edited

Loading

TakaHiR07 commented Aug 3, 2022

nodece commented Aug 4, 2022

TakaHiR07 commented Aug 18, 2022 •

edited

Loading

nodece commented Aug 22, 2022

nodece commented Aug 22, 2022

michaeljmarshall left a comment

michaeljmarshall Oct 17, 2022

nodece Oct 17, 2022

michaeljmarshall Oct 17, 2022

michaeljmarshall Oct 17, 2022

TakaHiR07 Oct 17, 2022

congbobo184 commented Nov 17, 2022

github-actions bot commented Jan 6, 2023

poorbarcode commented Apr 10, 2023

michaeljmarshall commented Jun 9, 2023

TakaHiR07 commented Jun 9, 2023

[improve][broker] enhance grant permission module to reduce zk metadata #16792

[improve][broker] enhance grant permission module to reduce zk metadata #16792

Conversation

TakaHiR07 commented Jul 26, 2022 • edited Loading

Motivation

Modifications

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

Documentation

TakaHiR07 commented Aug 3, 2022

nodece commented Aug 4, 2022

TakaHiR07 commented Aug 18, 2022 • edited Loading

nodece commented Aug 22, 2022

nodece commented Aug 22, 2022

michaeljmarshall left a comment

Choose a reason for hiding this comment

michaeljmarshall Oct 17, 2022

Choose a reason for hiding this comment

nodece Oct 17, 2022

Choose a reason for hiding this comment

michaeljmarshall Oct 17, 2022

Choose a reason for hiding this comment

michaeljmarshall Oct 17, 2022

Choose a reason for hiding this comment

TakaHiR07 Oct 17, 2022

Choose a reason for hiding this comment

congbobo184 commented Nov 17, 2022

github-actions bot commented Jan 6, 2023

poorbarcode commented Apr 10, 2023

michaeljmarshall commented Jun 9, 2023

TakaHiR07 commented Jun 9, 2023

TakaHiR07 commented Jul 26, 2022 •

edited

Loading

TakaHiR07 commented Aug 18, 2022 •

edited

Loading