Cancel instance refresh on any relevant change to ASG instead of blocking until previous one is finished (which may have led to failing nodes due to outdated join token)

AndiDog · AndiDog · commit 8811464d5ef6 · 2024-10-22T09:29:29.000+02:00
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/cluster_api_controller.go b/cmd/clusterawsadm/cloudformation/bootstrap/cluster_api_controller.go
@@ -193,6 +193,7 @@ func (t Template) ControllersPolicy() *iamv1.PolicyDocument {
 				"arn:*:autoscaling:*:*:autoScalingGroup:*:autoScalingGroupName/*",
 			},
 			Action: iamv1.Actions{
+				"autoscaling:CancelInstanceRefresh",
 				"autoscaling:CreateAutoScalingGroup",
 				"autoscaling:UpdateAutoScalingGroup",
 				"autoscaling:CreateOrUpdateTags",
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/customsuffix.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/customsuffix.yaml
@@ -249,6 +249,7 @@ Resources:
           Resource:
           - '*'
         - Action:
+          - autoscaling:CancelInstanceRefresh
           - autoscaling:CreateAutoScalingGroup
           - autoscaling:UpdateAutoScalingGroup
           - autoscaling:CreateOrUpdateTags
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/default.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/default.yaml
@@ -249,6 +249,7 @@ Resources:
           Resource:
           - '*'
         - Action:
+          - autoscaling:CancelInstanceRefresh
           - autoscaling:CreateAutoScalingGroup
           - autoscaling:UpdateAutoScalingGroup
           - autoscaling:CreateOrUpdateTags
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_all_secret_backends.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_all_secret_backends.yaml
@@ -255,6 +255,7 @@ Resources:
           Resource:
           - '*'
         - Action:
+          - autoscaling:CancelInstanceRefresh
           - autoscaling:CreateAutoScalingGroup
           - autoscaling:UpdateAutoScalingGroup
           - autoscaling:CreateOrUpdateTags
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_allow_assume_role.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_allow_assume_role.yaml
@@ -249,6 +249,7 @@ Resources:
           Resource:
           - '*'
         - Action:
+          - autoscaling:CancelInstanceRefresh
           - autoscaling:CreateAutoScalingGroup
           - autoscaling:UpdateAutoScalingGroup
           - autoscaling:CreateOrUpdateTags
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_bootstrap_user.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_bootstrap_user.yaml
@@ -255,6 +255,7 @@ Resources:
           Resource:
           - '*'
         - Action:
+          - autoscaling:CancelInstanceRefresh
           - autoscaling:CreateAutoScalingGroup
           - autoscaling:UpdateAutoScalingGroup
           - autoscaling:CreateOrUpdateTags
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_custom_bootstrap_user.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_custom_bootstrap_user.yaml
@@ -255,6 +255,7 @@ Resources:
           Resource:
           - '*'
         - Action:
+          - autoscaling:CancelInstanceRefresh
           - autoscaling:CreateAutoScalingGroup
           - autoscaling:UpdateAutoScalingGroup
           - autoscaling:CreateOrUpdateTags
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_different_instance_profiles.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_different_instance_profiles.yaml
@@ -249,6 +249,7 @@ Resources:
           Resource:
           - '*'
         - Action:
+          - autoscaling:CancelInstanceRefresh
           - autoscaling:CreateAutoScalingGroup
           - autoscaling:UpdateAutoScalingGroup
           - autoscaling:CreateOrUpdateTags
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_console.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_console.yaml
@@ -249,6 +249,7 @@ Resources:
           Resource:
           - '*'
         - Action:
+          - autoscaling:CancelInstanceRefresh
           - autoscaling:CreateAutoScalingGroup
           - autoscaling:UpdateAutoScalingGroup
           - autoscaling:CreateOrUpdateTags
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_default_roles.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_default_roles.yaml
@@ -249,6 +249,7 @@ Resources:
           Resource:
           - '*'
         - Action:
+          - autoscaling:CancelInstanceRefresh
           - autoscaling:CreateAutoScalingGroup
           - autoscaling:UpdateAutoScalingGroup
           - autoscaling:CreateOrUpdateTags
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_disable.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_disable.yaml
@@ -249,6 +249,7 @@ Resources:
           Resource:
           - '*'
         - Action:
+          - autoscaling:CancelInstanceRefresh
           - autoscaling:CreateAutoScalingGroup
           - autoscaling:UpdateAutoScalingGroup
           - autoscaling:CreateOrUpdateTags
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_kms_prefix.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_kms_prefix.yaml
@@ -249,6 +249,7 @@ Resources:
           Resource:
           - '*'
         - Action:
+          - autoscaling:CancelInstanceRefresh
           - autoscaling:CreateAutoScalingGroup
           - autoscaling:UpdateAutoScalingGroup
           - autoscaling:CreateOrUpdateTags
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_extra_statements.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_extra_statements.yaml
@@ -255,6 +255,7 @@ Resources:
           Resource:
           - '*'
         - Action:
+          - autoscaling:CancelInstanceRefresh
           - autoscaling:CreateAutoScalingGroup
           - autoscaling:UpdateAutoScalingGroup
           - autoscaling:CreateOrUpdateTags
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_s3_bucket.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_s3_bucket.yaml
@@ -249,6 +249,7 @@ Resources:
           Resource:
           - '*'
         - Action:
+          - autoscaling:CancelInstanceRefresh
           - autoscaling:CreateAutoScalingGroup
           - autoscaling:UpdateAutoScalingGroup
           - autoscaling:CreateOrUpdateTags
diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_ssm_secret_backend.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_ssm_secret_backend.yaml
@@ -249,6 +249,7 @@ Resources:
           Resource:
           - '*'
         - Action:
+          - autoscaling:CancelInstanceRefresh
           - autoscaling:CreateAutoScalingGroup
           - autoscaling:UpdateAutoScalingGroup
           - autoscaling:CreateOrUpdateTags
diff --git a/exp/controllers/awsmachinepool_controller.go b/exp/controllers/awsmachinepool_controller.go
@@ -193,13 +193,13 @@ func (r *AWSMachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 			return ctrl.Result{}, r.reconcileDelete(machinePoolScope, infraScope, infraScope)
 		}
 
-		return ctrl.Result{}, r.reconcileNormal(ctx, machinePoolScope, infraScope, infraScope, s3Scope)
+		return r.reconcileNormal(ctx, machinePoolScope, infraScope, infraScope, s3Scope)
 	case *scope.ClusterScope:
 		if !awsMachinePool.ObjectMeta.DeletionTimestamp.IsZero() {
 			return ctrl.Result{}, r.reconcileDelete(machinePoolScope, infraScope, infraScope)
 		}
 
-		return ctrl.Result{}, r.reconcileNormal(ctx, machinePoolScope, infraScope, infraScope, s3Scope)
+		return r.reconcileNormal(ctx, machinePoolScope, infraScope, infraScope, s3Scope)
 	default:
 		return ctrl.Result{}, errors.New("infraCluster has unknown type")
 	}
