Skip to content

cdi,SPECS.md: allow empty cgroup permissions.#301

Merged
elezar merged 1 commit intocncf-tags:mainfrom
klihub:devel/allow-empty-cgroup-permissions
Jan 22, 2026
Merged

cdi,SPECS.md: allow empty cgroup permissions.#301
elezar merged 1 commit intocncf-tags:mainfrom
klihub:devel/allow-empty-cgroup-permissions

Conversation

@klihub
Copy link
Contributor

@klihub klihub commented Dec 16, 2025 •
edited
Loading

Allow injecting devices with empty cgroup permissions, requested by the "none" permission string or the new pkg/cdi.NoPermissions constant.

Fixes #300

@klihub
Copy link
Contributor Author

klihub commented Dec 16, 2025

/cc @oOraph

@klihub klihub requested a review from elezar December 16, 2025 11:03
@klihub klihub mentioned this pull request Dec 16, 2025
Closed
@klihub
Copy link
Contributor Author

klihub commented Dec 17, 2025 •
edited
Loading

@elezar According to @oOraph this would be useful outside the CRI domain for you guys. He also said you can share a bit more context about why and how.

kad
kad approved these changes Jan 19, 2026
View reviewed changes
Copy link
Contributor

@kad kad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks ok to me.

@klihub
Copy link
Contributor Author

klihub commented Jan 20, 2026

@elezar PTAL, if you have a few spare cycles.

elezar
elezar previously requested changes Jan 20, 2026
View reviewed changes
Copy link
Contributor

@elezar elezar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The addition of none as a special case looks good. I do think that the device validation needs to be properly updated for this though.

@oOraph
Copy link

oOraph commented Jan 20, 2026

hello, thanks for this pull request. Since the cdi is generic it may still be worth adding the none case. I just want to mention some runc specificity regarding permissions that would make the none case ineffective for this runtime:
opencontainers/runc#5073
because none would be equivalent to "m" anyway (which is already possible). This is not the only implem concerned though so, so it might still be worth adding it :)

@klihub klihub force-pushed the devel/allow-empty-cgroup-permissions branch 2 times, most recently from f8d98e4 to df2a787 Compare January 20, 2026 11:03
@klihub klihub requested a review from elezar January 20, 2026 11:04
@elezar elezar dismissed their stale review January 20, 2026 12:28

Changes applied.

@klihub klihub requested a review from bart0sh January 20, 2026 16:29
bart0sh
bart0sh reviewed Jan 21, 2026
View reviewed changes
bart0sh
bart0sh reviewed Jan 21, 2026
View reviewed changes
@klihub klihub force-pushed the devel/allow-empty-cgroup-permissions branch from df2a787 to e762a89 Compare January 21, 2026 17:36
@klihub
cdi,SPECS.md: allow empty cgroup permissions.
90e09e4 
Allow injecting devices with empty cgroup permissions, requested by
the "none" permission string, also defined as pkg/cdi.NoPermissions.

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
@klihub klihub force-pushed the devel/allow-empty-cgroup-permissions branch from e762a89 to 90e09e4 Compare January 21, 2026 17:39
@klihub klihub requested a review from bart0sh January 21, 2026 17:40
bart0sh
bart0sh approved these changes Jan 22, 2026
View reviewed changes
Copy link
Contributor

@bart0sh bart0sh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@bart0sh
Copy link
Contributor

bart0sh commented Jan 22, 2026

/assign @elezar
for approval & merge

elezar
elezar reviewed Jan 22, 2026
View reviewed changes
pkg/cdi/container-edits.go
Comment on lines +373 to +375
case strings.Trim(d.Permissions, "rwm") != "":
return fmt.Errorf("device %q: invalid permissions %q",
d.Path, d.Permissions)
Copy link
Contributor

@elezar elezar Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit: If we invert the logic here, we can have the switch cover all the valid cases and then return an error in the default case. Not a blocker though. Merging for now and we can always revisit.

@elezar elezar merged commit e263219 into cncf-tags:main Jan 22, 2026
8 checks passed
@klihub klihub deleted the devel/allow-empty-cgroup-permissions branch January 23, 2026 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kad kad kad approved these changes

@elezar elezar elezar approved these changes

@bart0sh bart0sh bart0sh approved these changes

Assignees

@elezar elezar

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CDI spec does not allow empty permissions for device nodes

5 participants

@klihub @oOraph @bart0sh @kad @elezar