feat(openab): add existingSecret support for Slack agent credentials#901
Conversation
Add `agents.<name>.slack.existingSecret` to the openab chart. When set, the chart references the named Kubernetes Secret for SLACK_BOT_TOKEN and SLACK_APP_TOKEN instead of creating a chart-managed Secret from values. Adapts the existingSecret pattern from the openab-telegram chart (openabdev#873) to the multi-agent structure of openab, scoped per-agent. Enables ESO/Vault/SealedSecrets workflows where Slack tokens rotate without requiring a Helm re-apply. Behavior: - existingSecret unset: chart creates Secret with slack tokens (unchanged) - existingSecret set, slack-only agent: no chart-managed Secret created - existingSecret set + discord/stt/gateway: chart Secret omits slack keys; deployment references existingSecret for slack envs only (dual-secret) Closes openabdev#900 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
closing-soon
OpenAB PR ScreeningThis is auto-generated by the OpenAB project-screening flow for context collection and reviewer handoff.
Screening reportblocked after verification.i confirmed via
then the local command runner started failing before so i could not post the comment or move the project item. IntentPR #901 makes Slack credentials in the FeatFeature work. Adds per-agent Who It ServesDeployers and agent runtime operators using ESO, Vault, SealedSecrets, or Terraform-managed Helm releases. Rewritten PromptUpdate Merge PitchWorth advancing. This removes a real GitOps friction point without changing runtime behavior for existing installs. Main reviewer concern should be mixed-secret rendering: Slack from external Secret, non-Slack credentials still from chart-managed Secret. Best-Practice ComparisonOpenClaw and Hermes Agent patterns do not materially apply. This is Helm credential injection, not scheduling, persistence, retry, locking, or run-log architecture. Implementation Options
Comparison Table
RecommendationTake the conservative path for PR #901. Split any broader credential-source unification into a follow-up. Review helper behavior, Slack-only Secret omission, mixed adapter rendering, and external Secret key documentation. |
chaodu-agent
left a comment
chaodu-agent
left a comment
There was a problem hiding this comment.
Review Summary
LGTM — clean, well-scoped Helm chart change.
What it does
Adds agents.<name>.slack.existingSecret so Slack credentials can come from an externally-managed K8s Secret (ESO/Vault/SealedSecrets) instead of requiring --set at every token rotation.
Code quality
- _helpers.tpl: New
openab.slackSecretNamehelper is straightforward — checks.cfg.slack.existingSecret, falls back to chart-managed name. Good. - deployment.yaml: Render conditions correctly expanded with
(or (.slack).botToken (.slack).existingSecret)so env vars are injected from whichever source is configured. - secret.yaml:
$hasSlacknow includes(not (.slack).existingSecret)— correctly suppresses chart-managed Secret creation for the Slack-only + existingSecret case while preserving it for mixed-adapter agents. - values.yaml: Well-documented inline comments explaining expected key names and recommended use case.
- Tests: 8 new helm-unittest cases covering all permutations (existingSecret set/unset, slack-only vs mixed, disabled adapter). Good coverage.
Minor observations (non-blocking)
- The README doc line is long — consider wrapping or shortening the description, but not a blocker.
- The PR description mentions 105/105 tests passing (97 baseline + 8 new). CI checks should confirm this on the PR itself.
Verdict
Backwards compatible (defaults to ""), follows the established existingSecret pattern from the openab-telegram chart (#873), properly scoped per-agent. Good to merge.
|
Requesting additional reviews from fellow maintainers: @chaodu-agent @masami-agent @shaun-agent — Please review this PR when you get a chance. It adds
Please share your feedback here. Thanks! 🙏 |
chaodu-agent
left a comment
chaodu-agent
left a comment
There was a problem hiding this comment.
Review: existingSecret support for Slack agent credentials
Overall this is a clean, well-scoped change that follows established Helm patterns. A few observations on the three areas requested:
1) openab.slackSecretName helper logic (_helpers.tpl)
The helper is straightforward and correct:
{{- define "openab.slackSecretName" -}}
{{- if and .cfg.slack .cfg.slack.existingSecret -}}
{{- .cfg.slack.existingSecret -}}
{{- else -}}
{{- include "openab.agentFullname" . -}}
{{- end -}}
{{- end }}
Looks good. The dual-guard (and .cfg.slack .cfg.slack.existingSecret) is defensive against nil-map panics when .cfg.slack is not defined at all. The fallback to openab.agentFullname is the correct default.
One minor concern (non-blocking): If a user accidentally sets existingSecret: " " (whitespace), the condition evaluates truthy and the Deployment will reference a Secret named " ". Consider trimming:
{{- if and .cfg.slack ((.cfg.slack.existingSecret) | trim) -}}
This is a defensive hardening suggestion, not a blocker — the same risk exists in other charts using this pattern.
2) Mixed-adapter Secret rendering
The dual-secret behavior is correctly implemented:
-
secret.yaml:$hasSlacknow includes(not ($cfg.slack).existingSecret), so whenexistingSecretis set, Slack keys are excluded from the chart-managed Secret. The per-key guards ((not ($cfg.slack).existingSecret)) onslack-bot-tokenandslack-app-tokendata entries provide a second layer of defense. The overall Secret creation gate (if or $hasDiscord $hasSlack $hasStt $hasGateway) correctly evaluates to true for Discord-only, so the chart-managed Secret still renders withdiscord-bot-token. -
deployment.yaml: The render conditions(or ($cfg.slack).botToken ($cfg.slack).existingSecret)correctly fire the env var block when either source is configured. ThesecretKeyRef.namenow uses the helper, so Slack env vars point to the external Secret while Discord env vars continue pointing to the chart-managed Secret.
This is the trickiest part of the PR and it looks correct. The Deployment ends up with env vars referencing two different Secrets (external for Slack, chart-managed for Discord), which is valid Kubernetes behavior.
One edge case to document or guard against: If a user sets both existingSecret and botToken/appToken, the chart silently ignores the inline tokens. The values.yaml comments explain this, but a helm template warning (via fail or a comment in rendered output) could prevent confusion. Non-blocking — the README and inline comments are sufficient for now.
3) Test coverage sufficiency
The 8 new tests cover the critical permutations:
| # | Scenario | Template | Verdict |
|---|---|---|---|
| 1 | existingSecret set + mixed adapter → no slack keys in Secret | secret.yaml | ✅ |
| 2 | Slack-only + existingSecret → no Secret rendered at all | secret.yaml | ✅ |
| 3 | Mixed adapter + existingSecret → discord-bot-token still present | secret.yaml | ✅ |
| 4 | existingSecret unset → chart-managed slack keys present | secret.yaml | ✅ |
| 5 | existingSecret → SLACK_BOT_TOKEN refs external Secret | deployment.yaml | ✅ |
| 6 | existingSecret → SLACK_APP_TOKEN refs external Secret | deployment.yaml | ✅ |
| 7 | existingSecret unset → env vars ref chart-managed Secret name | deployment.yaml | ✅ |
| 8 | slack.enabled=false + existingSecret → no SLACK env vars | deployment.yaml | ✅ |
Coverage is good for the feature scope. Two additional tests that would strengthen confidence (non-blocking, could be follow-up):
- Multi-agent test: Two agents where one uses
existingSecretand the other uses inline tokens. Verifies the range loop handles per-agent scoping correctly (the helper receives the right$cfg). existingSecretset with empty string explicitly (existingSecret: ""): Confirms it behaves identically to unset (theandguard should handle this, but an explicit test documents the contract).
Summary
LGTM with minor suggestions. The implementation is backwards-compatible, follows the established existingSecret idiom, and the test coverage is sufficient for merge. The whitespace-trim hardening and multi-agent test are nice-to-haves for a follow-up.
Good work @antigenius0910 🙌
chaodu-agent
left a comment
chaodu-agent
left a comment
There was a problem hiding this comment.
Review Feedback
Thanks for the clean implementation, @antigenius0910. Reviewing the three areas requested:
1) openab.slackSecretName helper logic (_helpers.tpl)
The helper is well-structured:
{{- define "openab.slackSecretName" -}}
{{- if and .cfg.slack .cfg.slack.existingSecret -}}
{{- .cfg.slack.existingSecret -}}
{{- else -}}
{{- include "openab.agentFullname" . -}}
{{- end -}}
{{- end }}
Looks good. Two observations:
- Nil-safety: The
and .cfg.slack .cfg.slack.existingSecretguard correctly handles the case where.cfg.slackis nil (short-circuit evaluation). Good defensive coding. - Minor concern — empty-string truthiness: In Go templates, an empty string
""is falsy, soexistingSecret: ""(the default) correctly falls through to theelsebranch. This is correct but worth a brief inline comment for future maintainers who might not know Go template truthiness rules. Non-blocking.
No issues found here.
2) Mixed-adapter Secret rendering
The dual-secret behavior (Slack from external, Discord from chart-managed) is correctly implemented across two files:
secret.yaml:
$hasSlacknow includes(not ($cfg.slack).existingSecret)— this correctly suppresses Slack keys from the chart-managed Secret whenexistingSecretis set.- The individual key guards (
{{- if and ($cfg.slack).enabled ($cfg.slack).botToken (not ($cfg.slack).existingSecret) }}) are belt-and-suspenders with$hasSlackbut provide clarity. Fine. - The top-level
{{- if or $hasDiscord $hasSlack $hasStt $hasGateway }}correctly skips Secret creation entirely when Slack is the only adapter andexistingSecretis set.
deployment.yaml:
- Render conditions expanded to
(or ($cfg.slack).botToken ($cfg.slack).existingSecret)— correctly injects env vars from whichever source is configured. secretKeyRef.namenow uses{{ include "openab.slackSecretName" $d }}while Discord still uses{{ include "openab.agentFullname" $d }}. This is the correct split.
One potential edge case to consider (non-blocking): If a user sets both existingSecret AND botToken/appToken, the chart silently ignores the inline tokens. The values.yaml comment says "When set, botToken and appToken above are ignored" which is documented, but there is no fail or warning emitted. This is consistent with how other charts (bitnami) handle it, so acceptable — just noting it.
3) Test coverage sufficiency
The 8 new tests cover:
| # | Scenario | Template | Verdict |
|---|---|---|---|
| 1 | existingSecret set + mixed adapter → no slack keys in Secret | secret.yaml | ✅ |
| 2 | Slack-only + existingSecret → no Secret rendered at all | secret.yaml | ✅ |
| 3 | Mixed adapter + existingSecret → discord key still present | secret.yaml | ✅ |
| 4 | existingSecret unset → chart-managed slack keys present | secret.yaml | ✅ |
| 5 | existingSecret set → SLACK_BOT_TOKEN refs external secret | deployment.yaml | ✅ |
| 6 | existingSecret set → SLACK_APP_TOKEN refs external secret | deployment.yaml | ✅ |
| 7 | existingSecret unset → env vars ref chart-managed secret name | deployment.yaml | ✅ |
| 8 | slack.enabled=false + existingSecret → no SLACK env vars | deployment.yaml | ✅ |
Coverage is good for the happy paths. Suggestions for additional coverage (non-blocking, could be follow-up):
- Mixed-adapter deployment test: Verify that in the Slack-existingSecret + Discord scenario,
DISCORD_BOT_TOKENstill references the chart-managed secret name (i.e., confirm the two secrets coexist in the same Deployment env). Currently test #3 only checkssecret.yaml, not the Deployment side of the mixed case. existingSecretwith custom key names: The chart hardcodesslack-bot-tokenandslack-app-tokenas the expected keys in the external Secret. Consider whether aexistingSecretKeys.botToken/existingSecretKeys.appTokenoverride would be useful for users whose external secrets use different key names (e.g.,SLACK_BOT_TOKENinstead ofslack-bot-token). This is a feature request, not a bug — but worth considering for a follow-up.
Summary
Clean, well-scoped change. Backwards compatible. The helper logic is correct, mixed-adapter rendering works as described, and test coverage is solid for the core scenarios. The one deployment-side mixed-adapter test gap is minor and non-blocking.
LGTM with the above non-blocking suggestions. 👍
chaodu-agent
left a comment
chaodu-agent
left a comment
There was a problem hiding this comment.
Review: existingSecret support for Slack agent credentials
Overall this is a clean, well-scoped change that follows established Helm idioms. A few observations on the three areas requested:
1) openab.slackSecretName helper logic (_helpers.tpl)
The helper is straightforward:
{{- if and .cfg.slack .cfg.slack.existingSecret -}}
{{- .cfg.slack.existingSecret -}}
{{- else -}}
{{- include "openab.agentFullname" . -}}
{{- end -}}
Concern — falsy-value handling: In Helm, and .cfg.slack .cfg.slack.existingSecret evaluates truthiness. If someone accidentally sets existingSecret: " " (whitespace) or a YAML anchor that resolves to a non-empty but semantically empty string, the helper will happily return it. Consider trimming:
{{- $es := (.cfg.slack.existingSecret | default "" | trim) -}}
{{- if $es -}}
{{- $es -}}
{{- else -}}
{{- include "openab.agentFullname" . -}}
{{- end -}}
This is a minor hardening suggestion, not a blocker — the current code works for all documented usage.
Nil-safety is good: The outer and .cfg.slack guard prevents nil-pointer panics when slack: is entirely absent from an agent config. 👍
2) Mixed-adapter Secret rendering
The dual-secret logic is correct:
secret.yaml:$hasSlacknow includes(not ($cfg.slack).existingSecret), so whenexistingSecretis set the Slack keys are excluded from the chart-managed Secret. The$hasDiscord/$hasStt/$hasGatewayconditions are untouched, so the chart Secret is still created for those adapters. Correct.deployment.yaml: The render condition(or ($cfg.slack).botToken ($cfg.slack).existingSecret)correctly fires the env var block from whichever source is configured, andopenab.slackSecretNameresolves the right Secret name. Correct.
One edge case to document (non-blocking): If a user sets both existingSecret and botToken/appToken, the existingSecret wins (tokens are ignored). This is stated in the README and values.yaml comments, which is good. However, the secret.yaml data-key guards ((not ($cfg.slack).existingSecret)) will suppress the chart-managed Slack keys even though botToken is non-empty. This means the chart-managed Secret won't contain stale/conflicting Slack data — correct behavior, just worth confirming it's intentional (it clearly is).
Potential improvement for a follow-up: The SLACK_BOT_TOKEN and SLACK_APP_TOKEN render conditions in deployment.yaml are now asymmetric with each other in a subtle way. The bot-token block checks (or ($cfg.slack).botToken ($cfg.slack).existingSecret) and the app-token block checks (or ($cfg.slack).appToken ($cfg.slack).existingSecret). When using existingSecret, both fire because existingSecret is truthy. But if someone sets existingSecret and the external Secret only contains slack-bot-token (missing slack-app-token), the pod will fail at runtime with a missing key error. Consider adding a note in values.yaml that the external Secret must contain both keys. (The current comment does say this — just confirming it is the right call.)
3) Test coverage sufficiency
The 8 new tests cover:
| Scenario | Template | Verdict |
|---|---|---|
| existingSecret set + mixed adapter: no slack keys in Secret | secret.yaml | Good |
| Slack-only + existingSecret: no Secret rendered at all | secret.yaml | Good |
| Mixed adapter + existingSecret: discord key still present | secret.yaml | Good |
| Baseline (no existingSecret): slack keys present | secret.yaml | Good |
| existingSecret: SLACK_BOT_TOKEN refs external Secret | deployment.yaml | Good |
| existingSecret: SLACK_APP_TOKEN refs external Secret | deployment.yaml | Good |
| No existingSecret: refs chart-managed Secret name | deployment.yaml | Good |
| slack.enabled=false + existingSecret: no SLACK env vars | deployment.yaml | Good |
Gaps (nice-to-have, non-blocking):
- Multi-agent test: All tests use a single agent (
kiro). A test with two agents where one usesexistingSecretand the other uses inline tokens would confirm no cross-agent interference in Secret rendering. existingSecretset to empty string explicitly: Verifying thatexistingSecret: ""behaves identically to the field being absent (it should, since empty string is falsy in Go templates, but an explicit test would be defensive).- Mixed adapter deployment test: A test asserting that in the mixed case (Slack from external, Discord from chart-managed), the same Deployment has
DISCORD_BOT_TOKENreferencing the chart-managed Secret name ANDSLACK_BOT_TOKENreferencing the external Secret name — in one assertion block.
Verdict
LGTM with minor suggestions. The whitespace-trim hardening in the helper and the multi-agent test are nice-to-haves. The core logic is sound, backwards-compatible, and well-tested. Good to merge as-is or with the minor tweaks above.
…er and multi-agent tests - Pipe existingSecret through | trim in openab.slackSecretName helper to handle whitespace-only values gracefully - Add mixed-adapter deployment test verifying Discord refs chart-managed Secret while Slack refs existingSecret in the same Deployment - Add multi-agent scoping test confirming agent A's existingSecret does not affect agent B's inline token resolution
chaodu-agent
left a comment
chaodu-agent
left a comment
There was a problem hiding this comment.
Re-approving after nit-fix commit (95e04d5). Changes since last review:
existingSecretnow piped through| trimin the helper (handles whitespace-only edge case)- Added mixed-adapter deployment test (Discord chart-managed + Slack external in same Deployment)
- Added multi-agent scoping test (agent A's existingSecret doesn't affect agent B)
All 107 helm-unittest tests pass. LGTM ✅
This comment was marked as outdated.
This comment was marked as outdated.
|
@chaodu-agent 最終 review 結果： PR #901 — Post-Nit-Fix Review ✅已確認 commit 已修復
驗證結果
結論所有 review feedback 已 addressed。PR ready to merge，只差 thepagent (code owner) 的 approve。 請 re-approve 🙏 |
…evel (#914) * feat(chart): add serviceAccountName support at per-agent and global level Per-agent value (agents.<name>.serviceAccountName) wins when set; otherwise falls back to chart-global $.Values.serviceAccountName. Both empty preserves current behaviour (no serviceAccountName rendered, Kubernetes uses cluster default SA). This is required to activate IRSA on EKS — without an explicit serviceAccountName, the pod-identity-webhook never injects AWS credentials and workloads silently fall back to the broad EC2 node role, breaking least-privilege. Scope: string reference to an existing SA only. The chart does NOT create a new SA or manage IRSA annotations (operators provision out-of-band via Terraform / IDP / kubectl), matching how PR #901 (existingSecret) and #910 (imagePullSecrets) reference existing K8s resources rather than creating them. Closes #913 * docs: generalize serviceAccountName descriptions, remove EKS/IRSA-specific wording --------- Co-authored-by: chaodu-agent[bot] <chaodu-agent[bot]@users.noreply.github.com>
Per-agent value (agents.<name>.imagePullSecrets) wins when set; otherwise falls back to chart-global $.Values.imagePullSecrets. Both empty preserves current behaviour (no imagePullSecrets rendered). This enables multi-agent deployments where only some agents pull from a private registry without forcing pull credentials onto every pod. Follows the same per-agent K8s-native secrets pattern as PR openabdev#901 (slack existingSecret). Closes openabdev#910
…el (#911) Per-agent value (agents.<name>.imagePullSecrets) wins when set; otherwise falls back to chart-global $.Values.imagePullSecrets. Both empty preserves current behaviour (no imagePullSecrets rendered). This enables multi-agent deployments where only some agents pull from a private registry without forcing pull credentials onto every pod. Follows the same per-agent K8s-native secrets pattern as PR #901 (slack existingSecret). Closes #910
4 participants
What problem does this solve?
In GitOps environments using External Secrets Operator (ESO), Vault, or SealedSecrets, Kubernetes Secrets are populated by the operator from an external store. The
openabchart already supports this pattern forANTHROPIC_API_KEYviasecretEnv(rendered asvalueFrom.secretKeyRef).Slack tokens currently cannot follow the same path. Because the chart creates its own Secret from Helm values,
SLACK_BOT_TOKENandSLACK_APP_TOKENmust be passed athelm install/helm upgradetime via--set-stringorhelm_release_set_sensitive. Every token rotation requires a Helm re-apply, blocking automation and making platform engineers a bottleneck for what should be a self-service operation.Closes #900
Discord Discussion URL: https://discord.com/channels/1491295327620169908/1491365158868619404/1507286287428878348
At a Glance
Prior Art & Industry Research
Not applicable — this is a Helm chart credential-injection change, not an architectural/runtime/scheduling/persistence change. The pattern itself (
existingSecretfield that toggles between chart-managed and externally-managed Secrets) is the standard Helm idiom used by community charts such asbitnami/*,prometheus-community/*, andexternal-secrets/external-secrets. Theopenab-telegramchart (#873) already implements this same pattern within this repo.Proposed Solution
Add
agents.<name>.slack.existingSecretto theopenabchart. When set, the chart references the named Kubernetes Secret forSLACK_BOT_TOKENandSLACK_APP_TOKENinstead of creating a chart-managed Secret from Helm values.Behavior:
existingSecretset?slack-bot-token,slack-app-token(unchanged)existingSecretdirectlyexistingSecretfor Slack keysFiles changed:
charts/openab/values.yaml— newagents.<name>.slack.existingSecret: ""fieldcharts/openab/templates/secret.yaml— guardslack-bot-token/slack-app-tokenentries with(not .existingSecret); updated$hasSlackso a slack-only agent withexistingSecretskips Secret creation entirelycharts/openab/templates/_helpers.tpl— newopenab.slackSecretNamehelper returningexistingSecretor the chart-managed agent fullnamecharts/openab/templates/deployment.yaml— Slack env vars use the new helper forsecretKeyRef.name; render condition expanded to fire when eitherbotToken/appTokenorexistingSecretis setcharts/openab/tests/slack-existing-secret_test.yaml— new helm-unittest suite (8 tests)charts/openab/README.md— new value documentedWhy this approach?
Scoped per-agent under
agents.<name>.slack.existingSecret(not chart-level, not agent-level). Theopenabchart is multi-agent and credentials are already namespaced per-adapter (slack.botToken,discord.botToken, etc.). PlacingexistingSecretunderslackkeeps it consistent with existing field locations and lets users with mixed-adapter agents manage each credential family independently.Dual-secret behavior is intentional. When an agent uses both Slack (via
existingSecret) and Discord (viabotToken), the chart creates one Secret for Discord and the Deployment references the external Secret for Slack. This is consistent with the pattern where each adapter's credentials are independently sourced.Backwards compatible.
existingSecretdefaults to"". No change in behavior for any existing deployment.Alternatives Considered
Agent-level
agents.<name>.existingSecret— would replace all five credential keys (discord-bot-token,slack-bot-token,slack-app-token,stt-api-key,gateway-ws-token) in a single Secret. Rejected because it forces all credentials into one external Secret, blocking the common case where Slack tokens come from a different store (e.g. team-owned AWS SM path) than Discord/STT tokens (e.g. platform-owned Vault path).Chart-level
existingSecret— mirroring theopenab-telegramchart exactly. Rejected becauseopenab-telegramis single-agent and chart-level was natural there;openabis multi-agent and chart-level would force all agents to share one credential Secret.secretEnvonly, withoutexistingSecret—secretEnvalready supportsvalueFrom.secretKeyRef, so a user could in theory configure Slack env vars viasecretEnv. Rejected because (a)secretEnvis documented for inheritable env vars and adds keys toinherit_envinconfig.toml, which is not appropriate forSLACK_BOT_TOKEN(it's asecret_keyrefin the adapter config, not an inherited env), and (b) theexistingSecretpattern is more discoverable for users familiar with the rest of the Helm ecosystem.Validation
Helm chart changes:
helm lint charts/openabpasseshelm templaterenders correctly for all three cases (existingSecret unset, existingSecret set with slack-only agent, existingSecret set with mixed Slack+Discord agent)helm unittest charts/openab— 105/105 pass (97 baseline + 8 new tests covering bothsecret.yamlanddeployment.yamlrendering)Manual
helm templateverification: