[GitHub] [rocketmq] caigy commented on a diff in pull request #4152: [#4151]Add option p to the updateGlobalWhiteAddr command.

[GitBox <gi...@apache.org>]

caigy commented on code in PR #4152:
URL: https://github.com/apache/rocketmq/pull/4152#discussion_r877665039


##########
acl/src/main/java/org/apache/rocketmq/acl/plain/PlainPermissionManager.java:
##########
@@ -466,11 +466,11 @@ public boolean deleteAccessConfig(String accesskey) {
         return false;
     }
 
-    public boolean updateGlobalWhiteAddrsConfig(List<String> globalWhiteAddrsList) {
-        return this.updateGlobalWhiteAddrsConfig(globalWhiteAddrsList, this.defaultAclFile);
-    }
-

Review Comment:
   It may be better to keep original implementation for compatibility.



##########
tools/src/main/java/org/apache/rocketmq/tools/admin/MQAdminExt.java:
##########
@@ -77,7 +77,7 @@ void createAndUpdatePlainAccessConfig(final String addr, final PlainAccessConfig
     void deletePlainAccessConfig(final String addr, final String accessKey) throws RemotingException, MQBrokerException,
         InterruptedException, MQClientException;
 
-    void updateGlobalWhiteAddrConfig(final String addr, final String globalWhiteAddrs)throws RemotingException, MQBrokerException,
+    void updateGlobalWhiteAddrConfig(final String addr, final String globalWhiteAddrs, String aclFileFullPath)throws RemotingException, MQBrokerException,

Review Comment:
   It may be more appropriate to add a new method instead of changing its signature in an interface.



##########
acl/src/main/java/org/apache/rocketmq/acl/plain/PlainPermissionManager.java:
##########
@@ -466,11 +466,11 @@ public boolean deleteAccessConfig(String accesskey) {
         return false;
     }
 
-    public boolean updateGlobalWhiteAddrsConfig(List<String> globalWhiteAddrsList) {
-        return this.updateGlobalWhiteAddrsConfig(globalWhiteAddrsList, this.defaultAclFile);
-    }
-
     public boolean updateGlobalWhiteAddrsConfig(List<String> globalWhiteAddrsList, String fileName) {
+        if (fileName == null) {

Review Comment:
   May `fileName` be empty string?



##########
acl/src/main/java/org/apache/rocketmq/acl/plain/PlainPermissionManager.java:
##########
@@ -482,6 +482,16 @@ public boolean updateGlobalWhiteAddrsConfig(List<String> globalWhiteAddrsList, S
             return false;
         }
 
+        if (!fileName.startsWith(fileHome)) {
+            log.error("Parameter value " + fileName + " is not in the directory rocketmq.home.dir");

Review Comment:
   Logging with the value of `rocketmq.home.dir` will be more clear.



##########
acl/src/main/java/org/apache/rocketmq/acl/AccessValidator.java:
##########
@@ -71,7 +71,7 @@
      *
      * @return
      */
-    boolean updateGlobalWhiteAddrsConfig(List<String> globalWhiteAddrsList);
+    boolean updateGlobalWhiteAddrsConfig(List<String> globalWhiteAddrsList, String aclFileFullPath);
 

Review Comment:
   +1. Compatibility should be kept.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org