diff --git a/controllers/spacebindingrequest/spacebindingrequest_controller.go b/controllers/spacebindingrequest/spacebindingrequest_controller.go index 054b179bd..ac7791f53 100644 --- a/controllers/spacebindingrequest/spacebindingrequest_controller.go +++ b/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 diff --git a/controllers/spacebindingrequest/spacebindingrequest_controller_test.go b/controllers/spacebindingrequest/spacebindingrequest_controller_test.go index fe2165979..9b12a18c1 100644 --- a/controllers/spacebindingrequest/spacebindingrequest_controller_test.go +++ b/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() + }) + }) }