feat(helm): global values for registry, pull config, and namespace scope - #81
Merged
Merged
Conversation
Matches the convention landing across the kagent-family charts. global.imageRegistry overrides the image registry (air-gap mirror knob) via a shared kagent-tools.images.image helper. global.imagePullSecrets merges into the pod's own list via kagent-tools.imagePullSecrets. global.imagePullPolicy fills the gap when tools.image.pullPolicy is unset, via kagent-tools.imagePullPolicy; the declared IfNotPresent default moved into the template chain so the fallback is reachable. global.watchNamespaces is the namespace-scope fallback: rbac.namespaces overrides it when present -- including an explicit empty list, which is why the declared [] default ships commented out -- and on the global path the install namespace is auto-appended. The resolved scope drives the Role/ClusterRole switch and drops the nonResourceURLs rule namespaced Roles cannot carry. Verified: helm unittest passes (23 tests, including pins for the global scope, explicit-empty opt-out, and local-beats-global precedence); a default render is byte-identical to main. Signed-off-by: Jonathan Jamroga <[email protected]>
jjamroga
force-pushed
the
jjamroga/helm-global-values
branch
from
September 14, 2026 13:29
37e88ca to
aadbb2e
Compare
jjamroga
marked this pull request as ready for review
September 14, 2026 13:38
dhaifley
approved these changes
Sep 14, 2026
EItanya
approved these changes
Sep 16, 2026
EItanya
pushed a commit
to marosset/kagent
that referenced
this pull request
Sep 18, 2026
…ope (kagent-dev#2817) ### What this gives operators This PR adds a `global:` block. Each install-wide setting becomes one value. Helm passes `global.*` into subcharts automatically, so these values also reach the vendored subcharts as those adopt them. - **`global.imageRegistry`** — for air-gapped and private-registry installs. Mirror the images, then set this one value. Every image this chart composes pulls from the mirror: controller, ui, the bundled postgres, grafana-mcp, and the agent runtime the controller launches (`IMAGE_REGISTRY`). Repository paths and tags stay per-image, so a mirror copies each image under its existing path. - **`global.imagePullSecrets`** — the pull secret that mirror needs. The chart merges it (union) into each pod's own list. A local secret is never removed. - **`global.imagePullPolicy`** — one pull policy for the install. An explicitly set component policy or top-level `imagePullPolicy` still wins. - **`global.watchNamespaces`** — a namespace-scoped install in one value. A non-empty list renders Roles instead of ClusterRoles for the getter and writer RBAC. The controller's `WATCH_NAMESPACES` derives from the same list. RBAC scope and watch scope cannot disagree. Image references resolve through one helper, `kagent.images.image`. It implements the precedence in one place: a set global overrides the per-image registry, repository and tag stay per-image, and a digest pins the image in place of the tag. The name is chart-scoped because Helm `define` names are global across a release. A generic name could collide with another chart's helper, and the last-loaded copy would win silently. ### Not a breaking change Every new key is opt-in. A default render is byte-identical to main. Two behavior notes: - **One configuration that used to render now fails, on purpose.** Before this PR, `rbac.namespaces: [a]` with `controller.watchNamespaces: [a, b]` rendered successfully. The controller then watched namespace `b` with no Role in it. Every reconcile there returned `Forbidden` at runtime, with only a log line to show for it. That mix now fails the render, and the error names both keys and the fix. The only configurations that newly fail are ones that never worked. - **Multi-namespace RoleBindings now render in sorted order.** `writer-rolebinding.yaml` was the one rbac template without `sortAlpha`; the other three sorted. Object content is unchanged. Tooling that diffs rendered output may see a reorder once. ### Other compatibility notes - **An explicit `rbac.namespaces: []` opts out of the global.** An empty list has always meant "create ClusterRoles". The global is only a fallback, so it must not override that choice. But Helm's `coalesce` cannot see the choice: it skips empty values, so `[]` and "not set" look the same. The chart therefore checks key *presence* instead. Example: a parent sets `global.watchNamespaces: [team-a, team-b]` to scope its other subcharts. A kagent values file contains `rbac.namespaces: []`. That file keeps its ClusterRoles. Without the presence check, the same upgrade would silently replace them with Roles in `team-a` and `team-b`, and the controller would lose access everywhere else. One consequence: `values.yaml` no longer declares `namespaces: []` as a default. A declared empty default would make every install look explicitly cluster-scoped, and the global fallback would never fire. The key now ships commented out, with this explanation next to it. - **A global that omits kagent's namespace does not break the install.** The chart needs a Role where the controller runs. A parent may set `global.watchNamespaces` for its other subcharts and forget kagent's namespace. Failing the render for that would brick the whole umbrella install over a list that was never about kagent. So on the global path, the chart auto-appends its install namespace to the resolved scope. It also folds `controller.watchNamespaces` into that scope, so a wider watch gains matching Roles instead of failing. The hard errors remain for an explicit `rbac.namespaces`: a list without the install namespace, or a watch outside the list, stops the render and names the fix. - **Pull policies no longer declare `IfNotPresent` in values.** The top-level `imagePullPolicy` and the bundled postgres policy move the default into the template's fallback chain. The rendered output is the same. The reason: a declared default is always "set", so `global.imagePullPolicy` could never fire. With the default in the template, the global fallback is reachable. - **grafana-mcp (in this repo) adopts the globals too.** Its pod previously had no `imagePullSecrets` support at all; it gains the merge. Its `image.registry` value holds a docker.io org (`mcp`), not a host, so the mirror override prepends: the mirror serves `mcp/grafana` under its existing path. - **Coverage boundary.** The vendored subcharts (kagent-tools, kmcp, substrate) adopt the globals in their own repositories, and this chart picks them up as their pinned versions bump. The values comments state this boundary. Companion PRs: kagent-dev/tools#81, kagent-dev/kmcp#143, kagent-dev/substrate#38. ### Verification A default render is byte-identical to main for images, pull secrets, and RBAC kinds. The chart's test suite passes (288 tests), including a new `global-values` suite that pins: the registry override, per-image registry retention when the global is unset, the pull-secret merge, namespaced RBAC from the global alone, the install-namespace auto-append, the watch fold-in, the explicit-empty opt-out, local-wins precedence, and the watch-outside-scope failure. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Jonathan Jamroga <[email protected]>
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
3 participants
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.
What this gives operators
This PR adds install-wide values under the existing
global:block. Each setting becomes one value. Helm passesglobal.*down from a parent chart, so a parent that vendors kagent-tools (kagent does) sets each value once for its whole install.global.imageRegistry— for air-gapped and private-registry installs. Mirror the image, then set this one value. The chart pulls<your-registry>/kagent-dev/kagent/tools. Repository and tag stay per-image, so a mirror copies the image under its existing path.global.imagePullSecrets— the pull secret that mirror needs. The chart merges it (union) into the pod's own list. A local secret is never removed.global.imagePullPolicy— one pull policy for the install. An explicitly settools.image.pullPolicystill wins.global.watchNamespaces— a namespace-scoped install in one value. A non-empty list replaces the ClusterRole and ClusterRoleBinding with a Role and RoleBinding per namespace. ThenonResourceURLsrule is dropped in namespaced mode, because a namespaced Role cannot carry it.Image references resolve through one helper,
kagent-tools.images.image, with the same precedence the other kagent-family charts use: a set global overrides the per-image registry.Not a breaking change
Every new key is opt-in. A default render is byte-identical to main.
Compatibility notes
rbac.namespaces: []opts out of the global. An empty list has always meant "create the ClusterRole". The global is only a fallback, so it must not override that choice. Helm'scoalesceskips empty values, so[]and "not set" look the same; the chart checks key presence instead. A values file withrbac.namespaces: []keeps its ClusterRole even under a parent that setsglobal.watchNamespaces. One consequence:values.yamlno longer declaresnamespaces: []as a default. A declared empty default would make every install look explicitly cluster-scoped, and the global fallback would never fire. The key now ships commented out, with this explanation next to it.rbac.namespaces, where the operator is talking about this chart.tools.image.pullPolicyno longer declaresIfNotPresentin values. The template's fallback chain ends atIfNotPresentinstead. The rendered output is the same. A declared default is always "set", soglobal.imagePullPolicycould never fire; with the default in the template, the fallback is reachable.Verification
A default render is byte-identical to main. The chart's test suite passes (23 tests), including pins for: namespaced RBAC from the global alone, local-beats-global precedence, and the explicit-empty opt-out. The mirror override, the pull-secret merge, and the pull-policy fallback are verified by render.
🤖 Generated with Claude Code