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

Feature: config in a ConfigMap #84

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mheuser
Copy link

@mheuser mheuser commented May 3, 2022

Overview

This PR adds the option to use a ConfigMap to store the content of config.

What this PR does / why we need it

Closes #83

Special notes for your reviewer

Checklist

  • Change log updated in Chart.yaml (see the contributing guide for details)
  • Chart version bumped in Chart.yaml (see the contributing guide for details)
  • Documentation regenerated by running make docs

feat: Optional configMap for config

feat: Optional configMap for config

feat: Optional configMap for config

chore: Updated version and docs

chore: working on release action

Revert "chore: Updated version and docs"

This reverts commit 78e551c.

chore: Updated version and docs

Revert "chore: working on release action"

This reverts commit 7d68e18.
@sagikazarmark
Copy link
Member

Thanks for the PR @mheuser !

Can you please clarify why using a secret does not fit your use case? Dex config usually does contain information that's worth protecting from the prying eyes.

@mheuser
Copy link
Author

mheuser commented May 24, 2022

Since I use gitops for managing my clusters which are staged (dev, stage, prod), I try to reuse as much configuration as possible between them. So I try to use only secrets where needed to have as much text files that I can check into git.
The dex config makes it possible to get secrets from the env which then can be injected via env/secretKeyRef which I manage then with external secrets.

As it is implemented now the default case will still be config as secret and optionally as config map.

Hape that makes some sense to you @sagikazarmark

@twe-syde
Copy link

@sagikazarmark This PR is important to get the deployment of dex' via ArgoCD working.

Without ArgoCD does not recognize that there is a change as the secret never gets generated. So to have a GitOps style config managment (secrets are out sourced) I patched my helm chart and now it works like a charm.

@sagikazarmark
Copy link
Member

@mheuser sorry, I still don't understand why it matters that the config is stored in secret or not. It will get mounted the same way and you can still use external secrets or env vars to get secret values into the pod.

So what's the benefit of being able to choose between configmap and secret? From my vantage point it's an extra complexity with no actual benefit.

@pluhmen what secret does never get generated? What does ArgoCD has to do with it? Can you please elaborate?

@jkroepke
Copy link
Contributor

I'm using https://github.com/stakater/Reloader here to solving this issue.

@sagikazarmark
Copy link
Member

Reloader is great for automatically triggering a rollout when an externally managed secret changes.

When the chart managed secret changes, however, the deployment is also changed: https://github.com/dexidp/helm-charts/blob/master/charts/dex/templates/deployment.yaml#L25

So I'm still not sure how using a configmap instead is related.

@twe-syde
Copy link

twe-syde commented Apr 4, 2023

Sorry, but I don't understand this discussion, we as users want to configure the application by using a CONFIGmap and want to have separated secrets.

In addition we showed that we are using GitOps style to deploy services like dex. At the moment this does not work without this PR and we are forced to patch manually. :(

@sagikazarmark So the question is, will this PR ever being merged?

@jkroepke
Copy link
Contributor

jkroepke commented Apr 4, 2023

@pluhmen You can deploy your service using GitOps without this PR. Currently, no one understand it. You said you need this because of GitOps, but GitOps works withs secrets. too.

@twe-syde
Copy link

twe-syde commented Apr 4, 2023

@jkroepke & @sagikazarmark I've tested the upstream helm chart w/o changing the values file but that patch. Now it works, even with ArgoCD. 🙄

Sorry for the stress I made, I can't remember what was wrong back in the days and in the meantime several things changed... ☮️

@jcogilvie
Copy link

jcogilvie commented May 18, 2023

Here's a concrete use case: I use terraform. I don't want my helm values files containing secrets, because the template outputs can be public in a terraform plan or on a PR (we have a tool that posts diffs).

Instead, I want everything configurable in helm with links to externally-created secrets (which dex already supports). This way, non-secrets can be cleanly diffable in the helm chart while secrets are not revealed.

As it is today, this chart expects config to be populated in the values, which in turn puts secrets in the values. (As an aside, I believe this breaks the environment variable interpolation feature of dex config if I try to use external secrets for values that are actually secret.)

So either this PR, or a fix to variable interpolation, are necessary for my case.

@jcogilvie
Copy link

@sagikazarmark can you take another look at this in light of my use case, please?

@mheuser
Copy link
Author

mheuser commented Sep 9, 2024

@sagikazarmark I still have the need for this PR, my use cae is gitops where I render the manifests into a gitops manifest repo to be able to diff the changes of all manifests in a PR before I will let argo-cd do its magic.

Another Option would be to have the secret use secretString and let k8s do the base65 conversion. But still, why not lets just use a configmap for the config.

If u would give it a go I would update the PR.

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.

Add the option to store the config as ConfigMap
5 participants