Skip to content

feat: Improved convenience when using External Secrets through separation of ConfigMap and Secret#337

Merged
klesh merged 1 commit into
apache:mainfrom
kahirokunn:feat/configmap-secret-separation
Jul 9, 2025
Merged

feat: Improved convenience when using External Secrets through separation of ConfigMap and Secret#337
klesh merged 1 commit into
apache:mainfrom
kahirokunn:feat/configmap-secret-separation

Conversation

@kahirokunn
Copy link
Copy Markdown
Contributor

@kahirokunn kahirokunn commented Jul 1, 2025

Overview

Separates MySQL configuration information into confidential and non-confidential data to improve integration with external secret management systems such as External Secret.

Changes

🔧 Modified Files

  • charts/devlake/templates/secrets.yaml - Modified to contain only confidential information
  • charts/devlake/templates/configmap.yaml - Newly created: manages non-confidential settings
  • charts/devlake/templates/deployments.yaml - Updated to reference both ConfigMap and Secret

📋 Detailed Changes

Secret (confidential information only)

# Before changes
MYSQL_USER: "merico"           # Removed
MYSQL_PASSWORD: "merico"       # Retained
MYSQL_DATABASE: "lake"         # Removed  
MYSQL_ROOT_PASSWORD: "admin"   # Retained
DB_URL: "mysql://..."          # Removed
MYSQL_URL: "server:port"       # Removed

# After changes
MYSQL_PASSWORD: "merico"       # Retained
MYSQL_ROOT_PASSWORD: "admin"   # Retained

ConfigMap (non-confidential settings) - Newly created

MYSQL_USER: "merico"
MYSQL_DATABASE: "lake"
MYSQL_URL: "server:port"
DB_URL_TEMPLATE: "mysql://user:${MYSQL_PASSWORD}@server:port/db?..."

Deployment

envFrom:
  - configMapRef:              # Newly added
      name: devlake-config
  - secretRef:
      name: devlake-mysql-auth

🎯 Problems Resolved

Issues Before Changes

  • When injecting passwords with External Secret, MYSQL_USER, MYSQL_DATABASE, MYSQL_URL also needed to be managed externally
  • Non-confidential information was included in Secret, which goes against Kubernetes best practices
  • Secret updates were required when changing configurations

Improvements After Changes

  • External Secret only needs to manage passwords
  • Configuration information is managed in ConfigMap and properly separated
  • Follows Kubernetes best practices

🔄 Compatibility

  • Maintains backward compatibility with existing deployments
  • When option.autoCreateSecret: true, operates as before
  • When using External Secret, only passwords need to be provided

📚 Related Issues

Fixes #336

@klesh
Copy link
Copy Markdown
Contributor

klesh commented Jul 7, 2025

Hi, thanks for the PR. I saw the related issue and it made sense to me.
However, I have a couple of questions as I am not familiar with helm-chart.

  1. Will this affect existing users from upgrading to the latest version? is it a breaking change?
  2. If so, can it be modified to be compatible with upgrading an existing setup?
  3. Or, will you update the document as well? https://github.com/apache/incubator-devlake-website

@kahirokunn
Copy link
Copy Markdown
Contributor Author

Hi @klesh – thanks again for the review!

TL;DR : the change is backward-compatible, so we don’t need a compatibility shim or extra docs.

Topic Explanation
Breaking change? No. I’ve tested upgrades from the current chart to this branch in two scenarios:1. option.autoCreateSecret=true (default) – the upgrade is seamless.2. User-supplied Secret – the pod now receives duplicate env-vars with identical values; Kubernetes starts the container without error.
Compatibility layer? Not needed. Both upgrade paths above work out-of-the-box.
Docs update? The existing docs remain accurate: they already describe option.autoCreateSecret and custom-Secret workflows, which continue to work unchanged.

If you’d still prefer a brief “no action required” note in the website repo, just let me know and I’ll add it. Otherwise I think the chart can ship as-is.

Thanks for confirming!

@klesh
Copy link
Copy Markdown
Contributor

klesh commented Jul 7, 2025

Thanks for the explanation.
The CI is failing. Can you help investigate why?

@kahirokunn kahirokunn force-pushed the feat/configmap-secret-separation branch 3 times, most recently from 0a05cdc to d6629e2 Compare July 8, 2025 10:20
@kahirokunn
Copy link
Copy Markdown
Contributor Author

Thanks! I fixed it. 🙏

@klesh
Copy link
Copy Markdown
Contributor

klesh commented Jul 8, 2025

What is charts/devlake/charts/grafana-6.56.6.tgz for? Is it necessary?

- Move non-sensitive database config to ConfigMap
- Keep only passwords in Secret for better ExternalSecret integration
- Add configMapRef to deployment envFrom section

This change allows users to inject only passwords via ExternalSecret
while keeping database configuration in ConfigMap, following
Kubernetes best practices.
@kahirokunn kahirokunn force-pushed the feat/configmap-secret-separation branch from d6629e2 to 8a8e474 Compare July 8, 2025 23:06
@kahirokunn
Copy link
Copy Markdown
Contributor Author

kahirokunn commented Jul 8, 2025

charts/devlake/charts/grafana-6.56.6.tgz is an intermediate result from actually building and verifying helm locally, and was committed by mistake.
I added it to .gitignore so that others do not commit it in the same way.
Thank you 🙏

@klesh
Copy link
Copy Markdown
Contributor

klesh commented Jul 9, 2025

Thanks for the clarification.

@klesh klesh merged commit 42e02ad into apache:main Jul 9, 2025
3 checks passed
@klesh
Copy link
Copy Markdown
Contributor

klesh commented Sep 2, 2025

Hi, @kahirokunn. Could you take a look at #340 and #349 when you have a moment? I suspect it might be related to some shared secrets that were deleted in the PR.

@kahirokunn
Copy link
Copy Markdown
Contributor Author

kahirokunn commented Sep 2, 2025

Thank you for raising #340 and #349—I’ve reviewed them and realized I overlooked a few important considerations. I’m currently working on implementing the necessary fixes in PR #350. Additionally, to ensure ongoing stability of the Helm chart, I’ve introduced a CI workflow (smoke tests) to automatically validate key functionality going forward.

I sincerely apologize for the oversight and appreciate your patience and guidance throughout this process.

@klesh
Copy link
Copy Markdown
Contributor

klesh commented Sep 3, 2025

@kahirokunn Thanks for looking into the problem and for your quick response. Would you be interested in joining us as an Apache Committer for this project? We could really use an excellent DevOps expert like you to help maintain the helm-chart repository.

If you’re interested, please feel free to reach out to me on Slack.

Thanks again for your contributions!

fabioluciano pushed a commit to fabioluciano/incubator-devlake-helm-chart that referenced this pull request May 6, 2026
- Move non-sensitive database config to ConfigMap
- Keep only passwords in Secret for better ExternalSecret integration
- Add configMapRef to deployment envFrom section

This change allows users to inject only passwords via ExternalSecret
while keeping database configuration in ConfigMap, following
Kubernetes best practices.
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.

Improving Convenience When Using External Secrets by Separating ConfigMaps and Secrets

2 participants