bugfix(spacebindingrequest): allow deletion of failed SpaceBindingReq…

…uest (#938)

* fix check for existing SpaceBinding
---------
Co-authored-by: Andy Sadler <ansadler@redhat.com>
Co-authored-by: Matous Jobanek <mjobanek@redhat.com>

controllers/spacebindingrequest/spacebindingrequest_controller.go

@@ -124,10 +124,14 @@ func (r *Reconciler) ensureSpaceBindingDeletion(ctx context.Context, memberClust
 	if err != nil {
 		return err
 	}
-	if isBeingDeleted, err := r.deleteSpaceBinding(ctx, spaceBinding); err != nil {
-		return r.setStatusTerminatingFailed(ctx, memberClusterWithSpaceBindingRequest, spaceBindingRequest, err)
-	} else if isBeingDeleted {
-		return r.setStatusTerminating(ctx, memberClusterWithSpaceBindingRequest, spaceBindingRequest)
+
+	// delete the SpaceBinding only if it's owned by the current SBR
+	if spaceBinding != nil && sbrOwnsSpaceBinding(spaceBinding, spaceBindingRequest) {
+		if isBeingDeleted, err := r.deleteSpaceBinding(ctx, spaceBinding); err != nil {
+			return r.setStatusTerminatingFailed(ctx, memberClusterWithSpaceBindingRequest, spaceBindingRequest, err)
+		} else if isBeingDeleted {
+			return r.setStatusTerminating(ctx, memberClusterWithSpaceBindingRequest, spaceBindingRequest)
+		}
 	}
 
 	// Remove finalizer from SpaceBindingRequest
@@ -166,15 +170,6 @@ func (r *Reconciler) getSpaceBinding(ctx context.Context, spaceBindingRequest *t
 		return nil, space, fmt.Errorf("expected 1 spacebinding for Space %s and MUR %s. But found %d", space.GetName(), spaceBindingRequest.Spec.MasterUserRecord, len(spaceBindings.Items))
 	}
 
-	// check if existing spacebinding was created from spaceBindingRequest
-	if len(spaceBindings.Items) == 1 {
-		sbrLabel := spaceBindings.Items[0].Labels[toolchainv1alpha1.SpaceBindingRequestLabelKey]
-		sbrNamespaceLabel := spaceBindings.Items[0].Labels[toolchainv1alpha1.SpaceBindingRequestNamespaceLabelKey]
-		if sbrLabel != spaceBindingRequest.GetName() || sbrNamespaceLabel != spaceBindingRequest.GetNamespace() {
-			return nil, space, fmt.Errorf("A SpaceBinding for Space '%s' and MUR '%s' already exists, but it's not managed by this SpaceBindingRequest CR. It's not allowed to create multiple SpaceBindings for the same combination of Space and MasterUserRecord", space.GetName(), spaceBindingRequest.Spec.MasterUserRecord)
-		}
-	}
-
 	return &spaceBindings.Items[0], space, nil // all good
 }
 
@@ -231,6 +226,13 @@ func (r *Reconciler) ensureSpaceBinding(ctx context.Context, memberCluster clust
 	if err != nil {
 		return err
 	}
+
+	// check if existing spacebinding was created from spaceBindingRequest
+	// and return an error in case it was not.
+	if spaceBinding != nil && !sbrOwnsSpaceBinding(spaceBinding, spaceBindingRequest) {
+		return fmt.Errorf("A SpaceBinding for Space '%s' and MUR '%s' already exists, but it's not managed by this SpaceBindingRequest CR. It's not allowed to create multiple SpaceBindings for the same combination of Space and MasterUserRecord", space.GetName(), spaceBindingRequest.Spec.MasterUserRecord)
+	}
+
 	// space is being deleted
 	if util.IsBeingDeleted(space) {
 		return errs.New("space is being deleted")
@@ -260,6 +262,15 @@ func (r *Reconciler) ensureSpaceBinding(ctx context.Context, memberCluster clust
 	return r.updateExistingSpaceBinding(ctx, spaceBindingRequest, spaceBinding)
 }
 
+// sbrOwnsSpaceBinding check if the SpaceBinding was created by the given SpaceBindingRequest resource
+// returns true if it was created by the SBR
+// returns false if it wasn't created by the SBR
+func sbrOwnsSpaceBinding(spaceBinding *toolchainv1alpha1.SpaceBinding, spaceBindingRequest *toolchainv1alpha1.SpaceBindingRequest) bool {
+	sbrLabel := spaceBinding.Labels[toolchainv1alpha1.SpaceBindingRequestLabelKey]
+	sbrNamespaceLabel := spaceBinding.Labels[toolchainv1alpha1.SpaceBindingRequestNamespaceLabelKey]
+	return sbrLabel == spaceBindingRequest.GetName() && sbrNamespaceLabel == spaceBindingRequest.GetNamespace()
+}
+
 // updateExistingSpaceBinding updates the spacebinding with the config from the spaceBindingRequest.
 // returns true/nil if the spacebinding was updated
 // returns false/nil if the spacebinding was already up-to-date

controllers/spacebindingrequest/spacebindingrequest_controller_test.go

@@ -499,6 +499,35 @@ func TestCreateSpaceBindingRequest(t *testing.T) {
 				HasFinalizer()
 		})
 
+		t.Run("SpaceBindingRequest CR is deleted but admin-generated SpaceBinding stays there", func(t *testing.T) {
+			// given
+			spaceBinding := spacebindingcommon.NewSpaceBinding(janeMur, janeSpace, "john") // there is already an admin generated SpaceBinding
+			// this SBR will fail for the conflict with the already existing SpaceBinding
+			// but we should be able to delete it
+			sbrForDuplicatedSpaceBinding := spacebindingrequesttest.NewSpaceBindingRequest("jane", "jane-tenant",
+				spacebindingrequesttest.WithMUR(janeMur.Name),
+				spacebindingrequesttest.WithSpaceRole("admin"),
+				spacebindingrequesttest.WithFinalizer(), // we set the finalizer so we can check if it's being removed
+				spacebindingrequesttest.WithDeletionTimestamp(),
+			)
+
+			member1 := NewMemberClusterWithClient(test.NewFakeClient(t, sbrNamespace, sbrForDuplicatedSpaceBinding), "member-1", corev1.ConditionTrue)
+			hostClient := test.NewFakeClient(t, base1nsTier, janeSpace, janeMur, spaceBinding)
+			ctrl := newReconciler(t, hostClient, member1)
+
+			// when
+			_, err := ctrl.Reconcile(context.TODO(), requestFor(sbrForDuplicatedSpaceBinding))
+
+			// then
+			require.NoError(t, err)
+			// finalizer should be removed
+			spacebindingrequesttest.AssertThatSpaceBindingRequest(t, sbr.GetNamespace(), sbr.GetName(), member1.Client).
+				DoesNotExist()
+			// the spacebinding should not be deleted, since it doesn't belong to the SBR
+			spacebindingtest.AssertThatSpaceBinding(t, test.HostOperatorNs, janeMur.Name, janeSpace.Name, hostClient).
+				Exists()
+		})
+
 	})
 }