Added validation to prevent mutating Claim.Spec after binding

pkg/api/validation/validation.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"net"
 	"path"
+	"reflect"
 	"strings"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
@@ -28,6 +29,7 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 	errs "github.com/GoogleCloudPlatform/kubernetes/pkg/util/fielderrors"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/volume"
 
 	"github.com/golang/glog"
 )
@@ -421,7 +423,7 @@ func ValidatePersistentVolumeClaim(pvc *api.PersistentVolumeClaim) errs.Validati
 		allErrs = append(allErrs, errs.NewFieldInvalid("persistentVolumeClaim.Spec.AccessModes", pvc.Spec.AccessModes, "at least 1 AccessModeType is required"))
 	}
 	if len(pvc.Spec.Resources.Requests) == 0 {
-		allErrs = append(allErrs, errs.NewFieldInvalid("persistentVolumeClaim.Spec.Resources.Requests", pvc.Spec.AccessModes, "No Resource.Requests specified"))
+		allErrs = append(allErrs, errs.NewFieldInvalid("persistentVolumeClaim.Spec.Resources.Requests", pvc.Spec.Resources.Requests, "No Resource.Requests specified"))
 	}
 	return allErrs
 }
@@ -429,6 +431,16 @@ func ValidatePersistentVolumeClaim(pvc *api.PersistentVolumeClaim) errs.Validati
 func ValidatePersistentVolumeClaimUpdate(newPvc, oldPvc *api.PersistentVolumeClaim) errs.ValidationErrorList {
 	allErrs := errs.ValidationErrorList{}
 	allErrs = ValidatePersistentVolumeClaim(newPvc)
+	if oldPvc.Status.VolumeRef != nil {
+		oldModesAsString := volume.GetAccessModesAsString(oldPvc.Spec.AccessModes)
+		newModesAsString := volume.GetAccessModesAsString(newPvc.Spec.AccessModes)
+		if oldModesAsString != newModesAsString {
+			allErrs = append(allErrs, errs.NewFieldInvalid("spec.AccessModes", oldPvc.Spec.AccessModes, "field is immutable"))
+		}
+		if !reflect.DeepEqual(oldPvc.Spec.Resources, newPvc.Spec.Resources) {
+			allErrs = append(allErrs, errs.NewFieldInvalid("spec.Resources", oldPvc.Spec.Resources, "field is immutable"))
+		}
+	}
 	newPvc.Status = oldPvc.Status
 	return allErrs
 }

pkg/api/validation/validation_test.go

@@ -366,6 +366,136 @@ func TestValidatePersistentVolumeClaim(t *testing.T) {
 	}
 }
 
