OSSM-2006 Fix HasSynced()

Previously, if the set of namespaces in the multiNamespaceInformer was empty, HasSynced() would return true. This caused controllers to start, even though there are no informers in multiNamespaceeInformer. The controllers would read from the cache (which would return no resources), causing them to perform the reconcile on bad input data. Typically, this would cause the controller to remove resources that shouldn't be removed. Of course, moments later, the namespace list gets initialized, the controllers are notified of the appearance of new resources, and they recreate the resources they had removed a moment ago. Although the end state is correct, for some resource types, such as Routes, their deletion wreaks havoc.

HasSynced() shouldn't return true until SetNamespaces() has been called at least once (this is done by the ServiceMeshMemberRoll controller after it reads the ServiceMeshMemberRoll object for the first time). The informer also shouldn't be initialized with all namespaces, but should remain empty until the actual member namespace list is read from the ServiceMeshMemberRoll.

cmd/xns-informer-gen/generators/factory.go

@@ -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}}(v1.NamespaceAll),
+        namespaces:       {{.xnsNewNamespaceSet|raw}}(),
 		defaultResync:    defaultResync,
 		informers:        make(map[{{.reflectType|raw}}]{{.cacheSharedIndexInformer|raw}}),
 		startedInformers: make(map[{{.reflectType|raw}}]bool),

pkg/informers/dynamic.go

