Create an overall Ready to indicate whether the SBR reconcile is co…

…mpleted successfully #665 (#681)

pkg/controller/servicebinding/reconciler.go

@@ -6,6 +6,7 @@ import (
 
 	conditionsv1 "github.com/openshift/custom-resource-status/conditions/v1"
 	corev1 "k8s.io/api/core/v1"
+	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -15,10 +16,13 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	"github.com/redhat-developer/service-binding-operator/pkg/apis/operators/v1alpha1"
+	"github.com/redhat-developer/service-binding-operator/pkg/converter"
 	"github.com/redhat-developer/service-binding-operator/pkg/log"
 )
 
 const (
+	// BindingReady indicates that the overall sbr succeeded
+	BindingReady conditionsv1.ConditionType = "Ready"
 	// CollectionReady indicates readiness for collection and persistance of intermediate manifests
 	CollectionReady conditionsv1.ConditionType = "CollectionReady"
 	// InjectionReady indicates readiness to change application manifests to use those intermediate manifests
@@ -32,6 +36,8 @@ const (
 	EmptyApplicationReason = "EmptyApplication"
 	// ApplicationNotFoundReason is used when the application is not found.
 	ApplicationNotFoundReason = "ApplicationNotFound"
+	// ServiceNotFoundReason is used when the service is not found.
+	ServiceNotFoundReason = "ServiceNotFound"
 )
 
 // Reconciler reconciles a ServiceBinding object
@@ -131,10 +137,12 @@ func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err
 				Message: errEmptyServices.Error(),
 			},
 			conditionsv1.Condition{
-				Type:    InjectionReady,
-				Status:  corev1.ConditionFalse,
-				Reason:  EmptyServiceSelectorsReason,
-				Message: errEmptyServices.Error(),
+				Type:   InjectionReady,
+				Status: corev1.ConditionFalse,
+			},
+			conditionsv1.Condition{
+				Type:   BindingReady,
+				Status: corev1.ConditionFalse,
 			},
 		)
 		if updateErr == nil {
@@ -157,9 +165,30 @@ func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err
 		r.restMapper,
 	)
 	if err != nil {
+		//handle service not found error
+		if k8serrors.IsNotFound(err) {
+			err = updateSBRConditions(r.dynClient, sbr,
+				conditionsv1.Condition{
+					Type:    CollectionReady,
+					Status:  corev1.ConditionFalse,
+					Reason:  ServiceNotFoundReason,
+					Message: err.Error(),
+				},
+				conditionsv1.Condition{
+					Type:   InjectionReady,
+					Status: corev1.ConditionFalse,
+				},
+				conditionsv1.Condition{
+					Type:   BindingReady,
+					Status: corev1.ConditionFalse,
+				},
+			)
+			if err != nil {
+				logger.Error(err, "Failed to update SBR conditions", "sbr", sbr)
+			}
+		}
 		return requeueError(err)
 	}
-
 	binding, err := buildBinding(
 		r.dynClient,
 		sbr.Spec.CustomEnvVar,
@@ -220,3 +249,21 @@ func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err
 	logger.Info("Binding applications with intermediary secret...")
 	return sb.bind()
 }
+
+func updateSBRConditions(dynClient dynamic.Interface, sbr *v1alpha1.ServiceBinding, conditions ...conditionsv1.Condition) error {
+	for _, v := range conditions {
+		conditionsv1.SetStatusCondition(&sbr.Status.Conditions, v)
+	}
+	u, err := converter.ToUnstructured(sbr)
+	if err != nil {
+		return err
+	}
+
+	nsClient := dynClient.
+		Resource(groupVersion).
+		Namespace(sbr.GetNamespace())
+
+	_, err = nsClient.UpdateStatus(u, metav1.UpdateOptions{})
+
+	return err
+}

pkg/controller/servicebinding/reconciler_test.go

