Address review feedback: use IsAssigned directly, add nil checks

- Remove helper functions from operations.go, use IsAssigned directly
- Replace all usages in main.go with direct IsAssigned calls
- Add nil checks in audit.AddToManager and webhook.AddToManager
- Remove unnecessary comments per feedback

Co-authored-by: JaydipGabani <[email protected]>

Author: Copilot

main.go

@@ -380,12 +380,10 @@ blockingLoop:
 }
 
 func setupControllers(ctx context.Context, mgr ctrl.Manager, tracker *readiness.Tracker, setupFinished chan struct{}) error {
-	// Block until the setup (certificate generation) finishes.
 	<-setupFinished
 
-	// Determine which dependencies need to be created based on enabled operations
-	needsOPAClient := operations.HasValidationOperations() || *externaldata.ExternalDataEnabled
-	needsMutationSystem := mutation.Enabled() || *expansion.ExpansionEnabled
+	needsOPAClient := operations.IsAssigned(operations.Audit) || operations.IsAssigned(operations.Webhook) || operations.IsAssigned(operations.Status) || *externaldata.ExternalDataEnabled
+	needsMutationSystem := operations.IsAssigned(operations.MutationWebhook) || operations.IsAssigned(operations.MutationController) || operations.IsAssigned(operations.MutationStatus) || *expansion.ExpansionEnabled
 	needsExpansionSystem := *expansion.ExpansionEnabled
 	needsProviderCache := *externaldata.ExternalDataEnabled
 
@@ -395,12 +393,9 @@ func setupControllers(ctx context.Context, mgr ctrl.Manager, tracker *readiness.
 	var expansionSystem *expansion.System
 	var certWatcher *certwatcher.CertWatcher
 
-	// Setup external data provider cache if needed
 	if needsProviderCache {
-		setupLog.Info("setting up external data provider cache")
 		providerCache = frameworksexternaldata.NewCache()
 
-		// Set up cert watcher for external data (shared between OPA and mutation)
 		certFile := filepath.Join(*certDir, certName)
 		keyFile := filepath.Join(*certDir, keyName)
 
@@ -411,16 +406,13 @@ func setupControllers(ctx context.Context, mgr ctrl.Manager, tracker *readiness.
 			return err
 		}
 
-		setupLog.Info("setting up client cert watcher")
 		if err := mgr.Add(certWatcher); err != nil {
 			setupLog.Error(err, "unable to register client cert watcher")
 			return err
 		}
 	}
 
-	// Setup OPA client if needed for validation operations or external data
 	if needsOPAClient {
-		setupLog.Info("setting up OPA client")
 		args := []rego.Arg{rego.Tracing(false), rego.DisableBuiltins(disabledBuiltins.ToSlice()...)}
 
 		if needsProviderCache {
@@ -438,7 +430,6 @@ func setupControllers(ctx context.Context, mgr ctrl.Manager, tracker *readiness.
 				return err
 			}
 
-			// Register the client cert watcher to the driver
 			args = append(args, rego.EnableExternalDataClientAuth(), rego.AddExternalDataClientCertWatcher(certWatcher))
 		}
 
@@ -483,9 +474,7 @@ func setupControllers(ctx context.Context, mgr ctrl.Manager, tracker *readiness.
 		}
 	}
 
-	// Setup mutation system if needed
 	if needsMutationSystem {
-		setupLog.Info("setting up mutation system")
 		mutationOpts := mutation.SystemOpts{Reporter: mutation.NewStatsReporter()}
 
 		if needsProviderCache {
@@ -496,13 +485,10 @@ func setupControllers(ctx context.Context, mgr ctrl.Manager, tracker *readiness.
 		mutationSystem = mutation.NewSystem(mutationOpts)
 	}
 
-	// Setup expansion system if needed (depends on mutation system)
 	if needsExpansionSystem {
-		setupLog.Info("setting up expansion system")
 		expansionSystem = expansion.NewSystem(mutationSystem)
 	}
 
-	// Export system is always created
 	exportSystem := export.NewSystem()
 
 	c := mgr.GetCache()
@@ -529,18 +515,11 @@ func setupControllers(ctx context.Context, mgr ctrl.Manager, tracker *readiness.
 		return err
 	}
 
-	// processExcluder is used for namespace exclusion for specified processes in config
 	processExcluder := process.Get()
 
-	// Setup all Controllers
-	setupLog.Info("setting up controllers")
-
-	// Cache manager is only needed for validation operations
 	var cm *cachemanager.CacheManager
 	var events chan event.GenericEvent
-	if operations.HasValidationOperations() {
-		// Events ch will be used to receive events from dynamic watches registered
-		// via the registrar below.
+	if operations.IsAssigned(operations.Audit) || operations.IsAssigned(operations.Webhook) || operations.IsAssigned(operations.Status) {
 		events = make(chan event.GenericEvent, 1024)
 		reg, err := wm.NewRegistrar(
 			cachemanager.RegistrarName,

pkg/audit/controller.go

@@ -38,6 +38,10 @@ func AddToManager(m manager.Manager, deps *Dependencies) error {
 		log.Info("auditing is disabled")
 		return nil
 	}
+	if deps.Client == nil {
+		log.Info("audit requires OPA client, skipping")
+		return nil
+	}
 	am, err := New(m, deps)
 	if err != nil {
 		return err

pkg/operations/operations.go

@@ -131,20 +131,3 @@ func AssignedStringList() []string {
 func HasValidationOperations() bool {
 	return IsAssigned(Audit) || IsAssigned(Status) || IsAssigned(Webhook)
 }
-
-// HasMutationOperations returns `true` if there
-// are any operations that would require mutation controllers.
-func HasMutationOperations() bool {
-	return IsAssigned(MutationController) || IsAssigned(MutationStatus) || IsAssigned(MutationWebhook)
-}
-
-// HasStatusOperation returns `true` if the Status operation is assigned.
-func HasStatusOperation() bool {
-	return IsAssigned(Status)
-}
-
-// HasGenerateOperation returns `true` if the Generate operation is assigned.
-func HasGenerateOperation() bool {
-	return IsAssigned(Generate)
-}
-

pkg/operations/operations_test.go

@@ -48,139 +48,3 @@ func Test_Flags(t *testing.T) {
 		})
 	}
 }
-
-// Test_HelperFunctions validates the helper functions for checking operation types.
-func Test_HelperFunctions(t *testing.T) {
-	tests := []struct {
-		name                 string
-		operations           []string
-		expectValidation     bool
-		expectMutation       bool
-		expectStatus         bool
-		expectGenerate       bool
-	}{
-		{
-			name:                 "audit only",
-			operations:           []string{"audit"},
-			expectValidation:     true,
-			expectMutation:       false,
-			expectStatus:         false,
-			expectGenerate:       false,
-		},
-		{
-			name:                 "webhook only",
-			operations:           []string{"webhook"},
-			expectValidation:     true,
-			expectMutation:       false,
-			expectStatus:         false,
-			expectGenerate:       false,
-		},
-		{
-			name:                 "status only",
-			operations:           []string{"status"},
-			expectValidation:     true,
-			expectMutation:       false,
-			expectStatus:         true,
-			expectGenerate:       false,
-		},
-		{
-			name:                 "mutation-webhook only",
-			operations:           []string{"mutation-webhook"},
-			expectValidation:     false,
-			expectMutation:       true,
-			expectStatus:         false,
-			expectGenerate:       false,
-		},
-		{
-			name:                 "mutation-controller only",
-			operations:           []string{"mutation-controller"},
-			expectValidation:     false,
-			expectMutation:       true,
-			expectStatus:         false,
-			expectGenerate:       false,
-		},
-		{
-			name:                 "mutation-status only",
-			operations:           []string{"mutation-status"},
-			expectValidation:     false,
-			expectMutation:       true,
-			expectStatus:         false,
-			expectGenerate:       false,
-		},
-		{
-			name:                 "generate only",
-			operations:           []string{"generate"},
-			expectValidation:     false,
-			expectMutation:       false,
-			expectStatus:         false,
-			expectGenerate:       true,
-		},
-		{
-			name:                 "audit and mutation-webhook",
-			operations:           []string{"audit", "mutation-webhook"},
-			expectValidation:     true,
-			expectMutation:       true,
-			expectStatus:         false,
-			expectGenerate:       false,
-		},
-		{
-			name:                 "status and generate",
-			operations:           []string{"status", "generate"},
-			expectValidation:     true,
-			expectMutation:       false,
-			expectStatus:         true,
-			expectGenerate:       true,
-		},
-		{
-			name:                 "all operations",
-			operations:           []string{"audit", "webhook", "status", "mutation-webhook", "mutation-controller", "mutation-status", "generate"},
-			expectValidation:     true,
-			expectMutation:       true,
-			expectStatus:         true,
-			expectGenerate:       true,
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			// Save current state
-			operationsMtx.Lock()
-			oldOps := operations
-			operations = newOperationSet()
-			operationsMtx.Unlock()
-
-			// Set up test operations
-			flagSet := flag.NewFlagSet("test", flag.ContinueOnError)
-			flagSet.Var(operations, "operation", "test")
-			
-			args := []string{}
-			for _, op := range tt.operations {
-				args = append(args, "-operation", op)
-			}
-			
-			if err := flagSet.Parse(args); err != nil {
-				t.Fatalf("failed to parse flags: %v", err)
-			}
-
-			// Test helper functions
-			if got := HasValidationOperations(); got != tt.expectValidation {
-				t.Errorf("HasValidationOperations() = %v, want %v", got, tt.expectValidation)
-			}
-			if got := HasMutationOperations(); got != tt.expectMutation {
-				t.Errorf("HasMutationOperations() = %v, want %v", got, tt.expectMutation)
-			}
-			if got := HasStatusOperation(); got != tt.expectStatus {
-				t.Errorf("HasStatusOperation() = %v, want %v", got, tt.expectStatus)
-			}
-			if got := HasGenerateOperation(); got != tt.expectGenerate {
-				t.Errorf("HasGenerateOperation() = %v, want %v", got, tt.expectGenerate)
-			}
-
-			// Restore state
-			operationsMtx.Lock()
-			operations = oldOps
-			operationsMtx.Unlock()
-		})
-	}
-}
-

pkg/webhook/webhook.go

@@ -42,6 +42,10 @@ var AddToManagerFuncs []func(manager.Manager, Dependencies) error
 
 // AddToManager adds all Controllers to the Manager.
 func AddToManager(m manager.Manager, deps Dependencies) error {
+	if deps.OpaClient == nil && deps.MutationSystem == nil {
+		log.Info("webhook requires OPA client or mutation system, skipping")
+		return nil
+	}
 	for _, f := range AddToManagerFuncs {
 		if err := f(m, deps); err != nil {
 			return err