Generate CDI specification including additional GIDs

[elezar]

Includes the change from #1655 (which could be reviewed separately).

This change adds logic to include the AdditionalGIDs required for a device node in a generated CDI specification.

When a device node requires specific group membership for access, a non-root user running in a container (e.g. using the -u Docker command line argument) may not have access to the device.

In the v0.7.0 CDI specification, support was added for specifying additional GIDs in the CDI specification and these changes extract the required information from the device nodes on the host if available and update the (non-root) additional GIDs in the container.

Since this MAY affect the MINIMUM required CDI spec version, a feature flag is provided to allow users to opt-out of this. In: /etc/nvidia-container-runtime/config.toml

[features]
   no-additional-gids-for-device-nodes = true

or using the nvidia-ctk config CLI command:

sudo nvidia-ctk config --in-place --set features.no-additional-gids-for-device-nodes

It is also exposed as an nvcdi feature flag (no-additional-gids-for-device-nodes) which can be set at construction of the CDI spec generator:

nvcdi.WithFeatureFlag(nvcdi.FeatureNoAdditionalGIDsForDeviceNodes)

When using nvidia-ctk cdi generate to generate CDI specs:

nvidia-ctk cdi generate --feature-flag=no-additional-gids-for-device-nodes

In the case of the nvidia-cdi-refresh.service updating /etc/nvidia-container-toolkit/nvidia-cdi-refresh.env to include:

NVIDIA_CTK_CDI_GENERATE_FEATURE_FLAGS=no-additional-gids-for-device-nodes

will also ensure that a spec without the additional fields is generated.

The runtime support for the v0.7.0 spec is as follows:

• containerd
  • (>= 1.7.16) - containerd/containerd@7a2f49f
  • (>= 1.8.0, >= 2.0.0) - containerd/containerd@1b62224
• docker (>= 26.1.0) - moby/moby@745e235
• podman (>= 5.1.0) - containers/podman@a40cf31
• crio (>= 1.30.0) - cri-o/cri-o@fd9aa76

[elezar]

Note: This updates the default spec version to v0.7.0 since the DRM devices generally require additional GIDs. As such, this should probably be an opt-in (or at least an opt-out) feature.

[coveralls]

Pull Request Test Coverage Report for Build 22136305391

Details

• 73 of 119 (61.34%) changed or added relevant lines in 16 files are covered.
• 20 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.08%) to 39.245%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/nvcdi/lib-nvml.go	0	1	0.0%
pkg/nvcdi/mig-device-nvml.go	0	1	0.0%
internal/edits/device.go	23	25	92.0%
pkg/nvcdi/lib-wsl.go	0	2	0.0%
pkg/nvcdi/management.go	0	2	0.0%
internal/modifier/discover.go	14	18	77.78%
internal/modifier/cdi.go	0	6	0.0%
pkg/nvcdi/transform/deduplicate.go	11	17	64.71%
internal/edits/edits.go	6	28	21.43%

Files with Coverage Reduction	New Missed Lines	%
internal/edits/edits.go	20	8.82%

Totals
Change from base Build 22134051897:	0.08%
Covered Lines:	5769
Relevant Lines:	14700

---

💛 - Coveralls

[cdesiniotis]

Not sure I follow this. The device is world readable or writeable if (dn.FileMode.Perm() & 06) != 0. Don't we want to add the group id for the opposite condition when the device is NOT world readable AND world writeable?

In my mind, this should be re-written as:

if rwPermissionsForOther := (dn.FileMode.Perm() & 06); rwPermissionsForOther != 06 {
  return []uint32{*dn.GID}
}

but let me know if I am misunderstanding.

[elezar]

Yes. You're correct. The logic is not complete. Thanks for your suggestion. Let me also add some unit tests.

Update: After adding the unit test, I decided to also introduce two helpers (isWorldReadable, isWorldWritable` for readability).

[cdesiniotis]

Same comment as before. Should we mention both read and write permissions?
s/not readable by the user/not readable and writeable by the user/