Merge pull request #25285 from ingvagabund/extend-secrets-volumes-with-path-control

Automatic merge from submit-queue

Extend secrets volumes with path control

As per [1] this PR extends secrets mapped into volume with:

* key-to-path mapping the same way as is for configmap. E.g.

```
{
 "apiVersion": "v1",
 "kind": "Pod",
  "metadata": {
    "name": "mypod",
    "namespace": "default"
  },
  "spec": {
    "containers": [{
      "name": "mypod",
      "image": "redis",
      "volumeMounts": [{
        "name": "foo",
        "mountPath": "/etc/foo",
        "readOnly": true
      }]
    }],
    "volumes": [{
      "name": "foo",
      "secret": {
        "secretName": "mysecret",
        "items": [{
          "key": "username",
          "path": "my-username"
        }]
      }
    }]
  }
}
```

Here the ``spec.volumes[0].secret.items`` added changing original target ``/etc/foo/username`` to ``/etc/foo/my-username``.

* secondly, refactoring ``pkg/volumes/secrets/secrets.go`` volume plugin to use ``AtomicWritter`` to project a secret into file.

[1] https://github.com/kubernetes/kubernetes/blob/master/docs/design/configmap.md#changes-to-secret

pkg/volume/secret/secret.go

@@ -18,8 +18,6 @@ package secret
 
 import (
 	"fmt"
-	"os"
-	"path"
 
 	"github.com/golang/glog"
 	"k8s.io/kubernetes/pkg/api"
@@ -74,9 +72,9 @@ func (plugin *secretPlugin) NewMounter(spec *volume.Spec, pod *api.Pod, opts vol
 			plugin.host.GetWriter(),
 			volume.NewCachedMetrics(volume.NewMetricsDu(getPathFromHost(plugin.host, pod.UID, spec.Name()))),
 		},
-		secretName: spec.Volume.Secret.SecretName,
-		pod:        *pod,
-		opts:       &opts,
+		source: *spec.Volume.Secret,
+		pod:    *pod,
+		opts:   &opts,
 	}, nil
 }
 
@@ -117,9 +115,9 @@ func getPathFromHost(host volume.VolumeHost, podUID types.UID, volName string) s
 type secretVolumeMounter struct {
 	*secretVolume
 
-	secretName string
-	pod        api.Pod
-	opts       *volume.VolumeOptions
+	source api.SecretVolumeSource
+	pod    api.Pod
+	opts   *volume.VolumeOptions
 }
 
 var _ volume.Mounter = &secretVolumeMounter{}
@@ -135,24 +133,7 @@ func (b *secretVolumeMounter) SetUp(fsGroup *int64) error {
 	return b.SetUpAt(b.GetPath(), fsGroup)
 }
 
