From e9160db769c92e29ddf51be9e73199d2874154b4 Mon Sep 17 00:00:00 2001 From: Jose Diaz-Gonzalez Date: Sat, 30 May 2026 03:00:21 -0400 Subject: [PATCH] fix: expose attachment readonly and volume_options in storage:list json The `storage:list --format json` payload conflated the attachment's `Readonly` flag and its `VolumeOptions` field into a single derived `volume_options` string that rendered as `ro`, ``, or `ro,` depending on which fields were set on the underlying attachment. That shape is fine for the legacy `host:container[:options]` text view but it leaves drift-detection tooling unable to tell whether a `ro` token came from `Attachment.Readonly == true` or from the operator setting `Attachment.VolumeOptions = "ro"` directly. The JSON now exposes `readonly` (boolean) and `volume_options` (string) as separate omitempty keys populated straight from the attachment, and `formatStorageListEntry` combines them at format time for the colon- form text view. `ParseMountPath` was extended in lockstep so callers of the legacy `host:container:opts` form no longer have to special- case the `ro` token themselves. --- docs/advanced-usage/persistent-storage.md | 16 ++++++- plugins/storage/attachment_test.go | 20 ++++++++- plugins/storage/migrate.go | 9 +--- plugins/storage/storage.go | 55 +++++++++++++++-------- plugins/storage/storage_test.go | 21 ++++++++- plugins/storage/subcommands.go | 9 +--- tests/unit/storage.bats | 26 ++++++++++- 7 files changed, 118 insertions(+), 38 deletions(-) diff --git a/docs/advanced-usage/persistent-storage.md b/docs/advanced-usage/persistent-storage.md index 76aef20b4..f7eecb808 100644 --- a/docs/advanced-usage/persistent-storage.md +++ b/docs/advanced-usage/persistent-storage.md @@ -57,9 +57,23 @@ dokku storage:list node-js-app --format json ``` [ { + "entry_name": "node-js-app", "host_path": "/var/lib/dokku/data/storage/node-js-app", + "container_path": "/app/storage" + } +] +``` + +Each entry mirrors the underlying attachment. `readonly` (boolean) and `volume_options` (string) reflect `Attachment.Readonly` and `Attachment.VolumeOptions` directly and are only present when set, so external tooling can drift-detect attachments against the raw attachment fields. For example, a mount created with `--volume-options noexec,nosuid --volume-readonly` renders as: + +``` +[ + { + "entry_name": "node-js-data", + "host_path": "/var/lib/dokku/data/storage/node-js-data", "container_path": "/app/storage", - "volume_options": "" + "readonly": true, + "volume_options": "noexec,nosuid" } ] ``` diff --git a/plugins/storage/attachment_test.go b/plugins/storage/attachment_test.go index 035900cd5..63cd9889d 100644 --- a/plugins/storage/attachment_test.go +++ b/plugins/storage/attachment_test.go @@ -85,7 +85,8 @@ func TestListAppMountEntriesDockerLocal(t *testing.T) { Expect(rows[0].EntryName).To(Equal("demo-data")) Expect(rows[0].HostPath).To(Equal("/var/lib/dokku/data/storage/demo-data")) Expect(rows[0].ContainerPath).To(Equal("/data")) - Expect(rows[0].VolumeOptions).To(Equal("ro")) + Expect(rows[0].Readonly).To(BeTrue()) + Expect(rows[0].VolumeOptions).To(BeEmpty()) // Run phase shows it too because the attachment includes both phases. runRows, err := ListAppMountEntries("demo", PhaseRun) @@ -164,11 +165,26 @@ func TestListAppMountEntriesVolumeOptions(t *testing.T) { byPath[row.ContainerPath] = row } + Expect(byPath["/data"].Readonly).To(BeFalse()) Expect(byPath["/data"].VolumeOptions).To(Equal("Z")) Expect(formatStorageListEntry(byPath["/data"])).To(Equal("/var/lib/dokku/data/storage/demo-data:/data:Z")) - Expect(byPath["/ro"].VolumeOptions).To(Equal("ro,noexec,nosuid")) + Expect(byPath["/ro"].Readonly).To(BeTrue()) + Expect(byPath["/ro"].VolumeOptions).To(Equal("noexec,nosuid")) Expect(formatStorageListEntry(byPath["/ro"])).To(Equal("/var/lib/dokku/data/storage/demo-data:/ro:ro,noexec,nosuid")) + + // The wire shape exposes Readonly and VolumeOptions as separate + // keys so external drift-detection tooling can compare them + // against the underlying Attachment fields one for one. + dataJSON, err := json.Marshal(byPath["/data"]) + Expect(err).NotTo(HaveOccurred()) + Expect(string(dataJSON)).To(ContainSubstring(`"volume_options":"Z"`)) + Expect(string(dataJSON)).NotTo(ContainSubstring(`"readonly"`)) + + roJSON, err := json.Marshal(byPath["/ro"]) + Expect(err).NotTo(HaveOccurred()) + Expect(string(roJSON)).To(ContainSubstring(`"readonly":true`)) + Expect(string(roJSON)).To(ContainSubstring(`"volume_options":"noexec,nosuid"`)) } func TestListAppMountEntriesPhaseFilter(t *testing.T) { diff --git a/plugins/storage/migrate.go b/plugins/storage/migrate.go index 46bb8eba2..20c31909e 100644 --- a/plugins/storage/migrate.go +++ b/plugins/storage/migrate.go @@ -174,13 +174,8 @@ func migrateMount(appName string, mount string, phases []string) error { ContainerPath: containerPath, Phases: phases, ProcessType: DefaultProcessType, - } - if parsed.VolumeOptions != "" { - if parsed.VolumeOptions == "ro" { - attachment.Readonly = true - } else { - attachment.VolumeOptions = parsed.VolumeOptions - } + Readonly: parsed.Readonly, + VolumeOptions: parsed.VolumeOptions, } existing, err := LoadAttachments(appName) diff --git a/plugins/storage/storage.go b/plugins/storage/storage.go index 3054688be..edeab30cd 100644 --- a/plugins/storage/storage.go +++ b/plugins/storage/storage.go @@ -82,11 +82,17 @@ func GetBindMountsForDisplay(appName string, phase string) string { } // StorageListEntry represents a storage mount entry for JSON output. +// Readonly and VolumeOptions mirror the underlying Attachment fields one +// for one so external drift-detection tooling can compare against them +// directly; the combined "ro," colon-form string used by the text +// view is derived at format time by formatStorageListEntry rather than +// cached on the struct. type StorageListEntry struct { EntryName string `json:"entry_name,omitempty"` HostPath string `json:"host_path"` ContainerPath string `json:"container_path"` - VolumeOptions string `json:"volume_options"` + Readonly bool `json:"readonly,omitempty"` + VolumeOptions string `json:"volume_options,omitempty"` } // ListAppMountEntries returns one StorageListEntry per attachment on @@ -117,36 +123,41 @@ func ListAppMountEntries(appName string, phase string) ([]StorageListEntry, erro host = entry.Name } - options := "" - switch { - case attachment.Readonly && attachment.VolumeOptions != "": - options = "ro," + attachment.VolumeOptions - case attachment.Readonly: - options = "ro" - case attachment.VolumeOptions != "": - options = attachment.VolumeOptions - } - rows = append(rows, StorageListEntry{ EntryName: entry.Name, HostPath: host, ContainerPath: attachment.ContainerPath, - VolumeOptions: options, + Readonly: attachment.Readonly, + VolumeOptions: attachment.VolumeOptions, }) } return rows, nil } // formatStorageListEntry renders a StorageListEntry into the legacy -// host:container[:options] colon form for textual output. +// host:container[:options] colon form for textual output. Combines +// Readonly and VolumeOptions into a single "ro," token to match +// the historical shape consumers expect. func formatStorageListEntry(entry StorageListEntry) string { - if entry.VolumeOptions == "" { + options := "" + switch { + case entry.Readonly && entry.VolumeOptions != "": + options = "ro," + entry.VolumeOptions + case entry.Readonly: + options = "ro" + case entry.VolumeOptions != "": + options = entry.VolumeOptions + } + if options == "" { return fmt.Sprintf("%s:%s", entry.HostPath, entry.ContainerPath) } - return fmt.Sprintf("%s:%s:%s", entry.HostPath, entry.ContainerPath, entry.VolumeOptions) + return fmt.Sprintf("%s:%s:%s", entry.HostPath, entry.ContainerPath, options) } -// ParseMountPath parses a mount path into its components +// ParseMountPath parses a mount path into its components. The optional +// third colon-separated section is a comma-separated mount-options list; +// any "ro" token is hoisted into the Readonly field and the remaining +// tokens (preserving order) are rejoined into VolumeOptions. func ParseMountPath(mountPath string) StorageListEntry { parts := strings.SplitN(mountPath, ":", 3) entry := StorageListEntry{} @@ -157,8 +168,16 @@ func ParseMountPath(mountPath string) StorageListEntry { if len(parts) >= 2 { entry.ContainerPath = parts[1] } - if len(parts) >= 3 { - entry.VolumeOptions = parts[2] + if len(parts) >= 3 && parts[2] != "" { + remaining := []string{} + for _, token := range strings.Split(parts[2], ",") { + if token == "ro" { + entry.Readonly = true + continue + } + remaining = append(remaining, token) + } + entry.VolumeOptions = strings.Join(remaining, ",") } return entry diff --git a/plugins/storage/storage_test.go b/plugins/storage/storage_test.go index 305680d3c..871bdf53a 100644 --- a/plugins/storage/storage_test.go +++ b/plugins/storage/storage_test.go @@ -69,16 +69,35 @@ func TestParseMountPath(t *testing.T) { entry := ParseMountPath("/host/path:/container/path") Expect(entry.HostPath).To(Equal("/host/path")) Expect(entry.ContainerPath).To(Equal("/container/path")) + Expect(entry.Readonly).To(BeFalse()) Expect(entry.VolumeOptions).To(BeEmpty()) entry = ParseMountPath("/host/path:/container/path:ro") Expect(entry.HostPath).To(Equal("/host/path")) Expect(entry.ContainerPath).To(Equal("/container/path")) - Expect(entry.VolumeOptions).To(Equal("ro")) + Expect(entry.Readonly).To(BeTrue()) + Expect(entry.VolumeOptions).To(BeEmpty()) + + entry = ParseMountPath("/host/path:/container/path:Z") + Expect(entry.HostPath).To(Equal("/host/path")) + Expect(entry.ContainerPath).To(Equal("/container/path")) + Expect(entry.Readonly).To(BeFalse()) + Expect(entry.VolumeOptions).To(Equal("Z")) + + entry = ParseMountPath("/host/path:/container/path:ro,Z") + Expect(entry.HostPath).To(Equal("/host/path")) + Expect(entry.ContainerPath).To(Equal("/container/path")) + Expect(entry.Readonly).To(BeTrue()) + Expect(entry.VolumeOptions).To(Equal("Z")) + + entry = ParseMountPath("/host/path:/container/path:noexec,nosuid,ro") + Expect(entry.Readonly).To(BeTrue()) + Expect(entry.VolumeOptions).To(Equal("noexec,nosuid")) entry = ParseMountPath("volume_name:/container/path") Expect(entry.HostPath).To(Equal("volume_name")) Expect(entry.ContainerPath).To(Equal("/container/path")) + Expect(entry.Readonly).To(BeFalse()) Expect(entry.VolumeOptions).To(BeEmpty()) } diff --git a/plugins/storage/subcommands.go b/plugins/storage/subcommands.go index 8e0c91580..9001cd1fc 100644 --- a/plugins/storage/subcommands.go +++ b/plugins/storage/subcommands.go @@ -242,13 +242,8 @@ func mountLegacyColon(appName string, mountPath string) error { ContainerPath: parsed.ContainerPath, Phases: []string{PhaseDeploy, PhaseRun}, ProcessType: DefaultProcessType, - } - switch parsed.VolumeOptions { - case "": - case "ro": - attachment.Readonly = true - default: - attachment.VolumeOptions = parsed.VolumeOptions + Readonly: parsed.Readonly, + VolumeOptions: parsed.VolumeOptions, } if err := AddAttachment(appName, attachment); err != nil { diff --git a/tests/unit/storage.bats b/tests/unit/storage.bats index eba0b21f2..7e78f5848 100644 --- a/tests/unit/storage.bats +++ b/tests/unit/storage.bats @@ -137,11 +137,11 @@ teardown() { assert_success assert_output "/mount" - run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r '.[].volume_options'" + run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r 'map(has(\"volume_options\") or has(\"readonly\")) | any'" echo "output: $output" echo "status: $status" assert_success - assert_output_not_exists + assert_output "false" run /bin/bash -c "dokku storage:mount $TEST_APP /tmp/mount:/mount" echo "output: $output" @@ -386,6 +386,28 @@ teardown() { assert_success assert_output_contains ":ro,noexec,nosuid" + # The JSON output of storage:list exposes readonly and volume_options + # as separate keys so external drift-detection tooling can compare + # them against the underlying attachment fields one for one. + run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r '.[] | select(.container_path == \"/ro\") | .readonly'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "true" + + run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r '.[] | select(.container_path == \"/ro\") | .volume_options'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "noexec,nosuid" + + # The /data mount has no readonly flag, so the key is absent. + run /bin/bash -c "dokku storage:list $TEST_APP --format json | jq -r '.[] | select(.container_path == \"/data\") | has(\"readonly\")'" + echo "output: $output" + echo "status: $status" + assert_success + assert_output "false" + # cleanup run /bin/bash -c "dokku storage:unmount $TEST_APP rdmtest-opts --container-dir /data" assert_success