Re: [PR] feat(helm): use env redis_driver, render additional deployment pod spec [superset]

Mon, 22 Jun 2026 21:58:33 -0700 


codeant-ai-for-open-source[bot] commented on code in PR #26040:
URL: https://github.com/apache/superset/pull/26040#discussion_r3457102495


##########
helm/superset/templates/_helpers.tpl:
##########
@@ -112,8 +112,13 @@ SQLALCHEMY_TRACK_MODIFICATIONS = True
 
 class CeleryConfig:
   imports  = ("superset.sql_lab", )
-  broker_url = CELERY_REDIS_URL
-  result_backend = CELERY_REDIS_URL
+  {{- if .Values.supersetNode.connections.redis_password }}
+  broker_url = 
f"{env('REDIS_DRIVER')}://:{env('REDIS_PASSWORD')}@{env('REDIS_HOST')}:{env('REDIS_PORT')}/0"
+  result_backend = 
f"{env('REDIS_DRIVER')}://:{env('REDIS_PASSWORD')}@{env('REDIS_HOST')}:{env('REDIS_PORT')}/0"
+  {{- else }}
+  broker_url = 
f"{env('REDIS_DRIVER')}://{env('REDIS_HOST')}:{env('REDIS_PORT')}/0"

Review Comment:
   **Suggestion:** The broker/backend DB is now hardcoded to `/0`, so the 
