Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[prometheus-redis-exporter] Allow REDIS_ADDR environment variable to be configured through a secret #3775

Merged
merged 3 commits into from
Sep 26, 2023
Merged

Conversation

daconstenla
Copy link
Contributor

@daconstenla daconstenla commented Sep 11, 2023

What this PR does / why we need it

This pr adds support for (prometheus-redis-exporter)[./charts/prometheus-redis-exporter/] redis address environment variable to be configured from a secret.
It would unlock for redis address to be fetched though eg: external-secrets.

Which issue this PR fixes

There is no issue linked to this change (that I know of).

Special notes for your reviewer

I've performed some sanity checks locally to verify the chart configuration stays as is and the new configuration also works.
Since changes are all around deployment's configuration I'm using helm + yq to focus on that part of the rendered template.

This is the specific used command:

helm template . -f values.yaml |yq 'select(.kind == "Deployment").spec.template.spec.containers[0].env'
using redisAddressConfig.enabled = false

No diff, it's literally values.yaml by default, renders as:

- name: REDIS_ADDR
  value: redis://myredis:6379
using redisAddressConfig.enabled = true + redisAddressConfig.isASecret = false
 redisAddressConfig:
   # configure `REDIS_ADDR` from a configmap
-  enabled: false
+  enabled: true
   # if `true` the `REDIS_ADDR` is sourced on a secret instead of a configmap
   isASecret: false
   # Use existing configmap (ignores redisAddress)
   source:
-    name: ""
-    key: ""
+    name: "testName"
+    key: "testKey"

Renders as:

- name: REDIS_ADDR
  valueFrom:
    configMapKeyRef:
      name: testName
      key: testKey
using redisAddressConfig.enabled = true + redisAddressConfig.isASecret = true
 redisAddressConfig:
   # configure `REDIS_ADDR` from a configmap
-  enabled: false
+  enabled: true
   # if `true` the `REDIS_ADDR` is sourced on a secret instead of a configmap
-  isASecret: false
+  isASecret: true
   # Use existing configmap (ignores redisAddress)
   source:
-    name: ""
-    key: ""
+    name: "testName"
+    key: "testKey"

renders as:

- name: REDIS_ADDR
  valueFrom:
    secretKeyRef:
      name: testName
      key: testKey

Checklist

  • DCO signed
  • Chart Version bumped
  • Title of the PR starts with chart name (e.g. [prometheus-couchdb-exporter])

zanhsieh
zanhsieh previously approved these changes Sep 14, 2023
@daconstenla
Copy link
Contributor Author

@zanhsieh should I do anything else to be able to merge this change? 🙏

@zanhsieh
Copy link
Contributor

@zanhsieh should I do anything else to be able to merge this change? 🙏

Nothing. Just give another maintainer @acondrat some time before I merging this.

charts/prometheus-redis-exporter/Chart.yaml Outdated Show resolved Hide resolved
charts/prometheus-redis-exporter/templates/deployment.yaml Outdated Show resolved Hide resolved
charts/prometheus-redis-exporter/values.yaml Outdated Show resolved Hide resolved
charts/prometheus-redis-exporter/values.yaml Outdated Show resolved Hide resolved
daconstenla and others added 3 commits September 25, 2023 11:00
…ally to the configmap

Signed-off-by: David Constenla <[email protected]>
And adjust variable names

Co-authored-by: zeritti <[email protected]>
Signed-off-by: David Constenla <[email protected]>
@daconstenla
Copy link
Contributor Author

daconstenla commented Sep 25, 2023

Thank you so much for the comments @zanhsieh, good point.

Accepted the suggestions and changed the outdated comments, I hope they are good 👌.
I've also rebased to latest state of the repo.

Did again the validation of helm rendering with the combination of variables and everything seems fine.

redisAddressConfig.enabled = false

- name: REDIS_ADDR
  value: redis://myredis:6379

redisAddressConfig.enabled = true

- name: REDIS_ADDR
  valueFrom:
    configMapKeyRef:
      name: testName
      key: testKey

redisAddressConfig.enabled = true && redisAddressConfig.isSecret = true

- name: REDIS_ADDR
  valueFrom:
    secretKeyRef:
      name: testName
      key: testKey

@zanhsieh zanhsieh merged commit 5868e00 into prometheus-community:main Sep 26, 2023
4 checks passed
Matiasmct pushed a commit to giffgaff/prometheus-charts-backup that referenced this pull request Mar 20, 2024
…o be configured through a secret (prometheus-community#3775)

* Allow `REDIS_ADDR` env var to be configured through a secret additionally to the configmap

Signed-off-by: David Constenla <[email protected]>

* Be explicit about the change breaking the values signature

And adjust variable names

Co-authored-by: zeritti <[email protected]>
Signed-off-by: David Constenla <[email protected]>

* Update the comment so it represents the current configuration

Signed-off-by: David Constenla <[email protected]>

---------

Signed-off-by: David Constenla <[email protected]>
Signed-off-by: David Constenla <[email protected]>
Co-authored-by: zeritti <[email protected]>
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.

3 participants