From 1a316015e3a17a26f1cb0f20995a23346ebb48ff Mon Sep 17 00:00:00 2001 From: andrewsykim Date: Fri, 18 Jan 2019 18:56:26 -0500 Subject: [PATCH] refactor persistent volume labeler admission controller to use cloudprovider.PVLabler --- .../storage/persistentvolume/label/BUILD | 10 +- .../persistentvolume/label/admission.go | 112 ++++++++++-------- .../persistentvolume/label/admission_test.go | 60 ++-------- 3 files changed, 76 insertions(+), 106 deletions(-) diff --git a/plugin/pkg/admission/storage/persistentvolume/label/BUILD b/plugin/pkg/admission/storage/persistentvolume/label/BUILD index 74fe386c13..bb54f90c6c 100644 --- a/plugin/pkg/admission/storage/persistentvolume/label/BUILD +++ b/plugin/pkg/admission/storage/persistentvolume/label/BUILD @@ -15,13 +15,12 @@ go_library( importpath = "k8s.io/kubernetes/plugin/pkg/admission/storage/persistentvolume/label", deps = [ "//pkg/apis/core:go_default_library", - "//pkg/cloudprovider/providers/aws:go_default_library", - "//pkg/cloudprovider/providers/azure:go_default_library", - "//pkg/cloudprovider/providers/gce:go_default_library", + "//pkg/apis/core/v1:go_default_library", "//pkg/kubeapiserver/admission:go_default_library", "//pkg/kubelet/apis:go_default_library", "//pkg/volume:go_default_library", "//pkg/volume/util:go_default_library", + "//staging/src/k8s.io/api/core/v1:go_default_library", "//staging/src/k8s.io/apiserver/pkg/admission:go_default_library", "//staging/src/k8s.io/cloud-provider:go_default_library", "//vendor/k8s.io/klog:go_default_library", @@ -34,13 +33,12 @@ go_test( embed = [":go_default_library"], deps = [ "//pkg/apis/core:go_default_library", - "//pkg/cloudprovider/providers/aws:go_default_library", "//pkg/kubelet/apis:go_default_library", "//pkg/volume/util:go_default_library", - "//staging/src/k8s.io/apimachinery/pkg/api/resource:go_default_library", + "//staging/src/k8s.io/api/core/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", - "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", "//staging/src/k8s.io/apiserver/pkg/admission:go_default_library", + "//staging/src/k8s.io/cloud-provider:go_default_library", ], ) diff --git a/plugin/pkg/admission/storage/persistentvolume/label/admission.go b/plugin/pkg/admission/storage/persistentvolume/label/admission.go index 4125dd52c3..9352df782a 100644 --- a/plugin/pkg/admission/storage/persistentvolume/label/admission.go +++ b/plugin/pkg/admission/storage/persistentvolume/label/admission.go @@ -18,17 +18,18 @@ package label import ( "bytes" + "context" + "errors" "fmt" "io" "sync" + v1 "k8s.io/api/core/v1" "k8s.io/apiserver/pkg/admission" cloudprovider "k8s.io/cloud-provider" "k8s.io/klog" api "k8s.io/kubernetes/pkg/apis/core" - "k8s.io/kubernetes/pkg/cloudprovider/providers/aws" - "k8s.io/kubernetes/pkg/cloudprovider/providers/azure" - "k8s.io/kubernetes/pkg/cloudprovider/providers/gce" + k8s_api_v1 "k8s.io/kubernetes/pkg/apis/core/v1" kubeapiserveradmission "k8s.io/kubernetes/pkg/kubeapiserver/admission" kubeletapis "k8s.io/kubernetes/pkg/kubelet/apis" vol "k8s.io/kubernetes/pkg/volume" @@ -53,11 +54,12 @@ var _ = admission.Interface(&persistentVolumeLabel{}) type persistentVolumeLabel struct { *admission.Handler - mutex sync.Mutex - ebsVolumes aws.Volumes - cloudConfig []byte - gceCloudProvider *gce.Cloud - azureProvider *azure.Cloud + mutex sync.Mutex + cloudConfig []byte + awsPVLabeler cloudprovider.PVLabeler + gcePVLabeler cloudprovider.PVLabeler + azurePVLabeler cloudprovider.PVLabeler + openStackPVLabeler cloudprovider.PVLabeler } var _ admission.MutationInterface = &persistentVolumeLabel{} @@ -186,47 +188,47 @@ func (l *persistentVolumeLabel) findAWSEBSLabels(volume *api.PersistentVolume) ( if volume.Spec.AWSElasticBlockStore.VolumeID == vol.ProvisionedVolumeName { return nil, nil } - ebsVolumes, err := l.getEBSVolumes() + pvlabler, err := l.getAWSPVLabeler() if err != nil { return nil, err } - if ebsVolumes == nil { + if pvlabler == nil { return nil, fmt.Errorf("unable to build AWS cloud provider for EBS") } - // TODO: GetVolumeLabels is actually a method on the Volumes interface - // If that gets standardized we can refactor to reduce code duplication - spec := aws.KubernetesVolumeID(volume.Spec.AWSElasticBlockStore.VolumeID) - labels, err := ebsVolumes.GetVolumeLabels(spec) + pv := &v1.PersistentVolume{} + err = k8s_api_v1.Convert_core_PersistentVolume_To_v1_PersistentVolume(volume, pv, nil) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to convert PersistentVolume to core/v1: %q", err) } - return labels, nil + return pvlabler.GetLabelsForVolume(context.TODO(), pv) } -// getEBSVolumes returns the AWS Volumes interface for ebs -func (l *persistentVolumeLabel) getEBSVolumes() (aws.Volumes, error) { +// getAWSPVLabeler returns the AWS implementation of PVLabeler +func (l *persistentVolumeLabel) getAWSPVLabeler() (cloudprovider.PVLabeler, error) { l.mutex.Lock() defer l.mutex.Unlock() - if l.ebsVolumes == nil { + if l.awsPVLabeler == nil { var cloudConfigReader io.Reader if len(l.cloudConfig) > 0 { cloudConfigReader = bytes.NewReader(l.cloudConfig) } + cloudProvider, err := cloudprovider.GetCloudProvider("aws", cloudConfigReader) if err != nil || cloudProvider == nil { return nil, err } - awsCloudProvider, ok := cloudProvider.(*aws.Cloud) + + awsPVLabeler, ok := cloudProvider.(cloudprovider.PVLabeler) if !ok { - // GetCloudProvider has gone very wrong - return nil, fmt.Errorf("error retrieving AWS cloud provider") + return nil, errors.New("AWS cloud provider does not implement PV labeling") } - l.ebsVolumes = awsCloudProvider + + l.awsPVLabeler = awsPVLabeler } - return l.ebsVolumes, nil + return l.awsPVLabeler, nil } func (l *persistentVolumeLabel) findGCEPDLabels(volume *api.PersistentVolume) (map[string]string, error) { @@ -235,72 +237,73 @@ func (l *persistentVolumeLabel) findGCEPDLabels(volume *api.PersistentVolume) (m return nil, nil } - provider, err := l.getGCECloudProvider() + pvlabler, err := l.getGCEPVLabeler() if err != nil { return nil, err } - if provider == nil { + if pvlabler == nil { return nil, fmt.Errorf("unable to build GCE cloud provider for PD") } - // If the zone is already labeled, honor the hint - zone := volume.Labels[kubeletapis.LabelZoneFailureDomain] - - labels, err := provider.GetAutoLabelsForPD(volume.Spec.GCEPersistentDisk.PDName, zone) + pv := &v1.PersistentVolume{} + err = k8s_api_v1.Convert_core_PersistentVolume_To_v1_PersistentVolume(volume, pv, nil) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to convert PersistentVolume to core/v1: %q", err) } - - return labels, nil + return pvlabler.GetLabelsForVolume(context.TODO(), pv) } -// getGCECloudProvider returns the GCE cloud provider, for use for querying volume labels -func (l *persistentVolumeLabel) getGCECloudProvider() (*gce.Cloud, error) { +// getGCEPVLabeler returns the GCE implementation of PVLabeler +func (l *persistentVolumeLabel) getGCEPVLabeler() (cloudprovider.PVLabeler, error) { l.mutex.Lock() defer l.mutex.Unlock() - if l.gceCloudProvider == nil { + if l.gcePVLabeler == nil { var cloudConfigReader io.Reader if len(l.cloudConfig) > 0 { cloudConfigReader = bytes.NewReader(l.cloudConfig) } + cloudProvider, err := cloudprovider.GetCloudProvider("gce", cloudConfigReader) if err != nil || cloudProvider == nil { return nil, err } - gceCloudProvider, ok := cloudProvider.(*gce.Cloud) + + gcePVLabeler, ok := cloudProvider.(cloudprovider.PVLabeler) if !ok { - // GetCloudProvider has gone very wrong - return nil, fmt.Errorf("error retrieving GCE cloud provider") + return nil, errors.New("GCE cloud provider does not implement PV labeling") } - l.gceCloudProvider = gceCloudProvider + + l.gcePVLabeler = gcePVLabeler + } - return l.gceCloudProvider, nil + return l.gcePVLabeler, nil } -// getAzureCloudProvider returns the Azure cloud provider, for use for querying volume labels -func (l *persistentVolumeLabel) getAzureCloudProvider() (*azure.Cloud, error) { +// getAzurePVLabeler returns the Azure implementation of PVLabeler +func (l *persistentVolumeLabel) getAzurePVLabeler() (cloudprovider.PVLabeler, error) { l.mutex.Lock() defer l.mutex.Unlock() - if l.azureProvider == nil { + if l.azurePVLabeler == nil { var cloudConfigReader io.Reader if len(l.cloudConfig) > 0 { cloudConfigReader = bytes.NewReader(l.cloudConfig) } + cloudProvider, err := cloudprovider.GetCloudProvider("azure", cloudConfigReader) if err != nil || cloudProvider == nil { return nil, err } - azureProvider, ok := cloudProvider.(*azure.Cloud) + + azurePVLabeler, ok := cloudProvider.(cloudprovider.PVLabeler) if !ok { - // GetCloudProvider has gone very wrong - return nil, fmt.Errorf("error retrieving Azure cloud provider") + return nil, errors.New("Azure cloud provider does not implement PV labeling") } - l.azureProvider = azureProvider + l.azurePVLabeler = azurePVLabeler } - return l.azureProvider, nil + return l.azurePVLabeler, nil } func (l *persistentVolumeLabel) findAzureDiskLabels(volume *api.PersistentVolume) (map[string]string, error) { @@ -309,13 +312,18 @@ func (l *persistentVolumeLabel) findAzureDiskLabels(volume *api.PersistentVolume return nil, nil } - provider, err := l.getAzureCloudProvider() + pvlabler, err := l.getAzurePVLabeler() if err != nil { return nil, err } - if provider == nil { + if pvlabler == nil { return nil, fmt.Errorf("unable to build Azure cloud provider for AzureDisk") } - return provider.GetAzureDiskLabels(volume.Spec.AzureDisk.DataDiskURI) + pv := &v1.PersistentVolume{} + err = k8s_api_v1.Convert_core_PersistentVolume_To_v1_PersistentVolume(volume, pv, nil) + if err != nil { + return nil, fmt.Errorf("failed to convert PersistentVolume to core/v1: %q", err) + } + return pvlabler.GetLabelsForVolume(context.TODO(), pv) } diff --git a/plugin/pkg/admission/storage/persistentvolume/label/admission_test.go b/plugin/pkg/admission/storage/persistentvolume/label/admission_test.go index 99c1843f1b..541c19965d 100644 --- a/plugin/pkg/admission/storage/persistentvolume/label/admission_test.go +++ b/plugin/pkg/admission/storage/persistentvolume/label/admission_test.go @@ -17,18 +17,17 @@ limitations under the License. package label import ( - "testing" - + "context" "fmt" "reflect" "sort" + "testing" - "k8s.io/apimachinery/pkg/api/resource" + "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" "k8s.io/apiserver/pkg/admission" + cloudprovider "k8s.io/cloud-provider" api "k8s.io/kubernetes/pkg/apis/core" - "k8s.io/kubernetes/pkg/cloudprovider/providers/aws" kubeletapis "k8s.io/kubernetes/pkg/kubelet/apis" volumeutil "k8s.io/kubernetes/pkg/volume/util" ) @@ -38,47 +37,12 @@ type mockVolumes struct { volumeLabelsError error } -var _ aws.Volumes = &mockVolumes{} +var _ cloudprovider.PVLabeler = &mockVolumes{} -func (v *mockVolumes) AttachDisk(diskName aws.KubernetesVolumeID, nodeName types.NodeName) (string, error) { - return "", fmt.Errorf("not implemented") -} - -func (v *mockVolumes) DetachDisk(diskName aws.KubernetesVolumeID, nodeName types.NodeName) (string, error) { - return "", fmt.Errorf("not implemented") -} - -func (v *mockVolumes) CreateDisk(volumeOptions *aws.VolumeOptions) (volumeName aws.KubernetesVolumeID, err error) { - return "", fmt.Errorf("not implemented") -} - -func (v *mockVolumes) DeleteDisk(volumeName aws.KubernetesVolumeID) (bool, error) { - return false, fmt.Errorf("not implemented") -} - -func (v *mockVolumes) GetVolumeLabels(volumeName aws.KubernetesVolumeID) (map[string]string, error) { +func (v *mockVolumes) GetLabelsForVolume(ctx context.Context, pv *v1.PersistentVolume) (map[string]string, error) { return v.volumeLabels, v.volumeLabelsError } -func (v *mockVolumes) GetDiskPath(volumeName aws.KubernetesVolumeID) (string, error) { - return "", fmt.Errorf("not implemented") -} - -func (v *mockVolumes) DiskIsAttached(volumeName aws.KubernetesVolumeID, nodeName types.NodeName) (bool, error) { - return false, fmt.Errorf("not implemented") -} - -func (v *mockVolumes) DisksAreAttached(nodeDisks map[types.NodeName][]aws.KubernetesVolumeID) (map[types.NodeName]map[aws.KubernetesVolumeID]bool, error) { - return nil, fmt.Errorf("not implemented") -} - -func (v *mockVolumes) ResizeDisk( - diskName aws.KubernetesVolumeID, - oldSize resource.Quantity, - newSize resource.Quantity) (resource.Quantity, error) { - return oldSize, nil -} - func mockVolumeFailure(err error) *mockVolumes { return &mockVolumes{volumeLabelsError: err} } @@ -135,7 +99,7 @@ func TestAdmission(t *testing.T) { } // Errors from the cloudprovider block creation of the volume - pvHandler.ebsVolumes = mockVolumeFailure(fmt.Errorf("invalid volume")) + pvHandler.awsPVLabeler = mockVolumeFailure(fmt.Errorf("invalid volume")) err = handler.Admit(admission.NewAttributesRecord(&awsPV, nil, api.Kind("PersistentVolume").WithVersion("version"), awsPV.Namespace, awsPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Create, false, nil)) if err == nil { t.Errorf("Expected error when aws pv info fails") @@ -143,7 +107,7 @@ func TestAdmission(t *testing.T) { // Don't add labels if the cloudprovider doesn't return any labels := make(map[string]string) - pvHandler.ebsVolumes = mockVolumeLabels(labels) + pvHandler.awsPVLabeler = mockVolumeLabels(labels) err = handler.Admit(admission.NewAttributesRecord(&awsPV, nil, api.Kind("PersistentVolume").WithVersion("version"), awsPV.Namespace, awsPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Create, false, nil)) if err != nil { t.Errorf("Expected no error when creating aws pv") @@ -156,7 +120,7 @@ func TestAdmission(t *testing.T) { } // Don't panic if the cloudprovider returns nil, nil - pvHandler.ebsVolumes = mockVolumeFailure(nil) + pvHandler.awsPVLabeler = mockVolumeFailure(nil) err = handler.Admit(admission.NewAttributesRecord(&awsPV, nil, api.Kind("PersistentVolume").WithVersion("version"), awsPV.Namespace, awsPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Create, false, nil)) if err != nil { t.Errorf("Expected no error when cloud provider returns empty labels") @@ -168,7 +132,7 @@ func TestAdmission(t *testing.T) { labels["b"] = "2" zones, _ := volumeutil.ZonesToSet("1,2,3") labels[kubeletapis.LabelZoneFailureDomain] = volumeutil.ZonesSetToLabelValue(zones) - pvHandler.ebsVolumes = mockVolumeLabels(labels) + pvHandler.awsPVLabeler = mockVolumeLabels(labels) err = handler.Admit(admission.NewAttributesRecord(&awsPV, nil, api.Kind("PersistentVolume").WithVersion("version"), awsPV.Namespace, awsPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Create, false, nil)) if err != nil { t.Errorf("Expected no error when creating aws pv") @@ -223,7 +187,7 @@ func TestAdmission(t *testing.T) { labels["a"] = "1" labels["b"] = "2" labels["c"] = "3" - pvHandler.ebsVolumes = mockVolumeLabels(labels) + pvHandler.awsPVLabeler = mockVolumeLabels(labels) err = handler.Admit(admission.NewAttributesRecord(&awsPV, nil, api.Kind("PersistentVolume").WithVersion("version"), awsPV.Namespace, awsPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Create, false, nil)) if err != nil { t.Errorf("Expected no error when creating aws pv") @@ -244,7 +208,7 @@ func TestAdmission(t *testing.T) { labels["e"] = "1" labels["f"] = "2" labels["g"] = "3" - pvHandler.ebsVolumes = mockVolumeLabels(labels) + pvHandler.awsPVLabeler = mockVolumeLabels(labels) err = handler.Admit(admission.NewAttributesRecord(&awsPV, nil, api.Kind("PersistentVolume").WithVersion("version"), awsPV.Namespace, awsPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Create, false, nil)) if err != nil { t.Errorf("Expected no error when creating aws pv")