Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

import environment variables that are present #1019

Merged
merged 3 commits into from
Jul 16, 2018
Merged

import environment variables that are present #1019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1e89745
add test for undefined variable environment file import
ktomk Jul 2, 2018
96c026e
import environment variables that are present
ktomk Jul 1, 2018
b91fd12
add test for zero length variable name
ktomk Jul 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion opts/envfile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
37 changes: 37 additions & 0 deletions opts/envfile_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,3 +139,40 @@ 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)")
}
}

// 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")
}
}
14 changes: 10 additions & 4 deletions opts/file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also testing the validation, using this env-file;

FROMENVIRONMENT
# this is a comment
NOSUCHVAR
NOSUCHWITHEQUAL=
# empty line:

# whitespace-only line (spaces):
     
# whitespace-only line (tabs):
				
# whitespace-only line (tabs + spaces):
	 	 	
# starts with = (invalid)
=nothing
EOF

$ docker run --rm --env-file ./myenv busybox env
docker: poorly formatted environment: no variable name on line '=nothing'.
See 'docker run --help'.

Copy link
Contributor Author

@ktomk ktomk Jul 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the test for the fix in question before the fix. signed both commits. I rbased fresh on master, I guess this goes easily into the RC but I don't know of the Docker projects internal workflow regarding versioning, so just FYI. Let me know if you'd like to get TESTING.md cleaned up or about any back ports.

}

if len(data) > 1 {
// pass the value through, no trimming
lines = append(lines, fmt.Sprintf("%s=%s", variable, data[1]))
} else {
var value string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

if emptyFn != nil{
    if value, isSet := emptyFn(line); isSet{
        // if only a pass-through variable is given, clean it up.
        lines = append(lines, fmt.Sprintf("%s=%s", strings.TrimSpace(line), value))
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not suggested within POSIX that a well-formed variable name may contain a space character, however: "Other characters may be permitted by an implementation; applications shall tolerate the presence of such names." - so either refuse or accept but not alter (like trimming spaces). (quote from: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least needs tests against emptyFn() implementation how it deals with such thought-to-be-trim-able variable names as it is used to acquire the value. The behavior must then be same.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we had some validation in the past, but there's no strict rule what's "allowed" as variable name, so we decided to remove validation and leave it to the container's process to decide what's valid and what not.

Having said the above; trimming trailing whitespace in variable names could be an exception (i.e., I don't think it's possible to define a var with leading/trailing whitespace, as the whitespace would be stripped by the shell), but I'm not sure we should; for example, the following would produce an error in your shell as well;

$ export foo = bar
bash: export: `=': not a valid identifier

Leading whitespace in the value leads to "no value being set";

$ export bar=    bar
$ env | grep bar
bar=

This looks to be something where we differ, because the shell requires quotes for that:

$ export baz="     bla"
$ env | grep baz
baz=     bla

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My nit was only a small refactor and should be equivalent with your version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe were you answering to @thaJeztah previous comment 😅

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))
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions opts/parse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down