From 617af9bea974f9f17d92995c60dc3a97ade0f98d Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Wed, 4 Oct 2023 17:01:18 +0200 Subject: [PATCH] Revert "feat(env): support multiline in env-file" This reverts commit 170a78631b4b0a0e5963e860cc3c3b297b4a7d09. This was a breaking change and users are hitting it, see https://github.com/containers/podman/issues/19565 Fixes #19565 Signed-off-by: Paul Holzinger --- pkg/env/env.go | 134 ++++++----------------- pkg/env/env_test.go | 242 +---------------------------------------- pkg/env/env_unix.go | 2 +- pkg/env/env_windows.go | 2 +- 4 files changed, 34 insertions(+), 346 deletions(-) diff --git a/pkg/env/env.go b/pkg/env/env.go index 80b13ed684..8e87834f85 100644 --- a/pkg/env/env.go +++ b/pkg/env/env.go @@ -1,29 +1,13 @@ +// Package for processing environment variables. package env +// TODO: we need to add tests for this package. + import ( + "bufio" "fmt" - "io" "os" "strings" - - "github.com/containers/storage/pkg/regexp" -) - -var ( - // Form: https://github.com/motdotla/dotenv/blob/aa03dcad1002027390dac1e8d96ac236274de354/lib/main.js#L9C76-L9C76 - // (?:export\s+)?([\w.-]+) match key - // ([\w.%-]+)(\s*[=|*]\s*?|:\s+?) match separator - // Remaining match value - // e.g. KEY=VALUE => KEY, =, VALUE - // - // KEY= => KEY, =, "" - // KEY* => KEY, *, "" - // KEY*=1 => KEY, *, =1 - lineRegexp = regexp.Delayed( - `(?m)(?:^|^)\s*(?:export\s+)?([\w.%-]+)(\s*[=|*]\s*?|:\s+?)(\s*'(?:\\'|[^'])*'|\s*"(?:\\"|[^"])*"|\s*` + - "`(?:\\`|[^`])*`" + `|[^#\r\n]+)?\s*(?:#.*)?(?:$|$)`, - ) - onlyKeyRegexp = regexp.Delayed(`^[\w.-]+$`) ) const whiteSpaces = " \t" @@ -95,103 +79,47 @@ func ParseFile(path string) (_ map[string]string, err error) { } defer fh.Close() - content, err := io.ReadAll(fh) - if err != nil { - return nil, err + scanner := bufio.NewScanner(fh) + for scanner.Scan() { + // trim the line from all leading whitespace first + line := strings.TrimLeft(scanner.Text(), whiteSpaces) + // line is not empty, and not starting with '#' + if len(line) > 0 && !strings.HasPrefix(line, "#") { + if err := parseEnv(env, line); err != nil { + return nil, err + } + } } - - // replace all \r\n and \r with \n - text := strings.NewReplacer("\r\n", "\n", "\r", "\n").Replace(string(content)) - if err := parseEnv(env, text, false); err != nil { - return nil, err - } - - return env, nil + return env, scanner.Err() } -// parseEnv parse the given content into env format -// @param enforceMatch bool "it throws an error if there is no match" -// -// @example: parseEnv(env, "#comment", true) => error("invalid variable: #comment") -// @example: parseEnv(env, "#comment", false) => nil -// @example: parseEnv(env, "", false) => nil -// @example: parseEnv(env, "KEY=FOO", true) => nil -// @example: parseEnv(env, "KEY", true) => nil -func parseEnv(env map[string]string, content string, enforceMatch bool) error { - m := envMatch(content) +func parseEnv(env map[string]string, line string) error { + data := strings.SplitN(line, "=", 2) - if len(m) == 0 && enforceMatch { - return fmt.Errorf("invalid variable: %q", content) + // catch invalid variables such as "=" or "=A" + if data[0] == "" { + return fmt.Errorf("invalid variable: %q", line) } - - for _, match := range m { - key := match[1] - separator := strings.Trim(match[2], whiteSpaces) - value := match[3] - - if strings.Contains(value, "\n") { - if strings.HasPrefix(value, "`") { - return fmt.Errorf("only support multi-line environment variables surrounded by "+ - "double quotation marks or single quotation marks. invalid variable: %q", match[0]) - } - - // In the case of multi-line values, we need to remove the surrounding " ' - value = strings.Trim(value, "\"'") - } - - // KEY*=1 => KEY, *, =1 => KEY*, =, 1 - if separator == "*" && strings.HasPrefix(value, "=") { - key += "*" - separator = "=" - value = strings.TrimPrefix(value, "=") - } - - switch separator { - case "=": - // KEY= - if value == "" { - if val, ok := os.LookupEnv(key); ok { - env[key] = val - } - } else { - env[key] = value - } - case "*": + // trim the front of a variable, but nothing else + name := strings.TrimLeft(data[0], whiteSpaces) + if len(data) > 1 { + env[name] = data[1] + } else { + if strings.HasSuffix(name, "*") { + name = strings.TrimSuffix(name, "*") for _, e := range os.Environ() { part := strings.SplitN(e, "=", 2) if len(part) < 2 { continue } - if strings.HasPrefix(part[0], key) { + if strings.HasPrefix(part[0], name) { env[part[0]] = part[1] } } + } else if val, ok := os.LookupEnv(name); ok { + // if only a pass-through variable is given, clean it up. + env[name] = val } } return nil } - -func envMatch(content string) [][]string { - m := lineRegexp.FindAllStringSubmatch(content, -1) - - // KEY => KEY, =, "" - // Due to the above regex pattern, it will skip cases where only KEY is present (e.g., foo). - // However, in our requirement, this situation is equivalent to foo=(i.e., "foo" == "foo="). - // Therefore, we need to perform additional processing. - // The reason for needing to support this scenario is that we need to consider: `podman run -e CI -e USERNAME`. - { - noMatched := lineRegexp.ReplaceAllString(content, "") - nl := strings.Split(noMatched, "\n") - for _, key := range nl { - key := strings.Trim(key, whiteSpaces) - if key == "" { - continue - } - if onlyKeyRegexp.MatchString(key) { - m = append(m, []string{key, key, "=", ""}) - } - } - } - - return m -} diff --git a/pkg/env/env_test.go b/pkg/env/env_test.go index 1dc2b10eef..c77061ecf2 100644 --- a/pkg/env/env_test.go +++ b/pkg/env/env_test.go @@ -1,8 +1,6 @@ package env import ( - "fmt" - "os" "testing" "github.com/stretchr/testify/assert" @@ -101,228 +99,6 @@ func TestJoin(t *testing.T) { } } -func createTmpFile(content string) (string, error) { - tmpfile, err := os.CreateTemp(os.TempDir(), "podman-test-parse-env-") - if err != nil { - return "", err - } - - if _, err := tmpfile.WriteString(content); err != nil { - return "", err - } - if err := tmpfile.Close(); err != nil { - return "", err - } - return tmpfile.Name(), nil -} - -func Test_ParseFile(t *testing.T) { - tests := []struct { - name string - key string - separator string // = or * - value string - - // environment variable - envKey string - envValue string - - expectedKey string - expectedValue string - success bool - }{ - { - name: "Good", - key: "Key", - separator: "=", - value: "Value1", - success: true, - }, - { - name: "HasDoubleQuotesWithSingleLine", - key: "Key2", - separator: "=", - value: `"Value2"`, - success: true, - }, - { - name: "HasSingleQuotesWithSingleLine", - key: "Key3", - separator: "=", - value: `'Value3'`, - success: true, - }, - { - name: "KeepValueSpace", - key: "Key4", - separator: "=", - value: " Value4 ", - success: true, - }, - { - name: "RemoveKeySpace", - key: " Key5 ", - separator: "=", - expectedKey: "Key5", - value: "Value5", - success: true, - }, - { - name: "NoValue", - key: "Key6", - separator: "=", - value: "", - envValue: "Value6", - expectedValue: "Value6", - success: true, - }, - { - name: "FromEnv", - key: "Key7", - separator: "=", - value: "", - envValue: "Value7", - expectedValue: "Value7", - success: true, - }, - { - name: "OnlyKey", - key: "Key8", - separator: "", - value: "", - envValue: "Value8", - expectedValue: "Value8", - success: true, - }, - { - name: "GlobKey", - key: "Key9", - separator: "*", - value: "", - envKey: "Key9999", - envValue: "Value9", - expectedKey: "Key9999", - expectedValue: "Value9", - success: true, - }, - { - name: "InvalidGlobKey", - key: "Key10*", - separator: "=", - value: "1", - envKey: "Key1010", - envValue: "Value10", - expectedKey: "Key10*", - expectedValue: "1", - success: true, - }, - { - name: "MultilineWithDoubleQuotes", - key: "Key11", - separator: "=", - value: "\"First line1\nlast line1\"", - expectedValue: "First line1\nlast line1", - success: true, - }, - { - name: "MultilineWithSingleQuotes", - key: "Key12", - separator: "=", - value: "'First line2\nlast line2'", - expectedValue: "First line2\nlast line2", - success: true, - }, - { - name: "Has%", - key: "BASH_FUNC__fmt_ctx%%", - separator: "=", - value: "() { echo 1; }", - success: true, - }, - { - name: "Export syntax", - key: "export Key13", - separator: "=", - value: "Value13", - expectedKey: "Key13", - expectedValue: "Value13", - success: true, - }, - { - name: "NoValueAndNoEnv", - key: "Key14", - separator: "=", - value: "", - success: false, - }, - { - name: "OnlyValue", - key: "", - separator: "=", - value: "Value", - success: false, - }, - { - name: "OnlyDelim", - key: "", - separator: "=", - value: "", - success: false, - }, - { - name: "Comment", - key: "#aaaa", - separator: "", - value: "", - success: false, - }, - } - - content := "" - for _, tt := range tests { - content += fmt.Sprintf("%s%s%s\n", tt.key, tt.separator, tt.value) - - if tt.envValue != "" { - key := tt.key - if tt.envKey != "" { - key = tt.envKey - } - t.Setenv(key, tt.envValue) - } - } - tFile, err := createTmpFile(content) - defer os.Remove(tFile) - assert.NoError(t, err) - - env, err := ParseFile(tFile) - assert.NoError(t, err) - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - key := tt.key - if tt.expectedKey != "" { - key = tt.expectedKey - } - val, ok := env[key] - if ok && !tt.success { - t.Errorf("not should set key:%s ", tt.key) - return - } else if !ok && tt.success { - t.Errorf("should set key:%s ", tt.key) - return - } - - if tt.success { - value := tt.value - if tt.expectedValue != "" { - value = tt.expectedValue - } - assert.Equal(t, value, val, "value should be equal") - } - }) - } -} - func Test_parseEnv(t *testing.T) { good := make(map[string]string) @@ -359,14 +135,6 @@ func Test_parseEnv(t *testing.T) { }, wantErr: true, }, - { - name: "GoodOnlyKey", - args: args{ - env: good, - line: "apple", - }, - wantErr: false, - }, { name: "BadNoKey", args: args{ @@ -383,18 +151,10 @@ func Test_parseEnv(t *testing.T) { }, wantErr: true, }, - { - name: "MultilineWithBackticksQuotes", - args: args{ - env: good, - line: "apple=`foo\nbar`", - }, - wantErr: true, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if err := parseEnv(tt.args.env, tt.args.line, true); (err != nil) != tt.wantErr { + if err := parseEnv(tt.args.env, tt.args.line); (err != nil) != tt.wantErr { t.Errorf("parseEnv() error = %v, wantErr %v", err, tt.wantErr) } }) diff --git a/pkg/env/env_unix.go b/pkg/env/env_unix.go index 7fa3d59c91..690078f33c 100644 --- a/pkg/env/env_unix.go +++ b/pkg/env/env_unix.go @@ -8,7 +8,7 @@ package env func ParseSlice(s []string) (map[string]string, error) { env := make(map[string]string, len(s)) for _, e := range s { - if err := parseEnv(env, e, true); err != nil { + if err := parseEnv(env, e); err != nil { return nil, err } } diff --git a/pkg/env/env_windows.go b/pkg/env/env_windows.go index b9e4f4c589..1496dbfebf 100644 --- a/pkg/env/env_windows.go +++ b/pkg/env/env_windows.go @@ -17,7 +17,7 @@ func ParseSlice(s []string) (map[string]string, error) { continue } - if err := parseEnv(env, e, true); err != nil { + if err := parseEnv(env, e); err != nil { return nil, err } }