Fix ratelimit key (#403)

* use key

* convert back

* improve error strings

* return after forget

* move new funcs into more appropriate file

* fix imports

* changelog

* add tests

* upgrade ubuntu

* update some ci versions

.github/workflows/pull_request.yaml

@@ -9,10 +9,10 @@ on:
 jobs:
   gen:
     name: Code Gen
-    runs-on: ubuntu-18.04
+    runs-on: ubuntu-22.04
     steps:
       - name: Set up Go 1.18
-        uses: actions/setup-go@v1
+        uses: actions/setup-go@v2
         with:
           go-version: 1.18.5
         id: go
@@ -30,23 +30,24 @@ jobs:
           ./ci/check-code-and-docs-gen.sh
   test:
     name: Tests
-    runs-on: ubuntu-18.04
+    runs-on: ubuntu-22.04
     steps:
       - uses: actions/checkout@v2
       - name: Set up Go 1.18
-        uses: actions/setup-go@v1
+        uses: actions/setup-go@v2
         with:
           go-version: 1.18.5
       # Required for codegen tests
       - uses: engineerd/[email protected]
         with:
           skipClusterCreation: "true"
+          version: v0.17.0
       - name: Setup Kind
         run: |
           ./ci/setup-kind.sh skv2-test-remote
       - uses: azure/setup-kubectl@v1
         with:
-          version: 'v1.18.0'
+          version: 'v1.24.7'
       - uses: actions/cache@v1
         with:
           path: ~/go/pkg/mod

changelog/v0.26.1/fix-workqueue-panic.yaml

@@ -0,0 +1,5 @@
+changelog:
+  - type: FIX
+    description: >
+      Convert ResourceIds to a string before adding them to the reconcile queue, to avoid panic caused by ClusterResourceIds being unhashable.
+    issueLink: https://github.com/solo-io/skv2/issues/404

contrib/pkg/input/input_reconciler.go

@@ -8,12 +8,11 @@ import (
 	"time"
 
 	"github.com/rotisserie/eris"
+	"github.com/solo-io/go-utils/contextutils"
 	"github.com/solo-io/skv2/contrib/pkg/sets"
 	"github.com/solo-io/skv2/pkg/ezkube"
-	"k8s.io/client-go/util/workqueue"
-
-	"github.com/solo-io/go-utils/contextutils"
 	"github.com/solo-io/skv2/pkg/reconcile"
+	"k8s.io/client-go/util/workqueue"
 )
 
 // reconcile a resource in a single cluster.
@@ -43,6 +42,8 @@ type inputReconciler struct {
 	singleClusterReconcileFunc SingleClusterReconcileFunc
 }
 
+const keySeparator = "/"
+
 // Note(ilackarms): in the current implementation, the constructor
 // also starts the reconciler's event processor in a goroutine.
 // Make sure to cancel the parent context in order to ensure the goroutine started here is gc'ed.
@@ -83,14 +84,13 @@ func (r *inputReconciler) reconcileGeneric(id ezkube.ResourceId) (reconcile.Resu
 	if r.ctx == nil {
 		return reconcile.Result{}, eris.Errorf("internal error: reconciler not started")
 	}
-	key := sets.TypedKey(id)
 
 	// no need to queue more than one event in reconcile-the-world approach
 	if r.queue.Len() == 0 {
-		contextutils.LoggerFrom(r.ctx).Debugw("adding event to reconciler queue", "id", key)
-		r.queue.AddRateLimited(id)
+		contextutils.LoggerFrom(r.ctx).Debugw("adding event to reconciler queue", "id", sets.TypedKey(id))
+		r.queue.AddRateLimited(ezkube.KeyWithSeparator(id, keySeparator))
 	} else {
-		contextutils.LoggerFrom(r.ctx).Debugw("dropping event as there are are objects in the reconciler's queue", "id", key)
+		contextutils.LoggerFrom(r.ctx).Debugw("dropping event as there are objects in the reconciler's queue", "id", sets.TypedKey(id))
 	}
 	return reconcile.Result{}, nil
 }
@@ -110,31 +110,43 @@ func (r *inputReconciler) reconcileEventsForever(reconcileInterval time.Duration
 
 // processNextWorkItem deals with one key off the queue.  It returns false when it's time to quit.
 func (r *inputReconciler) processNextWorkItem() bool {
-	key, quit := r.queue.Get()
+	queueItem, quit := r.queue.Get()
 	if quit {
 		return false
 	}
-	defer r.queue.Done(key)
+	defer r.queue.Done(queueItem)
+
+	// get the string representation of the queue item
+	key, ok := queueItem.(string)
+	if !ok {
+		contextutils.LoggerFrom(r.ctx).Errorw("got a work queue item of non-string type", "item", queueItem)
+		r.queue.Forget(queueItem)
+		return true
+	}
 
-	var isRemoteCluster bool
+	// convert the key to a ResourceId/ClusterResourceId
+	resource, err := ezkube.ResourceIdFromKeyWithSeparator(key, keySeparator)
+	if err != nil {
+		contextutils.LoggerFrom(r.ctx).Errorw("could not convert work queue item to resource", "error", err)
+		r.queue.Forget(key)
+		return true
+	}
 
 	// determine whether the resource has been read from a remote cluster
 	// based on whether its ClusterName field is set
-	if clusterResource, ok := key.(ezkube.ClusterResourceId); ok {
+	var isRemoteCluster bool
+	if clusterResource, ok := resource.(ezkube.ClusterResourceId); ok {
 		if ezkube.GetClusterName(clusterResource) != "" {
 			isRemoteCluster = true
 		}
 	}
 
-	var (
-		requeue bool
-		err     error
-	)
+	var requeue bool
 	// TODO (ilackarms): this is a workaround for an issue where Relay sets the clusterName field of an object, but the underlying reconciler is a single-cluster reconciler.
 	if isRemoteCluster && r.multiClusterReconcileFunc != nil {
-		requeue, err = r.multiClusterReconcileFunc(key.(ezkube.ClusterResourceId))
+		requeue, err = r.multiClusterReconcileFunc(resource.(ezkube.ClusterResourceId))
 	} else {
-		requeue, err = r.singleClusterReconcileFunc(key.(ezkube.ResourceId))
+		requeue, err = r.singleClusterReconcileFunc(resource)
 	}
 
 	switch {

contrib/pkg/sets/sets.go

@@ -2,7 +2,6 @@ package sets
 
 import (
 	"fmt"
-	"strings"
 	"sync"
 
 	"github.com/rotisserie/eris"
@@ -14,43 +13,11 @@ var NotFoundErr = func(resourceType ezkube.ResourceId, id ezkube.ResourceId) err
 	return eris.Errorf("%T with id %v not found", resourceType, Key(id))
 }
 
-const separator = "."
-
-var builderPool = sync.Pool{
-	New: func() any {
-		return &strings.Builder{}
-	},
-}
+const defaultSeparator = "."
 
 // k8s resources are uniquely identified by their name and namespace
 func Key(id ezkube.ResourceId) string {
-	b := builderPool.Get().(*strings.Builder)
-	defer func() {
-		b.Reset()
-		builderPool.Put(b)
-	}()
-	// When kubernetes objects are passed in here, a call to the GetX() functions will panic, so
-	// this will return "<unknown>" always if the input is nil.
-	if id == nil {
-		return "<unknown>"
-	}
-	b.WriteString(id.GetName())
-	b.WriteString(separator)
-	b.WriteString(id.GetNamespace())
-	b.WriteString(separator)
-	// handle the possibility that clusterName could be set either as an annotation (new way)
-	// or as a field (old way pre-k8s 1.25)
-	if clusterId, ok := id.(ezkube.ClusterResourceId); ok {
-		clusterNameByAnnotation := ezkube.GetClusterName(clusterId)
-		if clusterNameByAnnotation != "" {
-			b.WriteString(clusterNameByAnnotation)
-			return b.String()
-		}
-	}
-	if deprecatedClusterId, ok := id.(interface{ GetClusterName() string }); ok {
-		b.WriteString(deprecatedClusterId.GetClusterName())
-	}
-	return b.String()
+	return ezkube.KeyWithSeparator(id, defaultSeparator)
 }
 
 // typed keys are helpful for logging; currently unused in the Set implementation but placed here for convenience

pkg/ezkube/resource_id.go

@@ -1,11 +1,21 @@
 package ezkube
 
 import (
+	"strings"
+	"sync"
+
+	"github.com/rotisserie/eris"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 const ClusterAnnotation = "cluster.solo.io/cluster"
 
+var builderPool = sync.Pool{
+	New: func() any {
+		return &strings.Builder{}
+	},
+}
+
 // ResourceId represents a global identifier for a k8s resource.
 type ResourceId interface {
 	GetName() string
@@ -19,7 +29,21 @@ type ClusterResourceId interface {
 	GetAnnotations() map[string]string
 }
 
-// internal struct needed to create helper func that converts ref to struct that satisfies ClusterResourceId interface
+// internal struct that satisfies ResourceId interface
+type resourceId struct {
+	name      string
+	namespace string
+}
+
+func (id resourceId) GetName() string {
+	return id.name
+}
+
+func (id resourceId) GetNamespace() string {
+	return id.namespace
+}
+
+// internal struct that satisfies ClusterResourceId interface
 type clusterResourceId struct {
 	name, namespace string
 	annotations     map[string]string
@@ -84,3 +108,57 @@ func SetClusterName(obj client.Object, cluster string) {
 	}
 	obj.GetAnnotations()[ClusterAnnotation] = cluster
 }
+
+// KeyWithSeparator constructs a string consisting of the field values of the given resource id,
+// separated by the given separator. It can be used to construct a unique key for a resource.
+func KeyWithSeparator(id ResourceId, separator string) string {
+	b := builderPool.Get().(*strings.Builder)
+	defer func() {
+		b.Reset()
+		builderPool.Put(b)
+	}()
+	// When kubernetes objects are passed in here, a call to the GetX() functions will panic, so
+	// this will return "<unknown>" always if the input is nil.
+	if id == nil {
+		return "<unknown>"
+	}
+	b.WriteString(id.GetName())
+	b.WriteString(separator)
+	b.WriteString(id.GetNamespace())
+	b.WriteString(separator)
+	// handle the possibility that clusterName could be set either as an annotation (new way)
+	// or as a field (old way pre-k8s 1.25)
+	if clusterId, ok := id.(ClusterResourceId); ok {
+		clusterNameByAnnotation := GetClusterName(clusterId)
+		if clusterNameByAnnotation != "" {
+			b.WriteString(clusterNameByAnnotation)
+			return b.String()
+		}
+	}
+	if deprecatedClusterId, ok := id.(interface{ GetClusterName() string }); ok {
+		b.WriteString(deprecatedClusterId.GetClusterName())
+	}
+	return b.String()
+}
+
+// ResourceIdFromKeyWithSeparator converts a key back into a ResourceId, using the given separator.
+// Returns an error if it cannot be converted.
+func ResourceIdFromKeyWithSeparator(key string, separator string) (ResourceId, error) {
+	parts := strings.Split(key, separator)
+	if len(parts) == 2 {
+		return resourceId{
+			name:      parts[0],
+			namespace: parts[1],
+		}, nil
+	} else if len(parts) == 3 {
+		return clusterResourceId{
+			name:      parts[0],
+			namespace: parts[1],
+			annotations: map[string]string{
+				ClusterAnnotation: parts[2],
+			},
+		}, nil
+	} else {
+		return nil, eris.Errorf("could not convert key %s with separator %s into resource id; unexpected number of parts: %d", key, separator, len(parts))
+	}
+}