From 5e1d9600e31377b72372db3251607868cfa75c7d Mon Sep 17 00:00:00 2001 From: Yuvaraj Kakaraparthi Date: Mon, 13 Jun 2022 18:29:43 -0700 Subject: [PATCH] Implement BeforeClusterDelete hook --- .../clusterresourcesetbinding_controller.go | 8 + .../api/v1alpha1/extensionconfig_types.go | 4 + .../controllers/cluster/cluster_controller.go | 9 + .../cluster/cluster_controller_test.go | 51 ++++++ .../topology/cluster/cluster_controller.go | 30 +++- .../cluster/cluster_controller_test.go | 155 ++++++++++++++++++ internal/hooks/tracking.go | 33 ++++ internal/hooks/tracking_test.go | 79 +++++++++ 8 files changed, 366 insertions(+), 3 deletions(-) diff --git a/exp/addons/internal/controllers/clusterresourcesetbinding_controller.go b/exp/addons/internal/controllers/clusterresourcesetbinding_controller.go index 85682aeccdd3..b0e12e3dd3f5 100644 --- a/exp/addons/internal/controllers/clusterresourcesetbinding_controller.go +++ b/exp/addons/internal/controllers/clusterresourcesetbinding_controller.go @@ -30,6 +30,8 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" addonsv1 "sigs.k8s.io/cluster-api/exp/addons/api/v1beta1" + "sigs.k8s.io/cluster-api/feature" + "sigs.k8s.io/cluster-api/internal/hooks" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/predicates" ) @@ -91,6 +93,12 @@ func (r *ClusterResourceSetBindingReconciler) Reconcile(ctx context.Context, req } // If the owner cluster is in deletion process, delete its ClusterResourceSetBinding if !cluster.DeletionTimestamp.IsZero() { + if feature.Gates.Enabled(feature.RuntimeSDK) && feature.Gates.Enabled(feature.ClusterTopology) { + if cluster.Spec.Topology != nil && !hooks.IsOkToDelete(cluster) { + // If the Cluster is not yet ready to be deleted then do not delete the ClusterResourceSetBinding. + return ctrl.Result{}, nil + } + } log.Info("deleting ClusterResourceSetBinding because the owner Cluster is currently being deleted") return ctrl.Result{}, r.Client.Delete(ctx, binding) } diff --git a/exp/runtime/api/v1alpha1/extensionconfig_types.go b/exp/runtime/api/v1alpha1/extensionconfig_types.go index ca2a7a5e7fb8..dc809c6415d8 100644 --- a/exp/runtime/api/v1alpha1/extensionconfig_types.go +++ b/exp/runtime/api/v1alpha1/extensionconfig_types.go @@ -210,4 +210,8 @@ const ( // The annotation will be used to track the intent to call a hook as soon as an operation completes; // the intent will be removed as soon as the hook call completes successfully. PendingHooksAnnotation string = "runtime.cluster.x-k8s.io/pending-hooks" + + // OkToDeleteAnnotation is the annotation used to indicate if a cluster is ready to be fully deleted. + // This annotation is added to the cluster after the BeforeClusterDelete hook has passed. + OkToDeleteAnnotation string = "runtime.cluster.x-k8s.io/ok-to-delete" ) diff --git a/internal/controllers/cluster/cluster_controller.go b/internal/controllers/cluster/cluster_controller.go index f987e76069d8..856a63b719a5 100644 --- a/internal/controllers/cluster/cluster_controller.go +++ b/internal/controllers/cluster/cluster_controller.go @@ -41,6 +41,7 @@ import ( "sigs.k8s.io/cluster-api/controllers/external" expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1" "sigs.k8s.io/cluster-api/feature" + "sigs.k8s.io/cluster-api/internal/hooks" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/annotations" "sigs.k8s.io/cluster-api/util/collections" @@ -215,6 +216,14 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster) func (r *Reconciler) reconcileDelete(ctx context.Context, cluster *clusterv1.Cluster) (reconcile.Result, error) { log := ctrl.LoggerFrom(ctx) + // If the RuntimeSDK and ClusterTopology flags are enabled, for clusters with managed topologies + // only proceed with delete if the cluster is marked as `ok-to-delete` + if feature.Gates.Enabled(feature.RuntimeSDK) && feature.Gates.Enabled(feature.ClusterTopology) { + if cluster.Spec.Topology != nil && !hooks.IsOkToDelete(cluster) { + return ctrl.Result{}, nil + } + } + descendants, err := r.listDescendants(ctx, cluster) if err != nil { log.Error(err, "Failed to list descendants") diff --git a/internal/controllers/cluster/cluster_controller_test.go b/internal/controllers/cluster/cluster_controller_test.go index 721ef6af496c..e6e01a774939 100644 --- a/internal/controllers/cluster/cluster_controller_test.go +++ b/internal/controllers/cluster/cluster_controller_test.go @@ -21,7 +21,9 @@ import ( . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + utilfeature "k8s.io/component-base/featuregate/testing" "k8s.io/utils/pointer" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -29,7 +31,9 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1" + runtimev1 "sigs.k8s.io/cluster-api/exp/runtime/api/v1alpha1" "sigs.k8s.io/cluster-api/feature" + "sigs.k8s.io/cluster-api/internal/test/builder" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/conditions" "sigs.k8s.io/cluster-api/util/patch" @@ -350,6 +354,53 @@ func TestClusterReconciler(t *testing.T) { }) } +func TestClusterReconciler_reconcileDelete(t *testing.T) { + defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.RuntimeSDK, true)() + defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.ClusterTopology, true)() + + fakeInfraCluster := builder.InfrastructureCluster("test-ns", "test-cluster").Build() + + tests := []struct { + name string + cluster *clusterv1.Cluster + wantDelete bool + }{ + { + name: "should proceed with delete if the cluster has the ok-to-delete annotation", + cluster: func() *clusterv1.Cluster { + fakeCluster := builder.Cluster("test-ns", "test-cluster").WithTopology(&clusterv1.Topology{}).WithInfrastructureCluster(fakeInfraCluster).Build() + if fakeCluster.Annotations == nil { + fakeCluster.Annotations = map[string]string{} + } + fakeCluster.Annotations[runtimev1.OkToDeleteAnnotation] = "" + return fakeCluster + }(), + wantDelete: true, + }, + { + name: "should not proceed with delete if the cluster does not have the ok-to-delete annotation", + cluster: builder.Cluster("test-ns", "test-cluster").WithTopology(&clusterv1.Topology{}).WithInfrastructureCluster(fakeInfraCluster).Build(), + wantDelete: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + fakeClient := fake.NewClientBuilder().WithObjects(fakeInfraCluster, tt.cluster).Build() + r := &Reconciler{ + Client: fakeClient, + APIReader: fakeClient, + } + + _, _ = r.reconcileDelete(ctx, tt.cluster) + infraCluster := builder.InfrastructureCluster("", "").Build() + err := fakeClient.Get(ctx, client.ObjectKeyFromObject(fakeInfraCluster), infraCluster) + g.Expect(apierrors.IsNotFound(err)).To(Equal(tt.wantDelete)) + }) + } +} + func TestClusterReconcilerNodeRef(t *testing.T) { t.Run("machine to cluster", func(t *testing.T) { cluster := &clusterv1.Cluster{ diff --git a/internal/controllers/topology/cluster/cluster_controller.go b/internal/controllers/topology/cluster/cluster_controller.go index 7d76b0f469e1..67b2175c8264 100644 --- a/internal/controllers/topology/cluster/cluster_controller.go +++ b/internal/controllers/topology/cluster/cluster_controller.go @@ -42,6 +42,7 @@ import ( "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/patches" "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/scope" "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/structuredmerge" + "sigs.k8s.io/cluster-api/internal/hooks" runtimeclient "sigs.k8s.io/cluster-api/internal/runtime/client" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/annotations" @@ -162,9 +163,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re // In case the object is deleted, the managed topology stops to reconcile; // (the other controllers will take care of deletion). if !cluster.ObjectMeta.DeletionTimestamp.IsZero() { - // TODO: When external patching is supported, we should handle the deletion - // of those external CRDs we created. - return ctrl.Result{}, nil + return r.reconcileDelete(ctx, cluster) } patchHelper, err := patch.NewHelper(cluster, r.Client) @@ -336,6 +335,31 @@ func (r *Reconciler) machineDeploymentToCluster(o client.Object) []ctrl.Request }} } +func (r *Reconciler) reconcileDelete(ctx context.Context, cluster *clusterv1.Cluster) (ctrl.Result, error) { + // Call the BeforeClusterDelete hook if the 'ok-to-delete' annotation is not set + // and add the annotation to the cluster after receiving a successful non-blocking response. + if feature.Gates.Enabled(feature.RuntimeSDK) { + if !hooks.IsOkToDelete(cluster) { + hookRequest := &runtimehooksv1.BeforeClusterDeleteRequest{ + Cluster: *cluster, + } + hookResponse := &runtimehooksv1.BeforeClusterDeleteResponse{} + if err := r.RuntimeClient.CallAllExtensions(ctx, runtimehooksv1.BeforeClusterDelete, cluster, hookRequest, hookResponse); err != nil { + return ctrl.Result{}, err + } + if hookResponse.RetryAfterSeconds != 0 { + return ctrl.Result{RequeueAfter: time.Duration(hookResponse.RetryAfterSeconds) * time.Second}, nil + } + // The BeforeClusterDelete hook returned a non-blocking response. Now the cluster is ready to be deleted. + // Lets mark the cluster as `ok-to-delete` + if err := hooks.MarkAsOkToDelete(ctx, r.Client, cluster); err != nil { + return ctrl.Result{}, errors.Wrapf(err, "failed to mark %s/%s cluster as ok to delete", cluster.Namespace, cluster.Name) + } + } + } + return ctrl.Result{}, nil +} + // serverSideApplyPatchHelperFactory makes use of managed fields provided by server side apply and is used by the controller. func serverSideApplyPatchHelperFactory(c client.Client) structuredmerge.PatchHelperFactoryFunc { return func(original, modified client.Object, opts ...structuredmerge.HelperOption) (structuredmerge.PatchHelper, error) { diff --git a/internal/controllers/topology/cluster/cluster_controller_test.go b/internal/controllers/topology/cluster/cluster_controller_test.go index cdf473604168..278af2d94ca3 100644 --- a/internal/controllers/topology/cluster/cluster_controller_test.go +++ b/internal/controllers/topology/cluster/cluster_controller_test.go @@ -23,19 +23,23 @@ import ( . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" utilfeature "k8s.io/component-base/featuregate/testing" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/reconcile" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + runtimev1 "sigs.k8s.io/cluster-api/exp/runtime/api/v1alpha1" runtimecatalog "sigs.k8s.io/cluster-api/exp/runtime/catalog" runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1" "sigs.k8s.io/cluster-api/feature" "sigs.k8s.io/cluster-api/internal/contract" "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/scope" + "sigs.k8s.io/cluster-api/internal/hooks" fakeruntimeclient "sigs.k8s.io/cluster-api/internal/runtime/client/fake" "sigs.k8s.io/cluster-api/internal/test/builder" "sigs.k8s.io/cluster-api/util/conditions" @@ -389,6 +393,157 @@ func TestClusterReconciler_reconcileClusterClassRebase(t *testing.T) { }, timeout).Should(Succeed()) } +func TestClusterReconciler_reconcileDelete(t *testing.T) { + defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.RuntimeSDK, true)() + + catalog := runtimecatalog.New() + _ = runtimehooksv1.AddToCatalog(catalog) + + beforeClusterDeleteGVH, err := catalog.GroupVersionHook(runtimehooksv1.BeforeClusterDelete) + if err != nil { + panic(err) + } + + blockingResponse := &runtimehooksv1.BeforeClusterDeleteResponse{ + CommonRetryResponse: runtimehooksv1.CommonRetryResponse{ + RetryAfterSeconds: int32(10), + CommonResponse: runtimehooksv1.CommonResponse{ + Status: runtimehooksv1.ResponseStatusSuccess, + }, + }, + } + nonBlockingResponse := &runtimehooksv1.BeforeClusterDeleteResponse{ + CommonRetryResponse: runtimehooksv1.CommonRetryResponse{ + RetryAfterSeconds: int32(0), + CommonResponse: runtimehooksv1.CommonResponse{ + Status: runtimehooksv1.ResponseStatusSuccess, + }, + }, + } + failureResponse := &runtimehooksv1.BeforeClusterDeleteResponse{ + CommonRetryResponse: runtimehooksv1.CommonRetryResponse{ + CommonResponse: runtimehooksv1.CommonResponse{ + Status: runtimehooksv1.ResponseStatusFailure, + }, + }, + } + + tests := []struct { + name string + cluster *clusterv1.Cluster + hookResponse *runtimehooksv1.BeforeClusterDeleteResponse + wantHookToBeCalled bool + wantResult ctrl.Result + wantOkToDelete bool + wantErr bool + }{ + { + name: "should apply the ok-to-delete annotation if the BeforeClusterDelete hook returns a non-blocking response", + cluster: &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + Namespace: "test-ns", + }, + Spec: clusterv1.ClusterSpec{ + Topology: &clusterv1.Topology{}, + }, + }, + hookResponse: nonBlockingResponse, + wantResult: ctrl.Result{}, + wantHookToBeCalled: true, + wantOkToDelete: true, + wantErr: false, + }, + { + name: "should requeue if the BeforeClusterDelete hook returns a blocking response", + cluster: &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + Namespace: "test-ns", + }, + Spec: clusterv1.ClusterSpec{ + Topology: &clusterv1.Topology{}, + }, + }, + hookResponse: blockingResponse, + wantResult: ctrl.Result{RequeueAfter: time.Duration(10) * time.Second}, + wantHookToBeCalled: true, + wantOkToDelete: false, + wantErr: false, + }, + { + name: "should fail if the BeforeClusterDelete hook returns a failure response", + cluster: &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + Namespace: "test-ns", + }, + Spec: clusterv1.ClusterSpec{ + Topology: &clusterv1.Topology{}, + }, + }, + hookResponse: failureResponse, + wantResult: ctrl.Result{}, + wantHookToBeCalled: true, + wantOkToDelete: false, + wantErr: true, + }, + { + name: "should succeed if the ok-to-delete annotation is already present", + cluster: &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + Namespace: "test-ns", + Annotations: map[string]string{ + // If the hook is already marked the hook should not be called during cluster delete. + runtimev1.OkToDeleteAnnotation: "", + }, + }, + Spec: clusterv1.ClusterSpec{ + Topology: &clusterv1.Topology{}, + }, + }, + // Using a blocking response here should not matter as the hook should never be called. + // Using a blocking response to enforce the point. + hookResponse: blockingResponse, + wantResult: ctrl.Result{}, + wantHookToBeCalled: false, + wantOkToDelete: true, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + fakeClient := fake.NewClientBuilder().WithObjects(tt.cluster).Build() + fakeRuntimeClient := fakeruntimeclient.NewRuntimeClientBuilder(). + WithCallAllExtensionResponses(map[runtimecatalog.GroupVersionHook]runtimehooksv1.ResponseObject{ + beforeClusterDeleteGVH: tt.hookResponse, + }). + WithCatalog(catalog). + Build() + + r := &Reconciler{ + Client: fakeClient, + APIReader: fakeClient, + RuntimeClient: fakeRuntimeClient, + } + + res, err := r.reconcileDelete(ctx, tt.cluster) + if tt.wantErr { + g.Expect(err).NotTo(BeNil()) + } else { + g.Expect(err).To(BeNil()) + g.Expect(res).To(Equal(tt.wantResult)) + g.Expect(hooks.IsOkToDelete(tt.cluster)).To(Equal(tt.wantOkToDelete)) + g.Expect(fakeRuntimeClient.CallAllCount(runtimehooksv1.BeforeClusterDelete) == 1).To(Equal(tt.wantHookToBeCalled)) + } + }) + } +} + // TestClusterReconciler_deleteClusterClass tests the correct deletion behaviour for a ClusterClass with references in existing Clusters. // In this case deletion of the ClusterClass should be blocked by the webhook. func TestClusterReconciler_deleteClusterClass(t *testing.T) { diff --git a/internal/hooks/tracking.go b/internal/hooks/tracking.go index 36150970aa4d..fc6effbab9f1 100644 --- a/internal/hooks/tracking.go +++ b/internal/hooks/tracking.go @@ -99,6 +99,39 @@ func MarkAsDone(ctx context.Context, c client.Client, obj client.Object, hooks . return nil } +// IsOkToDelete returns true if object has the OkToDeleteAnnotation in the annotations of the object, false otherwise. +func IsOkToDelete(obj client.Object) bool { + annotations := obj.GetAnnotations() + if annotations == nil { + return false + } + if _, ok := annotations[runtimev1.OkToDeleteAnnotation]; ok { + return true + } + return false +} + +// MarkAsOkToDelete adds the OkToDeleteAnnotation annotation to the object and patches it. +func MarkAsOkToDelete(ctx context.Context, c client.Client, obj client.Object) error { + patchHelper, err := patch.NewHelper(obj, c) + if err != nil { + return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: obj}) + } + + annotations := obj.GetAnnotations() + if annotations == nil { + annotations = map[string]string{} + } + annotations[runtimev1.OkToDeleteAnnotation] = "" + obj.SetAnnotations(annotations) + + if err := patchHelper.Patch(ctx, obj); err != nil { + return errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: obj}) + } + + return nil +} + func addToCommaSeparatedList(list string, items ...string) string { set := sets.NewString(strings.Split(list, ",")...) set.Insert(items...) diff --git a/internal/hooks/tracking_test.go b/internal/hooks/tracking_test.go index 084fba92c0a8..f2165bdbf265 100644 --- a/internal/hooks/tracking_test.go +++ b/internal/hooks/tracking_test.go @@ -212,3 +212,82 @@ func TestMarkAsDone(t *testing.T) { }) } } + +func TestIsOkToDelete(t *testing.T) { + tests := []struct { + name string + obj client.Object + want bool + }{ + { + name: "should return true if the object has the 'ok-to-delete' annotation", + obj: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + Namespace: "test-ns", + Annotations: map[string]string{ + runtimev1.OkToDeleteAnnotation: "", + }, + }, + }, + want: true, + }, + { + name: "should return false if the object does not have the 'ok-to-delete' annotation", + obj: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + Namespace: "test-ns", + }, + }, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + g.Expect(IsOkToDelete(tt.obj)).To(Equal(tt.want)) + }) + } +} + +func TestMarkAsOkToDelete(t *testing.T) { + tests := []struct { + name string + obj client.Object + }{ + { + name: "should add the 'ok-to-delete' annotation on the object if not already present", + obj: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + Namespace: "test-ns", + }, + }, + }, + { + name: "should succeed if the 'ok-to-delete' annotation is already present", + obj: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + Namespace: "test-ns", + Annotations: map[string]string{ + runtimev1.OkToDeleteAnnotation: "", + }, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + fakeClient := fake.NewClientBuilder().WithObjects(tt.obj).Build() + ctx := context.Background() + g.Expect(MarkAsOkToDelete(ctx, fakeClient, tt.obj)).To(Succeed()) + annotations := tt.obj.GetAnnotations() + g.Expect(annotations).To(HaveKey(runtimev1.OkToDeleteAnnotation)) + }) + } +}