search

nextcloud / helm

A community maintained helm chart for deploying Nextcloud on Kubernetes.
GNU Affero General Public License v3.0
334 stars 269 forks source link

Redis config customizations superseded by builtin environment variables #568

Open Kaurin opened 6 months ago

Kaurin commented 6 months ago

Describe your Issue

In my example I want to use TLS with the builtin redis. Config overrides get provisioned, but environmnet variables take precedence not allowing my config to use the tls:// prefix. Instead, the default tcp:// prefix is used.

Housekeeping

Clean setup, PVCs deleted

Config

nextcloud:
  defaultConfigs:
    redis.config.php: false
  configs:
    redis.config.php: |-
      <?php
      $CONFIG = array (
        'memcache.distributed' => '\OC\Memcache\Redis',
        'memcache.locking' => '\OC\Memcache\Redis',
        'redis' => array(
          'host' => "tls://nextcloud-wrapper-redis-master.default.svc.cluster.local",
          'port' => getenv('REDIS_HOST_PORT') ?: 6379,
          'password' => getenv('REDIS_HOST_PASSWORD'),
          'ssl_context' => [
              'verify_peer_name' => false
          ],
        ),
      );

redis:
  architecture: standalone
  enabled: true

  auth:
    enabled: true
    existingSecret: nextcloud-redis
    existingSecretPasswordKey: redisPw

  tls:
    enabled: true
    authClients: true
    autoGenerated: true

  metrics.enabled: true

Error

Error from /var/www/html/data/nextcloud.log (removed "Trace" and "Previous" to save space)

Notice tcp:// instead of tls://

{
  "reqId": "7zewVy1y4TPodURLtaNj",
  "level": 3,
  "time": "2024-05-08T02:16:04+00:00",
  "remoteAddr": "10.42.0.1",
  "user": "--",
  "app": "remote",
  "method": "GET",
  "url": "/status.php",
  "message": "read error on connection to tcp://nextcloud-wrapper-redis-master:6379",
  "userAgent": "kube-probe/1.29",
  "version": "29.0.0.19",
  "exception": {
    "Exception": "RedisException",
    "Message": "read error on connection to tcp://nextcloud-wrapper-redis-master:6379",
    "Code": 0,
    "Trace": [],
    "File": "/var/www/html/lib/private/Session/Internal.php",
    "Line": 213,
    "Previous": {},
    "message": "read error on connection to tcp://nextcloud-wrapper-redis-master:6379",
    "exception": {},
    "CustomMessage": "read error on connection to tcp://nextcloud-wrapper-redis-master:6379"
  }
}

Provisioned configs

Configs look good (not sure why doubled, but OK)

root@nextcloud-wrapper-6bd7885d8d-r7j5f:/var/www/html# grep -RIi redis /var/www/html/config

Output:

