Fix/3139 store root named volume permissions - #3152
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The push-docker-image profile’s run-image build uses an invalid docker build --load invocation and the store-root creation uses mkdir() (not mkdirs()), both of which can cause real build/runtime failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Restores Docker named-volume persistence for S3Mock’s Buildpacks-based (non-root cnb) OCI image by ensuring the default store-root mount point is writable, failing fast with a clearer error when it isn’t, and documenting the supported persistent-storage setup.
Changes:
- Add a custom Buildpacks run image that pre-creates
/s3mockrootwithcnbownership and restoresWORKDIR /. - Improve store-root handling: create/validate root dir, fail fast if not writable, and log configured vs resolved root path.
- Add Testcontainers support for named volumes plus a restart-persistence regression test; update docs and changelog.
Findings
-
Must fix
server/pom.xml: the release profile usesdocker build --platform ... --load, but--loadrequiresdocker buildx build; this will break the release image build flow (rootAGENTS.mdCI/Docker gate context:AGENTS.md:116-125).StoreConfiguration.rootFolder(): usesmkdir()(notmkdirs()), which fails for nested store-root paths and contradicts the “arbitrary absolute or relative path” contract described inStoreProperties(StoreProperties.kt:27-31).
-
Should fix
StoreConfiguration.kt: error message includes a trailing colon afterCOM_ADOBE_TESTING_S3MOCK_STORE_ROOT=/s3mockroot:, which is easy to misread/copy (clarity for fail-fast behavior; seeStoreConfiguration.kt:147-151).README.md: the “relative store root” example switches to/s3root, which can be misread as a named-volume recommendation despite the preceding/s3mockrootrequirement (README accuracy is part of DoD:INVARIANTS.md:83-84).CHANGELOG.md: 5.2.1 lists CI/build dependency bumps that are not present in the workflows in this branch; changelog entries should reflect actual changes (changelog DoD:INVARIANTS.md:83).
File summaries
| File | Description |
|---|---|
| testsupport/testcontainers/src/test/kotlin/com/adobe/testing/s3mock/testcontainers/S3MockContainerRestartOnNamedVolumeTest.kt | Adds regression test for persistence across restart using a named volume. |
| testsupport/testcontainers/src/main/kotlin/com/adobe/testing/s3mock/testcontainers/S3MockContainer.kt | Adds withNamedVolume(...) and consolidates store-root mount path constant. |
| testsupport/testcontainers/AGENTS.md | Documents withNamedVolume(...) vs bind-mount/root behavior. |
| testsupport/AGENTS.md | Updates listed Testcontainers configuration methods. |
| server/src/test/kotlin/com/adobe/testing/s3mock/s3/store/StoreConfigurationTest.kt | Adds unit tests for root-folder creation/writability checks. |
| server/src/main/kotlin/com/adobe/testing/s3mock/s3/store/StoreProperties.kt | Clarifies store-root semantics (env var, relative paths). |
| server/src/main/kotlin/com/adobe/testing/s3mock/s3/store/StoreConfiguration.kt | Fails fast when store root isn’t writable; logs resolved vs configured root. |
| server/src/main/docker/run-image/Dockerfile | New custom run image: pre-creates /s3mockroot and sets WORKDIR /. |
| server/pom.xml | Builds custom run image and configures buildpacks to use it for app image builds. |
| README.md | Updates Docker Compose persistent storage guidance for named volumes and /s3mockroot. |
| pom.xml | Adds docker.run.image.name property; adjusts release/spotless workflow configuration. |
| docs/SETUP.md | Updates persistent storage documentation and troubleshooting notes for named volumes. |
| CHANGELOG.md | Adds 5.2.1 entry describing the regression fix and related changes. |
Review details
Suppressed comments (2)
server/pom.xml:408
- Same issue for the arm64 run-image build:
--loadrequiresdocker buildx build, notdocker build, otherwise this execution will fail when thepush-docker-imageprofile runs.
<executable>docker</executable>
<arguments>
<argument>build</argument>
<argument>--platform</argument>
<argument>linux/arm64</argument>
<argument>--load</argument>
<argument>--tag</argument>
server/src/main/kotlin/com/adobe/testing/s3mock/s3/store/StoreConfiguration.kt:142
rootFolder()only usesmkdir(), which fails for nested paths (e.g./data/s3mock/root) and can also throw spuriously if another process creates the directory between theexists()check and themkdir()call. Since the docs now explicitly call out arbitrary root paths, this should create parents (mkdirs()) and tolerate the race.
val dir = File(rootPath)
if (!dir.exists()) {
check(dir.mkdir()) {
("Root folder could not be created. Path: ${dir.absolutePath}")
}
}
- Files reviewed: 13/13 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
232361c to
df97778
Compare
There was a problem hiding this comment.
🟡 Changes recommended
It introduces a fail-fast writability check but still doesn’t validate the store root is a directory, and the 5.2.1 changelog includes build-dependency bump entries that aren’t reflected in the repo.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
CHANGELOG.md:172
- The 5.2.1 changelog lists build-dependency bumps (setup-java/codeql-action/setup-qemu/harden-runner), but this PR does not update the corresponding workflow pins (they remain at setup-java v5.7.0, codeql-action v4.37.7, setup-qemu v4.2.0, harden-runner v2.21.0). Please remove or correct these entries so the changelog matches the actual changes shipped in 5.2.1.
* Bump actions/setup-java from 5.7.0 to 6.0.0
* Bump github/codeql-action from 4.37.7 to 4.37.9
* Bump docker/setup-qemu-action from 4.2.0 to 4.3.0
* Bump step-security/harden-runner from 2.21.0 to 2.21.1
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
df97778 to
4dfb626
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The changelog edits appear internally inconsistent (notably rewriting a released 5.2.0 harden-runner bump) and should be corrected before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
CHANGELOG.md:172
- This 5.2.1 build-dependency bullet implies a 2.21.0→2.21.1 bump for step-security/harden-runner, but 5.2.0 already included 2.21.1; consider removing this line unless there is an actual new bump in this release.
* Bump step-security/harden-runner from 2.21.0 to 2.21.1
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Lite
4dfb626 to
71b1380
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new fail-fast store-root permission check should also validate directory traverse (execute) permission to avoid false positives that still fail at runtime.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Lite
71b1380 to
d8ea72e
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
It changes the OCI image build pipeline (custom run image + Buildpacks configuration) and runtime storage-root validation, which should be verified by a human with end-to-end Docker build/run testing across the supported environments.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
d8ea72e to
8a85f92
Compare
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
) Since the migration to the Cloud Native Buildpacks OCI image, the container runs as the non-root `cnb` user (uid 1000). A fresh Docker named volume mounted onto an arbitrary path is owned by root, so writes to the configured store root failed with HTTP 500. - Add a custom Buildpacks run image (server/src/main/docker/run-image) that pre-creates /s3mockroot owned by cnb, so Docker initializes a named volume mounted there with writable ownership. - StoreConfiguration now fails fast at startup with a clear message when the configured store root is not writable, instead of silently serving requests that later fail. - Add S3MockContainer.withNamedVolume(...) as the preferred, non-root-friendly alternative to withVolumeAsRoot(...), plus a container restart persistence regression test. - Clarify StoreProperties.root KDoc: settable via JVM/Spring property or the COM_ADOBE_TESTING_S3MOCK_STORE_ROOT env var, and may be any writable path (relative paths resolve against the process's working directory as usual). - Update README.md / docs/SETUP.md with the new named-volume example, the ownership requirement for bind mounts, and pointers for finding a named volume's data on disk vs. using a bind mount for a predictable host path. Fixes #3139
…3139) The Buildpacks-built OCI image sets the launch working directory to /workspace (the pre-5.2 root-based image had no explicit WORKDIR, defaulting to /). A relative COM_ADOBE_TESTING_S3MOCK_STORE_ROOT (e.g. "s3root") therefore silently resolved to /workspace/s3root instead of /s3root: the app started and served requests successfully, writing into a directory a mounted volume never touched, so persisted data was silently lost on container recreation. - Restore WORKDIR / in the custom run image so relative store roots resolve exactly as they did before 5.2. - Always log the resolved absolute root folder alongside the configured value at startup, so a mismatch between the two (e.g. against a mount path) is immediately visible instead of silent. - Document the relative-path resolution behavior and add a troubleshooting entry for the "starts fine but mounted volume stays empty" symptom. Fixes #3139
The maven release plugin rewrites poms which makes spotless fail during release if we check, and it will fail main with the generated POMs if we don't apply.
8a85f92 to
e8fc845
Compare
Description
Changes to the Docker container build in 5.2.0 broke the mounting of volumes as the root storage for S3Mock.
This PR restores that functionality. See README.md for documentation on how to use this feature now.
Related Issue
#3139
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: