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

updated deploy preview workflow and docker files accordingly (test Nginx config) #4045

Open
wants to merge 8 commits into
base: staging
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions .github/workflows/deploy-previews.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1186,17 +1186,14 @@ jobs:

- name: Deploy to Cloud Run
run: |-
gcloud run deploy ${{ needs.branch-name.outputs.lowercase }}-website-preview \
--region=${{ secrets.REGION }} \
gcloud run deploy website-backend-nginix-website-preview \
--region=${REGION} \
--max-instances=10 \
--timeout=60 \
--concurrency=10 \
--image=${{ env.REGISTRY_URL }}/${{ env.PROJECT_ID }}/pr-previews/website-pr-previews:${{ github.sha }} \
--port=8000 \
--image=${REGISTRY_URL}/${PROJECT_ID}/pr-previews/website-pr-previews:${GITHUB_SHA} \
--cpu=1000m \
--memory=1024Mi \
--update-secrets=/etc/env/.env=sta-env-website-backend:latest,/etc/config/google_application_credentials.json=sta-key-analytics-service-account:latest \
--command="/bin/sh","-c","cat /etc/env/.env >> /app/.env; /app/entrypoint.sh" \
--allow-unauthenticated

- name: Get preview service url
Expand All @@ -1221,5 +1218,5 @@ jobs:
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo,
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})'
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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 }})'
})

})
29 changes: 21 additions & 8 deletions src/website/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -5,27 +5,40 @@ FROM python:3.11-slim
ENV PYTHONDONTWRITEBYTECODE 1
ENV PYTHONUNBUFFERED 1

# Set the working directory inside the container
WORKDIR /app

# Install system dependencies
RUN apt-get update && apt-get install -y \
gcc \
libpq-dev \
nginx \
supervisor \
&& apt-get clean

# Copy requirements file and install dependencies
COPY requirements.txt ./
# Copy requirements and install
COPY requirements.txt .
RUN pip install --no-cache-dir -r requirements.txt

# Copy the rest of the backend code into the container
# Copy the rest of the code
COPY . .

# Expose the port the Django app will run on
EXPOSE 8000
# Remove default Nginx config
RUN rm /etc/nginx/conf.d/default.conf

# Add execution permissions to entrypoint.sh
# Make sure we have a directory for static files (to serve them via Nginx)
RUN mkdir -p /usr/share/nginx/html/static/

# Add execute permission to entrypoint.sh
RUN chmod +x /app/entrypoint.sh

# Set the entrypoint for the container
# Copy custom Nginx config
COPY nginx.conf /etc/nginx/nginx.conf

# Copy supervisor config
COPY supervisord.conf /etc/supervisor/conf.d/supervisord.conf

# Default port for Cloud Run is 8080, ensure Nginx listens on this port
ENV PORT 8080
EXPOSE 8080

ENTRYPOINT ["/app/entrypoint.sh"]
9 changes: 3 additions & 6 deletions src/website/entrypoint.sh
Original file line number Diff line number Diff line change
@@ -1,16 +1,13 @@
#!/bin/sh

# Exit immediately if a command exits with a non-zero status
# Exit on error
set -e

# Run Django migrations
echo "Running migrations..."
python manage.py migrate --noinput

# Collect static files (ensure the static files directory exists)
echo "Collecting static files..."
python manage.py collectstatic --noinput

# Start Gunicorn server to serve the Django application
echo "Starting Gunicorn server..."
exec gunicorn core.wsgi:application --bind 0.0.0.0:8000 --timeout 600 --workers 3 --log-level info
echo "Starting Supervisor (which runs Nginx + Gunicorn)..."
exec /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf
48 changes: 48 additions & 0 deletions src/website/nginx.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
user nginx;
worker_processes auto;
pid /var/run/nginx.pid;

events {
worker_connections 1024;
}

http {
sendfile on;
tcp_nopush on;
tcp_nodelay on;
keepalive_timeout 65;
types_hash_max_size 2048;

include /etc/nginx/mime.types;
default_type application/octet-stream;

access_log /var/log/nginx/access.log;
error_log /var/log/nginx/error.log;

client_max_body_size 10M;

upstream django_app {
# Gunicorn will be running on 127.0.0.1:8000 inside the same container
server 127.0.0.1:8000;
}

server {
listen ${PORT};
server_name _;

location /static/ {
alias /usr/share/nginx/html/static/;
expires 1y;
access_log off;
add_header Cache-Control "public";
}

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;
}
Comment on lines +43 to +49
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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;

}
}
13 changes: 13 additions & 0 deletions src/website/supervisord.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
[supervisord]
nodaemon=true

[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
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Review Gunicorn worker configuration and timeout

A few considerations for the Gunicorn configuration:

  1. The worker count (3) should be validated against your container's available CPU resources
  2. The 600s timeout seems excessive for most web requests
  3. 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


[program:nginx]
command=/usr/sbin/nginx -g "daemon off;"
autostart=true
autorestart=true
Loading