feat: Implement SpaceBindingRequest controller main logic #848

mfrancisc · 2023-08-14T09:01:33Z

This PR adds main logic for handling SpaceBindingRequests, the logic supports:

create/update/delete of SpaceBindingRequests and associated SpaceBindings
validation of MUR, SpaceRole

Jira: https://issues.redhat.com/browse/ASC-397

Related PRs:

api feat: Implement SpaceBindingRequest controller main logic api#372
toolchain-common feat: Implement SpaceBindingRequest controller main logic toolchain-common#322

Paired with: codeready-toolchain/toolchain-e2e#779

mfrancisc · 2023-08-14T13:18:58Z

/retest

update e2e PR

drpaneas · 2023-08-14T13:59:56Z

test/spacerequest/spacerequest.go

+		toolchainv1alpha1.TypeLabelKey:     "tenant",
+		toolchainv1alpha1.ProviderLabelKey: toolchainv1alpha1.ProviderLabelValue,
+	}
+	if spacename != "nospace" {


What happens if == nospace ? Is it still a valid namespace? Do think this ever returns an invalid namespace? I mean should we return (*corev1.Namespace, error) or not necessary?

The function is used only for mocking namespace while unit testing. When is being called with "nospace" is done with the purpose of configuring (misconfiguring) the namespace, and verify the corner case in which a required label is missing, for example in this unit test: https://github.com/codeready-toolchain/host-operator/pull/848/files#diff-f15c73194e22112fd24e7e852200c395d2cddb8cb016fe76710f4c38bab782e1R232-R252

In short, I want the controller to throw this error here: https://github.com/codeready-toolchain/host-operator/pull/848/files#diff-e48d49741db80226348ae5ffd08100dd10afa2e83e5d18b2a94776dfdfb616f3R333-R335

Hope this makes sense.

yes it makes sense. Sorry I missed the test part ... sigh...

No worries! My pleasure getting more feedback/reviews and discussing alternative solutions! Thanks for checking!

drpaneas · 2023-08-14T14:05:06Z

test/spacebindingrequest/spacebindingrequest.go

+
+func WithFinalizer() Option {
+	return func(spaceBindingRequest *toolchainv1alpha1.SpaceBindingRequest) {
+		spaceBindingRequest.Finalizers = append(spaceBindingRequest.Finalizers, toolchainv1alpha1.FinalizerName)


what if the spaceBindingRequest object is not nil before applying the finalizer? Would that be a problem? If so:

if spaceBindingRequest != nil { spaceBindingRequest.Finalizers = append(spaceBindingRequest.Finalizers, toolchainv1alpha1.FinalizerName) } else { // Handle the case where the input spaceBindingRequest is nil, possibly with logging or an error return. }

Unless I'm missing something, it cannot be nil, since those Option functions are being passed to the NewSpaceBindingRequest function, which takes care of creating the SpaceBindingRequest object: https://github.com/codeready-toolchain/host-operator/blob/master/test/spacebindingrequest/spacebindingrequest.go#L13-L19

Of course if somehow misused you could break them, but those are "only" used for unit testing, and we might want to modify all our With... helper in this case (also the ones in toolchain-common), since they all follow the same style, eg: https://github.com/codeready-toolchain/toolchain-common/blob/master/pkg/test/usersignup/usersignup.go

same mistake from my part. I didn't see this is for testing purposes. Please ignore.

drpaneas · 2023-08-14T14:10:34Z

go.mod

@@ -114,3 +114,5 @@ require (
 )

 go 1.19
+
+replace github.com/codeready-toolchain/api => github.com/mfrancisc/api v0.0.0-20230814083608-6d8633a3bcf3


Please update the relevant api repo first and then update thus replicate statement, instead.

This is done only for testing (unit and e2e) the PR with the updated version of the API repo, once all the PRs are approved and passing all the tests/checks, we:

merge the API pr

update other PRs with the new version

merge other repos with new master API version

we also have a CI check in place which prevents from merging if there is any replace statement, see: https://github.com/codeready-toolchain/host-operator/actions/runs/5857146355/job/15878356586?pr=848

So this will be replaced once all the tests are passing and all the PRs are approved.

Hopefully this answers you question/concern. But plz let me know if I misunderstood.

drpaneas · 2023-08-14T14:12:05Z

controllers/spacerequest/spacerequest_controller_test.go

@@ -853,7 +852,7 @@ func TestDeleteSpaceRequest(t *testing.T) {
 	err := apis.AddToScheme(scheme.Scheme)
 	require.NoError(t, err)
 	appstudioTier := tiertest.AppStudioTier(t, tiertest.AppStudioTemplates)
-	srNamespace := newNamespace("jane")
+	srNamespace := spacerequesttest.NewNamespace("jane")


Not your code per se -- can we put "jane" in a const? Seems it's been used in many place, to avoid any typos in the future.

Yeah, thanks for pointing this out, it doesn't look very clean, there are usually some strings that can be extracted to const or variable in our testing code, just didn't saw this aligned with the other unit tests for other controllers.

Or maybe we could put the string into a variable instead of const ? 🤔

drpaneas · 2023-08-14T14:18:12Z

controllers/spacebindingrequest/spacebindingrequest_controller_test.go

+
+			// then
+			require.EqualError(t, err, "failed to remove finalizer: mock error")
+		})


great work on testing and mocking this :D +1

I can't take credit for this , I was just good at copy pasting and following same approach I saw in other unit tests :D .
The repository is mature and full of great examples on how to solve many of these "tricky" problems with testing and not only.

Kudos to the team 🏅

alexeykazakov · 2023-08-14T23:09:05Z

controllers/spacebindingrequest/spacebindingrequest_controller.go

@@ -92,7 +99,360 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.
 	}
 	logger.Info("spacebindingrequest found", "member cluster", memberClusterWithSpaceBindingRequest.Name)

-	// todo implement logic
+	if util.IsBeingDeleted(spaceBindingRequest) {


It's not part of this PR but I missed the previous PR which introduced member cluster lookup. So I'm using this opportunity to point out that we can improve the lookup mechanism :)
Let's say there are two member clusters m-1 and m-2. And m-1 cluster is having an outage and temporally down. The reconcile won't work for all space requests even from m-2 if m-1 happens to be first in r.MemberClusters.

What we could do to mitigate it is too save the error in line 82 above but proceed with the lookup. And only if no SBR found in other members then return the saved error. Otherwise just log the error but do not return it. In that case m-2 won't be affected by the m-1 outage.

Feel free to address it in a separate PR though.

Yeah, good point! The logic can be improved as you suggested, also I can check if it can be extracted to a reusable function since I see it's similar to what I'm using in spacerequest_controller and soon also in spacebindingcleanup_controller (for the SBR cleanup task) 🤔

Thanks for your suggestion, I'll address it in a separate PR 👍

mfrancisc · 2023-08-16T14:15:48Z

/retest

infra

MatousJobanek

Nice work 👍

MatousJobanek · 2023-08-16T12:46:40Z

controllers/spacebindingrequest/spacebindingrequest_controller.go

+	spaceBindingLabels := runtimeclient.MatchingLabels{
+		toolchainv1alpha1.SpaceBindingRequestLabelKey:          spaceBindingRequest.GetName(),
+		toolchainv1alpha1.SpaceBindingRequestNamespaceLabelKey: spaceBindingRequest.GetNamespace(),
+	}


it's just a detail, but you could have a function:

spaceBindingLabels(spaceBindingRequest)

that would return the labels so you could use it when creating and when getting the SpaceBinding - in that way, you could be sure that you use always the same labels

🤔 maybe it doesn't make sense

I'm not 100% sure I understand how I would use it, since the spacebinding now is being created by this utility function https://github.com/codeready-toolchain/toolchain-common/blob/c909c72d8f627a9f72917531eded2a5e765faea1/pkg/spacebinding/spacebinding.go#L14

Maybe I can add some utility function like WithSpaceBindingRequestLabels in that package and use that instead of setting the labels directly as I'm doing now when creating the object https://github.com/codeready-toolchain/host-operator/pull/848/files#diff-e48d49741db80226348ae5ffd08100dd10afa2e83e5d18b2a94776dfdfb616f3R304-R305

never mind - I made the comment when I started reviewing the PR. When I saw the rest of the code, I agree that it doesn't make any sense.

MatousJobanek · 2023-08-16T12:53:00Z

controllers/spacebindingrequest/spacebindingrequest_controller.go

+		if err := r.setStatusTerminating(memberClusterWithSpaceBindingRequest, spaceBindingRequest); err != nil {
+			return errs.Wrap(err, "error updating status")
+		}


btw, the error handling of the failed status updates could be moved into the updateStatus method, so you would just return whatever you get and you don't have to differentiate between the failed and no-failed ones

return r.setStatusTerminating(memberClusterWithSpaceBindingRequest, spaceBindingRequest)

I've simplified that plz see: 0d24598

I've noticed that in the other cases I was just returning , and that should be probably enough, since the error will already contain the message saying that it was a status update failure, no need to wrap that with the custom message: "error updating status" IMO.

MatousJobanek · 2023-08-16T13:06:59Z

controllers/spacebindingrequest/spacebindingrequest_controller.go

+	}
+	// space is being deleted
+	if util.IsBeingDeleted(space) {
+		return false, errs.New("space is being deleted")


I would rather also update the SBR CR to make it visible to end users:

Suggested change

return false, errs.New("space is being deleted")

return false, r.setStatusFailedToCreateSpaceBinding(logger, memberCluster, spaceBindingRequest, errs.New("space is being deleted"))

Makes sense! Thanks! done in 0d24598

MatousJobanek · 2023-08-16T13:07:24Z

controllers/spacebindingrequest/spacebindingrequest_controller.go

+	}
+	// mur is being deleted
+	if util.IsBeingDeleted(mur) {
+		return false, errs.New("mur is being deleted")


the same here

Suggested change

return false, errs.New("mur is being deleted")

return false, r.setStatusFailedToCreateSpaceBinding(logger, memberCluster, spaceBindingRequest, errs.New("mur is being deleted"))

Makes sense! Thanks! done in 0d24598

MatousJobanek · 2023-08-16T13:12:09Z

controllers/spacebindingrequest/spacebindingrequest_controller.go

+	isRoleValid := false
+	for actual := range nsTemplTier.Spec.SpaceRoles {
+		if spaceBindingRequest.Spec.SpaceRole == actual {
+			isRoleValid = true
+		}
+	}
+	if !isRoleValid {
+		return fmt.Errorf("invalid role '%s' for space '%s'", spaceBindingRequest.Spec.SpaceRole, space.Name)
+	}
+
+	return nil


we could simplify this

Suggested change

isRoleValid := false

for actual := range nsTemplTier.Spec.SpaceRoles {

if spaceBindingRequest.Spec.SpaceRole == actual {

isRoleValid = true

}

}

if !isRoleValid {

return fmt.Errorf("invalid role '%s' for space '%s'", spaceBindingRequest.Spec.SpaceRole, space.Name)

}

return nil

for actual := range nsTemplTier.Spec.SpaceRoles {

if spaceBindingRequest.Spec.SpaceRole == actual {

return nil

}

}

return fmt.Errorf("invalid role '%s' for space '%s'", spaceBindingRequest.Spec.SpaceRole, space.Name)

Agree this makes it shorter, thanks! Done in 0d24598

MatousJobanek · 2023-08-16T14:32:31Z

controllers/spacebindingrequest/spacebindingrequest_controller.go

+	if err := r.Client.Create(context.TODO(), spaceBinding); err != nil {
+		return errs.Wrap(err, "unable to create SpaceBinding")
+	}


btw, we should take care of conflicts with the already existing (default) SpaceBindings. Or with those that are created by admins. Such SpaceBindings are immutable for end users, so it would be good to set corresponding status in SBR CR.
It's fine if you do it in a new PR.

If I'm not mistaken this is already covered by the fact that the name of the SpaceBinding is computed here https://github.com/codeready-toolchain/toolchain-common/blob/c909c72d8f627a9f72917531eded2a5e765faea1/pkg/spacebinding/spacebinding.go#L24

So in case an SBR tries to create a SB resource for mur and space that already has a SpaceBinding, the Create will fail saying that the name already exists and it will be set in the SBR status : https://github.com/codeready-toolchain/host-operator/pull/848/files#diff-e48d49741db80226348ae5ffd08100dd10afa2e83e5d18b2a94776dfdfb616f3R250-R252

sure, my point was to show it properly in the status by saying:
"SpaceBinding for the given name and space already exist but is not managed by SBR controller" or something like that - more explanatory than just returning the conflict.

Ok, then let's do it in separate PR, because if we want to specify if the SB that already exists is or not managed by SBR controller , we need to get it and check the labels. Not sure if we want to provide such information to the user/client?

MatousJobanek · 2023-08-16T14:36:18Z

controllers/spacebindingrequest/spacebindingrequest_controller_test.go

+			spacebindingrequesttest.AssertThatSpaceBindingRequest(t, badSBR.GetNamespace(), badSBR.GetName(), member1.Client).
+				HasSpecSpaceRole("admin").
+				HasSpecMasterUserRecord(""). // empty
+				HasFinalizer()


verify that it has corresponding reason, status, and message set

Done in 0d24598

Thanks!

MatousJobanek · 2023-08-16T14:36:29Z

controllers/spacebindingrequest/spacebindingrequest_controller_test.go

+			spacebindingrequesttest.AssertThatSpaceBindingRequest(t, badSBR.GetNamespace(), badSBR.GetName(), member1.Client).
+				HasSpecSpaceRole(""). // empty
+				HasSpecMasterUserRecord(janeMur.Name).
+				HasFinalizer()


the same here

Done in 0d24598

Thanks!

MatousJobanek · 2023-08-16T14:41:22Z

controllers/spacebindingrequest/spacebindingrequest_controller_test.go

+				HasLabelWithValue(toolchainv1alpha1.SpaceBindingRequestLabelKey, sbr.GetName()). // check expected labels are there
+				HasLabelWithValue(toolchainv1alpha1.SpaceBindingRequestNamespaceLabelKey, sbr.GetNamespace())


we expect that the labels should match the values in spec - this could be part of the AssertThatSpaceBinding function.

I'm not sure I'm understanding this . The AssertThatSpaceBinding function is being used by other tests like usersignup, spacebindingcleanup. If I move this check inside the function it will not work there, but maybe I'm not understanding this correctly.

my bad.
You are right - I misread the names of the labels 🤦‍♂️
so never mind ;-)

MatousJobanek · 2023-08-16T14:43:51Z

controllers/spacebindingrequest/spacebindingrequest_controller_test.go

+	})
+
+	t.Run("failure", func(t *testing.T) {
+		t.Run("SpaceBinding resource is already being deleted", func(t *testing.T) {


let's make clear why it's a failure case:

Suggested change

t.Run("SpaceBinding resource is already being deleted", func(t *testing.T) {

t.Run("SpaceBinding resource is already being deleted for more than 2 minutes", func(t *testing.T) {

Done in 0d24598

Thanks!

mfrancisc · 2023-08-16T15:47:43Z

/retest

infra again ..

…eBinding only once, unit tests assertions

mfrancisc · 2023-08-17T07:54:48Z

/retest

infra

mfrancisc · 2023-08-17T10:28:31Z

/retest

updated e2e PR

MatousJobanek

Looks great 👍 🚀
Thanks a lot for addressing my comments 🥇

controllers/spacebindingrequest/spacebindingrequest_controller.go

openshift-ci · 2023-08-17T12:27:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MatousJobanek, mfrancisc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [MatousJobanek]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Co-authored-by: Matous Jobanek <[email protected]>

mfrancisc · 2023-08-21T10:58:51Z

/retest

flaky? https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/codeready-toolchain_host-operator/848/pull-ci-codeready-toolchain-host-operator-master-e2e/1693521960136347648/build-log.txt

mfrancisc · 2023-08-21T17:49:07Z

/retest

updated e2e PR

mfrancisc · 2023-08-22T08:25:08Z

/retest

updated e2e PR

sonarcloud · 2023-08-23T08:44:33Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

No Coverage information
13.0% Duplication

codecov · 2023-08-23T10:57:32Z

Codecov Report

Merging #848 (0c9e22e) into master (73b335d) will increase coverage by 0.36%.
The diff coverage is 91.43%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #848      +/-   ##
==========================================
+ Coverage   83.01%   83.38%   +0.36%     
==========================================
  Files          52       52              
  Lines        5570     5826     +256     
==========================================
+ Hits         4624     4858     +234     
- Misses        774      790      +16     
- Partials      172      178       +6

Files Changed	Coverage Δ
...cebindingrequest/spacebindingrequest_controller.go	`86.64% <91.43%> (+23.89%)`	⬆️

mfrancisc added 2 commits August 14, 2023 10:41

implement SBR main logic

75564eb

update api and fix test

f727536

mfrancisc requested review from sbryzak, xcoulon, alexeykazakov, MatousJobanek, rajivnathan and ranakan19 August 14, 2023 09:01

Merge branch 'master' into sbrmain

6b81c44

This was referenced Aug 14, 2023

feat: Implement SpaceBindingRequest controller main logic codeready-toolchain/toolchain-e2e#779

Merged

feat: Implement SpaceBindingRequest controller main logic codeready-toolchain/api#372

Merged

drpaneas reviewed Aug 14, 2023

View reviewed changes

update api version

4971daa

alexeykazakov reviewed Aug 14, 2023

View reviewed changes

mfrancisc added 2 commits August 16, 2023 10:06

Merge branch 'master' into sbrmain

9b0fe34

update api version

a40298c

mfrancisc mentioned this pull request Aug 16, 2023

feat: Implement SpaceBindingRequest controller main logic codeready-toolchain/toolchain-common#322

Merged

move SBR conditions helpers to common

add5cd7

MatousJobanek reviewed Aug 16, 2023

View reviewed changes

mfrancisc added 3 commits August 16, 2023 18:14

address PR review comments

0d24598

remove bool from ensureSpaceBinding, call setStatusFailedToCreateSpac…

0d077fd

…eBinding only once, unit tests assertions

Merge branch 'master' into sbrmain

a6b92ea

MatousJobanek approved these changes Aug 17, 2023

View reviewed changes

controllers/spacebindingrequest/spacebindingrequest_controller.go Outdated Show resolved Hide resolved

openshift-ci bot added the approved label Aug 17, 2023

mfrancisc and others added 2 commits August 17, 2023 14:37

Update controllers/spacebindingrequest/spacebindingrequest_controller.go

cc264d1

Co-authored-by: Matous Jobanek <[email protected]>

Merge branch 'master' into sbrmain

7f499b2

mfrancisc added 2 commits August 23, 2023 10:43

update api and common version

4df75e3

Merge remote-tracking branch 'origin/sbrmain' into sbrmain

0c9e22e

mfrancisc merged commit 6c9e07d into codeready-toolchain:master Aug 23, 2023
11 checks passed

mfrancisc deleted the sbrmain branch August 23, 2023 11:43

mfrancisc mentioned this pull request Aug 24, 2023

refactor: introduce LookupMember cluster function #855

Merged

	return false, errs.New("space is being deleted")
	return false, r.setStatusFailedToCreateSpaceBinding(logger, memberCluster, spaceBindingRequest, errs.New("space is being deleted"))

	return false, errs.New("mur is being deleted")
	return false, r.setStatusFailedToCreateSpaceBinding(logger, memberCluster, spaceBindingRequest, errs.New("mur is being deleted"))

		HasLabelWithValue(toolchainv1alpha1.SpaceBindingRequestLabelKey, sbr.GetName()). // check expected labels are there
		HasLabelWithValue(toolchainv1alpha1.SpaceBindingRequestNamespaceLabelKey, sbr.GetNamespace())

	t.Run("SpaceBinding resource is already being deleted", func(t *testing.T) {
	t.Run("SpaceBinding resource is already being deleted for more than 2 minutes", func(t *testing.T) {

feat: Implement SpaceBindingRequest controller main logic #848

feat: Implement SpaceBindingRequest controller main logic #848

Conversation

mfrancisc commented Aug 14, 2023 • edited Loading

mfrancisc commented Aug 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drpaneas Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfrancisc Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfrancisc commented Aug 16, 2023

MatousJobanek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfrancisc Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfrancisc Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfrancisc commented Aug 16, 2023

mfrancisc commented Aug 17, 2023

mfrancisc commented Aug 17, 2023

MatousJobanek left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 17, 2023

mfrancisc commented Aug 21, 2023

mfrancisc commented Aug 21, 2023

mfrancisc commented Aug 22, 2023

sonarcloud bot commented Aug 23, 2023

codecov bot commented Aug 23, 2023

Codecov Report

mfrancisc commented Aug 14, 2023 •

edited

Loading

drpaneas Aug 14, 2023 •

edited

Loading

mfrancisc Aug 14, 2023 •

edited

Loading

mfrancisc Aug 16, 2023 •

edited

Loading

mfrancisc Aug 16, 2023 •

edited

Loading