From 7f7fff69401ee85e9632bd9be7ff6165342517ab Mon Sep 17 00:00:00 2001 From: Ray Low Date: Thu, 9 Jul 2026 12:15:36 +0800 Subject: [PATCH] fixed operator failed to watch multiple namespaces --- chart/jenkins-operator/templates/_helpers.tpl | 15 ++ .../jenkins-operator/templates/operator.yaml | 8 +- chart/jenkins-operator/templates/role.yaml | 9 +- .../templates/role_binding.yaml | 7 +- chart/jenkins-operator/values.yaml | 3 + cmd/main.go | 22 ++- cmd/main_test.go | 50 ++++++ test/chart/render_test.go | 163 ++++++++++++++++++ 8 files changed, 262 insertions(+), 15 deletions(-) create mode 100644 cmd/main_test.go create mode 100644 test/chart/render_test.go diff --git a/chart/jenkins-operator/templates/_helpers.tpl b/chart/jenkins-operator/templates/_helpers.tpl index df05cf14..b4df6f72 100644 --- a/chart/jenkins-operator/templates/_helpers.tpl +++ b/chart/jenkins-operator/templates/_helpers.tpl @@ -31,6 +31,21 @@ Create chart name and version as used by the chart label. {{- printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 | trimSuffix "-" -}} {{- end -}} +{{/* +Returns "true" when the operator should watch all namespaces (WATCH_NAMESPACE=""). +This happens when the bundled Jenkins is enabled with an empty jenkins.namespace, or +when jenkins is disabled and operator.watchNamespace is explicitly set to "". +An empty string is falsy in templates, so operator.watchNamespace is detected with +hasKey to distinguish "set to empty (all namespaces)" from "unset (own namespace)". +*/}} +{{- define "jenkins-operator.watchAllNamespaces" -}} +{{- if .Values.jenkins.enabled -}} +{{- if eq .Values.jenkins.namespace "" -}}true{{- end -}} +{{- else if hasKey .Values.operator "watchNamespace" -}} +{{- if eq .Values.operator.watchNamespace "" -}}true{{- end -}} +{{- end -}} +{{- end -}} + {{/* Common labels */}} diff --git a/chart/jenkins-operator/templates/operator.yaml b/chart/jenkins-operator/templates/operator.yaml index 967d2264..eed18996 100644 --- a/chart/jenkins-operator/templates/operator.yaml +++ b/chart/jenkins-operator/templates/operator.yaml @@ -55,10 +55,12 @@ spec: periodSeconds: 10 env: - name: WATCH_NAMESPACE - {{- if .Values.jenkins.enabled }} - value: {{ .Values.jenkins.namespace }} + {{- if eq (include "jenkins-operator.watchAllNamespaces" .) "true" }} + value: "" + {{- else if .Values.jenkins.enabled }} + value: {{ .Values.jenkins.namespace | quote }} {{- else if .Values.operator.watchNamespace }} - value: {{ .Values.operator.watchNamespace }} + value: {{ .Values.operator.watchNamespace | quote }} {{- else }} valueFrom: fieldRef: diff --git a/chart/jenkins-operator/templates/role.yaml b/chart/jenkins-operator/templates/role.yaml index 07bdec63..f50b9468 100644 --- a/chart/jenkins-operator/templates/role.yaml +++ b/chart/jenkins-operator/templates/role.yaml @@ -1,12 +1,11 @@ -{{ if eq .Values.jenkins.namespace "" }} +{{ if eq (include "jenkins-operator.watchAllNamespaces" .) "true" }} {{- /* -# This is a special case when .Values.jenkins.namespace is equal to empty -# string which leads to WATCH_NAMESPACE env of jenkins-operator to be set to -# empty string and leads to operator actually watching all namespaces. In this +# Special case where WATCH_NAMESPACE is set to an empty string, which makes the +# operator watch all namespaces (see jenkins-operator.watchAllNamespaces). In this # case we need to create clusterrole and clusterrolebinding instead of role and # rolebinding */ -}} - {{- template "jenkins-operator.role" .Values.jenkins.namespace }} + {{- template "jenkins-operator.role" "" }} {{ else }} {{- template "jenkins-operator.role" .Release.Namespace }} {{- if ne .Release.Namespace .Values.jenkins.namespace -}} diff --git a/chart/jenkins-operator/templates/role_binding.yaml b/chart/jenkins-operator/templates/role_binding.yaml index c36b6a2a..1b986d50 100644 --- a/chart/jenkins-operator/templates/role_binding.yaml +++ b/chart/jenkins-operator/templates/role_binding.yaml @@ -1,8 +1,7 @@ -{{ if eq .Values.jenkins.namespace "" }} +{{ if eq (include "jenkins-operator.watchAllNamespaces" .) "true" }} {{- /* -# This is a special case when .Values.jenkins.namespace is equal to empty -# string which leads to WATCH_NAMESPACE env of jenkins-operator to be set to -# empty string and leads to operator actually watching all namespaces. In this +# Special case where WATCH_NAMESPACE is set to an empty string, which makes the +# operator watch all namespaces (see jenkins-operator.watchAllNamespaces). In this # case we need to create clusterrole and clusterrolebinding instead of role and # rolebinding */ -}} diff --git a/chart/jenkins-operator/values.yaml b/chart/jenkins-operator/values.yaml index 2335465e..6ad3be1c 100644 --- a/chart/jenkins-operator/values.yaml +++ b/chart/jenkins-operator/values.yaml @@ -336,6 +336,9 @@ operator: # Select a different namespace to look for the Jenkins CR and deploy Jenkins in. Defaults to the same namespace as # the operator. + # Only used when jenkins.enabled is false. + # Set to an empty string ("") to make a single operator watch all namespaces; this also switches the operator's + # RBAC to a ClusterRole/ClusterRoleBinding. # watchNamespace: "jenkins-namespace" resources: {} diff --git a/cmd/main.go b/cmd/main.go index a2caf3cb..34546991 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -129,8 +129,7 @@ func main() { fatal(errors.Wrap(err, "failed to get config"), *debug) } - cacheNamespace := map[string]cache.Config{} - cacheNamespace[namespace] = cache.Config{} + cacheOptions := buildCacheOptions(namespace) mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ // MetricsBindAddress: fmt.Sprintf("%s:%d", metricsHost, metricsPort), Metrics: server.Options{ @@ -145,7 +144,7 @@ func main() { LeaderElection: enableLeaderElection, LeaderElectionID: "c674355f.jenkins.io", // Namespace: namespace, - Cache: cache.Options{DefaultNamespaces: cacheNamespace}, + Cache: cacheOptions, }) if err != nil { fatal(errors.Wrap(err, "unable to start manager"), *debug) @@ -212,6 +211,23 @@ func main() { } } +// buildCacheOptions returns the controller-runtime cache options for the given +// WATCH_NAMESPACE value. +// +// An empty namespace means "watch all namespaces". In that case we must leave +// Cache.DefaultNamespaces unset so controller-runtime builds a single +// cluster-wide cache. Setting DefaultNamespaces{"": {}} instead builds a +// multiNamespaceCache keyed only by "", which cannot serve namespace-scoped +// List calls (e.g. ensureExtraRBAC lists RoleBindings InNamespace(jenkins.Namespace)) +// and fails with "unable to list: because of unknown namespace for the cache". +func buildCacheOptions(namespace string) cache.Options { + cacheOptions := cache.Options{} + if namespace != "" { + cacheOptions.DefaultNamespaces = map[string]cache.Config{namespace: {}} + } + return cacheOptions +} + func fatal(err error, debug bool) { if debug { logger.Error(nil, fmt.Sprintf("%+v", err)) diff --git a/cmd/main_test.go b/cmd/main_test.go new file mode 100644 index 00000000..c2e1ecf9 --- /dev/null +++ b/cmd/main_test.go @@ -0,0 +1,50 @@ +package main + +import ( + "testing" +) + +func TestBuildCacheOptions(t *testing.T) { + t.Run("empty namespace watches all namespaces with a cluster-wide cache", func(t *testing.T) { + opts := buildCacheOptions("") + + // DefaultNamespaces must stay nil so controller-runtime builds a single + // cluster-wide cache that can serve namespace-scoped List calls. + if opts.DefaultNamespaces != nil { + t.Errorf("expected DefaultNamespaces to be nil for all-namespaces mode, got %v", opts.DefaultNamespaces) + } + }) + + t.Run("single namespace scopes the cache to that namespace", func(t *testing.T) { + opts := buildCacheOptions("jenkins") + + if len(opts.DefaultNamespaces) != 1 { + t.Fatalf("expected exactly one namespace in DefaultNamespaces, got %d", len(opts.DefaultNamespaces)) + } + if _, ok := opts.DefaultNamespaces["jenkins"]; !ok { + t.Errorf("expected DefaultNamespaces to be keyed by %q, got %v", "jenkins", opts.DefaultNamespaces) + } + }) + + t.Run("does not use the empty-string key that breaks namespace-scoped lists", func(t *testing.T) { + // Regression guard: DefaultNamespaces{"": {}} builds a multiNamespaceCache + // keyed only by "" and fails with "unknown namespace for the cache". + opts := buildCacheOptions("") + if _, ok := opts.DefaultNamespaces[""]; ok { + t.Error("DefaultNamespaces must not contain the empty-string key in all-namespaces mode") + } + }) + + // A scoped namespace should carry no extra restrictions (default cache.Config). + t.Run("scoped namespace uses a default cache.Config", func(t *testing.T) { + opts := buildCacheOptions("team-a") + got, ok := opts.DefaultNamespaces["team-a"] + if !ok { + t.Fatalf("expected namespace %q to be present", "team-a") + } + // cache.Config is not comparable (it holds a func), so assert on its fields. + if got.LabelSelector != nil || got.FieldSelector != nil { + t.Errorf("expected no selectors on scoped namespace config, got %+v", got) + } + }) +} diff --git a/test/chart/render_test.go b/test/chart/render_test.go new file mode 100644 index 00000000..7fe43311 --- /dev/null +++ b/test/chart/render_test.go @@ -0,0 +1,163 @@ +// Package chart holds lightweight `helm template` rendering tests for the +// jenkins-operator chart. They do not need a Kubernetes cluster: they only +// assert that the chart renders the expected manifests for a given set of +// values. They are excluded from the heavyweight e2e suites in test/helm. +package chart + +import ( + "os/exec" + "strings" + "testing" +) + +const chartPath = "../../chart/jenkins-operator" + +// helmTemplate renders a single chart template with the given --set overrides +// and returns its stdout. The test is skipped when helm is not installed. +func helmTemplate(t *testing.T, showOnly string, sets ...string) string { + t.Helper() + if _, err := exec.LookPath("helm"); err != nil { + t.Skip("helm binary not found in PATH; skipping chart render test") + } + + args := []string{"template", "test-release", chartPath, "--show-only", showOnly} + for _, s := range sets { + args = append(args, "--set", s) + } + + out, err := exec.Command("helm", args...).CombinedOutput() + if err != nil { + t.Fatalf("helm template %v failed: %v\n%s", args, err, out) + } + return string(out) +} + +// watchNamespaceValue extracts the value the chart assigned to the +// WATCH_NAMESPACE env var. It returns the raw literal (e.g. `""` or +// `"team-a"`), or "valueFrom" when the operator falls back to the downward API. +func watchNamespaceValue(t *testing.T, rendered string) string { + t.Helper() + lines := strings.Split(rendered, "\n") + for i, l := range lines { + if !strings.Contains(l, "name: WATCH_NAMESPACE") { + continue + } + for j := i + 1; j < len(lines) && j <= i+4; j++ { + s := strings.TrimSpace(lines[j]) + switch { + case strings.HasPrefix(s, "value:"): + return strings.TrimSpace(strings.TrimPrefix(s, "value:")) + case strings.HasPrefix(s, "valueFrom:"): + return "valueFrom" + } + } + } + t.Fatalf("WATCH_NAMESPACE env var not found in rendered output:\n%s", rendered) + return "" +} + +// hasKindLine reports whether the rendered manifest declares the given kind. +// It matches the whole line so "Role" does not match "RoleBinding". +func hasKindLine(rendered, kind string) bool { + for _, l := range strings.Split(rendered, "\n") { + if strings.TrimSpace(l) == "kind: "+kind { + return true + } + } + return false +} + +// TestWatchNamespaceEnv covers how the chart derives the operator's +// WATCH_NAMESPACE env var. An empty value means "watch all namespaces". +func TestWatchNamespaceEnv(t *testing.T) { + cases := []struct { + name string + sets []string + want string + }{ + { + name: "bundled jenkins with default namespace is scoped", + sets: nil, + want: `"default"`, + }, + { + name: "bundled jenkins with empty namespace watches all namespaces", + sets: []string{"jenkins.namespace="}, + want: `""`, + }, + { + name: "standalone operator with explicit watchNamespace is scoped", + sets: []string{"jenkins.enabled=false", "operator.watchNamespace=team-a"}, + want: `"team-a"`, + }, + { + name: "standalone operator with empty watchNamespace watches all namespaces", + sets: []string{"jenkins.enabled=false", "operator.watchNamespace="}, + want: `""`, + }, + { + name: "standalone operator without watchNamespace defaults to its own namespace", + sets: []string{"jenkins.enabled=false"}, + want: "valueFrom", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + rendered := helmTemplate(t, "templates/operator.yaml", tc.sets...) + if got := watchNamespaceValue(t, rendered); got != tc.want { + t.Errorf("WATCH_NAMESPACE = %q, want %q", got, tc.want) + } + }) + } +} + +// TestOperatorRBACKind verifies the fix's RBAC switch: watching all namespaces +// must produce a ClusterRole/ClusterRoleBinding, while a scoped watch produces a +// namespace-bound Role/RoleBinding. +func TestOperatorRBACKind(t *testing.T) { + cases := []struct { + name string + sets []string + wantCluster bool + }{ + { + name: "scoped namespace uses namespaced Role", + sets: nil, + wantCluster: false, + }, + { + name: "bundled jenkins watching all namespaces uses ClusterRole", + sets: []string{"jenkins.namespace="}, + wantCluster: true, + }, + { + name: "standalone operator watching all namespaces uses ClusterRole", + sets: []string{"jenkins.enabled=false", "operator.watchNamespace="}, + wantCluster: true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + role := helmTemplate(t, "templates/role.yaml", tc.sets...) + binding := helmTemplate(t, "templates/role_binding.yaml", tc.sets...) + + if tc.wantCluster { + if !hasKindLine(role, "ClusterRole") { + t.Errorf("expected a ClusterRole, got:\n%s", role) + } + if !hasKindLine(binding, "ClusterRoleBinding") { + t.Errorf("expected a ClusterRoleBinding, got:\n%s", binding) + } + } else { + if !hasKindLine(role, "Role") { + t.Errorf("expected a namespaced Role, got:\n%s", role) + } + if !hasKindLine(binding, "RoleBinding") { + t.Errorf("expected a namespaced RoleBinding, got:\n%s", binding) + } + } + }) + } +}