Conversation
722e6d9 to
a81daf0
Compare
drew
left a comment
drew
left a comment
There was a problem hiding this comment.
Two fail-closed validation issues need correction before this layer merges. I reproduced the cache-refresh case with a focused openshell-core test: two conflicting specs defining the selected device currently return Ok.
| context: &CdiContext, | ||
| selected_devices: &[String], | ||
| ) -> Result<CdiContainerEdits, CdiError> { | ||
| let (mut cache, refresh_error) = build_cache(&context.spec_dirs); |
There was a problem hiding this comment.
[P1] Fail on cache refresh errors before resolving selected devices. The pinned CDI cache can return an error while retaining a resolvable device (for example, two equal-priority specs defining nvidia.com/gpu=0). Because refresh_error is only consulted when lookup fails, this resolver accepts one ambiguous spec instead of failing closed; I reproduced this with two conflicting YAML specs and resolve_cdi_context returned Ok. Reject the refresh error here, or prove specifically that the selected device was unaffected, and add a duplicate-selected-device regression test.
There was a problem hiding this comment.
The issue here is that the building the cache loads all CDI specs from the specified spec directories. These could include specs from different vendors or devices. Furthermore, if a spec fails to load (e.g due to malfomed YAML) there is no way to know which vendor or devices it was intended for.
This is the reason that we retain this information and include it if a device is unresolvable. This behaviour aligns with the behaviour for other CDI consumers including Docker, Podman, Containerd, Crio, and Kata.
I can add a test to prove that duplicate devices of the same priority causes a failure though.
a81daf0 to
386bbb5
Compare
elezar
added
test:e2e
|
Label |
|
Label |
b65e4d8 to
1878d3a
Compare
1878d3a to
7ab8ef5
Compare
7ab8ef5 to
620db0f
Compare
620db0f to
922d540
Compare
7fbf7e2 to
3c5d1ec
Compare
3c5d1ec to
6b7e889
Compare
|
The current required E2E run (34984111251) is red in three jobs that appear unrelated to the CDI resolver: the WSL GPU sandbox create timed out, rust-docker timed out in the download-only case, and oidc-pkce-docker hit SQLite |
4913587 to
ed094c9
Compare
drew
left a comment
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
This roadmap-backed core layer is project-valid, and I reviewed it with #2776 as the downstream stack consumer. Thanks @elezar—I checked your explanation that cache refresh errors can originate in unrelated vendor specs; because #1606 makes malformed or ambiguous CDI input fail closed and the consumer treats resolver success as permission to launch, the remaining refresh behavior still needs correction.
Action required: please address the two fail-closed resolver findings below and push an updated head.
Blocking findings:
GATOR-ed094c94-01: shared-library parent expansion can recreate a prohibited broad policy path.GATOR-ed094c94-02: selected devices can resolve successfully without a complete, successful cache refresh.
Carried findings:
- None
Non-blocking suggestions:
- None
Gator metadata
- Validation: Project-valid as the maintainer-authored shared-core layer for roadmap issue #1606 and stacked consumer PR #2776.
- Docs: Not needed in this internal resolver layer; the user-visible consumer documentation is in #2776.
- Checks: Current-head Branch Checks, Helm Lint, and Trivy Changes are green.
- E2E:
test:e2eandtest:e2e-gpuare applied; both current-head required gates are green. - Head SHA:
ed094c94e95b75058836e3ba5c125a89e938cc14 - Base SHA:
473d1e997439c3577fabd9014ba8e8959e3c2444 - Merge base SHA:
473d1e997439c3577fabd9014ba8e8959e3c2444 - Patch ID:
9e2add7e653940199201e01aab9880af26a1a42f - Gator payload:
10 - Review mode:
initial - Previous reviewed SHA: none
- Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review
| for (path, access) in self.mount_paths { | ||
| match access { | ||
| CdiAccess::ReadOnly => { | ||
| read_only_paths.insert(shared_library_parent(&path).unwrap_or(path)); |
There was a problem hiding this comment.
gator-agent
Warning — GATOR-ed094c94-01 · Revalidate paths after library-parent expansion
Summary: A selected CDI spec can mount a shared library directly below a prohibited broad directory, such as /libcuda.so.1. The resolver validates the file path, then replaces it with / without reapplying the broad-path guard. The stacked consumer would grant read access to the entire workload filesystem; /usr/libcuda.so.1 similarly recreates the prohibited /usr grant.
Fix: Validate the expanded parent with the same unsafe-path rules before inserting it, and reject prohibited parents. Add regressions for /libcuda.so.1 and /usr/libcuda.so.1.
Verify: Resolve a selected spec containing each mount. Current head returns / or /usr in read_only_paths; the fixed resolver must return UnsafePolicyPath.
Agent context
- Ownership: This PR performs parent expansion after its only unsafe-path validation point.
- Location:
crates/openshell-core/src/cdi_linux.rs:123
| pub fn resolve_cdi_context(context: &CdiContext) -> Result<CdiDerivedRequirements, CdiError> { | ||
| validate_context(context)?; | ||
| let selected_devices = selected_cdi_devices(&context.selected_devices); | ||
| if selected_devices.is_empty() || context.spec_dirs.is_empty() { |
There was a problem hiding this comment.
gator-agent
Warning — GATOR-ed094c94-02 · Require complete CDI discovery before success
Summary: A compute driver can request a CDI device while providing no spec directories, and the resolver reports empty success. Separately, a malformed or conflicting projected spec can make refresh fail while a retained selected device still resolves. The downstream sandbox then launches with missing, incomplete, or ambiguous device policy instead of failing closed.
Fix: Error when selection is non-empty but spec directories are empty. Also terminate on every cache refresh error before lookup, preserving the upstream diagnostic; retain missing-ID, malformed-spec, and duplicate-selected-device coverage.
Verify: Resolve a selected ID with no directories, then with a valid selected spec beside malformed YAML. Both currently can return Ok; both must deterministically fail.
Agent context
- Ownership: This PR defines the new resolver success boundary and suppresses these incomplete-discovery states.
- Location:
crates/openshell-core/src/cdi_linux.rs:140
ed094c9 to
eab5406
Compare
drew
left a comment
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
I rechecked the rebased head in stack context with #2776. The author-only delta updates lockfile dependency versions but does not change either fail-closed CDI resolver path, so both existing findings remain open; no new findings were added.
Action required: @elezar, please address the two carried findings in their existing review threads and push an updated head.
Blocking findings:
- No new blocking findings.
Carried findings:
GATOR-ed094c94-01: shared-library parent expansion still allows a prohibited broad policy path.GATOR-ed094c94-02: non-empty device selection can still succeed without complete CDI discovery and a successful cache refresh.
Gator metadata
- Validation: Project-valid as the maintainer-authored shared-core layer for roadmap issue #1606 and stacked consumer PR #2776.
- Docs: Not needed in this internal resolver layer; the user-visible consumer documentation is in #2776.
- Checks: Current-head Branch Checks, Helm Lint, and Trivy Changes are green.
- E2E:
test:e2eandtest:e2e-gpuare applied; both current-head required gates are green. - Head SHA:
eab54064acd753a8cfda0f92e28bbdb4d0994e00 - Base SHA:
d02ebe2c4b22068960acefc28a2515be97d24c17 - Merge base SHA:
d02ebe2c4b22068960acefc28a2515be97d24c17 - Patch ID:
a3c8870dc3ad0c632af919ce155841e065bfc3b6 - Gator payload:
10 - Review mode:
follow_up - Previous reviewed SHA:
ed094c94e95b75058836e3ba5c125a89e938cc14 - Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review
Signed-off-by: Evan Lezar <elezar@nvidia.com>
eab5406 to
ff1ffe7
Compare
drew
left a comment
drew
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
The current head is a rebase-equivalent copy of the already reviewed CDI resolver patch, so no new code-review round was needed. Evaluating it with stacked consumer PR #2776 confirms that both existing fail-closed obligations remain relevant: the consumer still turns resolver output into launch policy, while the resolver patch has not changed either affected path.
Action required: @elezar, please address the two carried findings in their existing review threads and push an updated head.
Blocking findings:
- No new blocking findings.
Carried findings:
GATOR-ed094c94-01: shared-library parent expansion still allows a prohibited broad policy path.GATOR-ed094c94-02: non-empty device selection can still succeed without complete CDI discovery and a successful cache refresh.
Gator metadata
- Validation: Project-valid as the maintainer-authored shared-core layer for roadmap issue #1606 and stacked consumer PR #2776.
- Docs: Not needed in this internal resolver layer; the user-visible consumer documentation is in #2776.
- Checks: Branch Checks, Helm Lint, Trivy Changes, and GPU E2E are green; Core E2E is failing in the rust-docker test job. Review feedback remains the active gate.
- E2E:
test:e2eandtest:e2e-gpuare applied; the current-head GPU gate is green and the core gate is red. - Head SHA:
ff1ffe791bce60d38a29c429925231fb435ed31c - Base SHA:
069ae6bd966c4ced02b5cff0a26aee0a7f23a82f - Merge base SHA:
069ae6bd966c4ced02b5cff0a26aee0a7f23a82f - Patch ID:
a3c8870dc3ad0c632af919ce155841e065bfc3b6 - Gator payload:
10 - Review mode:
already_reviewed(rebase-equivalent) - Previous reviewed SHA:
eab54064acd753a8cfda0f92e28bbdb4d0994e00 - Review budget exhausted: no
- Maintainer decision required: no
- Next state:
gator:in-review - Blocked reason:
author_changes_required
drew
added
gator:blocked
This branch has not been deployed
3 participants
Summary
Add a portable, fail-closed CDI context resolver that turns validated CDI specs into policy-relevant device, mount, and group requirements. This is the foundation of the stacked #1606 implementation.
Related Issue
Part of #1606.
Changes
Testing
mise run pre-commitpassesChecklist