nextcloud / docker

⛴ Docker image of Nextcloud
https://hub.docker.com/_/nextcloud/
GNU Affero General Public License v3.0
6.06k stars 1.83k forks source link

Docker image does not follow changes to `MYSQL_HOST` variable #2086

Open gabriel-v opened 1 year ago

gabriel-v commented 1 year ago

Initially opened nextcloud/server/issues/40904, was told it's a docker issue, so here I copy over the relevant sections.

Bug description

In the "Auto configuration via environment variables" section of the Docker Readme, one can use MYSQL_HOST to set the database connection info, and NEXTCLUOD_UPDATE=1 to automate installation.

The problem is when we want to change that MySQL_HOST into something else. We change the environment variable, restart the container, and we get this error: SQLSTATE[HY000] [2006] MySQL server has gone away.

We investigate and find that, while the autoconfig.php reads environment variables, the config.php is hard-coded at installation time with the actual IP/PORTS:

<?php
$CONFIG = array (
...
  'dbhost' => '10.66.60.1:9971',
...

OK, let's try to use the CLI command to edit that hard-coded file, like this:

php occ config:system:set dbhost --value 1.2.3.4

We then get this error again: SQLSTATE[HY000] [2006] MySQL server has gone away

So the config:system:set can't fix the dbhost variable without trying to connect to the old, faulty dbhost!

That means the only option is either to manually edit the config file, to overwrite it with sed, or to overwrite it entirely (which sounds complex because it has to keep all the generated secrets already present in it).

The same issue happens with all other environment variables: db pass, user, database name, https override settings, etc.

Steps to reproduce

Trivial Docker-compose example that renames "db" into "db2": https://gist.github.com/gabriel-v/c6e5a1e18686f39649546c1161fadd64

  1. Install docker container using auto-configuration environment variable for MYSQL
  2. Change the IP/PORT of the MYSQL server
  3. Update the environment variables for auto-configuration and restart the Nextcloud server

Expected behavior

Nextcloud starts successfully with the new DB connection info

Installation method

Community Docker image

Nextcloud Server version

confirmed on 26, 27, latest docker images

Work-Around

https://github.com/nextcloud/helm/tree/main/charts/nextcloud#multiple-configphp-file

Create file config/dbhostoverride.config.php with content:

<?php
$CONFIG = array (
  'dbhost' =>  getenv('MYSQL_HOST'),
);

And add keys to this file for each value expected to change (dbport, passwords, etc.)

This solution is the cleanest, but still requires injecting templates into the image for environment variables that we already set.

Suggestions

This causes a great deal of confusion: one expects the environment variable to be the source of truth for configuring the image. Inspecting the autoconfig.php file also does not help, because it's not obvious the $AUTOCONFIG var is used only at install time, while the $CONFIG var is used at run time.

$CONFIG = array (
  'dbhost' =>  getenv('MYSQL_HOST') || getenv('POSTGRES_HOST') || ...,
  ... => ...
pnpninja commented 9 months ago

i noticed this too - this needs to be fixed IMO. whats the point of passing env variables then?

joshtrichards commented 8 months ago

This only happens with the db variables, and it's because they're injected via autoconfig.php[1] rather than as extra *.config.php files like many of the other image parameters. It does seem like maybe we could have a db.config.php that brings them in as well at runtime.

But a serious drawback comes to mind:

So making the change today may be more complicated than it seems at first glance.

Given this downside - combined with the variables primarily being only for installation anyhow (not changes post-installation) - I'm not sure myself of what the path forward here would even look like in terms of risk versus reward.

[1] https://docs.nextcloud.com/server/latest/admin_manual/configuration_server/automatic_configuration.html

gabriel-v commented 8 months ago

Thank you for looking into this.

the variables primarily being only for installation anyhow (not changes post-installation)

This is not true, for example in Docker or Kubernetes the database IP changes every reboot. In Docker-Compose setups, database hostnames are changed all the time during normal development. In all setups, database passwords may be rotated.

backwards compatibility

The relevant environment variables could be renamed, the old ones deprecated. For example MYSQL_HOST could be deprecated for a couple of NC versions, and the new variable that works as expected could be called NEXTCLOUD_MYSQL_HOST.

The root cause of the issue in my opinion is lacking documentation - this limitation should be plastered in the readme with bold letters.

martadinata666 commented 8 months ago

Is there something that prevent you to use the container name as db host rather than using IP? Docker container IP change doesn't really matter anyways. As nextcloud config can use container name as db host just fine.

gabriel-v commented 8 months ago

Is there something that prevent you to fix it

It's certainly not a difficult problem to fix once you understand where it comes from and how to mitigate it. But the realization that this docker container reads the env once but doesn't read it again comes as a shock, especially for configs that shouldn't need to be cached, my docker envs aren't going anywhere.

This only happens when you do non-trivial things (e.g. rotate your passwords, support custom db connections from user-provided strings, change hosting domain, etc). So I understand the reason for seeing this as "low reward" - if we got here all the way to successfully host the app enough time to need to change something and actually see this limitation, we're best suited to fix our problem by attaching more configuration files or overriding docker's DNS or something.


The expectation for docker images is that environment configuration is the source of truth - and when this is not the case, it's usually documented.

for example, this warning here:

https://hub.docker.com/_/postgres

Warning: the Docker specific variables will only have an effect if you start the container with a data directory that is empty; any pre-existing database will be left untouched on container startup.

Copy/paste of that is good enough to close this issue


I think Nextcloud is the only modern & popular app that's packaged on Docker with this complication that feels like it's left over from the early Apache days, all these other apps' Docker containers do follow changes in environment for basic things like the server name/location or DB connection options:

bean5 commented 3 months ago

It came as a surprise to me as well. Some variables can be changed, but others are ignored. I think one I tried broke my installation even though I reverted it.