+func TestValidatePersistentVolumeClaimUpdate(t *testing.T) {
+
+	pvcA := &api.PersistentVolumeClaim{
+		ObjectMeta: api.ObjectMeta{
+			Name:      "foo",
+			Namespace: "ns",
+			Labels: map[string]string{
+				"nice-label": "fizzbuzz",
+			},
+		},
+		Spec: api.PersistentVolumeClaimSpec{
+			AccessModes: []api.AccessModeType{
+				api.ReadWriteOnce,
+			},
+			Resources: api.ResourceRequirements{
+				Requests: api.ResourceList{
+					api.ResourceName(api.ResourceStorage): resource.MustParse("10G"),
+				},
+			},
+		},
+	}
+
+	// AccessModes differ from pvcA
+	pvcB := &api.PersistentVolumeClaim{
+		ObjectMeta: api.ObjectMeta{
+			Name:      "foo",
+			Namespace: "ns",
+			Labels: map[string]string{
+				"nice-label": "fizzbuzz",
+			},
+		},
+		Spec: api.PersistentVolumeClaimSpec{
+			AccessModes: []api.AccessModeType{
+				api.ReadWriteMany,
+			},
+			Resources: api.ResourceRequirements{
+				Requests: api.ResourceList{
+					api.ResourceName(api.ResourceStorage): resource.MustParse("10G"),
+				},
+			},
+		},
+	}
+
+	// Resources differ from pvcA
+	pvcC := &api.PersistentVolumeClaim{
+		ObjectMeta: api.ObjectMeta{
+			Name:      "foo",
+			Namespace: "ns",
+			Labels: map[string]string{
+				"nice-label": "fizzbuzz",
+			},
+		},
+		Spec: api.PersistentVolumeClaimSpec{
+			AccessModes: []api.AccessModeType{
+				api.ReadWriteOnce,
+			},
+			Resources: api.ResourceRequirements{
+				Requests: api.ResourceList{
+					api.ResourceName(api.ResourceStorage): resource.MustParse("7G"),
+				},
+			},
+		},
+	}
+
+	// Labels differ from pvcA
+	pvcD := &api.PersistentVolumeClaim{
+		ObjectMeta: api.ObjectMeta{
+			Name:      "foo",
+			Namespace: "ns",
+			Labels: map[string]string{
+				"nice-label": "buzzfizz",
+			},
+		},
+		Spec: api.PersistentVolumeClaimSpec{
+			AccessModes: []api.AccessModeType{
+				api.ReadWriteOnce,
+			},
+			Resources: api.ResourceRequirements{
+				Requests: api.ResourceList{
+					api.ResourceName(api.ResourceStorage): resource.MustParse("10G"),
+				},
+			},
+		},
+	}
+
+	scenarios := map[string]struct {
+		isExpectedFailure bool
+		oldClaim          *api.PersistentVolumeClaim
+		newClaim          *api.PersistentVolumeClaim
+	}{
+		"invalid-accessmodes-change": {
+			isExpectedFailure: true,
+			oldClaim:          pvcA,
+			newClaim:          pvcB,
+		},
+		"invalid-resources-change": {
+			isExpectedFailure: true,
+			oldClaim:          pvcA,
+			newClaim:          pvcC,
+		},
+		"valid-label-change": {
+			isExpectedFailure: false,
+			oldClaim:          pvcA,
+			newClaim:          pvcD,
+		},
+	}
+
+	// validation errors on Update only occur if the Claim is already bound.
+	// failures are only expected after binding
+	for name, scenario := range scenarios {
+		errs := ValidatePersistentVolumeClaimUpdate(scenario.newClaim, scenario.oldClaim)
+		if len(errs) > 0 && !scenario.isExpectedFailure {
+			t.Errorf("Unexpected failure for scenario: %s - %+v", name, errs)
+		}
+	}
+
+	// 2 of 3 scenarios above are invalid if the old PVC has a bound PV
+	for name, scenario := range scenarios {
+		scenario.oldClaim.Status.VolumeRef = &api.ObjectReference{Name: "foo", Namespace: "ns"}
+		errs := ValidatePersistentVolumeClaimUpdate(scenario.newClaim, scenario.oldClaim)
+		if len(errs) == 0 && scenario.isExpectedFailure {
+			t.Errorf("Unexpected success for scenario: %s", name)
+		}
+		if len(errs) > 0 && !scenario.isExpectedFailure {
+			t.Errorf("Unexpected failure for scenario: %s - %+v", name, errs)
+		}
+	}
+
+}
+
 func TestValidateVolumes(t *testing.T) {
 	successCase := []api.Volume{
 		{Name: "abc", VolumeSource: api.VolumeSource{HostPath: &api.HostPathVolumeSource{"/mnt/path1"}}},

pkg/volume/util.go

@@ -24,24 +24,23 @@ func GetAccessModesAsString(modes []api.AccessModeType) string {
 	modesAsString := ""
 
 	if contains(modes, api.ReadWriteOnce) {
-		appendAccessMode(modesAsString, "RWO")
+		appendAccessMode(&modesAsString, "RWO")
 	}
 	if contains(modes, api.ReadOnlyMany) {
-		appendAccessMode(modesAsString, "ROX")
+		appendAccessMode(&modesAsString, "ROX")
 	}
 	if contains(modes, api.ReadWriteMany) {
-		appendAccessMode(modesAsString, "RWX")
+		appendAccessMode(&modesAsString, "RWX")
 	}
 
 	return modesAsString
 }
 
-func appendAccessMode(modes, mode string) string {
+func appendAccessMode(modes *string, mode string) {
-	if modes != "" {
+	if *modes != "" {
-		modes += ","
+		*modes += ","
 	}
-	modes += mode
+	*modes += mode
-	return modes
 }
 
 func contains(modes []api.AccessModeType, mode api.AccessModeType) bool {