diff --git a/pkg/apis/servicecatalog/types.go b/pkg/apis/servicecatalog/types.go index 331da87cab1f..b7970b78d846 100644 --- a/pkg/apis/servicecatalog/types.go +++ b/pkg/apis/servicecatalog/types.go @@ -477,6 +477,10 @@ type BindingConditionType string const ( // BindingConditionReady represents a BindingCondition is in ready state. BindingConditionReady BindingConditionType = "Ready" + + // BindingConditionFailed represents a BindingCondition that has failed + // completely and should not be retried. + BindingConditionFailed BindingConditionType = "Failed" ) // These are internal finalizer values to service catalog, must be qualified name. diff --git a/pkg/apis/servicecatalog/v1alpha1/types.go b/pkg/apis/servicecatalog/v1alpha1/types.go index 81d2a13c2504..fdabfa1393ff 100644 --- a/pkg/apis/servicecatalog/v1alpha1/types.go +++ b/pkg/apis/servicecatalog/v1alpha1/types.go @@ -478,6 +478,10 @@ type BindingConditionType string const ( // BindingConditionReady represents a binding condition is in ready state. BindingConditionReady BindingConditionType = "Ready" + + // BindingConditionFailed represents a BindingCondition that has failed + // completely and should not be retried. + BindingConditionFailed BindingConditionType = "Failed" ) // These are external finalizer values to service catalog, must be qualified name. diff --git a/pkg/controller/controller_binding.go b/pkg/controller/controller_binding.go index 38ef60bf0b89..41d6b9ec7946 100644 --- a/pkg/controller/controller_binding.go +++ b/pkg/controller/controller_binding.go @@ -18,7 +18,6 @@ package controller import ( "fmt" - "time" "github.com/golang/glog" osb "github.com/pmorie/go-open-service-broker-client/v2" @@ -69,9 +68,42 @@ func (c *controller) bindingUpdate(oldObj, newObj interface{}) { c.bindingAdd(newObj) } +func makeBindingClone(binding *v1alpha1.Binding) (*v1alpha1.Binding, error) { + clone, err := api.Scheme.DeepCopy(binding) + if err != nil { + return nil, err + } + return clone.(*v1alpha1.Binding), nil +} + +func isBindingFailed(binding *v1alpha1.Binding) bool { + for _, condition := range binding.Status.Conditions { + if condition.Type == v1alpha1.BindingConditionFailed && condition.Status == v1alpha1.ConditionTrue { + return true + } + } + return false +} + // an error is returned to indicate that the binding has not been // fully processed and should be resubmitted at a later time. func (c *controller) reconcileBinding(binding *v1alpha1.Binding) error { + // TODO: this will change once we fully implement orphan mitigation, see: + // https://github.com/kubernetes-incubator/service-catalog/issues/988 + if isBindingFailed(binding) && binding.ObjectMeta.DeletionTimestamp == nil { + glog.V(4).Infof( + "Not processing event for Binding %v/%v because status showed that it has failed", + binding.Namespace, + binding.Name, + ) + return nil + } + + toUpdate, err := makeBindingClone(binding) + if err != nil { + return err + } + // Determine whether the checksum has been invalidated by a change to the // object. If the binding's checksum matches the calculated checksum, // there is no work to do. @@ -104,13 +136,14 @@ func (c *controller) reconcileBinding(binding *v1alpha1.Binding) error { binding.Spec.InstanceRef.Name, err, ) - c.updateBindingCondition( - binding, + c.setBindingCondition( + toUpdate, v1alpha1.BindingConditionReady, v1alpha1.ConditionFalse, errorNonexistentInstanceReason, "The binding references an Instance that does not exist. "+s, ) + c.updateBindingStatus(toUpdate) c.recorder.Event(binding, api.EventTypeWarning, errorNonexistentInstanceReason, s) return err } @@ -124,13 +157,14 @@ func (c *controller) reconcileBinding(binding *v1alpha1.Binding) error { binding.Spec.InstanceRef.Name, ) glog.Info(s) - c.updateBindingCondition( - binding, + c.setBindingCondition( + toUpdate, v1alpha1.BindingConditionReady, v1alpha1.ConditionFalse, errorWithOngoingAsyncOperation, errorWithOngoingAsyncOperationMessage, ) + c.updateBindingStatus(toUpdate) c.recorder.Event(binding, api.EventTypeWarning, errorWithOngoingAsyncOperation, s) return fmt.Errorf("Ongoing Asynchronous operation") } @@ -149,13 +183,14 @@ func (c *controller) reconcileBinding(binding *v1alpha1.Binding) error { instance.Spec.PlanName, ) glog.Warning(s) - c.updateBindingCondition( - binding, + c.setBindingCondition( + toUpdate, v1alpha1.BindingConditionReady, v1alpha1.ConditionFalse, errorNonbindableServiceClassReason, s, ) + c.updateBindingStatus(toUpdate) c.recorder.Event(binding, api.EventTypeWarning, errorNonbindableServiceClassReason, s) return nil } @@ -169,13 +204,14 @@ func (c *controller) reconcileBinding(binding *v1alpha1.Binding) error { if err != nil { s := fmt.Sprintf("Failed to prepare Binding parameters\n%s\n %s", binding.Spec.Parameters, err) glog.Warning(s) - c.updateBindingCondition( - binding, + c.setBindingCondition( + toUpdate, v1alpha1.BindingConditionReady, v1alpha1.ConditionFalse, errorWithParameters, s, ) + c.updateBindingStatus(toUpdate) c.recorder.Event(binding, api.EventTypeWarning, errorWithParameters, s) return err } @@ -185,13 +221,14 @@ func (c *controller) reconcileBinding(binding *v1alpha1.Binding) error { if err != nil { s := fmt.Sprintf("Failed to get namespace %q during binding: %s", instance.Namespace, err) glog.Info(s) - c.updateBindingCondition( - binding, + c.setBindingCondition( + toUpdate, v1alpha1.BindingConditionReady, v1alpha1.ConditionFalse, errorFindingNamespaceInstanceReason, "Error finding namespace for instance. "+s, ) + c.updateBindingStatus(toUpdate) c.recorder.Eventf(binding, api.EventTypeWarning, errorFindingNamespaceInstanceReason, s) return err } @@ -199,13 +236,14 @@ func (c *controller) reconcileBinding(binding *v1alpha1.Binding) error { if !isInstanceReady(instance) { s := fmt.Sprintf(`Binding cannot begin because referenced instance "%v/%v" is not ready`, instance.Namespace, instance.Name) glog.Info(s) - c.updateBindingCondition( - binding, + c.setBindingCondition( + toUpdate, v1alpha1.BindingConditionReady, v1alpha1.ConditionFalse, errorInstanceNotReadyReason, s, ) + c.updateBindingStatus(toUpdate) c.recorder.Eventf(binding, api.EventTypeWarning, errorInstanceNotReadyReason, s) return nil } @@ -235,24 +273,34 @@ func (c *controller) reconcileBinding(binding *v1alpha1.Binding) error { httpErr.Error(), ) glog.Warning(s) - c.updateBindingCondition( - binding, + + c.setBindingCondition( + toUpdate, + v1alpha1.BindingConditionFailed, + v1alpha1.ConditionTrue, + "BindingReturnedFailure", + s, + ) + c.setBindingCondition( + toUpdate, v1alpha1.BindingConditionReady, v1alpha1.ConditionFalse, errorBindCallReason, "Bind call failed. "+s) + c.updateBindingStatus(toUpdate) c.recorder.Event(binding, api.EventTypeWarning, errorBindCallReason, s) return err } s := fmt.Sprintf("Error creating Binding \"%s/%s\" for Instance \"%s/%s\" of ServiceClass %q at Broker %q: %s", binding.Name, binding.Namespace, instance.Namespace, instance.Name, serviceClass.Name, brokerName, err) glog.Warning(s) - c.updateBindingCondition( - binding, + c.setBindingCondition( + toUpdate, v1alpha1.BindingConditionReady, v1alpha1.ConditionFalse, errorBindCallReason, "Bind call failed. "+s) + c.updateBindingStatus(toUpdate) c.recorder.Event(binding, api.EventTypeWarning, errorBindCallReason, s) return err } @@ -261,23 +309,25 @@ func (c *controller) reconcileBinding(binding *v1alpha1.Binding) error { if err != nil { s := fmt.Sprintf("Error injecting binding results for Binding \"%s/%s\": %s", binding.Namespace, binding.Name, err) glog.Warning(s) - c.updateBindingCondition( - binding, + c.setBindingCondition( + toUpdate, v1alpha1.BindingConditionReady, v1alpha1.ConditionFalse, errorInjectingBindResultReason, "Error injecting bind result "+s, ) + c.updateBindingStatus(toUpdate) c.recorder.Event(binding, api.EventTypeWarning, errorInjectingBindResultReason, s) return err } - c.updateBindingCondition( - binding, + c.setBindingCondition( + toUpdate, v1alpha1.BindingConditionReady, v1alpha1.ConditionTrue, successInjectedBindResultReason, successInjectedBindResultMessage, ) + c.updateBindingStatus(toUpdate) c.recorder.Event(binding, api.EventTypeNormal, successInjectedBindResultReason, successInjectedBindResultMessage) glog.V(5).Infof("Successfully bound to Instance %v/%v of ServiceClass %v at Broker %v", instance.Namespace, instance.Name, serviceClass.Name, brokerName) @@ -294,13 +344,14 @@ func (c *controller) reconcileBinding(binding *v1alpha1.Binding) error { if err != nil { s := fmt.Sprintf("Error deleting secret: %s", err) glog.Warning(s) - c.updateBindingCondition( - binding, + c.setBindingCondition( + toUpdate, v1alpha1.BindingConditionReady, v1alpha1.ConditionUnknown, errorEjectingBindReason, errorEjectingBindMessage+s, ) + c.updateBindingStatus(toUpdate) c.recorder.Eventf(binding, api.EventTypeWarning, errorEjectingBindReason, "%v %v", errorEjectingBindMessage, s) return err } @@ -325,12 +376,13 @@ func (c *controller) reconcileBinding(binding *v1alpha1.Binding) error { httpErr.Error(), ) glog.Warning(s) - c.updateBindingCondition( - binding, + c.setBindingCondition( + toUpdate, v1alpha1.BindingConditionReady, v1alpha1.ConditionFalse, errorUnbindCallReason, "Unbind call failed. "+s) + c.updateBindingStatus(toUpdate) c.recorder.Event(binding, api.EventTypeWarning, errorUnbindCallReason, s) return err } @@ -345,26 +397,30 @@ func (c *controller) reconcileBinding(binding *v1alpha1.Binding) error { err, ) glog.Warning(s) - c.updateBindingCondition( - binding, + c.setBindingCondition( + toUpdate, v1alpha1.BindingConditionReady, v1alpha1.ConditionFalse, errorUnbindCallReason, "Unbind call failed. "+s) + c.updateBindingStatus(toUpdate) c.recorder.Event(binding, api.EventTypeWarning, errorUnbindCallReason, s) return err } - c.updateBindingCondition( - binding, + c.setBindingCondition( + toUpdate, v1alpha1.BindingConditionReady, v1alpha1.ConditionFalse, successUnboundReason, "The binding was deleted successfully", ) + c.updateBindingStatus(toUpdate) // Clear the finalizer finalizers.Delete(v1alpha1.FinalizerServiceCatalog) - c.updateBindingFinalizers(binding, finalizers.List()) + if err = c.updateBindingFinalizers(binding, finalizers.List()); err != nil { + return err + } c.recorder.Event(binding, api.EventTypeNormal, successUnboundReason, "This binding was deleted successfully") glog.V(5).Infof("Successfully deleted Binding %v/%v of Instance %v/%v of ServiceClass %v at Broker %v", binding.Namespace, binding.Name, instance.Namespace, instance.Name, serviceClass.Name, brokerName) @@ -472,19 +528,21 @@ func (c *controller) ejectBinding(binding *v1alpha1.Binding) error { return nil } -// updateBindingCondition updates the given condition for the given Binding -// with the given status, reason, and message. -func (c *controller) updateBindingCondition( - binding *v1alpha1.Binding, +func (c *controller) setBindingCondition(toUpdate *v1alpha1.Binding, conditionType v1alpha1.BindingConditionType, status v1alpha1.ConditionStatus, - reason, message string) error { + reason, message string) { - clone, err := api.Scheme.DeepCopy(binding) - if err != nil { - return err - } - toUpdate := clone.(*v1alpha1.Binding) + setBindingConditionInternal(toUpdate, conditionType, status, reason, message, metav1.Now()) +} + +func setBindingConditionInternal(toUpdate *v1alpha1.Binding, + conditionType v1alpha1.BindingConditionType, + status v1alpha1.ConditionStatus, + reason, message string, + t metav1.Time) { + + glog.V(5).Infof("Setting Binding '%v/%v' condition %q to %v", toUpdate.Namespace, toUpdate.Name, conditionType, status) newCondition := v1alpha1.BindingCondition{ Type: conditionType, @@ -493,34 +551,64 @@ func (c *controller) updateBindingCondition( Message: message, } - t := time.Now() - - if len(binding.Status.Conditions) == 0 { - glog.Infof(`Setting lastTransitionTime for Binding "%v/%v" condition %q to %v`, binding.Namespace, binding.Name, conditionType, t) - newCondition.LastTransitionTime = metav1.NewTime(t) + if len(toUpdate.Status.Conditions) == 0 { + glog.Infof(`Setting lastTransitionTime for Binding "%v/%v" condition %q to %v`, + toUpdate.Namespace, toUpdate.Name, conditionType, t) + newCondition.LastTransitionTime = t toUpdate.Status.Conditions = []v1alpha1.BindingCondition{newCondition} - } else { - for i, cond := range binding.Status.Conditions { - if cond.Type == conditionType { - if cond.Status != newCondition.Status { - glog.Infof(`Found status change for Binding "%v/%v" condition %q: %q -> %q; setting lastTransitionTime to %v`, binding.Namespace, binding.Name, conditionType, cond.Status, status, t) - newCondition.LastTransitionTime = metav1.NewTime(time.Now()) - } else { - newCondition.LastTransitionTime = cond.LastTransitionTime - } - - toUpdate.Status.Conditions[i] = newCondition - break + return + } + for i, cond := range toUpdate.Status.Conditions { + if cond.Type == conditionType { + if cond.Status != newCondition.Status { + glog.V(3).Infof(`Found status change for Binding "%v/%v" condition %q: %q -> %q; setting lastTransitionTime to %v`, + toUpdate.Namespace, toUpdate.Name, conditionType, cond.Status, status, t) + newCondition.LastTransitionTime = t + } else { + newCondition.LastTransitionTime = cond.LastTransitionTime } + + toUpdate.Status.Conditions[i] = newCondition + return } } - logContext := fmt.Sprintf("%v condition for Binding %v/%v to %v (Reason: %q, Message: %q)", + glog.V(3).Infof("Setting lastTransitionTime for Binding '%v/%v' condition %q to %v", + toUpdate.Namespace, toUpdate.Name, conditionType, t) + + newCondition.LastTransitionTime = t + toUpdate.Status.Conditions = append(toUpdate.Status.Conditions, newCondition) +} + +func (c *controller) updateBindingStatus(toUpdate *v1alpha1.Binding) error { + glog.V(4).Infof("Updating status for Binding %v/%v", toUpdate.Namespace, toUpdate.Name) + _, err := c.serviceCatalogClient.Bindings(toUpdate.Namespace).UpdateStatus(toUpdate) + if err != nil { + glog.Errorf("Error updating status for Binding %v/%v", toUpdate.Namespace, toUpdate.Name) + } + return err +} + +// updateBindingCondition updates the given condition for the given Binding +// with the given status, reason, and message. +func (c *controller) updateBindingCondition( + binding *v1alpha1.Binding, + conditionType v1alpha1.BindingConditionType, + status v1alpha1.ConditionStatus, + reason, message string) error { + + toUpdate, err := makeBindingClone(binding) + if err != nil { + return err + } + + c.setBindingCondition(toUpdate, conditionType, status, reason, message) + + glog.V(4).Infof("Updating %v condition for Binding %v/%v to %v (Reason: %q, Message: %q)", conditionType, binding.Namespace, binding.Name, status, reason, message) - glog.V(4).Infof("Updating %v", logContext) _, err = c.serviceCatalogClient.Bindings(binding.Namespace).UpdateStatus(toUpdate) if err != nil { - glog.Errorf("Error updating %v: %v", logContext, err) + glog.Errorf("Error updating %v condition for Binding %v/%v to %v: %v", conditionType, binding.Namespace, binding.Name, status, err) } return err } diff --git a/pkg/controller/controller_binding_test.go b/pkg/controller/controller_binding_test.go index ba3e29b9685a..c3cc3c10dffa 100644 --- a/pkg/controller/controller_binding_test.go +++ b/pkg/controller/controller_binding_test.go @@ -26,12 +26,14 @@ import ( "time" scmeta "github.com/kubernetes-incubator/service-catalog/pkg/api/meta" + checksum "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/checksum/versioned/v1alpha1" "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1alpha1" osb "github.com/pmorie/go-open-service-broker-client/v2" fakeosb "github.com/pmorie/go-open-service-broker-client/v2/fake" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/diff" "k8s.io/client-go/pkg/api" "k8s.io/client-go/pkg/api/v1" @@ -712,6 +714,217 @@ func TestReconcileBindingDelete(t *testing.T) { } } +// TestSetBindingCondition verifies setting a condition on a binding yields +// the results as expected with respect to the changed condition and transition +// time. +func TestSetBindingCondition(t *testing.T) { + bindingWithCondition := func(condition *v1alpha1.BindingCondition) *v1alpha1.Binding { + binding := getTestBinding() + binding.Status = v1alpha1.BindingStatus{ + Conditions: []v1alpha1.BindingCondition{*condition}, + } + + return binding + } + + // The value of the LastTransitionTime field on conditions has to be + // tested to ensure it is updated correctly. + // + // Time basis for all condition changes: + newTs := metav1.Now() + oldTs := metav1.NewTime(newTs.Add(-5 * time.Minute)) + + // condition is a shortcut method for creating conditions with the 'old' timestamp. + condition := func(cType v1alpha1.BindingConditionType, status v1alpha1.ConditionStatus, s ...string) *v1alpha1.BindingCondition { + c := &v1alpha1.BindingCondition{ + Type: cType, + Status: status, + } + + if len(s) > 0 { + c.Reason = s[0] + } + + if len(s) > 1 { + c.Message = s[1] + } + + // This is the expected 'before' timestamp for all conditions under + // test. + c.LastTransitionTime = oldTs + + return c + } + + // shortcut methods for creating conditions of different types + + readyFalse := func() *v1alpha1.BindingCondition { + return condition(v1alpha1.BindingConditionReady, v1alpha1.ConditionFalse, "Reason", "Message") + } + + readyFalsef := func(reason, message string) *v1alpha1.BindingCondition { + return condition(v1alpha1.BindingConditionReady, v1alpha1.ConditionFalse, reason, message) + } + + readyTrue := func() *v1alpha1.BindingCondition { + return condition(v1alpha1.BindingConditionReady, v1alpha1.ConditionTrue, "Reason", "Message") + } + + failedTrue := func() *v1alpha1.BindingCondition { + return condition(v1alpha1.BindingConditionFailed, v1alpha1.ConditionTrue, "Reason", "Message") + } + + // withNewTs sets the LastTransitionTime to the 'new' basis time and + // returns it. + withNewTs := func(c *v1alpha1.BindingCondition) *v1alpha1.BindingCondition { + c.LastTransitionTime = newTs + return c + } + + // this test works by calling setBindingCondition with the input and + // condition fields of the test case, and ensuring that afterward the + // input (which is mutated by the setBindingCondition call) is deep-equal + // to the test case result. + // + // take note of where withNewTs is used when declaring the result to + // indicate that the LastTransitionTime field on a condition should have + // changed. + cases := []struct { + name string + input *v1alpha1.Binding + condition *v1alpha1.BindingCondition + result *v1alpha1.Binding + }{ + { + name: "new ready condition", + input: getTestBinding(), + condition: readyFalse(), + result: bindingWithCondition(withNewTs(readyFalse())), + }, + { + name: "not ready -> not ready; no ts update", + input: bindingWithCondition(readyFalse()), + condition: readyFalse(), + result: bindingWithCondition(readyFalse()), + }, + { + name: "not ready -> not ready, reason and message change; no ts update", + input: bindingWithCondition(readyFalse()), + condition: readyFalsef("DifferentReason", "DifferentMessage"), + result: bindingWithCondition(readyFalsef("DifferentReason", "DifferentMessage")), + }, + { + name: "not ready -> ready", + input: bindingWithCondition(readyFalse()), + condition: readyTrue(), + result: bindingWithCondition(withNewTs(readyTrue())), + }, + { + name: "ready -> ready; no ts update", + input: bindingWithCondition(readyTrue()), + condition: readyTrue(), + result: bindingWithCondition(readyTrue()), + }, + { + name: "ready -> not ready", + input: bindingWithCondition(readyTrue()), + condition: readyFalse(), + result: bindingWithCondition(withNewTs(readyFalse())), + }, + { + name: "not ready -> not ready + failed", + input: bindingWithCondition(readyFalse()), + condition: failedTrue(), + result: func() *v1alpha1.Binding { + i := bindingWithCondition(readyFalse()) + i.Status.Conditions = append(i.Status.Conditions, *withNewTs(failedTrue())) + return i + }(), + }, + } + + for _, tc := range cases { + setBindingConditionInternal(tc.input, tc.condition.Type, tc.condition.Status, tc.condition.Reason, tc.condition.Message, newTs) + + if !reflect.DeepEqual(tc.input, tc.result) { + t.Errorf("%v: unexpected diff: %v", tc.name, diff.ObjectReflectDiff(tc.input, tc.result)) + } + } +} + +// TestReconcileBindingDeleteFailedBinding tests reconcileBinding to ensure +// a binding with a failed status is deleted properly. +func TestReconcileBindingDeleteFailedBinding(t *testing.T) { + fakeKubeClient, fakeCatalogClient, fakeBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + UnbindReaction: &fakeosb.UnbindReaction{}, + }) + + sharedInformers.Brokers().Informer().GetStore().Add(getTestBroker()) + sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass()) + sharedInformers.Instances().Informer().GetStore().Add(getTestInstance()) + + binding := getTestBindingWithFailedStatus() + binding.ObjectMeta.DeletionTimestamp = &metav1.Time{} + binding.ObjectMeta.Finalizers = []string{v1alpha1.FinalizerServiceCatalog} + + checksum := checksum.BindingSpecChecksum(binding.Spec) + binding.Status.Checksum = &checksum + + fakeCatalogClient.AddReactor("get", "bindings", func(action clientgotesting.Action) (bool, runtime.Object, error) { + return true, binding, nil + }) + + err := testController.reconcileBinding(binding) + if err != nil { + t.Fatalf("%v", err) + } + + brokerActions := fakeBrokerClient.Actions() + assertNumberOfBrokerActions(t, brokerActions, 1) + assertUnbind(t, brokerActions[0], &osb.UnbindRequest{ + BindingID: bindingGUID, + InstanceID: instanceGUID, + ServiceID: serviceClassGUID, + PlanID: planGUID, + }) + + // verify one kube action occurred + kubeActions := fakeKubeClient.Actions() + if err := checkKubeClientActions(kubeActions, []kubeClientAction{ + {verb: "delete", resourceName: "secrets", checkType: checkGetActionType}, + }); err != nil { + t.Fatal(err) + } + + deleteAction := kubeActions[0].(clientgotesting.DeleteActionImpl) + if e, a := binding.Spec.SecretName, deleteAction.Name; e != a { + t.Fatalf("Unexpected name of secret: expected %v, got %v", e, a) + } + + actions := fakeCatalogClient.Actions() + // The three actions should be: + // 0. Updating the ready condition + // 1. Get against the binding in question + // 2. Removing the finalizer + assertNumberOfActions(t, actions, 3) + + updatedBinding := assertUpdateStatus(t, actions[0], binding) + assertBindingReadyFalse(t, updatedBinding) + + assertGet(t, actions[1], binding) + + updatedBinding = assertUpdateStatus(t, actions[2], binding) + assertEmptyFinalizers(t, updatedBinding) + + events := getRecordedEvents(testController) + assertNumEvents(t, events, 1) + + expectedEvent := api.EventTypeNormal + " " + successUnboundReason + " " + "This binding was deleted successfully" + if e, a := expectedEvent, events[0]; e != a { + t.Fatalf("Received unexpected event: %v", a) + } +} + func TestReconcileBindingWithBrokerError(t *testing.T) { _, _, _, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ BindReaction: &fakeosb.BindReaction{ @@ -794,6 +1007,154 @@ func TestReconcileBindingWithBrokerHTTPError(t *testing.T) { } } +// TestReconcileBindingWithFailureCondition tests reconcileBinding to ensure +// no processing is done on a binding containing a failed status. +func TestReconcileBindingWithFailureCondition(t *testing.T) { + fakeKubeClient, fakeCatalogClient, fakeBrokerClient, testController, sharedInformers := newTestController(t, noFakeActions()) + + sharedInformers.Brokers().Informer().GetStore().Add(getTestBroker()) + sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass()) + sharedInformers.Instances().Informer().GetStore().Add(getTestInstanceWithStatus(v1alpha1.ConditionTrue)) + + binding := getTestBindingWithFailedStatus() + + if err := testController.reconcileBinding(binding); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + kubeActions := fakeKubeClient.Actions() + assertNumberOfActions(t, kubeActions, 0) + + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 0) + + brokerActions := fakeBrokerClient.Actions() + assertNumberOfBrokerActions(t, brokerActions, 0) + + events := getRecordedEvents(testController) + assertNumEvents(t, events, 0) +} + +// TestReconcileBindingWithBindingCallFailure tests reconcileBinding to ensure +// a bind creation failure is handled properly. +func TestReconcileBindingWithBindingCallFailure(t *testing.T) { + fakeKubeClient, fakeCatalogClient, fakeBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + BindReaction: &fakeosb.BindReaction{ + Error: errors.New("fake creation failure"), + }, + }) + + sharedInformers.Brokers().Informer().GetStore().Add(getTestBroker()) + sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass()) + sharedInformers.Instances().Informer().GetStore().Add(getTestInstanceWithStatus(v1alpha1.ConditionTrue)) + + binding := getTestBinding() + + if err := testController.reconcileBinding(binding); err == nil { + t.Fatal("Binding creation should fail") + } + + // verify one kube action occurred + kubeActions := fakeKubeClient.Actions() + if err := checkKubeClientActions(kubeActions, []kubeClientAction{ + {verb: "get", resourceName: "namespaces", checkType: checkGetActionType}, + }); err != nil { + t.Fatal(err) + } + + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 1) + + brokerActions := fakeBrokerClient.Actions() + assertNumberOfBrokerActions(t, brokerActions, 1) + assertBind(t, brokerActions[0], &osb.BindRequest{ + BindingID: bindingGUID, + InstanceID: instanceGUID, + ServiceID: serviceClassGUID, + PlanID: planGUID, + AppGUID: strPtr(""), + BindResource: &osb.BindResource{ + AppGUID: strPtr(""), + }, + }) + + events := getRecordedEvents(testController) + assertNumEvents(t, events, 1) + + expectedEvent := api.EventTypeWarning + " " + errorBindCallReason + " " + "Error creating Binding \"test-binding/test-ns\" for Instance \"test-ns/test-instance\" of ServiceClass \"test-serviceclass\" at Broker \"test-broker\": fake creation failure" + + if e, a := expectedEvent, events[0]; e != a { + t.Fatalf("Received unexpected event: %v", a) + } +} + +// TestReconcileBindingWithBindingFailure tests reconcileBinding to ensure +// a binding request that receives an error from the broker is handled properly. +func TestReconcileBindingWithBindingFailure(t *testing.T) { + fakeKubeClient, fakeCatalogClient, fakeBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + BindReaction: &fakeosb.BindReaction{ + Error: osb.HTTPStatusCodeError{ + StatusCode: http.StatusConflict, + ErrorMessage: strPtr("ServiceBindingExists"), + Description: strPtr("Service binding with the same id, for the same service instance already exists."), + }, + }, + }) + + sharedInformers.Brokers().Informer().GetStore().Add(getTestBroker()) + sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass()) + sharedInformers.Instances().Informer().GetStore().Add(getTestInstanceWithStatus(v1alpha1.ConditionTrue)) + + binding := getTestBinding() + + if err := testController.reconcileBinding(binding); err == nil { + t.Fatal("Binding creation should fail") + } + + // verify one kube action occurred + kubeActions := fakeKubeClient.Actions() + if err := checkKubeClientActions(kubeActions, []kubeClientAction{ + {verb: "get", resourceName: "namespaces", checkType: checkGetActionType}, + }); err != nil { + t.Fatal(err) + } + + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 1) + + updatedObject := assertUpdateStatus(t, actions[0], binding) + assertBindingReadyFalse(t, updatedObject) + updatedBinding, ok := updatedObject.(*v1alpha1.Binding) + if !ok { + t.Fatal("Couldn't convert to v1alpha1.Binding") + } + if num := len(updatedBinding.Status.Conditions); num != 2 { + t.Fatalf("Expected two conditions, got %v", num) + } + + brokerActions := fakeBrokerClient.Actions() + assertNumberOfBrokerActions(t, brokerActions, 1) + assertBind(t, brokerActions[0], &osb.BindRequest{ + BindingID: bindingGUID, + InstanceID: instanceGUID, + ServiceID: serviceClassGUID, + PlanID: planGUID, + AppGUID: strPtr(""), + BindResource: &osb.BindResource{ + AppGUID: strPtr(""), + }, + }) + + events := getRecordedEvents(testController) + assertNumEvents(t, events, 1) + + expectedEvent := api.EventTypeWarning + " " + errorBindCallReason + " " + "Error creating Binding \"test-binding/test-ns\" for Instance \"test-ns/test-instance\" of ServiceClass \"test-serviceclass\" at Broker \"test-broker\", Status: 409; ErrorMessage: ServiceBindingExists; Description: Service binding with the same id, for the same service instance already exists.; ResponseError: " + + if e, a := expectedEvent, events[0]; e != a { + t.Fatalf("Received unexpected event: %v", a) + } +} + func TestUpdateBindingCondition(t *testing.T) { getTestBindingWithStatus := func(status v1alpha1.ConditionStatus) *v1alpha1.Binding { instance := getTestBinding() diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 0d6152372d3c..cb6d5205f7dc 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -511,6 +511,18 @@ func getTestBinding() *v1alpha1.Binding { } } +func getTestBindingWithFailedStatus() *v1alpha1.Binding { + binding := getTestBinding() + binding.Status = v1alpha1.BindingStatus{ + Conditions: []v1alpha1.BindingCondition{{ + Type: v1alpha1.BindingConditionFailed, + Status: v1alpha1.ConditionTrue, + }}, + } + + return binding +} + type instanceParameters struct { Name string `json:"name"` Args map[string]string `json:"args"` @@ -1420,7 +1432,7 @@ func testNumberOfBrokerActions(t *testing.T, name string, f failfFunc, actions [ if e, a := number, len(actions); e != a { t.Logf("%+v\n", actions) - f(t, "%vUnexpected number of actions: expected %v, got %v", logContext, e, a) + f(t, "%vUnexpected number of actions: expected %v, got %v\nactions: %+v", logContext, e, a, actions) return false } diff --git a/test/integration/controller_test.go b/test/integration/controller_test.go index 3f4fdf1290e4..a3448c48f1e0 100644 --- a/test/integration/controller_test.go +++ b/test/integration/controller_test.go @@ -575,6 +575,184 @@ func TestProvisionFailure(t *testing.T) { } } +// TestBindingFailure tests that a binding gets a failure condition when the +// broker returns a failure response for a bind operation. +func TestBindingFailure(t *testing.T) { + _, fakeCatalogClient, _, _, _, shutdownServer := newTestController(t, fakeosb.FakeClientConfiguration{ + CatalogReaction: &fakeosb.CatalogReaction{ + Response: &osb.CatalogResponse{ + Services: []osb.Service{ + { + Name: testServiceClassName, + ID: "12345", + Description: "a test service", + Bindable: true, + Plans: []osb.Plan{ + { + Name: testPlanName, + Free: truePtr(), + ID: "34567", + Description: "a test plan", + }, + }, + }, + }, + }, + }, + BindReaction: &fakeosb.BindReaction{ + Error: osb.HTTPStatusCodeError{ + StatusCode: http.StatusConflict, + ErrorMessage: strPtr("ServiceBindingExists"), + Description: strPtr("Service binding with the same id, for the same service instance already exists."), + }, + }, + UnbindReaction: &fakeosb.UnbindReaction{}, + ProvisionReaction: &fakeosb.ProvisionReaction{ + Response: &osb.ProvisionResponse{ + Async: false, + }, + }, + DeprovisionReaction: &fakeosb.DeprovisionReaction{ + Response: &osb.DeprovisionResponse{ + Async: false, + }, + }, + }) + defer shutdownServer() + + client := fakeCatalogClient.ServicecatalogV1alpha1() + broker := &v1alpha1.Broker{ + ObjectMeta: metav1.ObjectMeta{Name: testBrokerName}, + Spec: v1alpha1.BrokerSpec{ + URL: testBrokerURL, + }, + } + + _, err := client.Brokers().Create(broker) + if nil != err { + t.Fatalf("error creating the broker %q (%q)", broker, err) + } + + err = util.WaitForBrokerCondition(client, + testBrokerName, + v1alpha1.BrokerCondition{ + Type: v1alpha1.BrokerConditionReady, + Status: v1alpha1.ConditionTrue, + }) + if err != nil { + t.Fatalf("error waiting for broker to become ready: %v", err) + } + + err = util.WaitForServiceClassToExist(client, testServiceClassName) + if nil != err { + t.Fatalf("error waiting from ServiceClass to exist: %v", err) + } + + // TODO: find some way to compose scenarios; extract method here for real + // logic for this test. + + //----------------- + + instance := &v1alpha1.Instance{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testInstanceName}, + Spec: v1alpha1.InstanceSpec{ + ServiceClassName: testServiceClassName, + PlanName: testPlanName, + ExternalID: testExternalID, + }, + } + + if _, err := client.Instances(testNamespace).Create(instance); err != nil { + t.Fatalf("error creating Instance: %v", err) + } + + if err := util.WaitForInstanceCondition(client, testNamespace, testInstanceName, v1alpha1.InstanceCondition{ + Type: v1alpha1.InstanceConditionReady, + Status: v1alpha1.ConditionTrue, + }); err != nil { + t.Fatalf("error waiting for instance to become ready: %v", err) + } + + retInst, err := client.Instances(instance.Namespace).Get(instance.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("error getting instance %s/%s back", instance.Namespace, instance.Name) + } + if retInst.Spec.ExternalID != instance.Spec.ExternalID { + t.Fatalf( + "returned OSB GUID '%s' doesn't match original '%s'", + retInst.Spec.ExternalID, + instance.Spec.ExternalID, + ) + } + + // Binding test begins here + //----------------- + + binding := &v1alpha1.Binding{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testBindingName}, + Spec: v1alpha1.BindingSpec{ + InstanceRef: v1.LocalObjectReference{ + Name: testInstanceName, + }, + }, + } + + _, err = client.Bindings(testNamespace).Create(binding) + if err != nil { + t.Fatalf("error creating Binding: %v", binding) + } + + err = util.WaitForBindingCondition(client, testNamespace, testBindingName, v1alpha1.BindingCondition{ + Type: v1alpha1.BindingConditionFailed, + Status: v1alpha1.ConditionTrue, + }) + if err != nil { + t.Fatalf("error waiting for binding to become failed: %v", err) + } + + err = client.Bindings(testNamespace).Delete(testBindingName, &metav1.DeleteOptions{}) + if err != nil { + t.Fatalf("binding delete should have been accepted: %v", err) + } + + err = util.WaitForBindingToNotExist(client, testNamespace, testBindingName) + if err != nil { + t.Fatalf("error waiting for binding to not exist: %v", err) + } + + //----------------- + // End binding test + + err = client.Instances(testNamespace).Delete(testInstanceName, &metav1.DeleteOptions{}) + if nil != err { + t.Fatalf("instance delete should have been accepted: %v", err) + } + + err = util.WaitForInstanceToNotExist(client, testNamespace, testInstanceName) + if err != nil { + t.Fatalf("error waiting for instance to be deleted: %v", err) + } + + //----------------- + // End provision test + + // Delete the broker + err = client.Brokers().Delete(testBrokerName, &metav1.DeleteOptions{}) + if nil != err { + t.Fatalf("broker should be deleted (%s)", err) + } + + err = util.WaitForServiceClassToNotExist(client, testServiceClassName) + if err != nil { + t.Fatalf("error waiting for ServiceClass to not exist: %v", err) + } + + err = util.WaitForBrokerToNotExist(client, testBrokerName) + if err != nil { + t.Fatalf("error waiting for Broker to not exist: %v", err) + } +} + // newTestController creates a new test controller injected with fake clients // and returns: //