Merge pull request #35022 from jsafrane/gluster-secrets2

Automatic merge from submit-queue

Remove PV annotations for Gluster provisioner.

Don't store Gluster SotrageClass parameters in annotations, it's insecure.
Instead, expect that there is the StorageClass available at the time
when it's needed by Gluster deleter.

See #34822 for detais

@kubernetes/sig-storage

pkg/volume/glusterfs/glusterfs.go

@@ -403,21 +403,16 @@ func (d *glusterfsVolumeDeleter) Delete() error {
 	volumeName := d.glusterfsMounter.path
 	volumeId := dstrings.TrimPrefix(volumeName, volprefix)
 
-	err = d.annotationsToParam(d.spec)
+	class, err := volutil.GetClassForVolume(d.plugin.host.GetKubeClient(), d.spec)
 	if err != nil {
 		return err
 	}
-	if len(d.secretName) > 0 {
-		d.secretValue, err = parseSecret(d.secretNamespace, d.secretName, d.plugin.host.GetKubeClient())
-		if err != nil {
-			glog.Errorf("glusterfs: failed to read secret: %v", err)
-			return err
-		}
-	} else if len(d.userKey) > 0 {
-		d.secretValue = d.userKey
-	} else {
-		d.secretValue = ""
+
+	cfg, err := parseClassParameters(class.Parameters, d.plugin.host.GetKubeClient())
+	if err != nil {
+		return err
 	}
+	d.provisioningConfig = *cfg
 
 	glog.V(4).Infof("glusterfs: deleting volume %q with configuration %+v", volumeId, d.provisioningConfig)
 
@@ -444,56 +439,11 @@ func (r *glusterfsVolumeProvisioner) Provision() (*api.PersistentVolume, error)
 	}
 	glog.V(4).Infof("glusterfs: Provison VolumeOptions %v", r.options)
 
-	authEnabled := true
-	for k, v := range r.options.Parameters {
-		switch dstrings.ToLower(k) {
-		case "endpoint":
-			r.endpoint = v
-		case "resturl":
-			r.url = v
-		case "restuser":
-			r.user = v
-		case "restuserkey":
-			r.userKey = v
-		case "secretname":
-			r.secretName = v
-		case "secretnamespace":
-			r.secretNamespace = v
-		case "restauthenabled":
-			authEnabled = dstrings.ToLower(v) == "true"
-		default:
-			return nil, fmt.Errorf("glusterfs: invalid option %q for volume plugin %s", k, r.plugin.GetPluginName())
-		}
-	}
-
-	if len(r.url) == 0 {
-		return nil, fmt.Errorf("StorageClass for provisioner %q must contain 'resturl' parameter", r.plugin.GetPluginName())
-	}
-	if len(r.endpoint) == 0 {
-		return nil, fmt.Errorf("StorageClass for provisioner %q must contain 'endpoint' parameter", r.plugin.GetPluginName())
-	}
-
-	if !authEnabled {
-		r.user = ""
-		r.secretName = ""
-		r.secretNamespace = ""
-		r.userKey = ""
-		r.secretValue = ""
-	}
-
-	if len(r.secretName) != 0 || len(r.secretNamespace) != 0 {
-		// secretName + Namespace has precedence over userKey
-		if len(r.secretName) != 0 && len(r.secretNamespace) != 0 {
-			r.secretValue, err = parseSecret(r.secretNamespace, r.secretName, r.plugin.host.GetKubeClient())
-			if err != nil {
-				return nil, err
-			}
-		} else {
-			return nil, fmt.Errorf("StorageClass for provisioner %q must have secretNamespace and secretName either both set or both empty", r.plugin.GetPluginName())
-		}
-	} else {
-		r.secretValue = r.userKey
+	cfg, err := parseClassParameters(r.options.Parameters, r.plugin.host.GetKubeClient())
+	if err != nil {
+		return nil, err
 	}
+	r.provisioningConfig = *cfg
 
 	glog.V(4).Infof("glusterfs: creating volume with configuration %+v", r.provisioningConfig)
 	glusterfs, sizeGB, err := r.CreateVolume()
@@ -511,7 +461,6 @@ func (r *glusterfsVolumeProvisioner) Provision() (*api.PersistentVolume, error)
 	pv.Spec.Capacity = api.ResourceList{
 		api.ResourceName(api.ResourceStorage): resource.MustParse(fmt.Sprintf("%dGi", sizeGB)),
 	}
-	r.paramToAnnotations(pv)
 	return pv, nil
 }
 
@@ -564,37 +513,60 @@ func parseSecret(namespace, secretName string, kubeClient clientset.Interface) (
 	return secret, nil
 }
 
-// paramToAnnotations stores parameters needed to delete the volume in the PV
-// annotations.
-func (p *glusterfsVolumeProvisioner) paramToAnnotations(pv *api.PersistentVolume) {
-	ann := map[string]string{
-		annGlusterURL:             p.url,
-		annGlusterUser:            p.user,
-		annGlusterSecretName:      p.secretName,
-		annGlusterSecretNamespace: p.secretNamespace,
-		annGlusterUserKey:         p.userKey,
-	}
-	volutil.AddVolumeAnnotations(pv, ann)
-}
+// parseClassParameters parses StorageClass.Parameters
+func parseClassParameters(params map[string]string, kubeClient clientset.Interface) (*provisioningConfig, error) {
+	var cfg provisioningConfig
+	var err error
 
-// annotationsToParam parses annotations stored by paramToAnnotations
-func (d *glusterfsVolumeDeleter) annotationsToParam(pv *api.PersistentVolume) error {
-	annKeys := []string{
-		annGlusterSecretName,
-		annGlusterSecretNamespace,
-		annGlusterURL,
-		annGlusterUser,
-		annGlusterUserKey,
-	}
-	params, err := volutil.ParseVolumeAnnotations(pv, annKeys)
-	if err != nil {
-		return err
+	authEnabled := true
+	for k, v := range params {
+		switch dstrings.ToLower(k) {
+		case "endpoint":
+			cfg.endpoint = v
+		case "resturl":
+			cfg.url = v
+		case "restuser":
+			cfg.user = v
+		case "restuserkey":
+			cfg.userKey = v
+		case "secretname":
+			cfg.secretName = v
+		case "secretnamespace":
+			cfg.secretNamespace = v
+		case "restauthenabled":
+			authEnabled = dstrings.ToLower(v) == "true"
+		default:
+			return nil, fmt.Errorf("glusterfs: invalid option %q for volume plugin %s", k, glusterfsPluginName)
+		}
 	}
 
-	d.url = params[annGlusterURL]
-	d.user = params[annGlusterUser]
-	d.userKey = params[annGlusterUserKey]
-	d.secretName = params[annGlusterSecretName]
-	d.secretNamespace = params[annGlusterSecretNamespace]
-	return nil
+	if len(cfg.url) == 0 {
+		return nil, fmt.Errorf("StorageClass for provisioner %s must contain 'resturl' parameter", glusterfsPluginName)
+	}
+	if len(cfg.endpoint) == 0 {
+		return nil, fmt.Errorf("StorageClass for provisioner %s must contain 'endpoint' parameter", glusterfsPluginName)
+	}
+
+	if !authEnabled {
+		cfg.user = ""
+		cfg.secretName = ""
+		cfg.secretNamespace = ""
+		cfg.userKey = ""
+		cfg.secretValue = ""
+	}
+
+	if len(cfg.secretName) != 0 || len(cfg.secretNamespace) != 0 {
+		// secretName + Namespace has precedence over userKey
+		if len(cfg.secretName) != 0 && len(cfg.secretNamespace) != 0 {
+			cfg.secretValue, err = parseSecret(cfg.secretNamespace, cfg.secretName, kubeClient)
+			if err != nil {
+				return nil, err
+			}
+		} else {
+			return nil, fmt.Errorf("StorageClass for provisioner %q must have secretNamespace and secretName either both set or both empty", glusterfsPluginName)
+		}
+	} else {
+		cfg.secretValue = cfg.userKey
+	}
+	return &cfg, nil
 }

pkg/volume/glusterfs/glusterfs_test.go

@@ -19,10 +19,13 @@ package glusterfs
 import (
 	"fmt"
 	"os"
+	"reflect"
 	"testing"
 
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/fake"
+	"k8s.io/kubernetes/pkg/client/testing/core"
+	"k8s.io/kubernetes/pkg/runtime"
 	"k8s.io/kubernetes/pkg/types"
 	"k8s.io/kubernetes/pkg/util/exec"
 	"k8s.io/kubernetes/pkg/util/mount"
@@ -237,62 +240,153 @@ func TestPersistentClaimReadOnlyFlag(t *testing.T) {
 	}
 }
 
-func TestAnnotations(t *testing.T) {
-	// Pass a provisioningConfigs through paramToAnnotations and back through
-	// annotationsToParam and check it did not change in the process.
-	tests := []provisioningConfig{
-		{
-		// Everything empty
-		},
-		{
-			// Everything with a value
-			url:             "http://localhost",
-			user:            "admin",
-			secretNamespace: "default",
-			secretName:      "gluster-secret",
-			userKey:         "mykey",
-		},
-		{
-			// No secret
-			url:             "http://localhost",
-			user:            "admin",
-			secretNamespace: "",
-			secretName:      "",
-			userKey:         "",
+func TestParseClassParameters(t *testing.T) {
+	secret := api.Secret{
+		Data: map[string][]byte{
+			"data": []byte("mypassword"),
 		},
 	}
 
-	for i, test := range tests {
-		provisioner := &glusterfsVolumeProvisioner{
-			provisioningConfig: test,
-		}
-		deleter := &glusterfsVolumeDeleter{}
-
-		pv := &api.PersistentVolume{
-			ObjectMeta: api.ObjectMeta{
-				Name: "pv",
+	tests := []struct {
+		name         string
+		parameters   map[string]string
+		secret       *api.Secret
+		expectError  bool
+		expectConfig *provisioningConfig
+	}{
+		{
+			"password",
+			map[string]string{
+				"endpoint":    "endpointname",
+				"resturl":     "https://localhost:8080",
+				"restuser":    "admin",
+				"restuserkey": "password",
 			},
-		}
+			nil,   // secret
+			false, // expect error
+			&provisioningConfig{
+				endpoint:    "endpointname",
+				url:         "https://localhost:8080",
+				user:        "admin",
+				userKey:     "password",
+				secretValue: "password",
+			},
+		},
+		{
+			"secret",
+			map[string]string{
+				"endpoint":        "endpointname",
+				"resturl":         "https://localhost:8080",
+				"restuser":        "admin",
+				"secretname":      "mysecret",
+				"secretnamespace": "default",
+			},
+			&secret,
+			false, // expect error
+			&provisioningConfig{
+				endpoint:        "endpointname",
+				url:             "https://localhost:8080",
+				user:            "admin",
+				secretName:      "mysecret",
+				secretNamespace: "default",
+				secretValue:     "mypassword",
+			},
+		},
+		{
+			"no authentication",
+			map[string]string{
+				"endpoint":        "endpointname",
+				"resturl":         "https://localhost:8080",
+				"restauthenabled": "false",
+			},
+			&secret,
+			false, // expect error
+			&provisioningConfig{
+				endpoint: "endpointname",
+				url:      "https://localhost:8080",
+			},
+		},
+		{
+			"missing secret",
+			map[string]string{
+				"endpoint":        "endpointname",
+				"resturl":         "https://localhost:8080",
+				"secretname":      "mysecret",
+				"secretnamespace": "default",
+			},
+			nil,  // secret
+			true, // expect error
+			nil,
+		},
+		{
+			"secret with no namespace",
+			map[string]string{
+				"endpoint":   "endpointname",
+				"resturl":    "https://localhost:8080",
+				"secretname": "mysecret",
+			},
+			&secret,
+			true, // expect error
+			nil,
+		},
+		{
+			"missing url",
+			map[string]string{
+				"endpoint":    "endpointname",
+				"restuser":    "admin",
+				"restuserkey": "password",
+			},
+			nil,  // secret
+			true, // expect error
+			nil,
+		},
+		{
+			"missing endpoint",
+			map[string]string{
+				"resturl":     "https://localhost:8080",
+				"restuser":    "admin",
+				"restuserkey": "password",
+			},
+			nil,  // secret
+			true, // expect error
+			nil,
+		},
+		{
+			"unknown parameter",
+			map[string]string{
+				"unknown":     "yes",
+				"resturl":     "https://localhost:8080",
+				"restuser":    "admin",
+				"restuserkey": "password",
+			},
+			nil,  // secret
+			true, // expect error
+			nil,
+		},
+	}
 
-		provisioner.paramToAnnotations(pv)
-		err := deleter.annotationsToParam(pv)
-		if err != nil {
-			t.Errorf("test %d failed: %v", i, err)
+	for _, test := range tests {
+
+		client := &fake.Clientset{}
+		client.AddReactor("get", "secrets", func(action core.Action) (handled bool, ret runtime.Object, err error) {
+			if test.secret != nil {
+				return true, test.secret, nil
+			}
+			return true, nil, fmt.Errorf("Test %s did not set a secret", test.name)
+		})
+
+		cfg, err := parseClassParameters(test.parameters, client)
+
+		if err != nil && !test.expectError {
+			t.Errorf("Test %s got unexpected error %v", test.name, err)
 		}
-		if test.url != deleter.url {
-			t.Errorf("test %d failed: expected url %q, got %q", i, test.url, deleter.url)
+		if err == nil && test.expectError {
+			t.Errorf("test %s expected error and got none", test.name)
 		}
-		if test.user != deleter.user {
-			t.Errorf("test %d failed: expected user %q, got %q", i, test.user, deleter.user)
-		}
-		if test.userKey != deleter.userKey {
-			t.Errorf("test %d failed: expected userKey %q, got %q", i, test.userKey, deleter.userKey)
-		}
-		if test.secretNamespace != deleter.secretNamespace {
-			t.Errorf("test %d failed: expected secretNamespace %q, got %q", i, test.secretNamespace, deleter.secretNamespace)
-		}
-		if test.secretName != deleter.secretName {
-			t.Errorf("test %d failed: expected secretName %q, got %q", i, test.secretName, deleter.secretName)
+		if test.expectConfig != nil {
+			if !reflect.DeepEqual(cfg, test.expectConfig) {
+				t.Errorf("Test %s returned unexpected data, expected: %+v, got: %+v", test.name, test.expectConfig, cfg)
+			}
 		}
 	}
 }

pkg/volume/util/util.go

@@ -23,6 +23,7 @@ import (
 
 	"github.com/golang/glog"
 	"k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/apis/storage"
 	clientset "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"
 	"k8s.io/kubernetes/pkg/util/mount"
 )
@@ -157,3 +158,17 @@ func ParseVolumeAnnotations(pv *api.PersistentVolume, parseAnnotations []string)
 
 	return result, nil
 }
+
+func GetClassForVolume(kubeClient clientset.Interface, pv *api.PersistentVolume) (*storage.StorageClass, error) {
+	// TODO: replace with a real attribute after beta
+	className, found := pv.Annotations["volume.beta.kubernetes.io/storage-class"]
+	if !found {
+		return nil, fmt.Errorf("Volume has no class annotation")
+	}
+
+	class, err := kubeClient.Storage().StorageClasses().Get(className)
+	if err != nil {
+		return nil, err
+	}
+	return class, nil
+}