Set Redis through environment variables

[marceljd]

@J0WI added memcache.distributed

[J0WI]

I tested this with a linked Redis container. Redis does already set a REDIS_PORT env to tcp://[ip]:[port], so NextCloud crashes with a server error.

Please also update the example docker-compose files using Redis to avoid conflicts and code duplication:

• https://github.com/nextcloud/docker/tree/master/.examples/docker-compose/insecure/mariadb-cron-redis
• https://github.com/nextcloud/docker/tree/master/.examples/docker-compose/with-nginx-proxy/mariadb-cron-redis

[marceljd]

@J0WI Did you test it with v15.0 / Apache / FPM? I have to test the port settings again, it worked when I tried it a few months ago.

Do we need the port setting? The port is a default port (like 3306 for MySQL) and people who need to run their own Redis servers on different ports are probably running a non-standard version anyway.

[J0WI]

Did you test it with v15.0 / Apache / FPM?

I used nextcloud:1{3,4,5}-apache with redis:alpine.

Do we need the port setting?

Yes, the port should be customizable.

[marceljd]

@J0WI I have investigated this, and all Redis 5.0 Docker containers have a hardcoded port set (6379). Adding one variable (REDIS_HOST) is sufficient, there are no official Redis containers that use other ports. Shall I change everything to only that single variable? This is in all flavours of the Redis containers:

EXPOSE 6379

I tested this on a non-standard Redis container previously, but I think we should stick to the official one.

[J0WI]

@marceljd your recent commits didn't made any changes

[marceljd]

@J0WI I do not know why there are Dockerfile changes in here, I think they appeared because of the DCO correction I had to maken (force update). Is there any way to remove them? The rest should be ok: REDIS_HOST only because the port is hard-coded in the official Redis images. I also changed the examples and README files.

[SnowMB]

Please try to rebase your changes onto master, then the version changes in the dockerfiles should go away.

[marceljd]

Tried a few times to get the DCO error out of the way, but unfortunately the instructions given are not working...

[J0WI]

REDIS_HOST only because the port is hard-coded in the official Redis images.

This is only the case, if the nextcloud and redis container are in the same network. But you can expose port 6379 to any other port. We also support custom ports for databases.

Tried a few times to get the DCO error out of the way, but unfortunately the instructions given are not working...

Use git rebase --signoff master.

[marceljd]

That did not help, now it is one big mess again... And the DCO error is still there unfortunately.

I know that you can expose container ports to whatever port you want, but the default Redis containers do not support that directly. If you just want to start a default Redis container from the official image the way that is described in the examples than you do not have a choice. It runs on 6379 and this image does not support any environment variables out of the box. So that is why I think this is sufficient. MySQL has a documented way of changing the default port though an environment variable, I think that is a different case. For Redis you should use the Bitnami version instead. My solution is the easy way for the official Redis container, it just works.

[SnowMB]

I know that you can expose container ports to whatever port you want, but the default Redis containers do not support that directly. If you just want to start a default Redis container from the official image the way that is described in the examples than you do not have a choice. It runs on 6379 and this image does not support any environment variables out of the box. So that is why I think this is sufficient. MySQL has a documented way of changing the default port though an environment variable, I think that is a different case. For Redis you should use the Bitnami version instead. My solution is the easy way for the official Redis container, it just works.

Yeah but in most cases you would not expose your redis container on a network outside the host. When you use for example 2 servers (docker hosts) one running redis and the other one running nextcloud, nextcloud would connect to a port on the 2nd host. and there the redis port could be anything because of the required port mapping to the host.

It would just look like this:

services:
  redis:
    image: redis
    ports:
      - "11111:6379"
...

An other example would be someone running redis natively.

The fix is fairly easy: Just introduce a 2nd variable called REDIS_PORT and default it to 6479 so for the user nothing will change but he gets the possibility to customize the port redis is running on.

In the config (untested):

    'redis' => array(
      'host' => getenv('REDIS_HOST'),
      'port' => getenv('REDIS_PORT') ? getenv('REDIS_PORT')  : 6379
    ),

