From 4b5d1591f160089ef948a6271081f26d5b860369 Mon Sep 17 00:00:00 2001 From: Nail Islamov Date: Fri, 16 Mar 2018 04:31:06 +1100 Subject: [PATCH] Pass correct plan ID in deprovision request (for both deleting and orphan mitigation) (#1803) * In deprovisioning pass the plan id that was used during the last provision/update operation * Leave a TODO * Fix nil pointer dereference panic * Rewrite handling deletion for provisioning that is in progress * Fix review comments * Fix tests * Fix deprovisioning when no orphan mitigation is required * Tests cleanup --- .../servicecatalog/validation/instance.go | 6 +- .../validation/instance_test.go | 11 +-- pkg/controller/controller.go | 37 ++++--- pkg/controller/controller_instance.go | 93 +++++++++-------- pkg/controller/controller_instance_test.go | 35 +++++-- pkg/controller/controller_test.go | 99 +++++-------------- 6 files changed, 128 insertions(+), 153 deletions(-) diff --git a/pkg/apis/servicecatalog/validation/instance.go b/pkg/apis/servicecatalog/validation/instance.go index 88a0bb44d39c..0e28b891ce0c 100644 --- a/pkg/apis/servicecatalog/validation/instance.go +++ b/pkg/apis/servicecatalog/validation/instance.go @@ -140,13 +140,13 @@ func validateServiceInstanceStatus(status *sc.ServiceInstanceStatus, fldPath *fi } switch status.CurrentOperation { - case sc.ServiceInstanceOperationProvision, sc.ServiceInstanceOperationUpdate: + case sc.ServiceInstanceOperationProvision, sc.ServiceInstanceOperationUpdate, sc.ServiceInstanceOperationDeprovision: if status.InProgressProperties == nil { - allErrs = append(allErrs, field.Required(fldPath.Child("inProgressProperties"), `inProgressProperties is required when currentOperation is "Provision" or "Update"`)) + allErrs = append(allErrs, field.Required(fldPath.Child("inProgressProperties"), `inProgressProperties is required when currentOperation is "Provision", "Update" or "Deprovision"`)) } default: if status.InProgressProperties != nil { - allErrs = append(allErrs, field.Forbidden(fldPath.Child("inProgressProperties"), `inProgressProperties must not be present when currentOperation is neither "Provision" nor "Update"`)) + allErrs = append(allErrs, field.Forbidden(fldPath.Child("inProgressProperties"), `inProgressProperties must not be present when currentOperation is not "Provision", "Update" or "Deprovision"`)) } } diff --git a/pkg/apis/servicecatalog/validation/instance_test.go b/pkg/apis/servicecatalog/validation/instance_test.go index 7dd80571c6bb..187b0767d190 100644 --- a/pkg/apis/servicecatalog/validation/instance_test.go +++ b/pkg/apis/servicecatalog/validation/instance_test.go @@ -89,6 +89,7 @@ func validServiceInstanceWithInProgressDeprovision() *servicecatalog.ServiceInst instance.Status.CurrentOperation = servicecatalog.ServiceInstanceOperationDeprovision now := metav1.Now() instance.Status.OperationStartTime = &now + instance.Status.InProgressProperties = validServiceInstancePropertiesState() instance.Status.ExternalProperties = validServiceInstancePropertiesState() return instance } @@ -221,7 +222,6 @@ func TestValidateServiceInstance(t *testing.T) { instance: func() *servicecatalog.ServiceInstance { i := validServiceInstanceWithInProgressProvision() i.Status.CurrentOperation = servicecatalog.ServiceInstanceOperationDeprovision - i.Status.InProgressProperties = nil return i }(), valid: true, @@ -319,15 +319,6 @@ func TestValidateServiceInstance(t *testing.T) { }(), valid: false, }, - { - name: "in-progress deprovision with present InProgressProperties", - instance: func() *servicecatalog.ServiceInstance { - i := validServiceInstanceWithInProgressProvision() - i.Status.CurrentOperation = servicecatalog.ServiceInstanceOperationDeprovision - return i - }(), - valid: false, - }, { name: "in-progress properties with no external plan name", instance: func() *servicecatalog.ServiceInstance { diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 20143c5d0696..c41de3d3c394 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -264,16 +264,9 @@ func (e *operationError) Error() string { return e.message } // is nil. This will happen when deleting a ServiceInstance that previously // had an update to a non-existent plan. func (c *controller) getClusterServiceClassPlanAndClusterServiceBroker(instance *v1beta1.ServiceInstance) (*v1beta1.ClusterServiceClass, *v1beta1.ClusterServicePlan, string, osb.Client, error) { - pcb := pretty.NewContextBuilder(pretty.ServiceInstance, instance.Namespace, instance.Name) - serviceClass, err := c.serviceClassLister.Get(instance.Spec.ClusterServiceClassRef.Name) + serviceClass, brokerName, brokerClient, err := c.getClusterServiceClassAndClusterServiceBroker(instance) if err != nil { - return nil, nil, "", nil, &operationError{ - reason: errorNonexistentClusterServiceClassReason, - message: fmt.Sprintf( - "The instance references a non-existent ClusterServiceClass (K8S: %q ExternalName: %q)", - instance.Spec.ClusterServiceClassRef.Name, instance.Spec.ClusterServiceClassExternalName, - ), - } + return nil, nil, "", nil, err } var servicePlan *v1beta1.ClusterServicePlan @@ -290,10 +283,28 @@ func (c *controller) getClusterServiceClassPlanAndClusterServiceBroker(instance } } } + return serviceClass, servicePlan, brokerName, brokerClient, nil +} + +// getClusterServiceClassAndClusterServiceBroker is a sequence of operations that's done in couple of +// places so this method fetches the Service Class and creates +// a brokerClient to use for that method given an ServiceInstance. +func (c *controller) getClusterServiceClassAndClusterServiceBroker(instance *v1beta1.ServiceInstance) (*v1beta1.ClusterServiceClass, string, osb.Client, error) { + pcb := pretty.NewContextBuilder(pretty.ServiceInstance, instance.Namespace, instance.Name) + serviceClass, err := c.serviceClassLister.Get(instance.Spec.ClusterServiceClassRef.Name) + if err != nil { + return nil, "", nil, &operationError{ + reason: errorNonexistentClusterServiceClassReason, + message: fmt.Sprintf( + "The instance references a non-existent ClusterServiceClass (K8S: %q ExternalName: %q)", + instance.Spec.ClusterServiceClassRef.Name, instance.Spec.ClusterServiceClassExternalName, + ), + } + } broker, err := c.brokerLister.Get(serviceClass.Spec.ClusterServiceBrokerName) if err != nil { - return nil, nil, "", nil, &operationError{ + return nil, "", nil, &operationError{ reason: errorNonexistentClusterServiceBrokerReason, message: fmt.Sprintf( "The instance references a non-existent broker %q", @@ -305,7 +316,7 @@ func (c *controller) getClusterServiceClassPlanAndClusterServiceBroker(instance authConfig, err := getAuthCredentialsFromClusterServiceBroker(c.kubeClient, broker) if err != nil { - return nil, nil, "", nil, &operationError{ + return nil, "", nil, &operationError{ reason: errorAuthCredentialsReason, message: fmt.Sprintf( "Error getting broker auth credentials for broker %q: %s", @@ -318,10 +329,10 @@ func (c *controller) getClusterServiceClassPlanAndClusterServiceBroker(instance glog.V(4).Info(pcb.Messagef("Creating client for ClusterServiceBroker %v, URL: %v", broker.Name, broker.Spec.URL)) brokerClient, err := c.brokerClientCreateFunc(clientConfig) if err != nil { - return nil, nil, "", nil, err + return nil, "", nil, err } - return serviceClass, servicePlan, broker.Name, brokerClient, nil + return serviceClass, broker.Name, brokerClient, nil } // getClusterServiceClassPlanAndClusterServiceBrokerForServiceBinding is a sequence of operations that's diff --git a/pkg/controller/controller_instance.go b/pkg/controller/controller_instance.go index bdaeb17a72d0..0d97eddba437 100644 --- a/pkg/controller/controller_instance.go +++ b/pkg/controller/controller_instance.go @@ -17,6 +17,7 @@ limitations under the License. package controller import ( + stderrors "errors" "fmt" "net/url" @@ -69,8 +70,6 @@ const ( errorOrphanMitigationFailedReason string = "OrphanMitigationFailed" errorInvalidDeprovisionStatusReason string = "InvalidDeprovisionStatus" errorInvalidDeprovisionStatusMessage string = "The deprovision status is invalid" - errorUnknownServicePlanReason string = "UnknownServicePlan" - errorUnknownServicePlanMessage string = "The ServicePlan is not known" asyncProvisioningReason string = "Provisioning" asyncProvisioningMessage string = "The instance is being provisioned asynchronously" @@ -542,17 +541,18 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1beta1.ServiceIns return c.handleServiceInstanceReconciliationError(instance, err) } - serviceClass, servicePlan, brokerName, brokerClient, err := c.getClusterServiceClassPlanAndClusterServiceBroker(instance) + serviceClass, brokerName, brokerClient, err := c.getClusterServiceClassAndClusterServiceBroker(instance) if err != nil { return c.handleServiceInstanceReconciliationError(instance, err) } - request, err := c.prepareDeprovisionRequest(instance, serviceClass, servicePlan) + request, inProgressProperties, err := c.prepareDeprovisionRequest(instance, serviceClass) if err != nil { return c.handleServiceInstanceReconciliationError(instance, err) } if instance.DeletionTimestamp == nil { + // Orphan mitigation if instance.Status.OperationStartTime == nil { // if mitigating an orphan, set the operation start time if unset now := metav1.Now() @@ -560,7 +560,7 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1beta1.ServiceIns } } else { if instance.Status.CurrentOperation != v1beta1.ServiceInstanceOperationDeprovision { - instance, err = c.recordStartOfServiceInstanceOperation(instance, v1beta1.ServiceInstanceOperationDeprovision, nil) + instance, err = c.recordStartOfServiceInstanceOperation(instance, v1beta1.ServiceInstanceOperationDeprovision, inProgressProperties) if err != nil { // There has been an update to the instance. Start reconciliation // over with a fresh view of the instance. @@ -1176,6 +1176,7 @@ func (c *controller) recordStartOfServiceInstanceOperation(toUpdate *v1beta1.Ser toUpdate.Status.CurrentOperation = operation now := metav1.Now() toUpdate.Status.OperationStartTime = &now + toUpdate.Status.InProgressProperties = inProgressProperties reason := "" message := "" switch operation { @@ -1183,11 +1184,9 @@ func (c *controller) recordStartOfServiceInstanceOperation(toUpdate *v1beta1.Ser reason = provisioningInFlightReason message = provisioningInFlightMessage toUpdate.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired - toUpdate.Status.InProgressProperties = inProgressProperties case v1beta1.ServiceInstanceOperationUpdate: reason = instanceUpdatingInFlightReason message = instanceUpdatingInFlightMessage - toUpdate.Status.InProgressProperties = inProgressProperties case v1beta1.ServiceInstanceOperationDeprovision: reason = deprovisioningInFlightReason message = deprovisioningInFlightMessage @@ -1289,7 +1288,7 @@ type requestHelper struct { // prepareRequestHelper is a helper function that generates a struct with // properties common to multiple request types. -func (c *controller) prepareRequestHelper(instance *v1beta1.ServiceInstance, serviceClass *v1beta1.ClusterServiceClass, servicePlan *v1beta1.ClusterServicePlan) (*requestHelper, error) { +func (c *controller) prepareRequestHelper(instance *v1beta1.ServiceInstance, servicePlan *v1beta1.ClusterServicePlan, setInProgressProperties bool) (*requestHelper, error) { rh := &requestHelper{} if utilfeature.DefaultFeatureGate.Enabled(scfeatures.OriginatingIdentity) { @@ -1318,26 +1317,28 @@ func (c *controller) prepareRequestHelper(instance *v1beta1.ServiceInstance, ser } rh.ns = ns - parameters, parametersChecksum, rawParametersWithRedaction, err := prepareInProgressPropertyParameters( - c.kubeClient, - instance.Namespace, - instance.Spec.Parameters, - instance.Spec.ParametersFrom, - ) - if err != nil { - return nil, &operationError{ - reason: errorWithParameters, - message: err.Error(), + if setInProgressProperties { + parameters, parametersChecksum, rawParametersWithRedaction, err := prepareInProgressPropertyParameters( + c.kubeClient, + instance.Namespace, + instance.Spec.Parameters, + instance.Spec.ParametersFrom, + ) + if err != nil { + return nil, &operationError{ + reason: errorWithParameters, + message: err.Error(), + } + } + rh.parameters = parameters + + rh.inProgressProperties = &v1beta1.ServiceInstancePropertiesState{ + ClusterServicePlanExternalName: servicePlan.Spec.ExternalName, + ClusterServicePlanExternalID: servicePlan.Spec.ExternalID, + Parameters: rawParametersWithRedaction, + ParametersChecksum: parametersChecksum, + UserInfo: instance.Spec.UserInfo, } - } - rh.parameters = parameters - - rh.inProgressProperties = &v1beta1.ServiceInstancePropertiesState{ - ClusterServicePlanExternalName: servicePlan.Spec.ExternalName, - ClusterServicePlanExternalID: servicePlan.Spec.ExternalID, - Parameters: rawParametersWithRedaction, - ParametersChecksum: parametersChecksum, - UserInfo: instance.Spec.UserInfo, } // osb client handles whether or not to really send this based @@ -1353,7 +1354,7 @@ func (c *controller) prepareRequestHelper(instance *v1beta1.ServiceInstance, ser // prepareProvisionRequest creates a provision request object to be passed to // the broker client to provision the given instance. func (c *controller) prepareProvisionRequest(instance *v1beta1.ServiceInstance, serviceClass *v1beta1.ClusterServiceClass, servicePlan *v1beta1.ClusterServicePlan) (*osb.ProvisionRequest, *v1beta1.ServiceInstancePropertiesState, error) { - rh, err := c.prepareRequestHelper(instance, serviceClass, servicePlan) + rh, err := c.prepareRequestHelper(instance, servicePlan, true) if err != nil { return nil, nil, err } @@ -1376,7 +1377,7 @@ func (c *controller) prepareProvisionRequest(instance *v1beta1.ServiceInstance, // prepareUpdateInstanceRequest creates an update instance request object to be // passed to the broker client to update the given instance. func (c *controller) prepareUpdateInstanceRequest(instance *v1beta1.ServiceInstance, serviceClass *v1beta1.ClusterServiceClass, servicePlan *v1beta1.ClusterServicePlan) (*osb.UpdateInstanceRequest, *v1beta1.ServiceInstancePropertiesState, error) { - rh, err := c.prepareRequestHelper(instance, serviceClass, servicePlan) + rh, err := c.prepareRequestHelper(instance, servicePlan, true) if err != nil { return nil, nil, err } @@ -1410,39 +1411,42 @@ func (c *controller) prepareUpdateInstanceRequest(instance *v1beta1.ServiceInsta // prepareDeprovisionRequest creates a deprovision request object to be passed // to the broker client to deprovision the given instance. -func (c *controller) prepareDeprovisionRequest(instance *v1beta1.ServiceInstance, serviceClass *v1beta1.ClusterServiceClass, servicePlan *v1beta1.ClusterServicePlan) (*osb.DeprovisionRequest, error) { - rh, err := c.prepareRequestHelper(instance, serviceClass, servicePlan) +func (c *controller) prepareDeprovisionRequest(instance *v1beta1.ServiceInstance, serviceClass *v1beta1.ClusterServiceClass) (*osb.DeprovisionRequest, *v1beta1.ServiceInstancePropertiesState, error) { + rh, err := c.prepareRequestHelper(instance, nil, false) if err != nil { - return nil, err + return nil, nil, err } - var servicePlanExternalID string - if instance.Status.ExternalProperties != nil { - servicePlanExternalID = instance.Status.ExternalProperties.ClusterServicePlanExternalID - } else if servicePlan != nil { - servicePlanExternalID = servicePlan.Spec.ExternalID + // The plan reference in the spec might be updated since the latest + // provisioning/update request, thus we need to take values from the original + // provisioning request instead that we previously stored in status + if instance.Status.CurrentOperation != "" || instance.Status.OrphanMitigationInProgress { + if instance.Status.InProgressProperties == nil { + return nil, nil, stderrors.New("InProgressProperties must be set when there is an operation or orphan mitigation in progress") + } + rh.inProgressProperties = instance.Status.InProgressProperties } else { - return nil, &operationError{ - reason: errorUnknownServicePlanReason, - message: errorUnknownServicePlanMessage, + if instance.Status.ExternalProperties == nil { + return nil, nil, stderrors.New("ExternalProperties must be set before deprovisioning") } + rh.inProgressProperties = instance.Status.ExternalProperties } request := &osb.DeprovisionRequest{ InstanceID: instance.Spec.ExternalID, ServiceID: serviceClass.Spec.ExternalID, - PlanID: servicePlanExternalID, + PlanID: rh.inProgressProperties.ClusterServicePlanExternalID, OriginatingIdentity: rh.originatingIdentity, AcceptsIncomplete: true, } - return request, nil + return request, rh.inProgressProperties, nil } // preparePollServiceInstanceRequest creates a request object to be passed to // the broker client to query the given instance's last operation endpoint. func (c *controller) prepareServiceInstanceLastOperationRequest(instance *v1beta1.ServiceInstance, serviceClass *v1beta1.ClusterServiceClass, servicePlan *v1beta1.ClusterServicePlan) (*osb.LastOperationRequest, error) { - rh, err := c.prepareRequestHelper(instance, serviceClass, servicePlan) + rh, err := c.prepareRequestHelper(instance, servicePlan, false) if err != nil { return nil, err } @@ -1558,6 +1562,9 @@ func (c *controller) processProvisionFailure(instance *v1beta1.ServiceInstance, err = fmt.Errorf(failedCond.Message) } else { clearServiceInstanceCurrentOperation(instance) + // Deprovisioning is not required for provisioning that has failed with an + // error that doesn't require orphan mitigation + instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusNotRequired instance.Status.ReconciledGeneration = instance.Status.ObservedGeneration } diff --git a/pkg/controller/controller_instance_test.go b/pkg/controller/controller_instance_test.go index 6afc82cda918..293566d1ad6f 100644 --- a/pkg/controller/controller_instance_test.go +++ b/pkg/controller/controller_instance_test.go @@ -976,7 +976,7 @@ func TestReconcileServiceInstanceWithProvisionFailure(t *testing.T) { assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceRequestFailingErrorNoOrphanMitigation( + assertServiceInstanceProvisionRequestFailingErrorNoOrphanMitigation( t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, @@ -1759,7 +1759,11 @@ func TestReconcileServiceInstanceDeleteFailedProvisionWithRequest(t *testing.T) instance := getTestServiceInstanceWithFailedStatus() instance.ObjectMeta.DeletionTimestamp = &metav1.Time{} instance.ObjectMeta.Finalizers = []string{v1beta1.FinalizerServiceCatalog} - instance.Status.ExternalProperties = nil + instance.Status.CurrentOperation = v1beta1.ServiceInstanceOperationProvision + instance.Status.InProgressProperties = &v1beta1.ServiceInstancePropertiesState{ + ClusterServicePlanExternalName: testClusterServicePlanName, + ClusterServicePlanExternalID: testClusterServicePlanGUID, + } instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired instance.Generation = 2 @@ -2303,7 +2307,7 @@ func TestPollServiceInstanceFailureProvisioningWithOperation(t *testing.T) { assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceRequestFailingErrorNoOrphanMitigation( + assertServiceInstanceProvisionRequestFailingErrorNoOrphanMitigation( t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, @@ -2555,7 +2559,7 @@ func TestPollServiceInstanceFailureDeprovisioningWithReconciliationTimeout(t *te assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceRequestFailingErrorNoOrphanMitigation( + assertServiceInstanceUpdateRequestFailingErrorNoOrphanMitigation( t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, @@ -2875,7 +2879,7 @@ func TestReconcileServiceInstanceFailureOnFinalRetry(t *testing.T) { assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceRequestFailingErrorNoOrphanMitigation( + assertServiceInstanceProvisionRequestFailingErrorNoOrphanMitigation( t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, @@ -3447,6 +3451,10 @@ func TestReconcileInstanceDeleteUsingOriginatingIdentity(t *testing.T) { instance.Status.ObservedGeneration = 1 instance.Status.ProvisionStatus = v1beta1.ServiceInstanceProvisionStatusProvisioned instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired + instance.Status.ExternalProperties = &v1beta1.ServiceInstancePropertiesState{ + ClusterServicePlanExternalName: testClusterServicePlanName, + ClusterServicePlanExternalID: testClusterServicePlanGUID, + } if tc.includeUserInfo { instance.Spec.UserInfo = testUserInfo } @@ -3861,6 +3869,10 @@ func TestReconcileServiceInstanceOrphanMitigation(t *testing.T) { instance.Status.CurrentOperation = v1beta1.ServiceInstanceOperationProvision instance.Status.OrphanMitigationInProgress = true instance.Status.DeprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired + instance.Status.InProgressProperties = &v1beta1.ServiceInstancePropertiesState{ + ClusterServicePlanExternalName: testClusterServicePlanName, + ClusterServicePlanExternalID: testClusterServicePlanGUID, + } if tc.async { instance.Status.AsyncOpInProgress = true @@ -4654,7 +4666,7 @@ func TestReconcileServiceInstanceWithUpdateFailure(t *testing.T) { assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceRequestFailingErrorNoOrphanMitigation(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, errorUpdateInstanceCallFailedReason, errorUpdateInstanceCallFailedReason, instance) + assertServiceInstanceUpdateRequestFailingErrorNoOrphanMitigation(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, errorUpdateInstanceCallFailedReason, errorUpdateInstanceCallFailedReason, instance) events := getRecordedEvents(testController) @@ -5093,7 +5105,7 @@ func TestPollServiceInstanceAsyncFailureUpdating(t *testing.T) { assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceRequestFailingErrorNoOrphanMitigation(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, errorUpdateInstanceCallFailedReason, errorUpdateInstanceCallFailedReason, instance) + assertServiceInstanceUpdateRequestFailingErrorNoOrphanMitigation(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, errorUpdateInstanceCallFailedReason, errorUpdateInstanceCallFailedReason, instance) } func TestCheckClassAndPlanForDeletion(t *testing.T) { @@ -5203,7 +5215,10 @@ func TestReconcileServiceInstanceDeleteDuringOngoingOperation(t *testing.T) { instance.Status.CurrentOperation = v1beta1.ServiceInstanceOperationProvision startTime := metav1.NewTime(time.Now().Add(-1 * time.Hour)) instance.Status.OperationStartTime = &startTime - instance.Status.InProgressProperties = &v1beta1.ServiceInstancePropertiesState{} + instance.Status.InProgressProperties = &v1beta1.ServiceInstancePropertiesState{ + ClusterServicePlanExternalName: testClusterServicePlanName, + ClusterServicePlanExternalID: testClusterServicePlanGUID, + } fakeCatalogClient.AddReactor("get", "serviceinstances", func(action clientgotesting.Action) (bool, runtime.Object, error) { return true, instance, nil @@ -5281,6 +5296,10 @@ func TestReconcileServiceInstanceDeleteWithOngoingOperation(t *testing.T) { startTime := metav1.NewTime(time.Now().Add(-1 * time.Hour)) instance.Status.OperationStartTime = &startTime instance.Status.OrphanMitigationInProgress = true + instance.Status.InProgressProperties = &v1beta1.ServiceInstancePropertiesState{ + ClusterServicePlanExternalName: testClusterServicePlanName, + ClusterServicePlanExternalID: testClusterServicePlanGUID, + } fakeCatalogClient.AddReactor("get", "serviceinstances", func(action clientgotesting.Action) (bool, runtime.Object, error) { return true, instance, nil diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index ab24ac4721ba..f6ab16f8d9c5 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -695,28 +695,6 @@ func getTestServiceInstanceUpdatingPlan() *v1beta1.ServiceInstance { return instance } -func getTestServiceInstanceUpdatingParameters() *v1beta1.ServiceInstance { - instance := getTestServiceInstanceWithRefs() - instance.Generation = 2 - instance.Status = v1beta1.ServiceInstanceStatus{ - Conditions: []v1beta1.ServiceInstanceCondition{{ - Type: v1beta1.ServiceInstanceConditionReady, - Status: v1beta1.ConditionTrue, - }}, - ExternalProperties: &v1beta1.ServiceInstancePropertiesState{ - ClusterServicePlanExternalName: testClusterServicePlanName, - ClusterServicePlanExternalID: testClusterServicePlanGUID, - }, - // It's been provisioned successfully. - ReconciledGeneration: 1, - ObservedGeneration: 1, - ProvisionStatus: v1beta1.ServiceInstanceProvisionStatusProvisioned, - DeprovisionStatus: v1beta1.ServiceInstanceDeprovisionStatusRequired, - } - - return instance -} - func getTestServiceInstanceUpdatingParametersOfDeletedPlan() *v1beta1.ServiceInstance { instance := getTestServiceInstanceWithRefs() instance.Generation = 2 @@ -1150,41 +1128,6 @@ func checkPlan(plan *v1beta1.ClusterServicePlan, planID, planName, planDescripti } } -const testCatalogWithMultipleServices = `{ - "services": [ - { - "name": "service1", - "description": "service 1 description", - "metadata": { - "field1": "value1" - }, - "plans": [{ - "name": "s1plan1", - "id": "s1_plan1_id", - "description": "s1 plan1 description" - }, - { - "name": "s1plan2", - "id": "s1_plan2_id", - "description": "s1 plan2 description" - }] - }, - { - "name": "service2", - "description": "service 2 description", - "plans": [{ - "name": "s2plan1", - "id": "s2_plan1_id", - "description": "s2 plan1 description" - }, - { - "name": "s2plan2", - "id": "s2_plan2_id", - "description": "s2 plan2 description" - }] - } -]}` - // FIX: there is an inconsistency between the current broker API types re: the // Service.Metadata field. Our repo types it as `interface{}`, the go-open- // service-broker-client types it as `map[string]interface{}`. @@ -2149,13 +2092,8 @@ func assertServiceInstanceOperationInProgressWithParameters(t *testing.T, obj ru assertServiceInstanceProvisioned(t, obj, originalInstance.Status.ProvisionStatus) assertAsyncOpInProgressFalse(t, obj) assertServiceInstanceOrphanMitigationInProgressFalse(t, obj) - switch operation { - case v1beta1.ServiceInstanceOperationProvision, v1beta1.ServiceInstanceOperationUpdate: - assertServiceInstanceInProgressPropertiesPlan(t, obj, planName, planID) - assertServiceInstanceInProgressPropertiesParameters(t, obj, inProgressParameters, inProgressParametersChecksum) - case v1beta1.ServiceInstanceOperationDeprovision: - assertServiceInstanceInProgressPropertiesNil(t, obj) - } + assertServiceInstanceInProgressPropertiesPlan(t, obj, planName, planID) + assertServiceInstanceInProgressPropertiesParameters(t, obj, inProgressParameters, inProgressParametersChecksum) assertServiceInstanceExternalPropertiesUnchanged(t, obj, originalInstance) assertServiceInstanceDeprovisionStatus(t, obj, v1beta1.ServiceInstanceDeprovisionStatusRequired) } @@ -2229,7 +2167,7 @@ func assertServiceInstanceOperationSuccessWithParameters(t *testing.T, obj runti assertServiceInstanceDeprovisionStatus(t, obj, deprovisionStatus) } -func assertServiceInstanceRequestFailingError(t *testing.T, obj runtime.Object, operation v1beta1.ServiceInstanceOperation, readyReason string, failureReason string, originalInstance *v1beta1.ServiceInstance) { +func assertServiceInstanceRequestFailingError(t *testing.T, obj runtime.Object, operation v1beta1.ServiceInstanceOperation, readyReason string, failureReason string, orphanMitigationRequired bool, originalInstance *v1beta1.ServiceInstance) { var ( readyStatus v1beta1.ConditionStatus deprovisionStatus v1beta1.ServiceInstanceDeprovisionStatus @@ -2237,7 +2175,11 @@ func assertServiceInstanceRequestFailingError(t *testing.T, obj runtime.Object, switch operation { case v1beta1.ServiceInstanceOperationProvision, v1beta1.ServiceInstanceOperationUpdate: readyStatus = v1beta1.ConditionFalse - deprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired + if orphanMitigationRequired { + deprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusRequired + } else { + deprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusNotRequired + } case v1beta1.ServiceInstanceOperationDeprovision: readyStatus = v1beta1.ConditionUnknown deprovisionStatus = v1beta1.ServiceInstanceDeprovisionStatusFailed @@ -2251,8 +2193,18 @@ func assertServiceInstanceRequestFailingError(t *testing.T, obj runtime.Object, assertServiceInstanceDeprovisionStatus(t, obj, deprovisionStatus) } -func assertServiceInstanceRequestFailingErrorNoOrphanMitigation(t *testing.T, obj runtime.Object, operation v1beta1.ServiceInstanceOperation, readyReason string, failureReason string, originalInstance *v1beta1.ServiceInstance) { - assertServiceInstanceRequestFailingError(t, obj, operation, readyReason, failureReason, originalInstance) +func assertServiceInstanceProvisionRequestFailingErrorNoOrphanMitigation(t *testing.T, obj runtime.Object, operation v1beta1.ServiceInstanceOperation, readyReason string, failureReason string, originalInstance *v1beta1.ServiceInstance) { + assertServiceInstanceRequestFailingError(t, obj, operation, readyReason, failureReason, false, originalInstance) + assertServiceInstanceCurrentOperationClear(t, obj) + assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Generation) + assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) + assertServiceInstanceProvisioned(t, obj, originalInstance.Status.ProvisionStatus) + assertServiceInstanceOrphanMitigationInProgressFalse(t, obj) + assertServiceInstanceInProgressPropertiesNil(t, obj) +} + +func assertServiceInstanceUpdateRequestFailingErrorNoOrphanMitigation(t *testing.T, obj runtime.Object, operation v1beta1.ServiceInstanceOperation, readyReason string, failureReason string, originalInstance *v1beta1.ServiceInstance) { + assertServiceInstanceRequestFailingError(t, obj, operation, readyReason, failureReason, true, originalInstance) assertServiceInstanceCurrentOperationClear(t, obj) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Generation) assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) @@ -2262,7 +2214,7 @@ func assertServiceInstanceRequestFailingErrorNoOrphanMitigation(t *testing.T, ob } func assertServiceInstanceRequestFailingErrorStartOrphanMitigation(t *testing.T, obj runtime.Object, operation v1beta1.ServiceInstanceOperation, readyReason string, failureReason string, originalInstance *v1beta1.ServiceInstance) { - assertServiceInstanceRequestFailingError(t, obj, operation, readyReason, failureReason, originalInstance) + assertServiceInstanceRequestFailingError(t, obj, operation, readyReason, failureReason, true, originalInstance) assertServiceInstanceCurrentOperation(t, obj, v1beta1.ServiceInstanceOperationProvision) assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) @@ -2316,13 +2268,8 @@ func assertServiceInstanceAsyncStartInProgress(t *testing.T, obj runtime.Object, assertServiceInstanceReconciledGeneration(t, obj, originalInstance.Status.ReconciledGeneration) assertServiceInstanceObservedGeneration(t, obj, originalInstance.Generation) assertServiceInstanceProvisioned(t, obj, originalInstance.Status.ProvisionStatus) - switch operation { - case v1beta1.ServiceInstanceOperationProvision, v1beta1.ServiceInstanceOperationUpdate: - assertServiceInstanceInProgressPropertiesPlan(t, obj, planName, planID) - assertServiceInstanceInProgressPropertiesParameters(t, obj, nil, "") - case v1beta1.ServiceInstanceOperationDeprovision: - assertServiceInstanceInProgressPropertiesNil(t, obj) - } + assertServiceInstanceInProgressPropertiesPlan(t, obj, planName, planID) + assertServiceInstanceInProgressPropertiesParameters(t, obj, nil, "") assertAsyncOpInProgressTrue(t, obj) assertServiceInstanceDeprovisionStatus(t, obj, originalInstance.Status.DeprovisionStatus) }