fix!: allow values files from any directory on Zarf package create

Signed-off-by: Wayne Starr <[email protected]>

Author: Racer159

examples/values-templating/zarf.yaml

@@ -4,7 +4,7 @@ metadata:
   description: Example nginx package to demonstrate Zarf Values templating
 values:
   files:
-    - values.yaml
+    - values/values.yaml
 
 # Until Consts and Vars are fully deprecated, they'll be available in go-templates
 constants:

src/internal/value/value.go

@@ -85,7 +85,7 @@ func ParseFiles(ctx context.Context, paths []string, _ ParseFilesOptions) (_ Val
 			if err != nil {
 				return nil, err
 			}
-			vals, err = parseLocalFile(ctx, path)
+			vals, err = ParseLocalFile(ctx, path)
 			if err != nil {
 				return nil, err
 			}
@@ -96,7 +96,8 @@ func ParseFiles(ctx context.Context, paths []string, _ ParseFilesOptions) (_ Val
 	return m, nil
 }
 
-func parseLocalFile(ctx context.Context, path string) (Values, error) {
+// ParseLocalFile reads and parses a single local YAML file into a Values map.
+func ParseLocalFile(ctx context.Context, path string) (Values, error) {
 	m := make(Values)
 
 	// Handle files
@@ -114,7 +115,8 @@ func parseLocalFile(ctx context.Context, path string) (Values, error) {
 	// Decode and merge values
 	if err = yaml.NewDecoder(f).DecodeContext(ctx, &m); err != nil {
 		if errors.Is(err, io.EOF) {
-			return m, nil // Empty file is ok
+			// Empty file - ensure we return initialized map not nil
+			return make(Values), nil
 		}
 		return nil, &YAMLDecodeError{
 			FilePath: path,

src/internal/value/value_test.go

@@ -216,6 +216,111 @@ func TestParseFiles_Errors(t *testing.T) {
 	}
 }
 
+func TestParseLocalFile(t *testing.T) {
+	tests := []struct {
+		name           string
+		file           string
+		expectedResult Values
+		expectError    bool
+	}{
+		{
+			name: "single valid YAML file",
+			file: "testdata/valid/simple.yaml",
+			expectedResult: Values{
+				"my-component": map[string]any{
+					"key1": "value1",
+					"key2": "value2",
+				},
+			},
+			expectError: false,
+		},
+		{
+			name: "complex nested YAML",
+			file: "testdata/valid/complex.yaml",
+			expectedResult: Values{
+				"deployment": map[string]any{
+					"replicas": uint64(3),
+					"image": map[string]any{
+						"repository": "nginx",
+						"tag":        "1.21",
+					},
+					"resources": map[string]any{
+						"limits": map[string]any{
+							"cpu":    "100m",
+							"memory": "128Mi",
+						},
+						"requests": map[string]any{
+							"cpu":    "50m",
+							"memory": "64Mi",
+						},
+					},
+				},
+				"service": map[string]any{
+					"type":       "ClusterIP",
+					"port":       uint64(80),
+					"targetPort": uint64(8080),
+				},
+				"ingress": map[string]any{
+					"enabled": true,
+					"annotations": map[string]any{
+						"kubernetes.io/ingress.class": "nginx",
+					},
+					"hosts": []any{
+						map[string]any{
+							"host": "example.com",
+							"paths": []any{
+								map[string]any{
+									"path":     "/",
+									"pathType": "Prefix",
+								},
+							},
+						},
+					},
+				},
+			},
+			expectError: false,
+		},
+		{
+			name:           "empty YAML file",
+			file:           "testdata/valid/empty.yaml",
+			expectedResult: Values{},
+			expectError:    false,
+		},
+		{
+			name:        "file does not exist",
+			file:        "testdata/nonexistent.yaml",
+			expectError: true,
+		},
+		{
+			name:        "invalid YAML syntax",
+			file:        "testdata/invalid/malformed.yaml",
+			expectError: true,
+		},
+		{
+			name:        "malformed YAML with tabs",
+			file:        "testdata/invalid/tabs.yaml",
+			expectError: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			ctx := testutil.TestContext(t)
+
+			result, err := ParseLocalFile(ctx, tt.file)
+
+			if tt.expectError {
+				require.Error(t, err)
+				require.Nil(t, result)
+			} else {
+				require.NoError(t, err)
+				require.Equal(t, tt.expectedResult, result)
+			}
+		})
+	}
+}
+
 func TestExtract(t *testing.T) {
 	tests := []struct {
 		name   string

src/pkg/packager/deploy.go

@@ -142,15 +142,15 @@ func Deploy(ctx context.Context, pkgLayout *layout.PackageLayout, opts DeployOpt
 			" Run again with --features=\"%s=true\"", feature.Values, feature.Values)
 	}
 
-	// Read the default package values off of the pkgLayout.Pkg.Values.Files.
-	// Resolve values file paths relative to the package directory
-	valueFilePaths := make([]string, len(pkgLayout.Pkg.Values.Files))
-	for i, vf := range pkgLayout.Pkg.Values.Files {
-		valueFilePaths[i] = filepath.Join(pkgLayout.DirPath(), layout.ValuesDir, vf)
-	}
-	vals, err := value.ParseFiles(ctx, valueFilePaths, value.ParseFilesOptions{})
-	if err != nil {
-		return DeployResult{}, err
+	// Read the package values from values.yaml if it exists
+	vals := make(value.Values)
+	valuesPath := filepath.Join(pkgLayout.DirPath(), layout.ValuesYAML)
+	if _, err := os.Stat(valuesPath); err == nil {
+		parsedVals, err := value.ParseLocalFile(ctx, valuesPath)
+		if err != nil {
+			return DeployResult{}, err
+		}
+		vals = parsedVals
 	}
 	// Package defaults are overridden by deploy values.
 	vals.DeepMerge(opts.Values)

src/pkg/packager/layout/assemble.go

@@ -29,6 +29,7 @@ import (
 	"github.com/zarf-dev/zarf/src/internal/packager/helm"
 	"github.com/zarf-dev/zarf/src/internal/packager/images"
 	"github.com/zarf-dev/zarf/src/internal/packager/kustomize"
+	"github.com/zarf-dev/zarf/src/internal/value"
 	"github.com/zarf-dev/zarf/src/pkg/archive"
 	"github.com/zarf-dev/zarf/src/pkg/logger"
 	"github.com/zarf-dev/zarf/src/pkg/packager/actions"
@@ -156,11 +157,9 @@ func AssemblePackage(ctx context.Context, pkg v1alpha1.ZarfPackage, packagePath
 		}
 	}
 
-	l.Debug("copying values files to package", "files", pkg.Values.Files)
-	for _, file := range pkg.Values.Files {
-		if err = copyValuesFile(ctx, file, packagePath, buildPath); err != nil {
-			return nil, err
-		}
+	l.Debug("merging values files to package", "files", pkg.Values.Files)
+	if err = mergeAndWriteValuesFile(ctx, pkg.Values.Files, packagePath, buildPath); err != nil {
+		return nil, err
 	}
 
 	checksumContent, checksumSha, err := getChecksum(buildPath)
@@ -876,35 +875,34 @@ func createReproducibleTarballFromDir(dirPath, dirPrefix, tarballPath string, ov
 	})
 }
 
-func copyValuesFile(ctx context.Context, file, packagePath, buildPath string) error {
+func mergeAndWriteValuesFile(ctx context.Context, files []string, packagePath, buildPath string) error {
 	l := logger.From(ctx)
 
-	// Process local values file
-	src := file
-	if !filepath.IsAbs(src) {
-		src = filepath.Join(packagePath, ValuesDir, file)
-	}
-	// Validate src
-	if _, err := os.Stat(src); err != nil {
-		return fmt.Errorf("unable to access values file %s: %w", src, err)
-	}
-
-	// Ensure relative paths don't munge the destination and write outside of the package tmpdir
-	cleanFile := filepath.Clean(file)
-	if strings.HasPrefix(cleanFile, "..") {
-		return fmt.Errorf("values file path %s escapes package directory", file)
+	// Build absolute paths for all values files
+	valueFilePaths := make([]string, len(files))
+	for i, file := range files {
+		src := file
+		if !filepath.IsAbs(src) {
+			src = filepath.Join(packagePath, file)
+		}
+		// Validate src exists
+		if _, err := os.Stat(src); err != nil {
+			return fmt.Errorf("unable to access values file %s: %w", src, err)
+		}
+		valueFilePaths[i] = src
 	}
 
-	//Copy file to pre-archive package - destination includes ValuesDir
-	dst := filepath.Join(buildPath, ValuesDir, cleanFile)
-	l.Debug("copying values file", "src", src, "dst", dst)
-	if err := helpers.CreatePathAndCopy(src, dst); err != nil {
-		return fmt.Errorf("failed to copy values file %s: %w", src, err)
+	// Parse and merge all values files
+	vals, err := value.ParseFiles(ctx, valueFilePaths, value.ParseFilesOptions{})
+	if err != nil {
+		return fmt.Errorf("failed to parse values files: %w", err)
 	}
 
-	// Set appropriate file permissions
-	if err := os.Chmod(dst, helpers.ReadWriteUser); err != nil {
-		return fmt.Errorf("failed to set permissions on values file %s: %w", dst, err)
+	// Write merged values to YAML
+	dst := filepath.Join(buildPath, ValuesYAML)
+	l.Debug("writing merged values file", "dst", dst, "fileCount", len(files))
+	if err := utils.WriteYaml(dst, vals, helpers.ReadWriteUser); err != nil {
+		return fmt.Errorf("failed to write merged values file: %w", err)
 	}
 
 	return nil

src/pkg/packager/layout/layout.go

@@ -10,13 +10,13 @@ import (
 
 // Constants used in the default package layout.
 const (
-	ZarfYAML  = "zarf.yaml"
-	Signature = "zarf.yaml.sig"
-	Checksums = "checksums.txt"
+	ZarfYAML   = "zarf.yaml"
+	Signature  = "zarf.yaml.sig"
+	Checksums  = "checksums.txt"
+	ValuesYAML = "values.yaml"
 
 	ImagesDir     = "images"
 	ComponentsDir = "components"
-	ValuesDir     = "values"
 
 	SBOMDir = "zarf-sbom"
 	SBOMTar = "sboms.tar"

src/pkg/zoci/pull_test.go

@@ -89,9 +89,9 @@ func TestAssembleLayers(t *testing.T) {
 			// get all layers
 			layers, err := remote.AssembleLayers(ctx, layoutExpected.Pkg.Components, false, zoci.AllLayers)
 			require.NoError(t, err)
-			require.Len(t, layers, 9)
+			require.Len(t, layers, 10)
 
-			nonDeterministicLayers := []string{"zarf.yaml", "checksums.txt"}
+			nonDeterministicLayers := []string{"zarf.yaml", "checksums.txt", "values.yaml"}
 
 			// get sbom layers - it appears as though the sbom layers are not deterministic
 			sbomInspectLayers, err := remote.AssembleLayers(ctx, layoutExpected.Pkg.Components, false, zoci.SbomLayers)

src/test/packages/42-values/zarf.yaml

@@ -5,7 +5,7 @@ metadata:
 
 values:
   files:
-    - values.yaml
+    - values/values.yaml
 
 components:
   - name: test-values-component