[ISSUE #10498] Update ACL documentation to include required 5.x properties#10499
[ISSUE #10498] Update ACL documentation to include required 5.x properties#10499SummCoder wants to merge 2 commits into
Conversation
In RocketMQ 5.x, enabling ACL requires more than just 'aclEnable=true'. The following properties are also required: - authenticationEnabled - authorizationEnabled - migrateAuthFromV1Enabled - authenticationMetadataProvider - authorizationMetadataProvider Without these, ACL either silently allows all traffic or throws 'authenticationMetadataProvider is not configured'.
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Updates ACL documentation to include required 5.x properties (admin, default.group.perm, etc.) that are mandatory in RocketMQ 5.x but were missing from the docs.
LGTM. Important documentation gap filled — users deploying ACL on 5.x need these properties. 👍
Automated review by github-manager-bot
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #10499 +/- ##
=============================================
- Coverage 48.13% 48.10% -0.04%
- Complexity 13355 13376 +21
=============================================
Files 1377 1377
Lines 100707 100753 +46
Branches 13010 13019 +9
=============================================
- Hits 48477 48463 -14
- Misses 46296 46335 +39
- Partials 5934 5955 +21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
SummCoder
changed the title
majialoong
left a comment
majialoong
left a comment
There was a problem hiding this comment.
Thanks for the patch! I left a few comments.
| ## RocketMQ 5.x 需要额外配置以下ACL属性 | ||
| authenticationEnabled=true | ||
| authorizationEnabled=true | ||
| migrateAuthFromV1Enabled=true |
There was a problem hiding this comment.
I think migrateAuthFromV1Enabled is not necessary here, since it is only required when migrating from ACL v1 to ACL v2.
| migrateAuthFromV1Enabled=true | ||
| authenticationMetadataProvider=org.apache.rocketmq.auth.authentication.provider.LocalAuthenticationMetadataProvider | ||
| authorizationMetadataProvider=org.apache.rocketmq.auth.authorization.provider.LocalAuthorizationMetadataProvider | ||
| listenPort=10911 |
There was a problem hiding this comment.
Maybe we should also add the default authentication and authorization providers here for completeness:
authenticationProvider=org.apache.rocketmq.auth.authentication.provider.DefaultAuthenticationProvider
authorizationProvider=org.apache.rocketmq.auth.authorization.provider.DefaultAuthorizationProvider
| ## RocketMQ 5.x requires the following additional ACL properties | ||
| authenticationEnabled=true | ||
| authorizationEnabled=true | ||
| migrateAuthFromV1Enabled=true |
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Updates both Chinese and English ACL documentation to include the six additional properties required for RocketMQ 5.x ACL configuration: authenticationEnabled, authorizationEnabled, migrateAuthFromV1Enabled, authenticationMetadataProvider, authorizationMetadataProvider.
Findings
- [Positive]
docs/cn/acl/user_guide.mdanddocs/en/acl/Operations_ACL.md— Both language versions are updated consistently. The new properties are added in the correct location within the broker configuration example. - [Positive] The prose is updated to reflect that these are 5.x-specific requirements, not just optional additions.
- [Info] The
migrateAuthFromV1Enabled=trueproperty is particularly important for users migrating from 4.x — it enables backward compatibility with the legacy ACL format.
Suggestions
- Consider adding a brief explanation of what each new property does, especially
migrateAuthFromV1Enabled. Users copying the config block without understanding may enable migration mode unnecessarily or forget to enable it when needed.
Verdict
LGTM. Addresses a real documentation gap that causes confusion for 5.x deployers.
Automated review by github-manager-bot
| ## RocketMQ 5.x 需要额外配置以下ACL属性 | ||
| authenticationEnabled=true | ||
| authorizationEnabled=true | ||
| migrateAuthFromV1Enabled=true |
There was a problem hiding this comment.
I do not think migrateAuthFromV1Enabled=true should be documented as part of the baseline 5.x ACL setup. It is specifically for migrating v1 plain_acl.yml data into the new auth model. Please move it into a migration note instead of the default configuration block.
| migrateAuthFromV1Enabled=true | ||
| authenticationMetadataProvider=org.apache.rocketmq.auth.authentication.provider.LocalAuthenticationMetadataProvider | ||
| authorizationMetadataProvider=org.apache.rocketmq.auth.authorization.provider.LocalAuthorizationMetadataProvider | ||
| listenPort=10911 |
There was a problem hiding this comment.
Since the default authentication/authorization providers are part of the effective setup, please document them explicitly or explain that they are defaulted by code. Otherwise users may copy an incomplete config and still be unclear about the provider chain.
| ## RocketMQ 5.x requires the following additional ACL properties | ||
| authenticationEnabled=true | ||
| authorizationEnabled=true | ||
| migrateAuthFromV1Enabled=true |
There was a problem hiding this comment.
Same issue as the Chinese doc: migrateAuthFromV1Enabled=true should not be presented as a general required setting. Please document it only for v1 ACL migration scenarios.
- Replace aclEnable with authenticationEnabled + authorizationEnabled - Add initAuthenticationUser (replaces plain_acl.yml) - Add innerClientAuthenticationCredentials - Remove migrateAuthFromV1Enabled (migration only, not baseline) - Add link to official ACL 2.0 docs Verified on a clean 5.5.0 deployment.
|
Updated to match the official ACL 2.0 configuration:
Verified on a clean 5.5.0 deployment — mqadmin and producer both |
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Updated ACL documentation to align with RocketMQ 5.x ACL 2.0 configuration. This revision removes migrateAuthFromV1Enabled from the baseline config (addressing reviewer feedback) and adds initAuthenticationUser and innerClientAuthenticationCredentials properties.
Findings
- [Positive]
docs/cn/acl/user_guide.mdanddocs/en/acl/Operations_ACL.md— Correctly removedmigrateAuthFromV1Enabledfrom the baseline setup as requested by @RongtongJin and @majialoong. This property is migration-specific and should not appear in the default config block. - [Positive] Added
initAuthenticationUserandinnerClientAuthenticationCredentials— These are practical additions that help users understand the full bootstrap configuration. - [Positive] The note clarifying that
aclEnable=truehas been replaced byauthenticationEnabled/authorizationEnabledin 5.x is helpful for users migrating from 4.x. - [Warning]
docs/cn/acl/user_guide.md:50anddocs/en/acl/Operations_ACL.md:50— @RongtongJin requested on Jun 18 that the default authentication/authorization providers be documented explicitly or explained as code defaults. This appears to remain unaddressed. Consider adding a note like:## If not specified, the following defaults are used: # authenticationProvider=org.apache.rocketmq.auth.authentication.provider.DefaultAuthenticationProvider # authorizationProvider=org.apache.rocketmq.auth.authorization.provider.DefaultAuthorizationProvider - [Info] The example credentials
{"username":"rocketmq","password":"12345678"}and{"accessKey":"rocketmq","secretKey":"12345678"}use a weak password. Consider adding a warning comment like# WARNING: Change this in productionto prevent users from copying insecure defaults.
Suggestions
- Address the pending reviewer feedback about default providers to unblock merge.
- Add a production-readiness warning next to the example credentials.
Verdict
Good progress addressing reviewer feedback. Two items remain before this is ready to merge: the default provider documentation and a credential security note.
Automated review by github-manager-bot
7 participants
Which Issue(s) This PR Fixes
Fixes #10498
Brief Description
The current ACL documentation (both English and Chinese) only lists
aclEnable=trueas the required broker property. In RocketMQ 5.x,this is insufficient — the new authentication/authorization framework
introduced in 5.x requires five additional properties to function.
Code evidence for each property:
Without these, users experience either:
How Did You Test This Change?
mvn -Prelease-all -DskipTests clean install -Uinvalid credentials correctly rejected with "User not found"
tools.ymlcredentialsmatching the ACL account
failure mode (silent bypass or configuration error)