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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
