Merge pull request #64855 from gnufied/fix-configmap-cleanup-on-fail

Automatic merge from submit-queue (batch tested with PRs 63905, 64855). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Fix setup of configmap/secret/projected/downwardapi

Only call setup after they are found; otherwise
we are left with orphan directories that are never
cleaned up.

Fixes https://github.com/kubernetes/kubernetes/issues/64788 and https://github.com/kubernetes/kubernetes/issues/64779

cc @aveshagarwal @saad-ali 

/sig storage

```release-note
None
```

pkg/volume/configmap/configmap.go

@@ -191,12 +191,6 @@ func (b *configMapVolumeMounter) SetUpAt(dir string, fsGroup *int64) error {
 	if err != nil {
 		return err
 	}
-	if err := wrapped.SetUpAt(dir, fsGroup); err != nil {
-		return err
-	}
-	if err := volumeutil.MakeNestedMountpoints(b.volName, dir, b.pod); err != nil {
-		return err
-	}
 
 	optional := b.source.Optional != nil && *b.source.Optional
 	configMap, err := b.getConfigMap(b.pod.Namespace, b.source.Name)
@@ -213,6 +207,13 @@ func (b *configMapVolumeMounter) SetUpAt(dir string, fsGroup *int64) error {
 		}
 	}
 
+	if err := wrapped.SetUpAt(dir, fsGroup); err != nil {
+		return err
+	}
+	if err := volumeutil.MakeNestedMountpoints(b.volName, dir, b.pod); err != nil {
+		return err
+	}
+
 	totalBytes := totalBytes(configMap)
 	glog.V(3).Infof("Received configMap %v/%v containing (%v) pieces of data, %v total bytes",
 		b.pod.Namespace,
@@ -243,7 +244,6 @@ func (b *configMapVolumeMounter) SetUpAt(dir string, fsGroup *int64) error {
 		glog.Errorf("Error applying volume ownership settings for group: %v", fsGroup)
 		return err
 	}
-
 	return nil
 }
 

pkg/volume/downwardapi/downwardapi.go

@@ -180,13 +180,6 @@ func (b *downwardAPIVolumeMounter) SetUpAt(dir string, fsGroup *int64) error {
 		glog.Errorf("Couldn't setup downwardAPI volume %v for pod %v/%v: %s", b.volName, b.pod.Namespace, b.pod.Name, err.Error())
 		return err
 	}
