From 9dce34706b5bac72def480cc7485e5a4bd79d3a9 Mon Sep 17 00:00:00 2001 From: Francisc Munteanu Date: Mon, 6 Nov 2023 12:26:57 +0100 Subject: [PATCH] fix: add check for spacebinding existence in SBR controller (#898) * check if spacebinding for the same mur and space already exists Co-authored-by: Matous Jobanek --- .../spacebindingrequest_controller.go | 49 ++++++++++------- .../spacebindingrequest_controller_test.go | 53 ++++++++++++++++++- 2 files changed, 83 insertions(+), 19 deletions(-) diff --git a/controllers/spacebindingrequest/spacebindingrequest_controller.go b/controllers/spacebindingrequest/spacebindingrequest_controller.go index 3ba3613ce..7d444b87e 100644 --- a/controllers/spacebindingrequest/spacebindingrequest_controller.go +++ b/controllers/spacebindingrequest/spacebindingrequest_controller.go @@ -94,12 +94,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl. return reconcile.Result{}, err } - // create spacebinding if not found for given spaceBindingRequest - spaceBinding, err := r.getSpaceBinding(ctx, spaceBindingRequest) - if err != nil { - return reconcile.Result{}, err - } - err = r.ensureSpaceBinding(ctx, memberClusterWithSpaceBindingRequest, spaceBindingRequest, spaceBinding) + err = r.ensureSpaceBinding(ctx, memberClusterWithSpaceBindingRequest, spaceBindingRequest) if err != nil { if errStatus := r.setStatusFailedToCreateSpaceBinding(ctx, memberClusterWithSpaceBindingRequest, spaceBindingRequest, err); errStatus != nil { logger.Error(errStatus, "error updating SpaceBindingRequest status") @@ -125,7 +120,7 @@ func (r *Reconciler) ensureSpaceBindingDeletion(ctx context.Context, memberClust // finalizer was already removed, nothing to delete anymore... return nil } - spaceBinding, err := r.getSpaceBinding(ctx, spaceBindingRequest) + spaceBinding, _, err := r.getSpaceBinding(ctx, spaceBindingRequest, memberClusterWithSpaceBindingRequest) if err != nil { return err } @@ -145,23 +140,42 @@ func (r *Reconciler) ensureSpaceBindingDeletion(ctx context.Context, memberClust } // getSpaceBinding retrieves the spacebinding created by the spacebindingrequest -func (r *Reconciler) getSpaceBinding(ctx context.Context, spaceBindingRequest *toolchainv1alpha1.SpaceBindingRequest) (*toolchainv1alpha1.SpaceBinding, error) { +func (r *Reconciler) getSpaceBinding(ctx context.Context, spaceBindingRequest *toolchainv1alpha1.SpaceBindingRequest, memberCluster cluster.Cluster) (*toolchainv1alpha1.SpaceBinding, *toolchainv1alpha1.Space, error) { + // find space from namespace labels + space, err := r.getSpace(ctx, memberCluster, spaceBindingRequest) + if err != nil { + return nil, nil, err + } spaceBindings := &toolchainv1alpha1.SpaceBindingList{} spaceBindingLabels := runtimeclient.MatchingLabels{ - toolchainv1alpha1.SpaceBindingRequestLabelKey: spaceBindingRequest.GetName(), - toolchainv1alpha1.SpaceBindingRequestNamespaceLabelKey: spaceBindingRequest.GetNamespace(), + toolchainv1alpha1.SpaceBindingSpaceLabelKey: space.GetName(), + toolchainv1alpha1.SpaceBindingMasterUserRecordLabelKey: spaceBindingRequest.Spec.MasterUserRecord, } - err := r.Client.List(ctx, spaceBindings, spaceBindingLabels, runtimeclient.InNamespace(r.Namespace)) + err = r.Client.List(ctx, spaceBindings, spaceBindingLabels, runtimeclient.InNamespace(r.Namespace)) if err != nil { - return nil, errs.Wrap(err, "unable to list spacebindings") + return nil, space, errs.Wrap(err, "unable to list spacebindings") } // spacebinding not found if len(spaceBindings.Items) == 0 { - return nil, nil + return nil, space, nil } - return &spaceBindings.Items[0], nil // all good + // more than one spacebinding + if len(spaceBindings.Items) > 1 { + 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 } // deleteSpaceBinding deletes a given spacebinding object in case deletion was not issued already. @@ -209,12 +223,11 @@ func (r *Reconciler) addFinalizer(ctx context.Context, memberCluster cluster.Clu return nil } -func (r *Reconciler) ensureSpaceBinding(ctx context.Context, memberCluster cluster.Cluster, spaceBindingRequest *toolchainv1alpha1.SpaceBindingRequest, spaceBinding *toolchainv1alpha1.SpaceBinding) error { +func (r *Reconciler) ensureSpaceBinding(ctx context.Context, memberCluster cluster.Cluster, spaceBindingRequest *toolchainv1alpha1.SpaceBindingRequest) error { logger := log.FromContext(ctx) logger.Info("ensuring spacebinding") - - // find space from namespace labels - space, err := r.getSpace(ctx, memberCluster, spaceBindingRequest) + // create spacebinding if not found for given spaceBindingRequest + spaceBinding, space, err := r.getSpaceBinding(ctx, spaceBindingRequest, memberCluster) if err != nil { return err } diff --git a/controllers/spacebindingrequest/spacebindingrequest_controller_test.go b/controllers/spacebindingrequest/spacebindingrequest_controller_test.go index 47a9414a9..fe2165979 100644 --- a/controllers/spacebindingrequest/spacebindingrequest_controller_test.go +++ b/controllers/spacebindingrequest/spacebindingrequest_controller_test.go @@ -16,6 +16,7 @@ import ( spacebindingrequesttest "github.com/codeready-toolchain/host-operator/test/spacebindingrequest" spacerequesttest "github.com/codeready-toolchain/host-operator/test/spacerequest" commoncluster "github.com/codeready-toolchain/toolchain-common/pkg/cluster" + spacebindingcommon "github.com/codeready-toolchain/toolchain-common/pkg/spacebinding" "github.com/codeready-toolchain/toolchain-common/pkg/test" "github.com/codeready-toolchain/toolchain-common/pkg/test/masteruserrecord" spacetest "github.com/codeready-toolchain/toolchain-common/pkg/test/space" @@ -450,6 +451,54 @@ func TestCreateSpaceBindingRequest(t *testing.T) { HasFinalizer() }) + t.Run("There are more than one SpaceBinding for the given MUR and Space", func(t *testing.T) { + // we have an SBR that will try to create the same SpaceBinding + sbrForDuplicatedSpaceBinding := spacebindingrequesttest.NewSpaceBindingRequest("jane", "jane-tenant", + spacebindingrequesttest.WithMUR(janeMur.Name), + spacebindingrequesttest.WithSpaceRole("admin")) + + // given + spaceBinding1 := spacebindingcommon.NewSpaceBinding(janeMur, janeSpace, "john") // there is already an admin generated SpaceBinding + spaceBinding2 := spaceBinding1.DeepCopy() // there is another spacebinding (this should not happen) + spaceBinding2.Name = "somerandom name" // the name doesn't matter since spacebindings are retrieved using lables + member1 := NewMemberClusterWithClient(test.NewFakeClient(t, sbrNamespace, sbrForDuplicatedSpaceBinding), "member-1", corev1.ConditionTrue) + hostClient := test.NewFakeClient(t, base1nsTier, janeSpace, janeMur, spaceBinding1, spaceBinding2) + ctrl := newReconciler(t, hostClient, member1) + + // when + _, err := ctrl.Reconcile(context.TODO(), requestFor(sbrForDuplicatedSpaceBinding)) + + // then + cause := "expected 1 spacebinding for Space jane and MUR jane. But found 2" + require.EqualError(t, err, cause) + spacebindingrequesttest.AssertThatSpaceBindingRequest(t, sbr.GetNamespace(), sbr.GetName(), member1.Client). + HasConditions(spacebindingrequesttestcommon.UnableToCreateSpaceBinding(cause)). + HasFinalizer() + }) + + t.Run("SpaceBinding not managed by this SpaceBindingRequest CR", 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 + sbrForDuplicatedSpaceBinding := spacebindingrequesttest.NewSpaceBindingRequest("jane", "jane-tenant", + spacebindingrequesttest.WithMUR(janeMur.Name), + spacebindingrequesttest.WithSpaceRole("admin")) + + 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 + cause := fmt.Sprintf("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", janeSpace.GetName(), janeMur.GetName()) + require.EqualError(t, err, cause) + spacebindingrequesttest.AssertThatSpaceBindingRequest(t, sbr.GetNamespace(), sbr.GetName(), member1.Client). + HasConditions(spacebindingrequesttestcommon.UnableToCreateSpaceBinding(cause)). + HasFinalizer() + }) + }) } @@ -595,6 +644,7 @@ func TestDeleteSpaceBindingRequest(t *testing.T) { sbr := spacebindingrequesttest.NewSpaceBindingRequest("jane", sbrNamespace.GetName(), spacebindingrequesttest.WithDeletionTimestamp(), spacebindingrequesttest.WithFinalizer(), + spacebindingrequesttest.WithMUR(janeMur.Name), ) // sbr is being deleted spaceBinding := spacebindingtest.NewSpaceBinding(janeMur.Name, janeSpace.Name, "admin", sbr.Name, spacebindingtest.WithSpaceBindingRequest(sbr)) spaceBinding.DeletionTimestamp = &metav1.Time{Time: time.Now().Add(-121 * time.Second)} // is being deleted since more than 2 minutes @@ -618,6 +668,7 @@ func TestDeleteSpaceBindingRequest(t *testing.T) { sbr := spacebindingrequesttest.NewSpaceBindingRequest("jane", sbrNamespace.GetName(), spacebindingrequesttest.WithDeletionTimestamp(), spacebindingrequesttest.WithFinalizer(), + spacebindingrequesttest.WithMUR(janeMur.Name), ) // sbr is being deleted spaceBinding := spacebindingtest.NewSpaceBinding(janeMur.Name, janeSpace.Name, "admin", sbr.Name, spacebindingtest.WithSpaceBindingRequest(sbr)) member1 := NewMemberClusterWithClient(test.NewFakeClient(t, sbr, sbrNamespace), "member-1", corev1.ConditionTrue) @@ -651,7 +702,7 @@ func TestDeleteSpaceBindingRequest(t *testing.T) { return member1Client.Client.Update(ctx, obj, opts...) } member1 := NewMemberClusterWithClient(member1Client, "member-1", corev1.ConditionTrue) - hostClient := test.NewFakeClient(t) + hostClient := test.NewFakeClient(t, janeSpace) ctrl := newReconciler(t, hostClient, member1) // when