⚠️ Make the AfterClusterUpgrade hook blocking (#12984)

* Make the AfterClusterUpgrade hook blocking

# Conflicts:
#	internal/webhooks/cluster.go

* Address comments

Author: fabriziopandini

api/runtime/hooks/v1alpha1/lifecyclehooks_types.go

@@ -322,8 +322,8 @@ var _ ResponseObject = &AfterClusterUpgradeResponse{}
 type AfterClusterUpgradeResponse struct {
 	metav1.TypeMeta `json:",inline"`
 
-	// CommonResponse contains Status and Message fields common to all response types.
-	CommonResponse `json:",inline"`
+	// CommonRetryResponse contains Status, Message and RetryAfterSeconds fields.
+	CommonRetryResponse `json:",inline"`
 }
 
 // AfterClusterUpgrade is the hook that is called after the entire cluster is updated
@@ -458,7 +458,7 @@ func init() {
 			"Notes:\n" +
 			"- This hook will be called only for Clusters with a managed topology\n" +
 			"- The call's request contains the Cluster object and the Kubernetes version we upgraded to \n" +
-			"- This is a non-blocking hook",
+			"- This is a blocking hook; Runtime Extension implementers can use this hook to prevent the next upgrade to start.\n",
 	})
 
 	catalogBuilder.RegisterHook(BeforeClusterDelete, &runtimecatalog.HookMeta{

api/runtime/hooks/v1alpha1/zz_generated.deepcopy.go

@@ -164,7 +164,7 @@ func TestHookResponseTracker_IsBlocking(t *testing.T) {
 		},
 	}
 
-	afterClusterUpgradeResponse := &runtimehooksv1.AfterClusterUpgradeResponse{
+	afterAfterControlPlaneInitializedResponse := &runtimehooksv1.AfterControlPlaneInitializedResponse{
 		CommonResponse: runtimehooksv1.CommonResponse{},
 	}
 
@@ -195,8 +195,8 @@ func TestHookResponseTracker_IsBlocking(t *testing.T) {
 	t.Run("should return false if the hook is non-blocking", func(t *testing.T) {
 		g := NewWithT(t)
 		hrt := NewHookResponseTracker()
-		// AfterClusterUpgradeHook is non-blocking.
-		hrt.Add(runtimehooksv1.AfterClusterUpgrade, afterClusterUpgradeResponse)
-		g.Expect(hrt.IsBlocking(runtimehooksv1.AfterClusterUpgrade)).To(BeFalse())
+		// AfterControlPlaneInitialized is non-blocking.
+		hrt.Add(runtimehooksv1.AfterControlPlaneInitialized, afterAfterControlPlaneInitializedResponse)
+		g.Expect(hrt.IsBlocking(runtimehooksv1.AfterControlPlaneInitialized)).To(BeFalse())
 	})
 }

api/runtime/hooks/v1alpha1/zz_generated.openapi.go

@@ -580,7 +580,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, s *scope.Scope) (ctrl.
 			if err := hooks.MarkAsOkToDelete(ctx, r.Client, cluster); err != nil {
 				return ctrl.Result{}, err
 			}
-			log.Info(fmt.Sprintf("Cluster deletion js unblocked by %s hook", runtimecatalog.HookName(runtimehooksv1.BeforeClusterDelete)))
+			log.Info(fmt.Sprintf("Cluster deletion is unblocked by %s hook", runtimecatalog.HookName(runtimehooksv1.BeforeClusterDelete)))
 		}
 	}
 	return ctrl.Result{}, nil

exp/topology/scope/hookresponsetracker_test.go

@@ -919,55 +919,52 @@ func TestReconcileTopologyReconciledCondition(t *testing.T) {
 			wantConditionMessage: "Cluster is upgrading to v1.22.0\n" +
 				"  * Following hooks are blocking upgrade: AfterWorkersUpgrade: msg",
 		},
