[stable/redis] support redis docker library image

[jlegrone]

What this PR does / why we need it:
This release switches the default image from docker.io/bitnami/redis to docker.io/redis. The bitnami image remains compatible with this chart; see values-bitnami.yaml.

The default image in this release may be switched out for any image containing the redis-server binary. If redis-server is not the default image ENTRYPOINT, master.command must be specified.

This allows stable/redis to offer a higher degree of flexibility for those who may need to run images containing redis modules or based on a different linux distribution than what is currently offered by bitnami.

Breaking changes:

• master.args and slave.args are removed. Use master.command or slave.command instead in order to override the image entrypoint, or master.extraFlags to pass additional flags to redis-server.
• disableCommands is now interpreted as an array of strings instead of a string of comma separated values.
• master.persistence.path now defaults to /data.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged):

Related: https://github.com/bitnami/bitnami-docker-redis/issues/92

Special notes for your reviewer:

Several interesting test cases:

• helm upgrade --install redis-test ./stable/redis -f ./stable/redis/values-bitnami.yaml
• helm upgrade --install redis-test ./stable/redis --set usePassword=false
• helm upgrade --install redis-test ./stable/redis --set master.persistence.enabled=false
• helm upgrade --install redis-test ./stable/redis --set configmap="maxmemory-policy volatile-lru"
• helm upgrade --install redis-test ./stable/redis --set image.repository=redislabs/rejson --set image.tag=latest

[k8s-ci-robot]

Hi @jlegrone. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[jlegrone]

/assign @carrodher

[jlegrone]

/assign @davidkarlsen

[desaintmartin]

Thanks!
Let me just mention that we need to solve #7680 (comment) before any other contribution can be done on redis.
See #7745

[prydonius]

This PR is doing two different things, that I think should be split into different PRs, and probably warrant some discussion.

1. Changing the default image to Docker Inc.'s Redis

