fix(api): multi-stage build, non-root user, add healthcheck#612
Conversation
|
@pavsoss is attempting to deploy a commit to the sreerevanth's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthrough
ChangesAPI container hardening
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
🧪 PR Test Results
Python 3.12 · commit 5a05591 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Dockerfile.api (1)
14-15: 🩺 Stability & Availability | 🔵 TrivialConsider a longer
--start-periodfor Python application startup.The
--start-period=5sgrace window may be too short for a Python/FastAPI application that initializes database sessions, Redis connections, and other subsystems before the/healthendpoint becomes responsive. If the app takes longer than 5 seconds to bind port 8000, the healthcheck will begin failing immediately, and orchestrators (e.g., Kubernetes, Docker Swarm) may mark the container unhealthy and restart it before it finishes starting. A value of15s–30sis more typical for Python web applications.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile.api` around lines 14 - 15, Increase the HEALTHCHECK --start-period from 5s to a 15s–30s grace window, preferably 30s, so the Python application can complete initialization before healthcheck failures count.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Dockerfile.api`:
- Around line 14-15: Update the Dockerfile healthcheck’s urllib.request.urlopen
call to pass timeout=5, and wrap the request in exception handling that
suppresses HTTPError and other expected healthcheck failures while still exiting
non-zero. Keep successful /health responses passing and ensure failures complete
within Docker’s 10-second timeout without traceback output.
---
Nitpick comments:
In `@Dockerfile.api`:
- Around line 14-15: Increase the HEALTHCHECK --start-period from 5s to a
15s–30s grace window, preferably 30s, so the Python application can complete
initialization before healthcheck failures count.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 99332262-2cd4-4a9b-bd9b-9545bbada46d
📒 Files selected for processing (1)
Dockerfile.api
| HEALTHCHECK --interval=30s --timeout=10s --start-period=5s --retries=3 \ | ||
| CMD python -c "import urllib.request; urllib.request.urlopen('http://localhost:8000/health')" || exit 1 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add a timeout to the urlopen call in the healthcheck.
urllib.request.urlopen is invoked without a timeout argument. If the API accepts the TCP connection but hangs before responding, the Python process will block until Docker's --timeout=10s kills it — producing a less informative timeout failure and holding a connection open longer than necessary. Passing timeout=5 ensures the check fails fast and cleanly within the Docker timeout window.
Additionally, when the /health endpoint returns 503 (degraded), urlopen raises HTTPError and Python prints a full traceback to stderr on every check interval, which clutters container logs. Wrapping in a try/except would suppress the noise while still exiting non-zero.
🛡️ Proposed healthcheck improvement
HEALTHCHECK --interval=30s --timeout=10s --start-period=5s --retries=3 \
- CMD python -c "import urllib.request; urllib.request.urlopen('http://localhost:8000/health')" || exit 1
+ CMD python -c "import urllib.request, sys; sys.exit(0 if urllib.request.urlopen('http://localhost:8000/health', timeout=5).status == 200 else 1)" || exit 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| HEALTHCHECK --interval=30s --timeout=10s --start-period=5s --retries=3 \ | |
| CMD python -c "import urllib.request; urllib.request.urlopen('http://localhost:8000/health')" || exit 1 | |
| HEALTHCHECK --interval=30s --timeout=10s --start-period=5s --retries=3 \ | |
| CMD python -c "import urllib.request, sys; sys.exit(0 if urllib.request.urlopen('http://localhost:8000/health', timeout=5).status == 200 else 1)" || exit 1 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Dockerfile.api` around lines 14 - 15, Update the Dockerfile healthcheck’s
urllib.request.urlopen call to pass timeout=5, and wrap the request in exception
handling that suppresses HTTPError and other expected healthcheck failures while
still exiting non-zero. Keep successful /health responses passing and ensure
failures complete within Docker’s 10-second timeout without traceback output.
Closes #594
This PR addresses the security vulnerabilities in
Dockerfile.apiby:build-essentialandgitout of the final container.agentwatchnon-root user (uid 10001) instead ofroot.HEALTHCHECK(usingurllib.request) to verify the API is up without needing to installcurl.Summary by CodeRabbit
Bug Fixes
Security
Performance