Refactor frontend container

[jmgrady]

Split frontend container into two - one for the frontend running Nginx as a reverse proxy and one for a certificate manager.

Closes #696

---

This change is 

[codecov-io]

Codecov Report

Merging #759 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #759   +/-   ##
=======================================
  Coverage   51.16%   51.16%           
=======================================
  Files         235      235           
  Lines        6240     6240           
  Branches      404      404           
=======================================
  Hits         3193     3193           
  Misses       2749     2749           
  Partials      298      298           

Flag	Coverage Δ
#backend	55.30% <ø> (ø)
#frontend	47.32% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a6a453...e318a5b. Read the comment docs.

[johnthagen]

---

certmgr/scripts/entrypoint.sh, line 20 at r2 (raw file):

esac

exit 99

Is there significance to using 99 as an exit code here?

[imnasnainaec]

Reviewed 2 of 15 files at r1, 3 of 19 files at r2.
Reviewable status: 5 of 24 files reviewed, 4 unresolved discussions (waiting on @imnasnainaec, @jmgrady, and @johnthagen)

---

.gitignore, line 19 at r2 (raw file):

.env.backend
.env.frontend
.env.certmgr

Is .env.* too general to use?

---

scripts/docker_setup.py, line 113 at r2 (raw file):

            env_var["value"] = os.environ[env_var["key"]]

    # Set backend common env_vars if they are defined for our process

This comment could use updating from "common" to "backend"

---

scripts/docker_setup.py, line 116 at r2 (raw file):

Quoted 8 lines of code…

    # Set backend private env_vars if they are defined for our process
    for env_var in dev_config["combine_private_env_vars"]:
        if env_var["key"] in os.environ:
            env_var["value"] = os.environ[env_var["key"]]
    # Set backend common env_vars if they are defined for our process
    for env_var in dev_config["combine_backend_env_vars"]:
        if env_var["key"] in os.environ:
            env_var["value"] = os.environ[env_var["key"]]

Only _private_ and _backend_ are set here, but not _frontend_ or _cert_... just making sure that is as it should be.

[johnthagen]

---

certmgr/scripts/README.md, line 31 at r2 (raw file):

| ----------------- | ----------------------------------------------------------------------------------------------------------------------------------------------- |
| CERT_MODE         | May be one of `self-signed`, `letsencrypt`, `cert-server`, or `cert-client` to specify which of the certificate management personalities to use |
| CERT_EMAIL        | Set to 1 to remove old certificates first                                                                                                       |

I assume this is specified by certbot but the description/effect was surprisingly for CERT_EMAIL

[johnthagen]

---

certmgr/scripts/README.md, line 74 at r2 (raw file):

When installing TheCombine for using self-signed certificates, e.g. for the QA server
or for development use, the following steps are required:
  1. Build each container:

Does this mean to build them independently rather than with Docker Compose (docker-compose build --parallel)?

[johnthagen]

---

certmgr/scripts/README.md, line 40 at r2 (raw file):

### selfsigned.sh
`selfsigned.sh` implements the certmgr behavior when `CERT_MODE` is set to `self-signed`.
It creates a self-signed certificate to be used by the nginx web server and enters a loop to periodically check to see if the certificate needs to be renewed/recreated.  The certificate is valid for 10 years and will be renewed when there are less than 10 days until is expires.

Not a huge deal, but it would be easier to read this file side by side other files if the contents were wrapped to 120 characters.

[johnthagen]

---

.gitignore, line 19 at r2 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

.env.backend
.env.frontend
.env.certmgr

Is .env.* too general to use?

That could work? I know we want to keep .env for React, but it think you're suggestion could work.

[johnthagen]

---

docker_deploy/group_vars/server/main.yml, line 20 at r2 (raw file):

# SSL cert variables specific to Server targets
cert_mode: "letsencrypt"
cert_email: "jimgrady.jg@gmail.com"

Do we want your personal email committed into the repo?

[johnthagen]

---

certmgr/scripts/selfsigned.sh, line 18 at r2 (raw file):

while true; do
  renew_selfsigned_cert
  sleep 60 &

I think we could slow this down. Self-signed certs are mostly going to be used for development, but checking this often leaves a good amount of logs in docker-compose logs -f. Perhaps we could slow this down to 1 day?

[imnasnainaec]

Reviewable status: 5 of 24 files reviewed, 9 unresolved discussions (waiting on @jmgrady and @johnthagen)

[johnthagen]

Reviewed 3 of 15 files at r1, 17 of 19 files at r2.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @jmgrady)