@@ -217,7 +217,7 @@ func (r *AWSMachinePoolReconciler) SetupWithManager(ctx context.Context, mgr ctr
 		Complete(r)
 }
 
-func (r *AWSMachinePoolReconciler) reconcileNormal(ctx context.Context, machinePoolScope *scope.MachinePoolScope, clusterScope cloud.ClusterScoper, ec2Scope scope.EC2Scope, s3Scope scope.S3Scope) error {
+func (r *AWSMachinePoolReconciler) reconcileNormal(ctx context.Context, machinePoolScope *scope.MachinePoolScope, clusterScope cloud.ClusterScoper, ec2Scope scope.EC2Scope, s3Scope scope.S3Scope) (ctrl.Result, error) {
 	clusterScope.Info("Reconciling AWSMachinePool")
 
 	// If the AWSMachine is in an error state, return early.
@@ -226,28 +226,28 @@ func (r *AWSMachinePoolReconciler) reconcileNormal(ctx context.Context, machineP
 
 		// TODO: If we are in a failed state, delete the secret regardless of instance state
 
-		return nil
+		return ctrl.Result{}, nil
 	}
 
 	// If the AWSMachinepool doesn't have our finalizer, add it
 	if controllerutil.AddFinalizer(machinePoolScope.AWSMachinePool, expinfrav1.MachinePoolFinalizer) {
 		// Register finalizer immediately to avoid orphaning AWS resources
 		if err := machinePoolScope.PatchObject(); err != nil {
-			return err
+			return ctrl.Result{}, err
 		}
 	}
 
 	if !machinePoolScope.Cluster.Status.InfrastructureReady {
 		machinePoolScope.Info("Cluster infrastructure is not ready yet")
 		conditions.MarkFalse(machinePoolScope.AWSMachinePool, expinfrav1.ASGReadyCondition, infrav1.WaitingForClusterInfrastructureReason, clusterv1.ConditionSeverityInfo, "")
-		return nil
+		return ctrl.Result{}, nil
 	}
 
 	// Make sure bootstrap data is available and populated
 	if machinePoolScope.MachinePool.Spec.Template.Spec.Bootstrap.DataSecretName == nil {
 		machinePoolScope.Info("Bootstrap data secret reference is not yet available")
 		conditions.MarkFalse(machinePoolScope.AWSMachinePool, expinfrav1.ASGReadyCondition, infrav1.WaitingForBootstrapDataReason, clusterv1.ConditionSeverityInfo, "")
-		return nil
+		return ctrl.Result{}, nil
 	}
 
 	ec2Svc := r.getEC2Service(ec2Scope)
@@ -259,20 +259,24 @@ func (r *AWSMachinePoolReconciler) reconcileNormal(ctx context.Context, machineP
 	asg, err := r.findASG(machinePoolScope, asgsvc)
 	if err != nil {
 		conditions.MarkUnknown(machinePoolScope.AWSMachinePool, expinfrav1.ASGReadyCondition, expinfrav1.ASGNotFoundReason, err.Error())
-		return err
+		return ctrl.Result{}, err
 	}
 
-	canUpdateLaunchTemplate := func() (bool, error) {
+	canStartInstanceRefresh := func() (bool, *string, error) {
 		// If there is a change: before changing the template, check if there exist an ongoing instance refresh,
 		// because only 1 instance refresh can be "InProgress". If template is updated when refresh cannot be started,
 		// that change will not trigger a refresh. Do not start an instance refresh if only userdata changed.
 		if asg == nil {
 			// If the ASG hasn't been created yet, there is no need to check if we can start the instance refresh.
 			// But we want to update the LaunchTemplate because an error in the LaunchTemplate may be blocking the ASG creation.
-			return true, nil
+			return true, nil, nil
 		}
 		return asgsvc.CanStartASGInstanceRefresh(machinePoolScope)
 	}
+	cancelInstanceRefresh := func() error {
+		machinePoolScope.Info("cancelling instance refresh")
+		return asgsvc.CancelASGInstanceRefresh(machinePoolScope)
+	}
 	runPostLaunchTemplateUpdateOperation := func() error {
 		// skip instance refresh if ASG is not created yet
 		if asg == nil {
@@ -296,10 +300,14 @@ func (r *AWSMachinePoolReconciler) reconcileNormal(ctx context.Context, machineP
 		machinePoolScope.Info("starting instance refresh", "number of instances", machinePoolScope.MachinePool.Spec.Replicas)
 		return asgsvc.StartASGInstanceRefresh(machinePoolScope)
 	}
-	if err := reconSvc.ReconcileLaunchTemplate(machinePoolScope, machinePoolScope, s3Scope, ec2Svc, objectStoreSvc, canUpdateLaunchTemplate, runPostLaunchTemplateUpdateOperation); err != nil {
+	res, err := reconSvc.ReconcileLaunchTemplate(machinePoolScope, machinePoolScope, s3Scope, ec2Svc, objectStoreSvc, canStartInstanceRefresh, cancelInstanceRefresh, runPostLaunchTemplateUpdateOperation)
+	if err != nil {
 		r.Recorder.Eventf(machinePoolScope.AWSMachinePool, corev1.EventTypeWarning, "FailedLaunchTemplateReconcile", "Failed to reconcile launch template: %v", err)
 		machinePoolScope.Error(err, "failed to reconcile launch template")
-		return err
+		return ctrl.Result{}, err
+	}
+	if res != nil {
+		return *res, nil
 	}
 
 	// set the LaunchTemplateReady condition
@@ -309,9 +317,9 @@ func (r *AWSMachinePoolReconciler) reconcileNormal(ctx context.Context, machineP
 		// Create new ASG
 		if err := r.createPool(machinePoolScope, clusterScope); err != nil {
 			conditions.MarkFalse(machinePoolScope.AWSMachinePool, expinfrav1.ASGReadyCondition, expinfrav1.ASGProvisionFailedReason, clusterv1.ConditionSeverityError, err.Error())
-			return err
+			return ctrl.Result{}, err
 		}
-		return nil
+		return ctrl.Result{}, nil
 	}
 
 	if annotations.ReplicasManagedByExternalAutoscaler(machinePoolScope.MachinePool) {
@@ -322,14 +330,14 @@ func (r *AWSMachinePoolReconciler) reconcileNormal(ctx context.Context, machineP
 				"external", asg.DesiredCapacity)
 			machinePoolScope.MachinePool.Spec.Replicas = asg.DesiredCapacity
 			if err := machinePoolScope.PatchCAPIMachinePoolObject(ctx); err != nil {
-				return err
+				return ctrl.Result{}, err
 			}
 		}
 	}
 
 	if err := r.updatePool(machinePoolScope, clusterScope, asg); err != nil {
 		machinePoolScope.Error(err, "error updating AWSMachinePool")
-		return err
+		return ctrl.Result{}, err
 	}
 
 	launchTemplateID := machinePoolScope.GetLaunchTemplateIDStatus()
@@ -346,7 +354,7 @@ func (r *AWSMachinePoolReconciler) reconcileNormal(ctx context.Context, machineP
 	}
 	err = reconSvc.ReconcileTags(machinePoolScope, resourceServiceToUpdate)
 	if err != nil {
-		return errors.Wrap(err, "error updating tags")
+		return ctrl.Result{}, errors.Wrap(err, "error updating tags")
 	}
 
 	// Make sure Spec.ProviderID is always set.
@@ -369,7 +377,7 @@ func (r *AWSMachinePoolReconciler) reconcileNormal(ctx context.Context, machineP
 		machinePoolScope.Error(err, "failed updating instances", "instances", asg.Instances)
 	}
 
-	return nil
+	return ctrl.Result{}, nil
 }
 
 func (r *AWSMachinePoolReconciler) reconcileDelete(machinePoolScope *scope.MachinePoolScope, clusterScope cloud.ClusterScoper, ec2Scope scope.EC2Scope) error {
diff --git a/exp/controllers/awsmachinepool_controller_test.go b/exp/controllers/awsmachinepool_controller_test.go
diff --git a/exp/controllers/awsmanagedmachinepool_controller.go b/exp/controllers/awsmanagedmachinepool_controller.go
diff --git a/pkg/cloud/services/autoscaling/autoscalinggroup.go b/pkg/cloud/services/autoscaling/autoscalinggroup.go
diff --git a/pkg/cloud/services/autoscaling/autoscalinggroup_test.go b/pkg/cloud/services/autoscaling/autoscalinggroup_test.go
diff --git a/pkg/cloud/services/ec2/launchtemplate.go b/pkg/cloud/services/ec2/launchtemplate.go
diff --git a/pkg/cloud/services/interfaces.go b/pkg/cloud/services/interfaces.go
diff --git a/pkg/cloud/services/mock_services/autoscaling_interface_mock.go b/pkg/cloud/services/mock_services/autoscaling_interface_mock.go
diff --git a/pkg/cloud/services/mock_services/reconcile_interface_mock.go b/pkg/cloud/services/mock_services/reconcile_interface_mock.go
