Skip to content

[fix][broker] Modify authorization to update topic's properties#18252

Merged
nodece merged 5 commits intoapache:masterfrom
yuruguo:modify_authorization_to_update_topic_properties
Oct 31, 2022
Merged

[fix][broker] Modify authorization to update topic's properties#18252
nodece merged 5 commits intoapache:masterfrom
yuruguo:modify_authorization_to_update_topic_properties

Conversation

@yuruguo
Copy link
Contributor

@yuruguo yuruguo commented Oct 28, 2022

Motivation

Current users with production permission can change the properties of topic in PR-#17238, which will cause the properties of topic to become uncontrollable due to arbitrary changes. In fact, only tenant administrator and super user can change them.

Modifications

  • Modify TopicOperation.PRODUCE to TopicOperation.UPDATE_METADATA in internalUpdatePropertiesAsync method

Documentation

  • doc-not-needed

Matching PR in forked repository

PR in forked repository:

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 28, 2022
@yuruguo yuruguo changed the title [Authorization] Modify authorization to update topic's properties [fix][Authorization] Modify authorization to update topic's properties Oct 28, 2022
@yuruguo yuruguo self-assigned this Oct 29, 2022
@yuruguo yuruguo added this to the 2.12.0 milestone Oct 29, 2022
@yuruguo yuruguo changed the title [fix][Authorization] Modify authorization to update topic's properties [fix][broker] Modify authorization to update topic's properties Oct 29, 2022
@labuladong
Copy link
Contributor

/pulsarbot rerun-failure-checks

@codelipenghui
Copy link
Contributor

@yuruguo It's a behavior change. It's better to have a discussion under the dev mailing list to make sure everyone can know what will be changed.

@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2022

Codecov Report

Merging #18252 (c6e9f99) into master (3f5acfe) will decrease coverage by 0.45%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18252      +/-   ##
============================================
- Coverage     38.91%   38.46%   -0.46%     
+ Complexity     8283     8203      -80     
============================================
  Files           683      683              
  Lines         67290    67323      +33     
  Branches       7218     7215       -3     
============================================
- Hits          26187    25893     -294     
- Misses        38095    38485     +390     
+ Partials       3008     2945      -63     
Flag Coverage Δ
unittests 38.46% <100.00%> (-0.46%) ⬇️

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

Impacted Files Coverage Δ
...pulsar/broker/admin/impl/PersistentTopicsBase.java 52.30% <100.00%> (-0.28%) ⬇️
.../pulsar/broker/service/SharedConsumerAssignor.java 3.70% <0.00%> (-57.41%) ⬇️
...lsar/broker/service/StickyKeyConsumerSelector.java 50.00% <0.00%> (-50.00%) ⬇️
...apache/pulsar/broker/service/TopicListService.java 10.65% <0.00%> (-44.27%) ⬇️
...apache/pulsar/broker/service/EntryAndMetadata.java 0.00% <0.00%> (-40.75%) ⬇️
...e/HashRangeExclusiveStickyKeyConsumerSelector.java 0.00% <0.00%> (-36.93%) ⬇️
...lsar/broker/loadbalance/impl/ThresholdShedder.java 3.26% <0.00%> (-30.07%) ⬇️
...tent/NonPersistentDispatcherMultipleConsumers.java 40.74% <0.00%> (-12.35%) ⬇️
...n/java/org/apache/pulsar/broker/service/Topic.java 9.09% <0.00%> (-9.10%) ⬇️
...org/apache/pulsar/broker/loadbalance/LoadData.java 58.33% <0.00%> (-8.34%) ⬇️
... and 32 more

@nodece
Copy link
Member

nodece commented Oct 31, 2022

@yuruguo It's a behavior change. It's better to have a discussion under the dev mailing list to make sure everyone can know what will be changed.

First, add this feature in #17238, which will release in the 2.12 version, no breaking changes.

Copy link
Contributor

@Technoboy- Technoboy- left a comment

Choose a reason for hiding this comment

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

LGTM

@yuruguo
Copy link
Contributor Author

yuruguo commented Oct 31, 2022

@yuruguo It's a behavior change. It's better to have a discussion under the dev mailing list to make sure everyone can know what will be changed.

First, add this feature in #17238, which will release in the 2.12 version, no breaking changes.

You are right @nodece, PTAL @codelipenghui

@nodece nodece merged commit 8985143 into apache:master Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

7 participants