fix: clickhouse secret #149
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the ClickHouse configuration to support using external secrets for authentication credentials instead of embedding them in ConfigMaps. When an external secret is configured, the user configuration is dynamically generated at runtime via an init container.
Changes:
- Added init container to generate ClickHouse user configuration from external secrets
- Conditionally mount user configuration based on whether external secrets are used
- Updated ConfigMap to exclude user configuration when external secrets are enabled
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| charts/portkey-app/templates/clickhouse/stateful-set.yaml | Added init container for dynamic user config generation and conditional volume mounts based on external secret usage |
| charts/portkey-app/templates/clickhouse/config-map.yaml | Wrapped users.xml generation in conditional to exclude it when external secrets are used |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {{- if .Values.clickhouse.customConfig.enabled }} | ||
| - name: clickhouse-conf | ||
| configMap: | ||
| name: {{ include "portkey.fullname" . }}-{{ .Values.clickhouse.name }} | ||
| items: | ||
| - key: config.xml | ||
| path: config.xml | ||
| {{- end }} |
There was a problem hiding this comment.
This block duplicates the clickhouse-conf volume definition that appears later in the else branch (lines 191-200). When existingSecretName is set and customConfig is enabled, both volume definitions will be present, causing a conflict with duplicate volume names.
| cat > /etc/clickhouse-server/users.d/users.xml << EOF | ||
| <clickhouse> | ||
| <users> | ||
| <${CLICKHOUSE_USER}> | ||
| <access_management>1</access_management> | ||
| <named_collection_control>1</named_collection_control> | ||
| <show_named_collections>1</show_named_collections> | ||
| <show_named_collections_secrets>1</show_named_collections_secrets> | ||
| <profile>default</profile> | ||
| </${CLICKHOUSE_USER}> |
There was a problem hiding this comment.
Using variable substitution in XML without escaping could allow XML injection if the username contains XML special characters like <, >, or &. Consider validating or escaping the username value.
| cat > /etc/clickhouse-server/users.d/users.xml << EOF | |
| <clickhouse> | |
| <users> | |
| <${CLICKHOUSE_USER}> | |
| <access_management>1</access_management> | |
| <named_collection_control>1</named_collection_control> | |
| <show_named_collections>1</show_named_collections> | |
| <show_named_collections_secrets>1</show_named_collections_secrets> | |
| <profile>default</profile> | |
| </${CLICKHOUSE_USER}> | |
| SAFE_CLICKHOUSE_USER=$(printf '%s' "$CLICKHOUSE_USER" | sed 's/[^A-Za-z0-9_.-]/_/g') | |
| cat > /etc/clickhouse-server/users.d/users.xml << EOF | |
| <clickhouse> | |
| <users> | |
| <${SAFE_CLICKHOUSE_USER}> | |
| <access_management>1</access_management> | |
| <named_collection_control>1</named_collection_control> | |
| <show_named_collections>1</show_named_collections> | |
| <show_named_collections_secrets>1</show_named_collections_secrets> | |
| <profile>default</profile> | |
| </${SAFE_CLICKHOUSE_USER}> |
| {{- if .Values.clickhouse.external.existingSecretName }} | ||
| initContainers: | ||
| - name: generate-users-config | ||
| image: busybox:1.36 |
There was a problem hiding this comment.
The busybox image version is pinned to a tag rather than a digest, which doesn't guarantee immutability and could pose a supply chain security risk. Consider using a digest-based reference for the init container image.
| image: busybox:1.36 | |
| image: busybox@sha256:6d4d57701fb9b31c8ed6dfa81857232ece2b2043614d248f90f42344f9b11a0d |
No description provided.