bito-code-review[bot] commented on code in PR #41777:
URL: https://github.com/apache/superset/pull/41777#discussion_r3525214374
##########
helm/superset/templates/_helpers.tpl:
##########
@@ -105,80 +106,564 @@ app.kubernetes.io/component: {{ .component }}
{{- end -}}
-{{- define "superset-config" }}
+{{/*
+Coalescing resolvers for DB and Redis connection parameters.
+Each resolver checks (in order):
+ 1. New top-level database.* / cache.* values
+ 2. Legacy supersetNode.connections.* keys (deprecation path, using safe
index to avoid errors on absent maps)
+ 3. cluster.* service name overrides
+ 4. Hard-coded defaults derived from the release name
+Call with root context: {{ include "superset.db.host" . }}
+*/}}
+
+{{/*
+Helper to safely read .Values.supersetNode.connections.<key> without erroring
when maps are absent.
+*/}}
+{{- define "_superset.legacyConn" -}}
+{{- $sn := index .Values "supersetNode" | default dict -}}
+{{- $conn := index $sn "connections" | default dict -}}
+{{- $conn | toJson -}}
+{{- end -}}
+
+{{- define "superset.db.host" -}}
+{{- $conn := include "_superset.legacyConn" . | fromJson -}}
+{{- tpl (coalesce .Values.database.host (index $conn "db_host")
.Values.cluster.databaseServiceName (printf "%s-postgresql" .Release.Name)) $
-}}
+{{- end -}}
+
+{{- define "superset.db.port" -}}
+{{- $conn := include "_superset.legacyConn" . | fromJson -}}
+{{- if .Values.database.port -}}
+{{- .Values.database.port | toString -}}
+{{- else -}}
+{{- coalesce (index $conn "db_port") "5432" -}}
+{{- end -}}
+{{- end -}}
+
+{{- define "superset.db.user" -}}
+{{- $conn := include "_superset.legacyConn" . | fromJson -}}
+{{- coalesce .Values.database.user (index $conn "db_user") "superset" -}}
+{{- end -}}
+
+{{- define "superset.db.password" -}}
+{{- $conn := include "_superset.legacyConn" . | fromJson -}}
+{{- coalesce .Values.database.password (index $conn "db_pass") "superset" -}}
+{{- end -}}
+
+{{- define "superset.db.name" -}}
+{{- $conn := include "_superset.legacyConn" . | fromJson -}}
+{{- coalesce .Values.database.name (index $conn "db_name") "superset" -}}
+{{- end -}}
+
+{{- define "superset.db.driver" -}}
+{{- $conn := include "_superset.legacyConn" . | fromJson -}}
+{{- coalesce .Values.database.driver (index $conn "db_type")
"postgresql+psycopg2" -}}
+{{- end -}}
+
+{{- define "superset.redis.host" -}}
+{{- $conn := include "_superset.legacyConn" . | fromJson -}}
+{{- tpl (coalesce .Values.cache.host (index $conn "redis_host")
.Values.cluster.redisServiceName (printf "%s-redis-headless" .Release.Name)) $
-}}
+{{- end -}}
+
+{{- define "superset.redis.port" -}}
+{{- $conn := include "_superset.legacyConn" . | fromJson -}}
+{{- if .Values.cache.port -}}
+{{- .Values.cache.port | toString -}}
+{{- else -}}
+{{- coalesce (index $conn "redis_port") "6379" -}}
+{{- end -}}
+{{- end -}}
+
+{{- define "superset.redis.user" -}}
+{{- $conn := include "_superset.legacyConn" . | fromJson -}}
+{{- coalesce .Values.cache.user (index $conn "redis_user") "" -}}
+{{- end -}}
+
+{{- define "superset.redis.password" -}}
+{{- $conn := include "_superset.legacyConn" . | fromJson -}}
+{{- coalesce .Values.cache.password (index $conn "redis_password") "" -}}
+{{- end -}}
+
+{{- define "superset.redis.cacheDb" -}}
+{{- $conn := include "_superset.legacyConn" . | fromJson -}}
+{{- coalesce (.Values.cache.cacheDb | toString) (index $conn "redis_cache_db")
"1" -}}
+{{- end -}}
+
+{{- define "superset.redis.celeryDb" -}}
+{{- $conn := include "_superset.legacyConn" . | fromJson -}}
+{{- coalesce (.Values.cache.celeryDb | toString) (index $conn
"redis_celery_db") "0" -}}
+{{- end -}}
+
+{{- define "superset.redis.proto" -}}
+{{- if .Values.cache.driver }}{{ .Values.cache.driver }}{{- else if
.Values.cache.ssl.enabled }}rediss{{- else }}redis{{- end -}}
+{{- end -}}
+
+{{- define "superset.config" }}
+{{- /* SECURITY: Validate admin password is set if admin creation is enabled
*/}}
+{{- 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 }}
+
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_DRIVER') or
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 }}
+{{- $driver := include "superset.db.driver" . }}
+{{- $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.db.user" .
}}:{{ include "superset.db.password" . }}@{{ include "superset.db.host" . }}:{{
include "superset.db.port" . }}/{{ include "superset.db.name" . }}{{ $sslParams
}}"
+{{- end }}
+{{- if hasKey .Values.config "SQLALCHEMY_TRACK_MODIFICATIONS" }}
+SQLALCHEMY_TRACK_MODIFICATIONS = {{
.Values.config.SQLALCHEMY_TRACK_MODIFICATIONS | toString | title }}
{{- else }}
-REDIS_BASE_URL=f"{env('REDIS_DRIVER') or
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')}"
+{{- /* Redis Configuration - only if Redis cache is configured */}}
+{{- if .Values.cache.enabled }}
+{{- if .Values.cache.cacheUrl }}
+CACHE_REDIS_URL = {{ .Values.cache.cacheUrl | quote }}
+{{- else }}
+{{- $redisHost := include "superset.redis.host" . }}
+{{- $redisUser := include "superset.redis.user" . }}
+{{- $redisPort := include "superset.redis.port" . }}
+{{- $redisPassword := include "superset.redis.password" . }}
+{{- $useSSL := and (hasKey .Values.cache "ssl") .Values.cache.ssl.enabled }}
+{{- if $redisPassword }}
+{{- if $redisUser }}
+REDIS_BASE_URL = f"{{ include "superset.redis.proto" . }}://{{ $redisUser
}}:{{ $redisPassword }}@{{ $redisHost }}:{{ $redisPort }}"
+{{- else }}
+REDIS_BASE_URL = f"{{ include "superset.redis.proto" . }}://:{{ $redisPassword
}}@{{ $redisHost }}:{{ $redisPort }}"
+{{- end }}
+{{- else }}
+REDIS_BASE_URL = f"{{ include "superset.redis.proto" . }}://{{ $redisHost
}}:{{ $redisPort }}"
+{{- end }}
+{{- if $useSSL }}
+{{- $sslCertReqs := .Values.cache.ssl.ssl_cert_reqs | default "required" }}
+REDIS_URL_PARAMS = f"?ssl_cert_reqs={{ $sslCertReqs }}"
{{- else }}
REDIS_URL_PARAMS = ""
-{{- 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}"
+{{- end }}
+{{- $cacheDb := include "superset.redis.cacheDb" . }}
+CACHE_REDIS_URL = f"{REDIS_BASE_URL}/{{ $cacheDb }}{REDIS_URL_PARAMS}"
+{{- end }}
+{{- if .Values.cache.celeryUrl }}
+CELERY_REDIS_URL = {{ .Values.cache.celeryUrl | quote }}
+{{- else if not .Values.cache.cacheUrl }}
+{{- $celeryDb := include "superset.redis.celeryDb" . }}
+CELERY_REDIS_URL = f"{REDIS_BASE_URL}/{{ $celeryDb }}{REDIS_URL_PARAMS}"
+{{- else }}
+{{- if or .Values.config.celeryConfig (not .Values.cache.enabled) }}
+{{- /* Custom celeryConfig provided or cache disabled - OK */}}
+{{- else }}
+{{- fail "CONFIGURATION ERROR: cache.cacheUrl is set but cache.celeryUrl is
not set. When using cacheUrl, you must also set celeryUrl for Celery to work.
Alternatively, set config.celeryConfig to provide a custom Celery
configuration." }}
+{{- end }}
+{{- end }}
+{{- end }}
-MAPBOX_API_KEY = env('MAPBOX_API_KEY', '')
+{{- /* Cache Configuration */}}
+{{- if .Values.config.cacheConfig }}
+CACHE_CONFIG = {{ .Values.config.cacheConfig | toJson | indent 2 }}
+{{- else if .Values.cache.enabled }}
CACHE_CONFIG = {
- 'CACHE_TYPE': 'RedisCache',
- 'CACHE_DEFAULT_TIMEOUT': 300,
- 'CACHE_KEY_PREFIX': 'superset_',
- 'CACHE_REDIS_URL': CACHE_REDIS_URL,
+ 'CACHE_TYPE': 'RedisCache',
+ 'CACHE_DEFAULT_TIMEOUT': {{ .Values.cache.defaultTimeout | default
(.Values.config.cacheDefaultTimeout | default 86400) | int }},
+ 'CACHE_KEY_PREFIX': {{ .Values.cache.keyPrefix | default "superset_" |
quote }},
+ 'CACHE_REDIS_URL': CACHE_REDIS_URL,
}
+{{- end }}
+
+{{- if .Values.config.dataCacheConfig }}
+DATA_CACHE_CONFIG = {{ .Values.config.dataCacheConfig | toJson | indent 2 }}
+{{- else if .Values.config.cacheConfig }}
DATA_CACHE_CONFIG = CACHE_CONFIG
+{{- else if .Values.cache.enabled }}
+DATA_CACHE_CONFIG = CACHE_CONFIG
+{{- end }}
+{{- /* SQLLAB_ASYNC_TIME_LIMIT_SEC - Required for async_queries module import
(default: 6 hours) */}}
+{{- if .Values.config.SQLLAB_ASYNC_TIME_LIMIT_SEC }}
+SQLLAB_ASYNC_TIME_LIMIT_SEC = {{ .Values.config.SQLLAB_ASYNC_TIME_LIMIT_SEC |
int }}
+{{- else }}
+from datetime import timedelta
+SQLLAB_ASYNC_TIME_LIMIT_SEC = int(timedelta(hours=6).total_seconds())
+{{- end }}
-if os.getenv("SQLALCHEMY_DATABASE_URI"):
- SQLALCHEMY_DATABASE_URI = os.getenv("SQLALCHEMY_DATABASE_URI")
-else:
- {{- if eq .Values.supersetNode.connections.db_type "postgresql" }}
- SQLALCHEMY_DATABASE_URI =
f"postgresql+psycopg2://{os.getenv('DB_USER')}:{os.getenv('DB_PASS')}@{os.getenv('DB_HOST')}:{os.getenv('DB_PORT')}/{os.getenv('DB_NAME')}"
- {{- else if eq .Values.supersetNode.connections.db_type "mysql" }}
- SQLALCHEMY_DATABASE_URI =
f"mysql+mysqldb://{os.getenv('DB_USER')}:{os.getenv('DB_PASS')}@{os.getenv('DB_HOST')}:{os.getenv('DB_PORT')}/{os.getenv('DB_NAME')}"
- {{- else }}
- {{ fail (printf "Unsupported database type: %s. Please use 'postgresql' or
'mysql'." .Values.supersetNode.connections.db_type) }}
- {{- end }}
+{{- /* Celery Configuration */}}
+{{- if .Values.config.celeryConfig }}
+{{- if kindIs "string" .Values.config.celeryConfig }}
+{{ .Values.config.celeryConfig }}
+{{- else }}
+class CeleryConfig:
+{{- range $key, $value := .Values.config.celeryConfig }}
+ {{ $key }} = {{ $value | toJson }}
+{{- end }}
+
+{{- if hasKey .Values.config.celeryConfig "imports" }}
+CELERY_IMPORTS = CeleryConfig.imports
+{{- end }}
+CELERY_CONFIG = CeleryConfig
+{{- end }}
+{{- else if .Values.cache.enabled }}
+from celery.schedules import crontab
+from datetime import timedelta
class CeleryConfig:
- imports = ("superset.sql_lab", )
- broker_url = CELERY_REDIS_URL
- result_backend = CELERY_REDIS_URL
+ imports = (
+ "superset.sql_lab",
+ "superset.tasks.scheduler",
+ "superset.tasks.thumbnails",
+ "superset.tasks.cache",
+ )
+ broker_connection_retry_on_startup = True
+ worker_prefetch_multiplier = 10
+ task_acks_late = True
+ broker_url = CELERY_REDIS_URL
+ result_backend = CELERY_REDIS_URL
+ task_annotations = {
+ "sql_lab.get_sql_results": {
+ "rate_limit": "100/s",
+ },
+ }
+ beat_schedule = {
+ "reports.scheduler": {
+ "task": "reports.scheduler",
+ "schedule": crontab(minute="*", hour="*"),
+ "options": {"expires": int(timedelta(weeks=1).total_seconds())},
+ },
+ "reports.prune_log": {
+ "task": "reports.prune_log",
+ "schedule": crontab(minute=0, hour=0),
+ },
+ }
+CELERY_IMPORTS = CeleryConfig.imports
CELERY_CONFIG = CeleryConfig
+{{- end }}
+
+{{- /* Celery Worker Health Check - File-based health probes for Kubernetes
*/}}
+{{- if and .Values.supersetWorker.healthCheck
.Values.supersetWorker.healthCheck.enabled }}
+# Celery Worker Health Check Configuration
+import threading
+from celery import bootsteps
+from celery.signals import worker_ready, worker_shutdown, worker_init
+
+_readiness_file = {{ .Values.supersetWorker.healthCheck.readinessFile |
default "/tmp/celery_worker_ready" | quote }}
+_liveness_file = {{ .Values.supersetWorker.healthCheck.livenessFile | default
"/tmp/celery_worker_alive" | quote }}
+_heartbeat_interval = {{
.Values.supersetWorker.healthCheck.livenessHeartbeatInterval | default 10 | int
}}
+_liveness_thread = None
+_liveness_stop_event = None
+
+@worker_ready.connect
+def create_ready_file(sender, **kwargs):
+ try:
+ open(_readiness_file, 'w').close()
+ except Exception as e:
+ print(f"Warning: Could not create readiness file: {e}")
+
+@worker_shutdown.connect
+def remove_ready_file(sender, **kwargs):
+ global _liveness_thread, _liveness_stop_event
+ if _liveness_stop_event:
+ _liveness_stop_event.set()
+ if _liveness_thread:
+ _liveness_thread.join(timeout=5)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-778: Missing healthCheck config</b></div>
<div id="fix">
The template references `.Values.supersetWorker.healthCheck.enabled` (line
355) and its subkeys, but `values.yaml` does not define
`supersetWorker.healthCheck`. This causes the template to generate `None` for
all values at render time, making the health check configuration
non-functional. Add the missing `healthCheck` section under `supersetWorker` in
`values.yaml`. (See also:
[CWE-778](https://cwe.mitre.org/data/definitions/778.html))
</div>
</div>
<small><i>Code Review Run #4f624c</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
