refactor PVL unit tests to use test tables & add test cases for remaining cloud providers

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",
     ],

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

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,21 +52,24 @@ func mockVolumeLabels(labels map[string]string) *mockVolumes {
 	return &mockVolumes{volumeLabels: labels}
 }
 
-func getNodeSelectorRequirementWithKey(key string, term api.NodeSelectorTerm) (*api.NodeSelectorRequirement, error) {
+func Test_PVLAdmission(t *testing.T) {
-	for _, r := range term.MatchExpressions {
+	testcases := []struct {
-		if r.Key != key {
+		name            string
-			continue
+		handler         *persistentVolumeLabel
-		}
+		pvlabeler       cloudprovider.PVLabeler
-		return &r, nil
+		preAdmissionPV  *api.PersistentVolume
-	}
+		postAdmissionPV *api.PersistentVolume
-	return nil, fmt.Errorf("key %s not found", key)
+		err             error
-}
+	}{
-
+		{
-// TestAdmission
+			name:    "non-cloud PV ignored",
-func TestAdmission(t *testing.T) {
+			handler: newPersistentVolumeLabel(),
-	pvHandler := newPersistentVolumeLabel()
+			pvlabeler: mockVolumeLabels(map[string]string{
-	handler := admission.NewChainHandler(pvHandler)
+				"a":                                "1",
-	ignoredPV := api.PersistentVolume{
+				"b":                                "2",
+				kubeletapis.LabelZoneFailureDomain: "1__2__3",
+			}),
+			preAdmissionPV: &api.PersistentVolume{
 				ObjectMeta: metav1.ObjectMeta{Name: "noncloud", Namespace: "myns"},
 				Spec: api.PersistentVolumeSpec{
 					PersistentVolumeSource: api.PersistentVolumeSource{
@@ -74,9 +78,25 @@ func TestAdmission(t *testing.T) {
 						},
 					},
 				},
-	}
+			},
-	awsPV := api.PersistentVolume{
+			postAdmissionPV: &api.PersistentVolume{
 				ObjectMeta: metav1.ObjectMeta{Name: "noncloud", Namespace: "myns"},
+				Spec: api.PersistentVolumeSpec{
+					PersistentVolumeSource: api.PersistentVolumeSource{
+						HostPath: &api.HostPathVolumeSource{
+							Path: "/",
+						},
+					},
+				},
+			},
+			err: nil,
+		},
+		{
+			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{
@@ -84,148 +104,635 @@ func TestAdmission(t *testing.T) {
 						},
 					},
 				},
+			},
+			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
+	for _, testcase := range testcases {
-	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))
+		t.Run(testcase.name, func(t *testing.T) {
-	if err != nil {
+			setPVLabeler(testcase.handler, testcase.pvlabeler)
-		t.Errorf("Unexpected error returned from admission handler (on ignored pv): %v", err)
+			handler := admission.NewChainHandler(testcase.handler)
+
+			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")
 			}
 
-	// We only add labels on creation
+			// sort node selector match expression by key because they are added out of order in the admission controller
-	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))
+			sortMatchExpressions(testcase.preAdmissionPV)
-	if err != nil {
+			if !reflect.DeepEqual(testcase.preAdmissionPV, testcase.postAdmissionPV) {
-		t.Errorf("Unexpected error returned from admission handler (when deleting aws pv):  %v", err)
+				t.Logf("expected PV: %+v", testcase.postAdmissionPV)
+				t.Logf("actual PV: %+v", testcase.preAdmissionPV)
+				t.Error("unexpected 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")
 }
 
-	// Don't add labels if the cloudprovider doesn't return any
+// setPVLabler applies the given mock pvlabeler to implement PV labeling for all cloud providers.
-	labels := make(map[string]string)
+// Given we mock out the values of the labels anyways, assigning the same mock labeler for every
-	pvHandler.awsPVLabeler = mockVolumeLabels(labels)
+// provider does not reduce test coverage but it does simplify/clean up the tests here because
-	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))
+// the provider is then decided based on the type of PV (EBS, Cinder, GCEPD, Azure Disk, etc)
-	if err != nil {
+func setPVLabeler(handler *persistentVolumeLabel, pvlabeler cloudprovider.PVLabeler) {
-		t.Errorf("Expected no error when creating aws pv")
+	handler.awsPVLabeler = pvlabeler
-	}
+	handler.gcePVLabeler = pvlabeler
-	if len(awsPV.ObjectMeta.Labels) != 0 {
+	handler.azurePVLabeler = pvlabeler
-		t.Errorf("Unexpected number of labels")
+	handler.openStackPVLabeler = pvlabeler
-	}
-	if awsPV.Spec.NodeAffinity != nil {
-		t.Errorf("Unexpected NodeAffinity found")
 }
 
-	// Don't panic if the cloudprovider returns nil, nil
+// sortMatchExpressions sorts a PV's node selector match expressions by key name if it is not nil
-	pvHandler.awsPVLabeler = mockVolumeFailure(nil)
+func sortMatchExpressions(pv *api.PersistentVolume) {
-	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 pv.Spec.NodeAffinity == nil ||
-	if err != nil {
+		pv.Spec.NodeAffinity.Required == nil ||
-		t.Errorf("Expected no error when cloud provider returns empty labels")
+		pv.Spec.NodeAffinity.Required.NodeSelectorTerms == nil {
+		return
 	}
 
-	// Labels from the cloudprovider should be applied to the volume as labels and node affinity expressions
+	match := pv.Spec.NodeAffinity.Required.NodeSelectorTerms[0].MatchExpressions
-	labels = make(map[string]string)
+	sort.Slice(match, func(i, j int) bool {
-	labels["a"] = "1"
+		return match[i].Key < match[j].Key
-	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
+	pv.Spec.NodeAffinity.Required.NodeSelectorTerms[0].MatchExpressions = match
-	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)
-		}
-	}
 }