EN-11853: expose standard Helm chart configuration options by thefirstofthe300 · Pull Request #152 · gremlin/helm

thefirstofthe300 · 2026-09-18T01:20:59Z

Summary

Exposes nine previously-hardcoded/absent areas of Kubernetes pod-spec configuration as Helm values on both the gremlin DaemonSet and chao Deployment, so support engineers and customers have a supported, GitOps-safe alternative to hand-patching a live workload (kubectl patch) — which is fragile and gets silently reverted by drift-correcting GitOps tools (Flux, Argo CD). Triggered by EN-11846, a Bottlerocket/EKS Auto Mode race condition whose only workaround (a startupProbe) had no supported way to apply.

New values (root = gremlin DaemonSet only, chao.* = Chao Deployment only): livenessProbe/readinessProbe/startupProbe, initContainers, extraVolumes/extraVolumeMounts, lifecycle, envFrom, terminationGracePeriodSeconds, dnsConfig, hostAliases, podSecurityContext. All are pure Kubernetes pass-through (no chart-side schema validation, matching the chart's existing extraEnv/GPU-vendor-block convention). initContainers/extraVolumes/extraVolumeMounts fail the render on a name collision with a chart-managed resource, rather than silently shadowing it or letting the Kubernetes API reject it with an opaque error (see CLAUDE.md).

Every new value defaults to a no-op; rendered output is byte-identical to today's until an operator opts in (verified as its own contract clause, C10).

Process

This went through the full jig contract workflow — requirements elicitation → design → sealed contract (11 clauses, base-verified) → implementation → validation. Full history:

Requirements (Gate 1, assented) — includes one post-assent revision: a reserved-name collision check originally proposed for the pre-existing extraEnv value was dropped after design found extraEnv renders last in both containers' env: lists, making a "collision" there a working last-wins override (e.g. for GREMLIN_SERVICE_URL), not a bug — see .claude/contracts/EN-11853/requirements.md's Post-assent revisions.
Design — exact template placements, the initContainers restructure, and the chao volumeMounts: gating fix.
Contract (Gate 2, sealed) — 9 behavior clauses + 2 property invariants (backward compatibility; extraEnv exemption), each independently base-verified.
Dossier (Gate 3, validation) — all 11 clauses pass against the implementation. Not yet signed — signing happens after review settles, per this repo's contract workflow.

Review

Both a design-conformance review (jig-reviewer) and a hostile-input security review (jig-security-reviewer) ran against commit 146d3b3 (full diff, pre-fix). Findings and how each was handled:

Fixed in this PR (commit d60ebcd):
- terminationGracePeriodSeconds was interpolated as a bare scalar ({{ .Values...TerminationGracePeriodSeconds }}) on both workloads — a string value with embedded newlines could inject sibling pod-spec keys. Same shape as the chart's pre-existing dnsPolicy interpolation (confirmed reproducible on master too), so not a new class of issue, but cheap to close: coerced with | int.
- Documented (values.yaml + README) that hostAliases has no runtime effect while gremlin.hostNetwork: true (the default) — renders correctly, but the kubelet doesn't manage /etc/hosts for host-network pods.
- Documented that envFrom does not override chart-managed env vars, unlike the adjacent extraEnv (Kubernetes gives env: precedence over envFrom:).
- Documented the interaction between extraVolumes/initContainers and the chart's own PodSecurityPolicy/SecurityContextConstraints, when those are enabled.
- Added all 24 new values to gremlin/README.md's values table (previously only values.yaml was updated).
Acknowledged, no action (both reviews converged on the same shape independently): the reserved-name collision check is a rename-with-clear-error control, not a security boundary — e.g. initContainers[].volumeMounts[].name isn't checked, so a chart-managed volume stays mountable by name through that path. This matches the design's own stated framing (see design.md §5, CLAUDE.md) and isn't a new privilege surface — a values author who can add an initContainer at all already controls image.repository and gremlin.podSecurity.privileged.
Not re-reviewed: the fix commit (d60ebcd) itself — a one-token filter change plus doc comments — was not independently re-reviewed by a fresh agent. Stated here as a gap rather than a silent one; full suite (30 suites, 214 tests) and contract re-validation (all 11 clauses) both pass against it.

Test plan

helm lint gremlin — clean
Full existing suite regression-free: 30 suites, 214 tests
All 11 contract clauses (9 behavior + 2 property invariants) pass against the implementation — see dossier
Backward compatibility verified as its own clause: default (unset) rendering is byte-identical to pre-change output on both workloads

🤖 Generated with Claude Code

Index the chart's structure and conventions so future sessions get symbol-aware navigation instead of falling back to plain grep, and gitignore Serena's regenerable cache directory. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…alues Any new chart value that lets a user define a named Kubernetes fragment alongside chart-managed resources must fail the render on a name collision rather than shadowing it silently or letting the Kubernetes API reject it with an opaque error. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Gate 1 assented by Danny Seymour on 2026-09-17 (re-assented same day after dropping the extraEnv reserved-name retrofit, a genuine design fork surfaced during design). Design resolves every template insertion point, the initContainers restructure, the chao volumeMounts gating fix, and the reserved-name validation approach. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Helm-unittest specs proving B1-B9 (probes, initContainers, extraVolumes/extraVolumeMounts, lifecycle, envFrom, terminationGracePeriodSeconds, dnsConfig, hostAliases, pod-level securityContext) and the backward-compatibility invariant, ahead of any implementation. Each currently fails (or, for the invariant, passes) for the reason the contract clause requires, verified by running every probe directly. This commit is the contract's base_commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

11 clauses (9 behavior, 2 property) base-verified against 4279bc2. Approved by Danny Seymour. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Adds the 24 new pass-through values (12 root, 12 chao.*) to values.yaml and the reserved-name collision check templates, wired into both daemonset.yaml and chao-deployment.yaml. Satisfies C2 and C3's rejects clauses in full; the additive placements in _daemonset.tpl and chao-deployment.yaml follow in subsequent commits. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3TrXY8bWRGNv3aRFZZwGZ

Adds pod-spec fields (terminationGracePeriodSeconds, dnsConfig, hostAliases, podSecurityContext), the initContainers restructure (user containers appended after seccomp-init), container fields (envFrom, probes, lifecycle), and extraVolumes/extraVolumeMounts appends to _daemonset.tpl. Satisfies C1, C4-C9, and C2's additive half on the gremlin DaemonSet. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3TrXY8bWRGNv3aRFZZwGZ

Adds pod-spec fields, initContainers, container fields (envFrom, probes, lifecycle), the volumeMounts gate fix (extended to fire on chao.extraVolumeMounts alone), and extraVolumes/extraVolumeMounts appends to chao-deployment.yaml. All 11 contract clauses now pass in full; full existing suite (30 suites, 214 tests) is regression-free. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3TrXY8bWRGNv3aRFZZwGZ

seal.txt's sha256 line needed the bare "sha256 <hash>" form build_dossier.py parses, not "sha256: <hash>". All 11 clauses validated PASS against the implementation; dossier and run records added ahead of Gate 3 signature. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3TrXY8bWRGNv3aRFZZwGZ

- Coerce terminationGracePeriodSeconds with `| int` on both workloads, closing a YAML-injection path a bare-scalar string value could use to inject sibling pod-spec keys (security review, low severity; same shape as the chart's pre-existing dnsPolicy interpolation). - Document that hostAliases has no effect while gremlin.hostNetwork is true (the default) -- correct rendering, no runtime effect. - Document that envFrom does not override chart-managed env vars, unlike the adjacent extraEnv (Kubernetes gives env: precedence). - Document the interaction between extraVolumes/initContainers and the chart's own PodSecurityPolicy/SecurityContextConstraints when enabled. - Add all 24 new values to gremlin/README.md's values table, which was otherwise the only chart doc surface left undocumented. None of these change any tested behavior -- comments and an integer coercion that is a no-op for every value the contract's probes set. Full suite re-run clean: 30 suites, 214 tests, base verification still passes for all 11 clauses. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3TrXY8bWRGNv3aRFZZwGZ

Re-run validation and rebuilt the dossier after the review-fix commit; all 11 clauses still pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3TrXY8bWRGNv3aRFZZwGZ

Tests describe behavior, not the ticket that introduced it -- drop the EN-11853/B<n> prefixes from filenames and suite: descriptions, matching the existing repo convention (daemonset_resources_test.yaml, chao_deployment_namespaces_test.yaml, etc). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3TrXY8bWRGNv3aRFZZwGZ

Resolved the README.md table conflict by keeping master's wider column formatting and its new chao.tls.identity.enabled row, with our 24 new values spliced in at their original positions. values.yaml and _daemonset.tpl auto-merged cleanly. Refreshed the backward-compat fixtures against the new baseline -- the only drift was master's own changes (Chart.yaml version bump, chao.features.dynamicQuery.enabled now defaults true), nothing from this branch. Full suite re-verified clean: 32 suites, 231 tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3TrXY8bWRGNv3aRFZZwGZ

Test files and suite: descriptions name the behavior under test, not the ticket that introduced it -- ticket context belongs in the commit message and PR description instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3TrXY8bWRGNv3aRFZZwGZ

Copilot

Copilot review overview

🟡 Changes recommended

Moderate termination-value coercion and reserved-name validation issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 4 · 2

Open (6)

Preserve grace period scalar type during serialization · New
Reserve socket volumes for custom container driver names · New
Use Chao-specific reserved volume and mount names · New
Preserve grace period scalar type during serialization · New
Add test coverage for the default DaemonSet fixture · New
Add test coverage for the default Chao fixture · New

What changed in this PR

Exposes additional Kubernetes pod-spec configuration for the Gremlin DaemonSet and Chao Deployment.

Changes:

Adds probes, lifecycle, init containers, volumes, environment sources, DNS, security, and termination settings.
Adds reserved-name collision validation.
Adds Helm tests, compatibility fixtures, and documentation.

File	Description
`gremlin/values.yaml`	Defines the new configurable values.
`gremlin/tests/termination_grace_period_test.yaml`	Tests termination grace periods.
`gremlin/tests/probes_test.yaml`	Tests probe rendering.
`gremlin/tests/pod_security_context_test.yaml`	Tests pod security contexts.
`gremlin/tests/lifecycle_test.yaml`	Tests lifecycle hooks.
`gremlin/tests/init_containers_test.yaml`	Tests additional init containers.
`gremlin/tests/init_containers_reject_test.yaml`	Tests init-container collision handling.
`gremlin/tests/hostaliases_test.yaml`	Tests host aliases.
`gremlin/tests/fixtures/backward_compat_daemonset.yaml`	DaemonSet compatibility fixture; nit: it is not referenced by a test or workflow.
`gremlin/tests/fixtures/backward_compat_chao.yaml`	Chao compatibility fixture; nit: it is not referenced by a test or workflow.
`gremlin/tests/extra_volumes_test.yaml`	Tests additional volumes and mounts.
`gremlin/tests/extra_volumes_reject_test.yaml`	Tests volume-name collision handling.
`gremlin/tests/extra_env_exempt_test.yaml`	Tests the `extraEnv` exemption.
`gremlin/tests/envfrom_test.yaml`	Tests environment sources.
`gremlin/tests/dnsconfig_test.yaml`	Tests DNS configuration.
`gremlin/templates/daemonset.yaml`	Invokes DaemonSet validation.
`gremlin/templates/chao-deployment.yaml`	Renders Chao options; moderate issue: `int` can silently coerce invalid termination grace-period values.
`gremlin/templates/_validation.tpl`	Implements collision checks; moderate issues remain in workload-specific reserved-name handling and generated socket-volume coverage.
`gremlin/templates/_daemonset.tpl`	Renders DaemonSet options; moderate issue: `int` can silently coerce invalid termination grace-period values.
`gremlin/README.md`	Documents the new values.
`CLAUDE.md`	Documents chart conventions.
`.gitignore`	Ignores local review and cache artifacts.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

philgebhardt

Nice work. I left mostly minor comments, and a question about some test fixtures (which appears to be duplicated by copilot). I don't think every copilot callout is actionable but there are some worth looking at.

This repo is public-facing, so CLAUDE.md, README.md, code comments, and values.yaml doc comments should state reasoning in prose rather than point at a Jira ticket or an internal contract's design doc -- the latter is also now a dead link, since that tooling's working files were deliberately removed from this repo's history. Fixes the one existing violation (the extraEnv exemption's rationale). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3TrXY8bWRGNv3aRFZZwGZ

- terminationGracePeriodSeconds: replace | int with | toJson on both workloads. int silently coerces a non-numeric typo to 0 (a real bug -- confirmed a typo'd grace period renders as 0, a silent behavior change contrary to the chart's documented pass-through posture); toJson preserves the type (bare number stays a number) while still escaping embedded newlines into a single-line string, closing the YAML-injection path the earlier | int fix was for without reintroducing silent coercion. - Reserved-name lists: split into DaemonSet-specific and Chao-specific lists instead of one shared list. Chao was rejecting extraVolumes/initContainers names it doesn't actually render (gremlin-state, cgroup-root, seccomp-init, gremlin), a real over-blocking bug with no documented rationale. - Reserved-name lists: derive the container-driver socket volume names from containerDrivers.*.name instead of hardcoding docker-sock/containerd-sock/crio-sock, so a renamed driver stays covered by the collision check instead of silently duplicating a chart-managed volume. Also switched both lists to newline-separated entries for cleaner diffs. - Removed the two backward-compat fixture files: unreferenced by any test or workflow (their only consumer was a diff command that lived in the now-removed contract tooling), version-brittle, and functionally redundant with each new test file's own default-case assertions. - CLAUDE.md: documented the corrected per-workload reserved-name split. Added regression coverage: a renamed-driver socket collision, Chao correctly NOT rejecting a DaemonSet-only reserved name, and terminationGracePeriodSeconds preserving a non-numeric value's literal type (and blocking a multiline injection attempt) instead of silently becoming 0. Full suite: 32 suites, 235 tests, clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3TrXY8bWRGNv3aRFZZwGZ

Copilot

Copilot review overview

🔵 Needs a closer look

Reserved-volume validation incorrectly rejects names for driver sockets that are not rendered.

Review effort: Balanced
Findings: None

Resolved since last review (6)

Preserve grace period scalar type during serialization
Use Chao-specific reserved volume and mount names
Reserve socket volumes for custom container driver names
Preserve grace period scalar type during serialization
Add test coverage for the default Chao fixture
Add test coverage for the default DaemonSet fixture

philgebhardt

looks good. don't forget to minor bump the chart version, unless saving for a separate PR

thefirstofthe300 and others added 11 commits September 17, 2026 16:38

chore: onboard Serena for this repository

3bb62aa

Index the chart's structure and conventions so future sessions get symbol-aware navigation instead of falling back to plain grep, and gitignore Serena's regenerable cache directory. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

feat(EN-11853): seal contract at Gate 2

2169813

11 clauses (9 behavior, 2 property) base-verified against 4279bc2. Approved by Danny Seymour. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

docs(EN-11853): re-validate against post-review commit

dd122cb

Re-run validation and rebuilt the dossier after the review-fix commit; all 11 clauses still pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3TrXY8bWRGNv3aRFZZwGZ

thefirstofthe300 requested review from a team as code owners September 18, 2026 01:21

sirged approved these changes Sep 18, 2026

View reviewed changes

thefirstofthe300 force-pushed the EN-11853/expose-standard-helm-config-options branch from 71fa7d3 to dd122cb Compare September 22, 2026 17:22

thefirstofthe300 and others added 3 commits September 22, 2026 11:02

philgebhardt requested a balanced review from Copilot September 23, 2026 00:24

Copilot started reviewing on behalf of philgebhardt September 23, 2026 00:25 View session

Copilot AI reviewed Sep 23, 2026

View reviewed changes

philgebhardt reviewed Sep 23, 2026

View reviewed changes

thefirstofthe300 and others added 2 commits September 22, 2026 18:05

thefirstofthe300 requested review from philgebhardt and a balanced review from Copilot September 23, 2026 01:16

Copilot started reviewing on behalf of thefirstofthe300 September 23, 2026 01:17 View session

Copilot AI reviewed Sep 23, 2026

View reviewed changes

philgebhardt approved these changes Sep 23, 2026

View reviewed changes

thefirstofthe300 merged commit 15dd510 into master Sep 23, 2026
4 checks passed

thefirstofthe300 mentioned this pull request Sep 23, 2026

Bump gremlin chart version to 0.30.0 #154

Merged

2 tasks done

Navigation Menu

Repository navigation

EN-11853: expose standard Helm chart configuration options - #152

EN-11853: expose standard Helm chart configuration options#152
thefirstofthe300 merged 16 commits into
masterfrom
EN-11853/expose-standard-helm-config-options

thefirstofthe300 commented Sep 18, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

philgebhardt left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

philgebhardt left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

thefirstofthe300 commented Sep 18, 2026

Summary

Process

Review

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

philgebhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

philgebhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants