Merge pull request #179 from elezar/add-additional-guids

Add AdditionalGIDs field to ContainerEdits

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":  [ "<envName>=<envValue>"], (optional)
                     "timeout": <int> (optional)
                 }
+            ],
+            // Additional GIDs to add to the container process.
+            // Note that a value of 0 is ignored.
+            additionalGIDs: [ (optional)
+              <uint32>
             ]
             "intelRdt": { (optional)
                 "closID": "<name>", (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.

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,25 @@ 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
+	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.

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

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 {

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

schema/defs.json

@@ -157,6 +157,12 @@
                             "type": "boolean"
                         }
                     }
+                },
+                "additionalGids": {
+                    "type": "array",
+                    "items": {
+                        "$ref": "#/definitions/uint32"
+                    }
                 }
             }
         },

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.