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 1 commit
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
12 changes: 10 additions & 2 deletions test/e2e/quick_start_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import (
"sigs.k8s.io/cluster-api/test/framework/kubetest"
)

var _ = Describe("When following the Cluster API quick-start", func() {
var _ = Describe("When following the Cluster API quick-start FP", func() {
QuickStartSpec(ctx, func() QuickStartSpecInput {
return QuickStartSpecInput{
E2EConfig: e2eConfig,
Expand All @@ -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,6 +60,7 @@ 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.KubeadmControlPlaneFinalizersAssertion,
Expand All @@ -66,6 +69,7 @@ var _ = Describe("When following the Cluster API quick-start", func() {
)
// 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,6 +108,7 @@ 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.KubeadmControlPlaneFinalizersAssertion,
Expand All @@ -110,6 +117,7 @@ var _ = Describe("When following the Cluster API quick-start with ClusterClass [
)
// 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
7 changes: 5 additions & 2 deletions test/framework/finalizers_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,10 +124,13 @@ func getObjectsWithFinalizers(ctx context.Context, proxy ClusterProxy, namespace
Expect(err).ToNot(HaveOccurred())

setFinalizers := obj.GetFinalizers()
expectedFinalizers := allFinalizerAssertions[node.Object.Kind](types.NamespacedName{Namespace: node.Object.Namespace, Name: node.Object.Name})

if len(setFinalizers) > 0 || len(expectedFinalizers) > 0 {
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
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)
objsWithFinalizers[fmt.Sprintf("%s/%s/%s", node.Object.Kind, node.Object.Namespace, node.Object.Name)] = obj
}
Expand Down
Loading