From 360f2eb9270329e9d6581aec05c53b906dce66e8 Mon Sep 17 00:00:00 2001 From: Daniel Smith Date: Thu, 14 Jul 2016 17:47:46 -0700 Subject: [PATCH] Revert "Remove pod mutation for PVs with supplemental GIDs" --- pkg/kubelet/dockertools/docker_manager.go | 11 +- .../dockertools/docker_manager_test.go | 10 +- pkg/kubelet/dockertools/docker_test.go | 2 +- pkg/kubelet/dockertools/fake_manager.go | 4 +- pkg/kubelet/kubelet.go | 29 +- .../volumemanager/fake_volume_manager.go | 55 ---- pkg/kubelet/volumemanager/volume_manager.go | 107 ++++--- .../volumemanager/volume_manager_test.go | 274 ------------------ pkg/securitycontext/fake.go | 2 +- pkg/securitycontext/provider.go | 23 +- pkg/securitycontext/provider_test.go | 22 +- pkg/securitycontext/types.go | 6 +- test/e2e_node/image.go | 2 +- 13 files changed, 95 insertions(+), 452 deletions(-) delete mode 100644 pkg/kubelet/volumemanager/fake_volume_manager.go delete mode 100644 pkg/kubelet/volumemanager/volume_manager_test.go diff --git a/pkg/kubelet/dockertools/docker_manager.go b/pkg/kubelet/dockertools/docker_manager.go index 9d962fca58b..4e6d181df92 100644 --- a/pkg/kubelet/dockertools/docker_manager.go +++ b/pkg/kubelet/dockertools/docker_manager.go @@ -54,7 +54,6 @@ 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" @@ -139,9 +138,6 @@ type DockerManager struct { // Network plugin. networkPlugin network.NetworkPlugin - // Kubelet Volume Manager. - volumeManager volumemanager.VolumeManager - // Health check results. livenessManager proberesults.Manager @@ -214,7 +210,6 @@ func NewDockerManager( containerLogsDir string, osInterface kubecontainer.OSInterface, networkPlugin network.NetworkPlugin, - volumeManager volumemanager.VolumeManager, runtimeHelper kubecontainer.RuntimeHelper, httpClient types.HttpGetter, execHandler ExecHandler, @@ -253,7 +248,6 @@ func NewDockerManager( dockerRoot: dockerRoot, containerLogsDir: containerLogsDir, networkPlugin: networkPlugin, - volumeManager: volumeManager, livenessManager: livenessManager, runtimeHelper: runtimeHelper, execHandler: execHandler, @@ -696,12 +690,9 @@ 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, supplementalGids) + securityContextProvider.ModifyHostConfig(pod, container, dockerOpts.HostConfig) 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) diff --git a/pkg/kubelet/dockertools/docker_manager_test.go b/pkg/kubelet/dockertools/docker_manager_test.go index 12a9c45fa2d..cd9c90b2c68 100644 --- a/pkg/kubelet/dockertools/docker_manager_test.go +++ b/pkg/kubelet/dockertools/docker_manager_test.go @@ -47,7 +47,6 @@ 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" @@ -110,13 +109,7 @@ 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, @@ -127,7 +120,6 @@ func createTestDockerManager(fakeHTTPClient *fakeHTTP, fakeDocker *FakeDockerCli 0, 0, "", &containertest.FakeOS{}, networkPlugin, - volumemanager.NewFakeVolumeManager(), &fakeRuntimeHelper{}, fakeHTTPClient, flowcontrol.NewBackOff(time.Second, 300*time.Second)) diff --git a/pkg/kubelet/dockertools/docker_test.go b/pkg/kubelet/dockertools/docker_test.go index 6bc2dda1867..1c063fe1aa0 100644 --- a/pkg/kubelet/dockertools/docker_test.go +++ b/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, nil) + containerManager := NewFakeDockerManager(fakeClient, &record.FakeRecorder{}, nil, nil, &cadvisorapi.MachineInfo{}, options.GetDefaultPodInfraContainerImage(), 0, 0, "", &containertest.FakeOS{}, np, nil, nil, nil) for i, test := range tests { fakeClient.RunningContainerList = test.runningContainerList fakeClient.ExitedContainerList = test.exitedContainerList diff --git a/pkg/kubelet/dockertools/fake_manager.go b/pkg/kubelet/dockertools/fake_manager.go index 10ec4a7ea5c..1bb5cf59b82 100644 --- a/pkg/kubelet/dockertools/fake_manager.go +++ b/pkg/kubelet/dockertools/fake_manager.go @@ -25,7 +25,6 @@ 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" @@ -44,7 +43,6 @@ func NewFakeDockerManager( containerLogsDir string, osInterface kubecontainer.OSInterface, networkPlugin network.NetworkPlugin, - volumeManager volumemanager.VolumeManager, runtimeHelper kubecontainer.RuntimeHelper, httpClient kubetypes.HttpGetter, imageBackOff *flowcontrol.Backoff) *DockerManager { @@ -52,7 +50,7 @@ func NewFakeDockerManager( fakeProcFs := procfs.NewFakeProcFS() fakePodGetter := &fakePodGetter{} dm := NewDockerManager(client, recorder, livenessManager, containerRefManager, fakePodGetter, machineInfo, podInfraContainerImage, qps, - burst, containerLogsDir, osInterface, networkPlugin, volumeManager, runtimeHelper, httpClient, &NativeExecHandler{}, + burst, containerLogsDir, osInterface, networkPlugin, runtimeHelper, httpClient, &NativeExecHandler{}, fakeOOMAdjuster, fakeProcFs, false, imageBackOff, false, false, true, "/var/lib/kubelet/seccomp") dm.dockerPuller = &FakeDockerPuller{} diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index 3fc8210df0d..b8ed3cc1890 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -403,20 +403,6 @@ 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": @@ -434,7 +420,6 @@ func NewMainKubelet( containerLogsDir, osInterface, klet.networkPlugin, - klet.volumeManager, klet, klet.httpClient, dockerExecHandler, @@ -511,6 +496,20 @@ 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 diff --git a/pkg/kubelet/volumemanager/fake_volume_manager.go b/pkg/kubelet/volumemanager/fake_volume_manager.go deleted file mode 100644 index 886d912e58b..00000000000 --- a/pkg/kubelet/volumemanager/fake_volume_manager.go +++ /dev/null @@ -1,55 +0,0 @@ -/* -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 -} diff --git a/pkg/kubelet/volumemanager/volume_manager.go b/pkg/kubelet/volumemanager/volume_manager.go index 0f68727c9f2..54906491c06 100644 --- a/pkg/kubelet/volumemanager/volume_manager.go +++ b/pkg/kubelet/volumemanager/volume_manager.go @@ -98,10 +98,16 @@ type VolumeManager interface { // volumes. GetMountedVolumesForPod(podName types.UniquePodName) 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 + // 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 // Returns a list of all volumes that implement the volume.Attacher // interface and are currently in use according to the actual and desired @@ -218,34 +224,12 @@ func (vm *volumeManager) Run(stopCh <-chan struct{}) { func (vm *volumeManager) GetMountedVolumesForPod( podName types.UniquePodName) container.VolumeMap { - podVolumes := make(container.VolumeMap) - for _, mountedVolume := range vm.actualStateOfWorld.GetMountedVolumesForPod(podName) { - podVolumes[mountedVolume.OuterVolumeSpecName] = container.VolumeInfo{Mounter: mountedVolume.Mounter} - } - return podVolumes + return vm.getVolumesForPodHelper(podName, nil /* 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) GetVolumesForPodAndAppendSupplementalGroups( + pod *api.Pod) container.VolumeMap { + return vm.getVolumesForPodHelper("" /* podName */, pod) } func (vm *volumeManager) GetVolumesInUse() []api.UniqueVolumeName { @@ -292,6 +276,33 @@ 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 { @@ -381,26 +392,32 @@ func getExpectedVolumes(pod *api.Pod) []string { return expectedVolumes } -// 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 - } +// 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) + } - gid, err := strconv.ParseInt(volumeGidValue, 10, 64) - if err != nil { - return 0, false - } - - if pod.Spec.SecurityContext != nil { + if pod.Spec.SecurityContext == nil { + pod.Spec.SecurityContext = &api.PodSecurityContext{} + } for _, existingGid := range pod.Spec.SecurityContext.SupplementalGroups { if gid == existingGid { - return 0, false + return nil } } + pod.Spec.SecurityContext.SupplementalGroups = + append(pod.Spec.SecurityContext.SupplementalGroups, gid) } - return gid, true + return nil } diff --git a/pkg/kubelet/volumemanager/volume_manager_test.go b/pkg/kubelet/volumemanager/volume_manager_test.go deleted file mode 100644 index 7bb3491926b..00000000000 --- a/pkg/kubelet/volumemanager/volume_manager_test.go +++ /dev/null @@ -1,274 +0,0 @@ -/* -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 - } - } -} diff --git a/pkg/securitycontext/fake.go b/pkg/securitycontext/fake.go index ef86de3c868..829679a36e9 100644 --- a/pkg/securitycontext/fake.go +++ b/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, supplementalGids []int64) { +func (p FakeSecurityContextProvider) ModifyHostConfig(pod *api.Pod, container *api.Container, hostConfig *dockercontainer.HostConfig) { } diff --git a/pkg/securitycontext/provider.go b/pkg/securitycontext/provider.go index baf2a28075d..2663ef77e21 100644 --- a/pkg/securitycontext/provider.go +++ b/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, supplementalGids []int64) { - // Apply supplemental groups - if container.Name != leaky.PodInfraContainerName { +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 { // 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,17 +60,14 @@ 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 != nil { - for _, group := range pod.Spec.SecurityContext.SupplementalGroups { - hostConfig.GroupAdd = append(hostConfig.GroupAdd, strconv.Itoa(int(group))) + 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)) } } - for _, group := range supplementalGids { - hostConfig.GroupAdd = append(hostConfig.GroupAdd, strconv.Itoa(int(group))) - } - - if pod.Spec.SecurityContext != nil && pod.Spec.SecurityContext.FSGroup != nil { + if pod.Spec.SecurityContext.FSGroup != nil { hostConfig.GroupAdd = append(hostConfig.GroupAdd, strconv.Itoa(int(*pod.Spec.SecurityContext.FSGroup))) } } diff --git a/pkg/securitycontext/provider_test.go b/pkg/securitycontext/provider_test.go index f3c3ccc88d2..0f31d62ab31 100644 --- a/pkg/securitycontext/provider_test.go +++ b/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, nil) + provider.ModifyHostConfig(pod, dummyContainer, dockerCfg) 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,32 +181,25 @@ 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{ @@ -214,17 +207,6 @@ 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, }, } @@ -238,7 +220,7 @@ func TestModifyHostConfigPodSecurityContext(t *testing.T) { for k, v := range testCases { dummyPod.Spec.SecurityContext = v.securityContext dockerCfg := &dockercontainer.HostConfig{} - provider.ModifyHostConfig(dummyPod, dummyContainer, dockerCfg, v.extra) + provider.ModifyHostConfig(dummyPod, dummyContainer, dockerCfg) if !reflect.DeepEqual(v.expected, dockerCfg) { t.Errorf("unexpected modification of host config for %s. Expected: %#v Got: %#v", k, v.expected, dockerCfg) } diff --git a/pkg/securitycontext/types.go b/pkg/securitycontext/types.go index 2ab2d0390b5..9f27ca7d052 100644 --- a/pkg/securitycontext/types.go +++ b/pkg/securitycontext/types.go @@ -33,11 +33,7 @@ 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. - // - // - 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) + ModifyHostConfig(pod *api.Pod, container *api.Container, hostConfig *dockercontainer.HostConfig) } const ( diff --git a/test/e2e_node/image.go b/test/e2e_node/image.go index 3c9089fd50e..614b686b316 100644 --- a/test/e2e_node/image.go +++ b/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, "", )