diff --git a/.github/workflows/test-helm.yml b/.github/workflows/test-helm.yml new file mode 100644 index 0000000..8dc33b6 --- /dev/null +++ b/.github/workflows/test-helm.yml @@ -0,0 +1,34 @@ +name: Helm Tests + +on: + push: + branches: [main] + pull_request: + branches: [main] + +jobs: + helm-test: + name: Run on Ubuntu + runs-on: ubuntu-latest + steps: + - name: Clone the code + uses: actions/checkout@v4 + + - name: Install Helm + uses: azure/setup-helm@v4 + + - name: Lint the chart + run: make helm-lint + + # Pre-installed because modern helm verifies plugin provenance by + # default and the unittest plugin publishes none; the Makefile's own + # bootstrap (helm plugin install without --verify=false) predates that + # and fails on a fresh runner. With the plugin present, make helm-test + # skips its bootstrap. + - name: Install helm-unittest plugin + run: helm plugin install https://github.com/helm-unittest/helm-unittest --verify=false + + # Enforced here rather than developer-run only -- a green PR previously + # said nothing about the chart tests. + - name: Run chart unit tests + run: make helm-test diff --git a/helm/kmcp/templates/_helpers.tpl b/helm/kmcp/templates/_helpers.tpl index e71ec96..59e93db 100644 --- a/helm/kmcp/templates/_helpers.tpl +++ b/helm/kmcp/templates/_helpers.tpl @@ -65,10 +65,41 @@ Allows overriding it for multi-namespace deployments in combined charts. Create the image reference */}} {{- define "kmcp.image" -}} -{{- $tag := .Values.image.tag | default .Chart.AppVersion | default "latest" }} -{{- printf "%s:%s" .Values.image.repository $tag }} +{{- $tag := .Values.image.tag | default .Chart.AppVersion | default "latest" -}} +{{/* image.repository used to be one string carrying its registry + (ghcr.io/kagent-dev/kmcp/controller). It is now the environment-invariant + path only, joined onto image.registry -- the same registry/repository split + every kagent-family chart uses, so one global.imageRegistry value redirects + them all. A values file still carrying a host in repository would render a + doubled path that fails only at pod start, as ImagePullBackOff, so it fails + the render here instead and names the split. */}} +{{- $first := first (splitList "/" .Values.image.repository) -}} +{{- if or (contains "." $first) (contains ":" $first) -}} +{{- fail (printf "image.repository (%q) carries a registry host. It is now the image path only: move the host into image.registry (or global.imageRegistry) and keep repository as the path, e.g. registry: ghcr.io, repository: kagent-dev/kmcp/controller." .Values.image.repository) -}} +{{- end -}} +{{- include "kmcp.images.image" (dict "imageRoot" (dict "registry" .Values.image.registry "repository" .Values.image.repository "tag" $tag) "global" .Values.global) -}} {{- end }} +{{/* +The resolved RBAC scope, as a JSON list so callers can range over it. +Precedence: rbac.namespaces > global.watchNamespaces > empty (cluster-scoped). +The global is a fallback, not an override: a values file that sets rbac.namespaces +renders exactly what it rendered before the global existed, and an explicit empty +list forces cluster-scoped RBAC (hasKey, not coalesce, so a present-but-empty key +wins). On the global path the install namespace is auto-appended: the global is a +shared signal a parent may aim at other charts, and failing this chart's render +over it would brick an install the value was never about. +*/}} +{{- define "kmcp.rbacNamespaces" -}} +{{- $scope := list -}} +{{- if and .Values.rbac (hasKey .Values.rbac "namespaces") -}} +{{- $scope = .Values.rbac.namespaces | default list -}} +{{- else if ((.Values.global).watchNamespaces) -}} +{{- $scope = concat (.Values.global).watchNamespaces (list (include "kmcp.namespace" .)) -}} +{{- end -}} +{{- $scope | uniq | sortAlpha | toJson -}} +{{- end -}} + {{/* Guards on the rbac block */}} @@ -98,9 +129,29 @@ Create controller manager container args {{- if .Values.controller.metrics.enabled }} {{- $args = append $args (printf "--metrics-bind-address=%s" .Values.controller.metrics.bindAddress) }} {{- end }} -{{- if and .Values.rbac .Values.rbac.namespaces }} -{{- $namespaces := .Values.rbac.namespaces | uniq }} -{{- $args = append $args (printf "--watch-namespaces=%s" (join "," $namespaces)) }} +{{- $watchNs := include "kmcp.rbacNamespaces" . | fromJsonArray }} +{{- if $watchNs }} +{{- $args = append $args (printf "--watch-namespaces=%s" (join "," $watchNs)) }} {{- end }} {{- toYaml $args }} -{{- end }} \ No newline at end of file +{{- end }} +{{/* +Pull secrets for the pod: the chart's own list merged (union) with +global.imagePullSecrets. Renders nothing when both are empty. +*/}} +{{- define "kmcp.imagePullSecrets" -}} +{{- $merged := concat (.Values.imagePullSecrets | default list) (((.Values.global).imagePullSecrets) | default list) | uniq -}} +{{- if $merged -}} +imagePullSecrets: +{{- toYaml $merged | nindent 2 }} +{{- end -}} +{{- end -}} + +{{/* +imagePullPolicy for a container: the component's own value, then +global.imagePullPolicy, then IfNotPresent. One definition so the fallback chain +cannot drift between pods. +*/}} +{{- define "kmcp.imagePullPolicy" -}} +{{- .local | default (((.root.Values.global)).imagePullPolicy) | default "IfNotPresent" -}} +{{- end -}} diff --git a/helm/kmcp/templates/_images.tpl b/helm/kmcp/templates/_images.tpl new file mode 100644 index 0000000..b707613 --- /dev/null +++ b/helm/kmcp/templates/_images.tpl @@ -0,0 +1,32 @@ +{{/* +Copyright Broadcom, Inc. All Rights Reserved. +SPDX-License-Identifier: APACHE-2.0 +*/}} + +{{/* vim: set filetype=mustache: */}} +{{/* +Return the proper image name. +If image tag and digest are not defined, termination fallbacks to chart appVersion. +{{ include "kmcp.images.image" ( dict "imageRoot" .Values.path.to.the.image "global" .Values.global "chart" .Chart ) }} +*/}} +{{- define "kmcp.images.image" -}} +{{- $registryName := default .imageRoot.registry ((.global).imageRegistry) -}} +{{- $repositoryName := .imageRoot.repository -}} +{{- $separator := ":" -}} +{{- $termination := .imageRoot.tag | toString -}} + +{{- if not .imageRoot.tag }} + {{- if .chart }} + {{- $termination = .chart.AppVersion | toString -}} + {{- end -}} +{{- end -}} +{{- if .imageRoot.digest }} + {{- $separator = "@" -}} + {{- $termination = .imageRoot.digest | toString -}} +{{- end -}} +{{- if $registryName }} + {{- printf "%s/%s%s%s" $registryName $repositoryName $separator $termination -}} +{{- else -}} + {{- printf "%s%s%s" $repositoryName $separator $termination -}} +{{- end -}} +{{- end -}} diff --git a/helm/kmcp/templates/deployment.yaml b/helm/kmcp/templates/deployment.yaml index bb044fc..a50de3b 100644 --- a/helm/kmcp/templates/deployment.yaml +++ b/helm/kmcp/templates/deployment.yaml @@ -20,17 +20,14 @@ spec: labels: {{- include "kmcp.selectorLabels" . | nindent 8 }} spec: - {{- with .Values.imagePullSecrets }} - imagePullSecrets: - {{- toYaml . | nindent 8 }} - {{- end }} + {{- with include "kmcp.imagePullSecrets" . }}{{- . | nindent 6 }}{{- end }} serviceAccountName: {{ include "kmcp.serviceAccountName" . }} securityContext: {{- toYaml .Values.podSecurityContext | nindent 8 }} containers: - name: manager image: {{ include "kmcp.image" . }} - imagePullPolicy: {{ .Values.image.pullPolicy }} + imagePullPolicy: {{ include "kmcp.imagePullPolicy" (dict "root" . "local" .Values.image.pullPolicy) }} command: - /manager args: diff --git a/helm/kmcp/templates/rbac/clusterrole.yaml b/helm/kmcp/templates/rbac/clusterrole.yaml index a30d106..1e510a3 100644 --- a/helm/kmcp/templates/rbac/clusterrole.yaml +++ b/helm/kmcp/templates/rbac/clusterrole.yaml @@ -55,8 +55,9 @@ {{- if .Values.rbac.create }} {{- include "kmcp.rbac.validate" . -}} -{{- if .Values.rbac.namespaces }} -{{- range $namespace := (.Values.rbac.namespaces | uniq | sortAlpha) }} +{{- $rbacNamespaces := include "kmcp.rbacNamespaces" . | fromJsonArray }} +{{- if $rbacNamespaces }} +{{- range $namespace := $rbacNamespaces }} --- apiVersion: rbac.authorization.k8s.io/v1 kind: Role diff --git a/helm/kmcp/templates/rbac/clusterrolebinding.yaml b/helm/kmcp/templates/rbac/clusterrolebinding.yaml index 4e93a75..d8be6cd 100644 --- a/helm/kmcp/templates/rbac/clusterrolebinding.yaml +++ b/helm/kmcp/templates/rbac/clusterrolebinding.yaml @@ -1,7 +1,8 @@ {{- if .Values.rbac.create }} {{- include "kmcp.rbac.validate" . -}} -{{- if .Values.rbac.namespaces }} -{{- range $namespace := (.Values.rbac.namespaces | uniq | sortAlpha) }} +{{- $rbacNamespaces := include "kmcp.rbacNamespaces" . | fromJsonArray }} +{{- if $rbacNamespaces }} +{{- range $namespace := $rbacNamespaces }} --- apiVersion: rbac.authorization.k8s.io/v1 kind: RoleBinding diff --git a/helm/kmcp/tests/__snapshot__/deployment_test.yaml.snap b/helm/kmcp/tests/__snapshot__/deployment_test.yaml.snap index f7bf18a..4221046 100644 --- a/helm/kmcp/tests/__snapshot__/deployment_test.yaml.snap +++ b/helm/kmcp/tests/__snapshot__/deployment_test.yaml.snap @@ -34,7 +34,7 @@ should create deployment with default values: - --metrics-bind-address=:8443 command: - /manager - image: test-repo:v1.0.0 + image: ghcr.io/test-repo:v1.0.0 imagePullPolicy: IfNotPresent livenessProbe: httpGet: @@ -112,7 +112,7 @@ should include health probe ports when health probe enabled: - --metrics-bind-address=:8443 command: - /manager - image: test-repo:v1.0.0 + image: ghcr.io/test-repo:v1.0.0 imagePullPolicy: IfNotPresent livenessProbe: httpGet: @@ -190,7 +190,7 @@ should include image pull secrets when specified: - --metrics-bind-address=:8443 command: - /manager - image: test-repo:v1.0.0 + image: ghcr.io/test-repo:v1.0.0 imagePullPolicy: IfNotPresent livenessProbe: httpGet: @@ -270,7 +270,7 @@ should include metrics port when metrics enabled: - --metrics-bind-address=:8443 command: - /manager - image: test-repo:v1.0.0 + image: ghcr.io/test-repo:v1.0.0 imagePullPolicy: IfNotPresent livenessProbe: httpGet: @@ -348,7 +348,7 @@ should include node selector when specified: - --metrics-bind-address=:8443 command: - /manager - image: test-repo:v1.0.0 + image: ghcr.io/test-repo:v1.0.0 imagePullPolicy: IfNotPresent livenessProbe: httpGet: @@ -430,7 +430,7 @@ should include pod annotations when specified: - --metrics-bind-address=:8443 command: - /manager - image: test-repo:v1.0.0 + image: ghcr.io/test-repo:v1.0.0 imagePullPolicy: IfNotPresent livenessProbe: httpGet: @@ -508,7 +508,7 @@ should include tolerations when specified: - --metrics-bind-address=:8443 command: - /manager - image: test-repo:v1.0.0 + image: ghcr.io/test-repo:v1.0.0 imagePullPolicy: IfNotPresent livenessProbe: httpGet: @@ -591,7 +591,7 @@ should set custom replica count: - --metrics-bind-address=:8443 command: - /manager - image: test-repo:v1.0.0 + image: ghcr.io/test-repo:v1.0.0 imagePullPolicy: IfNotPresent livenessProbe: httpGet: @@ -669,7 +669,7 @@ should set custom resources: - --metrics-bind-address=:8443 command: - /manager - image: test-repo:v1.0.0 + image: ghcr.io/test-repo:v1.0.0 imagePullPolicy: IfNotPresent livenessProbe: httpGet: @@ -747,7 +747,7 @@ should set pod security context: - --metrics-bind-address=:8443 command: - /manager - image: test-repo:v1.0.0 + image: ghcr.io/test-repo:v1.0.0 imagePullPolicy: IfNotPresent livenessProbe: httpGet: @@ -825,7 +825,7 @@ should set security context: - --metrics-bind-address=:8443 command: - /manager - image: test-repo:v1.0.0 + image: ghcr.io/test-repo:v1.0.0 imagePullPolicy: IfNotPresent livenessProbe: httpGet: @@ -903,7 +903,7 @@ should set termination grace period: - --metrics-bind-address=:8443 command: - /manager - image: test-repo:v1.0.0 + image: ghcr.io/test-repo:v1.0.0 imagePullPolicy: IfNotPresent livenessProbe: httpGet: diff --git a/helm/kmcp/tests/__snapshot__/integration_test.yaml.snap b/helm/kmcp/tests/__snapshot__/integration_test.yaml.snap index 1db023f..1a32e21 100644 --- a/helm/kmcp/tests/__snapshot__/integration_test.yaml.snap +++ b/helm/kmcp/tests/__snapshot__/integration_test.yaml.snap @@ -34,7 +34,7 @@ should create deployment when metrics enabled: - --metrics-bind-address=:8443 command: - /manager - image: test-repo:v1.0.0 + image: ghcr.io/test-repo:v1.0.0 imagePullPolicy: IfNotPresent livenessProbe: httpGet: @@ -112,7 +112,7 @@ should create deployment with default values: - --metrics-bind-address=:8443 command: - /manager - image: test-repo:v1.0.0 + image: ghcr.io/test-repo:v1.0.0 imagePullPolicy: IfNotPresent livenessProbe: httpGet: @@ -189,7 +189,7 @@ should create deployment with minimal configuration: - --health-probe-bind-address=:8081 command: - /manager - image: test-repo:v1.0.0 + image: ghcr.io/test-repo:v1.0.0 imagePullPolicy: IfNotPresent livenessProbe: httpGet: @@ -264,7 +264,7 @@ should include metrics port in deployment when metrics enabled: - --metrics-bind-address=:8443 command: - /manager - image: test-repo:v1.0.0 + image: ghcr.io/test-repo:v1.0.0 imagePullPolicy: IfNotPresent livenessProbe: httpGet: @@ -342,7 +342,7 @@ should use custom namespace: - --metrics-bind-address=:8443 command: - /manager - image: test-repo:v1.0.0 + image: ghcr.io/test-repo:v1.0.0 imagePullPolicy: IfNotPresent livenessProbe: httpGet: @@ -420,7 +420,7 @@ should use custom release name: - --metrics-bind-address=:8443 command: - /manager - image: test-repo:v1.0.0 + image: ghcr.io/test-repo:v1.0.0 imagePullPolicy: IfNotPresent livenessProbe: httpGet: diff --git a/helm/kmcp/tests/global_values_test.yaml b/helm/kmcp/tests/global_values_test.yaml new file mode 100644 index 0000000..9d5d26b --- /dev/null +++ b/helm/kmcp/tests/global_values_test.yaml @@ -0,0 +1,98 @@ +suite: Test global values +templates: + - deployment.yaml + - rbac/clusterrole.yaml +tests: + - it: should override the registry from global.imageRegistry + set: + global.imageRegistry: mirror.example + template: deployment.yaml + asserts: + - matchRegex: + path: spec.template.spec.containers[0].image + pattern: "^mirror\\.example/kagent-dev/kmcp/controller:" + + - it: should let a local image.registry override the default + set: + image.registry: my.registry.example + template: deployment.yaml + asserts: + - matchRegex: + path: spec.template.spec.containers[0].image + pattern: "^my\\.registry\\.example/kagent-dev/kmcp/controller:" + + - it: should render an unqualified image when registry is explicitly empty + set: + image.registry: "" + image.repository: test-repo + image.tag: v1.0.0 + template: deployment.yaml + asserts: + - equal: + path: spec.template.spec.containers[0].image + value: test-repo:v1.0.0 + + - it: should fail when repository carries a registry host (the old shape) + set: + image.repository: my.registry.example/platform/kmcp-controller + template: deployment.yaml + asserts: + - failedTemplate: + errorPattern: "carries a registry host" + + - it: should merge global.imagePullSecrets with the local list + set: + imagePullSecrets: + - name: local + global.imagePullSecrets: + - name: shared + template: deployment.yaml + asserts: + - contains: + path: spec.template.spec.imagePullSecrets + content: + name: local + - contains: + path: spec.template.spec.imagePullSecrets + content: + name: shared + + - it: should fall back to global.imagePullPolicy when image.pullPolicy is unset + set: + global.imagePullPolicy: Always + template: deployment.yaml + asserts: + - equal: + path: spec.template.spec.containers[0].imagePullPolicy + value: Always + + - it: should render namespaced RBAC from global.watchNamespaces, install namespace appended + release: + namespace: kmcp-system + set: + global.watchNamespaces: + - team-a + template: rbac/clusterrole.yaml + asserts: + - containsDocument: + any: true + apiVersion: rbac.authorization.k8s.io/v1 + kind: Role + namespace: team-a + - containsDocument: + any: true + apiVersion: rbac.authorization.k8s.io/v1 + kind: Role + namespace: kmcp-system + + - it: should keep ClusterRoles when rbac.namespaces is an explicit empty list + set: + rbac.namespaces: [] + global.watchNamespaces: + - team-a + template: rbac/clusterrole.yaml + asserts: + - containsDocument: + any: true + apiVersion: rbac.authorization.k8s.io/v1 + kind: ClusterRole diff --git a/helm/kmcp/values.yaml b/helm/kmcp/values.yaml index efd8b7d..e1183b0 100644 --- a/helm/kmcp/values.yaml +++ b/helm/kmcp/values.yaml @@ -10,9 +10,35 @@ fullnameOverride: "" namespaceOverride: "" # Image configuration +# Values under `global` are visible to this chart and every subchart. A parent chart +# (or an operator) sets one value here instead of one per chart. +global: + # -- Registry that overrides image.registry when set. This is the air-gap + # mirror knob: one value redirects the image. image.repository stays untouched, + # so a mirror serves the image under its existing kagent-dev/kmcp/controller + # path. For control without the override, leave this unset and set + # image.registry. + imageRegistry: "" + # -- Pull secrets merged (union) into the pod's own imagePullSecrets list. + imagePullSecrets: [] + # -- Fallback imagePullPolicy when image.pullPolicy is unset. + imagePullPolicy: "" + # -- Namespace scope fallback. rbac.namespaces overrides it when the key is + # present. A non-empty resolved list replaces ClusterRoles with Roles and + # scopes the controller's watch. + watchNamespaces: [] + image: - repository: ghcr.io/kagent-dev/kmcp/controller - pullPolicy: IfNotPresent + # -- Registry host for the image, and nothing else. To change environments (a + # private mirror, an air-gapped registry), change only this value or + # global.imageRegistry, which overrides it. A host inside `repository` fails + # the render, and the error names this split. + registry: ghcr.io + # -- Image path under the registry. The path is identical on every registry + # that serves the image. A mirror copies the image under this same path. + repository: kagent-dev/kmcp/controller + # -- Pull policy. Empty falls back to global.imagePullPolicy, then IfNotPresent. + pullPolicy: "" # Overrides the image tag whose default is the chart's appVersion. # Set to a specific version (e.g., "v0.1.7") to pin to that version. # tag: "" @@ -99,9 +125,14 @@ rbac: # -- Namespaces in which to create Role and RoleBinding resources. # If empty (default), the chart creates cluster-scoped ClusterRole and ClusterRoleBinding # resources and the controller watches all namespaces. - # If set, the chart creates a Role + RoleBinding per listed namespace and the controller's - # WATCH_NAMESPACES is derived from this list. - namespaces: [] + # If set, the chart creates a Role + RoleBinding per listed namespace and the controller + # receives the same list as its --watch-namespaces flag, so RBAC scope and watch scope + # cannot disagree. Left unset, the scope falls back to global.watchNamespaces. + # An explicit empty list forces cluster-scoped RBAC, even under a parent that + # sets the global. This is why the key ships commented out: a declared default + # [] would make every install look explicitly cluster-scoped, and the fallback + # would be unreachable. + # namespaces: [] # Service configuration for metrics service: diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 753c608..b231e36 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -61,7 +61,8 @@ var _ = ginkgo.Describe("Manager", ginkgo.Ordered, func() { cmd = exec.Command("helm", "install", "kmcp", "helm/kmcp", "--namespace", namespace, "--wait", "--timeout=5m", - "--set", fmt.Sprintf("image.repository=%s", getImageRepository(projectImage)), + "--set", fmt.Sprintf("image.registry=%s", getImageRegistry(projectImage)), + "--set", fmt.Sprintf("image.repository=%s", getImagePath(projectImage)), "--set", fmt.Sprintf("image.tag=%s", getImageTag(projectImage)), "--set", "image.pullPolicy=Never") _, err = utils.Run(cmd) @@ -418,8 +419,8 @@ func getService(name, namespace string) *corev1.Service { return &service } -// getImageRepository extracts the repository part from a full image name -// e.g., "example.com/kmcp:v0.0.1" -> "example.com/kmcp" +// getImageRepository extracts everything before the tag from a full image name +// e.g., "example.com/org/kmcp:v0.0.1" -> "example.com/org/kmcp" func getImageRepository(image string) string { if idx := strings.LastIndex(image, ":"); idx != -1 { return image[:idx] @@ -427,6 +428,31 @@ func getImageRepository(image string) string { return image } +// getImageRegistry extracts the registry host from a full image name, matching +// the chart's registry/repository split. The first path segment is a registry +// when it contains "." or ":" (the containerd rule). +// e.g., "example.com/org/kmcp:v0.0.1" -> "example.com" +func getImageRegistry(image string) string { + repo := getImageRepository(image) + if idx := strings.Index(repo, "/"); idx != -1 { + if first := repo[:idx]; strings.ContainsAny(first, ".:") { + return first + } + } + return "" +} + +// getImagePath extracts the image path without registry host or tag, matching +// the chart's image.repository value. +// e.g., "example.com/org/kmcp:v0.0.1" -> "org/kmcp" +func getImagePath(image string) string { + repo := getImageRepository(image) + if registry := getImageRegistry(image); registry != "" { + return strings.TrimPrefix(repo, registry+"/") + } + return repo +} + // getImageTag extracts the tag part from a full image name // e.g., "example.com/kmcp:v0.0.1" -> "v0.0.1" func getImageTag(image string) string {