fix: check conflicts with Space name for complaintUsername

controllers/usersignup/usersignup_controller.go

@@ -530,19 +530,31 @@ func (r *Reconciler) generateCompliantUsername(config toolchainconfig.ToolchainC
 	for i := 2; i < 101; i++ { // No more than 100 attempts to find a vacant name
 		mur := &toolchainv1alpha1.MasterUserRecord{}
 		// Check if a MasterUserRecord exists with the same transformed name
-		namespacedMurName := types.NamespacedName{Namespace: instance.Namespace, Name: newUsername}
-		err := r.Client.Get(context.TODO(), namespacedMurName, mur)
+		namespacedName := types.NamespacedName{Namespace: instance.Namespace, Name: newUsername}
+		err := r.Client.Get(context.TODO(), namespacedName, mur)
 		if err != nil {
 			if !errors.IsNotFound(err) {
 				return "", err
 			}
-			// If there was a NotFound error looking up the mur, it means we found an available name
-			return newUsername, nil
+			if shouldManageSpace(instance) {
+				space := &toolchainv1alpha1.Space{}
+				if err = r.Client.Get(context.TODO(), namespacedName, space); err != nil {
+					if errors.IsNotFound(err) {
+						// If there was a NotFound error looking up the mur as well as space, it means we found an available name
+						return newUsername, nil
+					}
+					return "", err
+				}
+			} else {
+				// If there was a NotFound error looking up the mur and the creation of the default space should be skipped, then it means we found an available name
+				return newUsername, nil
+			}
 		} else if mur.Labels[toolchainv1alpha1.MasterUserRecordOwnerLabelKey] == instance.Name {
 			// If the found MUR has the same UserID as the UserSignup, then *it* is the correct MUR -
 			// Return an error here and allow the reconcile() function to pick it up on the next loop
 			return "", fmt.Errorf(fmt.Sprintf("INFO: could not generate compliant username as MasterUserRecord with the same name [%s] and user id [%s] already exists. The next reconcile loop will pick it up.", mur.Name, instance.Name))
 		}
+
 		if len(transformed) > maxlengthWithSuffix {
 			newUsername = transformed[:maxlengthWithSuffix] + fmt.Sprintf("-%d", i)
 		} else {

controllers/usersignup/usersignup_controller_test.go

@@ -3479,7 +3479,7 @@ func TestDeathBy100Signups(t *testing.T) {
 			initObjs = append(initObjs, userSignup, deactivate30Tier)
 			initObjs = append(initObjs, commonconfig.NewToolchainConfigObjWithReset(t, testconfig.AutomaticApproval().Enabled(true)))
 
-			// create 100 MURs that follow the naming pattern used by `generateCompliantUsername()`: `foo`, `foo-2`, ..., `foo-100`
+			// create 100 MURs and Spaces that follow the naming pattern used by `generateCompliantUsername()`: `foo`, `foo-2`, ..., `foo-100`
 			initObjs = append(initObjs, &toolchainv1alpha1.MasterUserRecord{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      testusername.compliantUsername,
@@ -3488,14 +3488,14 @@ func TestDeathBy100Signups(t *testing.T) {
 				},
 			})
 
-			for i := 2; i <= 100; i++ {
-				initObjs = append(initObjs, &toolchainv1alpha1.MasterUserRecord{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      fmt.Sprintf("%s-%d", testusername.replacedCompliantUsername, i),
-						Namespace: test.HostOperatorNs,
-						Labels:    map[string]string{toolchainv1alpha1.MasterUserRecordOwnerLabelKey: uuid.Must(uuid.NewV4()).String()},
-					},
-				})
+			// stagger the numbering for MURs and Spaces so that one of them will always be missing. eg. MURs will not be found on odd numbers and Spaces not found on even numbers until it makes 100 attempts
+			for i := 2; i <= 100; i += 2 {
+				initObjs = append(initObjs,
+					murtest.NewMasterUserRecord(t, fmt.Sprintf("%s-%d", testusername.replacedCompliantUsername, i), murtest.WithOwnerLabel(uuid.Must(uuid.NewV4()).String())))
+			}
+			for i := 3; i <= 100; i += 2 {
+				initObjs = append(initObjs,
+					spacetest.NewSpace(test.HostOperatorNs, fmt.Sprintf("%s-%d", testusername.replacedCompliantUsername, i)))
 			}
 
 			initObjs = append(initObjs, baseNSTemplateTier)
@@ -3563,6 +3563,102 @@ func TestDeathBy100Signups(t *testing.T) {
 	}
 }
 
+func TestGenerateUniqueCompliantUsername(t *testing.T) {
+	// given
+	logf.SetLogger(zap.New(zap.UseDevMode(true)))
+
+	mur := murtest.NewMasterUserRecord(t, "cool-user")
+	space := spacetest.NewSpace(test.HostOperatorNs, "cool-user")
+	spaceInTerminating := spacetest.NewSpace(test.HostOperatorNs, "cool-user",
+		spacetest.WithFinalizer(),
+		spacetest.WithDeletionTimestamp(),
+		spacetest.WithCreatorLabel("cool-user"))
+
+	for testcase, params := range map[string]struct {
+		conflictingObject runtimeclient.Object
+		skipSpaceCreation string
+		expectedUsername  string
+	}{
+		"with conflicting MasterUserRecord": {
+			conflictingObject: mur,
+			skipSpaceCreation: "false",
+			expectedUsername:  "cool-user-2",
+		},
+		"with conflicting MasterUserRecord when space creation is skipped": {
+			conflictingObject: mur,
+			skipSpaceCreation: "true",
+			expectedUsername:  "cool-user-2",
+		},
+		"with conflicting Space": {
+			conflictingObject: space,
+			skipSpaceCreation: "false",
+			expectedUsername:  "cool-user-2",
+		},
+		"with conflicting Space  when space creation is skipped": {
+			conflictingObject: space,
+			skipSpaceCreation: "true",
+			expectedUsername:  "cool-user",
+		},
+		"with conflicting Space in terminating state": {
+			conflictingObject: spaceInTerminating,
+			skipSpaceCreation: "false",
+			expectedUsername:  "cool-user-2",
+		},
+		"with conflicting Space in terminating state when space creation is skipped": {
+			conflictingObject: spaceInTerminating,
+			skipSpaceCreation: "true",
+			expectedUsername:  "cool-user",
+		},
+	} {
+		t.Run(testcase, func(t *testing.T) {
+			userSignup := commonsignup.NewUserSignup(
+				commonsignup.WithName("cool-user"),
+				commonsignup.ApprovedManually())
+
+			userSignup.Annotations[toolchainv1alpha1.SkipAutoCreateSpaceAnnotationKey] = params.skipSpaceCreation
+
+			ready := NewGetMemberClusters(NewMemberClusterWithTenantRole(t, "member1", corev1.ConditionTrue))
+			r, req, _ := prepareReconcile(t, userSignup.Name, ready, userSignup, baseNSTemplateTier,
+				deactivate30Tier, params.conflictingObject, commonconfig.NewToolchainConfigObjWithReset(t, testconfig.AutomaticApproval().Enabled(true)))
+
+			InitializeCounters(t, NewToolchainStatus())
+
+			// when
+			res, err := r.Reconcile(context.TODO(), req)
+
+			// then
+			require.NoError(t, err)
+			require.Equal(t, reconcile.Result{}, res)
+
+			// Lookup the user signup again
+			murtest.AssertThatMasterUserRecord(t, params.expectedUsername, r.Client).
+				HasLabelWithValue(toolchainv1alpha1.MasterUserRecordOwnerLabelKey, "cool-user")
+			userSignup = AssertThatUserSignup(t, test.HostOperatorNs, "cool-user", r.Client).
+				HasCompliantUsername("").
+				HasLabel(toolchainv1alpha1.UserSignupStateLabelKey, "approved").
+				Get()
+
+			test.AssertConditionsMatch(t, userSignup.Status.Conditions,
+				toolchainv1alpha1.Condition{
+					Type:   toolchainv1alpha1.UserSignupApproved,
+					Status: corev1.ConditionTrue,
+					Reason: "ApprovedByAdmin",
+				},
+				toolchainv1alpha1.Condition{
+					Type:   toolchainv1alpha1.UserSignupUserDeactivatingNotificationCreated,
+					Status: corev1.ConditionFalse,
+					Reason: "UserNotInPreDeactivation",
+				},
+				toolchainv1alpha1.Condition{
+					Type:   toolchainv1alpha1.UserSignupUserDeactivatedNotificationCreated,
+					Status: corev1.ConditionFalse,
+					Reason: "UserIsActive",
+				},
+			)
+		})
+	}
+}
+
 func TestUserSignupWithMultipleExistingMURNotOK(t *testing.T) {
 	// given
 	userSignup := commonsignup.NewUserSignup()