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.

Sign up for GitHub

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

May Security Update #610

Merged
merged 5 commits into from
Jun 7, 2022
Merged

May Security Update #610

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fda6197
auth0 changes + supd, py38
willronchetti Jun 7, 2022
68c9b6f
lock changes
willronchetti Jun 7, 2022
b9858ca
Merge branch 'master' into auth02
willronchetti Jun 7, 2022
809863a
build in 3.8
willronchetti Jun 7, 2022
35d0d06
Merge branch 'auth02' of https://github.com/dbmi-bgm/cgap-portal into…
willronchetti Jun 7, 2022
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
4 changes: 2 additions & 2 deletions .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ on:
jobs:
# This workflow contains a single job called "build"
build:
name: Test Suite for cgap-portal (Python 3.7, Node 16)
name: Test Suite for cgap-portal (Python 3.8, Node 16)
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we done a check to see if names can usefully have environment variables in them? It feels like that would save us a lot of little updates like this if it works. It might not, though.


# The type of runner that the job will run on
runs-on: ubuntu-18.04
Expand All @@ -27,7 +27,7 @@ jobs:
matrix:
test_type: ['UNIT', 'NPM', 'Docker']
# We are really not set up for these next two to be multiplicative, so be careful adding more.
python_version: ['3.7']
python_version: ['3.8']
node_version: ['16']

# Steps represent a sequence of tasks that will be executed as part of the job
Expand Down
37 changes: 21 additions & 16 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
# CGAP-Portal (Production) Dockerfile
# Take latest 3.7.12 Debian variant
FROM python:3.7.12-slim-buster
# bullseye seems to perform worse
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this note about bullseye is worth mentioning, though it would have been more helpful if dated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe this comment is accurate as it pre-dates the performance optimizations. I suspect if I were to build with bullseye, performance would be comparable.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK.

#FROM python:3.7.12-slim-bullseye
# Take latest 3.8.13 Debian variant
FROM python:3.8.13-slim-buster

MAINTAINER William Ronchetti "[email protected]"

Expand Down Expand Up @@ -35,7 +33,7 @@ WORKDIR /home/nginx/.nvm
ENV NVM_DIR=/home/nginx/.nvm
COPY deploy/docker/production/install_nginx.sh /install_nginx.sh
RUN apt-get update && apt-get upgrade -y && \
apt-get install -y --no-install-recommends vim emacs net-tools ca-certificates \
apt-get install -y --no-install-recommends vim emacs net-tools ca-certificates build-essential \
gcc zlib1g-dev postgresql-client libpq-dev git make curl libmagic-dev && \
pip install --upgrade pip && \
curl -sSL https://install.python-poetry.org | POETRY_HOME=/opt/venv python - && \
Expand Down Expand Up @@ -114,24 +112,31 @@ RUN chown nginx:nginx development.ini
RUN chmod +x entrypoint_local.sh

# Production setup
RUN touch production.ini
RUN touch session-secret.b64
RUN chown nginx:nginx session-secret.b64
RUN chown nginx:nginx production.ini
RUN chown nginx:nginx poetry.toml
RUN chown nginx:nginx poetry.toml && \
touch production.ini && \
chown nginx:nginx production.ini && \
touch session-secret.b64 && \
chown nginx:nginx session-secret.b64 && \
touch supervisord.log && \
chown nginx:nginx supervisord.log && \
touch supervisord.sock && \
chown nginx:nginx supervisord.sock && \
touch supervisord.pid && \
chown nginx:nginx supervisord.pid
COPY deploy/docker/production/$INI_BASE deploy/ini_files/.
COPY deploy/docker/production/entrypoint.sh .
COPY deploy/docker/production/entrypoint_portal.sh .
COPY deploy/docker/production/entrypoint_deployment.sh .
COPY deploy/docker/production/entrypoint_indexer.sh .
COPY deploy/docker/production/entrypoint_ingester.sh .
COPY deploy/docker/production/supervisord.conf .
COPY deploy/docker/production/assume_identity.py .
RUN chmod +x entrypoint.sh
RUN chmod +x entrypoint_deployment.sh
RUN chmod +x entrypoint_deployment.sh
RUN chmod +x entrypoint_indexer.sh
RUN chmod +x entrypoint_ingester.sh
RUN chmod +x assume_identity.py
RUN chmod +x entrypoint.sh && \
chmod +x entrypoint_deployment.sh && \
chmod +x entrypoint_deployment.sh && \
chmod +x entrypoint_indexer.sh && \
chmod +x entrypoint_ingester.sh && \
chmod +x assume_identity.py
EXPOSE 8000

