Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🌱 Improve E2E ValidateFinalizers and ValidateOwnerRef #10693

Merged
merged 4 commits into from
May 31, 2024
Merged

🌱 Improve E2E ValidateFinalizers and ValidateOwnerRef #10693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bb99cd6
Improve E2E ValidateFinalizers and ValidateOwnerRef
fabriziopandini May 28, 2024
f5840e8
Fix nil pointer exception
fabriziopandini May 28, 2024
502e490
Check for missing finalizers
fabriziopandini May 29, 2024
7a4cd28
Fail if finalizer assertions are missing
fabriziopandini May 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions test/e2e/quick_start_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ var _ = Describe("When following the Cluster API quick-start", func() {
InfrastructureProvider: ptr.To("docker"),
PostMachinesProvisioned: func(proxy framework.ClusterProxy, namespace, clusterName string) {
// This check ensures that owner references are resilient - i.e. correctly re-reconciled - when removed.
By("Checking that owner references are resilient")
framework.ValidateOwnerReferencesResilience(ctx, proxy, namespace, clusterName, clusterctlcluster.FilterClusterObjectsWithNameFilter(clusterName),
framework.CoreOwnerReferenceAssertion,
framework.ExpOwnerReferenceAssertions,
Expand All @@ -49,6 +50,7 @@ var _ = Describe("When following the Cluster API quick-start", func() {
framework.KubernetesReferenceAssertions,
)
// This check ensures that owner references are correctly updated to the correct apiVersion.
By("Checking that owner references are updated to the correct API version")
framework.ValidateOwnerReferencesOnUpdate(ctx, proxy, namespace, clusterName, clusterctlcluster.FilterClusterObjectsWithNameFilter(clusterName),
framework.CoreOwnerReferenceAssertion,
framework.ExpOwnerReferenceAssertions,
Expand All @@ -58,14 +60,16 @@ var _ = Describe("When following the Cluster API quick-start", func() {
framework.KubernetesReferenceAssertions,
)
// This check ensures that finalizers are resilient - i.e. correctly re-reconciled - when removed.
By("Checking that finalizers are resilient")
framework.ValidateFinalizersResilience(ctx, proxy, namespace, clusterName, clusterctlcluster.FilterClusterObjectsWithNameFilter(clusterName),
framework.CoreFinalizersAssertion,
framework.CoreFinalizersAssertionWithLegacyClusters,
framework.KubeadmControlPlaneFinalizersAssertion,
framework.ExpFinalizersAssertion,
framework.DockerInfraFinalizersAssertion,
)
// This check ensures that the resourceVersions are stable, i.e. it verifies there are no
// continuous reconciles when everything should be stable.
By("Checking that resourceVersions are stable")
framework.ValidateResourceVersionStable(ctx, proxy, namespace, clusterctlcluster.FilterClusterObjectsWithNameFilter(clusterName))
},
}
Expand All @@ -82,8 +86,9 @@ var _ = Describe("When following the Cluster API quick-start with ClusterClass [
SkipCleanup: skipCleanup,
Flavor: ptr.To("topology"),
InfrastructureProvider: ptr.To("docker"),
// This check ensures that owner references are resilient - i.e. correctly re-reconciled - when removed.
PostMachinesProvisioned: func(proxy framework.ClusterProxy, namespace, clusterName string) {
// This check ensures that owner references are resilient - i.e. correctly re-reconciled - when removed.
By("Checking that owner references are resilient")
framework.ValidateOwnerReferencesResilience(ctx, proxy, namespace, clusterName, clusterctlcluster.FilterClusterObjectsWithNameFilter(clusterName),
framework.CoreOwnerReferenceAssertion,
framework.ExpOwnerReferenceAssertions,
Expand All @@ -93,6 +98,7 @@ var _ = Describe("When following the Cluster API quick-start with ClusterClass [
framework.KubernetesReferenceAssertions,
)
// This check ensures that owner references are correctly updated to the correct apiVersion.
By("Checking that owner references are updated to the correct API version")
framework.ValidateOwnerReferencesOnUpdate(ctx, proxy, namespace, clusterName, clusterctlcluster.FilterClusterObjectsWithNameFilter(clusterName),
framework.CoreOwnerReferenceAssertion,
framework.ExpOwnerReferenceAssertions,
Expand All @@ -102,14 +108,16 @@ var _ = Describe("When following the Cluster API quick-start with ClusterClass [
framework.KubernetesReferenceAssertions,
)
// This check ensures that finalizers are resilient - i.e. correctly re-reconciled - when removed.
By("Checking that finalizers are resilient")
framework.ValidateFinalizersResilience(ctx, proxy, namespace, clusterName, clusterctlcluster.FilterClusterObjectsWithNameFilter(clusterName),
framework.CoreFinalizersAssertion,
framework.CoreFinalizersAssertionWithClassyClusters,
framework.KubeadmControlPlaneFinalizersAssertion,
framework.ExpFinalizersAssertion,
framework.DockerInfraFinalizersAssertion,
)
// This check ensures that the resourceVersions are stable, i.e. it verifies there are no
// continuous reconciles when everything should be stable.
By("Checking that resourceVersions are stable")
framework.ValidateResourceVersionStable(ctx, proxy, namespace, clusterctlcluster.FilterClusterObjectsWithNameFilter(clusterName))
},
}
Expand Down
87 changes: 56 additions & 31 deletions test/framework/finalizers_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,12 @@ import (
"context"
"fmt"
"reflect"
"strings"
"time"

. "github.com/onsi/gomega"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/types"
kerrors "k8s.io/apimachinery/pkg/util/errors"
"sigs.k8s.io/controller-runtime/pkg/client"

Expand All @@ -37,34 +39,43 @@ import (
"sigs.k8s.io/cluster-api/util/patch"
)

// CoreFinalizersAssertion maps Cluster API core types to their expected finalizers.
var CoreFinalizersAssertion = map[string][]string{
"Cluster": {clusterv1.ClusterFinalizer},
"Machine": {clusterv1.MachineFinalizer},
"MachineSet": {clusterv1.MachineSetTopologyFinalizer},
"MachineDeployment": {clusterv1.MachineDeploymentTopologyFinalizer},
// CoreFinalizersAssertionWithLegacyClusters maps Cluster API core types to their expected finalizers for legacy Clusters.
var CoreFinalizersAssertionWithLegacyClusters = map[string]func(types.NamespacedName) []string{
clusterKind: func(_ types.NamespacedName) []string { return []string{clusterv1.ClusterFinalizer} },
machineKind: func(_ types.NamespacedName) []string { return []string{clusterv1.MachineFinalizer} },
}

// CoreFinalizersAssertionWithClassyClusters maps Cluster API core types to their expected finalizers for classy Clusters.
var CoreFinalizersAssertionWithClassyClusters = func() map[string]func(types.NamespacedName) []string {
r := map[string]func(types.NamespacedName) []string{}
for k, v := range CoreFinalizersAssertionWithLegacyClusters {
r[k] = v
}
r[machineSetKind] = func(_ types.NamespacedName) []string { return []string{clusterv1.MachineSetTopologyFinalizer} }
r[machineDeploymentKind] = func(_ types.NamespacedName) []string { return []string{clusterv1.MachineDeploymentTopologyFinalizer} }
return r
}()

// ExpFinalizersAssertion maps experimental resource types to their expected finalizers.
var ExpFinalizersAssertion = map[string][]string{
"ClusterResourceSet": {addonsv1.ClusterResourceSetFinalizer},
"MachinePool": {expv1.MachinePoolFinalizer},
var ExpFinalizersAssertion = map[string]func(types.NamespacedName) []string{
clusterResourceSetKind: func(_ types.NamespacedName) []string { return []string{addonsv1.ClusterResourceSetFinalizer} },
machinePoolKind: func(_ types.NamespacedName) []string { return []string{expv1.MachinePoolFinalizer} },
}

// DockerInfraFinalizersAssertion maps docker infrastructure resource types to their expected finalizers.
var DockerInfraFinalizersAssertion = map[string][]string{
"DockerMachine": {infrav1.MachineFinalizer},
"DockerCluster": {infrav1.ClusterFinalizer},
"DockerMachinePool": {infraexpv1.MachinePoolFinalizer},
var DockerInfraFinalizersAssertion = map[string]func(types.NamespacedName) []string{
dockerMachineKind: func(_ types.NamespacedName) []string { return []string{infrav1.MachineFinalizer} },
dockerClusterKind: func(_ types.NamespacedName) []string { return []string{infrav1.ClusterFinalizer} },
dockerMachinePoolKind: func(_ types.NamespacedName) []string { return []string{infraexpv1.MachinePoolFinalizer} },
}

// KubeadmControlPlaneFinalizersAssertion maps Kubeadm resource types to their expected finalizers.
var KubeadmControlPlaneFinalizersAssertion = map[string][]string{
"KubeadmControlPlane": {controlplanev1.KubeadmControlPlaneFinalizer},
var KubeadmControlPlaneFinalizersAssertion = map[string]func(types.NamespacedName) []string{
kubeadmControlPlaneKind: func(_ types.NamespacedName) []string { return []string{controlplanev1.KubeadmControlPlaneFinalizer} },
}

// ValidateFinalizersResilience checks that expected finalizers are in place, deletes them, and verifies that expected finalizers are properly added again.
func ValidateFinalizersResilience(ctx context.Context, proxy ClusterProxy, namespace, clusterName string, ownerGraphFilterFunction clusterctlcluster.GetOwnerGraphFilterFunction, finalizerAssertions ...map[string][]string) {
func ValidateFinalizersResilience(ctx context.Context, proxy ClusterProxy, namespace, clusterName string, ownerGraphFilterFunction clusterctlcluster.GetOwnerGraphFilterFunction, finalizerAssertions ...map[string]func(name types.NamespacedName) []string) {
clusterKey := client.ObjectKey{Namespace: namespace, Name: clusterName}
allFinalizerAssertions, err := concatenateFinalizerAssertions(finalizerAssertions...)
Expect(err).ToNot(HaveOccurred())
Expand Down Expand Up @@ -107,7 +118,7 @@ func removeFinalizers(ctx context.Context, proxy ClusterProxy, namespace string,
}
}

func getObjectsWithFinalizers(ctx context.Context, proxy ClusterProxy, namespace string, allFinalizerAssertions map[string][]string, ownerGraphFilterFunction clusterctlcluster.GetOwnerGraphFilterFunction) map[string]*unstructured.Unstructured {
func getObjectsWithFinalizers(ctx context.Context, proxy ClusterProxy, namespace string, allFinalizerAssertions map[string]func(name types.NamespacedName) []string, ownerGraphFilterFunction clusterctlcluster.GetOwnerGraphFilterFunction) map[string]*unstructured.Unstructured {
graph, err := clusterctlcluster.GetOwnerGraph(ctx, namespace, proxy.GetKubeconfigPath(), ownerGraphFilterFunction)
Expect(err).ToNot(HaveOccurred())

Expand All @@ -121,11 +132,15 @@ func getObjectsWithFinalizers(ctx context.Context, proxy ClusterProxy, namespace
err = proxy.GetClient().Get(ctx, nodeNamespacedName, obj)
Expect(err).ToNot(HaveOccurred())

// assert if the expected finalizers are set on the resource (including also checking if there are unexpected finalizers)
setFinalizers := obj.GetFinalizers()
var expectedFinalizers []string
if assertion, ok := allFinalizerAssertions[node.Object.Kind]; ok {
expectedFinalizers = assertion(types.NamespacedName{Namespace: node.Object.Namespace, Name: node.Object.Name})
}

Expect(setFinalizers).To(Equal(expectedFinalizers), "for resource type %s", node.Object.Kind)
if len(setFinalizers) > 0 {
Copy link
Member

@sbueringer sbueringer May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'm aware this was the same before)

This only validates finalizers if there are actually finalizers on the object, which means missing finalizers are accepted

Do I understand it correctly that basically we assume the finalizers are correct the first time we call getObjectsWithFinalizers and then in assertFinalizersExist we would detect if a finalizer is missing compared to the first time we retrieved them?

(So tl;dr: if finalizers are set, we verify that they are the expected ones and in addition we test that they are re-added after we remove them ("resilience"))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct.
I have noticed this as well and now it should be fixed (we also check for missing finalizers + we check for additional finalizers that can pop up after we remove them)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx, nice one!

// assert if the expected finalizers are set on the resource
Expect(setFinalizers).To(Equal(allFinalizerAssertions[node.Object.Kind]), "for resource type %s", node.Object.Kind)
objsWithFinalizers[fmt.Sprintf("%s/%s/%s", node.Object.Kind, node.Object.Namespace, node.Object.Name)] = obj
}
}
Expand All @@ -134,24 +149,27 @@ func getObjectsWithFinalizers(ctx context.Context, proxy ClusterProxy, namespace
}

// 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][]string, ownerGraphFilterFunction clusterctlcluster.GetOwnerGraphFilterFunction) {
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",
objKindNamespacedName))
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
expectedFinalizers, assert := allFinalizerAssertions[obj.GetKind()]
if !assert {
continue
}
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 !reflect.DeepEqual(expectedFinalizers, setFinalizers) {
Expand All @@ -160,21 +178,28 @@ func assertFinalizersExist(ctx context.Context, proxy ClusterProxy, namespace st
}
}

// 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())
}

// concatenateFinalizerAssertions concatenates all finalizer assertions into one map. It reports errors if assertions already exist.
func concatenateFinalizerAssertions(finalizerAssertions ...map[string][]string) (map[string][]string, error) {
func concatenateFinalizerAssertions(finalizerAssertions ...map[string]func(name types.NamespacedName) []string) (map[string]func(name types.NamespacedName) []string, error) {
var allErrs []error
allFinalizerAssertions := make(map[string][]string, 0)
allFinalizerAssertions := make(map[string]func(name types.NamespacedName) []string, 0)

for i := range finalizerAssertions {
for kind, finalizers := range finalizerAssertions[i] {
if _, alreadyExists := allFinalizerAssertions[kind]; alreadyExists {
allErrs = append(allErrs, fmt.Errorf("finalizer assertion cannot be applied as it already exists for kind: %s, existing value: %v, new value: %v",
kind, allFinalizerAssertions[kind], finalizers))

allErrs = append(allErrs, fmt.Errorf("finalizer assertion cannot be applied as it already exists for kind: %s", kind))
continue
}

Expand Down
Loading
Loading