feat: Add Helm chart support, scripts, and docs for externalized config (Phase 5 of 5)#134
Conversation
There was a problem hiding this comment.
Pull request overview
Final phase of the externalized-config effort: adds Helm chart wiring (ConfigMaps + volume mounts + CLI flags), operational scripts, and updated docs/tests to support multi-layer injection precedence with hot-reloadable platform defaults and feature gates.
Changes:
- Add Helm chart templates for platform defaults / feature gates ConfigMaps and mount them into the controller with
--config-path/--feature-gates-path. - Extend local operational scripts for rollout + automated/interactive precedence verification.
- Update injector logic/tests/docs for SPIRE opt-out behavior and tighten sidecar security context.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 22 comments.
Show a summary per file
| File | Description |
|---|---|
| kagenti-webhook/testing-plan-precedence.md | Manual testing plan for precedence behavior and hot-reload. |
| kagenti-webhook/scripts/webhook-rollout.sh | Local build/deploy workflow extended to apply Helm-rendered ConfigMaps and patch deployment. |
| kagenti-webhook/scripts/test-precedence.sh | Automated precedence test runner (kind-based) including hot-reload validation. |
| kagenti-webhook/scripts/demo-precedence.sh | Interactive tmux demo for precedence scenarios. |
| kagenti-webhook/internal/webhook/injector/precedence_test.go | Updates precedence tests for SPIRE opt-out semantics. |
| kagenti-webhook/internal/webhook/injector/precedence.go | SPIRE opt-out layer added for spiffe-helper after standard chain. |
| kagenti-webhook/internal/webhook/injector/pod_mutator.go | SPIRE decision now follows precedence decision instead of label helper. |
| kagenti-webhook/internal/webhook/injector/container_builder.go | Add RunAsNonRoot: true for spiffe-helper; remove deprecated wrapper functions. |
| kagenti-webhook/README.md | Document precedence chain, feature gates, rollout workflow, and platform defaults. |
| kagenti-webhook/ARCHITECTURE.md | Update architecture/injection-flow docs for new precedence behavior. |
| charts/kagenti-webhook/values.yaml | Add featureGates and defaults values to drive generated ConfigMaps. |
| charts/kagenti-webhook/templates/deployment.yaml | Mount ConfigMaps and pass config paths via CLI flags. |
| charts/kagenti-webhook/templates/configmap-platform-defaults.yaml | Render values.defaults into config.yaml ConfigMap. |
| charts/kagenti-webhook/templates/configmap-feature-gates.yaml | Render values.featureGates into feature-gates.yaml ConfigMap. |
| AuthBridge/k8s/configmaps-webhook.yaml | Namespace-scoped ConfigMaps required for AuthBridge webhook demo. |
| AuthBridge/k8s/authbridge-deployment.yaml | Full AuthBridge demo manifest (SPIFFE). |
| AuthBridge/k8s/authbridge-deployment-no-spiffe.yaml | Simplified AuthBridge demo manifest (no SPIFFE). |
| AuthBridge/k8s/auth-target-deployment-webhook.yaml | Target service manifest for webhook demo. |
| AuthBridge/k8s/agent-deployment-webhook.yaml | Agent workload manifest for webhook demo (SPIFFE). |
| AuthBridge/k8s/agent-deployment-webhook-no-spiffe.yaml | Agent workload manifest for webhook demo (SPIFFE opt-out). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d5610d1 to
76d453d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
76d453d to
260ee0c
Compare
pdettori
left a comment
There was a problem hiding this comment.
Review Summary
Comprehensive final phase of the externalize-config epic. Well-structured PR with Helm chart wiring (ConfigMaps + volume mounts), operational scripts (rollout, automated tests, interactive demo), and updated docs. The opt-out model for spiffe-helper, SecurityContext hardening, and dead code removal are clean.
A few issues in the Helm templates and scripts need attention — one could leave a cluster in a modified state on script failure.
Areas reviewed: Go, Helm/K8s, Shell scripts, YAML manifests, Security
Commits: 4, all signed-off ✓
CI status: passing ✓
Additional notes (not inline)
- The
configmap-feature-gates.yamltemplate has the same nil-guard issue asconfigmap-platform-defaults.yaml(see inline comment). - Demo manifests (
AuthBridge/k8s/) store Keycloak admin credentials in ConfigMaps rather than Secrets. Acceptable for local demos but worth a# WARNING: demo onlycomment on each. demo-precedence.shhas no cleanuptrap— if interrupted, test deployments and namespace are left behind.spiffe-helpercontainer inauthbridge-deployment.yamlis missing resource limits (all other containers have them).
Code reviewFound 1 issue:
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
260ee0c to
d150dc3
Compare
mrsabath
left a comment
There was a problem hiding this comment.
Review Summary
Final phase of the externalize-config epic. The Helm chart wiring (ConfigMaps + volume mounts + CLI flags), operational scripts (rollout, automated tests, interactive demo), and two-stage injection refactor are well-structured. The Go code quality improvements are excellent — error handling fixes, dead code removal, per-request ContainerBuilder (enabling proper hot-reload), and SecurityContext hardening.
Main concern: The opt-in → opt-out behavioral change could silently inject sidecars into existing workloads on upgrade. This needs explicit migration documentation / release notes. The empty feature gates edge case could also silently disable all injection if the featureGates key is accidentally removed from values.yaml.
Areas reviewed: Go, Helm/K8s, Shell scripts, YAML manifests, Security, Docs
Commits: 8, all signed-off ✓
CI status: passing ✓
Highlights
- ✅
ContainerBuildercreated per-request with current config snapshot — hot-reload now works correctly - ✅
RunAsNonRoot: trueadded to spiffe-helper SecurityContext - ✅ Error handling fixed in
ensureServiceAccount(was silently swallowed) - ✅ Good error wrapping with file path context in
feature_gate_loader.go - ✅ Dead wrapper functions cleanly removed
- ✅ Review feedback from @pdettori addressed (demo credential warnings, cleanup traps, Podman compat, nil guards)
Additional comments not attached inline
- nit:
test-precedence.shuses untaggedbusybox(:latest) whiledemo-precedence.shpinsbusybox:1.36. Pin consistently for reproducibility. - nit: Test coverage for precedence evaluator reduced significantly (290 lines removed, 56 added). Edge cases for the new two-layer model could be added: nil
featureGates, concurrent evaluator access. - suggestion: Multiple demo manifests echo
Client Secret: $CLIENT_SECRETto stdout in container startup scripts. Container logs are often collected — even in demos, printing secrets sets a bad precedent. - suggestion: Keycloak admin credentials in ConfigMaps across demo manifests. The
# WARNING: demo onlycomments help, but using Secrets withsecretKeyRefeven in demos establishes the correct pattern.
- Pin spiffe-helper image from nightly to 0.11.0 in compiled defaults and demo manifest (values.yaml already pinned) - Detect all-zero FeatureGates struct and fall back to compiled defaults to prevent silently disabling injection when featureGates key is removed - Add resource limits to spiffe-helper in authbridge-deployment.yaml - Tighten jwt_svid_file_mode from 0644 to 0600 in demo manifest - Redact client secret from container startup logs in demo manifests - Pin busybox to 1.36 in test-precedence.sh (consistency with demo script) - Add migration note to README for opt-in to opt-out behavioral change Addresses review comments on PR rossoctl#134. Signed-off-by: cwiklik <cwiklikj@gmail.com>
mrsabath
left a comment
There was a problem hiding this comment.
Re-Review (after a76d6dd)
The author addressed the majority of feedback from both @pdettori and @mrsabath reviews in commit a76d6dd.
Previous Issues — Resolution
| # | Issue | Status |
|---|---|---|
| 1 | Breaking change needs migration docs | FIXED — README has "Upgrading from the opt-in model" section |
| 2 | Empty feature gates → all-false struct | FIXED — All-zero detection with fallback to defaults |
| 3 | Nested ConfigMap volume mounts | Not addressed (functional, noted) |
| 4 | jwt_svid_file_mode = 0644 |
PARTIALLY FIXED — Fixed in standalone demo, missing in webhook ConfigMap |
| 5 | Missing set -euo pipefail |
Not addressed |
| 6 | Busybox pinning | PARTIALLY FIXED — Pinned in scripts, not in testing-plan doc |
| 7 | Test coverage reduced | Not addressed (acceptable for simplified architecture) |
| 8 | Client secret echoed to stdout | FIXED — Redacted in both manifests |
| 9 | Keycloak admin in ConfigMaps | PARTIALLY FIXED — Warning comments added |
Highlights
- ✅ Migration documentation added for opt-in → opt-out behavioral change
- ✅ All-zero
FeatureGatesdetection prevents accidental injection disablement - ✅ Client secrets redacted from container startup logs
- ✅
jwt_svid_file_modetightened to 0600 in demo manifest - ✅
spiffe-helperimage pinned from nightly to 0.11.0 - ✅
busyboxpinned to 1.36 in test scripts - ✅ Resource limits added to spiffe-helper in demo manifest
Remaining items (see inline comments)
Lower-severity items that can be addressed in follow-up — none are blocking.
Areas reviewed: Go, Helm/K8s, Shell scripts, YAML manifests, Security, Docs
Commits: 9, all signed-off ✓
CI status: passing ✓
…scripts - Add ConfigMap templates for platform-defaults and feature-gates - Update deployment template with volume mounts for config files - Update values.yaml with new configuration sections - Add test-precedence.sh script for validating injection decisions - Update webhook-rollout.sh and README.md - Change spiffe-helper injection to opt-out model: no longer requires kagenti.io/spire=enabled; set kagenti.io/spire=disabled to opt out Part of kagenti#109 (Phase 5 of 5 — final) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: cwiklik <cwiklikj@gmail.com>
- Flip spiffe-helper L7 check from opt-in to opt-out: kagenti.io/spire=disabled blocks injection; all other values (including absent) allow injection - Add layer 7 to PrecedenceEvaluator godoc - Add SecurityContext (RunAsUser/RunAsGroup/RunAsNonRoot) to spiffe-helper container - test-precedence.sh: add kubelet syncFrequency speed-up (10s during tests), add Test 5 (SPIRE opt-out label), fix selector bug in t7-no-optin, fix --tail=5000 for Test 1 startup banners, fix ordering of reset/speed-up - demo-precedence.sh: add tmux-based interactive demo for precedence layers - Update ARCHITECTURE.md, README.md, testing-plan-precedence.md to reflect opt-out model and 7-layer chain Part of rossoctl#109 (Phase 5 of 5 — final) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: cwiklik <cwiklikj@gmail.com>
The four package-level wrapper functions (BuildSpiffeHelperContainer, BuildClientRegistrationContainerWithSpireOption, BuildEnvoyProxyContainer, BuildProxyInitContainer) were introduced in Phase 3 for backward compatibility during the transition. Phase 4 rewired all callers to use m.Builder.* methods directly — no callers remain outside the package. Remove the dead code and the stale "will be removed in Phase 4" comment. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: cwiklik <cwiklikj@gmail.com>
- Rename spireEnabled → spiffeHelperInjected in InjectAuthBridge for
clarity (the variable reflects the precedence decision, not a label)
- Update isAlreadyInjected comment: envoy-proxy is no longer guaranteed
injected post-feature-gates; any sidecar presence marks injection done
- Fix README and testing-plan-precedence.md: spiffe-helper and
client-registration are injected by default (opt-out model), not
"optional if SPIRE enabled"
- Update ARCHITECTURE.md: AuthBridge decision flow now shows
7-layer precedence evaluator and opt-out SPIRE label semantics
- Upgrade go.opentelemetry.io/otel/sdk v1.38.0 → v1.40.0 to fix
GO-2026-4394 (PATH hijacking / arbitrary code execution)
- Fix test-precedence.sh Test 1: restart webhook pod for fresh startup
logs, move speed_up_kubelet before reset_feature_gates so hot-reload
waits use 10s kubelet sync (eliminates WARNING), use --tail=500
- Fix demo-precedence.sh: correct single-quoted heredoc in deploy_no_type
so ${name} expands, wire Scenario 5 to the function
- Fix webhook-rollout.sh: replace non-idempotent JSON-patch op:add with
ensure_volume_and_mount() helper that pre-checks by name
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: cwiklik <cwiklikj@gmail.com>
…Evaluator Replace the multi-layer opt-in precedence chain with a cleaner two-stage design: Stage 1 (PodMutator pre-filters) — all-or-nothing, any "no" skips all injection: - kagenti.io/type must be agent or tool - featureGates.globalEnabled kill switch - featureGates.injectTools gate for tool workloads - kagenti.io/inject=disabled whole-workload opt-out Stage 2 (PrecedenceEvaluator per-sidecar) — independent for each sidecar: - L1: per-sidecar feature gate - L2: workload opt-out label (kagenti.io/<sidecar>-inject=false) Remove namespace opt-in label requirement (kagenti-enabled=true), TokenExchange CR override stubs, and platform defaults from PrecedenceEvaluator — these were either never wired, not relevant to admission, or handled elsewhere. Drop tokenexchange_overrides.go and the now-dead LabelNamespaceInject constant. Switch MutatingWebhookConfiguration from namespaceSelector+matchLabels to objectSelector so the webhook only fires for workloads labelled with kagenti.io/type=agent or =tool, eliminating the per-namespace opt-in label requirement and moving pre-filtering to the Kubernetes API server level. Update unit tests to match the simplified two-layer interface. Signed-off-by: cwiklik <cwiklikj@gmail.com>
Scripts:
- demo-precedence.sh: detect ConfigMap no-op patches to avoid hot-reload
timeout when content is already at the desired state; add cleanup trap
so interrupted runs always tear down test resources; restart webhook pod
after speed_up_kubelet so the new pod benefits from fast CM sync
- test-precedence.sh: same no-op patch detection; add trap reset_kubelet EXIT
so the Kind node kubelet syncFrequency is always restored on early exit
- webhook-rollout.sh: replace hardcoded docker with ${DETECTED} for all
build/save/exec invocations (Podman compatibility)
Helm:
- configmap-feature-gates.yaml: guard .Values.featureGates with | default dict
to avoid rendering literal "null" when the key is omitted in values override
- configmap-platform-defaults.yaml: same guard for .Values.defaults
- values.yaml: document featureGates and defaults keys with inline comments
Signed-off-by: cwiklik <cwiklikj@gmail.com>
- CLAUDE.md (root): add AI assistant instructions (no attribution in commits/PRs) - kagenti-webhook/CLAUDE.md: rewrite injection decision flow to reflect the two-stage model; update directory listing to include test-precedence.sh; fix gotcha kagenti#1 (config system is now wired in, not a gap); update architecture patterns to reference per-sidecar workload labels - kagenti-webhook/ARCHITECTURE.md: fix "disable" → "disabled" in Mermaid diagram - kagenti-webhook/README.md: fix "disable" → "disabled" typo; update step 6 description; add Testing the Precedence System section for test-precedence.sh - kagenti-webhook/testing-plan-precedence.md: align test plan with two-stage model - AuthBridge demos: minor wording fixes Signed-off-by: cwiklik <cwiklikj@gmail.com>
…ression - AuthBridge/k8s/authbridge-deployment.yaml: add demo-only warning comment above ConfigMap containing hardcoded Keycloak admin credentials, matching the pattern already present in configmaps-webhook.yaml - AuthBridge/k8s/authbridge-deployment-no-spiffe.yaml: same warning comment - kagenti-webhook/internal/webhook/v1alpha1/authbridge_webhook.go: suppress gocritic hugeParam false positive on Handle() — admission.Handler interface requires value receiver for admission.Request Signed-off-by: cwiklik <cwiklikj@gmail.com>
- Pin spiffe-helper image from nightly to 0.11.0 in compiled defaults and demo manifest (values.yaml already pinned) - Detect all-zero FeatureGates struct and fall back to compiled defaults to prevent silently disabling injection when featureGates key is removed - Add resource limits to spiffe-helper in authbridge-deployment.yaml - Tighten jwt_svid_file_mode from 0644 to 0600 in demo manifest - Redact client secret from container startup logs in demo manifests - Pin busybox to 1.36 in test-precedence.sh (consistency with demo script) - Add migration note to README for opt-in to opt-out behavioral change Addresses review comments on PR rossoctl#134. Signed-off-by: cwiklik <cwiklikj@gmail.com>
When webhook.enableClientRegistration was set to false in values.yaml, the flag was omitted from the deployment args. Since the Go binary defaults --enable-client-registration to true, this meant the Helm value had no effect. Now the flag is always rendered with the explicit configured value. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: cwiklik <cwiklikj@gmail.com>
a76d6dd to
4f63e66
Compare
Summary
Final phase of the externalize-config epic (#109). Adds the Helm chart infrastructure, operational scripts, and documentation that wire together all four previous phases.
configmap-platform-defaults.yamlandconfigmap-feature-gates.yamltemplates rendervalues.yamldefaults into cluster ConfigMaps. Keys match exactly what the Go loaders read (config.yaml,feature-gates.yaml).deployment.yamlupdated with--config-pathand--feature-gates-pathCLI args, and matching volume mounts at/etc/kagentiand/etc/kagenti/feature-gates.values.yaml: NewfeatureGatesanddefaultssections — all sidecar images, ports, resources, and per-sidecar enabled flags are version-controlled with the chart.webhook-rollout.sh: Extended to 6-step flow — builds image, loads to kind, deploys both ConfigMaps viahelm template, patches deployment volumes, waits for rollout, creates authbridge webhook.test-precedence.sh: Automated 8-test runner covering all 7 precedence layers. Speeds up kubelet ConfigMap sync to 10s for the test run, then resets.demo-precedence.sh: Interactive tmux demo — 6 scenarios with live pod-watch and webhook log panes.kagenti.io/spire=disabledblocks injection). No label required for injection.spireEnabledinInjectAuthBridgenow followsdecision.SpiffeHelper.Inject.BuildSpiffeHelperContainergainsRunAsNonRoot: truealongside existing UID/GID 1000 — required for admission controllers enforcingrunAsNonRoot.ContainerBuilderwrapper functions (introduced for Phase 3 backward compat, superseded by Phase 4) removed.Test plan
helm lint charts/kagenti-webhook/— 0 failureshelm template kagenti-webhook charts/kagenti-webhook/— ConfigMap keys and volume paths render correctlygo build ./... && go vet ./... && go test ./internal/webhook/...— all pass./scripts/webhook-rollout.sh— deploys webhook + ConfigMaps in one command./scripts/test-precedence.sh— all 8 tests pass against a live kind cluster./scripts/demo-precedence.sh— 6 interactive scenarios run correctly in tmuxPart of #109 (Phase 5 of 5 — final)
This PR depends on changes in kagenti UI described here: rossoctl/rossoctl#836