Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: check conflicts with Space name for complaintUsername #867

Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions controllers/usersignup/usersignup_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -530,19 +530,26 @@ 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
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 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 {
Expand Down
80 changes: 71 additions & 9 deletions controllers/usersignup/usersignup_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -3488,14 +3488,13 @@ 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()},
},
})
for i := 2; i <= 100; i += 2 {
MatousJobanek marked this conversation as resolved.
Show resolved Hide resolved
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)
Expand Down Expand Up @@ -3563,6 +3562,69 @@ 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, object := range map[string]runtimeclient.Object{
"with conflicting MasterUserRecord": mur,
"with conflicting Space": space,
"with conflicting Space in terminating state": spaceInTerminating,
} {
t.Run(testcase, func(t *testing.T) {
userSignup := commonsignup.NewUserSignup(
commonsignup.WithName("cool-user"),
commonsignup.ApprovedManually())

ready := NewGetMemberClusters(NewMemberClusterWithTenantRole(t, "member1", corev1.ConditionTrue))
r, req, _ := prepareReconcile(t, userSignup.Name, ready, userSignup, baseNSTemplateTier,
deactivate30Tier, object, 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, "cool-user-2", 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()
Expand Down