move parsing key-value files to a separate package

Move the code for parsing key-value files, such as used for
env-files and label-files to a separate package. This allows
other projects (such as compose) to use the same parsing
logic, but provide custom lookup functions for their situation
(which is slightly different).

The new package provides utilities for parsing key-value files
for either a file or an io.Reader. Most tests for EnvFile were
now testing functionality that's already tested in the new package,
so were (re)moved.

Co-authored-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 9ecfe4f5a7)
Signed-off-by: Austin Vazquez <macedonv@amazon.com>

cli/command/container/opts_test.go

@@ -921,7 +921,7 @@ func TestParseEnvfileVariablesWithBOMUnicode(t *testing.T) {
 	}
 
 	// UTF16 with BOM
-	e := "contains invalid utf8 bytes at line"
+	e := "invalid utf8 bytes at line"
 	if _, _, _, err := parseRun([]string{"--env-file=testdata/utf16.env", "img", "cmd"}); err == nil || !strings.Contains(err.Error(), e) {
 		t.Fatalf("Expected an error with message '%s', got %v", e, err)
 	}

opts/envfile.go

@@ -2,6 +2,8 @@ package opts
 
 import (
 	"os"
+
+	"github.com/docker/cli/pkg/kvfile"
 )
 
 // ParseEnvFile reads a file with environment variables enumerated by lines
