Add Nginx target to API Dockerfile

[sarayourfriend]

Fixes

Fixes #821 by @sarayourfriend

Description

This PR adds a new nginx target to the Dockerfile that allows us to build an nginx image with the API's static files built into it. As described in the linked issue, this allows us to get around the need to distribute these files inside the Django container and without needing to share mounts across images in the ECS service.

The reason we cannot just store these in the Django image and share across volumes anyway is that collectstatic runs the entire Django app's settings, and therefore requires connections to Elasticsearch to work. I tried working around this so that it didn't require running the entire Docker compose stack, but there's a lot of assumptions built into how our Django app settings are configured that just assume that ES, Redis, and a database will be available. That's not a bad thing or something we need to change, but it would require a lot more work than just splitting the nginx target out to a separate build step in the CI. It was much easier to solve this way than to pull apart the assumptions built into the Django app (which would be riskier to change anyway).

Thanks @AetherUnbound for helping debug the networking of the running image.

Testing Instructions

Build and run the container as described above. Make test requests to ensure it works, against the following running container:

cd api
just collectstatic
docker build --target nginx . -t openverse-api-nginx:latest
docker run --rm -p 8080:8080 -e DJANGO_UPSTREAM_URL='api.openverse.engineering' -it openverse-api-nginx:latest

You won't get a successful response back because the image isn't able to make a valid request to api.openverse.engineering that Cloudflare will accept.

I can't think of a way to get this to successfully run inside of docker-compose as the image requires the compose stack to be running beforehand to even build due to needing collectstatic to populate the local filesystem's ./api/static directory.

Checklist

• My pull request has a descriptive title (not a vague title like Update index.md).
• My pull request targets the default branch of the repository (main) or a parent feature branch.
• My commit messages follow best practices.
• My code follows the established code style of the repository.
• I added or updated tests for the changes I made (if applicable).
• I added or updated documentation (if applicable).
• I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

[github-actions]

API Developer Docs Preview: Ready

https://wordpress.github.io/openverse-api/_preview/990

Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again.

You can check the GitHub pages deployment action list to see the current status of the deployments.

[AetherUnbound]

Okay admittedly I'm not sure I understand all the pieces in play here, but this does work locally for me! The following command ran:

$ docker run --rm -p 8080:8080 -e DJANGO_UPSTREAM_URL='api.openverse.engineering' -it openverse-api-nginx:latest

I ran into issues when I tried to hit http://localhost:8080, but the response I got back was 400 Bad Request (cloudflare). I take it to mean it's forwarding to api.openverse.engineering correctly!

Visiting a URL like http://localhost:8080/static/admin/img/search.svg returned the static file as expected too.

I tried some variations of DJANGO_UPSTREAM_URL, namely localhost:50820 and 172.17.0.1:50820 but I wasn't able to get either to work.

[AetherUnbound]

Got it working, accidentally swapped port numbers! It should be docker run --rm -p 8080:8080 -e DJANGO_UPSTREAM_URL='172.17.0.1:50280' -it openverse-api-nginx:latest (50280 not 50820). Now I'm getting this error, but at least it's coming from Django!

<h1>DisallowedHost
       at /</h1>
  <pre class="exception_value">Invalid HTTP_HOST header: '\\localhost:8080'. The domain name provided is not valid according to RFC 1034/1035.</pre>
  

Request Method: | GET
-- | --
http://\localhost:8080/
4.1.2
DisallowedHost
Invalid HTTP_HOST header: '\\localhost:8080'. The domain name provided is not valid according to RFC 1034/1035.
/venv/lib/python3.10/site-packages/django/http/request.py, line 148, in get_host
django.views.generic.base.RedirectView
/venv/bin/python
3.10.8
['/api',  '/usr/local/lib/python310.zip',  '/usr/local/lib/python3.10',  '/usr/local/lib/python3.10/lib-dynload',  '/venv/lib/python3.10/site-packages']
Fri, 28 Oct 2022 20:43:30 +0000

