Merge pull request #82830 from jsafrane/pv-admission-fix

Do not query the cloud if dynamic PV has all the labels

plugin/pkg/admission/storage/persistentvolume/label/BUILD

@@ -16,8 +16,10 @@ go_library(
     deps = [
         "//pkg/apis/core:go_default_library",
         "//pkg/apis/core/v1:go_default_library",
+        "//pkg/controller/volume/persistentvolume/util:go_default_library",
         "//pkg/kubeapiserver/admission: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/apiserver/pkg/admission:go_default_library",
         "//staging/src/k8s.io/cloud-provider:go_default_library",
         "//staging/src/k8s.io/cloud-provider/volume:go_default_library",
@@ -32,6 +34,7 @@ go_test(
     embed = [":go_default_library"],
     deps = [
         "//pkg/apis/core:go_default_library",
+        "//pkg/controller/volume/persistentvolume/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",

plugin/pkg/admission/storage/persistentvolume/label/admission.go

@@ -25,6 +25,7 @@ import (
 	"sync"
 
 	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apiserver/pkg/admission"
 	cloudprovider "k8s.io/cloud-provider"
 	cloudvolume "k8s.io/cloud-provider/volume"
@@ -32,6 +33,7 @@ import (
 	"k8s.io/klog"
 	api "k8s.io/kubernetes/pkg/apis/core"
 	k8s_api_v1 "k8s.io/kubernetes/pkg/apis/core/v1"
+	persistentvolume "k8s.io/kubernetes/pkg/controller/volume/persistentvolume/util"
 	kubeapiserveradmission "k8s.io/kubernetes/pkg/kubeapiserver/admission"
 )
 
@@ -110,42 +112,11 @@ func (l *persistentVolumeLabel) Admit(ctx context.Context, a admission.Attribute
 		return nil
 	}
 
-	var volumeLabels map[string]string
+	volumeLabels, err := l.findVolumeLabels(volume)
-	if volume.Spec.AWSElasticBlockStore != nil {
+	if err != nil {
-		labels, err := l.findAWSEBSLabels(volume)
+		return admission.NewForbidden(a, err)
-		if err != nil {
-			return admission.NewForbidden(a, fmt.Errorf("error querying AWS EBS volume %s: %v", volume.Spec.AWSElasticBlockStore.VolumeID, err))
-		}
-		volumeLabels = labels
-	}
-	if volume.Spec.GCEPersistentDisk != nil {
-		labels, err := l.findGCEPDLabels(volume)
-		if err != nil {
-			return admission.NewForbidden(a, fmt.Errorf("error querying GCE PD volume %s: %v", volume.Spec.GCEPersistentDisk.PDName, err))
-		}
-		volumeLabels = labels
-	}
-	if volume.Spec.AzureDisk != nil {
-		labels, err := l.findAzureDiskLabels(volume)
-		if err != nil {
-			return admission.NewForbidden(a, fmt.Errorf("error querying AzureDisk volume %s: %v", volume.Spec.AzureDisk.DiskName, err))
-		}
-		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
-	}
-	if volume.Spec.VsphereVolume != nil {
-		labels, err := l.findVsphereVolumeLabels(volume)
-		if err != nil {
-			return admission.NewForbidden(a, fmt.Errorf("error querying vSphere Volume %s: %v", volume.Spec.VsphereVolume.VolumePath, err))
-		}
-		volumeLabels = labels
 	}
+
 	requirements := make([]api.NodeSelectorRequirement, 0)
 	if len(volumeLabels) != 0 {
 		if volume.Labels == nil {
@@ -197,6 +168,58 @@ func (l *persistentVolumeLabel) Admit(ctx context.Context, a admission.Attribute
 	return nil
 }
 
+func (l *persistentVolumeLabel) findVolumeLabels(volume *api.PersistentVolume) (map[string]string, error) {
+	existingLabels := volume.Labels
+
+	// All cloud providers set only these two labels.
+	domain, domainOK := existingLabels[v1.LabelZoneFailureDomain]
+	region, regionOK := existingLabels[v1.LabelZoneRegion]
+	isDynamicallyProvisioned := metav1.HasAnnotation(volume.ObjectMeta, persistentvolume.AnnDynamicallyProvisioned)
+	if isDynamicallyProvisioned && domainOK && regionOK {
+		// PV already has all the labels and we can trust the dynamic provisioning that it provided correct values.
+		return map[string]string{
+			v1.LabelZoneFailureDomain: domain,
+			v1.LabelZoneRegion:        region,
+		}, nil
+	}
+
+	// Either missing labels or we don't trust the user provided correct values.
+	switch {
+	case volume.Spec.AWSElasticBlockStore != nil:
+		labels, err := l.findAWSEBSLabels(volume)
+		if err != nil {
+			return nil, fmt.Errorf("error querying AWS EBS volume %s: %v", volume.Spec.AWSElasticBlockStore.VolumeID, err)
+		}
+		return labels, nil
+	case volume.Spec.GCEPersistentDisk != nil:
+		labels, err := l.findGCEPDLabels(volume)
+		if err != nil {
+			return nil, fmt.Errorf("error querying GCE PD volume %s: %v", volume.Spec.GCEPersistentDisk.PDName, err)
+		}
+		return labels, nil
+	case volume.Spec.AzureDisk != nil:
+		labels, err := l.findAzureDiskLabels(volume)
+		if err != nil {
+			return nil, fmt.Errorf("error querying AzureDisk volume %s: %v", volume.Spec.AzureDisk.DiskName, err)
+		}
+		return labels, nil
+	case volume.Spec.Cinder != nil:
+		labels, err := l.findCinderDiskLabels(volume)
+		if err != nil {
+			return nil, fmt.Errorf("error querying Cinder volume %s: %v", volume.Spec.Cinder.VolumeID, err)
+		}
+		return labels, nil
+	case volume.Spec.VsphereVolume != nil:
+		labels, err := l.findVsphereVolumeLabels(volume)
+		if err != nil {
+			return nil, fmt.Errorf("error querying vSphere Volume %s: %v", volume.Spec.VsphereVolume.VolumePath, err)
+		}
+		return labels, nil
+	}
+	// Unrecognized volume, do not add any labels
+	return nil, nil
+}
+
 func (l *persistentVolumeLabel) findAWSEBSLabels(volume *api.PersistentVolume) (map[string]string, error) {
 	// Ignore any volumes that are being provisioned
 	if volume.Spec.AWSElasticBlockStore.VolumeID == cloudvolume.ProvisionedVolumeName {

plugin/pkg/admission/storage/persistentvolume/label/admission_test.go

@@ -31,6 +31,7 @@ import (
 	admissiontesting "k8s.io/apiserver/pkg/admission/testing"
 	cloudprovider "k8s.io/cloud-provider"
 	api "k8s.io/kubernetes/pkg/apis/core"
+	persistentvolume "k8s.io/kubernetes/pkg/controller/volume/persistentvolume/util"
 )
 
 type mockVolumes struct {
@@ -232,6 +233,136 @@ func Test_PVLAdmission(t *testing.T) {
 			},
 			err: nil,
 		},
+		{
+			name:    "existing labels from dynamic provisioning are not changed",
+			handler: newPersistentVolumeLabel(),
+			pvlabeler: mockVolumeLabels(map[string]string{
+				v1.LabelZoneFailureDomain: "domain1",
+				v1.LabelZoneRegion:        "region1",
+			}),
+			preAdmissionPV: &api.PersistentVolume{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "awsebs", Namespace: "myns",
+					Labels: map[string]string{
+						v1.LabelZoneFailureDomain: "existingDomain",
+						v1.LabelZoneRegion:        "existingRegion",
+					},
+					Annotations: map[string]string{
+						persistentvolume.AnnDynamicallyProvisioned: "kubernetes.io/aws-ebs",
+					},
+				},
+				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{
+						v1.LabelZoneFailureDomain: "existingDomain",
+						v1.LabelZoneRegion:        "existingRegion",
+					},
+					Annotations: map[string]string{
+						persistentvolume.AnnDynamicallyProvisioned: "kubernetes.io/aws-ebs",
+					},
+				},
+				Spec: api.PersistentVolumeSpec{
+					PersistentVolumeSource: api.PersistentVolumeSource{
+						AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{
+							VolumeID: "123",
+						},
+					},
+					NodeAffinity: &api.VolumeNodeAffinity{
+						Required: &api.NodeSelector{
+							NodeSelectorTerms: []api.NodeSelectorTerm{
+								{
+									MatchExpressions: []api.NodeSelectorRequirement{
+										{
+											Key:      v1.LabelZoneRegion,
+											Operator: api.NodeSelectorOpIn,
+											Values:   []string{"existingRegion"},
+										},
+										{
+											Key:      v1.LabelZoneFailureDomain,
+											Operator: api.NodeSelectorOpIn,
+											Values:   []string{"existingDomain"},
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			err: nil,
+		},
+		{
+			name:    "existing labels from user are changed",
+			handler: newPersistentVolumeLabel(),
+			pvlabeler: mockVolumeLabels(map[string]string{
+				v1.LabelZoneFailureDomain: "domain1",
+				v1.LabelZoneRegion:        "region1",
+			}),
+			preAdmissionPV: &api.PersistentVolume{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "awsebs", Namespace: "myns",
+					Labels: map[string]string{
+						v1.LabelZoneFailureDomain: "existingDomain",
+						v1.LabelZoneRegion:        "existingRegion",
+					},
+				},
+				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{
+						v1.LabelZoneFailureDomain: "domain1",
+						v1.LabelZoneRegion:        "region1",
+					},
+				},
+				Spec: api.PersistentVolumeSpec{
+					PersistentVolumeSource: api.PersistentVolumeSource{
+						AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{
+							VolumeID: "123",
+						},
+					},
+					NodeAffinity: &api.VolumeNodeAffinity{
+						Required: &api.NodeSelector{
+							NodeSelectorTerms: []api.NodeSelectorTerm{
+								{
+									MatchExpressions: []api.NodeSelectorRequirement{
+										{
+											Key:      v1.LabelZoneRegion,
+											Operator: api.NodeSelectorOpIn,
+											Values:   []string{"region1"},
+										},
+										{
+											Key:      v1.LabelZoneFailureDomain,
+											Operator: api.NodeSelectorOpIn,
+											Values:   []string{"domain1"},
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			err: nil,
+		},
 		{
 			name:    "GCE PD PV labeled correctly",
 			handler: newPersistentVolumeLabel(),