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 +}