feat: implement UpdateAclSubCommand (#5665)#5694
Conversation
|
🔊@magogosora 🚀Thanks for your contribution🎉！ 💡CodeRabbit(AI) will review your code first🔥！ Note 🚨The code review suggestions from CodeRabbit are to be used as a reference only, and the PR submitter can decide whether to make changes based on their own judgment. Ultimately, the project management personnel will conduct the final code review💥. |
rocketmq-rust-bot
added
AI review first
rocketmq-rust-robot
added
Difficulty level/Moderate
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds an Auth subcommand group with a new UpdateAcl subcommand (CLI parsing, execution, and tests), wires it into the top-level Commands enum and classification table, updates CHANGELOG, and changes DefaultMQAdminExt constructors to accept an Arc and forward update_acl calls. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant UpdateAclCmd as "UpdateAclCmd (rust)"
participant MQAdminExt as "DefaultMQAdminExt"
participant Broker as "Broker(s)"
User->>CLI: invoke updateAcl [args]
CLI->>UpdateAclCmd: execute(rpc_hook)
alt brokerAddr provided
UpdateAclCmd->>MQAdminExt: create & start
MQAdminExt->>Broker: connect to brokerAddr
UpdateAclCmd->>Broker: updateAcl(subject, resources, actions, decision, sourceIps)
Broker-->>UpdateAclCmd: success/error
UpdateAclCmd->>MQAdminExt: shutdown
else clusterName provided
UpdateAclCmd->>MQAdminExt: create & start
MQAdminExt->>Broker: fetch cluster master addresses
Broker-->>MQAdminExt: master addresses
loop per master address
UpdateAclCmd->>Broker: updateAcl(address, ... )
Broker-->>UpdateAclCmd: success/error
end
UpdateAclCmd->>MQAdminExt: shutdown
end
UpdateAclCmd-->>CLI: RocketMQResult
CLI-->>User: print outcome
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@rocketmq-tools/src/commands/auth_commands/update_acl_sub_command.rs`:
- Around line 161-166: The error branch in UpdateAclSubCommand duplicates the
prefix ("UpdateAclSubCommand: UpdateAclSubCommand:"); update the Err(...)
construction inside the update handler to use a single, clear prefix (e.g.,
"UpdateAclSubCommand: Failed to update access control list (ACL): {}") so the
RocketMQError::Internal message is not duplicated; locate the Err(e) arm in the
update_acl_sub_command.rs handling code for UpdateAclSubCommand and replace the
duplicated prefix with the single desired prefix.
- Line 140: The branch is using self.subject.clone() instead of the trimmed
local variable subject defined earlier; replace the use of self.subject.clone()
with the trimmed subject (e.g., subject.to_string() or subject.into()) so the
code uses the trimmed value; locate the occurrence in update_acl_sub_command.rs
(around the branch where subject is passed) and swap the clone for the trimmed
subject variable to ensure whitespace is removed.
- Around line 154-157: The println message in update_acl_sub_command.rs contains
an extra space between "ACL)" and "at"; locate the println call that prints
"Updated access control list (ACL) at broker with address {} successfully." and
remove the duplicate space so it reads "Updated access control list (ACL) at
broker with address {} successfully."; verify in the function that performs the
ACL update (the println inside the update ACL subcommand handler) that only a
single space remains.
- Line 59: The execute function currently ignores the rpc_hook parameter; update
execute(&self, _rpc_hook: Option<Arc<dyn RPCHook>>) to use the provided RPCHook
when constructing the admin client by removing the underscore from the parameter
name and creating DefaultMQAdminExt with
DefaultMQAdminExt::with_rpc_hook(hook.clone()) when rpc_hook is Some(hook),
otherwise fall back to DefaultMQAdminExt::new(); ensure subsequent code uses
that admin instance and that types/cloning match the Arc<dyn RPCHook> signature.
🧹 Nitpick comments (2)
rocketmq-tools/src/commands/auth_commands/update_acl_sub_command.rs (2)
60-66: Redundant validation – clap'sArgGroupalready enforces this.The
ArgGroupon lines 32-34 withrequired(true)already ensures exactly one ofcluster_nameorbroker_addris provided. This manual check is unreachable and can be removed.♻️ Suggested fix
impl CommandExecute for UpdateAclSubCommand { async fn execute(&self, _rpc_hook: Option<Arc<dyn RPCHook>>) -> RocketMQResult<()> { - if (self.cluster_name.is_none() && self.broker_addr.is_none()) - || (self.cluster_name.is_some() && self.broker_addr.is_some()) - { - return Err(RocketMQError::IllegalArgument( - "UpdateAclSubCommand: Specify exactly one of --brokerAddr (-b) or --clusterName (-c)".into(), - )); - } - let mut default_mqadmin_ext = DefaultMQAdminExt::new();
102-131: Consolidate duplicatedMQAdminExt::startcalls.
MQAdminExt::startis called identically in both branches. Move it before the conditional to reduce duplication.♻️ Suggested refactor
let decision: CheetahString = self.decision.trim().into(); + MQAdminExt::start(&mut default_mqadmin_ext).await.map_err(|e| { + RocketMQError::Internal(format!("UpdateAclSubCommand: Failed to start MQAdminExt: {}", e)) + })?; + if let Some(ref broker_addr) = self.broker_addr { - MQAdminExt::start(&mut default_mqadmin_ext).await.map_err(|e| { - RocketMQError::Internal(format!("UpdateAclSubCommand: Failed to start MQAdminExt: {}", e)) - })?; - match default_mqadmin_ext .update_acl( // ... } else if let Some(ref cluster_name) = self.cluster_name { - MQAdminExt::start(&mut default_mqadmin_ext).await.map_err(|e| { - RocketMQError::Internal(format!("UpdateAclSubCommand: Failed to start MQAdminExt: {}", e)) - })?; - let cluster_info = default_mqadmin_ext.examine_broker_cluster_info().await?;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.mdrocketmq-tools/src/commands.rsrocketmq-tools/src/commands/auth_commands.rsrocketmq-tools/src/commands/auth_commands/update_acl_sub_command.rs
🧰 Additional context used
🧬 Code graph analysis (1)
rocketmq-tools/src/commands/auth_commands/update_acl_sub_command.rs (3)
rocketmq-common/src/utils/time_utils.rs (1)
get_current_millis(16-21)rocketmq-tools/src/commands.rs (3)
execute(45-45)execute(95-103)execute(123-225)rocketmq-tools/src/commands/auth_commands.rs (1)
execute(37-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Check (fmt + clippy)
- GitHub Check: auto-approve
🔇 Additional comments (5)
rocketmq-tools/src/commands/auth_commands/update_acl_sub_command.rs (1)
178-248: Good test coverage for CLI parsing.Tests appropriately verify argument parsing for both targeting modes and the mutual exclusivity constraint.
rocketmq-tools/src/commands/auth_commands.rs (1)
26-42: LGTM – Clean delegation pattern.The enum structure and delegation to the inner command's
executemethod follows the established pattern in other command modules.rocketmq-tools/src/commands.rs (2)
71-74: LGTM – Consistent integration with existing command structure.The new
Authvariant follows the same pattern asNameServer,Topic, andControllercommand groups.Also applies to: 97-97
125-129: LGTM – Help table updated correctly.The new Auth command entry is properly documented in the classification table.
CHANGELOG.md (1)
12-12: LGTM – Changelog entry properly documents the new feature.The entry correctly references the linked issue
#5665.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rocketmq-tools/src/admin/default_mq_admin_ext.rs (1)
106-121: API signature inconsistency: Alignwith_rpc_hook_and_timeoutto matchwith_rpc_hook.The change to accept
Arc<dyn RPCHook>inwith_rpc_hookis sound, but it creates an API inconsistency withwith_rpc_hook_and_timeout(line 123), which still acceptsimpl RPCHookand wraps internally. This forces callers to use different patterns depending on whether they need a custom timeout.Since
with_rpc_hook_and_timeouthas no callers in the codebase, update it to also acceptArc<dyn RPCHook>directly:
- Remove the
Arc::new(rpc_hook)wrapping on line 125- Change the parameter to
rpc_hook: Arc<dyn RPCHook>- Pass
rpc_hookdirectly toDefaultMQAdminExtImpl::new
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rocketmq-tools/src/admin/default_mq_admin_ext.rsrocketmq-tools/src/commands/auth_commands/update_acl_sub_command.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rocketmq-tools/src/commands/auth_commands/update_acl_sub_command.rs
🧰 Additional context used
🧬 Code graph analysis (1)
rocketmq-tools/src/admin/default_mq_admin_ext.rs (4)
rocketmq-client/src/producer/default_mq_producer.rs (4)
rpc_hook(174-176)rpc_hook(339-341)client_config(254-256)new(249-251)rocketmq-client/src/producer/default_mq_produce_builder.rs (2)
rpc_hook(211-214)client_config(86-89)rocketmq-client/src/admin/default_mq_admin_ext_impl.rs (1)
new(95-111)rocketmq-client/src/implementation/mq_client_api_impl.rs (1)
new(329-353)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Check (fmt + clippy)
- GitHub Check: auto-approve
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5694 +/- ##
==========================================
- Coverage 39.86% 39.83% -0.03%
==========================================
Files 831 833 +2
Lines 115468 115682 +214
==========================================
+ Hits 46033 46086 +53
- Misses 69435 69596 +161 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
magogosora
force-pushed
the
feature/issue-5665-implement-update-acl-command
branch
2 times, most recently
from
d469192 to
aad3ff6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@rocketmq-tools/src/commands/auth_commands.rs`:
- Around line 27-33: The CLI subcommand name for the
AuthCommands::UpdateAclSubCommand variant is incorrectly declared as
"updateAclSubCommand"; change the #[command(name = "...")] attribute on the
UpdateAclSubCommand(UpdateAclSubCommand) enum variant to "updateAcl" so it
matches the documented interface and the naming pattern of other subcommands
(e.g., updateControllerConfig, deleteSubGroup); update only the name attribute
on the enum variant to "updateAcl".
In `@rocketmq-tools/src/commands/auth_commands/update_acl_sub_command.rs`:
- Around line 137-170: The cluster update branch prints per-broker failures but
still returns Ok(()), hiding partial failures; change the logic in
UpdateAclSubCommand after CommandUtil::fetch_master_addr_by_cluster_name to
collect failures when calling default_mqadmin_ext.update_acl for each addr
(e.g., maintain a Vec<String> or a boolean flag), continue attempting all
addresses, and after the loop return Err(RocketMQError::Internal(...)) if any
update failed (include aggregated error info or failed addresses), otherwise
return Ok(()). Ensure you update the control flow where addresses are iterated
to use the aggregator and adjust the final return accordingly.
🧹 Nitpick comments (1)
rocketmq-tools/src/commands/auth_commands/update_acl_sub_command.rs (1)
51-55: Constraindecisionto valid values at parse-time.
decisionis free-form, which defers mistakes to runtime. Consider avalue_parserorValueEnumto restrict to allowed values (e.g.,ALLOW,DENY) and produce immediate CLI feedback.✅ Example (minimal validation)
- #[arg(short = 'd', long = "decision", required = true)] + #[arg(short = 'd', long = "decision", value_parser = ["ALLOW", "DENY"], required = true)] decision: String,
magogosora
force-pushed
the
feature/issue-5665-implement-update-acl-command
branch
from
aad3ff6 to
703930d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@rocketmq-tools/src/commands/auth_commands/update_acl_sub_command.rs`:
- Around line 111-119: The call sites use DefaultMQAdminExt::update_acl and
::examine_broker_cluster_info but those methods are still todo!() and will panic
at runtime; implement them in default_mq_admin_ext.rs by delegating to the
underlying MQ admin/client implementation the same way other methods in
DefaultMQAdminExt do (follow the pattern used by methods like
create_topic/update_permissions/etc.), accept the same parameters (broker_addr,
subject, resources, actions, source_ips, decision for update_acl; cluster
identifier or options for examine_broker_cluster_info), perform any necessary
argument transformation, call the inner client's corresponding RPC/HTTP methods,
and surface errors instead of panicking so update_acl and
examine_broker_cluster_info are production-safe.
- Around line 80-103: The split-and-map pipelines for resources, actions, and
source_ip in update_acl_sub_command.rs currently can produce empty CheetahString
entries (e.g., from "Topic:a,," or " PUB, "); update the parsing of
self.resources, self.actions, and self.source_ip to trim each item, filter out
empty strings before converting to CheetahString, and if the resulting
Vec<CheetahString> for resources or actions is empty return an error (or bail)
explaining the input was invalid; reference the variables/resources created in
this block (resources, actions, source_ips) and make the same empty-filtering
logic consistent across all three parsers.
♻️ Duplicate comments (1)
rocketmq-tools/src/commands/auth_commands/update_acl_sub_command.rs (1)
140-163: Surface partial failures when updating a cluster.Right now the command prints per‑broker failures but still returns
Ok(()), which hides errors from automation.🛠️ Aggregate errors and return non‑zero on failures
match CommandUtil::fetch_master_addr_by_cluster_name(&cluster_info, cluster_name.as_str()) { Ok(addresses) => { + let mut update_errors: Vec<String> = Vec::new(); for addr in addresses { if let Err(e) = default_mqadmin_ext .update_acl( addr.as_str().into(), subject.into(), resources.clone(), actions.clone(), source_ips.clone(), decision.clone(), ) .await { eprintln!( "UpdateAclSubCommand: Failed to update access control list (ACL) for broker with \ address {}: {}", addr, e ); + update_errors.push(format!("{}: {}", addr, e)); } else { println!( "Updated access control list (ACL) at broker with address {} successfully.", addr ); } } + if !update_errors.is_empty() { + return Err(RocketMQError::Internal(format!( + "UpdateAclSubCommand: Failed to update ACL on {} broker(s): {}", + update_errors.len(), + update_errors.join("; ") + ))); + } }
magogosora
force-pushed
the
feature/issue-5665-implement-update-acl-command
branch
from
703930d to
de5a0f8
Compare
rocketmq-rust-bot
left a comment
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
magogosora
force-pushed
the
feature/issue-5665-implement-update-acl-command
branch
from
de5a0f8 to
22f7779
Compare
rocketmq-rust-bot
left a comment
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
magogosora
force-pushed
the
feature/issue-5665-implement-update-acl-command
branch
from
22f7779 to
6f4da2b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@rocketmq-tools/src/commands/auth_commands/update_acl_sub_command.rs`:
- Around line 118-122: The validation currently checks resources.is_empty() when
it should validate the actions list; in the function/method handling the
UpdateAclSubCommand (the block that returns RocketMQError::IllegalArgument with
message "UpdateAclSubCommand: actions cannot be empty"), change the condition to
check actions.is_empty() instead of resources.is_empty() so empty actions are
rejected (and keep or adjust the error message accordingly).
- Around line 189-217: The update_broker function currently unwraps
parsed_command.broker_addr and will panic when broker_addr is None (called from
update_cluster); change the signature of update_broker to accept a broker_addr
parameter (e.g., broker_addr: String or &str) and stop reading broker_addr from
ParsedUpdateAclSubCommand inside the function, then use that passed-in
broker_addr in the call to default_mqadmin_ext.update_acl and in the
success/error messages; update the caller(s) (notably update_cluster) to pass
the appropriate broker address when invoking update_broker and remove the unwrap
usage on parsed_command.broker_addr.
- Around line 229-240: The loop over addresses never passes the broker address
into update_broker, so change update_broker's signature to accept the per-broker
address (e.g., update_broker(parsed_command: &ParsedCommand,
default_mqadmin_ext: &MqAdminExt, addr: CheetahString) -> ...), then update the
call site inside execute where addresses is iterated (the async map that builds
error_addr) to call update_broker(parsed_command, default_mqadmin_ext, addr).
Also update any other call sites of update_broker accordingly so the actual
broker address from addresses is used for the ACL update rather than only for
error collection.
🧹 Nitpick comments (1)
rocketmq-tools/src/commands/auth_commands/update_acl_sub_command.rs (1)
258-328: Consider adding validation tests forParsedUpdateAclSubCommand::new.The current tests cover CLI argument parsing but don't test the validation logic in
ParsedUpdateAclSubCommand::new(). Consider adding tests for edge cases like empty subject, empty resources, empty actions, and whitespace-only inputs to ensure validation errors are properly returned.
magogosora
force-pushed
the
feature/issue-5665-implement-update-acl-command
branch
from
6f4da2b to
c9772d3
Compare
rocketmq-rust-bot
left a comment
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
magogosora
force-pushed
the
feature/issue-5665-implement-update-acl-command
branch
from
c9772d3 to
9ae40b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@rocketmq-tools/src/commands/auth_commands/update_acl_sub_command.rs`:
- Around line 69-150: ParsedUpdateAclSubCommand::new trims cluster_name and
broker_addr for validation but then stores the original
command.cluster_name/command.broker_addr (which may contain surrounding
whitespace), causing later lookups/parsing to fail; change the struct population
to store the trimmed values instead of cloning the raw fields. In the Ok(Self {
... }) return, replace cluster_name: command.cluster_name.clone() and
broker_addr: command.broker_addr.clone() with the trimmed Option values computed
earlier (e.g., use the local cluster_name and broker_addr variables converted to
owned Strings via map(|s| s.to_string()) or similar) so the stored fields
contain trimmed text.
rocketmq-rust-bot
left a comment
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
magogosora
force-pushed
the
feature/issue-5665-implement-update-acl-command
branch
from
9ae40b1 to
26a9719
Compare
rocketmq-rust-bot
left a comment
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
rocketmq-rust-bot
added
approved
4 participants
Which Issue(s) This PR Fixes(Closes)
Resolves #5665
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.