[J0WI]

We can't use REDIS_PORT:

Redis does already set a REDIS_PORT env to tcp://[ip]:[port], so NextCloud crashes with a server error [if linked to a redis container].

But changing the name of the variable should work.

[SnowMB]

Oh missed that. Maybe REDIS_HOST_PORT 😁

[marceljd]

 'port' => getenv('REDIS_PORT') ? getenv('REDIS_PORT')  : 6379

I had this in the original config already (slightly different), but I am still wondering what the use case would be. In my opinion most users will start up 3 containers: SQL, webserver and redis. If you start them together they will connect on standard ports, no further config needed. The example configs and environment variables are not ment for expert users, they will change the config with custom Dockerfiles anyway. And a non-expert will not run a Redis server outside of the cluster. What you certainly do not want is non-expert users exposing host-ports to the outside world running a redis server. You want them to stay within the docker-compose boundaries, and exposing ports 80 and 443 to the outside world is more than enough. So I do not see the point of adding custom ports to this. Running Redis is faster and gets rid of file locks, so I think everyone should run 3 instead of only 2 containers, but adding custom ports will only be more complicated and insecure, especially if you are not an expert.

[SnowMB]

Completely agree: most users will not ever use this and most users will run their setup on one host and most users may not have enough expertise to fiddle around with the details. (Including myself as I'm missing quite a bit of whats needed for proper server administration)

That said, I think in this case adding the possibility to change the redis port is very straight forward on both the documentation and the implementation side so that this feature is completely opt-in. For a normal user nothing changes and he never has to worry about setting this port to any value.

[J0WI]

There is a great example at nextcloud/server#13148 (comment)

[marceljd]

There is a great example at nextcloud/server#13148 (comment)

I don't know, looks like the issue is a routing problem with CSFR errors. This should be solvable with the sticky settings in Kubernetes, I would not try to solve networking problems in an application. But perhaps I misunderstood the exact problem. Besides: they still use the default port.

@J0WI @SnowMB I added the port to the config files and the README, but I did not include them in the examples on purpose because I think these should be as simple as possible. Should I? And I still have trouble with the DCO check. I tried to rebase and signoff, but it still does not work. When it is necessary I think I need to create a new branch and change the files again, or do you have the golden hint?

[J0WI]

Use git rebase --signoff master, fix any conflicts and git push -f <your upstream branch>.
You can also squash your commits with git rebase -i master or git reset --soft HEAD~<amount of commits>

[marceljd]

@J0WI The files are correct now but I cannot get rid of the commits that are in the branch already. I cannot get the signoff to work. I think the only possibility is to remove the entire fork and my local copies and start from scratch. I already lost too much time playing around with Git, is there any way to get this to work without trying for hours? The info on the internet was not really helpfull, tried just about everything except deleting everything and start over again.

[tilosp]

@marceljd i manged to clean up most of the mess locally. I can force push it to your branch if you want me to.

[marceljd]

@tilosp I would be very happy! Next time I will be very careful not to create this kind of mess again :-)

Thank you very much!

[marceljd]

@tilosp , is it safe to signoff with these commands now, or do you have to sign because you pushed?

git rebase HEAD~8 --signoff
git push --force origin redis-env-vars

[tilosp]

@marceljd the commit mess is gone but there is still some sign off problem.
but this should be solvable by creating a new clone and running the commands that DCO bot recommends:

git clone https://github.com/marceljd/docker.git -b redis-env-vars
git rebase HEAD~8 --signoff
git push --force origin redis-env-vars

[J0WI]

LGTM

[J0WI]

@tilosp @SnowMB do you agree with the latest patch?

[SnowMB]

LGTM

[J0WI]

@tilosp could you merge this once RC are removed?

[marceljd]

@J0WI @tilosp question: can I already remove my fork and create a new one for another PR or will that break things in this PR? I would like to start with a clean fork so I do not run into a mess later on.

[J0WI]

Please keep your fork until this is merged.