Address review comments

Author: AndiDog

exp/api/v1beta2/awsmachinepool_webhook.go

@@ -17,7 +17,6 @@ limitations under the License.
 package v1beta2
 
 import (
-	"fmt"
 	"time"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -166,33 +165,6 @@ func (r *AWSMachinePool) validateLifecycleHooks() field.ErrorList {
 	return validateLifecycleHooks(r.Spec.AWSLifecycleHooks)
 }
 
-func validateLifecycleHooks(hooks []AWSLifecycleHook) field.ErrorList {
-	var allErrs field.ErrorList
-
-	for _, hook := range hooks {
-		if hook.Name == "" {
-			allErrs = append(allErrs, field.Required(field.NewPath("spec.lifecycleHooks.name"), "Name is required"))
-		}
-		if hook.NotificationTargetARN != nil && hook.RoleARN == nil {
-			allErrs = append(allErrs, field.Required(field.NewPath("spec.lifecycleHooks.roleARN"), "RoleARN is required if NotificationTargetARN is provided"))
-		}
-		if hook.RoleARN != nil && hook.NotificationTargetARN == nil {
-			allErrs = append(allErrs, field.Required(field.NewPath("spec.lifecycleHooks.notificationTargetARN"), "NotificationTargetARN is required if RoleARN is provided"))
-		}
-		if hook.LifecycleTransition != LifecycleTransitionInstanceLaunch && hook.LifecycleTransition != LifecycleTransitionInstanceTerminate {
-			allErrs = append(allErrs, field.Invalid(field.NewPath("spec.lifecycleHooks.lifecycleTransition"), hook.LifecycleTransition, fmt.Sprintf("LifecycleTransition must be either %q or %q", LifecycleTransitionInstanceLaunch, LifecycleTransitionInstanceTerminate)))
-		}
-		if hook.DefaultResult != nil && (*hook.DefaultResult != DefaultResultContinue && *hook.DefaultResult != DefaultResultAbandon) {
-			allErrs = append(allErrs, field.Invalid(field.NewPath("spec.lifecycleHooks.defaultResult"), *hook.DefaultResult, "DefaultResult must be either CONTINUE or ABANDON"))
-		}
-		if hook.HeartbeatTimeout != nil && (hook.HeartbeatTimeout.Seconds() < float64(30) || hook.HeartbeatTimeout.Seconds() > float64(172800)) {
-			allErrs = append(allErrs, field.Invalid(field.NewPath("spec.lifecycleHooks.heartbeatTimeout"), *hook.HeartbeatTimeout, "HeartbeatTimeout must be between 30 and 172800 seconds"))
-		}
-	}
-
-	return allErrs
-}
-
 // ValidateCreate will do any extra validation when creating a AWSMachinePool.
 func (r *AWSMachinePool) ValidateCreate() (admission.Warnings, error) {
 	log.Info("AWSMachinePool validate create", "machine-pool", klog.KObj(r))

exp/api/v1beta2/conditions_consts.go

@@ -57,8 +57,6 @@ const (
 
 	// LifecycleHookReadyCondition reports on the status of the lifecycle hook.
 	LifecycleHookReadyCondition clusterv1.ConditionType = "LifecycleHookReady"
-	// LifecycleHookNotFoundReason used when the lifecycle hook couldn't be retrieved.
-	LifecycleHookNotFoundReason = "LifecycleHookNotFound"
 	// LifecycleHookCreationFailedReason used for failures during lifecycle hook creation.
 	LifecycleHookCreationFailedReason = "LifecycleHookCreationFailed"
 	// LifecycleHookUpdateFailedReason used for failures during lifecycle hook update.

exp/api/v1beta2/types.go

@@ -251,7 +251,7 @@ type AWSLifecycleHook struct {
 	// +optional
 	// +kubebuilder:validation:Enum=CONTINUE;ABANDON
 	// +kubebuilder:validation:default:=none
-	DefaultResult *DefaultResult `json:"defaultResult,omitempty"`
+	DefaultResult *LifecycleHookDefaultResult `json:"defaultResult,omitempty"`
 
 	// Contains additional metadata that will be passed to the notification target.
 	// +optional
@@ -262,27 +262,27 @@ type AWSLifecycleHook struct {
 type LifecycleTransition string
 
 const (
-	// LifecycleTransitionInstanceLaunch is the launching state of the EC2 instance.
-	LifecycleTransitionInstanceLaunch LifecycleTransition = "autoscaling:EC2_INSTANCE_LAUNCHING"
-	// LifecycleTransitionInstanceTerminate is the terminating state of the EC2 instance.
-	LifecycleTransitionInstanceTerminate LifecycleTransition = "autoscaling:EC2_INSTANCE_TERMINATING"
+	// LifecycleHookTransitionInstanceLaunching is the launching state of the EC2 instance.
+	LifecycleHookTransitionInstanceLaunching LifecycleTransition = "autoscaling:EC2_INSTANCE_LAUNCHING"
+	// LifecycleHookTransitionInstanceTerminating is the terminating state of the EC2 instance.
+	LifecycleHookTransitionInstanceTerminating LifecycleTransition = "autoscaling:EC2_INSTANCE_TERMINATING"
 )
 
 func (l *LifecycleTransition) String() string {
 	return string(*l)
 }
 
-// DefaultResult is the default result for the lifecycle hook.
-type DefaultResult string
+// LifecycleHookDefaultResult is the default result for the lifecycle hook.
+type LifecycleHookDefaultResult string
 
 const (
-	// DefaultResultContinue is the default result for the lifecycle hook to continue.
-	DefaultResultContinue DefaultResult = "CONTINUE"
-	// DefaultResultAbandon is the default result for the lifecycle hook to abandon.
-	DefaultResultAbandon DefaultResult = "ABANDON"
+	// LifecycleHookDefaultResultContinue is the default result for the lifecycle hook to continue.
+	LifecycleHookDefaultResultContinue LifecycleHookDefaultResult = "CONTINUE"
+	// LifecycleHookDefaultResultAbandon is the default result for the lifecycle hook to abandon.
+	LifecycleHookDefaultResultAbandon LifecycleHookDefaultResult = "ABANDON"
 )
 
-func (d *DefaultResult) String() string {
+func (d *LifecycleHookDefaultResult) String() string {
 	return string(*d)
 }
 

exp/api/v1beta2/validation.go

@@ -0,0 +1,50 @@
+/*
+Copyright 2024 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+	http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1beta2
+
+import (
+	"fmt"
+
+	"k8s.io/apimachinery/pkg/util/validation/field"
+)
+
+func validateLifecycleHooks(hooks []AWSLifecycleHook) field.ErrorList {
+	var allErrs field.ErrorList
+
+	for _, hook := range hooks {
+		if hook.Name == "" {
+			allErrs = append(allErrs, field.Required(field.NewPath("spec.lifecycleHooks.name"), "Name is required"))
+		}
+		if hook.NotificationTargetARN != nil && hook.RoleARN == nil {
+			allErrs = append(allErrs, field.Required(field.NewPath("spec.lifecycleHooks.roleARN"), "RoleARN is required if NotificationTargetARN is provided"))
+		}
+		if hook.RoleARN != nil && hook.NotificationTargetARN == nil {
+			allErrs = append(allErrs, field.Required(field.NewPath("spec.lifecycleHooks.notificationTargetARN"), "NotificationTargetARN is required if RoleARN is provided"))
+		}
+		if hook.LifecycleTransition != LifecycleHookTransitionInstanceLaunching && hook.LifecycleTransition != LifecycleHookTransitionInstanceTerminating {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("spec.lifecycleHooks.lifecycleTransition"), hook.LifecycleTransition, fmt.Sprintf("LifecycleTransition must be either %q or %q", LifecycleHookTransitionInstanceLaunching, LifecycleHookTransitionInstanceTerminating)))
+		}
+		if hook.DefaultResult != nil && (*hook.DefaultResult != LifecycleHookDefaultResultContinue && *hook.DefaultResult != LifecycleHookDefaultResultAbandon) {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("spec.lifecycleHooks.defaultResult"), *hook.DefaultResult, fmt.Sprintf("DefaultResult must be either %s or %s", LifecycleHookDefaultResultContinue, LifecycleHookDefaultResultAbandon)))
+		}
+		if hook.HeartbeatTimeout != nil && (hook.HeartbeatTimeout.Seconds() < float64(30) || hook.HeartbeatTimeout.Seconds() > float64(172800)) {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("spec.lifecycleHooks.heartbeatTimeout"), *hook.HeartbeatTimeout, "HeartbeatTimeout must be between 30 and 172800 seconds"))
+		}
+	}
+
+	return allErrs
+}

exp/api/v1beta2/zz_generated.deepcopy.go

@@ -298,8 +298,8 @@ func (r *AWSMachinePoolReconciler) reconcileNormal(ctx context.Context, machineP
 		return nil
 	}
 
-	if err := r.ReconcileLifecycleHooks(*machinePoolScope, asgsvc); err != nil {
-		r.Recorder.Eventf(machinePoolScope.AWSMachinePool, corev1.EventTypeWarning, "FaileLifecycleHooksReconcile", "Failed to reconcile lifecycle hooks: %v", err)
+	if err := r.reconcileLifecycleHooks(machinePoolScope, asgsvc); err != nil {
+		r.Recorder.Eventf(machinePoolScope.AWSMachinePool, corev1.EventTypeWarning, "FailedLifecycleHooksReconcile", "Failed to reconcile lifecycle hooks: %v", err)
 		return errors.Wrap(err, "failed to reconcile lifecycle hooks")
 	}
 
@@ -612,70 +612,11 @@ func machinePoolToInfrastructureMapFunc(gvk schema.GroupVersionKind) handler.Map
 	}
 }
 
-// ReconcileLifecycleHooks periodically reconciles a lifecycle hook for the ASG.
-func (r *AWSMachinePoolReconciler) ReconcileLifecycleHooks(scope scope.MachinePoolScope, asgsvc services.ASGInterface) error {
-	lifecyleHooks := scope.GetLifecycleHooks()
-	for i := range lifecyleHooks {
-		if err := r.reconcileLifecycleHook(scope, &lifecyleHooks[i], asgsvc); err != nil {
-			return err
-		}
-	}
-
-	// Get a list of lifecycle hooks that are registered with the ASG but not defined in the MachinePool and delete them.
-	hooks, err := asgsvc.DescribeLifecycleHooks(scope.Name())
-	if err != nil {
-		return err
-	}
-	for _, hook := range hooks {
-		found := false
-		for _, definedHook := range scope.GetLifecycleHooks() {
-			if hook.Name == definedHook.Name {
-				found = true
-				break
-			}
-		}
-		if !found {
-			scope.Info("Deleting lifecycle hook", "hook", hook.Name)
-			if err := asgsvc.DeleteLifecycleHook(scope.Name(), hook); err != nil {
-				conditions.MarkFalse(scope.GetMachinePool(), expinfrav1.LifecycleHookReadyCondition, expinfrav1.LifecycleHookDeletionFailedReason, clusterv1.ConditionSeverityError, err.Error())
-				return err
-			}
-		}
-	}
-
-	return nil
-}
-
-func (r *AWSMachinePoolReconciler) reconcileLifecycleHook(scope scope.MachinePoolScope, hook *expinfrav1.AWSLifecycleHook, asgsvc services.ASGInterface) error {
-	scope.Info("Checking for existing lifecycle hook")
-	existingHook, err := asgsvc.DescribeLifecycleHook(scope.Name(), hook)
-	if err != nil {
-		conditions.MarkUnknown(scope.GetMachinePool(), expinfrav1.LifecycleHookReadyCondition, expinfrav1.LifecycleHookNotFoundReason, err.Error())
-		return err
-	}
-
-	if existingHook == nil {
-		scope.Info("Creating lifecycle hook")
-		if err := asgsvc.CreateLifecycleHook(scope.Name(), hook); err != nil {
-			conditions.MarkFalse(scope.GetMachinePool(), expinfrav1.LifecycleHookReadyCondition, expinfrav1.LifecycleHookCreationFailedReason, clusterv1.ConditionSeverityError, err.Error())
-			return err
-		}
-		return nil
-	}
-
-	// If the lifecycle hook exists, we need to check if it's up to date
-	needsUpdate := asgsvc.LifecycleHookNeedsUpdate(existingHook, hook)
-
-	if needsUpdate {
-		scope.Info("Updating lifecycle hook")
-		if err := asgsvc.UpdateLifecycleHook(scope.Name(), hook); err != nil {
-			conditions.MarkFalse(scope.GetMachinePool(), expinfrav1.LifecycleHookReadyCondition, expinfrav1.LifecycleHookUpdateFailedReason, clusterv1.ConditionSeverityError, err.Error())
-			return err
-		}
-	}
+// reconcileLifecycleHooks periodically reconciles a lifecycle hook for the ASG.
+func (r *AWSMachinePoolReconciler) reconcileLifecycleHooks(machinePoolScope *scope.MachinePoolScope, asgsvc services.ASGInterface) error {
+	asgName := machinePoolScope.Name()
 
-	conditions.MarkTrue(scope.GetMachinePool(), expinfrav1.LifecycleHookReadyCondition)
-	return nil
+	return asg.ReconcileLifecycleHooks(asgsvc, asgName, machinePoolScope.GetLifecycleHooks(), map[string]bool{}, machinePoolScope.GetMachinePool(), machinePoolScope)
 }
 
 func (r *AWSMachinePoolReconciler) getInfraCluster(ctx context.Context, log *logger.Logger, cluster *clusterv1.Cluster, awsMachinePool *expinfrav1.AWSMachinePool) (scope.EC2Scope, error) {

exp/controllers/awsmachinepool_controller.go

@@ -545,7 +545,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 						MixedInstancesPolicy: awsMachinePool.Spec.MixedInstancesPolicy.DeepCopy(),
 					}, nil
 				})
-				asgSvc.EXPECT().DescribeLifecycleHooks(gomock.Any()).Return(nil, nil).AnyTimes()
+				asgSvc.EXPECT().DescribeLifecycleHooks(gomock.Any()).Return(nil, nil)
 				asgSvc.EXPECT().SubnetIDs(gomock.Any()).Return([]string{"subnet-1"}, nil) // no change
 				// No changes, so there must not be an ASG update!
 				asgSvc.EXPECT().UpdateASG(gomock.Any()).Times(0)
@@ -825,8 +825,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 			ms.AWSMachinePool.Spec.AWSLifecycleHooks = append(ms.AWSMachinePool.Spec.AWSLifecycleHooks, newLifecycleHook)
 
 			reconSvc.EXPECT().ReconcileLaunchTemplate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
-			asgSvc.EXPECT().DescribeLifecycleHooks(ms.Name()).Return(nil, nil)
-			asgSvc.EXPECT().DescribeLifecycleHook(ms.Name(), &newLifecycleHook).Return(nil, nil)
+			asgSvc.EXPECT().DescribeLifecycleHooks(gomock.Eq(ms.Name())).Return(nil, nil)
 			asgSvc.EXPECT().CreateLifecycleHook(ms.Name(), &newLifecycleHook).Return(nil)
 			reconSvc.EXPECT().ReconcileTags(gomock.Any(), gomock.Any()).Return(nil)
 			asgSvc.EXPECT().GetASGByName(gomock.Any()).DoAndReturn(func(scope *scope.MachinePoolScope) (*expinfrav1.AutoScalingGroup, error) {
@@ -843,7 +842,6 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 					MixedInstancesPolicy: awsMachinePool.Spec.MixedInstancesPolicy.DeepCopy(),
 				}, nil
 			})
-			asgSvc.EXPECT().DescribeLifecycleHooks(gomock.Any()).Return(nil, nil).AnyTimes()
 			asgSvc.EXPECT().SubnetIDs(gomock.Any()).Return([]string{"subnet-1"}, nil) // no change
 			// No changes, so there must not be an ASG update!
 			asgSvc.EXPECT().UpdateASG(gomock.Any()).Times(0)
@@ -857,7 +855,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 			defer teardown(t, g)
 
 			reconSvc.EXPECT().ReconcileLaunchTemplate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
-			asgSvc.EXPECT().DescribeLifecycleHooks(ms.Name()).Return([]*expinfrav1.AWSLifecycleHook{
+			asgSvc.EXPECT().DescribeLifecycleHooks(gomock.Eq(ms.Name())).Return([]*expinfrav1.AWSLifecycleHook{
 				{
 					Name:                "hook-to-remove",
 					LifecycleTransition: "autoscaling:EC2_INSTANCE_LAUNCHING",
@@ -882,7 +880,6 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 					MixedInstancesPolicy: awsMachinePool.Spec.MixedInstancesPolicy.DeepCopy(),
 				}, nil
 			})
-			asgSvc.EXPECT().DescribeLifecycleHooks(gomock.Any()).Return(nil, nil).AnyTimes()
 			asgSvc.EXPECT().SubnetIDs(gomock.Any()).Return([]string{"subnet-1"}, nil) // no change
 			// No changes, so there must not be an ASG update!
 			asgSvc.EXPECT().UpdateASG(gomock.Any()).Times(0)
@@ -901,18 +898,13 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 			ms.AWSMachinePool.Spec.AWSLifecycleHooks = append(ms.AWSMachinePool.Spec.AWSLifecycleHooks, newLifecycleHook)
 
 			reconSvc.EXPECT().ReconcileLaunchTemplate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
-			asgSvc.EXPECT().DescribeLifecycleHook(ms.Name(), &newLifecycleHook).Return(nil, nil)
-			asgSvc.EXPECT().CreateLifecycleHook(ms.Name(), &newLifecycleHook).Return(nil)
-			asgSvc.EXPECT().DescribeLifecycleHooks(ms.Name()).Return([]*expinfrav1.AWSLifecycleHook{
-				{
-					Name:                "new-hook",
-					LifecycleTransition: "autoscaling:EC2_INSTANCE_LAUNCHING",
-				},
+			asgSvc.EXPECT().DescribeLifecycleHooks(gomock.Eq(ms.Name())).Return([]*expinfrav1.AWSLifecycleHook{
 				{
 					Name:                "hook-to-remove",
 					LifecycleTransition: "autoscaling:EC2_INSTANCE_LAUNCHING",
 				},
 			}, nil)
+			asgSvc.EXPECT().CreateLifecycleHook(ms.Name(), &newLifecycleHook).Return(nil)
 			asgSvc.EXPECT().DeleteLifecycleHook(ms.Name(), &expinfrav1.AWSLifecycleHook{
 				Name:                "hook-to-remove",
 				LifecycleTransition: "autoscaling:EC2_INSTANCE_LAUNCHING",
@@ -932,7 +924,6 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 					MixedInstancesPolicy: awsMachinePool.Spec.MixedInstancesPolicy.DeepCopy(),
 				}, nil
 			})
-			asgSvc.EXPECT().DescribeLifecycleHooks(gomock.Any()).Return(nil, nil).AnyTimes()
 			asgSvc.EXPECT().SubnetIDs(gomock.Any()).Return([]string{"subnet-1"}, nil) // no change
 			// No changes, so there must not be an ASG update!
 			asgSvc.EXPECT().UpdateASG(gomock.Any()).Times(0)
@@ -951,21 +942,13 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 			ms.AWSMachinePool.Spec.AWSLifecycleHooks = append(ms.AWSMachinePool.Spec.AWSLifecycleHooks, updateLifecycleHook)
 
 			reconSvc.EXPECT().ReconcileLaunchTemplate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
-			asgSvc.EXPECT().DescribeLifecycleHook(ms.Name(), &updateLifecycleHook).Return(&expinfrav1.AWSLifecycleHook{
-				Name:                "hook-to-update",
-				LifecycleTransition: "autoscaling:EC2_INSTANCE_LAUNCHING",
-			}, nil)
-			asgSvc.EXPECT().LifecycleHookNeedsUpdate(&expinfrav1.AWSLifecycleHook{
-				Name:                "hook-to-update",
-				LifecycleTransition: "autoscaling:EC2_INSTANCE_LAUNCHING",
-			}, &updateLifecycleHook).Return(true)
-			asgSvc.EXPECT().UpdateLifecycleHook(ms.Name(), &updateLifecycleHook).Return(nil)
-			asgSvc.EXPECT().DescribeLifecycleHooks(ms.Name()).Return([]*expinfrav1.AWSLifecycleHook{
+			asgSvc.EXPECT().DescribeLifecycleHooks(gomock.Eq(ms.Name())).Return([]*expinfrav1.AWSLifecycleHook{
 				{
 					Name:                "hook-to-update",
 					LifecycleTransition: "autoscaling:EC2_INSTANCE_LAUNCHING",
 				},
 			}, nil)
+			asgSvc.EXPECT().UpdateLifecycleHook(ms.Name(), &updateLifecycleHook).Return(nil)
 			reconSvc.EXPECT().ReconcileTags(gomock.Any(), gomock.Any()).Return(nil)
 			asgSvc.EXPECT().GetASGByName(gomock.Any()).DoAndReturn(func(scope *scope.MachinePoolScope) (*expinfrav1.AutoScalingGroup, error) {
 				g.Expect(scope.Name()).To(Equal("test"))
@@ -981,7 +964,6 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 					MixedInstancesPolicy: awsMachinePool.Spec.MixedInstancesPolicy.DeepCopy(),
 				}, nil
 			})
-			asgSvc.EXPECT().DescribeLifecycleHooks(gomock.Any()).Return(nil, nil).AnyTimes()
 			asgSvc.EXPECT().SubnetIDs(gomock.Any()).Return([]string{"subnet-1"}, nil) // no change
 			// No changes, so there must not be an ASG update!
 			asgSvc.EXPECT().UpdateASG(gomock.Any()).Times(0)
@@ -992,7 +974,6 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 	})
 }
 
-// TODO: This was taken from awsmachine_controller_test, i think it should be moved to elsewhere in both locations like test/helpers.
 type conditionAssertion struct {
 	conditionType clusterv1.ConditionType
 	status        corev1.ConditionStatus