🌱 Fix finalizers assertions (#10735)

* Fix finalizers tests

* Drop Expect in AssertOwnerReferences

test/framework/finalizers_helpers.go

@@ -19,10 +19,10 @@ package framework
 import (
 	"context"
 	"fmt"
-	"strings"
 	"time"
 
 	. "github.com/onsi/gomega"
+	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/types"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
@@ -82,8 +82,11 @@ func ValidateFinalizersResilience(ctx context.Context, proxy ClusterProxy, names
 	Expect(err).ToNot(HaveOccurred())
 
 	// Collect all objects where finalizers were initially set
-	objectsWithFinalizers := getObjectsWithFinalizers(ctx, proxy, namespace, allFinalizerAssertions, ownerGraphFilterFunction)
+	byf("Check that the finalizers are as expected")
+	err = checkObjectsWithFinalizers(ctx, proxy, namespace, allFinalizerAssertions, ownerGraphFilterFunction)
+	Expect(err).ToNot(HaveOccurred(), "Finalizers are not as expected")
 
+	byf("Removing all the finalizers")
 	// Setting the paused property on the Cluster resource will pause reconciliations, thereby having no effect on Finalizers.
 	// This also makes debugging easier.
 	setClusterPause(ctx, proxy.GetClient(), clusterKey, true)
@@ -97,7 +100,10 @@ func ValidateFinalizersResilience(ctx context.Context, proxy ClusterProxy, names
 	setClusterPause(ctx, proxy.GetClient(), clusterKey, false)
 
 	// Check that the Finalizers are as expected after further reconciliations.
-	assertFinalizersExist(ctx, proxy, namespace, objectsWithFinalizers, allFinalizerAssertions, ownerGraphFilterFunction)
+	byf("Check that the finalizers are rebuilt as expected")
+	Eventually(func() error {
+		return checkObjectsWithFinalizers(ctx, proxy, namespace, allFinalizerAssertions, ownerGraphFilterFunction)
+	}).WithTimeout(1*time.Minute).WithPolling(2*time.Second).Should(Succeed(), "Finalizers are not rebuilt as expected")
 }
 
 // removeFinalizers removes all Finalizers from objects in the owner graph.
@@ -119,19 +125,22 @@ func removeFinalizers(ctx context.Context, proxy ClusterProxy, namespace string,
 	}
 }
 
-func getObjectsWithFinalizers(ctx context.Context, proxy ClusterProxy, namespace string, allFinalizerAssertions map[string]func(name types.NamespacedName) []string, ownerGraphFilterFunction clusterctlcluster.GetOwnerGraphFilterFunction) map[string]*unstructured.Unstructured {
+func checkObjectsWithFinalizers(ctx context.Context, proxy ClusterProxy, namespace string, allFinalizerAssertions map[string]func(name types.NamespacedName) []string, ownerGraphFilterFunction clusterctlcluster.GetOwnerGraphFilterFunction) error {
 	graph, err := clusterctlcluster.GetOwnerGraph(ctx, namespace, proxy.GetKubeconfigPath(), ownerGraphFilterFunction)
-	Expect(err).ToNot(HaveOccurred())
-
-	objsWithFinalizers := map[string]*unstructured.Unstructured{}
+	if err != nil {
+		return err
+	}
 
+	var allErrs []error
 	for _, node := range graph {
 		nodeNamespacedName := client.ObjectKey{Namespace: node.Object.Namespace, Name: node.Object.Name}
 		obj := &unstructured.Unstructured{}
 		obj.SetAPIVersion(node.Object.APIVersion)
 		obj.SetKind(node.Object.Kind)
 		err = proxy.GetClient().Get(ctx, nodeNamespacedName, obj)
-		Expect(err).ToNot(HaveOccurred())
+		if err != nil {
+			return errors.Wrapf(err, "failed to get object %s, %s", node.Object.Kind, klog.KRef(node.Object.Namespace, node.Object.Name))
+		}
 
 		// assert if the expected finalizers are set on the resource (including also checking if there are unexpected finalizers)
 		setFinalizers := obj.GetFinalizers()
@@ -140,56 +149,12 @@ func getObjectsWithFinalizers(ctx context.Context, proxy ClusterProxy, namespace
 			expectedFinalizers = assertion(types.NamespacedName{Namespace: node.Object.Namespace, Name: node.Object.Name})
 		}
 
-		Expect(sets.NewString(setFinalizers...)).To(Equal(sets.NewString(expectedFinalizers...)), "for resource type %s, %s", node.Object.Kind, klog.KRef(node.Object.Namespace, node.Object.Name))
-		if len(setFinalizers) > 0 {
-			objsWithFinalizers[fmt.Sprintf("%s/%s/%s", node.Object.Kind, node.Object.Namespace, node.Object.Name)] = obj
+		if !sets.NewString(setFinalizers...).Equal(sets.NewString(expectedFinalizers...)) {
+			allErrs = append(allErrs, fmt.Errorf("unexpected finalizers for %s, %s: expected: %v, found: %v",
+				node.Object.Kind, klog.KRef(node.Object.Namespace, node.Object.Name), expectedFinalizers, setFinalizers))
 		}
 	}
-
-	return objsWithFinalizers
-}
-
-// assertFinalizersExist ensures that current Finalizers match those in the initialObjectsWithFinalizers.
-func assertFinalizersExist(ctx context.Context, proxy ClusterProxy, namespace string, initialObjsWithFinalizers map[string]*unstructured.Unstructured, allFinalizerAssertions map[string]func(name types.NamespacedName) []string, ownerGraphFilterFunction clusterctlcluster.GetOwnerGraphFilterFunction) {
-	Eventually(func() error {
-		var allErrs []error
-		finalObjsWithFinalizers := getObjectsWithFinalizers(ctx, proxy, namespace, allFinalizerAssertions, ownerGraphFilterFunction)
-
-		// Check if all the initial objects with finalizers have them back.
-		for objKindNamespacedName, obj := range initialObjsWithFinalizers {
-			// verify if finalizers for this resource were set on reconcile
-			if _, valid := finalObjsWithFinalizers[objKindNamespacedName]; !valid {
-				allErrs = append(allErrs, fmt.Errorf("no finalizers set for %s, at the beginning of the test it has %s",
-					objKindNamespacedName, obj.GetFinalizers()))
-				continue
-			}
-
-			// verify if this resource has the appropriate Finalizers set
-			expectedFinalizersF, assert := allFinalizerAssertions[obj.GetKind()]
-			// NOTE: this case should never happen because all the initialObjsWithFinalizers have been already checked
-			// against a finalizer assertion.
-			Expect(assert).To(BeTrue(), "finalizer assertions for %s are missing", objKindNamespacedName)
-			parts := strings.Split(objKindNamespacedName, "/")
-			expectedFinalizers := expectedFinalizersF(types.NamespacedName{Namespace: parts[1], Name: parts[2]})
-
-			setFinalizers := finalObjsWithFinalizers[objKindNamespacedName].GetFinalizers()
-			if !sets.NewString(setFinalizers...).Equal(sets.NewString(expectedFinalizers...)) {
-				allErrs = append(allErrs, fmt.Errorf("expected finalizers do not exist for %s: expected: %v, found: %v",
-					objKindNamespacedName, expectedFinalizers, setFinalizers))
-			}
-		}
-
-		// Check if there are objects with finalizers not existing initially
-		for objKindNamespacedName, obj := range finalObjsWithFinalizers {
-			// verify if finalizers for this resource were set on reconcile
-			if _, valid := initialObjsWithFinalizers[objKindNamespacedName]; !valid {
-				allErrs = append(allErrs, fmt.Errorf("%s has finalizers not existing at the beginning of the test: %s",
-					objKindNamespacedName, obj.GetFinalizers()))
-			}
-		}
-
-		return kerrors.NewAggregate(allErrs)
-	}).WithTimeout(1 * time.Minute).WithPolling(2 * time.Second).Should(Succeed())
+	return kerrors.NewAggregate(allErrs)
 }
 
 // concatenateFinalizerAssertions concatenates all finalizer assertions into one map. It reports errors if assertions already exist.

test/framework/ownerreference_helpers.go

@@ -21,7 +21,6 @@ import (
 	"fmt"
 	"reflect"
 	"sort"
-	"strings"
 	"time"
 
 	. "github.com/onsi/gomega"
@@ -49,6 +48,8 @@ import (
 func ValidateOwnerReferencesOnUpdate(ctx context.Context, proxy ClusterProxy, namespace, clusterName string, ownerGraphFilterFunction clusterctlcluster.GetOwnerGraphFilterFunction, assertFuncs ...map[string]func(obj types.NamespacedName, reference []metav1.OwnerReference) error) {
 	clusterKey := client.ObjectKey{Namespace: namespace, Name: clusterName}
 
+	byf("Changing all the ownerReferences to a different API version")
+
 	// Pause the cluster.
 	setClusterPause(ctx, proxy.GetClient(), clusterKey, true)
 
@@ -67,12 +68,14 @@ func ValidateOwnerReferencesOnUpdate(ctx context.Context, proxy ClusterProxy, na
 	forceClusterResourceSetReconcile(ctx, proxy.GetClient(), namespace)
 
 	// Check that the ownerReferences have updated their apiVersions to current versions after reconciliation.
+	byf("Check that the ownerReferences are rebuilt as expected")
 	AssertOwnerReferences(namespace, proxy.GetKubeconfigPath(), ownerGraphFilterFunction, assertFuncs...)
 }
 
 // ValidateOwnerReferencesResilience checks that expected owner references are in place, deletes them, and verifies that expect owner references are properly rebuilt.
 func ValidateOwnerReferencesResilience(ctx context.Context, proxy ClusterProxy, namespace, clusterName string, ownerGraphFilterFunction clusterctlcluster.GetOwnerGraphFilterFunction, assertFuncs ...map[string]func(obj types.NamespacedName, reference []metav1.OwnerReference) error) {
 	// Check that the ownerReferences are as expected on the first iteration.
+	byf("Check that the ownerReferences are as expected")
 	AssertOwnerReferences(namespace, proxy.GetKubeconfigPath(), ownerGraphFilterFunction, assertFuncs...)
 
 	clusterKey := client.ObjectKey{Namespace: namespace, Name: clusterName}
@@ -83,6 +86,7 @@ func ValidateOwnerReferencesResilience(ctx context.Context, proxy ClusterProxy,
 	// edge case where an external system intentionally nukes all the owner references in a single operation.
 	// The assumption is that if the system can recover from this edge case, it can also handle use cases where an owner
 	// reference is deleted by mistake.
+	byf("Removing all the ownerReferences")
 
 	// Setting the paused property on the Cluster resource will pause reconciliations, thereby having no effect on OwnerReferences.
 	// This also makes debugging easier.
@@ -99,6 +103,7 @@ func ValidateOwnerReferencesResilience(ctx context.Context, proxy ClusterProxy,
 	forceClusterClassReconcile(ctx, proxy.GetClient(), clusterKey)
 
 	// Check that the ownerReferences are as expected after additional reconciliations.
+	byf("Check that the ownerReferences are rebuilt as expected")
 	AssertOwnerReferences(namespace, proxy.GetKubeconfigPath(), ownerGraphFilterFunction, assertFuncs...)
 }
 
@@ -114,21 +119,18 @@ func AssertOwnerReferences(namespace, kubeconfigPath string, ownerGraphFilterFun
 		ctx := context.Background()
 
 		graph, err := clusterctlcluster.GetOwnerGraph(ctx, namespace, kubeconfigPath, ownerGraphFilterFunction)
-		// Sometimes the conversion-webhooks are not ready yet / cert-managers ca-injector
-		// may not yet have injected the new ca bundle after the upgrade.
-		// If this is the case we return an error to retry.
-		if err != nil && strings.Contains(err.Error(), "x509: certificate signed by unknown authority") {
+		if err != nil {
 			return err
 		}
-		Expect(err).ToNot(HaveOccurred())
+
 		for _, v := range graph {
 			if _, ok := allAssertFuncs[v.Object.Kind]; !ok {
 				allErrs = append(allErrs, fmt.Errorf("kind %s does not have an associated ownerRef assertion function", v.Object.Kind))
 				continue
 			}
 			for _, f := range allAssertFuncs[v.Object.Kind] {
 				if err := f(types.NamespacedName{Namespace: v.Object.Namespace, Name: v.Object.Name}, v.Owners); err != nil {
-					allErrs = append(allErrs, errors.Wrapf(err, "Unexpected ownerReferences for %s, %s", v.Object.Kind, klog.KRef(v.Object.Namespace, v.Object.Name)))
+					allErrs = append(allErrs, errors.Wrapf(err, "unexpected ownerReferences for %s, %s", v.Object.Kind, klog.KRef(v.Object.Namespace, v.Object.Name)))
 				}
 			}
 		}

test/framework/resourceversion_helpers.go

@@ -31,6 +31,7 @@ import (
 // ValidateResourceVersionStable checks that resource versions are stable.
 func ValidateResourceVersionStable(ctx context.Context, proxy ClusterProxy, namespace string, ownerGraphFilterFunction clusterctlcluster.GetOwnerGraphFilterFunction) {
 	// Wait until resource versions are stable for a bit.
+	byf("Check Resource versions are stable")
 	var previousResourceVersions map[string]string
 	Eventually(func(g Gomega) {
 		objectsWithResourceVersion, err := getObjectsWithResourceVersion(ctx, proxy, namespace, ownerGraphFilterFunction)
@@ -45,6 +46,7 @@ func ValidateResourceVersionStable(ctx context.Context, proxy ClusterProxy, name
 	}, 1*time.Minute, 15*time.Second).Should(Succeed(), "Resource versions never became stable")
 
 	// Verify resource versions are stable for a while.
+	byf("Check Resource versions remains stable")
 	Consistently(func(g Gomega) {
 		objectsWithResourceVersion, err := getObjectsWithResourceVersion(ctx, proxy, namespace, ownerGraphFilterFunction)
 		g.Expect(err).ToNot(HaveOccurred())