feat: Implement SpaceBindingRequest controller main logic #779

mfrancisc · 2023-08-14T09:03:56Z

Add e2e tests for SpaceBindingRequests controller.

Scenarios covered:

creation of a spacebindingrequest
update of a spacebindingrequest
deletion of a spacebindingrequest
validate spacebindingrequest fields

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

Paired with: codeready-toolchain/host-operator#848

Related PRs:

api codeready-toolchain/api#372
toolchain-common codeready-toolchain/toolchain-common#322

MatousJobanek · 2023-08-17T11:13:57Z

make/clean.mk

@@ -13,6 +13,7 @@ clean-users:
 	$(Q)-oc delete usersignups --all --all-namespaces
 	$(Q)-oc delete spacerequests --all --all-namespaces
 	$(Q)-oc delete spaces --all --all-namespaces
+	$(Q)-oc delete spacebindingrequests --all --all-namespaces


this should be executed before deleting Spaces. Deletion of Spaces should trigger deletion of everything in the namespaces, including SBR. That being said - do we need to execute the deletion of SRs and SBRs explicitly at all?

You are right , we don't need to delete spacebindingrequests , but we do need to delete spacerequests because the spaces created by those cannot be deleted without deleting their SR resource.

I'll remove this line.

Removed the deletion of spacebindingrequests in 9fda3e4

MatousJobanek · 2023-08-17T11:21:38Z

test/e2e/parallel/spacebindingrequest_test.go

