Merge pull request #9129 from dokku/9126-storage-plugin-does-not-respect-dokku-lib-host-root

Respect DOKKU_LIB_HOST_ROOT for storage bind mounts

docs/advanced-usage/persistent-storage.md

@@ -88,6 +88,8 @@ Each entry mirrors the underlying attachment. `readonly` (boolean) and `volume_o
 
 A storage directory can be created with the `storage:ensure-directory` command. This command will create a subdirectory in the recommended `/var/lib/dokku/data/storage` path - created during Dokku installation - and prepare it for use with an app.
 
+When Dokku itself runs in a container with a shared Docker socket, default named storage is still created under `DOKKU_LIB_ROOT` (typically `/var/lib/dokku`) inside the Dokku container. Docker `-v` mounts for `storage:exec` and app deploys rewrite that path to `DOKKU_LIB_HOST_ROOT` so the host Docker daemon binds the directory from the host-visible data volume. See the [Docker installation notes](/docs/getting-started/install/docker.md) for setting `DOKKU_LIB_HOST_ROOT`.
+
 ```shell
 dokku storage:ensure-directory node-js-app
 ```

docs/getting-started/install/docker.md

@@ -87,8 +87,9 @@ Dokku is run in the following configuration:
 - Data within the container is stored on the host within the `/var/lib/dokku` directory.
 - Image build cache is set to the data dir + `/home/dokku`.
 - The docker socket is mounted into container.
+- `DOKKU_HOST_ROOT` and `DOKKU_LIB_HOST_ROOT` tell Dokku how the host Docker daemon sees those same trees when creating bind mounts (app repos and plugin data such as named storage).
 
-Application repositories, plugin config, as well as plugin data are persisted to disk within the specified host directory for `/var/lib/dokku`.
+Application repositories, plugin config, as well as plugin data are persisted to disk within the specified host directory for `/var/lib/dokku`. `DOKKU_LIB_HOST_ROOT` must point at that host path's `var/lib/dokku` subtree so Docker `-v` mounts resolve under the data volume rather than under the host's own `/var/lib/dokku`.
 
 Other docker container options can also be used when running Dokku, though the specific outcome will depend upon the specified options. For example, the Dokku container's nginx port can be bound to a specific host ip by specifying `--publish $HOST_IP:8080:80`, where `$HOST_IP` is the IP desired. Please see the [docker container run documentation](https://docs.docker.com/engine/reference/commandline/run/) for further explanation for various docker arguments.
 

plugins/scheduler-docker-local/storage_exec.go

@@ -137,7 +137,7 @@ func buildDockerExecArgs(entry *storage.Entry, input StorageExecInput) ([]string
 		args = append(args, "--user", user)
 	}
 
-	args = append(args, "-v", fmt.Sprintf("%s:/data", entry.HostPath))
+	args = append(args, "-v", fmt.Sprintf("%s:/data", storage.HostPathForDocker(entry.HostPath)))
 	args = append(args, input.Image)
 	args = append(args, cmd...)
 	return args, nil

plugins/scheduler-docker-local/storage_exec_test.go

@@ -116,3 +116,25 @@ func TestBuildDockerExecArgsMountsHostPath(t *testing.T) {
 		t.Fatalf("expected -v /srv/demo:/data, got: %s", joined)
 	}
 }
+
+func TestBuildDockerExecArgsRewritesLibHostRoot(t *testing.T) {
+	t.Setenv("DOKKU_LIB_ROOT", "/var/lib/dokku")
+	t.Setenv("DOKKU_LIB_HOST_ROOT", "/host/var/lib/dokku")
+
+	entry := &storage.Entry{
+		Name:      "demo-data",
+		Scheduler: storage.SchedulerDockerLocal,
+		HostPath:  "/var/lib/dokku/data/storage/demo-data",
+	}
+	args, err := buildDockerExecArgs(entry, StorageExecInput{
+		Image:   "alpine:3",
+		Command: []string{"ls"},
+	})
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+	joined := strings.Join(args, " ")
+	if !strings.Contains(joined, "-v /host/var/lib/dokku/data/storage/demo-data:/data") {
+		t.Fatalf("expected rewritten -v host path, got: %s", joined)
+	}
+}

plugins/storage/commands_entries.go

@@ -9,6 +9,7 @@ import (
 	"strings"
 
 	"github.com/dokku/dokku/plugins/common"
+	"github.com/mattn/go-isatty"
 )
 
 // CommandCreateInput captures the flags accepted by storage:create.
@@ -422,14 +423,16 @@ func CommandExec(input CommandExecInput) error {
 
 // stdinModes inspects os.Stdin to decide whether docker / the k8s exec
 // SDK should request an interactive session and a TTY.
+//
+// TTY detection uses isatty rather than ModeCharDevice: /dev/null is a
+// character device but not a terminal, and treating it as a TTY makes
+// docker run -it fail with "cannot attach stdin to a TTY-enabled
+// container because stdin is not a terminal" (e.g. docker exec without
+// -t, as in CI).
 func stdinModes() (interactive bool, tty bool) {
+	tty = isatty.IsTerminal(os.Stdin.Fd()) || isatty.IsCygwinTerminal(os.Stdin.Fd())
 	fi, err := os.Stdin.Stat()
-	if err != nil || fi == nil {
+	hasStdinData := err == nil && fi != nil && fi.Mode()&os.ModeNamedPipe != 0
-		return false, false
-	}
-	mode := fi.Mode()
-	tty = mode&os.ModeCharDevice != 0
-	hasStdinData := mode&os.ModeNamedPipe != 0
 	interactive = tty || hasStdinData
 	return interactive, tty
 }

plugins/storage/commands_entries_test.go

@@ -1,6 +1,7 @@
 package storage
 
 import (
+	"os"
 	"path/filepath"
 	"testing"
 
@@ -218,3 +219,47 @@ func TestCommandSetPersistsAndValidates(t *testing.T) {
 	Expect(err).To(HaveOccurred())
 	Expect(err.Error()).To(ContainSubstring("does not exist"))
 }
+
+// TestStdinModesDevNullIsNotTTY guards the CI / docker-exec path: stdin is
+// often /dev/null, which is a character device but not a terminal. ModeCharDevice
+// alone would request docker run -it and fail attach.
+func TestStdinModesDevNullIsNotTTY(t *testing.T) {
+	devNull, err := os.Open(os.DevNull)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer devNull.Close()
+
+	old := os.Stdin
+	os.Stdin = devNull
+	t.Cleanup(func() { os.Stdin = old })
+
+	interactive, tty := stdinModes()
+	if tty {
+		t.Fatalf("expected tty=false for /dev/null, got tty=true interactive=%v", interactive)
+	}
+	if interactive {
+		t.Fatalf("expected interactive=false for /dev/null, got interactive=true")
+	}
+}
+
+func TestStdinModesNamedPipeIsInteractiveWithoutTTY(t *testing.T) {
+	r, w, err := os.Pipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer r.Close()
+	defer w.Close()
+
+	old := os.Stdin
+	os.Stdin = r
+	t.Cleanup(func() { os.Stdin = old })
+
+	interactive, tty := stdinModes()
+	if tty {
+		t.Fatalf("expected tty=false for a pipe, got tty=true")
+	}
+	if !interactive {
+		t.Fatalf("expected interactive=true for a named pipe")
+	}
+}

plugins/storage/go.mod

@@ -5,6 +5,7 @@ go 1.26.2
 require (
 	github.com/dokku/dokku/plugins/common v0.0.0-00010101000000-000000000000
 	github.com/dokku/dokku/plugins/docker-options v0.0.0-00010101000000-000000000000
+	github.com/mattn/go-isatty v0.0.24
 	github.com/onsi/gomega v1.44.0
 	github.com/spf13/pflag v1.0.10
 )
@@ -17,7 +18,6 @@ require (
 	github.com/hashicorp/go-multierror v1.1.1 // indirect
 	github.com/kr/fs v0.1.0 // indirect
 	github.com/mattn/go-colorable v0.1.14 // indirect
-	github.com/mattn/go-isatty v0.0.24 // indirect
 	github.com/melbahja/goph v1.5.2 // indirect
 	github.com/otiai10/copy v1.14.1 // indirect
 	github.com/otiai10/mint v1.6.3 // indirect

plugins/storage/storage.go

@@ -3,6 +3,7 @@ package storage
 import (
 	"errors"
 	"fmt"
+	"os"
 	"path/filepath"
 	"regexp"
 	"strings"
@@ -190,6 +191,26 @@ func GetStorageDirectory() string {
 	return fmt.Sprintf("%s/data/storage", dokkuLibRoot)
 }
 
+// HostPathForDocker rewrites a path under DOKKU_LIB_ROOT to the
+// DOKKU_LIB_HOST_ROOT view used by the host Docker daemon. Named
+// volumes and paths outside DOKKU_LIB_ROOT are returned unchanged.
+//
+// When Dokku runs in a container with a shared Docker socket, filesystem
+// operations must use DOKKU_LIB_ROOT (visible inside the Dokku container)
+// while Docker -v host paths must use DOKKU_LIB_HOST_ROOT (visible to the
+// daemon on the real host).
+func HostPathForDocker(path string) string {
+	libRoot := common.GetenvWithDefault("DOKKU_LIB_ROOT", "/var/lib/dokku")
+	libHost := os.Getenv("DOKKU_LIB_HOST_ROOT")
+	if libHost == "" || libHost == libRoot {
+		return path
+	}
+	if path == libRoot || strings.HasPrefix(path, libRoot+"/") {
+		return libHost + strings.TrimPrefix(path, libRoot)
+	}
+	return path
+}
+
 // storageDirScriptNames lists the sudo helpers shipped in the plugin's bin
 // directory. Each one takes a storage entry basename and builds the path
 // under $DOKKU_LIB_ROOT/data/storage itself, so no caller can point them

plugins/storage/storage_test.go

@@ -107,3 +107,46 @@ func TestGetStorageDirectory(t *testing.T) {
 	dir := GetStorageDirectory()
 	Expect(dir).To(ContainSubstring("data/storage"))
 }
+
+func TestHostPathForDocker(t *testing.T) {
+	RegisterTestingT(t)
+
+	t.Setenv("DOKKU_LIB_ROOT", "/var/lib/dokku")
+
+	t.Run("unset host root leaves path unchanged", func(t *testing.T) {
+		RegisterTestingT(t)
+		t.Setenv("DOKKU_LIB_HOST_ROOT", "")
+		Expect(HostPathForDocker("/var/lib/dokku/data/storage/foo")).To(Equal("/var/lib/dokku/data/storage/foo"))
+	})
+
+	t.Run("equal host root leaves path unchanged", func(t *testing.T) {
+		RegisterTestingT(t)
+		t.Setenv("DOKKU_LIB_HOST_ROOT", "/var/lib/dokku")
+		Expect(HostPathForDocker("/var/lib/dokku/data/storage/foo")).To(Equal("/var/lib/dokku/data/storage/foo"))
+	})
+
+	t.Run("rewrites paths under lib root", func(t *testing.T) {
+		RegisterTestingT(t)
+		t.Setenv("DOKKU_LIB_HOST_ROOT", "/host/var/lib/dokku")
+		Expect(HostPathForDocker("/var/lib/dokku/data/storage/foo")).To(Equal("/host/var/lib/dokku/data/storage/foo"))
+		Expect(HostPathForDocker("/var/lib/dokku")).To(Equal("/host/var/lib/dokku"))
+	})
+
+	t.Run("leaves named volumes unchanged", func(t *testing.T) {
+		RegisterTestingT(t)
+		t.Setenv("DOKKU_LIB_HOST_ROOT", "/host/var/lib/dokku")
+		Expect(HostPathForDocker("myvolume")).To(Equal("myvolume"))
+	})
+
+	t.Run("leaves custom absolute paths unchanged", func(t *testing.T) {
+		RegisterTestingT(t)
+		t.Setenv("DOKKU_LIB_HOST_ROOT", "/host/var/lib/dokku")
+		Expect(HostPathForDocker("/srv/demo")).To(Equal("/srv/demo"))
+	})
+
+	t.Run("does not false-match similar prefixes", func(t *testing.T) {
+		RegisterTestingT(t)
+		t.Setenv("DOKKU_LIB_HOST_ROOT", "/host/var/lib/dokku")
+		Expect(HostPathForDocker("/var/lib/dokku-extra/data")).To(Equal("/var/lib/dokku-extra/data"))
+	})
+}

plugins/storage/triggers.go

@@ -308,7 +308,7 @@ var appSchedulerFor = common.GetAppScheduler
 // buildDockerVFlag formats the Docker -v argument for a docker-local
 // attachment.
 func buildDockerVFlag(entry *Entry, attachment *Attachment) string {
-	host := entry.HostPath
+	host := HostPathForDocker(entry.HostPath)
 	container := attachment.ContainerPath
 	if host == "" || container == "" {
 		return ""

plugins/storage/triggers_test.go

@@ -61,6 +61,17 @@ func TestBuildDockerVFlagVolumeOptions(t *testing.T) {
 	Expect(roOpts).To(Equal("-v /host:/container:ro,noexec,nosuid"))
 }
 
+func TestBuildDockerVFlagRewritesLibHostRoot(t *testing.T) {
+	RegisterTestingT(t)
+
+	t.Setenv("DOKKU_LIB_ROOT", "/var/lib/dokku")
+	t.Setenv("DOKKU_LIB_HOST_ROOT", "/host/var/lib/dokku")
+
+	entry := &Entry{HostPath: "/var/lib/dokku/data/storage/demo"}
+	flag := buildDockerVFlag(entry, &Attachment{ContainerPath: "/app/data"})
+	Expect(flag).To(Equal("-v /host/var/lib/dokku/data/storage/demo:/app/data"))
+}
+
 func TestTriggerDockerArgsRejectsSchedulerMismatch(t *testing.T) {
 	RegisterTestingT(t)
 	root := withTempLibRoot(t)

tests.mk

@@ -336,7 +336,11 @@ tests-ci-retry-failed:
 	tar xzf /tmp/bats-retry.tgz -C /usr/local/bin
 	bats-retry --execute test-results/bats
 
-test-ci-docker: setup-docker-deploy-tests deploy-test-checks-root deploy-test-config deploy-test-multi deploy-test-go-fail-predeploy deploy-test-go-fail-postdeploy test-ci-binaries
+deploy-test-storage-lib-host-root:
+	@echo deploy-test-storage-lib-host-root
+	cd tests/docker && bats storage.bats
+
+test-ci-docker: setup-docker-deploy-tests deploy-test-checks-root deploy-test-config deploy-test-multi deploy-test-go-fail-predeploy deploy-test-go-fail-postdeploy deploy-test-storage-lib-host-root test-ci-binaries
 
 generate-ssl-tars: generate-ssl-tar generate-ssl-sans-tar generate-ssl-wildcard-tar
 

tests/ci/setup.sh

@@ -123,8 +123,13 @@ build_dokku_docker_image() {
 run_dokku_container() {
   echo "=====> run_dokku_container on CIRCLE_NODE_INDEX: $CIRCLE_NODE_INDEX"
   echo "redirect: https://github.com/dokku/dokku-redirect.git" | sudo tee /var/lib/dokku/plugin-list
+  # DOKKU_HOST_ROOT / DOKKU_LIB_HOST_ROOT match the install docs: the host
+  # mounts /var/lib/dokku at /mnt/dokku, and in-container /var/lib/dokku/{home,data}
+  # are symlinks into that volume.
   docker run -d \
     --env DOKKU_HOSTNAME=dokku.me \
+    --env DOKKU_HOST_ROOT=/var/lib/dokku/home/dokku \
+    --env DOKKU_LIB_HOST_ROOT=/var/lib/dokku/var/lib/dokku \
     --name dokku \
     --publish 3022:22 \
     --publish 80:80 \

tests/docker/storage.bats

@@ -0,0 +1,49 @@
+#!/usr/bin/env bats
+
+# Dokku-in-Docker coverage for DOKKU_LIB_HOST_ROOT. These tests talk to the
+# `dokku` container started by tests/ci/setup.sh docker and read/write the
+# host path that volume backs (/var/lib/dokku -> /mnt/dokku).
+
+setup_file() {
+  if ! docker inspect dokku >/dev/null 2>&1; then
+    echo "dokku container is not running; skip docker storage tests" >&2
+    return 1
+  fi
+
+  DOKKU_LIB_HOST_ROOT="$(docker exec dokku bash -c 'echo -n "$DOKKU_LIB_HOST_ROOT"')"
+  if [[ -z "$DOKKU_LIB_HOST_ROOT" ]]; then
+    echo "DOKKU_LIB_HOST_ROOT is unset inside the dokku container" >&2
+    return 1
+  fi
+  export DOKKU_LIB_HOST_ROOT
+}
+
+setup() {
+  ENTRY_NAME="rdmtest-docker-hostroot-$$"
+  export ENTRY_NAME
+}
+
+teardown() {
+  docker exec dokku dokku storage:destroy "$ENTRY_NAME" --force >/dev/null 2>&1 || true
+  sudo rm -rf "${DOKKU_LIB_HOST_ROOT}/data/storage/${ENTRY_NAME}"
+}
+
+@test "(docker) storage:exec sees a file written under DOKKU_LIB_HOST_ROOT" {
+  run docker exec dokku dokku storage:create "$ENTRY_NAME" --chown root
+  echo "output: $output"
+  echo "status: $status"
+  [[ "$status" -eq 0 ]]
+
+  # storage:create mkdirs under DOKKU_LIB_ROOT inside the container, which is
+  # the same tree as DOKKU_LIB_HOST_ROOT on the host via the /mnt/dokku volume.
+  HOST_STORAGE_DIR="${DOKKU_LIB_HOST_ROOT}/data/storage/${ENTRY_NAME}"
+  [[ -d "$HOST_STORAGE_DIR" ]]
+
+  echo "Hello World!" | sudo tee "${HOST_STORAGE_DIR}/welcome.txt" >/dev/null
+
+  run docker exec dokku dokku storage:exec "$ENTRY_NAME" -- cat /data/welcome.txt
+  echo "output: $output"
+  echo "status: $status"
+  [[ "$status" -eq 0 ]]
+  [[ "$output" == *"Hello World!"* ]]
+}

tests/unit/storage-5.bats

@@ -0,0 +1,117 @@
+#!/usr/bin/env bats
+
+load test_helper
+
+setup() {
+  global_setup
+  create_app
+  rm -rf "${DOKKU_LIB_ROOT}"/data/storage/rdmtest*
+}
+
+teardown() {
+  destroy_app
+  global_teardown
+}
+
+@test "(storage) without DOKKU_LIB_HOST_ROOT docker-args and storage:exec use DOKKU_LIB_ROOT" {
+  unset DOKKU_LIB_HOST_ROOT
+
+  run /bin/bash -c "dokku storage:create rdmtest-bare"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-bare --container-dir /data"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run /bin/bash -c "echo '' | dokku plugin:trigger docker-args-process-deploy $TEST_APP herokuish latest web | grep -o -- '-v [^ ]*'"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+  assert_output "-v ${DOKKU_LIB_ROOT}/data/storage/rdmtest-bare:/data"
+
+  run /bin/bash -c "docker pull alpine:3 >/dev/null"
+  assert_success
+
+  run /bin/bash -c "dokku storage:exec rdmtest-bare -- /bin/sh -c 'echo bare-metal > /data/marker && cat /data/marker'"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+  assert_output_contains "bare-metal"
+
+  run /bin/bash -c "cat ${DOKKU_LIB_ROOT}/data/storage/rdmtest-bare/marker"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+  assert_output "bare-metal"
+
+  run /bin/bash -c "dokku storage:unmount --all $TEST_APP"
+  assert_success
+  run /bin/bash -c "dokku storage:destroy rdmtest-bare --force"
+  assert_success
+}
+
+@test "(storage) with DOKKU_LIB_HOST_ROOT docker-args rewrites the mount host path" {
+  HOST_ROOT="$(mktemp -d /tmp/dokku-lib-host.XXXXXX)"
+  export DOKKU_LIB_HOST_ROOT="$HOST_ROOT"
+
+  run /bin/bash -c "dokku storage:create rdmtest-hostroot"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  # Create still lands under DOKKU_LIB_ROOT inside Dokku.
+  run /bin/bash -c "test -d ${DOKKU_LIB_ROOT}/data/storage/rdmtest-hostroot"
+  assert_success
+
+  run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-hostroot --container-dir /data"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run /bin/bash -c "echo '' | dokku plugin:trigger docker-args-process-deploy $TEST_APP herokuish latest web | grep -o -- '-v [^ ]*'"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+  assert_output "-v ${HOST_ROOT}/data/storage/rdmtest-hostroot:/data"
+
+  run /bin/bash -c "dokku storage:unmount --all $TEST_APP"
+  assert_success
+  run /bin/bash -c "dokku storage:destroy rdmtest-hostroot --force"
+  assert_success
+  rm -rf "$HOST_ROOT"
+}
+
+@test "(storage) with DOKKU_LIB_HOST_ROOT storage:exec mounts the host-root path" {
+  # Simulate Dokku-in-Docker: filesystem ops use DOKKU_LIB_ROOT, but Docker -v
+  # mounts must resolve under DOKKU_LIB_HOST_ROOT.
+  HOST_ROOT="$(mktemp -d /tmp/dokku-lib-host.XXXXXX)"
+  export DOKKU_LIB_HOST_ROOT="$HOST_ROOT"
+
+  run /bin/bash -c "dokku storage:create rdmtest-hostroot-exec --chown root"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  mkdir -p "${HOST_ROOT}/data/storage/rdmtest-hostroot-exec"
+  echo "Hello World!" >"${HOST_ROOT}/data/storage/rdmtest-hostroot-exec/welcome.txt"
+
+  run /bin/bash -c "docker pull alpine:3 >/dev/null"
+  assert_success
+
+  run /bin/bash -c "dokku storage:exec rdmtest-hostroot-exec -- cat /data/welcome.txt"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+  assert_output_contains "Hello World!"
+
+  # The in-container create path must not have received the host-written file.
+  run /bin/bash -c "test ! -f ${DOKKU_LIB_ROOT}/data/storage/rdmtest-hostroot-exec/welcome.txt"
+  assert_success
+
+  run /bin/bash -c "dokku storage:destroy rdmtest-hostroot-exec --force"
+  assert_success
+  rm -rf "$HOST_ROOT"
+}