From 1d0985428a6da03937942f47329a79b855cf8797 Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Sun, 20 Sep 2026 00:48:58 -0400 Subject: [PATCH] feat: add a whole-set form for an app's storage mounts The `storage` plugin mounted and unmounted one attachment at a time, so matching an app's mounts to a declared set meant reading `storage:list`, computing the difference in both directions and issuing one `storage:mount` or `storage:unmount` per addition and removal, with a failure partway through leaving the app holding a mixture of the two sets. A `--replace` flag on `storage:mount` now writes the declared set in a single call, taking `name:container-dir` pairs whose optional third field carries the same mount options the legacy colon form accepts. Every pair is parsed and validated before anything is written, so a rejected pair leaves the stored mounts untouched. The remaining mount-time flags scope the whole call the way `--process-type` already scoped a single mount, which means an omitted `--process-type` replaces the `_default_` scope and leaves mounts scoped to a named process type in place. An empty pair list is rejected rather than treated as a request to unmount everything, so a generated list that expands to nothing cannot silently drop an app's storage. `storage:unmount --all` handles that case instead, taking an optional `--process-type` filter and treating a scope that matches nothing as a no-op so it stays idempotent. `storage:unmount` also accepts more than one mount at a time, naming each by entry name or by the colon form, and resolves every argument against the app's attachments before removing any of them. The help listing now carries one entry per subcommand with its flags inline rather than a row per variant, and the command listing at the top of the storage documentation matches it line for line. Folding those rows together also surfaced `--global` on `storage:report` and `storage:migrate`, neither of which both listings had mentioned. Finally, `--volume-chown` is now validated on mount, where a typo previously exited zero and wrote a value that only the report ever read. --- docs/advanced-usage/persistent-storage.md | 113 +++- plugins/storage/attachment.go | 21 +- plugins/storage/mounts_test.go | 533 ++++++++++++++++++ plugins/storage/src/commands/commands.go | 12 +- .../storage/src/subcommands/subcommands.go | 21 +- plugins/storage/subcommands.go | 397 +++++++++++-- tests/unit/storage-3.bats | 339 +++++++++++ 7 files changed, 1368 insertions(+), 68 deletions(-) create mode 100644 plugins/storage/mounts_test.go create mode 100644 tests/unit/storage-3.bats diff --git a/docs/advanced-usage/persistent-storage.md b/docs/advanced-usage/persistent-storage.md index 6943a9b48..3e17ade6b 100644 --- a/docs/advanced-usage/persistent-storage.md +++ b/docs/advanced-usage/persistent-storage.md @@ -6,24 +6,23 @@ The preferred method to attach persistent storage to a Dokku-managed container is the Dokku storage plugin. ``` -storage:annotations:report [] [] # Display annotations for one or more storage entries -storage:annotations:set [] # Set or clear a single annotation on a storage entry -storage:create [] [flags] # Register a named storage entry -storage:destroy [--force] [--destroy-host-dir] # Remove a named storage entry (must be unmounted from every app first) -storage:ensure-directory [--chown option] # [DEPRECATED] use storage:create instead -storage:exec [-- ...] # Run a command (or shell) in a temporary container that mounts the entry -storage:info [--format text|json] # Show details for one storage entry -storage:labels:report [] [] # Display labels for one or more storage entries -storage:labels:set [] # Set or clear a single label on a storage entry -storage:list [--format text|json] # List bind mounts for an app's container(s) (legacy host:container view) -storage:list-entries [--scheduler s] [--format text|json] # List registered storage entries -storage:mount --container-dir [flags] # Mount a named entry into an app -storage:mount # [LEGACY] colon-form mount, docker-local only -storage:report [] [] # Display a storage report for one or more apps -storage:report --global # Display a cluster-wide entry inventory -storage:set [] # Update a storage entry in place -storage:unmount [--container-dir ] # Remove an attachment -storage:wait # Block until a k3s entry's PVC is bound +storage:annotations:report [] [] # Displays annotations for one or more storage entries +storage:annotations:set [] # Set or clear a single annotation on a storage entry +storage:create [] [flags] # Register a named storage entry +storage:destroy [--force] [--destroy-host-dir] # Remove a named storage entry (must be unmounted from every app first) +storage:ensure-directory [--chown option] # [DEPRECATED] use storage:create instead +storage:exec [-- ...] # Run a command (or shell) in a temporary container that mounts the entry +storage:info [--format text|json] # Show details for one storage entry +storage:labels:report [] [] # Displays labels for one or more storage entries +storage:labels:set [] # Set or clear a single label on a storage entry +storage:list [--format text|json] # List bind mounts for an app's container(s) (legacy host:container view) +storage:list-entries [--scheduler s] [--format text|json] # List registered storage entries +storage:migrate [|--all] # Re-run the legacy -v to attachment migration for an app +storage:mount [--replace] ... [flags] # Create or replace bind mounts +storage:report [|--global] [] # Displays a storage report for one or more apps +storage:set [] # Update a storage entry in place +storage:unmount [--all] [...] [flags] # Remove one or all bind mounts +storage:wait # Wait for a storage entry's PVC to be bound (k3s) ``` A storage entry is the source of truth for the underlying volume - a host directory on docker-local, or a PersistentVolumeClaim on k3s. Multiple apps can mount the same entry, and an attachment carries the per-app details (container path, phases, subpath, readonly, process type). Names are globally unique across the install and must be DNS-1123 labels (lowercase letters, digits, dashes) of 45 characters or less so they can be used verbatim as Helm release and PVC names. @@ -269,9 +268,50 @@ Once persistent storage is mounted, the app requires a restart. See the [process dokku ps:restart app-name ``` +#### Replacing the entire mount set + +> [!IMPORTANT] +> New as of 0.38.28 + +The form above mounts one entry at a time, so matching an app's mounts to a declared set means reading `storage:list`, computing the difference in both directions and issuing one `storage:mount` or `storage:unmount` per addition and removal - with a failure partway through leaving the app holding a mixture of the two sets. The `--replace` flag writes the whole set in a single call, taking `name:container-dir` pairs instead of a single entry and a `--container-dir` flag. + +```shell +dokku storage:mount --replace node-js-app node-js-data:/app/storage node-js-cache:/cache +``` + +Anything previously mounted and not named in the call is unmounted. Each pair takes the same optional third field as the legacy colon form, so `ro` and other mount options can vary between mounts: + +```shell +dokku storage:mount --replace node-js-app node-js-data:/app/storage:Z node-js-cache:/cache:ro,noexec +``` + +A pair whose first field starts with `/` is a legacy host path rather than an entry name, and registers its `legacy-` entry the same way the colon form does. Any other first field must name a registered entry, so a mistyped name is rejected rather than taken for a docker volume. A docker volume already mounted through the colon form is named here by the `legacy-` entry that `storage:list-entries` shows. + +The remaining mount-time flags scope the whole call rather than a single pair, so every mount in the replacement shares them: + +```shell +dokku storage:mount --replace node-js-app node-js-data:/app/storage --phase deploy --volume-subpath uploads --volume-chown herokuish +``` + +This means `--volume-subpath` and `--volume-chown` cannot differ between mounts declared in one call. Mounts needing different values for either are declared under a different `--process-type`, or set individually with the single-entry form. + +`--process-type` scopes the replacement the same way it scopes a single mount. Omitting it replaces the `_default_` process type, leaving mounts scoped to a named process type in place: + +```shell +dokku storage:mount --replace node-js-app node-js-data:/app/storage +dokku storage:mount --replace node-js-app node-js-cache:/cache --process-type web +``` + +Every pair is parsed and validated before anything is written, so a rejected pair leaves the stored mounts untouched. A pair without a container directory, a container directory that is not absolute, an entry that is not registered, an entry whose scheduler does not match the app's, and a container directory named more than once in one call are all rejected. + +> [!NOTE] +> An empty pair list is rejected rather than treated as a request to unmount everything, so a generated list that expands to nothing cannot silently drop an app's storage. Use `storage:unmount --all` for that. + +As with a single mount, the app requires a restart afterward. + ### Unmounting storage -If an app no longer requires a mounted volume or directory, the `storage:unmount` command can be called. This takes the same arguments as the `storage:mount` command, an app name and a `host-path:container-path` or `docker-volume:container-path` combination. +If an app no longer requires a mounted volume or directory, the `storage:unmount` command can be called. This takes the same arguments as the `storage:mount` command, an app name and a `host-path:container-path` or `docker-volume:container-path` combination, or a registered entry name. ```shell # unmount the directory from your container's /app/storage directory, relative to the container root (/) @@ -281,6 +321,39 @@ dokku storage:unmount node-js-app /var/lib/dokku/data/storage/node-js-app:/app/s dokku storage:unmount node-js-app some-docker-volume:/app/storage ``` +> [!IMPORTANT] +> Removing more than one attachment at a time is new as of 0.38.28 + +More than one attachment can be removed in a single call, naming each by entry name or by the colon form. Every argument is resolved against the app's attachments before any of them is removed, so an argument naming an attachment the app does not have removes none of them: + +```shell +dokku storage:unmount node-js-app node-js-data node-js-cache +``` + +An entry mounted at several container directories is disambiguated with `--container-dir`, which therefore applies only when a single entry is named. Naming the container directory in the argument itself works with any number of attachments: + +```shell +dokku storage:unmount node-js-app node-js-data --container-dir /app/storage +dokku storage:unmount node-js-app node-js-data:/app/storage node-js-data:/app/uploads +``` + +#### Removing every mount + +> [!IMPORTANT] +> New as of 0.38.28 + +The `--all` flag removes every attachment on an app. This is the empty case `storage:mount --replace` refuses to handle, and it takes no entry arguments: + +```shell +dokku storage:unmount --all node-js-app +``` + +`--process-type` narrows what is removed to a single scope, filtering the same way it scopes a replacement. Omitting it removes every scope, and a scope that matches nothing is a no-op rather than an error, so the command is safe to re-run: + +```shell +dokku storage:unmount --all node-js-app --process-type web +``` + Once persistent storage is unmounted, the app requires a restart. See the [process scaling documentation](/docs/processes/process-management.md) for more information. ```shell @@ -435,6 +508,8 @@ dokku storage:report node-js-app --format json | jq '. | with_entries(select(.ke } ``` +Indices are assigned in stored order, so `storage:mount --replace` renumbers them: the attachments it leaves in place keep their relative order and the declared ones follow. + A single attachment field can be fetched directly via the info-flag form: ```shell diff --git a/plugins/storage/attachment.go b/plugins/storage/attachment.go index 8b02fcec4..510428885 100644 --- a/plugins/storage/attachment.go +++ b/plugins/storage/attachment.go @@ -172,6 +172,19 @@ func RemoveAttachment(appName string, entryName string, containerPath string) er return err } + keep, err := removeMatchingAttachments(attachments, appName, entryName, containerPath) + if err != nil { + return err + } + + return SaveAttachments(appName, keep) +} + +// removeMatchingAttachments returns the given list with the attachments +// matching the entry and optional container path dropped. Working against an +// in-memory list lets storage:unmount resolve several mounts before any of +// them is written, so one unresolvable mount removes none of them. +func removeMatchingAttachments(attachments []*Attachment, appName string, entryName string, containerPath string) ([]*Attachment, error) { matches := []*Attachment{} keep := []*Attachment{} for _, attachment := range attachments { @@ -184,9 +197,9 @@ func RemoveAttachment(appName string, entryName string, containerPath string) er if len(matches) == 0 { if containerPath == "" { - return fmt.Errorf("storage entry %q is not mounted on app %q", entryName, appName) + return nil, fmt.Errorf("storage entry %q is not mounted on app %q", entryName, appName) } - return fmt.Errorf("storage entry %q is not mounted at %q on app %q", entryName, containerPath, appName) + return nil, fmt.Errorf("storage entry %q is not mounted at %q on app %q", entryName, containerPath, appName) } if len(matches) > 1 && containerPath == "" { paths := []string{} @@ -194,11 +207,11 @@ func RemoveAttachment(appName string, entryName string, containerPath string) er paths = append(paths, attachment.ContainerPath) } sort.Strings(paths) - return fmt.Errorf("storage entry %q is mounted at multiple paths on app %q (%s); pass --container-dir to disambiguate", + return nil, fmt.Errorf("storage entry %q is mounted at multiple paths on app %q (%s); pass --container-dir to disambiguate", entryName, appName, strings.Join(paths, ", ")) } - return SaveAttachments(appName, keep) + return keep, nil } // AttachmentsForPhase returns the subset of an app's attachments that diff --git a/plugins/storage/mounts_test.go b/plugins/storage/mounts_test.go new file mode 100644 index 000000000..a55a5e337 --- /dev/null +++ b/plugins/storage/mounts_test.go @@ -0,0 +1,533 @@ +package storage + +import ( + "testing" + + . "github.com/onsi/gomega" +) + +func TestParseMountSpec(t *testing.T) { + RegisterTestingT(t) + + parsed, err := parseMountSpec("demo-data:/app/storage") + Expect(err).NotTo(HaveOccurred()) + Expect(parsed.EntryName).To(Equal("demo-data")) + Expect(parsed.ContainerPath).To(Equal("/app/storage")) + Expect(parsed.Readonly).To(BeFalse()) + Expect(parsed.VolumeOptions).To(BeEmpty()) + + parsed, err = parseMountSpec("demo-data:/app/storage:ro,noexec,nosuid") + Expect(err).NotTo(HaveOccurred()) + Expect(parsed.Readonly).To(BeTrue()) + Expect(parsed.VolumeOptions).To(Equal("noexec,nosuid")) + + for _, spec := range []string{"demo-data", ":/app/storage", ""} { + _, err := parseMountSpec(spec) + Expect(err).To(HaveOccurred(), "expected %q to be rejected", spec) + Expect(err.Error()).To(ContainSubstring("Invalid mount specified")) + } +} + +func TestValidateChownOption(t *testing.T) { + RegisterTestingT(t) + + for _, chownFlag := range []string{"", "herokuish", "heroku", "paketo", "packeto", "root", "false", "0", "1000", "65535"} { + Expect(ValidateChownOption(chownFlag)).To(Succeed(), "expected %q to be accepted", chownFlag) + } + + for _, chownFlag := range []string{"bogus", "-1", "65536", "1.5", "herokuish "} { + err := ValidateChownOption(chownFlag) + Expect(err).To(HaveOccurred(), "expected %q to be rejected", chownFlag) + Expect(err.Error()).To(ContainSubstring("Unsupported chown permissions")) + } +} + +func TestCommandMountRejectsInvalidVolumeChown(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + stageDockerLocalEntry(t, "demo-data") + + err := CommandMount(CommandMountInput{ + AppName: "demo", + NameOrPath: "demo-data", + ContainerDir: "/app/storage", + VolumeChown: "bogus", + }) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("Unsupported chown permissions")) + + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(BeEmpty()) +} + +func TestCommandMountReplaceReplacesTheScope(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + stageDockerLocalEntry(t, "demo-data") + stageDockerLocalEntry(t, "demo-cache") + stageDockerLocalEntry(t, "demo-logs") + + Expect(CommandMount(CommandMountInput{ + AppName: "demo", + NameOrPath: "demo-data", + ContainerDir: "/app/storage", + })).To(Succeed()) + + Expect(CommandMount(CommandMountInput{ + AppName: "demo", + Replace: true, + Specs: []string{"demo-cache:/cache", "demo-logs:/logs"}, + ProcessType: DefaultProcessType, + })).To(Succeed()) + + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(HaveLen(2)) + Expect(attachments[0].EntryName).To(Equal("demo-cache")) + Expect(attachments[0].ContainerPath).To(Equal("/cache")) + Expect(attachments[0].Phases).To(Equal([]string{PhaseDeploy, PhaseRun})) + Expect(attachments[0].ProcessType).To(Equal(DefaultProcessType)) + Expect(attachments[1].EntryName).To(Equal("demo-logs")) +} + +func TestCommandMountReplaceLeavesOtherProcessTypesUntouched(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + stageDockerLocalEntry(t, "demo-data") + stageDockerLocalEntry(t, "demo-cache") + stageDockerLocalEntry(t, "demo-logs") + + Expect(CommandMount(CommandMountInput{ + AppName: "demo", + NameOrPath: "demo-data", + ContainerDir: "/app/storage", + })).To(Succeed()) + + Expect(CommandMount(CommandMountInput{ + AppName: "demo", + NameOrPath: "demo-cache", + ContainerDir: "/cache", + ProcessType: "web", + })).To(Succeed()) + + Expect(CommandMount(CommandMountInput{ + AppName: "demo", + Replace: true, + Specs: []string{"demo-logs:/logs"}, + })).To(Succeed()) + + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(HaveLen(2)) + Expect(attachments[0].EntryName).To(Equal("demo-cache")) + Expect(attachments[0].ProcessType).To(Equal("web")) + Expect(attachments[1].EntryName).To(Equal("demo-logs")) + Expect(attachments[1].ProcessType).To(Equal(DefaultProcessType)) +} + +func TestCommandMountReplaceAppliesScopeFlags(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + stageDockerLocalEntry(t, "demo-data") + stageDockerLocalEntry(t, "demo-cache") + + Expect(CommandMount(CommandMountInput{ + AppName: "demo", + Replace: true, + Specs: []string{"demo-data:/app/storage:Z", "demo-cache:/cache:ro,noexec"}, + Phases: []string{PhaseDeploy}, + ProcessType: "web", + Subpath: "uploads", + VolumeChown: "herokuish", + })).To(Succeed()) + + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(HaveLen(2)) + for _, attachment := range attachments { + Expect(attachment.Phases).To(Equal([]string{PhaseDeploy})) + Expect(attachment.ProcessType).To(Equal("web")) + Expect(attachment.Subpath).To(Equal("uploads")) + Expect(attachment.VolumeChown).To(Equal("herokuish")) + } + + Expect(attachments[0].Readonly).To(BeFalse()) + Expect(attachments[0].VolumeOptions).To(Equal("Z")) + Expect(attachments[1].Readonly).To(BeTrue()) + Expect(attachments[1].VolumeOptions).To(Equal("noexec")) +} + +func TestCommandMountReplaceReadonlyFlagAppliesToEverySpec(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + stageDockerLocalEntry(t, "demo-data") + stageDockerLocalEntry(t, "demo-cache") + + Expect(CommandMount(CommandMountInput{ + AppName: "demo", + Replace: true, + Specs: []string{"demo-data:/app/storage", "demo-cache:/cache"}, + Readonly: true, + })).To(Succeed()) + + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(HaveLen(2)) + Expect(attachments[0].Readonly).To(BeTrue()) + Expect(attachments[1].Readonly).To(BeTrue()) +} + +func TestCommandMountReplaceIsIdempotent(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + stageDockerLocalEntry(t, "demo-data") + stageDockerLocalEntry(t, "demo-cache") + + input := CommandMountInput{ + AppName: "demo", + Replace: true, + Specs: []string{"demo-data:/app/storage", "demo-cache:/cache"}, + } + + Expect(CommandMount(input)).To(Succeed()) + first, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + + Expect(CommandMount(input)).To(Succeed()) + second, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + + Expect(second).To(Equal(first)) +} + +func TestCommandMountReplaceRejectsEmptySpecs(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + stageDockerLocalEntry(t, "demo-data") + + Expect(CommandMount(CommandMountInput{ + AppName: "demo", + NameOrPath: "demo-data", + ContainerDir: "/app/storage", + })).To(Succeed()) + + err := CommandMount(CommandMountInput{AppName: "demo", Replace: true}) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(Equal("Must specify at least one mount, use storage:unmount --all to remove all mounts")) + + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(HaveLen(1)) +} + +func TestCommandMountReplaceRejectsContainerDirFlag(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + stageDockerLocalEntry(t, "demo-data") + + err := CommandMount(CommandMountInput{ + AppName: "demo", + Replace: true, + Specs: []string{"demo-data:/app/storage"}, + ContainerDir: "/app/storage", + }) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(Equal("The --container-dir flag cannot be used with --replace")) +} + +func TestCommandMountReplaceRejectsDuplicateContainerPath(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + stageDockerLocalEntry(t, "demo-data") + stageDockerLocalEntry(t, "demo-cache") + + err := CommandMount(CommandMountInput{ + AppName: "demo", + Replace: true, + Specs: []string{"demo-data:/app/storage", "demo-cache:/app/storage"}, + }) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(Equal("Container path /app/storage is specified more than once")) + + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(BeEmpty()) +} + +func TestCommandMountReplaceLeavesStoreUntouchedOnInvalidSpec(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + stageDockerLocalEntry(t, "demo-data") + stageDockerLocalEntry(t, "demo-cache") + + Expect(CommandMount(CommandMountInput{ + AppName: "demo", + NameOrPath: "demo-data", + ContainerDir: "/app/storage", + })).To(Succeed()) + + err := CommandMount(CommandMountInput{ + AppName: "demo", + Replace: true, + Specs: []string{"demo-cache:/cache", "demo-missing:/missing"}, + }) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(`storage entry "demo-missing" does not exist`)) + + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(HaveLen(1)) + Expect(attachments[0].EntryName).To(Equal("demo-data")) +} + +func TestCommandMountReplaceRejectsRelativeContainerPath(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + stageDockerLocalEntry(t, "demo-data") + + err := CommandMount(CommandMountInput{ + AppName: "demo", + Replace: true, + Specs: []string{"demo-data:app/storage"}, + }) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("must be absolute")) + + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(BeEmpty()) +} + +func TestCommandMountReplaceRejectsSchedulerMismatch(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + + Expect(SaveEntry(&Entry{ + Name: "demo-pvc", + Scheduler: SchedulerK3s, + })).To(Succeed()) + + err := CommandMount(CommandMountInput{ + AppName: "demo", + Replace: true, + Specs: []string{"demo-pvc:/data"}, + }) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(Equal(`storage entry "demo-pvc" is scheduler=k3s but cannot be mounted on a docker-local app; recreate it with --scheduler docker-local`)) +} + +func TestCommandMountReplaceRejectsUnregisteredEntry(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + + err := CommandMount(CommandMountInput{ + AppName: "demo", + Replace: true, + Specs: []string{"demo-data:/app/storage"}, + }) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(`storage entry "demo-data" does not exist`)) +} + +func TestCommandMountReplaceAcceptsLegacyHostPath(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + + Expect(CommandMount(CommandMountInput{ + AppName: "demo", + Replace: true, + Specs: []string{"/var/lib/dokku/data/storage/shared:/app/shared"}, + })).To(Succeed()) + + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(HaveLen(1)) + Expect(attachments[0].ContainerPath).To(Equal("/app/shared")) + + expected := LegacyMountToEntry("/var/lib/dokku/data/storage/shared:/app/shared") + Expect(attachments[0].EntryName).To(Equal(expected.Name)) + Expect(EntryExists(expected.Name)).To(BeTrue()) +} + +func TestCommandUnmountRemovesMultipleMounts(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + stageDockerLocalEntry(t, "demo-data") + stageDockerLocalEntry(t, "demo-cache") + stageDockerLocalEntry(t, "demo-logs") + + Expect(CommandMount(CommandMountInput{ + AppName: "demo", + Replace: true, + Specs: []string{"demo-data:/app/storage", "demo-cache:/cache", "demo-logs:/logs"}, + })).To(Succeed()) + + Expect(CommandUnmount(CommandUnmountInput{ + AppName: "demo", + Mounts: []string{"demo-data", "demo-logs"}, + })).To(Succeed()) + + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(HaveLen(1)) + Expect(attachments[0].EntryName).To(Equal("demo-cache")) +} + +func TestCommandUnmountAcceptsEntryColonContainerPath(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + stageDockerLocalEntry(t, "demo-data") + + Expect(CommandMount(CommandMountInput{ + AppName: "demo", + Replace: true, + Specs: []string{"demo-data:/app/storage", "demo-data:/app/uploads"}, + })).To(Succeed()) + + Expect(CommandUnmount(CommandUnmountInput{ + AppName: "demo", + Mounts: []string{"demo-data:/app/uploads"}, + })).To(Succeed()) + + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(HaveLen(1)) + Expect(attachments[0].ContainerPath).To(Equal("/app/storage")) +} + +func TestCommandUnmountLeavesStoreUntouchedOnUnknownMount(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + stageDockerLocalEntry(t, "demo-data") + stageDockerLocalEntry(t, "demo-cache") + + Expect(CommandMount(CommandMountInput{ + AppName: "demo", + Replace: true, + Specs: []string{"demo-data:/app/storage", "demo-cache:/cache"}, + })).To(Succeed()) + + err := CommandUnmount(CommandUnmountInput{ + AppName: "demo", + Mounts: []string{"demo-data", "demo-missing"}, + }) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(`storage entry "demo-missing" is not mounted`)) + + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(HaveLen(2)) +} + +func TestCommandUnmountRejectsContainerDirWithMultipleMounts(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + + err := CommandUnmount(CommandUnmountInput{ + AppName: "demo", + Mounts: []string{"demo-data", "demo-cache"}, + ContainerDir: "/app/storage", + }) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(Equal("The --container-dir flag cannot be used with multiple mounts")) +} + +func TestCommandUnmountRequiresAMount(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + + err := CommandUnmount(CommandUnmountInput{AppName: "demo"}) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(Equal("Must specify at least one mount, use storage:unmount --all to remove all mounts")) +} + +func TestCommandUnmountRejectsProcessTypeWithoutAll(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + + err := CommandUnmount(CommandUnmountInput{ + AppName: "demo", + Mounts: []string{"demo-data"}, + ProcessType: "web", + }) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(Equal("The --process-type flag can only be used with --all")) +} + +func TestCommandUnmountAllRemovesEverything(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + stageDockerLocalEntry(t, "demo-data") + stageDockerLocalEntry(t, "demo-cache") + + Expect(CommandMount(CommandMountInput{ + AppName: "demo", + Replace: true, + Specs: []string{"demo-data:/app/storage", "demo-cache:/cache"}, + })).To(Succeed()) + + Expect(CommandUnmount(CommandUnmountInput{AppName: "demo", All: true})).To(Succeed()) + + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(BeEmpty()) + + // Idempotent: clearing an app with no mounts is not an error. + Expect(CommandUnmount(CommandUnmountInput{AppName: "demo", All: true})).To(Succeed()) +} + +func TestCommandUnmountAllScopedToProcessType(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + stageDockerLocalEntry(t, "demo-data") + stageDockerLocalEntry(t, "demo-cache") + + Expect(CommandMount(CommandMountInput{ + AppName: "demo", + Replace: true, + Specs: []string{"demo-data:/app/storage"}, + })).To(Succeed()) + + Expect(CommandMount(CommandMountInput{ + AppName: "demo", + Replace: true, + Specs: []string{"demo-cache:/cache"}, + ProcessType: "web", + })).To(Succeed()) + + Expect(CommandUnmount(CommandUnmountInput{ + AppName: "demo", + All: true, + ProcessType: "web", + })).To(Succeed()) + + attachments, err := LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(HaveLen(1)) + Expect(attachments[0].EntryName).To(Equal("demo-data")) + + // A scope that matches nothing is a no-op rather than an error. + Expect(CommandUnmount(CommandUnmountInput{ + AppName: "demo", + All: true, + ProcessType: "worker", + })).To(Succeed()) + + attachments, err = LoadAttachments("demo") + Expect(err).NotTo(HaveOccurred()) + Expect(attachments).To(HaveLen(1)) +} + +func TestCommandUnmountAllRejectsNamedMount(t *testing.T) { + RegisterTestingT(t) + setupTestApp(t, "demo") + + err := CommandUnmount(CommandUnmountInput{ + AppName: "demo", + All: true, + Mounts: []string{"demo-data"}, + }) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(Equal("A mount cannot be specified with --all")) +} diff --git a/plugins/storage/src/commands/commands.go b/plugins/storage/src/commands/commands.go index 7c9828af0..6be542380 100644 --- a/plugins/storage/src/commands/commands.go +++ b/plugins/storage/src/commands/commands.go @@ -19,21 +19,21 @@ Additional commands:` helpContent = ` storage:annotations:report [] [], Displays annotations for one or more storage entries - storage:annotations:set [], Set or clear an annotation on a storage entry + storage:annotations:set [], Set or clear a single annotation on a storage entry storage:create [] [flags], Register a named storage entry storage:destroy [--force] [--destroy-host-dir], Remove a named storage entry (must be unmounted from every app first) storage:ensure-directory [--chown option] , [DEPRECATED] use storage:create instead storage:exec [-- ...], Run a command (or shell) in a temporary container that mounts the entry storage:info [--format text|json], Show details for one storage entry storage:labels:report [] [], Displays labels for one or more storage entries - storage:labels:set [], Set or clear a label on a storage entry - storage:list [--format text|json], List bind mounts for app's container(s) (host:container) + storage:labels:set [], Set or clear a single label on a storage entry + storage:list [--format text|json], List bind mounts for an app's container(s) (legacy host:container view) storage:list-entries [--scheduler s] [--format text|json], List registered storage entries storage:migrate [|--all], Re-run the legacy -v to attachment migration for an app - storage:mount , Create a new bind mount - storage:report [] [], Displays a storage report for one or more apps + storage:mount [--replace] ... [flags], Create or replace bind mounts + storage:report [|--global] [], Displays a storage report for one or more apps storage:set [], Update a storage entry in place - storage:unmount , Remove an existing bind mount + storage:unmount [--all] [...] [flags], Remove one or all bind mounts storage:wait , Wait for a storage entry's PVC to be bound (k3s)` ) diff --git a/plugins/storage/src/subcommands/subcommands.go b/plugins/storage/src/subcommands/subcommands.go index 59cd25408..b9c8955eb 100644 --- a/plugins/storage/src/subcommands/subcommands.go +++ b/plugins/storage/src/subcommands/subcommands.go @@ -224,10 +224,10 @@ func main() { readonly := args.Bool("volume-readonly", false, "--volume-readonly: mount the volume read-only") volumeChown := args.String("volume-chown", "", "--volume-chown: chown option applied at mount time") volumeOptions := args.String("volume-options", "", "--volume-options: comma-separated mount options (e.g. Z, noexec,nosuid)") + replace := args.Bool("replace", false, "--replace: replace the entire mount set for the process type") args.Parse(os.Args[2:]) - err = storage.CommandMount(storage.CommandMountInput{ + input := storage.CommandMountInput{ AppName: args.Arg(0), - NameOrPath: args.Arg(1), ContainerDir: *containerDir, Phases: *phases, ProcessType: *processType, @@ -235,7 +235,16 @@ func main() { Readonly: *readonly, VolumeChown: *volumeChown, VolumeOptions: *volumeOptions, - }) + Replace: *replace, + } + + if *replace { + input.Specs = common.VarArgs(args.Args(), 1) + } else { + input.NameOrPath = args.Arg(1) + } + + err = storage.CommandMount(input) case "report": args := flag.NewFlagSet("storage:report", flag.ExitOnError) format := args.String("format", "stdout", "format: [ stdout | json ]") @@ -251,11 +260,15 @@ func main() { case "unmount": args := flag.NewFlagSet("storage:unmount", flag.ExitOnError) containerDir := args.String("container-dir", "", "--container-dir: container path (named-entry form, disambiguates duplicates)") + all := args.Bool("all", false, "--all: remove every mount for the app or the specified process type") + processType := args.String("process-type", "", "--process-type: process type to remove mounts for (requires --all)") args.Parse(os.Args[2:]) err = storage.CommandUnmount(storage.CommandUnmountInput{ AppName: args.Arg(0), - NameOrPath: args.Arg(1), + Mounts: common.VarArgs(args.Args(), 1), ContainerDir: *containerDir, + All: *all, + ProcessType: *processType, }) default: err = fmt.Errorf("Invalid plugin subcommand call: %s", subcommand) diff --git a/plugins/storage/subcommands.go b/plugins/storage/subcommands.go index b6d04c4ab..aedf163c8 100644 --- a/plugins/storage/subcommands.go +++ b/plugins/storage/subcommands.go @@ -21,21 +21,21 @@ Additional commands:` helpContent = ` storage:annotations:report [] [], Displays annotations for one or more storage entries - storage:annotations:set [], Set or clear an annotation on a storage entry + storage:annotations:set [], Set or clear a single annotation on a storage entry storage:create [] [flags], Register a named storage entry storage:destroy [--force] [--destroy-host-dir], Remove a named storage entry (must be unmounted from every app first) storage:ensure-directory [--chown option] , [DEPRECATED] use storage:create instead storage:exec [-- ...], Run a command (or shell) in a temporary container that mounts the entry storage:info [--format text|json], Show details for one storage entry storage:labels:report [] [], Displays labels for one or more storage entries - storage:labels:set [], Set or clear a label on a storage entry - storage:list [--format text|json], List bind mounts for app's container(s) (host:container) + storage:labels:set [], Set or clear a single label on a storage entry + storage:list [--format text|json], List bind mounts for an app's container(s) (legacy host:container view) storage:list-entries [--scheduler s] [--format text|json], List registered storage entries storage:migrate [|--all], Re-run the legacy -v to attachment migration for an app - storage:mount , Create a new bind mount - storage:report [] [], Displays a storage report for one or more apps + storage:mount [--replace] ... [flags], Create or replace bind mounts + storage:report [|--global] [], Displays a storage report for one or more apps storage:set [], Update a storage entry in place - storage:unmount , Remove an existing bind mount + storage:unmount [--all] [...] [flags], Remove one or all bind mounts storage:wait , Wait for a storage entry's PVC to be bound (k3s)` ) @@ -118,6 +118,24 @@ func ResolveChownID(chownFlag string) (string, error) { return chownID, nil } +// ValidateChownOption reports whether a chown option is one the storage +// plugin understands, mirroring the set ResolveChownID maps. An empty value +// means no chown was requested. Unlike ResolveChownID it never shells out to +// docker for the user namespace offset, so it is safe to call as a pure +// validation step before anything has been written. +func ValidateChownOption(chownFlag string) error { + switch chownFlag { + case "", "herokuish", "heroku", "packeto", "paketo", "root", "false": + return nil + } + + if _, err := strconv.ParseUint(chownFlag, 10, 16); err != nil { + return errors.New("Unsupported chown permissions") + } + + return nil +} + // isUserNamespacesEnabled checks if Docker user namespaces are enabled func isUserNamespacesEnabled() (bool, error) { result, err := common.CallExecCommand(common.ExecCommandInput{ @@ -132,6 +150,10 @@ func isUserNamespacesEnabled() (bool, error) { // CommandMountInput captures the optional flags accepted by storage:mount // when the second argument is a named entry rather than a colon-form path. +// +// Replace and Specs drive the whole-set form, where the positional arguments +// are mount specs rather than a single entry and the remaining mount-time +// fields scope every spec in the call. type CommandMountInput struct { AppName string NameOrPath string @@ -142,6 +164,8 @@ type CommandMountInput struct { Readonly bool VolumeOptions string VolumeChown string + Replace bool + Specs []string } // CommandMount creates a new bind mount for an app. The second positional @@ -152,6 +176,10 @@ func CommandMount(input CommandMountInput) error { return err } + if input.Replace { + return replaceMounts(input) + } + appScheduler := common.GetAppScheduler(input.AppName) // Legacy colon form: synthesize a legacy- entry plus an @@ -182,6 +210,10 @@ func CommandMount(input CommandMountInput) error { return fmt.Errorf("storage entry %q is scheduler=%s but cannot be mounted on a %s app; recreate it with --scheduler %s", entry.Name, entry.Scheduler, appScheduler, appScheduler) } + if err := ValidateChownOption(input.VolumeChown); err != nil { + return err + } + phases := input.Phases if len(phases) == 0 { phases = []string{PhaseDeploy, PhaseRun} @@ -220,12 +252,199 @@ func CommandMount(input CommandMountInput) error { return nil } -// CommandUnmountInput captures the flags accepted by storage:unmount when -// the second argument is a named entry. +// mountSpec is one parsed positional argument of the storage:mount --replace +// form. It names an entry and the container path to bind it at, plus the +// mount-time fields the colon form can express on its own. +type mountSpec struct { + EntryName string + ContainerPath string + Readonly bool + VolumeOptions string +} + +// parseMountSpec splits an ":[:]" argument. +// ParseMountPath already implements that grammar - including hoisting the "ro" +// token out of the options list - so the only difference here is that the first +// field names a storage entry rather than a host path. +func parseMountSpec(spec string) (mountSpec, error) { + parsed := ParseMountPath(spec) + if parsed.HostPath == "" || parsed.ContainerPath == "" { + return mountSpec{}, fmt.Errorf("Invalid mount specified: %s", spec) + } + + return mountSpec{ + EntryName: parsed.HostPath, + ContainerPath: parsed.ContainerPath, + Readonly: parsed.Readonly, + VolumeOptions: parsed.VolumeOptions, + }, nil +} + +// resolveMountSpecEntry maps a parsed spec onto the storage entry it names. +// +// The single-mount form tells a named entry from a colon-form host path by the +// presence of a colon, which every --replace spec has, so a leading "/" is the +// discriminator instead. Without that rule a mistyped entry name would match +// the docker volume grammar and silently register a legacy entry for a volume +// that does not exist. +// +// The returned bool reports whether the entry still needs registering, so the +// registry write can wait until every spec in the call has validated. +func resolveMountSpecEntry(appName string, appScheduler string, spec string, parsed mountSpec) (*Entry, bool, error) { + if strings.HasPrefix(parsed.EntryName, "/") { + if appScheduler != SchedulerDockerLocal { + return nil, false, fmt.Errorf("colon-form mounts are only supported on docker-local apps; %s is a %s app. Create a named entry with 'dokku storage:create --scheduler %s' and mount it instead", appName, appScheduler, appScheduler) + } + + if err := VerifyPaths(spec); err != nil { + return nil, false, err + } + + entry := LegacyMountToEntry(spec) + return entry, !EntryExists(entry.Name), nil + } + + if !EntryExists(parsed.EntryName) { + return nil, false, fmt.Errorf("storage entry %q does not exist; create it first with `dokku storage:create`", parsed.EntryName) + } + + entry, err := LoadEntry(parsed.EntryName) + if err != nil { + return nil, false, err + } + + if entry.Scheduler != appScheduler { + return nil, false, fmt.Errorf("storage entry %q is scheduler=%s but cannot be mounted on a %s app; recreate it with --scheduler %s", entry.Name, entry.Scheduler, appScheduler, appScheduler) + } + + return entry, false, nil +} + +// attachmentProcessType returns the process type an attachment is scoped to, +// treating an unset value as the default scope so attachments written before +// the field existed still sort into a scope. +func attachmentProcessType(attachment *Attachment) string { + if attachment.ProcessType == "" { + return DefaultProcessType + } + + return attachment.ProcessType +} + +// replaceMounts swaps one process type's entire mount set for the declared one +// in a single write, so a caller converging an app onto a declared set no +// longer issues one storage:mount or storage:unmount per difference and can no +// longer fail partway through holding a mixture of the two sets. +// +// Every spec is parsed and validated before anything is written, so a rejected +// spec leaves the stored set untouched. Attachments belonging to other process +// types are kept: an omitted --process-type scopes the replacement to +// _default_ the same way it scopes a single storage:mount. +func replaceMounts(input CommandMountInput) error { + if input.ContainerDir != "" { + return errors.New("The --container-dir flag cannot be used with --replace") + } + + if len(input.Specs) == 0 { + return errors.New("Must specify at least one mount, use storage:unmount --all to remove all mounts") + } + + if err := ValidateChownOption(input.VolumeChown); err != nil { + return err + } + + phases := input.Phases + if len(phases) == 0 { + phases = []string{PhaseDeploy, PhaseRun} + } + + processType := input.ProcessType + if processType == "" { + processType = DefaultProcessType + } + + appScheduler := common.GetAppScheduler(input.AppName) + declared := []*Attachment{} + pending := []*Entry{} + containerPaths := map[string]bool{} + for _, spec := range input.Specs { + parsed, err := parseMountSpec(spec) + if err != nil { + return err + } + + entry, needsRegister, err := resolveMountSpecEntry(input.AppName, appScheduler, spec, parsed) + if err != nil { + return err + } + + // Two entries bound at one container path inside one process type + // would emit two -v flags for the same target, so the declared set + // must name each container path at most once. + if containerPaths[parsed.ContainerPath] { + return fmt.Errorf("Container path %s is specified more than once", parsed.ContainerPath) + } + containerPaths[parsed.ContainerPath] = true + + attachment := &Attachment{ + EntryName: entry.Name, + ContainerPath: parsed.ContainerPath, + Phases: phases, + ProcessType: processType, + Subpath: input.Subpath, + Readonly: input.Readonly || parsed.Readonly, + VolumeOptions: parsed.VolumeOptions, + VolumeChown: input.VolumeChown, + } + if err := attachment.Validate(); err != nil { + return err + } + + if needsRegister { + pending = append(pending, entry) + } + declared = append(declared, attachment) + } + + for _, entry := range pending { + if err := SaveEntry(entry); err != nil { + return err + } + } + + existing, err := LoadAttachments(input.AppName) + if err != nil { + return err + } + + attachments := []*Attachment{} + for _, attachment := range existing { + if attachmentProcessType(attachment) != processType { + attachments = append(attachments, attachment) + } + } + attachments = append(attachments, declared...) + + if err := SaveAttachments(input.AppName, attachments); err != nil { + return err + } + + common.LogInfo1(fmt.Sprintf("Storage mounts for process type %s replaced on %s", processType, input.AppName)) + return nil +} + +// CommandUnmountInput captures the arguments accepted by storage:unmount. +// Mounts holds one or more entry names or colon-form mount strings, each +// naming an attachment to remove. +// +// All and ProcessType drive the whole-set form, where no mount is named and +// every attachment in scope is removed at once. type CommandUnmountInput struct { AppName string - NameOrPath string + Mounts []string ContainerDir string + All bool + ProcessType string } // CommandUnmount removes an existing bind mount from an app. @@ -234,11 +453,142 @@ func CommandUnmount(input CommandUnmountInput) error { return err } - if strings.Contains(input.NameOrPath, ":") { - return unmountLegacyColon(input.AppName, input.NameOrPath) + if input.All { + return unmountAll(input) } - return RemoveAttachment(input.AppName, input.NameOrPath, input.ContainerDir) + if input.ProcessType != "" { + return errors.New("The --process-type flag can only be used with --all") + } + + if len(input.Mounts) == 0 { + return errors.New("Must specify at least one mount, use storage:unmount --all to remove all mounts") + } + + // --container-dir disambiguates a single entry mounted at several paths, + // which has no meaning once more than one mount is named. + if len(input.Mounts) > 1 && input.ContainerDir != "" { + return errors.New("The --container-dir flag cannot be used with multiple mounts") + } + + return unmountMounts(input) +} + +// unmountTarget names the attachment one storage:unmount argument resolves to. +type unmountTarget struct { + EntryName string + ContainerPath string + Legacy bool +} + +// resolveUnmountTarget maps one storage:unmount argument onto the attachment it +// names. An argument without a colon is a registered entry name, disambiguated +// by --container-dir when the entry is mounted at more than one path. An +// argument with a colon names a registered entry when its first field is one, +// and otherwise falls back to the legacy host:container form, which keeps a +// host path or docker volume name resolving the way it always has. +func resolveUnmountTarget(mount string, containerDir string) (unmountTarget, error) { + if !strings.Contains(mount, ":") { + return unmountTarget{EntryName: mount, ContainerPath: containerDir}, nil + } + + parsed := ParseMountPath(mount) + if !strings.HasPrefix(parsed.HostPath, "/") && EntryExists(parsed.HostPath) { + if parsed.ContainerPath == "" { + return unmountTarget{}, fmt.Errorf("Invalid mount specified: %s", mount) + } + + return unmountTarget{EntryName: parsed.HostPath, ContainerPath: parsed.ContainerPath}, nil + } + + if err := VerifyPaths(mount); err != nil { + return unmountTarget{}, err + } + + if parsed.ContainerPath == "" { + return unmountTarget{}, errors.New("Storage path must be two valid paths divided by colon.") + } + + return unmountTarget{ + EntryName: LegacyMountToEntry(mount).Name, + ContainerPath: parsed.ContainerPath, + Legacy: true, + }, nil +} + +// unmountMounts removes every named mount in a single write. Each argument is +// resolved and matched against the app's attachments before anything is +// written, so an argument naming a mount the app does not have leaves the rest +// of the set in place rather than removing a prefix of it. +func unmountMounts(input CommandUnmountInput) error { + attachments, err := LoadAttachments(input.AppName) + if err != nil { + return err + } + + for _, mount := range input.Mounts { + target, err := resolveUnmountTarget(mount, input.ContainerDir) + if err != nil { + return err + } + + attachments, err = removeMatchingAttachments(attachments, input.AppName, target.EntryName, target.ContainerPath) + if err != nil { + // The legacy form historically said "Mount path does not + // exist." - preserve that exact wording so existing automation + // and the bats suite keep matching. + if target.Legacy && strings.Contains(err.Error(), "is not mounted") { + return errors.New("Mount path does not exist.") + } + return err + } + } + + return SaveAttachments(input.AppName, attachments) +} + +// unmountAll removes every attachment on an app, or every attachment for one +// process type when --process-type is given. This is the empty case the +// --replace form refuses to treat as a request to remove everything, so that a +// generated spec list which expands to nothing cannot silently drop an app's +// mounts. A scope matching nothing is a no-op rather than an error, leaving the +// command idempotent. +func unmountAll(input CommandUnmountInput) error { + if len(input.Mounts) > 0 { + return errors.New("A mount cannot be specified with --all") + } + + if input.ContainerDir != "" { + return errors.New("The --container-dir flag cannot be used with --all") + } + + if input.ProcessType == "" { + if err := common.PropertyDelete(PluginName, input.AppName, AttachmentsProperty); err != nil { + return err + } + + common.LogInfo1(fmt.Sprintf("Removed all storage mounts on %s", input.AppName)) + return nil + } + + attachments, err := LoadAttachments(input.AppName) + if err != nil { + return err + } + + keep := []*Attachment{} + for _, attachment := range attachments { + if attachmentProcessType(attachment) != input.ProcessType { + keep = append(keep, attachment) + } + } + + if err := SaveAttachments(input.AppName, keep); err != nil { + return err + } + + common.LogInfo1(fmt.Sprintf("Removed all storage mounts for process type %s on %s", input.ProcessType, input.AppName)) + return nil } // mountLegacyColon translates a `:[:options]` mount @@ -283,29 +633,6 @@ func mountLegacyColon(appName string, mountPath string) error { return nil } -// unmountLegacyColon is the inverse of mountLegacyColon. The legacy -// mount string identifies an entry+container-path tuple deterministically -// via LegacyMountToEntry, so we can route to RemoveAttachment. -func unmountLegacyColon(appName string, mountPath string) error { - if err := VerifyPaths(mountPath); err != nil { - return err - } - parsed := ParseMountPath(mountPath) - if parsed.ContainerPath == "" { - return errors.New("Storage path must be two valid paths divided by colon.") - } - - entry := LegacyMountToEntry(mountPath) - if err := RemoveAttachment(appName, entry.Name, parsed.ContainerPath); err != nil { - // Match the legacy wording for "not currently mounted". - if strings.Contains(err.Error(), "is not mounted") { - return errors.New("Mount path does not exist.") - } - return err - } - return nil -} - // CommandList lists all bind mounts for an app. Reads attachments // directly from the storage plugin's own state rather than going through // the deprecated `storage-list` plugn trigger. diff --git a/tests/unit/storage-3.bats b/tests/unit/storage-3.bats new file mode 100644 index 000000000..2cfc1fbb7 --- /dev/null +++ b/tests/unit/storage-3.bats @@ -0,0 +1,339 @@ +#!/usr/bin/env bats + +load test_helper + +setup() { + global_setup + create_app + rm -rf "$DOKKU_LIB_ROOT/data/storage/rdmtestapp*" +} + +teardown() { + destroy_app + global_teardown +} + +@test "(storage:mount) --replace replaces the whole mount set" { + run /bin/bash -c "dokku storage:create rdmtest-data" + echo "output: $output" + echo "status: $status" + assert_success + run /bin/bash -c "dokku storage:create rdmtest-cache" + assert_success + run /bin/bash -c "dokku storage:create rdmtest-logs" + assert_success + + run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-data --container-dir /app/storage" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:mount --replace $TEST_APP rdmtest-cache:/cache rdmtest-logs:/logs" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r '.[].entry_name' | sort | xargs" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "rdmtest-cache rdmtest-logs" + + run /bin/bash -c "dokku storage:mount --replace $TEST_APP rdmtest-cache:/cache rdmtest-logs:/logs" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r '. | length'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "2" + + run /bin/bash -c "dokku storage:unmount --all $TEST_APP" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-data --force" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-cache --force" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-logs --force" + assert_success +} + +@test "(storage:mount) --replace leaves other process types untouched" { + run /bin/bash -c "dokku storage:create rdmtest-data" + assert_success + run /bin/bash -c "dokku storage:create rdmtest-cache" + assert_success + + run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-cache --container-dir /cache --process-type web" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:mount --replace $TEST_APP rdmtest-data:/app/storage" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r '.[].entry_name' | sort | xargs" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "rdmtest-cache rdmtest-data" + + run /bin/bash -c "dokku storage:report $TEST_APP --format json | jq -r '.\"attachment.1.process-type\"'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "web" + + run /bin/bash -c "dokku storage:report $TEST_APP --format json | jq -r '.\"attachment.2.process-type\"'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "_default_" + + run /bin/bash -c "dokku storage:unmount --all $TEST_APP" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-data --force" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-cache --force" + assert_success +} + +@test "(storage:mount) --replace applies scope flags to every spec" { + run /bin/bash -c "dokku storage:create rdmtest-data" + assert_success + run /bin/bash -c "dokku storage:create rdmtest-cache" + assert_success + + run /bin/bash -c "dokku storage:mount --replace $TEST_APP rdmtest-data:/app/storage:Z rdmtest-cache:/cache:ro,noexec --phase deploy --process-type web --volume-subpath uploads --volume-chown herokuish" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:report $TEST_APP --format json | jq -r '.\"attachment.1.phases\"'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "deploy" + + run /bin/bash -c "dokku storage:report $TEST_APP --format json | jq -r '.\"attachment.1.subpath\"'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "uploads" + + run /bin/bash -c "dokku storage:report $TEST_APP --format json | jq -r '.\"attachment.1.volume-chown\"'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "herokuish" + + run /bin/bash -c "dokku storage:report $TEST_APP --format json | jq -r '.\"attachment.1.volume-options\"'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "Z" + + run /bin/bash -c "dokku storage:report $TEST_APP --format json | jq -r '.\"attachment.2.readonly\"'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "true" + + run /bin/bash -c "dokku storage:report $TEST_APP --format json | jq -r '.\"attachment.2.volume-options\"'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "noexec" + + run /bin/bash -c "dokku storage:unmount --all $TEST_APP" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-data --force" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-cache --force" + assert_success +} + +@test "(storage:mount) --replace leaves the existing set untouched when a spec is invalid" { + run /bin/bash -c "dokku storage:create rdmtest-data" + assert_success + run /bin/bash -c "dokku storage:create rdmtest-cache" + assert_success + + run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-data --container-dir /app/storage" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:mount --replace $TEST_APP rdmtest-cache:/cache rdmtest-missing:/missing" + echo "output: $output" + echo "status: $status" + assert_failure + assert_output_contains "does not exist" -1 + + run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r '.[].entry_name' | xargs" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "rdmtest-data" + + run /bin/bash -c "dokku storage:mount --replace $TEST_APP rdmtest-data:/app/storage rdmtest-cache:/app/storage" + echo "output: $output" + echo "status: $status" + assert_failure + assert_output_contains "specified more than once" -1 + + run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r '.[].entry_name' | xargs" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "rdmtest-data" + + run /bin/bash -c "dokku storage:unmount --all $TEST_APP" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-data --force" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-cache --force" + assert_success +} + +@test "(storage:mount) --replace rejects an empty spec list" { + run /bin/bash -c "dokku storage:mount --replace $TEST_APP" + echo "output: $output" + echo "status: $status" + assert_failure + assert_output_contains "storage:unmount --all" -1 +} + +@test "(storage:unmount) removes multiple mounts in one call" { + run /bin/bash -c "dokku storage:create rdmtest-data" + assert_success + run /bin/bash -c "dokku storage:create rdmtest-cache" + assert_success + run /bin/bash -c "dokku storage:create rdmtest-logs" + assert_success + + run /bin/bash -c "dokku storage:mount --replace $TEST_APP rdmtest-data:/app/storage rdmtest-cache:/cache rdmtest-logs:/logs" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:unmount $TEST_APP rdmtest-data rdmtest-logs" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r '.[].entry_name' | xargs" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "rdmtest-cache" + + run /bin/bash -c "dokku storage:unmount $TEST_APP rdmtest-cache rdmtest-missing" + echo "output: $output" + echo "status: $status" + assert_failure + + run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r '.[].entry_name' | xargs" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "rdmtest-cache" + + run /bin/bash -c "dokku storage:unmount --all $TEST_APP" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-data --force" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-cache --force" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-logs --force" + assert_success +} + +@test "(storage:unmount) --all removes every mount and is idempotent" { + run /bin/bash -c "dokku storage:create rdmtest-data" + assert_success + run /bin/bash -c "dokku storage:create rdmtest-cache" + assert_success + + run /bin/bash -c "dokku storage:mount --replace $TEST_APP rdmtest-data:/app/storage rdmtest-cache:/cache" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:unmount --all $TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r '. | length'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "0" + + run /bin/bash -c "dokku storage:unmount --all $TEST_APP" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:unmount --all $TEST_APP rdmtest-data" + echo "output: $output" + echo "status: $status" + assert_failure + assert_output_contains "cannot be specified with --all" -1 + + run /bin/bash -c "dokku storage:destroy rdmtest-data --force" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-cache --force" + assert_success +} + +@test "(storage:unmount) --all --process-type removes only that scope" { + run /bin/bash -c "dokku storage:create rdmtest-data" + assert_success + run /bin/bash -c "dokku storage:create rdmtest-cache" + assert_success + + run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-data --container-dir /app/storage" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:mount $TEST_APP rdmtest-cache --container-dir /cache --process-type web" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:unmount --all $TEST_APP --process-type web" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r '.[].entry_name' | xargs" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "rdmtest-data" + + run /bin/bash -c "dokku storage:unmount --all $TEST_APP --process-type worker" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r '. | length'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "1" + + run /bin/bash -c "dokku storage:unmount --all $TEST_APP" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-data --force" + assert_success + run /bin/bash -c "dokku storage:destroy rdmtest-cache --force" + assert_success +}