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] Exclude system topics from namespace level publish and dispatch rate limiting #23589

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

poorbarcode
Copy link
Contributor

@poorbarcode poorbarcode commented Nov 11, 2024

Motivation

The effect is huge if the pub&sub rate limiter takes effect for system topics, such as __change_events, transaction topics... Almost all topics can not work if the system topic encounters a throttling

Modifications

  • Removes the broker-level/namespace-level/resource-group-level pub&sub throttling for system topics to avoid the issue of almost all topics not working ( load up, delete and other actions). And left the topic-level policies there( it will work if needed )

Documentation

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

Matching PR in forked repository

PR in forked repository: x

@poorbarcode poorbarcode added this to the 4.1.0 milestone Nov 11, 2024
@poorbarcode poorbarcode self-assigned this Nov 11, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 11, 2024
@lhotari
Copy link
Member

lhotari commented Nov 11, 2024

The only possible concern in a multi-tenant system is that a malicious user could circumvent rate-limits by using a topic name that gets considered as a system topic. I guess this is a matter of the definition of the security model in the multi-tenancy support that rate limits cannot be used to protect against malicious users. We don't currently document the security model so it's hard to resolve this as part of this PR. I guess we could create a separate issue for documenting the security model for multi-tenancy.

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM

@lhotari lhotari changed the title [fix] [broker] Do not take effect pub&sub rate-limit for system topics [improve][broker] Exclude system topics from namespace level publish and dispatch rate limiting Nov 11, 2024
Copy link
Contributor

@gaoran10 gaoran10 left a comment

Choose a reason for hiding this comment

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

LGTM

@poorbarcode
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@codecov-commenter
Copy link

codecov-commenter commented Nov 12, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 74.31%. Comparing base (bbc6224) to head (4564f67).
Report is 728 commits behind head on master.

Files with missing lines Patch % Lines
...sar/broker/service/DisabledPublishRateLimiter.java 50.00% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23589      +/-   ##
============================================
+ Coverage     73.57%   74.31%   +0.73%     
- Complexity    32624    34436    +1812     
============================================
  Files          1877     1944      +67     
  Lines        139502   147080    +7578     
  Branches      15299    16216     +917     
============================================
+ Hits         102638   109299    +6661     
- Misses        28908    29354     +446     
- Partials       7956     8427     +471     
Flag Coverage Δ
inttests 27.31% <80.00%> (+2.73%) ⬆️
systests 24.35% <80.00%> (+0.03%) ⬆️
unittests 73.70% <80.00%> (+0.86%) ⬆️

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

Files with missing lines Coverage Δ
...rg/apache/pulsar/broker/service/AbstractTopic.java 87.93% <100.00%> (-0.06%) ⬇️
.../pulsar/broker/service/persistent/SystemTopic.java 84.21% <100.00%> (+4.21%) ⬆️
...sar/broker/service/DisabledPublishRateLimiter.java 50.00% <50.00%> (ø)

... and 653 files with indirect coverage changes

@poorbarcode
Copy link
Contributor Author

The only possible concern in a multi-tenant system is that a malicious user could circumvent rate limits by using a topic name that gets considered as a system topic. I guess this is a matter of the definition of the security model in the multi-tenancy support that rate limits cannot be used to protect against malicious users. We don't currently document the security model so it's hard to resolve this as part of this PR. I guess we could create a separate issue for documenting the security model for multi-tenancy.

Seems we never focused on the isolation for multi-tenants before, that should be a huge PIP, which contains Transaction(all namespaces use the same Transaction metadata store) and other components. 😂

Thanks for mentioning this, let me merge the PR first

@poorbarcode poorbarcode merged commit 9bcbb20 into apache:master Nov 12, 2024
66 of 69 checks passed
poorbarcode added a commit that referenced this pull request Nov 12, 2024
…and dispatch rate limiting (#23589)

(cherry picked from commit 9bcbb20)
poorbarcode added a commit that referenced this pull request Nov 12, 2024
…and dispatch rate limiting (#23589)

(cherry picked from commit 9bcbb20)
poorbarcode added a commit that referenced this pull request Nov 12, 2024
…and dispatch rate limiting (#23589)

(cherry picked from commit 9bcbb20)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 13, 2024
…and dispatch rate limiting (apache#23589)

(cherry picked from commit 9bcbb20)
(cherry picked from commit aa4dbf3)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 13, 2024
…and dispatch rate limiting (apache#23589)

(cherry picked from commit 9bcbb20)
(cherry picked from commit aa4dbf3)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 13, 2024
…and dispatch rate limiting (apache#23589)

(cherry picked from commit 9bcbb20)
(cherry picked from commit aa4dbf3)
nikhil-ctds added a commit to datastax/pulsar that referenced this pull request Nov 14, 2024
…and dispatch rate limiting (apache#23589)

(cherry picked from commit 9bcbb20)
(cherry picked from commit aa4dbf3)
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