feat(controlplane): make contract-names unique and DNS1123 compatible (#601)

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

Author: migmartri

app/controlplane/internal/biz/biz.go

@@ -1,5 +1,5 @@
 //
-// Copyright 2023 The Chainloop Authors.
+// Copyright 2024 The Chainloop Authors.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -17,12 +17,13 @@ package biz
 
 import (
 	"crypto/rand"
+	"errors"
 	"fmt"
 	"math/big"
 	"strings"
 
 	"github.com/google/wire"
-	"github.com/moby/moby/pkg/namesgenerator"
+	"k8s.io/apimachinery/pkg/util/validation"
 )
 
 // ProviderSet is biz providers.
@@ -53,16 +54,35 @@ var ProviderSet = wire.NewSet(
 
 // 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
+func generateValidDNS1123WithSuffix(prefix string) (string, error) {
+	// 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), "_", "-")
+	name := strings.ReplaceAll(fmt.Sprintf("%s-%d", prefix, randomNumber), "_", "-")
+
+	if err := ValidateIsDNS1123(name); err != nil {
+		return "", fmt.Errorf("generated name is not DNS1123-valid: %w", err)
+	}
+
 	return name, nil
 }
+
+func ValidateIsDNS1123(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)
+	if len(err) > 0 {
+		errMsg := ""
+		for _, e := range err {
+			errMsg += e + "\n"
+		}
+
+		return errors.New(errMsg)
+	}
+
+	return nil
+}

app/controlplane/internal/biz/casmapping_integration_test.go

@@ -352,10 +352,10 @@ func (s *casMappingIntegrationSuite) SetupTest() {
 
 	// Create workflowRun in the database
 	// Workflow
-	workflow, err := s.Workflow.Create(ctx, &biz.WorkflowCreateOpts{Name: "test workflow", OrgID: s.org1.ID})
+	workflow, err := s.Workflow.Create(ctx, &biz.WorkflowCreateOpts{Name: "test-workflow", OrgID: s.org1.ID})
 	assert.NoError(err)
 
-	publicWorkflow, err := s.Workflow.Create(ctx, &biz.WorkflowCreateOpts{Name: "test workflow", OrgID: s.org1.ID, Public: true})
+	publicWorkflow, err := s.Workflow.Create(ctx, &biz.WorkflowCreateOpts{Name: "test-workflow", OrgID: s.org1.ID, Public: true})
 	assert.NoError(err)
 
 	// Robot account

app/controlplane/internal/biz/integration_test.go

@@ -193,7 +193,7 @@ func (s *testSuite) SetupTest() {
 	assert.NoError(err)
 
 	// Workflow
-	s.workflow, err = s.Workflow.Create(ctx, &biz.WorkflowCreateOpts{Name: "test workflow", OrgID: s.org.ID})
+	s.workflow, err = s.Workflow.Create(ctx, &biz.WorkflowCreateOpts{Name: "test-workflow", OrgID: s.org.ID})
 	assert.NoError(err)
 
 	// Integration configuration

app/controlplane/internal/biz/organization.go

@@ -23,7 +23,7 @@ import (
 
 	"github.com/go-kratos/kratos/v2/log"
 	"github.com/google/uuid"
-	"k8s.io/apimachinery/pkg/util/validation"
+	"github.com/moby/moby/pkg/namesgenerator"
 )
 
 type Organization struct {
@@ -55,7 +55,7 @@ func NewOrganizationUseCase(repo OrganizationRepo, repoUC *CASBackendUseCase, iU
 	}
 }
 
-const OrganizationRandomNameMaxTries = 10
+const RandomNameMaxTries = 10
 
 type createOptions struct {
 	createInlineBackend bool
@@ -72,9 +72,10 @@ func WithCreateInlineBackend() CreateOpt {
 
 func (uc *OrganizationUseCase) CreateWithRandomName(ctx context.Context, opts ...CreateOpt) (*Organization, error) {
 	// Try 10 times to create a random name
-	for i := 0; i < OrganizationRandomNameMaxTries; i++ {
+	for i := 0; i < RandomNameMaxTries; i++ {
 		// Create a random name
-		name, err := generateRandomName()
+		prefix := namesgenerator.GetRandomName(0)
+		name, err := generateValidDNS1123WithSuffix(prefix)
 		if err != nil {
 			return nil, fmt.Errorf("failed to generate random name: %w", err)
 		}
@@ -115,7 +116,7 @@ var errOrgName = errors.New("org names must only contain lowercase letters, numb
 func (uc *OrganizationUseCase) doCreate(ctx context.Context, name string, opts ...CreateOpt) (*Organization, error) {
 	uc.logger.Infow("msg", "Creating organization", "name", name)
 
-	if err := ValidateOrgName(name); err != nil {
+	if err := ValidateIsDNS1123(name); err != nil {
 		return nil, NewErrValidation(errOrgName)
 	}
 
@@ -139,22 +140,6 @@ func (uc *OrganizationUseCase) doCreate(ctx context.Context, name string, opts .
 	return org, nil
 }
 
-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)
-	if len(err) > 0 {
-		errMsg := ""
-		for _, e := range err {
-			errMsg += e + "\n"
-		}
-
-		return errors.New(errMsg)
-	}
-
-	return nil
-}
-
 func (uc *OrganizationUseCase) Update(ctx context.Context, userID, orgID string, name *string) (*Organization, error) {
 	userUUID, err := uuid.Parse(userID)
 	if err != nil {
@@ -168,7 +153,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 := ValidateIsDNS1123(*name); err != nil {
 			return nil, NewErrValidation(errOrgName)
 		}
 	}
@@ -185,7 +170,7 @@ func (uc *OrganizationUseCase) Update(ctx context.Context, userID, orgID string,
 	org, err := uc.orgRepo.Update(ctx, orgUUID, name)
 	if err != nil {
 		if errors.Is(err, ErrAlreadyExists) {
-			return nil, NewErrValidationStr("an organization with that name already exists")
+			return nil, NewErrValidationStr("a organization with that name already exists")
 		}
 
 		return nil, fmt.Errorf("failed to update organization: %w", err)

app/controlplane/internal/biz/organization_integration_test.go

@@ -248,7 +248,7 @@ func (s *OrgIntegrationTestSuite) SetupTest() {
 	assert.NoError(err)
 
 	// Workflow + contract
-	_, err = s.Workflow.Create(ctx, &biz.WorkflowCreateOpts{Name: "test workflow", OrgID: s.org.ID})
+	_, err = s.Workflow.Create(ctx, &biz.WorkflowCreateOpts{Name: "test-workflow", OrgID: s.org.ID})
 	assert.NoError(err)
 
 	// check integration, OCI repository and workflow and contracts are present in the db

app/controlplane/internal/biz/organization_test.go

@@ -49,7 +49,7 @@ func (s *organizationTestSuite) TestCreateWithRandomName() {
 	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)
+		repo.On("Create", ctx, mock.Anything).Times(biz.RandomNameMaxTries).Return(nil, biz.ErrAlreadyExists)
 		got, err := uc.CreateWithRandomName(ctx)
 		s.Error(err)
 		s.Nil(got)
@@ -78,7 +78,7 @@ func (s *organizationTestSuite) TestValidateOrgName() {
 
 	for _, tc := range testCases {
 		s.T().Run(tc.name, func(t *testing.T) {
-			err := biz.ValidateOrgName(tc.name)
+			err := biz.ValidateIsDNS1123(tc.name)
 			if tc.expectedError {
 				s.Error(err)
 			} else {

app/controlplane/internal/biz/referrer_integration_test.go

@@ -371,7 +371,7 @@ func (s *referrerIntegrationTestSuite) SetupTest() {
 
 	s.workflow1, err = s.Workflow.Create(ctx, &biz.WorkflowCreateOpts{Name: "wf", Team: "team", OrgID: s.org1.ID})
 	require.NoError(s.T(), err)
-	s.workflow2, err = s.Workflow.Create(ctx, &biz.WorkflowCreateOpts{Name: "wf from org 2", Team: "team", OrgID: s.org2.ID})
+	s.workflow2, err = s.Workflow.Create(ctx, &biz.WorkflowCreateOpts{Name: "wf-from-org-2", Team: "team", OrgID: s.org2.ID})
 	require.NoError(s.T(), err)
 
 	// user 1 has access to org 1 and 2

app/controlplane/internal/biz/workflow.go

@@ -122,8 +122,13 @@ func (uc *WorkflowUseCase) findOrCreateContract(ctx context.Context, orgID, cont
 	}
 
 	// No contractID has been provided so we create a new one
-	contractName := fmt.Sprintf("%s/%s", project, name)
-	return uc.contractUC.Create(ctx, orgID, contractName, nil)
+	contractName := name
+	// Project might be empty
+	if project != "" {
+		contractName = fmt.Sprintf("%s-%s", project, name)
+	}
+
+	return uc.contractUC.Create(ctx, &WorkflowContractCreateOpts{OrgID: orgID, Name: contractName, AddUniquePrefix: true})
 }
 
 func (uc *WorkflowUseCase) List(ctx context.Context, orgID string) ([]*Workflow, error) {

app/controlplane/internal/biz/workflow_integration_test.go

@@ -57,9 +57,9 @@ func (s *workflowIntegrationTestSuite) TestContractLatestAvailable() {
 func (s *workflowIntegrationTestSuite) TestUpdate() {
 	ctx := context.Background()
 	const (
-		name        = "test workflow"
+		name        = "test-workflow"
 		team        = "test team"
-		project     = "test project"
+		project     = "test-project"
 		description = "test description"
 	)
 

app/controlplane/internal/biz/workflowcontract.go

@@ -1,5 +1,5 @@
 //
-// Copyright 2023 The Chainloop Authors.
+// Copyright 2024 The Chainloop Authors.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -102,35 +102,94 @@ func (uc *WorkflowContractUseCase) FindByIDInOrg(ctx context.Context, orgID, con
 	return uc.repo.FindByIDInOrg(ctx, orgUUID, contractUUID)
 }
 
+type WorkflowContractCreateOpts struct {
+	OrgID, Name string
+	Schema      *schemav1.CraftingSchema
+	// Make sure that the name is unique in the organization
+	AddUniquePrefix bool
+}
+
 // we currently only support schema v1
-func (uc *WorkflowContractUseCase) Create(ctx context.Context, orgID, name string, schema *schemav1.CraftingSchema) (*WorkflowContract, error) {
-	orgUUID, err := uuid.Parse(orgID)
+func (uc *WorkflowContractUseCase) Create(ctx context.Context, opts *WorkflowContractCreateOpts) (*WorkflowContract, error) {
+	if opts.OrgID == "" || opts.Name == "" {
+		return nil, NewErrValidationStr("organization and name are required")
+	}
+
+	orgUUID, err := uuid.Parse(opts.OrgID)
 	if err != nil {
 		return nil, err
 	}
 
+	if err := ValidateIsDNS1123(opts.Name); err != nil {
+		return nil, NewErrValidation(err)
+	}
+
 	// If no schema is provided we create an empty one
-	if schema == nil {
-		schema = &schemav1.CraftingSchema{
+	if opts.Schema == nil {
+		opts.Schema = &schemav1.CraftingSchema{
 			SchemaVersion: "v1",
 		}
+	}
 
-		if err := schema.ValidateAll(); err != nil {
-			return nil, err
-		}
+	if err := opts.Schema.ValidateAll(); err != nil {
+		return nil, err
 	}
 
-	rawSchema, err := proto.Marshal(schema)
+	rawSchema, err := proto.Marshal(opts.Schema)
 	if err != nil {
 		return nil, err
 	}
 
-	opts := &ContractCreateOpts{
-		OrgID: orgUUID, Name: name,
+	// Create a workflow with a unique name if needed
+	args := &ContractCreateOpts{
+		OrgID: orgUUID, Name: opts.Name,
 		ContractBody: rawSchema,
 	}
 
-	return uc.repo.Create(ctx, opts)
+	var c *WorkflowContract
+	if opts.AddUniquePrefix {
+		c, err = uc.createWithUniqueName(ctx, args)
+	} else {
+		c, err = uc.repo.Create(ctx, args)
+	}
+
+	if err != nil {
+		if errors.Is(err, ErrAlreadyExists) {
+			return nil, NewErrValidationStr("that name is already taken")
+		}
+
+		return nil, fmt.Errorf("failed to create contract: %w", err)
+	}
+
+	return c, nil
+}
+
+func (uc *WorkflowContractUseCase) createWithUniqueName(ctx context.Context, opts *ContractCreateOpts) (*WorkflowContract, error) {
+	originalName := opts.Name
+
+	for i := 0; i < RandomNameMaxTries; i++ {
+		// append a suffix
+		if i > 0 {
+			var err error
+			opts.Name, err = generateValidDNS1123WithSuffix(originalName)
+			if err != nil {
+				return nil, fmt.Errorf("failed to generate random name: %w", err)
+			}
+		}
+
+		c, err := uc.repo.Create(ctx, opts)
+		if err != nil {
+			if errors.Is(err, ErrAlreadyExists) {
+				continue
+			}
+
+			return nil, fmt.Errorf("failed to create contract: %w", err)
+		}
+
+		return c, nil
+	}
+
+	return nil, NewErrValidationStr("that name is already taken")
 }
 
 func (uc *WorkflowContractUseCase) Describe(ctx context.Context, orgID, contractID string, revision int) (*WorkflowContractWithVersion, error) {
@@ -174,6 +233,10 @@ func (uc *WorkflowContractUseCase) Update(ctx context.Context, orgID, contractID
 		return nil, err
 	}
 
+	if err := ValidateIsDNS1123(name); err != nil {
+		return nil, NewErrValidation(err)
+	}
+
 	rawSchema, err := proto.Marshal(schema)
 	if err != nil {
 		return nil, err
@@ -182,11 +245,17 @@ func (uc *WorkflowContractUseCase) Update(ctx context.Context, orgID, contractID
 	opts := &ContractUpdateOpts{OrgID: orgUUID, ContractID: contractUUID, ContractBody: rawSchema, Name: name}
 
 	c, err := uc.repo.Update(ctx, opts)
-	if c == nil {
+	if err != nil {
+		if errors.Is(err, ErrAlreadyExists) {
+			return nil, NewErrValidationStr("that name is already taken")
+		}
+
+		return nil, fmt.Errorf("failed to update contract: %w", err)
+	} else if c == nil {
 		return nil, NewErrNotFound("contract")
 	}
 
-	return c, err
+	return c, nil
 }
 
 // Delete soft-deletes the entry