-		// TODO(chained): uncomment this as soon as AfterClusterUpgrade will be blocking
-		/*
-			{
-				name:         "should set the condition to false if AfterClusterUpgrade hook is blocking",
-				reconcileErr: nil,
-				s: &scope.Scope{
-					Current: &scope.ClusterState{
-						Cluster: &clusterv1.Cluster{
-							ObjectMeta: metav1.ObjectMeta{
-								Annotations: map[string]string{
-									runtimev1.PendingHooksAnnotation: "AfterClusterUpgrade",
-								},
-							},
-							Spec: clusterv1.ClusterSpec{
-								ControlPlaneRef:   clusterv1.ContractVersionedObjectReference{Name: "controlplane1"},
-								InfrastructureRef: clusterv1.ContractVersionedObjectReference{Name: "infra1"},
-								Topology: clusterv1.Topology{
-									Version: "v1.22.0",
-								},
+		{
+			name:         "should set the condition to false if AfterClusterUpgrade hook is blocking",
+			reconcileErr: nil,
+			s: &scope.Scope{
+				Current: &scope.ClusterState{
+					Cluster: &clusterv1.Cluster{
+						ObjectMeta: metav1.ObjectMeta{
+							Annotations: map[string]string{
+								runtimev1.PendingHooksAnnotation: "AfterClusterUpgrade",
 							},
 						},
-						ControlPlane: &scope.ControlPlaneState{
-							Object: builder.ControlPlane("ns1", "controlplane1").WithVersion("v1.22.0").Build(),
+						Spec: clusterv1.ClusterSpec{
+							ControlPlaneRef:   clusterv1.ContractVersionedObjectReference{Name: "controlplane1"},
+							InfrastructureRef: clusterv1.ContractVersionedObjectReference{Name: "infra1"},
+							Topology: clusterv1.Topology{
+								Version: "v1.22.0",
+							},
 						},
 					},
-					UpgradeTracker:  scope.NewUpgradeTracker(),
-					HookResponseTracker: func() *scope.HookResponseTracker {
-						hrt := scope.NewHookResponseTracker()
-						hrt.Add(runtimehooksv1.AfterClusterUpgrade, &runtimehooksv1.AfterClusterUpgradeResponse{
-							CommonRetryResponse: runtimehooksv1.CommonRetryResponse{
-								CommonResponse: runtimehooksv1.CommonResponse{
-									Message: "msg",
-								},
-								RetryAfterSeconds: 10,
-							},
-						})
-						return hrt
-					}(),
+					ControlPlane: &scope.ControlPlaneState{
+						Object: builder.ControlPlane("ns1", "controlplane1").WithVersion("v1.22.0").Build(),
+					},
 				},
-				wantV1Beta1ConditionStatus: corev1.ConditionFalse,
-				wantV1Beta1ConditionReason: clusterv1.TopologyReconciledClusterUpgradingV1Beta1Reason,
-				wantV1Beta1ConditionMessage: "Cluster is upgrading to v1.22.0\n" +
-					"  * Following hooks are blocking upgrade: AfterClusterUpgrade: msg",
-				wantConditionStatus: metav1.ConditionFalse,
-				wantConditionReason: clusterv1.ClusterTopologyReconciledClusterUpgradingReason,
-				wantConditionMessage: "Cluster is upgrading to v1.22.0\n" +
-					"  * Following hooks are blocking upgrade: AfterClusterUpgrade: msg",
+				UpgradeTracker: scope.NewUpgradeTracker(),
+				HookResponseTracker: func() *scope.HookResponseTracker {
+					hrt := scope.NewHookResponseTracker()
+					hrt.Add(runtimehooksv1.AfterClusterUpgrade, &runtimehooksv1.AfterClusterUpgradeResponse{
+						CommonRetryResponse: runtimehooksv1.CommonRetryResponse{
+							CommonResponse: runtimehooksv1.CommonResponse{
+								Message: "msg",
+							},
+							RetryAfterSeconds: 10,
+						},
+					})
+					return hrt
+				}(),
 			},
-		*/
+			wantV1Beta1ConditionStatus: corev1.ConditionFalse,
+			wantV1Beta1ConditionReason: clusterv1.TopologyReconciledClusterUpgradingV1Beta1Reason,
+			wantV1Beta1ConditionMessage: "Cluster is upgrading to v1.22.0\n" +
+				"  * Following hooks are blocking upgrade: AfterClusterUpgrade: msg",
+			wantConditionStatus: metav1.ConditionFalse,
+			wantConditionReason: clusterv1.ClusterTopologyReconciledClusterUpgradingReason,
+			wantConditionMessage: "Cluster is upgrading to v1.22.0\n" +
+				"  * Following hooks are blocking upgrade: AfterClusterUpgrade: msg",
+		},
 
 		// Hold & defer upgrade
 

internal/controllers/topology/cluster/cluster_controller.go

@@ -279,14 +279,10 @@ func (r *Reconciler) callAfterClusterUpgrade(ctx context.Context, s *scope.Scope
 			}
 			s.HookResponseTracker.Add(runtimehooksv1.AfterClusterUpgrade, hookResponse)
 
