fix(helm): follow directory symlinks when hashing charts

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
 }

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")
 		}
 	})
 

pkg/helm/testdata/broken_symlink_chart/dangling

@@ -0,0 +1 @@
+/nonexistent/path

pkg/helm/testdata/cycle_chart/loop

@@ -0,0 +1 @@
+.

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

pkg/helm/testdata/sym_chart/templates/a.yaml

@@ -0,0 +1,6 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: a
+data:
+  key: a

pkg/helm/testdata/sym_chart/vendored

@@ -0,0 +1 @@
+../sym_chart_vendored

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

pkg/helm/testdata/sym_chart_expanded/templates/a.yaml

@@ -0,0 +1,6 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: a
+data:
+  key: a

pkg/helm/testdata/sym_chart_expanded/vendored/b.yaml

@@ -0,0 +1,6 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: b
+data:
+  key: b

pkg/helm/testdata/sym_chart_vendored/b.yaml

@@ -0,0 +1,6 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: b
+data:
+  key: b