RockteMQ-AI commented on code in PR #10957:
URL: https://github.com/apache/rocketmq/pull/10957#discussion_r3801608240
##########
namesrv/src/main/java/org/apache/rocketmq/namesrv/NamesrvStartup.java:
##########
@@ -190,7 +195,11 @@ public static ControllerManager
createAndStartControllerManager() throws Excepti
public static ControllerManager createControllerManager() throws Exception
{
NettyServerConfig controllerNettyServerConfig = (NettyServerConfig)
nettyServerConfig.clone();
- ControllerManager controllerManager = new
ControllerManager(controllerConfig, controllerNettyServerConfig,
nettyClientConfig);
+ controllerAuthConfig.setConfigName("controller-" +
controllerConfig.getControllerDLegerSelfId());
Review Comment:
**[Warning]** Potential `NullPointerException`: `controllerAuthConfig` is
initialized to `null` and only set when
`namesrvConfig.isEnableControllerInNamesrv()` is true (line ~112). However,
`createControllerManager()` dereferences it unconditionally at line 198
(`controllerAuthConfig.setConfigName(...)`).
If `createControllerManager()` is called when the embedded controller is not
enabled (or if the config parsing path is skipped), this will NPE.
Suggestion: Add a null check, or ensure `controllerAuthConfig` is always
initialized when `createControllerManager()` is reachable.
##########
controller/src/main/java/org/apache/rocketmq/controller/ControllerManager.java:
##########
@@ -73,16 +83,26 @@ public class ControllerManager {
private BlockingQueue<Runnable> controllerRequestThreadPoolQueue;
private final NotifyService notifyService;
private ControllerMetricsManager controllerMetricsManager;
+ private AuthenticationMetadataManager authenticationMetadataManager;
Review Comment:
**[Info]** The `authenticationMetadataManager` and
`authorizationMetadataManager` fields are initialized in `initializeMetadata()`
which is called from `start()`. If any code path calls methods on these
managers before `start()`, it will NPE.
Consider initializing them in the constructor or adding null guards. This
follows the same pattern as the Broker, so it may be acceptable, but worth
noting for robustness.
##########
auth/src/main/java/org/apache/rocketmq/auth/authorization/builder/DefaultAuthorizationContextBuilder.java:
##########
@@ -565,6 +568,8 @@ staticTopic, requireResource(mappingDetail.getTopic(),
"mapping topic"))) {
Resource.of(ResourceType.GROUP, null,
ResourcePattern.ANY), Action.LIST, sourceIp));
break;
case RequestCode.SET_COMMITLOG_READ_MODE:
+ case RequestCode.UPDATE_CONTROLLER_CONFIG:
Review Comment:
**[Info]** `CLEAN_BROKER_DATA` is mapped to cluster UPDATE action alongside
`SET_COMMITLOG_READ_MODE`. This is a destructive operation (data deletion).
Please confirm this mapping is intentional and that only admin-level users
should have UPDATE permission on the cluster resource for this action.
Also, consider whether `CLEAN_BROKER_DATA` semantically belongs with the
other clean/delete operations in the DELETE action group rather than UPDATE.
--
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]
