add controller, leaf agents, Config CRD, and recurring-task-creator to helm chart#30
Conversation
![ben-s-pull-request-reviewer[bot]](https://avatars.githubusercontent.com/u/1193284?s=60&v=4){kind=small}
There was a problem hiding this comment.
Based on the diff captured in Step 0c, I have enough information to perform the review. Let me now compile the consolidated findings.
PR Review: add controller, leaf agents, Config CRD, and recurring-task-creator to helm chart
Changed Files Reviewed
CHANGELOG.md— version header + changelog bulletshelm/Chart.yaml— version 0.1.0 → 0.2.0, new sourceshelm/crds/config-crd.yaml— new Config CRDhelm/templates/_helpers.tpl— 3 new helper functionshelm/templates/agents.yaml— new multi-document agent templateshelm/templates/controller-secret.yaml— new controller Secrethelm/templates/controller-service.yaml— new controller Servicehelm/templates/controller-statefulset.yaml— new controller StatefulSethelm/templates/recurring-task-creator-kafkauser.yaml— new Strimzi KafkaUserhelm/templates/recurring-task-creator-statefulset.yaml— new recurring-task-creatorhelm/values.yaml— newcontroller,agents,recurringTaskCreatorblocks
MECHANICAL FUNNEL (ast-grep-runner) — skipped (ast-grep not available in this environment)
MANUAL REVIEW FINDINGS
Must Fix (Critical)
1. helm/templates/controller-secret.yaml — base64 encoding includes spurious quotes
The template uses {{ .Values.controller.sentry.dsn | b64enc | quote }}. The | quote after b64enc wraps the base64 output in double-quote characters. The resulting Secret value is "ZHVtbXk=" (with literal quote characters), not ZHVtbXk=. The application reading this secret via secretKeyRef will receive the quoted string including " characters, causing deserialization failures. The same bug exists for gateway-secret.
data:
sentry-dsn: {{ .Values.controller.sentry.dsn | b64enc | quote }} # BUG: quotes included
gateway-secret: {{ .Values.controller.gatewaySecret | b64enc | quote }} # BUG: sameFix: Remove | quote — b64enc already produces a valid string value for Secret.data.
2. helm/templates/recurring-task-creator-statefulset.yaml — tmp volume defined but never mounted, will cause write failures
The pod spec.volumes declares a tmp emptyDir volume, and the container securityContext sets readOnlyRootFilesystem: true + fsGroup: 65534 (expecting the tmp volume to be group-writable at /tmp). However, there is no volumeMounts entry for this tmp volume in the container. Writes to /tmp will hit the read-only rootfs and fail.
The diff fragment showing the StatefulSet container ends with:
volumeMounts:
- name: datadir
mountPath: /dataThe tmp volume is not mounted. The fsGroup: 65534 + readOnlyRootFilesystem: true combination is only safe when /tmp is an explicit emptyDir mount — without it, the container will fail on first write to /tmp.
Fix: Add volumeMounts: - name: tmp mountPath: /tmp to the recurring-task-creator container.
3. helm/templates/controller-statefulset.yaml — tmp volume defined but never mounted (dead code / potential future bug)
Same pattern: the pod spec.volumes has a tmp emptyDir volume, but the controller container only mounts datadir. Unlike recurring-task-creator, the controller does not set readOnlyRootFilesystem: true, so this doesn't cause an immediate failure — but the tmp volume is unused dead code that adds confusion.
Should Fix (Important)
4. helm/values.yaml — controller.storage.storageClassName: local-path is unsuitable for multi-node clusters
local-path is a Local PV provisioner — it only works on single-node clusters (or nodes with the Local Path Operator). Production clusters that run multiple nodes will fail to provision the BoltDB PVC. The comment says "local-path suits single-node / quant; override elsewhere" — but there is no validation preventing installation on a multi-node cluster without changing this value.
Fix: Change the default to "" (empty = use the cluster's default StorageClass) and add a comment warning that local-path requires the Local Path Operator.
5. helm/templates/recurring-task-creator-statefulset.yaml — Strimzi namespace convention hardcoded
strimziNamespace: strimzi is a common convention but not universal. If the Strimzi operator runs in a different namespace (e.g., strimzi-operators), the KafkaUser is created in the wrong namespace and mTLS auth will silently fail. The KafkaUser's metadata.namespace is set to this value, while the app namespace is only referenced via labels — a namespace mismatch would not produce an error at install time.
Fix: Add a comment noting the assumption, or make the default match the most common production setup.
6. helm/values.yaml — controller.gitRestUrl derivation assumes vault-obsidian-{vaultName} naming
The helper produces http://vault-obsidian-{vaultName}:9090. This is a specific internal service naming convention that is not universal. The comment says "in-cluster obsidian git-rest service" — if the service is named differently (e.g., git-rest, vault-git-rest, or in a different namespace), the controller will fail to reach it with no warning at install time. Only vaultName is required; there is no validation of the derived URL.
Fix: Document the naming assumption prominently in values.yaml comments.
7. helm/templates/recurring-task-creator-statefulset.yaml — dryRun: "false" is a string, not a boolean
dryRun: "false" in values.yaml produces the string "false", not the boolean false. In Go (and most languages), os.Getenv("DRY_RUN") == "false" may not behave as expected depending on how the application parses it. Many CLI libraries treat "false" (string) as falsy, but this is fragile and depends on the application's parsing logic.
Fix: Document in values.yaml whether the application expects a string or boolean, or add a comment explaining the string-based contract.
Nice to Have (Optional)
8. helm/templates/recurring-task-creator-kafkauser.yaml — KafkaUser in a different namespace than the app
The KafkaUser is created in strimziNamespace (default strimzi) which is typically a different namespace from where the recurring-task-creator runs. RBAC labels (strimzi.io/cluster) are correct, but standard Kubernetes tooling (e.g., kubectl get kafkausers -n <app-ns>) won't find it, which can be confusing during debugging.
Precommit
precommit skipped (selector mode) — CI covers lint+test.
Selector Mode Traceability
- Rules triggered (judgment tier): None applicable — helm chart changes are outside ast-grep rule scope.
- Funnel result: ast-grep unavailable in this environment; mechanical checks skipped.
- Adjudication: In-session, using diff content + helm conventions knowledge.
Next Steps
- Fix the
b64enc | quotebug in both secret templates (critical — creates malformed Secrets) - Mount the
tmpvolume in recurring-task-creator container (critical — causes write failures with readOnlyRootFilesystem) - Consider fixing the unused
tmpvolume in controller for clarity - Change
controller.storage.storageClassNamedefault to""with a warning comment - Document the
vault-obsidian-{vaultName}naming assumption
{
"verdict": "request-changes",
"summary": "The helm chart extension adds substantial new functionality (controller, agents, recurring-task-creator, Config CRD) with correct overall structure, but contains two critical correctness bugs: base64 values in Secrets include spurious quote characters due to `| quote` after `b64enc`, and the recurring-task-creator container defines a tmp volume that is never mounted while expecting writes to /tmp with a read-only root filesystem. Several important improvements are also recommended around storage class defaults, Strimzi namespace conventions, and git-rest URL derivation assumptions.",
"comments": [
{
"file": "helm/templates/controller-secret.yaml",
"line": 15,
"severity": "critical",
"message": "Must Fix (Critical): `b64enc | quote` produces base64 values wrapped in literal quote characters (e.g. `\"ZHVtbXk=\"`). Remove `| quote` — b64enc already produces a valid Secret data value. The application will receive quoted strings including `\"` characters when reading `secretKeyRef`."
},
{
"file": "helm/templates/recurring-task-creator-statefulset.yaml",
"line": 165,
"severity": "critical",
"message": "Must Fix (Critical): Pod defines a `tmp` emptyDir volume but the container has no `volumeMounts` entry for it. With `readOnlyRootFilesystem: true`, any write to `/tmp` will fail. Add `volumeMounts: - name: tmp mountPath: /tmp` to the service container."
},
{
"file": "helm/templates/controller-statefulset.yaml",
"line": 120,
"severity": "critical",
"message": "Must Fix (Critical): Same tmp volume bug as recurring-task-creator — pod defines `tmp` emptyDir but container does not mount it. Unlike recurring-task-creator, the controller does not set `readOnlyRootFilesystem: true`, so this is dead code rather than a runtime failure, but it should be cleaned up."
},
{
"file": "helm/values.yaml",
"line": 129,
"severity": "major",
"message": "Should Fix (Important): `controller.storage.storageClassName: local-path` is a single-node/local-path-provisioner-specific value. Multi-node production clusters will fail to schedule the BoltDB PVC. Change default to `""` (cluster default) and add a comment warning that `local-path` requires the Local Path Operator."
},
{
"file": "helm/values.yaml",
"line": 194,
"severity": "major",
"message": "Should Fix (Important): `recurringTaskCreator.kafkaUser.strimziNamespace: strimzi` assumes the Strimzi operator runs in the `strimzi` namespace. If installed elsewhere, the KafkaUser is created in the wrong namespace and mTLS auth silently fails. Add a comment noting this assumption."
},
{
"file": "helm/values.yaml",
"line": 89,
"severity": "major",
"message": "Should Fix (Important): `controller.gitRestUrl` derivation assumes the in-cluster service is named `vault-obsidian-{vaultName}:9090`. This is a specific internal naming convention. If the service is named differently, the controller will fail with no install-time warning. Document this assumption prominently."
},
{
"file": "helm/values.yaml",
"line": 174,
"severity": "major",
"message": "Should Fix (Important): `dryRun: \"false\"` is a string `\"false\"`, not boolean `false`. Depending on how the application parses this env var, the behavior may be inconsistent. Document the expected string values or clarify the application's parsing logic."
},
{
"file": "helm/templates/recurring-task-creator-kafkauser.yaml",
"line": 1,
"severity": "nit",
"message": "Nice to Have (Optional): KafkaUser is created in `strimziNamespace` (default `strimzi`), typically a different namespace from the app. Standard `kubectl get kafkausers -n <app-ns>` won't find it, which can be confusing during debugging. Consider a comment noting where to find it."
}
],
"concerns_addressed": [
"security: controller-secret.yaml — secrets are not hardcoded (empty by default), but the b64enc|quote bug causes malformed values",
"security: recurring-task-creator-kafkauser.yaml — kafkaUser.enabled gating is correctly honored; mTLS requirements properly documented with type: tls",
"security: _helpers.tpl — gitRestUrl derivation assumes specific vault-obsidian naming convention; should be documented",
"correctness: config-crd.yaml — CRD in crds/ dir is correct; preserveUnknownFields: false is appropriate",
"correctness: agents.yaml — multi-document YAML separator (---) is correctly placed; toYaml rendering appears correct",
"correctness: controller-statefulset.yaml — storage class is parameterized but defaults to local-path (unsuitable for multi-node); tmp volume not mounted",
"tests: helm template rendering tests — not visible in diff, but new values blocks suggest test coverage would be valuable"
]
}…lt, document gitRestUrl/strimzi assumptions
|
Thanks — addressed the reusability findings in 58aa2e4:
On the three Critical findings — all three are false positives (verified against the rendered output and the existing bot-approved templates):
Re-render is lint-clean and all docs parse. |
ben-s-pull-request-reviewer
Bot
dismissed
their stale review
superseded by new automated review
There was a problem hiding this comment.
Based on my comprehensive review of the changed files, here is my analysis:
Reviewed files (selector mode, in-session):
helm/Chart.yaml— chart metadata, version bump 0.1.0→0.2.0helm/crds/config-crd.yaml— new Config CRD with CEL validation rulehelm/templates/_helpers.tpl— new image helpers +gitRestUrlderivationhelm/templates/agents.yaml— Config CR + Secret + PVC + PriorityClass + ResourceQuota per agenthelm/templates/controller-secret.yaml— conditional Secret creationhelm/templates/controller-service.yaml— new headless-ish Servicehelm/templates/controller-statefulset.yaml— new StatefulSet with full env/Broker/VolumeClaimTemplatehelm/templates/recurring-task-creator-kafkauser.yaml— Strimzi KafkaUser (off by default)helm/templates/recurring-task-creator-statefulset.yaml— StatefulSet + RBAC (Role/RoleBinding/ClusterRole/ClusterRoleBinding) + Service + Secrethelm/values.yaml— newcontroller,agents[],recurringTaskCreatorblocksCHANGELOG.md— documented the additions
Concern adjudication:
| # | Area | Concern | Verdict |
|---|---|---|---|
| 1 | security | Secret with b64enc — off by one key when only one of sentry.dsn/gatewaySecret is set | Partially valid — the Secret has a spurious empty-key when only one value is set; statefulset uses required so it won't start, but the secret is still created |
| 2 | security | RBAC minimum permissions | Addressed — Role get/list/watch on schedules only; ClusterRole limited to CRD registration |
| 3 | security | Strimzi namespace placement | Documented — values.yaml lines 225–235 explicitly warn about strimziNamespace and the debugging implication |
| 4 | correctness | Hardcoded vault-obsidian-<vaultName>:9090 |
Documented — values.yaml lines 91–98 have a prominent warning, plus _helpers.tpl comment |
| 5 | correctness | controller.kafkaBrokers required |
Addressed — required at line 81 produces a clear error on empty |
| 6 | correctness | recurringTaskCreator.kafkaBrokers required |
Addressed — required at line 129 |
| 7 | correctness | ResourceQuota string quota | Valid concern, resolved — {{ $agent.concurrency | default 1 | quote }} renders "1" (quoted string), which is correct for Kubernetes |
| 8 | correctness | CRD CEL rule | Valid — rule size(self.taskType) > 0 (string length) and size(self.taskTypes) > 0 (array length) are correct CEL expressions; note: requires Kubernetes 1.25+ (CEL support) |
| 9 | performance | storageClassName empty-string default | Intentional + documented — comment at lines 133–136 explains omission when empty; portable across StorageClasses |
| 10 | performance | PVC per agent | By design — each agent needs dedicated storage; documented in values.yaml comment |
{
"verdict": "approve",
"summary": "The Helm chart additions (controller StatefulSet+Service+Secret, leaf agents, recurring-task-creator with full RBAC, Config CRD) are well-structured with proper security defaults (readOnlyRootFilesystem, runAsNonRoot, no privilege escalation), appropriate RBAC scoping, and thorough inline documentation of non-obvious assumptions (gitRestUrl naming, strimziNamespace placement). The only minor issue is the controller Secret creates a spurious empty-key when only one of sentry.dsn/gatewaySecret is set — the statefulset refuses to start with empty values via `required`, so it fails safe, but the secret is still emitted unnecessarily.",
"comments": [
{
"file": "helm/templates/controller-secret.yaml",
"line": 15,
"severity": "major",
"message": "The Secret always emits both `sentry-dsn` and `gateway-secret` keys when either value is set. If only `sentry.dsn` is configured, `gateway-secret` gets `{{ .Values.controller.gatewaySecret | b64enc }}` where the value is empty — producing base64 `\"Cg==\"` rather than omitting the key. The statefulset uses `required` so the pod won't start with empty values, but the secret is still created with a spurious empty key. Consider splitting into two distinct conditional blocks (one per key) so the Secret only contains keys that are actually configured."
}
],
"concerns_addressed": [
"security: RBAC (Role get/list/watch on schedules, ClusterRole for CRD registration only) — correctly scoped, not over-permissioned",
"security: Strimzi KafkaUser namespace placement — documented at values.yaml:225-235",
"correctness: gitRestUrl hardcoded derivation — documented at values.yaml:91-98 with prominent warning",
"correctness: controller.kafkaBrokers required guard — present at line 81",
"correctness: recurringTaskCreator.kafkaBrokers required guard — present at line 129",
"correctness: ResourceQuota string quota — correctly rendered as quoted string",
"correctness: CRD CEL rule — syntactically valid (requires K8s 1.25+)",
"performance: storageClassName empty-string default — intentional with explanatory comment at lines 133-136",
"performance: PVC per agent — by design, documented in values.yaml"
]
}1 participant
Extends the
agentHelm chart (0.1.0 → 0.2.0) from executor-only to the full core platform, per the reusable-framework goal architecture (three-layer: public images → chart → per-cluster values).What's added
crds/config-crd.yaml) —configs.agent.benjamin-borbe.de, shipped viacrds/so leaf Config CRs apply on first install. The executor still creates/updates the schema at runtime;crds/only guarantees install-time ordering.agent-task-controllerStatefulSet (1Gi BoltDB PVC) + Service + gated Secret (sentry-dsn + gateway-secret). Vault coupling parameterized:controller.vaultName,controller.gitRestUrl(deriveshttp://vault-obsidian-<vault>:9090when unset).agents: []emits Config CR + Secret + PVC + PriorityClass (cluster-scoped) + ResourceQuota (pods cap). No hard-coded agents; the 4 examples are documented in a commented values block.recurringTaskCreator.enabled(default false). Strimzi KafkaUser double-gated onkafkaUser.enabled(mTLS clusters only).agent.controller.image,agent.recurringTaskCreator.image,agent.controller.gitRestUrl; expandedvalues.yamlwith public docker.io defaults + per-cluster override comments.Verification
helm lintclean (only the cosmeticiconINFO).helm templaterenders valid YAML for: full-enable (all components), 2-agent (proves the multi-doc separator), and executor-only minimal (proves every new component is truly optional). All docs parse viayaml.safe_load_all.--include-crds(correct —crds/isn't templated).make precommitpasses (format/generate/test/lint/addlicense).🤖 Generated with Claude Code