Merge 7f7fff6940 into 43b00354ea

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
 */}}

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:

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 -}}

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
 */ -}}

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: {}

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: <ns> 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))

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)
+		}
+	})
+}

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)
+				}
+			}
+		})
+	}
+}