From 861fb310f83a097e493599e0e6830efc757bd5a6 Mon Sep 17 00:00:00 2001 From: fabriziopandini Date: Mon, 10 Jun 2024 14:12:19 +0200 Subject: [PATCH 1/2] Fix finalizers tests --- test/framework/finalizers_helpers.go | 77 +++++++---------------- test/framework/ownerreference_helpers.go | 8 ++- test/framework/resourceversion_helpers.go | 2 + 3 files changed, 30 insertions(+), 57 deletions(-) diff --git a/test/framework/finalizers_helpers.go b/test/framework/finalizers_helpers.go index cd01f1e4ab3a..946ec36ea3b3 100644 --- a/test/framework/finalizers_helpers.go +++ b/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. diff --git a/test/framework/ownerreference_helpers.go b/test/framework/ownerreference_helpers.go index fae6352bdb52..1665fd03193c 100644 --- a/test/framework/ownerreference_helpers.go +++ b/test/framework/ownerreference_helpers.go @@ -49,6 +49,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 +69,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 +87,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 +104,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...) } @@ -128,7 +134,7 @@ func AssertOwnerReferences(namespace, kubeconfigPath string, ownerGraphFilterFun } 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))) } } } diff --git a/test/framework/resourceversion_helpers.go b/test/framework/resourceversion_helpers.go index a4714a3a3e34..75f1420cbb5b 100644 --- a/test/framework/resourceversion_helpers.go +++ b/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()) From 1ead6101bb65050fe95ed14c0314479bdb826dc6 Mon Sep 17 00:00:00 2001 From: fabriziopandini Date: Mon, 10 Jun 2024 15:43:58 +0200 Subject: [PATCH 2/2] Drop Expect in AssertOwnerReferences --- test/framework/ownerreference_helpers.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/test/framework/ownerreference_helpers.go b/test/framework/ownerreference_helpers.go index 1665fd03193c..e8625849246a 100644 --- a/test/framework/ownerreference_helpers.go +++ b/test/framework/ownerreference_helpers.go @@ -21,7 +21,6 @@ import ( "fmt" "reflect" "sort" - "strings" "time" . "github.com/onsi/gomega" @@ -120,13 +119,10 @@ 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))