/var/www/html/config/redis.config.php:  'memcache.distributed' => '\OC\Memcache\Redis',
/var/www/html/config/redis.config.php:  'memcache.locking' => '\OC\Memcache\Redis',
/var/www/html/config/redis.config.php:  'redis' => array(
/var/www/html/config/redis.config.php:    'host' => "tls://nextcloud-wrapper-redis-master.default.svc.cluster.local",
/var/www/html/config/redis.config.php:    'port' => getenv('REDIS_HOST_PORT') ?: 6379,
/var/www/html/config/redis.config.php:    'password' => getenv('REDIS_HOST_PASSWORD'),
/var/www/html/config/config.php:  'memcache.distributed' => '\\OC\\Memcache\\Redis',
/var/www/html/config/config.php:  'memcache.locking' => '\\OC\\Memcache\\Redis',
/var/www/html/config/config.php:  'redis' => 
/var/www/html/config/config.php:    'host' => 'tls://nextcloud-wrapper-redis-master.default.svc.cluster.local',

Environment variables

Environment variables seem to have precedence over configs, rendering configs useless.

root@nextcloud-wrapper-6bd7885d8d-r7j5f:/var/www/html# env | grep -i redis

Output (notice tcp://)

NEXTCLOUD_WRAPPER_REDIS_MASTER_SERVICE_HOST=10.43.248.145
NEXTCLOUD_WRAPPER_REDIS_MASTER_PORT_6379_TCP_PORT=6379
REDIS_HOST=nextcloud-wrapper-redis-master
NEXTCLOUD_WRAPPER_REDIS_MASTER_PORT=tcp://10.43.248.145:6379
REDIS_HOST_PASSWORD=REDACTED_REDACTED
NEXTCLOUD_WRAPPER_REDIS_MASTER_SERVICE_PORT_TCP_REDIS=6379
NEXTCLOUD_WRAPPER_REDIS_MASTER_PORT_6379_TCP=tcp://10.43.248.145:6379
NEXTCLOUD_WRAPPER_REDIS_MASTER_SERVICE_PORT=6379
NEXTCLOUD_WRAPPER_REDIS_MASTER_PORT_6379_TCP_ADDR=10.43.248.145
NEXTCLOUD_WRAPPER_REDIS_MASTER_PORT_6379_TCP_PROTO=tcp
REDIS_HOST_PORT=6379

Probable culprit

First, the env variables are populated in the helpers file Then, nextcloud.env is used in nextcloud containers

If I am correct in saying that nextcloud favors env variables, then this helm chart approach probably has to be reworked so these customizations can actually take place.

Workaround

I thought I could just provision redis separately and just make sure to have the REDIS_HOST_PASSWORD in extraEnv. Unfortunately, this doesn't work either (different error - Redis went away). The workaround might be possible, but I have given up on it for now and will just go without TLS for now.

Conclusion

Would be nice if helm logic would allow us to override certain aspects of the redis config.

It would be even nicer if I'm completely wrong here and missing like one line of config :)

jessebot commented 6 months ago

Hoi!

Looks like redis host is templated here: https://github.com/nextcloud/helm/blob/1ae7421fd91a9e3b30f4f3fa27f60e2481dbbd63/charts/nextcloud/templates/_helpers.tpl#L215-L231

And the redis config is templated here if you're using default (which you're not): https://github.com/nextcloud/helm/blob/1ae7421fd91a9e3b30f4f3fa27f60e2481dbbd63/charts/nextcloud/templates/config.yaml#L32-L48

And your custom redis config would be templated here:

https://github.com/nextcloud/helm/blob/1ae7421fd91a9e3b30f4f3fa27f60e2481dbbd63/charts/nextcloud/templates/config.yaml#L12-L15

According to the above, it should allow your override, unless I'm missing something (which I could be).

Digging a bit further into this...

It looks like redis ssl support in nextcloud/server was added here 3 years ago: https://github.com/nextcloud/server/commit/ed10d85ff36f4fc72eb7c3ba62589105505a999e

but it doesn't look like the ssl_context options were passed in to the configs above or the one in nextcloud/docker here.

The config.sample.php for redis under the ssl section in the official nextcloud/server repo shows you may need an additional section. I've included the samples below for you to reference:

click for config.sample.php redis sections ```php /** * Connection details for redis to use for memory caching in a single server configuration. * * For enhanced security it is recommended to configure Redis * to require a password. See http://redis.io/topics/security * for more information. * * We also support redis SSL/TLS encryption as of version 6. * See https://redis.io/topics/encryption for more information. */ 'redis' => [ 'host' => 'localhost', // can also be a unix domain socket: '/tmp/redis.sock' 'port' => 6379, 'timeout' => 0.0, 'read_timeout' => 0.0, 'user' => '', // Optional: if not defined, no password will be used. 'password' => '', // Optional: if not defined, no password will be used. 'dbindex' => 0, // Optional: if undefined SELECT will not run and will use Redis Server's default DB Index. // If redis in-transit encryption is enabled, provide certificates // SSL context https://www.php.net/manual/en/context.ssl.php 'ssl_context' => [ 'local_cert' => '/certs/redis.crt', 'local_pk' => '/certs/redis.key', 'cafile' => '/certs/ca.crt' ] ], /** * Connection details for a Redis Cluster. * * Redis Cluster support requires the php module phpredis in version 3.0.0 or * higher. * * Available failover modes: * - \RedisCluster::FAILOVER_NONE - only send commands to master nodes (default) * - \RedisCluster::FAILOVER_ERROR - failover to slaves for read commands if master is unavailable (recommended) * - \RedisCluster::FAILOVER_DISTRIBUTE - randomly distribute read commands across master and slaves * * WARNING: FAILOVER_DISTRIBUTE is a not recommended setting, and we strongly * suggest to not use it if you use Redis for file locking. Due to the way Redis * is synchronized it could happen, that the read for an existing lock is * scheduled to a slave that is not fully synchronized with the connected master * which then causes a FileLocked exception. * * See https://redis.io/topics/cluster-spec for details about the Redis cluster * * Authentication works with phpredis version 4.2.1+. See * https://github.com/phpredis/phpredis/commit/c5994f2a42b8a348af92d3acb4edff1328ad8ce1 */ 'redis.cluster' => [ 'seeds' => [ // provide some or all of the cluster servers to bootstrap discovery, port required 'localhost:7000', 'localhost:7001', ], 'timeout' => 0.0, 'read_timeout' => 0.0, 'failover_mode' => \RedisCluster::FAILOVER_ERROR, 'user' => '', // Optional: if not defined, no password will be used. 'password' => '', // Optional: if not defined, no password will be used. // If redis in-transit encryption is enabled, provide certificates // SSL context https://www.php.net/manual/en/context.ssl.php 'ssl_context' => [ 'local_cert' => '/certs/redis.crt', 'local_pk' => '/certs/redis.key', 'cafile' => '/certs/ca.crt' ] ], ```

Perhaps you need to try passing in those ssl_context optons to your custom config? Let us know if that helps. Anyone else in the community familiar with this, please feel free to chime in.

Kaurin commented 6 months ago

This config:

nextcloud:
  defaultConfigs:
    redis.config.php: false
  configs:
    redis.config.php: |-
      <?php
      $CONFIG = array (
        'memcache.distributed' => '\OC\Memcache\Redis',
        'memcache.locking' => '\OC\Memcache\Redis',
        'redis' => array(
          'host' => "tls://nextcloud-wrapper-redis-master.default.svc.cluster.local",
          'port' => getenv('REDIS_HOST_PORT') ?: 6379,
          'password' => getenv('REDIS_HOST_PASSWORD'),
          'ssl_context' => [
            'verify_peer_name' => false
            'local_cert' => '/certs/redis.crt',
            'local_pk' => '/certs/redis.key',
            'cafile' => '/certs/ca.crt',
          ],
        ),
      );
  extraEnv:
    - name: PHP_MEMORY_LIMIT
      value: 4096M
    - name: PHP_UPLOAD_LIMIT
      value: 16G
  ###### Shelve for now - in case we want to try TLS again
    - name: REDIS_HOST_PASSWORD
      value: changeme!

redis:
  architecture: standalone
  enabled: true

  auth:
    enabled: false
    password: changeme!

  tls:
    enabled: true
    authClients: true
    autoGenerated: true

Produces this on the webpage itself

Internal Server Error
The server encountered an internal error and was unable to complete your request.
Please contact the server administrator if this error reappears multiple times, please include the technical details below in your report.
More details can be found in the webserver log.
<br />
<b>Parse error</b>:  syntax error, unexpected single-quoted string &quot;local_cert&quot;, expecting &quot;]&quot; in <b>/var/www/html/config/redis.config.php</b> on line <b>11</b><br />

