maas-controller anti-pattern scanner review - verified findings ranked by severity

maas-controller-anti-pattern-review.md

maas-controller: Anti-Pattern Scanner Review

Static analysis flagged 114 findings (53 CRITICAL, 60 HIGH, 1 MEDIUM) across 6 anti-pattern categories. After verifying each against source code with controller-runtime and K8s API domain expertise, ~75% are false positives.

The scanner doesn't trace field-level client resolution (e.g. APIReader vs cached Client), doesn't check go.mod for typed alternatives before flagging unstructured usage, and projects core-resource cardinality onto singleton CRDs.

Base ref: 290712c on main

---

Verified findings, ranked by severity

1. HIGH - Invisible cluster-wide informer for ConfigMap (AP-3)

r.List(ctx, &claimList, ...) in the AITenant controller creates a cluster-wide ConfigMap informer on first call. ConfigMaps are a high-cardinality core type - this can cache thousands of objects (including large ones like CA bundles) that the controller never needs.

Location	Code
aitenant_controller.go:736	r.List(ctx, &claimList, ...) in deleteGatewayClaim
aitenant_controller.go:760	r.List(ctx, &claimList, ...) in cleanupStaleClaims

Why it's real: The AITenantReconciler already has an APIReader field (set from mgr.GetAPIReader() in main.go:679). These two List calls use r.List (cached client) instead of r.APIReader - likely an oversight since the rest of the reconciler correctly routes through APIReader.

Fix: Switch to r.APIReader.List(ctx, &claimList, ...). The reconciler already has the field wired up.

---

2. HIGH - Invisible cluster-wide informer for Service (AP-3)

The ExternalModel reconciler does a r.Get() for corev1.Service which creates a cluster-wide Service informer.

Location	Code
externalmodel/reconciler.go:248	r.Get(ctx, ..., existing) where existing is &corev1.Service{}

Why it's real: Service is not in ByObject and not explicitly watched. First Get silently creates a cluster-wide informer caching every Service in every namespace.

Fix: Use mgr.GetAPIReader() for this one-off lookup, or add Service to ByObject with a label selector scoped to operator-managed services.

---

3. HIGH - Secret watched cluster-wide with predicate-only filtering (AP-1)

The TenantController watches Secrets with a name-based predicate (secretNamedMaaSDB()), but the underlying informer caches ALL Secrets cluster-wide.

Location	Code
tenant_controller.go:221	Watches(&corev1.Secret{}, ..., builder.WithPredicates(secretNamedMaaSDB(), r.inTenantWorkNamespaces()))

Why it's real: Secrets are high-cardinality and high-value - the informer LISTs and caches ALL Secrets, including ones with sensitive data the controller never touches. The predicates only filter which events trigger reconciliation, not what's cached.

Fix: Add &corev1.Secret{} to ByObject with a field selector or label selector limiting to the specific secret name, or switch to mgr.GetAPIReader() for occasional reads.

---

4. HIGH - Deployment watched cluster-wide with predicate-only filtering (AP-1)

The SelfDeploymentController uses For(&appsv1.Deployment{}, builder.WithPredicates(selfOnly)) - the predicate filters to only the controller's own Deployment, but the informer caches ALL Deployments.

Location	Code
self_deployment_controller.go:495	For(&appsv1.Deployment{}, builder.WithPredicates(selfOnly))

Why it's real: Deployment is high-cardinality. The informer caches every Deployment in every namespace when the controller only cares about its own single Deployment.

Fix: Add &appsv1.Deployment{} to ByObject with namespace + field selector narrowing to the controller's own Deployment.

---

5. HIGH - Tenant watched via both typed and unstructured (AP-5, duplicate informer)

The TenantController watches &maasv1alpha1.Tenant{} (typed), while the MaaSAuthPolicyController watches the same GVK via unstructured.Unstructured. controller-runtime maintains separate caches for typed vs unstructured - this creates two informers for the same resource.

Location	Representation
tenant_controller.go:202	For(&maasv1alpha1.Tenant{}) - typed
maasauthpolicy_controller.go:1744	Watches(unstructured Tenant{...}) - unstructured

Why it's real: This is the one genuine AP-5 finding. Two LIST+WATCH streams for the same GVK, double the memory and API server load.

Fix: Switch the unstructured Tenant watch in MaaSAuthPolicyController to typed &maasv1alpha1.Tenant{} - the Go type is already imported.

---

6. MEDIUM - No DefaultTransform to strip managedFields (AP-10)

The cache configuration has no transform function to strip managedFields metadata.

Location	Code
cmd/manager/main.go:591	cache.Options{...} - no DefaultTransform

Why it's real: Every cached object retains ~2-5KB of managedFields that the operator never reads. Easy win, zero risk.

Fix: Add DefaultTransform: cache.TransformStripManagedFields() to cache.Options. One line.

---

7. LOW - MaaSModelRef not in ByObject (AP-1)

MaaSModelRefController uses For(&maasv1alpha1.MaaSModelRef{}) but the type is not listed in ByObject, so it gets an implicit cluster-wide informer. Low severity because it's a custom CRD with manageable cardinality.

Location	Code
maasmodelref_controller.go:421	For(&maasv1alpha1.MaaSModelRef{}, ...)

Fix: Add &maasv1alpha1.MaaSModelRef{} to ByObject with namespace scoping.

---

8. LOW - Gateway lookups create informers (AP-3)

Multiple controllers do r.Get() for gatewayapiv1.Gateway. Gateway is low-cardinality (typically 1-5 per cluster), so the memory impact is negligible.

Location	Code
providers_external.go:245	h.r.Get(ctx, key, gateway)
providers_llmisvc.go:175	h.r.Get(ctx, key, gateway)
tenant_reconcile.go:471	c.Get(ctx, key, gw)
tenantreconcile/pipeline.go:64	c.Get(ctx, ..., gw)

Fix: Consider adding Gateway to ByObject or using APIReader. Low priority.

---

False positive summary

The scanner's true positive rate is roughly 25%. Here's what it got wrong and why:

Category	Scanner said	Actual	Root cause of false positive
AP-3: 20+ AITenant calls	CRITICAL	False positive	Scanner didn't trace APIReader field - all reads bypass cache
AP-3: All webhook calls	CRITICAL	False positive	Webhooks use mgr.GetAPIReader(), not cached client
AP-3: MaaS CRD List calls	CRITICAL	False positive	Types already in ByObject - List reads from existing cache, no new informer
AP-3: Watched type Get/List	CRITICAL	False positive	Types already have explicit For()/Watches() - informer exists by design, not "silently"
AP-5: Kuadrant unstructured (~38)	HIGH	False positive	No kuadrant.io types in go.mod - unstructured is the only option, no duplication possible
AP-5: externalmodel dynamic GVK	HIGH	False positive	Unstructured used at reconcile-time, no watch registered for same GVK
AP-4: No DefaultNamespaces	HIGH	Low / non-issue	ByObject already scopes each type explicitly
AP-6: Tenant cluster-wide	CRITICAL	False positive	Singleton CRD (CEL: self.metadata.name == 'default-tenant'), intentionally cluster-wide
AP-1: For() on ByObject types	HIGH	False positive	Predicate on primary type that's already namespace-scoped in ByObject

---

Scoreboard

#	Severity	What	Effort	Impact
1	HIGH	ConfigMap invisible informer - switch 2 calls to APIReader	5 min	Drops cluster-wide ConfigMap cache
2	HIGH	Service invisible informer - switch to APIReader	5 min	Drops cluster-wide Service cache
3	HIGH	Secret cluster-wide cache - add to ByObject with selector	15 min	Stops caching all Secrets
4	HIGH	Deployment cluster-wide cache - add to ByObject with selector	15 min	Stops caching all Deployments
5	HIGH	Tenant typed/unstructured duplicate - switch to typed	5 min	Eliminates duplicate informer
6	MEDIUM	Strip managedFields - one-line transform	2 min	~2-5KB savings per cached object
7	LOW	MaaSModelRef not in ByObject	10 min	Namespace-scope custom CRD cache
8	LOW	Gateway informer from Get()	10 min	Minimal - low cardinality type