Merge pull request #9074 from dokku/9072-allow-storage-mount-replace-to-vary-mount-time-fields-per-mount

Allow storage:mount --replace to vary fields per mount

docs/advanced-usage/persistent-storage.md

@@ -271,7 +271,7 @@ dokku ps:restart app-name
 #### Scoping a mount to a process type
 
 > [!IMPORTANT]
-> New as of 0.38.28
+> New as of 0.38.29
 
 By default a mount is bound into every one of an app's containers. `--process-type` narrows it to a single Procfile process type, which is how a worker gets a scratch volume the web process has no business seeing:
 
@@ -305,7 +305,7 @@ dokku storage:mount node-js-app node-js-cache --container-dir /app/storage --pro
 #### Replacing the entire mount set
 
 > [!IMPORTANT]
-> New as of 0.38.28
+> New as of 0.38.29. Per-mount attributes in the option list new as of 0.38.30.
 
 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.
 
@@ -313,30 +313,47 @@ The form above mounts one entry at a time, so matching an app's mounts to a decl
 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:
+Anything previously mounted and not named in the call is unmounted. Each pair takes the same optional third field as the legacy colon form, a comma-separated list, and that list carries every mount-time attribute the mount needs, so two mounts declared in one call can differ in all of them:
 
 ```shell
-dokku storage:mount --replace node-js-app node-js-data:/app/storage:Z node-js-cache:/cache:ro,noexec
+dokku storage:mount --replace node-js-app \
+  node-js-data:/app/storage:ro,Z,volume-subpath=uploads,volume-chown=herokuish \
+  node-js-cache:/cache:phase=deploy,phase=run,noexec
 ```
 
-A pair whose first field starts with `/` is a legacy host path rather than an entry name, and registers its `legacy-<hash>` 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-<hash>` entry that `storage:list-entries` shows.
+| Token | Meaning |
+|---|---|
+| `ro` | Mount the volume read-only |
+| `rw` | Mount it read-write, the explicit opposite of `ro` and the default |
+| `volume-subpath=<path>` | Subpath within the entry |
+| `volume-chown=<option>` | Chown option applied at mount time |
+| `phase=<deploy\|run>` | Phase to mount in, repeated once per phase; both phases when omitted |
+| anything else | A Docker mount option, passed through verbatim in the order given |
 
-The remaining mount-time flags scope the whole call rather than a single pair, so every mount in the replacement shares them:
+Order within the list does not matter, and the keys mirror the flags the single-entry form uses so each attribute has exactly one spelling. Phases are stored in `deploy,run` order however they were written, so re-issuing an equivalent call converges rather than registering a change. A token value cannot contain a comma, since the comma is what separates tokens; a subpath needing one is set with the single-entry form. Note that `storage:list` renders the Docker `-v` view and omits the subpath, chown and phases, so its output does not round-trip into `--replace` - `storage:report` shows every field.
+
+A pair whose first field starts with `/` is a legacy host path rather than an entry name, and registers its `legacy-<hash>` 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-<hash>` entry that `storage:list-entries` shows. Only the first field decides which entry a host path resolves to, so adding tokens to a pair never moves it onto a different one.
+
+Because each attribute has exactly one spelling in this form, `--phase`, `--volume-subpath`, `--volume-readonly`, `--volume-chown` and `--volume-options` are rejected with `--replace` rather than acting as call-level defaults a token overrides:
 
 ```shell
-dokku storage:mount --replace node-js-app node-js-data:/app/storage --phase deploy --volume-subpath uploads --volume-chown herokuish
+dokku storage:mount --replace node-js-app node-js-data:/app/storage --volume-subpath uploads
 ```
 
-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.
+```
+ !     The --volume-subpath flag cannot be used with --replace; set volume-subpath=<path> in the mount spec instead
+```
 
