Skip to content

Commit 345abaa

Browse files
fix: avoid false positives in import cycle detection (#3449)
Signed-off-by: Austin Abro <[email protected]>
1 parent bea609f commit 345abaa

File tree

10 files changed

+82
-14
lines changed

10 files changed

+82
-14
lines changed

src/internal/packager2/layout/create.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,7 @@ func LoadPackage(ctx context.Context, packagePath, flavor string, setVariables m
271271
return v1alpha1.ZarfPackage{}, err
272272
}
273273
pkg.Metadata.Architecture = config.GetArch(pkg.Metadata.Architecture)
274-
pkg, err = resolveImports(ctx, pkg, packagePath, pkg.Metadata.Architecture, flavor, map[string]interface{}{})
274+
pkg, err = resolveImports(ctx, pkg, packagePath, pkg.Metadata.Architecture, flavor, []string{})
275275
if err != nil {
276276
return v1alpha1.ZarfPackage{}, err
277277
}

src/internal/packager2/layout/import.go

+10-10
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"context"
88
"errors"
99
"fmt"
10-
"maps"
1110
"os"
1211
"path/filepath"
1312
"slices"
@@ -24,7 +23,9 @@ import (
2423
"github.com/zarf-dev/zarf/src/pkg/zoci"
2524
)
2625

27-
func resolveImports(ctx context.Context, pkg v1alpha1.ZarfPackage, packagePath, arch, flavor string, seenImports map[string]interface{}) (v1alpha1.ZarfPackage, error) {
26+
func resolveImports(ctx context.Context, pkg v1alpha1.ZarfPackage, packagePath, arch, flavor string, importStack []string) (v1alpha1.ZarfPackage, error) {
27+
importStack = append(importStack, packagePath)
28+
2829
variables := pkg.Variables
2930
constants := pkg.Constants
3031
components := []v1alpha1.ZarfComponent{}
@@ -47,12 +48,11 @@ func resolveImports(ctx context.Context, pkg v1alpha1.ZarfPackage, packagePath,
4748
var importedPkg v1alpha1.ZarfPackage
4849
if component.Import.Path != "" {
4950
importPath := filepath.Join(packagePath, component.Import.Path)
50-
importKey := fmt.Sprintf("%s-%s", component.Name, importPath)
51-
if _, ok := seenImports[importKey]; ok {
52-
return v1alpha1.ZarfPackage{}, fmt.Errorf("package %s imported in cycle by %s in component %s", filepath.ToSlash(importPath), filepath.ToSlash(packagePath), component.Name)
51+
for _, sp := range importStack {
52+
if sp == importPath {
53+
return v1alpha1.ZarfPackage{}, fmt.Errorf("package %s imported in cycle by %s in component %s", filepath.ToSlash(importPath), filepath.ToSlash(packagePath), component.Name)
54+
}
5355
}
54-
seenImports = maps.Clone(seenImports)
55-
seenImports[importKey] = nil
5656
b, err := os.ReadFile(filepath.Join(importPath, layout.ZarfYAML))
5757
if err != nil {
5858
return v1alpha1.ZarfPackage{}, err
@@ -61,7 +61,7 @@ func resolveImports(ctx context.Context, pkg v1alpha1.ZarfPackage, packagePath,
6161
if err != nil {
6262
return v1alpha1.ZarfPackage{}, err
6363
}
64-
importedPkg, err = resolveImports(ctx, importedPkg, importPath, arch, flavor, seenImports)
64+
importedPkg, err = resolveImports(ctx, importedPkg, importPath, arch, flavor, importStack)
6565
if err != nil {
6666
return v1alpha1.ZarfPackage{}, err
6767
}
@@ -91,7 +91,7 @@ func resolveImports(ctx context.Context, pkg v1alpha1.ZarfPackage, packagePath,
9191
}
9292
}
9393
if len(found) == 0 {
94-
return v1alpha1.ZarfPackage{}, fmt.Errorf("component %s not found", name)
94+
return v1alpha1.ZarfPackage{}, fmt.Errorf("no compatible component named %s found", name)
9595
} else if len(found) > 1 {
9696
return v1alpha1.ZarfPackage{}, fmt.Errorf("multiple components named %s found", name)
9797
}
@@ -122,7 +122,7 @@ func resolveImports(ctx context.Context, pkg v1alpha1.ZarfPackage, packagePath,
122122
pkg.Constants = slices.CompactFunc(constants, func(l, r v1alpha1.Constant) bool {
123123
return l.Name == r.Name
124124
})
125-
125+
importStack = importStack[0:len(importStack)-1]
126126
return pkg, nil
127127
}
128128

src/internal/packager2/layout/import_test.go

+25-3
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,35 @@ func TestResolveImportsCircular(t *testing.T) {
2323

2424
lint.ZarfSchema = testutil.LoadSchema(t, "../../../../zarf.schema.json")
2525

26-
b, err := os.ReadFile(filepath.Join("./testdata/import/first", ZarfYAML))
26+
b, err := os.ReadFile(filepath.Join("./testdata/import/circular/first", ZarfYAML))
2727
require.NoError(t, err)
2828
pkg, err := ParseZarfPackage(b)
2929
require.NoError(t, err)
3030

31-
_, err = resolveImports(ctx, pkg, "./testdata/import/first", "", "", map[string]interface{}{})
32-
require.EqualError(t, err, "package testdata/import/second imported in cycle by testdata/import/third in component component")
31+
_, err = resolveImports(ctx, pkg, "./testdata/import/circular/first", "", "", []string{})
32+
require.EqualError(t, err, "package testdata/import/circular/second imported in cycle by testdata/import/circular/third in component component")
33+
}
34+
35+
func TestResolveImportsBranches(t *testing.T) {
36+
t.Parallel()
37+
ctx := testutil.TestContext(t)
38+
lint.ZarfSchema = testutil.LoadSchema(t, "../../../../zarf.schema.json")
39+
40+
// Get the parent package
41+
b, err := os.ReadFile(filepath.Join("./testdata/import/branch", ZarfYAML))
42+
require.NoError(t, err)
43+
pkg, err := ParseZarfPackage(b)
44+
require.NoError(t, err)
45+
46+
resolvedPkg, err := resolveImports(ctx, pkg, "./testdata/import/branch", "", "", []string{})
47+
require.NoError(t, err)
48+
49+
// ensure imports were resolved correctly
50+
b, err = os.ReadFile(filepath.Join("./testdata/import/branch", "expected.yaml"))
51+
require.NoError(t, err)
52+
expectedPkg, err := ParseZarfPackage(b)
53+
require.NoError(t, err)
54+
require.Equal(t, expectedPkg, resolvedPkg)
3355
}
3456

3557
func TestValidateComponentCompose(t *testing.T) {
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
kind: ZarfPackageConfig
2+
metadata:
3+
name: example-child
4+
5+
components:
6+
- name: common
7+
import:
8+
path: ../common
9+
10+
- name: parent-child-common
11+
import:
12+
path: ../common
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
kind: ZarfPackageConfig
2+
metadata:
3+
name: example-shared
4+
5+
components:
6+
- name: common
7+
description: This gets imported by the parent and child directly
8+
9+
- name: parent-child-common
10+
description: This gets imported in a chain parent->child->common
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
kind: ZarfPackageConfig
2+
metadata:
3+
name: example-package
4+
components:
5+
- name: common
6+
description: This gets imported by the parent and child directly
7+
required: true
8+
- name: parent-child-common
9+
description: This gets imported in a chain parent->child->common
10+
required: true
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
kind: ZarfPackageConfig
2+
metadata:
3+
name: example-package
4+
5+
components:
6+
- name: common
7+
required: true
8+
import:
9+
path: common
10+
11+
- name: parent-child-common
12+
required: true
13+
import:
14+
path: child

0 commit comments

Comments
 (0)