-			// TODO(chained): uncomment this as soon as AfterClusterUpgrade will be blocking
-			//nolint:gocritic
-			/*
-				if hookResponse.RetryAfterSeconds != 0 {
-					log.Info(fmt.Sprintf("Cluster upgrade to version %s completed but %s hook is still blocking", hookRequest.KubernetesVersion, runtimecatalog.HookName(runtimehooksv1.AfterClusterUpgrade)))
-					return nil
-				}
-			*/
+			if hookResponse.RetryAfterSeconds != 0 {
+				log.Info(fmt.Sprintf("Cluster upgrade to version %s completed but next upgrades are blocked by %s hook", hookRequest.KubernetesVersion, runtimecatalog.HookName(runtimehooksv1.AfterClusterUpgrade)))
+				return nil
+			}
 
 			// The hook is successfully called; we can remove this hook from the list of pending-hooks.
 			if err := hooks.MarkAsDone(ctx, r.Client, s.Current.Cluster, runtimehooksv1.AfterClusterUpgrade); err != nil {

internal/controllers/topology/cluster/conditions_test.go

@@ -23,6 +23,7 @@ import (
 	"net/http"
 	"regexp"
 	"testing"
+	"time"
 
 	"github.com/google/go-cmp/cmp"
 	. "github.com/onsi/gomega"
@@ -558,13 +559,26 @@ func TestReconcile_callAfterClusterUpgrade(t *testing.T) {
 	}
 
 	successResponse := &runtimehooksv1.AfterClusterUpgradeResponse{
-		CommonResponse: runtimehooksv1.CommonResponse{
-			Status: runtimehooksv1.ResponseStatusSuccess,
+		CommonRetryResponse: runtimehooksv1.CommonRetryResponse{
+			CommonResponse: runtimehooksv1.CommonResponse{
+				Status: runtimehooksv1.ResponseStatusSuccess,
+			},
 		},
 	}
 	failureResponse := &runtimehooksv1.AfterClusterUpgradeResponse{
-		CommonResponse: runtimehooksv1.CommonResponse{
-			Status: runtimehooksv1.ResponseStatusFailure,
+		CommonRetryResponse: runtimehooksv1.CommonRetryResponse{
+			CommonResponse: runtimehooksv1.CommonResponse{
+				Status: runtimehooksv1.ResponseStatusFailure,
+			},
+		},
+	}
+	blockingResponse := &runtimehooksv1.AfterClusterUpgradeResponse{
+		CommonRetryResponse: runtimehooksv1.CommonRetryResponse{
+			CommonResponse: runtimehooksv1.CommonResponse{
+				Status:  runtimehooksv1.ResponseStatusSuccess,
+				Message: "foo",
+			},
+			RetryAfterSeconds: 60,
 		},
 	}
 
@@ -578,6 +592,7 @@ func TestReconcile_callAfterClusterUpgrade(t *testing.T) {
 		hookResponse       *runtimehooksv1.AfterClusterUpgradeResponse
 		wantMarked         bool
 		wantHookToBeCalled bool
+		wantRetryAfter     time.Duration
 		wantError          bool
 	}{
 		{
@@ -1172,6 +1187,44 @@ func TestReconcile_callAfterClusterUpgrade(t *testing.T) {
 			wantHookToBeCalled: true,
 			wantError:          false,
 		},
+		{
+			name: "hook should be called if the control plane, MDs, and MPs are stable at the topology version - retry response should leave the hook marked",
+			s: &scope.Scope{
+				Blueprint: &scope.ClusterBlueprint{
+					Topology: clusterv1.Topology{
+						ControlPlane: clusterv1.ControlPlaneTopology{
+							Replicas: ptr.To[int32](2),
+						},
+					},
+				},
+				Current: &scope.ClusterState{
+					Cluster: &clusterv1.Cluster{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "test-cluster",
+							Namespace: "test-ns",
+							Annotations: map[string]string{
+								runtimev1.PendingHooksAnnotation: "AfterClusterUpgrade",
+							},
+						},
+						Spec: clusterv1.ClusterSpec{
+							Topology: clusterv1.Topology{
+								Version: topologyVersion,
+							},
+						},
+					},
+					ControlPlane: &scope.ControlPlaneState{
+						Object: controlPlaneObj,
+					},
+				},
+				HookResponseTracker: scope.NewHookResponseTracker(),
+				UpgradeTracker:      scope.NewUpgradeTracker(),
+			},
+			wantMarked:         true,
+			hookResponse:       blockingResponse,
+			wantHookToBeCalled: true,
+			wantRetryAfter:     time.Second * time.Duration(blockingResponse.RetryAfterSeconds),
+			wantError:          false,
+		},
 		{
 			name: "hook should be called if the control plane, MDs, and MPs are stable at the topology version - failure response should leave the hook marked",
 			s: &scope.Scope{
@@ -1265,6 +1318,7 @@ func TestReconcile_callAfterClusterUpgrade(t *testing.T) {
 				g.Expect(err).ToNot(HaveOccurred())
 			}
 
+			g.Expect(tt.s.HookResponseTracker.AggregateRetryAfter()).To(Equal(tt.wantRetryAfter))
 			if tt.wantHookToBeCalled {
 				g.Expect(fakeRuntimeClient.CallAllCount(runtimehooksv1.AfterClusterUpgrade)).To(Equal(1), "Expected hook to be called once")
 			} else {

internal/controllers/topology/cluster/reconcile_state.go

@@ -29,6 +29,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/apimachinery/pkg/util/wait"
@@ -38,7 +39,10 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	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"
 	"sigs.k8s.io/cluster-api/controllers/external"
+	runtimecatalog "sigs.k8s.io/cluster-api/exp/runtime/catalog"
 	"sigs.k8s.io/cluster-api/feature"
 	"sigs.k8s.io/cluster-api/internal/contract"
 	"sigs.k8s.io/cluster-api/internal/topology/check"
@@ -400,7 +404,7 @@ func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newClu
 			// If a generateUpgradePlan extension is defined, we assume that additionally a Cluster validating webhook is implemented
 			// that validates Cluster.spec.topology.version in a way that matches with GenerateUpgradePlan responses.
 			shouldValidateVersionCeiling := len(clusterClass.Spec.KubernetesVersions) == 0 && clusterClass.Spec.Upgrade.External.GenerateUpgradePlanExtension == ""
-			if err := webhook.validateTopologyVersionUpdate(ctx, fldPath.Child("version"), newCluster.Spec.Topology.Version, inVersion, oldVersion, oldCluster, shouldValidateVersionCeiling); err != nil {
+			if err := webhook.validateTopologyVersionUpdate(ctx, fldPath.Child("version"), newCluster.Spec.Topology.Version, inVersion, oldVersion, newCluster, oldCluster, shouldValidateVersionCeiling); err != nil {
 				allErrs = append(allErrs, err)
 			}
 		}
@@ -430,7 +434,7 @@ func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newClu
 	return allWarnings, allErrs
 }
 
-func (webhook *Cluster) validateTopologyVersionUpdate(ctx context.Context, fldPath *field.Path, fldValue string, inVersion, oldVersion semver.Version, oldCluster *clusterv1.Cluster, shouldValidateCeiling bool) *field.Error {
+func (webhook *Cluster) validateTopologyVersionUpdate(ctx context.Context, fldPath *field.Path, fldValue string, inVersion, oldVersion semver.Version, newCluster, oldCluster *clusterv1.Cluster, shouldValidateCeiling bool) *field.Error {
 	// Nothing to do if the version doesn't change.
 	if inVersion.String() == oldVersion.String() {
 		return nil
@@ -461,6 +465,15 @@ func (webhook *Cluster) validateTopologyVersionUpdate(ctx context.Context, fldPa
 		}
 	}
 
+	// Cannot upgrade when lifecycle hooks are still being completed for the previous upgrade.
+	if IsPending(runtimehooksv1.AfterClusterUpgrade, newCluster) {
+		return field.Invalid(
+			fldPath,
+			fldValue,
+			fmt.Sprintf("version cannot be changed when the %q hook is still blocking", runtimecatalog.HookName(runtimehooksv1.AfterClusterUpgrade)),
+		)
+	}
+
 	allErrs := []error{}
 	// minor version cannot be increased if control plane is upgrading or not yet on the current version
 	if err := validateTopologyControlPlaneVersion(ctx, webhook.Client, oldCluster, oldVersion); err != nil {
@@ -1070,3 +1083,27 @@ func validateAutoscalerAnnotationsForCluster(cluster *clusterv1.Cluster, cluster
 	}
 	return allErrs
 }
+
+// Note: code duplicated from internal/hooks/tracking.go to avoid a circular dependency when running tests
+// # sigs.k8s.io/cluster-api/util/patch
+// package sigs.k8s.io/cluster-api/util/patch
+//        imports sigs.k8s.io/cluster-api/internal/test/envtest from suite_test.go
+//        imports sigs.k8s.io/cluster-api/internal/webhooks from environment.go
+//        imports sigs.k8s.io/cluster-api/internal/hooks from cluster.go
+//        imports sigs.k8s.io/cluster-api/util/patch from tracking.go: import cycle not allowed in test
+// TODO: investigate.
+
+// IsPending returns true if there is an intent to call a hook being tracked in the object's PendingHooksAnnotation.
+func IsPending(hook runtimecatalog.Hook, obj client.Object) bool {
+	hookName := runtimecatalog.HookName(hook)
+	annotations := obj.GetAnnotations()
+	if annotations == nil {
+		return false
+	}
+	return isInCommaSeparatedList(annotations[runtimev1.PendingHooksAnnotation], hookName)
+}
+
+func isInCommaSeparatedList(list, item string) bool {
+	set := sets.Set[string]{}.Insert(strings.Split(list, ",")...)
+	return set.Has(item)
+}