@@ -29,7 +29,7 @@ type DynamicSharedInformerFactory interface {
 
 // NewDynamicSharedInformerFactory constructs a new instance of dynamicSharedInformerFactory for all namespaces.
 func NewDynamicSharedInformerFactory(client dynamic.Interface, defaultResync time.Duration) DynamicSharedInformerFactory {
-	namespaces := NewNamespaceSet(metav1.NamespaceAll)
+	namespaces := NewNamespaceSet()
 	return NewFilteredDynamicSharedInformerFactory(client, defaultResync, namespaces, nil)
 }
 

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: xnsinformers.NewNamespaceSet("ns-bar"),
+			informNS: 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: xnsinformers.NewNamespaceSet("ns-foo"),
+			informNS: newNamespaceSet("ns-foo"),
 			ns:       "ns-foo",
 			gvr:      schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"},
 			trigger:  triggerFactory(t),
@@ -119,6 +119,7 @@ func TestFilteredDynamicSharedInformerFactory(t *testing.T) {
 }
 
 func TestDynamicSharedInformerFactory(t *testing.T) {
+	ns := "ns-foo"
 	scenarios := []struct {
 		name        string
 		existingObj *unstructured.Unstructured
@@ -130,10 +131,10 @@ func TestDynamicSharedInformerFactory(t *testing.T) {
 		// scenario 1
 		{
 			name: "scenario 1: test if adding an object triggers AddFunc",
-			ns:   "ns-foo",
+			ns:   ns,
 			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-foo", "name-foo")
+				testObject := newUnstructured("extensions/v1beta1", "Deployment", ns, "name-foo")
 				createdObj, err := fakeClient.Resource(gvr).Namespace(ns).Create(context.TODO(), testObject, metav1.CreateOptions{})
 				if err != nil {
 					t.Error(err)
@@ -152,9 +153,9 @@ func TestDynamicSharedInformerFactory(t *testing.T) {
 		// scenario 2
 		{
 			name:        "scenario 2: tests if updating an object triggers UpdateFunc",
-			ns:          "ns-foo",
+			ns:          ns,
 			gvr:         schema.GroupVersionResource{Group: "extensions", Version: "v1beta1", Resource: "deployments"},
-			existingObj: newUnstructured("extensions/v1beta1", "Deployment", "ns-foo", "name-foo"),
+			existingObj: newUnstructured("extensions/v1beta1", "Deployment", ns, "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{})
@@ -175,9 +176,9 @@ func TestDynamicSharedInformerFactory(t *testing.T) {
 		// scenario 3
 		{
 			name:        "scenario 3: test if deleting an object triggers DeleteFunc",
-			ns:          "ns-foo",
+			ns:          ns,
 			gvr:         schema.GroupVersionResource{Group: "extensions", Version: "v1beta1", Resource: "deployments"},
-			existingObj: newUnstructured("extensions/v1beta1", "Deployment", "ns-foo", "name-foo"),
+			existingObj: newUnstructured("extensions/v1beta1", "Deployment", ns, "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,6 +214,7 @@ func TestDynamicSharedInformerFactory(t *testing.T) {
 			}
 			fakeClient := fake.NewSimpleDynamicClient(scheme, objs...)
 			target := xnsinformers.NewDynamicSharedInformerFactory(fakeClient, 0)
+			target.SetNamespaces(ns)
 
 			// act
 			informerListerForGvr := target.ForResource(ts.gvr)

pkg/informers/informer.go

@@ -265,6 +265,10 @@ func (i *multiNamespaceInformer) HasSynced() bool {
 	i.lock.Lock()
 	defer i.lock.Unlock()
 
+	if !i.namespaces.Initialized() {
+		return false
+	}
+
 	for _, informer := range i.informers {
 		if synced := informer.HasSynced(); !synced {
 			return false

pkg/informers/informer_test.go

@@ -251,7 +251,7 @@ func TestMultiNamespaceInformerEventHandlers(t *testing.T) {
 
 	// These tests use the fake client instead of a FakeControllerSource.
 	client := kubefake.NewSimpleClientset()
-	namespaceSet := xnsinformers.NewNamespaceSet(namespaces...)
+	namespaceSet := newNamespaceSet(namespaces...)
 
 	informer := xnsinformers.NewMultiNamespaceInformer(namespaceSet, 0, func(namespace string) cache.SharedIndexInformer {
 		return cache.NewSharedIndexInformer(
@@ -330,3 +330,74 @@ func TestMultiNamespaceInformerEventHandlers(t *testing.T) {
 		t.Fatalf("Delete handler not called after namespace removal: %v", err)
 	}
 }
+
+func TestMultiNamespaceInformerHasSynced(t *testing.T) {
+	namespaceSet := xnsinformers.NewNamespaceSet()
+	hasSynced := false
+
+	informer := xnsinformers.NewMultiNamespaceInformer(namespaceSet, 0, func(namespace string) cache.SharedIndexInformer {
+		return mockInformer{
+			hasSynced: &hasSynced,
+		}
+	})
+
+	if informer.HasSynced() {
+		t.Fatalf("informer is synced, but shouldn't be because namespaces haven't been set yet")
+	}
+
+	namespaceSet.SetNamespaces("ns1", "ns2")
+
+	if informer.HasSynced() {
+		t.Fatalf("informer is synced, but shouldn't be because the underlying informers aren't synced")
+	}
+
+	hasSynced = true
+
+	if !informer.HasSynced() {
+		t.Fatalf("expected informer to be synced")
+	}
+}
+
+type mockInformer struct {
+	hasSynced *bool
+}
+
+func (m mockInformer) AddEventHandler(handler cache.ResourceEventHandler) {
+	panic("not implemented")
+}
+
+func (m mockInformer) AddEventHandlerWithResyncPeriod(handler cache.ResourceEventHandler, resyncPeriod time.Duration) {
+	panic("not implemented")
+}
+
+func (m mockInformer) GetStore() cache.Store {
+	panic("not implemented")
+}
+
+func (m mockInformer) GetController() cache.Controller {
+	panic("not implemented")
+}
+
+func (m mockInformer) Run(stopCh <-chan struct{}) {
+	panic("not implemented")
+}
+
+func (m mockInformer) HasSynced() bool {
+	return *m.hasSynced
+}
+
+func (m mockInformer) LastSyncResourceVersion() string {
+	panic("not implemented")
+}
+
+func (m mockInformer) SetWatchErrorHandler(handler cache.WatchErrorHandler) error {
+	panic("not implemented")
+}
+
+func (m mockInformer) AddIndexers(indexers cache.Indexers) error {
+	panic("not implemented")
+}
+
+func (m mockInformer) GetIndexer() cache.Indexer {
+	panic("not implemented")
+}

pkg/informers/metadata.go

@@ -25,7 +25,7 @@ type MetadataSharedInformerFactory interface {
 
 // NewMetadataSharedInformerFactory constructs a new instance of metadataSharedInformerFactory for all namespaces.
 func NewMetadataSharedInformerFactory(client metadata.Interface, defaultResync time.Duration) MetadataSharedInformerFactory {
-	namespaces := NewNamespaceSet(metav1.NamespaceAll)
+	namespaces := NewNamespaceSet()
 	return NewFilteredMetadataSharedInformerFactory(client, defaultResync, namespaces, nil)
 }
 

pkg/informers/metadata_test.go

@@ -27,6 +27,7 @@ import (
 // }
 
 func TestMetadataSharedInformerFactory(t *testing.T) {
+	ns := "ns-foo"
 	scenarios := []struct {
 		name        string
 		existingObj *metav1.PartialObjectMetadata
@@ -38,10 +39,10 @@ func TestMetadataSharedInformerFactory(t *testing.T) {
 		// scenario 1
 		{
 			name: "scenario 1: test if adding an object triggers AddFunc",
-			ns:   "ns-foo",
+			ns:   ns,
 			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-foo", "name-foo")
+				testObject := newPartialObjectMetadata("extensions/v1beta1", "Deployment", ns, "name-foo")
 				createdObj, err := fakeClient.Resource(gvr).Namespace(ns).(fake.MetadataClient).CreateFake(testObject, metav1.CreateOptions{})
 				if err != nil {
 					t.Error(err)
@@ -60,9 +61,9 @@ func TestMetadataSharedInformerFactory(t *testing.T) {
 		// scenario 2
 		{
 			name:        "scenario 2: tests if updating an object triggers UpdateFunc",
-			ns:          "ns-foo",
+			ns:          ns,
 			gvr:         schema.GroupVersionResource{Group: "extensions", Version: "v1beta1", Resource: "deployments"},
-			existingObj: newPartialObjectMetadata("extensions/v1beta1", "Deployment", "ns-foo", "name-foo"),
+			existingObj: newPartialObjectMetadata("extensions/v1beta1", "Deployment", ns, "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)
@@ -86,9 +87,9 @@ func TestMetadataSharedInformerFactory(t *testing.T) {
 		// scenario 3
 		{
 			name:        "scenario 3: test if deleting an object triggers DeleteFunc",
-			ns:          "ns-foo",
+			ns:          ns,
 			gvr:         schema.GroupVersionResource{Group: "extensions", Version: "v1beta1", Resource: "deployments"},
-			existingObj: newPartialObjectMetadata("extensions/v1beta1", "Deployment", "ns-foo", "name-foo"),
+			existingObj: newPartialObjectMetadata("extensions/v1beta1", "Deployment", ns, "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 {
@@ -121,6 +122,7 @@ func TestMetadataSharedInformerFactory(t *testing.T) {
 			}
 			fakeClient := fake.NewSimpleMetadataClient(scheme, objs...)
 			target := xnsinformers.NewMetadataSharedInformerFactory(fakeClient, 0)
+			target.SetNamespaces(ns)
 
 			// act
 			informerListerForGvr := target.ForResource(ts.gvr)

pkg/informers/namespace_set.go

@@ -4,9 +4,10 @@ import (
 	"sort"
 	"sync"
 
-	"github.com/maistra/xns-informer/pkg/internal/sets"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/klog/v2"
+
+	"github.com/maistra/xns-informer/pkg/internal/sets"
 )
 
 // NamespaceSetHandler handles add and remove events for namespace sets.
@@ -39,24 +40,24 @@ func (h NamespaceSetHandlerFuncs) OnRemove(namespace string) {
 // with SetNamespaces, and handlers can be added with AddHandler that will
 // respond to addition or removal of individual namespaces.
 type NamespaceSet interface {
+	// Initialized returns true if SetNamespaces() has been called at least once
+	Initialized() bool
 	SetNamespaces(namespaces ...string)
 	AddHandler(handler NamespaceSetHandler)
 	Contains(namespace string) bool
 	List() []string
 }
 
 type namespaceSet struct {
-	lock       sync.Mutex
-	namespaces sets.Set
-	handlers   []NamespaceSetHandler
+	initialized bool
+	lock        sync.Mutex
+	namespaces  sets.Set
+	handlers    []NamespaceSetHandler
 }
 
-// NewNamespaceSet returns a new NamespaceSet tracking the given namespaces.
-func NewNamespaceSet(namespaces ...string) NamespaceSet {
-	n := &namespaceSet{}
-	n.SetNamespaces(namespaces...)
-
-	return n
+// NewNamespaceSet returns a new NamespaceSet.
+func NewNamespaceSet() NamespaceSet {
+	return &namespaceSet{}
 }
 
 // Contains indicates whether the given namespace is in the set.
@@ -77,11 +78,20 @@ func (n *namespaceSet) List() []string {
 	return namespaces
 }
 
+// Initialized returns true after SetNamespaces is called at least once.
+func (n *namespaceSet) Initialized() bool {
+	return n.initialized
+}
+
 // SetNamespaces replaces the set of namespaces.
 func (n *namespaceSet) SetNamespaces(namespaces ...string) {
 	n.lock.Lock()
 	defer n.lock.Unlock()
 
+	if !n.initialized {
+		n.initialized = true
+	}
+
 	newNamespaceSet := sets.NewSet(namespaces...)
 
 	// If the set of namespaces, includes metav1.NamespaceAll, then it