fix: guard against some race conditions and add a basic race test

Author: FourFifthsCode

providers/kubeconfig/provider.go

@@ -231,15 +231,18 @@ func (p *Provider) handleSecret(ctx context.Context, secret *corev1.Secret, mgr
 		return fmt.Errorf("failed to create cluster: %w", err)
 	}
 
-	// Apply any field indexers
+	// Copy indexers to avoid holding lock.
 	p.lock.RLock()
-	for _, idx := range p.indexers {
+	indexers := make([]index, len(p.indexers))
+	copy(indexers, p.indexers)
+	p.lock.RUnlock()
+
+	// Apply any field indexers
+	for _, idx := range indexers {
 		if err := cl.GetFieldIndexer().IndexField(ctx, idx.object, idx.field, idx.extractValue); err != nil {
-			p.lock.RUnlock()
 			return fmt.Errorf("failed to index field %q: %w", idx.field, err)
 		}
 	}
-	p.lock.RUnlock()
 
 	// Create a context that will be canceled when this cluster is removed
 	clusterCtx, cancel := context.WithCancel(ctx)
@@ -309,32 +312,27 @@ func (p *Provider) removeCluster(clusterName string) error {
 	log := p.log.WithValues("cluster", clusterName)
 	log.Info("Removing cluster")
 
-	// Find the cluster and cancel function
-	p.lock.RLock()
+	p.lock.Lock()
 	ac, exists := p.clusters[clusterName]
 	if !exists {
-		p.lock.RUnlock()
-		return fmt.Errorf("cluster %s not found", clusterName)
+		p.lock.Unlock()
+		return fmt.Errorf("cluster not found")
 	}
-	p.lock.RUnlock()
+	delete(p.clusters, clusterName)
+	p.lock.Unlock()
 
-	// Cancel the context to trigger cleanup for this cluster
+	// Cancel the context to trigger cleanup for this cluster.
+	// This is done outside the lock to avoid holding the lock for a long time.
 	ac.Cancel()
 	log.Info("Cancelled cluster context")
 
-	// Clean up our maps
-	p.lock.Lock()
-	delete(p.clusters, clusterName)
-	p.lock.Unlock()
-
 	log.Info("Successfully removed cluster")
 	return nil
 }
 
 // IndexField indexes a field on all clusters, existing and future.
 func (p *Provider) IndexField(ctx context.Context, obj client.Object, field string, extractValue client.IndexerFunc) error {
 	p.lock.Lock()
-	defer p.lock.Unlock()
 
 	// Save for future clusters
 	p.indexers = append(p.indexers, index{
@@ -343,9 +341,16 @@ func (p *Provider) IndexField(ctx context.Context, obj client.Object, field stri
 		extractValue: extractValue,
 	})
 
-	// Apply to existing clusters
+	// Create a copy of the clusters to avoid holding the lock.
+	clustersSnapshot := make(map[string]cluster.Cluster, len(p.clusters))
 	for name, ac := range p.clusters {
-		if err := ac.Cluster.GetFieldIndexer().IndexField(ctx, obj, field, extractValue); err != nil {
+		clustersSnapshot[name] = ac.Cluster
+	}
+	p.lock.Unlock()
+
+	// Apply to existing clusters
+	for name, cl := range clustersSnapshot {
+		if err := cl.GetFieldIndexer().IndexField(ctx, obj, field, extractValue); err != nil {
 			return fmt.Errorf("failed to index field %q on cluster %q: %w", field, name, err)
 		}
 	}

providers/kubeconfig/provider_test.go

@@ -21,6 +21,8 @@ import (
 	"errors"
 	"fmt"
 	"strconv"
+	"sync"
+	"time"
 
 	"golang.org/x/sync/errgroup"
 
@@ -35,6 +37,7 @@ import (
 
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/cluster"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
@@ -317,3 +320,75 @@ func createKubeconfigSecret(ctx context.Context, name string, cfg *rest.Config,
 	}
 	return cl.Create(ctx, secret)
 }
+
+// mockCluster is a mock implementation of cluster.Cluster for testing.
+type mockCluster struct {
+	cluster.Cluster
+}
+
+func (c *mockCluster) GetFieldIndexer() client.FieldIndexer {
+	return &mockFieldIndexer{}
+}
+
+type mockFieldIndexer struct{}
+
+func (f *mockFieldIndexer) IndexField(ctx context.Context, obj client.Object, field string, extractValue client.IndexerFunc) error {
+	// Simulate work to increase chance of race
+	time.Sleep(time.Millisecond)
+	return nil
+}
+
+var _ = Describe("Provider race condition", func() {
+	It("should handle concurrent operations without issues", func() {
+		p := New(Options{})
+
+		// Pre-populate with some clusters to make the test meaningful
+		numClusters := 20
+		for i := 0; i < numClusters; i++ {
+			clusterName := fmt.Sprintf("cluster-%d", i)
+			p.clusters[clusterName] = activeCluster{
+				Cluster: &mockCluster{},
+				Cancel:  func() {},
+			}
+		}
+
+		var wg sync.WaitGroup
+		numGoroutines := 40
+		wg.Add(numGoroutines)
+
+		for i := 0; i < numGoroutines; i++ {
+			go func(i int) {
+				defer GinkgoRecover()
+				defer wg.Done()
+
+				// Mix of operations to stress the provider
+				switch i % 4 {
+				case 0:
+					// Concurrently index a field. This will read the cluster list.
+					err := p.IndexField(context.Background(), &corev1.Pod{}, "spec.nodeName", func(rawObj client.Object) []string {
+						return nil
+					})
+					Expect(err).NotTo(HaveOccurred())
+				case 1:
+					// Concurrently get a cluster.
+					_, err := p.Get(context.Background(), "cluster-1")
+					Expect(err).NotTo(HaveOccurred())
+				case 2:
+					// Concurrently list clusters.
+					p.ListClusters()
+				case 3:
+					// Concurrently delete a cluster. This will modify the cluster map.
+					clusterToRemove := fmt.Sprintf("cluster-%d", i/4)
+					secret := &corev1.Secret{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: clusterToRemove,
+						},
+					}
+					p.handleSecretDelete(secret)
+				}
+			}(i)
+		}
+
+		wg.Wait()
+	})
+})