Review comments fixes, adding comments, improving funcs signatures

cmd/glbc/main.go

@@ -246,14 +246,15 @@ func main() {
 		rootLogger.Info("Multi-project mode is enabled, starting project-syncer")
 
 		runWithWg(func() {
-			gceCreator, err := multiprojectgce.NewGCEFromFileStringCreator(rootLogger)
+			gceCreator, err := multiprojectgce.NewDefaultGCECreator(rootLogger)
 			if err != nil {
 				klog.Fatalf("Failed to create GCE creator: %v", err)
 			}
 			providerConfigClient, err := providerconfigclient.NewForConfig(kubeConfig)
 			if err != nil {
 				klog.Fatalf("Failed to create ProviderConfig client: %v", err)
 			}
+			informersFactory := informers.NewSharedInformerFactory(kubeClient, flags.F.ResyncPeriod)
 			if flags.F.LeaderElection.LeaderElect {
 				err := multiprojectstart.StartWithLeaderElection(
 					context.Background(),
@@ -265,6 +266,7 @@ func main() {
 					kubeSystemUID,
 					eventRecorderKubeClient,
 					providerConfigClient,
+					informersFactory,
 					gceCreator,
 					namer,
 					stopCh,
@@ -273,7 +275,6 @@ func main() {
 					rootLogger.Error(err, "Failed to start multi-project syncer with leader election")
 				}
 			} else {
-				informersFactory := informers.NewSharedInformerFactory(kubeClient, flags.F.ResyncPeriod)
 				multiprojectstart.Start(
 					rootLogger,
 					kubeClient,

pkg/multiproject/gce/fake.go

@@ -28,14 +28,8 @@ func providerConfigKey(providerConfig *v1.ProviderConfig) string {
 	return fmt.Sprintf("%s/%s", providerConfig.Namespace, providerConfig.Name)
 }
 
-func (g *GCEFake) GetGCEForProviderConfig(providerConfig *v1.ProviderConfig) (*cloudgce.Cloud, error) {
-	pcKey := providerConfigKey(providerConfig)
-	if g.clientsForProviderConfigs[pcKey] == nil {
-		return nil, fmt.Errorf("GCEFake does not have a client for provider config %q", pcKey)
-	}
-	return g.clientsForProviderConfigs[pcKey], nil
-}
-
+// GCEForProviderConfig returns a new Fake GCE client for the given provider config.
+// It stores the client in the GCEFake and returns it if the same provider config is requested again.
 func (g *GCEFake) GCEForProviderConfig(providerConfig *v1.ProviderConfig, logger klog.Logger) (*cloudgce.Cloud, error) {
 	pcKey := providerConfigKey(providerConfig)
 	if g.clientsForProviderConfigs[pcKey] != nil {
@@ -57,15 +51,15 @@ func (g *GCEFake) GCEForProviderConfig(providerConfig *v1.ProviderConfig, logger
 	if err != nil {
 		return nil, err
 	}
-	if err := g.CreateAndInsertNodes(fakeCloud, []string{"test-node-1"}, updatedConfig.ZoneName); err != nil {
+	if err := createAndInsertNodes(fakeCloud, []string{"test-node-1"}, updatedConfig.ZoneName); err != nil {
 		return nil, err
 	}
 
 	g.clientsForProviderConfigs[pcKey] = fakeCloud
 	return fakeCloud, nil
 }
 
-func (g *GCEFake) CreateAndInsertNodes(cloud *cloudgce.Cloud, nodeNames []string, zone string) error {
+func createAndInsertNodes(cloud *cloudgce.Cloud, nodeNames []string, zone string) error {
 	if _, err := test.CreateAndInsertNodes(cloud, nodeNames, zone); err != nil {
 		return err
 	}

pkg/multiproject/gce/gce.go

@@ -25,24 +25,24 @@ type GCECreator interface {
 	GCEForProviderConfig(providerConfig *v1.ProviderConfig, logger klog.Logger) (*cloudgce.Cloud, error)
 }
 
-type GCEFromFileStringCreator struct {
+type DefaultGCECreator struct {
 	defaultConfigFileString string
 }
 
-func NewGCEFromFileStringCreator(logger klog.Logger) (*GCEFromFileStringCreator, error) {
+func NewDefaultGCECreator(logger klog.Logger) (*DefaultGCECreator, error) {
 	defaultGCEConfig, err := app.GCEConfString(logger)
 	if err != nil {
 		return nil, fmt.Errorf("error getting default cluster GCE config: %v", err)
 	}
-	return &GCEFromFileStringCreator{
+	return &DefaultGCECreator{
 		defaultConfigFileString: defaultGCEConfig,
 	}, nil
 }
 
 // GCEForProviderConfig returns a new GCE client for the given project.
 // If providerConfig is nil, it returns the default cloud associated with the cluster's project.
 // It modifies the default configuration when a providerConfig is provided.
-func (g *GCEFromFileStringCreator) GCEForProviderConfig(providerConfig *v1.ProviderConfig, logger klog.Logger) (*cloudgce.Cloud, error) {
+func (g *DefaultGCECreator) GCEForProviderConfig(providerConfig *v1.ProviderConfig, logger klog.Logger) (*cloudgce.Cloud, error) {
 	modifiedConfigContent, err := generateConfigForProviderConfig(g.defaultConfigFileString, providerConfig)
 	if err != nil {
 		return nil, fmt.Errorf("failed to modify config content: %v", err)

pkg/multiproject/neg/neg.go

@@ -139,6 +139,9 @@ func initializeInformers(
 		informersFactory.Discovery().V1().EndpointSlices().Informer(),
 		providerConfigName,
 	)
+	// Even though we created separate "provider-config-filtered" informer, informers from the same
+	// factory will share indexers. That's why we need to add the indexer only if it's not present.
+	// This basically means we will only add indexer to the first provider config's informer.
 	err := addIndexerIfNotPresent(endpointSliceInformer.GetIndexer(), endpointslices.EndpointSlicesByServiceIndex, endpointslices.EndpointSlicesByServiceFunc)
 	if err != nil {
 		return nil, nil, fmt.Errorf("failed to add indexers to endpointSliceInformer: %v", err)
@@ -221,6 +224,8 @@ func initializeInformers(
 	return informers, hasSynced, nil
 }
 
+// addIndexerIfNotPresent adds an indexer to the indexer if it's not present.
+// This is needed because informers from the same factory will share indexers.
 func addIndexerIfNotPresent(indexer cache.Indexer, indexName string, indexFunc cache.IndexFunc) error {
 	indexers := indexer.GetIndexers()
 	if _, ok := indexers[indexName]; ok {

pkg/multiproject/start/start.go

@@ -39,14 +39,13 @@ func StartWithLeaderElection(
 	kubeSystemUID types.UID,
 	eventRecorderKubeClient kubernetes.Interface,
 	providerConfigClient providerconfigclient.Interface,
+	informersFactory informers.SharedInformerFactory,
 	gceCreator gce.GCECreator,
 	rootNamer *namer.Namer,
 	stopCh <-chan struct{},
 ) error {
 	recordersManager := recorders.NewManager(eventRecorderKubeClient, logger)
 
-	informersFactory := informers.NewSharedInformerFactoryWithOptions(kubeClient, flags.F.ResyncPeriod)
-
 	leConfig, err := makeLeaderElectionConfig(leaderElectKubeClient, hostname, recordersManager, logger, kubeClient, svcNegClient, kubeSystemUID, eventRecorderKubeClient, providerConfigClient, informersFactory, gceCreator, rootNamer, stopCh)
 	if err != nil {
 		return err

pkg/multiproject/start/start_test.go

@@ -180,6 +180,7 @@ func TestStartProviderConfigIntegration(t *testing.T) {
 			svcNegClient := svcnegfake.NewSimpleClientset()
 
 			// This simulates the automatic labeling that the real environment does.
+			// ProviderConfig name label is set to the namespace of the object.
 			testutil.EmulateProviderConfigLabelingWebhook(svcNegClient.Tracker(), &svcNegClient.Fake, "servicenetworkendpointgroups")
 
 			logger := klog.TODO()
@@ -294,7 +295,7 @@ func validateService(
 			t.Errorf("Svc NEG on Service %q is not in the expected state: %v", svc.Name, err)
 		}
 
-		gce, err := gceCreator.GetGCEForProviderConfig(pc)
+		gce, err := gceCreator.GCEForProviderConfig(pc, klog.TODO())
 		if err != nil {
 			t.Errorf("Failed to get GCE for ProviderConfig %q: %v", pc.Name, err)
 			continue

pkg/multiproject/testutil/providerconfigwebhook.go

@@ -17,6 +17,10 @@ type FakeTracker interface {
 // It will set the providerconfig name label on the object, on creation.
 // This is needed, as in unit/integration tests, when we create objects, we expect
 // the providerconfig name label to be set.
+//
+// Important: this function sets ProviderConfig name label to the namespace of the object.
+// However, in the real world, multiple namespaces can have the same providerconfig name.
+//
 // The function takes a fake client and the name of the CRD.
 func EmulateProviderConfigLabelingWebhook(tracker FakeTracker, fake *testing.Fake, crName string) {
 	fake.PrependReactor("create", crName, func(action testing.Action) (handled bool, ret runtime.Object, err error) {

pkg/neg/controller.go

@@ -178,7 +178,6 @@ func NewController(
 		apiv1.EventSource{Component: "neg-controller"})
 
 	syncerMetrics := syncMetrics.NewNegMetricsCollector(flags.F.NegMetricsExportInterval, logger)
-
 	manager := newSyncerManager(
 		namer,
 		l4Namer,