Skip to content

Commit

Permalink
fix: check conflicts with Space name for complaintUsername (#867)
Browse files Browse the repository at this point in the history 
* fix: check conflicts with Space name for complaintUsername

* Update controllers/usersignup/usersignup_controller.go

Co-authored-by: Xavier Coulon <[email protected]>

* cover when creation of a space should be skipped

* Update controllers/usersignup/usersignup_controller_test.go

Co-authored-by: Rajiv Senthilnathan <[email protected]>

* empty

---------

Co-authored-by: Xavier Coulon <[email protected]>
Co-authored-by: Rajiv Senthilnathan <[email protected]>
  • Loading branch information
@MatousJobanek @xcoulon @rajivnathan
3 people authored Sep 22, 2023
1 parent c68ff26 commit 4c62554
Show file tree
Hide file tree
Showing 2 changed files with 121 additions and 13 deletions.
20 changes: 16 additions & 4 deletions controllers/usersignup/usersignup_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
114 changes: 105 additions & 9 deletions controllers/usersignup/usersignup_controller_test.go
View file
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,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)
Expand Down Expand Up @@ -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()
Expand Down

0 comments on commit 4c62554

Please sign in to comment.