OSSM-2006 Alternative multiNamespaceInformer.HasSynced() fix (#24) (#27)

In the previous fix for informer.HasSynced(), the informer factories were constructed with no namespaces, enabling multiNamespaceInformer.HasSynced() to only return true after the member roll controller initialized the factory and informers by calling SetNamespaces(). Unfortunately, this broke all istio tests where the member roll controller isn't involved. In those cases, the factory and informers must be constructed so that they track all namespaces. Here, we add the ability for the member roll controller to call SetNamespaces(nil) when the factory is added to it as a listener. This ensures that the informer will return false on HasSynced() calls until the controller invokes SetNamespaces() with the actual list of namespaces read from the ServiceMeshMemberRoll. This ensures that the informer factory works correctly in both scenarios - with and without the member roll controller.
maistra · Oct 7, 2022 · 3ae7b65 · 3ae7b65
1 parent f0b868f
commit 3ae7b65
Show file tree

Hide file tree

Showing 11 changed files with 110 additions and 80 deletions.
diff --git a/cmd/xns-informer-gen/generators/factory.go b/cmd/xns-informer-gen/generators/factory.go
@@ -140,7 +140,7 @@ func WithTweakListOptions(tweakListOptions internalinterfaces.TweakListOptionsFu
 // WithNamespaces limits the SharedInformerFactory to the specified namespaces.
 func WithNamespaces(namespaces ...string) SharedInformerOption {
 	return func(factory *sharedInformerFactory) *sharedInformerFactory {
-        factory.SetNamespaces(namespaces...)
+        factory.SetNamespaces(namespaces)
 		return factory
 	}
 }
@@ -154,7 +154,7 @@ func NewSharedInformerFactory(client {{.clientSetInterface|raw}}, defaultResync
 func NewSharedInformerFactoryWithOptions(client {{.clientSetInterface|raw}}, defaultResync {{.timeDuration|raw}}, options ...SharedInformerOption) SharedInformerFactory {
 	factory := &sharedInformerFactory{
 		client:           client,
-        namespaces:       {{.xnsNewNamespaceSet|raw}}(),
+        namespaces:       {{.xnsNewNamespaceSet|raw}}(v1.NamespaceAll),
 		defaultResync:    defaultResync,
 		informers:        make(map[{{.reflectType|raw}}]{{.cacheSharedIndexInformer|raw}}),
 		startedInformers: make(map[{{.reflectType|raw}}]bool),
@@ -170,11 +170,11 @@ func NewSharedInformerFactoryWithOptions(client {{.clientSetInterface|raw}}, def
 }
 
 // SetNamespaces updates the set of namespaces for all current and future informers.
-func (f *sharedInformerFactory) SetNamespaces(namespaces ...string) {
+func (f *sharedInformerFactory) SetNamespaces(namespaces []string) {
     f.lock.Lock()
     defer f.lock.Unlock()
 
-    f.namespaces.SetNamespaces(namespaces...)
+    f.namespaces.SetNamespaces(namespaces)
 }
 
 // Start initializes all requested informers.
@@ -242,7 +242,7 @@ var sharedInformerFactoryInterface = `
 // API group versions.
 type SharedInformerFactory interface {
 	{{.informerFactoryInterface|raw}}
-    SetNamespaces(namespaces ...string)
+    SetNamespaces(namespaces []string)
 	ForResource(resource {{.schemaGroupVersionResource|raw}}) (GenericInformer, error)
 	WaitForCacheSync(stopCh <-chan struct{}) map[reflect.Type]bool
 

diff --git a/pkg/generated/istio/factory.go b/pkg/generated/istio/factory.go
diff --git a/pkg/generated/kube/factory.go b/pkg/generated/kube/factory.go
diff --git a/pkg/generated/serviceapis/factory.go b/pkg/generated/serviceapis/factory.go
diff --git a/pkg/informers/dynamic.go b/pkg/informers/dynamic.go
@@ -22,14 +22,14 @@ import (
 // namespaces, it will not work for cluster-scoped resources.
 type DynamicSharedInformerFactory interface {
 	Start(stopCh <-chan struct{})
-	SetNamespaces(namespaces ...string)
+	SetNamespaces(namespaces []string)
 	ForResource(gvr schema.GroupVersionResource) informers.GenericInformer
 	WaitForCacheSync(stopCh <-chan struct{}) map[schema.GroupVersionResource]bool
 }
 
 // NewDynamicSharedInformerFactory constructs a new instance of dynamicSharedInformerFactory for all namespaces.
 func NewDynamicSharedInformerFactory(client dynamic.Interface, defaultResync time.Duration) DynamicSharedInformerFactory {
-	namespaces := NewNamespaceSet()
+	namespaces := NewNamespaceSet(metav1.NamespaceAll)
 	return NewFilteredDynamicSharedInformerFactory(client, defaultResync, namespaces, nil)
 }
 
@@ -86,11 +86,11 @@ func (f *dynamicSharedInformerFactory) ForResource(gvr schema.GroupVersionResour
 }
 
 // SetNamespaces updates the set of namespaces for all current and future informers.
-func (f *dynamicSharedInformerFactory) SetNamespaces(namespaces ...string) {
+func (f *dynamicSharedInformerFactory) SetNamespaces(namespaces []string) {
 	f.lock.Lock()
 	defer f.lock.Unlock()
 
-	f.namespaces.SetNamespaces(namespaces...)
+	f.namespaces.SetNamespaces(namespaces)
 }
 
 // Start initializes all requested informers.

diff --git a/pkg/informers/dynamic_test.go b/pkg/informers/dynamic_test.go
@@ -55,7 +55,7 @@ func TestFilteredDynamicSharedInformerFactory(t *testing.T) {
 		// scenario 1
 		{
 			name:     "scenario 1: test adding an object in different namespace should not trigger AddFunc",
-			informNS: newNamespaceSet("ns-bar"),
+			informNS: xnsinformers.NewNamespaceSet("ns-bar"),
 			ns:       "ns-foo",
 			gvr:      schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"},
 			trigger:  triggerFactory(t),
@@ -64,7 +64,7 @@ func TestFilteredDynamicSharedInformerFactory(t *testing.T) {
 		// scenario 2
 		{
 			name:     "scenario 2: test adding an object should trigger AddFunc",
-			informNS: newNamespaceSet("ns-foo"),
+			informNS: xnsinformers.NewNamespaceSet("ns-foo"),
 			ns:       "ns-foo",
 			gvr:      schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"},
 			trigger:  triggerFactory(t),
@@ -119,7 +119,6 @@ func TestFilteredDynamicSharedInformerFactory(t *testing.T) {
 }
 
 func TestDynamicSharedInformerFactory(t *testing.T) {
-	ns := "ns-foo"
 	scenarios := []struct {
 		name        string
 		existingObj *unstructured.Unstructured
@@ -131,10 +130,10 @@ func TestDynamicSharedInformerFactory(t *testing.T) {
 		// scenario 1
 		{
 			name: "scenario 1: test if adding an object triggers AddFunc",
-			ns:   ns,
+			ns:   "ns-foo",
 			gvr:  schema.GroupVersionResource{Group: "extensions", Version: "v1beta1", Resource: "deployments"},
 			trigger: func(gvr schema.GroupVersionResource, ns string, fakeClient *fake.FakeDynamicClient, _ *unstructured.Unstructured) *unstructured.Unstructured {
-				testObject := newUnstructured("extensions/v1beta1", "Deployment", ns, "name-foo")
+				testObject := newUnstructured("extensions/v1beta1", "Deployment", "ns-foo", "name-foo")
 				createdObj, err := fakeClient.Resource(gvr).Namespace(ns).Create(context.TODO(), testObject, metav1.CreateOptions{})
 				if err != nil {
 					t.Error(err)
@@ -153,9 +152,9 @@ func TestDynamicSharedInformerFactory(t *testing.T) {
 		// scenario 2
 		{
 			name:        "scenario 2: tests if updating an object triggers UpdateFunc",
-			ns:          ns,
+			ns:          "ns-foo",
 			gvr:         schema.GroupVersionResource{Group: "extensions", Version: "v1beta1", Resource: "deployments"},
-			existingObj: newUnstructured("extensions/v1beta1", "Deployment", ns, "name-foo"),
+			existingObj: newUnstructured("extensions/v1beta1", "Deployment", "ns-foo", "name-foo"),
 			trigger: func(gvr schema.GroupVersionResource, ns string, fakeClient *fake.FakeDynamicClient, testObject *unstructured.Unstructured) *unstructured.Unstructured {
 				testObject.Object["spec"] = "updatedName"
 				updatedObj, err := fakeClient.Resource(gvr).Namespace(ns).Update(context.TODO(), testObject, metav1.UpdateOptions{})
@@ -176,9 +175,9 @@ func TestDynamicSharedInformerFactory(t *testing.T) {
 		// scenario 3
 		{
 			name:        "scenario 3: test if deleting an object triggers DeleteFunc",
-			ns:          ns,
+			ns:          "ns-foo",
 			gvr:         schema.GroupVersionResource{Group: "extensions", Version: "v1beta1", Resource: "deployments"},
-			existingObj: newUnstructured("extensions/v1beta1", "Deployment", ns, "name-foo"),
+			existingObj: newUnstructured("extensions/v1beta1", "Deployment", "ns-foo", "name-foo"),
 			trigger: func(gvr schema.GroupVersionResource, ns string, fakeClient *fake.FakeDynamicClient, testObject *unstructured.Unstructured) *unstructured.Unstructured {
 				err := fakeClient.Resource(gvr).Namespace(ns).Delete(context.TODO(), testObject.GetName(), metav1.DeleteOptions{})
 				if err != nil {
@@ -213,7 +212,6 @@ func TestDynamicSharedInformerFactory(t *testing.T) {
 			}
 			fakeClient := fake.NewSimpleDynamicClientWithCustomListKinds(scheme, gvrToListKind, objs...)
 			target := xnsinformers.NewDynamicSharedInformerFactory(fakeClient, 0)
-			target.SetNamespaces(ns)
 
 			// act
 			informerListerForGvr := target.ForResource(ts.gvr)

diff --git a/pkg/informers/informer_test.go b/pkg/informers/informer_test.go
@@ -103,7 +103,7 @@ func newInformer(obj runtime.Object, lws map[string]cache.ListerWatcher) xnsinfo
 		namespaces = append(namespaces, ns)
 	}
 
-	namespaceSet.SetNamespaces(namespaces...)
+	namespaceSet.SetNamespaces(namespaces)
 
 	return xnsinformers.NewMultiNamespaceInformer(namespaceSet, resync, func(ns string) cache.SharedIndexInformer {
 		return cache.NewSharedIndexInformer(lws[ns], obj, resync, indexers)
@@ -254,7 +254,7 @@ func TestMultiNamespaceInformerEventHandlers(t *testing.T) {
 
 	// These tests use the fake client instead of a FakeControllerSource.
 	client := kubefake.NewSimpleClientset()
-	namespaceSet := newNamespaceSet(namespaces...)
+	namespaceSet := xnsinformers.NewNamespaceSet(namespaces...)
 
 	informer := xnsinformers.NewMultiNamespaceInformer(namespaceSet, 0, func(namespace string) cache.SharedIndexInformer {
 		return cache.NewSharedIndexInformer(
@@ -356,7 +356,7 @@ func TestMultiNamespaceInformerHasSynced(t *testing.T) {
 		t.Fatalf("informer is synced, but shouldn't be because namespaces haven't been set yet")
 	}
 
-	namespaceSet.SetNamespaces("ns1", "ns2")
+	namespaceSet.SetNamespaces([]string{"ns1", "ns2"})
 
 	if informer.HasSynced() {
 		t.Fatalf("informer is synced, but shouldn't be because the underlying informers aren't synced")

diff --git a/pkg/informers/metadata.go b/pkg/informers/metadata.go
@@ -18,14 +18,14 @@ import (
 // MetadataSharedInformerFactory provides access to shared informers and listers for metadata client.
 type MetadataSharedInformerFactory interface {
 	Start(stopCh <-chan struct{})
-	SetNamespaces(namespaces ...string)
+	SetNamespaces(namespaces []string)
 	ForResource(gvr schema.GroupVersionResource) informers.GenericInformer
 	WaitForCacheSync(stopCh <-chan struct{}) map[schema.GroupVersionResource]bool
 }
 
 // NewMetadataSharedInformerFactory constructs a new instance of metadataSharedInformerFactory for all namespaces.
 func NewMetadataSharedInformerFactory(client metadata.Interface, defaultResync time.Duration) MetadataSharedInformerFactory {
-	namespaces := NewNamespaceSet()
+	namespaces := NewNamespaceSet(metav1.NamespaceAll)
 	return NewFilteredMetadataSharedInformerFactory(client, defaultResync, namespaces, nil)
 }
 
@@ -74,11 +74,11 @@ func (f *metadataSharedInformerFactory) ForResource(gvr schema.GroupVersionResou
 }
 
 // SetNamespaces updates the set of namespaces for all current and future informers.
-func (f *metadataSharedInformerFactory) SetNamespaces(namespaces ...string) {
+func (f *metadataSharedInformerFactory) SetNamespaces(namespaces []string) {
 	f.lock.Lock()
 	defer f.lock.Unlock()
 
-	f.namespaces.SetNamespaces(namespaces...)
+	f.namespaces.SetNamespaces(namespaces)
 }
 
 // Start initializes all requested informers.

diff --git a/pkg/informers/metadata_test.go b/pkg/informers/metadata_test.go
@@ -27,7 +27,6 @@ import (
 // }
 
 func TestMetadataSharedInformerFactory(t *testing.T) {
-	ns := "ns-foo"
 	scenarios := []struct {
 		name        string
 		existingObj *metav1.PartialObjectMetadata
@@ -39,10 +38,10 @@ func TestMetadataSharedInformerFactory(t *testing.T) {
 		// scenario 1
 		{
 			name: "scenario 1: test if adding an object triggers AddFunc",
-			ns:   ns,
+			ns:   "ns-foo",
 			gvr:  schema.GroupVersionResource{Group: "extensions", Version: "v1beta1", Resource: "deployments"},
 			trigger: func(gvr schema.GroupVersionResource, ns string, fakeClient *fake.FakeMetadataClient, _ *metav1.PartialObjectMetadata) *metav1.PartialObjectMetadata {
-				testObject := newPartialObjectMetadata("extensions/v1beta1", "Deployment", ns, "name-foo")
+				testObject := newPartialObjectMetadata("extensions/v1beta1", "Deployment", "ns-foo", "name-foo")
 				createdObj, err := fakeClient.Resource(gvr).Namespace(ns).(fake.MetadataClient).CreateFake(testObject, metav1.CreateOptions{})
 				if err != nil {
 					t.Error(err)
@@ -61,9 +60,9 @@ func TestMetadataSharedInformerFactory(t *testing.T) {
 		// scenario 2
 		{
 			name:        "scenario 2: tests if updating an object triggers UpdateFunc",
-			ns:          ns,
+			ns:          "ns-foo",
 			gvr:         schema.GroupVersionResource{Group: "extensions", Version: "v1beta1", Resource: "deployments"},
-			existingObj: newPartialObjectMetadata("extensions/v1beta1", "Deployment", ns, "name-foo"),
+			existingObj: newPartialObjectMetadata("extensions/v1beta1", "Deployment", "ns-foo", "name-foo"),
 			trigger: func(gvr schema.GroupVersionResource, ns string, fakeClient *fake.FakeMetadataClient, testObject *metav1.PartialObjectMetadata) *metav1.PartialObjectMetadata {
 				if testObject.Annotations == nil {
 					testObject.Annotations = make(map[string]string)
@@ -87,9 +86,9 @@ func TestMetadataSharedInformerFactory(t *testing.T) {
 		// scenario 3
 		{
 			name:        "scenario 3: test if deleting an object triggers DeleteFunc",
-			ns:          ns,
+			ns:          "ns-foo",
 			gvr:         schema.GroupVersionResource{Group: "extensions", Version: "v1beta1", Resource: "deployments"},
-			existingObj: newPartialObjectMetadata("extensions/v1beta1", "Deployment", ns, "name-foo"),
+			existingObj: newPartialObjectMetadata("extensions/v1beta1", "Deployment", "ns-foo", "name-foo"),
 			trigger: func(gvr schema.GroupVersionResource, ns string, fakeClient *fake.FakeMetadataClient, testObject *metav1.PartialObjectMetadata) *metav1.PartialObjectMetadata {
 				err := fakeClient.Resource(gvr).Namespace(ns).Delete(context.TODO(), testObject.GetName(), metav1.DeleteOptions{})
 				if err != nil {
@@ -122,7 +121,6 @@ func TestMetadataSharedInformerFactory(t *testing.T) {
 			}
 			fakeClient := fake.NewSimpleMetadataClient(scheme, objs...)
 			target := xnsinformers.NewMetadataSharedInformerFactory(fakeClient, 0)
-			target.SetNamespaces(ns)
 
 			// act
 			informerListerForGvr := target.ForResource(ts.gvr)