Skip to content

Commit

Permalink
fix: add check for spacebinding existence in SBR controller (#898)
Browse files Browse the repository at this point in the history 
* check if spacebinding for the same mur and space already exists
Co-authored-by: Matous Jobanek <mjobanek@redhat.com>
  • Loading branch information
@mfrancisc
mfrancisc authored Nov 6, 2023
1 parent 523e88f commit 9dce347
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 19 deletions.
49 changes: 31 additions & 18 deletions controllers/spacebindingrequest/spacebindingrequest_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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
}
Expand All @@ -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.
Expand Down Expand Up @@ -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
}
Expand Down
53 changes: 52 additions & 1 deletion controllers/spacebindingrequest/spacebindingrequest_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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()
})

})
}

Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 9dce347

Please sign in to comment.