feat(k8s): Helm packaging -- library chart, per-service charts, umbrella (RFC-0003 PR-2) - #201
Merged
Conversation
…lla (RFC-0003 PR-2) Package the polyglot platform as Helm charts, validated entirely offline (no cluster -- the Kind cluster and the Gateway are PR-3, the Prometheus Operator is PR-4). - A library chart (deploy/k8s/charts/common) is the D6 uniform contract made executable: a Deployment/StatefulSet, Service, ServiceAccount, resources, and probes/ServiceMonitor that default to the D6 posture (httpGet /healthz liveness, /readyz readiness, ServiceMonitor on) but are per-service opt-out. The nginx frontend is the one documented D6 exception (ADR-0013): an honest tcpSocket liveness, no /healthz probe, no ServiceMonitor -- the nginx-vs-Caddy contrast the platform teaches. The backend keeps the HTTP pair and adds a gRPC startup probe on :50051; Postgres A/B/C are StatefulSets + PVCs (ADR-0018) with the reports artifact PVC. - Thin per-service charts consume the library; an umbrella chart with per-profile values overlays mirrors the compose profiles (core + analytics / reports / reports-ui / synthetic / load), so `helm template` selects exactly a profile's services. - CI gate (k8s-lint.yml + make lint-k8s, make==CI): helm lint + helm template | kubeconform -strict for every profile, with a vendored ServiceMonitor CRD schema so the CR is genuinely validated (Skipped:0), never --ignore-missing. helm + kubeconform pinned in .mise.toml. yamllint/check-yaml excludes scoped to the Go-template dirs only.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughAdds a shared Helm library, charts for platform services and profile overlays, and an offline validation gate. Local Make targets and CI now run Helm linting, profile rendering, and strict kubeconform validation using pinned tools and a vendored ServiceMonitor schema. ChangesKubernetes packaging
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant Makefile
participant ValidateScript
participant Helm
participant Kubeconform
Developer->>Makefile: run lint-k8s
Makefile->>ValidateScript: execute validate.sh
ValidateScript->>Helm: lint and render chart profiles
Helm-->>ValidateScript: rendered manifests
ValidateScript->>Kubeconform: strict schema validation
Kubeconform-->>Developer: validation result
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
deploy/k8s/charts/common/templates/_workload.tpl (2)
95-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting a shared
common.container.The StatefulSet pod spec duplicates the Deployment's container block (image, pullPolicy, ports, env, probes, resources, volumeMounts) and has already drifted:
command/argsare honoured only incommon.deployment. A singlecommon.containerhelper would keep the two workload shapes in step, in the spirit of the "uniformity enforced by this one template" comment at the top of the 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/k8s/charts/common/templates/_workload.tpl` around lines 95 - 117, Extract the duplicated container definition into a shared common.container helper containing image, pullPolicy, ports, env, probes, command/args, resources, and volumeMounts. Update both common.deployment and the StatefulSet workload template to render this helper, preserving the existing container name and indentation so both workload shapes remain consistent.
118-131: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard
volumeClaimTemplatesso the key is not emitted empty.
volumeClaimTemplates:is emitted unconditionally. A consumer without apersistencekey fails to render (nil pointer on.volumeClaimTemplates's parent), and one with an emptyvolumeClaimslist rendersvolumeClaimTemplates: null, whichkubeconform -strictrejects. Onlycharts/postgresuses this today, so wrap it while the contract is still cheap to fix.♻️ Proposed guard
- volumeClaimTemplates: - {{- range .Values.persistence.volumeClaims }} + {{- with (.Values.persistence).volumeClaims }} + volumeClaimTemplates: + {{- range . }} - metadata: name: {{ .name }} spec: accessModes: - {{ .accessMode | default "ReadWriteOnce" }} resources: requests: storage: {{ .size | default "1Gi" }} {{- with .storageClassName }} storageClassName: {{ . }} {{- end }} {{- end }} + {{- end }}🤖 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/k8s/charts/common/templates/_workload.tpl` around lines 118 - 131, Guard the volumeClaimTemplates block around the existing persistence.volumeClaims range so the key is emitted only when persistence.volumeClaims is defined and non-empty. Preserve the current claim rendering and defaults, while ensuring workloads without persistence or with an empty volumeClaims list omit volumeClaimTemplates entirely rather than rendering null.deploy/k8s/charts/common/templates/_probes.tpl (1)
24-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFail loudly on an unrecognised probe
type.An unknown/mistyped
type(e.g.tcpsocket,http) matches no branch, so the probe renders with only the timing fields and no handler. That is schema-valid, sokubeconform -strictpasses and the misconfiguration reaches the cluster silently — precisely the "opt-out is a named posture, not a hole" property this file documents. An explicitfailkeeps the contract enforced at render time.♻️ Proposed guard
{{- else if eq $type "exec" }} exec: command: {{- toYaml .command | nindent 4 }} +{{- else if ne $type "none" }} +{{- fail (printf "common.probeHandler: unsupported probe type %q (httpGet|tcpSocket|grpc|exec|none)" $type) }} {{- end }}🤖 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/k8s/charts/common/templates/_probes.tpl` around lines 24 - 40, Update the common.probeHandler template to add an explicit fallback after the supported httpGet, tcpSocket, grpc, and exec branches that fails rendering for any unrecognised .type value, including the invalid type in the error message. Preserve the existing handlers and defaults unchanged.deploy/k8s/schemas/monitoring.coreos.com/servicemonitor_v1.json (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord the provenance of this vendored schema.
JSON has no comment syntax, so nothing here states which prometheus-operator release the CRD was extracted from or how to regenerate it. Since this file is the load-bearing part of the DK9 gate, it will silently drift from the operator version PR-4 installs. Please note the upstream source URL and operator tag in
deploy/k8s/README.md(next to theschemas/layout entry), or add a smallMakefiletarget that refreshes 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/k8s/schemas/monitoring.coreos.com/servicemonitor_v1.json` around lines 1 - 3, Document the vendored ServiceMonitor schema’s provenance in deploy/k8s/README.md near the schemas/ layout entry, including the upstream prometheus-operator source URL, exact operator release tag, and regeneration procedure; do not modify the JSON schema itself.deploy/k8s/charts/postgres-exporter/values.yaml (1)
31-35: 🩺 Stability & Availability | 🔵 TrivialGate ServiceMonitor use on CRD availability.
serviceMonitor.enabled: truerenders a Prometheus Operator resource, but this PR does not install the ServiceMonitor CRD. Ensure installation ordering or keep this chart disabled until the operator dependency is present.🤖 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/k8s/charts/postgres-exporter/values.yaml` around lines 31 - 35, Update the serviceMonitor configuration in values.yaml so it is not enabled by default unless the Prometheus Operator ServiceMonitor CRD is guaranteed to be installed first; otherwise set serviceMonitor.enabled to false and preserve the existing path, port, and interval settings.
🤖 Prompt for all review comments with 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.
Inline comments:
In @.github/workflows/k8s-lint.yml:
- Around line 10-20: Add "Makefile" to the paths filters for both the
pull_request and push triggers in the k8s-lint workflow, preserving all existing
path entries.
In @.yamllint.yml:
- Around line 22-27: Rename the top-level configuration key from exclude to
ignore in the yamllint configuration, preserving the existing exclusion patterns
for frontend dependencies, Grafana dashboards, and Helm chart templates.
In `@deploy/k8s/charts/backend/values.yaml`:
- Around line 46-50: Disable d6.serviceMonitor.enabled by default in
deploy/k8s/charts/backend/values.yaml (lines 46-50) and
deploy/k8s/charts/reports-ui/values.yaml (lines 30-34), so ServiceMonitor
resources are not rendered until the Prometheus Operator CRD is installed;
preserve the existing ServiceMonitor configuration for users who explicitly
enable it.
In `@deploy/k8s/charts/common/values.yaml`:
- Around line 1-37: Add an ADR or RFC amendment documenting the shared values
contract represented by the common values template, including images, ports,
probes, persistence, ServiceMonitors, and compatibility expectations. Reference
the contract symbols and fields shown in the common values definition, and
follow the repository’s existing ADR/RFC format and numbering conventions.
In `@deploy/k8s/charts/platform/Chart.yaml`:
- Around line 18-83: Add the required ADR/RFC amendment for the new Helm service
contract represented by the dependencies in the Chart.yaml section. Document
each service’s values contract, the postgres aliases (postgres-backend,
postgres-analytics, and postgres-reports), and how the enabled conditions and
profiles control deployment.
In `@deploy/k8s/charts/platform/values.yaml`:
- Around line 33-41: Replace the literal analytics POSTGRES_PASSWORD in the env
block near deploy/k8s/charts/platform/values.yaml lines 33-41 with a
valueFrom.secretKeyRef pointing to the appropriate externally supplied Secret
key, preserving the existing POSTGRES_DB, POSTGRES_USER, and PGDATA entries.
Apply the same Secret-backed change to the reports POSTGRES_PASSWORD near lines
61-69; do not commit plaintext credentials.
In `@deploy/k8s/charts/postgres/values.yaml`:
- Around line 20-21: Update the POSTGRES_PASSWORD entry in the chart values to
use valueFrom.secretKeyRef referencing an externally managed Kubernetes Secret
and its password key. Remove the literal app value entirely, with no fallback or
other committed plaintext credential.
In `@deploy/k8s/charts/reports/values.yaml`:
- Around line 33-40: Update the reports chart configuration around extraPVCs and
pvcMounts to prevent rolling updates from creating concurrent pods that contend
for the ReadWriteOnce artifacts claim. Configure the Deployment with a Recreate
strategy, or change the claim to ReadWriteOncePod or an RWX-capable storage
mode, while preserving the existing artifact mount.
---
Nitpick comments:
In `@deploy/k8s/charts/common/templates/_probes.tpl`:
- Around line 24-40: Update the common.probeHandler template to add an explicit
fallback after the supported httpGet, tcpSocket, grpc, and exec branches that
fails rendering for any unrecognised .type value, including the invalid type in
the error message. Preserve the existing handlers and defaults unchanged.
In `@deploy/k8s/charts/common/templates/_workload.tpl`:
- Around line 95-117: Extract the duplicated container definition into a shared
common.container helper containing image, pullPolicy, ports, env, probes,
command/args, resources, and volumeMounts. Update both common.deployment and the
StatefulSet workload template to render this helper, preserving the existing
container name and indentation so both workload shapes remain consistent.
- Around line 118-131: Guard the volumeClaimTemplates block around the existing
persistence.volumeClaims range so the key is emitted only when
persistence.volumeClaims is defined and non-empty. Preserve the current claim
rendering and defaults, while ensuring workloads without persistence or with an
empty volumeClaims list omit volumeClaimTemplates entirely rather than rendering
null.
In `@deploy/k8s/charts/postgres-exporter/values.yaml`:
- Around line 31-35: Update the serviceMonitor configuration in values.yaml so
it is not enabled by default unless the Prometheus Operator ServiceMonitor CRD
is guaranteed to be installed first; otherwise set serviceMonitor.enabled to
false and preserve the existing path, port, and interval settings.
In `@deploy/k8s/schemas/monitoring.coreos.com/servicemonitor_v1.json`:
- Around line 1-3: Document the vendored ServiceMonitor schema’s provenance in
deploy/k8s/README.md near the schemas/ layout entry, including the upstream
prometheus-operator source URL, exact operator release tag, and regeneration
procedure; do not modify the JSON schema itself.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 566e3f61-3afe-47c2-8581-f5b0fcc69e47
📒 Files selected for processing (62)
.github/workflows/k8s-lint.yml.mise.toml.pre-commit-config.yaml.yamllint.ymlMakefiledeploy/k8s/.gitignoredeploy/k8s/README.mddeploy/k8s/charts/analytics/Chart.yamldeploy/k8s/charts/analytics/templates/manifests.yamldeploy/k8s/charts/analytics/values.yamldeploy/k8s/charts/backend/Chart.yamldeploy/k8s/charts/backend/templates/manifests.yamldeploy/k8s/charts/backend/values.yamldeploy/k8s/charts/blackbox/Chart.yamldeploy/k8s/charts/blackbox/templates/manifests.yamldeploy/k8s/charts/blackbox/values.yamldeploy/k8s/charts/canary/Chart.yamldeploy/k8s/charts/canary/templates/manifests.yamldeploy/k8s/charts/canary/values.yamldeploy/k8s/charts/common/Chart.yamldeploy/k8s/charts/common/templates/_helpers.tpldeploy/k8s/charts/common/templates/_networking.tpldeploy/k8s/charts/common/templates/_ports.tpldeploy/k8s/charts/common/templates/_probes.tpldeploy/k8s/charts/common/templates/_workload.tpldeploy/k8s/charts/common/values.yamldeploy/k8s/charts/frontend/Chart.yamldeploy/k8s/charts/frontend/templates/manifests.yamldeploy/k8s/charts/frontend/values.yamldeploy/k8s/charts/loadgen/Chart.yamldeploy/k8s/charts/loadgen/templates/manifests.yamldeploy/k8s/charts/loadgen/values.yamldeploy/k8s/charts/loki/Chart.yamldeploy/k8s/charts/loki/templates/manifests.yamldeploy/k8s/charts/loki/values.yamldeploy/k8s/charts/mailpit/Chart.yamldeploy/k8s/charts/mailpit/templates/manifests.yamldeploy/k8s/charts/mailpit/values.yamldeploy/k8s/charts/platform/Chart.yamldeploy/k8s/charts/platform/templates/NOTES.txtdeploy/k8s/charts/platform/values-analytics.yamldeploy/k8s/charts/platform/values-full.yamldeploy/k8s/charts/platform/values-load.yamldeploy/k8s/charts/platform/values-reports-ui.yamldeploy/k8s/charts/platform/values-reports.yamldeploy/k8s/charts/platform/values-synthetic.yamldeploy/k8s/charts/platform/values.yamldeploy/k8s/charts/postgres-exporter/Chart.yamldeploy/k8s/charts/postgres-exporter/templates/manifests.yamldeploy/k8s/charts/postgres-exporter/values.yamldeploy/k8s/charts/postgres/Chart.yamldeploy/k8s/charts/postgres/templates/manifests.yamldeploy/k8s/charts/postgres/values.yamldeploy/k8s/charts/reports-ui/Chart.yamldeploy/k8s/charts/reports-ui/templates/manifests.yamldeploy/k8s/charts/reports-ui/values.yamldeploy/k8s/charts/reports/Chart.yamldeploy/k8s/charts/reports/templates/manifests.yamldeploy/k8s/charts/reports/values.yamldeploy/k8s/schemas/monitoring.coreos.com/servicemonitor_v1.jsondeploy/k8s/scripts/validate.shdocs/ci.md
…ts gate Each of these passes `helm template | kubeconform -strict` today and fails somewhere later, which is the failure mode DK9 exists to prevent. - A mistyped probe `type` matched no branch and rendered a probe with timing fields and no handler. kubeconform accepts that (every handler is optional in the schema); the API server does not. The typo would have survived the gate and surfaced at install. Unknown types now fail at render. - `common.statefulset` read `.Values.persistence.volumeClaims` unguarded, so a chart without a `persistence` key could not render at all, and an empty list emitted a null `volumeClaimTemplates`. Both are now absent instead. - The reports artifact PVC is ReadWriteOnce, but the Deployment took the default rolling update: the surge pod is scheduled before the old one releases the volume and, on any other node of a multi-node cluster, never attaches. Reports now uses Recreate, via a new optional `strategy` value on the shared Deployment template. Two gate repairs alongside them: - `.yamllint.yml` used `exclude:`, which yamllint does not support and silently ignores, so none of the three exclusions applied. Only prek's own regex kept chart templates out of the linter. Renamed to `ignore:`. - The chart workflow runs `make lint-k8s` but did not watch `Makefile`, so a change to the target skipped its own gate. Other workflows already list it. The vendored ServiceMonitor schema is the load-bearing part of the gate and recorded no provenance; its source, regeneration command, and the fact that its operator release is still unpinned are now written down.
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
1 participant
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.
Package the polyglot platform as Helm charts, validated entirely offline (no cluster -- the Kind cluster and the Gateway are PR-3, the Prometheus Operator is PR-4).
helm templateselects exactly a profile's services.Summary by CodeRabbit