Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

explore: non-root user for docker #1589

Open
wants to merge 67 commits into
base: main
Choose a base branch
from
Open

Conversation

eric-intuitem
Copy link
Collaborator

@eric-intuitem eric-intuitem commented Mar 1, 2025

test on backend

Summary by CodeRabbit

  • New Features

    • Improved startup process with dynamic waiting for backend readiness and streamlined Docker Compose commands.
    • Added support for specifying an alternative Docker Compose file and enforcing service dependencies.
  • Documentation

    • Updated README notes for fresh installation instructions, clarifying the removal of Docker volumes instead of directories.
  • Chores

    • Standardized container configurations with non-root user settings and Docker-managed named volumes.
    • Removed outdated directory checks and fixed permissions management for enhanced security and reliability.

Copy link
Contributor

coderabbitai bot commented Mar 1, 2025

Walkthrough

The pull request updates several Docker-related configurations and scripts. The CI workflow and build scripts now use a dynamic loop with curl to check backend readiness instead of fixed sleep delays. Additionally, Docker Compose files for both standard and enterprise setups replace bind mounts with named volumes, and Dockerfiles for backend and frontend services now create a non-root “app” user with specific UID/GID, set an environment variable, and configure persistent storage. Documentation in the README files has been updated to clarify Docker Compose usage under Linux and enterprise environments.

Changes

Files Change Summary
.github/workflows/startup-tests.yml Changed Docker app build command; replaced static sleep with a loop that sends curl requests to verify backend readiness.
docker-compose-build.sh, docker-compose.sh Updated build script: accepts an optional Docker Compose file argument; removed DB existence check; replaced fixed sleep with a dynamic backend readiness loop.
docker-compose-build.yml, docker-compose.yml, enterprise/docker-compose-build.yml Added named volumes (db, caddy_data) and updated volume mounts for services; added dependency for service startup in enterprise file.
backend/Dockerfile, enterprise/backend/Dockerfile Added HOME=/tmp environment variable; created non-root user/group app (UID/GID 1001); created and set ownership for /code/db and declared it as a volume.
frontend/Dockerfile, enterprise/frontend/Dockerfile Introduced non-root user and group creation (app with UID/GID 1001) and set the container to run as the new user.
README.md, enterprise/README.md Updated documentation to include instructions for running Docker Compose on Linux (bind-mount ownership fix) and simplified command usage/working directory for enterprise builds.

Sequence Diagram(s)

sequenceDiagram
    participant CI as CI Workflow / Build Script
    participant DC as Docker Compose
    participant BE as Backend Service

    CI->>DC: Start containers (build/up)
    loop Check Backend Readiness
        CI->>BE: Send curl request to API endpoint
        BE-->>CI: Respond (ready or not)
    end
    CI->>DC: Proceed with initialization after readiness confirmation
Loading

Possibly related PRs

  • feat!: docker compose custom config builder #1470: The changes in the main PR regarding the Docker app setup and readiness checks are related to the modifications in the retrieved PR, which also involve updates to Docker configurations and readiness checks for services, specifically in the context of managing the backend service readiness.

Suggested labels

deployment

Suggested reviewers

  • ab-smith

Poem

Hop, hop, in code I bound,
A rabbit’s leap with changes found.
No more fixed sleeps, we now check on time,
Volumes named and users in line.
From Docker builds to workflows neat,
I celebrate these hops so sweet!
ʕ•ᴥ•ʔ


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f9f2b2c and 4e03efd.

