-
Notifications
You must be signed in to change notification settings - Fork 12
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
feat(schematic): added nginx to schematic docker container #2403
feat(schematic): added nginx to schematic docker container #2403
Conversation
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 PR introduces changes that break best practices (security) and standards that the monorepo aims to establish. I suggest to schedule a call to clarify the content of this review.
@tschaffter a nice overview - adding @andrewelamb for awareness. |
@tschaffter Do you mind reviewing this PR again? I have stored SSL private key and certificate on AWS, and after more trouble shooting, the deployment seems to be working now: https://schematic-dev-refactor.api.sagebionetworks.org/api/v1/ui/. If the Dockerfile looks good to you, I think I could work on resolve conflicts and merge this PR. |
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.
I skimmed thought the PR and made a few comment. I can make a formal review once the PR is ready (after cleanup and conflict resolution).
Does the command nx serve schematic-api
listen to the new port? This is a convention of the monorepo so that developers can run the REST API either manually or with Docker while keeping the same behavior for the REST API.
@linglp You check is failing. The error message should point to the solution :-) |
Quality Gate passed for 'schematic-api'Issues Measures |
@tschaffter If the PR looks good, maybe you could merge it on your end? I am not authorized to merge the PR. Thank you again for reviewing it |
Context
To make sure that schematic APIs could be deployed to production, we have to add nginx. This is similar but different from what I did here in this PR: Sage-Bionetworks/schematic#1202
Changes
Some configuration files are the same as before. For example,
self-signed.conf
,ssl-params.conf
,uwsgi.ini
.. but there are also some changes:redirect.conf
tiangolo/uwsgi-nginx-flask:python3.10
, the python version that we use in schematic API has to match the python version allowed by the base image. I ended up having to change python version from3.10.12
to3.10.13
in mono repo. If you donx prepare schematic-api
again, you should be able to get3.10.13
root
folder andapp
folder. But this is NOT recommended. Please see "future items" for more details.To test this PR:
cd apps/schematic/api
apps/schematic/api
folder, make sure that you have renamed.env.example
to.env
or you already have a.env
filenx prepare schematic-api
to get the correct version of Pythonpoetry shell
to make sure that you are in an virtual environment. And to prepare for certificate and private key locally, runprepare_key_certificate.py
This script will generateprivate_localhost_certificate.crt
andprivate_localhost.key
files and save the content of private key and certificate as environment variables in .envdocker build -t schematic-api .
And run the schematic command by doing:
docker run -d --name mycontainer --env-file .env -p 7443:7443 schematic-api
OR
nx build-image schematic-api
when running
docker images
, you should be able to see something like:And then you could create a docker container by doing something like:
docker run -p 7443:7443 -t <docker image id>
or you could go back to the root folder where you ran "docker build-image" command and ran:
nx serve-detach schematic-api
And if you are using VS code, you should be able to open the docker container like this:
When seeing the page below:
make sure you click on: Advanced -> "Proceed to localhost(unsafe)"
Test by
curl
Using
curl
to test and ensure that everything is working as expected. The actual port number depends on your setting. Below are examples:curl -I https://localhost:7443/api/v1/ui/
I saw:
This makes sense because we are using a self-signed certificate
curl -I -k https://localhost:7443/api/v1/ui/
I saw:
This is also expected because
-k
allow connections to SSL sites without certificates or to ignore certificate validation errors.curl -I -k http://localhost:7443/api/v1/ui/
:This makes sense because we redirect http to https
Future items
This Dockerfile is not a final product. There are some important issues that need to be address before actually using it for production deployment:
.synapseCache is stored in
/root
This is not a good practice. But currently schematic doesn't allow cache of .synapseCache folder to be configurable. This work is captured in ticket: FDS 1446
After we move
.synapseCache
out of root folder, we need to revisit this dockerfile and update the file permission according to the new location of cache.Run the dockerfile as a non-root user
I tried adding
USER www-data
in dockerfile but it didn't work because the base image:tiangolo/uwsgi-nginx-flask:python3.10
that we are using is usingsupervisord
to bind pots. And binding pots in docker requires super user permission. I can't havewww-data
to bind pots or work withsupervisord
for now. This will require additional tickets and attention if we want to meet the best practice.purging cache function needs to be moved out of schematic library
The purging cache function in schematic library currently calculate the cache size of .synapseCache and if it meets a certain criteria, the cache will get purge. We should move that logic to the mono repo (see issue FDS 1445)
Modify check_synapse_cache_size function to deal with empty .synapseCache folder
For the container built in schematic library, even when
.synapseCache
is empty,du -sh .synapseCache
returns 4KB. But this is not the case when I tried running the same command in this docker container for some reasons. When .synapseCache folder is empty,du -sh .synapseCache
returns 0 without a unit. We have to modifycheck_synapse_cache_size
to deal with 0. (See FDS 1445)Note:
This should be merged after: #2417