oci: fix additional GIDs

Test suite:
```yaml

---
apiVersion: v1
kind: Pod
metadata:
  name: test-no-option
  annotations:
    description: "Equivalent of `docker run` (no option)"
spec:
  restartPolicy: Never
  containers:
    - name: main
      image: ghcr.io/containerd/busybox:1.28
      args: ['sh', '-euxc',
             '[ "$(id)" = "uid=0(root) gid=0(root) groups=0(root),10(wheel)" ]']
---
apiVersion: v1
kind: Pod
metadata:
  name: test-group-add-1-group-add-1234
  annotations:
    description: "Equivalent of `docker run --group-add 1 --group-add 1234`"
spec:
  restartPolicy: Never
  containers:
    - name: main
      image: ghcr.io/containerd/busybox:1.28
      args: ['sh', '-euxc',
             '[ "$(id)" = "uid=0(root) gid=0(root) groups=0(root),1(daemon),10(wheel),1234" ]']
  securityContext:
    supplementalGroups: [1, 1234]
---
apiVersion: v1
kind: Pod
metadata:
  name: test-user-1234
  annotations:
    description: "Equivalent of `docker run --user 1234`"
spec:
  restartPolicy: Never
  containers:
    - name: main
      image: ghcr.io/containerd/busybox:1.28
      args: ['sh', '-euxc',
             '[ "$(id)" = "uid=1234 gid=0(root) groups=0(root)" ]']
  securityContext:
    runAsUser: 1234
---
apiVersion: v1
kind: Pod
metadata:
  name: test-user-1234-1234
  annotations:
    description: "Equivalent of `docker run --user 1234:1234`"
spec:
  restartPolicy: Never
  containers:
    - name: main
      image: ghcr.io/containerd/busybox:1.28
      args: ['sh', '-euxc',
             '[ "$(id)" = "uid=1234 gid=1234 groups=1234" ]']
  securityContext:
    runAsUser: 1234
    runAsGroup: 1234
---
apiVersion: v1
kind: Pod
metadata:
  name: test-user-1234-group-add-1234
  annotations:
    description: "Equivalent of `docker run --user 1234 --group-add 1234`"
spec:
  restartPolicy: Never
  containers:
    - name: main
      image: ghcr.io/containerd/busybox:1.28
      args: ['sh', '-euxc',
             '[ "$(id)" = "uid=1234 gid=0(root) groups=0(root),1234" ]']
  securityContext:
    runAsUser: 1234
    supplementalGroups: [1234]
```

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

integration/addition_gids_test.go

@@ -19,6 +19,7 @@
 package integration
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
 	"testing"
