API key: Fix API key migration to service account (#97100)

fix api key migration to service account

pkg/services/serviceaccounts/database/store.go

@@ -509,11 +509,30 @@ func (s *ServiceAccountsStoreImpl) CreateServiceAccountFromApikey(ctx context.Co
 		IsServiceAccount: true,
 	}
 
+	// maximum number of attempts for creating a service account
+	attempts := 10
+
 	return s.sqlStore.InTransaction(ctx, func(tctx context.Context) error {
 		newSA, errCreateSA := s.userService.CreateServiceAccount(tctx, &cmd)
+		if errCreateSA != nil {
+			if errors.Is(errCreateSA, serviceaccounts.ErrServiceAccountAlreadyExists) {
+				// The service account we tried to create already exists with that login name. We will attempt to create
+				// a unique service account by adding suffixes to the initial login name (e.g. -001, -002, ... , -010).
+				for i := 1; errCreateSA != nil && i <= attempts; i++ {
+					serviceAccountName := fmt.Sprintf("%s-%03d", key.Name, i)
+					cmd.Login = generateLogin(prefix, key.OrgID, serviceAccountName)
+					newSA, errCreateSA = s.userService.CreateServiceAccount(tctx, &cmd)
+					if errCreateSA != nil && !errors.Is(errCreateSA, serviceaccounts.ErrServiceAccountAlreadyExists) {
+						break
+					}
+				}
+			}
+		}
+
 		if errCreateSA != nil {
 			return fmt.Errorf("failed to create service account: %w", errCreateSA)
 		}
+
 		return s.assignApiKeyToServiceAccount(tctx, key.ID, newSA.ID)
 	})
 }

pkg/services/serviceaccounts/database/store_test.go

@@ -324,26 +324,85 @@ func TestIntegrationStore_MigrateApiKeys(t *testing.T) {
 		t.Skip("skipping test in short mode")
 	}
 	cases := []struct {
-		desc        string
+		desc            string
-		key         tests.TestApiKey
+		serviceAccounts []user.CreateUserCommand
-		expectedErr error
+		key             tests.TestApiKey
+		expectedLogin   string
+		expectedErr     error
 	}{
 		{
-			desc:        "api key should be migrated to service account token",
+			desc:            "api key should be migrated to service account token",
-			key:         tests.TestApiKey{Name: "Test1", Role: org.RoleEditor, OrgId: 1},
+			serviceAccounts: []user.CreateUserCommand{},
-			expectedErr: nil,
+			key:             tests.TestApiKey{Name: "test1", Role: org.RoleEditor, OrgId: 1},
+			expectedLogin:   "sa-autogen-1-test1",
+			expectedErr:     nil,
+		},
+		{
+			desc: "api key should be migrated to service account token on second attempt",
+			serviceAccounts: []user.CreateUserCommand{
+				{Login: "sa-autogen-1-test2"},
+			},
+			key:           tests.TestApiKey{Name: "test2", Role: org.RoleEditor, OrgId: 1},
+			expectedLogin: "sa-autogen-1-test2-001",
+			expectedErr:   nil,
+		},
+		{
+			desc: "api key should be migrated to service account token on last attempt (the 10th)",
+			serviceAccounts: []user.CreateUserCommand{
+				{Login: "sa-autogen-1-test3"},
+				{Login: "sa-autogen-1-test3-001"},
+				{Login: "sa-autogen-1-test3-002"},
+				{Login: "sa-autogen-1-test3-003"},
+				{Login: "sa-autogen-1-test3-004"},
+				{Login: "sa-autogen-1-test3-005"},
+				{Login: "sa-autogen-1-test3-006"},
+				{Login: "sa-autogen-1-test3-007"},
+				{Login: "sa-autogen-1-test3-008"},
+				{Login: "sa-autogen-1-test3-009"},
+			},
+			key:           tests.TestApiKey{Name: "test3", Role: org.RoleEditor, OrgId: 1},
+			expectedLogin: "sa-autogen-1-test3-010",
+			expectedErr:   nil,
+		},
+		{
+			desc: "api key should not be migrated to service account token because all attempts failed",
+			serviceAccounts: []user.CreateUserCommand{
+				{Login: "sa-autogen-1-test4"},
+				{Login: "sa-autogen-1-test4-001"},
+				{Login: "sa-autogen-1-test4-002"},
+				{Login: "sa-autogen-1-test4-003"},
+				{Login: "sa-autogen-1-test4-004"},
+				{Login: "sa-autogen-1-test4-005"},
+				{Login: "sa-autogen-1-test4-006"},
+				{Login: "sa-autogen-1-test4-007"},
+				{Login: "sa-autogen-1-test4-008"},
+				{Login: "sa-autogen-1-test4-009"},
+				{Login: "sa-autogen-1-test4-010"},
+			},
+			key:         tests.TestApiKey{Name: "test4", Role: org.RoleEditor, OrgId: 1},
+			expectedErr: serviceaccounts.ErrServiceAccountAlreadyExists,
 		},
 	}
 
 	for _, c := range cases {
 		t.Run(c.desc, func(t *testing.T) {
 			db, store := setupTestDatabase(t)
+
 			store.cfg.AutoAssignOrg = true
 			store.cfg.AutoAssignOrgId = 1
 			store.cfg.AutoAssignOrgRole = "Viewer"
 			_, err := store.orgService.CreateWithMember(context.Background(), &org.CreateOrgCommand{Name: "main"})
 			require.NoError(t, err)
+
 			key := tests.SetupApiKey(t, db, store.cfg, c.key)
+
+			for _, sa := range c.serviceAccounts {
+				sa.IsServiceAccount = true
+				sa.OrgID = key.OrgID
+				_, err := store.userService.CreateServiceAccount(context.Background(), &sa)
+				require.NoError(t, err)
+			}
+
 			err = store.MigrateApiKey(context.Background(), key.OrgID, key.ID)
 			if c.expectedErr != nil {
 				require.ErrorIs(t, err, c.expectedErr)
@@ -352,7 +411,7 @@ func TestIntegrationStore_MigrateApiKeys(t *testing.T) {
 
 				q := serviceaccounts.SearchOrgServiceAccountsQuery{
 					OrgID: key.OrgID,
-					Query: "",
+					Query: c.expectedLogin,
 					Page:  1,
 					Limit: 50,
 					SignedInUser: &user.SignedInUser{
@@ -370,6 +429,7 @@ func TestIntegrationStore_MigrateApiKeys(t *testing.T) {
 				require.Equal(t, int64(1), serviceAccounts.TotalCount)
 				saMigrated := serviceAccounts.ServiceAccounts[0]
 				require.Equal(t, string(key.Role), saMigrated.Role)
+				require.Equal(t, c.expectedLogin, saMigrated.Login)
 
 				tokens, err := store.ListTokens(context.Background(), &serviceaccounts.GetSATokensQuery{
 					OrgID:            &key.OrgID,