From da6e1ec6613148a9bc2cd65b9a0a6a4afbff54f3 Mon Sep 17 00:00:00 2001
From: Yang Le <yangle@redhat.com>
Date: Mon, 11 Sep 2023 15:23:07 +0800
Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20fix=20the=20AlreadyExists=20logi?=
 =?UTF-8?q?c=20case=20if=20the=20manifest=20is=20not=20cached=20by=20Gette?=
 =?UTF-8?q?r=20when=20applying=20a=20manifest?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Yang Le <yangle@redhat.com>
---
 pkg/common/apply/interface.go | 23 +++++++++-----
 pkg/common/apply/rbac_test.go | 58 +++++++++++++++++++++++++++--------
 2 files changed, 61 insertions(+), 20 deletions(-)

diff --git a/pkg/common/apply/interface.go b/pkg/common/apply/interface.go
index ba22f0e08..cbe93ca1d 100644
--- a/pkg/common/apply/interface.go
+++ b/pkg/common/apply/interface.go
@@ -19,6 +19,8 @@ type Getter[T runtime.Object] interface {
 
 // Client is a wrapper interface of client
 type Client[T runtime.Object] interface {
+	Get(ctx context.Context, name string, opts metav1.GetOptions) (T, error)
+
 	Create(ctx context.Context, obj T, opts metav1.CreateOptions) (T, error)
 
 	Update(ctx context.Context, obj T, opts metav1.UpdateOptions) (T, error)
@@ -43,20 +45,27 @@ func Apply[T runtime.Object](
 	existing, err := getter.Get(requiredAccessor.GetName())
 	if errors.IsNotFound(err) {
 		actual, createErr := client.Create(ctx, required, metav1.CreateOptions{})
-		if errors.IsAlreadyExists(createErr) {
-			return required, false, nil
-		}
-		if createErr == nil {
+		switch {
+		case errors.IsAlreadyExists(createErr):
+			// This happens when the getter fetches the resource from a cache, like informer cache,
+			// while the resource is filtered by a label/field selector.
+			// Get the resource with the client and update it if it is different from the required.
+			actual, getErr := client.Get(ctx, requiredAccessor.GetName(), metav1.GetOptions{})
+			if getErr != nil {
+				return required, false, getErr
+			}
+			existing = actual
+		case createErr == nil:
 			recorder.Eventf(
 				fmt.Sprintf("%sCreated", gvk.Kind),
 				"Created %s because it was missing", resourcehelper.FormatResourceForCLIWithNamespace(actual))
-		} else {
+			return actual, true, nil
+		default:
 			recorder.Warningf(
 				fmt.Sprintf("%sCreateFailed", gvk.Kind),
 				"Failed to create %s: %v", resourcehelper.FormatResourceForCLIWithNamespace(required), createErr)
+			return actual, true, createErr
 		}
-
-		return actual, true, createErr
 	}
 
 	updated, modified := compare(required, existing)
diff --git a/pkg/common/apply/rbac_test.go b/pkg/common/apply/rbac_test.go
index f8516341a..9cbe41fe3 100644
--- a/pkg/common/apply/rbac_test.go
+++ b/pkg/common/apply/rbac_test.go
@@ -20,7 +20,10 @@ func TestPermissionApply(t *testing.T) {
 		name             string
 		manifest         string
 		existingManifest string
-		validateAction   func(t *testing.T, actions []clienttesting.Action)
+		// filtered indicates if the existing manifest is in the informer cache or not. For example,
+		// it may not match the expected label/field selector of the informer factory.
+		filtered       bool
+		validateAction func(t *testing.T, actions []clienttesting.Action)
 	}{
 		{
 			name: "create clusterrole",
@@ -64,6 +67,33 @@ rules:
 				testingcommon.AssertActions(t, actions, "update")
 			},
 		},
+		{
+			name: "upate clusterrole with no cache",
+			existingManifest: `
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole
+metadata:
+  name: test
+rules:
+- apiGroups: [""]
+  resources: ["configmaps", "events"]
+  verbs: ["get", "list", "watch", "create"] 
+`,
+			manifest: `
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole
+metadata:
+  name: test
+rules:
+- apiGroups: [""]
+  resources: ["configmaps", "events"]
+  verbs: ["get", "list", "watch"] 
+`,
+			filtered: true,
+			validateAction: func(t *testing.T, actions []clienttesting.Action) {
+				testingcommon.AssertActions(t, actions, "create", "get", "update")
+			},
+		},
 		{
 			name: "compare and no update clusterrole",
 			existingManifest: `
@@ -349,18 +379,20 @@ subjects:
 				}
 				kubeClient = kubefake.NewSimpleClientset(o)
 				informerFactory = informers.NewSharedInformerFactory(kubeClient, 3*time.Minute)
-				switch t := o.(type) {
-				case *rbacv1.ClusterRole:
-					err = informerFactory.Rbac().V1().ClusterRoles().Informer().GetStore().Add(t)
-				case *rbacv1.ClusterRoleBinding:
-					err = informerFactory.Rbac().V1().ClusterRoleBindings().Informer().GetStore().Add(t)
-				case *rbacv1.Role:
-					err = informerFactory.Rbac().V1().Roles().Informer().GetStore().Add(t)
-				case *rbacv1.RoleBinding:
-					err = informerFactory.Rbac().V1().RoleBindings().Informer().GetStore().Add(t)
-				}
-				if err != nil {
-					t.Fatal(err)
+				if !c.filtered {
+					switch t := o.(type) {
+					case *rbacv1.ClusterRole:
+						err = informerFactory.Rbac().V1().ClusterRoles().Informer().GetStore().Add(t)
+					case *rbacv1.ClusterRoleBinding:
+						err = informerFactory.Rbac().V1().ClusterRoleBindings().Informer().GetStore().Add(t)
+					case *rbacv1.Role:
+						err = informerFactory.Rbac().V1().Roles().Informer().GetStore().Add(t)
+					case *rbacv1.RoleBinding:
+						err = informerFactory.Rbac().V1().RoleBindings().Informer().GetStore().Add(t)
+					}
+					if err != nil {
+						t.Fatal(err)
+					}
 				}
 			} else {
 				kubeClient = kubefake.NewSimpleClientset()