-
Notifications
You must be signed in to change notification settings - Fork 169
feat: Support running Docker container as non-root #599
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
Conversation
WalkthroughThe pull request introduces multi-user support to the Docker image by creating a non-root sourcebot user, installing required dependencies, and modifying the entrypoint script to handle both root and non-root execution contexts for database initialization and startup. Changes
Sequence DiagramssequenceDiagram
participant Script as entrypoint.sh
participant FS as Filesystem
participant DB as PostgreSQL
participant OS as OS Process
Script->>Script: Detect IS_ROOT
alt Running as Root
Script->>FS: mkdir -p /var/lib/postgres
Script->>FS: chown -R postgres:postgres
Script->>OS: su postgres -c initdb
Script->>OS: su postgres -c "postgres -D ..." (background)
else Running as Non-Root
Script->>FS: mkdir -p /var/lib/postgres
Script->>OS: initdb directly
Script->>OS: postgres -D ... (background)
Script->>DB: Create postgres role (if missing)
end
Script->>OS: Check pg_isready in loop
Script->>OS: Verify postgres process still running
alt Process died
Script->>Script: Exit with error
else Process alive
Script->>DB: Continue with database setup
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Running containers as a non-root user is a long standing security practice. The changes ensure that the sourcebot user is created and has the correct level of permissions to run all its dependencies (postgres, redis and node). Please note that as a side effect, existing mounted volumes would need to have their ownership reviewed or it may not be able to access the files. This is specially the case for previous versions that would create said files as 0:0. To fix that, users can run chown -R 1500:1500 /path/.sourcebot. The chmod may also need to be a bit more strict in such cases, so changing that is advised: chown -R 0750 /path/.sourcebot. Signed-off-by: Paulo Gomes <pjbgf@linux.com>
64f970b to
60ed95e
Compare
|
@brendan-kellam your pull request is missing a changelog! |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
entrypoint.sh (2)
15-15: Address POSIX sh compatibility warnings forecho -ecolor codes.Static analysis flags
echo -eas undefined in POSIX sh (SC3037). While Alpine's ash likely supports-e, for strict POSIX compatibility consider usingprintfinstead.- echo -e "\e[34m[Info] Running as root user.\e[0m" + printf "\033[34m[Info] Running as root user.\033[0m\n"However, if Alpine sh support is guaranteed in your deployment, this can be deferred.
Also applies to: 17-17
96-105: Conditional database initialization handles both contexts but verify non-root initdb.When running as non-root (lines 102-104),
initdb -Dis executed directly without the postgres system user. Verify this works correctly in your test environments and that the resulting database cluster can be used by postgres or the container user later.Also note: Line 98 uses unquoted
$DATABASE_DATA_DIRinmkdir -p; Line 101 uses unquoted path insu postgres -c. Recommend consistent quoting for safety:- mkdir -p $DATABASE_DATA_DIR + mkdir -p "$DATABASE_DATA_DIR"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Dockerfile(2 hunks)entrypoint.sh(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit inference engine (.cursor/rules/style.mdc)
Filenames should always be camelCase. Exception: if there are filenames in the same directory with a format other than camelCase, use that format to keep things consistent.
Files:
entrypoint.shDockerfile
🪛 Shellcheck (0.11.0)
entrypoint.sh
[warning] 15-15: In POSIX sh, echo flags are undefined.
(SC3037)
[warning] 17-17: In POSIX sh, echo flags are undefined.
(SC3037)
[warning] 207-207: In POSIX sh, echo flags are undefined.
(SC3037)
🔇 Additional comments (7)
Dockerfile (4)
185-186: Consolidate runtime dependencies in single layer.Dependency installation is now centralized and includes postgres, redis, supervisor, and related tools. This is good for layer efficiency.
188-199: User/group creation supports both root and non-root execution.The sourcebot user is created with UID/GID 1500 and properly added to postgres, redis, and node groups to allow non-root execution while maintaining necessary permissions. The /var/log/sourcebot directory is correctly created and owned by sourcebot.
241-242: Verify chown of /app aligns with entrypoint database initialization logic.Line 242 chowns /app to sourcebot:sourcebot. Ensure this doesn't conflict with postgres data directory ownership expectations when running database initialization as non-root in entrypoint.sh (lines 102-104). If postgres needs to create files in /app/DATABASE_DATA_DIR as non-root, verify permissions are sufficient.
250-252: Good decision to maintain backward compatibility by not setting USER directive.Leaving the USER instruction commented preserves the default root execution while allowing runtime override via
docker run --useror KubernetespodSecurityContext. This ensures existing deployments continue to work unchanged.entrypoint.sh (3)
8-18: Root detection logic is correct.The
id -ucheck properly detects root (returns 0) and sets IS_ROOT flag for conditional logic downstream. Informational messages help with debugging multi-user execution paths.
199-222: Database startup and health-check logic correctly handles both root and non-root execution.The conditional postgres startup (lines 200-203) appropriately uses
su postgresas root and direct execution as non-root. The process-monitoring loop (lines 210-216) that checks if postgres is still running is a good addition to prevent indefinite waiting if postgres crashes.The non-root postgres role creation (lines 218-222) defensively ensures the postgres superuser exists, though this should normally be guaranteed by initdb. Verify this handles edge cases gracefully.
206-207: Verify postgres process health-check doesn't mask initialization failures.The
pg_isreadyloop waits for the database to accept connections, and lines 212-216 now check if the postgres process is still running. Confirm that if postgres fails during startup (e.g., due to corrupted cluster), thepgrepcheck catches it and exits clearly rather than silently looping. The error message "postgres failed to run" is helpful.Also applies to: 212-216
This PR adds support for running the Docker container as a non-root user. It sets up a
sourcebotuser with UID & GID of 1500. To avoid introducing a breaking change here, the Docker container still runs as root by default. It can be configured to run as non-root by specifying the--userparam todocker runlocally, or by configuring thepodSecurityContextwhen deploying w/ kubernetes.Todos in follow up PRs:
Thanks to @pjbgf for getting this effort started in #510
Summary by CodeRabbit