refactor(policy): extract shared L7 endpoint validation#2389
refactor(policy): extract shared L7 endpoint validation#2389gracesmith6504 wants to merge 1 commit into
Conversation
Move 9 L7 endpoint semantic checks into a shared validate_l7_endpoint_semantics() function in openshell-policy. Both profile lint (openshell-providers) and the runtime validator (openshell-supervisor-network) now call the shared function via a lightweight L7EndpointFields field bag, eliminating duplication and preventing the two implementations from drifting apart. Update server test fixtures that used protocol without rules or access, which the shared validator now correctly rejects. Fixes NVIDIA#1714 Signed-off-by: Grace Smith <grasmith@redhat.com>
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This PR is project-valid because it implements the confirmed provider-v2 lint/runtime validation gap in #1714 using the shared policy-layer approach recommended during maintainer triage.
Head SHA: 73f81c6673a825dde5b41c8f1d8710b5cb89a1f9
Review findings:
- Changes are requested for collection-presence regression, mixed-case MCP validation/runtime inconsistency, and divergent malformed-rule classification.
- One additional suggestion asks the author to preserve or explicitly test the intended JSON-RPC diagnostic aggregation.
Docs: Not needed; this restores lint/runtime parity for already-documented endpoint semantics and does not introduce new user-facing behavior.
Next state: gator:in-review pending an author update.
| pub access: &'a str, | ||
|
|
||
| /// `true` when the endpoint has a non-empty rules list. | ||
| pub has_rules: bool, |
There was a problem hiding this comment.
gator-agent
Warning: has_rules conflates an absent rules field with an explicitly empty rules: []. The old runtime validator explicitly rejected an empty list, but that check is removed, so an L4 endpoint with rules: [] now produces no L7 error and rules: [] plus access is accepted. Preserve presence separately or retain the runtime-specific empty-list check, with regression coverage for absent versus empty collections.
| impl L7Protocol { | ||
| /// Parse a protocol string into a known variant. | ||
| pub fn parse(s: &str) -> Option<Self> { | ||
| match s.to_ascii_lowercase().as_str() { |
There was a problem hiding this comment.
gator-agent
Warning: Case-insensitive parsing now drives the shared MCP exemptions, but downstream validation and expand_access_presets still compare the authored string to lowercase "mcp". For example, protocol: MCP with allow_all_known_mcp_methods: true and no rules now passes semantic validation, but the runtime does not synthesize the MCP allow-all rule. Normalize protocol values before all downstream processing or reject noncanonical casing, then add a validation-to-expansion regression test.
| ep.get("rules") | ||
| .and_then(|v| v.as_array()) | ||
| .is_some_and(|rules| { | ||
| !rules.is_empty() && rules.iter().all(|rule| rule.get("allow").is_none()) |
There was a problem hiding this comment.
gator-agent
Warning: The two callers still disagree on what constitutes a missing allow clause: profile lint uses Option::is_none, while this adapter only detects a missing JSON key. allow: null passes here, and proto-to-OPA conversion turns allow: None into an empty allow object, so configurations rejected by lint can pass runtime semantic validation. Preserve optionality through conversion or pass a shared rule-shape classification, and test the same malformed rule through both callers.
| } | ||
|
|
||
| // 4. json-rpc requires explicit rules | ||
| if is_jsonrpc && !ep.has_rules && ep.access.is_empty() { |
There was a problem hiding this comment.
gator-agent
Suggestion: This changes runtime error aggregation: JSON-RPC without rules previously emitted both the explicit-rules and base-allow errors, while JSON-RPC with an access preset also emitted the missing-rules error. If reducing redundant diagnostics is intentional, add exact error-list tests for these combinations; otherwise preserve the previous aggregation.
johntmyers
added
the
gator:in-review
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 #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