Do not block instance spec changes unless there is an on-going operation (#1536)

* Do not block instance spec changes unless there is an on-going operation.

* Do not consider reconciled generation when considering whether to block a spec change

Author: staebler

pkg/apis/servicecatalog/validation/instance.go

@@ -269,7 +269,7 @@ func validateServiceInstanceUpdate(instance *sc.ServiceInstance) field.ErrorList
 // to the spec to go through.
 func internalValidateServiceInstanceUpdateAllowed(new *sc.ServiceInstance, old *sc.ServiceInstance) field.ErrorList {
 	errors := field.ErrorList{}
-	if old.Generation != new.Generation && old.Status.ReconciledGeneration != old.Generation {
+	if old.Generation != new.Generation && old.Status.CurrentOperation != "" {
 		errors = append(errors, field.Forbidden(field.NewPath("spec"), "Another update for this service instance is in progress"))
 	}
 	if old.Spec.ClusterServicePlanExternalName != new.Spec.ClusterServicePlanExternalName && new.Spec.ClusterServicePlanRef != nil {

pkg/apis/servicecatalog/validation/instance_test.go

@@ -698,34 +698,34 @@ func TestValidateServiceInstance(t *testing.T) {
 
 func TestInternalValidateServiceInstanceUpdateAllowed(t *testing.T) {
 	cases := []struct {
-		name              string
-		newSpecChange     bool
-		onGoingSpecChange bool
-		valid             bool
+		name             string
+		specChange       bool
+		onGoingOperation bool
+		valid            bool
 	}{
 		{
-			name:              "spec change when no on-going spec change",
-			newSpecChange:     true,
-			onGoingSpecChange: false,
-			valid:             true,
+			name:             "spec change when no on-going operation",
+			specChange:       true,
+			onGoingOperation: false,
+			valid:            true,
 		},
 		{
-			name:              "spec change when on-going spec change",
-			newSpecChange:     true,
-			onGoingSpecChange: true,
-			valid:             false,
+			name:             "spec change when on-going operation",
+			specChange:       true,
+			onGoingOperation: true,
+			valid:            false,
 		},
 		{
-			name:              "meta change when no on-going spec change",
-			newSpecChange:     false,
-			onGoingSpecChange: false,
-			valid:             true,
+			name:             "meta change when no on-going operation",
+			specChange:       false,
+			onGoingOperation: false,
+			valid:            true,
 		},
 		{
-			name:              "meta change when on-going spec change",
-			newSpecChange:     false,
-			onGoingSpecChange: true,
-			valid:             true,
+			name:             "meta change when on-going operation",
+			specChange:       false,
+			onGoingOperation: true,
+			valid:            true,
 		},
 	}
 
@@ -742,12 +742,10 @@ func TestInternalValidateServiceInstanceUpdateAllowed(t *testing.T) {
 				},
 			},
 		}
-		if tc.onGoingSpecChange {
-			oldInstance.Generation = 2
-		} else {
-			oldInstance.Generation = 1
+		oldInstance.Generation = 1
+		if tc.onGoingOperation {
+			oldInstance.Status.CurrentOperation = servicecatalog.ServiceInstanceOperationProvision
 		}
-		oldInstance.Status.ReconciledGeneration = 1
 
 		newInstance := &servicecatalog.ServiceInstance{
 			ObjectMeta: metav1.ObjectMeta{
@@ -756,17 +754,16 @@ func TestInternalValidateServiceInstanceUpdateAllowed(t *testing.T) {
 			},
 			Spec: servicecatalog.ServiceInstanceSpec{
 				PlanReference: servicecatalog.PlanReference{
-					ClusterServiceClassExternalName: "test-serviceclass",
-					ClusterServicePlanExternalName:  "test-plan",
+					ClusterServiceClassExternalName: clusterServiceClassExternalName,
+					ClusterServicePlanExternalName:  clusterServicePlanExternalName,
 				},
 			},
 		}
-		if tc.newSpecChange {
+		if tc.specChange {
 			newInstance.Generation = oldInstance.Generation + 1
 		} else {
 			newInstance.Generation = oldInstance.Generation
 		}
-		newInstance.Status.ReconciledGeneration = 1
 
 		errs := internalValidateServiceInstanceUpdateAllowed(newInstance, oldInstance)
 		if len(errs) != 0 && tc.valid {