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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace eval with xargs for parsing BUILDKITE_COMMAND #165

Merged
merged 2 commits into from Aug 29, 2018
Merged

Replace eval with xargs for parsing BUILDKITE_COMMAND #165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
efa40aa
Replace eval with xargs for parsing BUILDKITE_COMMAND
lox Aug 17, 2018
5cd983a
Handle empty BUILDKITE_COMMAND
lox Aug 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 16 additions & 13 deletions commands/run.sh
View file
Open in desktop
Expand Up @@ -95,12 +95,12 @@ while IFS=$'\n' read -r vol ; do
[[ -n "${vol:-}" ]] && run_params+=("-v" "$(expand_relative_volume_path "$vol")")
done <<< "$(plugin_read_list VOLUMES)"

# Parse BUILDKITE_DOCKER_DEFAULT_VOLUMES delimited by semi-colons, normalized to
# ignore spaces and leading or trailing semi-colons
IFS=';' read -r -a default_volumes <<< "${BUILDKITE_DOCKER_DEFAULT_VOLUMES:-}"
for vol in "${default_volumes[@]:-}"
do
# Trim all whitespace when checking for variable definition, handling cases
# with repeated delimiters.
[[ ! -z "${vol// }" ]] && run_params+=("-v" "$(expand_relative_volume_path "$vol")")
for vol in "${default_volumes[@]:-}" ; do
trimmed_vol="$(echo -n "$vol" | sed -e 's/^[[:space:]]*//' | sed -e 's/[[:space:]]*$//')"
[[ -n "$trimmed_vol" ]] && run_params+=("-v" "$(expand_relative_volume_path "$trimmed_vol")")
done

# Optionally disable allocating a TTY
Expand Down Expand Up @@ -135,18 +135,21 @@ fi
set +e

(
# Reset bash to the default IFS with no glob expanding and no failing on error
# Reset bash to the default IFS
unset IFS
set -f

# The eval statements below are used to allow $BUILDKITE_COMMAND to be interpolated correctly
# When paired with -f we ensure that it word splits correctly, e.g bash -c "pwd" should split
# into [bash, -c, "pwd"]. Eval ends up the simplest way to do this, and when paired with the
# set -f above we ensure globs aren't expanded (imagine a command like `cat *`, which bash would
# helpfully expand prior to passing it to docker-compose)
# Because we receive $BUILDKITE_COMMAND as a single string, it needs to be tokenized as a shell
# would to respect things like quoted strings. Previously we used eval to do this with some
# careful control around preventing glob expansion, but this ends up being difficult to get just
# right and also very risky. The new implementation uses xargs and printf to tokenize the string,
# which is portable and much harder to shoot ourselves in the foot 馃帀

while IFS= read -rd '' token; do
[[ -n "$token" ]] && run_params+=("$token")
done < <(xargs printf '%s\0' <<< "$BUILDKITE_COMMAND")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don鈥檛 fully grok how this works. What鈥檚 the /0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first interesting bit is that xargs tokenizes quoted strings, which was news to me:

# echo "blah 'a long string'" | xargs -n1 echo token
token blah
token a long string

Then the next bit of magic is getting those tokens into bash so that we can loop over them. Because quoted strings can contain any normal delimiters like newlines or semi-colons or whatever, we use null bytes \0 to delimit our tokens so that we can loop over them. But because bash is c-based, it's slightly allergic to null bytes, because they tend to mean the end of strings, so to produce the null bytes we use printf to output the tokens with a \0 at the end.

Then to read them, we set IFS (the field separator that is normally used for word-splitting) to empty and then use the -d '' param in the read built-in, which will split on those null bytes!

So the end result is to tokenize the $BUILDKITE_COMMAND into arguments, respecting single and double quotes and then iterate over them and put them in an array as strings. 馃帀

Copy link
Contributor

Choose a reason for hiding this comment

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

This is an excellent story that belongs in the code!

Because quoted strings can contain any normal delimiters like newlines or semi-colons or whatever, we use null bytes \0 to delimit our tokens so that we can loop over them.

Is this because it's impossible for people the contents of the args themselves to contain null bytes, so we can use them to split?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this because it's impossible for people the contents of the args themselves to contain null bytes, so we can use them to split?

Yup correct, because if there was a \0 in there it would cause the string to terminate.


echo "+++ :docker: Running command in Docker Compose service: $run_service" >&2
eval "run_docker_compose \${run_params[@]} $BUILDKITE_COMMAND"
run_docker_compose "${run_params[@]}"
)

exitcode=$?
Expand Down
3 changes: 2 additions & 1 deletion tests/run.bats
View file
Open in desktop
Expand Up @@ -121,10 +121,11 @@ load '../lib/run'
export BUILDKITE_PLUGIN_DOCKER_COMPOSE_ENV_1=MYENV
export BUILDKITE_PLUGIN_DOCKER_COMPOSE_ENVIRONMENT_0=MYENV=2
export BUILDKITE_PLUGIN_DOCKER_COMPOSE_ENVIRONMENT_1=MYENV
export BUILDKITE_PLUGIN_DOCKER_COMPOSE_ENVIRONMENT_2=ANOTHER="this is a long string with spaces; and semi-colons"

stub docker-compose \
"-f docker-compose.yml -p buildkite1111 build --pull myservice : echo built myservice" \
"-f docker-compose.yml -p buildkite1111 run --name buildkite1111_myservice_build_1 -e MYENV=0 -e MYENV -e MYENV=2 -e MYENV myservice pwd : echo ran myservice"
"-f docker-compose.yml -p buildkite1111 run --name buildkite1111_myservice_build_1 -e MYENV=0 -e MYENV -e MYENV=2 -e MYENV -e ANOTHER=this\ is\ a\ long\ string\ with\ spaces\;\ and\ semi-colons myservice pwd : echo ran myservice"

stub buildkite-agent \
"meta-data get docker-compose-plugin-built-image-tag-myservice : exit 1"
Expand Down