Add unit test to check for apply once logic re-entry

Author: g-gaston

exp/addons/internal/controllers/clusterresourceset_controller.go

@@ -298,8 +298,8 @@ func (r *ClusterResourceSetReconciler) ApplyClusterResourceSet(ctx context.Conte
 			errList = append(errList, err)
 		}
 
-		resourceScope, scopeError := reconcileScopeForResource(clusterResourceSet, resource, resourceSetBinding, unstructuredObj)
-		if scopeError == nil && !resourceScope.needsApply() {
+		resourceScope, err := reconcileScopeForResource(clusterResourceSet, resource, resourceSetBinding, unstructuredObj)
+		if err == nil && !resourceScope.needsApply() {
 			continue
 		}
 
@@ -312,8 +312,8 @@ func (r *ClusterResourceSetReconciler) ApplyClusterResourceSet(ctx context.Conte
 			LastAppliedTime: &metav1.Time{Time: time.Now().UTC()},
 		})
 
-		if scopeError != nil {
-			errList = append(errList, scopeError)
+		if err != nil {
+			errList = append(errList, err)
 			continue
 		}
 

exp/addons/internal/controllers/clusterresourceset_helpers.go

@@ -34,6 +34,7 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/types"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
+	"k8s.io/klog/v2"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
@@ -112,7 +113,7 @@ func createUnstructured(ctx context.Context, c client.Client, obj *unstructured.
 			err,
 			"creating object %s %s",
 			obj.GroupVersionKind(),
-			klog.Obj(obj),
+			klog.KObj(obj),
 		)
 	}
 

exp/addons/internal/controllers/clusterresourceset_scope.go

@@ -22,6 +22,7 @@ import (
 	"github.com/pkg/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/klog/v2"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	addonsv1 "sigs.k8s.io/cluster-api/exp/addons/api/v1beta1"
@@ -128,7 +129,7 @@ func (r *reconcileStrategyScope) apply(ctx context.Context, c client.Client, obj
 			err,
 			"reading object %s %s",
 			obj.GroupVersionKind(),
-			klog.Obj(obj),
+			klog.KObj(obj),
 		)
 	}
 

exp/addons/internal/controllers/clusterresourceset_scope_test.go

@@ -17,11 +17,16 @@ limitations under the License.
 package controllers
 
 import (
+	"context"
 	"testing"
 
 	. "github.com/onsi/gomega"
-
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	addonsv1 "sigs.k8s.io/cluster-api/exp/addons/api/v1beta1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 )
 
 func TestReconcileStrategyScopeNeedsApply(t *testing.T) {
@@ -185,3 +190,61 @@ func TestReconcileApplyOnceScopeNeedsApply(t *testing.T) {
 		})
 	}
 }
+
+func TestReconcileApplyOnceScopeApply(t *testing.T) {
+	tests := []struct {
+		name         string
+		existingObjs []client.Object
+		obj          *unstructured.Unstructured
+		wantErr      string
+	}{
+		{
+			name: "object doesn't exist",
+			obj: &unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "v1",
+					"kind":       "ConfigMap",
+					"metadata": map[string]interface{}{
+						"name":      "my-cm",
+						"namespace": "that-ns",
+					},
+				},
+			},
+		},
+		{
+			name: "object exists",
+			existingObjs: []client.Object{
+				&corev1.ConfigMap{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "my-cm",
+						Namespace: "that-ns",
+					},
+				},
+			},
+			obj: &unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "v1",
+					"kind":       "ConfigMap",
+					"metadata": map[string]interface{}{
+						"name":      "my-cm",
+						"namespace": "that-ns",
+					},
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gs := NewWithT(t)
+			ctx := context.Background()
+			client := fake.NewClientBuilder().WithObjects(tt.existingObjs...).Build()
+			scope := &reconcileApplyOnceScope{}
+			err := scope.apply(ctx, client, tt.obj)
+			if tt.wantErr == "" {
+				gs.Expect(err).NotTo(HaveOccurred())
+			} else {
+				gs.Expect(err).To(MatchError(ContainSubstring(tt.wantErr)))
+			}
+		})
+	}
+}