configurable `redis_celery_db` (`REDIS_CELERY_DB`) is ignored and deployments 
using a non-zero Celery DB will silently connect to the wrong Redis database. 
[logic error]
   
   <details>
   <summary><b>Severity Level:</b> Major ⚠️</summary>
   
   ```mdx
   - ⚠️ Celery worker always uses Redis DB 0 ignoring config.
   - ⚠️ Deployments with nonzero `redis_celery_db` hit wrong database.
   ```
   </details>
   <details>
   <summary><b>Steps of Reproduction ✅ </b></summary>
   
   ```mdx
   1. Set a non-zero Celery DB, for example 
`supersetNode.connections.redis_celery_db: "2"`,
   in `helm/superset/values.yaml:80-82` and deploy the chart.
   
   2. The secret template `helm/superset/templates/secret-env.yaml:30-69` then 
renders
   `REDIS_CELERY_DB: "2"` into the environment secret referenced by 
`envFromSecret`.
   
   3. The helper `helm/superset/templates/_helpers.tpl:27-29` builds 
`CELERY_REDIS_URL =
   f"{REDIS_BASE_URL}/{env('REDIS_CELERY_DB', 0)}{REDIS_URL_PARAMS}"`, which 
reflects the
   configured DB index 2, but `class CeleryConfig` (lines 54-61) hardcodes 
`broker_url` and
   `result_backend` to `.../0` (lines 119-120) instead of using 
`CELERY_REDIS_URL` or
   `REDIS_CELERY_DB`.
   
   4. When the Celery worker Deployment from
   `helm/superset/templates/deployment-worker.yaml:100-47` starts, it connects 
to Redis
   database 0 while the configuration and expectations (e.g. external 
monitoring or other
   components) point to database 2, so Celery queues and metadata end up in the 
wrong Redis
   DB.
   ```
   </details>
   
   [![Fix in 
Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1eac5db84aae4df7b03377c158c8ef1e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
 [![Fix in VSCode 
Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1eac5db84aae4df7b03377c158c8ef1e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   
   *(Use Cmd/Ctrl + Click for best experience)*
   <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:** 119:119
   **Comment:**
        *Logic Error: The broker/backend DB is now hardcoded to `/0`, so the 
configurable `redis_celery_db` (`REDIS_CELERY_DB`) is ignored and deployments 
using a non-zero Celery DB will silently connect to the wrong Redis database.
   
   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.
   Once fix is implemented, also check other comments on the same PR, and ask 
user if the user wants to fix the rest of the comments as well. if said yes, 
then fetch all the comments validate the correctness and implement a minimal fix
   ```
   </details>
   <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F26040&comment_hash=cad005b1d6c3d8f0f55ee67a63fff999ab5f4a74daa3108a2da3ed4baf58bd02&reaction=like'>👍</a>
 | <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F26040&comment_hash=cad005b1d6c3d8f0f55ee67a63fff999ab5f4a74daa3108a2da3ed4baf58bd02&reaction=dislike'>👎</a>



##########
helm/superset/templates/_helpers.tpl:
##########
@@ -112,8 +112,13 @@ SQLALCHEMY_TRACK_MODIFICATIONS = True
 
 class CeleryConfig:
   imports  = ("superset.sql_lab", )
-  broker_url = CELERY_REDIS_URL
-  result_backend = CELERY_REDIS_URL
+  {{- if .Values.supersetNode.connections.redis_password }}
+  broker_url = 
f"{env('REDIS_DRIVER')}://:{env('REDIS_PASSWORD')}@{env('REDIS_HOST')}:{env('REDIS_PORT')}/0"

Review Comment:
   **Suggestion:** These URLs depend on `env('REDIS_DRIVER')`, but the chart 
still only injects `REDIS_PROTO` in `secret-env.yaml`; when `REDIS_DRIVER` is 
unset, Celery gets an invalid scheme like `None://...` and fails to connect. 
Add a fallback default (for example `redis`) or ensure `REDIS_DRIVER` is always 
injected. [api mismatch]
   
   <details>
   <summary><b>Severity Level:</b> Critical 🚨</summary>
   
   ```mdx
   - ❌ Celery worker pods fail starting with invalid broker URL.
   - ❌ Async SQL Lab and scheduler tasks stop processing.
   ```
   </details>
   <details>
   <summary><b>Steps of Reproduction ✅ </b></summary>
   
   ```mdx
   1. Deploy the Helm chart using default values from 
`helm/superset/values.yaml:48-87`,
   leaving `.Values.supersetNode.connections.redis_driver` at `"redis"` and not 
adding
   `REDIS_DRIVER` to `extraSecretEnv`.
   
   2. The environment secret is rendered by 
`helm/superset/templates/secret-env.yaml:30-69`,
   which sets `REDIS_HOST`, `REDIS_PORT`, `REDIS_PROTO`, `REDIS_DB`, and 
`REDIS_CELERY_DB`
   but does not define any `REDIS_DRIVER` variable.
   
   3. The Celery worker Deployment in 
`helm/superset/templates/deployment-worker.yaml:100-20`
   uses `envFrom.secretRef.name: {{ tpl .Values.envFromSecret . | quote }}` 
(lines 14–20) so
   the worker container only receives the variables from `secret-env.yaml`, and
   `REDIS_DRIVER` is absent in the pod environment.
   
   4. The Superset config file mounted into the worker (`configMountPath` from
   `values.yaml:67-69`) is generated by 
`helm/superset/templates/_helpers.tpl:6-37,52-62`;
   helper `env(key, default=None)` wraps `os.getenv`, and `CeleryConfig` builds
   `broker_url`/`result_backend` as `f"{env('REDIS_DRIVER')}://..."` (line 
116). With
   `REDIS_DRIVER` unset, `env('REDIS_DRIVER')` is `None`, so Celery receives a 
broker URL
   like `"None://redis-host:6379/0"` and fails to start due to an invalid 
scheme.
   ```
   </details>
   
   [![Fix in 
Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=983c31b225de403a819c667568085f14&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
 [![Fix in VSCode 
Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=983c31b225de403a819c667568085f14&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   
   *(Use Cmd/Ctrl + Click for best experience)*
   <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:** 116:116
   **Comment:**
        *Api Mismatch: These URLs depend on `env('REDIS_DRIVER')`, but the 
chart still only injects `REDIS_PROTO` in `secret-env.yaml`; when 
`REDIS_DRIVER` is unset, Celery gets an invalid scheme like `None://...` and 
fails to connect. Add a fallback default (for example `redis`) or ensure 
`REDIS_DRIVER` is always injected.
   
   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.
   Once fix is implemented, also check other comments on the same PR, and ask 
user if the user wants to fix the rest of the comments as well. if said yes, 
then fetch all the comments validate the correctness and implement a minimal fix
   ```
   </details>
   <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F26040&comment_hash=b70d41a6a7262caf4b2fee4ffbe59b5a0ea9de73d6854e932458437d6ba5b876&reaction=like'>👍</a>
 | <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F26040&comment_hash=b70d41a6a7262caf4b2fee4ffbe59b5a0ea9de73d6854e932458437d6ba5b876&reaction=dislike'>👎</a>



##########
helm/superset/templates/_helpers.tpl:
##########
@@ -112,8 +112,13 @@ SQLALCHEMY_TRACK_MODIFICATIONS = True
 
 class CeleryConfig:
   imports  = ("superset.sql_lab", )
-  broker_url = CELERY_REDIS_URL
-  result_backend = CELERY_REDIS_URL
+  {{- if .Values.supersetNode.connections.redis_password }}
+  broker_url = 
f"{env('REDIS_DRIVER')}://:{env('REDIS_PASSWORD')}@{env('REDIS_HOST')}:{env('REDIS_PORT')}/0"
+  result_backend = 
f"{env('REDIS_DRIVER')}://:{env('REDIS_PASSWORD')}@{env('REDIS_HOST')}:{env('REDIS_PORT')}/0"
+  {{- else }}
+  broker_url = 
f"{env('REDIS_DRIVER')}://{env('REDIS_HOST')}:{env('REDIS_PORT')}/0"
+  result_backend = 
f"{env('REDIS_DRIVER')}://{env('REDIS_HOST')}:{env('REDIS_PORT')}/0"

Review Comment:
   **Suggestion:** Building Celery URLs directly here drops the SSL URL 
parameters (`REDIS_URL_PARAMS` / `REDIS_SSL_CERT_REQS`) that this template 
computes earlier, so SSL-enabled Redis deployments can fail TLS negotiation or 
cert validation for Celery connections. [logic error]
   
   <details>
   <summary><b>Severity Level:</b> Critical 🚨</summary>
   
   ```mdx
   - ❌ Celery broker over TLS ignores `REDIS_SSL_CERT_REQS` setting.
   - ❌ SSL Redis deployments may fail Celery broker connections.
   ```
   </details>
   <details>
   <summary><b>Steps of Reproduction ✅ </b></summary>
   
   ```mdx
   1. Enable TLS for Redis by setting 
`.Values.supersetNode.connections.redis_ssl.enabled:
   true` in `helm/superset/values.yaml:82-86` and provide an appropriate
   `redis_ssl.ssl_cert_reqs` (for example `"CERT_NONE"` for self-signed certs).
   
   2. The secret template `helm/superset/templates/secret-env.yaml:11-17` 
renders
   `REDIS_PROTO="rediss"` and `REDIS_SSL_CERT_REQS` into the environment used 
by all pods via
   `envFromSecret`.
   
   3. The helper `helm/superset/templates/_helpers.tpl:20-29` computes 
`REDIS_URL_PARAMS =
   f"?ssl_cert_reqs={env('REDIS_SSL_CERT_REQS')}"` and includes it in 
`CELERY_REDIS_URL`, but
   the `CeleryConfig` definition (lines 54-61) builds `broker_url` and 
`result_backend` as
   `f"{env('REDIS_DRIVER')}://.../0"` without appending `REDIS_URL_PARAMS`.
   
   4. When the Celery worker from 
`helm/superset/templates/deployment-worker.yaml:100-47`
   starts against a TLS-enabled Redis instance whose certificate would only be 
accepted if
   `ssl_cert_reqs` matched `REDIS_SSL_CERT_REQS`, the missing URL parameters 
mean Celery uses
   Redis defaults, causing TLS handshake or certificate validation failures and 
preventing
   Celery from connecting.
   ```
   </details>
   
   [![Fix in 
Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a2653fd71e98480cb676ff83f290e0e4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
 [![Fix in VSCode 
Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a2653fd71e98480cb676ff83f290e0e4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   
   *(Use Cmd/Ctrl + Click for best experience)*
   <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:** 120:120
   **Comment:**
        *Logic Error: Building Celery URLs directly here drops the SSL URL 
parameters (`REDIS_URL_PARAMS` / `REDIS_SSL_CERT_REQS`) that this template 
computes earlier, so SSL-enabled Redis deployments can fail TLS negotiation or 
cert validation for Celery connections.
   
   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.
   Once fix is implemented, also check other comments on the same PR, and ask 
user if the user wants to fix the rest of the comments as well. if said yes, 
then fetch all the comments validate the correctness and implement a minimal fix
   ```
   </details>
   <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F26040&comment_hash=457a08b2a2ae71f3aa5712a4825c6502374589914d620250679a727732505f44&reaction=like'>👍</a>
 | <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F26040&comment_hash=457a08b2a2ae71f3aa5712a4825c6502374589914d620250679a727732505f44&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]

Reply via email to