@@ -18,5 +20,5 @@ import (
 // environment variables, that's why we just strip leading whitespace and
 // nothing more.
 func ParseEnvFile(filename string) ([]string, error) {
-	return parseKeyValueFile(filename, os.LookupEnv)
+	return kvfile.Parse(filename, os.LookupEnv)
 }

opts/envfile_test.go

@@ -1,10 +1,8 @@
 package opts
 
 import (
-	"bufio"
 	"os"
 	"path/filepath"
-	"strings"
 	"testing"
 
 	"gotest.tools/v3/assert"
@@ -19,86 +17,12 @@ func tmpFileWithContent(t *testing.T, content string) string {
 	return fileName
 }
 
-// Test ParseEnvFile for a file with a few well formatted lines
-func TestParseEnvFileGoodFile(t *testing.T) {
-	content := `foo=bar
-    baz=quux
-# comment
-
-_foobar=foobaz
-with.dots=working
-and_underscore=working too
-`
-	// Adding a newline + a line with pure whitespace.
-	// This is being done like this instead of the block above
-	// because it's common for editors to trim trailing whitespace
-	// from lines, which becomes annoying since that's the
-	// exact thing we need to test.
-	content += "\n    \t  "
-	tmpFile := tmpFileWithContent(t, content)
-
-	lines, err := ParseEnvFile(tmpFile)
-	assert.NilError(t, err)
-
-	expectedLines := []string{
-		"foo=bar",
-		"baz=quux",
-		"_foobar=foobaz",
-		"with.dots=working",
-		"and_underscore=working too",
-	}
-
-	assert.Check(t, is.DeepEqual(lines, expectedLines))
-}
-
-// Test ParseEnvFile for an empty file
-func TestParseEnvFileEmptyFile(t *testing.T) {
-	tmpFile := tmpFileWithContent(t, "")
-
-	lines, err := ParseEnvFile(tmpFile)
-	assert.NilError(t, err)
-	assert.Check(t, is.Len(lines, 0))
-}
-
 // Test ParseEnvFile for a non existent file
 func TestParseEnvFileNonExistentFile(t *testing.T) {
 	_, err := ParseEnvFile("no_such_file")
 	assert.Check(t, is.ErrorType(err, os.IsNotExist))
 }
 
-// Test ParseEnvFile for a badly formatted file
-func TestParseEnvFileBadlyFormattedFile(t *testing.T) {
-	content := `foo=bar
-    f   =quux
-`
-	tmpFile := tmpFileWithContent(t, content)
-
-	_, err := ParseEnvFile(tmpFile)
-	const expectedMessage = "variable 'f   ' contains whitespaces"
-	assert.Check(t, is.ErrorContains(err, expectedMessage))
-}
-
-// Test ParseEnvFile for a file with a line exceeding bufio.MaxScanTokenSize
-func TestParseEnvFileLineTooLongFile(t *testing.T) {
-	content := "foo=" + strings.Repeat("a", bufio.MaxScanTokenSize+42)
-	tmpFile := tmpFileWithContent(t, content)
-
-	_, err := ParseEnvFile(tmpFile)
-	const expectedMessage = "bufio.Scanner: token too long"
-	assert.Check(t, is.ErrorContains(err, expectedMessage))
-}
-
-// ParseEnvFile with a random file, pass through
-func TestParseEnvFileRandomFile(t *testing.T) {
-	content := `first line
-another invalid line`
-	tmpFile := tmpFileWithContent(t, content)
-
-	_, err := ParseEnvFile(tmpFile)
-	const expectedMessage = "variable 'first line' contains whitespaces"
-	assert.Check(t, is.ErrorContains(err, expectedMessage))
-}
-
 // ParseEnvFile with environment variable import definitions
 func TestParseEnvVariableDefinitionsFile(t *testing.T) {
 	content := `# comment=
@@ -114,15 +38,3 @@ DEFINED_VAR
 	expectedLines := []string{"DEFINED_VAR=defined-value"}
 	assert.Check(t, is.DeepEqual(variables, expectedLines))
 }
-
-// ParseEnvFile with empty variable name
-func TestParseEnvVariableWithNoNameFile(t *testing.T) {
-	content := `# comment=
-=blank variable names are an error case
-`
-	tmpFile := tmpFileWithContent(t, content)
-
-	_, err := ParseEnvFile(tmpFile)
-	const expectedMessage = "no variable name on line '=blank variable names are an error case'"
-	assert.Check(t, is.ErrorContains(err, expectedMessage))
-}

opts/file.go

@@ -1,70 +0,0 @@
-package opts
-
-import (
-	"bufio"
-	"bytes"
-	"fmt"
-	"os"
-	"strings"
-	"unicode"
-	"unicode/utf8"
-)
-
-const whiteSpaces = " \t"
-
-func parseKeyValueFile(filename string, lookupFn func(string) (string, bool)) ([]string, error) {
-	fh, err := os.Open(filename)
-	if err != nil {
-		return []string{}, err
-	}
-	defer fh.Close()
-
-	lines := []string{}
-	scanner := bufio.NewScanner(fh)
-	utf8bom := []byte{0xEF, 0xBB, 0xBF}
-	for currentLine := 1; scanner.Scan(); currentLine++ {
-		scannedBytes := scanner.Bytes()
-		if !utf8.Valid(scannedBytes) {
-			return []string{}, fmt.Errorf("env file %s contains invalid utf8 bytes at line %d: %v", filename, currentLine, scannedBytes)
-		}
-		// We trim UTF8 BOM
-		if currentLine == 1 {
-			scannedBytes = bytes.TrimPrefix(scannedBytes, utf8bom)
-		}
-		// trim the line from all leading whitespace first. trailing whitespace
-		// is part of the value, and is kept unmodified.
-		line := strings.TrimLeftFunc(string(scannedBytes), unicode.IsSpace)
-
-		if len(line) == 0 || line[0] == '#' {
-			// skip empty lines and comments (lines starting with '#')
-			continue
-		}
-
-		key, _, hasValue := strings.Cut(line, "=")
-		if len(key) == 0 {
-			return []string{}, fmt.Errorf("no variable name on line '%s'", line)
-		}
-
-		// leading whitespace was already removed from the line, but
-		// variables are not allowed to contain whitespace or have
-		// trailing whitespace.
-		if strings.ContainsAny(key, whiteSpaces) {
-			return []string{}, fmt.Errorf("variable '%s' contains whitespaces", key)
-		}
-
-		if hasValue {
-			// key/value pair is valid and has a value; add the line as-is.
-			lines = append(lines, line)
-			continue
-		}
-
-		if lookupFn != nil {
-			// No value given; try to look up the value. The value may be
-			// empty but if no value is found, the key is omitted.
-			if value, found := lookupFn(line); found {
-				lines = append(lines, key+"="+value)
-			}
-		}
-	}
-	return lines, scanner.Err()
-}

opts/opts.go

@@ -266,6 +266,8 @@ func validateDomain(val string) (string, error) {
 	return "", fmt.Errorf("%s is not a valid domain", val)
 }
 
+const whiteSpaces = " \t"
+
 // ValidateLabel validates that the specified string is a valid label, and returns it.
 //
 // Labels are in the form of key=value; key must be a non-empty string, and not

opts/parse.go

@@ -6,6 +6,7 @@ import (
 	"strconv"
 	"strings"
 
+	"github.com/docker/cli/pkg/kvfile"
 	"github.com/docker/docker/api/types/container"
 )
 
@@ -25,7 +26,7 @@ func ReadKVEnvStrings(files []string, override []string) ([]string, error) {
 func readKVStrings(files []string, override []string, emptyFn func(string) (string, bool)) ([]string, error) {
 	var variables []string
 	for _, ef := range files {
-		parsedVars, err := parseKeyValueFile(ef, emptyFn)
+		parsedVars, err := kvfile.Parse(ef, emptyFn)
 		if err != nil {
 			return nil, err
 		}

pkg/kvfile/kvfile.go

@@ -0,0 +1,130 @@
+// Package kvfile provides utilities to parse line-delimited key/value files
+// such as used for label-files and env-files.
+//
+// # File format
+//
+// key/value files use the following syntax:
+//
+//   - File must be valid UTF-8.
+//   - BOM headers are removed.
+//   - Leading whitespace is removed for each line.
+//   - Lines starting with "#" are ignored.
+//   - Empty lines are ignored.
+//   - Key/Value pairs are provided as "KEY[=<VALUE>]".
+//   - Maximum line-length is limited to [bufio.MaxScanTokenSize].
+//
+// # Interpolation, substitution, and escaping
+//
+// Both keys and values are used as-is; no interpolation, substitution or
+// escaping is supported, and quotes are considered part of the key or value.
+// Whitespace in values (including leading and trailing) is preserved. Given
+// that the file format is line-delimited, neither key, nor value, can contain
+// newlines.
+//
+// # Key/Value pairs
+//
+// Key/Value pairs take the following format:
+//
+//	KEY[=<VALUE>]
+//
+// KEY is required and may not contain whitespaces or NUL characters. Any
+// other character (except for the "=" delimiter) are accepted, but  it is
+// recommended to use a subset of the POSIX portable character set, as
+// outlined in [Environment Variables].
+//
+// VALUE is optional, but may be empty. If no value is provided (i.e., no
+// equal sign ("=") is present), the KEY is omitted in the result, but some
+// functions accept a lookup-function to provide a default value for the
+// given key.
+//
+// [Environment Variables]: https://pubs.opengroup.org/onlinepubs/7908799/xbd/envvar.html
+package kvfile
+
+import (
+	"bufio"
+	"bytes"
+	"fmt"
+	"io"
+	"os"
+	"strings"
+	"unicode"
+	"unicode/utf8"
+)
+
+// Parse parses a line-delimited key/value pairs separated by equal sign.
+// It accepts a lookupFn to lookup default values for keys that do not define
+// a value. An error is produced if parsing failed, the content contains invalid
+// UTF-8 characters, or a key contains whitespaces.
+func Parse(filename string, lookupFn func(key string) (value string, found bool)) ([]string, error) {
+	fh, err := os.Open(filename)
+	if err != nil {
+		return []string{}, err
+	}
+	out, err := parseKeyValueFile(fh, lookupFn)
+	_ = fh.Close()
+	if err != nil {
+		return []string{}, fmt.Errorf("invalid env file (%s): %v", filename, err)
+	}
+	return out, nil
+}
+
+// ParseFromReader parses a line-delimited key/value pairs separated by equal sign.
+// It accepts a lookupFn to lookup default values for keys that do not define
+// a value. An error is produced if parsing failed, the content contains invalid
+// UTF-8 characters, or a key contains whitespaces.
+func ParseFromReader(r io.Reader, lookupFn func(key string) (value string, found bool)) ([]string, error) {
+	return parseKeyValueFile(r, lookupFn)
+}
+
+const whiteSpaces = " \t"
+
+func parseKeyValueFile(r io.Reader, lookupFn func(string) (string, bool)) ([]string, error) {
+	lines := []string{}
+	scanner := bufio.NewScanner(r)
+	utf8bom := []byte{0xEF, 0xBB, 0xBF}
+	for currentLine := 1; scanner.Scan(); currentLine++ {
+		scannedBytes := scanner.Bytes()
+		if !utf8.Valid(scannedBytes) {
+			return []string{}, fmt.Errorf("invalid utf8 bytes at line %d: %v", currentLine, scannedBytes)
+		}
+		// We trim UTF8 BOM
+		if currentLine == 1 {
+			scannedBytes = bytes.TrimPrefix(scannedBytes, utf8bom)
+		}
+		// trim the line from all leading whitespace first. trailing whitespace
+		// is part of the value, and is kept unmodified.
+		line := strings.TrimLeftFunc(string(scannedBytes), unicode.IsSpace)
+
+		if len(line) == 0 || line[0] == '#' {
+			// skip empty lines and comments (lines starting with '#')
+			continue
+		}
+
+		key, _, hasValue := strings.Cut(line, "=")
+		if len(key) == 0 {
+			return []string{}, fmt.Errorf("no variable name on line '%s'", line)
+		}
+
+		// leading whitespace was already removed from the line, but
+		// variables are not allowed to contain whitespace or have
+		// trailing whitespace.
+		if strings.ContainsAny(key, whiteSpaces) {
+			return []string{}, fmt.Errorf("variable '%s' contains whitespaces", key)
+		}
+
+		if hasValue {
+			// key/value pair is valid and has a value; add the line as-is.
+			lines = append(lines, line)
+			continue
+		}
+
+		if lookupFn != nil {
+			// No value given; try to look up the value. The value may be
+			// empty but if no value is found, the key is omitted.
+			if value, found := lookupFn(line); found {
+				lines = append(lines, key+"="+value)
+			}
+		}
+	}
+	return lines, scanner.Err()
+}

pkg/kvfile/kvfile_test.go

@@ -0,0 +1,145 @@
+package kvfile
+
+import (
+	"bufio"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"gotest.tools/v3/assert"
+	is "gotest.tools/v3/assert/cmp"
+)
+
+// Test Parse for a non existent file.
+func TestParseNonExistentFile(t *testing.T) {
+	_, err := Parse("no_such_file", nil)
+	assert.Check(t, is.ErrorType(err, os.IsNotExist))
+}
+
+// Test Parse from a file with a lookup function.
+func TestParseWithLookup(t *testing.T) {
+	content := `# comment=
+VAR=VAR_VALUE
+EMPTY_VAR=
+UNDEFINED_VAR
+DEFINED_VAR
+`
+	vars := map[string]string{
+		"DEFINED_VAR": "defined-value",
+	}
+	lookupFn := func(name string) (string, bool) {
+		v, ok := vars[name]
+		return v, ok
+	}
+
+	fileName := filepath.Join(t.TempDir(), "envfile")
+	err := os.WriteFile(fileName, []byte(content), 0o644)
+	assert.NilError(t, err)
+
+	variables, err := Parse(fileName, lookupFn)
+	assert.NilError(t, err)
+
+	expectedLines := []string{"VAR=VAR_VALUE", "EMPTY_VAR=", "DEFINED_VAR=defined-value"}
+	assert.Check(t, is.DeepEqual(variables, expectedLines))
+}
+
+// Test ParseEnvFile for a file with a few well formatted lines
+func TestParseFromReaderGoodFile(t *testing.T) {
+	content := `foo=bar
+    baz=quux
+# comment
+
+_foobar=foobaz
+with.dots=working
+and_underscore=working too
+`
+	// Adding a newline + a line with pure whitespace.
+	// This is being done like this instead of the block above
+	// because it's common for editors to trim trailing whitespace
+	// from lines, which becomes annoying since that's the
+	// exact thing we need to test.
+	content += "\n    \t  "
+
+	lines, err := ParseFromReader(strings.NewReader(content), nil)
+	assert.NilError(t, err)
+
+	expectedLines := []string{
+		"foo=bar",
+		"baz=quux",
+		"_foobar=foobaz",
+		"with.dots=working",
+		"and_underscore=working too",
+	}
+
+	assert.Check(t, is.DeepEqual(lines, expectedLines))
+}
+
+// Test ParseFromReader for an empty file
+func TestParseFromReaderEmptyFile(t *testing.T) {
+	lines, err := ParseFromReader(strings.NewReader(""), nil)
+	assert.NilError(t, err)
+	assert.Check(t, is.Len(lines, 0))
+}
+
+// Test ParseFromReader for a badly formatted file
+func TestParseFromReaderBadlyFormattedFile(t *testing.T) {
+	content := `foo=bar
+    f   =quux
+`
+	_, err := ParseFromReader(strings.NewReader(content), nil)
+	const expectedMessage = "variable 'f   ' contains whitespaces"
+	assert.Check(t, is.ErrorContains(err, expectedMessage))
+}
+
+// Test ParseFromReader for a file with a line exceeding bufio.MaxScanTokenSize
+func TestParseFromReaderLineTooLongFile(t *testing.T) {
+	content := "foo=" + strings.Repeat("a", bufio.MaxScanTokenSize+42)
+
+	_, err := ParseFromReader(strings.NewReader(content), nil)
+	const expectedMessage = "bufio.Scanner: token too long"
+	assert.Check(t, is.ErrorContains(err, expectedMessage))
+}
+
+// ParseEnvFile with a random file, pass through
+func TestParseFromReaderRandomFile(t *testing.T) {
+	content := `first line
+another invalid line`
+
+	_, err := ParseFromReader(strings.NewReader(content), nil)
+	const expectedMessage = "variable 'first line' contains whitespaces"
+	assert.Check(t, is.ErrorContains(err, expectedMessage))
+}
+
+// Test ParseFromReader with a lookup function.
+func TestParseFromReaderWithLookup(t *testing.T) {
+	content := `# comment=
+VAR=VAR_VALUE
+EMPTY_VAR=
+UNDEFINED_VAR
+DEFINED_VAR
+`
+	vars := map[string]string{
+		"DEFINED_VAR": "defined-value",
+	}
+	lookupFn := func(name string) (string, bool) {
+		v, ok := vars[name]
+		return v, ok
+	}
+
+	variables, err := ParseFromReader(strings.NewReader(content), lookupFn)
+	assert.NilError(t, err)
+
+	expectedLines := []string{"VAR=VAR_VALUE", "EMPTY_VAR=", "DEFINED_VAR=defined-value"}
+	assert.Check(t, is.DeepEqual(variables, expectedLines))
+}
+
+// Test ParseFromReader with empty variable name
+func TestParseFromReaderWithNoName(t *testing.T) {
+	content := `# comment=
+=blank variable names are an error case
+`
+	_, err := ParseFromReader(strings.NewReader(content), nil)
+	const expectedMessage = "no variable name on line '=blank variable names are an error case'"
+	assert.Check(t, is.ErrorContains(err, expectedMessage))
+}