updated deploy preview workflow and docker files accordingly (test Nginx config) #4045
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request involve updates to the deployment configuration for the website service and the introduction of new configuration files for Nginx and Supervisord. The directory structure for the website service has been modified, and the deployment command has been simplified. A new Nginx configuration file has been added to optimize serving the web application, while a Supervisord configuration file has been introduced to manage the Gunicorn and Nginx processes effectively. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## staging #4045 +/- ##
========================================
Coverage 11.73% 11.73%
========================================
Files 114 114
Lines 15349 15349
Branches 319 319
========================================
Hits 1801 1801
Misses 13548 13548 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
src/website/nginx/Dockerfile (1)
1-1: Consider pinning the Nginx version for better reproducibilityWhile using the Alpine-based image is excellent for security and size, it's recommended to pin the specific version (e.g.,
nginx:1.24-alpine) to ensure consistent builds.-FROM nginx:alpine +FROM nginx:1.24-alpinesrc/website/docker-compose.yml (1)
27-34: Enhance database service configurationAdd volume permissions and restart policy for the database service:
db: image: postgres:15 + restart: unless-stopped environment: POSTGRES_USER: ${POSTGRES_USER} POSTGRES_PASSWORD: ${POSTGRES_PASSWORD} POSTGRES_DB: ${POSTGRES_DB} volumes: - db_data:/var/lib/postgresql/data + user: postgres + healthcheck: + test: ["CMD-SHELL", "pg_isready -U ${POSTGRES_USER} -d ${POSTGRES_DB}"] + interval: 10s + timeout: 5s + retries: 5src/website/nginx/nginx.conf (2)
43-49: Add rate limiting to prevent abuseImplement rate limiting for the proxy location:
+ # Rate limiting zone + limit_req_zone $binary_remote_addr zone=one:10m rate=1r/s; + location / { + limit_req zone=one burst=10 nodelay; proxy_pass http://django_app; proxy_set_header Host $host; proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; proxy_set_header X-Forwarded-Proto $scheme; + proxy_read_timeout 60s; + proxy_connect_timeout 60s; }
30-32: Consider adding SSL configurationThe current configuration only listens on port 80. For production, SSL should be configured:
server { - listen 80; + listen 80 default_server; + listen [::]:80 default_server; + return 301 https://$server_name$request_uri; + } + + server { + listen 443 ssl http2; + listen [::]:443 ssl http2; server_name _; + + ssl_certificate /etc/nginx/ssl/server.crt; + ssl_certificate_key /etc/nginx/ssl/server.key; + ssl_protocols TLSv1.2 TLSv1.3;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
.github/workflows/deploy-previews.yml(2 hunks)src/website/Dockerfile(1 hunks)src/website/docker-compose.yml(1 hunks)src/website/entrypoint.sh(1 hunks)src/website/nginx/Dockerfile(1 hunks)src/website/nginx/nginx.conf(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/website/entrypoint.sh
- src/website/Dockerfile
🧰 Additional context used
🪛 yamllint (1.35.1)
src/website/docker-compose.yml
[error] 1-1: wrong new line character: expected \n
(new-lines)
src/website/nginx/Dockerfile
Outdated
| # Expose port 80 for Nginx | ||
| EXPOSE 80 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add HEALTHCHECK instruction for container health monitoring
Consider adding a health check to ensure the Nginx service is running properly:
EXPOSE 80
+
+HEALTHCHECK --interval=30s --timeout=3s \
+ CMD wget --quiet --tries=1 --spider http://localhost:80/ || 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.
| # Expose port 80 for Nginx | |
| EXPOSE 80 | |
| # Expose port 80 for Nginx | |
| EXPOSE 80 | |
| HEALTHCHECK --interval=30s --timeout=3s \ | |
| CMD wget --quiet --tries=1 --spider http://localhost:80/ || exit 1 |
src/website/docker-compose.yml
Outdated
| POSTGRES_USER: your_user | ||
| POSTGRES_PASSWORD: your_password | ||
| POSTGRES_DB: your_db |
There was a problem hiding this comment.
Security: Avoid hardcoded database credentials
Database credentials should be moved to environment variables or secrets management:
environment:
- POSTGRES_USER: your_user
- POSTGRES_PASSWORD: your_password
- POSTGRES_DB: your_db
+ POSTGRES_USER: ${POSTGRES_USER}
+ POSTGRES_PASSWORD: ${POSTGRES_PASSWORD}
+ POSTGRES_DB: ${POSTGRES_DB}📝 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.
| POSTGRES_USER: your_user | |
| POSTGRES_PASSWORD: your_password | |
| POSTGRES_DB: your_db | |
| POSTGRES_USER: ${POSTGRES_USER} | |
| POSTGRES_PASSWORD: ${POSTGRES_PASSWORD} | |
| POSTGRES_DB: ${POSTGRES_DB} |
src/website/docker-compose.yml
Outdated
| web: | ||
| build: | ||
| context: . | ||
| dockerfile: Dockerfile | ||
| env_file: .env | ||
| expose: | ||
| - "8000" | ||
| depends_on: | ||
| - db | ||
| volumes: | ||
| - ./staticfiles:/app/staticfiles | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add essential container configurations for production readiness
The web service should include restart policy and health check configurations:
web:
build:
context: .
dockerfile: Dockerfile
env_file: .env
expose:
- "8000"
depends_on:
- db
+ restart: unless-stopped
+ healthcheck:
+ test: ["CMD", "curl", "-f", "http://localhost:8000/health"]
+ interval: 30s
+ timeout: 10s
+ retries: 3
volumes:
- ./staticfiles:/app/staticfiles📝 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.
| web: | |
| build: | |
| context: . | |
| dockerfile: Dockerfile | |
| env_file: .env | |
| expose: | |
| - "8000" | |
| depends_on: | |
| - db | |
| volumes: | |
| - ./staticfiles:/app/staticfiles | |
| web: | |
| build: | |
| context: . | |
| dockerfile: Dockerfile | |
| env_file: .env | |
| expose: | |
| - "8000" | |
| depends_on: | |
| - db | |
| restart: unless-stopped | |
| healthcheck: | |
| test: ["CMD", "curl", "-f", "http://localhost:8000/health"] | |
| interval: 30s | |
| timeout: 10s | |
| retries: 3 | |
| volumes: | |
| - ./staticfiles:/app/staticfiles |
src/website/nginx/nginx.conf
Outdated
| # Serving static files directly from Nginx | ||
| # Assuming 'staticfiles' directory is where Django collectstatic places files | ||
| location /static/ { | ||
| alias /usr/share/nginx/html/static/; | ||
| expires 1y; | ||
| access_log off; | ||
| add_header Cache-Control "public"; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance security headers for static file serving
Add security headers to protect against common web vulnerabilities:
location /static/ {
alias /usr/share/nginx/html/static/;
expires 1y;
access_log off;
add_header Cache-Control "public";
+ add_header X-Content-Type-Options "nosniff";
+ add_header X-Frame-Options "DENY";
+ add_header X-XSS-Protection "1; mode=block";
+ add_header Referrer-Policy "strict-origin-origin-when-cross-origin";
}📝 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.
| # Serving static files directly from Nginx | |
| # Assuming 'staticfiles' directory is where Django collectstatic places files | |
| location /static/ { | |
| alias /usr/share/nginx/html/static/; | |
| expires 1y; | |
| access_log off; | |
| add_header Cache-Control "public"; | |
| } | |
| # Serving static files directly from Nginx | |
| # Assuming 'staticfiles' directory is where Django collectstatic places files | |
| location /static/ { | |
| alias /usr/share/nginx/html/static/; | |
| expires 1y; | |
| access_log off; | |
| add_header Cache-Control "public"; | |
| add_header X-Content-Type-Options "nosniff"; | |
| add_header X-Frame-Options "DENY"; | |
| add_header X-XSS-Protection "1; mode=block"; | |
| add_header Referrer-Policy "strict-origin-origin-when-cross-origin"; | |
| } |
| }) |
There was a problem hiding this comment.
Fix the string interpolation syntax in the comment body.
The current syntax ${needs.website.outputs.url} won't work in GitHub Actions. GitHub Actions requires the ${{ }} expression syntax for variable interpolation.
Apply this fix:
- body: 'website changes in this PR available for preview [here](${needs.website.outputs.url})'
+ body: 'website changes in this PR available for preview [here](${{ needs.website.outputs.url }})'📝 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.
| body: 'website changes in this PR available for preview [here](${needs.website.outputs.url})' | |
| }) | |
| body: 'website changes in this PR available for preview [here](${{ needs.website.outputs.url }})' | |
| }) |
| @@ -1192,7 +1192,7 @@ | |||
| --timeout=60 \ | |||
| --concurrency=10 \ | |||
| --image=${{ env.REGISTRY_URL }}/${{ env.PROJECT_ID }}/pr-previews/website-pr-previews:${{ github.sha }} \ | |||
| --port=8000 \ | |||
| --port=8080 \ | |||
There was a problem hiding this comment.
💡 Codebase verification
Port change requires additional configuration updates
The port change from 8000 to 8080 in the workflow file requires corresponding updates in the nginx configuration. The nginx.conf still points to port 8000:
upstream django_app {
server web:8000;
}
src/website/nginx/nginx.confneeds to be updated to use port 8080 to match the new configuration- The virtual server configurations in both staging and production environments still reference port 8000 for the website service
🔗 Analysis chain
Port configuration change looks good.
The port change from 8000 to 8080 aligns with the website service configuration.
Let's verify the Nginx configuration is consistent with this port:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if Nginx configuration matches the new port
# Search for port configuration in Nginx files
fd -e conf -e 'conf.template' . src/website/nginx/ --exec rg -l '(?:listen|proxy_pass).*:?8080'
Length of output: 95
Script:
#!/bin/bash
# Let's try a broader search for nginx configurations and port references
# First, find any nginx configuration files
fd -e conf -e conf.template . --exec echo {}
# Then search for port 8080 references in the entire codebase
rg -l '(?:port|PORT).*8080|:8080'
# Also search for the old port 8000 to verify the change
rg -l '(?:port|PORT).*8000|:8000'
Length of output: 1603
Script:
#!/bin/bash
# Let's examine the nginx.conf file to understand the port configuration
cat src/website/nginx/nginx.conf
# Also check the platform virtual service configurations since they show both ports
cat k8s/nginx/production/platform-vs.yaml
cat k8s/nginx/staging/platform-vs.yaml
Length of output: 16101
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/website/supervisord.conf (1)
10-13: Add stdout/stderr logging configuration for NginxContainer best practices recommend redirecting logs to stdout/stderr for proper log aggregation.
Add these configurations:
[program:nginx] command=/usr/sbin/nginx -g "daemon off;" autostart=true autorestart=true +stdout_logfile=/dev/stdout +stdout_logfile_maxbytes=0 +stderr_logfile=/dev/stderr +stderr_logfile_maxbytes=0src/website/nginx.conf (2)
33-38: Optimize static file serving configurationWhile the caching configuration is good, consider adding:
- gzip compression for static assets
- More specific cache control for different file types
Add these configurations before the server block:
gzip on; gzip_types text/plain text/css application/json application/javascript text/xml application/xml application/xml+rss text/javascript; gzip_comp_level 6; gzip_min_length 1000;
29-31: Consider adding SSL configurationWhile the server configuration works, it might benefit from SSL-specific settings if HTTPS is used.
Consider adding:
# Add if SSL is terminated at Nginx ssl_protocols TLSv1.2 TLSv1.3; ssl_prefer_server_ciphers off; ssl_ciphers ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:DHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES256-GCM-SHA384; ssl_session_timeout 1d; ssl_session_cache shared:SSL:50m; ssl_session_tickets off;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.github/workflows/deploy-previews.yml(2 hunks)src/website/Dockerfile(1 hunks)src/website/entrypoint.sh(1 hunks)src/website/nginx.conf(1 hunks)src/website/supervisord.conf(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/website/Dockerfile
- .github/workflows/deploy-previews.yml
🔇 Additional comments (3)
src/website/entrypoint.sh (2)
3-4: Good practice using explicit error handling!
The set -e directive ensures the script fails fast on any error, which is crucial for container initialization.
12-13:
Verify Supervisord configuration path
The path /etc/supervisor/conf.d/supervisord.conf differs from the actual configuration file location in the repository (src/website/supervisord.conf).
Consider updating the path to match the actual configuration location or ensure the Dockerfile copies the config to the correct location.
✅ Verification successful
I need to verify how the supervisord.conf file is being copied and where it's being placed in the container. This will help determine if the path in the entrypoint.sh is correct or needs to be updated.
Configuration path is correctly set in entrypoint.sh
The Dockerfile explicitly copies the supervisord.conf file to /etc/supervisor/conf.d/supervisord.conf with the command COPY supervisord.conf /etc/supervisor/conf.d/supervisord.conf. Therefore, the path used in entrypoint.sh is correct and matches the actual configuration location in the container.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the supervisord.conf is properly copied in Dockerfile
rg -l "COPY.*supervisord.conf.*" | grep -i dockerfile
Length of output: 78
Script:
#!/bin/bash
# Let's check the content of the Dockerfile to see how supervisord.conf is handled
cat src/website/Dockerfile
Length of output: 1077
Script:
#!/bin/bash
# Also check the content of the supervisord.conf file
cat src/website/supervisord.conf
Length of output: 333
src/website/nginx.conf (1)
22-22: Review client_max_body_size setting
10MB might be too permissive for your application's needs. Consider your actual requirements and adjust accordingly.
Consider implementing graduated limits based on user roles or endpoints if needed.
✅ Verification successful
Let me gather more specific information about the file upload configurations and actual file field definitions.
This will help us understand the actual file size requirements of the application by examining the field definitions and any upload configurations in Django settings.
10MB client_max_body_size is appropriate for this application
The current 10MB limit aligns perfectly with Django's configured upload limits:
- Django's
FILE_UPLOAD_MAX_MEMORY_SIZEis set to 10MB DATA_UPLOAD_MAX_MEMORY_SIZEis also set to 10MB- The application handles various file uploads including images and resources across multiple apps (press, team, publications, etc.)
- All file uploads use custom fields with proper validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for file upload handling in Django views
rg -l "FileField|ImageField|FILES" --type py
Length of output: 2635
Script:
#!/bin/bash
# Check for actual FileField and ImageField definitions and their max_size configurations
rg "FileField|ImageField" --type py -A 3
# Check Django settings for any file upload configurations
cat src/website/core/settings.py | grep -i "upload\|file\|media"
Length of output: 39446
src/website/supervisord.conf
Outdated
| [program:gunicorn] | ||
| command=gunicorn core.wsgi:application --bind 0.0.0.0:8000 --timeout 600 --workers 3 --log-level info | ||
| directory=/app | ||
| autostart=true | ||
| autorestart=true |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review Gunicorn worker configuration and timeout
A few considerations for the Gunicorn configuration:
- The worker count (3) should be validated against your container's available CPU resources
- The 600s timeout seems excessive for most web requests
- Missing stdout/stderr log configuration which is important for container environments
Consider these adjustments:
-command=gunicorn core.wsgi:application --bind 0.0.0.0:8000 --timeout 600 --workers 3 --log-level info
+command=gunicorn core.wsgi:application --bind 0.0.0.0:8000 --timeout 120 --workers %(ENV_GUNICORN_WORKERS)s --log-level info --access-logfile - --error-logfile -And add environment variable in your deployment configuration:
GUNICORN_WORKERS=3 # adjust based on: (2 * cpu_cores) + 1| location / { | ||
| proxy_pass http://django_app; | ||
| proxy_set_header Host $host; | ||
| proxy_set_header X-Real-IP $remote_addr; | ||
| proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; | ||
| proxy_set_header X-Forwarded-Proto $scheme; | ||
| } |
There was a problem hiding this comment.
Add security headers and rate limiting
The proxy configuration looks good, but missing important security headers and rate limiting.
Add these security enhancements:
# Add before the location blocks
limit_req_zone $binary_remote_addr zone=one:10m rate=1r/s;
# Add inside the location / block
add_header X-Frame-Options "SAMEORIGIN" always;
add_header X-XSS-Protection "1; mode=block" always;
add_header X-Content-Type-Options "nosniff" always;
add_header Referrer-Policy "strict-origin-when-cross-origin" always;
add_header Content-Security-Policy "default-src 'self' http: https: data: blob: 'unsafe-inline'" always;
limit_req zone=one burst=5 nodelay;
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/website/supervisord.conf (1)
12-17: Consider cloud-native logging approach for NginxThe Nginx configuration is correct for containerized environments with
daemon off, but the logging setup could be improved:Consider redirecting logs to stdout/stderr instead of files:
-stdout_logfile=/var/log/supervisor/nginx.log -stderr_logfile=/var/log/supervisor/nginx_err.log +stdout_logfile=/dev/stdout +stderr_logfile=/dev/stderr +stdout_logfile_maxbytes=0 +stderr_logfile_maxbytes=0src/website/Dockerfile (1)
12-18: Consider multi-stage build for smaller image sizeWhile the cleanup steps are good, we could further optimize:
Consider using a multi-stage build to separate build dependencies:
# Build stage FROM python:3.11-slim as builder RUN apt-get update && apt-get install -y gcc libpq-dev COPY requirements.txt . RUN pip wheel --no-deps -r requirements.txt -w /wheels # Final stage FROM python:3.11-slim COPY --from=builder /wheels /wheels RUN apt-get update && apt-get install -y \ nginx \ supervisor \ libpq-dev \ && pip install /wheels/* \ && apt-get clean \ && rm -rf /var/lib/apt/lists/*
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.github/workflows/deploy-previews.yml(2 hunks)src/website/Dockerfile(1 hunks)src/website/entrypoint.sh(1 hunks)src/website/nginx.conf(1 hunks)src/website/supervisord.conf(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/website/entrypoint.sh
- .github/workflows/deploy-previews.yml
- src/website/nginx.conf
🔇 Additional comments (5)
src/website/supervisord.conf (2)
1-2: LGTM! Appropriate configuration for containerized environment.
The nodaemon=true setting is correct for container environments where processes should run in the foreground.
4-10: Review Gunicorn configuration and log persistence
- Worker and timeout configuration concerns:
The previous review comment about worker count and timeout settings remains valid.
- Additional considerations:
- The local bind address
127.0.0.1:8000is correct for Nginx reverse proxy setup - Log files are written to
/var/log/supervisor/which won't persist across container restarts. Consider:- Mounting a volume for logs
- Using stdout/stderr logging instead, which is more cloud-native
src/website/Dockerfile (3)
Line range hint 1-10: LGTM! Solid base configuration.
The base image selection and environment variables follow container best practices.
30-37: Verify nginx.conf file existence
The configuration looks good, but let's verify the referenced files:
✅ Verification successful
Nginx configuration file exists at the expected location
The nginx.conf file is present at ./src/website/nginx.conf, which is in the same directory as the Dockerfile, confirming that the COPY instruction will work as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if nginx.conf exists in the correct location
fd --type f "nginx.conf" --exec echo "Found nginx.conf at: {}"
Length of output: 108
42-49: Verify entrypoint.sh contents
The Cloud Run port configuration is correct. Let's verify the entrypoint script:
✅ Verification successful
Entrypoint script and supervisor configuration look correct
The entrypoint.sh script follows best practices:
- Handles Django migrations and static files collection
- Uses supervisor to manage both Nginx and Gunicorn processes
- Gunicorn is properly configured to bind to port 8000 internally
- Nginx is set up as a reverse proxy (implied by the configuration)
- Appropriate logging is configured for both services
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if entrypoint.sh exists and contains supervisor
fd --type f "entrypoint.sh" --exec sh -c 'echo "Found entrypoint.sh at: {}"; cat {} | grep -A 5 "supervisor"'
Length of output: 230
Script:
#!/bin/bash
# Check the contents of entrypoint.sh and supervisor config
echo "=== entrypoint.sh contents ==="
cat ./src/website/entrypoint.sh
echo -e "\n=== supervisor config ==="
fd --type f "supervisord.conf" --exec cat {}
Length of output: 1024
|
website changes in this PR available for preview here |
Description
Summary by CodeRabbit
New Features
Chores