Skip to content

Commit 4495204

Browse files
lapentadlapentad
authored
Disallow package creation inside an existing package (kptdev#373)
* disallow clone with slash in cli and server Signed-off-by: lapentafd <francesco.lapenta@est.tech> * moved valiadation into engine and squash commits Signed-off-by: lapentafd <francesco.lapenta@est.tech> * revert changes in registry Signed-off-by: lapentafd <francesco.lapenta@est.tech> * restored cli test Signed-off-by: lapentafd <francesco.lapenta@est.tech> * using ErrorContains Signed-off-by: lapentafd <francesco.lapenta@est.tech> * moved fn in util and reduced use of listpackagerev Signed-off-by: lapentafd <francesco.lapenta@est.tech> * change init for edit Signed-off-by: lapentafd <francesco.lapenta@est.tech> * removed logs in test Signed-off-by: lapentafd <francesco.lapenta@est.tech> * extra unit tests in util Signed-off-by: lapentafd <francesco.lapenta@est.tech> --------- Signed-off-by: lapentafd <francesco.lapenta@est.tech>
1 parent 2204b03 commit 4495204

6 files changed

Lines changed: 288 additions & 4 deletions

File tree

‎api/porch/v1alpha1/util.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,3 +62,13 @@ var validFirstTaskTypes = []TaskType{TaskTypeInit, TaskTypeEdit, TaskTypeClone,
6262
func IsValidFirstTaskType(t TaskType) bool {
6363
return slices.Contains(validFirstTaskTypes, t)
6464
}
65+
66+
// IsPackageCreation checks if the package revision is an init or clone operation
67+
func IsPackageCreation(pkgRev *PackageRevision) bool {
68+
for _, task := range pkgRev.Spec.Tasks {
69+
if task.Type == TaskTypeInit || task.Type == TaskTypeClone {
70+
return true
71+
}
72+
}
73+
return false
74+
}

‎pkg/engine/engine.go‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,11 +155,27 @@ func (cad *cadEngine) CreatePackageRevision(ctx context.Context, repositoryObj *
155155
return nil, fmt.Errorf("failed to create packagerevision: %w", err)
156156
}
157157

158-
revs, err := repo.ListPackageRevisions(ctx, repository.ListPackageRevisionFilter{Key: repository.PackageRevisionKey{PkgKey: pkgKey}})
158+
sameRepoFilter := repository.ListPackageRevisionFilter{
159+
Key: repository.PackageRevisionKey{
160+
PkgKey: repository.PackageKey{
161+
RepoKey: repository.RepositoryKey{
162+
Name: newPr.Spec.RepositoryName,
163+
},
164+
},
165+
},
166+
}
167+
sameRepoRevs, err := repo.ListPackageRevisions(ctx, sameRepoFilter)
159168
if err != nil {
160169
return nil, pkgerrors.Wrapf(err, "error listing package revisions")
161170
}
162171

172+
var revs []repository.PackageRevision
173+
for _, rev := range sameRepoRevs {
174+
if rev.Key().PkgKey.Package == newPr.Spec.PackageName {
175+
revs = append(revs, rev)
176+
}
177+
}
178+
163179
if err := ensureUniqueWorkspaceName(newPr, revs); err != nil {
164180
return nil, err
165181
}
@@ -170,6 +186,12 @@ func (cad *cadEngine) CreatePackageRevision(ctx context.Context, repositoryObj *
170186
}
171187
}
172188

189+
if porchapi.IsPackageCreation(newPr) {
190+
if err := repository.ValidatePackagePathOverlap(newPr, sameRepoRevs); err != nil {
191+
return nil, err
192+
}
193+
}
194+
173195
if newPr.Spec.Tasks[0].Type == porchapi.TaskTypeUpgrade {
174196
if err := validateUpgradeTask(ctx, revs, newPr.Spec.Tasks[0].Upgrade); err != nil {
175197
return nil, err

‎pkg/engine/engine_test.go‎

Lines changed: 167 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,7 @@ func TestCreatePackageRevisionRollback(t *testing.T) {
187187
_, err := f.engine.CreatePackageRevision(context.Background(), f.repositoryObj, f.packageRevision, nil)
188188
if tt.expectedError {
189189
assert.Error(t, err)
190-
assert.Contains(t, err.Error(), tt.errorContains)
190+
assert.ErrorContains(t, err, tt.errorContains)
191191
} else {
192192
assert.NoError(t, err)
193193
}
@@ -498,7 +498,7 @@ func TestCreateCloneTaskValidation(t *testing.T) {
498498

499499
if tt.expectedError {
500500
assert.Error(t, err)
501-
assert.Contains(t, err.Error(), tt.errorContains)
501+
assert.ErrorContains(t, err, tt.errorContains)
502502
} else {
503503
assert.NoError(t, err)
504504
}
@@ -508,3 +508,168 @@ func TestCreateCloneTaskValidation(t *testing.T) {
508508
})
509509
}
510510
}
511+
512+
func TestPathsOverlap(t *testing.T) {
513+
tests := []struct {
514+
name string
515+
path1 string
516+
path2 string
517+
overlaps bool
518+
}{
519+
{
520+
name: "identical paths",
521+
path1: "pkg",
522+
path2: "pkg",
523+
overlaps: false,
524+
},
525+
{
526+
name: "path2 is child of path1",
527+
path1: "parent",
528+
path2: "parent/child",
529+
overlaps: true,
530+
},
531+
{
532+
name: "path1 is child of path2",
533+
path1: "parent/child",
534+
path2: "parent",
535+
overlaps: true,
536+
},
537+
{
538+
name: "sibling paths",
539+
path1: "pkg1",
540+
path2: "pkg2",
541+
overlaps: false,
542+
},
543+
{
544+
name: "similar prefix no overlap",
545+
path1: "test",
546+
path2: "test-package",
547+
overlaps: false,
548+
},
549+
}
550+
551+
for _, tt := range tests {
552+
t.Run(tt.name, func(t *testing.T) {
553+
result := repository.PathsOverlap(tt.path1, tt.path2)
554+
assert.Equal(t, tt.overlaps, result)
555+
})
556+
}
557+
}
558+
559+
func TestValidatePackagePathOverlap(t *testing.T) {
560+
tests := []struct {
561+
name string
562+
newPr *porchapi.PackageRevision
563+
existingRevs []repository.PackageRevision
564+
expectError bool
565+
errorContains string
566+
}{
567+
{
568+
name: "no conflict - empty list",
569+
newPr: &porchapi.PackageRevision{
570+
Spec: porchapi.PackageRevisionSpec{
571+
PackageName: "pkg1",
572+
RepositoryName: "repo1",
573+
},
574+
},
575+
existingRevs: []repository.PackageRevision{},
576+
expectError: false,
577+
},
578+
{
579+
name: "no conflict - sibling paths",
580+
newPr: &porchapi.PackageRevision{
581+
Spec: porchapi.PackageRevisionSpec{
582+
PackageName: "pkg1",
583+
RepositoryName: "repo1",
584+
},
585+
},
586+
existingRevs: []repository.PackageRevision{
587+
&fake.FakePackageRevision{
588+
PrKey: repository.PackageRevisionKey{
589+
PkgKey: repository.PackageKey{
590+
RepoKey: repository.RepositoryKey{Name: "repo1"},
591+
Package: "pkg2",
592+
},
593+
},
594+
},
595+
},
596+
expectError: false,
597+
},
598+
{
599+
name: "conflict - nested path",
600+
newPr: &porchapi.PackageRevision{
601+
Spec: porchapi.PackageRevisionSpec{
602+
PackageName: "parent/child",
603+
RepositoryName: "repo1",
604+
},
605+
},
606+
existingRevs: []repository.PackageRevision{
607+
&fake.FakePackageRevision{
608+
PrKey: repository.PackageRevisionKey{
609+
PkgKey: repository.PackageKey{
610+
RepoKey: repository.RepositoryKey{Name: "repo1"},
611+
Package: "parent",
612+
},
613+
},
614+
},
615+
},
616+
expectError: true,
617+
errorContains: "conflicts with existing package",
618+
},
619+
{
620+
name: "error - duplicate package",
621+
newPr: &porchapi.PackageRevision{
622+
Spec: porchapi.PackageRevisionSpec{
623+
PackageName: "pkg1",
624+
RepositoryName: "repo1",
625+
},
626+
},
627+
existingRevs: []repository.PackageRevision{
628+
&fake.FakePackageRevision{
629+
PrKey: repository.PackageRevisionKey{
630+
PkgKey: repository.PackageKey{
631+
RepoKey: repository.RepositoryKey{Name: "repo1"},
632+
Package: "pkg1",
633+
},
634+
},
635+
},
636+
},
637+
expectError: true,
638+
errorContains: "already exists",
639+
},
640+
{
641+
name: "no conflict - different repository",
642+
newPr: &porchapi.PackageRevision{
643+
Spec: porchapi.PackageRevisionSpec{
644+
PackageName: "pkg1",
645+
RepositoryName: "repo1",
646+
},
647+
},
648+
existingRevs: []repository.PackageRevision{
649+
&fake.FakePackageRevision{
650+
PrKey: repository.PackageRevisionKey{
651+
PkgKey: repository.PackageKey{
652+
RepoKey: repository.RepositoryKey{Name: "repo2"},
653+
Package: "pkg1",
654+
},
655+
},
656+
},
657+
},
658+
expectError: false,
659+
},
660+
}
661+
662+
for _, tt := range tests {
663+
t.Run(tt.name, func(t *testing.T) {
664+
err := repository.ValidatePackagePathOverlap(tt.newPr, tt.existingRevs)
665+
if tt.expectError {
666+
assert.Error(t, err)
667+
if tt.errorContains != "" {
668+
assert.ErrorContains(t, err, tt.errorContains)
669+
}
670+
} else {
671+
assert.NoError(t, err)
672+
}
673+
})
674+
}
675+
}

‎pkg/repository/util.go‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,3 +152,39 @@ func KptUpstreamLock2KptUpstream(kptLock kptfile.UpstreamLock) kptfile.Upstream
152152

153153
return kptUpstream
154154
}
155+
156+
// ValidatePackagePathOverlap checks for path conflicts with existing packages
157+
func ValidatePackagePathOverlap(newPr *porchapi.PackageRevision, existingRevs []PackageRevision) error {
158+
existingPaths := make(map[string]bool)
159+
for _, r := range existingRevs {
160+
pkgPath := r.Key().PkgKey.Package
161+
if pkgPath == newPr.Spec.PackageName && r.Key().PkgKey.RepoKey.Name == newPr.Spec.RepositoryName {
162+
return fmt.Errorf("package %q already exists in repository %q", newPr.Spec.PackageName, newPr.Spec.RepositoryName)
163+
}
164+
if r.Key().PkgKey.RepoKey.Name == newPr.Spec.RepositoryName {
165+
existingPaths[pkgPath] = true
166+
}
167+
}
168+
169+
newPath := newPr.Spec.PackageName
170+
for existingPath := range existingPaths {
171+
if PathsOverlap(newPath, existingPath) {
172+
return fmt.Errorf("package path %q conflicts with existing package %q: packages cannot be nested", newPath, existingPath)
173+
}
174+
}
175+
return nil
176+
}
177+
178+
// PathsOverlap checks if two package paths would create a nesting conflict
179+
func PathsOverlap(path1, path2 string) bool {
180+
if path1 == path2 {
181+
return false
182+
}
183+
if strings.HasPrefix(path2+"/", path1+"/") {
184+
return true
185+
}
186+
if strings.HasPrefix(path1+"/", path2+"/") {
187+
return true
188+
}
189+
return false
190+
}

‎pkg/repository/util_test.go‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,3 +204,45 @@ func TestKptUpstreamLock2KptUpstream(t *testing.T) {
204204

205205
assert.Equal(t, "my-repo", KptUpstreamLock2KptUpstream(kptLock).Git.Repo)
206206
}
207+
208+
func TestPathsOverlap(t *testing.T) {
209+
assert.False(t, PathsOverlap("pkg1", "pkg1"))
210+
assert.True(t, PathsOverlap("pkg", "pkg/sub"))
211+
assert.True(t, PathsOverlap("pkg/sub", "pkg"))
212+
assert.False(t, PathsOverlap("pkg1", "pkg2"))
213+
assert.False(t, PathsOverlap("pkg", "pkg-other"))
214+
}
215+
216+
func TestValidatePackagePathOverlap(t *testing.T) {
217+
newPr := &porchapi.PackageRevision{
218+
Spec: porchapi.PackageRevisionSpec{
219+
PackageName: "new-pkg",
220+
RepositoryName: "repo1",
221+
},
222+
}
223+
224+
assert.NoError(t, ValidatePackagePathOverlap(newPr, []PackageRevision{}))
225+
226+
samePackageNameRepoRev := &fakePackageRevision{
227+
repoName: "repo1",
228+
packageName: "new-pkg",
229+
}
230+
err := ValidatePackagePathOverlap(newPr, []PackageRevision{samePackageNameRepoRev})
231+
assert.Error(t, err)
232+
assert.Contains(t, err.Error(), "already exists")
233+
234+
overlappingPathRev := &fakePackageRevision{
235+
repoName: "repo1",
236+
packageName: "new",
237+
}
238+
newPr.Spec.PackageName = "new/sub"
239+
err = ValidatePackagePathOverlap(newPr, []PackageRevision{overlappingPathRev})
240+
assert.Error(t, err)
241+
assert.Contains(t, err.Error(), "conflicts")
242+
243+
differentRepoRev := &fakePackageRevision{
244+
repoName: "repo2",
245+
packageName: "new",
246+
}
247+
assert.NoError(t, ValidatePackagePathOverlap(newPr, []PackageRevision{differentRepoRev}))
248+
}

‎test/e2e/e2e_test.go‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2627,7 +2627,16 @@ func (t *PorchSuite) TestLatestVersionOnDelete() {
26272627
porchapi.LatestPackageRevisionKey: porchapi.LatestPackageRevisionValue,
26282628
})
26292629

2630-
pr2 := t.CreatePackageDraftF(repositoryName, packageName, workspacev2)
2630+
pr2 := t.CreatePackageSkeleton(repositoryName, packageName, workspacev2)
2631+
pr2.Spec.Tasks = []porchapi.Task{{
2632+
Type: porchapi.TaskTypeEdit,
2633+
Edit: &porchapi.PackageEditTaskSpec{
2634+
Source: &porchapi.PackageRevisionRef{
2635+
Name: pr1.Name,
2636+
},
2637+
},
2638+
}}
2639+
t.CreateF(pr2)
26312640

26322641
pr2.Spec.Lifecycle = porchapi.PackageRevisionLifecycleProposed
26332642
t.UpdateF(pr2)

0 commit comments

Comments
 (0)