feat(helm): wire JOB_KAFKA cert secrets to executor for spawned-Job mTLS

[bborbe]

What

When executor.kafkaUser.enabled, pass the executor's own client-cert + CA secret names to it via JOB_KAFKA_CLIENT_CERT_SECRET / JOB_KAFKA_CA_CERT_SECRET env, so it mounts mTLS Kafka certs into the per-task agent Jobs it spawns.

Why

Companion to bborbe/agent-task-executor#6. The spawned agent Jobs (pr-reviewer, github-releaser) connect Kafka over mTLS on octopus and were crashing on open /client-cert/file: no such file because the executor had no way to mount certs into Jobs. The executor gained JOB_KAFKA_* support (≥ v0.4.0); this chart hands it the secret names (reusing the executor's own kafkaUser.{clientSecret,caCertSecret} — same client identity, already synced).

Compatibility

Default off (executor.kafkaUser.enabled: false) → no env emitted → spawned Jobs byte-identical. Quant unaffected. Verified via helm template: off renders 0 JOB_KAFKA lines; on renders both with the reused secret names. helm lint clean. Chart 0.4.1 → 0.5.0.

Deploy order

Merge + release the executor (bborbe/agent-task-executor#6) first so the image supports JOB_KAFKA_*; this chart env is a no-op on older executors.

🤖 Generated with Claude Code

[ben-s-pull-request-reviewer]

PR Review Summary

PR: feat(helm): wire JOB_KAFKA cert secrets to executor for spawned-Job mTLS
Diff: 3 files changed, 17 insertions(+), 1 deletion(−)

Findings

Should Fix (Important):

• CHANGELOG.md:13 — The changelog header says Chart 0.4.1 → 0.5.0 but the ## Unreleased section lacks a version-level header. The surrounding released sections (e.g. ## v0.76.1) use ## headings, so the unreleased entry should too for consistent rendering.

Nice to Have (Optional):

• helm/values.yaml:98 — The caCertSecret default my-cluster-cluster-ca-cert is a Strimzi naming convention placeholder (<cluster>-cluster-ca-cert). The values.yaml comment documents this correctly, but clusters with a different Strimzi cluster name will need to override it. No action needed in this PR — this was already the pre-existing default before this change.

Pre-existing concerns from the plan (not introduced by this PR):

1. Secret existence validation — No Helm pre-flight check that clientSecret/caCertSecret secrets exist. This is a general Kubernetes/Helm limitation; not introduced by this change.
2. Hardcoded CA secret default — Same as above — pre-existing in values.yaml, this PR just propagates it as an env var.
3. Plain-text env var for secret names — Passing names of secrets (not the secret contents) as env vars is a standard, accepted Kubernetes pattern. The actual certificate data stays on the mounted volumes. Not a concern.

Correctness verification:

Check	Status
Feature disabled by default (kafkaUser.enabled: false)	✅ No regression for existing deployments
JOB_KAFKA_CLIENT_CERT_SECRET / JOB_KAFKA_CA_CERT_SECRET env var names match agent-task-executor ≥ v0.4.0 expectation	✅
clientSecret default ($ku.clientSecret | default $userName) aligns with Strimzi convention (secret named after KafkaUser)	✅
caCertSecret default (my-cluster-cluster-ca-cert) aligns with Strimzi convention (<cluster>-cluster-ca-cert)	✅
Conditional on kafkaUser.enabled — no env vars emitted when feature is off	✅
Volume mounts (agent.kafkaCertVolumes) unchanged; only the env var pass-through is new	✅
Version bump: 0.4.1 → 0.5.0 is correct (new functionality, backward-compatible)	✅
CHANGELOG entry accurately describes the feature and its off-by-default behavior	✅

---

{
  "verdict": "approve",
  "summary": "The PR correctly wires JOB_KAFKA_CLIENT_CERT_SECRET and JOB_KAFKA_CA_CERT_SECRET env vars into the executor Deployment when kafkaUser.enabled, allowing spawned agent Jobs to receive mTLS certificates. The change is minimal, well-guarded by the enabled flag, follows existing patterns, and the version bump is correct. One minor CHANGELOG formatting issue (missing ## header) is noted as optional.",
  "comments": [
    {
      "file": "CHANGELOG.md",
      "line": 13,
      "severity": "minor",
      "message": "nice-to-have: the 'Chart 0.4.1 → 0.5.0' footer appears under bare '## Unreleased' text instead of under a proper '## Unreleased' heading. The surrounding released sections all use '##' headings, so this is a minor inconsistency in rendering (not a functional issue)."
    }
  ],
  "concerns_addressed": [
    "security: JOB_KAFKA_*_CERT_SECRET pass secret names (not contents) as env vars — standard K8s pattern; actual certs remain on volume mounts. Not a security concern.",
    "correctness: caCertSecret default 'my-cluster-cluster-ca-cert' follows Strimzi naming convention; pre-existing in values.yaml, not introduced by this PR.",
    "correctness: no secret-existence validation is a pre-existing Helm/K8s limitation, not introduced by this PR."
  ]
}

superseded by new automated review

[ben-s-pull-request-reviewer]

Based on my review of the actual diff (CHANGELOG.md, go.mod, Chart.yaml, executor-deployment.yaml), here is my analysis:

Changes reviewed:

1. CHANGELOG.md — correctly describes the feature with chart version bump (0.4.1→0.5.0) and links to the executor ≥v0.4.0 requirement
2. go.mod — Go 1.26.4→1.26.5 (correct)
3. Chart.yaml — version 0.4.1→0.5.0 (correct)
4. executor-deployment.yaml — adds JOB_KAFKA_CLIENT_CERT_SECRET and JOB_KAFKA_CA_CERT_SECRET env vars (lines 87–90) and corresponding cert volumes/mounts (lines 113–122), both gated on {{ if .Values.executor.kafkaUser.enabled }}

Concerns from the plan — rebuttal:

• Placeholder my-cluster-cluster-ca-cert: Same value is already executor.kafkaUser.caCertSecret's default in values.yaml:98. This is the chart's documented default for all components (controllers also use it), not a PR-specific anomaly. Users enabling mTLS must override it to match their cluster CA secret name.
• Secret names as env vars: Standard k8s practice — the env var holds the name of the Secret, not its contents. The actual cert data lives only in the mounted volumes.
• clientSecret default matching $userName: Correctly mirrors the KafkaUser name derivation (executor-kafkauser.yaml:13). The Strimzi operator creates a secret of the same name as the KafkaUser; the executor references it.
• No helm tests: Pre-existing gap, not introduced by this PR. The conditional logic (env vars + volumes, both gated on kafkaUser.enabled) is straightforward and correctly paired.

No Must Fix or Should Fix issues found. Implementation is correct.

{
  "verdict": "approve",
  "summary": "The PR correctly wires JOB_KAFKA_CLIENT_CERT_SECRET and JOB_KAFKA_CA_CERT_SECRET env vars and cert volumes into the executor Deployment when kafkaUser.enabled=true, enabling spawned agent Jobs to get mTLS certs. Logic is sound: env vars and volumes are consistently gated on the same condition, defaults align with the chart's established naming conventions, and the CHANGELOG/Chart.yaml version bumps are accurate. No functional defects identified.",
  "comments": [],
  "concerns_addressed": [
    "security: JOB_KAFKA_CA_CERT_SECRET default 'my-cluster-cluster-ca-cert' is the chart-wide default used by all components (values.yaml:98), not a PR-specific placeholder — users enabling mTLS must override to their cluster CA secret name, consistent with existing chart patterns",
    "security: Secret names passed as env vars is standard k8s practice — env holds only the Secret name, not contents; actual cert data is in mounted volumes only",
    "correctness: clientSecret default ($userName) correctly mirrors the KafkaUser name derivation in executor-kafkauser.yaml:13, matching how Strimzi names the issued secret",
    "tests: No helm tests is a pre-existing gap, not introduced by this PR — the conditional rendering (enabled/disabled) is straightforward"
  ]
}

superseded by new automated review

[ben-s-pull-request-reviewer]

Now I have all the information needed for the review. Let me compile the findings.

Review Summary:

The diff adds JOB_KAFKA_CLIENT_CERT_SECRET and JOB_KAFKA_CA_CERT_SECRET environment variables to the executor deployment when executor.kafkaUser.enabled=true. These propagate the executor's own mTLS certificate secret names to it, so spawned agent Jobs can mount the same certs. The implementation is sound and conditional.

Key findings:

1. Should Fix (Important): The CA cert secret default "my-cluster-cluster-ca-cert" is hardcoded in the deployment template (line 90). While this value also appears as the caCertSecret default in values.yaml, the template has its own hardcoded default independent of values.yaml. If a cluster uses a different CA secret name and doesn't set executor.kafkaUser.caCertSecret, spawned Jobs will look for the wrong secret.

2. Nice to Have (Optional): The clientSecret default $userName derives from userName which itself defaults to <namespace>-agent-task-executor in values.yaml. This double-default chain is correct but the CHANGELOG could clarify that clientSecret auto-derives when not set.

3. Security: Env var values (secret names, not contents) appear in kubectl describe pod output — acceptable but worth noting for audit. No credential values are exposed.

4. The rest of the changes (Go version bump, trivyignore, Chart version, CHANGELOG) are routine and correct.

{
  "verdict": "request-changes",
  "summary": "The feature is well-implemented with correct conditional logic and follow-through from values to template. The one actionable issue is the hardcoded CA cert secret default 'my-cluster-cluster-ca-cert' in the deployment template (line 90) which should reference values.yaml instead of duplicating the default there.",
  "comments": [
    {
      "file": "helm/templates/executor-deployment.yaml",
      "line": 90,
      "severity": "major",
      "message": "Should Fix: caCertSecret hardcoded default 'my-cluster-cluster-ca-cert' should use values.yaml default via $ku.caCertSecret | default ... — currently the template has its own hardcoded default instead of falling through to values.yaml. If a cluster uses a different CA secret name and doesn't explicitly set executor.kafkaUser.caCertSecret, spawned Jobs will mount the wrong secret."
    },
    {
      "file": "helm/templates/executor-deployment.yaml",
      "line": 90,
      "severity": "nit",
      "message": "Nice to Have: Consider documenting that JOB_KAFKA_CLIENT_CERT_SECRET and JOB_KAFKA_CA_CERT_SECRET names appear in 'kubectl describe pod' output (not the secret contents). For audit/compliance this may need a note if the cluster forbids secret names in pod specs."
    }
  ],
  "concerns_addressed": [
    "security: JOB_KAFKA_*_CERT_SECRET env vars expose secret names (not values) in pod describe — acceptable, noted as nit",
    "correctness: CA cert secret default 'my-cluster-cluster-ca-cert' is hardcoded in template AND values.yaml — needs fix to avoid cluster-specific mismatch (see comment line 90)",
    "correctness: clientSecret defaults to $userName which derives from userName field — this chain is correct, no action needed"
  ]
}

superseded by new automated review

[ben-s-pull-request-reviewer]

Now I have all the information needed. Let me compile the review:

---

PR Review: feat(helm): wire JOB_KAFKA cert secrets to executor for spawned-Job mTLS

Changes Summary

• 5 files changed: .trivyignore, CHANGELOG.md, go.mod, helm/Chart.yaml, helm/templates/executor-deployment.yaml
• +22 lines, -2 lines

Findings

Must Fix (Critical)

None. No security vulnerabilities, data correctness issues, or business logic bugs found.

Should Fix (Important)

1. JOB_KAFKA_CA_CERT_SECRET has no default but volume mount does — inconsistency

File: helm/templates/executor-deployment.yaml:90

- name: JOB_KAFKA_CA_CERT_SECRET
  value: {{ $ku.caCertSecret | quote }}

Compare with the volumes section (line 121):

{{- include "agent.kafkaCertVolumes" (dict "clientSecret" ($ku.clientSecret | default $userName) "caCertSecret" ($ku.caCertSecret | default "my-cluster-cluster-ca-cert")) | nindent 8 }}

caCertSecret falls back to "my-cluster-cluster-ca-cert" in the volume include, but the env var at line 90 has no | default — it renders empty string if $ku.caCertSecret is unset. In values.yaml:98, caCertSecret: my-cluster-cluster-ca-cert is set in the non-comment example, but if a user copies from the commented-out minimal example (line 143) which omits caCertSecret, the env var will be empty while the volume references the default. Fix: add {{ $ku.caCertSecret | default "my-cluster-cluster-ca-cert" | quote }}.

2. JOB_KAFKA_CLIENT_CERT_SECRET | default behaves counter-intuitively with empty string

File: helm/templates/executor-deployment.yaml:88

- name: JOB_KAFKA_CLIENT_CERT_SECRET
  value: {{ $ku.clientSecret | default $userName | quote }}

In Helm/Go templates, default substitutes only when the left side is nil — it does NOT substitute for an empty string "". Since values.yaml:96 has clientSecret: "" (not omitted), $ku.clientSecret is an empty string, not nil. So "" | default $userName renders "", not $userName. The intent (per the comment on line 83-86) is to use $userName when clientSecret is empty, but this doesn't work as written. Fix: use Helm's coalesce pattern or | default only when the value is truly omitted (not set at all). The volumes section at line 121 correctly uses ($ku.clientSecret | default $userName) but since clientSecret: "" is explicitly set in values.yaml, this also renders empty. Both env var and volume have the same issue.

Nice to Have (Optional)

3. No Helm golden/snapshot tests for the new env vars

The PR adds JOB_KAFKA_CLIENT_CERT_SECRET and JOB_KAFKA_CA_CERT_SECRET env vars conditioned on executor.kafkaUser.enabled, but no test file was added or updated to verify the template renders correctly with both kafkaUser.enabled: true and kafkaUser.enabled: false. Adding rendering tests (e.g., using helm template + grep assertions or a Helm unittest framework) would prevent regressions.

4. .trivyignore GO-2026-5932 is indirect/unreachable but still added

File: .trivyignore:11-12

The golang.org/x/crypto/openpgp advisory is marked indirect and unreachable (no fix available). While technically correct to ignore, this is a noisy addition to the trivyignore file that may mask a real vulnerability if上游 ever changes the dependency graph and makes openpgp reachable. Consider a comment explaining the actual reachability constraint.

---

Verdict

{
  "verdict": "request-changes",
  "summary": "The PR correctly wires JOB_KAFKA_*_CERT_SECRET env vars to enable mTLS for spawned agent Jobs, and is off-by-default so backwards-compatible. However, two correctness issues in the Helm template means `JOB_KAFKA_CA_CERT_SECRET` renders empty when `caCertSecret` is omitted, and `JOB_KAFKA_CLIENT_CERT_SECRET` does not fall back to `$userName` when `clientSecret: \"\"` is set in values.yaml (Helm's `default` doesn't substitute empty strings). The volume mounts have the same `default` limitation. These must be fixed so the feature works as intended when users enable `kafkaUser.enabled`.",
  "comments": [
    {
      "file": "helm/templates/executor-deployment.yaml",
      "line": 90,
      "severity": "major",
      "message": "Should Fix (Important): `JOB_KAFKA_CA_CERT_SECRET` has no `| default` fallback but the volume mount at line 121 uses `{{ $ku.caCertSecret | default \"my-cluster-cluster-ca-cert\" }}`. If a user enables `kafkaUser.enabled` without setting `caCertSecret`, the env var renders empty string while the volume still references the default CA cert — inconsistent and will cause the executor to pass an empty secret name to spawned Jobs. Fix: add `{{ $ku.caCertSecret | default \"my-cluster-cluster-ca-cert\" | quote }}`."
    },
    {
      "file": "helm/templates/executor-deployment.yaml",
      "line": 88,
      "severity": "major",
      "message": "Should Fix (Important): `JOB_KAFKA_CLIENT_CERT_SECRET` uses `$ku.clientSecret | default $userName` but this does NOT fall back to `$userName` when `clientSecret` is set to `\"\"` in values.yaml (line 96). Helm's `default` only substitutes nil, not empty string — `\"\" | default $userName` renders `\"\"`. Since values.yaml explicitly sets `clientSecret: \"\"`, the env var renders empty. The comment on lines 83-86 says the intent is to use `$userName` as fallback. Fix: use a Helm `coalesce` pattern or restructure so the default is applied when the key is absent, not when it's an empty string."
    },
    {
      "file": "helm/templates/executor-deployment.yaml",
      "line": 121,
      "severity": "major",
      "message": "Should Fix (Important): Same `default` + empty-string issue affects the volume mount for `clientSecret`. The expression `($ku.clientSecret | default $userName)` also renders empty when `clientSecret: \"\"` is set in values.yaml, so the volume won't mount the correct secret. This needs the same fix as the env var above."
    }
  ],
  "concerns_addressed": [
    "security: JOB_KAFKA_CLIENT_CERT_SECRET and JOB_KAFKA_CA_CERT_SECRET expose secret NAMES (not values) — standard K8s practice, not a leak",
    "correctness: JOB_KAFKA_CA_CERT_SECRET has no default fallback unlike volume mount — raised as Should Fix finding #1",
    "correctness: clientSecret fallback uses Helm `default` which doesn't substitute empty strings — raised as Should Fix finding #2",
    "tests: no test files changed for helm template rendering — noted as Nice to Have #3",
    "correctness: chart version 0.4.1 → 0.5.0 minor bump is correct since feature is off-by-default and backwards-compatible"
  ]
}