Improve Redis Password Handling in Helm Chart

[swade1987]

Current Situation

The current Helm chart doesn't provide a secure way to configure Redis with a password, unlike the PostgreSQL credentials which can be passed securely.

Problem

1. The Redis password needs to be stored as plain text in the values file, which is a security risk.
2. There's no built-in support for fetching the Redis password from a Secret, unlike the PostgreSQL credentials.

Current Workaround

To mitigate this, I've implemented a workaround in my fork of the chart:

1. Added an initContainer to substitute environment variables:

initContainers:
  - name: envsubst
    image: redacted/envsubst:alpine-3.16
    args:
      - sh
      - -c
      - cat /tempDir/config.yml | envsubst > /etc/sentry/config.yml && cat /tempDir/sentry.conf.py | envsubst > /etc/sentry/sentry.conf.py && ls -la /etc/sentry && cat /etc/sentry/config.yml
    env:
      - name: REDIS_PASSWORD
        valueFrom:
          secretKeyRef:
            name: redis-credentials
            key: redis-password
    volumeMounts:
      - mountPath: /tempDir
        name: config
      - mountPath: /etc/sentry
        name: env-config-volume

2. Modified the Redis configuration to use the environment variable:

redis.clusters:
  default:
    hosts:
      0:
        host: {{ $redisHost | quote }}
        port: {{ $redisPort }}
        password: "${REDIS_PASSWORD}"

3. Changed the volumeMounts configuration in the main container:

volumeMounts:
# - mountPath: /etc/sentry
#   name: config
#   readOnly: true
# BEGIN - FORKED CHANGES (comment block above is the original)
- mountPath: /etc/sentry
  name: env-config-volume

Issues with Current Workaround

1. It's cumbersome and requires significant modifications to the chart, including changes to volume mounts.
2. It makes upgrading to newer versions of the chart laborious, as these changes need to be reapplied and tested with each upgrade.
3. It's not aligned with the chart's existing patterns for credential management.
4. The changes to volume mounts may have unintended consequences or conflicts with other parts of the chart.

Proposed Solution

Implement a solution similar to how PostgreSQL credentials are handled. This could involve:

1. Adding support for a redis.existingSecret value in the chart, allowing users to specify an existing Secret containing the Redis password.
2. Modifying the templates to use this Secret when configured, returning to the current method if not specified.
3. Updating the documentation to reflect these changes and provide examples of how to use the new feature.
4. Ensuring that the solution doesn't require changes to volume mounts or other core parts of the chart structure.

Benefits of Proposed Solution

1. Improved security by not requiring plain text passwords in values files.
2. Consistency with existing credential management patterns in the chart.
3. Easier maintenance and upgrades for users configuring Redis with a password.
4. Eliminating the need for custom volume mount changes and init containers for password substitution.

Questions

1. Are there any technical limitations preventing this implementation?
2. Are there any other approaches the maintainers would prefer?
3. Is any additional information needed to progress with this improvement?
4. Are there concerns about backward compatibility or migration for users using custom solutions similar to my workaround?

[nadecancode]

@swade1987 Hi, I picked up a closed PR in this repo and reintroduced it: https://github.com/sentry-kubernetes/charts/pull/1492. I think that should allow us to specify an existing secret :D

[swade1987]

@swade1987 Hi, I picked up a closed PR in this repo and reintroduced it: https://github.com/sentry-kubernetes/charts/pull/1492. I think that should allow us to specify an existing secret :D

Love this, this will be a huge help as 85% of the customisations in our forked chart are for this. The others are for geoIP license and Istio support.

[nadecancode]

@swade1987 Hi, I picked up a closed PR in this repo and reintroduced it: #1492. I think that should allow us to specify an existing secret :D

Love this, this will be a huge help as 85% of the customisations in our forked chart are for this. The others are for geoIP license and Istio support.

It looks like this PR is merged! Feel free to test if it works - I tested it in my cluster and it seems to be fine (but there may be edge cases)

[swade1987]

@swade1987 Hi, I picked up a closed PR in this repo and reintroduced it: #1492. I think that should allow us to specify an existing secret :D

Love this, this will be a huge help as 85% of the customisations in our forked chart are for this. The others are for geoIP license and Istio support.

It looks like this PR is merged! Feel free to test if it works - I tested it in my cluster and it seems to be fine (but there may be edge cases)

Will do, will most likely be next week as we are currently working on getting 25.9.0 rolled out as the client currently have 18.0.0 installed.