feat(controlplane): make organization names unique (#525)

Signed-off-by: Miguel Martinez Trivino <[email protected]>

Author: migmartri

app/controlplane/internal/biz/apitoken_integration_test.go

@@ -161,7 +161,7 @@ func (s *apiTokenTestSuite) TestList() {
 	})
 
 	s.T().Run("returns empty list", func(t *testing.T) {
-		emptyOrg, err := s.Organization.Create(ctx, "org1")
+		emptyOrg, err := s.Organization.CreateWithRandomName(ctx)
 		require.NoError(s.T(), err)
 		tokens, err := s.APIToken.List(ctx, emptyOrg.ID, false)
 		s.NoError(err)
@@ -240,9 +240,9 @@ func (s *apiTokenTestSuite) SetupTest() {
 	ctx := context.Background()
 
 	s.TestingUseCases = testhelpers.NewTestingUseCases(t)
-	s.org, err = s.Organization.Create(ctx, "org1")
+	s.org, err = s.Organization.CreateWithRandomName(ctx)
 	assert.NoError(err)
-	s.org2, err = s.Organization.Create(ctx, "org2")
+	s.org2, err = s.Organization.CreateWithRandomName(ctx)
 	assert.NoError(err)
 
 	// Create 2 tokens for org 1

app/controlplane/internal/biz/biz.go

@@ -15,7 +15,15 @@
 
 package biz
 
-import "github.com/google/wire"
+import (
+	"crypto/rand"
+	"fmt"
+	"math/big"
+	"strings"
+
+	"github.com/google/wire"
+	"github.com/moby/moby/pkg/namesgenerator"
+)
 
 // ProviderSet is biz providers.
 var ProviderSet = wire.NewSet(
@@ -42,3 +50,19 @@ var ProviderSet = wire.NewSet(
 	wire.Struct(new(NewIntegrationUseCaseOpts), "*"),
 	wire.Struct(new(NewUserUseCaseParams), "*"),
 )
+
+// generate a DNS1123-valid random name using moby's namesgenerator
+// plus an additional random number
+func generateRandomName() (string, error) {
+	// Create a random name
+	name := namesgenerator.GetRandomName(0)
+	// and append a random number to it
+	randomNumber, err := rand.Int(rand.Reader, big.NewInt(10000))
+	if err != nil {
+		return "", fmt.Errorf("failed to generate random number: %w", err)
+	}
+
+	// Replace underscores with dashes to make it compatible with DNS1123
+	name = strings.ReplaceAll(fmt.Sprintf("%s-%d", name, randomNumber), "_", "-")
+	return name, nil
+}

app/controlplane/internal/biz/errors.go

@@ -20,6 +20,8 @@ import (
 	"fmt"
 )
 
+var ErrAlreadyExists = errors.New("duplicate")
+
 type ErrNotFound struct {
 	entity string
 }

app/controlplane/internal/biz/membership_integration_test.go

@@ -34,9 +34,9 @@ func (s *membershipIntegrationTestSuite) TestDeleteWithOrg() {
 	s.NoError(err)
 	user2, err := s.User.FindOrCreateByEmail(ctx, "[email protected]")
 	s.NoError(err)
-	userOrg, err := s.Organization.Create(ctx, "foo")
+	userOrg, err := s.Organization.CreateWithRandomName(ctx)
 	s.NoError(err)
-	sharedOrg, err := s.Organization.Create(ctx, "shared-org")
+	sharedOrg, err := s.Organization.CreateWithRandomName(ctx)
 	s.NoError(err)
 
 	mUser, err := s.Membership.Create(ctx, userOrg.ID, user.ID, true)
@@ -100,7 +100,7 @@ func (s *membershipIntegrationTestSuite) TestCreateMembership() {
 	assert.NoError(err)
 
 	s.T().Run("Create default", func(t *testing.T) {
-		org, err := s.Organization.Create(ctx, "foo")
+		org, err := s.Organization.CreateWithRandomName(ctx)
 		assert.NoError(err)
 
 		m, err := s.Membership.Create(ctx, org.ID, user.ID, true)
@@ -119,7 +119,7 @@ func (s *membershipIntegrationTestSuite) TestCreateMembership() {
 	})
 
 	s.T().Run("Non current", func(t *testing.T) {
-		org, err := s.Organization.Create(ctx, "foo")
+		org, err := s.Organization.CreateWithRandomName(ctx)
 		assert.NoError(err)
 
 		m, err := s.Membership.Create(ctx, org.ID, user.ID, false)
@@ -134,7 +134,7 @@ func (s *membershipIntegrationTestSuite) TestCreateMembership() {
 	})
 
 	s.T().Run("Invalid User", func(t *testing.T) {
-		org, err := s.Organization.Create(ctx, "foo")
+		org, err := s.Organization.CreateWithRandomName(ctx)
 		assert.NoError(err)
 		m, err := s.Membership.Create(ctx, org.ID, uuid.NewString(), false)
 		assert.Error(err)

app/controlplane/internal/biz/organization.go

@@ -19,12 +19,10 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"strings"
 	"time"
 
 	"github.com/go-kratos/kratos/v2/log"
 	"github.com/google/uuid"
-	"github.com/moby/moby/pkg/namesgenerator"
 	"k8s.io/apimachinery/pkg/util/validation"
 )
 
@@ -57,22 +55,64 @@ func NewOrganizationUseCase(repo OrganizationRepo, repoUC *CASBackendUseCase, iU
 	}
 }
 
+const OrganizationRandomNameMaxTries = 10
+
+func (uc *OrganizationUseCase) CreateWithRandomName(ctx context.Context) (*Organization, error) {
+	// Try 10 times to create a random name
+	for i := 0; i < OrganizationRandomNameMaxTries; i++ {
+		// Create a random name
+		name, err := generateRandomName()
+		if err != nil {
+			return nil, fmt.Errorf("failed to generate random name: %w", err)
+		}
+
+		org, err := uc.doCreate(ctx, name)
+		if err != nil {
+			// We retry if the organization already exists
+			if errors.Is(err, ErrAlreadyExists) {
+				uc.logger.Debugw("msg", "Org exists!", "name", name)
+				continue
+			}
+			uc.logger.Debugw("msg", "BOOM", "name", name, "err", err)
+			return nil, err
+		}
+
+		return org, nil
+	}
+
+	return nil, errors.New("failed to create a random organization name")
+}
+
+// Create an organization with the given name
 func (uc *OrganizationUseCase) Create(ctx context.Context, name string) (*Organization, error) {
-	// Create a random name if none is provided
-	if name == "" {
-		name = namesgenerator.GetRandomName(0)
-		// Replace underscores with dashes to make it compatible with DNS1123
-		name = strings.ReplaceAll(name, "_", "-")
+	org, err := uc.doCreate(ctx, name)
+	if err != nil {
+		if errors.Is(err, ErrAlreadyExists) {
+			return nil, NewErrValidationStr("organization already exists")
+		}
+
+		return nil, fmt.Errorf("failed to create organization: %w", err)
 	}
 
-	if err := validateOrgName(name); err != nil {
+	return org, nil
+}
+
+func (uc *OrganizationUseCase) doCreate(ctx context.Context, name string) (*Organization, error) {
+	uc.logger.Infow("msg", "Creating organization", "name", name)
+
+	if err := ValidateOrgName(name); err != nil {
 		return nil, NewErrValidation(fmt.Errorf("invalid organization name: %w", err))
 	}
 
-	return uc.orgRepo.Create(ctx, name)
+	org, err := uc.orgRepo.Create(ctx, name)
+	if err != nil {
+		return nil, fmt.Errorf("failed to create organization: %w", err)
+	}
+
+	return org, nil
 }
 
-func validateOrgName(name string) error {
+func ValidateOrgName(name string) error {
 	// The same validation done by Kubernetes for their namespace name
 	// https://github.com/kubernetes/apimachinery/blob/fa98d6eaedb4caccd69fc07d90bbb6a1e551f00f/pkg/api/validation/generic.go#L63
 	err := validation.IsDNS1123Label(name)
@@ -101,7 +141,7 @@ func (uc *OrganizationUseCase) Update(ctx context.Context, userID, orgID string,
 
 	// We validate the name to get ready for the name to become identifiers
 	if name != nil {
-		if err := validateOrgName(*name); err != nil {
+		if err := ValidateOrgName(*name); err != nil {
 			return nil, NewErrValidation(fmt.Errorf("invalid organization name: %w", err))
 		}
 	}

app/controlplane/internal/biz/organization_integration_test.go

@@ -35,16 +35,22 @@ import (
 	"github.com/stretchr/testify/suite"
 )
 
-// and delete cascades that we want to validate that they work too
+func (s *OrgIntegrationTestSuite) TestCreateWithRandomName() {
+	// It can create thousands of orgs without any problem
+	for i := 0; i < 1000; i++ {
+		org, err := s.Organization.CreateWithRandomName(context.Background())
+		s.NoError(err)
+		s.NotNil(org)
+	}
+}
+
 func (s *OrgIntegrationTestSuite) TestCreate() {
 	ctx := context.Background()
 
 	testCases := []struct {
 		name          string
 		expectedError bool
 	}{
-		// autogenerated name
-		{"", false},
 		{"a", false},
 		{"aa-aa", false},
 		{"-aaa", true},
@@ -96,7 +102,7 @@ func (s *OrgIntegrationTestSuite) TestUpdate() {
 	})
 
 	s.T().Run("org not accessible to user", func(t *testing.T) {
-		org2, err := s.Organization.Create(ctx, "testing-org")
+		org2, err := s.Organization.CreateWithRandomName(ctx)
 		require.NoError(s.T(), err)
 		_, err = s.Organization.Update(ctx, s.user.ID, org2.ID, nil)
 		s.Error(err)

app/controlplane/internal/biz/organization_test.go

@@ -13,18 +13,49 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package biz
+package biz_test
 
 import (
+	"context"
+	"io"
 	"testing"
 
+	"github.com/chainloop-dev/chainloop/app/controlplane/internal/biz"
+	repoM "github.com/chainloop-dev/chainloop/app/controlplane/internal/biz/mocks"
+	"github.com/go-kratos/kratos/v2/log"
+	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/suite"
 )
 
 type organizationTestSuite struct {
 	suite.Suite
 }
 
+func (s *organizationTestSuite) TestCreateWithRandomName() {
+	repo := repoM.NewOrganizationRepo(s.T())
+	uc := biz.NewOrganizationUseCase(repo, nil, nil, nil, log.NewStdLogger(io.Discard))
+
+	s.Run("the org exists, we retry", func() {
+		ctx := context.Background()
+		// the first one fails because it already exists
+		repo.On("Create", ctx, mock.Anything).Once().Return(nil, biz.ErrAlreadyExists)
+		// but the second call creates the org
+		repo.On("Create", ctx, mock.Anything).Once().Return(&biz.Organization{Name: "foobar"}, nil)
+		got, err := uc.CreateWithRandomName(ctx)
+		s.NoError(err)
+		s.Equal("foobar", got.Name)
+	})
+
+	s.Run("if it runs out of tries, it fails", func() {
+		ctx := context.Background()
+		// the first one fails because it already exists
+		repo.On("Create", ctx, mock.Anything).Times(biz.OrganizationRandomNameMaxTries).Return(nil, biz.ErrAlreadyExists)
+		got, err := uc.CreateWithRandomName(ctx)
+		s.Error(err)
+		s.Nil(got)
+	})
+}
+
 func (s *organizationTestSuite) TestValidateOrgName() {
 	testCases := []struct {
 		name          string
@@ -47,7 +78,7 @@ func (s *organizationTestSuite) TestValidateOrgName() {
 
 	for _, tc := range testCases {
 		s.T().Run(tc.name, func(t *testing.T) {
-			err := validateOrgName(tc.name)
+			err := biz.ValidateOrgName(tc.name)
 			if tc.expectedError {
 				s.Error(err)
 			} else {

app/controlplane/internal/biz/referrer_integration_test.go

@@ -359,9 +359,9 @@ func (s *referrerIntegrationTestSuite) SetupTest() {
 	ctx := context.Background()
 
 	var err error
-	s.org1, err = s.Organization.Create(ctx, "testing-org")
+	s.org1, err = s.Organization.CreateWithRandomName(ctx)
 	require.NoError(s.T(), err)
-	s.org2, err = s.Organization.Create(ctx, "testing-org-2")
+	s.org2, err = s.Organization.CreateWithRandomName(ctx)
 	require.NoError(s.T(), err)
 
 	s.org1UUID, err = uuid.Parse(s.org1.ID)

app/controlplane/internal/biz/user_integration_test.go

@@ -78,9 +78,9 @@ func (s *userIntegrationTestSuite) SetupTest() {
 
 	s.TestingUseCases = testhelpers.NewTestingUseCases(t)
 
-	s.userOneOrg, err = s.Organization.Create(ctx, "user-1-org")
+	s.userOneOrg, err = s.Organization.CreateWithRandomName(ctx)
 	assert.NoError(err)
-	s.sharedOrg, err = s.Organization.Create(ctx, "shared-org")
+	s.sharedOrg, err = s.Organization.CreateWithRandomName(ctx)
 	assert.NoError(err)
 
 	// Create User 1

app/controlplane/internal/biz/workflow_integration_test.go

@@ -38,7 +38,7 @@ func (s *workflowIntegrationTestSuite) TestUpdate() {
 		project = "test project"
 	)
 
-	org2, err := s.Organization.Create(context.Background(), "testing-org")
+	org2, err := s.Organization.CreateWithRandomName(context.Background())
 	require.NoError(s.T(), err)
 	workflow, err := s.Workflow.Create(ctx, &biz.WorkflowCreateOpts{Name: name, OrgID: s.org.ID})
 	require.NoError(s.T(), err)
@@ -148,7 +148,7 @@ func (s *workflowIntegrationTestSuite) SetupTest() {
 	s.TestingUseCases = testhelpers.NewTestingUseCases(s.T())
 
 	ctx := context.Background()
-	s.org, err = s.Organization.Create(ctx, "testing-org")
+	s.org, err = s.Organization.CreateWithRandomName(ctx)
 	assert.NoError(err)
 }