From 6f62636763324d56c42bbfed8f60584da0a4cb76 Mon Sep 17 00:00:00 2001 From: Fabian <32016596+FabianHertwig@users.noreply.github.com> Date: Tue, 20 May 2025 00:14:33 +0200 Subject: [PATCH] Refactor: Improve Docker volume permission handling and remove run-once service (#11405) **Problem:** The previous `docker-compose.yml` included a `change-vol-ownership` service. This service was designed to run once upon startup to `chown` the `server-local-data` and `docker-data` volumes to user/group `1000:1000`. This was necessary because: 1. The main `server` and `worker` containers run as the non-root user `1000` for security. 2. Docker typically creates/mounts named volumes initially owned by `root`. 3. The application needs write access to these volumes. However, this run-once service pattern causes problems in certain deployment environments (like Coolify) that don't gracefully handle services designed to exit after completing their task. This can lead to deployment failures or warnings. **Solution:** This PR refactors the Docker setup to address the volume permission issue directly within the Docker image build process, eliminating the need for the run-once service. **Changes:** 1. **`packages/twenty-docker/docker-compose.yml`:** * Removed the `change-vol-ownership` service definition entirely. * Removed the `depends_on: change-vol-ownership` condition from the `server` service definition. * **Proposed Change:** Removed the `${STORAGE_LOCAL_PATH}` environment variable from the `server-local-data` volume mounts for both `server` and `worker` services. The path is now hardcoded to `/app/packages/twenty-server/.local-storage`. (See Reasoning below). 2. **`packages/twenty-docker/twenty/Dockerfile`:** * In the final stage, *before* the `USER 1000` command, added lines to: * Create the necessary directories: `RUN mkdir -p /app/packages/twenty-server/.local-storage /app/docker-data` (and also `/app/.local-storage` for safety, though it's likely unused by volumes). * Set the correct ownership: `RUN chown -R 1000:1000 /app/.local-storage /app/packages/twenty-server/.local-storage /app/docker-data`. 3. **`packages/twenty-docker/twenty/entrypoint.sh`:** * Added a check near the beginning of the script for the presence of the now-potentially-unused `STORAGE_LOCAL_PATH` environment variable. * If the variable is set, a warning message is printed to standard output, informing the user that the variable might be deprecated and ignored if the hardcoded path change in `docker-compose.yml` is accepted. **Reasoning:** By creating the target directories (`/app/packages/twenty-server/.local-storage` and `/app/docker-data`) within the Docker image *and* setting their ownership to `1000:1000` during the build (while still running as root), we leverage Docker's volume initialization behavior. When a named volume is mounted to a non-empty directory in the container image, Docker copies the content and ownership from the image directory into the volume. This ensures that when the `server` and `worker` containers start (running as user `1000`), the volumes they mount already have the correct permissions, eliminating the need for the separate `change-vol-ownership` service. **Regarding `STORAGE_LOCAL_PATH`:** The `docker-compose.yml` previously allowed configuring the path for local storage via the `STORAGE_LOCAL_PATH` variable, defaulting to `.local-storage`. Since the Dockerfile now explicitly creates and sets permissions for `/app/packages/twenty-server/.local-storage`, maintaining this configuration might be unnecessary or could potentially lead to permission errors if a user sets it to a path *not* prepared in the Dockerfile. This PR proposes hardcoding the path in `docker-compose.yml` to `/app/packages/twenty-server/.local-storage` to align with the Dockerfile changes and simplify configuration. Is this acceptable, or is there a specific use case for retaining the `STORAGE_LOCAL_PATH` variable that needs to be considered? If retained, the Dockerfile would need further changes to dynamically handle permissions based on this variable. **Impact:** * Improves compatibility with deployment platforms that struggle with run-once containers. * Simplifies the `docker-compose.yml` setup (potentially, pending discussion on `STORAGE_LOCAL_PATH`). * Fixes volume permissions at the source (image build) rather than relying on a runtime fix. * Adds a warning for users who might have the potentially deprecated variable set. **Testing:** The changes have been tested locally using `docker compose up`. The services start correctly, the application is accessible, and the warning message for the potentially deprecated variable appears as expected when the variable is set. --------- Co-authored-by: Charles Bochet --- packages/twenty-docker/docker-compose.yml | 21 ++------------------- packages/twenty-docker/twenty/Dockerfile | 4 ++-- 2 files changed, 4 insertions(+), 21 deletions(-) diff --git a/packages/twenty-docker/docker-compose.yml b/packages/twenty-docker/docker-compose.yml index 929f4e2a1..313b02f06 100644 --- a/packages/twenty-docker/docker-compose.yml +++ b/packages/twenty-docker/docker-compose.yml @@ -1,22 +1,10 @@ name: twenty services: - change-vol-ownership: - image: ubuntu - user: root - volumes: - - server-local-data:/tmp/server-local-data - - docker-data:/tmp/docker-data - command: > - bash -c " - chown -R 1000:1000 /tmp/server-local-data - && chown -R 1000:1000 /tmp/docker-data" - server: image: twentycrm/twenty:${TAG:-latest} volumes: - - server-local-data:/app/packages/twenty-server/${STORAGE_LOCAL_PATH:-.local-storage} - - docker-data:/app/docker-data + - server-local-data:/app/packages/twenty-server/.local-storage ports: - "3000:3000" environment: @@ -31,7 +19,6 @@ services: STORAGE_S3_ENDPOINT: ${STORAGE_S3_ENDPOINT} APP_SECRET: ${APP_SECRET:-replace_me_with_a_random_string} - # MESSAGING_PROVIDER_GMAIL_ENABLED: ${MESSAGING_PROVIDER_GMAIL_ENABLED} # CALENDAR_PROVIDER_GOOGLE_ENABLED: ${CALENDAR_PROVIDER_GOOGLE_ENABLED} # AUTH_GOOGLE_CLIENT_ID: ${AUTH_GOOGLE_CLIENT_ID} @@ -57,8 +44,6 @@ services: # EMAIL_SMTP_PASSWORD: ${EMAIL_SMTP_PASSWORD:-} depends_on: - change-vol-ownership: - condition: service_completed_successfully db: condition: service_healthy healthcheck: @@ -85,7 +70,6 @@ services: STORAGE_S3_ENDPOINT: ${STORAGE_S3_ENDPOINT} APP_SECRET: ${APP_SECRET:-replace_me_with_a_random_string} - # MESSAGING_PROVIDER_GMAIL_ENABLED: ${MESSAGING_PROVIDER_GMAIL_ENABLED} # CALENDAR_PROVIDER_GOOGLE_ENABLED: ${CALENDAR_PROVIDER_GOOGLE_ENABLED} # AUTH_GOOGLE_CLIENT_ID: ${AUTH_GOOGLE_CLIENT_ID} @@ -109,7 +93,7 @@ services: # EMAIL_SMTP_PORT: ${EMAIL_SMTP_PORT:-465} # EMAIL_SMTP_USER: ${EMAIL_SMTP_USER:-} # EMAIL_SMTP_PASSWORD: ${EMAIL_SMTP_PASSWORD:-} - + depends_on: db: condition: service_healthy @@ -137,6 +121,5 @@ services: command: ["--maxmemory-policy", "noeviction"] volumes: - docker-data: db-data: server-local-data: diff --git a/packages/twenty-docker/twenty/Dockerfile b/packages/twenty-docker/twenty/Dockerfile index 54f559ee7..410069433 100644 --- a/packages/twenty-docker/twenty/Dockerfile +++ b/packages/twenty-docker/twenty/Dockerfile @@ -77,8 +77,8 @@ COPY --chown=1000 --from=twenty-front-build /app/packages/twenty-front/build /ap LABEL org.opencontainers.image.source=https://github.com/twentyhq/twenty LABEL org.opencontainers.image.description="This image provides a consistent and reproducible environment for the backend and frontend, ensuring it deploys faster and runs the same way regardless of the deployment environment." -RUN mkdir /app/.local-storage -RUN chown -R 1000 /app +RUN mkdir -p /app/.local-storage /app/packages/twenty-server/.local-storage && \ + chown -R 1000:1000 /app # Use non root user with uid 1000 USER 1000