diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index 50c19ca7f2a..8db404c2970 100644 --- a/pkg/api/validation/validation.go +++ b/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 } diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index 82672d34dc2..0d136be862e 100644 --- a/pkg/api/validation/validation_test.go +++ b/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"}}}, diff --git a/pkg/volume/util.go b/pkg/volume/util.go index 1e64d4078ee..f5f0019bc3f 100644 --- a/pkg/volume/util.go +++ b/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 { - if modes != "" { - modes += "," +func appendAccessMode(modes *string, mode string) { + if *modes != "" { + *modes += "," } - modes += mode - return modes + *modes += mode } func contains(modes []api.AccessModeType, mode api.AccessModeType) bool {