-func (b *secretVolumeMounter) getMetaDir() string {
-	return path.Join(b.plugin.host.GetPodPluginDir(b.podUID, strings.EscapeQualifiedNameForDisk(secretPluginName)), b.volName)
-}
-
 func (b *secretVolumeMounter) SetUpAt(dir string, fsGroup *int64) error {
-	notMnt, err := b.mounter.IsLikelyNotMountPoint(dir)
-	// Getting an os.IsNotExist err from is a contingency; the directory
-	// may not exist yet, in which case, setup should run.
-	if err != nil && !os.IsNotExist(err) {
-		return err
-	}
-
-	// If the plugin readiness file is present for this volume and
-	// the setup dir is a mountpoint, this volume is already ready.
-	if volumeutil.IsReady(b.getMetaDir()) && !notMnt {
-		return nil
-	}
-
 	glog.V(3).Infof("Setting up volume %v for pod %v at %v", b.volName, b.pod.UID, dir)
 
 	// Wrap EmptyDir, let it do the setup.
@@ -169,34 +150,66 @@ func (b *secretVolumeMounter) SetUpAt(dir string, fsGroup *int64) error {
 		return fmt.Errorf("Cannot setup secret volume %v because kube client is not configured", b.volName)
 	}
 
-	secret, err := kubeClient.Core().Secrets(b.pod.Namespace).Get(b.secretName)
+	secret, err := kubeClient.Core().Secrets(b.pod.Namespace).Get(b.source.SecretName)
 	if err != nil {
-		glog.Errorf("Couldn't get secret %v/%v", b.pod.Namespace, b.secretName)
+		glog.Errorf("Couldn't get secret %v/%v", b.pod.Namespace, b.source.SecretName)
 		return err
-	} else {
-		totalBytes := totalSecretBytes(secret)
-		glog.V(3).Infof("Received secret %v/%v containing (%v) pieces of data, %v total bytes",
-			b.pod.Namespace,
-			b.secretName,
-			len(secret.Data),
-			totalBytes)
 	}
 
-	for name, data := range secret.Data {
-		hostFilePath := path.Join(dir, name)
-		glog.V(3).Infof("Writing secret data %v/%v/%v (%v bytes) to host file %v", b.pod.Namespace, b.secretName, name, len(data), hostFilePath)
-		err := b.writer.WriteFile(hostFilePath, data, 0444)
-		if err != nil {
-			glog.Errorf("Error writing secret data to host path: %v, %v", hostFilePath, err)
-			return err
+	totalBytes := totalSecretBytes(secret)
+	glog.V(3).Infof("Received secret %v/%v containing (%v) pieces of data, %v total bytes",
+		b.pod.Namespace,
+		b.source.SecretName,
+		len(secret.Data),
+		totalBytes)
+
+	payload, err := makePayload(b.source.Items, secret)
+	if 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 {
+		glog.Errorf("Error creating atomic writer: %v", err)
+		return err
+	}
+
+	err = writer.Write(payload)
+	if err != nil {
+		glog.Errorf("Error writing payload to dir: %v", err)
+		return err
+	}
+
+	err = volume.SetVolumeOwnership(b, fsGroup)
+	if err != nil {
+		glog.Errorf("Error applying volume ownership settings for group: %v", fsGroup)
+		return err
+	}
+
+	return nil
+}
+
+func makePayload(mappings []api.KeyToPath, secret *api.Secret) (map[string][]byte, error) {
+	payload := make(map[string][]byte, len(secret.Data))
+
+	if len(mappings) == 0 {
+		for name, data := range secret.Data {
+			payload[name] = []byte(data)
+		}
+	} else {
+		for _, ktp := range mappings {
+			content, ok := secret.Data[ktp.Key]
+			if !ok {
+				glog.Errorf("references non-existent secret key")
+				return nil, fmt.Errorf("references non-existent secret key")
+			}
+
+			payload[ktp.Path] = []byte(content)
 		}
 	}
 
-	volume.SetVolumeOwnership(b, fsGroup)
-
-	volumeutil.SetReady(b.getMetaDir())
-
-	return nil
+	return payload, nil
 }
 
 func totalSecretBytes(secret *api.Secret) int {

pkg/volume/secret/secret_test.go

@@ -21,6 +21,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path"
+	"reflect"
 	"runtime"
 	"strings"
 	"testing"
@@ -29,7 +30,6 @@ import (
 	clientset "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"
 	"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/fake"
 	"k8s.io/kubernetes/pkg/types"
-	"k8s.io/kubernetes/pkg/util/mount"
 	"k8s.io/kubernetes/pkg/volume"
 	"k8s.io/kubernetes/pkg/volume/empty_dir"
 	volumetest "k8s.io/kubernetes/pkg/volume/testing"
@@ -38,6 +38,149 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
+func TestMakePayload(t *testing.T) {
+	cases := []struct {
+		name     string
+		mappings []api.KeyToPath
+		secret   *api.Secret
+		payload  map[string][]byte
+		success  bool
+	}{
+		{
+			name: "no overrides",
+			secret: &api.Secret{
+				Data: map[string][]byte{
+					"foo": []byte("foo"),
+					"bar": []byte("bar"),
+				},
+			},
+			payload: map[string][]byte{
+				"foo": []byte("foo"),
+				"bar": []byte("bar"),
+			},
+			success: true,
+		},
+		{
+			name: "basic 1",
+			mappings: []api.KeyToPath{
+				{
+					Key:  "foo",
+					Path: "path/to/foo.txt",
+				},
+			},
+			secret: &api.Secret{
+				Data: map[string][]byte{
+					"foo": []byte("foo"),
+					"bar": []byte("bar"),
+				},
+			},
+			payload: map[string][]byte{
+				"path/to/foo.txt": []byte("foo"),
+			},
+			success: true,
+		},
+		{
+			name: "subdirs",
+			mappings: []api.KeyToPath{
+				{
+					Key:  "foo",
+					Path: "path/to/1/2/3/foo.txt",
+				},
+			},
+			secret: &api.Secret{
+				Data: map[string][]byte{
+					"foo": []byte("foo"),
+					"bar": []byte("bar"),
+				},
+			},
+			payload: map[string][]byte{
+				"path/to/1/2/3/foo.txt": []byte("foo"),
+			},
+			success: true,
+		},
+		{
+			name: "subdirs 2",
+			mappings: []api.KeyToPath{
+				{
+					Key:  "foo",
+					Path: "path/to/1/2/3/foo.txt",
+				},
+			},
+			secret: &api.Secret{
+				Data: map[string][]byte{
+					"foo": []byte("foo"),
+					"bar": []byte("bar"),
+				},
+			},
+			payload: map[string][]byte{
+				"path/to/1/2/3/foo.txt": []byte("foo"),
+			},
+			success: true,
+		},
+		{
+			name: "subdirs 3",
+			mappings: []api.KeyToPath{
+				{
+					Key:  "foo",
+					Path: "path/to/1/2/3/foo.txt",
+				},
+				{
+					Key:  "bar",
+					Path: "another/path/to/the/esteemed/bar.bin",
+				},
+			},
+			secret: &api.Secret{
+				Data: map[string][]byte{
+					"foo": []byte("foo"),
+					"bar": []byte("bar"),
+				},
+			},
+			payload: map[string][]byte{
+				"path/to/1/2/3/foo.txt":                []byte("foo"),
+				"another/path/to/the/esteemed/bar.bin": []byte("bar"),
+			},
+			success: true,
+		},
+		{
+			name: "non existent key",
+			mappings: []api.KeyToPath{
+				{
+					Key:  "zab",
+					Path: "path/to/foo.txt",
+				},
+			},
+			secret: &api.Secret{
+				Data: map[string][]byte{
+					"foo": []byte("foo"),
+					"bar": []byte("bar"),
+				},
+			},
+			success: false,
+		},
+	}
+
+	for _, tc := range cases {
+		actualPayload, err := makePayload(tc.mappings, tc.secret)
+		if err != nil && tc.success {
+			t.Errorf("%v: unexpected failure making payload: %v", tc.name, err)
+			continue
+		}
+
+		if err == nil && !tc.success {
+			t.Errorf("%v: unexpected success making payload", tc.name)
+			continue
+		}
+
+		if !tc.success {
+			continue
+		}
+
+		if e, a := tc.payload, actualPayload; !reflect.DeepEqual(e, a) {
+			t.Errorf("%v: expected and actual payload do not match", tc.name)
+		}
+	}
+}
+
 func newTestHost(t *testing.T, clientset clientset.Interface) (string, volume.VolumeHost) {
 	tempDir, err := ioutil.TempDir("/tmp", "secret_volume_test.")
 	if err != nil {
@@ -137,66 +280,6 @@ func TestPlugin(t *testing.T) {
 	}
 }
 
-// Test the case where the 'ready' file has been created and the pod volume dir
-// is a mountpoint.  Mount should not be called.
-func TestPluginIdempotent(t *testing.T) {
-	var (
-		testPodUID     = types.UID("test_pod_uid2")
-		testVolumeName = "test_volume_name"
-		testNamespace  = "test_secret_namespace"
-		testName       = "test_secret_name"
-
-		volumeSpec    = volumeSpec(testVolumeName, testName)
-		secret        = secret(testNamespace, testName)
-		client        = fake.NewSimpleClientset(&secret)
-		pluginMgr     = volume.VolumePluginMgr{}
-		rootDir, host = newTestHost(t, client)
-	)
-
-	pluginMgr.InitPlugins(ProbeVolumePlugins(), host)
-
-	plugin, err := pluginMgr.FindPluginByName(secretPluginName)
-	if err != nil {
-		t.Errorf("Can't find the plugin by name")
-	}
-
-	podVolumeDir := fmt.Sprintf("%v/pods/test_pod_uid2/volumes/kubernetes.io~secret/test_volume_name", rootDir)
-	podMetadataDir := fmt.Sprintf("%v/pods/test_pod_uid2/plugins/kubernetes.io~secret/test_volume_name", rootDir)
-	pod := &api.Pod{ObjectMeta: api.ObjectMeta{UID: testPodUID}}
-	physicalMounter := host.GetMounter().(*mount.FakeMounter)
-	physicalMounter.MountPoints = []mount.MountPoint{
-		{
-			Path: podVolumeDir,
-		},
-	}
-	util.SetReady(podMetadataDir)
-	mounter, err := plugin.NewMounter(volume.NewSpecFromVolume(volumeSpec), pod, volume.VolumeOptions{})
-	if err != nil {
-		t.Errorf("Failed to make a new Mounter: %v", err)
-	}
-	if mounter == nil {
-		t.Errorf("Got a nil Mounter")
-	}
-
-	volumePath := mounter.GetPath()
-	err = mounter.SetUp(nil)
-	if err != nil {
-		t.Errorf("Failed to setup volume: %v", err)
-	}
-
-	if len(physicalMounter.Log) != 0 {
-		t.Errorf("Unexpected calls made to physicalMounter: %v", physicalMounter.Log)
-	}
-
-	if _, err := os.Stat(volumePath); err != nil {
-		if !os.IsNotExist(err) {
-			t.Errorf("SetUp() failed unexpectedly: %v", err)
-		}
-	} else {
-		t.Errorf("volume path should not exist: %v", volumePath)
-	}
-}
-
 // Test the case where the plugin's ready file exists, but the volume dir is not a
 // mountpoint, which is the state the system will be in after reboot.  The dir
 // should be mounter and the secret data written to it.