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][broker] do not grant permission for each partition to reduce unnecessary zk metadata #18222

Merged
merged 2 commits into from Jul 3, 2023

Conversation

TakaHiR07
Copy link
Contributor

@TakaHiR07 TakaHiR07 commented Oct 27, 2022

Master Issue: #16768

Motivation

The problem has been discussed before in #16792. And now I found that when creating a topic with large partition such as 500 on a cluster which already has many topics, it would frequently update zk node, put more pressure on zk, maybe result in zk node not available. Therefore I think we'd better fix this problem.

Since the discussion in #16792 , there are 2 ways to enhance grant permission module. This is the second implementation, which I think is a better way to fix, and can have better compatibility with previous version.

Modifications

do not grant permission for each partition

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 (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

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

Documentation

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

Matching PR in forked repository

PR in forked repository: TakaHiR07#1

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 27, 2022
@codelipenghui
Copy link
Contributor

@TakaHiR07 It's better to have a discussion on the dev mailing list. We will change the behavior of the current implementation. Just make sure everyone is clear about what we will change.

@TakaHiR07
Copy link
Contributor Author

TakaHiR07 commented Nov 8, 2022

@TakaHiR07 It's better to have a discussion on the dev mailing list. We will change the behavior of the current implementation. Just make sure everyone is clear about what we will change.

Discussion on https://lists.apache.org/thread/6hy1jl725r20xn48y8wh9t76k9f3lw4c. PTAL @codelipenghui @michaeljmarshall

@github-actions
Copy link

github-actions bot commented Dec 9, 2022

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

@nodece
Copy link
Member

nodece commented Jun 19, 2023

/pulsarbot rerun-failure-checks

@nodece nodece requested a review from tisonkun June 19, 2023 15:14
@github-actions github-actions bot removed the Stale label Jun 20, 2023
@TakaHiR07 TakaHiR07 force-pushed the enhance_grant_permission_on_topic_2 branch from 86046fc to 5fc1659 Compare July 3, 2023 02:41
@nodece
Copy link
Member

nodece commented Jul 3, 2023

This PR considers the partition topic as a whole, and the user needs to use the base topic instead of the partition topic, I think this change is correct.

If there is no problem, I will merge this PR after the CI is passed.

@TakaHiR07 TakaHiR07 force-pushed the enhance_grant_permission_on_topic_2 branch from 5fc1659 to 5e555e4 Compare July 3, 2023 06:26
@codecov-commenter
Copy link

Codecov Report

Merging #18222 (5e555e4) into master (c5237ea) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18222      +/-   ##
============================================
- Coverage     73.13%   73.13%   -0.01%     
+ Complexity    32101    32089      -12     
============================================
  Files          1871     1871              
  Lines        138982   138974       -8     
  Branches      15283    15281       -2     
============================================
- Hits         101651   101637      -14     
- Misses        29280    29285       +5     
- Partials       8051     8052       +1     
Flag Coverage Δ
inttests 24.14% <0.00%> (-0.06%) ⬇️
systests 25.06% <100.00%> (-0.06%) ⬇️
unittests 72.39% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...pulsar/broker/admin/impl/PersistentTopicsBase.java 64.31% <100.00%> (-0.15%) ⬇️

... and 63 files with indirect coverage changes

@nodece nodece added this to the 3.1.0 milestone Jul 3, 2023
@nodece nodece merged commit e8a3011 into apache:master Jul 3, 2023
44 checks passed
@eolivelli
Copy link
Contributor

@nodece why did you commit this patch?

It looks like a breaking change and it would have required a PIP and a discussion on the ML.
The discussion you linked doesn't seems to have been answered much (as seen by the ML archive)

Also in order to commit this kind of security related patches it is better to wait for a review by other experienced committers

@michaeljmarshall @codelipenghui @lhotari

@nodece
Copy link
Member

nodece commented Jul 3, 2023

In our authorization provider, we already check the partition topic and base topic permissions, see https://github.com/apache/pulsar/blob/branch-3.0/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java#L386-L397, so I think this PR can be merged into the master branch.

There may be some changes for third-party plugins, but for the built-in plugins, I think there is no breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker doc-not-needed Your PR changes do not impact docs ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants