From e8b29f7c6907329abb2143cb4104a175c90b782d Mon Sep 17 00:00:00 2001 From: Luis Sanchez Date: Fri, 24 Jul 2026 11:11:23 -0400 Subject: [PATCH 1/2] Add operator controller DI consistency review rules Add a custom check and two path instruction sets that guide reviewers toward consistent dependency injection patterns in OpenShift operator controller constructors and starter.go files. The rules encode a seven-point consistency standard derived from surveying 11 operator repos (~80 controller constructors): 1. Typed informers over SharedInformerFactory 2. Narrowest client sub-interface 3. Standard Run(ctx, workers) lifecycle contract 4. Consistent parameter ordering 5. Resolved-at-construction feature gate handling (default) 6. Internal lister extraction 7. Five-phase starter.go structure All rules are warning-mode. They fire only on files matching operator patterns (pkg/operator/**/starter.go, pkg/**/controller*.go) and self-scope via instructions to skip non-operator repos. --- .coderabbit.yaml | 110 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 109 insertions(+), 1 deletion(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 28168f4..9654e13 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -508,6 +508,57 @@ reviews: > g.It("should fetch external content [Skipped:Disconnected]", func() { ... }) > ``` + - name: "Operator Controller DI Consistency" + mode: warning + instructions: | + This check applies only when a PR touches operator startup wiring + (starter.go, RunOperator) or controller constructors (New*Controller + functions) in OpenShift operator repositories. Skip this check if + the PR does not modify these files. + + When evaluating, check for these rules: + + 1. INFORMERS: Controller constructors should accept individual typed + informers (e.g., configv1informers.InfrastructureInformer), not + SharedInformerFactory, raw cache.SharedIndexInformer, or bare + Listers. Exception: v1helpers.KubeInformersForNamespaces is + acceptable for multi-namespace kube resources. + + 2. CLIENTS: Controller constructors should accept the narrowest + client sub-interface needed (e.g., corev1client.SecretsGetter + over CoreV1Interface over kubernetes.Interface). Passing + *rest.Config to a controller constructor is a flag. + + 3. LIFECYCLE CONTRACT: Controllers must have a standard Run + signature: Run(ctx context.Context, workers int). Flag + non-standard signatures like Run(stopCh <-chan struct{}), + Run(workers int, stopCh <-chan struct{}), or any Run that + does not take (ctx, workers). + + 4. PARAMETER ORDER: Constructor parameters should follow: + operatorClient, KubeInformersForNamespaces, typed informers, + client interfaces, feature gate state, eventRecorder (last). + + 5. FEATURE GATES: The default pattern (resolved-at-construction) + passes resolved FeatureGate snapshots or extracted bools to + controllers after starter.go blocks on gate resolution. Flag + controllers receiving FeatureGateAccess unless they use the + resolved-at-sync pattern (no construction-time gate decisions, + defensive AreInitialFeatureGatesObserved check in sync). Flag + map[string]bool for feature gates (stringly-typed). + + 6. LISTER EXTRACTION: Controllers should extract listers from + informers internally (informer.Lister()). Flag starter.go + code that pre-extracts listers and passes them to controllers. + Flag constructors that take both an informer and its lister. + + 7. STARTER STRUCTURE: starter.go should follow five phases: + (1) Clients, (2) Informer Factories, (3) Feature Gates, + (4) Controllers, (5) Start. Flag client creation inside + controller packages, informer factory objects passed to + controller constructors, or controller logic defined inline + in starter.go. + - name: "no-weak-crypto" instructions: | Flag MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB mode usage. @@ -529,9 +580,66 @@ reviews: hostnames, or customer data. mode: "error" - # Taken from prodsec suggested configuration: https://github.com/RedHatProductSecurity/prodsec-skills/blob/main/.coderabbit.yaml path_instructions: + # ── Operator starter.go (DI consistency) ──────────────────── + - path: "**/pkg/operator/**/starter.go" + instructions: | + Operator starter file. Review for the five-phase structure: + + Phase 1 - Clients: All clients created from ControllerContext + rest.Configs. No client creation elsewhere in the operator. + + Phase 2 - Informer Factories: All shared informer factories + created here. Controllers never receive factory objects. + + Phase 3 - Feature Gates: (3a) Start config informers early, + (3b) block on InitialFeatureGatesObserved with timeout, + (3c) resolve FeatureGate snapshot, extract per-gate bools. + The resolved-at-sync pattern (non-blocking) is also valid when + no construction-time or informer-registration decisions depend + on gates. + + Phase 4 - Controllers: Each controller constructed with typed + informers extracted from factories here. CRD-conditional + informers (v1alpha1 CRDs gated by feature gates) accessed + inside if-blocks, relying on lazy creation. Feature gate + state passed as resolved bool or FeatureGate snapshot. + + Phase 5 - Start: Start remaining informer factories (config + informers already started in Phase 3a). Start controllers + via go controller.Run(ctx, 1). + + Flag: client creation inside controller packages, informer + factory objects passed to controllers, controllers constructed + before feature gates are resolved (when using + resolved-at-construction), controller logic or sync functions + defined inline in this file. + + # ── Operator controller constructors (DI consistency) ─────── + - path: "**/pkg/**/controller*.go" + instructions: | + If this file contains a controller constructor (New*Controller + or similar function returning a controller), review for: + + - Typed informers (configv1informers.InfrastructureInformer), + not SharedInformerFactory or cache.SharedIndexInformer. + Exception: v1helpers.KubeInformersForNamespaces. + - Narrowest client sub-interface needed (SecretsGetter over + kubernetes.Interface). Never *rest.Config. + - Listers extracted internally (informer.Lister()), not + received as separate parameters from the caller. + - Feature gate state as resolved bool or FeatureGate snapshot, + not FeatureGateAccess, unless using the resolved-at-sync + pattern. + - Parameter order: operatorClient, KubeInformersForNamespaces, + typed informers, client interfaces, feature gate state, + eventRecorder (always last). + - Run signature: Run(ctx context.Context, workers int). Flag + non-standard signatures (Run(stopCh), Run(workers, stopCh)). + + # Taken from prodsec suggested configuration: https://github.com/RedHatProductSecurity/prodsec-skills/blob/main/.coderabbit.yaml + # ── Injection & input validation ───────────────────────────── # Skills: input-validation-injection, web-application-security - path: "**/*.{py,js,ts,go,rs,java,rb,php,kt,swift,cs}" From daa69fefa2c26de1bd25e3d4b1ab192d5f084d34 Mon Sep 17 00:00:00 2001 From: Luis Sanchez Date: Sat, 25 Jul 2026 00:14:56 -0400 Subject: [PATCH 2/2] Add KubeInformersForNamespaces exception consistently The custom check rule already allowed KubeInformersForNamespaces as an exception to the "no factory objects" rule, but the starter.go and controller path instructions did not mention it. Add the exception to both path instruction sets for consistency. --- .coderabbit.yaml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 9654e13..b015207 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -591,7 +591,9 @@ reviews: rest.Configs. No client creation elsewhere in the operator. Phase 2 - Informer Factories: All shared informer factories - created here. Controllers never receive factory objects. + created here. Controllers must not receive shared informer + factory objects, except v1helpers.KubeInformersForNamespaces + for multi-namespace kube resources. Phase 3 - Feature Gates: (3a) Start config informers early, (3b) block on InitialFeatureGatesObserved with timeout, @@ -624,6 +626,8 @@ reviews: - Typed informers (configv1informers.InfrastructureInformer), not SharedInformerFactory or cache.SharedIndexInformer. + Exception: v1helpers.KubeInformersForNamespaces for + multi-namespace kube resources. Exception: v1helpers.KubeInformersForNamespaces. - Narrowest client sub-interface needed (SecretsGetter over kubernetes.Interface). Never *rest.Config.