From 1e8974570434ae0069596ac08aea0e5d6fe69920 Mon Sep 17 00:00:00 2001 From: Tom Klingenberg Date: Mon, 2 Jul 2018 07:33:44 +0200 Subject: [PATCH 1/3] add test for undefined variable environment file import test to show current behavior is wrong at parsing an environment file defining an undefined variable - it must not be defined! NOTE: this test assume the $HOME variable is always set (see POSIX, this normally is the case, e.g. the test suite remains stable). Signed-off-by: Tom Klingenberg --- opts/envfile_test.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/opts/envfile_test.go b/opts/envfile_test.go index 6302263848..dd79f60925 100644 --- a/opts/envfile_test.go +++ b/opts/envfile_test.go @@ -139,3 +139,26 @@ another invalid line` t.Fatalf("Expected [%v], got [%v]", expectedMessage, err.Error()) } } + +// ParseEnvFile with environment variable import definitions +func TestParseEnvVariableDefinitionsFile(t *testing.T) { + content := `# comment= +UNDEFINED_VAR +HOME +` + tmpFile := tmpFileWithContent(content, t) + defer os.Remove(tmpFile) + + variables, err := ParseEnvFile(tmpFile) + if nil != err { + t.Fatal("There must not be any error") + } + + if "HOME="+os.Getenv("HOME") != variables[0] { + t.Fatal("the HOME variable is not properly imported as the first variable (but it is the only one to import)") + } + + if 1 != len(variables) { + t.Fatal("exactly one variable is imported (as the other one is not set at all)") + } +} From 96c026eb301f4f9d2cb224d0ee1a2312b7f5b60c Mon Sep 17 00:00:00 2001 From: Tom Klingenberg Date: Sun, 1 Jul 2018 23:55:38 +0200 Subject: [PATCH 2/3] import environment variables that are present previously docker did import environment variables if they were present but created them if they were not when it was asked via a --env-file cli option to import but not create them. fix is to only import the variable into the environment if it is present. additionally do not import variable names of zero-length (which are lines w/ a potential variable definition w/o a variable name). refs: - https://github.com/docker/for-linux/issues/284 Signed-off-by: Tom Klingenberg --- opts/envfile.go | 2 +- opts/file.go | 14 ++++++++++---- opts/parse.go | 4 ++-- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/opts/envfile.go b/opts/envfile.go index 10054c896c..69d3ca6f60 100644 --- a/opts/envfile.go +++ b/opts/envfile.go @@ -18,5 +18,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.Getenv) + return parseKeyValueFile(filename, os.LookupEnv) } diff --git a/opts/file.go b/opts/file.go index 281905949b..1721a46ef9 100644 --- a/opts/file.go +++ b/opts/file.go @@ -21,7 +21,7 @@ func (e ErrBadKey) Error() string { return fmt.Sprintf("poorly formatted environment: %s", e.msg) } -func parseKeyValueFile(filename string, emptyFn func(string) string) ([]string, error) { +func parseKeyValueFile(filename string, emptyFn func(string) (string, bool)) ([]string, error) { fh, err := os.Open(filename) if err != nil { return []string{}, err @@ -53,17 +53,23 @@ func parseKeyValueFile(filename string, emptyFn func(string) string) ([]string, if strings.ContainsAny(variable, whiteSpaces) { return []string{}, ErrBadKey{fmt.Sprintf("variable '%s' has white spaces", variable)} } + if len(variable) == 0 { + return []string{}, ErrBadKey{fmt.Sprintf("no variable name on line '%s'", line)} + } if len(data) > 1 { // pass the value through, no trimming lines = append(lines, fmt.Sprintf("%s=%s", variable, data[1])) } else { var value string + var present bool if emptyFn != nil { - value = emptyFn(line) + value, present = emptyFn(line) + } + if present { + // if only a pass-through variable is given, clean it up. + lines = append(lines, fmt.Sprintf("%s=%s", strings.TrimSpace(line), value)) } - // if only a pass-through variable is given, clean it up. - lines = append(lines, fmt.Sprintf("%s=%s", strings.TrimSpace(line), value)) } } } diff --git a/opts/parse.go b/opts/parse.go index 679759deda..70b60e142f 100644 --- a/opts/parse.go +++ b/opts/parse.go @@ -19,10 +19,10 @@ func ReadKVStrings(files []string, override []string) ([]string, error) { // present in the file with additional pairs specified in the override parameter. // If a key has no value, it will get the value from the environment. func ReadKVEnvStrings(files []string, override []string) ([]string, error) { - return readKVStrings(files, override, os.Getenv) + return readKVStrings(files, override, os.LookupEnv) } -func readKVStrings(files []string, override []string, emptyFn func(string) string) ([]string, error) { +func readKVStrings(files []string, override []string, emptyFn func(string) (string, bool)) ([]string, error) { variables := []string{} for _, ef := range files { parsedVars, err := parseKeyValueFile(ef, emptyFn) From b91fd129966f4d84948cac5e690424eb14fbf0f1 Mon Sep 17 00:00:00 2001 From: Tom Klingenberg Date: Mon, 2 Jul 2018 07:52:02 +0200 Subject: [PATCH 3/3] add test for zero length variable name parsing an environment file should give an error in case a zero-length variable name (definition w/o a variable name) is encountered. previously these lines went through unnoticed not informing the user about a potential configuration error. Signed-off-by: Tom Klingenberg --- opts/envfile_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/opts/envfile_test.go b/opts/envfile_test.go index dd79f60925..bda9dcb6e7 100644 --- a/opts/envfile_test.go +++ b/opts/envfile_test.go @@ -162,3 +162,17 @@ HOME t.Fatal("exactly one variable is imported (as the other one is not set at all)") } } + +// ParseEnvFile with empty variable name +func TestParseEnvVariableWithNoNameFile(t *testing.T) { + content := `# comment= +=blank variable names are an error case +` + tmpFile := tmpFileWithContent(content, t) + defer os.Remove(tmpFile) + + _, err := ParseEnvFile(tmpFile) + if nil == err { + t.Fatal("if a variable has no name parsing an environment file must fail") + } +}