-	if err := wrapped.SetUpAt(dir, fsGroup); err != nil {
-		glog.Errorf("Unable to setup downwardAPI volume %v for pod %v/%v: %s", b.volName, b.pod.Namespace, b.pod.Name, err.Error())
-		return err
-	}
-	if err := volumeutil.MakeNestedMountpoints(b.volName, dir, *b.pod); err != nil {
-		return err
-	}
 
 	data, err := CollectData(b.source.Items, b.pod, b.plugin.host, b.source.DefaultMode)
 	if err != nil {
@@ -194,6 +187,15 @@ func (b *downwardAPIVolumeMounter) SetUpAt(dir string, fsGroup *int64) error {
 		return err
 	}
 
+	if err := wrapped.SetUpAt(dir, fsGroup); err != nil {
+		glog.Errorf("Unable to setup downwardAPI volume %v for pod %v/%v: %s", b.volName, b.pod.Namespace, b.pod.Name, err.Error())
+		return err
+	}
+
+	if err := volumeutil.MakeNestedMountpoints(b.volName, dir, *b.pod); err != nil {
+		return err
+	}
+
 	writerContext := fmt.Sprintf("pod %v/%v volume %v", b.pod.Namespace, b.pod.Name, b.volName)
 	writer, err := volumeutil.NewAtomicWriter(dir, writerContext)
 	if err != nil {

pkg/volume/projected/projected.go

@@ -194,18 +194,19 @@ func (s *projectedVolumeMounter) SetUpAt(dir string, fsGroup *int64) error {
 	if err != nil {
 		return err
 	}
-	if err := wrapped.SetUpAt(dir, fsGroup); err != nil {
-		return err
-	}
-	if err := volumeutil.MakeNestedMountpoints(s.volName, dir, *s.pod); err != nil {
-		return err
-	}
 
 	data, err := s.collectData()
 	if err != nil {
 		glog.Errorf("Error preparing data for projected volume %v for pod %v/%v: %s", s.volName, s.pod.Namespace, s.pod.Name, err.Error())
 		return err
 	}
+	if err := wrapped.SetUpAt(dir, fsGroup); err != nil {
+		return err
+	}
+
+	if err := volumeutil.MakeNestedMountpoints(s.volName, dir, *s.pod); err != nil {
+		return err
+	}
 
 	writerContext := fmt.Sprintf("pod %v/%v volume %v", s.pod.Namespace, s.pod.Name, s.volName)
 	writer, err := volumeutil.NewAtomicWriter(dir, writerContext)
@@ -225,7 +226,6 @@ func (s *projectedVolumeMounter) SetUpAt(dir string, fsGroup *int64) error {
 		glog.Errorf("Error applying volume ownership settings for group: %v", fsGroup)
 		return err
 	}
-
 	return nil
 }
 

pkg/volume/secret/secret.go

@@ -190,12 +190,6 @@ func (b *secretVolumeMounter) SetUpAt(dir string, fsGroup *int64) error {
 	if err != nil {
 		return err
 	}
-	if err := wrapped.SetUpAt(dir, fsGroup); err != nil {
-		return err
-	}
-	if err := volumeutil.MakeNestedMountpoints(b.volName, dir, b.pod); err != nil {
-		return err
-	}
 
 	optional := b.source.Optional != nil && *b.source.Optional
 	secret, err := b.getSecret(b.pod.Namespace, b.source.SecretName)
@@ -212,6 +206,13 @@ func (b *secretVolumeMounter) SetUpAt(dir string, fsGroup *int64) error {
 		}
 	}
 
+	if err := wrapped.SetUpAt(dir, fsGroup); err != nil {
+		return err
+	}
+	if err := volumeutil.MakeNestedMountpoints(b.volName, dir, b.pod); err != nil {
+		return err
+	}
+
 	totalBytes := totalSecretBytes(secret)
 	glog.V(3).Infof("Received secret %v/%v containing (%v) pieces of data, %v total bytes",
 		b.pod.Namespace,
@@ -242,7 +243,6 @@ func (b *secretVolumeMounter) SetUpAt(dir string, fsGroup *int64) error {
 		glog.Errorf("Error applying volume ownership settings for group: %v", fsGroup)
 		return err
 	}
-
 	return nil
 }
 

test/e2e/storage/BUILD

@@ -6,6 +6,7 @@ go_library(
         "csi_objects.go",
         "csi_volumes.go",
         "empty_dir_wrapper.go",
+        "ephemeral_volume.go",
         "flexvolume.go",
         "generic_persistent_volume-disruptive.go",
         "mounted_volume_resize.go",

test/e2e/storage/ephemeral_volume.go

@@ -0,0 +1,138 @@
+/*
+Copyright 2018 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 storage
+
+import (
+	"fmt"
+	"strings"
+	"time"
+
+	"k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/rand"
+	clientset "k8s.io/client-go/kubernetes"
+	"k8s.io/kubernetes/test/e2e/framework"
+	"k8s.io/kubernetes/test/e2e/storage/utils"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+)
+
+var _ = utils.SIGDescribe("Ephemeralstorage", func() {
+	var (
+		c clientset.Interface
+	)
+
+	f := framework.NewDefaultFramework("pv")
+
+	BeforeEach(func() {
+		c = f.ClientSet
+	})
+
+	Describe("When pod refers to non-existent ephemeral storage", func() {
+		for _, testSource := range invalidEphemeralSource("pod-ephm-test") {
+			It(fmt.Sprintf("should allow deletion of pod with invalid volume : %s", testSource.volumeType), func() {
+				pod := testEphemeralVolumePod(f, testSource.volumeType, testSource.source)
+				pod, err := c.CoreV1().Pods(f.Namespace.Name).Create(pod)
+				Expect(err).NotTo(HaveOccurred())
+
+				// Allow it to sleep for 30 seconds
+				time.Sleep(30 * time.Second)
+				framework.Logf("Deleting pod %q/%q", pod.Namespace, pod.Name)
+				framework.ExpectNoError(framework.DeletePodWithWait(f, c, pod))
+			})
+		}
+	})
+})
+
+type ephemeralTestInfo struct {
+	volumeType string
+	source     *v1.VolumeSource
+}
+
+func testEphemeralVolumePod(f *framework.Framework, volumeType string, source *v1.VolumeSource) *v1.Pod {
+	var (
+		suffix = strings.ToLower(fmt.Sprintf("%s-%s", volumeType, rand.String(4)))
+	)
+	return &v1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      fmt.Sprintf("pod-ephm-test-%s", suffix),
+			Namespace: f.Namespace.Name,
+		},
+		Spec: v1.PodSpec{
+			Containers: []v1.Container{
+				{
+					Name:  fmt.Sprintf("test-container-subpath-%s", suffix),
+					Image: mountImage,
+					VolumeMounts: []v1.VolumeMount{
+						{
+							Name:      volumeName,
+							MountPath: volumePath,
+						},
+					},
+				},
+			},
+			RestartPolicy: v1.RestartPolicyNever,
+			Volumes: []v1.Volume{
+				{
+					Name:         volumeName,
+					VolumeSource: *source,
+				},
+			},
+		},
+	}
+}
+
+func invalidEphemeralSource(suffix string) []ephemeralTestInfo {
+	testInfo := []ephemeralTestInfo{
+		{
+			volumeType: "secret",
+			source: &v1.VolumeSource{
+				Secret: &v1.SecretVolumeSource{
+					SecretName: fmt.Sprintf("secert-%s", suffix),
+				},
+			},
+		},
+		{
+			volumeType: "configmap",
+			source: &v1.VolumeSource{
+				ConfigMap: &v1.ConfigMapVolumeSource{
+					LocalObjectReference: v1.LocalObjectReference{
+						Name: fmt.Sprintf("configmap-%s", suffix),
+					},
+				},
+			},
+		},
+		{
+			volumeType: "projected",
+			source: &v1.VolumeSource{
+				Projected: &v1.ProjectedVolumeSource{
+					Sources: []v1.VolumeProjection{
+						{
+							Secret: &v1.SecretProjection{
+								LocalObjectReference: v1.LocalObjectReference{
+									Name: fmt.Sprintf("secret-%s", suffix),
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+	return testInfo
+}