-`--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:
+The single-entry form keeps all five flags.
+
+`--process-type` is the exception and scopes the replacement the same way it scopes a single mount, since it selects which mounts the call is authoritative over rather than describing any one of them. 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.
+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. So are the option list's own mistakes: an unknown key, a key given more than once, an empty value, an empty option, `ro` and `rw` together, a phase that is neither `deploy` nor `run`, and a chown value the plugin does not understand. Rejecting an unknown key is what catches a typo like `volume-subpth=uploads`, which would otherwise be stored as a Docker mount option and surface only when the next deploy failed.
 
 > [!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.
@@ -356,7 +373,7 @@ 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
+> Removing more than one attachment at a time is new as of 0.38.29
 
 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:
 
@@ -374,7 +391,7 @@ dokku storage:unmount node-js-app node-js-data:/app/storage node-js-data:/app/up
 #### Removing every mount
 
 > [!IMPORTANT]
-> New as of 0.38.28
+> New as of 0.38.29
 
 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:
 

plugins/storage/mounts_test.go

@@ -21,6 +21,12 @@ func TestParseMountSpec(t *testing.T) {
 	Expect(parsed.Readonly).To(BeTrue())
 	Expect(parsed.VolumeOptions).To(Equal("noexec,nosuid"))
 
+	// A trailing colon is the no-options case, not an empty option.
+	parsed, err = parseMountSpec("demo-data:/app/storage:")
+	Expect(err).NotTo(HaveOccurred())
+	Expect(parsed.VolumeOptions).To(BeEmpty())
+	Expect(parsed.Phases).To(Equal([]string{PhaseDeploy, PhaseRun}))
+
 	for _, spec := range []string{"demo-data", ":/app/storage", ""} {
 		_, err := parseMountSpec(spec)
 		Expect(err).To(HaveOccurred(), "expected %q to be rejected", spec)
@@ -28,6 +34,132 @@ func TestParseMountSpec(t *testing.T) {
 	}
 }
 
+// TestParseMountSpecTokens covers the key=value tokens the option list carries
+// on top of the bare docker options it has always passed through. Order within
+// the list is not significant, so the cases deliberately mix the two.
+func TestParseMountSpecTokens(t *testing.T) {
+	RegisterTestingT(t)
+
+	for _, testCase := range []struct {
+		spec     string
+		expected mountSpec
+	}{
+		{
+			spec:     "demo-data:/app/storage",
+			expected: mountSpec{Phases: []string{PhaseDeploy, PhaseRun}},
+		},
+		{
+			spec:     "demo-data:/app/storage:rw",
+			expected: mountSpec{Phases: []string{PhaseDeploy, PhaseRun}},
+		},
+		{
+			spec:     "demo-data:/app/storage:volume-subpath=uploads",
+			expected: mountSpec{Phases: []string{PhaseDeploy, PhaseRun}, Subpath: "uploads"},
+		},
+		{
+			spec:     "demo-data:/app/storage:volume-chown=herokuish",
+			expected: mountSpec{Phases: []string{PhaseDeploy, PhaseRun}, VolumeChown: "herokuish"},
+		},
+		{
+			spec:     "demo-data:/app/storage:phase=deploy",
+			expected: mountSpec{Phases: []string{PhaseDeploy}},
+		},
+		{
+			spec:     "demo-data:/app/storage:phase=deploy,phase=run",
+			expected: mountSpec{Phases: []string{PhaseDeploy, PhaseRun}},
+		},
+		{
+			spec: "demo-data:/app/storage:ro,Z,volume-subpath=uploads,volume-chown=herokuish",
+			expected: mountSpec{
+				Phases:        []string{PhaseDeploy, PhaseRun},
+				Subpath:       "uploads",
+				Readonly:      true,
+				VolumeOptions: "Z",
+				VolumeChown:   "herokuish",
+			},
+		},
+		{
+			// The same fields written in a different order, with the
+			// remaining bare options keeping the order they were given in.
+			spec: "demo-data:/app/storage:volume-chown=herokuish,noexec,phase=run,nosuid,ro,volume-subpath=uploads",
+			expected: mountSpec{
+				Phases:        []string{PhaseRun},
+				Subpath:       "uploads",
+				Readonly:      true,
+				VolumeOptions: "noexec,nosuid",
+				VolumeChown:   "herokuish",
+			},
+		},
+	} {
+		parsed, err := parseMountSpec(testCase.spec)
+		Expect(err).NotTo(HaveOccurred(), "expected %q to be accepted", testCase.spec)
+
+		expected := testCase.expected
+		expected.EntryName = "demo-data"
+		expected.ContainerPath = "/app/storage"
+		Expect(parsed).To(Equal(expected), "parsing %q", testCase.spec)
+	}
+}
+
+// TestParseMountSpecNormalizesPhaseOrder pins phases to a canonical order, so
+// two specs naming the same phases store identical JSON and render identically
+// in storage:report rather than looking like a change to a declarative caller.
+func TestParseMountSpecNormalizesPhaseOrder(t *testing.T) {
+	RegisterTestingT(t)
+
+	for _, spec := range []string{
+		"demo-data:/app/storage:phase=run,phase=deploy",
+		"demo-data:/app/storage:phase=deploy,phase=run",
+	} {
+		parsed, err := parseMountSpec(spec)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(parsed.Phases).To(Equal([]string{PhaseDeploy, PhaseRun}), "parsing %q", spec)
+	}
+}
+
+// TestParseMountSpecAllowsSeparatorsInValues documents that only a comma ends
+// a token value: the spec splits on the first two colons and a token on its
+// first equals sign, so both characters survive inside a value.
+func TestParseMountSpecAllowsSeparatorsInValues(t *testing.T) {
+	RegisterTestingT(t)
+
+	parsed, err := parseMountSpec("demo-data:/app/storage:volume-subpath=a:b")
+	Expect(err).NotTo(HaveOccurred())
+	Expect(parsed.Subpath).To(Equal("a:b"))
+
+	parsed, err = parseMountSpec("demo-data:/app/storage:volume-subpath=a=b")
+	Expect(err).NotTo(HaveOccurred())
+	Expect(parsed.Subpath).To(Equal("a=b"))
+}
+
+func TestParseMountSpecRejectsInvalidTokens(t *testing.T) {
+	RegisterTestingT(t)
+
+	for _, testCase := range []struct {
+		spec     string
+		contains string
+	}{
+		{"demo-data:/app/storage:volume-subpth=uploads", `specifies unknown key "volume-subpth"`},
+		{"demo-data:/app/storage:volume-subpath=", "has an empty value for volume-subpath"},
+		{"demo-data:/app/storage:volume-chown=", "has an empty value for volume-chown"},
+		{"demo-data:/app/storage:phase=", "has an empty value for phase"},
+		{"demo-data:/app/storage:ro,,Z", "has an empty mount option"},
+		{"demo-data:/app/storage:volume-subpath=a,volume-subpath=b", "specifies volume-subpath more than once"},
+		{"demo-data:/app/storage:volume-chown=root,volume-chown=false", "specifies volume-chown more than once"},
+		{"demo-data:/app/storage:phase=run,phase=run", "specifies phase run more than once"},
+		{"demo-data:/app/storage:ro,rw", "sets both ro and rw"},
+		{"demo-data:/app/storage:phase=build", `specifies unsupported phase "build"`},
+		{"demo-data:/app/storage:volume-chown=bogus", "Unsupported chown permissions"},
+	} {
+		_, err := parseMountSpec(testCase.spec)
+		Expect(err).To(HaveOccurred(), "expected %q to be rejected", testCase.spec)
+		Expect(err.Error()).To(ContainSubstring(testCase.contains), "rejecting %q", testCase.spec)
+		// Every rejection names the spec it came from, since a --replace
+		// call can carry any number of them.
+		Expect(err.Error()).To(ContainSubstring(testCase.spec), "rejecting %q", testCase.spec)
+	}
+}
+
 func TestValidateChownOption(t *testing.T) {
 	RegisterTestingT(t)
 
@@ -126,56 +258,162 @@ func TestCommandMountReplaceLeavesOtherProcessTypesUntouched(t *testing.T) {
 	Expect(attachments[1].ProcessType).To(Equal(DefaultProcessType))
 }
 
-func TestCommandMountReplaceAppliesScopeFlags(t *testing.T) {
+// TestCommandMountReplaceVariesFieldsPerSpec is the point of the token
+// grammar: every mount-time field except the process type is declared by the
+// spec that carries it, so one call can give two mounts different values.
+func TestCommandMountReplaceVariesFieldsPerSpec(t *testing.T) {
 	RegisterTestingT(t)
 	setupTestApp(t, "demo")
 	stageDockerLocalEntry(t, "demo-data")
 	stageDockerLocalEntry(t, "demo-cache")
 
 	Expect(CommandMount(CommandMountInput{
-		AppName:     "demo",
+		AppName: "demo",
-		Replace:     true,
+		Replace: true,
-		Specs:       []string{"demo-data:/app/storage:Z", "demo-cache:/cache:ro,noexec"},
+		Specs: []string{
-		Phases:      []string{PhaseDeploy},
+			"demo-data:/app/storage:Z,volume-subpath=uploads,volume-chown=herokuish",
+			"demo-cache:/cache:ro,noexec,phase=deploy",
+		},
 		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())
+	// --process-type is the one field that still scopes the whole call.
+	Expect(attachments[0].ProcessType).To(Equal("web"))
+	Expect(attachments[1].ProcessType).To(Equal("web"))
+
+	Expect(attachments[0].Phases).To(Equal([]string{PhaseDeploy, PhaseRun}))
+	Expect(attachments[0].Subpath).To(Equal("uploads"))
+	Expect(attachments[0].VolumeChown).To(Equal("herokuish"))
 	Expect(attachments[0].VolumeOptions).To(Equal("Z"))
-	Expect(attachments[1].Readonly).To(BeTrue())
+	Expect(attachments[0].Readonly).To(BeFalse())
+
+	Expect(attachments[1].Phases).To(Equal([]string{PhaseDeploy}))
+	Expect(attachments[1].Subpath).To(BeEmpty())
+	Expect(attachments[1].VolumeChown).To(BeEmpty())
 	Expect(attachments[1].VolumeOptions).To(Equal("noexec"))
+	Expect(attachments[1].Readonly).To(BeTrue())
 }
 
-func TestCommandMountReplaceReadonlyFlagAppliesToEverySpec(t *testing.T) {
+// TestCommandMountReplaceReadonlyIsPerSpec covers the ro/rw pair, including
+// that rw is consumed into the attachment rather than left in VolumeOptions,
+// where it would be handed to docker as a mount option.
+func TestCommandMountReplaceReadonlyIsPerSpec(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:ro", "demo-cache:/cache:rw", "demo-logs:/logs"},
+	})).To(Succeed())
+
+	attachments, err := LoadAttachments("demo")
+	Expect(err).NotTo(HaveOccurred())
+	Expect(attachments).To(HaveLen(3))
+	Expect(attachments[0].Readonly).To(BeTrue())
+	Expect(attachments[1].Readonly).To(BeFalse())
+	Expect(attachments[1].VolumeOptions).To(BeEmpty())
+	Expect(attachments[2].Readonly).To(BeFalse())
+}
+
+// TestCommandMountReplaceDefaultsPhasesWithoutAToken pins the default a spec
+// without a phase token gets, which is the same pair a mount has always had.
+func TestCommandMountReplaceDefaultsPhasesWithoutAToken(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:Z"},
+	})).To(Succeed())
+
+	attachments, err := LoadAttachments("demo")
+	Expect(err).NotTo(HaveOccurred())
+	Expect(attachments).To(HaveLen(1))
+	Expect(attachments[0].Phases).To(Equal([]string{PhaseDeploy, PhaseRun}))
+}
+
+// TestCommandMountReplaceRejectsMountTimeFlags covers the other half of "one
+// spelling per field": a flag that a token now expresses is refused rather
+// than silently acting as a default. The --volume-options row is the case that
+// was previously accepted and then dropped on the floor.
+func TestCommandMountReplaceRejectsMountTimeFlags(t *testing.T) {
+	RegisterTestingT(t)
+	setupTestApp(t, "demo")
+	stageDockerLocalEntry(t, "demo-data")
+
+	for _, testCase := range []struct {
+		name     string
+		input    CommandMountInput
+		contains string
+	}{
+		{"--phase", CommandMountInput{Phases: []string{PhaseDeploy}}, "The --phase flag cannot be used with --replace; set phase=deploy in the mount spec instead"},
+		{"--volume-subpath", CommandMountInput{Subpath: "uploads"}, "The --volume-subpath flag cannot be used with --replace; set volume-subpath=<path> in the mount spec instead"},
+		{"--volume-readonly", CommandMountInput{Readonly: true}, "The --volume-readonly flag cannot be used with --replace; set ro in the mount spec instead"},
+		{"--volume-chown", CommandMountInput{VolumeChown: "herokuish"}, "The --volume-chown flag cannot be used with --replace; set volume-chown=<option> in the mount spec instead"},
+		{"--volume-options", CommandMountInput{VolumeOptions: "Z"}, "The --volume-options flag cannot be used with --replace; set the option as a bare token in the mount spec instead"},
+	} {
+		input := testCase.input
+		input.AppName = "demo"
+		input.Replace = true
+		input.Specs = []string{"demo-data:/app/storage"}
+
+		err := CommandMount(input)
+		Expect(err).To(HaveOccurred(), "expected %s to be rejected", testCase.name)
+		Expect(err.Error()).To(Equal(testCase.contains), "rejecting %s", testCase.name)
+
+		attachments, err := LoadAttachments("demo")
+		Expect(err).NotTo(HaveOccurred())
+		Expect(attachments).To(BeEmpty(), "rejecting %s wrote an attachment", testCase.name)
+	}
+}
+
+// TestCommandMountReplaceValidatesTokensPerSpec keeps the all-or-nothing
+// contract over the failures the token grammar adds: a bad token in a later
+// spec leaves an earlier one unwritten and the stored set as it was.
+func TestCommandMountReplaceValidatesTokensPerSpec(t *testing.T) {
 	RegisterTestingT(t)
 	setupTestApp(t, "demo")
 	stageDockerLocalEntry(t, "demo-data")
 	stageDockerLocalEntry(t, "demo-cache")
 
 	Expect(CommandMount(CommandMountInput{
-		AppName:  "demo",
+		AppName:      "demo",
-		Replace:  true,
+		NameOrPath:   "demo-data",
-		Specs:    []string{"demo-data:/app/storage", "demo-cache:/cache"},
+		ContainerDir: "/app/storage",
-		Readonly: true,
 	})).To(Succeed())
 
-	attachments, err := LoadAttachments("demo")
+	for _, testCase := range []struct {
-	Expect(err).NotTo(HaveOccurred())
+		spec     string
-	Expect(attachments).To(HaveLen(2))
+		contains string
-	Expect(attachments[0].Readonly).To(BeTrue())
+	}{
-	Expect(attachments[1].Readonly).To(BeTrue())
+		{"demo-cache:/cache:volume-chown=bogus", "Unsupported chown permissions"},
+		{"demo-cache:/cache:phase=build", `specifies unsupported phase "build"`},
+		{"demo-cache:/cache:volume-subpth=uploads", `specifies unknown key "volume-subpth"`},
+	} {
+		err := CommandMount(CommandMountInput{
+			AppName: "demo",
+			Replace: true,
+			Specs:   []string{"demo-cache:/cache2", testCase.spec},
+		})
+		Expect(err).To(HaveOccurred(), "expected %q to be rejected", testCase.spec)
+		Expect(err.Error()).To(ContainSubstring(testCase.contains))
+		Expect(err.Error()).To(ContainSubstring(testCase.spec))
+
+		attachments, err := LoadAttachments("demo")
+		Expect(err).NotTo(HaveOccurred())
+		Expect(attachments).To(HaveLen(1), "rejecting %q changed the stored set", testCase.spec)
+		Expect(attachments[0].EntryName).To(Equal("demo-data"))
+	}
 }
 
 // TestCommandMountReplaceRejectsPathHeldByAnotherScope covers the scopes a
@@ -410,6 +648,31 @@ func TestCommandMountReplaceAcceptsLegacyHostPath(t *testing.T) {
 	Expect(EntryExists(expected.Name)).To(BeTrue())
 }
 
+// TestCommandMountReplaceAcceptsLegacyHostPathWithTokens guards the entry
+// hash: LegacyMountToEntry only ever sees the first colon field, so a token in
+// the third one must not move a host path onto a different legacy-<hash>
+// entry than the same path without it.
+func TestCommandMountReplaceAcceptsLegacyHostPathWithTokens(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:ro,volume-subpath=uploads"},
+	})).To(Succeed())
+
+	attachments, err := LoadAttachments("demo")
+	Expect(err).NotTo(HaveOccurred())
+	Expect(attachments).To(HaveLen(1))
+	Expect(attachments[0].Subpath).To(Equal("uploads"))
+	Expect(attachments[0].Readonly).To(BeTrue())
+
+	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")
@@ -456,6 +719,33 @@ func TestCommandUnmountAcceptsEntryColonContainerPath(t *testing.T) {
 	Expect(attachments[0].ContainerPath).To(Equal("/app/storage"))
 }
 
+// TestCommandUnmountIgnoresMountSpecTokens keeps a mount spec pasteable into
+// an unmount call. An unmount target is identified by entry and container path
+// alone, and the third field has always been ignored here - including "ro" -
+// so the tokens that field now also carries are ignored the same way rather
+// than being rejected only because they are newer.
+func TestCommandUnmountIgnoresMountSpecTokens(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:ro,volume-subpath=uploads"},
+	})).To(Succeed())
+
+	Expect(CommandUnmount(CommandUnmountInput{
+		AppName: "demo",
+		Mounts:  []string{"demo-data:/app/uploads:ro,volume-subpath=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")

plugins/storage/mountspec.go

@@ -0,0 +1,164 @@
+package storage
+
+import (
+	"fmt"
+	"strings"
+)
+
+// The key=value tokens a storage:mount --replace option list recognizes. Each
+// one mirrors the flag the single-entry form uses, so every mount-time field
+// has exactly one spelling across the two forms. Readonly keeps its docker
+// spelling - the bare "ro" token this grammar already had, plus "rw" as its
+// explicit opposite - rather than gaining a key of its own.
+const (
+	mountSpecKeyPhase       = "phase"
+	mountSpecKeySubpath     = "volume-subpath"
+	mountSpecKeyVolumeChown = "volume-chown"
+)
+
+// mountSpec is one parsed positional argument of the storage:mount --replace
+// form. Every mount-time field an attachment carries except ProcessType is
+// expressible per spec, so one call can vary them between mounts.
+type mountSpec struct {
+	EntryName     string
+	ContainerPath string
+	Phases        []string
+	Subpath       string
+	Readonly      bool
+	VolumeOptions string
+	VolumeChown   string
+}
+
+// parseMountSpec splits an "<entry>:<container-dir>[:<options>]" argument,
+// where the first field names a storage entry rather than a host path and the
+// option list carries every mount-time field rather than only the docker ones.
+//
+// It deliberately does not route through ParseMountPath, which hoists "ro" out
+// of the list before any caller sees it. Keeping the whole grammar here leaves
+// the ro/rw pair as a single rule in a single place, lets an error quote the
+// token list the operator actually typed, and keeps the legacy colon form,
+// storage:unmount and the migration on the grammar they already had.
+func parseMountSpec(spec string) (mountSpec, error) {
+	parts := strings.SplitN(spec, ":", 3)
+	parsed := mountSpec{Phases: []string{PhaseDeploy, PhaseRun}}
+	if len(parts) >= 1 {
+		parsed.EntryName = parts[0]
+	}
+	if len(parts) >= 2 {
+		parsed.ContainerPath = parts[1]
+	}
+
+	if parsed.EntryName == "" || parsed.ContainerPath == "" {
+		return mountSpec{}, fmt.Errorf("Invalid mount specified: %s", spec)
+	}
+
+	// A trailing colon is the no-options case rather than an empty option.
+	if len(parts) >= 3 && parts[2] != "" {
+		if err := parseMountSpecOptions(&parsed, spec, parts[2]); err != nil {
+			return mountSpec{}, err
+		}
+	}
+
+	return parsed, nil
+}
+
+// parseMountSpecOptions consumes a spec's comma-separated option list, lifting
+// the recognized tokens into their mount-time fields and rejoining what is left
+// into VolumeOptions the way the colon form always has.
+//
+// Order within the list is not significant, so anything the list can say twice
+// is rejected rather than resolved by position, and an unrecognized key is
+// rejected rather than passed through - a typo that reaches VolumeOptions would
+// otherwise surface only when docker refuses the container at deploy time.
+func parseMountSpecOptions(parsed *mountSpec, spec string, options string) error {
+	sawReadonly := false
+	sawWritable := false
+	seenKeys := map[string]bool{}
+	seenPhases := map[string]bool{}
+	remaining := []string{}
+
+	for _, token := range strings.Split(options, ",") {
+		if token == "" {
+			return fmt.Errorf("Mount spec %q has an empty mount option", spec)
+		}
+
+		switch token {
+		case "ro":
+			sawReadonly = true
+			continue
+		case "rw":
+			sawWritable = true
+			continue
+		}
+
+		// A bare docker option carries no "=", and neither does a token
+		// starting with one, which is no key at all. Both pass through.
+		index := strings.Index(token, "=")
+		if index <= 0 {
+			remaining = append(remaining, token)
+			continue
+		}
+
+		key, value := token[:index], token[index+1:]
+		if key != mountSpecKeyPhase && key != mountSpecKeySubpath && key != mountSpecKeyVolumeChown {
+			return fmt.Errorf("Mount spec %q specifies unknown key %q; supported keys are %s, %s and %s",
+				spec, key, mountSpecKeyPhase, mountSpecKeyVolumeChown, mountSpecKeySubpath)
+		}
+
+		if value == "" {
+			return fmt.Errorf("Mount spec %q has an empty value for %s", spec, key)
+		}
+
+		// A phase names one of a pair and so appears once per phase; every
+		// other key names a single value and so appears at most once.
+		if key != mountSpecKeyPhase {
+			if seenKeys[key] {
+				return fmt.Errorf("Mount spec %q specifies %s more than once", spec, key)
+			}
+			seenKeys[key] = true
+		}
+
+		switch key {
+		case mountSpecKeyPhase:
+			if value != PhaseDeploy && value != PhaseRun {
+				return fmt.Errorf("Mount spec %q specifies unsupported phase %q (must be %q or %q)", spec, value, PhaseDeploy, PhaseRun)
+			}
+			if seenPhases[value] {
+				return fmt.Errorf("Mount spec %q specifies phase %s more than once", spec, value)
+			}
+			seenPhases[value] = true
+		case mountSpecKeySubpath:
+			parsed.Subpath = value
+		case mountSpecKeyVolumeChown:
+			if err := ValidateChownOption(value); err != nil {
+				return fmt.Errorf("Mount spec %q has an invalid %s value %q: %w", spec, key, value, err)
+			}
+			parsed.VolumeChown = value
+		}
+	}
+
+	if sawReadonly && sawWritable {
+		return fmt.Errorf("Mount spec %q sets both ro and rw", spec)
+	}
+
+	parsed.Readonly = sawReadonly
+	parsed.VolumeOptions = strings.Join(remaining, ",")
+	if len(seenPhases) > 0 {
+		parsed.Phases = canonicalPhases(seenPhases)
+	}
+
+	return nil
+}
+
+// canonicalPhases renders a phase set in a fixed order, so two spec strings
+// naming the same phases in a different order store identical JSON and render
+// identically in storage:report.
+func canonicalPhases(seen map[string]bool) []string {
+	phases := []string{}
+	for _, phase := range []string{PhaseDeploy, PhaseRun} {
+		if seen[phase] {
+			phases = append(phases, phase)
+		}
+	}
+	return phases
+}

plugins/storage/src/subcommands/subcommands.go

@@ -218,12 +218,12 @@ func main() {
 	case "mount":
 		args := flag.NewFlagSet("storage:mount", flag.ExitOnError)
 		containerDir := args.String("container-dir", "", "--container-dir: container path (named-entry form)")
-		phases := args.StringSlice("phase", nil, "--phase: phase to mount in (deploy, run; default both)")
+		phases := args.StringSlice("phase", nil, "--phase: phase to mount in (deploy, run; default both); not valid with --replace")
 		processType := args.String("process-type", "", "--process-type: process type to mount for")
-		subpath := args.String("volume-subpath", "", "--volume-subpath: subpath within the entry")
+		subpath := args.String("volume-subpath", "", "--volume-subpath: subpath within the entry; not valid with --replace")
-		readonly := args.Bool("volume-readonly", false, "--volume-readonly: mount the volume read-only")
+		readonly := args.Bool("volume-readonly", false, "--volume-readonly: mount the volume read-only; not valid with --replace")
-		volumeChown := args.String("volume-chown", "", "--volume-chown: chown option applied at mount time")
+		volumeChown := args.String("volume-chown", "", "--volume-chown: chown option applied at mount time; not valid with --replace")
-		volumeOptions := args.String("volume-options", "", "--volume-options: comma-separated mount options (e.g. Z, noexec,nosuid)")
+		volumeOptions := args.String("volume-options", "", "--volume-options: comma-separated mount options (e.g. Z, noexec,nosuid); not valid with --replace")
 		replace := args.Bool("replace", false, "--replace: replace the entire mount set for the process type")
 		args.Parse(os.Args[2:])
 		input := storage.CommandMountInput{

plugins/storage/subcommands.go

@@ -152,8 +152,9 @@ func isUserNamespacesEnabled() (bool, error) {
 // 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
+// are mount specs carrying their own mount-time fields. Only ProcessType
-// fields scope every spec in the call.
+// scopes that call; every other mount-time field is rejected with Replace so
+// each one has exactly one spelling.
 type CommandMountInput struct {
 	AppName       string
 	NameOrPath    string
@@ -252,34 +253,6 @@ func CommandMount(input CommandMountInput) error {
 	return nil
 }
 
-// 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 "<entry>:<container-dir>[:<options>]" 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
@@ -329,24 +302,40 @@ func resolveMountSpecEntry(appName string, appScheduler string, spec string, par
 // 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.
+//
+// Every other mount-time field comes from the spec that declares it rather
+// than from a call-level flag, so one call can vary them between mounts.
 func replaceMounts(input CommandMountInput) error {
 	if input.ContainerDir != "" {
 		return errors.New("The --container-dir flag cannot be used with --replace")
 	}
 
+	// Each mount-time field has exactly one spelling in this form, the token
+	// in the spec that declares it, rather than a call-level default a token
+	// overrides. A flag is taken as passed when it holds anything but its
+	// zero value, which is what pflag leaves behind when it is absent; the
+	// values that slip through - an empty string or an explicit false - ask
+	// for what an omitted flag already gives.
+	for _, conflict := range []struct {
+		flag string
+		set  bool
+		hint string
+	}{
+		{"--phase", len(input.Phases) > 0, mountSpecKeyPhase + "=" + PhaseDeploy},
+		{"--volume-subpath", input.Subpath != "", mountSpecKeySubpath + "=<path>"},
+		{"--volume-readonly", input.Readonly, "ro"},
+		{"--volume-chown", input.VolumeChown != "", mountSpecKeyVolumeChown + "=<option>"},
+		{"--volume-options", input.VolumeOptions != "", "the option as a bare token"},
+	} {
+		if conflict.set {
+			return fmt.Errorf("The %s flag cannot be used with --replace; set %s in the mount spec instead", conflict.flag, conflict.hint)
+		}
+	}
+
 	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
@@ -378,12 +367,12 @@ func replaceMounts(input CommandMountInput) error {
 		attachment := &Attachment{
 			EntryName:     entry.Name,
 			ContainerPath: parsed.ContainerPath,
-			Phases:        phases,
+			Phases:        parsed.Phases,
 			ProcessType:   processType,
-			Subpath:       input.Subpath,
+			Subpath:       parsed.Subpath,
-			Readonly:      input.Readonly || parsed.Readonly,
+			Readonly:      parsed.Readonly,
 			VolumeOptions: parsed.VolumeOptions,
-			VolumeChown:   input.VolumeChown,
+			VolumeChown:   parsed.VolumeChown,
 		}
 		if err := attachment.Validate(); err != nil {
 			return err

tests/unit/storage-3.bats

@@ -102,13 +102,13 @@ teardown() {
   assert_success
 }
 
-@test "(storage:mount) --replace applies scope flags to every spec" {
+@test "(storage:mount) --replace varies mount-time fields per 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"
+  run /bin/bash -c "dokku storage:mount --replace $TEST_APP rdmtest-data:/app/storage:Z,volume-subpath=uploads,volume-chown=herokuish rdmtest-cache:/cache:ro,noexec,phase=deploy --process-type web"
   echo "output: $output"
   echo "status: $status"
   assert_success
@@ -117,7 +117,7 @@ teardown() {
   echo "output: $output"
   echo "status: $status"
   assert_success
-  assert_output "deploy"
+  assert_output "deploy,run"
 
   run /bin/bash -c "dokku storage:report $TEST_APP --format json | jq -r '.\"attachment.1.subpath\"'"
   echo "output: $output"
@@ -137,6 +137,24 @@ teardown() {
   assert_success
   assert_output "Z"
 
+  run /bin/bash -c "dokku storage:report $TEST_APP --format json | jq -r '.\"attachment.2.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.2.subpath\"'"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+  assert_output ""
+
+  run /bin/bash -c "dokku storage:report $TEST_APP --format json | jq -r '.\"attachment.2.volume-chown\"'"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+  assert_output ""
+
   run /bin/bash -c "dokku storage:report $TEST_APP --format json | jq -r '.\"attachment.2.readonly\"'"
   echo "output: $output"
   echo "status: $status"
@@ -149,6 +167,139 @@ teardown() {
   assert_success
   assert_output "noexec"
 
+  run /bin/bash -c "dokku storage:report $TEST_APP --format json | jq -r '.\"attachment.1.process-type\", .\"attachment.2.process-type\"' | sort -u | xargs"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+  assert_output "web"
+
+  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 mount-time flags" {
+  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
+
+  for flag in "--phase deploy" "--volume-subpath uploads" "--volume-readonly" "--volume-chown herokuish" "--volume-options Z"; do
+    run /bin/bash -c "dokku storage:mount --replace $TEST_APP rdmtest-cache:/cache $flag"
+    echo "flag: $flag"
+    echo "output: $output"
+    echo "status: $status"
+    assert_failure
+    assert_output_contains "cannot be used with --replace" -1
+  done
+
+  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 malformed spec options" {
+  run /bin/bash -c "dokku storage:create rdmtest-data"
+  assert_success
+
+  run /bin/bash -c "dokku storage:mount --replace $TEST_APP rdmtest-data:/app/storage:volume-subpth=uploads"
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+  assert_output_contains "unknown key" -1
+
+  run /bin/bash -c "dokku storage:mount --replace $TEST_APP rdmtest-data:/app/storage:volume-subpath="
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+  assert_output_contains "empty value" -1
+
+  run /bin/bash -c "dokku storage:mount --replace $TEST_APP rdmtest-data:/app/storage:volume-subpath=a,volume-subpath=b"
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+  assert_output_contains "more than once" -1
+
+  run /bin/bash -c "dokku storage:mount --replace $TEST_APP rdmtest-data:/app/storage:ro,rw"
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+  assert_output_contains "sets both ro and rw" -1
+
+  run /bin/bash -c "dokku storage:mount --replace $TEST_APP rdmtest-data:/app/storage:phase=build"
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+  assert_output_contains "unsupported phase" -1
+
+  run /bin/bash -c "dokku storage:mount --replace $TEST_APP rdmtest-data:/app/storage:volume-chown=bogus"
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+  assert_output_contains "Unsupported chown permissions" -1
+
+  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:destroy rdmtest-data --force"
+  assert_success
+}
+
+@test "(storage:mount) --replace rw leaves the option list alone" {
+  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:rw rdmtest-cache:/cache:rw,Z"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+
+  run /bin/bash -c "dokku storage:report $TEST_APP --format json | jq -r '.\"attachment.1.readonly\"'"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+  assert_output "false"
+
+  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 ""
+
+  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 "Z"
+
+  run /bin/bash -c "dokku storage:report $TEST_APP --storage-deploy-mounts"
+  echo "output: $output"
+  echo "status: $status"
+  assert_success
+  assert_output_not_contains "rw"
+
   run /bin/bash -c "dokku storage:unmount --all $TEST_APP"
   assert_success
   run /bin/bash -c "dokku storage:destroy rdmtest-data --force"