From 1a316015e3a17a26f1cb0f20995a23346ebb48ff Mon Sep 17 00:00:00 2001 From: andrewsykim Date: Fri, 18 Jan 2019 18:56:26 -0500 Subject: [PATCH 1/4] 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 74fe386c138..bb54f90c6ca 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 4125dd52c35..9352df782ac 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 99c1843f1b5..541c19965df 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") From 22fce22a7e255725d97ad84cf6b4437a72a567f7 Mon Sep 17 00:00:00 2001 From: andrewsykim Date: Mon, 21 Jan 2019 16:20:15 -0500 Subject: [PATCH 2/4] add support for Cinder volumes in PersistentVolumeLabel admission controller --- .../persistentvolume/label/admission.go | 63 ++++++++++++++++++- 1 file changed, 60 insertions(+), 3 deletions(-) diff --git a/plugin/pkg/admission/storage/persistentvolume/label/admission.go b/plugin/pkg/admission/storage/persistentvolume/label/admission.go index 9352df782ac..fb28017adba 100644 --- a/plugin/pkg/admission/storage/persistentvolume/label/admission.go +++ b/plugin/pkg/admission/storage/persistentvolume/label/admission.go @@ -70,9 +70,9 @@ var _ kubeapiserveradmission.WantsCloudConfig = &persistentVolumeLabel{} // // As a side effect, the cloud provider may block invalid or non-existent volumes. func newPersistentVolumeLabel() *persistentVolumeLabel { - // DEPRECATED: cloud-controller-manager will now start NewPersistentVolumeLabelController - // which does exactly what this admission controller used to do. So once GCE, AWS and AZURE can - // run externally, we can remove this admission controller. + // DEPRECATED: in a future release, we will use mutating admission webhooks to apply PV labels. + // Once the mutating admission webhook is used for AWS, Azure, GCE, and OpenStack, + // this admission controller will be removed. klog.Warning("PersistentVolumeLabel admission controller is deprecated. " + "Please remove this controller from your configuration files and scripts.") return &persistentVolumeLabel{ @@ -132,6 +132,13 @@ func (l *persistentVolumeLabel) Admit(a admission.Attributes) (err error) { } volumeLabels = labels } + if volume.Spec.Cinder != nil { + labels, err := l.findCinderDiskLabels(volume) + if err != nil { + return admission.NewForbidden(a, fmt.Errorf("error querying Cinder volume %s: %v", volume.Spec.Cinder.VolumeID, err)) + } + volumeLabels = labels + } requirements := make([]api.NodeSelectorRequirement, 0) if len(volumeLabels) != 0 { @@ -327,3 +334,53 @@ func (l *persistentVolumeLabel) findAzureDiskLabels(volume *api.PersistentVolume } return pvlabler.GetLabelsForVolume(context.TODO(), pv) } + +func (l *persistentVolumeLabel) getOpenStackPVLabeler() (cloudprovider.PVLabeler, error) { + l.mutex.Lock() + defer l.mutex.Unlock() + + if l.openStackPVLabeler == nil { + var cloudConfigReader io.Reader + if len(l.cloudConfig) > 0 { + cloudConfigReader = bytes.NewReader(l.cloudConfig) + } + + cloudProvider, err := cloudprovider.GetCloudProvider("openstack", cloudConfigReader) + if err != nil || cloudProvider == nil { + return nil, err + } + + openStackPVLabeler, ok := cloudProvider.(cloudprovider.PVLabeler) + if !ok { + return nil, errors.New("OpenStack cloud provider does not implement PV labeling") + } + + l.openStackPVLabeler = openStackPVLabeler + } + + return l.openStackPVLabeler, nil + +} + +func (l *persistentVolumeLabel) findCinderDiskLabels(volume *api.PersistentVolume) (map[string]string, error) { + // Ignore any volumes that are being provisioned + if volume.Spec.Cinder.VolumeID == vol.ProvisionedVolumeName { + return nil, nil + } + + pvlabler, err := l.getOpenStackPVLabeler() + if err != nil { + return nil, err + } + if pvlabler == nil { + return nil, fmt.Errorf("unable to build OpenStack cloud provider for Cinder disk") + } + + 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) + +} From 32b6225c72f1a9825e8a8f0868ce36b0f7b381da Mon Sep 17 00:00:00 2001 From: andrewsykim Date: Mon, 21 Jan 2019 19:08:57 -0500 Subject: [PATCH 3/4] refactor PVL unit tests to use test tables & add test cases for remaining cloud providers --- .../storage/persistentvolume/label/BUILD | 3 +- .../persistentvolume/label/admission.go | 3 +- .../persistentvolume/label/admission_test.go | 841 ++++++++++++++---- 3 files changed, 678 insertions(+), 169 deletions(-) diff --git a/plugin/pkg/admission/storage/persistentvolume/label/BUILD b/plugin/pkg/admission/storage/persistentvolume/label/BUILD index bb54f90c6ca..115603d0cf8 100644 --- a/plugin/pkg/admission/storage/persistentvolume/label/BUILD +++ b/plugin/pkg/admission/storage/persistentvolume/label/BUILD @@ -34,9 +34,10 @@ go_test( deps = [ "//pkg/apis/core:go_default_library", "//pkg/kubelet/apis:go_default_library", - "//pkg/volume/util:go_default_library", "//staging/src/k8s.io/api/core/v1:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/runtime/schema: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 fb28017adba..ddaa5189e05 100644 --- a/plugin/pkg/admission/storage/persistentvolume/label/admission.go +++ b/plugin/pkg/admission/storage/persistentvolume/label/admission.go @@ -158,7 +158,8 @@ func (l *persistentVolumeLabel) Admit(a admission.Attributes) (err error) { if err != nil { return admission.NewForbidden(a, fmt.Errorf("failed to convert label string for Zone: %s to a Set", v)) } - values = zones.UnsortedList() + // zone values here are sorted for better testability. + values = zones.List() } else { values = []string{v} } diff --git a/plugin/pkg/admission/storage/persistentvolume/label/admission_test.go b/plugin/pkg/admission/storage/persistentvolume/label/admission_test.go index 541c19965df..a35a845971a 100644 --- a/plugin/pkg/admission/storage/persistentvolume/label/admission_test.go +++ b/plugin/pkg/admission/storage/persistentvolume/label/admission_test.go @@ -18,18 +18,19 @@ package label import ( "context" - "fmt" + "errors" "reflect" "sort" "testing" "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apiserver/pkg/admission" cloudprovider "k8s.io/cloud-provider" api "k8s.io/kubernetes/pkg/apis/core" kubeletapis "k8s.io/kubernetes/pkg/kubelet/apis" - volumeutil "k8s.io/kubernetes/pkg/volume/util" ) type mockVolumes struct { @@ -51,181 +52,687 @@ func mockVolumeLabels(labels map[string]string) *mockVolumes { return &mockVolumes{volumeLabels: labels} } -func getNodeSelectorRequirementWithKey(key string, term api.NodeSelectorTerm) (*api.NodeSelectorRequirement, error) { - for _, r := range term.MatchExpressions { - if r.Key != key { - continue - } - return &r, nil - } - return nil, fmt.Errorf("key %s not found", key) -} - -// TestAdmission -func TestAdmission(t *testing.T) { - pvHandler := newPersistentVolumeLabel() - handler := admission.NewChainHandler(pvHandler) - ignoredPV := api.PersistentVolume{ - ObjectMeta: metav1.ObjectMeta{Name: "noncloud", Namespace: "myns"}, - Spec: api.PersistentVolumeSpec{ - PersistentVolumeSource: api.PersistentVolumeSource{ - HostPath: &api.HostPathVolumeSource{ - Path: "/", +func Test_PVLAdmission(t *testing.T) { + testcases := []struct { + name string + handler *persistentVolumeLabel + pvlabeler cloudprovider.PVLabeler + preAdmissionPV *api.PersistentVolume + postAdmissionPV *api.PersistentVolume + err error + }{ + { + name: "non-cloud PV ignored", + handler: newPersistentVolumeLabel(), + pvlabeler: mockVolumeLabels(map[string]string{ + "a": "1", + "b": "2", + kubeletapis.LabelZoneFailureDomain: "1__2__3", + }), + preAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{Name: "noncloud", Namespace: "myns"}, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + HostPath: &api.HostPathVolumeSource{ + Path: "/", + }, + }, }, }, + postAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{Name: "noncloud", Namespace: "myns"}, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + HostPath: &api.HostPathVolumeSource{ + Path: "/", + }, + }, + }, + }, + err: nil, }, - } - awsPV := api.PersistentVolume{ - ObjectMeta: metav1.ObjectMeta{Name: "noncloud", Namespace: "myns"}, - Spec: api.PersistentVolumeSpec{ - PersistentVolumeSource: api.PersistentVolumeSource{ - AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{ - VolumeID: "123", + { + name: "cloud provider error blocks creation of volume", + handler: newPersistentVolumeLabel(), + pvlabeler: mockVolumeFailure(errors.New("invalid volume")), + preAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{Name: "awsebs", Namespace: "myns"}, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{ + VolumeID: "123", + }, + }, }, }, + postAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{Name: "awsebs", Namespace: "myns"}, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{ + VolumeID: "123", + }, + }, + }, + }, + err: apierrors.NewForbidden(schema.ParseGroupResource("persistentvolumes"), "awsebs", errors.New("error querying AWS EBS volume 123: invalid volume")), + }, + { + name: "cloud provider returns no labels", + handler: newPersistentVolumeLabel(), + pvlabeler: mockVolumeLabels(map[string]string{}), + preAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{Name: "awsebs", Namespace: "myns"}, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{ + VolumeID: "123", + }, + }, + }, + }, + postAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{Name: "awsebs", Namespace: "myns"}, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{ + VolumeID: "123", + }, + }, + }, + }, + err: nil, + }, + { + name: "cloud provider returns nil, nil", + handler: newPersistentVolumeLabel(), + pvlabeler: mockVolumeFailure(nil), + preAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{Name: "awsebs", Namespace: "myns"}, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{ + VolumeID: "123", + }, + }, + }, + }, + postAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{Name: "awsebs", Namespace: "myns"}, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{ + VolumeID: "123", + }, + }, + }, + }, + err: nil, + }, + { + name: "AWS EBS PV labeled correctly", + handler: newPersistentVolumeLabel(), + pvlabeler: mockVolumeLabels(map[string]string{ + "a": "1", + "b": "2", + kubeletapis.LabelZoneFailureDomain: "1__2__3", + }), + preAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{Name: "awsebs", Namespace: "myns"}, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{ + VolumeID: "123", + }, + }, + }, + }, + postAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "awsebs", + Namespace: "myns", + Labels: map[string]string{ + "a": "1", + "b": "2", + kubeletapis.LabelZoneFailureDomain: "1__2__3", + }, + }, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{ + VolumeID: "123", + }, + }, + NodeAffinity: &api.VolumeNodeAffinity{ + Required: &api.NodeSelector{ + NodeSelectorTerms: []api.NodeSelectorTerm{ + { + MatchExpressions: []api.NodeSelectorRequirement{ + { + Key: "a", + Operator: api.NodeSelectorOpIn, + Values: []string{"1"}, + }, + { + Key: "b", + Operator: api.NodeSelectorOpIn, + Values: []string{"2"}, + }, + { + Key: kubeletapis.LabelZoneFailureDomain, + Operator: api.NodeSelectorOpIn, + Values: []string{"1", "2", "3"}, + }, + }, + }, + }, + }, + }, + }, + }, + err: nil, + }, + { + name: "GCE PD PV labeled correctly", + handler: newPersistentVolumeLabel(), + pvlabeler: mockVolumeLabels(map[string]string{ + "a": "1", + "b": "2", + kubeletapis.LabelZoneFailureDomain: "1__2__3", + }), + preAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{Name: "gcepd", Namespace: "myns"}, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + GCEPersistentDisk: &api.GCEPersistentDiskVolumeSource{ + PDName: "123", + }, + }, + }, + }, + postAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gcepd", + Namespace: "myns", + Labels: map[string]string{ + "a": "1", + "b": "2", + kubeletapis.LabelZoneFailureDomain: "1__2__3", + }, + }, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + GCEPersistentDisk: &api.GCEPersistentDiskVolumeSource{ + PDName: "123", + }, + }, + NodeAffinity: &api.VolumeNodeAffinity{ + Required: &api.NodeSelector{ + NodeSelectorTerms: []api.NodeSelectorTerm{ + { + MatchExpressions: []api.NodeSelectorRequirement{ + { + Key: "a", + Operator: api.NodeSelectorOpIn, + Values: []string{"1"}, + }, + { + Key: "b", + Operator: api.NodeSelectorOpIn, + Values: []string{"2"}, + }, + { + Key: kubeletapis.LabelZoneFailureDomain, + Operator: api.NodeSelectorOpIn, + Values: []string{"1", "2", "3"}, + }, + }, + }, + }, + }, + }, + }, + }, + err: nil, + }, + { + name: "Azure Disk PV labeled correctly", + handler: newPersistentVolumeLabel(), + pvlabeler: mockVolumeLabels(map[string]string{ + "a": "1", + "b": "2", + kubeletapis.LabelZoneFailureDomain: "1__2__3", + }), + preAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "azurepd", + Namespace: "myns", + }, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + AzureDisk: &api.AzureDiskVolumeSource{ + DiskName: "123", + }, + }, + }, + }, + postAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "azurepd", + Namespace: "myns", + Labels: map[string]string{ + "a": "1", + "b": "2", + kubeletapis.LabelZoneFailureDomain: "1__2__3", + }, + }, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + AzureDisk: &api.AzureDiskVolumeSource{ + DiskName: "123", + }, + }, + NodeAffinity: &api.VolumeNodeAffinity{ + Required: &api.NodeSelector{ + NodeSelectorTerms: []api.NodeSelectorTerm{ + { + MatchExpressions: []api.NodeSelectorRequirement{ + { + Key: "a", + Operator: api.NodeSelectorOpIn, + Values: []string{"1"}, + }, + { + Key: "b", + Operator: api.NodeSelectorOpIn, + Values: []string{"2"}, + }, + { + Key: kubeletapis.LabelZoneFailureDomain, + Operator: api.NodeSelectorOpIn, + Values: []string{"1", "2", "3"}, + }, + }, + }, + }, + }, + }, + }, + }, + err: nil, + }, + { + name: "Cinder Disk PV labeled correctly", + handler: newPersistentVolumeLabel(), + pvlabeler: mockVolumeLabels(map[string]string{ + "a": "1", + "b": "2", + kubeletapis.LabelZoneFailureDomain: "1__2__3", + }), + preAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "azurepd", + Namespace: "myns", + }, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + Cinder: &api.CinderPersistentVolumeSource{ + VolumeID: "123", + }, + }, + }, + }, + postAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "azurepd", + Namespace: "myns", + Labels: map[string]string{ + "a": "1", + "b": "2", + kubeletapis.LabelZoneFailureDomain: "1__2__3", + }, + }, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + Cinder: &api.CinderPersistentVolumeSource{ + VolumeID: "123", + }, + }, + NodeAffinity: &api.VolumeNodeAffinity{ + Required: &api.NodeSelector{ + NodeSelectorTerms: []api.NodeSelectorTerm{ + { + MatchExpressions: []api.NodeSelectorRequirement{ + { + Key: "a", + Operator: api.NodeSelectorOpIn, + Values: []string{"1"}, + }, + { + Key: "b", + Operator: api.NodeSelectorOpIn, + Values: []string{"2"}, + }, + { + Key: kubeletapis.LabelZoneFailureDomain, + Operator: api.NodeSelectorOpIn, + Values: []string{"1", "2", "3"}, + }, + }, + }, + }, + }, + }, + }, + }, + err: nil, + }, + { + name: "AWS EBS PV overrides user applied labels", + handler: newPersistentVolumeLabel(), + pvlabeler: mockVolumeLabels(map[string]string{ + "a": "1", + "b": "2", + kubeletapis.LabelZoneFailureDomain: "1__2__3", + }), + preAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "awsebs", + Namespace: "myns", + Labels: map[string]string{ + "a": "not1", + }, + }, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{ + VolumeID: "123", + }, + }, + }, + }, + postAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "awsebs", + Namespace: "myns", + Labels: map[string]string{ + "a": "1", + "b": "2", + kubeletapis.LabelZoneFailureDomain: "1__2__3", + }, + }, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{ + VolumeID: "123", + }, + }, + NodeAffinity: &api.VolumeNodeAffinity{ + Required: &api.NodeSelector{ + NodeSelectorTerms: []api.NodeSelectorTerm{ + { + MatchExpressions: []api.NodeSelectorRequirement{ + { + Key: "a", + Operator: api.NodeSelectorOpIn, + Values: []string{"1"}, + }, + { + Key: "b", + Operator: api.NodeSelectorOpIn, + Values: []string{"2"}, + }, + { + Key: kubeletapis.LabelZoneFailureDomain, + Operator: api.NodeSelectorOpIn, + Values: []string{"1", "2", "3"}, + }, + }, + }, + }, + }, + }, + }, + }, + err: nil, + }, + { + name: "AWS EBS PV conflicting affinity rules left in-tact", + handler: newPersistentVolumeLabel(), + pvlabeler: mockVolumeLabels(map[string]string{ + "a": "1", + "b": "2", + "c": "3", + }), + preAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "awsebs", + Namespace: "myns", + Labels: map[string]string{ + "c": "3", + }, + }, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{ + VolumeID: "123", + }, + }, + NodeAffinity: &api.VolumeNodeAffinity{ + Required: &api.NodeSelector{ + NodeSelectorTerms: []api.NodeSelectorTerm{ + { + MatchExpressions: []api.NodeSelectorRequirement{ + { + Key: "c", + Operator: api.NodeSelectorOpIn, + Values: []string{"3"}, + }, + }, + }, + }, + }, + }, + }, + }, + postAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "awsebs", + Namespace: "myns", + Labels: map[string]string{ + "a": "1", + "b": "2", + "c": "3", + }, + }, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{ + VolumeID: "123", + }, + }, + NodeAffinity: &api.VolumeNodeAffinity{ + Required: &api.NodeSelector{ + NodeSelectorTerms: []api.NodeSelectorTerm{ + { + MatchExpressions: []api.NodeSelectorRequirement{ + { + Key: "c", + Operator: api.NodeSelectorOpIn, + Values: []string{"3"}, + }, + }, + }, + }, + }, + }, + }, + }, + err: nil, + }, + { + name: "AWS EBS PV non-conflicting affinity rules added", + handler: newPersistentVolumeLabel(), + pvlabeler: mockVolumeLabels(map[string]string{ + "d": "1", + "e": "2", + "f": "3", + }), + preAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "awsebs", + Namespace: "myns", + Labels: map[string]string{ + "a": "1", + "b": "2", + "c": "3", + }, + }, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{ + VolumeID: "123", + }, + }, + NodeAffinity: &api.VolumeNodeAffinity{ + Required: &api.NodeSelector{ + NodeSelectorTerms: []api.NodeSelectorTerm{ + { + MatchExpressions: []api.NodeSelectorRequirement{ + { + Key: "a", + Operator: api.NodeSelectorOpIn, + Values: []string{"1"}, + }, + { + Key: "b", + Operator: api.NodeSelectorOpIn, + Values: []string{"2"}, + }, + { + Key: "c", + Operator: api.NodeSelectorOpIn, + Values: []string{"3"}, + }, + }, + }, + }, + }, + }, + }, + }, + postAdmissionPV: &api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "awsebs", + Namespace: "myns", + Labels: map[string]string{ + "a": "1", + "b": "2", + "c": "3", + "d": "1", + "e": "2", + "f": "3", + }, + }, + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{ + VolumeID: "123", + }, + }, + NodeAffinity: &api.VolumeNodeAffinity{ + Required: &api.NodeSelector{ + NodeSelectorTerms: []api.NodeSelectorTerm{ + { + MatchExpressions: []api.NodeSelectorRequirement{ + { + Key: "a", + Operator: api.NodeSelectorOpIn, + Values: []string{"1"}, + }, + { + Key: "b", + Operator: api.NodeSelectorOpIn, + Values: []string{"2"}, + }, + { + Key: "c", + Operator: api.NodeSelectorOpIn, + Values: []string{"3"}, + }, + { + Key: "d", + Operator: api.NodeSelectorOpIn, + Values: []string{"1"}, + }, + { + Key: "e", + Operator: api.NodeSelectorOpIn, + Values: []string{"2"}, + }, + { + Key: "f", + Operator: api.NodeSelectorOpIn, + Values: []string{"3"}, + }, + }, + }, + }, + }, + }, + }, + }, + err: nil, }, } - // Non-cloud PVs are ignored - err := handler.Admit(admission.NewAttributesRecord(&ignoredPV, nil, api.Kind("PersistentVolume").WithVersion("version"), ignoredPV.Namespace, ignoredPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Create, false, nil)) - if err != nil { - t.Errorf("Unexpected error returned from admission handler (on ignored pv): %v", err) - } + for _, testcase := range testcases { + t.Run(testcase.name, func(t *testing.T) { + setPVLabeler(testcase.handler, testcase.pvlabeler) + handler := admission.NewChainHandler(testcase.handler) - // We only add labels on creation - err = handler.Admit(admission.NewAttributesRecord(&awsPV, nil, api.Kind("PersistentVolume").WithVersion("version"), awsPV.Namespace, awsPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Delete, false, nil)) - if err != nil { - t.Errorf("Unexpected error returned from admission handler (when deleting aws pv): %v", err) - } + err := handler.Admit(admission.NewAttributesRecord(testcase.preAdmissionPV, nil, api.Kind("PersistentVolume").WithVersion("version"), testcase.preAdmissionPV.Namespace, testcase.preAdmissionPV.Name, api.Resource("persistentvolumes").WithVersion("version"), "", admission.Create, false, nil)) + if !reflect.DeepEqual(err, testcase.err) { + t.Logf("expected error: %q", testcase.err) + t.Logf("actual error: %q", err) + t.Error("unexpected error when admitting PV") + } - // Errors from the cloudprovider block creation of the 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") - } + // sort node selector match expression by key because they are added out of order in the admission controller + sortMatchExpressions(testcase.preAdmissionPV) + if !reflect.DeepEqual(testcase.preAdmissionPV, testcase.postAdmissionPV) { + t.Logf("expected PV: %+v", testcase.postAdmissionPV) + t.Logf("actual PV: %+v", testcase.preAdmissionPV) + t.Error("unexpected PV") + } - // Don't add labels if the cloudprovider doesn't return any - labels := make(map[string]string) - 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") - } - if len(awsPV.ObjectMeta.Labels) != 0 { - t.Errorf("Unexpected number of labels") - } - if awsPV.Spec.NodeAffinity != nil { - t.Errorf("Unexpected NodeAffinity found") - } - - // Don't panic if the cloudprovider returns nil, 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") - } - - // Labels from the cloudprovider should be applied to the volume as labels and node affinity expressions - labels = make(map[string]string) - labels["a"] = "1" - labels["b"] = "2" - zones, _ := volumeutil.ZonesToSet("1,2,3") - labels[kubeletapis.LabelZoneFailureDomain] = volumeutil.ZonesSetToLabelValue(zones) - 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") - } - if awsPV.Labels["a"] != "1" || awsPV.Labels["b"] != "2" { - t.Errorf("Expected label a and b to be added when creating aws pv") - } - if awsPV.Spec.NodeAffinity == nil { - t.Errorf("Unexpected nil NodeAffinity found") - } - if len(awsPV.Spec.NodeAffinity.Required.NodeSelectorTerms) != 1 { - t.Errorf("Unexpected number of NodeSelectorTerms") - } - term := awsPV.Spec.NodeAffinity.Required.NodeSelectorTerms[0] - if len(term.MatchExpressions) != 3 { - t.Errorf("Unexpected number of NodeSelectorRequirements in volume NodeAffinity: %d", len(term.MatchExpressions)) - } - r, _ := getNodeSelectorRequirementWithKey("a", term) - if r == nil || r.Values[0] != "1" || r.Operator != api.NodeSelectorOpIn { - t.Errorf("NodeSelectorRequirement a-in-1 not found in volume NodeAffinity") - } - r, _ = getNodeSelectorRequirementWithKey("b", term) - if r == nil || r.Values[0] != "2" || r.Operator != api.NodeSelectorOpIn { - t.Errorf("NodeSelectorRequirement b-in-2 not found in volume NodeAffinity") - } - r, _ = getNodeSelectorRequirementWithKey(kubeletapis.LabelZoneFailureDomain, term) - if r == nil { - t.Errorf("NodeSelectorRequirement %s-in-%v not found in volume NodeAffinity", kubeletapis.LabelZoneFailureDomain, zones) - } - sort.Strings(r.Values) - if !reflect.DeepEqual(r.Values, zones.List()) { - t.Errorf("ZoneFailureDomain elements %v does not match zone labels %v", r.Values, zones) - } - - // User-provided labels should be honored, but cloudprovider labels replace them when they overlap - awsPV.ObjectMeta.Labels = make(map[string]string) - awsPV.ObjectMeta.Labels["a"] = "not1" - awsPV.ObjectMeta.Labels["c"] = "3" - 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") - } - if awsPV.Labels["a"] != "1" || awsPV.Labels["b"] != "2" { - t.Errorf("Expected cloudprovider labels to replace user labels when creating aws pv") - } - if awsPV.Labels["c"] != "3" { - t.Errorf("Expected (non-conflicting) user provided labels to be honored when creating aws pv") - } - - // if a conflicting affinity is already specified, leave affinity in-tact - labels = make(map[string]string) - labels["a"] = "1" - labels["b"] = "2" - labels["c"] = "3" - 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") - } - if awsPV.Spec.NodeAffinity == nil { - t.Errorf("Unexpected nil NodeAffinity found") - } - if awsPV.Spec.NodeAffinity.Required == nil { - t.Errorf("Unexpected nil NodeAffinity.Required %v", awsPV.Spec.NodeAffinity.Required) - } - r, _ = getNodeSelectorRequirementWithKey("c", awsPV.Spec.NodeAffinity.Required.NodeSelectorTerms[0]) - if r != nil { - t.Errorf("NodeSelectorRequirement c not expected in volume NodeAffinity") - } - - // if a non-conflicting affinity is specified, check for new affinity being added - labels = make(map[string]string) - labels["e"] = "1" - labels["f"] = "2" - labels["g"] = "3" - 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") - } - if awsPV.Spec.NodeAffinity == nil { - t.Errorf("Unexpected nil NodeAffinity found") - } - if awsPV.Spec.NodeAffinity.Required == nil { - t.Errorf("Unexpected nil NodeAffinity.Required %v", awsPV.Spec.NodeAffinity.Required) - } - // populate old entries - labels["a"] = "1" - labels["b"] = "2" - for k, v := range labels { - r, _ = getNodeSelectorRequirementWithKey(k, awsPV.Spec.NodeAffinity.Required.NodeSelectorTerms[0]) - if r == nil || r.Values[0] != v || r.Operator != api.NodeSelectorOpIn { - t.Errorf("NodeSelectorRequirement %s-in-%v not found in volume NodeAffinity", k, v) - } + }) } } + +// setPVLabler applies the given mock pvlabeler to implement PV labeling for all cloud providers. +// Given we mock out the values of the labels anyways, assigning the same mock labeler for every +// provider does not reduce test coverage but it does simplify/clean up the tests here because +// the provider is then decided based on the type of PV (EBS, Cinder, GCEPD, Azure Disk, etc) +func setPVLabeler(handler *persistentVolumeLabel, pvlabeler cloudprovider.PVLabeler) { + handler.awsPVLabeler = pvlabeler + handler.gcePVLabeler = pvlabeler + handler.azurePVLabeler = pvlabeler + handler.openStackPVLabeler = pvlabeler +} + +// sortMatchExpressions sorts a PV's node selector match expressions by key name if it is not nil +func sortMatchExpressions(pv *api.PersistentVolume) { + if pv.Spec.NodeAffinity == nil || + pv.Spec.NodeAffinity.Required == nil || + pv.Spec.NodeAffinity.Required.NodeSelectorTerms == nil { + return + } + + match := pv.Spec.NodeAffinity.Required.NodeSelectorTerms[0].MatchExpressions + sort.Slice(match, func(i, j int) bool { + return match[i].Key < match[j].Key + }) + + pv.Spec.NodeAffinity.Required.NodeSelectorTerms[0].MatchExpressions = match +} From 467a3e5f208e8c7afb5d4411e936c7c4ae2626e6 Mon Sep 17 00:00:00 2001 From: Andrew Kim Date: Wed, 23 Jan 2019 12:43:52 -0500 Subject: [PATCH 4/4] add andrewsykim, dims and msau42 as PVL admission OWNERS --- .../pkg/admission/storage/persistentvolume/label/OWNERS | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 plugin/pkg/admission/storage/persistentvolume/label/OWNERS diff --git a/plugin/pkg/admission/storage/persistentvolume/label/OWNERS b/plugin/pkg/admission/storage/persistentvolume/label/OWNERS new file mode 100644 index 00000000000..cbf8f2b48e7 --- /dev/null +++ b/plugin/pkg/admission/storage/persistentvolume/label/OWNERS @@ -0,0 +1,8 @@ +reviewers: + - andrewsykim + - dims + - msau42 +approvers: + - andrewsykim + - dims + - msau42