-
Notifications
You must be signed in to change notification settings - Fork 6
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
[WIP]: Harden images and reorg #647
base: master
Are you sure you want to change the base?
Conversation
c7a0561
to
2106e20
Compare
Hey, could the Docker Compose .yaml files be restored, please? We still use that as a single node alternative to K8S/Helm? Thanks! |
@sashakames first I just want to say that I know this is rough and needs a lot of work so you're not going to hurt my feelings, please give it a thorough scrub 😁 re: Docker compose -- my drive there was to consolidate the various pieces into a single Docker compose file that new users could stand up out-of-the-box and not need to configure anything. Then an overlay compose file could be used to override things that needed to be changed for production (in conjunction with the existing env vars). So, there should now be only two Docker compose related files in the root of the repo. (Fair warning that I can't fully test the Production setup so I'm hoping you can 😁 ) Does that address your concern? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-review to help with explaining some rationale
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The *.env files were removed in favor of ensuring that all settings had a sane default out of the box though each service still respects settings loaded from .env files as before in order to preserve backwards compatibility
@@ -1,4 +1,4 @@ | |||
FROM python:3.9-slim-buster | |||
FROM python:3.9-slim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing the -buster
suffix allows automatic building on newer versions to avoid outdated packages
COPY ./docker/production/django/entrypoint /entrypoint | ||
RUN sed -i 's/\r$//g' /entrypoint | ||
RUN chmod +x /entrypoint | ||
RUN chown django /entrypoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Files and directories owned by the running user are a security risk. Removing the entrypoint script in favor of running the gunicorn server directly allows avoiding the user chown
COPY --chown=django:django . /app | ||
|
||
USER django | ||
RUN python /app/manage.py collectstatic --noinput \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running collectstatic
during build allows faster startup and an immutable image
USER django | ||
RUN python /app/manage.py collectstatic --noinput \ | ||
&& mkdir -p /app/staticfiles/.well-known \ | ||
&& cyclonedx-py requirements requirements/base.txt --output-format json --outfile /app/staticfiles/.well-known/bom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Satisfies Executive Order 14028
# the production load balancer (ingress controller in k8s, Traefik on | ||
# bare metal/vms) should route these paths before they reach this container. | ||
location ~* ^/proxy|api|accounts|account-confirm-email|swagger|redoc|frontend-config\.js { | ||
proxy_pass http://10.89.9.82:5000$request_uri; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using ipaddress instead of http://django
as nginx's proxy_pass
doesn't include docker's DNS resolver by default. We could add resolver 127.0.0.11 ipv6=off;
to point it to docker compose's DNS resolver, but then it's hard coded and doesn't work for things like podman compose
gtag('js', new Date()); | ||
gtag('config', '%REACT_APP_GOOGLE_ANALYTICS_TRACKING_ID%'); | ||
<!-- Load config from backend --> | ||
<script type="text/javascript"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the most important frontend change -- load FRONTEND_SETTINGS
from the backend endpoint and make it available on the window
object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could more be tested here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make all routes relative to the page they're being served from so that the same image can be served from any url
@@ -316,13 +313,9 @@ const DatasetDownloadForm: React.FC<React.PropsWithChildren<unknown>> = () => { | |||
} | |||
}) | |||
.catch(async (error: ResponseError) => { | |||
if (error.message !== '') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always log errors to console so they show up in test logs
@bstrdsmkr You'll need to merge/rebase with the update to |
Description
Type of change
How Has This Been Tested?
Checklist