diff --git a/pkg/helm/helm.go b/pkg/helm/helm.go index 1bfe1e2..b994869 100644 --- a/pkg/helm/helm.go +++ b/pkg/helm/helm.go @@ -262,95 +262,93 @@ type result struct { err error } -type nonRegularFile struct { - fileName string - isDir bool -} - -func resolvesTo(filePath string) (nonRegularFile, error) { - fileData := nonRegularFile{} - info, err := os.Lstat(filePath) - if err != nil { - return fileData, fmt.Errorf("failed to lstat file: %w", err) - } - - if info.IsDir() { - fileData.fileName = filePath - fileData.isDir = true - return fileData, nil - } - - if info.Mode()&fs.ModeSymlink != 0 { - fileName, err := os.Readlink(filePath) - if err != nil { - return fileData, fmt.Errorf("failed to follow symlink: %w", err) - } - fileName = strings.ReplaceAll(filePath, info.Name(), fileName) - fileData.fileName = fileName - fileInfo, err := os.Lstat(fileName) - if err != nil { - return fileData, fmt.Errorf("failed to lstat file: %w", err) - } - if fileInfo.IsDir() { - fileData.isDir = true - return fileData, nil - } - } - return fileData, nil -} - // sumFiles starts goroutines to walk the directory tree at root and digest each // regular file. These goroutines send the results of the digests on the result // channel and send the result of the walk on the error channel. If done is // closed, sumFiles abandons its work. func sumFiles(done <-chan struct{}, root string) (<-chan result, <-chan error) { - // For each regular file, start a goroutine that sums the file and sends - // the result on c. Send the result of the walk on errc. c := make(chan result) errc := make(chan error, 1) - go func() { // HL + go func() { var wg sync.WaitGroup - err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error { - if err != nil { - return fmt.Errorf("error walking the file path %s: %w", root, err) - } - if !info.Mode().IsRegular() { - resolvedInfo, err := resolvesTo(root) - if err != nil { - return err - } - if resolvedInfo.isDir { - // TODO: figure out how to handle dirs and - // symlinked dirs - return nil - } - path = resolvedInfo.fileName - } + + hashFile := func(keyPath, readPath string) { wg.Add(1) - go func() { // HL - data, err := os.ReadFile(path) + go func() { + data, err := os.ReadFile(readPath) select { - case c <- result{path, sha256.Sum256(data), err}: // HL - case <-done: // HL + case c <- result{keyPath, sha256.Sum256(data), err}: + case <-done: } wg.Done() }() - // Abort the walk if done is closed. - select { - case <-done: // HL - return errors.New("walk canceled") - default: + } + + // filepath.Walk does not descend into directory symlinks, so when + // one is encountered its resolved target is enqueued as its own + // walk job. The "logical" path (as seen through the symlink) is + // used for hash keys; the "physical" path (after resolution) is + // used for cycle detection. + type job struct{ logical, physical string } + visited := map[string]struct{}{filepath.Clean(root): {}} + queue := []job{{logical: root, physical: root}} + + var walkErr error + for len(queue) > 0 && walkErr == nil { + j := queue[0] + queue = queue[1:] + + walkErr = filepath.Walk(j.physical, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + select { + case <-done: + return errors.New("walk canceled") + default: + } + + rel, err := filepath.Rel(j.physical, path) + if err != nil { + return err + } + logical := filepath.Join(j.logical, rel) + + // For symlinks, follow once with Stat so symlink-to-file + // is treated like a regular file. Broken symlinks error + // here, propagating out and failing the hash. + target := info + if info.Mode()&fs.ModeSymlink != 0 { + target, err = os.Stat(path) + if err != nil { + return err + } + } + + switch { + case info.Mode()&fs.ModeSymlink != 0 && target.IsDir(): + resolved, err := filepath.EvalSymlinks(path) + if err != nil { + return err + } + if _, seen := visited[resolved]; seen { + log.Printf("sumFiles: skipping symlink cycle at %s -> %s", logical, resolved) + return nil + } + visited[resolved] = struct{}{} + queue = append(queue, job{logical: logical, physical: resolved}) + case target.Mode().IsRegular(): + hashFile(logical, path) + } return nil - } - }) - // Walk has returned, so all calls to wg.Add are done. Start a - // goroutine to close c once all the sends are done. - go func() { // HL + }) + } + + go func() { wg.Wait() - close(c) // HL + close(c) }() - // No select needed here, since errc is buffered. - errc <- err // HL + errc <- walkErr }() return c, errc } diff --git a/pkg/helm/helm_test.go b/pkg/helm/helm_test.go index 7818cac..a8db360 100644 --- a/pkg/helm/helm_test.go +++ b/pkg/helm/helm_test.go @@ -5,10 +5,22 @@ import ( "errors" "log" "os" + "path/filepath" + "sort" "strings" "testing" + "time" ) +func mapKeys(m map[string][32]byte) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + sort.Strings(out) + return out +} + func TestHelm(t *testing.T) { // Set up tests to use current package's testdata as the working directory oldWD, _ := os.Getwd() @@ -250,54 +262,65 @@ kind: Application } }) - t.Run("ResolvesTo", func(t *testing.T) { - scenarios := []struct { - name string - expected string - file string - isDirectory bool - }{ - { - name: "symlinked file resolves to its target", - expected: "crdData_override_testfile.yaml", - file: "crdData_override_testfile_sym_link.yaml", - isDirectory: false, - }, - { - name: "regular directory returns own name", - expected: "nonSymDir", - file: "nonSymDir", - isDirectory: true, - }, - { - name: "symlinked directory returns its target", - expected: "nonSymDir", - file: "SymDir", - isDirectory: true, - }, - /* - { - name: "fail to find", - expected: "nonSymDir", - file: "phantom", - isDirectory: true, - }, - */ + t.Run("SumFilesFollowsDirSymlink", func(t *testing.T) { + symMap, err := sha256Dir("sym_chart") + if err != nil { + t.Fatalf("sha256Dir(sym_chart): %v", err) + } + expandedMap, err := sha256Dir("sym_chart_expanded") + if err != nil { + t.Fatalf("sha256Dir(sym_chart_expanded): %v", err) } - for _, tt := range scenarios { - t.Run(tt.name, func(t *testing.T) { - dataGot, err := resolvesTo(tt.file) + // Compare per-file content sums under matching relative paths. + // generalHashFunction folds the absolute key into its hash, so we + // can't just compare hex hashes - we have to strip the root prefix. + stripPrefix := func(m map[string][32]byte, prefix string) map[string][32]byte { + out := make(map[string][32]byte, len(m)) + for k, v := range m { + rel, err := filepath.Rel(prefix, k) if err != nil { - t.Errorf("failed to resolve file err: %v", err) - } - if dataGot.fileName != tt.expected { - t.Errorf("resolved files do not match. got: %s wanted: %s", dataGot.fileName, tt.expected) - } - if dataGot.isDir != tt.isDirectory { - t.Errorf("failed checking directory status. got: %t wanted: %t", dataGot.isDir, tt.isDirectory) + t.Fatalf("filepath.Rel(%q, %q): %v", prefix, k, err) } - }) + out[rel] = v + } + return out + } + got := stripPrefix(symMap, "sym_chart") + want := stripPrefix(expandedMap, "sym_chart_expanded") + + gotKeys := mapKeys(got) + wantKeys := mapKeys(want) + if len(got) != len(want) { + t.Fatalf("file count mismatch: sym_chart=%v expanded=%v", gotKeys, wantKeys) + } + for k, v := range want { + if got[k] != v { + t.Errorf("hash mismatch at %s: sym_chart=%x expanded=%x", k, got[k], v) + } + } + }) + + t.Run("SumFilesHandlesSymlinkCycle", func(t *testing.T) { + done := make(chan error, 1) + go func() { + _, err := sha256Dir("cycle_chart") + done <- err + }() + select { + case err := <-done: + if err != nil { + t.Errorf("expected nil error for cycle_chart, got: %v", err) + } + case <-time.After(5 * time.Second): + t.Fatal("sha256Dir on cycle_chart did not terminate within 5s - cycle detection broken") + } + }) + + t.Run("SumFilesBrokenSymlinkErrors", func(t *testing.T) { + _, err := sha256Dir("broken_symlink_chart") + if err == nil { + t.Error("expected error for broken symlink, got nil") } }) diff --git a/pkg/helm/testdata/broken_symlink_chart/dangling b/pkg/helm/testdata/broken_symlink_chart/dangling new file mode 120000 index 0000000..6e1fa95 --- /dev/null +++ b/pkg/helm/testdata/broken_symlink_chart/dangling @@ -0,0 +1 @@ +/nonexistent/path \ No newline at end of file diff --git a/pkg/helm/testdata/cycle_chart/loop b/pkg/helm/testdata/cycle_chart/loop new file mode 120000 index 0000000..945c9b4 --- /dev/null +++ b/pkg/helm/testdata/cycle_chart/loop @@ -0,0 +1 @@ +. \ No newline at end of file diff --git a/pkg/helm/testdata/sym_chart/Chart.yaml b/pkg/helm/testdata/sym_chart/Chart.yaml new file mode 100644 index 0000000..e2516fa --- /dev/null +++ b/pkg/helm/testdata/sym_chart/Chart.yaml @@ -0,0 +1,4 @@ +apiVersion: v2 +name: sym-chart +description: Fixture chart for sumFiles directory-symlink test +version: 0.1.0 diff --git a/pkg/helm/testdata/sym_chart/templates/a.yaml b/pkg/helm/testdata/sym_chart/templates/a.yaml new file mode 100644 index 0000000..5eccb96 --- /dev/null +++ b/pkg/helm/testdata/sym_chart/templates/a.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: a +data: + key: a diff --git a/pkg/helm/testdata/sym_chart/vendored b/pkg/helm/testdata/sym_chart/vendored new file mode 120000 index 0000000..2b3f3da --- /dev/null +++ b/pkg/helm/testdata/sym_chart/vendored @@ -0,0 +1 @@ +../sym_chart_vendored \ No newline at end of file diff --git a/pkg/helm/testdata/sym_chart_expanded/Chart.yaml b/pkg/helm/testdata/sym_chart_expanded/Chart.yaml new file mode 100644 index 0000000..e2516fa --- /dev/null +++ b/pkg/helm/testdata/sym_chart_expanded/Chart.yaml @@ -0,0 +1,4 @@ +apiVersion: v2 +name: sym-chart +description: Fixture chart for sumFiles directory-symlink test +version: 0.1.0 diff --git a/pkg/helm/testdata/sym_chart_expanded/templates/a.yaml b/pkg/helm/testdata/sym_chart_expanded/templates/a.yaml new file mode 100644 index 0000000..5eccb96 --- /dev/null +++ b/pkg/helm/testdata/sym_chart_expanded/templates/a.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: a +data: + key: a diff --git a/pkg/helm/testdata/sym_chart_expanded/vendored/b.yaml b/pkg/helm/testdata/sym_chart_expanded/vendored/b.yaml new file mode 100644 index 0000000..3f78e63 --- /dev/null +++ b/pkg/helm/testdata/sym_chart_expanded/vendored/b.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: b +data: + key: b diff --git a/pkg/helm/testdata/sym_chart_vendored/b.yaml b/pkg/helm/testdata/sym_chart_vendored/b.yaml new file mode 100644 index 0000000..3f78e63 --- /dev/null +++ b/pkg/helm/testdata/sym_chart_vendored/b.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: b +data: + key: b