Skip to content

feat(helm): global values for registry, pull config, and namespace scope - #2817

Open
jjamroga wants to merge 1 commit into
kagent-dev:mainfrom
jjamroga:jjamroga/helm-global-values
Open

jjamroga wants to merge 1 commit into
kagent-dev:mainfrom
jjamroga:jjamroga/helm-global-values

Conversation

@jjamroga

Copy link
Copy Markdown
Collaborator

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: feat(helm): global values for registry, pull config, and namespace scope tools#81, feat(helm): global values, and the registry/repository image split kmcp#143, feat(helm): global image values, and the registry/repository split 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

global.imageRegistry overrides every per-image registry at once -- the
air-gap mirror knob, resolved by a shared kagent.images.image helper so
repository and tag stay per-image and a mirror serves each image under
its existing path. global.imagePullSecrets merges (union) into each
pod's own list via kagent.imagePullSecrets. global.imagePullPolicy
fills the gap where neither a component nor the top-level
imagePullPolicy sets one, via kagent.imagePullPolicy; declared
IfNotPresent defaults moved into the template chains 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 and any
controller.watchNamespaces entries are folded into the resolved scope
rather than failing the render. For an explicit rbac.namespaces, a
watch outside the list fails the render and names both keys: a watched
namespace without a Role is a permanent Forbidden loop at runtime.
writer-rolebinding gains the sortAlpha the other three rbac templates
already had. grafana-mcp adopts the globals in the same change, and
gains the pull-secret surface its pod spec lacked entirely.

Verified: helm unittest passes (288 tests, including a global-values
suite pinning override, merge, fallback, fold-in, auto-append,
explicit-empty opt-out, and the watch-outside-scope failure); a default
render is byte-identical to main for images, pull secrets, and RBAC
kinds.

Signed-off-by: Jonathan Jamroga <jjamroga@gmail.com>
@github-actions github-actions Bot added the enhancement New feature or request label Sep 14, 2026
@jjamroga
jjamroga marked this pull request as ready for review September 14, 2026 13:41
@jjamroga
jjamroga requested a review from a team as a code owner September 14, 2026 13:41
@@ -59,21 +59,66 @@ Precedence: controller.watchNamespaces (explicit override) > rbac.namespaces > e
{{- .Values.controller.watchNamespaces | uniq | join "," -}}
{{- else if and .Values.rbac .Values.rbac.namespaces -}}
{{- .Values.rbac.namespaces | uniq | join "," -}}
{{- else if ((.Values.global).watchNamespaces) -}}
{{- (.Values.global).watchNamespaces | uniq | join "," -}}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm reading this right, the watch helper reads the raw global while RBAC uses the resolved list. With release namespace kagent and only global.watchNamespaces=[team-a], the chart creates Roles in both namespaces but emits WATCH_NAMESPACES=team-a, so the install namespace is missing from both controller caches and Harnesses, AgentTemplates, ModelConfigs and MCP resources there are not watched (go/core/pkg/app/app.go:203-224 configures both caches from this value, and go/core/internal/controller/collections.go:60-70 applies it to the collections). The auto-append test only checks the Role.

There's a second effect of the separate fallback. With rbac.namespaces=[] and that same global, RBAC correctly goes cluster-wide but the watch stays restricted to team-a, which changes an existing explicitly cluster-wide configuration. Would it make sense to keep the controller.watchNamespaces override, then derive the default watch from kagent.rbacNamespaces with its key-presence and install-namespace rules, and test WATCH_NAMESPACES in both cases?

@@ -12,9 +12,10 @@ data:
{{- if .Values.ui.externalUrl }}
KAGENT_UI_URL: {{ .Values.ui.externalUrl | quote }}
{{- end }}
IMAGE_REGISTRY: {{ .Values.controller.agentImage.registry | default .Values.registry | quote }}
IMAGE_REGISTRY: {{ ((.Values.global).imageRegistry) | default (.Values.controller.agentImage.registry | default .Values.registry) | quote }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that changing IMAGE_REGISTRY redirects agent runtime images in the current controller. I didn't see a Go reader of IMAGE_REGISTRY, IMAGE_REPOSITORY or IMAGE_TAG at this head, and the harness compilers copy harness.Spec.Workload.Image into the revision (go/core/internal/translator/kagent/compiler.go:90). The optional substrate-workerpool.yaml also emits substrateWorkerPool.workerImage unchanged (line 18).

When I test rendered a WorkerPool under global.imageRegistry=mirror.example, its ghcr.io/.../ateom:v1 stayed on ghcr.io. The new ConfigMap test passes without showing that an agent can run from the mirror. Could the global be wired to the actual runtime image configuration, with a test that inspects the resulting runtime or WorkerPool image?

*/}}
{{- define "grafana-mcp.imagePullPolicy" -}}
{{- .Values.image.pullPolicy | default ((.Values.global).imagePullPolicy) | default "IfNotPresent" -}}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this fallback is unreachable for a normal install, since image.pullPolicy still defaults to Always in this subchart's values (values.yaml:9-13). With only global.imagePullPolicy=Never, the controller, UI, tools and kmcp all use Never, while grafana-mcp, which is enabled by default, still uses Always and contacts the registry. Would it work to move the default out of values and into the helper's final fallback, keeping Always as this chart's default when neither a local nor a global policy is set? An explicit local policy would still win, and the existing latest image behavior wouldn't change.

{{/* image.registry holds the docker.io org here ("mcp"), not a host, so the
air-gap override is prepended rather than substituted: the mirror serves
the image under its existing mcp/grafana path. */}}
{{- $parts := compact (list ((.Values.global).imageRegistry) $img.registry $img.repository $img.name) -}}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prepend works for the default Docker Hub shorthand (mcp/grafana), but image.registry is operator-configurable and can already hold a host. image.registry=docker.io, image.repository=mcp, image.name=grafana renders a valid docker.io/mcp/grafana:latest today, and with global.imageRegistry=mirror.example this helper emits mirror.example/docker.io/mcp/grafana:latest, which adds a directory rather than replacing the registry and disagrees with every other image override in the stack. Could the default shorthand be normalized to a registry plus repository, or the helper tell a real host apart from the shorthand before applying the global?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants