From d326c17dfbaaaefe182ef47a8495f42a620b595c Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Thu, 1 Aug 2019 19:32:04 -0400 Subject: [PATCH 01/13] chore: standardize on writing to file versus a destination --- plugins/app-json/internal-functions | 2 +- plugins/nginx-vhosts/functions | 4 ++-- plugins/ps/functions | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/plugins/app-json/internal-functions b/plugins/app-json/internal-functions index 939194235..9652cec02 100755 --- a/plugins/app-json/internal-functions +++ b/plugins/app-json/internal-functions @@ -12,7 +12,7 @@ get_phase_script() { local APP_JSON_FILE="$GET_PHASE_SCRIPT_TMP_WORK_DIR/app.json" trap "rm -rf '$GET_PHASE_SCRIPT_TMP_WORK_DIR' >/dev/null" RETURN INT TERM - copy_from_image "$IMAGE" "app.json" "$GET_PHASE_SCRIPT_TMP_WORK_DIR" 2>/dev/null || true + copy_from_image "$IMAGE" "app.json" "$APP_JSON_FILE" 2>/dev/null || true if [[ ! -f "$APP_JSON_FILE" ]]; then return 0 diff --git a/plugins/nginx-vhosts/functions b/plugins/nginx-vhosts/functions index 6de04743f..5254394da 100755 --- a/plugins/nginx-vhosts/functions +++ b/plugins/nginx-vhosts/functions @@ -185,12 +185,12 @@ get_custom_nginx_template() { declare desc="attempts to copy custom nginx template from app image" local APP="$1" verify_app_name "$APP" - local DESTINATION="$2" + local DESTINATION_FILE="$2" local IMAGE_TAG="$(get_running_image_tag "$APP")" local IMAGE=$(get_deploying_app_image_name "$APP" "$IMAGE_TAG") local NGINX_TEMPLATE_NAME="nginx.conf.sigil" - copy_from_image "$IMAGE" "$NGINX_TEMPLATE_NAME" "$DESTINATION" 2>/dev/null || true + copy_from_image "$IMAGE" "$NGINX_TEMPLATE_NAME" "$DESTINATION_FILE" 2>/dev/null || true } is_spdy_enabled() { diff --git a/plugins/ps/functions b/plugins/ps/functions index fe0cb0ca4..5cee618b7 100755 --- a/plugins/ps/functions +++ b/plugins/ps/functions @@ -62,7 +62,7 @@ generate_scale_file() { local DOKKU_PROCFILE="$DOKKU_ROOT/$APP/DOKKU_PROCFILE" verify_app_name "$APP" - if copy_from_image "$IMAGE" "DOKKU_SCALE" "$DOKKU_ROOT/$APP" >/dev/null 2>&1; then + if copy_from_image "$IMAGE" "DOKKU_SCALE" "$DOKKU_SCALE_FILE" >/dev/null 2>&1; then cp "$DOKKU_SCALE_FILE" "${DOKKU_SCALE_FILE}.extracted" else rm -f "${DOKKU_SCALE_FILE}.extracted" From 6c8f716c111a085f258eb0ccd22e1c4e202963dd Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Thu, 1 Aug 2019 19:33:52 -0400 Subject: [PATCH 02/13] refactor: handle case copied file is not directly readable If "docker cp" is run within docker, it seems the owner is root, resulting in other processes not being able to directly read the file without resorting to `cat` trickery. Doing this in a single place avoids the problem completely. --- plugins/common/functions | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/plugins/common/functions b/plugins/common/functions index c53c68c04..30f0af240 100755 --- a/plugins/common/functions +++ b/plugins/common/functions @@ -371,7 +371,7 @@ parse_args() { copy_from_image() { declare desc="copy file from named image to destination" - declare IMAGE="$1" SRC_FILE="$2" DST_DIR="$3" + declare IMAGE="$1" SRC_FILE="$2" DST_FILE="$3" local WORK_DIR="" verify_app_name "$APP" @@ -389,11 +389,19 @@ copy_from_image() { SRC_FILE="${WORKDIR}/${SRC_FILE}" fi fi + + TMP_FILE_COMMAND_OUTPUT=$(mktemp "/tmp/dokku-${FUNCNAME[0]}.XXXX") + trap "rm -rf '$TMP_FILE_COMMAND_OUTPUT' >/dev/null" RETURN + local CID=$("$DOCKER_BIN" create "${DOCKER_CREATE_LABEL_ARGS[@]}" $DOKKU_GLOBAL_RUN_ARGS "$IMAGE") - if ! "$DOCKER_BIN" cp "$CID:$SRC_FILE" "$DST_DIR"; then + if ! "$DOCKER_BIN" cp "$CID:$SRC_FILE" "$TMP_FILE_COMMAND_OUTPUT"; then "$DOCKER_BIN" rm -f "$CID" &>/dev/null return 1 fi + + # workaround for CHECKS file when owner is root. seems to only happen when running inside docker + cat "$TMP_FILE_COMMAND_OUTPUT" >"$DST_FILE" + "$DOCKER_BIN" rm -f "$CID" &>/dev/null else return 1 From f771b272e3b1b4c80994112022381ff1df96e2fc Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Thu, 1 Aug 2019 19:34:43 -0400 Subject: [PATCH 03/13] fix: handle case where file may have windows newlines Closes #3611 --- plugins/common/functions | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/common/functions b/plugins/common/functions index 30f0af240..41c0c3d01 100755 --- a/plugins/common/functions +++ b/plugins/common/functions @@ -400,7 +400,7 @@ copy_from_image() { fi # workaround for CHECKS file when owner is root. seems to only happen when running inside docker - cat "$TMP_FILE_COMMAND_OUTPUT" >"$DST_FILE" + cat "$TMP_FILE_COMMAND_OUTPUT" | dos2unix -l >"$DST_FILE" "$DOCKER_BIN" rm -f "$CID" &>/dev/null else From dc9d597cd8d2e23c9cf576dc03c1f5705681b701 Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Thu, 1 Aug 2019 19:35:24 -0400 Subject: [PATCH 04/13] feat: handle case where file may not have trailing newline and parsing depends on it --- plugins/common/functions | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/plugins/common/functions b/plugins/common/functions index 41c0c3d01..61a7bbb89 100755 --- a/plugins/common/functions +++ b/plugins/common/functions @@ -402,6 +402,11 @@ copy_from_image() { # workaround for CHECKS file when owner is root. seems to only happen when running inside docker cat "$TMP_FILE_COMMAND_OUTPUT" | dos2unix -l >"$DST_FILE" + # add trailing newline for certain places where file parsing depends on it + if [[ "$(tail -c1 "$DST_FILE")" != "" ]]; then + echo "" >>"$DST_FILE" + fi + "$DOCKER_BIN" rm -f "$CID" &>/dev/null else return 1 From 8dbf855d569fcbf9ed1378a381c6e3e8638e3690 Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Thu, 1 Aug 2019 19:40:30 -0400 Subject: [PATCH 05/13] feat: handle case where docker cp is run as non root and tries to chown the file This _may_ be cargo-cult, but its an easy way to handle any potential issues, so we'll include it regardless. --- plugins/common/functions | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/plugins/common/functions b/plugins/common/functions index 61a7bbb89..55561de88 100755 --- a/plugins/common/functions +++ b/plugins/common/functions @@ -394,8 +394,13 @@ copy_from_image() { trap "rm -rf '$TMP_FILE_COMMAND_OUTPUT' >/dev/null" RETURN local CID=$("$DOCKER_BIN" create "${DOCKER_CREATE_LABEL_ARGS[@]}" $DOKKU_GLOBAL_RUN_ARGS "$IMAGE") - if ! "$DOCKER_BIN" cp "$CID:$SRC_FILE" "$TMP_FILE_COMMAND_OUTPUT"; then - "$DOCKER_BIN" rm -f "$CID" &>/dev/null + "$DOCKER_BIN" cp "$CID:$SRC_FILE" "$TMP_FILE_COMMAND_OUTPUT" 2>/dev/null || true + "$DOCKER_BIN" rm -f "$CID" &>/dev/null + + # docker cp exits with status 1 when run as non-root user when it tries to chown the file + # after successfully copying the file. Thus, we suppress stderr. + # ref: https://github.com/dotcloud/docker/issues/3986 + if [[ ! -s "$TMP_FILE_COMMAND_OUTPUT" ]]; then return 1 fi From 7dbeffd04162d1f72b9c1780c7dc6a62a7aeb58d Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Thu, 1 Aug 2019 19:44:07 -0400 Subject: [PATCH 06/13] chore: drop unnecessary stdout redirect to /dev/null --- plugins/ps/functions | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/ps/functions b/plugins/ps/functions index 5cee618b7..00f1f6a9e 100755 --- a/plugins/ps/functions +++ b/plugins/ps/functions @@ -62,7 +62,7 @@ generate_scale_file() { local DOKKU_PROCFILE="$DOKKU_ROOT/$APP/DOKKU_PROCFILE" verify_app_name "$APP" - if copy_from_image "$IMAGE" "DOKKU_SCALE" "$DOKKU_SCALE_FILE" >/dev/null 2>&1; then + if copy_from_image "$IMAGE" "DOKKU_SCALE" "$DOKKU_SCALE_FILE" 2>/dev/null; then cp "$DOKKU_SCALE_FILE" "${DOKKU_SCALE_FILE}.extracted" else rm -f "${DOKKU_SCALE_FILE}.extracted" From 1a322eeeb8ac266d1bbb76499082d8abdf419429 Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Thu, 1 Aug 2019 19:45:40 -0400 Subject: [PATCH 07/13] refactor: use copy_from_image helper for copying CHECKS files Closes #3611 --- plugins/scheduler-docker-local/check-deploy | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/plugins/scheduler-docker-local/check-deploy b/plugins/scheduler-docker-local/check-deploy index f3d8a96f8..f5b657bd0 100755 --- a/plugins/scheduler-docker-local/check-deploy +++ b/plugins/scheduler-docker-local/check-deploy @@ -80,14 +80,11 @@ scheduler-docker-local-check-deploy() { # use this number of retries for checks local ATTEMPTS="${DOKKU_CHECKS_ATTEMPTS:-5}" - # try to copy CHECKS from container if not in APP dir & quit gracefully if it doesn't exist - # docker cp exits with status 1 when run as non-root user when it tries to chown the file - # after successfully copying the file. Thus, we suppress stderr. - # ref: https://github.com/dotcloud/docker/issues/3986 local CHECK_DEPLOY_TMP_WORK_DIR=$(mktemp -d "/tmp/dokku-${FUNCNAME[0]}.XXXX") - "$DOCKER_BIN" cp "$DOKKU_APP_CONTAINER_ID:/app/CHECKS" "$CHECK_DEPLOY_TMP_WORK_DIR" 2>/dev/null || true - local CHECKS_FILENAME=${CHECK_DEPLOY_TMP_WORK_DIR}/CHECKS + local IMAGE_TAG="$(get_running_image_tag "$APP")" + local IMAGE=$(get_deploying_app_image_name "$APP" "$IMAGE_TAG") + copy_from_image "$IMAGE" "CHECKS" "$CHECKS_FILENAME" 2>/dev/null checks_check_deploy_cleanup() { declare desc="cleans up CHECK_DEPLOY_TMP_WORK_DIR and print container output" From 78a5e4d33160d388a77757757ada5183902bcc09 Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Thu, 1 Aug 2019 20:16:14 -0400 Subject: [PATCH 08/13] docs: document copying files from images --- docs/development/plugin-creation.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/docs/development/plugin-creation.md b/docs/development/plugin-creation.md index b853cc29e..9450397a2 100644 --- a/docs/development/plugin-creation.md +++ b/docs/development/plugin-creation.md @@ -187,6 +187,23 @@ local DOKKU_COMMIT_ARGS=("--change" "LABEL org.label-schema.schema-version=1.0" "$DOCKER_BIN" run "--label=com.dokku.app-name=${APP}" $DOKKU_GLOBAL_RUN_ARGS ... ``` +## Copy files from the built image using `copy_from_image` + +Avoid copying files from running containers as these files may change over time. Instead copy files from the image built during the deploy process. This can be done via the `copy_from_image` helper function. This will correctly handle various corner cases in copying files from an image. + +```shell +source "$PLUGIN_CORE_AVAILABLE_PATH/common/functions" + +local TMP_FILE=$(mktemp "/tmp/dokku-${FUNCNAME[0]}.XXXX") +trap "rm -rf '$TMP_FILE' >/dev/null" RETURN INT TERM + +local IMAGE_TAG="$(get_running_image_tag "$APP")" +local IMAGE=$(get_deploying_app_image_name "$APP" "$IMAGE_TAG") +copy_from_image "$IMAGE" "file-being-copied" "$TMP_FILE" 2>/dev/null +``` + +Files are copied from the `/app` directory - for images built via buildpacks - or `WORKDIR` - for images built via Dockerfile. + ## Avoid calling the `dokku` binary directly > New as of 0.6.0 From 697afaa2cc935e3ff5e4c10044793e4343740687 Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Mon, 5 Aug 2019 12:09:53 -0400 Subject: [PATCH 09/13] fix: add missing dos2unix dependency --- Makefile | 5 ++++- debian/control | 2 +- rpm.mk | 1 + tests/ci/setup.sh | 2 +- 4 files changed, 7 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index 61718d955..967e9ef90 100644 --- a/Makefile +++ b/Makefile @@ -119,12 +119,15 @@ plugin-dependencies: plugn procfile-util plugins: plugn procfile-util docker sudo -E dokku plugin:install --core -dependencies: apt-update sshcommand plugn procfile-util docker help2man man-db sigil +dependencies: apt-update sshcommand plugn procfile-util docker help2man man-db sigil dos2unix $(MAKE) -e stack apt-update: apt-get update -qq +dos2unix: + apt-get install -qq -y dos2unix + help2man: apt-get install -qq -y help2man diff --git a/debian/control b/debian/control index 0dde0157d..d7022ca9f 100644 --- a/debian/control +++ b/debian/control @@ -3,7 +3,7 @@ Version: 0.17.9 Section: web Priority: optional Architecture: amd64 -Depends: locales, git, curl, man-db, netcat, sshcommand (>= 0.6.0), gliderlabs-sigil, docker-engine-cs (>= 1.7.1) | docker-engine (>= 1.7.1) | docker-io (>= 1.7.1) | docker.io (>= 1.7.1) | docker-ce | docker-ee, net-tools, software-properties-common, procfile-util, python-software-properties | python3-software-properties, rsyslog +Depends: locales, git, curl, man-db, netcat, sshcommand (>= 0.6.0), gliderlabs-sigil, docker-engine-cs (>= 1.7.1) | docker-engine (>= 1.7.1) | docker-io (>= 1.7.1) | docker.io (>= 1.7.1) | docker-ce | docker-ee, net-tools, software-properties-common, procfile-util, python-software-properties | python3-software-properties, rsyslog, dos2unix Recommends: herokuish (>= 0.3.4), parallel, dokku-update Pre-Depends: nginx (>= 1.8.0) | openresty, dnsutils, cgroupfs-mount | cgroup-lite, plugn (>= 0.3.0), sudo, python2.7, debconf Maintainer: Jose Diaz-Gonzalez diff --git a/rpm.mk b/rpm.mk index 02dca57ea..6e9f740aa 100644 --- a/rpm.mk +++ b/rpm.mk @@ -63,6 +63,7 @@ endif --depends '/usr/bin/docker' \ --depends 'bind-utils' \ --depends 'curl' \ + --depends 'dos2unix' \ --depends 'git' \ --depends 'gliderlabs-sigil' \ --depends 'man-db' \ diff --git a/tests/ci/setup.sh b/tests/ci/setup.sh index 8f4c88a33..0ebe43714 100755 --- a/tests/ci/setup.sh +++ b/tests/ci/setup.sh @@ -29,7 +29,7 @@ install_dependencies() { sudo add-apt-repository -y ppa:nginx/stable sudo apt-get update - sudo apt-get -qq -y install cgroupfs-mount nginx + sudo apt-get -qq -y install cgroupfs-mount dos2unix nginx sudo cp "${ROOT_DIR}/tests/dhparam.pem" /etc/nginx/dhparam.pem sudo dpkg -i "${ROOT_DIR}/build/$HEROKUISH_PACKAGE_NAME" \ From 86febeb7ca1f7e8892f88d4d7a78fe4f17799f56 Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Mon, 5 Aug 2019 13:35:03 -0400 Subject: [PATCH 10/13] fix: do not try to remove the container twice --- plugins/common/functions | 2 -- 1 file changed, 2 deletions(-) diff --git a/plugins/common/functions b/plugins/common/functions index 55561de88..7fb20c575 100755 --- a/plugins/common/functions +++ b/plugins/common/functions @@ -411,8 +411,6 @@ copy_from_image() { if [[ "$(tail -c1 "$DST_FILE")" != "" ]]; then echo "" >>"$DST_FILE" fi - - "$DOCKER_BIN" rm -f "$CID" &>/dev/null else return 1 fi From eef9f5eba53c344312dc69563ad1f104b9334fe5 Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Mon, 5 Aug 2019 13:35:33 -0400 Subject: [PATCH 11/13] fix: ignore errors in copy_from_image within the check-deploy trigger The trigger will later on check if it needs to run checks on its own by inspecting the file. --- plugins/scheduler-docker-local/check-deploy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/scheduler-docker-local/check-deploy b/plugins/scheduler-docker-local/check-deploy index f5b657bd0..e2840e578 100755 --- a/plugins/scheduler-docker-local/check-deploy +++ b/plugins/scheduler-docker-local/check-deploy @@ -84,7 +84,7 @@ scheduler-docker-local-check-deploy() { local CHECKS_FILENAME=${CHECK_DEPLOY_TMP_WORK_DIR}/CHECKS local IMAGE_TAG="$(get_running_image_tag "$APP")" local IMAGE=$(get_deploying_app_image_name "$APP" "$IMAGE_TAG") - copy_from_image "$IMAGE" "CHECKS" "$CHECKS_FILENAME" 2>/dev/null + copy_from_image "$IMAGE" "CHECKS" "$CHECKS_FILENAME" 2>/dev/null || true checks_check_deploy_cleanup() { declare desc="cleans up CHECK_DEPLOY_TMP_WORK_DIR and print container output" From 5affe4269985110d70daff60835834525b74ec2e Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Mon, 5 Aug 2019 15:24:15 -0400 Subject: [PATCH 12/13] fix: remove unnecessary cat --- plugins/common/functions | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/common/functions b/plugins/common/functions index 7fb20c575..35a134231 100755 --- a/plugins/common/functions +++ b/plugins/common/functions @@ -405,7 +405,7 @@ copy_from_image() { fi # workaround for CHECKS file when owner is root. seems to only happen when running inside docker - cat "$TMP_FILE_COMMAND_OUTPUT" | dos2unix -l >"$DST_FILE" + dos2unix -l <"$TMP_FILE_COMMAND_OUTPUT" >"$DST_FILE" # add trailing newline for certain places where file parsing depends on it if [[ "$(tail -c1 "$DST_FILE")" != "" ]]; then From 6fcb53113a4d0e995d62a22dc6060f04d5505a3e Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Mon, 5 Aug 2019 15:57:54 -0400 Subject: [PATCH 13/13] fix: return true when removing temporary files extracted from docker If you are running docker in docker, the permissions on the generated file are such that they may not be accessible by the normal dokku user. For dokku, it is good enough to ignore the failed removal. --- plugins/common/functions | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/common/functions b/plugins/common/functions index 35a134231..c6cf31225 100755 --- a/plugins/common/functions +++ b/plugins/common/functions @@ -391,7 +391,7 @@ copy_from_image() { fi TMP_FILE_COMMAND_OUTPUT=$(mktemp "/tmp/dokku-${FUNCNAME[0]}.XXXX") - trap "rm -rf '$TMP_FILE_COMMAND_OUTPUT' >/dev/null" RETURN + trap "rm -rf '$TMP_FILE_COMMAND_OUTPUT' >/dev/null || true" RETURN local CID=$("$DOCKER_BIN" create "${DOCKER_CREATE_LABEL_ARGS[@]}" $DOKKU_GLOBAL_RUN_ARGS "$IMAGE") "$DOCKER_BIN" cp "$CID:$SRC_FILE" "$TMP_FILE_COMMAND_OUTPUT" 2>/dev/null || true