From 2b2508ba15649884555c2fcb5f9fd2c741938971 Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Tue, 18 Oct 2016 09:54:35 +0200 Subject: [PATCH] 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. --- pkg/volume/glusterfs/glusterfs.go | 154 ++++++++------------ pkg/volume/glusterfs/glusterfs_test.go | 192 ++++++++++++++++++------- pkg/volume/util/util.go | 15 ++ 3 files changed, 221 insertions(+), 140 deletions(-) diff --git a/pkg/volume/glusterfs/glusterfs.go b/pkg/volume/glusterfs/glusterfs.go index 311c341679..6ec398ae1a 100644 --- a/pkg/volume/glusterfs/glusterfs.go +++ b/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 } diff --git a/pkg/volume/glusterfs/glusterfs_test.go b/pkg/volume/glusterfs/glusterfs_test.go index 5723334dcb..346b9a21ca 100644 --- a/pkg/volume/glusterfs/glusterfs_test.go +++ b/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) + } } } } diff --git a/pkg/volume/util/util.go b/pkg/volume/util/util.go index 7e01b5c581..95aa4aa6ed 100644 --- a/pkg/volume/util/util.go +++ b/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 +}