🐛 reconciler/cache: remove reflection and fix replicate+NotFound logic

.golangci.yaml

@@ -33,7 +33,6 @@ linters:
   - prealloc
   - revive
   - staticcheck
-  - thelper
   - unconvert
   - unused
   - usestdlibvars

pkg/reconciler/cache/replication/replication_reconcile.go

@@ -19,7 +19,6 @@ package replication
 import (
 	"context"
 	"fmt"
-	"reflect"
 	"strings"
 
 	kcpcache "github.com/kcp-dev/apimachinery/v2/pkg/cache"
@@ -55,7 +54,10 @@ func (c *controller) reconcile(ctx context.Context, gvrKey string) error {
 			},
 			func(cluster logicalcluster.Name, _, name string) (interface{}, bool, error) {
 				obj, err := c.localAPIExportLister.Cluster(cluster).Get(name)
-				return obj, true, err
+				if err != nil {
+					return nil, false, err
+				}
+				return obj, true, nil
 			})
 	case apisv1alpha1.SchemeGroupVersion.WithResource("apiresourceschemas").String():
 		return c.reconcileObject(ctx,
@@ -67,7 +69,10 @@ func (c *controller) reconcile(ctx context.Context, gvrKey string) error {
 			},
 			func(cluster logicalcluster.Name, _, name string) (interface{}, bool, error) {
 				obj, err := c.localAPIResourceSchemaLister.Cluster(cluster).Get(name)
-				return obj, true, err
+				if err != nil {
+					return nil, false, err
+				}
+				return obj, true, nil
 			})
 	case corev1alpha1.SchemeGroupVersion.WithResource("shards").String():
 		return c.reconcileObject(ctx,
@@ -79,7 +84,10 @@ func (c *controller) reconcile(ctx context.Context, gvrKey string) error {
 			},
 			func(cluster logicalcluster.Name, _, name string) (interface{}, bool, error) {
 				obj, err := c.localShardLister.Cluster(cluster).Get(name)
-				return obj, true, err
+				if err != nil {
+					return nil, false, err
+				}
+				return obj, true, nil
 			})
 	case tenancyv1alpha1.SchemeGroupVersion.WithResource("workspacetypes").String():
 		return c.reconcileObject(ctx,
@@ -91,7 +99,10 @@ func (c *controller) reconcile(ctx context.Context, gvrKey string) error {
 			},
 			func(cluster logicalcluster.Name, _, name string) (interface{}, bool, error) {
 				obj, err := c.localWorkspaceTypeLister.Cluster(cluster).Get(name)
-				return obj, true, err
+				if err != nil {
+					return nil, false, err
+				}
+				return obj, true, nil
 			})
 	case rbacv1.SchemeGroupVersion.WithResource("clusterroles").String():
 		return c.reconcileObject(ctx,
@@ -104,9 +115,9 @@ func (c *controller) reconcile(ctx context.Context, gvrKey string) error {
 			func(cluster logicalcluster.Name, _, name string) (interface{}, bool, error) {
 				obj, err := c.localClusterRoleLister.Cluster(cluster).Get(name)
 				if err != nil {
-					return nil, true, err
+					return nil, false, err
 				}
-				return obj, obj.Annotations[core.ReplicateAnnotationKey] != "", err
+				return obj, obj.Annotations[core.ReplicateAnnotationKey] != "", nil
 			})
 	case rbacv1.SchemeGroupVersion.WithResource("clusterrolebindings").String():
 		return c.reconcileObject(ctx,
@@ -119,9 +130,9 @@ func (c *controller) reconcile(ctx context.Context, gvrKey string) error {
 			func(cluster logicalcluster.Name, _, name string) (interface{}, bool, error) {
 				obj, err := c.localClusterRoleBindingLister.Cluster(cluster).Get(name)
 				if err != nil {
-					return nil, true, err
+					return nil, false, err
 				}
-				return obj, obj.Annotations[core.ReplicateAnnotationKey] != "", err
+				return obj, obj.Annotations[core.ReplicateAnnotationKey] != "", nil
 			})
 	default:
 		return fmt.Errorf("unsupported resource %v", keyParts[0])
@@ -149,9 +160,6 @@ func (c *controller) reconcileObject(ctx context.Context,
 	if err != nil && !errors.IsNotFound(err) {
 		return err
 	}
-	if !replicate {
-		return nil
-	}
 	if errors.IsNotFound(err) {
 		// issue a live GET to make sure the localObject was removed
 		_, err = c.dynamicKcpLocalClient.Cluster(cluster.Path()).Resource(gvr).Namespace(namespace).Get(ctx, name, metav1.GetOptions{})
@@ -161,27 +169,29 @@ func (c *controller) reconcileObject(ctx context.Context,
 		if !errors.IsNotFound(err) {
 			return err
 		}
+	} else if !replicate {
+		localObject = nil // like if it wasn't there
 	}
 
 	var unstructuredCacheObject *unstructured.Unstructured
 	var unstructuredLocalObject *unstructured.Unstructured
-	if isNotNil(cacheObject) {
+	if cacheObject != nil {
 		unstructuredCacheObject, err = toUnstructured(cacheObject)
 		if err != nil {
 			return err
 		}
 		unstructuredCacheObject.SetKind(gvk.Kind)
 		unstructuredCacheObject.SetAPIVersion(gvr.GroupVersion().String())
 	}
-	if isNotNil(localObject) {
+	if localObject != nil {
 		unstructuredLocalObject, err = toUnstructured(localObject)
 		if err != nil {
 			return err
 		}
 		unstructuredLocalObject.SetKind(gvk.Kind)
 		unstructuredLocalObject.SetAPIVersion(gvr.GroupVersion().String())
 	}
-	if cluster.Empty() && isNotNil(localObject) {
+	if cluster.Empty() && localObject != nil {
 		metadata, err := meta.Accessor(localObject)
 		if err != nil {
 			return err
@@ -205,7 +215,3 @@ func retrieveCacheObject(gvr *schema.GroupVersionResource, cacheIndex cache.Inde
 	}
 	return cacheObjects[0], nil
 }
-
-func isNotNil(obj interface{}) bool {
-	return obj != nil && (reflect.ValueOf(obj).Kind() == reflect.Ptr && !reflect.ValueOf(obj).IsNil())
-}

pkg/reconciler/cache/replication/replication_reconcile_test.go

@@ -59,8 +59,6 @@ func TestReconcileAPIExports(t *testing.T) {
 			initialLocalAPIExports: []runtime.Object{newAPIExport("foo")},
 			reconcileKey:           fmt.Sprintf("%s::root|foo", apisv1alpha1.SchemeGroupVersion.WithResource("apiexports")),
 			validateFunc: func(t *testing.T, cacheClientActions []kcptesting.Action, localClientActions []kcptesting.Action) {
-				t.Helper()
-
 				if len(localClientActions) != 0 {
 					t.Fatalf("unexpected REST calls were made to the localDynamicClient: %#v", localClientActions)
 				}
@@ -105,8 +103,6 @@ func TestReconcileAPIExports(t *testing.T) {
 			initCacheFakeClientWithInitialAPIExports: true,
 			reconcileKey:                             fmt.Sprintf("%s::root|foo", apisv1alpha1.SchemeGroupVersion.WithResource("apiexports")),
 			validateFunc: func(t *testing.T, cacheClientActions []kcptesting.Action, localClientActions []kcptesting.Action) {
-				t.Helper()
-
 				if len(localClientActions) != 0 {
 					t.Fatalf("unexpected REST calls were made to the localDynamicClient: %#v", localClientActions)
 				}
@@ -135,8 +131,6 @@ func TestReconcileAPIExports(t *testing.T) {
 			initCacheFakeClientWithInitialAPIExports: true,
 			reconcileKey:                             fmt.Sprintf("%s::root|foo", apisv1alpha1.SchemeGroupVersion.WithResource("apiexports")),
 			validateFunc: func(t *testing.T, cacheClientActions []kcptesting.Action, localClientActions []kcptesting.Action) {
-				t.Helper()
-
 				wasGlobalAPIExportDeletionValidated := false
 				wasGlobalAPIExportRetrievalValidated := false
 				for _, action := range localClientActions {
@@ -186,8 +180,6 @@ func TestReconcileAPIExports(t *testing.T) {
 			initCacheFakeClientWithInitialAPIExports: true,
 			reconcileKey:                             fmt.Sprintf("%s::root|foo", apisv1alpha1.SchemeGroupVersion.WithResource("apiexports")),
 			validateFunc: func(t *testing.T, cacheClientActions []kcptesting.Action, localClientActions []kcptesting.Action) {
-				t.Helper()
-
 				if len(localClientActions) != 0 {
 					t.Fatalf("unexpected REST calls were made to the localDynamicClient: %#v", localClientActions)
 				}
@@ -231,8 +223,6 @@ func TestReconcileAPIExports(t *testing.T) {
 			initCacheFakeClientWithInitialAPIExports: true,
 			reconcileKey:                             fmt.Sprintf("%s::root|foo", apisv1alpha1.SchemeGroupVersion.WithResource("apiexports")),
 			validateFunc: func(t *testing.T, cacheClientActions []kcptesting.Action, localClientActions []kcptesting.Action) {
-				t.Helper()
-
 				if len(localClientActions) != 0 {
 					t.Fatalf("unexpected REST calls were made to the localDynamicClient: %#v", localClientActions)
 				}
@@ -277,8 +267,6 @@ func TestReconcileAPIExports(t *testing.T) {
 			initCacheFakeClientWithInitialAPIExports: true,
 			reconcileKey:                             fmt.Sprintf("%s::root|foo", apisv1alpha1.SchemeGroupVersion.WithResource("apiexports")),
 			validateFunc: func(t *testing.T, cacheClientActions []kcptesting.Action, localClientActions []kcptesting.Action) {
-				t.Helper()
-
 				if len(localClientActions) != 0 {
 					t.Fatalf("unexpected REST calls were made to the localDynamicClient: %#v", localClientActions)
 				}

pkg/reconciler/cache/replication/replication_reconcile_unstructured.go

@@ -18,14 +18,14 @@ package replication
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"reflect"
 
 	"github.com/kcp-dev/logicalcluster/v3"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	genericrequest "k8s.io/apiserver/pkg/endpoints/request"
 )
@@ -210,10 +210,12 @@ func ensureRemaining(cacheObject *unstructured.Unstructured, localObject *unstru
 
 func toUnstructured(obj interface{}) (*unstructured.Unstructured, error) {
 	unstructured := &unstructured.Unstructured{Object: map[string]interface{}{}}
-	raw, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj)
+	bs, err := json.Marshal(obj)
 	if err != nil {
 		return nil, err
 	}
-	unstructured.Object = raw
+	if err := json.Unmarshal(bs, &unstructured.Object); err != nil {
+		return nil, err
+	}
 	return unstructured, nil
 }