From 32b6225c72f1a9825e8a8f0868ce36b0f7b381da Mon Sep 17 00:00:00 2001 From: andrewsykim Date: Mon, 21 Jan 2019 19:08:57 -0500 Subject: [PATCH] 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 +}