improvements jan2025 (#6)

* feat(controller): add lazy watcher, improve resource usage and add pattern validation - [x] Add cache sync health check for readiness probe verification - [x] Create namespace lister with API reader support for fresh label queries - [x] Add pattern validation with warning logs for invalid glob patterns - [x] Implement lazy watcher initialization mode to scan for active resources - [x] Add requeue delay to namespace reconciler for cache settlement - [x] Replace custom containsString with slices.Contains from stdlib - [x] Add structured logging context to reconcilers (kind, group, version) - [x] Improve error variable naming for clarity in nested conditions - [x] Add nil-safe label access in namespace reconciler setup - [x] Add APIReader to namespace and source reconcilers for fresh data - [x] Improve type assertions with proper error handling in mirror operations - [x] Reorder struct fields for consistency and readability - [x] Add comprehensive pattern validation tests and validation API * feat(controller): add lazy watcher, improve resource usage and add pattern validation - [x] Add circuit breaker for reconciliation failure tracking and prevention - [x] Implement granular registration state tracking (not-registered, source-only, fully-registered) - [x] Add lazy controller initialization for active resource types only - [x] Consolidate namespace listing into single API call for efficiency - [x] Add mirror creation verification to catch webhook rejections - [x] Implement high-cardinality resource detection and warnings - [x] Add source deletion check in mirror reconciler to prevent races - [x] Preserve transformation annotations on errors in mirror reconciliation - [x] Expand constants documentation with labels vs annotations design rationale - [x] Add comprehensive test coverage for circuit breaker and registration states - [x] Add mutation-safety tests for hash computation * fixup! feat(controller): add lazy watcher, improve resource usage and add pattern validation
2026-06-13 02:51:59 +00:00 · 2026-01-14 13:07:11 +00:00
parent 4f8e2783cf
commit 096dca47d1
22 changed files with 1937 additions and 266 deletions
@@ -4,6 +4,8 @@ package controller
 import (
 	"context"
 	"fmt"
+	"slices"
+	"time"

 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -21,6 +23,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"

+	"github.com/lukaszraczylo/kubemirror/pkg/circuitbreaker"
 	"github.com/lukaszraczylo/kubemirror/pkg/config"
 	"github.com/lukaszraczylo/kubemirror/pkg/constants"
 	"github.com/lukaszraczylo/kubemirror/pkg/filter"
@@ -30,12 +33,13 @@ import (
 // SourceReconciler reconciles source resources that need mirroring.
 type SourceReconciler struct {
 	client.Client
+	NamespaceLister NamespaceLister
+	APIReader       client.Reader
 	Scheme          *runtime.Scheme
 	Config          *config.Config
 	Filter          *filter.NamespaceFilter
-	NamespaceLister NamespaceLister
-	GVK             schema.GroupVersionKind // The resource type this reconciler handles
-	APIReader       client.Reader           // Direct API reader (bypasses cache)
+	CircuitBreaker  *circuitbreaker.CircuitBreaker
+	GVK             schema.GroupVersionKind
 }

 // NamespaceLister provides a list of all namespaces in the cluster.
@@ -44,6 +48,8 @@ type NamespaceLister interface {
 	ListNamespaces(ctx context.Context) ([]string, error)
 	ListAllowMirrorsNamespaces(ctx context.Context) ([]string, error)
 	ListOptOutNamespaces(ctx context.Context) ([]string, error)
+	// ListNamespacesWithLabels returns all namespace info in a single API call (preferred)
+	ListNamespacesWithLabels(ctx context.Context) (*NamespaceInfo, error)
 }

 // +kubebuilder:rbac:groups=core,resources=secrets,verbs=get;list;watch;create;update;patch;delete
@@ -115,7 +121,13 @@ func (r *SourceReconciler) getSourceWithFreshness(ctx context.Context, key clien

 // Reconcile processes a single source resource.
 func (r *SourceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
-	logger := log.FromContext(ctx).WithValues("namespace", req.Namespace, "name", req.Name)
+	logger := log.FromContext(ctx).WithValues(
+		"namespace", req.Namespace,
+		"name", req.Name,
+		"kind", r.GVK.Kind,
+		"group", r.GVK.Group,
+		"version", r.GVK.Version,
+	)

 	// Fetch the source resource with optional freshness verification
 	source, err := r.getSourceWithFreshness(ctx, req.NamespacedName, r.GVK)
@@ -136,24 +148,40 @@ func (r *SourceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 		return ctrl.Result{}, nil
 	}

+	// Check circuit breaker - skip if circuit is open (too many failures)
+	if r.CircuitBreaker != nil {
+		if !r.CircuitBreaker.AllowRequest(req.Namespace, req.Name, r.GVK.Kind) {
+			cbState := r.CircuitBreaker.GetState(req.Namespace, req.Name, r.GVK.Kind)
+			failCount := r.CircuitBreaker.GetFailureCount(req.Namespace, req.Name, r.GVK.Kind)
+			logger.Info("circuit breaker open, skipping reconciliation",
+				"state", cbState.String(),
+				"consecutiveFailures", failCount,
+				"lastError", r.CircuitBreaker.GetLastError(req.Namespace, req.Name, r.GVK.Kind))
+			// Requeue after circuit breaker reset timeout to try again
+			return ctrl.Result{RequeueAfter: 5 * time.Minute}, nil
+		}
+	}
+
 	// Check if resource is enabled for mirroring
 	// Check if resource is being deleted
 	if !sourceObj.GetDeletionTimestamp().IsZero() {
 		// Resource is being deleted - clean up mirrors and remove finalizer
-		if containsString(sourceObj.GetFinalizers(), constants.FinalizerName) {
+		if slices.Contains(sourceObj.GetFinalizers(), constants.FinalizerName) {
 			logger.Info("source being deleted, cleaning up all mirrors")
-			if err := r.deleteAllMirrors(ctx, sourceObj); err != nil {
-				logger.Error(err, "failed to delete all mirrors during source deletion")
-				return ctrl.Result{}, err
+			deleteErr := r.deleteAllMirrors(ctx, sourceObj)
+			if deleteErr != nil {
+				logger.Error(deleteErr, "failed to delete all mirrors during source deletion")
+				return ctrl.Result{}, deleteErr
 			}

 			// Remove finalizer to allow resource deletion
 			logger.Info("removing finalizer from source resource")
 			finalizers := removeString(sourceObj.GetFinalizers(), constants.FinalizerName)
 			sourceObj.SetFinalizers(finalizers)
-			if err := r.Update(ctx, source); err != nil {
-				logger.Error(err, "failed to remove finalizer")
-				return ctrl.Result{}, err
+			updateErr := r.Update(ctx, source)
+			if updateErr != nil {
+				logger.Error(updateErr, "failed to remove finalizer")
+				return ctrl.Result{}, updateErr
 			}
 			logger.Info("finalizer removed, resource can now be deleted")
 		}
@@ -162,7 +190,7 @@ func (r *SourceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr

 	if !isEnabledForMirroring(sourceObj) {
 		// Resource is disabled - remove finalizer if present and delete all mirrors
-		if containsString(sourceObj.GetFinalizers(), constants.FinalizerName) {
+		if slices.Contains(sourceObj.GetFinalizers(), constants.FinalizerName) {
 			return r.handleDisabled(ctx, sourceObj)
 		}
 		// No finalizer, just skip
@@ -170,13 +198,14 @@ func (r *SourceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 	}

 	// Add finalizer if not present
-	if !containsString(sourceObj.GetFinalizers(), constants.FinalizerName) {
+	if !slices.Contains(sourceObj.GetFinalizers(), constants.FinalizerName) {
 		logger.Info("adding finalizer to source resource")
 		finalizers := append(sourceObj.GetFinalizers(), constants.FinalizerName)
 		sourceObj.SetFinalizers(finalizers)
-		if err := r.Update(ctx, source); err != nil {
-			logger.Error(err, "failed to add finalizer")
-			return ctrl.Result{}, err
+		addFinalizerErr := r.Update(ctx, source)
+		if addFinalizerErr != nil {
+			logger.Error(addFinalizerErr, "failed to add finalizer")
+			return ctrl.Result{}, addFinalizerErr
 		}
 		logger.Info("finalizer added")
 		// Requeue to continue with reconciliation after finalizer is added
@@ -187,6 +216,9 @@ func (r *SourceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 	targetNamespaces, err := r.resolveTargetNamespaces(ctx, sourceObj)
 	if err != nil {
 		logger.Error(err, "failed to resolve target namespaces")
+		if r.CircuitBreaker != nil {
+			r.CircuitBreaker.RecordFailure(req.Namespace, req.Name, r.GVK.Kind, err)
+		}
 		return ctrl.Result{}, err
 	}

@@ -200,8 +232,9 @@ func (r *SourceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 	// Reconcile each target namespace
 	var reconciledCount, errorCount int
 	for _, targetNs := range targetNamespaces {
-		if err := r.reconcileMirror(ctx, source, sourceObj, targetNs); err != nil {
-			logger.Error(err, "failed to reconcile mirror", "targetNamespace", targetNs)
+		reconcileErr := r.reconcileMirror(ctx, source, sourceObj, targetNs)
+		if reconcileErr != nil {
+			logger.Error(reconcileErr, "failed to reconcile mirror", "targetNamespace", targetNs)
 			errorCount++
 		} else {
 			reconciledCount++
@@ -220,6 +253,9 @@ func (r *SourceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 	// Update status annotation with last sync info
 	if err := r.updateLastSyncStatus(ctx, source, sourceObj, reconciledCount, errorCount); err != nil {
 		logger.Error(err, "failed to update sync status")
+		if r.CircuitBreaker != nil {
+			r.CircuitBreaker.RecordFailure(req.Namespace, req.Name, r.GVK.Kind, err)
+		}
 		return ctrl.Result{}, err
 	}

@@ -230,7 +266,22 @@ func (r *SourceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr

 	// Return error if there were errors (controller-runtime will automatically requeue with exponential backoff)
 	if errorCount > 0 {
-		return ctrl.Result{}, fmt.Errorf("failed to reconcile %d/%d mirrors", errorCount, len(targetNamespaces))
+		err := fmt.Errorf("failed to reconcile %d/%d mirrors", errorCount, len(targetNamespaces))
+		// Record failure with circuit breaker
+		if r.CircuitBreaker != nil {
+			state, justOpened := r.CircuitBreaker.RecordFailure(req.Namespace, req.Name, r.GVK.Kind, err)
+			if justOpened {
+				logger.Info("circuit breaker opened due to repeated failures",
+					"state", state.String(),
+					"consecutiveFailures", r.CircuitBreaker.GetFailureCount(req.Namespace, req.Name, r.GVK.Kind))
+			}
+		}
+		return ctrl.Result{}, err
+	}
+
+	// Record success with circuit breaker
+	if r.CircuitBreaker != nil {
+		r.CircuitBreaker.RecordSuccess(req.Namespace, req.Name, r.GVK.Kind)
 	}

 	return ctrl.Result{}, nil
@@ -247,7 +298,7 @@ func (r *SourceReconciler) handleDisabled(ctx context.Context, sourceObj metav1.
 	}

 	// Remove finalizer if present
-	if containsString(sourceObj.GetFinalizers(), constants.FinalizerName) {
+	if slices.Contains(sourceObj.GetFinalizers(), constants.FinalizerName) {
 		logger.Info("removing finalizer from disabled resource")
 		finalizers := removeString(sourceObj.GetFinalizers(), constants.FinalizerName)
 		sourceObj.SetFinalizers(finalizers)
@@ -301,9 +352,9 @@ func (r *SourceReconciler) reconcileMirror(ctx context.Context, source runtime.O
 		}

 		// Check if update is needed
-		needsSync, err := hash.NeedsSync(source, existing, existing.GetAnnotations())
-		if err != nil {
-			return fmt.Errorf("failed to check if sync needed: %w", err)
+		needsSync, syncCheckErr := hash.NeedsSync(source, existing, existing.GetAnnotations())
+		if syncCheckErr != nil {
+			return fmt.Errorf("failed to check if sync needed: %w", syncCheckErr)
 		}

 		if !needsSync {
@@ -312,12 +363,14 @@ func (r *SourceReconciler) reconcileMirror(ctx context.Context, source runtime.O
 		}

 		// Update mirror
-		if err := UpdateMirror(existing, source); err != nil {
-			return fmt.Errorf("failed to update mirror: %w", err)
+		updateErr := UpdateMirror(existing, source)
+		if updateErr != nil {
+			return fmt.Errorf("failed to update mirror: %w", updateErr)
 		}

-		if err := r.Update(ctx, existing); err != nil {
-			return fmt.Errorf("failed to update mirror in cluster: %w", err)
+		clusterUpdateErr := r.Update(ctx, existing)
+		if clusterUpdateErr != nil {
+			return fmt.Errorf("failed to update mirror in cluster: %w", clusterUpdateErr)
 		}

 		logger.V(1).Info("mirror updated")
@@ -330,11 +383,21 @@ func (r *SourceReconciler) reconcileMirror(ctx context.Context, source runtime.O
 		return fmt.Errorf("failed to create mirror: %w", err)
 	}

-	if err := r.Create(ctx, mirror.(client.Object)); err != nil {
+	mirrorObj := mirror.(client.Object)
+	if err := r.Create(ctx, mirrorObj); err != nil {
 		return fmt.Errorf("failed to create mirror in cluster: %w", err)
 	}

-	logger.V(1).Info("mirror created")
+	// Verify mirror was actually created (catches webhook rejections, quota issues)
+	verifyMirror := &unstructured.Unstructured{}
+	verifyMirror.SetGroupVersionKind(sourceUnstructured.GroupVersionKind())
+	verifyKey := client.ObjectKey{Namespace: targetNs, Name: sourceObj.GetName()}
+	if verifyErr := r.Get(ctx, verifyKey, verifyMirror); verifyErr != nil {
+		logger.Error(verifyErr, "mirror creation verification failed - mirror may have been rejected")
+		return fmt.Errorf("mirror creation verification failed: %w", verifyErr)
+	}
+
+	logger.V(1).Info("mirror created and verified")
 	return nil
 }

@@ -384,11 +447,12 @@ func (r *SourceReconciler) deleteAllMirrors(ctx context.Context, sourceObj metav
 func (r *SourceReconciler) cleanupOrphanedMirrors(ctx context.Context, sourceObj metav1.Object, targetNamespaces []string) (int, error) {
 	logger := log.FromContext(ctx)

-	// List all namespaces
-	allNamespaces, err := r.NamespaceLister.ListNamespaces(ctx)
+	// List all namespaces using unified method for consistency
+	nsInfo, err := r.NamespaceLister.ListNamespacesWithLabels(ctx)
 	if err != nil {
 		return 0, fmt.Errorf("failed to list namespaces: %w", err)
 	}
+	allNamespaces := nsInfo.All

 	// Get GVK from source object
 	sourceUnstructured, ok := sourceObj.(*unstructured.Unstructured)
@@ -472,30 +536,47 @@ func (r *SourceReconciler) resolveTargetNamespaces(ctx context.Context, sourceOb
 		return nil, nil
 	}

-	// Get all namespaces
-	allNamespaces, err := r.NamespaceLister.ListNamespaces(ctx)
+	// Validate patterns and log warnings for invalid ones
+	validationResults, allValid := filter.ValidatePatterns(patterns)
+	if !allValid {
+		logger := log.FromContext(ctx)
+		invalidPatterns := filter.InvalidPatterns(validationResults)
+		for _, invalid := range invalidPatterns {
+			logger.Info("invalid glob pattern in target-namespaces annotation, pattern will be skipped",
+				"pattern", invalid.Pattern,
+				"error", invalid.Error.Error(),
+				"source", sourceObj.GetName(),
+				"namespace", sourceObj.GetNamespace(),
+			)
+		}
+
+		// Filter to only valid patterns
+		var validPatterns []string
+		for _, result := range validationResults {
+			if result.Valid {
+				validPatterns = append(validPatterns, result.Pattern)
+			}
+		}
+		patterns = validPatterns
+
+		// If no valid patterns remain, return empty
+		if len(patterns) == 0 {
+			return nil, nil
+		}
+	}
+
+	// Get all namespace info in a single API call (more efficient than 3 separate calls)
+	nsInfo, err := r.NamespaceLister.ListNamespacesWithLabels(ctx)
 	if err != nil {
 		return nil, fmt.Errorf("failed to list namespaces: %w", err)
 	}

-	// Get namespaces with allow-mirrors label
-	allowMirrorsNamespaces, err := r.NamespaceLister.ListAllowMirrorsNamespaces(ctx)
-	if err != nil {
-		return nil, fmt.Errorf("failed to list allow-mirrors namespaces: %w", err)
-	}
-
-	// Get namespaces that have explicitly opted out (allow-mirrors="false")
-	optOutNamespaces, err := r.NamespaceLister.ListOptOutNamespaces(ctx)
-	if err != nil {
-		return nil, fmt.Errorf("failed to list opt-out namespaces: %w", err)
-	}
-
-	// Resolve target namespaces
+	// Resolve target namespaces using the pre-categorized namespace info
 	targetNamespaces := filter.ResolveTargetNamespaces(
 		patterns,
-		allNamespaces,
-		allowMirrorsNamespaces,
-		optOutNamespaces,
+		nsInfo.All,
+		nsInfo.AllowMirrors,
+		nsInfo.OptOut,
 		sourceObj.GetNamespace(),
 		r.Filter,
 	)
@@ -611,16 +692,6 @@ func (r *SourceReconciler) mapMirrorToSource(ctx context.Context, obj client.Obj
 	}
 }

-// containsString checks if a slice contains a string.
-func containsString(slice []string, s string) bool {
-	for _, item := range slice {
-		if item == s {
-			return true
-		}
-	}
-	return false
-}
-
 // removeString removes a string from a slice.
 func removeString(slice []string, s string) []string {
 	result := make([]string, 0, len(slice))