@@ -32,26 +33,75 @@ import (
 )
 
 func TestAdditionalGids(t *testing.T) {
+	testImage := images.Get(images.BusyBox)
+	EnsureImageExists(t, testImage)
+	type testCase struct {
+		description string
+		opts        []ContainerOpts
+		expected    string
+	}
+
+	testCases := []testCase{
+		{
+			description: "Equivalent of `docker run` (no option)",
+			opts:        nil,
+			expected:    "groups=0(root),10(wheel)",
+		},
+		{
+			description: "Equivalent of `docker run --group-add 1 --group-add 1234`",
+			opts:        []ContainerOpts{WithSupplementalGroups([]int64{1 /*daemon*/, 1234 /*new group*/})},
+			expected:    "groups=0(root),1(daemon),10(wheel),1234",
+		},
+		{
+			description: "Equivalent of `docker run --user 1234`",
+			opts:        []ContainerOpts{WithRunAsUser(1234)},
+			expected:    "groups=0(root)",
+		},
+		{
+			description: "Equivalent of `docker run --user 1234:1234`",
+			opts:        []ContainerOpts{WithRunAsUser(1234), WithRunAsGroup(1234)},
+			expected:    "groups=1234",
+		},
+		{
+			description: "Equivalent of `docker run --user 1234 --group-add 1234`",
+			opts:        []ContainerOpts{WithRunAsUser(1234), WithSupplementalGroups([]int64{1234})},
+			expected:    "groups=0(root),1234",
+		},
+		{
+			description: "Equivalent of `docker run --user daemon` (Supported by CRI, although unsupported by kube-apiserver)",
+			opts:        []ContainerOpts{WithRunAsUsername("daemon")},
+			expected:    "groups=1(daemon)",
+		},
+		{
+			description: "Equivalent of `docker run --user daemon --group-add 1234` (Supported by CRI, although unsupported by kube-apiserver)",
+			opts:        []ContainerOpts{WithRunAsUsername("daemon"), WithSupplementalGroups([]int64{1234})},
+			expected:    "groups=1(daemon),1234",
+		},
+	}
+
+	for i, tc := range testCases {
+		i, tc := i, tc
+		tBasename := fmt.Sprintf("case-%d", i)
+		t.Run(tBasename, func(t *testing.T) {
+			t.Log(tc.description)
+			t.Logf("Expected=%q", tc.expected)
+
 			testPodLogDir := t.TempDir()
 
 			t.Log("Create a sandbox with log directory")
-	sb, sbConfig := PodSandboxConfigWithCleanup(t, "sandbox", "additional-gids",
+			sb, sbConfig := PodSandboxConfigWithCleanup(t, "sandbox", tBasename,
 				WithPodLogDirectory(testPodLogDir))
 
-	var (
-		testImage     = images.Get(images.BusyBox)
-		containerName = "test-container"
-	)
-
-	EnsureImageExists(t, testImage)
-
 			t.Log("Create a container to print id")
+			containerName := tBasename
 			cnConfig := ContainerConfig(
 				containerName,
 				testImage,
+				append(
+					[]ContainerOpts{
 						WithCommand("id"),
 						WithLogPath(containerName),
-		WithSupplementalGroups([]int64{1 /*daemon*/, 1234 /*new group*/}),
+					}, tc.opts...)...,
 			)
 			cn, err := runtimeService.CreateContainer(sb, cnConfig, sbConfig)
 			require.NoError(t, err)
@@ -74,5 +124,7 @@ func TestAdditionalGids(t *testing.T) {
 			t.Log("Search additional groups in container log")
 			content, err := os.ReadFile(filepath.Join(testPodLogDir, containerName))
 			assert.NoError(t, err)
-	assert.Contains(t, string(content), "groups=1(daemon),10(wheel),1234")
+			assert.Contains(t, string(content), tc.expected+"\n")
+		})
+	}
 }

integration/main_test.go

@@ -384,6 +384,45 @@ func WithLogPath(path string) ContainerOpts {
 	}
 }
 
+// WithRunAsUser sets the uid.
+func WithRunAsUser(uid int64) ContainerOpts {
+	return func(c *runtime.ContainerConfig) {
+		if c.Linux == nil {
+			c.Linux = &runtime.LinuxContainerConfig{}
+		}
+		if c.Linux.SecurityContext == nil {
+			c.Linux.SecurityContext = &runtime.LinuxContainerSecurityContext{}
+		}
+		c.Linux.SecurityContext.RunAsUser = &runtime.Int64Value{Value: uid}
+	}
+}
+
+// WithRunAsUsername sets the username.
+func WithRunAsUsername(username string) ContainerOpts {
+	return func(c *runtime.ContainerConfig) {
+		if c.Linux == nil {
+			c.Linux = &runtime.LinuxContainerConfig{}
+		}
+		if c.Linux.SecurityContext == nil {
+			c.Linux.SecurityContext = &runtime.LinuxContainerSecurityContext{}
+		}
+		c.Linux.SecurityContext.RunAsUsername = username
+	}
+}
+
+// WithRunAsGroup sets the gid.
+func WithRunAsGroup(gid int64) ContainerOpts {
+	return func(c *runtime.ContainerConfig) {
+		if c.Linux == nil {
+			c.Linux = &runtime.LinuxContainerConfig{}
+		}
+		if c.Linux.SecurityContext == nil {
+			c.Linux.SecurityContext = &runtime.LinuxContainerSecurityContext{}
+		}
+		c.Linux.SecurityContext.RunAsGroup = &runtime.Int64Value{Value: gid}
+	}
+}
+
 // WithSupplementalGroups adds supplemental groups.
 func WithSupplementalGroups(gids []int64) ContainerOpts {
 	return func(c *runtime.ContainerConfig) {

oci/spec_opts.go

@@ -121,6 +121,17 @@ func setCapabilities(s *Spec) {
 	}
 }
 
+// ensureAdditionalGids ensures that the primary GID is also included in the additional GID list.
+func ensureAdditionalGids(s *Spec) {
+	setProcess(s)
+	for _, f := range s.Process.User.AdditionalGids {
+		if f == s.Process.User.GID {
+			return
+		}
+	}
+	s.Process.User.AdditionalGids = append([]uint32{s.Process.User.GID}, s.Process.User.AdditionalGids...)
+}
+
 // WithDefaultSpec returns a SpecOpts that will populate the spec with default
 // values.
 //
@@ -586,7 +597,9 @@ func WithNamespacedCgroup() SpecOpts {
 //	user, uid, user:group, uid:gid, uid:group, user:gid
 func WithUser(userstr string) SpecOpts {
 	return func(ctx context.Context, client Client, c *containers.Container, s *Spec) error {
+		defer ensureAdditionalGids(s)
 		setProcess(s)
+		s.Process.User.AdditionalGids = nil
 
 		// For LCOW it's a bit harder to confirm that the user actually exists on the host as a rootfs isn't
 		// mounted on the host and shared into the guest, but rather the rootfs is constructed entirely in the
@@ -679,7 +692,9 @@ func WithUser(userstr string) SpecOpts {
 // WithUIDGID allows the UID and GID for the Process to be set
 func WithUIDGID(uid, gid uint32) SpecOpts {
 	return func(_ context.Context, _ Client, _ *containers.Container, s *Spec) error {
+		defer ensureAdditionalGids(s)
 		setProcess(s)
+		s.Process.User.AdditionalGids = nil
 		s.Process.User.UID = uid
 		s.Process.User.GID = gid
 		return nil
@@ -692,7 +707,9 @@ func WithUIDGID(uid, gid uint32) SpecOpts {
 // additionally sets the gid to 0, and does not return an error.
 func WithUserID(uid uint32) SpecOpts {
 	return func(ctx context.Context, client Client, c *containers.Container, s *Spec) (err error) {
+		defer ensureAdditionalGids(s)
 		setProcess(s)
+		s.Process.User.AdditionalGids = nil
 		setUser := func(root string) error {
 			user, err := UserFromPath(root, func(u user.User) bool {
 				return u.Uid == int(uid)
@@ -738,7 +755,9 @@ func WithUserID(uid uint32) SpecOpts {
 // the container.
 func WithUsername(username string) SpecOpts {
 	return func(ctx context.Context, client Client, c *containers.Container, s *Spec) (err error) {
+		defer ensureAdditionalGids(s)
 		setProcess(s)
+		s.Process.User.AdditionalGids = nil
 		if s.Linux != nil {
 			setUser := func(root string) error {
 				user, err := UserFromPath(root, func(u user.User) bool {
@@ -789,7 +808,9 @@ func WithAdditionalGIDs(userstr string) SpecOpts {
 			return nil
 		}
 		setProcess(s)
+		s.Process.User.AdditionalGids = nil
 		setAdditionalGids := func(root string) error {
+			defer ensureAdditionalGids(s)
 			var username string
 			uid, err := strconv.Atoi(userstr)
 			if err == nil {
@@ -860,6 +881,7 @@ func WithAppendAdditionalGroups(groups ...string) SpecOpts {
 		}
 		setProcess(s)
 		setAdditionalGids := func(root string) error {
+			defer ensureAdditionalGids(s)
 			gpath, err := fs.RootPath(root, "/etc/group")
 			if err != nil {
 				return err

oci/spec_opts_linux_test.go

@@ -180,23 +180,23 @@ sys:x:3:root,bin,adm
 	}{
 		{
 			user:     "root",
-			expected: []uint32{1, 2, 3},
+			expected: []uint32{0, 1, 2, 3},
 		},
 		{
 			user:     "1000",
-			expected: nil,
+			expected: []uint32{0},
 		},
 		{
 			user:     "bin",
-			expected: []uint32{2, 3},
+			expected: []uint32{0, 2, 3},
 		},
 		{
 			user:     "bin:root",
-			expected: []uint32{},
+			expected: []uint32{0},
 		},
 		{
 			user:     "daemon",
-			expected: []uint32{1},
+			expected: []uint32{0, 1},
 		},
 	}
 	for _, testCase := range testCases {
@@ -463,6 +463,7 @@ daemon:x:2:root,bin,daemon
 		{
 			name:     "no additional gids",
 			groups:   []string{},
+			expected: []uint32{0},
 		},
 		{
 			name:     "no additional gids, append root gid",
@@ -472,7 +473,7 @@ daemon:x:2:root,bin,daemon
 		{
 			name:     "no additional gids, append bin and daemon gids",
 			groups:   []string{"bin", "daemon"},
-			expected: []uint32{1, 2},
+			expected: []uint32{0, 1, 2},
 		},
 		{
 			name:           "has root additional gids, append bin and daemon gids",
@@ -483,12 +484,13 @@ daemon:x:2:root,bin,daemon
 		{
 			name:     "append group id",
 			groups:   []string{"999"},
-			expected: []uint32{999},
+			expected: []uint32{0, 999},
 		},
 		{
 			name:     "unknown group",
 			groups:   []string{"unknown"},
 			err:      "unable to find group unknown",
+			expected: []uint32{0},
 		},
 	}
 

pkg/cri/sbserver/container_create_linux.go

@@ -137,7 +137,8 @@ func (c *criService) containerSpecOpts(config *runtime.ContainerConfig, imageCon
 		// Because it is still useful to get additional gids for uid 0.
 		userstr = strconv.FormatInt(securityContext.GetRunAsUser().GetValue(), 10)
 	}
-	specOpts = append(specOpts, customopts.WithAdditionalGIDs(userstr))
+	specOpts = append(specOpts, customopts.WithAdditionalGIDs(userstr),
+		customopts.WithSupplementalGroups(securityContext.GetSupplementalGroups()))
 
 	asp := securityContext.GetApparmor()
 	if asp == nil {

pkg/cri/server/container_create_linux.go

@@ -376,7 +376,8 @@ func (c *criService) containerSpecOpts(config *runtime.ContainerConfig, imageCon
 		// Because it is still useful to get additional gids for uid 0.
 		userstr = strconv.FormatInt(securityContext.GetRunAsUser().GetValue(), 10)
 	}
-	specOpts = append(specOpts, customopts.WithAdditionalGIDs(userstr))
+	specOpts = append(specOpts, customopts.WithAdditionalGIDs(userstr),
+		customopts.WithSupplementalGroups(securityContext.GetSupplementalGroups()))
 
 	asp := securityContext.GetApparmor()
 	if asp == nil {