---

certmgr/scripts/README.md, line 40 at r2 (raw file):

Previously, johnthagen wrote…

Not a huge deal, but it would be easier to read this file side by side other files if the contents were wrapped to 120 characters.

As for the 10 years part, don't browsers only accept certificates that are valid for 1 year now? Perhaps we should reduce the validity of the cert life to make it match typical certs, especially if we are going to auto-renew.

[jmgrady]

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @johnthagen)

---

.gitignore, line 19 at r2 (raw file):

Previously, johnthagen wrote…

That could work? I know we want to keep .env for React, but it think you're suggestion could work.

I thought that is the usual pattern for docker-compose. c.f. https://docs.docker.com/compose/environment-variables/
Let me know if you prefer a different naming convention.

---

Dockerfile, line 14 at r2 (raw file):

Previously, johnthagen wrote…

Any reason not to use the latest, 1.19.3?

1.18 is listed as the stable version on https://hub.docker.com/_/nginx

---

certmgr/scripts/entrypoint.sh, line 20 at r2 (raw file):

Previously, johnthagen wrote…

Is there significance to using 99 as an exit code here?

None at all but it should never get here so I wanted an obvious indication of that (at least to me).

---

certmgr/scripts/func.sh, line 1 at r2 (raw file):

Previously, johnthagen wrote…

I know this would be a bit of work at this point, but for the amount of logic involved in these scripts, I personally think using a full programming language like Python would make this a lot more maintainable. bash starts to scale poorly at this size in my opinion.

This would also have the added benefit that certbot image already comes with Python 3.8 and we wouldn't need to install bash and curl at all.

With Python, we can also apply more software engineering tooling to this code (linting, type checking, formatting, etc).

Perhaps but I don't think that the logic is very complex. In addition, Python would require running simple commands to be replaced by more cumbersome subprocess calls.

---

certmgr/scripts/README.md, line 1 at r2 (raw file):

Previously, johnthagen wrote…

Should this README be moved up one directory rather than being in the scripts folder?

Done.

---

certmgr/scripts/README.md, line 31 at r2 (raw file):

Previously, johnthagen wrote…

I assume this is specified by certbot but the description/effect was surprisingly for CERT_EMAIL

No. I was rearranging the table and missed that. Done.

---

certmgr/scripts/README.md, line 40 at r2 (raw file):

Previously, johnthagen wrote…

As for the 10 years part, don't browsers only accept certificates that are valid for 1 year now? Perhaps we should reduce the validity of the cert life to make it match typical certs, especially if we are going to auto-renew.

Looks fine in Atom. ;-)
Done - except for the table - Markdown table specs make it difficult to do that and keep the table. (at least with my knowledge)

Browsers don't accept self-signed certs anyway. You have to override it to get to the website.

---

certmgr/scripts/README.md, line 74 at r2 (raw file):

Previously, johnthagen wrote…

Does this mean to build them independently rather than with Docker Compose (docker-compose build --parallel)?

Yes because they are not built on the targets. But perhaps I'm making a bad assumption. I suppose I can build them on the TeamCity server using docker-compose and then push them to AWS ECR without trying to run them on the build agent.

---

certmgr/scripts/selfsigned.sh, line 18 at r2 (raw file):

Previously, johnthagen wrote…

I think we could slow this down. Self-signed certs are mostly going to be used for development, but checking this often leaves a good amount of logs in docker-compose logs -f. Perhaps we could slow this down to 1 day?

Done.

Note that nothing gets logged unless you set verbose to 1 or the certificate gets renewed (10 years from now).

---

docker_deploy/group_vars/server/main.yml, line 20 at r2 (raw file):

Previously, johnthagen wrote…

Do we want your personal email committed into the repo?

I really want an SIL email. I will move it to the vault.

---

scripts/docker_setup.py, line 113 at r2 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

This comment could use updating from "common" to "backend"

There are two types of environment variables for the backend. "Common" which are defined in vars/common_config.yml, and "private" which are defined in the encrypted vars/vault_config.yml file. At least that is the intent. Currently there are no common variables defined for the backend.

---

scripts/docker_setup.py, line 116 at r2 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

    # Set backend private env_vars if they are defined for our process
    for env_var in dev_config["combine_private_env_vars"]:
        if env_var["key"] in os.environ:
            env_var["value"] = os.environ[env_var["key"]]
    # Set backend common env_vars if they are defined for our process
    for env_var in dev_config["combine_backend_env_vars"]:
        if env_var["key"] in os.environ:
            env_var["value"] = os.environ[env_var["key"]]

