DB config, including port, through libpq env vars

[brontolosone]

Alternative to this contributor PR: #915

Companion PR to getodk/central-backend#1748

This demonstrates using the backend's new DB configuration approach (through libpq's PG* environment variables) in the Central dockerized setup.

In a nutshell:

• Backwards compatible with existing .env files
• Though users should be encouraged to head to our docs to learn about the PostgreSQL libpq env vars in order to set PG* environment variables in the .env to configure their database access to the full extent now made possible.

What has been done to verify that this works as intended?

Deployed to dev.getodk.cloud .

Why is this the best possible solution? Were any other approaches considered?

Not being backwards compatible would have been easier and more clear. But then users would be required to do a (trivial? 🤷‍♂️ ) migration of their .env database directives to the new variables.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Users may have workarounds for our previous incapability of exposing the full configuration surface (for instance, one couldn't configure the port number).

In particular for the port number, users have been tacking it onto the host variable, which would work in a number of places (but also not, in some others, eg probably not for wait-for-it) as we would concatenate host and port number anyway to form a connection URI, and had hardcoded the port number there). But this doesn't work with the environment variables.

If we want to pursue backwards compatibility even for THAT kind of workaround, then I would suggest we modify service/scripts/start-odk.sh some more, see if the port is unset yet the host ends with a port specification, and then mangle the variables appropriately.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

The additional comments I made in .env.example suffice to explain, in my opinion.

Before submitting this PR, please make sure you have:

• branched off and targeted the next branch OR only changed documentation/infrastructure (master is stable and used in production)
• verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

[matthew-white]

Users may have workarounds for our previous incapability of exposing the full configuration surface …
In particular for the port number, users have been tacking it onto the host variable, which would work in a number of places (but also not, in some others) … If we want to pursue backwards compatibility even for THAT kind of workaround, then I would suggest we modify service/scripts/start-odk.sh some more

My initial feeling is that we don't need to support this particular workaround. Let's just make it clear how to set the port going forward.

[matthew-white]

Just FYI, it looks like CI failed. It looks like #1640 to me. Could it be because .gitmodules changed? Maybe try rerunning the failed action? Maybe we just need the Backend PR to get merged so .gitmodules doesn't have to change.

[matthew-white]

Users may have workarounds for our previous incapability of exposing the full configuration surface …
In particular for the port number, users have been tacking it onto the host variable, which would work in a number of places (but also not, in some others) … If we want to pursue backwards compatibility even for THAT kind of workaround, then I would suggest we modify service/scripts/start-odk.sh some more

My initial feeling is that we don't need to support this particular workaround. Let's just make it clear how to set the port going forward.

I may have spoken too hastily here. Let's get a second opinion from @lognaturel. I see that she requested that:

could you please check whether [including the port in DB_HOST is] possible and if so make sure that the approach taken here is compatible with it?

[lognaturel]

I disagree with myself of the past! Anyone who had been somehow getting a port number in is probably more technical. I can put a note in the upgrade docs on how to migrate to the new way. I've added that to the docs issue.

[matthew-white]

Suggested change

	- DB_SSL=${DB_SSL:-null}
	- DB_SSL=${DB_SSL:-}

If DB_SSL is only relevant to shell stuff now, maybe an empty string reads better than null? null made sense when this value was being passed off to JSON/JS, but maybe it's kind of historical baggage now.

No strong feelings, also fine to keep.

[brontolosone]

Actually, we can now drop this line (and comment) altogether. If DB_SSL is set in the .env, it will now end up in the container's env (and thus start-odk.sh) anyways, because of that newly added env_file: ".env" in the dockerfile. Thus: 860b29d

[matthew-white]

I actually think the explictness is helpful. It documents which environment variables in .env are relevant to the service container and which aren't. E.g., DOMAIN=${DOMAIN} and SYSADMIN_EMAIL=${SYSADMIN_EMAIL} could now go away given env_file: "env", but I'm not totally convinced we want to do that yet.

So I'd say let's keep it DB_SSL=${DB_SSL:-null} or make it DB_SSL=${DB_SSL:-}, but I think it's worth it to continue listing the variable. Otherwise, there's no single list of environment variables anymore.

[matthew-white]

It makes sense to get the PG* env vars out of docker-compose.yml so that you can distinguish between empty vs. unset. It's necessary to move them out, and there's also no point in trying to list all the PG* vars, since we don't know which PG* vars will actually be used in .env. But I think DB_SSL is a different case, as we aren't passing it to Postgres. It's our own variable, and it'd be good to document its existence. So for DB_SSL, I'd prefer leaving it in docker-compose.yml for now. We can always reevaluate that later, just in terms of getting this PR merged.

[matthew-white]

Though I see you added a comment about DB_SSL to docker-compose.yml, so maybe that's enough. 🤔 But then it feels kind of inconsistent with our treatment of vars like DOMAIN and SYSADMIN_EMAIL.

[matthew-white]

I can see how it's nice that start-odk.sh handles all DB-related vars now without docker-compose.yml getting in the way. E.g., start-odk.sh is also now the only place where DB_HOST appears. So I think I'm talking myself into DB_SSL staying just in start-odk.sh and not docker-compose.yml. Sorry for the back and forth, I'm happy either way at this point.

[brontolosone]

BackwardsCompat and TidyBranchless are seldom seen walking hand in hand. 🤷‍♂️

[matthew-white]

In practice, will users ever need one or more of these env vars to be empty strings? Seems like that's not possible right now: they will always fall back to some default value.

[brontolosone]

Ugh.

On my machine if I empty all these PG*s, I get connected to the database of the same name as my unix user, over a unix socket, using peer auth (no password).
So that's a scenario in which it matters (that these vars are empty), but that's not a scenario you'd encounter in our docker setup; you'd always connect over the network, not over a unix socket, thus no peer auth and no visibility of your unix uid on the serverside, and thus also no default database.

But I guess you could have an empty password. Plus PostgreSQL has a 'trust' auth mode which does what it says and works over the network. And in that case you'd also be annoyed because you can't express "no or empty password". You couldn't before this change either, though.

So what we really need is to be able to distinguish "variable is undefined" from "variable is empty". Bash can do that in a clean way apparently, so if we move this logic out of this yaml into start-odk.sh (where, anyway, the DB_SSL logic already exclusively lives now, since 860b29d), then we could behave properly.

[brontolosone]

40c0b52 implements the above idea, it's running on dev.getodk.cloud .

[matthew-white]

It's looking great!

My only real question at this point is my comment about whether there are cases where one of PGHOST, PGUSER, PGPASSWORD, or PGDATABASE must be an empty string. I don't think we really support that case right now. But maybe it's a non-issue. Certainly it would be a preexisting issue — one that we could track in a follow-up issue. I'll go ahead and approve assuming that it's a non-issue, but I'm also happy to keep discussing it here.

[brontolosone]

I added support for specifying empty PG* vars, we won't override them, and they'll end up in the container env.
@matthew-white Please re-review!

[matthew-white]

Looks good to me! I don't have super strong feelings on DB_SSL anymore, so I'd say just do whatever feels the most consistent.