I'm curious what the motivation for this change is -- is there something that Docker Inc.'s Redis image has that the Bitnami Redis image does not? In particular, changing the image of this chart has deeper consequences as it has automation setup to keep the bitnami/redis image up-to-date and secure (e.g. #7065).

2. Generalising the chart to support custom Redis images

I'm not fully convinced it makes sense for this chart to try to support every type of Redis image, they're all going to be a little bit different (env vars, volume paths, etc.). This sort of approach will make charts much more complex and harder to maintain and test. Instead, we can create different charts that are tailored to specific usecases (e.g. a redis chart that focuses on making it easy to add extra modules, or includes some more common modules vs a bare bones Redis chart). With the charts project moving to a more distributed chart repositories model, it's going to be more common to see different variants of the same application for different usecases, and they can be hosted and maintained by anyone.

[prydonius]

Why is this change needed? It is not described in the PR description.

[jlegrone]

I copied and pasted my updates from values.yaml, and I think there were a few additional things that had gotten out of sync. This didn't look like a value that should be different in prod environments.

[davidkarlsen]

/unassign

[jlegrone]

@prydonius thanks for the feedback. Just to be clear, I am a big fan of Bitnami's maintained docker images, and I've had lots of success deploying them in the past. 😄

1. Changing the default image to Docker Inc.'s Redis

I actually tried to avoid making the switch as part of this pull request, however I couldn't figure out a way to do it without specifying a default command array in values.yaml (see values-bitnami.yaml).

I think it's important to avoid specifying a default command so that users can naively consume the default image entrypoint, which may be doing important things (ie. running gosu). In the particular case of redis, the person/team involved in deploying the chart is likely to be different than the maintainer of the image, so not requiring specialized knowledge of the image's entrypoint is valuable.
 
The problem is that I don't see a way to provide redis-server args directly to the bitnami/redis entrypoint. If this is already possible, or it can be supported in the future, I'm all for not switching out the default image. The bitnami image patch automation is great!

2. Generalising the chart to support custom Redis images

I'm not fully convinced it makes sense for this chart to try to support every type of Redis image, they're all going to be a little bit different (env vars, volume paths, etc.).

I certainly don't recommend that this chart attempt to support every redis use case or every possible redis image. That said, currently in order to create a custom redis image (to add redis modules, build off a different base image, etc) that is compatible with this chart, reverse engineering what is going on in the bitnami/redis image and building most of that functionality into the customized image is necessary. This is a non-trivial task and the root of what I'm attempting to improve upon in this PR.

There will still be limits on what the chart supports, but this change makes it possible to deploy any image that has redis-server and redis-cli on its path. This is a much easier requirement for users to meet when customizing their own images.

we can create different charts that are tailored to specific usecases (e.g. a redis chart that focuses on making it easy to add extra modules, or includes some more common modules vs a bare bones Redis chart)

I would very much like to avoid a fork of this chart, as users already have to decide between stable/redis, stable/redis-ha, and incubator/redis-cache. A more distributed chart repository model will be great when it comes, but as long as this chart occupies the stable/redis namespace I think it makes sense to meet users where they are when possible. If the answer to a user asking how to run redis on a RHEL-based image or use an image that includes a redis module while otherwise keeping the same deployment model is to create a new chart, then I think we've failed to do that.

[prydonius]

Thanks for the explanations @jlegrone.

I actually tried to avoid making the switch as part of this pull request, however I couldn't figure out a way to do it without specifying a default command array in values.yaml (see values-bitnami.yaml).

I think by overriding the entrypoint there we're bypassing some of the setup the bitnami/redis image does (e.g. validating env vars: https://github.com/bitnami/bitnami-docker-redis/blob/master/4.0/debian-9/rootfs/libredis.sh#L25). Also that image makes more use of the the redis.conf file rather than configuring args for redis-server: https://github.com/bitnami/bitnami-docker-redis/blob/master/4.0/debian-9/rootfs/libredis.sh#L56. I'm not that familiar with Redis to know what can and cannot be configured on the CLI (for example, can replication be configured using args?). @juan131 might be able to provide more guidance on how we might be able to make this work.

There will still be limits on what the chart supports, but this change makes it possible to deploy any image that has redis-server and redis-cli on its path. This is a much easier requirement for users to meet when customizing their own images.

I see the value in this, but I am still concerned that it might not be that straightforward (though again this may be due to my limitation of Redis knowledge). For example, what if a particular image doesn't support a configuration option that the default one does (replication for example)? I think the potential for confusion when environment variables and other configurations don't transfer over from image to image can lead to a worse experience. I'd love to see if we can resolve this in the bitnami/redis image and try to make the scenarios you're working with easier to achieve.

[jlegrone]

I'm not that familiar with Redis to know what can and cannot be configured on the CLI (for example, can replication be configured using args?)

Yes, that is exactly what is happening in this changeset:
https://github.com/helm/charts/blob/6cfd52503eea383ac8c52a7a23ad37b4c6aa2623/stable/redis/templates/redis-slave-deployment.yaml#L71-L73

Since Redis 2.6 it is possible to pass any configuration options as flags to redis-server:

https://redis.io/topics/config#passing-arguments-via-the-command-line

I think by overriding the entrypoint there we're bypassing some of the setup the bitnami/redis image does

Yes, I could use some feedback on exactly what the command array should be for the bitnami image, OR the bitnami image should support accepting args for redis-server directly to its entrypoint rather than only via the REDIS_EXTRA_FLAGS environment variable. The latter option would allow us to keep the bitnami image as the default for this chart.

Also that image makes more use of the the redis.conf file rather than configuring args for redis-server

Redis images can still bake in custom configuration by specifying an ENTRYPOINT of ["redis-server", "/path/to/custom.conf"].

For example, what if a particular image doesn't support a configuration option that the default one does

There is a difference between redis-server configuration options and other customizations that can be configured in various images. Custom configuration would need to be handled entirely by the image entrypoint, and this chart would be responsible only for passing in redis-server flags supported by the major version of Redis defined in .Chart.AppVersion.

I'd love to see if we can resolve this in the bitnami/redis image and try to make the scenarios you're working with easier to achieve

As far as I can tell, the bitnami image does not do anything using bespoke configuration mechanisms that can't also be supported with redis-server flags today, so this should certainly be possible!

[javsalgar]

The problem is that I don't see a way to provide redis-server args directly to the bitnami/redis entrypoint. If this is already possible, or it can be supported in the future, I'm all for not switching out the default image. The bitnami image patch automation is great!

I think that, before chaning the image, we should see if we can implement the missing features in the Bitnami Redis image, so we do not lose all the security and automation features already available. I will forward this information for evaluation.

[juan131]

Hi @jlegrone

I created this issue on Bitnami Redis Image to discuss how to implement the feature you're requesting:
https://github.com/bitnami/bitnami-docker-redis/issues/117

[juan131]

Hi @jlegrone

We just released a new Bitnami Redis image including the changes that allow you to send the redis-server arguments directly to the entrypoint.

Please feel free to use it (bitnami/redis:4.0.11-r50) and provide us feedback about it. I think this image now supports the missing features so we can continue using bitnami/redis on the chart.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

[jlegrone]

/remove-lifecycle stale

Sorry about the delay -- I made a big move recently and lost track of this PR. Will work on updating based on the recent work to modify the bitnami redis image.

[tuananh]

@juan131 hi, what's the recommended way to load an additional module with the chart?

[tompizmor]

@jlegrone can you resolve the conflict in stable/redis/Chart.yaml ?

[mattfarina]

/ok-to-test

[prydonius]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlegrone, prydonius

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• stable/redis/OWNERS [prydonius]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[tuananh]

@jlegrone nice <3

[qinix]

seems like extraFlags is not working 😟