Only _private_ and _backend_ are set here, but not _frontend_ or _cert_... just making sure that is as it should be.

This is correct. What this is doing is looking up the values in your environment variables if they are defined. For the backend, most of these environment variable values are secrets that we don't want to put into the Python script. Most developers will have these environment variables defined so that npm start will work. This saves you from having to manually edit the environment variable file manually to add the secrets.

[johnthagen]

---

.gitignore, line 19 at r2 (raw file):

Previously, jmgrady (Jim Grady) wrote…

I thought that is the usual pattern for docker-compose. c.f. https://docs.docker.com/compose/environment-variables/
Let me know if you prefer a different naming convention.

I think @imnasnainaec was saying that perhaps we could generalize the .gitignore lines into one, but not change the actual file names.

[imnasnainaec]

Reviewable status: 21 of 25 files reviewed, 9 unresolved discussions (waiting on @jmgrady and @johnthagen)

---

.gitignore, line 19 at r2 (raw file):

Previously, johnthagen wrote…

I think @imnasnainaec was saying that perhaps we could generalize the .gitignore lines into one, but not change the actual file names.

Indeed.

[johnthagen]

---

Dockerfile, line 14 at r2 (raw file):

Previously, jmgrady (Jim Grady) wrote…

1.18 is listed as the stable version on https://hub.docker.com/_/nginx

Huh, I hadn't noticed that before. According to this post, mainline is recommended though? https://serverfault.com/a/715126

[johnthagen]

---

certmgr/scripts/README.md, line 74 at r2 (raw file):

Previously, jmgrady (Jim Grady) wrote…

Yes because they are not built on the targets. But perhaps I'm making a bad assumption. I suppose I can build them on the TeamCity server using docker-compose and then push them to AWS ECR without trying to run them on the build agent.

Yeah, seems like it would be safest to build them using compose just so that we are consistently using that same configuration as much as we can.

[johnthagen]

---

certmgr/scripts/entrypoint.sh, line 20 at r2 (raw file):

Previously, jmgrady (Jim Grady) wrote…

None at all but it should never get here so I wanted an obvious indication of that (at least to me).

Could we add a comment stating that it should never reach this point?

[johnthagen]

---

certmgr/scripts/func.sh, line 1 at r2 (raw file):

Previously, jmgrady (Jim Grady) wrote…

Perhaps but I don't think that the logic is very complex. In addition, Python would require running simple commands to be replaced by more cumbersome subprocess calls.

To really streamline, you can use os.system("command") to do the equivalent of bash and not do any checking/piping of values.

[johnthagen]

Reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jmgrady)

[johnthagen]

---

Dockerfile, line 14 at r2 (raw file):

Previously, johnthagen wrote…

Huh, I hadn't noticed that before. According to this post, mainline is recommended though? https://serverfault.com/a/715126

Here they recommend mainline in general: https://www.nginx.com/blog/nginx-1-6-1-7-released/#Which-Version-Should-I-Use

[jmgrady]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @johnthagen)

---

.gitignore, line 19 at r2 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

Indeed.

Sorry. I was being a little dense. So, I would specify .env.* so that .env can be kept for React, correct?

---

Dockerfile, line 14 at r2 (raw file):

Previously, johnthagen wrote…

Here they recommend mainline in general: https://www.nginx.com/blog/nginx-1-6-1-7-released/#Which-Version-Should-I-Use

OK. That works for me. I can specify 1.19 and get the latest of 1.19.x, correct?

---

certmgr/scripts/entrypoint.sh, line 20 at r2 (raw file):

Previously, johnthagen wrote…

Could we add a comment stating that it should never reach this point?

Actually, it will get there if you specify a mode that is not implemented yet or does not exist. I can pick a different return code if you prefer.

---

certmgr/scripts/README.md, line 74 at r2 (raw file):

Previously, johnthagen wrote…

Yeah, seems like it would be safest to build them using compose just so that we are consistently using that same configuration as much as we can.

The other problem is that the docker-compose.yml is not installed on the build agents. The docker-compose.yml should not influence the build of the container - it should just use the Dockerfile.

[jmgrady]

Reviewable status: 23 of 25 files reviewed, 2 unresolved discussions (waiting on @imnasnainaec and @johnthagen)

---

Dockerfile, line 14 at r2 (raw file):

Previously, jmgrady (Jim Grady) wrote…

OK. That works for me. I can specify 1.19 and get the latest of 1.19.x, correct?

Done

[johnthagen]

Reviewed 2 of 2 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved