[ISSUE #3281]fix fail to delete topic perm list and global white address(#3128) #3280

yuz10 · 2021-08-20T14:45:00Z

Make sure set the target branch to develop

What is the purpose of the change

#3281 fix fail to delete topic perm list

#3128 still cant delete global white address
- the pr #3132 missed the client side, and its still sending null, resulting in global white address not updated

#3177 updateAclConfig in all brokers
- the pr #3184 only chages updateAclConfig, missed deleteAccessConfig and updateGlobalWhiteAddr

Brief changelog

XX

Verifying this change

XXXX

Follow this checklist to help us incorporate your contribution quickly and easily. Notice, it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR.

Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Write necessary unit-test(over 80% coverage) to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
Run mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle to make sure basic checks pass. Run mvn clean install -DskipITs to make sure unit-test pass. Run mvn clean test-compile failsafe:integration-test to make sure integration-test pass.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

coveralls · 2021-08-20T17:45:29Z

Coverage increased (+0.08%) to 54.304% when pulling f9f432a on yuz10:develop into 2cac866 on apache:develop.

codecov-commenter · 2021-08-20T17:45:47Z

Codecov Report

Merging #3280 (f9f432a) into develop (7c7e9ac) will increase coverage by 0.05%.
The diff coverage is 75.00%.

@@              Coverage Diff              @@
##             develop    #3280      +/-   ##
=============================================
+ Coverage      48.02%   48.08%   +0.05%     
- Complexity      4564     4575      +11     
=============================================
  Files            552      552              
  Lines          36536    36589      +53     
  Branches        4822     4826       +4     
=============================================
+ Hits           17546    17592      +46     
- Misses         16768    16773       +5     
- Partials        2222     2224       +2

Impacted Files	Coverage Δ
...ocketmq/broker/processor/AdminBrokerProcessor.java	`7.92% <0.00%> (ø)`
...che/rocketmq/acl/plain/PlainPermissionManager.java	`71.87% <100.00%> (+3.12%)`	⬆️
...g/apache/rocketmq/client/impl/MQClientAPIImpl.java	`12.18% <100.00%> (ø)`
.../main/java/org/apache/rocketmq/common/UtilAll.java	`41.21% <100.00%> (+0.95%)`	⬆️
...rg/apache/rocketmq/common/stats/StatsSnapshot.java	`84.61% <0.00%> (-15.39%)`	⬇️
...in/java/org/apache/rocketmq/test/util/MQAdmin.java	`38.88% <0.00%> (-5.56%)`	⬇️
...va/org/apache/rocketmq/common/stats/StatsItem.java	`50.00% <0.00%> (-5.00%)`	⬇️
...org/apache/rocketmq/common/stats/StatsItemSet.java	`41.79% <0.00%> (-2.99%)`	⬇️
...a/org/apache/rocketmq/store/StoreStatsService.java	`29.50% <0.00%> (-0.66%)`	⬇️
...ocketmq/broker/processor/SendMessageProcessor.java	`39.74% <0.00%> (-0.53%)`	⬇️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c7e9ac...f9f432a. Read the comment docs.

zongtanghu · 2021-08-23T05:42:28Z

Can you resolve the codes' conflicts in your local enviroments. @yuz10

yuz10 · 2021-08-23T05:55:36Z

resolved. @zongtanghu

zongtanghu · 2021-08-27T02:04:25Z

I see the travis ci test failed. Please check the reason of travis ci failed. @yuz10

yuz10 · 2021-08-27T02:52:53Z

I see the travis ci test failed. Please check the reason of travis ci failed. @yuz10

The tests are fixed

vongosling · 2021-08-27T04:16:20Z

@yuz10 Generally speaking, one pr solves the one issue. I am confused by your topic. could you clarify why you involve a big pr to solve so many problems, although I see we really have many problems in the ACL module?

yuz10 · 2021-08-27T04:30:06Z

@vongosling UtilAll.string2List is invoked by both topicPerms and globalWhiteAddrs，and modify the function can solve two issue s. The other issue, "updateAclConfig in all brokers" can be seperated, If you request

vongosling · 2021-08-27T04:42:24Z

I hope to see changes to UtilAll class, and you'll have a new idea right away. That's we could gradually strip out some of the cohesive functions to form new proprietary tool classes :-) We don't need such a God class, string2list is also confused :-(

…f acl config

yuz10 · 2021-08-27T14:23:09Z

@vongosling I have removed some confusing functions from the UtilAll class.

zongtanghu · 2021-09-04T04:12:21Z

client/src/main/java/org/apache/rocketmq/client/impl/MQClientAPIImpl.java

+    private static String join(List<String> list, String splitter) {
+        if (list == null) {
+            return null;
+        }
+        StringBuilder str = new StringBuilder();
+        for (int i = 0; i < list.size(); i++) {
+            str.append(list.get(i));
+            if (i == list.size() - 1) {
+                break;
+            }
+            str.append(splitter);
+        }
+        return str.toString();
+    }


Maybe, you put this function into UtilAll or AclUtils class is better!

Thanks for your advice, I have moved this two functions to UtilAll

zongtanghu · 2021-09-04T04:12:59Z

broker/src/main/java/org/apache/rocketmq/broker/processor/AdminBrokerProcessor.java

