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
[ISSUE #2986] Support for multiple ACL files in a fixed directory #3761
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3761 +/- ##
=============================================
+ Coverage 46.85% 47.37% +0.52%
- Complexity 4835 4930 +95
=============================================
Files 636 636
Lines 42251 42650 +399
Branches 5523 5593 +70
=============================================
+ Hits 19795 20204 +409
+ Misses 19973 19926 -47
- Partials 2483 2520 +37
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this PR, there is a very important assumption: The ACL directory MUST contain and ONLY contain ACL config files without subdirectories, but I couldn't find any strong statements and checking about it.
It requires more considerations on compatibility to the original one ACL config file and scalability of supporting multiple config files in different directories.
At present, there are some incompatible modifications:
- Return type of
AccessValidator.getAclConfigVersion()
changed fromString
toMap<String, DataVersion>
, users who have implemented their own implementation would meet NoSuchMethod error. PlainPermissionManager.getAclConfigDataVersion()
is deleted.- Some modification breaks protocol, such as changing the type of
version
inGetBrokerAclConfigResponseHeader
, removingaclConfigDataVersion
inClusterAclVersionInfo
. - Users using
DEFAULT_PLAIN_ACL_FILE
(/conf/plain_acl.yml) can't load ACL config file after this change, because it will be loaded from /conf/acl directory. If they change the acl directory to /conf/, not only acl config files, but also other config files like broker config will be loaded, and the loading process will encounter errors.
@@ -60,16 +63,18 @@ | |||
* | |||
* @return | |||
*/ | |||
String getAclConfigVersion(); | |||
Map<String, DataVersion> getAclConfigVersion(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes the interface incompatible to older versions, you'd better add a new method and deprecate the original ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
private static final String DEFAULT_PLAIN_ACL_FILE = "/conf/plain_acl.yml"; | ||
|
||
private String fileHome = System.getProperty(MixAll.ROCKETMQ_HOME_PROPERTY, | ||
System.getenv(MixAll.ROCKETMQ_HOME_ENV)); | ||
|
||
private String fileName = System.getProperty("rocketmq.acl.plain.file", DEFAULT_PLAIN_ACL_FILE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users using DEFAULT_PLAIN_ACL_FILE
can't load ACL config file after this change, it requires more compatible considerations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1.
This pr need to supply a detailed design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems users can't specify config file by property rocketmq.acl.plain.file
?
if (fileHome == null || fileHome.isEmpty()) { | ||
throw new AclException(String.format("%s file is empty", fileHome)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it more appropriate to just returning than throwing exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
File aclDir = new File(defaultAclDir); | ||
File[] aclFiles = aclDir.listFiles(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Array of File
objects is created here but only path string is used(org.apache.rocketmq.acl.common.AclUtils#getYamlDataObject
uses file name to read this file again), it will use less resource if using java.nio.file.Files#list
, or add overloaded method for org.apache.rocketmq.acl.common.AclUtils#getYamlDataObject
accepting file object.
List<String> fileList = new ArrayList<>(); | ||
for (File aclFile : aclFiles) { | ||
String aclFileAbsolutePath = aclFile.getAbsolutePath(); | ||
load(aclFileAbsolutePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we surround load
by try-catch?
public class GetBrokerAclConfigResponseHeader implements CommandCustomHeader { | ||
|
||
@CFNotNull | ||
private String version; | ||
private Map<String, DataVersion> version; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It changes the protocol, some users may already use it as String.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
public class ClusterAclVersionInfo extends RemotingSerializable { | ||
|
||
private String brokerName; | ||
|
||
private String brokerAddr; | ||
|
||
private DataVersion aclConfigDataVersion; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a new field other than changing it for compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
} else { | ||
for (int i = 0; i < aclFilesNum; i++) { | ||
String fileName = aclFiles[i].getAbsolutePath(); | ||
String newHash = hash(fileName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calculating hash of a file consumes a lot of system resources, can file modified time and file size be used instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
for (int i = 0; i < aclFilesNum; i++) { | ||
String fileName = aclFiles[i].getAbsolutePath(); | ||
String newHash = hash(fileName); | ||
if (!newHash.equals(fileCurrentHash.get(i))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be fileCurrentHash.get(fileName)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
private int aclFilesNum; | ||
private final Map<String, String> fileCurrentHash; | ||
private final AclFileWatchService.Listener listener; | ||
private static final int WATCH_INTERVAL = 500; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it too often?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
The multi ACL files are not compatible with the default ACL file naturally. |
IMO. |
…le and scalability of supporting multiple config files in different directories.
@sunxi92 Could you post your detailed design so that more people can join this pr? BTW, it should be well documented about how to use this new feature and how it effects the old implementation, not only in comments but also in user guide. |
versionNum, | ||
timeStampStr | ||
); | ||
for (Map.Entry<String, DataVersion> entry : aclDataVersion.entrySet()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would AccessValidator.getAllAclConfigVersion()
return an empty set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
@Deprecated | ||
String getAclConfigVersion(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pls add more description about:
- the reason to deprecate
String getAclConfigVersion()
and replace it withMap<String, DataVersion> getAllAclConfigVersion()
- What is the object or scope that version describe? In previous implementation, version is used to mark the whole config. In this multi-filed feature, each config file has its own version. And if ACL config are stored in DB, each record may have its own version. We should define version precisely.
- In
Map<String, DataVersion> getAllAclConfigVersion()
, what is the key of Map? Users may curious of it. If this is the full path of config files, it brings implementation to interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
private static final String DEFAULT_PLAIN_ACL_FILE = "/conf/plain_acl.yml"; | ||
|
||
private String fileHome = System.getProperty(MixAll.ROCKETMQ_HOME_PROPERTY, | ||
System.getenv(MixAll.ROCKETMQ_HOME_ENV)); | ||
|
||
private String fileName = System.getProperty("rocketmq.acl.plain.file", DEFAULT_PLAIN_ACL_FILE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems users can't specify config file by property rocketmq.acl.plain.file
?
|
||
private Map<String/** AccessKey **/, PlainAccessResource> plainAccessResourceMap = new HashMap<>(); | ||
private Map<String/** aclFileName **/, Map<String/** AccessKey **/, PlainAccessResource>> aclPlainAccessResourceMap = new HashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is 'file full path' more precise than 'aclFileName'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
File f = new File(fileName); | ||
if (fileName.endsWith(".yml")) { | ||
fileList.add(fileName); | ||
} else if (!f.isFile()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it should be file.isDirectory()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
this.globalWhiteRemoteAddressStrategy.remove(remoteAddressStrategyList.get(i)); | ||
} | ||
this.globalWhiteRemoteAddressStrategyMap.remove(aclFilePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing items in globalWhiteRemoteAddressStrategy
may cause ACL check failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
if (dataVersionMap.size() == 1) { | ||
for (Map.Entry<String, DataVersion> entry : dataVersionMap.entrySet()) { | ||
dataVersion.assignNewOne(entry.getValue()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would dataVersion be empty or updated if there are multiple ACL config files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
if (aclPlainAccessResourceMap.size() != 0 && accessKeyTable.size() != 0) { | ||
aclPlainAccessResourceMap.clear(); | ||
accessKeyTable.clear(); | ||
} | ||
if (globalWhiteRemoteAddressStrategy.size() != 0) { | ||
globalWhiteRemoteAddressStrategy.clear(); | ||
} | ||
if (globalWhiteRemoteAddressStrategyMap.size() != 0) { | ||
globalWhiteRemoteAddressStrategyMap.clear(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clearing globalWhiteRemoteAddressStrategy or globalWhiteRemoteAddressStrategyMap may cause ACL check failures before load() finished.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
globalWhiteRemoteAddrList.clear(); | ||
if (globalWhiteAddrsList != null) { | ||
globalWhiteRemoteAddrList.addAll(globalWhiteAddrsList); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Combination of clear()
and addAll()
is not atomic, which may cause ACL failure in this process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
private String version; | ||
|
||
private Map<String, DataVersion> versions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version
and versions
are too similar and confusing, maybe we should name them more clearly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
2.add a detailed design document
|
||
public class PlainPermissionManager { | ||
|
||
private static final InternalLogger log = InternalLoggerFactory.getLogger(LoggerName.COMMON_LOGGER_NAME); | ||
|
||
private static final String DEFAULT_PLAIN_ACL_FILE = "/conf/plain_acl.yml"; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove old acl file directory incompatible with old version. Can old acl file and new acl dir coexist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default acl file is added.
public List<String> getAllAclFiles(String path) { | ||
List<String> allAclFileFullPath = new ArrayList<>(); | ||
File file = new File(path); | ||
File[] files = file.listFiles(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, check file is directory will be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
acl/src/main/java/org/apache/rocketmq/acl/plain/PlainPermissionManager.java
Show resolved
Hide resolved
|
||
public AclFileWatchService(String path, final AclFileWatchService.Listener listener) throws Exception { | ||
this.aclPath = path; | ||
this.defaultAclFile = path.substring(0, path.length() - 4) + System.getProperty("rocketmq.acl.plain.file", "conf/plain_acl.yml"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just get parent directory instead of string truncation, or make path
as a List
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
2.Add the logic to check if path is a directory in the method of getAllAclFiles(String path)
…th) method in PlainPermissionManager.java and AclFileWatchService.java
…lConfigVersion command
…AclConfigVersion command 2.Improve the logic of updateAccessConfig method
@@ -387,6 +388,12 @@ public ClusterAclVersionInfo getBrokerClusterAclInfo(final String addr, | |||
clusterAclVersionInfo.setBrokerName(responseHeader.getBrokerName()); | |||
clusterAclVersionInfo.setBrokerAddr(responseHeader.getBrokerAddr()); | |||
clusterAclVersionInfo.setAclConfigDataVersion(DataVersion.fromJson(responseHeader.getVersion(), DataVersion.class)); | |||
HashMap<String, Object> dataVersionMap = JSON.parseObject(responseHeader.getAllAclFileVersion(), HashMap.class); | |||
Map<String, DataVersion> allAclConfigDataVersion = new HashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, specify the concrete type for HashMap for java 1.6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
…ry (apache#3761) * acl temp * acl * fix test case * fix code style issues * add considerations on compatibility to the original one ACL config file and scalability of supporting multiple config files in different directories. * fix test case testWatch * 1.fix some issues 2.add a detailed design document * Add warn log when the accesskey is repeated in multiple ACL files. * 1.Change the folder of acl configuration to conf/acl 2.Add the logic to check if path is a directory in the method of getAllAclFiles(String path) * Add a parameter in AclFileWatchService constructor. * Add logic to determine if path exists in the getAllAclFiles(String path) method in PlainPermissionManager.java and AclFileWatchService.java * Fix the serialization problem of allAclFileVersion field in clusterAclConfigVersion command * 1.Fix the serialization problem of allAclFileVersion field in clusterAclConfigVersion command 2.Improve the logic of updateAccessConfig method
…ry (apache#3761) * acl temp * acl * fix test case * fix code style issues * add considerations on compatibility to the original one ACL config file and scalability of supporting multiple config files in different directories. * fix test case testWatch * 1.fix some issues 2.add a detailed design document * Add warn log when the accesskey is repeated in multiple ACL files. * 1.Change the folder of acl configuration to conf/acl 2.Add the logic to check if path is a directory in the method of getAllAclFiles(String path) * Add a parameter in AclFileWatchService constructor. * Add logic to determine if path exists in the getAllAclFiles(String path) method in PlainPermissionManager.java and AclFileWatchService.java * Fix the serialization problem of allAclFileVersion field in clusterAclConfigVersion command * 1.Fix the serialization problem of allAclFileVersion field in clusterAclConfigVersion command 2.Improve the logic of updateAccessConfig method
Make sure set the target branch to
develop
What is the purpose of the change
Support for multiple ACL files in a fixed directory
#2986
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
.[ISSUE #123] Fix UnknownException when host config not exist
. Each commit in the pull request should have a meaningful subject line and body.mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle
to make sure basic checks pass. Runmvn clean install -DskipITs
to make sure unit-test pass. Runmvn clean test-compile failsafe:integration-test
to make sure integration-test pass.