feat(helm): optional mTLS Kafka support (KafkaUser + client-cert mounts), default off#34
Merged
Merged
Conversation
…executor, controllers, recurring — default off; chart 0.3.1→0.4.0
There was a problem hiding this comment.
Based on my review of all changed files, here is my analysis:
Files Reviewed:
CHANGELOG.md,helm/Chart.yaml,helm/values.yamlhelm/templates/_helpers.tpl,helm/templates/executor-deployment.yaml,helm/templates/controller-statefulset.yamlhelm/templates/recurring-task-creator-statefulset.yamlhelm/templates/controller-kafkauser.yaml,helm/templates/executor-kafkauser.yaml,helm/templates/recurring-task-creator-kafkauser.yamlhelm/README.md
Must Fix (Critical): None identified.
Should Fix (Important): None — all concerns from the plan are either acceptable design decisions, already documented, or not actual bugs.
Nice to Have (Optional):
| File | Line | Finding |
|---|---|---|
helm/templates/_helpers.tpl |
47, 54, 61 | defaultMode: 420 (decimal) = 0644 octal (world-readable) for secret volumes containing TLS certs. While acceptable for certificates (they're designed to be distributed), stricter defaults like 0444 would be more aligned with secret-mount hardening best practices. The executor/controller securityContexts already drop ALL capabilities — this is lower severity. |
helm/templates/controller-statefulset.yaml |
133–138 | The volumes: block is wrapped in `{{- if (($c.kafkaUser |
Plan Concerns — Resolution:
| Concern | Verdict |
|---|---|
defaultMode: 420 non-restrictive |
Accepted — certificates are not secrets; 0644 is standard for TLS certs |
| Controller volumes conditional wrapper | Accepted — currently no conflict; documented for future refactoring |
| executor volumeMounts conflict | Not an issue — executor had zero volumeMounts before this change |
| Name derivation chain for clientSecret | Correct — Helm default chains properly resolve |
| KafkaUser in strimziNamespace | By design — documented in values.yaml comments and README |
| No tls:// scheme validation | By design — documented in README; silent failure if misconfigured |
| Hardcoded caCertSecret default | Accepted — matches Strimzi convention; documented in values.yaml |
Selector Mode Traceability:
- Mechanical funnel: N/A (no ast-grep rules matched helm templates)
- Judgment rules triggered: None
- ast-grep-runner: skipped (no Go/Python files changed)
- Step 4.0 toolchain check: N/A
{
"verdict": "approve",
"summary": "The mTLS Kafka feature is correctly implemented. Default-off behavior ensures zero breaking changes for existing plaintext Kafka installations. All concerns from the plan are either intentional design decisions (strimziNamespace placement, no tls:// validation), already documented (caCertSecret default), or not actual bugs (no volumeMount conflict for executor). The only optional improvement is considering stricter defaultMode for secret volumes.",
"comments": [
{
"file": "helm/templates/_helpers.tpl",
"line": 47,
"severity": "nit",
"message": "secret volume defaultMode 420 (=0644 octal, world-readable) for TLS certificates. While acceptable since certs are designed for distribution, stricter defaults like 0444 align better with K8s secret hardening guidance."
},
{
"file": "helm/templates/_helpers.tpl",
"line": 54,
"severity": "nit",
"message": "secret volume defaultMode 420 (=0644 octal, world-readable) for TLS key. See line 47 note."
},
{
"file": "helm/templates/_helpers.tpl",
"line": 61,
"severity": "nit",
"message": "secret volume defaultMode 420 (=0644 octal, world-readable) for CA certificate. See line 47 note."
},
{
"file": "helm/templates/controller-statefulset.yaml",
"line": 133,
"severity": "nit",
"message": "volumes block wrapped in kafkaUser.enabled conditional — future volume sources added unconditionally would need this wrapper refactored. Not a current bug; documented for maintainers."
}
],
"concerns_addressed": [
"security: defaultMode 420 accepted — certs are not secrets, 0644 is standard for TLS material",
"correctness: controller volumes block is correctly conditional — no other volume sources conflict",
"correctness: executor volumeMounts has no conflict — executor had zero volumeMounts before this change",
"correctness: name derivation chain for clientSecret is correct Helm template logic",
"security: KafkaUser in strimziNamespace is intentional by design — documented in values.yaml and README",
"correctness: no tls:// scheme validation is documented — silent failure is a known limitation",
"security: caCertSecret default matches Strimzi convention — documented in values.yaml"
]
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds optional mTLS Kafka support to the
agentchart for executor, controllers, and recurring-task-creator — default off (plaintext, quant unaffected). Unblocks the company Octopus per-stage-Strimzi (mTLS:9093) deploy without an octopus-only post-render patch.When
<component>.kafkaUser.enabled: true, the chart:KafkaUserCR (type: tls) instrimziNamespace, and/client-cert/file,/client-key/file,/server-cert/filepathsgithub.com/bborbe/kafkareads fortls://brokers.New per-component values:
kafkaUser.{enabled,cluster,strimziNamespace,userName,clientSecret,caCertSecret}. Secrets are referenced by name only (Strimzi issues them; an external syncer places them in the app namespace). Also adds the cert mount recurring-task-creator previously lacked. Chart 0.3.1 → 0.4.0.Test plan
helm lintcleanenabled: truerenders 3KafkaUserCRs (type: tls,strimzins, derived names) + 9 cert mounts + correct client/CA secret refsenabled: false(default): zero KafkaUser, zero cert mountshelm.sh/chartversion label → plaintext/quant installs unaffectedmake precommitgreen