[keyserver] Introduce getDBConfig that can source from environmental variables
ClosedPublic
Actions

Authored by ashoat on May 20 2022, 2:55 PM.

Details

Reviewers

tomek
atul

Commits

Summary

As we Dockerize the keyserver, we need to make it possible to define config via environmental variables.
In most of those cases, we can define the entire JSON file as an environmental variable.
But for the case of the DB config, we'd like to be able to share a config style with the mysql Docker image, which uses separate environmental variables for each key
So we're going to special-case DB config here
The rest of the configs will be handled through importJSON in a following diff

Depends on D4025

Test Plan

Test to make sure the keyserver still runs and can load the website with db_config.json defined
Test to make sure scripts that need DB access (such as reset-password.js) still runs with db_config.json defined
In combination with a later diff I'll make sure to test that the environmental variables work too

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

ashoat requested review of this revision.May 20 2022, 3:00 PM

tomek requested changes to this revision.May 23 2022, 2:57 AM

tomek added inline comments.

keyserver/src/database/database.js
34–36 ↗	(On Diff #12989)	This sounds fragile as we assume here that `loadPool` was called at least once before calling this function. It looks like this function is only used in `endScript`, so maybe we should instead expose a function `endPool` that ends the pool if it was loaded? For all the other use cases, we should use `loadPool` that creates a new pool if it wasn't created.
keyserver/src/database/db-config.js
19–34 ↗	(On Diff #12989)	In this approach we assume that either all or none values are overwritten. It might be a good idea, given our use case, but what do you think about modifying it so that: We import the config and set all the present fields in a config object Read env variables and modify the object if a variable is set Add an invariant that checks if all the fields are set By using this approach it should be possible to use file config as a default and modify only the variables we need. Do you think it might be useful?

This revision now requires changes to proceed.May 23 2022, 2:57 AM

keyserver/src/database/database.js
34–36 ↗	(On Diff #12989)	Good call
keyserver/src/database/db-config.js
19–34 ↗	(On Diff #12989)	To be honest, this seems like overengineering... I don't foresee a need to partially configure via JSON file, and partially via environmental variable. I think in the dev / old prod environment we will use JSON file, and in the Docker / new prod environment we will use environmental variables. If we ever have a need for something like this, I think we can address it then

Replace getPool function with endPool

atul added inline comments.

keyserver/src/database/db-config.js
16–18 ↗	(On Diff #13013)	This is fine as is because `dbConfig` defaults to `undefined`.. But, this check might be a bit more "robust" if we also check that `dbConfig` isn't `null`? Might help to avoid subtle `undefined` vs `null` bugs in the future? Maybe we can just check the truthiness of `dbConfig` with something like `if (dbConfig)`? Not sure if this matters, just a thought
25 ↗	(On Diff #13013)	Should we use `??` (nullish coalescing) instead of OR here? IIRC `\|\|` checks whether the left hand is "truthy" whereas `??` checks if the left side is `!null && !undefined` Unless I'm missing something and we intentionally want to handle empty string as a "falsey" case? (Looks like `process.env.WHATEVER` is `undefined` by rather than `""` if it doesn't exist: )
32 ↗	(On Diff #13013)	Wonder if specifying the file path could help point the developer in the right direction?

keyserver/src/database/db-config.js
16–18 ↗	(On Diff #13013)	I think this is better... generally I think `null` indicates "fetched but there was nothing", `undefined` indicates "hasn't been fetched yet"
25 ↗	(On Diff #13013)	I purposefully opted for `??` so that if `COMM_MYSQL_HOST` is the empty string or something like that, it would use `localhost`
32 ↗	(On Diff #13013)	We're no longer requiring DB config to be specified via filepath... it can specified via env variables too now. It felt misleading to say something was missing if that thing doesn't necessarily need to be specified