Skip to content

fix: pass REDIS_PASSWORD to metrics exporter when auth is enabled#141

Open
jessebye wants to merge 1 commit intovalkey-io:mainfrom
jessebye:main
Open

fix: pass REDIS_PASSWORD to metrics exporter when auth is enabled#141
jessebye wants to merge 1 commit intovalkey-io:mainfrom
jessebye:main

Conversation

@jessebye
Copy link

Passes the REDIS_PASSWORD env to the metrics exporter container when auth is enabled. This resolves #135

Signed-off-by: jessebye <8467862+jessebye@users.noreply.github.com>
Comment on lines 193 to +209
env:
- name: REDIS_ALIAS
value: {{ include "valkey.fullname" . }}
{{- if .Values.auth.enabled }}
- name: REDIS_PASSWORD
valueFrom:
secretKeyRef:
{{- if .Values.auth.usersExistingSecret }}
{{- $defaultUser := index .Values.auth.aclUsers "default" | default dict }}
{{- $passwordKey := $defaultUser.passwordKey | default "default" }}
name: {{ .Values.auth.usersExistingSecret }}
key: {{ $passwordKey }}
{{- else }}
name: {{ include "valkey.fullname" . }}-auth
key: default-password
{{- end }}
{{- end }}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jessebye 👋

I’m planning to use a dedicated redis user for the exporter, as described in the docs:
https://github.com/oliver006/redis_exporter/blob/master/README.md?plain=1#L260

Would it be possible to make this more flexible and allow configuring a different user if needed? For example:

Suggested change
env:
- name: REDIS_ALIAS
value: {{ include "valkey.fullname" . }}
{{- if .Values.auth.enabled }}
- name: REDIS_PASSWORD
valueFrom:
secretKeyRef:
{{- if .Values.auth.usersExistingSecret }}
{{- $defaultUser := index .Values.auth.aclUsers "default" | default dict }}
{{- $passwordKey := $defaultUser.passwordKey | default "default" }}
name: {{ .Values.auth.usersExistingSecret }}
key: {{ $passwordKey }}
{{- else }}
name: {{ include "valkey.fullname" . }}-auth
key: default-password
{{- end }}
{{- end }}
env:
- name: REDIS_ALIAS
value: {{ include "valkey.fullname" . }}
{{- if .Values.auth.enabled }}
- name: REDIS_USER
value: {{ .Values.metrics.exporter.user | default "default" }}
- name: REDIS_PASSWORD
valueFrom:
secretKeyRef:
{{- if .Values.auth.usersExistingSecret }}
{{- $metricsUser := .Values.metrics.exporter.user | default "default" }}
{{- $userConfig := index .Values.auth.aclUsers $metricsUser | default dict }}
{{- $passwordKey := $userConfig.passwordKey | default $metricsUser }}
name: {{ .Values.auth.usersExistingSecret }}
key: {{ $passwordKey }}
{{- else }}
name: {{ include "valkey.fullname" . }}-auth
key: default-password
{{- end }}
{{- end }}

Then it can allow dedicated redis user:

    auth:
      enabled: true
      usersExistingSecret: test-valkey-auth
      aclUsers:
        default:
          permissions: "~* &* +@all"
          passwordKey: default
        monitor:
          permissions: >- # Limited permissions for monitoring: https://github.com/oliver006/redis_exporter/blob/master/README.md?plain=1#L260
            ~* &* -@all +@connection +memory -readonly +strlen +config|get +xinfo
            +pfcount -quit +zcard +type +xlen -readwrite -command +client -wait
            +scard +llen +hlen +get +eval +slowlog +cluster|info +cluster|slots
            +cluster|nodes -hello -echo +info +latency +scan -reset -auth -asking
          passwordKey: monitor

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaelgoth that seems reasonable, but does go outside the scope of this PR a bit. perhaps this could be added first, then a separate PR that adds support for using a different user in the metrics exporter?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m fine shipping this part first, and then adding support for extra users in metrics exporter 👍🏾

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Metrics exporter cannot authenticate when auth is enabled

2 participants