From 4c625541d79ccc6808657dace5605e3d7ea53d33 Mon Sep 17 00:00:00 2001 From: Matous Jobanek Date: Sat, 23 Sep 2023 01:11:50 +0200 Subject: [PATCH] fix: check conflicts with Space name for complaintUsername (#867) * fix: check conflicts with Space name for complaintUsername * Update controllers/usersignup/usersignup_controller.go Co-authored-by: Xavier Coulon * cover when creation of a space should be skipped * Update controllers/usersignup/usersignup_controller_test.go Co-authored-by: Rajiv Senthilnathan * empty --------- Co-authored-by: Xavier Coulon Co-authored-by: Rajiv Senthilnathan --- .../usersignup/usersignup_controller.go | 20 ++- .../usersignup/usersignup_controller_test.go | 114 ++++++++++++++++-- 2 files changed, 121 insertions(+), 13 deletions(-) diff --git a/controllers/usersignup/usersignup_controller.go b/controllers/usersignup/usersignup_controller.go index 0560e0c19..d03612ab6 100644 --- a/controllers/usersignup/usersignup_controller.go +++ b/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 { diff --git a/controllers/usersignup/usersignup_controller_test.go b/controllers/usersignup/usersignup_controller_test.go index f86774c0c..70528f374 100644 --- a/controllers/usersignup/usersignup_controller_test.go +++ b/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()