feat(core): add CDI policy resolver - #2775
Conversation
722e6d9 to
a81daf0
Compare
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.
| } | ||
|
|
||
| fn add_gid(&mut self, gid: u32) { | ||
| if gid == 0 { |
There was a problem hiding this comment.
[P2] Reject root supplemental GID instead of silently dropping it. The accepted #1606 implementation plan requires GID 0 to fail validation. Skipping it makes an unsafe CDI request look valid and prevents the supervisor from emitting its validation finding. Return a dedicated CdiError and change the existing root-GID test to expect rejection.
There was a problem hiding this comment.
That's a fair point. When generating a CDI spec, the NVIDIA Contianer Toolkit skips gid==0 but there is no guarantee that other vendors do.
Note that the the Go implementation of the CDI injection code also skips gid==0.
Signed-off-by: Evan Lezar <elezar@nvidia.com>
Signed-off-by: Evan Lezar <elezar@nvidia.com>
a81daf0 to
386bbb5
Compare
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