bito-code-review[bot] commented on code in PR #41777: URL: https://github.com/apache/superset/pull/41777#discussion_r3524887603
########## helm/superset/tests/values_defaults_test.yaml: ########## @@ -0,0 +1,27 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +suite: values defaults +templates: + - secret-env.yaml +tests: + - it: renders with new database defaults + set: + database.host: db.example + cache.enabled: true + asserts: + - hasDocuments: + count: 1 Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete test assertions</b></div> <div id="fix"> This test sets `database.host` and `cache.enabled` but only asserts `hasDocuments: count: 1`. It never validates that the secret contains the expected `DB_HOST: db.example` value or that `REDIS_*` variables are rendered. Compare to `secret_env_test.yaml` lines 26-29 which uses specific `equal` assertions to verify rendered content. </div> </div> <small><i>Code Review Run #dc10d1</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 ########## helm/superset/tests/deprecation_test.yaml: ########## @@ -0,0 +1,28 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +suite: deprecation path does not fail render +templates: + - secret-superset-config.yaml Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Wrong template targeted</b></div> <div id="fix"> This test references `secret-superset-config.yaml` but the deprecation mappings for `supersetNode.connections` and `serviceAccountName` are defined in `_helpers.tpl` and consumed by deployment and serviceaccount templates, not by the secret config template. The test will pass even when deprecation mapping is broken. </div> </div> <small><i>Code Review Run #dc10d1</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 ########## helm/superset/templates/deployment.yaml: ########## @@ -44,10 +44,13 @@ spec: metadata: annotations: # Force reload on config changes - checksum/superset_config.py: {{ include "superset-config" . | sha256sum }} - checksum/superset_init.sh: {{ tpl .Values.init.initscript . | sha256sum }} + checksum/superset_config.py: {{ include "superset.config" . | sha256sum }} + checksum/superset_init.sh: {{ include "superset.initScript" . | sha256sum }} Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing checksum annotation consistency</b></div> <div id="fix"> The web deployment adds checksum for `superset_init.sh` (line 48) to trigger restarts when the init script changes. Worker, beat, and flower deployments do not have this checksum, so they won't restart when the init script changes. This inconsistency could lead to worker pods running stale initialization logic while web pods use the new config. </div> </div> <small><i>Code Review Run #dc10d1</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 ########## helm/superset/templates/_helpers.tpl: ########## @@ -105,80 +106,560 @@ 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 }} Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete admin password security check</b></div> <div id="fix"> The admin password validation at line 202 checks for empty password but not for the insecure default 'admin'. Since `.Values.init.adminUser.password` defaults to 'admin' in values.yaml (line 927), the check `.Values.init.adminUser.password == ""` will never be true with default values, leaving production deployments using the default password. </div> </div> <small><i>Code Review Run #dc10d1</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 ########## helm/superset/templates/_helpers.tpl: ########## @@ -105,80 +106,560 @@ 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 | lower }} {{- 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 }} + +CELERY_IMPORTS = CeleryConfig.imports +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) + try: + if os.path.exists(_readiness_file): + os.remove(_readiness_file) + if os.path.exists(_liveness_file): + os.remove(_liveness_file) + except Exception as e: + print(f"Warning: Could not remove health check files: {e}") + +@worker_init.connect +def start_liveness_heartbeat(sender, **kwargs): + global _liveness_thread, _liveness_stop_event + _liveness_stop_event = threading.Event() + + def update_liveness(): + while not _liveness_stop_event.is_set(): + try: + with open(_liveness_file, 'w') as f: + f.write(str(os.getpid())) + except Exception as e: + print(f"Warning: Could not update liveness file: {e}") + _liveness_stop_event.wait(_heartbeat_interval) + + _liveness_thread = threading.Thread(target=update_liveness, daemon=True) + _liveness_thread.start() +{{- else }} +CELERY_WORKER_HEALTH_CHECK_ENABLED = False +{{- end }} + +{{- /* Results Backend */}} +{{- $redisHostForBackend := include "superset.redis.host" . }} +{{- $redisPortForBackend := include "superset.redis.port" . }} +{{- $redisPasswordForBackend := include "superset.redis.password" . }} +{{- if .Values.config.resultsBackend }} +{{- if kindIs "string" .Values.config.resultsBackend }} +RESULTS_BACKEND = {{ .Values.config.resultsBackend }} +{{- else }} +RESULTS_BACKEND = RedisCache( + host={{ $redisHostForBackend | quote }}, + {{- if $redisPasswordForBackend }} + password={{ $redisPasswordForBackend | quote }}, + {{- end }} + port={{ $redisPortForBackend | int }}, + key_prefix={{ .Values.cache.resultsBackendKeyPrefix | default "superset_results" | quote }}, + {{- if and (hasKey .Values.cache "ssl") .Values.cache.ssl.enabled }} + ssl=True, + ssl_cert_reqs={{ .Values.cache.ssl.ssl_cert_reqs | default "required" | quote }}, + {{- end }} +) +{{- end }} +{{- else if .Values.cache.enabled }} RESULTS_BACKEND = RedisCache( - host=env('REDIS_HOST'), - {{- if .Values.supersetNode.connections.redis_password }} - password=env('REDIS_PASSWORD'), - {{- end }} - port=env('REDIS_PORT'), - key_prefix='superset_results', - {{- if .Values.supersetNode.connections.redis_ssl.enabled }} - ssl=True, - ssl_cert_reqs=env('REDIS_SSL_CERT_REQS'), - {{- end }} + host={{ $redisHostForBackend | quote }}, + {{- if $redisPasswordForBackend }} + password={{ $redisPasswordForBackend | quote }}, + {{- end }} + port={{ $redisPortForBackend | int }}, + key_prefix={{ .Values.cache.resultsBackendKeyPrefix | default "superset_results" | quote }}, + {{- if and (hasKey .Values.cache "ssl") .Values.cache.ssl.enabled }} + ssl=True, + ssl_cert_reqs={{ .Values.cache.ssl.ssl_cert_reqs | default "required" | quote }}, + {{- end }} ) +{{- end }} + +{{- /* Global Async Queries Cache Backend */}} +{{- $redisUserForGaq := include "superset.redis.user" . }} +{{- if .Values.config.GLOBAL_ASYNC_QUERIES_CACHE_BACKEND }} +GLOBAL_ASYNC_QUERIES_CACHE_BACKEND = {{ .Values.config.GLOBAL_ASYNC_QUERIES_CACHE_BACKEND | toJson | indent 2 }} +{{- else if .Values.cache.enabled }} +GLOBAL_ASYNC_QUERIES_CACHE_BACKEND = { + "CACHE_TYPE": "RedisCache", + "CACHE_REDIS_HOST": {{ $redisHostForBackend | quote }}, + "CACHE_REDIS_PORT": {{ $redisPortForBackend | int }}, + "CACHE_REDIS_USER": {{ $redisUserForGaq | quote }}, Review Comment: <div> <div id="suggestion"> <div id="issue"><b>CWE-20: Empty user causes Redis URL malformed</b></div> <div id="fix"> When `cache.user` is not configured (defaults to empty string), `CACHE_REDIS_USER = ""` is generated. Flask-Caching interprets this as an empty username in the Redis URL, causing malformed URL construction (`://:` prefix) and connection failures. Only set CACHE_REDIS_USER when `$redisUserForGaq` is non-empty. (See also: [CWE-20](https://cwe.mitre.org/data/definitions/20.html)) </div> </div> <small><i>Code Review Run #dc10d1</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]
