From eb0daa749172388a852c17c8a1e055823a427a7e Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Wed, 10 Jan 2024 14:05:37 +0100 Subject: [PATCH 1/2] Add AdditionalGIDs field to ContainerEdits This change adds an AdditionalGIDs field to the ContainerEdits structure. This allows CDI spec producers to associate a group ID with the container's user to allow access to, for example, device nodes with specific permissions. Signed-off-by: Evan Lezar --- SPEC.md | 7 ++++ pkg/cdi/container-edits.go | 10 ++++- pkg/cdi/container-edits_test.go | 66 +++++++++++++++++++++++++++++++++ pkg/cdi/spec_test.go | 24 ++++++++++++ pkg/cdi/version.go | 8 ++++ schema/defs.json | 6 +++ specs-go/config.go | 13 ++++--- 7 files changed, 127 insertions(+), 7 deletions(-) diff --git a/SPEC.md b/SPEC.md index 324aa6d3..a6ebbfe0 100644 --- a/SPEC.md +++ b/SPEC.md @@ -29,6 +29,7 @@ Released versions of the spec are available as Git tags. | v0.6.0 | | Add `Annotations` field to `Spec` and `Device` specifications | | | | Allow dots (`.`) in name segment of `Kind` field. | | v0.7.0 | | Add `IntelRdt`field. | +| v0.7.0 | | Add `AdditionalGIDs` to `ContainerEdits` | *Note*: The initial release of a **spec** with version `v0.x.0` will be tagged as `v0.x.0` with subsequent changes to the API applicable to this version tagged as `v0.x.y`. @@ -150,6 +151,11 @@ The keywords "must", "must not", "required", "shall", "shall not", "should", "sh "env": [ "="], (optional) "timeout": (optional) } + ], + // Additional GIDs to add to the container process. + // Note that a value of 0 is ignored. + additionalGIDs: [ (optional) + ] "intelRdt": { (optional) "closID": "", (optional) @@ -234,6 +240,7 @@ The `containerEdits` field has the following definition: * `memBwSchema` (string, OPTIONAL) memory bandwidth allocation schema for the `CLOS`. * `enableCMT` (boolean, OPTIONAL) whether to enable cache monitoring * `enableMBM` (boolean, OPTIONAL) whether to enable memory bandwidth monitoring + * `additionalGids` (array of uint32s, OPTIONAL) A list of additional group IDs to add with the container process. These values are added to the `user.additionalGids` field in the OCI runtime specification. Values of 0 are ignored. ## Error Handling * Kind requested is not present in any CDI file. diff --git a/pkg/cdi/container-edits.go b/pkg/cdi/container-edits.go index 7f92cbbc..824da2df 100644 --- a/pkg/cdi/container-edits.go +++ b/pkg/cdi/container-edits.go @@ -151,6 +151,13 @@ func (e *ContainerEdits) Apply(spec *oci.Spec) error { spec.Linux.IntelRdt = e.IntelRdt.ToOCI() } + for _, additionalGID := range e.AdditionalGIDs { + if additionalGID == 0 { + continue + } + specgen.AddProcessAdditionalGid(additionalGID) + } + return nil } @@ -207,6 +214,7 @@ func (e *ContainerEdits) Append(o *ContainerEdits) *ContainerEdits { if o.IntelRdt != nil { e.IntelRdt = o.IntelRdt } + e.AdditionalGIDs = append(e.AdditionalGIDs, o.AdditionalGIDs...) return e } @@ -217,7 +225,7 @@ func (e *ContainerEdits) isEmpty() bool { if e == nil { return false } - return len(e.Env)+len(e.DeviceNodes)+len(e.Hooks)+len(e.Mounts) == 0 && e.IntelRdt == nil + return len(e.Env)+len(e.DeviceNodes)+len(e.Hooks)+len(e.Mounts)+len(e.AdditionalGIDs) == 0 && e.IntelRdt == nil } // ValidateEnv validates the given environment variables. diff --git a/pkg/cdi/container-edits_test.go b/pkg/cdi/container-edits_test.go index b9b53bd3..28f3378b 100644 --- a/pkg/cdi/container-edits_test.go +++ b/pkg/cdi/container-edits_test.go @@ -563,6 +563,42 @@ func TestApplyContainerEdits(t *testing.T) { }, }, }, + { + name: "additional GIDs are applied", + spec: &oci.Spec{}, + edits: &cdi.ContainerEdits{ + AdditionalGIDs: []uint32{4, 5, 6}, + }, + result: &oci.Spec{ + Process: &oci.Process{ + User: oci.User{ + AdditionalGids: []uint32{4, 5, 6}, + }, + }, + }, + }, + { + name: "duplicate GIDs are ignored", + spec: &oci.Spec{}, + edits: &cdi.ContainerEdits{ + AdditionalGIDs: []uint32{4, 5, 6, 5, 6, 4}, + }, + result: &oci.Spec{ + Process: &oci.Process{ + User: oci.User{ + AdditionalGids: []uint32{4, 5, 6}, + }, + }, + }, + }, + { + name: "additional GID 0 is skipped", + spec: &oci.Spec{}, + edits: &cdi.ContainerEdits{ + AdditionalGIDs: []uint32{0}, + }, + result: &oci.Spec{}, + }, } { t.Run(tc.name, func(t *testing.T) { edits := ContainerEdits{tc.edits} @@ -718,6 +754,36 @@ func TestAppend(t *testing.T) { }, }, }, + { + name: "merge additional GIDs does not deduplicate", + dst: &ContainerEdits{ + ContainerEdits: &cdi.ContainerEdits{ + AdditionalGIDs: []uint32{5}, + }, + }, + src: []*ContainerEdits{ + { + ContainerEdits: &cdi.ContainerEdits{ + AdditionalGIDs: []uint32{0}, + }, + }, + { + ContainerEdits: &cdi.ContainerEdits{ + AdditionalGIDs: []uint32{5}, + }, + }, + { + ContainerEdits: &cdi.ContainerEdits{ + AdditionalGIDs: []uint32{6, 7, 6}, + }, + }, + }, + result: &ContainerEdits{ + ContainerEdits: &cdi.ContainerEdits{ + AdditionalGIDs: []uint32{5, 0, 5, 6, 7, 6}, + }, + }, + }, } { t.Run(tc.name, func(t *testing.T) { dst := tc.dst diff --git a/pkg/cdi/spec_test.go b/pkg/cdi/spec_test.go index 5215097a..0f5026a9 100644 --- a/pkg/cdi/spec_test.go +++ b/pkg/cdi/spec_test.go @@ -695,6 +695,30 @@ func TestRequiredVersion(t *testing.T) { }, expectedVersion: "0.7.0", }, + { + description: "additionalGIDs in spec require v0.7.0", + spec: &cdi.Spec{ + ContainerEdits: cdi.ContainerEdits{ + AdditionalGIDs: []uint32{5}, + }, + }, + expectedVersion: "0.7.0", + }, + { + + description: "additionalGIDs in device require v0.7.0", + spec: &cdi.Spec{ + Devices: []cdi.Device{ + { + Name: "device0", + ContainerEdits: cdi.ContainerEdits{ + AdditionalGIDs: []uint32{5}, + }, + }, + }, + }, + expectedVersion: "0.7.0", + }, } for _, tc := range testCases { diff --git a/pkg/cdi/version.go b/pkg/cdi/version.go index c7750608..e2266944 100644 --- a/pkg/cdi/version.go +++ b/pkg/cdi/version.go @@ -125,11 +125,19 @@ func requiresV070(spec *cdi.Spec) bool { if spec.ContainerEdits.IntelRdt != nil { return true } + // The v0.7.0 spec allows additional GIDs to be specified at a spec level. + if len(spec.ContainerEdits.AdditionalGIDs) > 0 { + return true + } for _, d := range spec.Devices { if d.ContainerEdits.IntelRdt != nil { return true } + // The v0.7.0 spec allows additional GIDs to be specified at a device level. + if len(d.ContainerEdits.AdditionalGIDs) > 0 { + return true + } } return false diff --git a/schema/defs.json b/schema/defs.json index e23aca51..f5494234 100644 --- a/schema/defs.json +++ b/schema/defs.json @@ -157,6 +157,12 @@ "type": "boolean" } } + }, + "additionalGids": { + "type": "array", + "items": { + "$ref": "#/definitions/uint32" + } } } }, diff --git a/specs-go/config.go b/specs-go/config.go index 69c39689..a311ffa1 100644 --- a/specs-go/config.go +++ b/specs-go/config.go @@ -3,7 +3,7 @@ package specs import "os" // CurrentVersion is the current version of the Spec. -const CurrentVersion = "0.6.0" +const CurrentVersion = "0.7.0" // Spec is the base configuration for CDI type Spec struct { @@ -25,11 +25,12 @@ type Device struct { // ContainerEdits are edits a container runtime must make to the OCI spec to expose the device. type ContainerEdits struct { - Env []string `json:"env,omitempty"` - DeviceNodes []*DeviceNode `json:"deviceNodes,omitempty"` - Hooks []*Hook `json:"hooks,omitempty"` - Mounts []*Mount `json:"mounts,omitempty"` - IntelRdt *IntelRdt `json:"intelRdt,omitempty"` + Env []string `json:"env,omitempty"` + DeviceNodes []*DeviceNode `json:"deviceNodes,omitempty"` + Hooks []*Hook `json:"hooks,omitempty"` + Mounts []*Mount `json:"mounts,omitempty"` + IntelRdt *IntelRdt `json:"intelRdt,omitempty"` + AdditionalGIDs []uint32 `json:"additionalGids,omitempty"` } // DeviceNode represents a device node that needs to be added to the OCI spec. From 9ba82ac15a42ef98347b00583f6f01a5f05d84e6 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Wed, 10 Jan 2024 17:34:42 +0100 Subject: [PATCH 2/2] Refactor ContainerEdits.isEmpty Signed-off-by: Evan Lezar --- pkg/cdi/container-edits.go | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/pkg/cdi/container-edits.go b/pkg/cdi/container-edits.go index 824da2df..e5b50db1 100644 --- a/pkg/cdi/container-edits.go +++ b/pkg/cdi/container-edits.go @@ -225,7 +225,25 @@ func (e *ContainerEdits) isEmpty() bool { if e == nil { return false } - return len(e.Env)+len(e.DeviceNodes)+len(e.Hooks)+len(e.Mounts)+len(e.AdditionalGIDs) == 0 && e.IntelRdt == nil + if len(e.Env) > 0 { + return false + } + if len(e.DeviceNodes) > 0 { + return false + } + if len(e.Hooks) > 0 { + return false + } + if len(e.Mounts) > 0 { + return false + } + if len(e.AdditionalGIDs) > 0 { + return false + } + if e.IntelRdt != nil { + return false + } + return true } // ValidateEnv validates the given environment variables.