You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/06/24 19:18:40 UTC
[PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
rusackas opened a new pull request, #41396:
URL: https://github.com/apache/superset/pull/41396
### SUMMARY
Adopts the work from [anandraiin3/superset#32](https://github.com/anandraiin3/superset/pull/32) (originally authored via Devin AI, thanks to @anandraiin3 for the fork work) and supersedes the stale #25799 by @Cerebus. **Closes #25798.**
When Istio automatic sidecar injection is enabled on the namespace where Superset is deployed, the `init-db` Job hangs forever: the injected `istio-proxy` container keeps running after the init-db container exits, so the Job never reaches `Completed`.
This change adds two opt-in mitigations under a new `init.istio` block in the chart's `values.yaml`. **Both default to `false`**, so the chart's behavior is unchanged for users not running Istio.
#### `init.istio.disableSidecarInjection` (default `false`)
When `true`, the init-db Job's pod template gains the `sidecar.istio.io/inject: "false"` label, excluding the Job from automatic sidecar injection. This is the cleanest fix and is recommended whenever the cluster's mesh policy allows opting out per pod. Rendered into `helm/superset/templates/init-job.yaml` via `dig` so that users who strip the `init.istio` block from custom values still get a clean render.
```yaml
init:
istio:
disableSidecarInjection: true
```
#### `init.istio.terminateSidecarOnExit` (default `false`)
When `true`, the rendered `superset_init.sh` registers an `EXIT` trap that POSTs to the Istio pilot-agent's `/quitquitquit` admin endpoint after the init logic finishes. The trap fires on both success and failure paths (`set -eu`), so the sidecar is terminated either way. Use this when the cluster's mesh policy enforces injection and per-pod opt-out is not allowed. The endpoint is configurable via `init.istio.quitEndpoint` (default `http://localhost:15020/quitquitquit`).
```yaml
init:
istio:
terminateSidecarOnExit: true
```
#### Other changes
- `helm/superset/Chart.yaml` version bumped `0.17.3` → `0.17.4` (patch — non-breaking, opt-in fix). Required by the `Helm: lint and test charts` workflow.
- `helm/superset/README.md` regenerated with `helm-docs`.
- `helm/superset/tests/test-istio.sh` added (13 assertions over 5 `helm template` value matrices).
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable — no UI changes (Helm chart only).
### TESTING INSTRUCTIONS
Automated chart-rendering tests:
```bash
cd helm/superset && helm dependency build
bash tests/test-istio.sh
# passed: 13, failed: 0
helm lint helm/superset
# 1 chart(s) linted, 0 chart(s) failed
```
Manual end-to-end verification (mirrors the bug repro):
1. Stand up a Kubernetes cluster with Istio installed.
2. Create a `superset` namespace with automatic sidecar injection:
```bash
kubectl create namespace superset
kubectl label namespace superset istio-injection=enabled
```
3. Install with the mitigation enabled:
```bash
helm install superset ./helm/superset -n superset \
--set init.istio.disableSidecarInjection=true
```
4. Watch the init-db Job:
```bash
kubectl -n superset get jobs -w
kubectl -n superset describe pod -l job-name=superset-init-db
```
The pod should have only the init-db container (no `istio-proxy` injected) and the Job should reach `Completed` instead of hanging.
5. Repeat with `--set init.istio.terminateSidecarOnExit=true` (and `disableSidecarInjection=false`) on a cluster where the namespace policy forces injection. Confirm the Job pod has both containers, the init-db container exits, the `istio-proxy` container terminates shortly after via the `/quitquitquit` POST, and the Job reaches `Completed`.
6. Confirm the default (`helm install superset ./helm/superset -n superset`) still produces the unchanged manifest and Job behavior in non-Istio namespaces.
### ADDITIONAL INFORMATION
- [x] Has associated issue: closes #25798
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [x] Introduces new feature or API (new opt-in `init.istio.*` chart values)
- [ ] Removes existing feature or API
---
This adopts and rebases the work from anandraiin3/superset#32 (authored via Devin AI) onto current `master`, and supersedes the stale draft #25799 by @Cerebus.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #41396:
URL: https://github.com/apache/superset/pull/41396#discussion_r3565306290
##########
helm/superset/templates/_helpers.tpl:
##########
@@ -685,6 +685,13 @@ TALISMAN_CONFIG = {
{{- define "superset.initScript" -}}
#!/bin/sh
set -eu
+{{- if .Values.init.istio.terminateSidecarOnExit }}
Review Comment:
Good catch. Switched to `dig` with a false default so a nulled `init.istio` override renders clean instead of erroring, matching the pattern already in `init-job.yaml`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41396:
URL: https://github.com/apache/superset/pull/41396#issuecomment-4949578527
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41396:
URL: https://github.com/apache/superset/pull/41396#issuecomment-5027188977
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41396:
URL: https://github.com/apache/superset/pull/41396#discussion_r3616799842
##########
helm/superset/tests/test-istio.sh:
##########
@@ -0,0 +1,177 @@
+#!/usr/bin/env bash
+#
+# 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.
+#
+# Renders the chart with `helm template` for several `init.istio.*` value
+# combinations and asserts that the relevant manifests contain (or omit)
+# the expected fields. Intended to be run from the chart directory or via
+# `bash helm/superset/tests/test-istio.sh` from the repo root.
+#
+# Covers the fix for:
+# https://github.com/apache/superset/issues/25798
+set -euo pipefail
+
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+CHART_DIR="$(cd "${SCRIPT_DIR}/.." && pwd)"
+
+pass=0
+fail=0
+
+assert_contains() {
+ local label="$1"
+ local needle="$2"
+ local haystack="$3"
+ if grep -qF -- "${needle}" <<<"${haystack}"; then
+ echo " PASS: ${label}"
+ pass=$((pass + 1))
+ else
+ echo " FAIL: ${label}"
+ echo " expected to contain: ${needle}"
+ fail=$((fail + 1))
+ fi
+}
+
+assert_not_contains() {
+ local label="$1"
+ local needle="$2"
+ local haystack="$3"
+ if ! grep -qF -- "${needle}" <<<"${haystack}"; then
+ echo " PASS: ${label}"
+ pass=$((pass + 1))
+ else
+ echo " FAIL: ${label}"
+ echo " expected NOT to contain: ${needle}"
+ fail=$((fail + 1))
+ fi
+}
+
+render() {
+ helm template release "${CHART_DIR}" "$@"
+}
+
+extract_init_job() {
+ awk '
+ /^# Source: superset\/templates\/init-job\.yaml/ { capture = 1 }
+ capture && /^---$/ { capture = 0 }
+ capture { print }
+ ' <<<"$1"
+}
+
+extract_config_secret() {
+ awk '
+ /^# Source: superset\/templates\/secret-superset-config\.yaml/ { capture = 1 }
+ capture && /^---$/ { capture = 0 }
+ capture { print }
+ ' <<<"$1"
+}
+
+echo "==> defaults: no istio mitigations applied"
+out_default="$(render)"
+init_job_default="$(extract_init_job "${out_default}")"
+config_default="$(extract_config_secret "${out_default}")"
+assert_not_contains "default does not set sidecar.istio.io/inject label" \
+ 'sidecar.istio.io/inject' "${init_job_default}"
+assert_not_contains "default initscript does not register quitquitquit trap" \
+ 'quitquitquit' "${config_default}"
+
+echo "==> init.istio.disableSidecarInjection=true"
+out_disable="$(render --set init.istio.disableSidecarInjection=true)"
+init_job_disable="$(extract_init_job "${out_disable}")"
+config_disable="$(extract_config_secret "${out_disable}")"
+assert_contains "init job sets sidecar.istio.io/inject: \"false\"" \
+ 'sidecar.istio.io/inject: "false"' "${init_job_disable}"
+assert_not_contains "disableSidecarInjection alone does not add quitquitquit trap" \
+ 'quitquitquit' "${config_disable}"
+
+echo "==> init.istio.terminateSidecarOnExit=true"
+out_terminate="$(render --set init.istio.terminateSidecarOnExit=true)"
+init_job_terminate="$(extract_init_job "${out_terminate}")"
+config_terminate="$(extract_config_secret "${out_terminate}")"
+assert_not_contains "terminateSidecarOnExit alone does not add inject label" \
+ 'sidecar.istio.io/inject' "${init_job_terminate}"
+assert_contains "initscript registers EXIT trap that calls quitquitquit" \
+ "trap 'rc=\$?; curl -fsS -m 5 -X POST" "${config_terminate}"
+assert_contains "initscript trap targets the configured quit endpoint" \
+ 'http://localhost:15020/quitquitquit' "${config_terminate}"
+assert_contains "initscript trap binds to the EXIT signal" \
+ "' EXIT" "${config_terminate}"
+
+echo "==> init.istio.quitEndpoint override"
+out_endpoint="$(render --set init.istio.terminateSidecarOnExit=true \
+ --set init.istio.quitEndpoint=http://127.0.0.1:15020/quitquitquit)"
+config_endpoint="$(extract_config_secret "${out_endpoint}")"
+assert_contains "trap honours custom quitEndpoint" \
+ 'http://127.0.0.1:15020/quitquitquit' "${config_endpoint}"
+
+echo "==> both options combined"
+out_both="$(render --set init.istio.disableSidecarInjection=true \
+ --set init.istio.terminateSidecarOnExit=true)"
+init_job_both="$(extract_init_job "${out_both}")"
+config_both="$(extract_config_secret "${out_both}")"
+assert_contains "combined: inject label present" \
+ 'sidecar.istio.io/inject: "false"' "${init_job_both}"
+assert_contains "combined: trap present" \
+ 'quitquitquit' "${config_both}"
+
+echo "==> existing init.podLabels are preserved alongside istio label"
+out_labels="$(render --set init.istio.disableSidecarInjection=true \
+ --set init.podLabels.team=data-platform)"
+init_job_labels="$(extract_init_job "${out_labels}")"
+assert_contains "user-supplied podLabel still rendered" \
+ 'team: data-platform' "${init_job_labels}"
+assert_contains "istio inject label rendered alongside" \
+ 'sidecar.istio.io/inject: "false"' "${init_job_labels}"
+
+echo "==> init.istio explicitly overridden to null"
+out_null_istio="$(render --set init.istio=null)"
+init_job_null_istio="$(extract_init_job "${out_null_istio}")"
+config_null_istio="$(extract_config_secret "${out_null_istio}")"
+assert_not_contains "null init.istio does not set inject label" \
+ 'sidecar.istio.io/inject' "${init_job_null_istio}"
+assert_not_contains "null init.istio does not register quitquitquit trap" \
+ 'quitquitquit' "${config_null_istio}"
+
+echo "==> EXIT trap propagates the script's exit code, not the notification's"
+# The trap's own curl call is best-effort (failures are logged with
+# `|| echo ... >&2`, not swallowed with `|| true`) and must not mask a
+# failed migration. Extract the two rendered lines and actually run them,
+# with curl pointed at a closed local port so the notification itself fails,
+# to make sure the wrapped script's real exit code still comes through.
+quit_endpoint_line="$(grep -F 'ISTIO_QUIT_ENDPOINT=' <<<"${config_terminate}")"
+trap_line="$(grep -F "trap 'rc=\$?; curl" <<<"${config_terminate}")"
Review Comment:
**Suggestion:** This `grep` is also fatal under `set -e`, so if the trap line format changes or is absent the script exits immediately rather than incrementing `fail` and continuing. Handle missing matches explicitly instead of relying on a command substitution that can hard-stop the run. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Istio Helm test script can abort mid-run on grep.
- ⚠️ EXIT-trap behavior test is skipped on missing line.
- ⚠️ CI output loses final test summary diagnostics.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In helm/superset/tests/test-istio.sh, `set -euo pipefail` at line 25 configures the
shell to exit when a simple command fails outside conditional contexts.
2. `config_terminate` is built at lines 100-102 from `render --set
init.istio.terminateSidecarOnExit=true` and is expected to contain the EXIT trap line that
calls curl, as asserted earlier at lines 103-110.
3. At line 154, the script assigns `trap_line="$(grep -F "trap 'rc=\$?; curl" ...)"`; if
the Helm template in superset/templates/secret-superset-config.yaml is refactored so the
trap format changes or the line is missing from `config_terminate`, this grep exits with
status 1.
4. Because of `set -e`, that non-zero exit status in the command substitution makes
helm/superset/tests/test-istio.sh terminate immediately at line 154, preventing the
subshell EXIT-code propagation check at lines 155-161 and the final pass/fail summary at
lines 173-177 from running.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7767ea461e0242e49909ae3e11f0bffd&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=7767ea461e0242e49909ae3e11f0bffd&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/tests/test-istio.sh
**Line:** 154:154
**Comment:**
*Logic Error: This `grep` is also fatal under `set -e`, so if the trap line format changes or is absent the script exits immediately rather than incrementing `fail` and continuing. Handle missing matches explicitly instead of relying on a command substitution that can hard-stop the run.
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%2F41396&comment_hash=0fe227c0488391ab5c971d14801daec7e8dacdf529e2b8bff76a8f768f08032c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41396&comment_hash=0fe227c0488391ab5c971d14801daec7e8dacdf529e2b8bff76a8f768f08032c&reaction=dislike'>👎</a>
##########
helm/superset/tests/test-istio.sh:
##########
@@ -0,0 +1,177 @@
+#!/usr/bin/env bash
+#
+# 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.
+#
+# Renders the chart with `helm template` for several `init.istio.*` value
+# combinations and asserts that the relevant manifests contain (or omit)
+# the expected fields. Intended to be run from the chart directory or via
+# `bash helm/superset/tests/test-istio.sh` from the repo root.
+#
+# Covers the fix for:
+# https://github.com/apache/superset/issues/25798
+set -euo pipefail
+
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+CHART_DIR="$(cd "${SCRIPT_DIR}/.." && pwd)"
+
+pass=0
+fail=0
+
+assert_contains() {
+ local label="$1"
+ local needle="$2"
+ local haystack="$3"
+ if grep -qF -- "${needle}" <<<"${haystack}"; then
+ echo " PASS: ${label}"
+ pass=$((pass + 1))
+ else
+ echo " FAIL: ${label}"
+ echo " expected to contain: ${needle}"
+ fail=$((fail + 1))
+ fi
+}
+
+assert_not_contains() {
+ local label="$1"
+ local needle="$2"
+ local haystack="$3"
+ if ! grep -qF -- "${needle}" <<<"${haystack}"; then
+ echo " PASS: ${label}"
+ pass=$((pass + 1))
+ else
+ echo " FAIL: ${label}"
+ echo " expected NOT to contain: ${needle}"
+ fail=$((fail + 1))
+ fi
+}
+
+render() {
+ helm template release "${CHART_DIR}" "$@"
+}
+
+extract_init_job() {
+ awk '
+ /^# Source: superset\/templates\/init-job\.yaml/ { capture = 1 }
+ capture && /^---$/ { capture = 0 }
+ capture { print }
+ ' <<<"$1"
+}
+
+extract_config_secret() {
+ awk '
+ /^# Source: superset\/templates\/secret-superset-config\.yaml/ { capture = 1 }
+ capture && /^---$/ { capture = 0 }
+ capture { print }
+ ' <<<"$1"
+}
+
+echo "==> defaults: no istio mitigations applied"
+out_default="$(render)"
+init_job_default="$(extract_init_job "${out_default}")"
+config_default="$(extract_config_secret "${out_default}")"
+assert_not_contains "default does not set sidecar.istio.io/inject label" \
+ 'sidecar.istio.io/inject' "${init_job_default}"
+assert_not_contains "default initscript does not register quitquitquit trap" \
+ 'quitquitquit' "${config_default}"
+
+echo "==> init.istio.disableSidecarInjection=true"
+out_disable="$(render --set init.istio.disableSidecarInjection=true)"
+init_job_disable="$(extract_init_job "${out_disable}")"
+config_disable="$(extract_config_secret "${out_disable}")"
+assert_contains "init job sets sidecar.istio.io/inject: \"false\"" \
+ 'sidecar.istio.io/inject: "false"' "${init_job_disable}"
+assert_not_contains "disableSidecarInjection alone does not add quitquitquit trap" \
+ 'quitquitquit' "${config_disable}"
+
+echo "==> init.istio.terminateSidecarOnExit=true"
+out_terminate="$(render --set init.istio.terminateSidecarOnExit=true)"
+init_job_terminate="$(extract_init_job "${out_terminate}")"
+config_terminate="$(extract_config_secret "${out_terminate}")"
+assert_not_contains "terminateSidecarOnExit alone does not add inject label" \
+ 'sidecar.istio.io/inject' "${init_job_terminate}"
+assert_contains "initscript registers EXIT trap that calls quitquitquit" \
+ "trap 'rc=\$?; curl -fsS -m 5 -X POST" "${config_terminate}"
+assert_contains "initscript trap targets the configured quit endpoint" \
+ 'http://localhost:15020/quitquitquit' "${config_terminate}"
+assert_contains "initscript trap binds to the EXIT signal" \
+ "' EXIT" "${config_terminate}"
+
+echo "==> init.istio.quitEndpoint override"
+out_endpoint="$(render --set init.istio.terminateSidecarOnExit=true \
+ --set init.istio.quitEndpoint=http://127.0.0.1:15020/quitquitquit)"
+config_endpoint="$(extract_config_secret "${out_endpoint}")"
+assert_contains "trap honours custom quitEndpoint" \
+ 'http://127.0.0.1:15020/quitquitquit' "${config_endpoint}"
+
+echo "==> both options combined"
+out_both="$(render --set init.istio.disableSidecarInjection=true \
+ --set init.istio.terminateSidecarOnExit=true)"
+init_job_both="$(extract_init_job "${out_both}")"
+config_both="$(extract_config_secret "${out_both}")"
+assert_contains "combined: inject label present" \
+ 'sidecar.istio.io/inject: "false"' "${init_job_both}"
+assert_contains "combined: trap present" \
+ 'quitquitquit' "${config_both}"
+
+echo "==> existing init.podLabels are preserved alongside istio label"
+out_labels="$(render --set init.istio.disableSidecarInjection=true \
+ --set init.podLabels.team=data-platform)"
+init_job_labels="$(extract_init_job "${out_labels}")"
+assert_contains "user-supplied podLabel still rendered" \
+ 'team: data-platform' "${init_job_labels}"
+assert_contains "istio inject label rendered alongside" \
+ 'sidecar.istio.io/inject: "false"' "${init_job_labels}"
+
+echo "==> init.istio explicitly overridden to null"
+out_null_istio="$(render --set init.istio=null)"
+init_job_null_istio="$(extract_init_job "${out_null_istio}")"
+config_null_istio="$(extract_config_secret "${out_null_istio}")"
+assert_not_contains "null init.istio does not set inject label" \
+ 'sidecar.istio.io/inject' "${init_job_null_istio}"
+assert_not_contains "null init.istio does not register quitquitquit trap" \
+ 'quitquitquit' "${config_null_istio}"
+
+echo "==> EXIT trap propagates the script's exit code, not the notification's"
+# The trap's own curl call is best-effort (failures are logged with
+# `|| echo ... >&2`, not swallowed with `|| true`) and must not mask a
+# failed migration. Extract the two rendered lines and actually run them,
+# with curl pointed at a closed local port so the notification itself fails,
+# to make sure the wrapped script's real exit code still comes through.
+quit_endpoint_line="$(grep -F 'ISTIO_QUIT_ENDPOINT=' <<<"${config_terminate}")"
Review Comment:
**Suggestion:** With `set -e` enabled, this bare `grep` inside command substitution will terminate the entire test script immediately when the line is missing, skipping the final pass/fail summary and other checks. Make this extraction non-fatal and assert explicitly so failures are reported through the test counters. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Istio Helm tests abort without aggregated pass/fail summary.
- ⚠️ Later Istio-related assertions are skipped on grep failure.
- ⚠️ Debugging template regressions harder due to abrupt exit.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open helm/superset/tests/test-istio.sh and note `set -euo pipefail` at line 25, which
makes any simple-command failure (outside if/while/||/&& contexts) terminate the script.
2. Observe at lines 100-108 that `config_terminate` is populated from `render --set
init.istio.terminateSidecarOnExit=true`, and at 103-110 earlier tests assert the presence
of the EXIT trap in this rendered config.
3. At line 153, the script performs `quit_endpoint_line="$(grep -F 'ISTIO_QUIT_ENDPOINT='
...)"` in a bare command substitution; if the Helm template in
superset/templates/secret-superset-config.yaml is later changed so that the
`ISTIO_QUIT_ENDPOINT=` line is missing from `config_terminate`, this grep exits with
status 1.
4. Under `set -e`, the non-zero status from the command substitution causes the script
helm/superset/tests/test-istio.sh to terminate immediately at line 153, so the EXIT-code
propagation test (lines 155-171) and the final pass/fail summary (lines 173-177) are never
executed or printed.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a0ffcb91cda5445f8e9778c8ffe89ab5&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=a0ffcb91cda5445f8e9778c8ffe89ab5&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/tests/test-istio.sh
**Line:** 153:153
**Comment:**
*Logic Error: With `set -e` enabled, this bare `grep` inside command substitution will terminate the entire test script immediately when the line is missing, skipping the final pass/fail summary and other checks. Make this extraction non-fatal and assert explicitly so failures are reported through the test counters.
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%2F41396&comment_hash=cd043b454213ec38fac5c314617675af74bd95f521cae5d21f7c276962035cc5&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41396&comment_hash=cd043b454213ec38fac5c314617675af74bd95f521cae5d21f7c276962035cc5&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #41396:
URL: https://github.com/apache/superset/pull/41396#discussion_r3614137355
##########
helm/superset/templates/_helpers.tpl:
##########
@@ -685,6 +685,14 @@ TALISMAN_CONFIG = {
{{- define "superset.initScript" -}}
#!/bin/sh
set -eu
+{{- if dig "istio" "terminateSidecarOnExit" false .Values.init }}
+# Notify the Istio pilot-agent sidecar to exit when this script completes
+# (whether successfully or via `set -e`), so that the Job can reach the
+# Completed state instead of hanging on a still-running envoy-proxy.
+# See https://github.com/apache/superset/issues/25798
+ISTIO_QUIT_ENDPOINT={{ dig "istio" "quitEndpoint" "http://localhost:15020/quitquitquit" .Values.init | squote }}
Review Comment:
`squote` doesn't escape embedded single quotes, so a `quitEndpoint` value containing `'` still breaks out of the assignment — same class of bug as the double-quote command-substitution issue just fixed in this PR, via single quotes instead of double.
Verified:
```
helm template --set-string 'init.istio.quitEndpoint=http://x/quit'"'"'; echo INJECTED; echo '"'"''
→ ISTIO_QUIT_ENDPOINT='http://x/quit'; echo INJECTED; echo ''
```
Not exploitable across a trust boundary (`quitEndpoint` is operator-set Helm config), but it's a real gap in a fix whose commit message claims "no metacharacters in the value are interpreted." Escape embedded quotes before the outer `squote`:
```suggestion
ISTIO_QUIT_ENDPOINT={{ dig "istio" "quitEndpoint" "http://localhost:15020/quitquitquit" .Values.init | replace "'" "'\\''" | squote }}
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #41396:
URL: https://github.com/apache/superset/pull/41396#discussion_r3616389515
##########
helm/superset/templates/_helpers.tpl:
##########
@@ -685,6 +685,14 @@ TALISMAN_CONFIG = {
{{- define "superset.initScript" -}}
#!/bin/sh
set -eu
+{{- if dig "istio" "terminateSidecarOnExit" false .Values.init }}
+# Notify the Istio pilot-agent sidecar to exit when this script completes
+# (whether successfully or via `set -e`), so that the Job can reach the
+# Completed state instead of hanging on a still-running envoy-proxy.
+# See https://github.com/apache/superset/issues/25798
+ISTIO_QUIT_ENDPOINT={{ dig "istio" "quitEndpoint" "http://localhost:15020/quitquitquit" .Values.init | replace "'" "'\\''" | squote }}
+trap 'rc=$?; curl -fsS -m 5 -X POST "$ISTIO_QUIT_ENDPOINT" >/dev/null 2>&1 || true; exit $rc' EXIT
Review Comment:
Fair point that failing silently makes this hard to debug, so I added a stderr warning when the notify curl fails (`02f62ef7`). I don't want to fail the script on that though, the whole reason for `|| true` there is so the Job's exit code reflects the actual init outcome and not whether we managed to poke istio-proxy. There's actually a test in `test-istio.sh` now asserting the original exit code survives a failed notification.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #41396:
URL: https://github.com/apache/superset/pull/41396#issuecomment-5023315810
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | ce7bf680e11935615509d8b9ba2a43850b052ce5 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a5e2e887273f00008925384 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-41396--superset-docs-preview.netlify.app](https://deploy-preview-41396--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcrBDYAgDEDRXXq2iSki1Q0cAwUjiUKDXIxxd_tP__BekFouaTADdLBWn7dD34ciDaWiIUx3SwVTTg3DqihEOcuzBGWjt5EisyNn9l7jiazhAb4f) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-41396--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41396:
URL: https://github.com/apache/superset/pull/41396#discussion_r3565210432
##########
helm/superset/templates/_helpers.tpl:
##########
@@ -685,6 +685,13 @@ TALISMAN_CONFIG = {
{{- define "superset.initScript" -}}
#!/bin/sh
set -eu
+{{- if .Values.init.istio.terminateSidecarOnExit }}
Review Comment:
**Suggestion:** The conditional directly dereferences a nested values path, which can break template rendering when `init.istio` is omitted or set to null in override values. In that case Helm can error before install/upgrade instead of simply treating the option as disabled. Read this flag via `dig` (with a false default) or guard the parent map before accessing `terminateSidecarOnExit`. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ Helm install or upgrade fails on misconfigured init.istio.
⚠️ Superset init-db job script never rendered when error occurs.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a Helm override values file (for example myvalues.yaml) that nulls the init
Istio block, such as:
- in myvalues.yaml set either `init: null` or `init:` with `istio: null`, overriding
the defaults from `helm/superset/values.yaml:8-24` and
`helm/superset/values.yaml:1019-1022` where init and init.istio are defined.
2. Run `helm template` or `helm install` using the Superset chart at `helm/superset` with
`-f myvalues.yaml`, so Helm renders all templates including
`helm/superset/templates/secret-superset-config.yaml:10-15`.
3. During rendering of `secret-superset-config.yaml`, Helm evaluates `superset_init.sh: |`
which includes `{{- include "superset.initScript" . | nindent 4 }}`; that in turn expands
the helper `superset.initScript` defined at
`helm/superset/templates/_helpers.tpl:685-694`.
4. The helper executes the conditional `{{- if .Values.init.istio.terminateSidecarOnExit
}}` at `helm/superset/templates/_helpers.tpl:688`, and because `.Values.init.istio` is
null or not a map under the override, Helm raises a template error about accessing
`terminateSidecarOnExit` on a nil value, causing the install/upgrade to fail instead of
simply treating the flag as false.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d14e1e27378d4fbf8bed1efc2f2a54bc&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=d14e1e27378d4fbf8bed1efc2f2a54bc&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:** 688:688
**Comment:**
*Possible Bug: The conditional directly dereferences a nested values path, which can break template rendering when `init.istio` is omitted or set to null in override values. In that case Helm can error before install/upgrade instead of simply treating the option as disabled. Read this flag via `dig` (with a false default) or guard the parent map before accessing `terminateSidecarOnExit`.
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%2F41396&comment_hash=ccac27365481b7a82d545c33510811c5dc6a06131284811c872442b5cf0d0078&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41396&comment_hash=ccac27365481b7a82d545c33510811c5dc6a06131284811c872442b5cf0d0078&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41396:
URL: https://github.com/apache/superset/pull/41396#issuecomment-5009253316
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #41396:
URL: https://github.com/apache/superset/pull/41396#issuecomment-5048302462
Just need a codeowner stamp from any of these fine folks to close out the Issue.
@dpgaspar
@villebro
@nytai
@michael-s-molina
@mistercrunch
@Antonio-RiveroMartnez
@hainenber
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41396:
URL: https://github.com/apache/superset/pull/41396#issuecomment-4949112288
<!-- Bito Reply -->
The flagged issue is correct. Directly accessing `.Values.init.istio.terminateSidecarOnExit` will cause a template rendering error if `init.istio` is null or missing in the override values. To resolve this, you should use the `dig` function to safely access the nested value with a default of `false`.
Here is the corrected code for `helm/superset/templates/_helpers.tpl`:
```yaml
{{- define "superset.initScript" -}}
#!/bin/sh
set -eu
{{- if (dig "istio" "terminateSidecarOnExit" false .Values.init) }}
# Notify the Istio pilot-agent sidecar to exit when this script completes
# (whether successfully or via `set -e`), so that the Job can reach the
# Completed state instead of hanging on a still-running envoy-proxy.
# See https://github.com/apache/superset/issues/25798
trap 'rc=$?; curl -fsS -m 5 -X POST {{ (dig "istio" "quitEndpoint" "http://localhost:15020/quitquitquit" .Values.init) | quote }} >/dev/null 2>&1 || true; exit $rc' EXIT
{{- end }}
echo "Upgrading DB schema..."
```
I have implemented the fix using `dig` for both `terminateSidecarOnExit` and `quitEndpoint` to ensure robustness. There are no other comments on this PR to address.
**helm/superset/templates/_helpers.tpl**
```
{{- define "superset.initScript" -}}
#!/bin/sh
set -eu
{{- if (dig "istio" "terminateSidecarOnExit" false .Values.init) }}
# Notify the Istio pilot-agent sidecar to exit when this script completes
# (whether successfully or via `set -e`), so that the Job can reach the
# Completed state instead of hanging on a still-running envoy-proxy.
# See https://github.com/apache/superset/issues/25798
trap 'rc=$?; curl -fsS -m 5 -X POST {{ (dig "istio" "quitEndpoint" "http://localhost:15020/quitquitquit" .Values.init) | quote }} >/dev/null 2>&1 || true; exit $rc' EXIT
{{- end }}
echo "Upgrading DB schema..."
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #41396:
URL: https://github.com/apache/superset/pull/41396#discussion_r3614973023
##########
helm/superset/templates/_helpers.tpl:
##########
@@ -685,6 +685,14 @@ TALISMAN_CONFIG = {
{{- define "superset.initScript" -}}
#!/bin/sh
set -eu
+{{- if dig "istio" "terminateSidecarOnExit" false .Values.init }}
+# Notify the Istio pilot-agent sidecar to exit when this script completes
+# (whether successfully or via `set -e`), so that the Job can reach the
+# Completed state instead of hanging on a still-running envoy-proxy.
+# See https://github.com/apache/superset/issues/25798
+ISTIO_QUIT_ENDPOINT={{ dig "istio" "quitEndpoint" "http://localhost:15020/quitquitquit" .Values.init | squote }}
Review Comment:
Good catch, pushed exactly that: escaping embedded single quotes with `replace "'" "'\\''" ` before `squote`. Verified with the same repro — the injected `; echo INJECTED` now renders as an inert single-quoted string segment instead of breaking out.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #41396:
URL: https://github.com/apache/superset/pull/41396#issuecomment-5023317289
@aminghadersohi added both cases to `test-istio.sh`: rendering with `init.istio: null`, and actually running the rendered EXIT trap against a dead endpoint to confirm the wrapped script's exit code survives the failed curl call.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41396:
URL: https://github.com/apache/superset/pull/41396#discussion_r3616393167
##########
helm/superset/templates/_helpers.tpl:
##########
@@ -685,6 +685,14 @@ TALISMAN_CONFIG = {
{{- define "superset.initScript" -}}
#!/bin/sh
set -eu
+{{- if dig "istio" "terminateSidecarOnExit" false .Values.init }}
+# Notify the Istio pilot-agent sidecar to exit when this script completes
+# (whether successfully or via `set -e`), so that the Job can reach the
+# Completed state instead of hanging on a still-running envoy-proxy.
+# See https://github.com/apache/superset/issues/25798
+ISTIO_QUIT_ENDPOINT={{ dig "istio" "quitEndpoint" "http://localhost:15020/quitquitquit" .Values.init | replace "'" "'\\''" | squote }}
+trap 'rc=$?; curl -fsS -m 5 -X POST "$ISTIO_QUIT_ENDPOINT" >/dev/null 2>&1 || true; exit $rc' EXIT
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> When the sidecar-shutdown notification is optional, do not fail the init script on notification errors; preserve the original exit code and emit a warning to stderr instead.
**Applied to:**
- `helm/superset/templates/**`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41396:
URL: https://github.com/apache/superset/pull/41396#issuecomment-5010902671
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41396:
URL: https://github.com/apache/superset/pull/41396#discussion_r3616417619
##########
helm/superset/tests/test-istio.sh:
##########
@@ -0,0 +1,176 @@
+#!/usr/bin/env bash
+#
+# 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.
+#
+# Renders the chart with `helm template` for several `init.istio.*` value
+# combinations and asserts that the relevant manifests contain (or omit)
+# the expected fields. Intended to be run from the chart directory or via
+# `bash helm/superset/tests/test-istio.sh` from the repo root.
+#
+# Covers the fix for:
+# https://github.com/apache/superset/issues/25798
+set -euo pipefail
+
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+CHART_DIR="$(cd "${SCRIPT_DIR}/.." && pwd)"
+
+pass=0
+fail=0
+
+assert_contains() {
+ local label="$1"
+ local needle="$2"
+ local haystack="$3"
+ if grep -qF -- "${needle}" <<<"${haystack}"; then
+ echo " PASS: ${label}"
+ pass=$((pass + 1))
+ else
+ echo " FAIL: ${label}"
+ echo " expected to contain: ${needle}"
+ fail=$((fail + 1))
+ fi
+}
+
+assert_not_contains() {
+ local label="$1"
+ local needle="$2"
+ local haystack="$3"
+ if ! grep -qF -- "${needle}" <<<"${haystack}"; then
+ echo " PASS: ${label}"
+ pass=$((pass + 1))
+ else
+ echo " FAIL: ${label}"
+ echo " expected NOT to contain: ${needle}"
+ fail=$((fail + 1))
+ fi
+}
+
+render() {
+ helm template release "${CHART_DIR}" "$@"
+}
+
+extract_init_job() {
+ awk '
+ /^# Source: superset\/templates\/init-job\.yaml/ { capture = 1 }
+ capture && /^---$/ { capture = 0 }
+ capture { print }
+ ' <<<"$1"
+}
+
+extract_config_secret() {
+ awk '
+ /^# Source: superset\/templates\/secret-superset-config\.yaml/ { capture = 1 }
+ capture && /^---$/ { capture = 0 }
+ capture { print }
+ ' <<<"$1"
+}
+
+echo "==> defaults: no istio mitigations applied"
+out_default="$(render)"
+init_job_default="$(extract_init_job "${out_default}")"
+config_default="$(extract_config_secret "${out_default}")"
+assert_not_contains "default does not set sidecar.istio.io/inject label" \
+ 'sidecar.istio.io/inject' "${init_job_default}"
+assert_not_contains "default initscript does not register quitquitquit trap" \
+ 'quitquitquit' "${config_default}"
+
+echo "==> init.istio.disableSidecarInjection=true"
+out_disable="$(render --set init.istio.disableSidecarInjection=true)"
+init_job_disable="$(extract_init_job "${out_disable}")"
+config_disable="$(extract_config_secret "${out_disable}")"
+assert_contains "init job sets sidecar.istio.io/inject: \"false\"" \
+ 'sidecar.istio.io/inject: "false"' "${init_job_disable}"
+assert_not_contains "disableSidecarInjection alone does not add quitquitquit trap" \
+ 'quitquitquit' "${config_disable}"
+
+echo "==> init.istio.terminateSidecarOnExit=true"
+out_terminate="$(render --set init.istio.terminateSidecarOnExit=true)"
+init_job_terminate="$(extract_init_job "${out_terminate}")"
+config_terminate="$(extract_config_secret "${out_terminate}")"
+assert_not_contains "terminateSidecarOnExit alone does not add inject label" \
+ 'sidecar.istio.io/inject' "${init_job_terminate}"
+assert_contains "initscript registers EXIT trap that calls quitquitquit" \
+ "trap 'rc=\$?; curl -fsS -m 5 -X POST" "${config_terminate}"
+assert_contains "initscript trap targets the configured quit endpoint" \
+ 'http://localhost:15020/quitquitquit' "${config_terminate}"
+assert_contains "initscript trap binds to the EXIT signal" \
+ "' EXIT" "${config_terminate}"
+
+echo "==> init.istio.quitEndpoint override"
+out_endpoint="$(render --set init.istio.terminateSidecarOnExit=true \
+ --set init.istio.quitEndpoint=http://127.0.0.1:15020/quitquitquit)"
+config_endpoint="$(extract_config_secret "${out_endpoint}")"
+assert_contains "trap honours custom quitEndpoint" \
+ 'http://127.0.0.1:15020/quitquitquit' "${config_endpoint}"
+
+echo "==> both options combined"
+out_both="$(render --set init.istio.disableSidecarInjection=true \
+ --set init.istio.terminateSidecarOnExit=true)"
+init_job_both="$(extract_init_job "${out_both}")"
+config_both="$(extract_config_secret "${out_both}")"
+assert_contains "combined: inject label present" \
+ 'sidecar.istio.io/inject: "false"' "${init_job_both}"
+assert_contains "combined: trap present" \
+ 'quitquitquit' "${config_both}"
+
+echo "==> existing init.podLabels are preserved alongside istio label"
+out_labels="$(render --set init.istio.disableSidecarInjection=true \
+ --set init.podLabels.team=data-platform)"
+init_job_labels="$(extract_init_job "${out_labels}")"
+assert_contains "user-supplied podLabel still rendered" \
+ 'team: data-platform' "${init_job_labels}"
+assert_contains "istio inject label rendered alongside" \
+ 'sidecar.istio.io/inject: "false"' "${init_job_labels}"
+
+echo "==> init.istio explicitly overridden to null"
+out_null_istio="$(render --set init.istio=null)"
+init_job_null_istio="$(extract_init_job "${out_null_istio}")"
+config_null_istio="$(extract_config_secret "${out_null_istio}")"
+assert_not_contains "null init.istio does not set inject label" \
+ 'sidecar.istio.io/inject' "${init_job_null_istio}"
+assert_not_contains "null init.istio does not register quitquitquit trap" \
+ 'quitquitquit' "${config_null_istio}"
+
+echo "==> EXIT trap propagates the script's exit code, not the notification's"
+# The trap's own curl call is best-effort (`|| true`) and must not mask a
Review Comment:
**Suggestion:** The comment says the trap uses `|| true`, but the rendered script now uses `|| echo ... >&2`; this contradiction is misleading for future maintainers validating exit-code behavior. Update the comment to match the actual trap behavior so the test intent stays accurate. [comment mismatch]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Test documentation misleads maintainers about trap exit behavior.
- ⚠️ Future changes risk aligning code with incorrect comment.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the test script helm/superset/tests/test-istio.sh and navigate to the comment at
lines 147–151, specifically line 148 which states: "The trap's own curl call is
best-effort (`|| true`) and must not mask a".
2. From the same file, observe the logic starting at lines 99–108 where the script renders
the chart with init.istio.terminateSidecarOnExit=true and captures the rendered secret
into config_terminate using extract_config_secret.
3. Still in helm/superset/tests/test-istio.sh, inspect the code at lines 152–158 where
quit_endpoint_line and trap_line are obtained from config_terminate and then evaluated in
a subshell to exercise the rendered EXIT trap behavior.
4. Render the chart manually (or run this test) and inspect the rendered init script in
the secret-superset-config.yaml manifest; the EXIT trap line obtained as trap_line uses a
best-effort curl followed by a logging fallback (for example, || echo ... >> stderr)
rather than the literal || true mentioned in the comment at line 148, showing the comment
is out of sync with the actual trap behavior and therefore misleading documentation rather
than a runtime bug.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=049b8854ee9245d1802ea95969d53156&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=049b8854ee9245d1802ea95969d53156&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/tests/test-istio.sh
**Line:** 148:148
**Comment:**
*Comment Mismatch: The comment says the trap uses `|| true`, but the rendered script now uses `|| echo ... >&2`; this contradiction is misleading for future maintainers validating exit-code behavior. Update the comment to match the actual trap behavior so the test intent stays accurate.
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%2F41396&comment_hash=21aa43e202a37b523e4b6183846e78b727a7ead58ca22cd23aa90cb307182167&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41396&comment_hash=21aa43e202a37b523e4b6183846e78b727a7ead58ca22cd23aa90cb307182167&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41396:
URL: https://github.com/apache/superset/pull/41396#discussion_r3615013457
##########
helm/superset/templates/_helpers.tpl:
##########
@@ -685,6 +685,14 @@ TALISMAN_CONFIG = {
{{- define "superset.initScript" -}}
#!/bin/sh
set -eu
+{{- if dig "istio" "terminateSidecarOnExit" false .Values.init }}
+# Notify the Istio pilot-agent sidecar to exit when this script completes
+# (whether successfully or via `set -e`), so that the Job can reach the
+# Completed state instead of hanging on a still-running envoy-proxy.
+# See https://github.com/apache/superset/issues/25798
+ISTIO_QUIT_ENDPOINT={{ dig "istio" "quitEndpoint" "http://localhost:15020/quitquitquit" .Values.init | replace "'" "'\\''" | squote }}
+trap 'rc=$?; curl -fsS -m 5 -X POST "$ISTIO_QUIT_ENDPOINT" >/dev/null 2>&1 || true; exit $rc' EXIT
Review Comment:
**Suggestion:** The sidecar-termination call is unconditionally masked with `|| true`, so when the POST fails (for example endpoint not reachable or `curl` not present) the script still exits normally and the Job can continue hanging with the sidecar running. Make this path observable/failable when the feature is enabled (or at least emit an explicit error and fail the init script) so the mitigation does not silently no-op. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ init-db Job may still hang with terminateSidecarOnExit enabled.
- ⚠️ Failure to contact Istio admin endpoint is fully silent.
- ⚠️ Operators cannot easily distinguish config vs. feature failure.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `helm/superset/values.yaml`, enable the feature by setting
`init.istio.terminateSidecarOnExit: true` and configure an incorrect or unreachable
endpoint in `init.istio.quitEndpoint` (e.g., wrong port or path), so the POST will fail
when called from the init container.
2. Deploy the chart so that the `init-db` Job uses the `superset.initScript` helper
defined at `helm/superset/templates/_helpers.tpl:685-703`, which renders the `trap` line
at `helm/superset/templates/_helpers.tpl:694` into the init script executed in the init-db
pod.
3. Ensure Istio sidecar injection is enabled on the namespace so the `init-db` pod gets an
`istio-proxy` sidecar; when the Job runs, the main container runs the rendered init
script, finishes all `superset db upgrade` / `superset init` steps, and exits with status
0.
4. On script exit, the EXIT trap at `helm/superset/templates/_helpers.tpl:694` executes
`curl -fsS -m 5 -X POST "$ISTIO_QUIT_ENDPOINT" >/dev/null 2>&1 || true`; because the
endpoint is unreachable, `curl` returns non‑zero but the `|| true` masks the failure, so
the script still exits with the original status 0, the istio-proxy sidecar keeps running,
and the Kubernetes Job remains stuck in `Running` without any log message indicating the
sidecar shutdown POST failed.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=263afc6e0ef2470295c5b3a8a91516d2&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=263afc6e0ef2470295c5b3a8a91516d2&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:** 694:694
**Comment:**
*Logic Error: The sidecar-termination call is unconditionally masked with `|| true`, so when the POST fails (for example endpoint not reachable or `curl` not present) the script still exits normally and the Job can continue hanging with the sidecar running. Make this path observable/failable when the feature is enabled (or at least emit an explicit error and fail the init script) so the mitigation does not silently no-op.
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%2F41396&comment_hash=001f30c4e212fd0b75a320cf8aec9a4a72014ed2ecda57e60a1828899313a0c9&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41396&comment_hash=001f30c4e212fd0b75a320cf8aec9a4a72014ed2ecda57e60a1828899313a0c9&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41396:
URL: https://github.com/apache/superset/pull/41396#issuecomment-4828997075
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41396:
URL: https://github.com/apache/superset/pull/41396#issuecomment-5083561107
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #41396:
URL: https://github.com/apache/superset/pull/41396#discussion_r3616936736
##########
helm/superset/tests/test-istio.sh:
##########
@@ -0,0 +1,177 @@
+#!/usr/bin/env bash
+#
+# 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.
+#
+# Renders the chart with `helm template` for several `init.istio.*` value
+# combinations and asserts that the relevant manifests contain (or omit)
+# the expected fields. Intended to be run from the chart directory or via
+# `bash helm/superset/tests/test-istio.sh` from the repo root.
+#
+# Covers the fix for:
+# https://github.com/apache/superset/issues/25798
+set -euo pipefail
+
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+CHART_DIR="$(cd "${SCRIPT_DIR}/.." && pwd)"
+
+pass=0
+fail=0
+
+assert_contains() {
+ local label="$1"
+ local needle="$2"
+ local haystack="$3"
+ if grep -qF -- "${needle}" <<<"${haystack}"; then
+ echo " PASS: ${label}"
+ pass=$((pass + 1))
+ else
+ echo " FAIL: ${label}"
+ echo " expected to contain: ${needle}"
+ fail=$((fail + 1))
+ fi
+}
+
+assert_not_contains() {
+ local label="$1"
+ local needle="$2"
+ local haystack="$3"
+ if ! grep -qF -- "${needle}" <<<"${haystack}"; then
+ echo " PASS: ${label}"
+ pass=$((pass + 1))
+ else
+ echo " FAIL: ${label}"
+ echo " expected NOT to contain: ${needle}"
+ fail=$((fail + 1))
+ fi
+}
+
+render() {
+ helm template release "${CHART_DIR}" "$@"
+}
+
+extract_init_job() {
+ awk '
+ /^# Source: superset\/templates\/init-job\.yaml/ { capture = 1 }
+ capture && /^---$/ { capture = 0 }
+ capture { print }
+ ' <<<"$1"
+}
+
+extract_config_secret() {
+ awk '
+ /^# Source: superset\/templates\/secret-superset-config\.yaml/ { capture = 1 }
+ capture && /^---$/ { capture = 0 }
+ capture { print }
+ ' <<<"$1"
+}
+
+echo "==> defaults: no istio mitigations applied"
+out_default="$(render)"
+init_job_default="$(extract_init_job "${out_default}")"
+config_default="$(extract_config_secret "${out_default}")"
+assert_not_contains "default does not set sidecar.istio.io/inject label" \
+ 'sidecar.istio.io/inject' "${init_job_default}"
+assert_not_contains "default initscript does not register quitquitquit trap" \
+ 'quitquitquit' "${config_default}"
+
+echo "==> init.istio.disableSidecarInjection=true"
+out_disable="$(render --set init.istio.disableSidecarInjection=true)"
+init_job_disable="$(extract_init_job "${out_disable}")"
+config_disable="$(extract_config_secret "${out_disable}")"
+assert_contains "init job sets sidecar.istio.io/inject: \"false\"" \
+ 'sidecar.istio.io/inject: "false"' "${init_job_disable}"
+assert_not_contains "disableSidecarInjection alone does not add quitquitquit trap" \
+ 'quitquitquit' "${config_disable}"
+
+echo "==> init.istio.terminateSidecarOnExit=true"
+out_terminate="$(render --set init.istio.terminateSidecarOnExit=true)"
+init_job_terminate="$(extract_init_job "${out_terminate}")"
+config_terminate="$(extract_config_secret "${out_terminate}")"
+assert_not_contains "terminateSidecarOnExit alone does not add inject label" \
+ 'sidecar.istio.io/inject' "${init_job_terminate}"
+assert_contains "initscript registers EXIT trap that calls quitquitquit" \
+ "trap 'rc=\$?; curl -fsS -m 5 -X POST" "${config_terminate}"
+assert_contains "initscript trap targets the configured quit endpoint" \
+ 'http://localhost:15020/quitquitquit' "${config_terminate}"
+assert_contains "initscript trap binds to the EXIT signal" \
+ "' EXIT" "${config_terminate}"
+
+echo "==> init.istio.quitEndpoint override"
+out_endpoint="$(render --set init.istio.terminateSidecarOnExit=true \
+ --set init.istio.quitEndpoint=http://127.0.0.1:15020/quitquitquit)"
+config_endpoint="$(extract_config_secret "${out_endpoint}")"
+assert_contains "trap honours custom quitEndpoint" \
+ 'http://127.0.0.1:15020/quitquitquit' "${config_endpoint}"
+
+echo "==> both options combined"
+out_both="$(render --set init.istio.disableSidecarInjection=true \
+ --set init.istio.terminateSidecarOnExit=true)"
+init_job_both="$(extract_init_job "${out_both}")"
+config_both="$(extract_config_secret "${out_both}")"
+assert_contains "combined: inject label present" \
+ 'sidecar.istio.io/inject: "false"' "${init_job_both}"
+assert_contains "combined: trap present" \
+ 'quitquitquit' "${config_both}"
+
+echo "==> existing init.podLabels are preserved alongside istio label"
+out_labels="$(render --set init.istio.disableSidecarInjection=true \
+ --set init.podLabels.team=data-platform)"
+init_job_labels="$(extract_init_job "${out_labels}")"
+assert_contains "user-supplied podLabel still rendered" \
+ 'team: data-platform' "${init_job_labels}"
+assert_contains "istio inject label rendered alongside" \
+ 'sidecar.istio.io/inject: "false"' "${init_job_labels}"
+
+echo "==> init.istio explicitly overridden to null"
+out_null_istio="$(render --set init.istio=null)"
+init_job_null_istio="$(extract_init_job "${out_null_istio}")"
+config_null_istio="$(extract_config_secret "${out_null_istio}")"
+assert_not_contains "null init.istio does not set inject label" \
+ 'sidecar.istio.io/inject' "${init_job_null_istio}"
+assert_not_contains "null init.istio does not register quitquitquit trap" \
+ 'quitquitquit' "${config_null_istio}"
+
+echo "==> EXIT trap propagates the script's exit code, not the notification's"
+# The trap's own curl call is best-effort (failures are logged with
+# `|| echo ... >&2`, not swallowed with `|| true`) and must not mask a
+# failed migration. Extract the two rendered lines and actually run them,
+# with curl pointed at a closed local port so the notification itself fails,
+# to make sure the wrapped script's real exit code still comes through.
+quit_endpoint_line="$(grep -F 'ISTIO_QUIT_ENDPOINT=' <<<"${config_terminate}")"
Review Comment:
Good catch, thanks. Under `set -euo pipefail` a missing match here would've aborted the script before the final pass/fail summary printed. Made the extraction non-fatal and falling through to an explicit FAIL if the lines aren't found (`0321483`).
##########
helm/superset/tests/test-istio.sh:
##########
@@ -0,0 +1,177 @@
+#!/usr/bin/env bash
+#
+# 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.
+#
+# Renders the chart with `helm template` for several `init.istio.*` value
+# combinations and asserts that the relevant manifests contain (or omit)
+# the expected fields. Intended to be run from the chart directory or via
+# `bash helm/superset/tests/test-istio.sh` from the repo root.
+#
+# Covers the fix for:
+# https://github.com/apache/superset/issues/25798
+set -euo pipefail
+
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+CHART_DIR="$(cd "${SCRIPT_DIR}/.." && pwd)"
+
+pass=0
+fail=0
+
+assert_contains() {
+ local label="$1"
+ local needle="$2"
+ local haystack="$3"
+ if grep -qF -- "${needle}" <<<"${haystack}"; then
+ echo " PASS: ${label}"
+ pass=$((pass + 1))
+ else
+ echo " FAIL: ${label}"
+ echo " expected to contain: ${needle}"
+ fail=$((fail + 1))
+ fi
+}
+
+assert_not_contains() {
+ local label="$1"
+ local needle="$2"
+ local haystack="$3"
+ if ! grep -qF -- "${needle}" <<<"${haystack}"; then
+ echo " PASS: ${label}"
+ pass=$((pass + 1))
+ else
+ echo " FAIL: ${label}"
+ echo " expected NOT to contain: ${needle}"
+ fail=$((fail + 1))
+ fi
+}
+
+render() {
+ helm template release "${CHART_DIR}" "$@"
+}
+
+extract_init_job() {
+ awk '
+ /^# Source: superset\/templates\/init-job\.yaml/ { capture = 1 }
+ capture && /^---$/ { capture = 0 }
+ capture { print }
+ ' <<<"$1"
+}
+
+extract_config_secret() {
+ awk '
+ /^# Source: superset\/templates\/secret-superset-config\.yaml/ { capture = 1 }
+ capture && /^---$/ { capture = 0 }
+ capture { print }
+ ' <<<"$1"
+}
+
+echo "==> defaults: no istio mitigations applied"
+out_default="$(render)"
+init_job_default="$(extract_init_job "${out_default}")"
+config_default="$(extract_config_secret "${out_default}")"
+assert_not_contains "default does not set sidecar.istio.io/inject label" \
+ 'sidecar.istio.io/inject' "${init_job_default}"
+assert_not_contains "default initscript does not register quitquitquit trap" \
+ 'quitquitquit' "${config_default}"
+
+echo "==> init.istio.disableSidecarInjection=true"
+out_disable="$(render --set init.istio.disableSidecarInjection=true)"
+init_job_disable="$(extract_init_job "${out_disable}")"
+config_disable="$(extract_config_secret "${out_disable}")"
+assert_contains "init job sets sidecar.istio.io/inject: \"false\"" \
+ 'sidecar.istio.io/inject: "false"' "${init_job_disable}"
+assert_not_contains "disableSidecarInjection alone does not add quitquitquit trap" \
+ 'quitquitquit' "${config_disable}"
+
+echo "==> init.istio.terminateSidecarOnExit=true"
+out_terminate="$(render --set init.istio.terminateSidecarOnExit=true)"
+init_job_terminate="$(extract_init_job "${out_terminate}")"
+config_terminate="$(extract_config_secret "${out_terminate}")"
+assert_not_contains "terminateSidecarOnExit alone does not add inject label" \
+ 'sidecar.istio.io/inject' "${init_job_terminate}"
+assert_contains "initscript registers EXIT trap that calls quitquitquit" \
+ "trap 'rc=\$?; curl -fsS -m 5 -X POST" "${config_terminate}"
+assert_contains "initscript trap targets the configured quit endpoint" \
+ 'http://localhost:15020/quitquitquit' "${config_terminate}"
+assert_contains "initscript trap binds to the EXIT signal" \
+ "' EXIT" "${config_terminate}"
+
+echo "==> init.istio.quitEndpoint override"
+out_endpoint="$(render --set init.istio.terminateSidecarOnExit=true \
+ --set init.istio.quitEndpoint=http://127.0.0.1:15020/quitquitquit)"
+config_endpoint="$(extract_config_secret "${out_endpoint}")"
+assert_contains "trap honours custom quitEndpoint" \
+ 'http://127.0.0.1:15020/quitquitquit' "${config_endpoint}"
+
+echo "==> both options combined"
+out_both="$(render --set init.istio.disableSidecarInjection=true \
+ --set init.istio.terminateSidecarOnExit=true)"
+init_job_both="$(extract_init_job "${out_both}")"
+config_both="$(extract_config_secret "${out_both}")"
+assert_contains "combined: inject label present" \
+ 'sidecar.istio.io/inject: "false"' "${init_job_both}"
+assert_contains "combined: trap present" \
+ 'quitquitquit' "${config_both}"
+
+echo "==> existing init.podLabels are preserved alongside istio label"
+out_labels="$(render --set init.istio.disableSidecarInjection=true \
+ --set init.podLabels.team=data-platform)"
+init_job_labels="$(extract_init_job "${out_labels}")"
+assert_contains "user-supplied podLabel still rendered" \
+ 'team: data-platform' "${init_job_labels}"
+assert_contains "istio inject label rendered alongside" \
+ 'sidecar.istio.io/inject: "false"' "${init_job_labels}"
+
+echo "==> init.istio explicitly overridden to null"
+out_null_istio="$(render --set init.istio=null)"
+init_job_null_istio="$(extract_init_job "${out_null_istio}")"
+config_null_istio="$(extract_config_secret "${out_null_istio}")"
+assert_not_contains "null init.istio does not set inject label" \
+ 'sidecar.istio.io/inject' "${init_job_null_istio}"
+assert_not_contains "null init.istio does not register quitquitquit trap" \
+ 'quitquitquit' "${config_null_istio}"
+
+echo "==> EXIT trap propagates the script's exit code, not the notification's"
+# The trap's own curl call is best-effort (failures are logged with
+# `|| echo ... >&2`, not swallowed with `|| true`) and must not mask a
+# failed migration. Extract the two rendered lines and actually run them,
+# with curl pointed at a closed local port so the notification itself fails,
+# to make sure the wrapped script's real exit code still comes through.
+quit_endpoint_line="$(grep -F 'ISTIO_QUIT_ENDPOINT=' <<<"${config_terminate}")"
+trap_line="$(grep -F "trap 'rc=\$?; curl" <<<"${config_terminate}")"
Review Comment:
Same fix as the sibling comment on line 153, both greps are now non-fatal and fall through to an explicit FAIL rather than tripping `set -e` (`0321483`).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #41396:
URL: https://github.com/apache/superset/pull/41396#issuecomment-5023349355
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 9536c6f63211ae66b77282720766a3e0ede518a5 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a5e2f0074ebd4000867bbc1 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-41396--superset-docs-preview.netlify.app](https://deploy-preview-41396--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcrLCYAwDADQXXI2EOsXN3CMxlQMaBtqLyLubm_v8F6wnC4rsAA0wNnH7aj2kqygZewc6l00oUYtKFyTBDvTs0ptox-C24mmPrD0RDSPE_PWwvcD) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-41396--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41396:
URL: https://github.com/apache/superset/pull/41396#discussion_r3652228247
##########
helm/superset/tests/test-istio.sh:
##########
@@ -0,0 +1,183 @@
+#!/usr/bin/env bash
+#
+# 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.
+#
+# Renders the chart with `helm template` for several `init.istio.*` value
+# combinations and asserts that the relevant manifests contain (or omit)
+# the expected fields. Intended to be run from the chart directory or via
+# `bash helm/superset/tests/test-istio.sh` from the repo root.
+#
+# Covers the fix for:
+# https://github.com/apache/superset/issues/25798
+set -euo pipefail
+
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+CHART_DIR="$(cd "${SCRIPT_DIR}/.." && pwd)"
+
+pass=0
+fail=0
+
+assert_contains() {
+ local label="$1"
+ local needle="$2"
+ local haystack="$3"
+ if grep -qF -- "${needle}" <<<"${haystack}"; then
+ echo " PASS: ${label}"
+ pass=$((pass + 1))
+ else
+ echo " FAIL: ${label}"
+ echo " expected to contain: ${needle}"
+ fail=$((fail + 1))
+ fi
+}
+
+assert_not_contains() {
+ local label="$1"
+ local needle="$2"
+ local haystack="$3"
+ if ! grep -qF -- "${needle}" <<<"${haystack}"; then
+ echo " PASS: ${label}"
+ pass=$((pass + 1))
+ else
+ echo " FAIL: ${label}"
+ echo " expected NOT to contain: ${needle}"
+ fail=$((fail + 1))
+ fi
+}
+
+render() {
+ helm template release "${CHART_DIR}" "$@"
+}
+
+extract_init_job() {
+ awk '
+ /^# Source: superset\/templates\/init-job\.yaml/ { capture = 1 }
+ capture && /^---$/ { capture = 0 }
+ capture { print }
+ ' <<<"$1"
+}
+
+extract_config_secret() {
+ awk '
+ /^# Source: superset\/templates\/secret-superset-config\.yaml/ { capture = 1 }
+ capture && /^---$/ { capture = 0 }
+ capture { print }
+ ' <<<"$1"
+}
+
+echo "==> defaults: no istio mitigations applied"
+out_default="$(render)"
+init_job_default="$(extract_init_job "${out_default}")"
+config_default="$(extract_config_secret "${out_default}")"
+assert_not_contains "default does not set sidecar.istio.io/inject label" \
+ 'sidecar.istio.io/inject' "${init_job_default}"
+assert_not_contains "default initscript does not register quitquitquit trap" \
+ 'quitquitquit' "${config_default}"
+
+echo "==> init.istio.disableSidecarInjection=true"
+out_disable="$(render --set init.istio.disableSidecarInjection=true)"
+init_job_disable="$(extract_init_job "${out_disable}")"
+config_disable="$(extract_config_secret "${out_disable}")"
+assert_contains "init job sets sidecar.istio.io/inject: \"false\"" \
+ 'sidecar.istio.io/inject: "false"' "${init_job_disable}"
Review Comment:
**Suggestion:** The test requires `init.istio.disableSidecarInjection=true` to render the `sidecar.istio.io/inject: "false"` label, but the production changes shown only add the termination trap in `superset.initScript`; no updated init Job template consumes `disableSidecarInjection`. This case will fail and the advertised opt-out mitigation will not work until the init Job pod template renders the label. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Istio-injected init-db Jobs remain unable to opt out.
- ❌ The init-db Job can continue hanging after Superset initialization.
- ⚠️ The new Helm test fails when the option is enabled.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run `bash helm/superset/tests/test-istio.sh`, which invokes `render()` at
`helm/superset/tests/test-istio.sh:61-63` and executes `helm template` against the chart.
2. The test renders the chart with `init.istio.disableSidecarInjection=true` at
`helm/superset/tests/test-istio.sh:90-92`.
3. `extract_init_job()` at `helm/superset/tests/test-istio.sh:65-71` extracts the manifest
generated from `helm/superset/templates/init-job.yaml`.
4. The assertion at `helm/superset/tests/test-istio.sh:94-95` expects
`sidecar.istio.io/inject: "false"`. If `helm/superset/templates/init-job.yaml` does not
consume this value, the label is absent, the assertion fails, and the opt-out mitigation
is not applied to the init-db Job.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4c084e124d974879b7c70b648424a690&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=4c084e124d974879b7c70b648424a690&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/tests/test-istio.sh
**Line:** 90:95
**Comment:**
*Possible Bug: The test requires `init.istio.disableSidecarInjection=true` to render the `sidecar.istio.io/inject: "false"` label, but the production changes shown only add the termination trap in `superset.initScript`; no updated init Job template consumes `disableSidecarInjection`. This case will fail and the advertised opt-out mitigation will not work until the init Job pod template renders the label.
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%2F41396&comment_hash=5d2707d38c50c0ea4e98f27c96ff8f9530ce36cf1426cec5ea2b91ab53895268&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41396&comment_hash=5d2707d38c50c0ea4e98f27c96ff8f9530ce36cf1426cec5ea2b91ab53895268&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41396:
URL: https://github.com/apache/superset/pull/41396#issuecomment-5025124196
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41396:
URL: https://github.com/apache/superset/pull/41396#issuecomment-5013350663
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #41396:
URL: https://github.com/apache/superset/pull/41396#discussion_r3616767850
##########
helm/superset/tests/test-istio.sh:
##########
@@ -0,0 +1,176 @@
+#!/usr/bin/env bash
+#
+# 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.
+#
+# Renders the chart with `helm template` for several `init.istio.*` value
+# combinations and asserts that the relevant manifests contain (or omit)
+# the expected fields. Intended to be run from the chart directory or via
+# `bash helm/superset/tests/test-istio.sh` from the repo root.
+#
+# Covers the fix for:
+# https://github.com/apache/superset/issues/25798
+set -euo pipefail
+
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+CHART_DIR="$(cd "${SCRIPT_DIR}/.." && pwd)"
+
+pass=0
+fail=0
+
+assert_contains() {
+ local label="$1"
+ local needle="$2"
+ local haystack="$3"
+ if grep -qF -- "${needle}" <<<"${haystack}"; then
+ echo " PASS: ${label}"
+ pass=$((pass + 1))
+ else
+ echo " FAIL: ${label}"
+ echo " expected to contain: ${needle}"
+ fail=$((fail + 1))
+ fi
+}
+
+assert_not_contains() {
+ local label="$1"
+ local needle="$2"
+ local haystack="$3"
+ if ! grep -qF -- "${needle}" <<<"${haystack}"; then
+ echo " PASS: ${label}"
+ pass=$((pass + 1))
+ else
+ echo " FAIL: ${label}"
+ echo " expected NOT to contain: ${needle}"
+ fail=$((fail + 1))
+ fi
+}
+
+render() {
+ helm template release "${CHART_DIR}" "$@"
+}
+
+extract_init_job() {
+ awk '
+ /^# Source: superset\/templates\/init-job\.yaml/ { capture = 1 }
+ capture && /^---$/ { capture = 0 }
+ capture { print }
+ ' <<<"$1"
+}
+
+extract_config_secret() {
+ awk '
+ /^# Source: superset\/templates\/secret-superset-config\.yaml/ { capture = 1 }
+ capture && /^---$/ { capture = 0 }
+ capture { print }
+ ' <<<"$1"
+}
+
+echo "==> defaults: no istio mitigations applied"
+out_default="$(render)"
+init_job_default="$(extract_init_job "${out_default}")"
+config_default="$(extract_config_secret "${out_default}")"
+assert_not_contains "default does not set sidecar.istio.io/inject label" \
+ 'sidecar.istio.io/inject' "${init_job_default}"
+assert_not_contains "default initscript does not register quitquitquit trap" \
+ 'quitquitquit' "${config_default}"
+
+echo "==> init.istio.disableSidecarInjection=true"
+out_disable="$(render --set init.istio.disableSidecarInjection=true)"
+init_job_disable="$(extract_init_job "${out_disable}")"
+config_disable="$(extract_config_secret "${out_disable}")"
+assert_contains "init job sets sidecar.istio.io/inject: \"false\"" \
+ 'sidecar.istio.io/inject: "false"' "${init_job_disable}"
+assert_not_contains "disableSidecarInjection alone does not add quitquitquit trap" \
+ 'quitquitquit' "${config_disable}"
+
+echo "==> init.istio.terminateSidecarOnExit=true"
+out_terminate="$(render --set init.istio.terminateSidecarOnExit=true)"
+init_job_terminate="$(extract_init_job "${out_terminate}")"
+config_terminate="$(extract_config_secret "${out_terminate}")"
+assert_not_contains "terminateSidecarOnExit alone does not add inject label" \
+ 'sidecar.istio.io/inject' "${init_job_terminate}"
+assert_contains "initscript registers EXIT trap that calls quitquitquit" \
+ "trap 'rc=\$?; curl -fsS -m 5 -X POST" "${config_terminate}"
+assert_contains "initscript trap targets the configured quit endpoint" \
+ 'http://localhost:15020/quitquitquit' "${config_terminate}"
+assert_contains "initscript trap binds to the EXIT signal" \
+ "' EXIT" "${config_terminate}"
+
+echo "==> init.istio.quitEndpoint override"
+out_endpoint="$(render --set init.istio.terminateSidecarOnExit=true \
+ --set init.istio.quitEndpoint=http://127.0.0.1:15020/quitquitquit)"
+config_endpoint="$(extract_config_secret "${out_endpoint}")"
+assert_contains "trap honours custom quitEndpoint" \
+ 'http://127.0.0.1:15020/quitquitquit' "${config_endpoint}"
+
+echo "==> both options combined"
+out_both="$(render --set init.istio.disableSidecarInjection=true \
+ --set init.istio.terminateSidecarOnExit=true)"
+init_job_both="$(extract_init_job "${out_both}")"
+config_both="$(extract_config_secret "${out_both}")"
+assert_contains "combined: inject label present" \
+ 'sidecar.istio.io/inject: "false"' "${init_job_both}"
+assert_contains "combined: trap present" \
+ 'quitquitquit' "${config_both}"
+
+echo "==> existing init.podLabels are preserved alongside istio label"
+out_labels="$(render --set init.istio.disableSidecarInjection=true \
+ --set init.podLabels.team=data-platform)"
+init_job_labels="$(extract_init_job "${out_labels}")"
+assert_contains "user-supplied podLabel still rendered" \
+ 'team: data-platform' "${init_job_labels}"
+assert_contains "istio inject label rendered alongside" \
+ 'sidecar.istio.io/inject: "false"' "${init_job_labels}"
+
+echo "==> init.istio explicitly overridden to null"
+out_null_istio="$(render --set init.istio=null)"
+init_job_null_istio="$(extract_init_job "${out_null_istio}")"
+config_null_istio="$(extract_config_secret "${out_null_istio}")"
+assert_not_contains "null init.istio does not set inject label" \
+ 'sidecar.istio.io/inject' "${init_job_null_istio}"
+assert_not_contains "null init.istio does not register quitquitquit trap" \
+ 'quitquitquit' "${config_null_istio}"
+
+echo "==> EXIT trap propagates the script's exit code, not the notification's"
+# The trap's own curl call is best-effort (`|| true`) and must not mask a
Review Comment:
Good catch, the comment was stale from before I switched that trap to log a warning instead of swallowing the failure. Fixed to describe the actual behavior.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #41396:
URL: https://github.com/apache/superset/pull/41396#discussion_r3652629875
##########
helm/superset/tests/test-istio.sh:
##########
@@ -0,0 +1,183 @@
+#!/usr/bin/env bash
+#
+# 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.
+#
+# Renders the chart with `helm template` for several `init.istio.*` value
+# combinations and asserts that the relevant manifests contain (or omit)
+# the expected fields. Intended to be run from the chart directory or via
+# `bash helm/superset/tests/test-istio.sh` from the repo root.
+#
+# Covers the fix for:
+# https://github.com/apache/superset/issues/25798
+set -euo pipefail
+
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+CHART_DIR="$(cd "${SCRIPT_DIR}/.." && pwd)"
+
+pass=0
+fail=0
+
+assert_contains() {
+ local label="$1"
+ local needle="$2"
+ local haystack="$3"
+ if grep -qF -- "${needle}" <<<"${haystack}"; then
+ echo " PASS: ${label}"
+ pass=$((pass + 1))
+ else
+ echo " FAIL: ${label}"
+ echo " expected to contain: ${needle}"
+ fail=$((fail + 1))
+ fi
+}
+
+assert_not_contains() {
+ local label="$1"
+ local needle="$2"
+ local haystack="$3"
+ if ! grep -qF -- "${needle}" <<<"${haystack}"; then
+ echo " PASS: ${label}"
+ pass=$((pass + 1))
+ else
+ echo " FAIL: ${label}"
+ echo " expected NOT to contain: ${needle}"
+ fail=$((fail + 1))
+ fi
+}
+
+render() {
+ helm template release "${CHART_DIR}" "$@"
+}
+
+extract_init_job() {
+ awk '
+ /^# Source: superset\/templates\/init-job\.yaml/ { capture = 1 }
+ capture && /^---$/ { capture = 0 }
+ capture { print }
+ ' <<<"$1"
+}
+
+extract_config_secret() {
+ awk '
+ /^# Source: superset\/templates\/secret-superset-config\.yaml/ { capture = 1 }
+ capture && /^---$/ { capture = 0 }
+ capture { print }
+ ' <<<"$1"
+}
+
+echo "==> defaults: no istio mitigations applied"
+out_default="$(render)"
+init_job_default="$(extract_init_job "${out_default}")"
+config_default="$(extract_config_secret "${out_default}")"
+assert_not_contains "default does not set sidecar.istio.io/inject label" \
+ 'sidecar.istio.io/inject' "${init_job_default}"
+assert_not_contains "default initscript does not register quitquitquit trap" \
+ 'quitquitquit' "${config_default}"
+
+echo "==> init.istio.disableSidecarInjection=true"
+out_disable="$(render --set init.istio.disableSidecarInjection=true)"
+init_job_disable="$(extract_init_job "${out_disable}")"
+config_disable="$(extract_config_secret "${out_disable}")"
+assert_contains "init job sets sidecar.istio.io/inject: \"false\"" \
+ 'sidecar.istio.io/inject: "false"' "${init_job_disable}"
Review Comment:
Not a bug — `init-job.yaml` already dereferences `dig "istio" "disableSidecarInjection" false .Values.init` to render the `sidecar.istio.io/inject: "false"` label (line 38-49). Ran `test-istio.sh` locally, all 16 cases including that one pass.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41396:
URL: https://github.com/apache/superset/pull/41396#issuecomment-5086356604
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41396:
URL: https://github.com/apache/superset/pull/41396#issuecomment-4792829581
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #41396:
URL: https://github.com/apache/superset/pull/41396#discussion_r3607155308
##########
helm/superset/templates/_helpers.tpl:
##########
@@ -685,6 +685,13 @@ TALISMAN_CONFIG = {
{{- define "superset.initScript" -}}
#!/bin/sh
set -eu
+{{- if dig "istio" "terminateSidecarOnExit" false .Values.init }}
+# Notify the Istio pilot-agent sidecar to exit when this script completes
+# (whether successfully or via `set -e`), so that the Job can reach the
+# Completed state instead of hanging on a still-running envoy-proxy.
+# See https://github.com/apache/superset/issues/25798
+trap 'rc=$?; curl -fsS -m 5 -X POST {{ dig "istio" "quitEndpoint" "http://localhost:15020/quitquitquit" .Values.init | quote }} >/dev/null 2>&1 || true; exit $rc' EXIT
Review Comment:
Good catch, the value was double-quoted right inside the trap string, and the whole thing gets re-parsed when the trap fires, so `$()`/backticks in it would actually run. Pushed a fix that stashes it in a single-quoted shell var first and references that instead, so nothing in there gets interpreted.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41396:
URL: https://github.com/apache/superset/pull/41396#discussion_r3606650575
##########
helm/superset/templates/_helpers.tpl:
##########
@@ -685,6 +685,13 @@ TALISMAN_CONFIG = {
{{- define "superset.initScript" -}}
#!/bin/sh
set -eu
+{{- if dig "istio" "terminateSidecarOnExit" false .Values.init }}
+# Notify the Istio pilot-agent sidecar to exit when this script completes
+# (whether successfully or via `set -e`), so that the Job can reach the
+# Completed state instead of hanging on a still-running envoy-proxy.
+# See https://github.com/apache/superset/issues/25798
+trap 'rc=$?; curl -fsS -m 5 -X POST {{ dig "istio" "quitEndpoint" "http://localhost:15020/quitquitquit" .Values.init | quote }} >/dev/null 2>&1 || true; exit $rc' EXIT
Review Comment:
**Suggestion:** The configured `quitEndpoint` is injected into a double-quoted shell argument, which still allows command substitution (`$(...)` or backticks) at runtime. If this value is ever set from untrusted or templated input, it can execute arbitrary shell commands in the init container. Render the endpoint as a shell-safe single-quoted literal (or otherwise escape `$`/backticks) before embedding it in the trap command. [security]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Init-db job shell injection via quitEndpoint command substitution.
- ⚠️ Init container can run unintended commands during initialization.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
4. Deploy the chart so that the init-db Job runs: the init container command in
`helm/superset/templates/init-job.yaml` at line ~111 (`command: {{ tpl (toJson
.Values.init.command) . }}`) sources the generated `superset_init.sh` (see `values.yaml`
init.command lines 19–24 in the "Showing lines 910 to 989" block); when the script
completes, the `EXIT` trap executes, and the shell evaluates the double-quoted
`quitEndpoint`, causing the injected `$(touch /tmp/pwned)` (or other command substitution)
to run inside the init container.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=95f9ed3123a244bdb7a6f0d2e4638076&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=95f9ed3123a244bdb7a6f0d2e4638076&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:** 693:693
**Comment:**
*Security: The configured `quitEndpoint` is injected into a double-quoted shell argument, which still allows command substitution (`$(...)` or backticks) at runtime. If this value is ever set from untrusted or templated input, it can execute arbitrary shell commands in the init container. Render the endpoint as a shell-safe single-quoted literal (or otherwise escape `$`/backticks) before embedding it in the trap command.
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%2F41396&comment_hash=8a20641e25ce9e80ee2c657ec83d76310ed4fc36c3c2716c233bc35c80a6e92f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41396&comment_hash=8a20641e25ce9e80ee2c657ec83d76310ed4fc36c3c2716c233bc35c80a6e92f&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41396:
URL: https://github.com/apache/superset/pull/41396#issuecomment-5087413072
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41396:
URL: https://github.com/apache/superset/pull/41396#issuecomment-5081787617
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #41396:
URL: https://github.com/apache/superset/pull/41396
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(helm): mitigate init-db Job hang under Istio sidecar injection (closes #25798) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41396:
URL: https://github.com/apache/superset/pull/41396#issuecomment-5095160785
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org