diff --git a/charts/catalog/templates/rbac.yaml b/charts/catalog/templates/rbac.yaml index 87e45d568ed..cdafcf24a63 100644 --- a/charts/catalog/templates/rbac.yaml +++ b/charts/catalog/templates/rbac.yaml @@ -35,7 +35,7 @@ items: verbs: ["get","list","watch","create","patch","update","delete"] - apiGroups: ["servicecatalog.k8s.io"] resources: ["clusterservicebrokers"] - verbs: ["get","list","watch"] + verbs: ["get","list","watch", "update"] - apiGroups: ["servicecatalog.k8s.io"] resources: ["serviceinstances","servicebindings"] verbs: ["get","list","watch", "update", "patch"] @@ -51,7 +51,7 @@ items: verbs: ["get","list","watch","create","patch","update","delete"] - apiGroups: ["servicecatalog.k8s.io"] resources: ["servicebrokers"] - verbs: ["get","list","watch"] + verbs: ["get","list","watch", "update"] - apiGroups: ["servicecatalog.k8s.io"] resources: ["servicebrokers/status","serviceclasses/status","serviceplans/status"] verbs: ["update"] diff --git a/charts/catalog/values.yaml b/charts/catalog/values.yaml index 59bb6a75188..85340015894 100644 --- a/charts/catalog/values.yaml +++ b/charts/catalog/values.yaml @@ -1,6 +1,6 @@ # Default values for Service Catalog # service-catalog image to use -image: eu.gcr.io/kyma-project/develop/service-catalog/service-catalog-amd64:crd-0.0.2 +image: eu.gcr.io/kyma-project/develop/service-catalog/service-catalog-amd64:crd-0.0.3 # imagePullPolicy for the service-catalog; valid values are "IfNotPresent", # "Never", and "Always" imagePullPolicy: Always diff --git a/pkg/controller/controller_binding.go b/pkg/controller/controller_binding.go index e619627dd9e..2d96e3ebd58 100644 --- a/pkg/controller/controller_binding.go +++ b/pkg/controller/controller_binding.go @@ -1516,15 +1516,23 @@ func (c *controller) handleServiceBindingReconciliationError(binding *v1beta1.Se // updating of a ServiceBinding that has successfully finished graceful // deletion. func (c *controller) processServiceBindingGracefulDeletionSuccess(binding *v1beta1.ServiceBinding) error { - finalizers := sets.NewString(binding.Finalizers...) - finalizers.Delete(v1beta1.FinalizerServiceCatalog) - binding.Finalizers = finalizers.List() + pcb := pretty.NewBindingContextBuilder(binding) - if _, err := c.updateServiceBindingStatus(binding); err != nil { - return err + updatedBinding, err := c.updateServiceBindingStatus(binding) + if err != nil { + return fmt.Errorf("while updating status: %v", err) } + klog.Info(pcb.Message("Status updated")) - pcb := pretty.NewBindingContextBuilder(binding) + toUpdate := updatedBinding.DeepCopy() + finalizers := sets.NewString(toUpdate.Finalizers...) + finalizers.Delete(v1beta1.FinalizerServiceCatalog) + toUpdate.Finalizers = finalizers.List() + + _, err = c.serviceCatalogClient.ServiceBindings(toUpdate.Namespace).Update(toUpdate) + if err != nil { + return fmt.Errorf("while removing finalizer entry: %v", err) + } klog.Info(pcb.Message("Cleared finalizer")) return nil diff --git a/pkg/controller/controller_binding_ns_test.go b/pkg/controller/controller_binding_ns_test.go index 7ff7a50b413..ee44f04b76d 100644 --- a/pkg/controller/controller_binding_ns_test.go +++ b/pkg/controller/controller_binding_ns_test.go @@ -380,10 +380,13 @@ func TestReconcileServiceBindingDeleteNamespacedRefs(t *testing.T) { } actions := fakeCatalogClient.Actions() - // The action should be updating the ready condition - assertNumberOfActions(t, actions, 1) + // The actions should be: + // 0. Updating the ready condition + // 1. Removing finalizer + assertNumberOfActions(t, actions, 2) - updatedServiceBinding := assertUpdateStatus(t, actions[0], binding) + assertUpdateStatus(t, actions[0], binding) + updatedServiceBinding := assertUpdate(t, actions[1], binding) assertServiceBindingOperationSuccess(t, updatedServiceBinding, v1beta1.ServiceBindingOperationUnbind, binding) assertServiceBindingOrphanMitigationSet(t, updatedServiceBinding, false) @@ -439,17 +442,17 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { } cases := []struct { - name string - binding *v1beta1.ServiceBinding - pollReaction *fakeosb.PollBindingLastOperationReaction - getBindingReaction *fakeosb.GetBindingReaction - environmentSetupFunc func(t *testing.T, fakeKubeClient *clientgofake.Clientset, sharedInformers v1beta1informers.Interface) - validateBrokerActionsFunc func(t *testing.T, actions []fakeosb.Action) - validateKubeActionsFunc func(t *testing.T, actions []clientgotesting.Action) - validateConditionsFunc func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) - shouldError bool - shouldFinishPolling bool - expectedEvents []string + name string + binding *v1beta1.ServiceBinding + pollReaction *fakeosb.PollBindingLastOperationReaction + getBindingReaction *fakeosb.GetBindingReaction + environmentSetupFunc func(t *testing.T, fakeKubeClient *clientgofake.Clientset, sharedInformers v1beta1informers.Interface) + validateBrokerActionsFunc func(t *testing.T, actions []fakeosb.Action) + validateKubeActionsFunc func(t *testing.T, actions []clientgotesting.Action) + assertPerfomerdActionsFunc func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) + shouldError bool + shouldFinishPolling bool + expectedEvents []string }{ // Bind { @@ -458,10 +461,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { pollReaction: &fakeosb.PollBindingLastOperationReaction{ Error: fmt.Errorf("random error"), }, - validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: nil, // does not update resources - shouldFinishPolling: false, - expectedEvents: []string{corev1.EventTypeWarning + " " + errorPollingLastOperationReason + " " + "Error polling last operation: random error"}, + validateBrokerActionsFunc: validatePollBindingLastOperationAction, + assertPerfomerdActionsFunc: nil, // does not update resources + shouldFinishPolling: false, + expectedEvents: []string{corev1.EventTypeWarning + " " + errorPollingLastOperationReason + " " + "Error polling last operation: random error"}, }, { // Special test for 410, as it is treated differently in other operations @@ -470,10 +473,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { pollReaction: &fakeosb.PollBindingLastOperationReaction{ Error: goneError, }, - validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: nil, // does not update resources - shouldFinishPolling: false, - expectedEvents: []string{corev1.EventTypeWarning + " " + errorPollingLastOperationReason + " " + "Error polling last operation: " + goneError.Error()}, + validateBrokerActionsFunc: validatePollBindingLastOperationAction, + assertPerfomerdActionsFunc: nil, // does not update resources + shouldFinishPolling: false, + expectedEvents: []string{corev1.EventTypeWarning + " " + errorPollingLastOperationReason + " " + "Error polling last operation: " + goneError.Error()}, }, { name: "bind - in progress", @@ -485,7 +488,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerfomerdActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncInProgress(t, updatedBinding, v1beta1.ServiceBindingOperationBind, asyncBindingReason, testOperation, originalBinding) }, shouldFinishPolling: false, @@ -501,7 +507,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerfomerdActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingRequestFailingError( t, updatedBinding, @@ -526,10 +535,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { Description: strPtr(lastOperationDescription), }, }, - validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: nil, // does not update resources - shouldFinishPolling: false, - expectedEvents: []string{}, // does not record event + validateBrokerActionsFunc: validatePollBindingLastOperationAction, + assertPerfomerdActionsFunc: nil, // does not update resources + shouldFinishPolling: false, + expectedEvents: []string{}, // does not record event }, { name: "bind - in progress - retry duration exceeded", @@ -541,7 +550,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerfomerdActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncBindRetryDurationExceeded(t, updatedBinding, originalBinding) }, shouldFinishPolling: true, @@ -561,7 +573,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerfomerdActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncBindRetryDurationExceeded(t, updatedBinding, originalBinding) }, shouldFinishPolling: true, @@ -584,7 +599,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { Error: fmt.Errorf("some error"), }, validateBrokerActionsFunc: validatePollBindingLastOperationAndGetBindingActions, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerfomerdActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncBindErrorAfterStateSucceeded(t, updatedBinding, errorFetchingBindingFailedReason, originalBinding) }, shouldFinishPolling: true, @@ -627,7 +645,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { assertNumberOfActions(t, actions, 1) assertActionEquals(t, actions[0], "get", "secrets") }, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerfomerdActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncBindErrorAfterStateSucceeded(t, updatedBinding, errorInjectingBindResultReason, originalBinding) }, shouldFinishPolling: true, // should not be requeued in polling queue; will drop back to default rate limiting @@ -669,7 +690,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { assertActionEquals(t, actions[0], "get", "secrets") assertActionEquals(t, actions[1], "create", "secrets") }, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerfomerdActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingOperationSuccess(t, updatedBinding, v1beta1.ServiceBindingOperationBind, originalBinding) }, shouldFinishPolling: true, @@ -686,7 +710,11 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerfomerdActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 2) + assertUpdateStatus(t, actions[0], originalBinding) + updatedBinding := assertUpdate(t, actions[1], originalBinding) + assertServiceBindingOperationSuccess(t, updatedBinding, v1beta1.ServiceBindingOperationUnbind, originalBinding) }, shouldFinishPolling: true, @@ -701,7 +729,11 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerfomerdActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 2) + assertUpdateStatus(t, actions[0], originalBinding) + updatedBinding := assertUpdate(t, actions[1], originalBinding) + assertServiceBindingOperationSuccess(t, updatedBinding, v1beta1.ServiceBindingOperationUnbind, originalBinding) }, shouldFinishPolling: true, @@ -717,7 +749,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerfomerdActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncInProgress(t, updatedBinding, v1beta1.ServiceBindingOperationUnbind, asyncUnbindingReason, testOperation, originalBinding) }, shouldFinishPolling: false, @@ -729,10 +764,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { pollReaction: &fakeosb.PollBindingLastOperationReaction{ Error: fmt.Errorf("random error"), }, - validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: nil, // does not update resources - shouldFinishPolling: false, - expectedEvents: []string{corev1.EventTypeWarning + " " + errorPollingLastOperationReason + " " + "Error polling last operation: random error"}, + validateBrokerActionsFunc: validatePollBindingLastOperationAction, + assertPerfomerdActionsFunc: nil, // does not update resources + shouldFinishPolling: false, + expectedEvents: []string{corev1.EventTypeWarning + " " + errorPollingLastOperationReason + " " + "Error polling last operation: random error"}, }, { name: "unbind - failed (retries)", @@ -744,7 +779,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerfomerdActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingRequestRetriableError( t, updatedBinding, @@ -766,10 +804,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { Description: strPtr(lastOperationDescription), }, }, - validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: nil, // does not update resources - shouldFinishPolling: false, - expectedEvents: []string{}, // does not record event + validateBrokerActionsFunc: validatePollBindingLastOperationAction, + assertPerfomerdActionsFunc: nil, // does not update resources + shouldFinishPolling: false, + expectedEvents: []string{}, // does not record event }, { name: "unbind - in progress - retry duration exceeded", @@ -781,7 +819,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerfomerdActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncUnbindRetryDurationExceeded( t, updatedBinding, @@ -807,7 +848,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerfomerdActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncUnbindRetryDurationExceeded( t, updatedBinding, @@ -833,7 +877,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerfomerdActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingRequestFailingError( t, updatedBinding, @@ -860,7 +907,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerfomerdActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingOrphanMitigationSuccess(t, updatedBinding, originalBinding) }, shouldFinishPolling: true, @@ -875,7 +925,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerfomerdActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingOrphanMitigationSuccess(t, updatedBinding, originalBinding) }, shouldFinishPolling: true, @@ -891,7 +944,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerfomerdActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncInProgress(t, updatedBinding, v1beta1.ServiceBindingOperationBind, asyncUnbindingReason, testOperation, originalBinding) }, shouldFinishPolling: false, @@ -903,10 +959,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { pollReaction: &fakeosb.PollBindingLastOperationReaction{ Error: fmt.Errorf("random error"), }, - validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: nil, // does not update resources - shouldFinishPolling: false, - expectedEvents: []string{corev1.EventTypeWarning + " " + errorPollingLastOperationReason + " " + "Error polling last operation: random error"}, + validateBrokerActionsFunc: validatePollBindingLastOperationAction, + assertPerfomerdActionsFunc: nil, // does not update resources + shouldFinishPolling: false, + expectedEvents: []string{corev1.EventTypeWarning + " " + errorPollingLastOperationReason + " " + "Error polling last operation: random error"}, }, { name: "orphan mitigation - failed (retries)", @@ -918,7 +974,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerfomerdActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingRequestRetriableOrphanMitigation(t, updatedBinding, errorUnbindCallReason, originalBinding) }, shouldError: true, @@ -934,10 +993,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { Description: strPtr(lastOperationDescription), }, }, - validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: nil, // does not update resources - shouldFinishPolling: false, - expectedEvents: []string{}, // does not record event + validateBrokerActionsFunc: validatePollBindingLastOperationAction, + assertPerfomerdActionsFunc: nil, // does not update resources + shouldFinishPolling: false, + expectedEvents: []string{}, // does not record event }, { name: "orphan mitigation - in progress - retry duration exceeded", @@ -949,7 +1008,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerfomerdActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncOrphanMitigationRetryDurationExceeded(t, updatedBinding, originalBinding) }, shouldFinishPolling: true, @@ -968,7 +1030,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerfomerdActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncOrphanMitigationRetryDurationExceeded(t, updatedBinding, originalBinding) }, shouldFinishPolling: true, @@ -987,7 +1052,10 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerfomerdActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncOrphanMitigationRetryDurationExceeded(t, updatedBinding, originalBinding) }, shouldFinishPolling: true, @@ -1050,10 +1118,8 @@ func TestPollServiceBindingNamespacedRefs(t *testing.T) { // Catalog actions actions := fakeCatalogClient.Actions() - if tc.validateConditionsFunc != nil { - assertNumberOfActions(t, actions, 1) - updatedBinding := assertUpdateStatus(t, actions[0], tc.binding).(*v1beta1.ServiceBinding) - tc.validateConditionsFunc(t, updatedBinding, tc.binding) + if tc.assertPerfomerdActionsFunc != nil { + tc.assertPerfomerdActionsFunc(t, actions, tc.binding) } else { assertNumberOfActions(t, actions, 0) } diff --git a/pkg/controller/controller_binding_test.go b/pkg/controller/controller_binding_test.go index 84eaf33c30d..a18be70acae 100644 --- a/pkg/controller/controller_binding_test.go +++ b/pkg/controller/controller_binding_test.go @@ -1189,10 +1189,13 @@ func TestReconcileServiceBindingDelete(t *testing.T) { } actions := fakeCatalogClient.Actions() - // The action should be updating the ready condition - assertNumberOfActions(t, actions, 1) + // The actions should be: + // 0. Updating the ready condition + // 1. Removing finalizer + assertNumberOfActions(t, actions, 2) - updatedServiceBinding := assertUpdateStatus(t, actions[0], binding) + assertUpdateStatus(t, actions[0], binding) + updatedServiceBinding := assertUpdate(t, actions[1], binding) assertServiceBindingOperationSuccess(t, updatedServiceBinding, v1beta1.ServiceBindingOperationUnbind, binding) assertServiceBindingOrphanMitigationSet(t, updatedServiceBinding, false) @@ -1254,8 +1257,11 @@ func TestReconcileServiceBindingDeleteUnresolvedClusterServiceClassReference(t * actions := fakeCatalogClient.Actions() // The actions should be: - // 0. Clear the finalizer - assertNumberOfActions(t, actions, 1) + // 0. Updating status + // 1. Removing finalizer + assertNumberOfActions(t, actions, 2) + assertUpdateStatus(t, actions[0], binding) + assertUpdate(t, actions[1], binding) events := getRecordedEvents(testController) assertNumEvents(t, events, 0) @@ -1427,9 +1433,17 @@ func TestReconcileServiceBindingDeleteFailedServiceBinding(t *testing.T) { return true, binding, nil }) + // updateObjectReactor is used to simulate real update and return updated object, + // without that fake client will return empty ServiceBinding struct + fakeCatalogClient.AddReactor(updateObjectReactor("servicebindings")) + + // After first reconcile only: + // - status should be change: ServiceBindingUnbindStatusRequired --> ServiceBindingOperationUnbind + // - and ServiceBinding Secret should be deleted if err := reconcileServiceBinding(t, testController, binding); err != nil { t.Fatalf("unexpected error: %v", err) } + binding = assertServiceBindingUnbindInProgressIsTheOnlyCatalogAction(t, fakeCatalogClient, binding) fakeCatalogClient.ClearActions() @@ -1438,6 +1452,10 @@ func TestReconcileServiceBindingDeleteFailedServiceBinding(t *testing.T) { assertNumberOfBrokerActions(t, fakeClusterServiceBrokerClient.Actions(), 0) + // After second reconcile only: + // - status should be change: ServiceBindingOperationUnbind --> ServiceBindingUnbindStatusSucceeded + // - ServiceBinding Secret should be deleted + // - Unbind request should be sent to broker err := reconcileServiceBinding(t, testController, binding) if err != nil { t.Fatalf("%v", err) @@ -1466,12 +1484,10 @@ func TestReconcileServiceBindingDeleteFailedServiceBinding(t *testing.T) { } actions := fakeCatalogClient.Actions() - // The four actions should be: - // 0. Updating the current operation - // 1. Updating the ready condition - assertNumberOfActions(t, actions, 1) + assertNumberOfActions(t, actions, 2) - updatedServiceBinding := assertUpdateStatus(t, actions[0], binding) + assertUpdateStatus(t, actions[0], binding) + updatedServiceBinding := assertUpdate(t, actions[1], binding) assertServiceBindingOperationSuccess(t, updatedServiceBinding, v1beta1.ServiceBindingOperationUnbind, binding) assertServiceBindingOrphanMitigationSet(t, updatedServiceBinding, false) @@ -2966,9 +2982,10 @@ func TestReconcileServiceBindingDeleteDuringOngoingOperation(t *testing.T) { // The actions should be: // 0. Updating the current operation // 1. Updating the ready condition - assertNumberOfActions(t, actions, 1) + assertNumberOfActions(t, actions, 2) - updatedServiceBinding := assertUpdateStatus(t, actions[0], binding).(*v1beta1.ServiceBinding) + assertUpdateStatus(t, actions[0], binding) + updatedServiceBinding := assertUpdate(t, actions[1], binding) assertServiceBindingOperationSuccess(t, updatedServiceBinding, v1beta1.ServiceBindingOperationUnbind, binding) assertServiceBindingOrphanMitigationSet(t, updatedServiceBinding, false) @@ -3060,11 +3077,12 @@ func TestReconcileServiceBindingDeleteDuringOrphanMitigation(t *testing.T) { actions := fakeCatalogClient.Actions() // The actions should be: - // 0. Updating the current operation - // 1. Updating the ready condition - assertNumberOfActions(t, actions, 1) + // 0. Updating status about successful deletion + // 1. Removing finalizers + assertNumberOfActions(t, actions, 2) - updatedServiceBinding := assertUpdateStatus(t, actions[0], binding).(*v1beta1.ServiceBinding) + assertUpdateStatus(t, actions[0], binding) + updatedServiceBinding := assertUpdate(t, actions[1], binding) assertServiceBindingOperationSuccess(t, updatedServiceBinding, v1beta1.ServiceBindingOperationUnbind, binding) assertServiceBindingOrphanMitigationSet(t, updatedServiceBinding, false) @@ -3292,17 +3310,17 @@ func TestPollServiceBinding(t *testing.T) { } cases := []struct { - name string - binding *v1beta1.ServiceBinding - pollReaction *fakeosb.PollBindingLastOperationReaction - getBindingReaction *fakeosb.GetBindingReaction - environmentSetupFunc func(t *testing.T, fakeKubeClient *clientgofake.Clientset, sharedInformers v1beta1informers.Interface) - validateBrokerActionsFunc func(t *testing.T, actions []fakeosb.Action) - validateKubeActionsFunc func(t *testing.T, actions []clientgotesting.Action) - validateConditionsFunc func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) - shouldError bool - shouldFinishPolling bool - expectedEvents []string + name string + binding *v1beta1.ServiceBinding + pollReaction *fakeosb.PollBindingLastOperationReaction + getBindingReaction *fakeosb.GetBindingReaction + environmentSetupFunc func(t *testing.T, fakeKubeClient *clientgofake.Clientset, sharedInformers v1beta1informers.Interface) + validateBrokerActionsFunc func(t *testing.T, actions []fakeosb.Action) + validateKubeActionsFunc func(t *testing.T, actions []clientgotesting.Action) + assertPerformedActionsFunc func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) + shouldError bool + shouldFinishPolling bool + expectedEvents []string }{ // Bind { @@ -3311,10 +3329,10 @@ func TestPollServiceBinding(t *testing.T) { pollReaction: &fakeosb.PollBindingLastOperationReaction{ Error: fmt.Errorf("random error"), }, - validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: nil, // does not update resources - shouldFinishPolling: false, - expectedEvents: []string{corev1.EventTypeWarning + " " + errorPollingLastOperationReason + " " + "Error polling last operation: random error"}, + validateBrokerActionsFunc: validatePollBindingLastOperationAction, + assertPerformedActionsFunc: nil, // does not update resources + shouldFinishPolling: false, + expectedEvents: []string{corev1.EventTypeWarning + " " + errorPollingLastOperationReason + " " + "Error polling last operation: random error"}, }, { // Special test for 410, as it is treated differently in other operations @@ -3323,10 +3341,10 @@ func TestPollServiceBinding(t *testing.T) { pollReaction: &fakeosb.PollBindingLastOperationReaction{ Error: goneError, }, - validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: nil, // does not update resources - shouldFinishPolling: false, - expectedEvents: []string{corev1.EventTypeWarning + " " + errorPollingLastOperationReason + " " + "Error polling last operation: " + goneError.Error()}, + validateBrokerActionsFunc: validatePollBindingLastOperationAction, + assertPerformedActionsFunc: nil, // does not update resources + shouldFinishPolling: false, + expectedEvents: []string{corev1.EventTypeWarning + " " + errorPollingLastOperationReason + " " + "Error polling last operation: " + goneError.Error()}, }, { name: "bind - in progress", @@ -3338,7 +3356,10 @@ func TestPollServiceBinding(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerformedActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncInProgress(t, updatedBinding, v1beta1.ServiceBindingOperationBind, asyncBindingReason, testOperation, originalBinding) }, shouldFinishPolling: false, @@ -3354,7 +3375,10 @@ func TestPollServiceBinding(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerformedActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingRequestFailingError( t, updatedBinding, @@ -3379,10 +3403,10 @@ func TestPollServiceBinding(t *testing.T) { Description: strPtr(lastOperationDescription), }, }, - validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: nil, // does not update resources - shouldFinishPolling: false, - expectedEvents: []string{}, // does not record event + validateBrokerActionsFunc: validatePollBindingLastOperationAction, + assertPerformedActionsFunc: nil, // does not update resources + shouldFinishPolling: false, + expectedEvents: []string{}, // does not record event }, { name: "bind - in progress - retry duration exceeded", @@ -3394,7 +3418,10 @@ func TestPollServiceBinding(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerformedActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncBindRetryDurationExceeded(t, updatedBinding, originalBinding) }, shouldFinishPolling: true, @@ -3414,7 +3441,10 @@ func TestPollServiceBinding(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerformedActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncBindRetryDurationExceeded(t, updatedBinding, originalBinding) }, shouldFinishPolling: true, @@ -3437,7 +3467,10 @@ func TestPollServiceBinding(t *testing.T) { Error: fmt.Errorf("some error"), }, validateBrokerActionsFunc: validatePollBindingLastOperationAndGetBindingActions, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerformedActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncBindErrorAfterStateSucceeded(t, updatedBinding, errorFetchingBindingFailedReason, originalBinding) }, shouldFinishPolling: true, @@ -3480,7 +3513,10 @@ func TestPollServiceBinding(t *testing.T) { assertNumberOfActions(t, actions, 1) assertActionEquals(t, actions[0], "get", "secrets") }, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerformedActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncBindErrorAfterStateSucceeded(t, updatedBinding, errorInjectingBindResultReason, originalBinding) }, shouldFinishPolling: true, // should not be requeued in polling queue; will drop back to default rate limiting @@ -3522,7 +3558,10 @@ func TestPollServiceBinding(t *testing.T) { assertActionEquals(t, actions[0], "get", "secrets") assertActionEquals(t, actions[1], "create", "secrets") }, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerformedActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingOperationSuccess(t, updatedBinding, v1beta1.ServiceBindingOperationBind, originalBinding) }, shouldFinishPolling: true, @@ -3539,7 +3578,11 @@ func TestPollServiceBinding(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerformedActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 2) + assertUpdateStatus(t, actions[0], originalBinding) + updatedBinding := assertUpdate(t, actions[1], originalBinding) + assertServiceBindingOperationSuccess(t, updatedBinding, v1beta1.ServiceBindingOperationUnbind, originalBinding) }, shouldFinishPolling: true, @@ -3554,7 +3597,11 @@ func TestPollServiceBinding(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerformedActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 2) + assertUpdateStatus(t, actions[0], originalBinding) + updatedBinding := assertUpdate(t, actions[1], originalBinding) + assertServiceBindingOperationSuccess(t, updatedBinding, v1beta1.ServiceBindingOperationUnbind, originalBinding) }, shouldFinishPolling: true, @@ -3570,7 +3617,10 @@ func TestPollServiceBinding(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerformedActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncInProgress(t, updatedBinding, v1beta1.ServiceBindingOperationUnbind, asyncUnbindingReason, testOperation, originalBinding) }, shouldFinishPolling: false, @@ -3582,10 +3632,10 @@ func TestPollServiceBinding(t *testing.T) { pollReaction: &fakeosb.PollBindingLastOperationReaction{ Error: fmt.Errorf("random error"), }, - validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: nil, // does not update resources - shouldFinishPolling: false, - expectedEvents: []string{corev1.EventTypeWarning + " " + errorPollingLastOperationReason + " " + "Error polling last operation: random error"}, + validateBrokerActionsFunc: validatePollBindingLastOperationAction, + assertPerformedActionsFunc: nil, // does not update resources + shouldFinishPolling: false, + expectedEvents: []string{corev1.EventTypeWarning + " " + errorPollingLastOperationReason + " " + "Error polling last operation: random error"}, }, { name: "unbind - failed (retries)", @@ -3597,7 +3647,10 @@ func TestPollServiceBinding(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerformedActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingRequestRetriableError( t, updatedBinding, @@ -3619,10 +3672,10 @@ func TestPollServiceBinding(t *testing.T) { Description: strPtr(lastOperationDescription), }, }, - validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: nil, // does not update resources - shouldFinishPolling: false, - expectedEvents: []string{}, // does not record event + validateBrokerActionsFunc: validatePollBindingLastOperationAction, + assertPerformedActionsFunc: nil, // does not update resources + shouldFinishPolling: false, + expectedEvents: []string{}, // does not record event }, { name: "unbind - in progress - retry duration exceeded", @@ -3634,7 +3687,10 @@ func TestPollServiceBinding(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerformedActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncUnbindRetryDurationExceeded( t, updatedBinding, @@ -3660,7 +3716,10 @@ func TestPollServiceBinding(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerformedActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncUnbindRetryDurationExceeded( t, updatedBinding, @@ -3686,7 +3745,10 @@ func TestPollServiceBinding(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerformedActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingRequestFailingError( t, updatedBinding, @@ -3713,7 +3775,10 @@ func TestPollServiceBinding(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerformedActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingOrphanMitigationSuccess(t, updatedBinding, originalBinding) }, shouldFinishPolling: true, @@ -3728,7 +3793,10 @@ func TestPollServiceBinding(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerformedActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingOrphanMitigationSuccess(t, updatedBinding, originalBinding) }, shouldFinishPolling: true, @@ -3744,7 +3812,10 @@ func TestPollServiceBinding(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerformedActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncInProgress(t, updatedBinding, v1beta1.ServiceBindingOperationBind, asyncUnbindingReason, testOperation, originalBinding) }, shouldFinishPolling: false, @@ -3756,10 +3827,10 @@ func TestPollServiceBinding(t *testing.T) { pollReaction: &fakeosb.PollBindingLastOperationReaction{ Error: fmt.Errorf("random error"), }, - validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: nil, // does not update resources - shouldFinishPolling: false, - expectedEvents: []string{corev1.EventTypeWarning + " " + errorPollingLastOperationReason + " " + "Error polling last operation: random error"}, + validateBrokerActionsFunc: validatePollBindingLastOperationAction, + assertPerformedActionsFunc: nil, // does not update resources + shouldFinishPolling: false, + expectedEvents: []string{corev1.EventTypeWarning + " " + errorPollingLastOperationReason + " " + "Error polling last operation: random error"}, }, { name: "orphan mitigation - failed (retries)", @@ -3771,7 +3842,10 @@ func TestPollServiceBinding(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerformedActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingRequestRetriableOrphanMitigation(t, updatedBinding, errorUnbindCallReason, originalBinding) }, shouldError: true, @@ -3787,10 +3861,10 @@ func TestPollServiceBinding(t *testing.T) { Description: strPtr(lastOperationDescription), }, }, - validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: nil, // does not update resources - shouldFinishPolling: false, - expectedEvents: []string{}, // does not record event + validateBrokerActionsFunc: validatePollBindingLastOperationAction, + assertPerformedActionsFunc: nil, // does not update resources + shouldFinishPolling: false, + expectedEvents: []string{}, // does not record event }, { name: "orphan mitigation - in progress - retry duration exceeded", @@ -3802,7 +3876,10 @@ func TestPollServiceBinding(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerformedActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncOrphanMitigationRetryDurationExceeded(t, updatedBinding, originalBinding) }, shouldFinishPolling: true, @@ -3821,7 +3898,10 @@ func TestPollServiceBinding(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerformedActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncOrphanMitigationRetryDurationExceeded(t, updatedBinding, originalBinding) }, shouldFinishPolling: true, @@ -3840,7 +3920,10 @@ func TestPollServiceBinding(t *testing.T) { }, }, validateBrokerActionsFunc: validatePollBindingLastOperationAction, - validateConditionsFunc: func(t *testing.T, updatedBinding *v1beta1.ServiceBinding, originalBinding *v1beta1.ServiceBinding) { + assertPerformedActionsFunc: func(t *testing.T, actions []clientgotesting.Action, originalBinding *v1beta1.ServiceBinding) { + assertNumberOfActions(t, actions, 1) + updatedBinding := assertUpdateStatus(t, actions[0], originalBinding) + assertServiceBindingAsyncOrphanMitigationRetryDurationExceeded(t, updatedBinding, originalBinding) }, shouldFinishPolling: true, @@ -3903,10 +3986,8 @@ func TestPollServiceBinding(t *testing.T) { // Catalog actions actions := fakeCatalogClient.Actions() - if tc.validateConditionsFunc != nil { - assertNumberOfActions(t, actions, 1) - updatedBinding := assertUpdateStatus(t, actions[0], tc.binding).(*v1beta1.ServiceBinding) - tc.validateConditionsFunc(t, updatedBinding, tc.binding) + if tc.assertPerformedActionsFunc != nil { + tc.assertPerformedActionsFunc(t, actions, tc.binding) } else { assertNumberOfActions(t, actions, 0) } diff --git a/pkg/controller/controller_clusterservicebroker.go b/pkg/controller/controller_clusterservicebroker.go index 0cf2a8191d3..c84376aa1cf 100644 --- a/pkg/controller/controller_clusterservicebroker.go +++ b/pkg/controller/controller_clusterservicebroker.go @@ -710,7 +710,7 @@ func (c *controller) updateClusterServiceBrokerFinalizers( logContext := fmt.Sprint(pcb.Messagef("Updating finalizers to %v", finalizers)) klog.V(4).Info(pcb.Messagef("Updating %v", logContext)) - _, err = c.serviceCatalogClient.ClusterServiceBrokers().UpdateStatus(toUpdate) + _, err = c.serviceCatalogClient.ClusterServiceBrokers().Update(toUpdate) if err != nil { klog.Error(pcb.Messagef("Error updating %v: %v", logContext, err)) } diff --git a/pkg/controller/controller_clusterservicebroker_test.go b/pkg/controller/controller_clusterservicebroker_test.go index 38e3868d168..d91f0cdc910 100644 --- a/pkg/controller/controller_clusterservicebroker_test.go +++ b/pkg/controller/controller_clusterservicebroker_test.go @@ -688,7 +688,7 @@ func TestReconcileClusterServiceBrokerDelete(t *testing.T) { assertGet(t, catalogActions[5], broker) - updatedClusterServiceBroker = assertUpdateStatus(t, catalogActions[6], broker) + updatedClusterServiceBroker = assertUpdate(t, catalogActions[6], broker) assertEmptyFinalizers(t, updatedClusterServiceBroker) events := getRecordedEvents(testController) diff --git a/pkg/controller/controller_instance.go b/pkg/controller/controller_instance.go index 9fdde327655..a41ffa2a3da 100644 --- a/pkg/controller/controller_instance.go +++ b/pkg/controller/controller_instance.go @@ -1903,8 +1903,7 @@ func (c *controller) updateServiceInstanceStatus(instance *v1beta1.ServiceInstan // version's status with the status on the ServiceInstance passed // to it; it also runs the provided postConflictUpdateFunc, // allowing the caller to make additional changes to the -// new version of the instance - to other parts of the object -// (e.g. finalizers). +// new version of the instance - to other parts of the object. func (c *controller) updateServiceInstanceStatusWithRetries( instance *v1beta1.ServiceInstance, postConflictUpdateFunc func(*v1beta1.ServiceInstance)) (*v1beta1.ServiceInstance, error) { @@ -2562,11 +2561,21 @@ func (c *controller) prepareServiceInstanceLastOperationRequest(instance *v1beta // updating of a ServiceInstance that has successfully finished graceful // deletion. func (c *controller) processServiceInstanceGracefulDeletionSuccess(instance *v1beta1.ServiceInstance) error { - c.removeFinalizer(instance) - if _, err := c.updateServiceInstanceStatusWithRetries(instance, c.removeFinalizer); err != nil { + updatedInstance, err := c.updateServiceInstanceStatusWithRetries(instance, nil) + if err != nil { return err } + toUpdate := updatedInstance.DeepCopy() + finalizers := sets.NewString(toUpdate.Finalizers...) + finalizers.Delete(v1beta1.FinalizerServiceCatalog) + toUpdate.Finalizers = finalizers.List() + + _, err = c.serviceCatalogClient.ServiceInstances(toUpdate.Namespace).Update(toUpdate) + if err != nil { + return fmt.Errorf("while removing finalizer entry: %v", err) + } + pcb := pretty.NewInstanceContextBuilder(instance) klog.Info(pcb.Message("Cleared finalizer")) @@ -2574,12 +2583,6 @@ func (c *controller) processServiceInstanceGracefulDeletionSuccess(instance *v1b return nil } -func (c *controller) removeFinalizer(instance *v1beta1.ServiceInstance) { - finalizers := sets.NewString(instance.Finalizers...) - finalizers.Delete(v1beta1.FinalizerServiceCatalog) - instance.Finalizers = finalizers.List() -} - // handleServiceInstanceReconciliationError is a helper function that handles // on error whether the error represents an operation error and should update // the ServiceInstance resource. diff --git a/pkg/controller/controller_instance_ns_test.go b/pkg/controller/controller_instance_ns_test.go index 6962050c5b3..8718a85718c 100644 --- a/pkg/controller/controller_instance_ns_test.go +++ b/pkg/controller/controller_instance_ns_test.go @@ -426,6 +426,10 @@ func TestReconcileServiceInstanceDeleteWithNamespacedRefs(t *testing.T) { return true, instance, nil }) + // simulate real update and return updated object, + // without that fake client will return empty ServiceInstances struct + fakeCatalogClient.AddReactor(updateObjectReactor("serviceinstances")) + if err := reconcileServiceInstance(t, testController, instance); err != nil { t.Fatalf("unexpected error: %v", err) } @@ -452,9 +456,10 @@ func TestReconcileServiceInstanceDeleteWithNamespacedRefs(t *testing.T) { assertNumberOfActions(t, kubeActions, 0) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 1) + assertNumberOfActions(t, actions, 2) - updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + assertUpdateStatus(t, actions[0], instance) + updatedServiceInstance := assertUpdate(t, actions[1], instance) assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) events := getRecordedEvents(testController) @@ -665,6 +670,10 @@ func TestPollServiceInstanceSuccessDeprovisioningWithOperationNoFinalizerNamespa sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass()) sharedInformers.ServicePlans().Informer().GetStore().Add(getTestServicePlan()) + // simulate real update and return updated object, + // without that fake client will return empty ServiceInstances struct + fakeCatalogClient.AddReactor(updateObjectReactor("serviceinstances")) + instance := getTestServiceInstanceAsyncDeprovisioningWithNamespacedRefs(testOperation) instanceKey := testNamespace + "/" + testServiceInstanceName @@ -697,9 +706,10 @@ func TestPollServiceInstanceSuccessDeprovisioningWithOperationNoFinalizerNamespa assertNumberOfActions(t, kubeActions, 0) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 1) + assertNumberOfActions(t, actions, 2) - updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + assertUpdateStatus(t, actions[0], instance) + updatedServiceInstance := assertUpdate(t, actions[1], instance) assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testServicePlanName, testServicePlanGUID, instance) events := getRecordedEvents(testController) diff --git a/pkg/controller/controller_instance_test.go b/pkg/controller/controller_instance_test.go index 4ba2f8ef64c..afd3a6f098d 100644 --- a/pkg/controller/controller_instance_test.go +++ b/pkg/controller/controller_instance_test.go @@ -1795,6 +1795,10 @@ func TestReconcileServiceInstanceDelete(t *testing.T) { return true, instance, nil }) + // simulate real update and return updated object, + // without that fake client will return empty ServiceInstances struct + fakeCatalogClient.AddReactor(updateObjectReactor("serviceinstances")) + if err := reconcileServiceInstance(t, testController, instance); err != nil { t.Fatalf("unexpected error: %v", err) } @@ -1821,9 +1825,10 @@ func TestReconcileServiceInstanceDelete(t *testing.T) { assertNumberOfActions(t, kubeActions, 0) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 1) + assertNumberOfActions(t, actions, 2) - updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + assertUpdateStatus(t, actions[0], instance) + updatedServiceInstance := assertUpdate(t, actions[1], instance) assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) events := getRecordedEvents(testController) @@ -1870,6 +1875,10 @@ func TestReconcileServiceInstanceDeleteBlockedByCredentials(t *testing.T) { return true, instance, nil }) + // simulate real update and return updated object, + // without that fake client will return empty ServiceInstances struct + fakeCatalogClient.AddReactor(updateObjectReactor("serviceinstances")) + if err := reconcileServiceInstance(t, testController, instance); err == nil { t.Fatalf("expected reconcileServiceInstance to return an error, but there was none") } @@ -1934,9 +1943,10 @@ func TestReconcileServiceInstanceDeleteBlockedByCredentials(t *testing.T) { // The actions should be: // 0. Updating the current operation // 1. Updating the ready condition - assertNumberOfActions(t, actions, 1) + assertNumberOfActions(t, actions, 2) - updateObject = assertUpdateStatus(t, actions[0], instance) + assertUpdateStatus(t, actions[0], instance) + updateObject = assertUpdate(t, actions[1], instance) assertServiceInstanceOperationSuccess(t, updateObject, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) events = getRecordedEvents(testController) @@ -2083,6 +2093,10 @@ func TestReconcileServiceInstanceDeleteFailedProvisionWithRequest(t *testing.T) return true, instance, nil }) + // simulate real update and return updated object, + // without that fake client will return empty ServiceInstances struct + fakeCatalogClient.AddReactor(updateObjectReactor("serviceinstances")) + if err := reconcileServiceInstance(t, testController, instance); err != nil { t.Fatalf("unexpected error: %v", err) } @@ -2109,9 +2123,10 @@ func TestReconcileServiceInstanceDeleteFailedProvisionWithRequest(t *testing.T) assertNumberOfActions(t, kubeActions, 0) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 1) + assertNumberOfActions(t, actions, 2) - updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + assertUpdateStatus(t, actions[0], instance) + updatedServiceInstance := assertUpdate(t, actions[1], instance) assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) events := getRecordedEvents(testController) @@ -2195,6 +2210,10 @@ func TestReconsileServiceInstanceDeleteWithParameters(t *testing.T) { return true, instance, nil }) + // simulate real update and return updated object, + // without that fake client will return empty ServiceInstances struct + fakeCatalogClient.AddReactor(updateObjectReactor("serviceinstances")) + err := reconcileServiceInstance(t, testController, instance) if err != nil { t.Fatalf("Unexpected error from reconcileServiceInstance: %v", err) @@ -2208,9 +2227,10 @@ func TestReconsileServiceInstanceDeleteWithParameters(t *testing.T) { assertNumberOfActions(t, kubeActions, 0) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 1) + assertNumberOfActions(t, actions, 2) - updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + assertUpdateStatus(t, actions[0], instance) + updatedServiceInstance := assertUpdate(t, actions[1], instance) assertEmptyFinalizers(t, updatedServiceInstance) events := getRecordedEvents(testController) @@ -2296,6 +2316,10 @@ func TestReconcileServiceInstanceDeleteFailedUpdate(t *testing.T) { return true, instance, nil }) + // simulate real update and return updated object, + // without that fake client will return empty ServiceInstances struct + fakeCatalogClient.AddReactor(updateObjectReactor("serviceinstances")) + if err := reconcileServiceInstance(t, testController, instance); err != nil { t.Fatalf("unexpected error: %v", err) } @@ -2322,9 +2346,10 @@ func TestReconcileServiceInstanceDeleteFailedUpdate(t *testing.T) { assertNumberOfActions(t, kubeActions, 0) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 1) + assertNumberOfActions(t, actions, 2) - updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + assertUpdateStatus(t, actions[0], instance) + updatedServiceInstance := assertUpdate(t, actions[1], instance) assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) events := getRecordedEvents(testController) @@ -2361,6 +2386,10 @@ func TestReconcileServiceInstanceDeleteDoesNotInvokeClusterServiceBroker(t *test return true, instance, nil }) + // simulate real update and return updated object, + // without that fake client will return empty ServiceInstances struct + fakeCatalogClient.AddReactor(updateObjectReactor("serviceinstances")) + if err := reconcileServiceInstance(t, testController, instance); err != nil { t.Fatalf("This should not fail : %v", err) } @@ -2373,11 +2402,13 @@ func TestReconcileServiceInstanceDeleteDoesNotInvokeClusterServiceBroker(t *test assertNumberOfActions(t, kubeActions, 0) actions := fakeCatalogClient.Actions() - // The one actions should be: - // 0. Removing the finalizer - assertNumberOfActions(t, actions, 1) + // The actions should be: + // 1. Updating status + // 2. Removing the finalizer + assertNumberOfActions(t, actions, 2) - updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + assertUpdateStatus(t, actions[0], instance) + updatedServiceInstance := assertUpdate(t, actions[1], instance) assertEmptyFinalizers(t, updatedServiceInstance) events := getRecordedEvents(testController) @@ -2418,7 +2449,7 @@ func TestFinalizerClearedWhen409ConflictEncounteredOnStatusUpdate(t *testing.T) if instance.ResourceVersion == "1" { return true, nil, apierrors.NewConflict(action.GetResource().GroupResource(), instance.Name, errors.New("object has changed")) } - return false, nil, nil + return true, object, nil }) if err := reconcileServiceInstance(t, testController, instance); err != nil { @@ -2434,14 +2465,16 @@ func TestFinalizerClearedWhen409ConflictEncounteredOnStatusUpdate(t *testing.T) actions := fakeCatalogClient.Actions() // The actions should be: - // 0. Update the instance status (with finalizer removed); the server responds with 409 Conflict + // 0. Update the instance status; the server responds with 409 Conflict // 1. Fetch the fresh instance - // 2. Update the fresh instance. Finalizer should be removed again. - assertNumberOfActions(t, actions, 3) + // 2. Update instance status with the fresh instance. + // 3. Remove the finalizer on the fresh instance. + assertNumberOfActions(t, actions, 4) assertUpdateStatus(t, actions[0], instance) assertGet(t, actions[1], instance) - updatedServiceInstance := assertUpdateStatus(t, actions[2], instance) + assertUpdateStatus(t, actions[2], instance) + updatedServiceInstance := assertUpdate(t, actions[3], instance) assertEmptyFinalizers(t, updatedServiceInstance) events := getRecordedEvents(testController) @@ -2791,6 +2824,10 @@ func TestPollServiceInstanceSuccessDeprovisioningWithOperationNoFinalizer(t *tes instance := getTestServiceInstanceAsyncDeprovisioning(testOperation) instanceKey := testNamespace + "/" + testServiceInstanceName + // simulate real update and return updated object, + // without that fake client will return empty ServiceInstances struct + fakeCatalogClient.AddReactor(updateObjectReactor("serviceinstances")) + if testController.instancePollingQueue.NumRequeues(instanceKey) != 0 { t.Fatalf("Expected polling queue to not have any record of test instance") } @@ -2820,9 +2857,10 @@ func TestPollServiceInstanceSuccessDeprovisioningWithOperationNoFinalizer(t *tes assertNumberOfActions(t, kubeActions, 0) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 1) + assertNumberOfActions(t, actions, 2) - updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + assertUpdateStatus(t, actions[0], instance) + updatedServiceInstance := assertUpdate(t, actions[1], instance) assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) events := getRecordedEvents(testController) @@ -2995,6 +3033,10 @@ func TestPollServiceInstanceStatusGoneDeprovisioningWithOperationNoFinalizer(t * instance := getTestServiceInstanceAsyncDeprovisioning(testOperation) instanceKey := testNamespace + "/" + testServiceInstanceName + // simulate real update and return updated object, + // without that fake client will return empty ServiceInstances struct + fakeCatalogClient.AddReactor(updateObjectReactor("serviceinstances")) + if testController.instancePollingQueue.NumRequeues(instanceKey) != 0 { t.Fatalf("Expected polling queue to not have any record of test instance") } @@ -3024,9 +3066,10 @@ func TestPollServiceInstanceStatusGoneDeprovisioningWithOperationNoFinalizer(t * assertNumberOfActions(t, kubeActions, 0) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 1) + assertNumberOfActions(t, actions, 2) - updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + assertUpdateStatus(t, actions[0], instance) + updatedServiceInstance := assertUpdate(t, actions[1], instance) assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) events := getRecordedEvents(testController) @@ -3186,6 +3229,10 @@ func TestPollServiceInstanceSuccessDeprovisioningWithOperationWithFinalizer(t *t return true, instance, nil }) + // simulate real update and return updated object, + // without that fake client will return empty ServiceInstances struct + fakeCatalogClient.AddReactor(updateObjectReactor("serviceinstances")) + if testController.instancePollingQueue.NumRequeues(instanceKey) != 0 { t.Fatalf("Expected polling queue to not have any record of test instance") } @@ -3215,9 +3262,10 @@ func TestPollServiceInstanceSuccessDeprovisioningWithOperationWithFinalizer(t *t assertNumberOfActions(t, kubeActions, 0) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 1) + assertNumberOfActions(t, actions, 2) - updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + assertUpdateStatus(t, actions[0], instance) + updatedServiceInstance := assertUpdate(t, actions[1], instance) assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) events := getRecordedEvents(testController) @@ -5950,6 +5998,10 @@ func TestReconcileServiceInstanceDeleteDuringOngoingOperation(t *testing.T) { return true, instance, nil }) + // simulate real update and return updated object, + // without that fake client will return empty ServiceInstances struct + fakeCatalogClient.AddReactor(updateObjectReactor("serviceinstances")) + timeOfReconciliation := metav1.Now() if err := reconcileServiceInstance(t, testController, instance); err != nil { @@ -5989,9 +6041,10 @@ func TestReconcileServiceInstanceDeleteDuringOngoingOperation(t *testing.T) { assertNumberOfActions(t, kubeActions, 0) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 1) + assertNumberOfActions(t, actions, 2) - updatedServiceInstance := assertUpdateStatus(t, actions[0], instance).(*v1beta1.ServiceInstance) + assertUpdateStatus(t, actions[0], instance) + updatedServiceInstance := assertUpdate(t, actions[1], instance) assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) events := getRecordedEvents(testController) @@ -6034,6 +6087,10 @@ func TestReconcileServiceInstanceDeleteWithOngoingOperation(t *testing.T) { return true, instance, nil }) + // simulate real update and return updated object, + // without that fake client will return empty ServiceInstances struct + fakeCatalogClient.AddReactor(updateObjectReactor("serviceinstances")) + timeOfReconciliation := metav1.Now() if err := reconcileServiceInstance(t, testController, instance); err != nil { @@ -6073,9 +6130,10 @@ func TestReconcileServiceInstanceDeleteWithOngoingOperation(t *testing.T) { assertNumberOfActions(t, kubeActions, 0) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 1) + assertNumberOfActions(t, actions, 2) - updatedServiceInstance := assertUpdateStatus(t, actions[0], instance).(*v1beta1.ServiceInstance) + assertUpdateStatus(t, actions[0], instance) + updatedServiceInstance := assertUpdate(t, actions[1], instance) assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, testClusterServicePlanName, testClusterServicePlanGUID, instance) events := getRecordedEvents(testController) @@ -6119,6 +6177,10 @@ func TestReconcileServiceInstanceDeleteWithNonExistentPlan(t *testing.T) { return true, instance, nil }) + // simulate real update and return updated object, + // without that fake client will return empty ServiceInstances struct + fakeCatalogClient.AddReactor(updateObjectReactor("serviceinstances")) + if err := reconcileServiceInstance(t, testController, instance); err != nil { t.Fatalf("unexpected error: %v", err) } @@ -6146,9 +6208,10 @@ func TestReconcileServiceInstanceDeleteWithNonExistentPlan(t *testing.T) { assertNumberOfActions(t, kubeActions, 0) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 1) + assertNumberOfActions(t, actions, 2) - updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + assertUpdateStatus(t, actions[0], instance) + updatedServiceInstance := assertUpdate(t, actions[1], instance) assertServiceInstanceOperationSuccess(t, updatedServiceInstance, v1beta1.ServiceInstanceOperationDeprovision, "old-plan-name", "old-plan-id", instance) events := getRecordedEvents(testController) diff --git a/pkg/controller/controller_servicebroker.go b/pkg/controller/controller_servicebroker.go index bd2c329e904..078b1660b2d 100644 --- a/pkg/controller/controller_servicebroker.go +++ b/pkg/controller/controller_servicebroker.go @@ -690,7 +690,7 @@ func (c *controller) updateServiceBrokerFinalizers( logContext := fmt.Sprint(pcb.Messagef("Updating finalizers to %v", finalizers)) klog.V(4).Info(pcb.Messagef("Updating %v", logContext)) - _, err = c.serviceCatalogClient.ServiceBrokers(broker.Namespace).UpdateStatus(toUpdate) + _, err = c.serviceCatalogClient.ServiceBrokers(broker.Namespace).Update(toUpdate) if err != nil { klog.Error(pcb.Messagef("Error updating %v: %v", logContext, err)) } diff --git a/pkg/controller/controller_servicebroker_test.go b/pkg/controller/controller_servicebroker_test.go index dc4b780bbdc..5f2a7d0fd97 100644 --- a/pkg/controller/controller_servicebroker_test.go +++ b/pkg/controller/controller_servicebroker_test.go @@ -182,7 +182,7 @@ func TestReconcileServiceBrokerDelete(t *testing.T) { assertGet(t, catalogActions[5], broker) - updatedServiceBroker = assertUpdateStatus(t, catalogActions[6], broker) + updatedServiceBroker = assertUpdate(t, catalogActions[6], broker) assertEmptyFinalizers(t, updatedServiceBroker) events := getRecordedEvents(testController) diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index c3b9fb5db08..b7a11e2cf93 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -1165,6 +1165,7 @@ func getTestServiceBinding() *v1beta1.ServiceBinding { Generation: 1, }, Spec: v1beta1.ServiceBindingSpec{ + SecretName: testServiceBindingSecretName, InstanceRef: v1beta1.LocalObjectReference{Name: testServiceInstanceName}, ExternalID: testServiceBindingGUID, }, @@ -4069,3 +4070,14 @@ func addGetSecretReaction(fakeKubeClient *clientgofake.Clientset, secret *corev1 return true, secret, nil }) } + +// updateObjectReactor is used to simulate real update and return updated object, +// without that fake client will return empty struct +// TODO: in future we should consider refactor of newTestController method to use servicecatalogclientset.NewSimpleClientset() instead of &servicecatalogclientset.Clientset{} +// thanks to that such methods will not be required because we will have that behaviour out-of-the-box - also other reactor like "get" etc. could be removed. +func updateObjectReactor(resource string) (string, string, clientgotesting.ReactionFunc) { + return "update", resource, func(action clientgotesting.Action) (bool, runtime.Object, error) { + e := action.(clientgotesting.UpdateAction) + return true, e.GetObject(), nil + } +}