+		t.Run("create space binding request", func(t *testing.T) {
+			// when
+			// we create a space to share , a new MUR and a SpaceBindingRequest
+			space, spaceBindingRequest, spaceBinding := NewSpaceBindingRequest(t, awaitilities, memberAwait, hostAwait, "admin")


NewSpaceBindingRequest name is very misleading when I look at what it all does. Not sure how to name it better 🤷‍♂️

Suggested change

space, spaceBindingRequest, spaceBinding := NewSpaceBindingRequest(t, awaitilities, memberAwait, hostAwait, "admin")

space, spaceBindingRequest, spaceBinding := CreateSbrForMur(t, awaitilities, memberAwait, hostAwait, "admin")

maybe

Suggested change

space, spaceBindingRequest, spaceBinding := NewSpaceBindingRequest(t, awaitilities, memberAwait, hostAwait, "admin")

space, spaceBindingRequest, spaceBinding := EnsureSpaceBindingRequest(t, awaitilities, memberAwait, hostAwait, "admin")

I named it this way because we also have NewSpaceRequest , which does similar thing but for SpaceRequest , if we want to rename it I would opt for EnsureSpaceBindingRequest.

Another alternative could be: setupSpaceBindingRequest .

what about CreateSpaceBindingRequest, since we also have CreateSpace? (which is also called in this func, btw)

CreateSpaceBindingRequest makes sense , but I've already used it here https://github.com/codeready-toolchain/toolchain-e2e/pull/779/files#diff-d21939085e6a53e4b5e5b3b1158e837fe749eee6e124ba8a29645c714f0a859dR33 😄 , so not sure ...

naming is hard 😄

MatousJobanek · 2023-08-17T11:26:01Z

test/e2e/parallel/spacebindingrequest_test.go

+	// wait for the namespace to be provisioned since we will be creating the SpaceBindingRequest into it.
+	space, err := hostAwait.WaitForSpace(t, space.Name, UntilSpaceHasAnyProvisionedNamespaces())
+	require.NoError(t, err)


the CreateSpace function already executes:

_, err = memberAwait.WaitForNSTmplSet(t, space.Name, wait.UntilNSTemplateSetHasSpaceRoles( wait.SpaceRole(tier.Spec.SpaceRoles["admin"].TemplateRef, mur.Name)))

so we can assume that it's ready for new SpaceBindings to be created

Suggested change

// wait for the namespace to be provisioned since we will be creating the SpaceBindingRequest into it.

space, err := hostAwait.WaitForSpace(t, space.Name, UntilSpaceHasAnyProvisionedNamespaces())

require.NoError(t, err)

I see your point, but the reason I added this wait , is because even when the NSTemplateSet was ready the Space.Status.ProvisionedNamespaces field was not populated yet, and I'm taking the name of the namespace from there and creating the SBR in that namespace, see few line below at https://github.com/codeready-toolchain/toolchain-e2e/pull/779/files#diff-c5776fe9a155ff05f73263e14f488ec3b27200aa736d4d367cc1c86b4bedb72eR248

but maybe I'm doing it in the wrong way?

ah, makes sense, I missed that part. Thanks for explanation 👍

MatousJobanek · 2023-08-17T11:39:29Z

test/e2e/parallel/spacebindingrequest_test.go

+				// a new SpaceBinding is created
+				// with the same name but creation timestamp should be greater (more recent).
+				require.NoError(t, err)
+				spaceBinding, err = hostAwait.WithRetryOptions(TimeoutOption(time.Second*10), RetryInterval(time.Second*2)).WaitForSpaceBinding(t, spaceBindingRequest.Spec.MasterUserRecord, space.Name,


is there any reason for using the different retry options?

shorter timeout maybe could cause flakiness if the go routine executing this test gets paused for a few seconds

I don't see any reasoning for the longer retry interval - we could keep it short to have fast feedback loop.

I increased the retry interval and timeout because I'm noticing the test fails from time to time with this error :

---- diffs: expected SpaceBinding to be created after 2023-08-17 08:43:00 +0000 UTC; Actual creation timestamp 2023-08-17 08:43:00 +0000 UTC

full logs here
I'm pretty sure the SpaceBinding get's deleted, because I've observed that and manually tested it on my temp cluster, but I think this WaitForSpaceBinding function is picking up the SpaceBinding before it get's deleted, for this reason I've tried waiting more and I've also added this check on the UID : UntilSpaceBindingHasDifferentUID(oldUID)

maybe I can remove now the retry interval and see if it's more stable, or at least add a comment to explain why is there. WDYT?

Ok, I've noticed after that the default timeout was actually bigger than what I was setting, so I've removed that and also removed the creationTimeStamp check ,I've left only the UID one, please see: 88eaeb5

cool, thanks 👍

MatousJobanek · 2023-08-17T11:41:14Z

test/e2e/parallel/spacebindingrequest_test.go

+			// wait for the namespace to be provisioned since we will be creating the SpaceBindingRequest into it.
+			space, err := hostAwait.WaitForSpace(t, space.Name, UntilSpaceHasAnyProvisionedNamespaces())
+			require.NoError(t, err)


the same - no need for another wait

Suggested change

// wait for the namespace to be provisioned since we will be creating the SpaceBindingRequest into it.

space, err := hostAwait.WaitForSpace(t, space.Name, UntilSpaceHasAnyProvisionedNamespaces())

require.NoError(t, err)

MatousJobanek · 2023-08-17T11:43:07Z

testsupport/spacebindingrequest.go

+	}
+}
+
+func WithNamespace(ns string) SpaceBindingRequestOption {


wouldn't be InNamespace more appropriate?

right , that is how I wanted to name it too but that function already exists for SpaceRequests in the same package:

toolchain-e2e/testsupport/spacerequest.go

Lines 37 to 41 in 82d14a8

func InNamespace(namespace string) SpaceRequestOption {

return func(s *toolchainv1alpha1.SpaceRequest) {

s.ObjectMeta.Namespace = namespace

}

}

Maybe I can make this more generic and use it for both ?

Alternatively we could group the files under testsupport folder into smaller packages , for example we can move:

space.go , spacerequest.go into a new folder testsupport/space

spacebinding.go , spacebindingrequest.go into testsupport/spacebinding

but I suggest we do this in a separate PR.

I like the With prefix because it's the same for all Option.
There's a pattern that works in case we want to keep everything in the same package, but it requires a bit of refactoring:

Something like:

type SpaceOption interface { ApplyToSpace(s *Space) } type SpaceRequestOption interface { ApplyToSpaceRequest(sr *SpaceRequest) } type WithNamespace struct { ns string } var _ SpaceOption = WithNamespace{} // requires a `ApplyToSpace(s *Space)` method func (w WithNamespace) ApplyToSpace(s *Space) {...} var _ SpaceRequestOption = WithNamespace{} // requires a `ApplyToSpaceRequest(s *SpaceRequest)` method func (w WithNamespace) ApplyToSpaceRequest(sr *SpaceRequest) {...}

hence, we can have the same WithNamespace(...) option to initialize Spaces and SpaceRequests resources.

that being said, having a sub package for each type is fine and probably easier to do, given the current codebase!

@xcoulon thanks a lot! indeed your pattern proposal is another option. I don't have a strong opinion, even if looking at it probably the package segregation would be "easier" ( just changing the import statements ), while this new pattern I'm not sure I saw it used anywhere else in our code ?

My proposal is to decide which path we want to go and I'll address it in a follow up PR. WDYT ?

I believe that splitting (or segregating) the code in testsupport/space + testsupport/spacebinding is probably the easiest thing to do for now, so let's go with that. Also, that's what we also did in toolchain-common/pkg/test anyways (see all the subfolders for masteruserrecords, etc.)

Ok, thanks ! I'm adding that to my backlog and will come up with a PR once I've merged the existing ones.

test/e2e/parallel/spacebindingrequest_test.go

MatousJobanek · 2023-08-17T12:01:56Z

test/e2e/parallel/spacebindingrequest_test.go

+	spaceBindingRequest, err = memberAwait.WaitForSpaceBindingRequest(t, types.NamespacedName{Namespace: spaceBindingRequest.GetNamespace(), Name: spaceBindingRequest.GetName()},
+		UntilSpaceBindingRequestHasConditions(spacebindingrequesttestcommon.Ready()),
+	)
+	require.NoError(t, err)


but we could maybe check that everything was propagated to the NSTemplateSet correctly here, WDYT?
something like this:

if spaceRole == "admin" { _, err = memberAwait.WaitForNSTmplSet(t, space.Name, UntilNSTemplateSetHasSpaceRoles( SpaceRole(tier.Spec.SpaceRoles[spaceRole].TemplateRef, userSignup.Status.CompliantUsername, userSignup.mur.Name))) } else { _, err = memberAwait.WaitForNSTmplSet(t, space.Name, UntilNSTemplateSetHasSpaceRoles( SpaceRole(tier.Spec.SpaceRoles["admin"].TemplateRef, userSignup.Status.CompliantUsername), SpaceRole(tier.Spec.SpaceRoles[spaceRole].TemplateRef, userSignup.mur.Name))) } VerifyResourcesProvisionedForSpace(t, awaitilities, space.Name)

you know - to be sure that the SpaceBinding that we created will be properly processed by the Space controller

Ok I see what you meant , but I'm not 100% sure I understood your example code - why do we need to do it this way?

If I'm not mistaken the usernames added to the NSTemplateSet are the just the SpaceBinding.Spec.MasterUserRecord : https://github.com/codeready-toolchain/host-operator/blob/723b3c9794f442315117b4fd55dc4eb028930246/controllers/space/space_controller.go#L458-L471

So in theory this could be just:

tier, err := awaitilities.Host().WaitForNSTemplateTier(t, space.Spec.TierName) _, err = memberAwait.WaitForNSTmplSet(t, space.Name, wait.UntilNSTemplateSetHasSpaceRoles( wait.SpaceRole(tier.Spec.SpaceRoles[spaceRole].TemplateRef, mur.Name))) require.NoError(t, err) VerifyResourcesProvisionedForSpace(t, awaitilities, space.Name)

but I'm sure I'm missing something.

I now understand why we need todo it this way, it's because we have 2 murs 🤦 .
Done in 88eaeb5

MatousJobanek · 2023-08-17T12:06:01Z

test/e2e/parallel/spacebindingrequest_test.go

+		require.NoError(t, err)
+	})
+
+	t.Run("update space binding request MasterUserRecord", func(t *testing.T) {


I'm wondering how much we want to support this scenario. I would love to make the MUR immutable, which would require an admission webhook of course...
After all, the SpaceBinding name contains the MUR name, so as soon as we would change it, then it wouldn't match anymore.

Makes sense, I haven't considered that the name doesn't change!
I'm now updating the mur name in the spacebinding spec and label but not changing the name, so maybe we should not allow this sort of update. But right now the user/client has the ability to update this field, so wanted to make sure the code works as "expected"

sure, that was more as a follow up task

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

mfrancisc · 2023-08-21T07:16:17Z

/retest

…mp check

xcoulon · 2023-08-22T12:20:35Z

test/e2e/parallel/spacebindingrequest_test.go

+				UntilSpaceBindingRequestHasConditions(spacebindingrequesttestcommon.UnableToCreateSpaceBinding("unable to get MUR: MasterUserRecord.toolchain.dev.openshift.com \"invalidMUR\" not found")),
+			)
+			require.NoError(t, err)
+			bindings, err := hostAwait.ListSpaceBindings(space.Name)


I believe it would make more sense to check the number of space bindings first, but it's just a minor suggestion

sorry, you mean checking the number of spacebindings before waiting for the SpaceBindingRequest to have the expected conditions ? Not sure I understood your suggestion ..

ah never mind, I mixed up space bindings and space requests 🤦‍♂️

Ok, no worries, just wanted to be sure!

xcoulon

looks good to me, only a couple of minor suggestions but nothing critical

openshift-ci · 2023-08-22T12:33:43Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [xcoulon]

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

openshift-ci · 2023-08-23T08:43:58Z

New changes are detected. LGTM label has been removed.

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

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

No Coverage information
3.4% Duplication

mfrancisc · 2023-08-23T11:43:54Z

@MatousJobanek I'm merging this , but we can keep the discussion going, and I'll address any other comment in follow up PRs.

add e2e tests for spacebindingrequests

e9c305e

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

Merge branch 'master' into sbrmain

8cc33c2

mfrancisc mentioned this pull request Aug 14, 2023

feat: Implement SpaceBindingRequest controller main logic codeready-toolchain/host-operator#848

Merged

mfrancisc added 4 commits August 14, 2023 12:12

fix linter

bc0dd70

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

6ee9078

increase timeout and retry interval for spacebinding deletion

ddb6944

Merge branch 'master' into sbrmain

fe4d402

mfrancisc mentioned this pull request Aug 16, 2023

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

Merged

mfrancisc added 3 commits August 16, 2023 15:45

add common test conditions and increase timeout for SBR deletion

ad360fc

wait for different spacebinding UID when testing deletion

5b7e715

Merge branch 'master' into sbrmain

688c198

MatousJobanek reviewed Aug 17, 2023

View reviewed changes

mfrancisc and others added 4 commits August 17, 2023 15:10

Update test/e2e/parallel/spacebindingrequest_test.go

01ec080

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

Update test/e2e/parallel/spacebindingrequest_test.go

c4e2d1a

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

remove spacebindingrequest deletion from make

9fda3e4

import assert

fb4504e

mfrancisc added 2 commits August 21, 2023 19:43

remove retry options, add nstemplateset check, remove creationtimesta…

88eaeb5

…mp check

sort usernames when checking nstemplateset

90d10e3

xcoulon reviewed Aug 22, 2023

View reviewed changes

xcoulon approved these changes Aug 22, 2023

View reviewed changes

openshift-ci bot assigned xcoulon Aug 22, 2023

openshift-ci bot added the lgtm label Aug 22, 2023

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

update api and common version

b70292b

openshift-ci bot removed the lgtm label Aug 23, 2023

mfrancisc merged commit 56cc5bc into codeready-toolchain:master Aug 23, 2023
7 checks passed

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

mfrancisc mentioned this pull request Aug 25, 2023

refactor: testsupport package segregation and cleanup #784

Merged

	space, spaceBindingRequest, spaceBinding := NewSpaceBindingRequest(t, awaitilities, memberAwait, hostAwait, "admin")
	space, spaceBindingRequest, spaceBinding := CreateSbrForMur(t, awaitilities, memberAwait, hostAwait, "admin")

	// wait for the namespace to be provisioned since we will be creating the SpaceBindingRequest into it.
	space, err := hostAwait.WaitForSpace(t, space.Name, UntilSpaceHasAnyProvisionedNamespaces())
	require.NoError(t, err)

	func InNamespace(namespace string) SpaceRequestOption {
	return func(s *toolchainv1alpha1.SpaceRequest) {
	s.ObjectMeta.Namespace = namespace
	}
	}

feat: Implement SpaceBindingRequest controller main logic #779

feat: Implement SpaceBindingRequest controller main logic #779

Conversation

mfrancisc commented 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

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 17, 2023 • edited Loading

Choose a reason for hiding this comment

mfrancisc Aug 21, 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

MatousJobanek Aug 17, 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 21, 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

xcoulon left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 22, 2023

openshift-ci bot commented Aug 23, 2023

sonarcloud bot commented Aug 23, 2023

mfrancisc commented Aug 23, 2023

mfrancisc commented Aug 14, 2023 •

edited

Loading

mfrancisc Aug 17, 2023 •

edited

Loading

mfrancisc Aug 21, 2023 •

edited

Loading

MatousJobanek Aug 17, 2023 •

edited

Loading