chore(oss): add missing helm chart files - #214
Conversation
22fff47 to
ee228b6
Compare
🛡️ CodeQL Analysis🚨 Found 2 issue(s) Severity Breakdown:
📋 Top Issues🔗 View full details in Security tab 🕐 Last updated: 2026-07-16 21:49:25 UTC | Commit: ee228b6 |
37b4cd2 to
67ab7ec
Compare
86ecc74 to
70decc2
Compare
🛡️ CodeQL Analysis🚨 Found 2 issue(s) Severity Breakdown:
📋 Top Issues🔗 View full details in Security tab 🕐 Last updated: 2026-07-16 22:53:29 UTC | Commit: 70decc2 |
🛡️ CodeQL Analysis🚨 Found 2 issue(s) Severity Breakdown:
📋 Top Issues🔗 View full details in Security tab 🕐 Last updated: 2026-07-16 23:16:05 UTC | Commit: 70decc2 |
97ee757 to
546083b
Compare
🛡️ CodeQL Analysis🚨 Found 2 issue(s) Severity Breakdown:
📋 Top Issues🔗 View full details in Security tab 🕐 Last updated: 2026-07-16 23:24:01 UTC | Commit: 546083b |
🛡️ CodeQL Analysis🚨 Found 2 issue(s) Severity Breakdown:
📋 Top Issues🔗 View full details in Security tab 🕐 Last updated: 2026-07-16 23:41:23 UTC | Commit: 546083b |
546083b to
3869ce6
Compare
📝 WalkthroughWalkthroughAdded Helm chart tooling, values, render tests, NVCA Operator installation and release automation, Kyverno mutation policies, OpenBao deployment and upgrade utilities, signing-key validation, and NVCA AI workflow skills. ChangesNVCA AI skills
Helm chart tooling
NVCA Operator
Platform and OpenBao operations
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 🔧 Trivy (0.72.0)Trivy execution failed: 2026-08-03T16:34:33Z FATAL Fatal error run error: fs scan error: scan error: scan failed: failed analysis: post analysis error: post analysis error: ansible scan error: fs filter error: fs filter error: walk error range error: stat .golangci.yml: no such file or directory: range error: stat .golangci.yml: no such file or directory Comment |
kristinapathak
left a comment
kristinapathak
left a comment
There was a problem hiding this comment.
Thanks for doing this.
Add Helm chart files that were missing from the repository: build and test tooling (Makefiles, render tests), example values files, and the shared tools/ci/helm-validate-values CI values used by chart lint and render checks. Registry, host, and endpoint references in the added files use generic <your-registry>/<your-org>/<your-otel-endpoint> placeholders, matching the convention in the existing chart READMEs. The image registry examples in the openbao and cassandra READMEs are generalized the same way. Not added: per-chart license-header tooling (apply/check scripts, .license-header.txt, and the Makefile license-header targets), CODEOWNERS files, chart AGENTS.md/CLAUDE.md pairs, vendored subchart .tgz binaries, internal CI automation (renovate.json), and internal runbooks/examples. Closes #218
3869ce6 to
b6b5c11
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/helm/openbao/upgrade/runbook.md (1)
1-526: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftRemove bold emphasis, emojis, and em-dashes from this runbook.
This file uses Markdown bold for emphasis throughout (for example, lines 3-5, 7, 25), emoji markers (✅,
⚠️ , 💡, ❌ at lines such as 161, 171, 204, 254, 307, 399, 453), and an em-dash at line 215. The coding guideline for**/*.{md,mdx}files requires succinct, direct prose without bold emphasis, emojis, or em-dashes.Rewrite the headers, callouts, and step markers as plain sentences instead of bold/emoji-decorated text.
As per coding guidelines: "Documentation must be succinct and use short, direct sentences; do not use Markdown bold for emphasis, emojis, em-dashes, or non-ASCII committed text."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/openbao/upgrade/runbook.md` around lines 1 - 526, Update the entire OpenBao Upgrade Runbook to remove all Markdown bold markers, emoji symbols, em-dashes, and other non-ASCII committed text. Rewrite affected headings, callouts, labels, expected-result markers, and prose as concise plain sentences while preserving the runbook’s commands, structure, and meaning.Source: Coding guidelines
🟠 Major comments (26)
deploy/helm/nvcf-unbound/templates/kyverno/kyverno-cert-mount.yaml-96-296 (1)
96-296: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve certificate initContainer ordering.
When a Pod already has an init container,
patchStrategicMergecan placea-toolboxandfast-merge-certsafter it. The fixed/spec/initContainers/1insertion can then runb-extract-inferencebefore/tools/busyboxexists, or run consumer init containers beforemerged-certsis populated. Insert these containers without relying on fixed indices.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/nvcf-unbound/templates/kyverno/kyverno-cert-mount.yaml` around lines 96 - 296, Update the initContainer mutations around a-toolbox, fast-merge-certs, and b-extract-inference to use ordering-aware insertion rather than fixed positions or patchStrategicMerge placement. Ensure a-toolbox runs first, b-extract-inference runs after it, fast-merge-certs runs after extraction, and consumer init/download containers run only after merged-certs is populated; specifically remove the hard-coded /spec/initContainers/1 insertion in the b-extract-inference mutation.deploy/helm/llm-request-router/scripts/check-pki-render.sh-17-17 (1)
17-17: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd
pipefailto guard against masked pipeline failures.This script uses
set -euwithoutpipefail. Many assertions read a value from a pipeline such asyq ... | head -n1(for example, lines 33, 37, 42, 45, 55, 58). Ifyqfails,headstill exits 0, so the pipeline's exit status is 0 andset -edoes not trigger. The script then treats the resulting empty variable as "resource not rendered", which can pass the corresponding check even though the underlying query failed.Use
set -euo pipefail, matchingcheck-multi-replica-render.sh.🐛 Proposed fix
-set -eu +set -euo pipefail🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/llm-request-router/scripts/check-pki-render.sh` at line 17, Update the shell options declaration at the start of the script from set -eu to set -euo pipefail, ensuring failures from yq pipelines used by the assertions are propagated instead of masked by head.deploy/helm/llm-request-router/scripts/check-pki-render.sh-78-81 (1)
78-81: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix the PKI image test inputs and assertion.
- Quote the repository override. Bash treats the unquoted
<your-org>as input redirection.- The hook image renders as
nvcr.io/<your-org>/nvcf-openbao-migrations:0.12.1. Update the assertion to expect this value instead of<your-registry>/....🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/llm-request-router/scripts/check-pki-render.sh` around lines 78 - 81, Update the Helm arguments in the PKI rendering test to quote the repository override containing <your-org>, preventing shell redirection, and adjust the hook image assertion to expect nvcr.io/<your-org>/nvcf-openbao-migrations:0.12.1 instead of the <your-registry> value.deploy/helm/api-keys-colocated/Makefile-36-36 (1)
36-36: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPrevent cleanup outside the chart workspace.
When callers do not set
charts_dir, Line 90 expands torm -rf /*.tgz. This can delete unrelated root-level chart artifacts. Remove only known generated directories. Declarecleanas phony so the cleanup always runs.Based on learnings, never delete artifact directories without explicit user confirmation.
Proposed fix
-.PHONY: deploy delete status lint template +.PHONY: install uninstall status lint template validate clean package push-oci @@ clean: - rm -rf $(charts_dir)/*.tgz rm -rf ./packaged-chartsAlso applies to: 89-91
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/api-keys-colocated/Makefile` at line 36, Update the Makefile cleanup target around the chart artifact removal at lines 89–91 so it deletes only explicitly known generated directories and never expands to a root-level wildcard when charts_dir is unset; require explicit user confirmation before deleting artifact directories. Add clean to the .PHONY declaration alongside deploy, delete, status, lint, and template.Source: Learnings
deploy/helm/llm-api-gateway/Makefile-33-35 (1)
33-35: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winA bare
maketriggers a livehelm installin all three Makefiles. None of these Makefiles puts a safe, non-mutating target first, so runningmakewith no arguments resolves to a target that performshelm install ... --atomic --waitagainst a live cluster.
deploy/helm/llm-api-gateway/Makefile#L33-L35: noalltarget is defined;install(line 35) is the first real target and becomes the default goal. Add a safe first target, e.g.all: lintor ahelptarget, beforeinstall.deploy/helm/llm-request-router/Makefile#L30-L32: same gap;install(line 32) is the first real target and becomes the default goal. Add a safe first target beforeinstall.deploy/helm/nats/Makefile#L45-L47:all: install(line 46) still makes the default goal a live install. Pointallat a non-mutating target such aslintortemplate, or require an explicitmake install.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/llm-api-gateway/Makefile` around lines 33 - 35, Prevent bare make from performing a live Helm install: in deploy/helm/llm-api-gateway/Makefile lines 33-35 and deploy/helm/llm-request-router/Makefile lines 30-32, add a safe first default target such as all: lint or help before install; in deploy/helm/nats/Makefile lines 45-47, change the all target from install to a non-mutating target such as lint or template, while keeping install available only when explicitly requested.Source: Linters/SAST tools
deploy/helm/llm-request-router/bin/manifest.yaml-172-172 (1)
172-172: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate
image.repositorybefore rendering the image.The image helper renders
:0.3.2whenimage.repositoryis empty, and it has norequiredguard. Add validation so Helm fails with a clear error instead of producing an invalid image reference.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/llm-request-router/bin/manifest.yaml` at line 172, Update the image rendering configuration around the image helper to validate image.repository with Helm’s required guard before constructing the image reference. Ensure an empty repository causes a clear template error instead of rendering an invalid value such as :0.1.0.deploy/helm/cassandra/Makefile-43-43 (1)
43-43: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not force insecure image validation in standard chart commands.
These commands bypass image validation even when callers provide valid production image references. Keep this setting only in local-development values or require an explicit local-only opt-in.
deploy/helm/cassandra/Makefile#L43-L43: remove--set global.security.allowInsecureImages=truefrominstall.deploy/helm/cassandra/Makefile#L47-L47: remove the forced setting frominstall-local;values.local.ymlalready scopes it to local use.deploy/helm/cassandra/Makefile#L51-L51: remove the forced setting fromdebug-local.deploy/helm/cassandra/Makefile#L58-L58: remove--set global.security.allowInsecureImages=truefromupgrade.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/cassandra/Makefile` at line 43, Remove the forced global.security.allowInsecureImages=true setting from the install, install-local, debug-local, and upgrade targets in deploy/helm/cassandra/Makefile at lines 43, 47, 51, and 58. Preserve local-only behavior through values.local.yml or an explicit local opt-in, without changing the remaining Helm arguments.deploy/helm/cassandra/Makefile-42-42 (1)
42-42: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not mutate the checked-in Kustomize configuration during deployment.
kustomize edit set namespacechanges the source configuration. A command with a customnamespacecan leave the worktree dirty and cause later deployments to use the wrong namespace. Build through a non-mutating overlay or a temporary Kustomize configuration.
deploy/helm/cassandra/Makefile#L42-L42: replace the mutating command ininstall.deploy/helm/cassandra/Makefile#L46-L46: replace the mutating command ininstall-local.deploy/helm/cassandra/Makefile#L50-L50: replace the mutating command indebug-local.deploy/helm/cassandra/Makefile#L57-L57: replace the mutating command inupgrade.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/cassandra/Makefile` at line 42, Replace the mutating kustomize edit set namespace invocation in the install, install-local, debug-local, and upgrade targets of deploy/helm/cassandra/Makefile at lines 42-42, 46-46, 50-50, and 57-57 with a non-mutating overlay or temporary Kustomize configuration that applies the requested namespace without modifying the checked-in helm/secrets configuration.deploy/helm/cassandra/Makefile-60-62 (1)
60-62: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep namespace deletion opt-in.
Line 62 deletes the complete
cassandra-systemnamespace after it removes the Helm release. The namespace can contain resources that do not belong to this release. Remove this command or require an explicit confirmation variable such asDELETE_NAMESPACE=true.Based on learnings: Never delete clusters, Helm releases, worktrees, secrets, or artifact directories without explicit user confirmation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/cassandra/Makefile` around lines 60 - 62, Update the uninstall target to keep namespace deletion opt-in: remove the kubectl delete ns ${namespace} command or guard it behind an explicit DELETE_NAMESPACE=true confirmation check, while preserving Helm release removal.Source: Learnings
deploy/helm/grpc-proxy/Makefile-20-40 (1)
20-40: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix the deploy command contract.
deployanddeleteare phony targets without recipes, so they do not invokeinstalloruninstall. Adddeploy: installanddelete: uninstall, mark the implementation targets phony, or rename the implementation targets.
values := $(helm_dir)/values.yamlalways definesvalues, soifndef valuesnever requires caller input. Requirevaluesexplicitly or document the default file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/grpc-proxy/Makefile` around lines 20 - 40, The Makefile’s deploy contract is incomplete and the values validation is ineffective. Add dependencies so deploy invokes install and delete invokes uninstall, mark install and uninstall as phony implementation targets, and update the values handling so the install check genuinely requires caller-provided input or explicitly documents and uses the default values file.deploy/helm/grpc-proxy/Makefile-84-88 (1)
84-88: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestrict cleanup to chart artifacts.
Define
charts_diror remove the first cleanup command. Without an externalcharts_dirvalue,makeexpands it torm -rf /*.tgz. Becausepackagerunsclean, this can delete unrelated.tgzfiles at the filesystem root. Keep cleanup under./packaged-charts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/grpc-proxy/Makefile` around lines 84 - 88, Update the clean target to avoid using an undefined charts_dir variable: either define charts_dir to the intended chart directory or remove the first cleanup command, while keeping cleanup restricted to ./packaged-charts artifacts so the package target cannot delete unrelated root-level .tgz files.deploy/helm/nvca-operator/Makefile-165-169 (1)
165-169: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not delete release artifacts as a side effect of
push-oci.A successful upload does not authorize deletion of
./packaged-charts. Leave cleanup to the explicitcleantarget, or require an explicit cleanup option.Based on learnings, never delete clusters, Helm releases, worktrees, secrets, or artifact directories without explicit user confirmation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/nvca-operator/Makefile` around lines 165 - 169, Remove the automatic cleanup commands from the push-oci target, specifically the rm -rf ./packaged-charts invocation and its related cleanup messages, so successfully pushed release artifacts remain available. Leave artifact deletion to the explicit clean target or an explicitly requested cleanup option.Source: Learnings
deploy/helm/nvca-operator/scripts/attach_release_assets.sh-45-64 (1)
45-64: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail when no release artifacts exist.
Without
nullglob, an empty or missingrelease-artifactsdirectory makes this loop process the literal pathrelease-artifacts/*. The job can then create a link named*and complete without attaching a real artifact. Validate that at least one regular artifact file exists before the loop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/nvca-operator/scripts/attach_release_assets.sh` around lines 45 - 64, Validate before the asset-processing loop that release-artifacts contains at least one regular file, and fail the script if it does not. Update the loop around release-artifacts and the existing curl logic so only real artifact files are processed, preventing a literal * link from being created.deploy/helm/nvca-operator/Makefile-21-23 (1)
21-23: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSet the default stack path to the supported stack source.
The current default resolves to
deploy/nvcf-self-managed-stack. The NVCA skills define the control-plane stack asdeploy/stacks/self-managed. A parameter-freemake render-values-from-stacktherefore reads the wrong environment path.Proposed fix
-stack_repo ?= ../../nvcf-self-managed-stack +stack_repo ?= ../../../deploy/stacks/self-managed🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/nvca-operator/Makefile` around lines 21 - 23, Update the stack_repo default used by render-values-from-stack to point to the supported deploy/stacks/self-managed stack source instead of the current nvcf-self-managed-stack path. Keep stack_env and stack_env_file behavior unchanged so the parameter-free target resolves the local environment YAML under the corrected repository path.deploy/helm/openbao/README.oidc.md-28-29 (1)
28-29: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove Markdown bold emphasis.
Lines 28-29 use
**Default Issuer Path**and**Custom Issuer Path**for emphasis. Replace the bold markers with plain text.As per coding guidelines,
**/*.{md,mdx}files must "not use Markdown bold for emphasis".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/openbao/README.oidc.md` around lines 28 - 29, Update the Default Issuer Path and Custom Issuer Path headings in the README text to remove their Markdown ** emphasis markers, leaving both labels as plain text while preserving the surrounding descriptions.Source: Coding guidelines
deploy/helm/openbao/test.md-17-30 (1)
17-30: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove Markdown bold emphasis and the em-dash.
This file uses bold for emphasis repeatedly (for example lines 17, 18, 24, 25, 30, 78, 91, 106, 121, 125). Line 24 also contains an em-dash ("Configure NGC credentials — see..."). Replace bold emphasis with plain text throughout, and replace the em-dash with a period or comma.
As per coding guidelines,
**/*.{md,mdx}files must "not use Markdown bold for emphasis, emojis, em-dashes, or non-ASCII committed text".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/openbao/test.md` around lines 17 - 30, Update deploy/openbao/test.md to remove all Markdown bold markers from headings and instructional text, including the repeated emphasis throughout the file. Replace the em-dash in the Image Pull Secrets prerequisite with a period or comma, and ensure the Markdown contains no emojis, em-dashes, or other non-ASCII committed text.Source: Coding guidelines
deploy/helm/openbao/deploy.sh-217-230 (1)
217-230: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix hardcoded pod name in
get_and_save_jwt_signing_key.Line 224 hardcodes
openbao-server-0instead of${statefulset}-0. Lines 220 and 228 in the same function correctly use${statefulset}-0. If a caller passes a non-defaultstatefulsetargument (the script accepts it as$2), this line targets the wrong pod and the function fails or reads the token from an unrelated pod.🐛 Proposed fix
- local svc_token=$(kubectl exec openbao-server-0 -c openbao -n ${namespace} -- \ + local svc_token=$(kubectl exec ${statefulset}-0 -c openbao -n ${namespace} -- \ cat /var/run/secrets/kubernetes.io/serviceaccount/token)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/openbao/deploy.sh` around lines 217 - 230, In get_and_save_jwt_signing_key, replace the hardcoded openbao-server-0 pod reference used to read svc_token with the function’s ${statefulset}-0 reference, matching the kubectl exec calls for kub_api_ip and jwt_pem.deploy/helm/openbao/deploy.sh-141-164 (1)
141-164: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAvoid passing root token and unseal key as command-line arguments.
Line 154 passes the base64 unseal key, and line 163 passes the root token, as literal
kubectlcommand-line arguments. Both values remain visible in the local process list (e.g.ps aux) for the command's duration. Pipe the sensitive value through stdin instead.🔒️ Proposed fix
- kubectl create secret generic ${statefulset}-root-token \ - -n ${namespace} \ - --from-literal=root_token=${root_token} + printf '%s' "${root_token}" | kubectl create secret generic ${statefulset}-root-token \ + -n ${namespace} \ + --from-file=root_token=/dev/stdin🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/openbao/deploy.sh` around lines 141 - 164, Update the secret-writing commands in the initialization flow around the unseal_key and root_token variables so neither sensitive value is passed as a kubectl command-line argument. Feed each value through stdin using kubectl’s supported input mechanism while preserving the existing target secret names and namespaces.deploy/helm/openbao/deploy.sh-228-241 (1)
228-241: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse
mktempinstead of predictable/tmppaths.
/tmp/cluster.jwks(line 230) and/tmp/jwt.pem(line 239) are fixed, predictable paths. A local attacker can pre-create or symlink these paths before the script runs (TOCTOU/symlink risk on JWT signing-key material, CWE-377). Thetrapcleanup is also set only at line 240, after/tmp/cluster.jwksalready exists, so an early failure before that point leaves it behind.🔒️ Proposed fix
+ local jwks_file jwt_pem_file + jwks_file=$(mktemp) + jwt_pem_file=$(mktemp) + trap 'rm -f "$jwks_file" "$jwt_pem_file"' RETURN local jwt_pem=$(kubectl exec ${statefulset}-0 -c openbao -n ${namespace} -- \ curl -s --cacert /var/run/secrets/kubernetes.io/serviceaccount/ca.crt --header "Authorization: Bearer ${svc_token}" \ - "https://${kub_api_ip}/openid/v1/jwks" | jq ".keys[0]" > /tmp/cluster.jwks && jwker /tmp/cluster.jwks | base64) + "https://${kub_api_ip}/openid/v1/jwks" | jq ".keys[0]" > "$jwks_file" && jwker "$jwks_file" | base64) ... - echo "${jwt_pem}" > /tmp/jwt.pem - trap 'rm -f /tmp/jwt.pem /tmp/cluster.jwks' EXIT + echo "${jwt_pem}" > "$jwt_pem_file"Note: the static-analysis hint flagging
jwt_pem_secret_name="cluster-jwt"as a hardcoded credential is a false positive; this is a Kubernetes Secret name, not a credential value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/openbao/deploy.sh` around lines 228 - 241, Replace the predictable /tmp/cluster.jwks and /tmp/jwt.pem paths in the JWT key-generation flow with securely created mktemp files, establish cleanup via trap before either file is written, and update the jq/jwker, echo, and cleanup references to use those temporary-file variables. Leave jwt_pem_secret_name="cluster-jwt" unchanged.Source: Linters/SAST tools
deploy/helm/openbao/tools/generate-admin-token.sh-18-20 (1)
18-20: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
-itcombined with output capture can fail outside a terminal.
kubectl exec -itallocates a TTY and requires an interactive stdin. When this function runs from a non-interactive context (CI, cron, another script),kubectltypically errors instead of returning output through$(...). Drop-itfor output-capturing exec calls, since no interactive input is needed here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/openbao/tools/generate-admin-token.sh` around lines 18 - 20, Remove the interactive `-it` flags from the `kubectl exec` invocation that assigns output to `jwt_output` in the admin-token generation flow. Keep the existing container, namespace, environment, and command arguments unchanged so the non-interactive output capture works in CI and other non-terminal contexts.deploy/helm/openbao/tools/generate-admin-token.sh-13-13 (1)
13-13: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRoot token is passed as a process argument.
$root_tokenis interpolated into the executedkubectl execcommand as a literalenv BAO_TOKEN=...argument. This exposes the token in the container's process listing and in thekubectl execrequest captured by Kubernetes audit logging. Pass the token through stdin or a temporary file mounted with restrictive permissions instead of a command-line argument.Also applies to: 18-20
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/openbao/tools/generate-admin-token.sh` at line 13, Update generate-admin-token.sh so the root token retrieved by get_root_token is not interpolated into the kubectl exec command or any env argument. Pass it through stdin or a securely mounted, restrictive-permission temporary file, and adjust the corresponding command usage around the additional affected lines while preserving the existing token-dependent behavior.deploy/helm/openbao/tools/bao.sh-19-23 (1)
19-23: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEchoed pod does not match the executed pod.
Line 22 echoes a command against pod
$statefulset-0, but line 23 executes against pod$statefulset-2. The operator sees one target and a different one runs.generate-admin-token.shtargets pod-0for the same kind of admin operation, suggesting-2here is a typo.🐛 Proposed fix to align the executed pod with the echoed pod
-kubectl exec -n $namespace $statefulset-2 -c openbao -- $COMMAND +kubectl exec -n $namespace $statefulset-0 -c openbao -- $COMMAND🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/openbao/tools/bao.sh` around lines 19 - 23, Update the kubectl exec target in the command execution block of bao.sh to use the same $statefulset-0 pod already shown by the preceding echo. Keep the redacted display command and token-bearing COMMAND behavior unchanged.deploy/helm/openbao/tests/signing-key-harness/keygen/keygen.go-66-87 (1)
66-87: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAvoid interpolating arguments into a
bash -cscript string.
g.ScriptPathis interpolated directly intobashScriptand passed toexec.Command("bash", "-c", bashScript). If--scriptcontains a double quote or backtick, it breaks out of thesource "%s"string and changes what the shell executes. Two static analysis tools flag this pattern as CWE-78 OS command injection.Pass the script path and
kidas positional arguments instead of interpolating them into the script text, so they are never re-parsed by the shell.🔒 Proposed fix using positional arguments
- bashScript := fmt.Sprintf(` - source "%s" - generate_asymmetric_signing_key "%s" - `, g.ScriptPath, kid) - - cmd := exec.Command("bash", "-c", bashScript) + cmd := exec.Command("bash", "-c", + `source "$1"; generate_asymmetric_signing_key "$2"`, + "bash", g.ScriptPath, kid)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/openbao/tests/signing-key-harness/keygen/keygen.go` around lines 66 - 87, Update Generate to stop interpolating g.ScriptPath and kid into bashScript. Keep the shell script static and pass both values as positional arguments to exec.Command, then reference those positional parameters when sourcing the script and invoking generate_asymmetric_signing_key, preserving the existing error handling.Source: Linters/SAST tools
deploy/helm/openbao/upgrade/upgrade-test.sh-34-39 (1)
34-39: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix dead
$?checks underset -e.
set -euo pipefailis active (line 9).docker context use ...(line 34) anddocker pull "${IMAGE_TAG}"(line 52) are standalone commands, not part of aniftest. Underset -e, a nonzero exit from either command terminates the script immediately, so theif [ $? -ne 0 ]blocks at lines 36-39 and 54-59 never run. The customlog_errormessages and context-restore logic in those blocks are unreachable.Guard each command with
if ! cmd; then ... fiinstead of checking$?afterward.🐛 Proposed fix
-docker context use colima-${COLIMA_PROFILE} - -if [ $? -ne 0 ]; then - log_error "Failed to switch Docker context to colima-${COLIMA_PROFILE}" - exit 1 -fi +if ! docker context use colima-${COLIMA_PROFILE}; then + log_error "Failed to switch Docker context to colima-${COLIMA_PROFILE}" + exit 1 +fi log_success "Docker context switched to colima-${COLIMA_PROFILE}"- docker pull "${IMAGE_TAG}" - - if [ $? -ne 0 ]; then + if ! docker pull "${IMAGE_TAG}"; then log_error "Failed to pull image ${IMAGE_TAG} — ensure the image has been built and pushed by the nvcf-openbao pipeline" trap - EXIT docker context use ${ORIGINAL_CONTEXT} > /dev/null 2>&1 || true exit 1 fiAlso applies to: 47-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/openbao/upgrade/upgrade-test.sh` around lines 34 - 39, Update the error handling around docker context use and docker pull to guard each command with an if ! condition, replacing the unreachable subsequent $? checks. Preserve the existing log_error messages, exits, and context-restore logic within the corresponding failure branches.deploy/helm/openbao/utils/utils.sh-32-67 (1)
32-67: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix the version boundary check in
version_gt.
check_helm(lines 32-44) andcheck_kubernetes(lines 55-67) both state a "X.Y+ required" requirement, but rely onversion_gt, which returns false whenversion1 == version2(strict greater-than). This means a Helm version of exactly3.12.0or a Kubernetes version of exactly1.29.0is rejected, even though it satisfies the stated "+" requirement.Add an equality short-circuit so
version_gtbehaves as greater-than-or-equal for these callers.🐛 Proposed fix
version_gt() { local version1=$1 local version2=$2 + if [ "$version1" = "$version2" ]; then + return 0 + fi if [[ "$(printf '%s\n' "$version2" "$version1" | sort -V | head -n1)" != "$version1" ]]; then return 0 fi return 1 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/openbao/utils/utils.sh` around lines 32 - 67, Update version_gt to return success when version1 equals version2, while preserving the existing success behavior for versions greater than version2 and failure for lower versions. This makes the 3.12.0 and 1.29.0 minimum checks in check_helm and check_kubernetes inclusive.deploy/helm/openbao/utils/lb.sh-180-214 (1)
180-214: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winScope
cleanup_metallbdeletions to resources this script created.
kubectl delete l2advertisements.metallb.io -n metallb-system --all(line 193) andkubectl delete ipaddresspools.metallb.io -n metallb-system --all(line 200) remove every L2Advertisement and IPAddressPool in themetallb-systemnamespace, not onlyopenbao-server-pool/openbao-server-pool-l2created bysetup_metallb. On a cluster where MetalLB is shared with other workloads, this cleanup deletes their address pools too.Recompute the same
pool_name/l2_nameconstants incleanup_metallband delete only those named resources.🔧 Proposed fix
cleanup_metallb() { local statefulset=$1 + local pool_name="openbao-server-pool" + local l2_name="${pool_name}-l2" log_section "Cleaning up MetalLB for ${statefulset}..." # Remove LoadBalancer service log_info "Removing LoadBalancer service..." if ! kubectl delete svc ${statefulset}-lb -n vault-system 2>/dev/null; then log_info "LoadBalancer service not found or already removed" fi # Remove L2 advertisements log_info "Removing L2 advertisements..." - if ! kubectl delete l2advertisements.metallb.io -n metallb-system --all; then + if ! kubectl delete l2advertisements.metallb.io "${l2_name}" -n metallb-system --ignore-not-found; then log_error "Failed to remove L2 advertisements" return 1 fi # Remove IP address pools log_info "Removing IP address pools..." - if ! kubectl delete ipaddresspools.metallb.io -n metallb-system --all; then + if ! kubectl delete ipaddresspools.metallb.io "${pool_name}" -n metallb-system --ignore-not-found; then log_error "Failed to remove IP address pools" return 1 fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/openbao/utils/lb.sh` around lines 180 - 214, Update cleanup_metallb to recompute the same pool_name and l2_name values used by setup_metallb, then replace both --all deletions with deletes targeting only those named IPAddressPool and L2Advertisement resources in metallb-system; preserve the existing failure handling and cleanup order.
🟡 Minor comments (16)
deploy/helm/nvcf-unbound/templates/kyverno/kyverno-dns.yaml-78-80 (1)
78-80: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winQuote and guard the templated nameserver values.
{{ $.Values.unbound.cluster_ip }}and{{ $.Values.unbound.stub_nameserver }}are rendered unquoted into a YAML list of strings. If either value is unset, the rendered list item becomes empty/null, which Kubernetes rejects fordnsConfig.nameservers(expects a string). Wrap both withquoteandrequiredto fail fast at render time instead of producing an invalid manifest that all NVCF pods depend on for DNS resolution.🛠️ Proposed fix
nameservers: - - {{ $.Values.unbound.cluster_ip }} # IP of UnboundDNS - - {{ $.Values.unbound.stub_nameserver }} # IP of coreDNS + - {{ required "Set .Values.unbound.cluster_ip" $.Values.unbound.cluster_ip | quote }} # IP of UnboundDNS + - {{ required "Set .Values.unbound.stub_nameserver" $.Values.unbound.stub_nameserver | quote }} # IP of coreDNS🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/nvcf-unbound/templates/kyverno/kyverno-dns.yaml` around lines 78 - 80, Update the nameservers entries in the dnsConfig template to apply Helm’s required validation and quote both $.Values.unbound.cluster_ip and $.Values.unbound.stub_nameserver, ensuring missing values fail during rendering and rendered list items remain strings.deploy/helm/nvcf-unbound/templates/kyverno/kyverno-cert-mount.yaml-199-199 (1)
199-199: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReplace the pipe expression with separate conditional-anchor entries. Kyverno does not support
|as an OR operator in validation or mutation patterns. Use separate entries for*init*and*download*; otherwise neither consumer may receive the certificate injection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/nvcf-unbound/templates/kyverno/kyverno-cert-mount.yaml` at line 199, Replace the combined “*init*|*download*” anchor in the Kyverno certificate-mount pattern with separate conditional-anchor entries for “*init*” and “*download*”, preserving certificate injection for both consumers.deploy/helm/gateway-routes/scripts/test-vanity-gateway-route.sh-31-36 (1)
31-36: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winScope each assertion to the rendered vanity route.
These
grepchecks search the complete manifest independently. The test can pass when the name, hostname, backend, port, and annotation occur in different resources. Useyqto select the expected route and verify all fields in that one resource.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/gateway-routes/scripts/test-vanity-gateway-route.sh` around lines 31 - 36, Update the assertions in the vanity route test to use yq to select the expected rendered route resource before validating fields. Scope the name, hostname, backend, namespace, port, and annotation checks to that single route, ensuring the test cannot pass based on values from different manifest resources.deploy/helm/nats-auth-callout/README.md-30-30 (1)
30-30: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace Markdown bold used for emphasis.
Lines 30, 46, 70, and 157 use bold text (
**Validation Rules:**,**ServiceMonitor Validation:**,**Dashboard Validation:**,**Available Health Endpoints:**) as section labels. Use a heading or plain sentence instead.Based on coding guidelines:
**/*.{md,mdx}: "do not use Markdown bold for emphasis, emojis, em-dashes, or non-ASCII committed text."Also applies to: 46-46, 70-70, 157-157
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/nats-auth-callout/README.md` at line 30, Replace the bold section labels Validation Rules, ServiceMonitor Validation, Dashboard Validation, and Available Health Endpoints in the README with appropriate Markdown headings or plain text labels, preserving their existing section structure and content.Source: Coding guidelines
deploy/helm/nats/Makefile-88-91 (1)
88-91: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
cleandoes not remove the packaged chart output.
packagewrites the built chart to./packaged-charts/(line 106), butcleanonly removes$(CHARTS_SUBDIR)/*.tgzandhelm-nvcf-nats-*.tgzfrom the current directory. Ifpackageruns without a subsequentpush-oci(which separately removes./packaged-chartson line 125), the packaged.tgzunder./packaged-charts/is never cleaned up.🧹 Proposed fix
clean: `@echo` "Cleaning up downloaded chart dependencies..." rm -rf $(CHARTS_SUBDIR)/*.tgz - rm -f helm-nvcf-nats-*.tgz # Remove packaged chart + rm -rf ./packaged-charts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/nats/Makefile` around lines 88 - 91, Update the clean target to remove the packaged chart artifact produced by package in ./packaged-charts/, while preserving its existing dependency cleanup and current-directory archive removal.deploy/helm/cassandra/Makefile-1-1 (1)
1-1: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd the required
testtarget.
make testhas no rule. The checkmake result requires this target. Add a target that renders the chart and runs its available validation checks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/cassandra/Makefile` at line 1, Add the missing test target to the Makefile’s .PHONY declaration and define it to render the Cassandra chart and run the available chart validation checks, reusing existing targets or commands where appropriate so make test succeeds.Source: Linters/SAST tools
deploy/helm/grpc-proxy/values.additionalvolumes.yaml-19-24 (1)
19-24: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRemove empty image fields from partial overlays.
- Remove them from
values.additionalvolumes.yamlandvalues.extraenv.yaml; they replace valid values fromvalues.deployment.yamland causegrpc-proxy.imagerequired-field failures.- Keep usable
registryandrepositoryvalues invalues.daemonset.yaml, or update its command to provide them. Removing them alone still inherits the chart’s empty required fields.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/grpc-proxy/values.additionalvolumes.yaml` around lines 19 - 24, Remove the empty grpcproxy.image.registry and grpcproxy.image.repository fields from deploy/helm/grpc-proxy/values.additionalvolumes.yaml lines 19-24 and deploy/helm/grpc-proxy/values.extraenv.yaml lines 19-24 so valid deployment values are inherited. In deploy/helm/grpc-proxy/values.daemonset.yaml lines 19-24, retain usable registry and repository values or update its command to supply them, ensuring grpc-proxy.image required fields remain populated.ai-tooling/dev/skills/nvca-values-customization/SKILL.md-44-51 (1)
44-51: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the documented service-key value.
scripts/render_values_from_stack_env.shsetsngcConfig.serviceKeyto"not-used". This skill says the vendoring flow sets it to"dummy-api-key". State the correct value and identify the script that applies it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ai-tooling/dev/skills/nvca-values-customization/SKILL.md` around lines 44 - 51, Update the documented ngcConfig.serviceKey default in the vendoring defaults list to "not-used", and identify scripts/render_values_from_stack_env.sh as the script that applies this value.ai-tooling/dev/skills/nvca-chart-release/SKILL.md-20-20 (1)
20-20: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the required
## Instructionssection to each public skill.Each skill has workflow content, but none has the required concise
## Instructionsheading.
ai-tooling/dev/skills/nvca-chart-release/SKILL.md#L20-L20: Add## Instructionsbefore the release workflow steps.ai-tooling/dev/skills/nvca-self-managed-install/SKILL.md#L20-L20: Add## Instructionsbefore the install workflow steps.ai-tooling/dev/skills/nvca-values-customization/SKILL.md#L20-L20: Add## Instructionsbefore the values workflow steps.As per coding guidelines, every public
SKILL.mdmust include a concise## Instructionssection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ai-tooling/dev/skills/nvca-chart-release/SKILL.md` at line 20, Add a concise “## Instructions” section before the workflow steps in ai-tooling/dev/skills/nvca-chart-release/SKILL.md at lines 20-20, ai-tooling/dev/skills/nvca-self-managed-install/SKILL.md at lines 20-20, and ai-tooling/dev/skills/nvca-values-customization/SKILL.md at lines 20-20, ensuring each public SKILL.md contains the required heading.Source: Coding guidelines
deploy/helm/nvca-operator/scripts/build_release_image_manifest.sh-42-70 (1)
42-70: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winBoth image-extraction scripts silently drop incomplete candidates with no diagnostic.
build_release_image_manifest.shandrender_release_supplemental_images.sheach filter out image references that lack a required part (tag/digest, or repo/tag) before adding them to the release image manifest used for SBOM generation. Neither script logs what it skips, so a malformed or unexpectedly incomplete image reference in a future chart change would silently disappear from the compliance artifact instead of raising a visible warning.
deploy/helm/nvca-operator/scripts/build_release_image_manifest.sh#L42-L70: invalid_image(), emit a warning to stderr (with the rejectedimage:line) whenever a candidate is rejected, instead of silently discarding it.deploy/helm/nvca-operator/scripts/render_release_supplemental_images.sh#L39-L53: inemit(), emit a warning to stderr (with the rejected repo/tag pair) wheneverrepoortagis empty or still templated, instead of silently discarding it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/nvca-operator/scripts/build_release_image_manifest.sh` around lines 42 - 70, Update valid_image() in deploy/helm/nvca-operator/scripts/build_release_image_manifest.sh (lines 42-70) to emit a warning to stderr containing the rejected image: line whenever validation fails, while preserving the existing filtering behavior. Update emit() in deploy/helm/nvca-operator/scripts/render_release_supplemental_images.sh (lines 39-53) to warn on stderr with the rejected repo/tag pair whenever repo or tag is empty or remains templated, instead of silently discarding it.deploy/helm/nvca-operator/scripts/ci_vendor_nvca_operator_chart-1-3 (1)
1-3: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the SPDX header and align the shebang with sibling scripts.
This file uses
#!/bin/bashand has no SPDX header. Every other script indeploy/helm/nvca-operator/scripts/(build_release_image_manifest.sh,generate_release_sbom.sh,normalize_release_artifact_permissions.sh,package_release_assets.sh,render_release_supplemental_images.sh,render_values_from_stack_env.sh) uses#!/usr/bin/env bashand carries an SPDX-FileCopyrightText / SPDX-License-Identifier Apache-2.0 header. This script even applies that same header to other files viaapply_license_headers_if_missing(). Add the missing header and shebang for consistency with the subtree convention.📝 Proposed header fix
-#!/bin/bash +#!/usr/bin/env bash +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed 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 +# +# https://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. + set -euo pipefail🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/nvca-operator/scripts/ci_vendor_nvca_operator_chart` around lines 1 - 3, Update the script header before the existing root_dir initialization: add the standard SPDX-FileCopyrightText and SPDX-License-Identifier Apache-2.0 lines used by sibling scripts, and replace the direct /bin/bash shebang with /usr/bin/env bash. Leave the script logic unchanged.deploy/helm/openbao/deploy.sh-329-337 (1)
329-337: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix misleading log message.
The
elsebranch here logs"Skipping local installation as install_method is not 'script'", but this block guardscreate_empty_secret, not "local installation" (that message is reused, correctly, at line 346 forinstall_openbao). Update the message to describe the step actually being skipped.📝 Proposed fix
- log_info "Skipping local installation as install_method is not 'script'" + log_info "Skipping empty-secret creation as install_method is not 'script'"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/openbao/deploy.sh` around lines 329 - 337, Update the else branch guarding create_empty_secret to log that creation of the empty unseal secret is being skipped when install_method is not "script"; leave the separate install_openbao log unchanged.deploy/helm/openbao/deploy.sh-100-109 (1)
100-109: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
--create-namespaceand resolvevalues.yamlrelative to the script directory.The
helm installininstall_openbaohas no--create-namespaceflag, so it fails if${namespace}does not already exist. It also referenceshelm/values.yamlas a path relative to the caller's current working directory, whileDEPLOY_SCRIPT_DIR(computed at line 23) is available and used for sourcingutils.shbut not reused here. Running the script from a different working directory breaks this reference.🐛 Proposed fix
- helm install -n ${namespace} ${statefulset} openbao/openbao --values helm/values.yaml --debug --set='global.imagePullSecrets[0].name=nvcr-secret' + helm install -n ${namespace} ${statefulset} openbao/openbao --create-namespace --values "${DEPLOY_SCRIPT_DIR}/helm/values.yaml" --debug --set='global.imagePullSecrets[0].name=nvcr-secret'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/openbao/deploy.sh` around lines 100 - 109, Update install_openbao so the helm install command includes --create-namespace and resolves the values file using DEPLOY_SCRIPT_DIR instead of the caller’s working directory, while preserving the existing namespace, release, chart, and image-pull-secret arguments.deploy/helm/openbao/upgrade/upgrade-test.sh-163-166 (1)
163-166: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the auto-unseal sidecar's image registry so it matches the server image.
server.image(lines 126-130) andinjector.agentImage(lines 204-208) both useregistry: nvcr.iowithrepository: <your-org>/nvcf-openbao. The auto-unseal-sidecar at line 166 instead uses${IMAGE_NAME}:${LOCAL_TEST_TAG}, whereIMAGE_NAME(line 19) is<your-registry>/<your-org>/nvcf-openbao. If a user fills in<your-registry>with a value other thannvcr.io, the sidecar and server point at different registries, which contradicts the comment at line 163 that this block should "match server version."Derive the sidecar image from the same
server.imagefields, asvalues-upgrades.yamldoes.🔧 Proposed fix
extraContainers: - name: auto-unseal-sidecar - image: ${IMAGE_NAME}:${LOCAL_TEST_TAG} + image: '{{ .Values.server.image.registry }}/{{ .Values.server.image.repository }}:{{ .Values.server.image.tag }}'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/openbao/upgrade/upgrade-test.sh` around lines 163 - 166, Update the auto-unseal-sidecar image in the upgrade test’s extraContainers configuration to derive its registry, repository, and tag from the existing server.image fields, matching the pattern used in values-upgrades.yaml. Remove the direct IMAGE_NAME reference while preserving the server-version alignment.deploy/helm/openbao/utils/utils.sh-10-14 (1)
10-14: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate
get_root_token's output before use.
kubectl get secret ... -o jsonpath='{.data.root_token}' | base64 -ddoesn't check whetherkubectlsucceeded or whether theroot_tokenfield is empty;base64 -don empty input still exits 0.tools/operator-stepdown.sh,tools/raft-list-peers.sh, andtools/register_plugin.shall call this function and pass the result straight intoBAO_TOKENfor akubectl execcommand. If the field is empty, those scripts proceed with an empty token and fail with a confusingbaoauth error instead of a clear message here.Add a check for an empty/failed retrieval and log a clear error before decoding.
🔧 Proposed fix
get_root_token() { local namespace=$1 local statefulset=$2 - kubectl get secret $statefulset-root-token -n $namespace -o jsonpath='{.data.root_token}' | base64 -d + local encoded_token + encoded_token=$(kubectl get secret "$statefulset-root-token" -n "$namespace" -o jsonpath='{.data.root_token}') || return 1 + if [ -z "$encoded_token" ]; then + log_error "root_token field is empty in secret $statefulset-root-token" + return 1 + fi + echo "$encoded_token" | base64 -d }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/openbao/utils/utils.sh` around lines 10 - 14, Update get_root_token to capture and validate the kubectl secret retrieval before decoding: fail when kubectl returns an error or the root_token field is empty, log a clear error message, and only then decode and return the token. Preserve the function’s existing namespace and statefulset inputs and output behavior for valid secrets.deploy/helm/openbao/tools/operator-stepdown.sh-10-18 (1)
10-18: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRemove
-itfromkubectl exec.
bao operator step-downdoes not require terminal input. In non-interactive execution,kubectl exec -itcan fail with a TTY error. Usekubectl execwithout TTY allocation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/openbao/tools/operator-stepdown.sh` around lines 10 - 18, Update the kubectl invocation in operator_stepdown to remove the interactive and TTY flags from kubectl exec, while preserving the existing namespace, container, token environment, and bao operator step-down arguments.
| clean: | ||
| rm -rf $(charts_dir)/*.tgz | ||
| rm -rf ./packaged-charts |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Remove the undefined destructive path.
charts_dir is not assigned in this Makefile. make clean expands Line 149 to rm -rf /*.tgz, which can delete archive files outside this chart workspace. Remove this command. Line 150 already removes the local package directory.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/helm/nvca-operator/Makefile` around lines 148 - 150, Remove the rm
command using the undefined charts_dir variable from the clean target; keep the
existing ./packaged-charts removal in clean so cleanup remains limited to the
local package directory.
|
🎉 This PR is included in version helm-nvca-operator-v1.17.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version nvcf-container-cache-v0.28.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
6 participants
Why
Several Helm chart files were missing from the repository: build and test tooling (Makefiles, license-header scripts, render tests), example values files, and the shared CI values used by chart lint and render checks. This adds them so the charts are complete and their tests are runnable.
What changed
Add the missing files across the deploy/helm charts, plus tools/ci/helm-validate-values. Registry, host, and endpoint references in the added files use generic // placeholders, matching the convention in the existing chart READMEs.
Chart AGENTS.md and CLAUDE.md files are out of scope here; they reference paths not yet present in the repository and land in a later change.
Testing
Chart lint and render values are included so the existing CI checks exercise the added files. SPDX license headers are present on added source files.
References
Closes #218
Summary by CodeRabbit
New Features
Documentation
Tests