Skip to content

Commit

Permalink
🌱 Improve E2E ValidateFinalizers and ValidateOwnerRef (kubernetes-sig…
Browse files Browse the repository at this point in the history 
…s#10693)

* Improve E2E ValidateFinalizers and ValidateOwnerRef

* Fix nil pointer exception

* Check for missing finalizers

* Fail if finalizer assertions are missing
  • Loading branch information
@fabriziopandini
fabriziopandini authored May 31, 2024
1 parent 32323cf commit ebdda34
Show file tree
Hide file tree
Showing 3 changed files with 100 additions and 67 deletions.
14 changes: 11 additions & 3 deletions test/e2e/quick_start_test.go
View file
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
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 {
// 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

0 comments on commit ebdda34

Please sign in to comment.