codeant-ai-for-open-source[bot] commented on code in PR #41777:
URL: https://github.com/apache/superset/pull/41777#discussion_r3543875186
##########
helm/superset/templates/_helpers.tpl:
##########
@@ -105,80 +106,561 @@ 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 -}}
Review Comment:
**Suggestion:** The Redis protocol helper does not read the legacy
`supersetNode.connections.redis_driver` value, so the advertised deprecation
compatibility path is incomplete for this setting. Upgrades that depended on
legacy `redis_driver` will fall back to `redis`/`rediss` logic instead of
preserving prior behavior. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Redis connections may lose TLS or custom driver.
- ⚠️ Legacy redis_driver values ignored after chart upgrade.
- ⚠️ Violates documented deprecation behavior for connections.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure an existing release using the legacy key
`supersetNode.connections.redis_driver` (e.g. `"rediss"`) as shown in the
pre-0.20.0
example in `helm/superset/UPGRADING.md:11-23`, with no `cache.driver` set.
2. Upgrade to chart 0.20.0 without changing values; Helm now applies
defaults from
`helm/superset/README.md:8-24`, where `.Values.cache.driver` has default
`""` and
`.Values.cache.ssl.enabled` defaults to `false`.
3. At render time, the helper `superset.redis.proto` in
`helm/superset/templates/_helpers.tpl:196-198` evaluates `if
.Values.cache.driver` (line
197) as false (empty string), then checks `.Values.cache.ssl.enabled`
(false), so it
returns `"redis"`; it never reads the legacy `redis_driver` from
`_superset.legacyConn`.
4. The secret-env template `helm/superset/templates/secret-env.yaml:18-27`
sets
`REDIS_PROTO` from `superset.redis.proto` and `REDIS_DRIVER` only if
`.Values.cache.driver` is set; upgraded deployments that depended on
`supersetNode.connections.redis_driver` now use plain `"redis"` instead of
their
configured protocol, breaking or changing Redis connectivity despite
`UPGRADING.md:44-57`
promising legacy keys are auto-mapped.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fc9e59a701094ab9b4625af234a6f9dd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=fc9e59a701094ab9b4625af234a6f9dd&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:** 197:197
**Comment:**
*Incomplete Implementation: The Redis protocol helper does not read the
legacy `supersetNode.connections.redis_driver` value, so the advertised
deprecation compatibility path is incomplete for this setting. Upgrades that
depended on legacy `redis_driver` will fall back to `redis`/`rediss` logic
instead of preserving prior behavior.
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%2F41777&comment_hash=83c210241bc71fbc6259e79e48f8b9be13c4b4d7aa0a7534cb9dd61a188b10d2&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41777&comment_hash=83c210241bc71fbc6259e79e48f8b9be13c4b4d7aa0a7534cb9dd61a188b10d2&reaction=dislike'>👎</a>
##########
helm/superset/templates/_helpers.tpl:
##########
@@ -105,80 +106,561 @@ 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 -}}
Review Comment:
**Suggestion:** The legacy fallback for database port is effectively dead
because `database.port` has a non-empty default (`5432`), so this condition is
always true and `supersetNode.connections.db_port` is never used. This breaks
the documented deprecation auto-mapping for existing installs that relied on
legacy `db_port` overrides. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Upgraded releases may connect to wrong database port.
- ❌ Init job wait-for-postgres loop can never succeed.
- ⚠️ Violates UPGRADING.md promise of auto-mapped legacy port.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start from an existing deployment that used the legacy key
`supersetNode.connections.db_port` (example config shown in
`helm/superset/UPGRADING.md:11-23`) to set a non-default port like `"5555"`,
with no
`database.port` set.
2. Upgrade to chart 0.20.0 (this PR) keeping the same values; Helm now
applies the chart
defaults from `helm/superset/values.yaml:35-40`, so `.Values.database.port`
becomes `5432`
even though the user never set it.
3. At render time, the resolver `superset.db.port` in
`helm/superset/templates/_helpers.tpl:133-140` evaluates `if
.Values.database.port` (line
135) as true and returns `.Values.database.port | toString` (line 136),
never reaching the
legacy fallback `coalesce (index $conn "db_port") "5432"` (line 138).
4. The secret template `helm/superset/templates/secret-env.yaml:10-14` sets
`DB_PORT` from
`superset.db.port`, so the wait-for-postgres init container in
`helm/superset/values.yaml:40-63` and Superset itself connect to port 5432
instead of the
configured legacy port, breaking connectivity for installs that relied on
`supersetNode.connections.db_port`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8b568bcc979d46cba47dee41256ca3b5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8b568bcc979d46cba47dee41256ca3b5&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:** 135:139
**Comment:**
*Incomplete Implementation: The legacy fallback for database port is
effectively dead because `database.port` has a non-empty default (`5432`), so
this condition is always true and `supersetNode.connections.db_port` is never
used. This breaks the documented deprecation auto-mapping for existing installs
that relied on legacy `db_port` overrides.
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%2F41777&comment_hash=06bbe55e4e04cc03780fda95bec2c990cb7f5c5dd2d69e5be2f2a975c86f2436&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41777&comment_hash=06bbe55e4e04cc03780fda95bec2c990cb7f5c5dd2d69e5be2f2a975c86f2436&reaction=dislike'>👎</a>
##########
helm/superset/templates/_helpers.tpl:
##########
@@ -187,7 +669,57 @@ RESULTS_BACKEND = RedisCache(
{{- end }}
{{- end }}
+{{- end -}}
+
+{{- define "superset.initScript" -}}
+#!/bin/sh
+set -eu
+echo "Upgrading DB schema..."
+superset db upgrade
+echo "Initializing roles and permissions..."
+superset init
+echo "Init job: Creating admin user and loading initial data..."
+{{- if .Values.init.createAdmin }}
+echo "Creating admin user (if not present)..."
+if superset fab list-users 2>/dev/null | grep -q "username:{{
.Values.init.adminUser.username }}"; then
Review Comment:
**Suggestion:** The admin username is interpolated directly into a
double-quoted shell command, so values containing command substitution syntax
like `$()` or backticks will execute arbitrary commands in the init job
container. Treat this value as untrusted shell input and escape it safely
before using it in shell commands. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Init job shell executes arbitrary commands from username.
- ⚠️ Cluster operators must manually avoid unsafe usernames.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure the chart with a malicious admin username, e.g.
`init.adminUser.username="$(touch /tmp/pwned)"`, in
`helm/superset/values.yaml:34-39` or
via `--set` on `helm install`.
2. Render the init script template `superset.initScript` from
`helm/superset/templates/_helpers.tpl:674-708`; observe the line `if
superset fab
list-users 2>/dev/null | grep -q "username:$(touch /tmp/pwned)"; then`.
3. The init job command in `helm/superset/values.yaml:20-25` runs `/bin/sh
-c ". {{
.Values.configMountPath }}/superset_bootstrap.sh; . {{
.Values.configMountPath
}}/superset_init.sh"`, executing the rendered script inside the init
container.
4. When the `grep -q` line at `helm/superset/templates/_helpers.tpl:684`
runs, the shell
performs command substitution on `$(touch /tmp/pwned)`, creating
`/tmp/pwned` (or running
any attacker-supplied command) inside the init job container.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=336ea8d19a6d41e2addc5800171cb18c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=336ea8d19a6d41e2addc5800171cb18c&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:** 684:684
**Comment:**
*Security: The admin username is interpolated directly into a
double-quoted shell command, so values containing command substitution syntax
like `$()` or backticks will execute arbitrary commands in the init job
container. Treat this value as untrusted shell input and escape it safely
before using it in shell commands.
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%2F41777&comment_hash=2a7b157caab8e12ac2d1c29b08d7698b1d181937f21ee0ba3a5c63ed9a7e8f85&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41777&comment_hash=2a7b157caab8e12ac2d1c29b08d7698b1d181937f21ee0ba3a5c63ed9a7e8f85&reaction=dislike'>👎</a>
##########
helm/superset/templates/_helpers.tpl:
##########
@@ -105,80 +106,561 @@ 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" -}}
Review Comment:
**Suggestion:** This resolver always prefers `database.driver`, which has a
non-empty chart default, so legacy `supersetNode.connections.db_type` can no
longer override the driver during upgrades. Existing deployments using legacy
DB driver settings will silently switch behavior instead of being auto-mapped.
[incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Deployments lose custom database driver on upgrade.
- ❌ SQLALCHEMY_DATABASE_URI silently changes driver backend.
- ⚠️ Undermines deprecation path for legacy db_type.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. On an older chart version, configure a non-default database driver via
the legacy key
`supersetNode.connections.db_type` (e.g. `"postgresql+pg8000"`) in values,
as described by
the structured connection schema in `helm/superset/UPGRADING.md:7-26`.
2. Upgrade to this chart version without explicitly setting
`database.driver`; Helm
applies the chart default `database.driver: "postgresql+psycopg2"` from
`helm/superset/helm/superset/README.md:35-40` / `values.yaml`.
3. At render time, the resolver `superset.db.driver` in
`helm/superset/templates/_helpers.tpl:157-160` computes `coalesce
.Values.database.driver
(index $conn "db_type") "postgresql+psycopg2"` (line 159); because
`.Values.database.driver` is the non-empty default, the legacy `db_type`
value from
`$conn` is never used.
4. The SQLAlchemy URI in `helm/superset/templates/_helpers.tpl:220-226` is
built with
`$driver := include "superset.db.driver" .`, so Superset starts using
`"postgresql+psycopg2"` instead of the legacy-configured driver, potentially
breaking
connectivity or changing behavior for deployments that relied on `db_type`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=78e95e27591e44f2a46158262a995ab3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=78e95e27591e44f2a46158262a995ab3&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:** 159:159
**Comment:**
*Incomplete Implementation: This resolver always prefers
`database.driver`, which has a non-empty chart default, so legacy
`supersetNode.connections.db_type` can no longer override the driver during
upgrades. Existing deployments using legacy DB driver settings will silently
switch behavior instead of being auto-mapped.
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%2F41777&comment_hash=22e3ceae7145b48e8e0002ae10c0d89b24b0f5b7a52742e7f8188383516846d4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41777&comment_hash=22e3ceae7145b48e8e0002ae10c0d89b24b0f5b7a52742e7f8188383516846d4&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]
