Remove pod mutation for PVs with supplemental GIDs

pkg/kubelet/dockertools/docker_manager.go

@@ -54,6 +54,7 @@ import (
 	"k8s.io/kubernetes/pkg/kubelet/types"
 	"k8s.io/kubernetes/pkg/kubelet/util/cache"
 	"k8s.io/kubernetes/pkg/kubelet/util/format"
+	"k8s.io/kubernetes/pkg/kubelet/volumemanager"
 	"k8s.io/kubernetes/pkg/runtime"
 	"k8s.io/kubernetes/pkg/securitycontext"
 	kubetypes "k8s.io/kubernetes/pkg/types"
@@ -137,6 +138,9 @@ type DockerManager struct {
 	// Network plugin.
 	networkPlugin network.NetworkPlugin
 
+	// Kubelet Volume Manager.
+	volumeManager volumemanager.VolumeManager
+
 	// Health check results.
 	livenessManager proberesults.Manager
 
@@ -209,6 +213,7 @@ func NewDockerManager(
 	containerLogsDir string,
 	osInterface kubecontainer.OSInterface,
 	networkPlugin network.NetworkPlugin,
+	volumeManager volumemanager.VolumeManager,
 	runtimeHelper kubecontainer.RuntimeHelper,
 	httpClient types.HttpGetter,
 	execHandler ExecHandler,
@@ -247,6 +252,7 @@ func NewDockerManager(
 		dockerRoot:             dockerRoot,
 		containerLogsDir:       containerLogsDir,
 		networkPlugin:          networkPlugin,
+		volumeManager:          volumeManager,
 		livenessManager:        livenessManager,
 		runtimeHelper:          runtimeHelper,
 		execHandler:            execHandler,
@@ -689,9 +695,12 @@ func (dm *DockerManager) runContainer(
 
 	glog.V(3).Infof("Container %v/%v/%v: setting entrypoint \"%v\" and command \"%v\"", pod.Namespace, pod.Name, container.Name, dockerOpts.Config.Entrypoint, dockerOpts.Config.Cmd)
 
+	// todo: query volume manager for supplemental GIDs
+	supplementalGids := dm.volumeManager.GetExtraSupplementalGroupsForPod(pod)
+
 	securityContextProvider := securitycontext.NewSimpleSecurityContextProvider()
 	securityContextProvider.ModifyContainerConfig(pod, container, dockerOpts.Config)
-	securityContextProvider.ModifyHostConfig(pod, container, dockerOpts.HostConfig)
+	securityContextProvider.ModifyHostConfig(pod, container, dockerOpts.HostConfig, supplementalGids)
 	createResp, err := dm.client.CreateContainer(dockerOpts)
 	if err != nil {
 		dm.recorder.Eventf(ref, api.EventTypeWarning, kubecontainer.FailedToCreateContainer, "Failed to create docker container with error: %v", err)

pkg/kubelet/dockertools/docker_manager_test.go

@@ -47,6 +47,7 @@ import (
 	nettest "k8s.io/kubernetes/pkg/kubelet/network/testing"
 	proberesults "k8s.io/kubernetes/pkg/kubelet/prober/results"
 	"k8s.io/kubernetes/pkg/kubelet/types"
+	"k8s.io/kubernetes/pkg/kubelet/volumemanager"
 	"k8s.io/kubernetes/pkg/runtime"
 	kubetypes "k8s.io/kubernetes/pkg/types"
 	"k8s.io/kubernetes/pkg/util"
@@ -109,7 +110,13 @@ func createTestDockerManager(fakeHTTPClient *fakeHTTP, fakeDocker *FakeDockerCli
 	}
 	fakeRecorder := &record.FakeRecorder{}
 	containerRefManager := kubecontainer.NewRefManager()
-	networkPlugin, _ := network.InitNetworkPlugin([]network.NetworkPlugin{}, "", nettest.NewFakeHost(nil), componentconfig.HairpinNone, "10.0.0.0/8")
+	networkPlugin, _ := network.InitNetworkPlugin(
+		[]network.NetworkPlugin{},
+		"",
+		nettest.NewFakeHost(nil),
+		componentconfig.HairpinNone,
+		"10.0.0.0/8")
+
 	dockerManager := NewFakeDockerManager(
 		fakeDocker,
 		fakeRecorder,
@@ -120,6 +127,7 @@ func createTestDockerManager(fakeHTTPClient *fakeHTTP, fakeDocker *FakeDockerCli
 		0, 0, "",
 		&containertest.FakeOS{},
 		networkPlugin,
+		volumemanager.NewFakeVolumeManager(),
 		&fakeRuntimeHelper{},
 		fakeHTTPClient,
 		flowcontrol.NewBackOff(time.Second, 300*time.Second))

pkg/kubelet/dockertools/docker_test.go

@@ -653,7 +653,7 @@ func TestFindContainersByPod(t *testing.T) {
 	fakeClient := NewFakeDockerClient()
 	np, _ := network.InitNetworkPlugin([]network.NetworkPlugin{}, "", nettest.NewFakeHost(nil), componentconfig.HairpinNone, "10.0.0.0/8")
 	// image back-off is set to nil, this test should not pull images
-	containerManager := NewFakeDockerManager(fakeClient, &record.FakeRecorder{}, nil, nil, &cadvisorapi.MachineInfo{}, options.GetDefaultPodInfraContainerImage(), 0, 0, "", &containertest.FakeOS{}, np, nil, nil, nil)
+	containerManager := NewFakeDockerManager(fakeClient, &record.FakeRecorder{}, nil, nil, &cadvisorapi.MachineInfo{}, options.GetDefaultPodInfraContainerImage(), 0, 0, "", &containertest.FakeOS{}, np, nil, nil, nil, nil)
 	for i, test := range tests {
 		fakeClient.RunningContainerList = test.runningContainerList
 		fakeClient.ExitedContainerList = test.exitedContainerList

pkg/kubelet/dockertools/fake_manager.go

@@ -25,6 +25,7 @@ import (
 	proberesults "k8s.io/kubernetes/pkg/kubelet/prober/results"
 	kubetypes "k8s.io/kubernetes/pkg/kubelet/types"
 	"k8s.io/kubernetes/pkg/kubelet/util/cache"
+	volumemanager "k8s.io/kubernetes/pkg/kubelet/volumemanager"
 	"k8s.io/kubernetes/pkg/types"
 	"k8s.io/kubernetes/pkg/util/flowcontrol"
 	"k8s.io/kubernetes/pkg/util/oom"
@@ -43,6 +44,7 @@ func NewFakeDockerManager(
 	containerLogsDir string,
 	osInterface kubecontainer.OSInterface,
 	networkPlugin network.NetworkPlugin,
+	volumeManager volumemanager.VolumeManager,
 	runtimeHelper kubecontainer.RuntimeHelper,
 	httpClient kubetypes.HttpGetter, imageBackOff *flowcontrol.Backoff) *DockerManager {
 
@@ -50,7 +52,7 @@ func NewFakeDockerManager(
 	fakeProcFs := procfs.NewFakeProcFS()
 	fakePodGetter := &fakePodGetter{}
 	dm := NewDockerManager(client, recorder, livenessManager, containerRefManager, fakePodGetter, machineInfo, podInfraContainerImage, qps,
-		burst, containerLogsDir, osInterface, networkPlugin, runtimeHelper, httpClient, &NativeExecHandler{},
+		burst, containerLogsDir, osInterface, networkPlugin, volumeManager, runtimeHelper, httpClient, &NativeExecHandler{},
 		fakeOOMAdjuster, fakeProcFs, false, imageBackOff, false, false, true, "/var/lib/kubelet/seccomp")
 	dm.dockerPuller = &FakeDockerPuller{}
 

pkg/kubelet/kubelet.go

@@ -402,6 +402,20 @@ func NewMainKubelet(
 	klet.podCache = kubecontainer.NewCache()
 	klet.podManager = kubepod.NewBasicPodManager(kubepod.NewBasicMirrorClient(klet.kubeClient))
 
+	klet.volumePluginMgr, err =
+		NewInitializedVolumePluginMgr(klet, volumePlugins)
+	if err != nil {
+		return nil, err
+	}
+
+	klet.volumeManager, err = volumemanager.NewVolumeManager(
+		enableControllerAttachDetach,
+		hostname,
+		klet.podManager,
+		klet.kubeClient,
+		klet.volumePluginMgr,
+		klet.containerRuntime)
+
 	// Initialize the runtime.
 	switch containerRuntime {
 	case "docker":
@@ -419,6 +433,7 @@ func NewMainKubelet(
 			containerLogsDir,
 			osInterface,
 			klet.networkPlugin,
+			klet.volumeManager,
 			klet,
 			klet.httpClient,
 			dockerExecHandler,
@@ -495,20 +510,6 @@ func NewMainKubelet(
 		containerRefManager,
 		recorder)
 
-	klet.volumePluginMgr, err =
-		NewInitializedVolumePluginMgr(klet, volumePlugins)
-	if err != nil {
-		return nil, err
-	}
-
-	klet.volumeManager, err = volumemanager.NewVolumeManager(
-		enableControllerAttachDetach,
-		hostname,
-		klet.podManager,
-		klet.kubeClient,
-		klet.volumePluginMgr,
-		klet.containerRuntime)
-
 	runtimeCache, err := kubecontainer.NewRuntimeCache(klet.containerRuntime)
 	if err != nil {
 		return nil, err

pkg/kubelet/volumemanager/fake_volume_manager.go

+/*
+Copyright 2016 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package volumemanager
+
+import (
+	"k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/kubelet/container"
+	"k8s.io/kubernetes/pkg/volume/util/types"
+)
+
+func NewFakeVolumeManager() *FakeVolumeManager {
+	return &FakeVolumeManager{}
+}
+
+type FakeVolumeManager struct{}
+
+func (f *FakeVolumeManager) Run(stopCh <-chan struct{}) {
+}
+
+func (f *FakeVolumeManager) WaitForAttachAndMount(pod *api.Pod) error {
+	return nil
+}
+
+func (f *FakeVolumeManager) GetMountedVolumesForPod(podName types.UniquePodName) container.VolumeMap {
+	return container.VolumeMap{}
+}
+
+func (f *FakeVolumeManager) GetExtraSupplementalGroupsForPod(pod *api.Pod) []int64 {
+	return nil
+}
+
+func (f *FakeVolumeManager) GetVolumesInUse() []api.UniqueVolumeName {
+	return nil
+}
+
+func (f *FakeVolumeManager) MarkVolumesAsReportedInUse(volumesReportedAsInUse []api.UniqueVolumeName) {
+}
+
+func (f *FakeVolumeManager) VolumeIsAttached(volumeName api.UniqueVolumeName) bool {
+	return false
+}

pkg/kubelet/volumemanager/volume_manager.go

@@ -98,16 +98,10 @@ type VolumeManager interface {
 	// volumes.
 	GetMountedVolumesForPod(podName types.UniquePodName) container.VolumeMap
 
-	// GetVolumesForPodAndApplySupplementalGroups, like GetVolumesForPod returns
-	// a VolumeMap containing the volumes referenced by the specified pod that
-	// are successfully attached and mounted. The key in the map is the
-	// OuterVolumeSpecName (i.e. pod.Spec.Volumes[x].Name).
-	// It returns an empty VolumeMap if pod has no volumes.
-	// In addition for every volume that specifies a VolumeGidValue, it appends
-	// the SecurityContext.SupplementalGroups for the specified pod.
-	// XXX: https://github.com/kubernetes/kubernetes/issues/27197 mutating the
-	// pod object is bad, and should be avoided.
-	GetVolumesForPodAndAppendSupplementalGroups(pod *api.Pod) container.VolumeMap
+	// GetExtraSupplementalGroupsForPod returns a list of the extra
+	// supplemental groups for the Pod. These extra supplemental groups come
+	// from annotations on persistent volumes that the pod depends on.
+	GetExtraSupplementalGroupsForPod(pod *api.Pod) []int64
 
 	// Returns a list of all volumes that implement the volume.Attacher
 	// interface and are currently in use according to the actual and desired
@@ -224,12 +218,34 @@ func (vm *volumeManager) Run(stopCh <-chan struct{}) {
 
 func (vm *volumeManager) GetMountedVolumesForPod(
 	podName types.UniquePodName) container.VolumeMap {
-	return vm.getVolumesForPodHelper(podName, nil /* pod */)
+	podVolumes := make(container.VolumeMap)
+	for _, mountedVolume := range vm.actualStateOfWorld.GetMountedVolumesForPod(podName) {
+		podVolumes[mountedVolume.OuterVolumeSpecName] = container.VolumeInfo{Mounter: mountedVolume.Mounter}
+	}
+	return podVolumes
 }
 
-func (vm *volumeManager) GetVolumesForPodAndAppendSupplementalGroups(
-	pod *api.Pod) container.VolumeMap {
-	return vm.getVolumesForPodHelper("" /* podName */, pod)
+func (vm *volumeManager) GetExtraSupplementalGroupsForPod(pod *api.Pod) []int64 {
+	podName := volumehelper.GetUniquePodName(pod)
+	supplementalGroups := sets.NewString()
+
+	for _, mountedVolume := range vm.actualStateOfWorld.GetMountedVolumesForPod(podName) {
+		if mountedVolume.VolumeGidValue != "" {
+			supplementalGroups.Insert(mountedVolume.VolumeGidValue)
+		}
+	}
+
+	result := make([]int64, 0, supplementalGroups.Len())
+	for _, group := range supplementalGroups.List() {
+		iGroup, extra := getExtraSupplementalGid(group, pod)
+		if !extra {
+			continue
+		}
+
+		result = append(result, int64(iGroup))
+	}
+
+	return result
 }
 
 func (vm *volumeManager) GetVolumesInUse() []api.UniqueVolumeName {
@@ -276,33 +292,6 @@ func (vm *volumeManager) MarkVolumesAsReportedInUse(
 	vm.desiredStateOfWorld.MarkVolumesReportedInUse(volumesReportedAsInUse)
 }
 
-// getVolumesForPodHelper is a helper method implements the common logic for
-// the GetVolumesForPod methods.
-// XXX: https://github.com/kubernetes/kubernetes/issues/27197 mutating the pod
-// object is bad, and should be avoided.
-func (vm *volumeManager) getVolumesForPodHelper(
-	podName types.UniquePodName, pod *api.Pod) container.VolumeMap {
-	if pod != nil {
-		podName = volumehelper.GetUniquePodName(pod)
-	}
-	podVolumes := make(container.VolumeMap)
-	for _, mountedVolume := range vm.actualStateOfWorld.GetMountedVolumesForPod(podName) {
-		podVolumes[mountedVolume.OuterVolumeSpecName] =
-			container.VolumeInfo{Mounter: mountedVolume.Mounter}
-		if pod != nil {
-			err := applyPersistentVolumeAnnotations(
-				mountedVolume.VolumeGidValue, pod)
-			if err != nil {
-				glog.Errorf("applyPersistentVolumeAnnotations failed for pod %q volume %q with: %v",
-					podName,
-					mountedVolume.VolumeName,
-					err)
-			}
-		}
-	}
-	return podVolumes
-}
-
 func (vm *volumeManager) WaitForAttachAndMount(pod *api.Pod) error {
 	expectedVolumes := getExpectedVolumes(pod)
 	if len(expectedVolumes) == 0 {
@@ -392,32 +381,26 @@ func getExpectedVolumes(pod *api.Pod) []string {
 	return expectedVolumes
 }
 
-// applyPersistentVolumeAnnotations appends a pod
-// SecurityContext.SupplementalGroups if a GID annotation is provided.
-// XXX: https://github.com/kubernetes/kubernetes/issues/27197 mutating the pod
-// object is bad, and should be avoided.
-func applyPersistentVolumeAnnotations(
-	volumeGidValue string, pod *api.Pod) error {
-	if volumeGidValue != "" {
-		gid, err := strconv.ParseInt(volumeGidValue, 10, 64)
-		if err != nil {
-			return fmt.Errorf(
-				"Invalid value for %s %v",
-				volumehelper.VolumeGidAnnotationKey,
-				err)
-		}
+// getExtraSupplementalGid returns the value of an extra supplemental GID as
+// defined by an annotation on a volume and a boolean indicating whether the
+// volume defined a GID.
+func getExtraSupplementalGid(volumeGidValue string, pod *api.Pod) (int64, bool) {
+	if volumeGidValue == "" {
+		return 0, false
+	}
 
-		if pod.Spec.SecurityContext == nil {
-			pod.Spec.SecurityContext = &api.PodSecurityContext{}
-		}
+	gid, err := strconv.ParseInt(volumeGidValue, 10, 64)
+	if err != nil {
+		return 0, false
+	}
+
+	if pod.Spec.SecurityContext != nil {
 		for _, existingGid := range pod.Spec.SecurityContext.SupplementalGroups {
 			if gid == existingGid {
-				return nil
+				return 0, false
 			}
 		}
-		pod.Spec.SecurityContext.SupplementalGroups =
-			append(pod.Spec.SecurityContext.SupplementalGroups, gid)
 	}
 
-	return nil
+	return gid, true
 }

pkg/kubelet/volumemanager/volume_manager_test.go

+/*
+Copyright 2016 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package volumemanager
+
+import (
+	"os"
+	"reflect"
+	"strconv"
+	"testing"
+	"time"
+
+	"k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"
+	"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/fake"
+	containertest "k8s.io/kubernetes/pkg/kubelet/container/testing"
+	"k8s.io/kubernetes/pkg/kubelet/pod"
+	kubepod "k8s.io/kubernetes/pkg/kubelet/pod"
+	podtest "k8s.io/kubernetes/pkg/kubelet/pod/testing"
+	utiltesting "k8s.io/kubernetes/pkg/util/testing"
+	"k8s.io/kubernetes/pkg/volume"
+	volumetest "k8s.io/kubernetes/pkg/volume/testing"
+	"k8s.io/kubernetes/pkg/volume/util/types"
+	"k8s.io/kubernetes/pkg/volume/util/volumehelper"
+)
+
+const (
+	testHostname = "test-hostname"
+)
+
+func TestGetMountedVolumesForPodAndGetVolumesInUse(t *testing.T) {
+	tmpDir, err := utiltesting.MkTmpdir("volumeManagerTest")
+	if err != nil {
+		t.Fatalf("can't make a temp dir: %v", err)
+	}
+	defer os.RemoveAll(tmpDir)
+	podManager := kubepod.NewBasicPodManager(podtest.NewFakeMirrorClient())
+
+	node, pod, pv, claim := createObjects()
+	kubeClient := fake.NewSimpleClientset(node, pod, pv, claim)
+
+	manager, err := newTestVolumeManager(tmpDir, podManager, kubeClient)
+	if err != nil {
+		t.Fatalf("Failed to initialize volume manager: %v", err)
+	}
+
+	stopCh := make(chan struct{})
+	go manager.Run(stopCh)
+	defer close(stopCh)
+
+	podManager.SetPods([]*api.Pod{pod})
+
+	// Fake node status update
+	go simulateVolumeInUseUpdate(
+		api.UniqueVolumeName(node.Status.VolumesAttached[0].Name),
+		stopCh,
+		manager)
+
+	err = manager.WaitForAttachAndMount(pod)
+	if err != nil {
+		t.Errorf("Expected success: %v", err)
+	}
+
+	expectedMounted := pod.Spec.Volumes[0].Name
+	actualMounted := manager.GetMountedVolumesForPod(types.UniquePodName(pod.ObjectMeta.UID))
+	if _, ok := actualMounted[expectedMounted]; !ok || (len(actualMounted) != 1) {
+		t.Errorf("Expected %v to be mounted to pod but got %v", expectedMounted, actualMounted)
+	}
+
+	expectedInUse := []api.UniqueVolumeName{api.UniqueVolumeName(node.Status.VolumesAttached[0].Name)}
+	actualInUse := manager.GetVolumesInUse()
+	if !reflect.DeepEqual(expectedInUse, actualInUse) {
+		t.Errorf("Expected %v to be in use but got %v", expectedInUse, actualInUse)
+	}
+}
+
+func TestGetExtraSupplementalGroupsForPod(t *testing.T) {
+	tmpDir, err := utiltesting.MkTmpdir("volumeManagerTest")
+	if err != nil {
+		t.Fatalf("can't make a temp dir: %v", err)
+	}
+	defer os.RemoveAll(tmpDir)
+	podManager := kubepod.NewBasicPodManager(podtest.NewFakeMirrorClient())
+
+	node, pod, _, claim := createObjects()
+
+	existingGid := pod.Spec.SecurityContext.SupplementalGroups[0]
+
+	cases := []struct {
+		gidAnnotation string
+		expected      []int64
+	}{
+		{
+			gidAnnotation: "666",
+			expected:      []int64{666},
+		},
+		{
+			gidAnnotation: strconv.FormatInt(existingGid, 10),
+			expected:      []int64{},
+		},
+		{
+			gidAnnotation: "a",
+			expected:      []int64{},
+		},
+		{
+			gidAnnotation: "",
+			expected:      []int64{},
+		},
+	}
+
+	for _, tc := range cases {
+		pv := &api.PersistentVolume{
+			ObjectMeta: api.ObjectMeta{
+				Name: "pvA",
+				Annotations: map[string]string{
+					volumehelper.VolumeGidAnnotationKey: tc.gidAnnotation,
+				},
+			},
+			Spec: api.PersistentVolumeSpec{
+				PersistentVolumeSource: api.PersistentVolumeSource{
+					GCEPersistentDisk: &api.GCEPersistentDiskVolumeSource{
+						PDName: "fake-device",
+					},
+				},
+				ClaimRef: &api.ObjectReference{
+					Name: claim.ObjectMeta.Name,
+				},
+			},
+		}
+		kubeClient := fake.NewSimpleClientset(node, pod, pv, claim)
+
+		manager, err := newTestVolumeManager(tmpDir, podManager, kubeClient)
+		if err != nil {
+			t.Fatalf("Failed to initialize volume manager: %v", err)
+		}
+
+		stopCh := make(chan struct{})
+		go manager.Run(stopCh)
+
+		podManager.SetPods([]*api.Pod{pod})
+
+		// Fake node status update
+		go simulateVolumeInUseUpdate(
+			api.UniqueVolumeName(node.Status.VolumesAttached[0].Name),
+			stopCh,
+			manager)
+
+		err = manager.WaitForAttachAndMount(pod)
+		if err != nil {
+			t.Errorf("Expected success: %v", err)
+		}
+
+		actual := manager.GetExtraSupplementalGroupsForPod(pod)
+		if !reflect.DeepEqual(tc.expected, actual) {
+			t.Errorf("Expected supplemental groups %v, got %v", tc.expected, actual)
+		}
+
+		close(stopCh)
+	}
+}
+
+func newTestVolumeManager(
+	tmpDir string,
+	podManager pod.Manager,
+	kubeClient internalclientset.Interface) (VolumeManager, error) {
+	plug := &volumetest.FakeVolumePlugin{PluginName: "fake", Host: nil}
+	plugMgr := &volume.VolumePluginMgr{}
+	plugMgr.InitPlugins([]volume.VolumePlugin{plug}, volumetest.NewFakeVolumeHost(tmpDir, kubeClient, nil, "" /* rootContext */))
+
+	vm, err := NewVolumeManager(
+		true,
+		testHostname,
+		podManager,
+		kubeClient,
+		plugMgr,
+		&containertest.FakeRuntime{})
+	return vm, err
+}
+
+// createObjects returns objects for making a fake clientset. The pv is
+// already attached to the node and bound to the claim used by the pod.
+func createObjects() (*api.Node, *api.Pod, *api.PersistentVolume, *api.PersistentVolumeClaim) {
+	node := &api.Node{
+		ObjectMeta: api.ObjectMeta{Name: testHostname},
+		Status: api.NodeStatus{
+			VolumesAttached: []api.AttachedVolume{
+				{
+					Name:       "fake/pvA",
+					DevicePath: "fake/path",
+				},
+			}},
+		Spec: api.NodeSpec{ExternalID: testHostname},
+	}
+	pod := &api.Pod{
+		ObjectMeta: api.ObjectMeta{
+			Name:      "abc",
+			Namespace: "nsA",
+			UID:       "1234",
+		},
+		Spec: api.PodSpec{
+			Volumes: []api.Volume{
+				{
+					Name: "vol1",
+					VolumeSource: api.VolumeSource{
+						PersistentVolumeClaim: &api.PersistentVolumeClaimVolumeSource{
+							ClaimName: "claimA",
+						},
+					},
+				},
+			},
+			SecurityContext: &api.PodSecurityContext{
+				SupplementalGroups: []int64{555},
+			},
+		},
+	}
+	pv := &api.PersistentVolume{
+		ObjectMeta: api.ObjectMeta{
+			Name: "pvA",
+		},
+		Spec: api.PersistentVolumeSpec{
+			PersistentVolumeSource: api.PersistentVolumeSource{
+				GCEPersistentDisk: &api.GCEPersistentDiskVolumeSource{
+					PDName: "fake-device",
+				},
+			},
+			ClaimRef: &api.ObjectReference{
+				Name: "claimA",
+			},
+		},
+	}
+	claim := &api.PersistentVolumeClaim{
+		ObjectMeta: api.ObjectMeta{
+			Name:      "claimA",
+			Namespace: "nsA",
+		},
+		Spec: api.PersistentVolumeClaimSpec{
+			VolumeName: "pvA",
+		},
+		Status: api.PersistentVolumeClaimStatus{
+			Phase: api.ClaimBound,
+		},
+	}
+	return node, pod, pv, claim
+}
+
+func simulateVolumeInUseUpdate(
+	volumeName api.UniqueVolumeName,
+	stopCh <-chan struct{},
+	volumeManager VolumeManager) {
+	ticker := time.NewTicker(100 * time.Millisecond)
+	defer ticker.Stop()
+	for {
+		select {
+		case <-ticker.C:
+			volumeManager.MarkVolumesAsReportedInUse(
+				[]api.UniqueVolumeName{volumeName})
+		case <-stopCh:
+			return
+		}
+	}
+}

pkg/securitycontext/fake.go

@@ -41,5 +41,5 @@ type FakeSecurityContextProvider struct{}
 
 func (p FakeSecurityContextProvider) ModifyContainerConfig(pod *api.Pod, container *api.Container, config *dockercontainer.Config) {
 }
-func (p FakeSecurityContextProvider) ModifyHostConfig(pod *api.Pod, container *api.Container, hostConfig *dockercontainer.HostConfig) {
+func (p FakeSecurityContextProvider) ModifyHostConfig(pod *api.Pod, container *api.Container, hostConfig *dockercontainer.HostConfig, supplementalGids []int64) {
 }

pkg/securitycontext/provider.go

@@ -47,12 +47,12 @@ func (p SimpleSecurityContextProvider) ModifyContainerConfig(pod *api.Pod, conta
 	}
 }
 
-// ModifyHostConfig is called before the Docker runContainer call.
-// The security context provider can make changes to the HostConfig, affecting
+// ModifyHostConfig is called before the Docker runContainer call. The
+// security context provider can make changes to the HostConfig, affecting
 // security options, whether the container is privileged, volume binds, etc.
-func (p SimpleSecurityContextProvider) ModifyHostConfig(pod *api.Pod, container *api.Container, hostConfig *dockercontainer.HostConfig) {
-	// Apply pod security context
-	if container.Name != leaky.PodInfraContainerName && pod.Spec.SecurityContext != nil {
+func (p SimpleSecurityContextProvider) ModifyHostConfig(pod *api.Pod, container *api.Container, hostConfig *dockercontainer.HostConfig, supplementalGids []int64) {
+	// Apply supplemental groups
+	if container.Name != leaky.PodInfraContainerName {
 		// TODO: We skip application of supplemental groups to the
 		// infra container to work around a runc issue which
 		// requires containers to have the '/etc/group'. For
@@ -60,14 +60,17 @@ func (p SimpleSecurityContextProvider) ModifyHostConfig(pod *api.Pod, container
 		// https://github.com/opencontainers/runc/pull/313
 		// This can be removed once the fix makes it into the
 		// required version of docker.
-		if pod.Spec.SecurityContext.SupplementalGroups != nil {
-			hostConfig.GroupAdd = make([]string, len(pod.Spec.SecurityContext.SupplementalGroups))
-			for i, group := range pod.Spec.SecurityContext.SupplementalGroups {
-				hostConfig.GroupAdd[i] = strconv.Itoa(int(group))
+		if pod.Spec.SecurityContext != nil {
+			for _, group := range pod.Spec.SecurityContext.SupplementalGroups {
+				hostConfig.GroupAdd = append(hostConfig.GroupAdd, strconv.Itoa(int(group)))
 			}
 		}
 
-		if pod.Spec.SecurityContext.FSGroup != nil {
+		for _, group := range supplementalGids {
+			hostConfig.GroupAdd = append(hostConfig.GroupAdd, strconv.Itoa(int(group)))
+		}
+
+		if pod.Spec.SecurityContext != nil && pod.Spec.SecurityContext.FSGroup != nil {
 			hostConfig.GroupAdd = append(hostConfig.GroupAdd, strconv.Itoa(int(*pod.Spec.SecurityContext.FSGroup)))
 		}
 	}

pkg/securitycontext/provider_test.go

@@ -166,7 +166,7 @@ func TestModifyHostConfig(t *testing.T) {
 		dummyContainer.SecurityContext = tc.sc
 		dockerCfg := &dockercontainer.HostConfig{}
 
-		provider.ModifyHostConfig(pod, dummyContainer, dockerCfg)
+		provider.ModifyHostConfig(pod, dummyContainer, dockerCfg, nil)
 
 		if e, a := tc.expected, dockerCfg; !reflect.DeepEqual(e, a) {
 			t.Errorf("%v: unexpected modification of host config\nExpected:\n\n%#v\n\nGot:\n\n%#v", tc.name, e, a)
@@ -181,25 +181,32 @@ func TestModifyHostConfigPodSecurityContext(t *testing.T) {
 	supplementalGroupHC.GroupAdd = []string{"2222"}
 	fsGroupHC := fullValidHostConfig()
 	fsGroupHC.GroupAdd = []string{"1234"}
+	extraSupplementalGroupHC := fullValidHostConfig()
+	extraSupplementalGroupHC.GroupAdd = []string{"1234"}
 	bothHC := fullValidHostConfig()
 	bothHC.GroupAdd = []string{"2222", "1234"}
 	fsGroup := int64(1234)
+	extraSupplementalGroup := []int64{1234}
 
 	testCases := map[string]struct {
 		securityContext *api.PodSecurityContext
 		expected        *dockercontainer.HostConfig
+		extra           []int64
 	}{
 		"nil": {
 			securityContext: nil,
 			expected:        fullValidHostConfig(),
+			extra:           nil,
 		},
 		"SupplementalGroup": {
 			securityContext: supplementalGroupsSC,
 			expected:        supplementalGroupHC,
+			extra:           nil,
 		},
 		"FSGroup": {
 			securityContext: &api.PodSecurityContext{FSGroup: &fsGroup},
 			expected:        fsGroupHC,
+			extra:           nil,
 		},
 		"FSGroup + SupplementalGroups": {
 			securityContext: &api.PodSecurityContext{
@@ -207,6 +214,17 @@ func TestModifyHostConfigPodSecurityContext(t *testing.T) {
 				FSGroup:            &fsGroup,
 			},
 			expected: bothHC,
+			extra:    nil,
+		},
+		"ExtraSupplementalGroup": {
+			securityContext: nil,
+			expected:        extraSupplementalGroupHC,
+			extra:           extraSupplementalGroup,
+		},
+		"ExtraSupplementalGroup + SupplementalGroups": {
+			securityContext: supplementalGroupsSC,
+			expected:        bothHC,
+			extra:           extraSupplementalGroup,
 		},
 	}
 
@@ -220,7 +238,7 @@ func TestModifyHostConfigPodSecurityContext(t *testing.T) {
 	for k, v := range testCases {
 		dummyPod.Spec.SecurityContext = v.securityContext
 		dockerCfg := &dockercontainer.HostConfig{}
-		provider.ModifyHostConfig(dummyPod, dummyContainer, dockerCfg)
+		provider.ModifyHostConfig(dummyPod, dummyContainer, dockerCfg, v.extra)
 		if !reflect.DeepEqual(v.expected, dockerCfg) {
 			t.Errorf("unexpected modification of host config for %s.  Expected: %#v Got: %#v", k, v.expected, dockerCfg)
 		}

pkg/securitycontext/types.go

@@ -33,7 +33,11 @@ type SecurityContextProvider interface {
 	// security options, whether the container is privileged, volume binds, etc.
 	// An error is returned if it's not possible to secure the container as requested
 	// with a security context.
-	ModifyHostConfig(pod *api.Pod, container *api.Container, hostConfig *dockercontainer.HostConfig)
+	//
+	// - pod: the pod to modify the docker hostconfig for
+	// - container: the container to modify the hostconfig for
+	// - supplementalGids: additional supplemental GIDs associated with the pod's volumes
+	ModifyHostConfig(pod *api.Pod, container *api.Container, hostConfig *dockercontainer.HostConfig, supplementalGids []int64)
 }
 
 const (

test/e2e_node/image.go

@@ -46,7 +46,7 @@ func dockerRuntime() kubecontainer.Runtime {
 		dockerClient,
 		nil, nil, nil, pm, nil,
 		"", 0, 0, "",
-		nil, nil, nil, nil, nil, nil, nil,
+		nil, nil, nil, nil, nil, nil, nil, nil,
 		false, nil, true, false, false, "",
 	)