The good news is that the override is getting interpreted by nextcloud, but I guess it doesn't know what to do with local_cert ?

albundy83 commented 6 months ago

But due to the fact that redis php is configured also here: https://github.com/nextcloud/docker/blob/7a4823180dc0a8c7d92a5dfb20701b04770c5706/29/apache/entrypoint.sh#L123

and as you can see tcp:// is hardcoded ... not sure it could work no ?

Kaurin commented 6 months ago

Ah. So fixing this would require changing the docker repo for dockerhub as well as this helm repo. Not sure where to open a feature request for this, or whether just this github issue will suffice

albundy83 commented 6 months ago

Maybe we could add a way to override the entrypoint by mounting a ConfigMap ? When I read this comment it's not sure we will be able to change the way. The best proposal will be to not have this file managed by entrypoint but by a clean ConfigMap I think.

joshtrichards commented 2 months ago

re: the PHP session handler... Yes, that's configured in the image entrypoint. It's independent of Nextcloud's config since it's a PHP config matter, but it does pull from the same auto-config environment variables.

IIRC though there may be some weirdness there... we'll need to research. tls is supported by phpredis, but I seem to recall there being some weirdness/lack of TLS support for the session handler with standalone Redis nodes. But maybe that's changed - e.g. phpredis/phpredis#2265

If it's actually changed, their docs don't seem to reflect that:

If it is possible, we can make a change in the image entrypoint to adapt to it.

@Kaurin -

          'ssl_context' => [
            'verify_peer_name' => false
            'local_cert' => '/certs/redis.crt',
            'local_pk' => '/certs/redis.key',
            'cafile' => '/certs/ca.crt',
          ],

You're missing a comma after false.