Allow deprovision after change to non-existent plan (openshift#1557)

Author: staebler

pkg/apis/servicecatalog/validation/instance.go

@@ -250,8 +250,15 @@ func validateServiceInstanceUpdate(instance *sc.ServiceInstance) field.ErrorList
 		if instance.Spec.ClusterServiceClassRef == nil {
 			allErrs = append(allErrs, field.Required(field.NewPath("spec").Child("clusterServiceClassRef"), "serviceClassRef is required when currentOperation is present"))
 		}
-		if instance.Spec.ClusterServicePlanRef == nil {
-			allErrs = append(allErrs, field.Required(field.NewPath("spec").Child("clusterServicePlanRef"), "servicePlanRef is required when currentOperation is present"))
+		if instance.Status.CurrentOperation != sc.ServiceInstanceOperationDeprovision {
+			if instance.Spec.ClusterServicePlanRef == nil {
+				allErrs = append(allErrs, field.Required(field.NewPath("spec").Child("clusterServicePlanRef"), "servicePlanRef is required when currentOperation is present and not Deprovision"))
+			}
+		} else {
+			if instance.Spec.ClusterServicePlanRef == nil &&
+				(instance.Status.ExternalProperties == nil || instance.Status.ExternalProperties.ClusterServicePlanExternalID == "") {
+				allErrs = append(allErrs, field.Invalid(field.NewPath("status").Child("currentOperation"), instance.Status.CurrentOperation, "spec.clusterServicePlanRef or status.externalProperties.clusterServicePlanExternalID is required when currentOperation is Deprovision"))
+			}
 		}
 	}
 	return allErrs

pkg/apis/servicecatalog/validation/instance_test.go

@@ -82,6 +82,17 @@ func validServiceInstanceWithInProgressProvision() *servicecatalog.ServiceInstan
 	return instance
 }
 
+func validServiceInstanceWithInProgressDeprovision() *servicecatalog.ServiceInstance {
+	instance := validServiceInstance()
+	instance.Generation = 2
+	instance.Status.ReconciledGeneration = 1
+	instance.Status.CurrentOperation = servicecatalog.ServiceInstanceOperationDeprovision
+	now := metav1.Now()
+	instance.Status.OperationStartTime = &now
+	instance.Status.ExternalProperties = validServiceInstancePropertiesState()
+	return instance
+}
+
 func validServiceInstancePropertiesState() *servicecatalog.ServiceInstancePropertiesState {
 	return &servicecatalog.ServiceInstancePropertiesState{
 		ClusterServicePlanExternalName: "plan-name",
@@ -516,7 +527,7 @@ func TestValidateServiceInstance(t *testing.T) {
 			valid:  false,
 		},
 		{
-			name: "in-progress operation with missing service plan ref",
+			name: "in-progress provision with missing service plan ref",
 			instance: func() *servicecatalog.ServiceInstance {
 				i := validServiceInstanceWithInProgressProvision()
 				i.Spec.ClusterServicePlanRef = nil
@@ -525,6 +536,65 @@ func TestValidateServiceInstance(t *testing.T) {
 			create: false,
 			valid:  false,
 		},
+		{
+			name:     "valid in-progress deprovision",
+			instance: validServiceInstanceWithInProgressDeprovision(),
+			create:   false,
+			valid:    true,
+		},
+		{
+			name: "in-progress deprovision with missing service plan ref",
+			instance: func() *servicecatalog.ServiceInstance {
+				i := validServiceInstanceWithInProgressDeprovision()
+				i.Spec.ClusterServicePlanRef = nil
+				return i
+			}(),
+			create: false,
+			valid:  true,
+		},
+		{
+			name: "in-progress deprovision with missing external properties",
+			instance: func() *servicecatalog.ServiceInstance {
+				i := validServiceInstanceWithInProgressDeprovision()
+				i.Status.ExternalProperties = nil
+				return i
+			}(),
+			create: false,
+			valid:  true,
+		},
+		{
+			name: "in-progress deprovision with missing external properties plan ID",
+			instance: func() *servicecatalog.ServiceInstance {
+				i := validServiceInstanceWithInProgressDeprovision()
+				i.Status.ExternalProperties.ClusterServicePlanExternalID = ""
+				return i
+			}(),
+			create: false,
+			// not valid because ClusterServicePlanExternalID is required when ExternalProperties is present
+			valid: false,
+		},
+		{
+			name: "in-progress deprovision with missing service plan ref and external properties",
+			instance: func() *servicecatalog.ServiceInstance {
+				i := validServiceInstanceWithInProgressDeprovision()
+				i.Spec.ClusterServicePlanRef = nil
+				i.Status.ExternalProperties = nil
+				return i
+			}(),
+			create: false,
+			valid:  false,
+		},
+		{
+			name: "in-progress deprovision with missing service plan ref and external properties plan ID",
+			instance: func() *servicecatalog.ServiceInstance {
+				i := validServiceInstanceWithInProgressDeprovision()
+				i.Spec.ClusterServicePlanRef = nil
+				i.Status.ExternalProperties.ClusterServicePlanExternalID = ""
+				return i
+			}(),
+			create: false,
+			valid:  false,
+		},
 		{
 			name: "external and k8s name specified in Spec.PlanReference",
 			instance: func() *servicecatalog.ServiceInstance {