fix: improve object cache reliability

* make sure objects expire after DefaultTTLSeconds
* make cached objects have GKV information
* fix cache retrieval and removal logic

Closes #502

Signed-off-by: Marco Nenciarini <[email protected]>

Author: mnencia

internal/cnpgi/instance/internal/client/client.go

@@ -21,11 +21,11 @@ const DefaultTTLSeconds = 10
 type cachedEntry struct {
 	entry         client.Object
 	fetchUnixTime int64
-	ttl           time.Duration
+	ttlSeconds    int64
 }
 
 func (e *cachedEntry) isExpired() bool {
-	return time.Now().Unix()-e.fetchUnixTime > int64(e.ttl)
+	return time.Now().Unix()-e.fetchUnixTime > e.ttlSeconds
 }
 
 // ExtendedClient is an extended client that is capable of caching multiple secrets without relying on informers
@@ -71,6 +71,28 @@ func (e *ExtendedClient) Get(
 	return e.Client.Get(ctx, key, obj, opts...)
 }
 
+// addTypeInformationToObject adds TypeMeta information to a client.Object based upon the client Scheme
+// inspired by: https://github.com/kubernetes/cli-runtime/blob/v0.19.2/pkg/printers/typesetter.go#L41
+func (e *ExtendedClient) addTypeInformationToObject(obj client.Object) error {
+	gvks, _, err := e.Client.Scheme().ObjectKinds(obj)
+	if err != nil {
+		return fmt.Errorf("missing apiVersion or kind and cannot assign it; %w", err)
+	}
+
+	for _, gvk := range gvks {
+		if len(gvk.Kind) == 0 {
+			continue
+		}
+		if len(gvk.Version) == 0 || gvk.Version == runtime.APIVersionInternal {
+			continue
+		}
+		obj.GetObjectKind().SetGroupVersionKind(gvk)
+		break
+	}
+
+	return nil
+}
+
 func (e *ExtendedClient) getCachedObject(
 	ctx context.Context,
 	key client.ObjectKey,
@@ -81,6 +103,12 @@ func (e *ExtendedClient) getCachedObject(
 		WithName("extended_client").
 		WithValues("name", key.Name, "namespace", key.Namespace)
 
+	// Make sure the object has GVK information
+	// This is needed to compare the object type with the cached one
+	if err := e.addTypeInformationToObject(obj); err != nil {
+		return fmt.Errorf("cannot add type metadata to object of type %T: %w", obj, err)
+	}
+
 	contextLogger.Trace("locking the cache")
 	e.mux.Lock()
 	defer e.mux.Unlock()
@@ -120,9 +148,16 @@ func (e *ExtendedClient) getCachedObject(
 		return err
 	}
 
+	// Populate the GKV information again, as the client.Get() may have
+	// returned an object without this information set
+	if err := e.addTypeInformationToObject(obj); err != nil {
+		return fmt.Errorf("cannot add type metadata to object of type %T: %w", obj, err)
+	}
+
 	cs := cachedEntry{
-		entry:         obj.(runtime.Object).DeepCopyObject().(client.Object),
+		entry:         obj.DeepCopyObject().(client.Object),
 		fetchUnixTime: time.Now().Unix(),
+		ttlSeconds:    DefaultTTLSeconds,
 	}
 
 	contextLogger.Debug("setting object in the cache")
@@ -143,7 +178,7 @@ func (e *ExtendedClient) removeObject(object client.Object) {
 	for i, cache := range e.cachedObjects {
 		if cache.entry.GetNamespace() == object.GetNamespace() &&
 			cache.entry.GetName() == object.GetName() &&
-			cache.entry.GetObjectKind().GroupVersionKind() != object.GetObjectKind().GroupVersionKind() {
+			cache.entry.GetObjectKind().GroupVersionKind() == object.GetObjectKind().GroupVersionKind() {
 			e.cachedObjects = append(e.cachedObjects[:i], e.cachedObjects[i+1:]...)
 			return
 		}

internal/cnpgi/instance/internal/client/client_test.go

@@ -9,7 +9,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
-	v1 "github.com/cloudnative-pg/plugin-barman-cloud/api/v1"
+	barmancloudv1 "github.com/cloudnative-pg/plugin-barman-cloud/api/v1"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -20,16 +20,28 @@ var scheme = buildScheme()
 func buildScheme() *runtime.Scheme {
 	scheme := runtime.NewScheme()
 	_ = corev1.AddToScheme(scheme)
-	_ = v1.AddToScheme(scheme)
+	_ = barmancloudv1.AddToScheme(scheme)
 
 	return scheme
 }
 
+func addToCache(c *ExtendedClient, obj client.Object, fetchUnixTime int64) {
+	ce := cachedEntry{
+		entry:         obj.DeepCopyObject().(client.Object),
+		fetchUnixTime: fetchUnixTime,
+		ttlSeconds:    DefaultTTLSeconds,
+	}
+	ce.entry.SetResourceVersion("from cache")
+	err := c.addTypeInformationToObject(ce.entry)
+	Expect(err).ToNot(HaveOccurred())
+	c.cachedObjects = append(c.cachedObjects, ce)
+}
+
 var _ = Describe("ExtendedClient Get", func() {
 	var (
 		extendedClient *ExtendedClient
 		secretInClient *corev1.Secret
-		objectStore    *v1.ObjectStore
+		objectStore    *barmancloudv1.ObjectStore
 	)
 
 	BeforeEach(func() {
@@ -39,12 +51,12 @@ var _ = Describe("ExtendedClient Get", func() {
 				Name:      "test-secret",
 			},
 		}
-		objectStore = &v1.ObjectStore{
+		objectStore = &barmancloudv1.ObjectStore{
 			ObjectMeta: metav1.ObjectMeta{
 				Namespace: "default",
 				Name:      "test-object-store",
 			},
-			Spec: v1.ObjectStoreSpec{},
+			Spec: barmancloudv1.ObjectStoreSpec{},
 		}
 
 		baseClient := fake.NewClientBuilder().
@@ -61,35 +73,34 @@ var _ = Describe("ExtendedClient Get", func() {
 			},
 		}
 
-		// manually add the secret to the cache, this is not present in the fake client so we are sure it is from the
-		// cache
-		extendedClient.cachedObjects = []cachedEntry{
-			{
-				entry:         secretNotInClient,
-				fetchUnixTime: time.Now().Unix(),
-			},
-		}
+		// manually add the secret to the cache, this is not present in the fake client,
+		// so we are sure it is from the cache
+		addToCache(extendedClient, secretNotInClient, time.Now().Unix())
 
 		err := extendedClient.Get(ctx, client.ObjectKeyFromObject(secretNotInClient), secretInClient)
 		Expect(err).NotTo(HaveOccurred())
-		Expect(secretNotInClient).To(Equal(extendedClient.cachedObjects[0].entry))
+		Expect(secretInClient).To(Equal(extendedClient.cachedObjects[0].entry))
+		Expect(secretInClient.GetResourceVersion()).To(Equal("from cache"))
 	})
 
 	It("fetches secret from base client if cache is expired", func(ctx SpecContext) {
-		extendedClient.cachedObjects = []cachedEntry{
-			{
-				entry:         secretInClient.DeepCopy(),
-				fetchUnixTime: time.Now().Add(-2 * time.Minute).Unix(),
-			},
-		}
+		addToCache(extendedClient, secretInClient, time.Now().Add(-2*time.Minute).Unix())
 
 		err := extendedClient.Get(ctx, client.ObjectKeyFromObject(secretInClient), secretInClient)
 		Expect(err).NotTo(HaveOccurred())
+		Expect(secretInClient.GetResourceVersion()).NotTo(Equal("from cache"))
+
+		// the cache is updated with the new value
+		Expect(extendedClient.cachedObjects).To(HaveLen(1))
+		Expect(extendedClient.cachedObjects[0].entry.GetResourceVersion()).NotTo(Equal("from cache"))
 	})
 
 	It("fetches secret from base client if not in cache", func(ctx SpecContext) {
 		err := extendedClient.Get(ctx, client.ObjectKeyFromObject(secretInClient), secretInClient)
 		Expect(err).NotTo(HaveOccurred())
+
+		// the cache is updated with the new value
+		Expect(extendedClient.cachedObjects).To(HaveLen(1))
 	})
 
 	It("does not cache non-secret objects", func(ctx SpecContext) {