From 07ef7431925799a80554cd8b20e42d9ecb38e431 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Luk=C5=A1a?= Date: Wed, 7 Mar 2018 06:55:46 +0100 Subject: [PATCH] Controller reconciliation rework - part 1 (ServiceInstance) (#1779) * Simplify generateChecksumOfParameters() in tests * Make the reconciliation loop exit after each update of the ServiceInstance object This prevents multiple provision requests from being sent to the broker and posting old resource versions to the API server. Fixes #1639 and #1363 * Make resolveReferences return true only when an error didn't occur * Remove unnecessary var * Don't return the updatedInstance in controller.resolveReferences() * Improve name of test helper function * Exit the reconciliation loop iteration after each update when handling ServiceInstance update and deprovision calls --- pkg/controller/controller_actions_test.go | 2 +- pkg/controller/controller_instance.go | 41 +- pkg/controller/controller_instance_test.go | 755 ++++++++++++--------- 3 files changed, 454 insertions(+), 344 deletions(-) diff --git a/pkg/controller/controller_actions_test.go b/pkg/controller/controller_actions_test.go index 681a388214f9..112b595b48d4 100644 --- a/pkg/controller/controller_actions_test.go +++ b/pkg/controller/controller_actions_test.go @@ -114,7 +114,7 @@ type instanceDescription struct { // kubernetes client func checkKubeClientActions(actual []testing.Action, expected []kubeClientAction) error { if len(actual) != len(expected) { - return fmt.Errorf("expected %d kube client actions, got %d", len(expected), len(actual)) + return fmt.Errorf("expected %d kube client actions, got %d; full action list: %v", len(expected), len(actual), actual) } for i, actualAction := range actual { expectedAction := expected[i] diff --git a/pkg/controller/controller_instance.go b/pkg/controller/controller_instance.go index dacbaae30923..bdaeb17a72d0 100644 --- a/pkg/controller/controller_instance.go +++ b/pkg/controller/controller_instance.go @@ -296,12 +296,14 @@ func (c *controller) reconcileServiceInstanceAdd(instance *v1beta1.ServiceInstan } // Update references to ClusterServicePlan / ClusterServiceClass if necessary. - // - // TODO(mkibbe): Make this trigger another reconciliation instead of continuing. - instance, err := c.resolveReferences(instance) + modified, err := c.resolveReferences(instance) if err != nil { return err } + if modified { + // resolveReferences has updated the instance, so we need to continue in the next iteration + return nil + } glog.V(4).Info(pcb.Message("Processing adding event")) @@ -328,6 +330,8 @@ func (c *controller) reconcileServiceInstanceAdd(instance *v1beta1.ServiceInstan // over with a fresh view of the instance. return err } + // recordStartOfServiceInstanceOperation has updated the instance, so we need to continue in the next iteration + return nil } glog.V(4).Info(pcb.Messagef( @@ -399,12 +403,14 @@ func (c *controller) reconcileServiceInstanceUpdate(instance *v1beta1.ServiceIns } // Update references to ClusterServicePlan / ClusterServiceClass if necessary. - // - // TODO(mkibbe): Make this trigger another reconciliation instead of continuing. - instance, err := c.resolveReferences(instance) + modified, err := c.resolveReferences(instance) if err != nil { return err } + if modified { + // resolveReferences has updated the instance, so we need to continue in the next iteration + return nil + } glog.V(4).Info(pcb.Message("Processing updating event")) @@ -431,6 +437,8 @@ func (c *controller) reconcileServiceInstanceUpdate(instance *v1beta1.ServiceIns // over with a fresh view of the instance. return err } + // recordStartOfServiceInstanceOperation has updated the instance, so we need to continue in the next iteration + return nil } glog.V(4).Info(pcb.Messagef( @@ -558,6 +566,8 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1beta1.ServiceIns // over with a fresh view of the instance. return err } + // recordStartOfServiceInstanceOperation has updated the instance, so we need to continue in the next iteration + return nil } } @@ -788,11 +798,12 @@ func (c *controller) processServiceInstancePollingFailureRetryTimeout(instance * // resolveReferences checks to see if ClusterServiceClassRef and/or ClusterServicePlanRef are // nil and if so, will resolve the references and update the instance. +// If references needed to be resolved, and the instance status was successfully updated, the method returns true // If either can not be resolved, returns an error and sets the InstanceCondition // with the appropriate error message. -func (c *controller) resolveReferences(instance *v1beta1.ServiceInstance) (*v1beta1.ServiceInstance, error) { +func (c *controller) resolveReferences(instance *v1beta1.ServiceInstance) (bool, error) { if instance.Spec.ClusterServiceClassRef != nil && instance.Spec.ClusterServicePlanRef != nil { - return instance, nil + return false, nil } var sc *v1beta1.ClusterServiceClass @@ -800,25 +811,25 @@ func (c *controller) resolveReferences(instance *v1beta1.ServiceInstance) (*v1be if instance.Spec.ClusterServiceClassRef == nil { instance, sc, err = c.resolveClusterServiceClassRef(instance) if err != nil { - return nil, err + return false, err } } if instance.Spec.ClusterServicePlanRef == nil { if sc == nil { - var scErr error - sc, scErr = c.serviceClassLister.Get(instance.Spec.ClusterServiceClassRef.Name) - if scErr != nil { - return nil, fmt.Errorf(`Couldn't find ClusterServiceClass (K8S: %s)": %v`, instance.Spec.ClusterServiceClassRef.Name, scErr.Error()) + sc, err = c.serviceClassLister.Get(instance.Spec.ClusterServiceClassRef.Name) + if err != nil { + return false, fmt.Errorf(`Couldn't find ClusterServiceClass (K8S: %s)": %v`, instance.Spec.ClusterServiceClassRef.Name, err.Error()) } } instance, err = c.resolveClusterServicePlanRef(instance, sc.Spec.ClusterServiceBrokerName) if err != nil { - return nil, err + return false, err } } - return c.updateServiceInstanceReferences(instance) + _, err = c.updateServiceInstanceReferences(instance) + return err == nil, err } // resolveClusterServiceClassRef resolves a reference to a ClusterServiceClass diff --git a/pkg/controller/controller_instance_test.go b/pkg/controller/controller_instance_test.go index fb21bac87de7..795b676fa2a9 100644 --- a/pkg/controller/controller_instance_test.go +++ b/pkg/controller/controller_instance_test.go @@ -41,6 +41,7 @@ import ( utilfeature "k8s.io/apiserver/pkg/util/feature" scfeatures "github.com/kubernetes-incubator/service-catalog/pkg/features" + "github.com/kubernetes-incubator/service-catalog/test/fake" corev1 "k8s.io/api/core/v1" clientgotesting "k8s.io/client-go/testing" ) @@ -595,6 +596,9 @@ func TestReconcileServiceInstanceWithParameters(t *testing.T) { instance.Spec.ParametersFrom = tc.paramsFrom + ////////////////////////////////////// + // Check 1st reconcilliation iteration (prepare/validate request & set status to in progress) + err := testController.reconcileServiceInstance(instance) if tc.expectedError { if err == nil { @@ -607,85 +611,100 @@ func TestReconcileServiceInstanceWithParameters(t *testing.T) { } brokerActions := fakeClusterServiceBrokerClient.Actions() - if tc.expectedError { - assertNumberOfClusterServiceBrokerActions(t, brokerActions, 0) - } else { - assertNumberOfClusterServiceBrokerActions(t, brokerActions, 1) - assertProvision(t, brokerActions[0], &osb.ProvisionRequest{ - AcceptsIncomplete: true, - InstanceID: testServiceInstanceGUID, - ServiceID: testClusterServiceClassGUID, - PlanID: testClusterServicePlanGUID, - Context: map[string]interface{}{ - "platform": "kubernetes", - "namespace": "test-ns", - }, - Parameters: tc.expectedParams, - }) + assertNumberOfClusterServiceBrokerActions(t, brokerActions, 0) + + expectedKubeActions := []kubeClientAction{ + {verb: "get", resourceName: "namespaces", checkType: checkGetActionType}, + } + for range tc.paramsFrom { + expectedKubeActions = append(expectedKubeActions, + kubeClientAction{verb: "get", resourceName: "secrets", checkType: checkGetActionType}) + } + kubeActions := fakeKubeClient.Actions() + if err := checkKubeClientActions(kubeActions, expectedKubeActions); err != nil { + t.Fatal(err) } actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 1) + updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + + events := getRecordedEvents(testController) if tc.expectedError { - assertNumberOfActions(t, actions, 1) - updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) assertServiceInstanceErrorBeforeRequest(t, updatedServiceInstance, errorWithParameters, instance) - } else { - expectedParametersChecksum, err := generateChecksumOfParameters(tc.expectedParams) - if err != nil { - t.Fatalf("Failed to generate parameters checksum: %v", err) - } - assertNumberOfActions(t, actions, 2) - - updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + expectedEvent := warningEventBuilder(errorWithParameters).msg("failed to prepare parameters") + if err := checkEventPrefixes(events, expectedEvent.stringArr()); err != nil { + t.Fatal(err) + } + } else { assertServiceInstanceOperationInProgressWithParameters(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testClusterServicePlanName, testClusterServicePlanGUID, tc.expectedParamsWithSecretsRedacted, - expectedParametersChecksum, + generateChecksumOfParametersOrFail(t, tc.expectedParams), instance, ) - updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) - assertServiceInstanceOperationSuccessWithParameters(t, - updatedServiceInstance, - v1beta1.ServiceInstanceOperationProvision, - testClusterServicePlanName, - testClusterServicePlanGUID, - tc.expectedParamsWithSecretsRedacted, - expectedParametersChecksum, - instance, - ) + if err := checkEvents(events, []string{}); err != nil { + t.Fatal(err) + } } - // verify one action comes from getting namespace uid and one - // action for each secret referenced in paramsFrom - expectedActions := []kubeClientAction{ - {verb: "get", resourceName: "namespaces", checkType: checkGetActionType}, + if tc.expectedError { + return // don't even try the 2nd iteration if the first one was supposed to return an error } - for range tc.paramsFrom { - expectedActions = append(expectedActions, - kubeClientAction{verb: "get", resourceName: "secrets", checkType: checkGetActionType}) + + ////////////////////////////////////// + // Check 2nd reconcilliation iteration (actual broker request) + + fakeCatalogClient.ClearActions() + fakeKubeClient.ClearActions() + instance = updatedServiceInstance.(*v1beta1.ServiceInstance) + + err = testController.reconcileServiceInstance(instance) + if err != nil { + t.Fatalf("Reconcile not expected to fail : %v", err) } - kubeActions := fakeKubeClient.Actions() - if err := checkKubeClientActions(kubeActions, expectedActions); err != nil { + + brokerActions = fakeClusterServiceBrokerClient.Actions() + assertNumberOfClusterServiceBrokerActions(t, brokerActions, 1) + assertProvision(t, brokerActions[0], &osb.ProvisionRequest{ + AcceptsIncomplete: true, + InstanceID: testServiceInstanceGUID, + ServiceID: testClusterServiceClassGUID, + PlanID: testClusterServicePlanGUID, + Context: map[string]interface{}{ + "platform": "kubernetes", + "namespace": "test-ns", + }, + Parameters: tc.expectedParams, + }) + + actions = fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 1) + updatedServiceInstance = assertUpdateStatus(t, actions[0], instance) + assertServiceInstanceOperationSuccessWithParameters(t, + updatedServiceInstance, + v1beta1.ServiceInstanceOperationProvision, + testClusterServicePlanName, + testClusterServicePlanGUID, + tc.expectedParamsWithSecretsRedacted, + generateChecksumOfParametersOrFail(t, tc.expectedParams), + instance, + ) + + kubeActions = fakeKubeClient.Actions() + if err := checkKubeClientActions(kubeActions, expectedKubeActions); err != nil { t.Fatal(err) } - events := getRecordedEvents(testController) - - if tc.expectedError { - expectedEvent := warningEventBuilder(errorWithParameters).msg("failed to prepare parameters") - if err := checkEventPrefixes(events, expectedEvent.stringArr()); err != nil { - t.Fatal(err) - } - } else { - expectedEvent := normalEventBuilder(successProvisionReason).msg("The instance was provisioned successfully") - if err := checkEvents(events, expectedEvent.stringArr()); err != nil { - t.Fatal(err) - } + events = getRecordedEvents(testController) + expectedEvent := normalEventBuilder(successProvisionReason).msg("The instance was provisioned successfully") + if err := checkEvents(events, expectedEvent.stringArr()); err != nil { + t.Fatal(err) } }) } @@ -725,26 +744,14 @@ func TestReconcileServiceInstanceResolvesReferences(t *testing.T) { } brokerActions := fakeClusterServiceBrokerClient.Actions() - assertNumberOfClusterServiceBrokerActions(t, brokerActions, 1) - assertProvision(t, brokerActions[0], &osb.ProvisionRequest{ - AcceptsIncomplete: true, - InstanceID: testServiceInstanceGUID, - ServiceID: testClusterServiceClassGUID, - PlanID: testClusterServicePlanGUID, - Context: map[string]interface{}{ - "platform": "kubernetes", - "namespace": "test-ns", - }, - }) + assertNumberOfClusterServiceBrokerActions(t, brokerActions, 0) // We should get the following actions: // list call for ClusterServiceClass // list call for ClusterServicePlan // setReferences on ServiceInstance - // updateStatus for inprogress - // updateStatus for success actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 5) + assertNumberOfActions(t, actions, 3) listRestrictions := clientgotesting.ListRestrictions{ Labels: labels.Everything(), @@ -771,28 +778,8 @@ func TestReconcileServiceInstanceResolvesReferences(t *testing.T) { t.Fatalf("ClusterServicePlanRef was not resolved correctly during reconcile") } - updatedServiceInstance = assertUpdateStatus(t, actions[3], instance) - assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testClusterServicePlanName, testClusterServicePlanGUID, instance) - - updatedServiceInstance = assertUpdateStatus(t, actions[4], instance) - assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testClusterServicePlanName, testClusterServicePlanGUID, instance) - - // verify no kube resources created - // One single action comes from getting namespace uid - kubeActions := fakeKubeClient.Actions() - if err := checkKubeClientActions(kubeActions, []kubeClientAction{ - {verb: "get", resourceName: "namespaces", checkType: checkGetActionType}, - }); err != nil { - t.Fatal(err) - } - - events := getRecordedEvents(testController) - assertNumEvents(t, events, 1) - - expectedEvent := normalEventBuilder(successProvisionReason).msg("The instance was provisioned successfully") - if err := checkEvents(events, expectedEvent.stringArr()); err != nil { - t.Fatal(err) - } + assertNumberOfActions(t, fakeKubeClient.Actions(), 0) + assertNumEvents(t, getRecordedEvents(testController), 0) } // TestReconcileServiceInstanceResolvesReferences tests a simple successful @@ -833,25 +820,13 @@ func TestReconcileServiceInstanceResolvesReferencesClusterServiceClassRefAlready } brokerActions := fakeServiceBrokerClient.Actions() - assertNumberOfClusterServiceBrokerActions(t, brokerActions, 1) - assertProvision(t, brokerActions[0], &osb.ProvisionRequest{ - AcceptsIncomplete: true, - InstanceID: testServiceInstanceGUID, - ServiceID: testClusterServiceClassGUID, - PlanID: testClusterServicePlanGUID, - Context: map[string]interface{}{ - "platform": "kubernetes", - "namespace": "test-ns", - }, - }) + assertNumberOfClusterServiceBrokerActions(t, brokerActions, 0) // We should get the following actions: // list call for ClusterServicePlan // setReferences on ServiceInstance - // updateStatus for inprogress - // updateStatus for success actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 4) + assertNumberOfActions(t, actions, 2) listRestrictions := clientgotesting.ListRestrictions{ Labels: labels.Everything(), @@ -872,27 +847,8 @@ func TestReconcileServiceInstanceResolvesReferencesClusterServiceClassRefAlready t.Fatalf("ClusterServicePlanRef was not resolved correctly during reconcile") } - updatedServiceInstance = assertUpdateStatus(t, actions[2], instance) - assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testClusterServicePlanName, testClusterServicePlanGUID, instance) - - updatedServiceInstance = assertUpdateStatus(t, actions[3], instance) - assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testClusterServicePlanName, testClusterServicePlanGUID, instance) - - // verify no kube resources created - // One single action comes from getting namespace uid - kubeActions := fakeKubeClient.Actions() - if err := checkKubeClientActions(kubeActions, []kubeClientAction{ - {verb: "get", resourceName: "namespaces", checkType: checkGetActionType}, - }); err != nil { - t.Fatal(err) - } - - events := getRecordedEvents(testController) - - expectedEvent := normalEventBuilder(successProvisionReason).msg("The instance was provisioned successfully") - if err := checkEvents(events, expectedEvent.stringArr()); err != nil { - t.Fatal(err) - } + assertNumberOfActions(t, fakeKubeClient.Actions(), 0) + assertNumEvents(t, getRecordedEvents(testController), 0) } // TestReconcileServiceInstanceWithProvisionCallFailure tests that when the provision @@ -911,6 +867,14 @@ func TestReconcileServiceInstanceWithProvisionCallFailure(t *testing.T) { instance := getTestServiceInstanceWithRefs() + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + instance = assertServiceInstanceProvisionInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance) + fakeCatalogClient.ClearActions() + fakeKubeClient.ClearActions() + if err := testController.reconcileServiceInstance(instance); err == nil { t.Fatalf("Should not be able to make the ServiceInstance") } @@ -938,12 +902,9 @@ func TestReconcileServiceInstanceWithProvisionCallFailure(t *testing.T) { } actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 2) + assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testClusterServicePlanName, testClusterServicePlanGUID, instance) - - updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) assertServiceInstanceRequestRetriableError(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, errorErrorCallingProvisionReason, testClusterServicePlanName, testClusterServicePlanGUID, instance) events := getRecordedEvents(testController) @@ -982,6 +943,14 @@ func TestReconcileServiceInstanceWithProvisionFailure(t *testing.T) { t.Fatalf("unexpected error: %v", err) } + instance = assertServiceInstanceProvisionInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance) + fakeCatalogClient.ClearActions() + fakeKubeClient.ClearActions() + + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + brokerActions := fakeClusterServiceBrokerClient.Actions() assertNumberOfClusterServiceBrokerActions(t, brokerActions, 1) assertProvision(t, brokerActions[0], &osb.ProvisionRequest{ @@ -1004,12 +973,9 @@ func TestReconcileServiceInstanceWithProvisionFailure(t *testing.T) { } actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 2) + assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testClusterServicePlanName, testClusterServicePlanGUID, instance) - - updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) assertServiceInstanceRequestFailingErrorNoOrphanMitigation( t, updatedServiceInstance, @@ -1053,6 +1019,16 @@ func TestReconcileServiceInstance(t *testing.T) { instance := getTestServiceInstanceWithRefs() + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + instance = assertServiceInstanceProvisionInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance) + fakeCatalogClient.ClearActions() + + assertNumberOfClusterServiceBrokerActions(t, fakeClusterServiceBrokerClient.Actions(), 0) + fakeKubeClient.ClearActions() + if err := testController.reconcileServiceInstance(instance); err != nil { t.Fatalf("This should not fail : %v", err) } @@ -1080,7 +1056,7 @@ func TestReconcileServiceInstance(t *testing.T) { } actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 2) + assertNumberOfActions(t, actions, 1) // verify no kube resources created. // One single action comes from getting namespace uid @@ -1092,9 +1068,6 @@ func TestReconcileServiceInstance(t *testing.T) { } updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testClusterServicePlanName, testClusterServicePlanGUID, instance) - - updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testClusterServicePlanName, testClusterServicePlanGUID, instance) assertServiceInstanceDashboardURL(t, updatedServiceInstance, testDashboardURL) @@ -1224,6 +1197,37 @@ func TestReconcileServiceInstanceSuccessWithK8SNames(t *testing.T) { instance := getTestServiceInstanceK8SNames() + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 1) + updatedServiceInstance := assertUpdateReference(t, actions[0], instance) + updateObject, ok := updatedServiceInstance.(*v1beta1.ServiceInstance) + if !ok { + t.Fatalf("couldn't convert to *v1beta1.ServiceInstance") + } + if updateObject.Spec.ClusterServiceClassRef == nil || updateObject.Spec.ClusterServiceClassRef.Name != "SCGUID" { + t.Fatalf("ClusterServiceClassRef was not resolved correctly during reconcile") + } + if updateObject.Spec.ClusterServicePlanRef == nil || updateObject.Spec.ClusterServicePlanRef.Name != "PGUID" { + t.Fatalf("ClusterServicePlanRef was not resolved correctly during reconcile") + } + + instance = updateObject + + fakeCatalogClient.ClearActions() + fakeKubeClient.ClearActions() + + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("This should not fail : %v", err) + } + + instance = assertServiceInstanceProvisionInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance) + fakeCatalogClient.ClearActions() + fakeKubeClient.ClearActions() + if err := testController.reconcileServiceInstance(instance); err != nil { t.Fatalf("This should not fail : %v", err) } @@ -1261,25 +1265,10 @@ func TestReconcileServiceInstanceSuccessWithK8SNames(t *testing.T) { // There are 3 actions, one to update references and update status // twice. - actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 3) - - updatedServiceInstance := assertUpdateReference(t, actions[0], instance) - updateObject, ok := updatedServiceInstance.(*v1beta1.ServiceInstance) - if !ok { - t.Fatalf("couldn't convert to *v1beta1.ServiceInstance") - } - if updateObject.Spec.ClusterServiceClassRef == nil || updateObject.Spec.ClusterServiceClassRef.Name != "SCGUID" { - t.Fatalf("ClusterServiceClassRef was not resolved correctly during reconcile") - } - if updateObject.Spec.ClusterServicePlanRef == nil || updateObject.Spec.ClusterServicePlanRef.Name != "PGUID" { - t.Fatalf("ClusterServicePlanRef was not resolved correctly during reconcile") - } - - updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) - assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testClusterServicePlanName, testClusterServicePlanGUID, instance) + actions = fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 1) - updatedServiceInstance = assertUpdateStatus(t, actions[2], instance) + updatedServiceInstance = assertUpdateStatus(t, actions[0], instance) assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testClusterServicePlanName, testClusterServicePlanGUID, instance) assertServiceInstanceDashboardURL(t, updatedServiceInstance, testDashboardURL) @@ -1313,6 +1302,14 @@ func TestReconcileServiceInstanceAsynchronous(t *testing.T) { sharedInformers.ClusterServicePlans().Informer().GetStore().Add(getTestClusterServicePlan()) instance := getTestServiceInstanceWithRefs() + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + instance = assertServiceInstanceProvisionInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance) + fakeCatalogClient.ClearActions() + fakeKubeClient.ClearActions() + instanceKey := testNamespace + "/" + testServiceInstanceName if testController.instancePollingQueue.NumRequeues(instanceKey) != 0 { @@ -1339,12 +1336,9 @@ func TestReconcileServiceInstanceAsynchronous(t *testing.T) { }) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 2) + assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testClusterServicePlanName, testClusterServicePlanGUID, instance) - - updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) assertServiceInstanceAsyncStartInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testOperation, testClusterServicePlanName, testClusterServicePlanGUID, instance) assertServiceInstanceDashboardURL(t, updatedServiceInstance, testDashboardURL) @@ -1380,6 +1374,15 @@ func TestReconcileServiceInstanceAsynchronousNoOperation(t *testing.T) { sharedInformers.ClusterServicePlans().Informer().GetStore().Add(getTestClusterServicePlan()) instance := getTestServiceInstanceWithRefs() + + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + instance = assertServiceInstanceProvisionInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance) + fakeCatalogClient.ClearActions() + fakeKubeClient.ClearActions() + instanceKey := testNamespace + "/" + testServiceInstanceName if testController.instancePollingQueue.NumRequeues(instanceKey) != 0 { @@ -1406,12 +1409,9 @@ func TestReconcileServiceInstanceAsynchronousNoOperation(t *testing.T) { }) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 2) + assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testClusterServicePlanName, testClusterServicePlanGUID, instance) - - updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) assertServiceInstanceAsyncStartInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, "", testClusterServicePlanName, testClusterServicePlanGUID, instance) assertServiceInstanceDashboardURL(t, updatedServiceInstance, testDashboardURL) @@ -1506,6 +1506,13 @@ func TestReconcileServiceInstanceDelete(t *testing.T) { return true, instance, nil }) + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + instance = assertServiceInstanceDeprovisionInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance) + fakeCatalogClient.ClearActions() + fakeKubeClient.ClearActions() + err := testController.reconcileServiceInstance(instance) if err != nil { t.Fatalf("This should not fail") @@ -1525,12 +1532,9 @@ func TestReconcileServiceInstanceDelete(t *testing.T) { assertNumberOfActions(t, kubeActions, 0) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 2) + assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) - - updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) events := getRecordedEvents(testController) @@ -1605,13 +1609,19 @@ func TestReconcileServiceInstanceDeleteBlockedByCredentials(t *testing.T) { // delete credentials sharedInformers.ServiceBindings().Informer().GetStore().Delete(credentials) - - fakeKubeClient.ClearActions() fakeCatalogClient.ClearActions() + fakeKubeClient.ClearActions() // credentials were removed, verify the next reconcilation removes // the instance + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + instance = assertServiceInstanceDeprovisionInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance) + fakeCatalogClient.ClearActions() + fakeKubeClient.ClearActions() + if err := testController.reconcileServiceInstance(instance); err != nil { t.Fatalf("This should not fail : %v", err) } @@ -1634,12 +1644,9 @@ func TestReconcileServiceInstanceDeleteBlockedByCredentials(t *testing.T) { // The actions should be: // 0. Updating the current operation // 1. Updating the ready condition - assertNumberOfActions(t, actions, 2) + assertNumberOfActions(t, actions, 1) updatedServiceInstance = assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) - - updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) events = getRecordedEvents(testController) @@ -1690,6 +1697,13 @@ func TestReconcileServiceInstanceDeleteAsynchronous(t *testing.T) { t.Fatalf("Expected polling queue to not have any record of test instance") } + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + instance = assertServiceInstanceDeprovisionInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance) + fakeCatalogClient.ClearActions() + fakeKubeClient.ClearActions() + err := testController.reconcileServiceInstance(instance) if err != nil { t.Fatalf("This should not fail : %v", err) @@ -1715,12 +1729,9 @@ func TestReconcileServiceInstanceDeleteAsynchronous(t *testing.T) { assertNumberOfActions(t, kubeActions, 0) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 2) + assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) - - updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) assertServiceInstanceAsyncStartInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testOperation, testClusterServicePlanName, testClusterServicePlanGUID, instance) events := getRecordedEvents(testController) @@ -1760,6 +1771,13 @@ func TestReconcileServiceInstanceDeleteFailedProvisionWithRequest(t *testing.T) return true, instance, nil }) + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + instance = assertServiceInstanceDeprovisionInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance) + fakeCatalogClient.ClearActions() + fakeKubeClient.ClearActions() + err := testController.reconcileServiceInstance(instance) if err != nil { t.Fatalf("Unexpected error from reconcileServiceInstance: %v", err) @@ -1779,12 +1797,9 @@ func TestReconcileServiceInstanceDeleteFailedProvisionWithRequest(t *testing.T) assertNumberOfActions(t, kubeActions, 0) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 2) + assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) - - updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) events := getRecordedEvents(testController) @@ -1967,6 +1982,13 @@ func TestReconcileServiceInstanceDeleteFailedUpdate(t *testing.T) { return true, instance, nil }) + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + instance = assertServiceInstanceDeprovisionInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance) + fakeCatalogClient.ClearActions() + fakeKubeClient.ClearActions() + err := testController.reconcileServiceInstance(instance) if err != nil { t.Fatalf("Unexpected error from reconcileServiceInstance: %v", err) @@ -1986,12 +2008,9 @@ func TestReconcileServiceInstanceDeleteFailedUpdate(t *testing.T) { assertNumberOfActions(t, kubeActions, 0) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 2) + assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) - - updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) events := getRecordedEvents(testController) @@ -3020,11 +3039,7 @@ func TestReconcileServiceInstanceWithStatusUpdateError(t *testing.T) { brokerActions := fakeClusterServiceBrokerClient.Actions() assertNumberOfClusterServiceBrokerActions(t, brokerActions, 0) - actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 1) - - updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testClusterServicePlanName, testClusterServicePlanGUID, instance) + instance = assertServiceInstanceProvisionInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance) events := getRecordedEvents(testController) assertNumEvents(t, events, 0) @@ -3351,7 +3366,7 @@ func TestReconcileInstanceUsingOriginatingIdentity(t *testing.T) { defer utilfeature.DefaultFeatureGate.Set(fmt.Sprintf("%v=false", scfeatures.OriginatingIdentity)) } - fakeKubeClient, _, fakeBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + fakeKubeClient, fakeCatalogClient, fakeBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ ProvisionReaction: &fakeosb.ProvisionReaction{ Response: &osb.ProvisionResponse{ DashboardURL: &testDashboardURL, @@ -3370,6 +3385,16 @@ func TestReconcileInstanceUsingOriginatingIdentity(t *testing.T) { instance.Spec.UserInfo = testUserInfo } + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 1) + updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + + instance = updatedServiceInstance.(*v1beta1.ServiceInstance) + if err := testController.reconcileServiceInstance(instance); err != nil { t.Fatalf("This should not fail : %v", err) } @@ -3426,6 +3451,13 @@ func TestReconcileInstanceDeleteUsingOriginatingIdentity(t *testing.T) { return true, instance, nil }) + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + instance = assertServiceInstanceDeprovisionInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance) + fakeCatalogClient.ClearActions() + err := testController.reconcileServiceInstance(instance) if err != nil { t.Fatalf("This should not fail") @@ -3546,17 +3578,23 @@ func TestReconcileServiceInstanceWithHTTPStatusCodeErrorOrphanMitigation(t *test sharedInformers.ClusterServicePlans().Informer().GetStore().Add(getTestClusterServicePlan()) instance := getTestServiceInstanceWithRefs() + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + instance = assertServiceInstanceProvisionInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance) + fakeCatalogClient.ClearActions() err := testController.reconcileServiceInstance(instance) // The action should be: // 0. Updating the status actions := fakeCatalogClient.Actions() - if ok := expectNumberOfActions(t, tc.name, actions, 2); !ok { + if ok := expectNumberOfActions(t, tc.name, actions, 1); !ok { continue } - updatedObject, ok := expectUpdateStatus(t, tc.name, actions[1], instance) + updatedObject, ok := expectUpdateStatus(t, tc.name, actions[0], instance) if !ok { continue } @@ -3596,15 +3634,21 @@ func TestReconcileServiceInstanceTimeoutTriggersOrphanMitigation(t *testing.T) { sharedInformers.ClusterServicePlans().Informer().GetStore().Add(getTestClusterServicePlan()) instance := getTestServiceInstanceWithRefs() + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + instance = assertServiceInstanceProvisionInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance) + fakeCatalogClient.ClearActions() if err := testController.reconcileServiceInstance(instance); err == nil { t.Fatal("Reconciler should return error for timeout so that instance is orphan mitigated") } actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 2) + assertNumberOfActions(t, actions, 1) - updatedObject := assertUpdateStatus(t, actions[1], instance) + updatedObject := assertUpdateStatus(t, actions[0], instance) updatedServiceInstance, ok := updatedObject.(*v1beta1.ServiceInstance) if !ok { fatalf(t, "Couldn't convert object %+v into a *v1beta1.ServiceInstance", updatedObject) @@ -3913,6 +3957,27 @@ func TestReconcileServiceInstanceWithSecretParameters(t *testing.T) { }, } + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + expectedParameters := map[string]interface{}{ + "a": "1", + "b": "", + } + expectedParametersChecksum := generateChecksumOfParametersOrFail(t, map[string]interface{}{ + "a": "1", + "b": "2", + }) + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 1) + updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + assertServiceInstanceOperationInProgressWithParameters(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testClusterServicePlanName, testClusterServicePlanGUID, expectedParameters, expectedParametersChecksum, instance) + + instance = updatedServiceInstance.(*v1beta1.ServiceInstance) + fakeCatalogClient.ClearActions() + fakeKubeClient.ClearActions() + if err = testController.reconcileServiceInstance(instance); err != nil { t.Fatalf("This should not fail : %v", err) } @@ -3934,25 +3999,10 @@ func TestReconcileServiceInstanceWithSecretParameters(t *testing.T) { }, }) - expectedParameters := map[string]interface{}{ - "a": "1", - "b": "", - } - expectedParametersChecksum, err := generateChecksumOfParameters(map[string]interface{}{ - "a": "1", - "b": "2", - }) - if err != nil { - t.Fatalf("Failed to generate parameters checksum: %v", err) - } - - actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 2) - - updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceOperationInProgressWithParameters(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testClusterServicePlanName, testClusterServicePlanGUID, expectedParameters, expectedParametersChecksum, instance) + actions = fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 1) - updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) + updatedServiceInstance = assertUpdateStatus(t, actions[0], instance) assertServiceInstanceOperationSuccessWithParameters(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationProvision, testClusterServicePlanName, testClusterServicePlanGUID, expectedParameters, expectedParametersChecksum, instance) updateObject, ok := updatedServiceInstance.(*v1beta1.ServiceInstance) @@ -3989,12 +4039,13 @@ func TestReconcileServiceInstanceWithSecretParameters(t *testing.T) { func TestResolveReferencesReferencesAlreadySet(t *testing.T) { fakeKubeClient, fakeCatalogClient, _, testController, _ := newTestController(t, noFakeActions()) instance := getTestServiceInstanceWithRefs() - updatedInstance, err := testController.resolveReferences(instance) + modified, err := testController.resolveReferences(instance) if err != nil { t.Fatalf("resolveReferences failed unexpectedly: %q", err) } - if e, a := instance, updatedInstance; !reflect.DeepEqual(instance, updatedInstance) { - t.Fatalf("Instance was modified, expected\n%v\nGot\n%v", e, a) + + if modified { + t.Fatalf("Should have returned false") } // No kube actions @@ -4013,7 +4064,7 @@ func TestResolveReferencesNoClusterServiceClass(t *testing.T) { instance := getTestServiceInstance() - updatedInstance, err := testController.resolveReferences(instance) + modified, err := testController.resolveReferences(instance) if err == nil { t.Fatalf("Should have failed with no service class") } @@ -4021,8 +4072,9 @@ func TestResolveReferencesNoClusterServiceClass(t *testing.T) { if e, a := "a non-existent ClusterServiceClass", err.Error(); !strings.Contains(a, e) { t.Fatalf("Did not get the expected error message %q got %q", e, a) } - if updatedInstance != nil { - t.Fatalf("updatedInstance retuend was non-nil: %+v", updatedInstance) + + if modified { + t.Fatalf("Should have returned false") } // We should get the following actions: @@ -4098,16 +4150,11 @@ func TestReconcileServiceInstanceUpdateParameters(t *testing.T) { Raw: oldParametersMarshaled, } - oldParametersChecksum, err := generateChecksumOfParameters(oldParameters) - if err != nil { - t.Fatalf("Failed to generate parameters checksum: %v", err) - } - instance.Status.ExternalProperties = &v1beta1.ServiceInstancePropertiesState{ ClusterServicePlanExternalName: testClusterServicePlanName, ClusterServicePlanExternalID: testClusterServicePlanGUID, Parameters: oldParametersRaw, - ParametersChecksum: oldParametersChecksum, + ParametersChecksum: generateChecksumOfParametersOrFail(t, oldParameters), } parameters := instanceParameters{Name: "test-param", Args: make(map[string]string)} @@ -4120,6 +4167,23 @@ func TestReconcileServiceInstanceUpdateParameters(t *testing.T) { } instance.Spec.Parameters = &runtime.RawExtension{Raw: b} + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + expectedParameters := map[string]interface{}{ + "args": map[string]interface{}{ + "first": "first-arg", + "second": "new-second-arg", + }, + "name": "test-param", + } + expectedParametersChecksum := generateChecksumOfParametersOrFail(t, expectedParameters) + + instance = assertServiceInstanceOperationInProgressWithParametersIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance, v1beta1.ServiceInstanceOperationUpdate, testClusterServicePlanName, testClusterServicePlanGUID, expectedParameters, expectedParametersChecksum) + fakeCatalogClient.ClearActions() + fakeKubeClient.ClearActions() + if err = testController.reconcileServiceInstance(instance); err != nil { t.Fatalf("This should not fail : %v", err) } @@ -4144,25 +4208,10 @@ func TestReconcileServiceInstanceUpdateParameters(t *testing.T) { }, }) - expectedParameters := map[string]interface{}{ - "args": map[string]interface{}{ - "first": "first-arg", - "second": "new-second-arg", - }, - "name": "test-param", - } - expectedParametersChecksum, err := generateChecksumOfParameters(expectedParameters) - if err != nil { - t.Fatalf("Failed to generate parameters checksum: %v", err) - } - actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 2) + assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceOperationInProgressWithParameters(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, testClusterServicePlanName, testClusterServicePlanGUID, expectedParameters, expectedParametersChecksum, instance) - - updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) assertServiceInstanceOperationSuccessWithParameters(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, testClusterServicePlanName, testClusterServicePlanGUID, expectedParameters, expectedParametersChecksum, instance) updateObject, ok := updatedServiceInstance.(*v1beta1.ServiceInstance) @@ -4227,17 +4276,19 @@ func TestReconcileServiceInstanceDeleteParameters(t *testing.T) { Raw: oldParametersMarshaled, } - oldParametersChecksum, err := generateChecksumOfParameters(oldParameters) - if err != nil { - t.Fatalf("Failed to generate parameters checksum: %v", err) - } - instance.Status.ExternalProperties = &v1beta1.ServiceInstancePropertiesState{ ClusterServicePlanExternalName: testClusterServicePlanName, ClusterServicePlanExternalID: testClusterServicePlanGUID, Parameters: oldParametersRaw, - ParametersChecksum: oldParametersChecksum, + ParametersChecksum: generateChecksumOfParametersOrFail(t, oldParameters), + } + + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) } + instance = assertServiceInstanceUpdateInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance) + fakeCatalogClient.ClearActions() + fakeKubeClient.ClearActions() if err = testController.reconcileServiceInstance(instance); err != nil { t.Fatalf("This should not fail : %v", err) @@ -4258,12 +4309,9 @@ func TestReconcileServiceInstanceDeleteParameters(t *testing.T) { }) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 2) + assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, testClusterServicePlanName, testClusterServicePlanGUID, instance) - - updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, testClusterServicePlanName, testClusterServicePlanGUID, instance) updateObject, ok := updatedServiceInstance.(*v1beta1.ServiceInstance) @@ -4306,7 +4354,7 @@ func TestResolveReferencesNoClusterServicePlan(t *testing.T) { return true, &v1beta1.ClusterServiceClassList{Items: scItems}, nil }) - updatedInstance, err := testController.resolveReferences(instance) + modified, err := testController.resolveReferences(instance) if err == nil { t.Fatalf("Should have failed with no service plan") } @@ -4315,8 +4363,8 @@ func TestResolveReferencesNoClusterServicePlan(t *testing.T) { t.Fatalf("Did not get the expected error message %q got %q", e, a) } - if updatedInstance != nil { - t.Fatalf("updatedInstance retuend was non-nil: %+v", updatedInstance) + if modified { + t.Fatalf("Should have returned false") } // We should get the following actions: @@ -4402,10 +4450,7 @@ func TestReconcileServiceInstanceUpdatePlan(t *testing.T) { Raw: oldParametersMarshaled, } - oldParametersChecksum, err := generateChecksumOfParameters(oldParameters) - if err != nil { - t.Fatalf("Failed to generate parameters checksum: %v", err) - } + oldParametersChecksum := generateChecksumOfParametersOrFail(t, oldParameters) instance.Status.ExternalProperties = &v1beta1.ServiceInstancePropertiesState{ ClusterServicePlanExternalName: "old-plan-name", @@ -4424,6 +4469,14 @@ func TestReconcileServiceInstanceUpdatePlan(t *testing.T) { } instance.Spec.Parameters = &runtime.RawExtension{Raw: b} + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + instance = assertServiceInstanceOperationInProgressWithParametersIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance, v1beta1.ServiceInstanceOperationUpdate, testClusterServicePlanName, testClusterServicePlanGUID, oldParameters, oldParametersChecksum) + fakeCatalogClient.ClearActions() + fakeKubeClient.ClearActions() + if err = testController.reconcileServiceInstance(instance); err != nil { t.Fatalf("This should not fail : %v", err) } @@ -4444,12 +4497,9 @@ func TestReconcileServiceInstanceUpdatePlan(t *testing.T) { }) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 2) + assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceOperationInProgressWithParameters(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, testClusterServicePlanName, testClusterServicePlanGUID, oldParameters, oldParametersChecksum, instance) - - updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) assertServiceInstanceOperationSuccessWithParameters(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, testClusterServicePlanName, testClusterServicePlanGUID, oldParameters, oldParametersChecksum, instance) updateObject, ok := updatedServiceInstance.(*v1beta1.ServiceInstance) @@ -4495,6 +4545,13 @@ func TestReconcileServiceInstanceWithUpdateCallFailure(t *testing.T) { instance := getTestServiceInstanceUpdatingPlan() + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + instance = assertServiceInstanceUpdateInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance) + fakeCatalogClient.ClearActions() + fakeKubeClient.ClearActions() + if err := testController.reconcileServiceInstance(instance); err == nil { t.Fatalf("Should not be able to make the ServiceInstance.") } @@ -4523,12 +4580,9 @@ func TestReconcileServiceInstanceWithUpdateCallFailure(t *testing.T) { } actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 2) + assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, testClusterServicePlanName, testClusterServicePlanGUID, instance) - - updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) assertServiceInstanceRequestRetriableError(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, errorErrorCallingUpdateInstanceReason, testClusterServicePlanName, testClusterServicePlanGUID, instance) events := getRecordedEvents(testController) @@ -4559,6 +4613,13 @@ func TestReconcileServiceInstanceWithUpdateFailure(t *testing.T) { instance := getTestServiceInstanceUpdatingPlan() + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + instance = assertServiceInstanceUpdateInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance) + fakeCatalogClient.ClearActions() + fakeKubeClient.ClearActions() + if err := testController.reconcileServiceInstance(instance); err != nil { t.Fatalf("unexpected error: %v", err) } @@ -4586,12 +4647,9 @@ func TestReconcileServiceInstanceWithUpdateFailure(t *testing.T) { } actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 2) + assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, testClusterServicePlanName, testClusterServicePlanGUID, instance) - - updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) assertServiceInstanceRequestFailingErrorNoOrphanMitigation(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, errorUpdateInstanceCallFailedReason, errorUpdateInstanceCallFailedReason, instance) events := getRecordedEvents(testController) @@ -4622,17 +4680,13 @@ func TestResolveReferencesWorks(t *testing.T) { return true, &v1beta1.ClusterServicePlanList{Items: spItems}, nil }) - updatedInstance, err := testController.resolveReferences(instance) + modified, err := testController.resolveReferences(instance) if err != nil { t.Fatalf("Should not have failed, but failed with: %q", err) } - if updatedInstance.Spec.ClusterServiceClassRef == nil || updatedInstance.Spec.ClusterServiceClassRef.Name != testClusterServiceClassGUID { - t.Fatalf("Did not find expected ClusterServiceClassRef, expected %q got %+v", testClusterServiceClassGUID, updatedInstance.Spec.ClusterServiceClassRef) - } - - if updatedInstance.Spec.ClusterServicePlanRef == nil || updatedInstance.Spec.ClusterServicePlanRef.Name != testClusterServicePlanGUID { - t.Fatalf("Did not find expected ClusterServicePlanRef, expected %q got %+v", testClusterServicePlanGUID, updatedInstance.Spec.ClusterServicePlanRef.Name) + if !modified { + t.Fatalf("Should have returned true") } // We should get the following actions: @@ -4704,17 +4758,13 @@ func TestResolveReferencesForPlanChange(t *testing.T) { instance.Spec.ClusterServicePlanExternalName = newPlanName instance.Spec.ClusterServicePlanRef = nil - updatedInstance, err := testController.resolveReferences(instance) + modified, err := testController.resolveReferences(instance) if err != nil { t.Fatalf("Should not have failed, but failed with: %q", err) } - if updatedInstance.Spec.ClusterServiceClassRef == nil || updatedInstance.Spec.ClusterServiceClassRef.Name != testClusterServiceClassGUID { - t.Fatalf("Did not find expected ClusterServiceClassRef, expected %q got %+v", testClusterServiceClassGUID, updatedInstance.Spec.ClusterServiceClassRef) - } - - if updatedInstance.Spec.ClusterServicePlanRef == nil || updatedInstance.Spec.ClusterServicePlanRef.Name != newPlanID { - t.Fatalf("Did not find expected ClusterServicePlanRef, expected %q got %+v", newPlanID, updatedInstance.Spec.ClusterServicePlanRef.Name) + if !modified { + t.Fatalf("Should have returned true") } // We should get the following actions: @@ -4761,17 +4811,13 @@ func TestResolveReferencesWorksK8SNames(t *testing.T) { instance := getTestServiceInstanceK8SNames() - updatedInstance, err := testController.resolveReferences(instance) + modified, err := testController.resolveReferences(instance) if err != nil { t.Fatalf("Should not have failed, but failed with: %q", err) } - if updatedInstance.Spec.ClusterServiceClassRef == nil || updatedInstance.Spec.ClusterServiceClassRef.Name != testClusterServiceClassGUID { - t.Fatalf("Did not find expected ClusterServiceClassRef, expected %q got %+v", testClusterServiceClassGUID, updatedInstance.Spec.ClusterServiceClassRef) - } - - if updatedInstance.Spec.ClusterServicePlanRef == nil || updatedInstance.Spec.ClusterServicePlanRef.Name != testClusterServicePlanGUID { - t.Fatalf("Did not find expected ClusterServicePlanRef, expected %q got %+v", testClusterServicePlanGUID, updatedInstance.Spec.ClusterServicePlanRef.Name) + if !modified { + t.Fatalf("Should have returned true") } // We should get the following actions: @@ -4837,6 +4883,13 @@ func TestReconcileServiceInstanceUpdateAsynchronous(t *testing.T) { t.Fatalf("Expected polling queue to not have any record of test instance") } + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + instance = assertServiceInstanceUpdateInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance) + fakeCatalogClient.ClearActions() + fakeKubeClient.ClearActions() + if err := testController.reconcileServiceInstance(instance); err != nil { t.Fatalf("This should not fail : %v", err) } @@ -4856,12 +4909,9 @@ func TestReconcileServiceInstanceUpdateAsynchronous(t *testing.T) { }) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 2) + assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, testClusterServicePlanName, testClusterServicePlanGUID, instance) - - updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) assertServiceInstanceAsyncStartInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationUpdate, testOperation, testClusterServicePlanName, testClusterServicePlanGUID, instance) // verify no kube resources created. @@ -5157,6 +5207,24 @@ func TestReconcileServiceInstanceDeleteDuringOngoingOperation(t *testing.T) { timeOfReconciliation := metav1.Now() + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + instance = assertServiceInstanceDeprovisionInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance) + + // Verify that the operation start time was reset to Now + if instance.Status.OperationStartTime.Before(&timeOfReconciliation) { + t.Fatalf( + "OperationStartTime should not be before the time that the reconciliation started. OperationStartTime=%v. timeOfReconciliation=%v", + instance.Status.OperationStartTime, + timeOfReconciliation, + ) + } + + fakeCatalogClient.ClearActions() + fakeKubeClient.ClearActions() + err := testController.reconcileServiceInstance(instance) if err != nil { t.Fatalf("This should not fail") @@ -5176,21 +5244,9 @@ func TestReconcileServiceInstanceDeleteDuringOngoingOperation(t *testing.T) { assertNumberOfActions(t, kubeActions, 0) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 2) + assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance).(*v1beta1.ServiceInstance) - assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) - - // Verify that the operation start time was reset to Now - if updatedServiceInstance.Status.OperationStartTime.Before(&timeOfReconciliation) { - t.Fatalf( - "OperationStartTime should not be before the time that the reconciliation started. OperationStartTime=%v. timeOfReconciliation=%v", - updatedServiceInstance.Status.OperationStartTime, - timeOfReconciliation, - ) - } - - updatedServiceInstance = assertUpdateStatus(t, actions[1], instance).(*v1beta1.ServiceInstance) assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) events := getRecordedEvents(testController) @@ -5228,6 +5284,24 @@ func TestReconcileServiceInstanceDeleteWithOngoingOperation(t *testing.T) { timeOfReconciliation := metav1.Now() + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + instance = assertServiceInstanceDeprovisionInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance) + + // Verify that the operation start time was reset to Now + if instance.Status.OperationStartTime.Before(&timeOfReconciliation) { + t.Fatalf( + "OperationStartTime should not be before the time that the reconciliation started. OperationStartTime=%v. timeOfReconciliation=%v", + instance.Status.OperationStartTime, + timeOfReconciliation, + ) + } + + fakeCatalogClient.ClearActions() + fakeKubeClient.ClearActions() + err := testController.reconcileServiceInstance(instance) if err != nil { t.Fatalf("This should not fail") @@ -5247,21 +5321,9 @@ func TestReconcileServiceInstanceDeleteWithOngoingOperation(t *testing.T) { assertNumberOfActions(t, kubeActions, 0) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 2) + assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance).(*v1beta1.ServiceInstance) - assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) - - // Verify that the operation start time was reset to Now - if updatedServiceInstance.Status.OperationStartTime.Before(&timeOfReconciliation) { - t.Fatalf( - "OperationStartTime should not be before the time that the reconciliation started. OperationStartTime=%v. timeOfReconciliation=%v", - updatedServiceInstance.Status.OperationStartTime, - timeOfReconciliation, - ) - } - - updatedServiceInstance = assertUpdateStatus(t, actions[1], instance).(*v1beta1.ServiceInstance) assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) events := getRecordedEvents(testController) @@ -5305,6 +5367,14 @@ func TestReconcileServiceInstanceDeleteWithNonExistentPlan(t *testing.T) { return true, instance, nil }) + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + instance = assertServiceInstanceOperationInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance, v1beta1.ServiceInstanceOperationDeprovision, "old-plan-name", "old-plan-id") + fakeCatalogClient.ClearActions() + fakeKubeClient.ClearActions() + err := testController.reconcileServiceInstance(instance) if err != nil { t.Fatalf("This should not fail") @@ -5324,12 +5394,9 @@ func TestReconcileServiceInstanceDeleteWithNonExistentPlan(t *testing.T) { assertNumberOfActions(t, kubeActions, 0) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 2) + assertNumberOfActions(t, actions, 1) updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) - assertServiceInstanceOperationInProgress(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, "old-plan-name", "old-plan-id", instance) - - updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, "old-plan-name", "old-plan-id", instance) events := getRecordedEvents(testController) @@ -5385,3 +5452,35 @@ func TestReconcileServiceInstanceUpdateMissingObservedGeneration(t *testing.T) { t.Fatalf("The instance was expected to be marked as Provisioned") } } + +func generateChecksumOfParametersOrFail(t *testing.T, params map[string]interface{}) string { + expectedParametersChecksum, err := generateChecksumOfParameters(params) + if err != nil { + t.Fatalf("Failed to generate parameters checksum: %v", err) + } + return expectedParametersChecksum +} + +func assertServiceInstanceProvisionInProgressIsTheOnlyCatalogClientAction(t *testing.T, fakeCatalogClient *fake.Clientset, instance *v1beta1.ServiceInstance) *v1beta1.ServiceInstance { + return assertServiceInstanceOperationInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance, v1beta1.ServiceInstanceOperationProvision, testClusterServicePlanName, testClusterServicePlanGUID) +} + +func assertServiceInstanceUpdateInProgressIsTheOnlyCatalogClientAction(t *testing.T, fakeCatalogClient *fake.Clientset, instance *v1beta1.ServiceInstance) *v1beta1.ServiceInstance { + return assertServiceInstanceOperationInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance, v1beta1.ServiceInstanceOperationUpdate, testClusterServicePlanName, testClusterServicePlanGUID) +} + +func assertServiceInstanceDeprovisionInProgressIsTheOnlyCatalogClientAction(t *testing.T, fakeCatalogClient *fake.Clientset, instance *v1beta1.ServiceInstance) *v1beta1.ServiceInstance { + return assertServiceInstanceOperationInProgressIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID) +} + +func assertServiceInstanceOperationInProgressIsTheOnlyCatalogClientAction(t *testing.T, fakeCatalogClient *fake.Clientset, instance *v1beta1.ServiceInstance, operation v1beta1.ServiceInstanceOperation, planName string, planGUID string) *v1beta1.ServiceInstance { + return assertServiceInstanceOperationInProgressWithParametersIsTheOnlyCatalogClientAction(t, fakeCatalogClient, instance, operation, planName, planGUID, nil, "") +} + +func assertServiceInstanceOperationInProgressWithParametersIsTheOnlyCatalogClientAction(t *testing.T, fakeCatalogClient *fake.Clientset, instance *v1beta1.ServiceInstance, operation v1beta1.ServiceInstanceOperation, planName string, planGUID string, parameters map[string]interface{}, parametersChecksum string) *v1beta1.ServiceInstance { + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 1) + updateObject := assertUpdateStatus(t, actions[0], instance) + assertServiceInstanceOperationInProgressWithParameters(t, updateObject, operation, planName, planGUID, parameters, parametersChecksum, instance) + return updateObject.(*v1beta1.ServiceInstance) +}