[sarayourfriend]

Thanks for taking a look, Madison 🙂 I wonder what I was doing wrong on my end that it wasn't working 🤔

Glad it's working in any case. When I can return to this next week it'll be fun to undraft it soon!

[openverse-bot]

Based on the medium urgency of this PR, the following reviewers are being gently reminded to review this PR:

@krysal
@AetherUnbound
This reminder is being automatically generated due to the urgency configuration.

Excluding weekend¹ days, this PR was updated 4 day(s) ago. PRs labelled with medium urgency are expected to be reviewed within 4 weekday(s)².

@sarayourfriend, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings.

Footnotes

1. Specifically, Saturday and Sunday. ↩

2. For the purpose of these reminders we treat Monday - Friday as weekdays. Please note that the that generates these reminders runs at midnight UTC on Monday - Friday. This means that depending on your timezone, you may be pinged outside of the expected range. ↩

[krysal]

This is very interesting, I didn't think about the possibility of a scoped stage build, good to know that is an option! 💡 I'm not grabbing yet why is not possible to separate the API and Nginx images since it seems we can share files between actions and state the order of the image builds 🤔

Also, currently, this command doesn't work for me:

docker build --target nginx . -t openverse-api-nginx:latest

This is the output I get:

[+] Building 0.0s (1/2)                                                                                                                               
 => [internal] load build definition from Dockerfile                                                                                             0.0s
 => => transferring dockerfile: 2B                                                                                                               0.0s
failed to solve with frontend dockerfile.v0: failed to read dockerfile: open /var/lib/docker/tmp/buildkit-mount3456991816/Dockerfile: no such file or directory

I hope is not something with Docker at macOS only. I tried with docker-compose as well and it didn't work: unknown flag: --target

[krysal]

I tried again changing to the api directory before the docker commands and it works! With a little addition.

[sarayourfriend]

I'm not grabbing yet why is not possible to separate the API and Nginx images since it seems we can share files between actions and state the order of the image builds

Can you clarify what you meant here? I'm not understanding what this is in reference to as the goal of this PR is to separate the API and Nginx images. Is it something with the GitHub actions?

[krysal]

Thank you for adding the steps to test it!

What I mean is the way in which the Nginx image is built under the api's Dockerfile is new to me (really cool though as I said!) Before this PR I was thinking the separation of Nginx and the API server would require another service entry in the Docker Compose file, as is currently being setup in the EC2 instance. The building from the Dockerfile doesn't tweak the original Nginx image, it only puts configuration and static files in and seems kind of odd being there, it's not affecting the API image built. I understand there is complexity with the Django's static files though, and this solution looks easier (now that it's already done!)

I imagine the alternative would be to make Terraform produce the static files in the API and then share them between the API and Nginx images (?) Anyway, that can be researched later.

Thanks for this amazing work!I just let some suggestion to use the last versions of GitHub actions and a question.

PD: Can we remove the "proxy" service from the docker-compose.yml after this? Not asking to include it in this PR but we're not using it.

[zackkrida]

This looks good. I wasn't able to get it working with 8080 as my local port, which took some debugging to figure out, but once I switched my local port to any other value it works great. I'm seeing the 400 Bad Request cloudflare message served by NGINX. The STATIC_ROOT documentation in the justfile is helpful too.

[sarayourfriend]

@krysal I've updated the actions 👍

Also, I realised that the obvious test for this I didn't mention or think about until yesterday, is to try to retrieve a static file. If you hit http://localhost:8080/static/admin/css/base.css locally after running the image with port 8080 exposed, you should get a CSS file. This should match this file from the production API: https://api.openverse.engineering/static/admin/css/base.css

[sarayourfriend]

I'm reverting the updates to the action versions to see if it fixes the ingestion server test failures. If not I'll reapply them and then ping for help on the test failures.

[AetherUnbound]

I was able to get this working and tested out the new instructions you provided with success (@zackkrida I also had a local port conflict with a proxy I was running, if that's what you were also running into).

This looks great!