+    private static List<String> split(String str, String splitter) {
+        if (str == null) {
+            return null;
+        }
+
+        String[] addrArray = str.split(splitter);
+        return Arrays.asList(addrArray);
+    }


The code-review advice is the same as anthor one.

zongtanghu · 2021-09-04T04:14:05Z

And you need to notice the coverage decrease issues!Maybe, add some more unit test case codes.

…al white address(apache#3128) (apache#3280) * fix fail to remove topic/group permissions and global white address of acl config * update acl config for all brokers * refactor rename UtilAll.string2List and list2String * fix fail to remove whiteRemoteAddress of acl config

yuz10 requested a review from zongtanghu August 24, 2021 03:43

yuz10 changed the title ~~[ISSUE #3281 #3177 #3128][acl] fix fail to delete topic perm list and #3128 still cant delete global white address, and #3177 updateAclConfig in all brokers~~ [ISSUE #3281 ][acl] fix fail to delete topic perm list and global white address(#3128) Aug 27, 2021

yuz10 added 2 commits August 27, 2021 22:07

fix fail to remove topic/group permissions and global white address o…

efd1f04

…f acl config

update acl config for all brokers

ef6d54a

yuz10 force-pushed the develop branch from 6d40658 to 536b0f7 Compare August 27, 2021 14:13

vongosling approved these changes Aug 28, 2021

View reviewed changes

vongosling added this to the 4.9.2 milestone Aug 28, 2021

zongtanghu reviewed Sep 4, 2021

View reviewed changes

yuz10 force-pushed the develop branch from 536b0f7 to d37b7fb Compare September 4, 2021 08:42

refactor rename UtilAll.string2List and list2String

49358c7

yuz10 force-pushed the develop branch from d37b7fb to 49358c7 Compare September 4, 2021 08:49

vongosling approved these changes Sep 8, 2021

View reviewed changes

vongosling added the progress/wip Work in progress. Accept or refused to be continue. label Sep 8, 2021

fix fail to remove whiteRemoteAddress of acl config

f9f432a

vongosling merged commit 147d23e into apache:develop Sep 14, 2021

francisoliverlee changed the title ~~[ISSUE #3281 ][acl] fix fail to delete topic perm list and global white address(#3128)~~ [ISSUE #3281]fix fail to delete topic perm list and global white address(#3128) Oct 11, 2021

onejimmyboy mentioned this pull request Sep 15, 2023

UpdateAclConfig, unable to clear groupPerms, topicPerms, and whiteRemoteAddress. Currently, at least one piece of data exists #7362

Closed

onejimmyboy mentioned this pull request Sep 19, 2023

[ISSUE #7370]The UpdateAccessConfigSubCommand instruction adds descriptions on how to delete all whiteRemoteAddress, topicPerms, and groupPerms #7368

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ISSUE #3281]fix fail to delete topic perm list and global white address(#3128) #3280

[ISSUE #3281]fix fail to delete topic perm list and global white address(#3128) #3280

Uh oh!

yuz10 commented Aug 20, 2021 •

edited

Loading

Uh oh!

coveralls commented Aug 20, 2021 •

edited

Loading

Uh oh!

codecov-commenter commented Aug 20, 2021 •

edited

Loading

Uh oh!

zongtanghu commented Aug 23, 2021

Uh oh!

yuz10 commented Aug 23, 2021

Uh oh!

zongtanghu commented Aug 27, 2021

Uh oh!

yuz10 commented Aug 27, 2021

Uh oh!

vongosling commented Aug 27, 2021

Uh oh!

yuz10 commented Aug 27, 2021

Uh oh!

vongosling commented Aug 27, 2021 •

edited

Loading

Uh oh!

yuz10 commented Aug 27, 2021

Uh oh!

zongtanghu Sep 4, 2021

Uh oh!

yuz10 Sep 11, 2021

Uh oh!

zongtanghu Sep 4, 2021

Uh oh!

zongtanghu commented Sep 4, 2021

Uh oh!

Uh oh!

[ISSUE #3281]fix fail to delete topic perm list and global white address(#3128) #3280

[ISSUE #3281]fix fail to delete topic perm list and global white address(#3128) #3280

Uh oh!

Conversation

yuz10 commented Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the purpose of the change

Brief changelog

Verifying this change

Uh oh!

coveralls commented Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zongtanghu commented Aug 23, 2021

Uh oh!

yuz10 commented Aug 23, 2021

Uh oh!

zongtanghu commented Aug 27, 2021

Uh oh!

yuz10 commented Aug 27, 2021

Uh oh!

vongosling commented Aug 27, 2021

Uh oh!

yuz10 commented Aug 27, 2021

Uh oh!

vongosling commented Aug 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yuz10 commented Aug 27, 2021

Uh oh!

zongtanghu Sep 4, 2021

Choose a reason for hiding this comment

Uh oh!

yuz10 Sep 11, 2021

Choose a reason for hiding this comment

Uh oh!

zongtanghu Sep 4, 2021

Choose a reason for hiding this comment

Uh oh!

zongtanghu commented Sep 4, 2021

Uh oh!

Uh oh!

yuz10 commented Aug 20, 2021 •

edited

Loading

coveralls commented Aug 20, 2021 •

edited

Loading

codecov-commenter commented Aug 20, 2021 •

edited

Loading

vongosling commented Aug 27, 2021 •

edited

Loading