From 6195dd91e14fe6e9f64d70bf90ccf10bdf1d12b3 Mon Sep 17 00:00:00 2001 From: Safwan Date: Wed, 15 Jul 2026 12:33:40 +0500 Subject: [PATCH] some refactoring and cleanup --- cmd/reloader/main.go | 37 +++--- .../chart/reloader/templates/_helpers.tpl | 61 ++++++++-- .../chart/reloader/templates/deployment.yaml | 1 + .../chart/reloader/templates/role.yaml | 16 +-- .../chart/reloader/templates/rolebinding.yaml | 2 +- .../kubernetes/chart/reloader/values.yaml | 12 +- internal/pkg/config/config.go | 22 ---- internal/pkg/config/config_test.go | 33 ----- internal/pkg/config/flags.go | 39 ++++-- internal/pkg/config/flags_test.go | 57 ++++----- internal/pkg/metadata/publisher.go | 19 ++- test/e2e/flags/watch_namespaces_test.go | 115 ++++++++++++++++++ 12 files changed, 263 insertions(+), 151 deletions(-) create mode 100644 test/e2e/flags/watch_namespaces_test.go diff --git a/cmd/reloader/main.go b/cmd/reloader/main.go index 781d28c5..b1f7f7f9 100644 --- a/cmd/reloader/main.go +++ b/cmd/reloader/main.go @@ -54,8 +54,15 @@ func newReloaderCommand() *cobra.Command { } func run(cmd *cobra.Command, args []string) error { - scopeWarnings, err := config.ApplyFlags(cfg) + // Configure logging first so ApplyFlags can surface namespace-scope warnings + // through a ready logger instead of returning them to the caller. + log, err := configureLogging(config.LoggingFlags()) if err != nil { + return fmt.Errorf("configuring logging: %w", err) + } + controllerruntime.SetLogger(log) + + if err := config.ApplyFlags(cfg, log); err != nil { return fmt.Errorf("applying flags: %w", err) } @@ -73,21 +80,8 @@ func run(cmd *cobra.Command, args []string) error { } } - log, err := configureLogging(cfg.LogFormat, cfg.LogLevel) - if err != nil { - return fmt.Errorf("configuring logging: %w", err) - } - - controllerruntime.SetLogger(log) - log.Info("Starting Reloader") - // Namespace-scope semantics are enforced in ApplyFlags; surface any warnings - // it produced now that logging is configured. - for _, w := range scopeWarnings { - log.Info(w) - } - if cfg.IsGlobalMode() { log.Info("watching all namespaces") } else { @@ -156,14 +150,13 @@ func run(cmd *cobra.Command, args []string) error { return fmt.Errorf("setting up reconcilers: %w", err) } - // Skip metadata publisher when ConfigMaps are ignored (no RBAC permissions) - if !cfg.IsResourceIgnored("configmaps") { - if err := mgr.Add(metadata.Runnable(mgr.GetClient(), cfg, log)); err != nil { - log.Error(err, "Failed to add metadata publisher") - // Non-fatal, continue starting - } - } else { - log.Info("skipping metadata publisher (configmaps ignored)") + // Meta-info is internal instance metadata and is always published. The + // publisher builds its own uncached client (see metadata.Runnable) because + // the ConfigMap lives in Reloader's own namespace, which the manager cache + // does not cover in scoped mode. + if err := mgr.Add(metadata.Runnable(mgr.GetConfig(), mgr.GetScheme(), cfg, log)); err != nil { + log.Error(err, "Failed to add metadata publisher") + // Non-fatal, continue starting } if cfg.EnablePProf { diff --git a/deployments/kubernetes/chart/reloader/templates/_helpers.tpl b/deployments/kubernetes/chart/reloader/templates/_helpers.tpl index ec268917..32d129c9 100644 --- a/deployments/kubernetes/chart/reloader/templates/_helpers.tpl +++ b/deployments/kubernetes/chart/reloader/templates/_helpers.tpl @@ -117,15 +117,56 @@ Comma-joined form of reloader-watchNamespaces, for the --namespaces CLI flag. {{- end -}} {{/* -Namespaces that need namespaced RBAC in scoped mode: the watched namespaces plus -the release namespace, so leader-election leases, the meta-info ConfigMap and -events keep working there even though it is not watched for reloads. -Returns a JSON-encoded list; consumers use mustFromJson to iterate. +Fails the render on an inconsistent namespace configuration: reloader.namespaces +(scoped mode) requires reloader.watchGlobally=false. Included from deployment.yaml +so it is validated once regardless of which templates render. */}} -{{- define "reloader-rbacNamespaces" -}} -{{- $relNs := .Values.namespace | default .Release.Namespace -}} -{{- $watch := include "reloader-watchNamespaces" . | mustFromJson -}} -{{- concat (list $relNs) $watch | uniq | sortAlpha | toJson -}} +{{- define "reloader-validate-namespaces" -}} +{{- if and .Values.reloader.watchGlobally .Values.reloader.namespaces -}} +{{- fail "reloader.namespaces is set but reloader.watchGlobally is true; set reloader.watchGlobally=false to use scoped namespace mode." -}} +{{- end -}} +{{- end -}} + +{{/* +RBAC rules Reloader needs in its own (release) namespace, independent of the +watched namespaces. Reloader publishes an internal meta-info ConfigMap there in +every mode, so configmap write access is always granted. In scoped mode the +release namespace is not covered by the watch RBAC, so leader-election events +(and leases under HA) are granted here too; in global/single mode those are +already covered by the ClusterRole or the single-namespace Role. +Expects the root context ($) as its argument. +*/}} +{{- define "reloader-release-rules" }} + - apiGroups: + - "" + resources: + - configmaps + verbs: + - get + - create + - update + - patch +{{- if .Values.reloader.namespaces }} + - apiGroups: + - "" + - "events.k8s.io" + resources: + - events + verbs: + - create + - patch + - update +{{- if .Values.reloader.enableHA }} + - apiGroups: + - "coordination.k8s.io" + resources: + - leases + verbs: + - create + - get + - update +{{- end }} +{{- end }} {{- end -}} {{/* @@ -183,6 +224,7 @@ the rule set is defined once. Expects the root context ($) as its argument. - watch - update - patch +{{- if .Values.reloader.ignoreCronJobs }}{{- else }} - apiGroups: - "batch" resources: @@ -193,6 +235,8 @@ the rule set is defined once. Expects the root context ($) as its argument. - watch - update - patch +{{- end }} +{{- if .Values.reloader.ignoreJobs }}{{- else }} - apiGroups: - "batch" resources: @@ -203,6 +247,7 @@ the rule set is defined once. Expects the root context ($) as its argument. - list - get - watch +{{- end }} {{- if .Values.reloader.enableHA }} - apiGroups: - "coordination.k8s.io" diff --git a/deployments/kubernetes/chart/reloader/templates/deployment.yaml b/deployments/kubernetes/chart/reloader/templates/deployment.yaml index da5a1d54..ff43894a 100644 --- a/deployments/kubernetes/chart/reloader/templates/deployment.yaml +++ b/deployments/kubernetes/chart/reloader/templates/deployment.yaml @@ -1,3 +1,4 @@ +{{- include "reloader-validate-namespaces" . -}} apiVersion: apps/v1 kind: Deployment metadata: diff --git a/deployments/kubernetes/chart/reloader/templates/role.yaml b/deployments/kubernetes/chart/reloader/templates/role.yaml index b7b5cd92..0454eadf 100644 --- a/deployments/kubernetes/chart/reloader/templates/role.yaml +++ b/deployments/kubernetes/chart/reloader/templates/role.yaml @@ -1,13 +1,10 @@ -{{- if and .Values.reloader.watchGlobally .Values.reloader.namespaces }} -{{- fail "reloader.namespaces is set but reloader.watchGlobally is true; set reloader.watchGlobally=false to use scoped namespace mode." }} -{{- end }} {{- if and (not (.Values.reloader.watchGlobally)) (.Values.reloader.rbac.enabled) }} {{- $apiVersion := "rbac.authorization.k8s.io/v1" }} {{- if not (.Capabilities.APIVersions.Has "rbac.authorization.k8s.io/v1") }} {{- $apiVersion = "rbac.authorization.k8s.io/v1beta1" }} {{- end }} {{- if .Values.reloader.namespaces }} -{{- range $ns := (include "reloader-rbacNamespaces" . | mustFromJson) }} +{{- range $ns := (include "reloader-watchNamespaces" . | mustFromJson) }} apiVersion: {{ $apiVersion }} kind: Role metadata: @@ -67,14 +64,5 @@ metadata: name: {{ template "reloader-fullname" . }}-metadata-role namespace: {{ .Values.namespace | default .Release.Namespace }} rules: - - apiGroups: - - "" - resources: - - configmaps - verbs: - - list - - get - - watch - - create - - update +{{- include "reloader-release-rules" . }} {{- end }} \ No newline at end of file diff --git a/deployments/kubernetes/chart/reloader/templates/rolebinding.yaml b/deployments/kubernetes/chart/reloader/templates/rolebinding.yaml index 187c90dd..7d73b182 100644 --- a/deployments/kubernetes/chart/reloader/templates/rolebinding.yaml +++ b/deployments/kubernetes/chart/reloader/templates/rolebinding.yaml @@ -4,7 +4,7 @@ {{- $apiVersion = "rbac.authorization.k8s.io/v1beta1" }} {{- end }} {{- if .Values.reloader.namespaces }} -{{- range $ns := (include "reloader-rbacNamespaces" . | mustFromJson) }} +{{- range $ns := (include "reloader-watchNamespaces" . | mustFromJson) }} apiVersion: {{ $apiVersion }} kind: RoleBinding metadata: diff --git a/deployments/kubernetes/chart/reloader/values.yaml b/deployments/kubernetes/chart/reloader/values.yaml index f236bdf7..119bc8e3 100644 --- a/deployments/kubernetes/chart/reloader/values.yaml +++ b/deployments/kubernetes/chart/reloader/values.yaml @@ -45,11 +45,13 @@ reloader: logFormat: "" # json logLevel: info # Log level to use (trace, debug, info, warning, error, fatal and panic) watchGlobally: true - # Scoped mode: explicit list of namespaces to watch. When non-empty (and watchGlobally - # is false), Reloader watches exactly these namespaces and the chart creates a namespace - # scoped Role + RoleBinding in each one — no ClusterRole is created. The release namespace - # is always included automatically. Leave empty ([]) for the default single-namespace or - # global behavior controlled by watchGlobally. + # Scoped mode: explicit list of namespaces to watch. When non-empty you must also set + # watchGlobally=false. Reloader watches exactly these namespaces and the chart creates a + # namespaced Role + RoleBinding in each one — no ClusterRole is created. Reloader's own + # (release) namespace is NOT watched for reloads; it only receives a small metadata Role + # for the internal meta-info ConfigMap and (under HA) leader-election leases/events. + # Leave empty ([]) for the default single-namespace or global behavior controlled by + # watchGlobally. # Accepts either a YAML list (e.g. ["team-a", "team-b"]) or a comma-separated string # (e.g. "team-a,team-b") namespaces: [] diff --git a/internal/pkg/config/config.go b/internal/pkg/config/config.go index 726a6e8b..c1c7faed 100644 --- a/internal/pkg/config/config.go +++ b/internal/pkg/config/config.go @@ -201,25 +201,3 @@ func (c *Config) IsNamespaceIgnored(namespace string) bool { func (c *Config) IsGlobalMode() bool { return len(c.WatchedNamespaces) == 0 } - -// ApplyNamespaceScope enforces master-parity semantics: namespace-selector and -// namespaces-to-ignore are only honored in global (all-namespaces) mode. In -// scoped or single-namespace mode the watched set is already explicit, so both -// are cleared. It returns human-readable warnings for any setting it dropped so -// the caller can log them. -func (c *Config) ApplyNamespaceScope() []string { - if c.IsGlobalMode() { - return nil - } - var warnings []string - if len(c.NamespaceSelectors) > 0 { - warnings = append(warnings, "namespace-selector is set but is only honored in global mode; ignoring it") - c.NamespaceSelectors = nil - c.NamespaceSelectorStrings = nil - } - if len(c.IgnoredNamespaces) > 0 { - warnings = append(warnings, "namespaces-to-ignore is set but is only honored in global mode; ignoring it") - c.IgnoredNamespaces = nil - } - return warnings -} diff --git a/internal/pkg/config/config_test.go b/internal/pkg/config/config_test.go index 62d1a610..218c34d5 100644 --- a/internal/pkg/config/config_test.go +++ b/internal/pkg/config/config_test.go @@ -3,8 +3,6 @@ package config import ( "testing" "time" - - "k8s.io/apimachinery/pkg/labels" ) func TestNewDefault(t *testing.T) { @@ -231,34 +229,3 @@ func TestIsGlobalMode(t *testing.T) { t.Errorf("non-empty WatchedNamespaces should not be global mode") } } - -func TestApplyNamespaceScope_GlobalKeepsSettings(t *testing.T) { - c := &Config{ - WatchedNamespaces: nil, - IgnoredNamespaces: []string{"kube-system"}, - NamespaceSelectors: []labels.Selector{labels.Everything()}, - } - warnings := c.ApplyNamespaceScope() - if len(warnings) != 0 { - t.Errorf("global mode should produce no warnings, got %v", warnings) - } - if len(c.IgnoredNamespaces) != 1 || len(c.NamespaceSelectors) != 1 { - t.Errorf("global mode should keep selectors and ignored namespaces") - } -} - -func TestApplyNamespaceScope_ScopedClearsSettings(t *testing.T) { - c := &Config{ - WatchedNamespaces: []string{"team-a"}, - IgnoredNamespaces: []string{"kube-system"}, - NamespaceSelectors: []labels.Selector{labels.Everything()}, - NamespaceSelectorStrings: []string{"env=prod"}, - } - warnings := c.ApplyNamespaceScope() - if len(warnings) != 2 { - t.Errorf("scoped mode should warn about both dropped settings, got %v", warnings) - } - if len(c.IgnoredNamespaces) != 0 || len(c.NamespaceSelectors) != 0 || len(c.NamespaceSelectorStrings) != 0 { - t.Errorf("scoped mode should clear selectors and ignored namespaces") - } -} diff --git a/internal/pkg/config/flags.go b/internal/pkg/config/flags.go index 6680abca..70ba5225 100644 --- a/internal/pkg/config/flags.go +++ b/internal/pkg/config/flags.go @@ -5,6 +5,7 @@ import ( "strings" "time" + "github.com/go-logr/logr" "github.com/spf13/pflag" "github.com/spf13/viper" "k8s.io/apimachinery/pkg/labels" @@ -264,11 +265,17 @@ func BindFlags(fs *pflag.FlagSet, cfg *Config) { _ = v.BindEnv("alert-proxy", "ALERT_PROXY", "ALERT_WEBHOOK_PROXY") } +// LoggingFlags returns the log format and level from parsed flags/env. The +// caller uses these to configure logging before ApplyFlags runs, so ApplyFlags +// can log warnings through a ready logger. +func LoggingFlags() (format, level string) { + return v.GetString("log-format"), v.GetString("log-level") +} + // ApplyFlags applies flag values from viper to the config struct. Call this -// after parsing flags. It returns any human-readable warnings produced while -// finalizing namespace scope (see ApplyNamespaceScope) so the caller can log -// them once a logger is available. -func ApplyFlags(cfg *Config) ([]string, error) { +// after parsing flags. It finalizes namespace scope and logs any warnings it +// produces through the given logger. +func ApplyFlags(cfg *Config, log logr.Logger) error { // Boolean flags cfg.AutoReloadAll = v.GetBool("auto-reload-all") cfg.SyncAfterRestart = v.GetBool("sync-after-restart") @@ -367,7 +374,7 @@ func ApplyFlags(cfg *Config) ([]string, error) { joinedNS := strings.Join(nsSelectors, ",") selector, err := labels.Parse(joinedNS) if err != nil { - return nil, fmt.Errorf("invalid selector %q: %w", joinedNS, err) + return fmt.Errorf("invalid selector %q: %w", joinedNS, err) } cfg.NamespaceSelectors = []labels.Selector{selector} } @@ -375,7 +382,7 @@ func ApplyFlags(cfg *Config) ([]string, error) { joinedRes := strings.Join(resSelectors, ",") selector, err := labels.Parse(joinedRes) if err != nil { - return nil, fmt.Errorf("invalid selector %q: %w", joinedRes, err) + return fmt.Errorf("invalid selector %q: %w", joinedRes, err) } cfg.ResourceSelectors = []labels.Selector{selector} } @@ -391,11 +398,21 @@ func ApplyFlags(cfg *Config) ([]string, error) { cfg.LeaderElection.RetryPeriod = 2 * time.Second } - // Enforce namespace-scope semantics here so the finalized config is - // self-consistent for every caller: selector/ignore lists are only honored - // in global mode. Warnings are returned for the caller to log once logging - // is set up. - return cfg.ApplyNamespaceScope(), nil + // Namespace-selector and namespaces-to-ignore are only honored in global + // mode; in scoped or single-namespace mode the watched set is already + // explicit, so drop them and log where it happens. + if !cfg.IsGlobalMode() { + if len(cfg.NamespaceSelectors) > 0 { + log.Info("namespace-selector is set but is only honored in global mode; ignoring it") + cfg.NamespaceSelectors = nil + cfg.NamespaceSelectorStrings = nil + } + if len(cfg.IgnoredNamespaces) > 0 { + log.Info("namespaces-to-ignore is set but is only honored in global mode; ignoring it") + cfg.IgnoredNamespaces = nil + } + } + return nil } // parseBoolString parses a string as a boolean, defaulting to false. diff --git a/internal/pkg/config/flags_test.go b/internal/pkg/config/flags_test.go index 233a6527..45d6d2cb 100644 --- a/internal/pkg/config/flags_test.go +++ b/internal/pkg/config/flags_test.go @@ -4,6 +4,7 @@ import ( "strings" "testing" + "github.com/go-logr/logr" "github.com/spf13/pflag" "github.com/spf13/viper" ) @@ -92,7 +93,7 @@ func TestBindFlags_DefaultValues(t *testing.T) { t.Fatalf("Parse() error = %v", err) } - if _, err := ApplyFlags(cfg); err != nil { + if err := ApplyFlags(cfg, logr.Discard()); err != nil { t.Fatalf("ApplyFlags() error = %v", err) } @@ -126,7 +127,7 @@ func TestBindFlags_CustomValues(t *testing.T) { t.Fatalf("Parse() error = %v", err) } - if _, err := ApplyFlags(cfg); err != nil { + if err := ApplyFlags(cfg, logr.Discard()); err != nil { t.Fatalf("ApplyFlags() error = %v", err) } @@ -168,7 +169,7 @@ func TestApplyFlags_SecretProviderClassAnnotations(t *testing.T) { if err := fs.Parse(nil); err != nil { t.Fatalf("Parse() error = %v", err) } - if _, err := ApplyFlags(cfg); err != nil { + if err := ApplyFlags(cfg, logr.Discard()); err != nil { t.Fatalf("ApplyFlags() error = %v", err) } defaults := DefaultAnnotations() @@ -195,7 +196,7 @@ func TestApplyFlags_SecretProviderClassAnnotations(t *testing.T) { if err := fs.Parse(args); err != nil { t.Fatalf("Parse() error = %v", err) } - if _, err := ApplyFlags(cfg); err != nil { + if err := ApplyFlags(cfg, logr.Discard()); err != nil { t.Fatalf("ApplyFlags() error = %v", err) } if cfg.Annotations.SecretProviderClassAuto != "spc.example.com/auto" { @@ -218,7 +219,7 @@ func TestApplyFlags_ExcludeAnnotations(t *testing.T) { if err := fs.Parse(nil); err != nil { t.Fatalf("Parse() error = %v", err) } - if _, err := ApplyFlags(cfg); err != nil { + if err := ApplyFlags(cfg, logr.Discard()); err != nil { t.Fatalf("ApplyFlags() error = %v", err) } defaults := DefaultAnnotations() @@ -241,7 +242,7 @@ func TestApplyFlags_ExcludeAnnotations(t *testing.T) { if err := fs.Parse(args); err != nil { t.Fatalf("Parse() error = %v", err) } - if _, err := ApplyFlags(cfg); err != nil { + if err := ApplyFlags(cfg, logr.Discard()); err != nil { t.Fatalf("ApplyFlags() error = %v", err) } if cfg.Annotations.ConfigmapExclude != "cm.example.com/exclude" { @@ -261,7 +262,7 @@ func TestApplyFlags_IgnoreAnnotation(t *testing.T) { if err := fs.Parse(nil); err != nil { t.Fatalf("Parse() error = %v", err) } - if _, err := ApplyFlags(cfg); err != nil { + if err := ApplyFlags(cfg, logr.Discard()); err != nil { t.Fatalf("ApplyFlags() error = %v", err) } if cfg.Annotations.Ignore != DefaultAnnotations().Ignore { @@ -276,7 +277,7 @@ func TestApplyFlags_IgnoreAnnotation(t *testing.T) { if err := fs.Parse([]string{"--ignore-annotation=my.company.com/reloader-ignore"}); err != nil { t.Fatalf("Parse() error = %v", err) } - if _, err := ApplyFlags(cfg); err != nil { + if err := ApplyFlags(cfg, logr.Discard()); err != nil { t.Fatalf("ApplyFlags() error = %v", err) } if cfg.Annotations.Ignore != "my.company.com/reloader-ignore" { @@ -313,7 +314,7 @@ func TestApplyFlags_BooleanStrings(t *testing.T) { t.Fatalf("Parse() error = %v", err) } - _, err := ApplyFlags(cfg) + err := ApplyFlags(cfg, logr.Discard()) if (err != nil) != tt.wantErr { t.Errorf("ApplyFlags() error = %v, wantErr %v", err, tt.wantErr) return @@ -343,7 +344,7 @@ func TestApplyFlags_CommaSeparatedLists(t *testing.T) { t.Fatalf("Parse() error = %v", err) } - if _, err := ApplyFlags(cfg); err != nil { + if err := ApplyFlags(cfg, logr.Discard()); err != nil { t.Fatalf("ApplyFlags() error = %v", err) } @@ -378,7 +379,7 @@ func TestApplyFlags_Selectors(t *testing.T) { t.Fatalf("Parse() error = %v", err) } - if _, err := ApplyFlags(cfg); err != nil { + if err := ApplyFlags(cfg, logr.Discard()); err != nil { t.Fatalf("ApplyFlags() error = %v", err) } @@ -409,7 +410,7 @@ func TestApplyFlags_InvalidSelector(t *testing.T) { t.Fatalf("Parse() error = %v", err) } - _, err := ApplyFlags(cfg) + err := ApplyFlags(cfg, logr.Discard()) if err == nil { t.Error("ApplyFlags() should return error for invalid selector") } @@ -461,7 +462,7 @@ func TestApplyFlags_AlertingEnvVars(t *testing.T) { t.Fatalf("Parse() error = %v", err) } - if _, err := ApplyFlags(cfg); err != nil { + if err := ApplyFlags(cfg, logr.Discard()); err != nil { t.Fatalf("ApplyFlags() error = %v", err) } @@ -494,7 +495,7 @@ func TestApplyFlags_LegacyProxyEnvVar(t *testing.T) { t.Fatalf("Parse() error = %v", err) } - if _, err := ApplyFlags(cfg); err != nil { + if err := ApplyFlags(cfg, logr.Discard()); err != nil { t.Fatalf("ApplyFlags() error = %v", err) } @@ -511,7 +512,7 @@ func TestApplyFlagsCSIIntegration(t *testing.T) { if err := fs.Parse([]string{"--enable-csi-integration=true"}); err != nil { t.Fatal(err) } - if _, err := ApplyFlags(cfg); err != nil { + if err := ApplyFlags(cfg, logr.Discard()); err != nil { t.Fatal(err) } if !cfg.CSIIntegrationEnabled { @@ -592,7 +593,7 @@ func TestApplyFlags_NamespacesScoped(t *testing.T) { if err := fs.Parse([]string{"--namespaces=team-a,team-b"}); err != nil { t.Fatalf("Parse() error = %v", err) } - if _, err := ApplyFlags(cfg); err != nil { + if err := ApplyFlags(cfg, logr.Discard()); err != nil { t.Fatalf("ApplyFlags() error = %v", err) } @@ -617,7 +618,7 @@ func TestApplyFlags_NamespacesFromEnv(t *testing.T) { if err := fs.Parse([]string{}); err != nil { t.Fatalf("Parse() error = %v", err) } - if _, err := ApplyFlags(cfg); err != nil { + if err := ApplyFlags(cfg, logr.Discard()); err != nil { t.Fatalf("ApplyFlags() error = %v", err) } @@ -636,7 +637,7 @@ func TestApplyFlags_NamespacesGlobal(t *testing.T) { if err := fs.Parse([]string{}); err != nil { t.Fatalf("Parse() error = %v", err) } - if _, err := ApplyFlags(cfg); err != nil { + if err := ApplyFlags(cfg, logr.Discard()); err != nil { t.Fatalf("ApplyFlags() error = %v", err) } @@ -657,7 +658,7 @@ func TestApplyFlags_NamespacesTrimsEmptyEntries(t *testing.T) { if err := fs.Parse([]string{"--namespaces=team-a, ,team-b,"}); err != nil { t.Fatalf("Parse() error = %v", err) } - if _, err := ApplyFlags(cfg); err != nil { + if err := ApplyFlags(cfg, logr.Discard()); err != nil { t.Fatalf("ApplyFlags() error = %v", err) } @@ -682,7 +683,7 @@ func TestApplyFlags_NamespacesAllEmptyIsGlobal(t *testing.T) { if err := fs.Parse([]string{"--namespaces=, ,"}); err != nil { t.Fatalf("Parse() error = %v", err) } - if _, err := ApplyFlags(cfg); err != nil { + if err := ApplyFlags(cfg, logr.Discard()); err != nil { t.Fatalf("ApplyFlags() error = %v", err) } @@ -695,8 +696,8 @@ func TestApplyFlags_NamespacesAllEmptyIsGlobal(t *testing.T) { } // ApplyFlags must finalize a self-consistent config: in scoped mode it enforces -// namespace-scope semantics (clears selector/ignore lists) and returns warnings, -// without the caller having to invoke ApplyNamespaceScope separately. +// namespace-scope semantics (clears selector/ignore lists) and logs a warning +// for each dropped setting. func TestApplyFlags_ScopedClearsSelectorsAndIgnores(t *testing.T) { resetViper() cfg := NewDefault() @@ -710,8 +711,7 @@ func TestApplyFlags_ScopedClearsSelectorsAndIgnores(t *testing.T) { }); err != nil { t.Fatalf("Parse() error = %v", err) } - warnings, err := ApplyFlags(cfg) - if err != nil { + if err := ApplyFlags(cfg, logr.Discard()); err != nil { t.Fatalf("ApplyFlags() error = %v", err) } @@ -721,9 +721,6 @@ func TestApplyFlags_ScopedClearsSelectorsAndIgnores(t *testing.T) { if len(cfg.IgnoredNamespaces) != 0 { t.Errorf("scoped mode should clear ignored namespaces, got %v", cfg.IgnoredNamespaces) } - if len(warnings) != 2 { - t.Errorf("expected 2 scope warnings, got %v", warnings) - } } func TestApplyFlags_GlobalKeepsSelectorsNoWarnings(t *testing.T) { @@ -739,8 +736,7 @@ func TestApplyFlags_GlobalKeepsSelectorsNoWarnings(t *testing.T) { }); err != nil { t.Fatalf("Parse() error = %v", err) } - warnings, err := ApplyFlags(cfg) - if err != nil { + if err := ApplyFlags(cfg, logr.Discard()); err != nil { t.Fatalf("ApplyFlags() error = %v", err) } @@ -750,7 +746,4 @@ func TestApplyFlags_GlobalKeepsSelectorsNoWarnings(t *testing.T) { if len(cfg.NamespaceSelectors) != 1 || len(cfg.IgnoredNamespaces) != 1 { t.Errorf("global mode should keep selectors and ignored namespaces") } - if len(warnings) != 0 { - t.Errorf("global mode should produce no warnings, got %v", warnings) - } } diff --git a/internal/pkg/metadata/publisher.go b/internal/pkg/metadata/publisher.go index 6c6a4222..8cd349e1 100644 --- a/internal/pkg/metadata/publisher.go +++ b/internal/pkg/metadata/publisher.go @@ -8,6 +8,8 @@ import ( "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/stakater/Reloader/internal/pkg/config" @@ -77,10 +79,21 @@ func PublishMetaInfoConfigMap(ctx context.Context, c client.Client, cfg *config. return publisher.Publish(ctx) } -// Runnable returns a controller-runtime Runnable that publishes the metadata ConfigMap -// when the manager starts. This ensures the cache is ready before accessing the API. -func Runnable(c client.Client, cfg *config.Config, log logr.Logger) RunnableFunc { +// Runnable returns a controller-runtime Runnable that publishes the meta-info +// ConfigMap when the manager starts. It builds its own uncached client from the +// given rest config and scheme: the ConfigMap lives in Reloader's own namespace, +// which the manager cache does not cover in scoped mode, so a cache-backed client +// cannot read or write it there. Meta-info is internal instance metadata and is +// always published regardless of which resources are watched. +func Runnable(restConfig *rest.Config, scheme *runtime.Scheme, cfg *config.Config, log logr.Logger) RunnableFunc { return func(ctx context.Context) error { + c, err := client.New(restConfig, client.Options{Scheme: scheme}) + if err != nil { + log.Error(err, "Failed to create client for meta info configmap publisher") + // Non-fatal, don't return error to avoid crashing the manager + <-ctx.Done() + return nil + } if err := PublishMetaInfoConfigMap(ctx, c, cfg, log); err != nil { log.Error(err, "Failed to create metadata ConfigMap") // Non-fatal, don't return error to avoid crashing the manager diff --git a/test/e2e/flags/watch_namespaces_test.go b/test/e2e/flags/watch_namespaces_test.go new file mode 100644 index 00000000..bcfbd6cb --- /dev/null +++ b/test/e2e/flags/watch_namespaces_test.go @@ -0,0 +1,115 @@ +package flags + +import ( + "fmt" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/stakater/Reloader/test/e2e/utils" +) + +var _ = Describe("Watch Namespaces (scoped mode) Flag Tests", Serial, func() { + var ( + deploymentName string + configMapName string + watchedNS string + unwatchedNS string + adapter *utils.DeploymentAdapter + ) + + BeforeEach(func() { + deploymentName = utils.RandName("deploy") + configMapName = utils.RandName("cm") + watchedNS = "watched-" + utils.RandName("ns") + unwatchedNS = "unwatched-" + utils.RandName("ns") + adapter = utils.NewDeploymentAdapter(kubeClient) + + // The watched namespace must exist before install: in scoped mode the + // chart creates a Role/RoleBinding in it. + Expect(utils.CreateNamespace(ctx, kubeClient, watchedNS)).To(Succeed()) + Expect(utils.CreateNamespace(ctx, kubeClient, unwatchedNS)).To(Succeed()) + + err := deployReloaderWithFlags(map[string]string{ + "reloader.watchGlobally": "false", + "reloader.namespaces": fmt.Sprintf("{%s}", watchedNS), + }) + Expect(err).NotTo(HaveOccurred()) + + Expect(waitForReloaderReady()).To(Succeed()) + }) + + AfterEach(func() { + _ = utils.DeleteDeployment(ctx, kubeClient, watchedNS, deploymentName) + _ = utils.DeleteConfigMap(ctx, kubeClient, watchedNS, configMapName) + _ = utils.DeleteDeployment(ctx, kubeClient, unwatchedNS, deploymentName) + _ = utils.DeleteConfigMap(ctx, kubeClient, unwatchedNS, configMapName) + _ = undeployReloader() + _ = utils.DeleteNamespace(ctx, kubeClient, watchedNS) + _ = utils.DeleteNamespace(ctx, kubeClient, unwatchedNS) + }) + + It("should reload workloads in a watched namespace", func() { + By("Creating a ConfigMap in the watched namespace") + _, err := utils.CreateConfigMap(ctx, kubeClient, watchedNS, configMapName, + map[string]string{"key": "initial"}, nil) + Expect(err).NotTo(HaveOccurred()) + + By("Creating a Deployment in the watched namespace with auto annotation") + _, err = utils.CreateDeployment(ctx, kubeClient, watchedNS, deploymentName, + utils.WithConfigMapEnvFrom(configMapName), + utils.WithAnnotations(utils.BuildAutoTrueAnnotation()), + ) + Expect(err).NotTo(HaveOccurred()) + + By("Waiting for Deployment to be ready") + Expect(adapter.WaitReady(ctx, watchedNS, deploymentName, utils.WorkloadReadyTimeout)).To(Succeed()) + + By("Updating the ConfigMap") + // Capture the reload-annotation baseline before the trigger to avoid the + // TOCTOU race where Reloader reloads before WaitReloaded records its baseline. + priorReload, err := adapter.GetPodTemplateAnnotation(ctx, watchedNS, deploymentName, utils.AnnotationLastReloadedFrom) + Expect(err).NotTo(HaveOccurred()) + err = utils.UpdateConfigMap(ctx, kubeClient, watchedNS, configMapName, map[string]string{"key": "updated"}) + Expect(err).NotTo(HaveOccurred()) + + By("Waiting for Deployment to be reloaded (watched namespace should work)") + reloaded, err := adapter.WaitReloadedFrom(ctx, watchedNS, deploymentName, + utils.AnnotationLastReloadedFrom, priorReload, utils.ReloadTimeout) + Expect(err).NotTo(HaveOccurred()) + Expect(reloaded).To(BeTrue(), "Deployment in a watched namespace should reload") + }) + + It("should NOT reload workloads in an unwatched namespace", func() { + By("Creating a ConfigMap in an unwatched namespace") + _, err := utils.CreateConfigMap(ctx, kubeClient, unwatchedNS, configMapName, + map[string]string{"key": "initial"}, nil) + Expect(err).NotTo(HaveOccurred()) + + By("Creating a Deployment in an unwatched namespace with auto annotation") + _, err = utils.CreateDeployment(ctx, kubeClient, unwatchedNS, deploymentName, + utils.WithConfigMapEnvFrom(configMapName), + utils.WithAnnotations(utils.BuildAutoTrueAnnotation()), + ) + Expect(err).NotTo(HaveOccurred()) + + By("Waiting for Deployment to be ready") + Expect(adapter.WaitReady(ctx, unwatchedNS, deploymentName, utils.WorkloadReadyTimeout)).To(Succeed()) + + By("Updating the ConfigMap in the unwatched namespace") + // Capture the reload-annotation baseline before the trigger to avoid the + // TOCTOU race where Reloader reloads before WaitReloaded records its baseline. + priorReload, err := adapter.GetPodTemplateAnnotation(ctx, unwatchedNS, deploymentName, utils.AnnotationLastReloadedFrom) + Expect(err).NotTo(HaveOccurred()) + err = utils.UpdateConfigMap(ctx, kubeClient, unwatchedNS, configMapName, map[string]string{"key": "updated"}) + Expect(err).NotTo(HaveOccurred()) + + By("Verifying Deployment was NOT reloaded (namespace not in --namespaces)") + time.Sleep(utils.NegativeTestWait) + reloaded, err := adapter.WaitReloadedFrom(ctx, unwatchedNS, deploymentName, + utils.AnnotationLastReloadedFrom, priorReload, utils.ShortTimeout) + Expect(err).NotTo(HaveOccurred()) + Expect(reloaded).To(BeFalse(), "Deployment in an unwatched namespace should NOT reload") + }) +})