From 6a6904792ff3cae116f0e42d12dddd905adc046e Mon Sep 17 00:00:00 2001 From: fabriziopandini Date: Mon, 21 Jul 2025 15:19:35 +0200 Subject: [PATCH 1/3] Fix lifecycle hooks conversions --- exp/topology/desiredstate/desired_state.go | 4 +- .../desiredstate/desired_state_test.go | 100 +++++++++++++----- .../topology/cluster/cluster_controller.go | 4 +- .../topology/cluster/reconcile_state.go | 4 +- .../topology/cluster/reconcile_state_test.go | 59 +++++++++-- 5 files changed, 133 insertions(+), 38 deletions(-) diff --git a/exp/topology/desiredstate/desired_state.go b/exp/topology/desiredstate/desired_state.go index d07b18f6da50..8100849c08f4 100644 --- a/exp/topology/desiredstate/desired_state.go +++ b/exp/topology/desiredstate/desired_state.go @@ -532,7 +532,7 @@ func (g *generator) computeControlPlaneVersion(ctx context.Context, s *scope.Sco // hook because we didn't go through an upgrade or we already called the hook after the upgrade. if hooks.IsPending(runtimehooksv1.AfterControlPlaneUpgrade, s.Current.Cluster) { v1beta1Cluster := &clusterv1beta1.Cluster{} - if err := clusterv1beta1.Convert_v1beta2_Cluster_To_v1beta1_Cluster(s.Current.Cluster, v1beta1Cluster, nil); err != nil { + if err := v1beta1Cluster.ConvertFrom(s.Current.Cluster); err != nil { return "", errors.Wrap(err, "error converting Cluster to v1beta1 Cluster") } @@ -606,7 +606,7 @@ func (g *generator) computeControlPlaneVersion(ctx context.Context, s *scope.Sco // At this point the control plane and the machine deployments are stable and we are almost ready to pick // up the desiredVersion. Call the BeforeClusterUpgrade hook before picking up the desired version. v1beta1Cluster := &clusterv1beta1.Cluster{} - if err := clusterv1beta1.Convert_v1beta2_Cluster_To_v1beta1_Cluster(s.Current.Cluster, v1beta1Cluster, nil); err != nil { + if err := v1beta1Cluster.ConvertFrom(s.Current.Cluster); err != nil { return "", errors.Wrap(err, "error converting Cluster to v1beta1 Cluster") } diff --git a/exp/topology/desiredstate/desired_state_test.go b/exp/topology/desiredstate/desired_state_test.go index 991523b57f50..1d16c9fef8c6 100644 --- a/exp/topology/desiredstate/desired_state_test.go +++ b/exp/topology/desiredstate/desired_state_test.go @@ -18,19 +18,22 @@ package desiredstate import ( "encoding/json" - "errors" "fmt" + "maps" "strings" "testing" "time" "github.com/google/go-cmp/cmp" . "github.com/onsi/gomega" + "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + apiequality "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/intstr" clientgoscheme "k8s.io/client-go/kubernetes/scheme" utilfeature "k8s.io/component-base/featuregate/testing" @@ -962,6 +965,27 @@ func TestComputeControlPlane(t *testing.T) { } func TestComputeControlPlaneVersion(t *testing.T) { + var testGVKs = []schema.GroupVersionKind{ + { + Group: "refAPIGroup1", + Kind: "refKind1", + Version: "v1beta4", + }, + } + + apiVersionGetter := func(gk schema.GroupKind) (string, error) { + for _, gvk := range testGVKs { + if gvk.GroupKind() == gk { + return schema.GroupVersion{ + Group: gk.Group, + Version: gvk.Version, + }.String(), nil + } + } + return "", fmt.Errorf("unknown GroupVersionKind: %v", gk) + } + clusterv1beta1.SetAPIVersionGetter(apiVersionGetter) + t.Run("Compute control plane version under various circumstances", func(t *testing.T) { utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.RuntimeSDK, true) @@ -1202,10 +1226,13 @@ func TestComputeControlPlaneVersion(t *testing.T) { FieldsV1: &metav1.FieldsV1{}, }, }, - Annotations: map[string]string{ - "fizz": "buzz", - corev1.LastAppliedConfigAnnotation: "should be cleaned up", - conversion.DataAnnotation: "should be cleaned up", + }, + // Add some more fields to check that conversion implemented when calling RuntimeExtension are properly handled. + Spec: clusterv1.ClusterSpec{ + InfrastructureRef: &clusterv1.ContractVersionedObjectReference{ + APIGroup: "refAPIGroup1", + Kind: "refKind1", + Name: "refName1", }, }, }, @@ -1229,7 +1256,7 @@ func TestComputeControlPlaneVersion(t *testing.T) { WithCallAllExtensionResponses(map[runtimecatalog.GroupVersionHook]runtimehooksv1.ResponseObject{ beforeClusterUpgradeGVH: tt.hookResponse, }). - WithCallAllExtensionValidations(validateCleanupCluster). + WithCallAllExtensionValidations(validateCleanupCluster(s.Current.Cluster)). Build() fakeClient := fake.NewClientBuilder().WithScheme(fakeScheme).WithObjects(s.Current.Cluster).Build() @@ -1549,7 +1576,7 @@ func TestComputeControlPlaneVersion(t *testing.T) { WithCallAllExtensionResponses(map[runtimecatalog.GroupVersionHook]runtimehooksv1.ResponseObject{ afterControlPlaneUpgradeGVH: tt.hookResponse, }). - WithCallAllExtensionValidations(validateCleanupCluster). + WithCallAllExtensionValidations(validateCleanupCluster(tt.s.Current.Cluster)). WithCatalog(catalog). Build() @@ -3545,25 +3572,46 @@ func TestCalculateRefDesiredAPIVersion(t *testing.T) { } } -func validateCleanupCluster(req runtimehooksv1.RequestObject) error { - var cluster clusterv1beta1.Cluster - switch req := req.(type) { - case *runtimehooksv1.BeforeClusterUpgradeRequest: - cluster = req.Cluster - case *runtimehooksv1.AfterControlPlaneUpgradeRequest: - cluster = req.Cluster - default: - return fmt.Errorf("unhandled request type %T", req) - } +func validateCleanupCluster(originalCluster *clusterv1.Cluster) func(req runtimehooksv1.RequestObject) error { + return func(req runtimehooksv1.RequestObject) error { + var cluster clusterv1beta1.Cluster + switch req := req.(type) { + case *runtimehooksv1.BeforeClusterUpgradeRequest: + cluster = req.Cluster + case *runtimehooksv1.AfterControlPlaneUpgradeRequest: + cluster = req.Cluster + default: + return fmt.Errorf("unhandled request type %T", req) + } - if cluster.GetManagedFields() != nil { - return errors.New("managedFields should have been cleaned up") - } - if _, ok := cluster.Annotations[corev1.LastAppliedConfigAnnotation]; ok { - return errors.New("last-applied-configuration annotation should have been cleaned up") - } - if _, ok := cluster.Annotations[conversion.DataAnnotation]; ok { - return errors.New("conversion annotation should have been cleaned up") + if cluster.GetManagedFields() != nil { + return errors.New("managedFields should have been cleaned up") + } + if _, ok := cluster.Annotations[corev1.LastAppliedConfigAnnotation]; ok { + return errors.New("last-applied-configuration annotation should have been cleaned up") + } + if _, ok := cluster.Annotations[conversion.DataAnnotation]; ok { + return errors.New("conversion annotation should have been cleaned up") + } + + v1beta2Cluster := &clusterv1.Cluster{} + if err := cluster.ConvertTo(v1beta2Cluster); err != nil { + return err + } + + // Apply same changes that are expected for a cluster sent to runtime hook. + originalClusterCopy := originalCluster.DeepCopy() + originalClusterCopy.SetManagedFields(nil) + if originalClusterCopy.Annotations != nil { + annotations := maps.Clone(cluster.Annotations) + delete(annotations, corev1.LastAppliedConfigAnnotation) + delete(annotations, conversion.DataAnnotation) + originalClusterCopy.Annotations = annotations + } + + if !apiequality.Semantic.DeepEqual(originalClusterCopy, v1beta2Cluster) { + return errors.Errorf("call to extension is not passing the expected cluster object: %s", cmp.Diff(originalClusterCopy, v1beta2Cluster)) + } + return nil } - return nil } diff --git a/internal/controllers/topology/cluster/cluster_controller.go b/internal/controllers/topology/cluster/cluster_controller.go index ab4664f84176..686afc1bf9e8 100644 --- a/internal/controllers/topology/cluster/cluster_controller.go +++ b/internal/controllers/topology/cluster/cluster_controller.go @@ -432,7 +432,7 @@ func (r *Reconciler) callBeforeClusterCreateHook(ctx context.Context, s *scope.S if s.Current.Cluster.Spec.InfrastructureRef == nil && s.Current.Cluster.Spec.ControlPlaneRef == nil { v1beta1Cluster := &clusterv1beta1.Cluster{} - if err := clusterv1beta1.Convert_v1beta2_Cluster_To_v1beta1_Cluster(s.Current.Cluster, v1beta1Cluster, nil); err != nil { + if err := v1beta1Cluster.ConvertFrom(s.Current.Cluster); err != nil { return ctrl.Result{}, errors.Wrap(err, "error converting Cluster to v1beta1 Cluster") } @@ -525,7 +525,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, cluster *clusterv1.Clu if feature.Gates.Enabled(feature.RuntimeSDK) { if !hooks.IsOkToDelete(cluster) { v1beta1Cluster := &clusterv1beta1.Cluster{} - if err := clusterv1beta1.Convert_v1beta2_Cluster_To_v1beta1_Cluster(cluster, v1beta1Cluster, nil); err != nil { + if err := v1beta1Cluster.ConvertFrom(cluster); err != nil { return ctrl.Result{}, errors.Wrap(err, "error converting Cluster to v1beta1 Cluster") } diff --git a/internal/controllers/topology/cluster/reconcile_state.go b/internal/controllers/topology/cluster/reconcile_state.go index 3c39c8441099..de9e10872246 100644 --- a/internal/controllers/topology/cluster/reconcile_state.go +++ b/internal/controllers/topology/cluster/reconcile_state.go @@ -197,7 +197,7 @@ func (r *Reconciler) callAfterControlPlaneInitialized(ctx context.Context, s *sc if hooks.IsPending(runtimehooksv1.AfterControlPlaneInitialized, s.Current.Cluster) { if isControlPlaneInitialized(s.Current.Cluster) { v1beta1Cluster := &clusterv1beta1.Cluster{} - if err := clusterv1beta1.Convert_v1beta2_Cluster_To_v1beta1_Cluster(s.Current.Cluster, v1beta1Cluster, nil); err != nil { + if err := v1beta1Cluster.ConvertFrom(s.Current.Cluster); err != nil { return errors.Wrap(err, "error converting Cluster to v1beta1 Cluster") } @@ -250,7 +250,7 @@ func (r *Reconciler) callAfterClusterUpgrade(ctx context.Context, s *scope.Scope !s.UpgradeTracker.MachinePools.IsAnyPendingUpgrade() && // No MachinePools are pending an upgrade !s.UpgradeTracker.MachinePools.DeferredUpgrade() { // No MachinePools have deferred an upgrade v1beta1Cluster := &clusterv1beta1.Cluster{} - if err := clusterv1beta1.Convert_v1beta2_Cluster_To_v1beta1_Cluster(s.Current.Cluster, v1beta1Cluster, nil); err != nil { + if err := v1beta1Cluster.ConvertFrom(s.Current.Cluster); err != nil { return errors.Wrap(err, "error converting Cluster to v1beta1 Cluster") } diff --git a/internal/controllers/topology/cluster/reconcile_state_test.go b/internal/controllers/topology/cluster/reconcile_state_test.go index b4e58e47e3d2..023cd921a7a0 100644 --- a/internal/controllers/topology/cluster/reconcile_state_test.go +++ b/internal/controllers/topology/cluster/reconcile_state_test.go @@ -33,6 +33,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" utilfeature "k8s.io/component-base/featuregate/testing" @@ -42,6 +43,7 @@ import ( . "sigs.k8s.io/controller-runtime/pkg/envtest/komega" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + clusterv1beta1 "sigs.k8s.io/cluster-api/api/core/v1beta1" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" runtimehooksv1 "sigs.k8s.io/cluster-api/api/runtime/hooks/v1alpha1" runtimev1 "sigs.k8s.io/cluster-api/api/runtime/v1beta2" @@ -290,6 +292,27 @@ func TestReconcileShim(t *testing.T) { } func TestReconcile_callAfterControlPlaneInitialized(t *testing.T) { + var testGVKs = []schema.GroupVersionKind{ + { + Group: "refAPIGroup1", + Kind: "refKind1", + Version: "v1beta4", + }, + } + + apiVersionGetter := func(gk schema.GroupKind) (string, error) { + for _, gvk := range testGVKs { + if gvk.GroupKind() == gk { + return schema.GroupVersion{ + Group: gk.Group, + Version: gvk.Version, + }.String(), nil + } + } + return "", fmt.Errorf("unknown GroupVersionKind: %v", gk) + } + clusterv1beta1.SetAPIVersionGetter(apiVersionGetter) + catalog := runtimecatalog.New() _ = runtimehooksv1.AddToCatalog(catalog) @@ -342,8 +365,16 @@ func TestReconcile_callAfterControlPlaneInitialized(t *testing.T) { }, }, Spec: clusterv1.ClusterSpec{ - ControlPlaneRef: &clusterv1.ContractVersionedObjectReference{}, - InfrastructureRef: &clusterv1.ContractVersionedObjectReference{}, + ControlPlaneRef: &clusterv1.ContractVersionedObjectReference{ + APIGroup: "refAPIGroup1", + Kind: "refKind1", + Name: "refName1", + }, + InfrastructureRef: &clusterv1.ContractVersionedObjectReference{ + APIGroup: "refAPIGroup1", + Kind: "refKind1", + Name: "refName1", + }, }, Status: clusterv1.ClusterStatus{ Conditions: []metav1.Condition{ @@ -367,8 +398,16 @@ func TestReconcile_callAfterControlPlaneInitialized(t *testing.T) { }, }, Spec: clusterv1.ClusterSpec{ - ControlPlaneRef: &clusterv1.ContractVersionedObjectReference{}, - InfrastructureRef: &clusterv1.ContractVersionedObjectReference{}, + ControlPlaneRef: &clusterv1.ContractVersionedObjectReference{ + APIGroup: "refAPIGroup1", + Kind: "refKind1", + Name: "refName1", + }, + InfrastructureRef: &clusterv1.ContractVersionedObjectReference{ + APIGroup: "refAPIGroup1", + Kind: "refKind1", + Name: "refName1", + }, }, Status: clusterv1.ClusterStatus{ Conditions: []metav1.Condition{ @@ -392,8 +431,16 @@ func TestReconcile_callAfterControlPlaneInitialized(t *testing.T) { }, }, Spec: clusterv1.ClusterSpec{ - ControlPlaneRef: &clusterv1.ContractVersionedObjectReference{}, - InfrastructureRef: &clusterv1.ContractVersionedObjectReference{}, + ControlPlaneRef: &clusterv1.ContractVersionedObjectReference{ + APIGroup: "refAPIGroup1", + Kind: "refKind1", + Name: "refName1", + }, + InfrastructureRef: &clusterv1.ContractVersionedObjectReference{ + APIGroup: "refAPIGroup1", + Kind: "refKind1", + Name: "refName1", + }, }, Status: clusterv1.ClusterStatus{ Conditions: []metav1.Condition{ From 8a870dc7b10cd8edae0ee8282528ed9a6249bb0f Mon Sep 17 00:00:00 2001 From: fabriziopandini Date: Tue, 22 Jul 2025 13:29:39 +0200 Subject: [PATCH 2/3] Address feedback --- exp/topology/desiredstate/desired_state_test.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/exp/topology/desiredstate/desired_state_test.go b/exp/topology/desiredstate/desired_state_test.go index 1d16c9fef8c6..0e8cd1e8eabe 100644 --- a/exp/topology/desiredstate/desired_state_test.go +++ b/exp/topology/desiredstate/desired_state_test.go @@ -1256,7 +1256,7 @@ func TestComputeControlPlaneVersion(t *testing.T) { WithCallAllExtensionResponses(map[runtimecatalog.GroupVersionHook]runtimehooksv1.ResponseObject{ beforeClusterUpgradeGVH: tt.hookResponse, }). - WithCallAllExtensionValidations(validateCleanupCluster(s.Current.Cluster)). + WithCallAllExtensionValidations(validateClusterParameter(s.Current.Cluster)). Build() fakeClient := fake.NewClientBuilder().WithScheme(fakeScheme).WithObjects(s.Current.Cluster).Build() @@ -1576,7 +1576,7 @@ func TestComputeControlPlaneVersion(t *testing.T) { WithCallAllExtensionResponses(map[runtimecatalog.GroupVersionHook]runtimehooksv1.ResponseObject{ afterControlPlaneUpgradeGVH: tt.hookResponse, }). - WithCallAllExtensionValidations(validateCleanupCluster(tt.s.Current.Cluster)). + WithCallAllExtensionValidations(validateClusterParameter(tt.s.Current.Cluster)). WithCatalog(catalog). Build() @@ -3572,7 +3572,9 @@ func TestCalculateRefDesiredAPIVersion(t *testing.T) { } } -func validateCleanupCluster(originalCluster *clusterv1.Cluster) func(req runtimehooksv1.RequestObject) error { +func validateClusterParameter(originalCluster *clusterv1.Cluster) func(req runtimehooksv1.RequestObject) error { + // return a func that allows to check if expected transformations are applied to the Cluster parameter which is + // included in the payload for lifecycle hooks calls. return func(req runtimehooksv1.RequestObject) error { var cluster clusterv1beta1.Cluster switch req := req.(type) { @@ -3584,6 +3586,7 @@ func validateCleanupCluster(originalCluster *clusterv1.Cluster) func(req runtime return fmt.Errorf("unhandled request type %T", req) } + // check if managed fields and well know annotations have been removed from the Cluster parameter included in the payload lifecycle hooks calls. if cluster.GetManagedFields() != nil { return errors.New("managedFields should have been cleaned up") } @@ -3594,12 +3597,13 @@ func validateCleanupCluster(originalCluster *clusterv1.Cluster) func(req runtime return errors.New("conversion annotation should have been cleaned up") } + // check the Cluster parameter included in the payload lifecycle hooks calls has been properly converted from v1beta2 to v1beta1. + // Note: to perform this check we convert the parameter back to v1beta2 and compare with the original cluster +/- expected transformations. v1beta2Cluster := &clusterv1.Cluster{} if err := cluster.ConvertTo(v1beta2Cluster); err != nil { return err } - // Apply same changes that are expected for a cluster sent to runtime hook. originalClusterCopy := originalCluster.DeepCopy() originalClusterCopy.SetManagedFields(nil) if originalClusterCopy.Annotations != nil { From cae82b48e5aa4998c24401ef4da156cf4e60c15e Mon Sep 17 00:00:00 2001 From: fabriziopandini Date: Wed, 23 Jul 2025 22:58:16 +0200 Subject: [PATCH 3/3] More comments --- .../desiredstate/desired_state_test.go | 8 ++ .../cluster/cluster_controller_test.go | 105 ++++++++++++------ .../topology/cluster/reconcile_state_test.go | 4 +- 3 files changed, 78 insertions(+), 39 deletions(-) diff --git a/exp/topology/desiredstate/desired_state_test.go b/exp/topology/desiredstate/desired_state_test.go index 0e8cd1e8eabe..239ec4022d90 100644 --- a/exp/topology/desiredstate/desired_state_test.go +++ b/exp/topology/desiredstate/desired_state_test.go @@ -1226,6 +1226,11 @@ func TestComputeControlPlaneVersion(t *testing.T) { FieldsV1: &metav1.FieldsV1{}, }, }, + Annotations: map[string]string{ + "fizz": "buzz", + corev1.LastAppliedConfigAnnotation: "should be cleaned up", + conversion.DataAnnotation: "should be cleaned up", + }, }, // Add some more fields to check that conversion implemented when calling RuntimeExtension are properly handled. Spec: clusterv1.ClusterSpec{ @@ -3613,6 +3618,9 @@ func validateClusterParameter(originalCluster *clusterv1.Cluster) func(req runti originalClusterCopy.Annotations = annotations } + // drop conditions, it is not possible to round trip without the data annotation. + originalClusterCopy.Status.Conditions = nil + if !apiequality.Semantic.DeepEqual(originalClusterCopy, v1beta2Cluster) { return errors.Errorf("call to extension is not passing the expected cluster object: %s", cmp.Diff(originalClusterCopy, v1beta2Cluster)) } diff --git a/internal/controllers/topology/cluster/cluster_controller_test.go b/internal/controllers/topology/cluster/cluster_controller_test.go index e43609c1ca37..12b7862a6ced 100644 --- a/internal/controllers/topology/cluster/cluster_controller_test.go +++ b/internal/controllers/topology/cluster/cluster_controller_test.go @@ -17,14 +17,17 @@ limitations under the License. package cluster import ( - "errors" "fmt" + "maps" "testing" "time" + "github.com/google/go-cmp/cmp" . "github.com/onsi/gomega" + "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + apiequality "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" utilfeature "k8s.io/component-base/featuregate/testing" @@ -582,7 +585,7 @@ func TestClusterReconciler_reconcileDelete(t *testing.T) { WithCallAllExtensionResponses(map[runtimecatalog.GroupVersionHook]runtimehooksv1.ResponseObject{ beforeClusterDeleteGVH: tt.hookResponse, }). - WithCallAllExtensionValidations(validateCleanupCluster). + WithCallAllExtensionValidations(validateClusterParameter(tt.cluster)). WithCatalog(catalog). Build() @@ -727,18 +730,6 @@ func TestReconciler_callBeforeClusterCreateHook(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - - runtimeClient := fakeruntimeclient.NewRuntimeClientBuilder(). - WithCatalog(catalog). - WithCallAllExtensionResponses(map[runtimecatalog.GroupVersionHook]runtimehooksv1.ResponseObject{ - gvh: tt.hookResponse, - }). - WithCallAllExtensionValidations(validateCleanupCluster). - Build() - - r := &Reconciler{ - RuntimeClient: runtimeClient, - } s := &scope.Scope{ Current: &scope.ClusterState{ Cluster: &clusterv1.Cluster{ @@ -764,6 +755,18 @@ func TestReconciler_callBeforeClusterCreateHook(t *testing.T) { }, HookResponseTracker: scope.NewHookResponseTracker(), } + + runtimeClient := fakeruntimeclient.NewRuntimeClientBuilder(). + WithCatalog(catalog). + WithCallAllExtensionResponses(map[runtimecatalog.GroupVersionHook]runtimehooksv1.ResponseObject{ + gvh: tt.hookResponse, + }). + WithCallAllExtensionValidations(validateClusterParameter(s.Current.Cluster)). + Build() + + r := &Reconciler{ + RuntimeClient: runtimeClient, + } res, err := r.callBeforeClusterCreateHook(ctx, s) if tt.wantErr { g.Expect(err).To(HaveOccurred()) @@ -1716,29 +1719,57 @@ func TestClusterClassToCluster(t *testing.T) { } } -func validateCleanupCluster(req runtimehooksv1.RequestObject) error { - var cluster clusterv1beta1.Cluster - switch req := req.(type) { - case *runtimehooksv1.BeforeClusterCreateRequest: - cluster = req.Cluster - case *runtimehooksv1.AfterControlPlaneInitializedRequest: - cluster = req.Cluster - case *runtimehooksv1.AfterClusterUpgradeRequest: - cluster = req.Cluster - case *runtimehooksv1.BeforeClusterDeleteRequest: - cluster = req.Cluster - default: - return fmt.Errorf("unhandled request type %T", req) - } +func validateClusterParameter(originalCluster *clusterv1.Cluster) func(req runtimehooksv1.RequestObject) error { + // return a func that allows to check if expected transformations are applied to the Cluster parameter which is + // included in the payload for lifecycle hooks calls. + return func(req runtimehooksv1.RequestObject) error { + var cluster clusterv1beta1.Cluster + switch req := req.(type) { + case *runtimehooksv1.BeforeClusterCreateRequest: + cluster = req.Cluster + case *runtimehooksv1.AfterControlPlaneInitializedRequest: + cluster = req.Cluster + case *runtimehooksv1.AfterClusterUpgradeRequest: + cluster = req.Cluster + case *runtimehooksv1.BeforeClusterDeleteRequest: + cluster = req.Cluster + default: + return fmt.Errorf("unhandled request type %T", req) + } - if cluster.GetManagedFields() != nil { - return errors.New("managedFields should have been cleaned up") - } - if _, ok := cluster.Annotations[corev1.LastAppliedConfigAnnotation]; ok { - return errors.New("last-applied-configuration annotation should have been cleaned up") - } - if _, ok := cluster.Annotations[conversion.DataAnnotation]; ok { - return errors.New("conversion annotation should have been cleaned up") + // check if managed fields and well know annotations have been removed from the Cluster parameter included in the payload lifecycle hooks calls. + if cluster.GetManagedFields() != nil { + return errors.New("managedFields should have been cleaned up") + } + if _, ok := cluster.Annotations[corev1.LastAppliedConfigAnnotation]; ok { + return errors.New("last-applied-configuration annotation should have been cleaned up") + } + if _, ok := cluster.Annotations[conversion.DataAnnotation]; ok { + return errors.New("conversion annotation should have been cleaned up") + } + + // check the Cluster parameter included in the payload lifecycle hooks calls has been properly converted from v1beta2 to v1beta1. + // Note: to perform this check we convert the parameter back to v1beta2 and compare with the original cluster +/- expected transformations. + v1beta2Cluster := &clusterv1.Cluster{} + if err := cluster.ConvertTo(v1beta2Cluster); err != nil { + return err + } + + originalClusterCopy := originalCluster.DeepCopy() + originalClusterCopy.SetManagedFields(nil) + if originalClusterCopy.Annotations != nil { + annotations := maps.Clone(cluster.Annotations) + delete(annotations, corev1.LastAppliedConfigAnnotation) + delete(annotations, conversion.DataAnnotation) + originalClusterCopy.Annotations = annotations + } + + // drop conditions, it is not possible to round trip without the data annotation. + originalClusterCopy.Status.Conditions = nil + + if !apiequality.Semantic.DeepEqual(originalClusterCopy, v1beta2Cluster) { + return errors.Errorf("call to extension is not passing the expected cluster object: %s", cmp.Diff(originalClusterCopy, v1beta2Cluster)) + } + return nil } - return nil } diff --git a/internal/controllers/topology/cluster/reconcile_state_test.go b/internal/controllers/topology/cluster/reconcile_state_test.go index 023cd921a7a0..a24e4dc8bf9b 100644 --- a/internal/controllers/topology/cluster/reconcile_state_test.go +++ b/internal/controllers/topology/cluster/reconcile_state_test.go @@ -509,7 +509,7 @@ func TestReconcile_callAfterControlPlaneInitialized(t *testing.T) { WithCallAllExtensionResponses(map[runtimecatalog.GroupVersionHook]runtimehooksv1.ResponseObject{ afterControlPlaneInitializedGVH: tt.hookResponse, }). - WithCallAllExtensionValidations(validateCleanupCluster). + WithCallAllExtensionValidations(validateClusterParameter(tt.cluster)). WithCatalog(catalog). Build() @@ -1153,7 +1153,7 @@ func TestReconcile_callAfterClusterUpgrade(t *testing.T) { WithCallAllExtensionResponses(map[runtimecatalog.GroupVersionHook]runtimehooksv1.ResponseObject{ afterClusterUpgradeGVH: tt.hookResponse, }). - WithCallAllExtensionValidations(validateCleanupCluster). + WithCallAllExtensionValidations(validateClusterParameter(tt.s.Current.Cluster)). WithCatalog(catalog). Build()