test(bmc-proxy): enumerate ACL parsing and matching#3979
Conversation
Consolidate ACL parser, matcher, and policy checks into grouped tables while removing equivalent cases and unused private verb conversions. Add the missing method normalization, URI validation, wildcard boundaries, and request path guards. This supports NVIDIA#3977 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Summary by CodeRabbit
WalkthroughACL verb conversion now uses string parsing and display formatting without Serde deserialization. ACL parsing, path matching, and configuration behavior tests were consolidated into labeled scenario-driven cases covering normalization, wildcards, invalid paths, ordering, and default denial. ChangesACL validation
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/bmc-proxy/src/acl.rs (1)
391-407: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winNormalize the error text to lowercase.
AclPathParseErrorrenderserrdirectly into the user-facing message, so"Invalid verb"should be"invalid verb"to match the repository convention.🔧 Proposed fix
_ => Err(AclPathParseError { orig: s.to_string(), - err: "Invalid verb".to_string(), + err: "invalid verb".to_string(), }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/bmc-proxy/src/acl.rs` around lines 391 - 407, Update the error text in the AclVerb::from_str implementation so the invalid-verb message uses lowercase, changing the err value from “Invalid verb” to “invalid verb” while preserving the existing parsing behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/bmc-proxy/src/acl.rs`:
- Around line 391-407: Update the error text in the AclVerb::from_str
implementation so the invalid-verb message uses lowercase, changing the err
value from “Invalid verb” to “invalid verb” while preserving the existing
parsing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 503b8ca4-690c-4c5a-915d-468818a5c50b
📒 Files selected for processing (1)
crates/bmc-proxy/src/acl.rs
|
@coderabbitai Thanks -- declining the out-of-diff casing suggestion. This PR intentionally leaves production diagnostics unchanged, and sibling AclPathParseError messages in the same parser are already sentence-cased (for example, "Path must..." and "ACL entry..."). Lowercasing only "Invalid verb" would not establish a consistent convention and is outside this test-only change. |
|
Tip For best results, initiate chat on the files or code changes.
🐇 |
|
Tip For best results, initiate chat on the files or code changes.
🐇 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/bmc-proxy/src/acl.rs (1)
391-407: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winError phrasing should conform to the lowercase convention.
The
FromStrallowlist itself is sound and appropriately restrictive. However, the diagnostic string"Invalid verb"begins with a capital letter, which departs from the repository's error-message casing convention (note the sibling"ACL entry cannot be empty"is intentionally capitalized only becauseACLis an acronym). Please normalize to a lowercase phrase.As per coding guidelines: "Error display messages must be lowercase phrases without trailing periods."
✏️ Proposed casing fix
_ => Err(AclPathParseError { orig: s.to_string(), - err: "Invalid verb".to_string(), + err: "invalid verb".to_string(), }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/bmc-proxy/src/acl.rs` around lines 391 - 407, Update the error message returned by AclVerb::from_str for unsupported verbs to use a lowercase phrase without trailing punctuation, changing only the existing "Invalid verb" diagnostic while preserving the allowlist and error structure.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/bmc-proxy/src/acl.rs`:
- Around line 391-407: Update the error message returned by AclVerb::from_str
for unsupported verbs to use a lowercase phrase without trailing punctuation,
changing only the existing "Invalid verb" diagnostic while preserving the
allowlist and error structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8ee9a56d-cee0-4d1b-9ca9-2a61ca11aa80
📒 Files selected for processing (1)
crates/bmc-proxy/src/acl.rs
2 participants
BMC proxy ACL tests were spread across one-off assertion functions, with equivalent path cases repeated at both the entry and configuration layers. This consolidates parser, matcher, and policy checks into grouped
carbide-test-supportscenarios, trims the overlapping cases, and adds the missing method, URI, wildcard-boundary, and request-path coverage.The private
AclVerbconversions and direct deserializer had no callers. Removing them keeps the coverage result focused on supported behavior instead of adding tests for unreachable interfaces.Related issues
Closes #3977
Part of #3914
Type of Change
Breaking Changes
Testing
Unit tests added/updated
Integration tests added/updated
Manual testing performed
No testing required (docs, internal refactor, etc.)
cargo test --locked -p carbide-bmc-proxy --bin carbide-bmc-proxy acl::testsFull
carbide-bmc-proxybinary suite undercargo llvm-cov(39 tests)cargo make format-nightlycargo make clippycargo make carbide-lintsAdditional Notes
Coverage below is for production executable lines in
crates/bmc-proxy/src/acl.rs; test-module lines are excluded.The remaining uncovered line propagates an injected formatter I/O error. Covering it would require an artificial failing formatter rather than a meaningful ACL behavior case.