🐛 fix the AlreadyExists logic case if the manifest is not cached by G…

…etter when applying a manifest Signed-off-by: Yang Le <yangle@redhat.com>
open-cluster-management-io · Sep 11, 2023 · 83620ba · 83620ba
1 parent 3fc013f
commit 83620ba
Show file tree

Hide file tree

Showing 2 changed files with 60 additions and 22 deletions.
diff --git 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,24 @@ 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):
+			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
@@ -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: `
@@ -348,23 +378,25 @@ subjects:
 					t.Fatal(err)
 				}
 				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)
+				informerFactory = informers.NewSharedInformerFactoryWithOptions(kubeClient, 3*time.Minute)
+				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()
-				informerFactory = informers.NewSharedInformerFactory(kubeClient, 3*time.Minute)
+				informerFactory = informers.NewSharedInformerFactoryWithOptions(kubeClient, 3*time.Minute)
 			}
 
 			applier := NewPermissionApplier(