codeant-ai-for-open-source[bot] commented on code in PR #38597:
URL: https://github.com/apache/superset/pull/38597#discussion_r2962761232
##########
helm/superset/templates/_helpers.tpl:
##########
@@ -61,83 +85,579 @@ Create chart name and version as used by the chart label.
{{- printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 |
trimSuffix "-" -}}
{{- end -}}
+{{/*
+Common labels for all resources - follows Kubernetes recommended labels
+https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/
+*/}}
+{{- define "superset.labels" -}}
+helm.sh/chart: {{ include "superset.chart" . }}
+{{ include "superset.selectorLabels" . }}
+{{- if .Chart.AppVersion }}
+app.kubernetes.io/version: {{ .Chart.AppVersion | quote }}
+{{- end }}
+app.kubernetes.io/managed-by: {{ .Release.Service }}
+app.kubernetes.io/part-of: superset
+{{- if .Values.extraLabels }}
+{{ toYaml .Values.extraLabels }}
+{{- end }}
+{{- end -}}
+
+{{/*
+Selector labels - used by selectors and matchLabels
+*/}}
+{{- define "superset.selectorLabels" -}}
+app.kubernetes.io/name: {{ include "superset.name" . }}
+app.kubernetes.io/instance: {{ .Release.Name }}
+{{- end -}}
+
+{{/*
+Component labels - extends superset.labels with component-specific labels
+Usage: {{ include "superset.componentLabels" (dict "component" "web" "root" .)
}}
+*/}}
+{{- define "superset.componentLabels" -}}
+{{ include "superset.labels" .root }}
+app.kubernetes.io/component: {{ .component }}
+{{- end -}}
+
+{{/*
+Component selector labels - for matchLabels with component
+Usage: {{ include "superset.componentSelectorLabels" (dict "component" "web"
"root" .) }}
+*/}}
+{{- define "superset.componentSelectorLabels" -}}
+app.kubernetes.io/name: {{ include "superset.name" .root }}
+app.kubernetes.io/instance: {{ .Release.Name }}
+app.kubernetes.io/component: {{ .component }}
+{{- end -}}
+
+
+{{- define "superset.config" }}
+{{- /* Check for deprecated configuration values */}}
+{{- include "superset.checkDeprecatedValues" . }}
+{{- /* SECURITY: Validate admin password is set if admin creation is enabled
*/}}
+{{- /* Note: JWT secret validation is in deployment-ws.yaml since websocket
config is in a separate secret */}}
+{{- if and .Values.init.createAdmin (or (not .Values.init.adminUser.password)
(eq .Values.init.adminUser.password "")) }}
+{{- fail "SECURITY ERROR: init.createAdmin is true but init.adminUser.password
is empty. You must set a secure password using --set
init.adminUser.password='your-password' or via external secret." }}
+{{- end }}
+{{- /* PRODUCTION: Validate resource limits are set for production deployments
*/}}
+{{- if and (not .Values.resources.limits) (not .Values.resources.requests) }}
+{{- /* Note: This is a warning - pre-install validation job will also check
this */}}
+{{- /* Resource limits are critical for production to prevent resource
exhaustion */}}
+{{- end }}
-{{- define "superset-config" }}
import os
+{{- if or .Values.config.cacheConfig .Values.config.dataCacheConfig
.Values.config.resultsBackend .Values.config.celeryConfig .Values.cache.enabled
}}
from flask_caching.backends.rediscache import RedisCache
+{{- end }}
def env(key, default=None):
return os.getenv(key, default)
-# Redis Base URL
-{{- if .Values.supersetNode.connections.redis_password }}
-REDIS_BASE_URL=f"{env('REDIS_PROTO')}://{env('REDIS_USER',
'')}:{env('REDIS_PASSWORD')}@{env('REDIS_HOST')}:{env('REDIS_PORT')}"
+{{- /* Database Configuration - Superset always requires a database */}}
+{{- if .Values.database.uri }}
+SQLALCHEMY_DATABASE_URI = {{ .Values.database.uri | quote }}
+{{- else }}
+{{- /* Determine database host - use explicit host, or default to service name
*/}}
+{{- $dbHost := .Values.database.host }}
+{{- if not $dbHost }}
+{{- if .Values.cluster.databaseServiceName }}
+{{- $dbHost = .Values.cluster.databaseServiceName }}
+{{- else }}
+{{- $dbHost = printf "%s-postgresql" .Release.Name }}
+{{- end }}
+{{- end }}
+{{- $driver := .Values.database.driver | default "postgresql+psycopg2" }}
+{{- $sslParams := "" }}
+{{- if and (hasKey .Values.database "ssl") .Values.database.ssl.enabled }}
+{{- $sslMode := .Values.database.ssl.mode | default "require" }}
+{{- $sslParams = printf "?sslmode=%s" $sslMode }}
+{{- end }}
+SQLALCHEMY_DATABASE_URI = f"{{ $driver }}://{{ include
"superset.urlencodeUserinfo" .Values.database.user }}:{{ include
"superset.urlencodeUserinfo" .Values.database.password }}@{{ $dbHost }}:{{
.Values.database.port }}/{{ .Values.database.name }}{{ $sslParams }}"
+{{- end }}
+{{- if hasKey .Values.config "SQLALCHEMY_TRACK_MODIFICATIONS" }}
+SQLALCHEMY_TRACK_MODIFICATIONS = {{ if
.Values.config.SQLALCHEMY_TRACK_MODIFICATIONS }}True{{ else }}False{{ end }}
{{- else }}
-REDIS_BASE_URL=f"{env('REDIS_PROTO')}://{env('REDIS_HOST')}:{env('REDIS_PORT')}"
+SQLALCHEMY_TRACK_MODIFICATIONS = False
{{- end }}
-# Redis URL Params
-{{- if .Values.supersetNode.connections.redis_ssl.enabled }}
-REDIS_URL_PARAMS = f"?ssl_cert_reqs={env('REDIS_SSL_CERT_REQS')}"
+{{- /* Cache/Redis host fallback - reuse same logic for results backend and
GAQ backends so null host does not break runtime */}}
+{{- $cacheHost := .Values.cache.host }}
+{{- if not $cacheHost }}
+{{- if .Values.cluster.redisServiceName }}
+{{- $cacheHost = .Values.cluster.redisServiceName }}
{{- else }}
-REDIS_URL_PARAMS = ""
-{{- end}}
+{{- $cacheHost = printf "%s-redis-headless" .Release.Name }}
+{{- end }}
+{{- end }}
-# Build Redis URLs
-CACHE_REDIS_URL = f"{REDIS_BASE_URL}/{env('REDIS_DB', 1)}{REDIS_URL_PARAMS}"
-CELERY_REDIS_URL = f"{REDIS_BASE_URL}/{env('REDIS_CELERY_DB',
0)}{REDIS_URL_PARAMS}"
+{{- /* Redis Configuration - only if Redis cache is configured */}}
+{{- if .Values.cache.enabled }}
+{{- if .Values.cache.cacheUrl }}
+CACHE_REDIS_URL = {{ .Values.cache.cacheUrl | quote }}
+{{- else }}
+{{- /* Automatically use rediss (SSL) protocol when SSL is enabled, otherwise
use redis */}}
+{{- /* Determine Redis host - use explicit host, or default to service name
*/}}
+{{- $redisHost := .Values.cache.host }}
+{{- if not $redisHost }}
+{{- if .Values.cluster.redisServiceName }}
+{{- $redisHost = .Values.cluster.redisServiceName }}
+{{- else }}
+{{- $redisHost = printf "%s-redis-headless" .Release.Name }}
+{{- end }}
+{{- end }}
+{{- $redisUser := .Values.cache.user | default "" }}
+{{- $redisPort := .Values.cache.port }}
+{{- $redisPassword := .Values.cache.password }}
+{{- $useSSL := and (hasKey .Values.cache "ssl") .Values.cache.ssl.enabled }}
+{{- if $redisPassword }}
+{{- if $redisUser }}
+REDIS_BASE_URL = f"{{ if $useSSL }}rediss{{ else }}redis{{ end }}://{{
$redisUser }}:{{ $redisPassword }}@{{ $redisHost }}:{{ $redisPort }}"
+{{- else }}
+REDIS_BASE_URL = f"{{ if $useSSL }}rediss{{ else }}redis{{ end }}://:{{
$redisPassword }}@{{ $redisHost }}:{{ $redisPort }}"
Review Comment:
**Suggestion:** Redis credentials are inserted into the URL without
URL-encoding, so passwords/usernames containing reserved URI characters (such
as `@`, `:`, `/`, `#`) will produce an invalid broker/cache URL and cause
runtime connection failures. Encode userinfo fields before building the URL.
[possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Celery broker/backend URL breaks worker task processing.
- ❌ Redis cache URL invalidates caching configuration.
- ⚠️ Web, worker, beat, flower runtime reliability degrades.
```
</details>
```suggestion
REDIS_BASE_URL = "{{ if $useSSL }}rediss{{ else }}redis{{ end }}://{{
include "superset.urlencodeUserinfo" $redisUser }}:{{ include
"superset.urlencodeUserinfo" $redisPassword }}@{{ $redisHost }}:{{ $redisPort
}}"
REDIS_BASE_URL = "{{ if $useSSL }}rediss{{ else }}redis{{ end }}://:{{
include "superset.urlencodeUserinfo" $redisPassword }}@{{ $redisHost }}:{{
$redisPort }}"
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use Redis via chart defaults (`cache.enabled: true` in
`helm/superset/values.yaml:285-288`) and set `cache.password`/`cache.user`
containing
reserved URI characters.
2. Install or template chart: `secret-superset-config.yaml:41` injects
`include
"superset.config"` into `superset_config.py`, where `_helpers.tpl:213/215`
builds
`REDIS_BASE_URL` from raw credentials.
3. That URL is reused to form `CACHE_REDIS_URL` and `CELERY_REDIS_URL`
(`_helpers.tpl:227`
and `233`), then used by cache config and Celery broker/backend
(`_helpers.tpl:252`,
`304-305`).
4. Pods mount this generated config secret (`deployment.yaml:210`,
`deployment-worker.yaml:204`), and Superset/Celery runtime fails Redis
connectivity when
malformed URL is parsed.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** helm/superset/templates/_helpers.tpl
**Line:** 213:215
**Comment:**
*Possible Bug: Redis credentials are inserted into the URL without
URL-encoding, so passwords/usernames containing reserved URI characters (such
as `@`, `:`, `/`, `#`) will produce an invalid broker/cache URL and cause
runtime connection failures. Encode userinfo fields before building the URL.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38597&comment_hash=c8b6f4c78ec6cc08da728da9946a6d143623dffa058629a7688b30902de872e5&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38597&comment_hash=c8b6f4c78ec6cc08da728da9946a6d143623dffa058629a7688b30902de872e5&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