📒 Files selected for processing (1)
  • README.md (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • README.md
⏰ Context from checks skipped due to timeout of 90000ms (8)
  • GitHub Check: startup-functional-test (3.12)
  • GitHub Check: startup-docker-compose-test
  • GitHub Check: functional-tests (3.12, chromium)
  • GitHub Check: enterprise-startup-functional-test (3.12)
  • GitHub Check: build (3.12)
  • GitHub Check: test (3.12)
  • GitHub Check: enterprise-startup-docker-compose-test
  • GitHub Check: enterprise-functional-tests (3.12, chromium)

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@ab-smith ab-smith changed the title fix: non-root user for docker explore: non-root user for docker Mar 2, 2025
@eric-intuitem eric-intuitem marked this pull request as ready for review March 5, 2025 00:09
@eric-intuitem eric-intuitem requested a review from ab-smith March 5, 2025 00:10
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (6)
enterprise/docker-compose-build.yml (1)

26-27: Huey Service Dependency Declaration.
Adding the depends_on directive for the huey service ensures that the backend starts first. Please note that line 27 contains trailing spaces; removing them will clear the YAMLlint warning.

🧰 Tools
🪛 YAMLlint (1.35.1)

[error] 27-27: trailing spaces

(trailing-spaces)

docker-compose.sh (1)

21-21: Extraneous Tilde Character.
Line 21 shows a tilde (or stray marker) without accompanying content. Please remove it if it is not intended to be part of the final script.

.github/workflows/startup-tests.yml (3)

144-147: Improved dynamic readiness check:
Using a loop with curl to verify backend readiness is a clear improvement over a static sleep delay. Consider adding a timeout mechanism or a maximum retry counter to avoid potential infinite waits if the backend never becomes ready.


280-281: Quote environment variable expansion:
In the step that creates the backend environment variables file, the command

export $(grep -v '^#' .env | xargs)

can lead to word splitting issues. Please consider quoting the command’s output (or using a safer loop) as recommended by shellcheck (SC2046).


283-286: Consistent readiness check in enterprise tests:
The enterprise Docker Compose test’s loop for checking backend readiness mirrors the earlier change, ensuring consistency across environments. As before, adding a timeout or maximum retry counter would further safeguard against prolonged hangs.

docker-compose-build.sh (1)

29-32: Dynamic backend readiness wait loop:
Switching to a dynamic wait loop using curl significantly improves the robustness of the startup process compared to a fixed delay. As a safeguard, consider implementing a timeout or maximum retry limit to handle the scenario where the backend does not become ready.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b32651d and f9f2b2c.

📒 Files selected for processing (12)
  • .github/workflows/startup-tests.yml (2 hunks)
  • README.md (1 hunks)
  • backend/Dockerfile (2 hunks)
  • docker-compose-build.sh (2 hunks)
  • docker-compose-build.yml (4 hunks)
  • docker-compose.sh (1 hunks)
  • docker-compose.yml (4 hunks)
  • enterprise/README.md (1 hunks)
  • enterprise/backend/Dockerfile (2 hunks)
  • enterprise/docker-compose-build.yml (3 hunks)
  • enterprise/frontend/Dockerfile (1 hunks)
  • frontend/Dockerfile (1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
enterprise/docker-compose-build.yml

[error] 27-27: trailing spaces

(trailing-spaces)

🪛 actionlint (1.7.4)
.github/workflows/startup-tests.yml

278-278: shellcheck reported issue in this script: SC2046:warning:2:8: Quote this to prevent word splitting

(shellcheck)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: enterprise-functional-tests (3.12, chromium)
🔇 Additional comments (27)
README.md (1)

276-278: Enhanced Documentation for Linux Bind-Mount Ownership
The newly added note helps clarify that when using a bind-mount on Linux, users need to adjust ownership via chown 1001:1001 db to match the non-root user configuration. This is a valuable clarification that aligns the README with the updated Dockerfile settings.

frontend/Dockerfile (1)

24-25: Implement Non-Root User in Frontend Container
The addition of the command to create a group and user (app with UID/GID 1001) and switching the process context via USER app improves container security by preventing operations under the root user. Ensure that all file permissions required for runtime operations are accessible by the app user.

enterprise/frontend/Dockerfile (1)

27-29: Secure Enterprise Frontend with Non-Root User Setup
Creating a non-root user (app with UID/GID 1001) and switching to that user using USER app is a commendable security enhancement. Confirm that any files or directories needing write permissions are appropriately set up for the app user to avoid permission issues during runtime.

enterprise/backend/Dockerfile (2)

8-9: Configure Environment Variables for Consistency
Introducing the environment variable DJANGO_SETTINGS_MODULE=enterprise_core.settings and setting HOME=/tmp assists with runtime consistency and isolation. Make sure these variable settings are compatible with other deployment environments.


57-59: Non-Root User Creation and Volume Declaration
Creating the app user and group (UID/GID 1001), ensuring the /code/db directory exists with correct ownership, and designating it as a persistent volume (via VOLUME /code/db) collectively enhance the container’s security and data management. Verify that these changes propagate to any scripts or configurations that rely on directory permissions.

backend/Dockerfile (2)

8-9: Streamline Environment Variables for Backend Container
Setting HOME=/tmp/ alongside establishing POETRY_CACHE_DIR=/tmp/poetry_cache is a solid move to isolate runtime artifacts and temporary data. Ensure that these environment settings do not conflict with any application-specific configurations.


51-53: Adopt Non-Root User and Define Persistent Volume
The introduction of non-root user creation commands (groupadd and useradd for app with UID/GID 1001), creating the /code/db directory with the appropriate ownership, and exposing the directory as a volume (VOLUME /code/db) significantly bolster the container’s security posture and data persistence. Double-check that all services interacting with this volume account for the updated permissions.

enterprise/docker-compose-build.yml (4)

1-3: Volume Definitions Verified.
The named volumes db and caddy_data are correctly declared at the top of the file, setting the foundation for standardized volume management across services.


18-20: Backend Service Volume Mapping.
The backend service now mounts the named volume db to /code/db, which enhances portability by avoiding host-dependent bind mounts.


35-35: Huey Service Volume Mapping.
The huey service’s volume configuration using the db named volume is consistent with the backend configuration.


65-66: Caddy Service Volume Mapping Update.
The caddy service correctly uses the caddy_data named volume for /data, aligning it with the overall volume management strategy.

docker-compose.yml (4)

1-3: Volumes Section Standardization.
The volumes section now declares db and caddy_data as named volumes, replacing host bind mounts for improved consistency and maintenance.


16-17: Backend Service Volume Mapping.
The backend service mounts the db named volume to /code/db, which promotes a more portable and predictable environment configuration.


39-40: Huey Service Volume Configuration.
The huey service similarly employs the db volume for its data, ensuring uniformity across services.


71-72: Caddy Service Volume Mapping.
Mapping the caddy service’s volume to caddy_data:/data aligns with the new volume management strategy and mirrors the configuration in the enterprise compose file.

docker-compose-build.yml (4)

1-3: Named Volumes Declaration.
The volumes section at the top of the file correctly defines db and caddy_data as named volumes, ensuring consistency in storage management across services.


15-16: Backend Service Volume Mapping.
The backend service mounts the db volume to /code/db, streamlining the transition away from bind mounts and enhancing container portability.


31-31: Huey Service Volume Mapping.
The huey service is configured to use the named volume db for its storage, following the same pattern as the backend service.


60-61: Caddy Service Volume Configuration.
Assigning the named volume caddy_data to the caddy service for /data is consistent with the changes in other compose files and ensures better data isolation.

docker-compose.sh (3)

1-8: Simplified Initialization Process.
The script now directly pulls images and starts the containers without conditionally checking for an existing db directory. This simplification aligns with the new Docker volume strategy.


9-13: Dynamic Backend Readiness Check.
Introducing a loop with curl to wait for the backend to become ready improves robustness by dynamically verifying service availability before proceeding.


14-20: Superuser Creation and Completion Messages.
The steps for creating a superuser and displaying progress messages are clear and sequential, ensuring users understand the initialization flow.

enterprise/README.md (2)

7-7: Clarify Repository Directory Requirement.
Updating the instruction to ensure the user is in the top directory of the repository is helpful for avoiding path issues when running Docker commands.


12-13: Streamlined Docker Compose Command.
The revised command (docker compose-build.sh enterprise/docker-compose-build.yml) simplifies the Docker startup process and aligns with recent script modifications.

docker-compose-build.sh (3)

4-4: Optional Docker Compose file parameter:
Using

DOCKER_COMPOSE_FILE="${1:-docker-compose-build.yml}"

provides flexibility to specify an alternative Docker Compose file when needed. This change enhances the script’s usability.


19-19: Early metadata preparation:
Calling prepare_meta_file at the beginning ensures that the build metadata is updated prior to container operations, which supports consistency and traceability.


34-39: Clear final messaging:
The final output messages clearly communicate the state and next steps to the user. The instructions are helpful and consistent with the previous changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant