[ISSUE #9966] Propagate policy type for ACL deletion

[skt-shinyruo]

Summary

• add an optional policyType parameter to the ACL delete path in mqadmin, admin, client, and remoting request header
• preserve existing behavior when policyType is not provided, while allowing mqadmin deleteAcl to target PolicyType=Default
• add focused regression coverage in client, broker, and auth tests for deleting ACLs with explicit policy types

Root Cause

DeleteAclRequestHeader supports policyType, but the delete path from mqadmin to broker did not populate it. When the field is absent, broker-side delete handling falls back to PolicyType.CUSTOM, so deleting a DEFAULT policy entry cannot target the intended ACL rule.

Test Plan

• mvn -o -pl tools -am -DskipTests compile
• mvn -o -pl client -am -DfailIfNoTests=false -Dtest=MQClientAPIImplTest#testDeleteAcl+testDeleteAclWithPolicyType test
• mvn -o -pl broker -am -DfailIfNoTests=false -Dtest=AdminBrokerProcessorTest#testDeleteAcl+testDeleteAclWithPolicyType test
• mvn -o -pl auth -am -DfailIfNoTests=false -Dtest=AuthorizationMetadataManagerTest#deleteAcl test

Closes #9966

[codecov-commenter]

Codecov Report

❌ Patch coverage is 11.53846% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.98%. Comparing base (2a9560c) to head (c854fce).
⚠️ Report is 5 commits behind head on develop.

Files with missing lines	Patch %	Lines
...cketmq/tools/command/auth/DeleteAclSubCommand.java	0.00%	14 Missing ⚠️
...moting/protocol/header/DeleteAclRequestHeader.java	0.00%	4 Missing ⚠️
...he/rocketmq/tools/admin/DefaultMQAdminExtImpl.java	0.00%	3 Missing ⚠️
...apache/rocketmq/tools/admin/DefaultMQAdminExt.java	0.00%	2 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10428      +/-   ##
=============================================
- Coverage      48.05%   47.98%   -0.07%     
+ Complexity     13303    13297       -6     
=============================================
  Files           1377     1377              
  Lines         100611   100653      +42     
  Branches       12991    12997       +6     
=============================================
- Hits           48347    48302      -45     
- Misses         46348    46422      +74     
- Partials        5916     5929      +13     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[oss-sentinel-ai]

Review by github-manager-bot

Summary

This PR fixes the ACL deletion path to properly propagate the policyType parameter from mqadmin to broker. Previously, the DeleteAclRequestHeader supported policyType, but the delete path did not populate it, causing broker-side handling to fall back to PolicyType.CUSTOM and preventing deletion of Default type ACLs.

Findings

• [Good] DeleteAclSubCommand.java:66-69 — Added -t/--policyType option to command line interface.
• [Good] DeleteAclSubCommand.java:90-94 — Parse and normalize policyType from command line.
• [Good] DeleteAclSubCommand.java:123-132 — Added normalizePolicyType() helper method with proper validation.
• [Good] MQClientAPIImpl.java:3659-3665 — Added overloaded deleteAcl() method accepting policyType parameter.
• [Good] DeleteAclRequestHeader.java:39-44 — Added constructor accepting policyType parameter.
• [Good] DefaultMQAdminExt/DefaultMQAdminExtImpl/MQAdminExt — Added corresponding method signatures.
• [Good] AuthorizationMetadataManagerTest.java:161-189 — Enhanced test coverage for deleting ACLs with different policy types.
• [Good] AdminBrokerProcessorTest.java:1191-1216 — Added test for deleteAcl with policyType parameter.
• [Good] MQClientAPIImplTest.java — Added client-side test coverage.

Analysis

The changes are well-structured and follow existing code patterns:

1. Backward compatibility: Existing method signatures are preserved, new overloads added.
2. Parameter validation: normalizePolicyType() validates input and throws IllegalArgumentException for invalid values.
3. Test coverage: Comprehensive tests added at client, broker, and auth layers.
4. Documentation: Command line option properly documented.

Suggestions

1. Consider adding a note in the PR description about the default behavior when policyType is not specified (falls back to CUSTOM).
2. The normalizePolicyType() method could potentially be reused in other ACL commands (create/update) for consistency.

Verdict

✅ Approved — Well-designed fix with proper backward compatibility and comprehensive test coverage.

---

Automated review by github-manager-bot

[RockteMQ-AI]

Review by github-manager-bot

Summary

Adds policyType parameter to ACL deletion across the full stack (CLI → admin API → client API → remoting → broker processor → auth manager), allowing users to specifically delete DEFAULT or CUSTOM policies.

Findings

• [Critical] DeleteAclRequestHeader.java:52-57 — Backward compatibility is well handled: the old constructor is preserved, and AdminBrokerProcessor checks request.getVersion() >= 441 before reading policyType from the header. Older clients will send requests without policyType, which defaults to null and triggers the legacy delete-all behavior.
• [Critical] AuthorizationMetadataManager.java:264-270 — The deleteAcl method now accepts PolicyType and correctly handles null (delete all policies) vs specific type. The implementation in both AclMetadataFileBackedStore and AclMetadataRocketMQStore consistently propagate the policy type filter.
• [Warning] DeleteAclSubCommand.java:113-117 — normalizePolicyType() throws IllegalArgumentException for invalid input. The error message "Invalid --policyType" could be more helpful by listing valid values (e.g., "Invalid --policyType. Valid values: CUSTOM, DEFAULT").
• [Info] BUILD.bazel:28 — Adding //auth dependency to //tools is needed for the PolicyType import but increases the dependency graph for the tools module.

Suggestions

• Improve the error message in normalizePolicyType() to list valid values.
• Consider adding an integration test that verifies backward compatibility: a client without policyType should still delete all policies.

Verdict: Well-structured feature addition. The change is consistent across all layers with proper backward compatibility.

---

Automated review by github-manager-bot