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 1 commit
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
6 changes: 3 additions & 3 deletions .github/workflows/deploy-previews.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1192,7 +1192,7 @@ jobs:
--timeout=60 \
--concurrency=10 \
--image=${{ env.REGISTRY_URL }}/${{ env.PROJECT_ID }}/pr-previews/website-pr-previews:${{ github.sha }} \
--port=8000 \
--port=8080 \
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 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.conf needs 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

--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 \
Expand Down Expand Up @@ -1221,5 +1221,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 }})'
})

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

# Set the working directory inside the container
WORKDIR /app

# Install system dependencies
Expand All @@ -14,18 +13,17 @@ RUN apt-get update && apt-get install -y \
libpq-dev \
&& 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 the port for Gunicorn
EXPOSE 8000

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

# Set the entrypoint for the container
ENTRYPOINT ["/app/entrypoint.sh"]
37 changes: 37 additions & 0 deletions src/website/docker-compose.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
version: "3.8"
services:
web:
build:
context: .
dockerfile: Dockerfile
env_file: .env
expose:
- "8000"
depends_on:
- db
volumes:
- ./staticfiles:/app/staticfiles

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

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.

Suggested change
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

nginx:
build:
context: ./nginx
dockerfile: Dockerfile
ports:
- "80:80"
depends_on:
- web
volumes:
# Mount the staticfiles into Nginx container
- ./staticfiles:/usr/share/nginx/html/static

db:
image: postgres:15
environment:
POSTGRES_USER: your_user
POSTGRES_PASSWORD: your_password
POSTGRES_DB: your_db
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

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.

Suggested change
POSTGRES_USER: your_user
POSTGRES_PASSWORD: your_password
POSTGRES_DB: your_db
POSTGRES_USER: ${POSTGRES_USER}
POSTGRES_PASSWORD: ${POSTGRES_PASSWORD}
POSTGRES_DB: ${POSTGRES_DB}

volumes:
- db_data:/var/lib/postgresql/data

volumes:
db_data:
7 changes: 2 additions & 5 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..."
echo "Starting Gunicorn..."
exec gunicorn core.wsgi:application --bind 0.0.0.0:8000 --timeout 600 --workers 3 --log-level info
10 changes: 10 additions & 0 deletions src/website/nginx/Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
FROM nginx:alpine

# Remove default configuration
RUN rm /etc/nginx/conf.d/default.conf

# Copy your custom nginx configuration
COPY nginx.conf /etc/nginx/nginx.conf

# Expose port 80 for Nginx
EXPOSE 80
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

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.

Suggested change
# 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

51 changes: 51 additions & 0 deletions src/website/nginx/nginx.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
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;

# Set client body size limit to 10MB
client_max_body_size 10M;

upstream django_app {
# Points to the Django/Gunicorn container service name defined in docker-compose
server web:8000;
}

server {
listen 80;
server_name _;

# 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";
}
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

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.

Suggested change
# 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";
}


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;
}
}
}
Loading