@@ -133,6 +133,7 @@ func TestApplicationByName(t *testing.T) {
 
 		requireConditionPresentAndTrue(t, CollectionReady, sbrOutput.Status.Conditions)
 		requireConditionPresentAndTrue(t, InjectionReady, sbrOutput.Status.Conditions)
+		requireConditionPresentAndTrue(t, BindingReady, sbrOutput.Status.Conditions)
 
 		require.Equal(t, 1, len(sbrOutput.Status.Applications))
 		expectedStatus := v1alpha1.BoundApplication{
@@ -196,6 +197,7 @@ func TestReconcilerReconcileUsingSecret(t *testing.T) {
 
 		requireConditionPresentAndTrue(t, CollectionReady, sbrOutput.Status.Conditions)
 		requireConditionPresentAndTrue(t, InjectionReady, sbrOutput.Status.Conditions)
+		requireConditionPresentAndTrue(t, BindingReady, sbrOutput.Status.Conditions)
 
 		require.Equal(t, reconcilerName, sbrOutput.Status.Secret)
 
@@ -259,6 +261,59 @@ func TestReconcilerReconcileUsingVolumes(t *testing.T) {
 	})
 }
 
+func TestServiceNotFound(t *testing.T) {
+	backingServiceResourceRef := "backingServiceRef"
+	applicationResourceRef := "applicationRef"
+	f := mocks.NewFake(t, reconcilerNs)
+	f.AddMockedUnstructuredServiceBinding(reconcilerName, backingServiceResourceRef, applicationResourceRef, deploymentsGVR, nil)
+	f.AddMockedUnstructuredDatabaseCRD()
+	f.AddMockedUnstructuredDeployment(applicationResourceRef, nil)
+
+	fakeDynClient := f.FakeDynClient()
+	mapper := testutils.BuildTestRESTMapper()
+	r := &reconciler{dynClient: fakeDynClient, restMapper: mapper, scheme: f.S}
+	r.resourceWatcher = newFakeResourceWatcher(mapper)
+
+	// Reconcile without service
+	res, _ := r.Reconcile(reconcileRequest())
+	require.True(t, res.Requeue)
+
+	namespacedName := types.NamespacedName{Namespace: reconcilerNs, Name: reconcilerName}
+	sbrOutput, err := r.getServiceBinding(namespacedName)
+	require.NoError(t, err)
+
+	requireConditionPresentAndFalse(t, CollectionReady, sbrOutput.Status.Conditions)
+	requireConditionPresentAndFalse(t, InjectionReady, sbrOutput.Status.Conditions)
+	requireConditionPresentAndFalse(t, BindingReady, sbrOutput.Status.Conditions)
+	require.Len(t, sbrOutput.Status.Applications, 0)
+
+	// Reconcile with service
+	f.AddMockedUnstructuredDatabaseCR(backingServiceResourceRef)
+	f.AddMockedUnstructuredSecret("db-credentials")
+	fakeDynClient = f.FakeDynClient()
+	r = &reconciler{dynClient: fakeDynClient, restMapper: testutils.BuildTestRESTMapper(), scheme: f.S}
+	r.resourceWatcher = newFakeResourceWatcher(mapper)
+	res, err = r.Reconcile(reconcileRequest())
+	require.NoError(t, err)
+	require.False(t, res.Requeue)
+
+	u, err := fakeDynClient.Resource(deploymentsGVR).Get(applicationResourceRef, metav1.GetOptions{})
+	require.NoError(t, err)
+
+	d := appsv1.Deployment{}
+	err = runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &d)
+	require.NoError(t, err)
+	sbrOutput2, err := r.getServiceBinding(namespacedName)
+	require.NoError(t, err)
+
+	requireConditionPresentAndTrue(t, CollectionReady, sbrOutput2.Status.Conditions)
+	requireConditionPresentAndTrue(t, InjectionReady, sbrOutput2.Status.Conditions)
+	requireConditionPresentAndTrue(t, BindingReady, sbrOutput2.Status.Conditions)
+
+	require.Equal(t, reconcilerName, sbrOutput2.Status.Secret)
+	require.Equal(t, 1, len(sbrOutput2.Status.Applications))
+}
+
 func TestApplicationNotFound(t *testing.T) {
 	backingServiceResourceRef := "backingService1"
 	matchLabels := map[string]string{
@@ -288,6 +343,7 @@ func TestApplicationNotFound(t *testing.T) {
 
 	requireConditionPresentAndTrue(t, CollectionReady, sbrOutput.Status.Conditions)
 	requireConditionPresentAndFalse(t, InjectionReady, sbrOutput.Status.Conditions)
+	requireConditionPresentAndFalse(t, BindingReady, sbrOutput.Status.Conditions)
 	require.Len(t, sbrOutput.Status.Applications, 0)
 
 	// Reconcile with deployment
@@ -310,6 +366,7 @@ func TestApplicationNotFound(t *testing.T) {
 
 	requireConditionPresentAndTrue(t, CollectionReady, sbrOutput2.Status.Conditions)
 	requireConditionPresentAndTrue(t, InjectionReady, sbrOutput2.Status.Conditions)
+	requireConditionPresentAndTrue(t, BindingReady, sbrOutput2.Status.Conditions)
 
 	require.Equal(t, reconcilerName, sbrOutput2.Status.Secret)
 	require.Equal(t, 1, len(sbrOutput2.Status.Applications))
@@ -361,6 +418,7 @@ func TestReconcilerUpdateCredentials(t *testing.T) {
 
 	requireConditionPresentAndTrue(t, CollectionReady, sbrOutput3.Status.Conditions)
 	requireConditionPresentAndTrue(t, InjectionReady, sbrOutput3.Status.Conditions)
+	requireConditionPresentAndTrue(t, BindingReady, sbrOutput3.Status.Conditions)
 
 	require.Equal(t, reconcilerName, sbrOutput3.Status.Secret)
 	require.Equal(t, s.Data["password"], []byte("abc123"))
@@ -420,6 +478,7 @@ func TestReconcilerReconcileWithConflictingAppSelc(t *testing.T) {
 
 		requireConditionPresentAndTrue(t, CollectionReady, sbrOutput.Status.Conditions)
 		requireConditionPresentAndTrue(t, InjectionReady, sbrOutput.Status.Conditions)
+		requireConditionPresentAndTrue(t, BindingReady, sbrOutput.Status.Conditions)
 
 		require.Equal(t, reconcilerName, sbrOutput.Status.Secret)
 		require.Len(t, sbrOutput.Status.Applications, 1)
@@ -449,6 +508,32 @@ func TestEmptyApplication(t *testing.T) {
 
 	requireConditionPresentAndTrue(t, CollectionReady, sbrOutput.Status.Conditions)
 	requireConditionPresentAndFalse(t, InjectionReady, sbrOutput.Status.Conditions)
+	requireConditionPresentAndTrue(t, BindingReady, sbrOutput.Status.Conditions)
+}
+
+// TestEmptyServiceSelector tests that CollectionReady,InjectionReady and BindingReady are all successfully updated to True when ServiceSelector is empty
+func TestEmptyServiceSelectorAndAllConditionAreSetToFalse(t *testing.T) {
+	applicationResourceRef := "applicationRef"
+	f := mocks.NewFake(t, reconcilerNs)
+	f.AddMockedUnstructuredServiceBindingWithoutService(reconcilerName, applicationResourceRef, deploymentsGVR)
+
+	fakeDynClient := f.FakeDynClient()
+	mapper := testutils.BuildTestRESTMapper()
+	r := &reconciler{dynClient: fakeDynClient, restMapper: mapper, scheme: f.S}
+	r.resourceWatcher = newFakeResourceWatcher(mapper)
+
+	res, err := r.Reconcile(reconcileRequest())
+	require.NoError(t, err)
+	require.False(t, res.Requeue)
+
+	namespacedName := types.NamespacedName{Namespace: reconcilerNs, Name: reconcilerName}
+	sbrOutput, err := r.getServiceBinding(namespacedName)
+	require.NoError(t, err)
+
+	requireConditionPresentAndFalse(t, CollectionReady, sbrOutput.Status.Conditions)
+	requireConditionPresentAndFalse(t, InjectionReady, sbrOutput.Status.Conditions)
+	requireConditionPresentAndFalse(t, BindingReady, sbrOutput.Status.Conditions)
+
 }
 
 func TestBindTwoSbrsWithSingleApplication(t *testing.T) {
@@ -505,6 +590,7 @@ func TestBindTwoSbrsWithSingleApplication(t *testing.T) {
 
 		requireConditionPresentAndTrue(t, CollectionReady, sbrOutput.Status.Conditions)
 		requireConditionPresentAndTrue(t, InjectionReady, sbrOutput.Status.Conditions)
+		requireConditionPresentAndTrue(t, BindingReady, sbrOutput.Status.Conditions)
 		require.Equal(t, sbrName1, sbrOutput.Status.Secret)
 		require.Len(t, sbrOutput.Status.Applications, 1)
 		require.True(t, reflect.DeepEqual(expectedStatus, sbrOutput.Status.Applications[0]))
@@ -542,6 +628,7 @@ func TestBindTwoSbrsWithSingleApplication(t *testing.T) {
 
 		requireConditionPresentAndTrue(t, CollectionReady, sbrOutput.Status.Conditions)
 		requireConditionPresentAndTrue(t, InjectionReady, sbrOutput.Status.Conditions)
+		requireConditionPresentAndTrue(t, BindingReady, sbrOutput.Status.Conditions)
 		require.Equal(t, sbrName2, sbrOutput.Status.Secret)
 		require.Len(t, sbrOutput.Status.Applications, 1)
 		require.True(t, reflect.DeepEqual(expectedStatus, sbrOutput.Status.Applications[0]))

pkg/controller/servicebinding/servicebinder.go

@@ -238,6 +238,10 @@ func (b *serviceBinder) onError(
 		Reason:  bindingFail,
 		Message: b.message(err),
 	})
+	conditionsv1.SetStatusCondition(&sbrStatus.Conditions, conditionsv1.Condition{
+		Type:   BindingReady,
+		Status: corev1.ConditionFalse,
+	})
 	newSbr, errStatus := b.updateStatusServiceBinding(sbr, sbrStatus)
 	if errStatus != nil {
 		return requeueError(errStatus)
@@ -274,14 +278,7 @@ func removeFinalizer(sbr *v1alpha1.ServiceBinding) {
 // handleApplicationError handles scenarios when:
 // 1. application not declared in the Service Binding
 // 2. application not found
-func (b *serviceBinder) handleApplicationError(reason string, applicationError error, sbrStatus *v1alpha1.ServiceBindingStatus) (reconcile.Result, error) {
-	conditionsv1.SetStatusCondition(&sbrStatus.Conditions, conditionsv1.Condition{
-		Type:    InjectionReady,
-		Status:  corev1.ConditionFalse,
-		Reason:  reason,
-		Message: applicationError.Error(),
-	})
-
+func (b *serviceBinder) handleApplicationError(applicationError error, sbrStatus *v1alpha1.ServiceBindingStatus) (reconcile.Result, error) {
 	// updating status of request instance
 	sbr, err := b.updateStatusServiceBinding(b.sbr, sbrStatus)
 	if err != nil {
@@ -320,13 +317,33 @@ func (b *serviceBinder) bind() (reconcile.Result, error) {
 	})
 
 	if isApplicationEmpty(b.sbr.Spec.Application) {
-		return b.handleApplicationError(EmptyApplicationReason, errEmptyApplication, sbrStatus)
+		conditionsv1.SetStatusCondition(&sbrStatus.Conditions, conditionsv1.Condition{
+			Type:    InjectionReady,
+			Status:  corev1.ConditionFalse,
+			Reason:  EmptyApplicationReason,
+			Message: errEmptyApplication.Error(),
+		})
+		conditionsv1.SetStatusCondition(&sbrStatus.Conditions, conditionsv1.Condition{
+			Type:   BindingReady,
+			Status: corev1.ConditionTrue,
+		})
+		return b.handleApplicationError(errEmptyApplication, sbrStatus)
 	}
 	updatedObjects, err := b.binder.bind()
 	if err != nil {
 		b.logger.Error(err, "On binding application.")
 		if errors.Is(err, errApplicationNotFound) {
-			return b.handleApplicationError(ApplicationNotFoundReason, errApplicationNotFound, sbrStatus)
+			conditionsv1.SetStatusCondition(&sbrStatus.Conditions, conditionsv1.Condition{
+				Type:    InjectionReady,
+				Status:  corev1.ConditionFalse,
+				Reason:  ApplicationNotFoundReason,
+				Message: errApplicationNotFound.Error(),
+			})
+			conditionsv1.SetStatusCondition(&sbrStatus.Conditions, conditionsv1.Condition{
+				Type:   BindingReady,
+				Status: corev1.ConditionFalse,
+			})
+			return b.handleApplicationError(errApplicationNotFound, sbrStatus)
 		}
 		return b.onError(err, b.sbr, sbrStatus, nil)
 	}
@@ -336,6 +353,10 @@ func (b *serviceBinder) bind() (reconcile.Result, error) {
 		Type:   InjectionReady,
 		Status: corev1.ConditionTrue,
 	})
+	conditionsv1.SetStatusCondition(&sbrStatus.Conditions, conditionsv1.Condition{
+		Type:   BindingReady,
+		Status: corev1.ConditionTrue,
+	})
 
 	// updating status of request instance
 	sbr, err := b.updateStatusServiceBinding(b.sbr, sbrStatus)