# Container does not run as root
Expand Down
3 changes: 1 addition & 2 deletions base.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,5 +61,4 @@ timeout = 60

[filter:memlimit]
use = egg:encoded#memlimit
rss_limit = 500MB
rss_percent_limit = 20
Copy link
Contributor

Choose a reason for hiding this comment

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

20% of what? I'm not sure what's going on here but it feels like 20% is a very low limit. Is this because we've got 4 processes that are each taking 25%? Or 5 that this allows 100% of each? (That seems questionable, but maybe it is discounted to 80% of the specified amount elsewhere.) If it's weird math like that, it seriously calls for a comment explaining how the value was chosen. But really, I'd like such a comment regardless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The functionality that uses this is based on psutil and does not work in Fargate, see giampaolo/psutil#2100. So in effect we are capping portal workers at 450MB of memory, while previously there was effectively no cap. If the cap is reached, the worker is killed and restarted by supervisor.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just what that info in a comment in the code.

rss_limit = 450MB
14 changes: 3 additions & 11 deletions deploy/docker/production/entrypoint_portal.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,6 @@ poetry run python -m assume_identity
# Start nginx proxy
service nginx start

# Start application
echo "Starting server 1"
pserve production.ini http_port=6543 &
echo "Starting server 2"
pserve production.ini http_port=6544 &
echo "Starting server 3"
pserve production.ini http_port=6545 &
echo "Starting server 4"
pserve production.ini http_port=6546 &
echo "Starting server 5"
pserve production.ini http_port=6547
# Start application workers
echo "Starting supervisor"
supervisord -c supervisord.conf
6 changes: 5 additions & 1 deletion deploy/docker/production/nginx.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ http {
types_hash_max_size 2048;
types_hash_bucket_size 64;

# If 502 is encountered, we may have been killed for memory
# fall back to the next server, until one is back
proxy_next_upstream error timeout http_502;

# Allow large POST requests
client_body_buffer_size 128K;
client_header_buffer_size 1k;
Expand Down Expand Up @@ -66,7 +70,7 @@ http {
server 0.0.0.0:6544 fail_timeout=45;
server 0.0.0.0:6545 fail_timeout=45;
server 0.0.0.0:6546 fail_timeout=45;
server 0.0.0.0:6547 fail_timeout=45;
server 0.0.0.0:6547 fail_timeout=45 backup;
keepalive 64;
}

Expand Down
54 changes: 54 additions & 0 deletions deploy/docker/production/supervisord.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
[supervisord]
pidfile=%(here)s/supervisord.pid
logfile_maxbytes=50MB
logfile_backups=10
loglevel=info
nodaemon=true
minfds=1024
minprocs=200
user=nginx

[program:cgap1]
autorestart=true
startsecs=6
command=pserve production.ini http_port=6543
stdout_logfile=/dev/stdout
stdout_logfile_maxbytes=0
stderr_logfile_maxbytes=0
redirect_stderr=true

[program:cgap2]
autorestart=true
startsecs=6
command=pserve production.ini http_port=6544
stdout_logfile=/dev/stdout
stdout_logfile_maxbytes=0
stderr_logfile_maxbytes=0
redirect_stderr=true

[program:cgap3]
autorestart=true
startsecs=6
command=pserve production.ini http_port=6545
stdout_logfile=/dev/stdout
stdout_logfile_maxbytes=0
stderr_logfile_maxbytes=0
redirect_stderr=true

[program:cgap4]
autorestart=true
startsecs=6
command=pserve production.ini http_port=6546
stdout_logfile=/dev/stdout
stdout_logfile_maxbytes=0
stderr_logfile_maxbytes=0
redirect_stderr=true

[program:cgap5]
autorestart=true
startsecs=6
command=pserve production.ini http_port=6547
stdout_logfile=/dev/stdout
stdout_logfile_maxbytes=0
stderr_logfile_maxbytes=0
redirect_stderr=true
Loading