From 30c0b66f1555af0f3f78b2c4772204fbba26d173 Mon Sep 17 00:00:00 2001 From: Sebastien Michel Date: Thu, 4 Jul 2024 21:11:36 +0200 Subject: [PATCH 01/13] feat: custom lifecyclehooks for machinepools Taken and rebased from unfinished PR https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/4875 at commit 2421ec37f8b5e7805b230d947810c1d8512465cc --- ...ture.cluster.x-k8s.io_awsmachinepools.yaml | 49 +++++ ...uster.x-k8s.io_awsmanagedmachinepools.yaml | 49 +++++ exp/api/v1beta1/conversion.go | 6 + exp/api/v1beta1/zz_generated.conversion.go | 2 + exp/api/v1beta2/awsmachinepool_types.go | 4 + exp/api/v1beta2/awsmachinepool_webhook.go | 35 ++- .../v1beta2/awsmachinepool_webhook_test.go | 28 +++ .../v1beta2/awsmanagedmachinepool_types.go | 4 + .../v1beta2/awsmanagedmachinepool_webhook.go | 10 + exp/api/v1beta2/conditions_consts.go | 11 + exp/api/v1beta2/types.go | 65 ++++++ exp/api/v1beta2/zz_generated.deepcopy.go | 54 +++++ exp/controllers/awsmachinepool_controller.go | 71 ++++++ .../awsmachinepool_controller_test.go | 203 +++++++++++++++++- pkg/cloud/scope/machinepool.go | 4 + pkg/cloud/scope/managednodegroup.go | 4 + .../services/autoscaling/autoscalinggroup.go | 2 +- .../services/autoscaling/lifecyclehook.go | 185 ++++++++++++++++ pkg/cloud/services/eks/nodegroup.go | 91 +++++++- pkg/cloud/services/eks/service.go | 4 + pkg/cloud/services/interfaces.go | 6 + .../autoscaling_interface_mock.go | 86 ++++++++ 22 files changed, 964 insertions(+), 9 deletions(-) create mode 100644 pkg/cloud/services/autoscaling/lifecyclehook.go diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachinepools.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachinepools.yaml index 6314fe7c62..8bcbaed4db 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachinepools.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmachinepools.yaml @@ -872,6 +872,55 @@ spec: after it enters the InService state. If no value is supplied by user a default value of 300 seconds is set type: string + lifecycleHooks: + description: AWSLifecycleHooks specifies lifecycle hooks for the autoscaling + group. + items: + description: AWSLifecycleHook describes an AWS lifecycle hook + properties: + defaultResult: + description: The default result for the lifecycle hook. The + possible values are CONTINUE and ABANDON. + enum: + - CONTINUE + - ABANDON + type: string + heartbeatTimeout: + description: |- + The maximum time, in seconds, that an instance can remain in a Pending:Wait or + Terminating:Wait state. The maximum is 172800 seconds (48 hours) or 100 times + HeartbeatTimeout, whichever is smaller. + format: duration + type: string + lifecycleTransition: + description: The state of the EC2 instance to which to attach + the lifecycle hook. + enum: + - autoscaling:EC2_INSTANCE_LAUNCHING + - autoscaling:EC2_INSTANCE_TERMINATING + type: string + name: + description: The name of the lifecycle hook. + type: string + notificationMetadata: + description: Contains additional metadata that will be passed + to the notification target. + type: string + notificationTargetARN: + description: |- + The ARN of the notification target that Amazon EC2 Auto Scaling uses to + notify you when an instance is in the transition state for the lifecycle hook. + type: string + roleARN: + description: |- + The ARN of the IAM role that allows the Auto Scaling group to publish to the + specified notification target. + type: string + required: + - lifecycleTransition + - name + type: object + type: array maxSize: default: 1 description: MaxSize defines the maximum size of the group. diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmanagedmachinepools.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmanagedmachinepools.yaml index 9ecac67715..12627475fe 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmanagedmachinepools.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsmanagedmachinepools.yaml @@ -879,6 +879,55 @@ spec: type: string description: Labels specifies labels for the Kubernetes node objects type: object + lifecycleHooks: + description: AWSLifecycleHooks specifies lifecycle hooks for the managed + node group. + items: + description: AWSLifecycleHook describes an AWS lifecycle hook + properties: + defaultResult: + description: The default result for the lifecycle hook. The + possible values are CONTINUE and ABANDON. + enum: + - CONTINUE + - ABANDON + type: string + heartbeatTimeout: + description: |- + The maximum time, in seconds, that an instance can remain in a Pending:Wait or + Terminating:Wait state. The maximum is 172800 seconds (48 hours) or 100 times + HeartbeatTimeout, whichever is smaller. + format: duration + type: string + lifecycleTransition: + description: The state of the EC2 instance to which to attach + the lifecycle hook. + enum: + - autoscaling:EC2_INSTANCE_LAUNCHING + - autoscaling:EC2_INSTANCE_TERMINATING + type: string + name: + description: The name of the lifecycle hook. + type: string + notificationMetadata: + description: Contains additional metadata that will be passed + to the notification target. + type: string + notificationTargetARN: + description: |- + The ARN of the notification target that Amazon EC2 Auto Scaling uses to + notify you when an instance is in the transition state for the lifecycle hook. + type: string + roleARN: + description: |- + The ARN of the IAM role that allows the Auto Scaling group to publish to the + specified notification target. + type: string + required: + - lifecycleTransition + - name + type: object + type: array providerIDList: description: |- ProviderIDList are the provider IDs of instances in the diff --git a/exp/api/v1beta1/conversion.go b/exp/api/v1beta1/conversion.go index 7c39f1fcbd..194ba103ba 100644 --- a/exp/api/v1beta1/conversion.go +++ b/exp/api/v1beta1/conversion.go @@ -52,6 +52,9 @@ func (src *AWSMachinePool) ConvertTo(dstRaw conversion.Hub) error { if restored.Spec.AvailabilityZoneSubnetType != nil { dst.Spec.AvailabilityZoneSubnetType = restored.Spec.AvailabilityZoneSubnetType } + if restored.Spec.AWSLifecycleHooks != nil { + dst.Spec.AWSLifecycleHooks = restored.Spec.AWSLifecycleHooks + } if restored.Spec.AWSLaunchTemplate.PrivateDNSName != nil { dst.Spec.AWSLaunchTemplate.PrivateDNSName = restored.Spec.AWSLaunchTemplate.PrivateDNSName @@ -113,6 +116,9 @@ func (src *AWSManagedMachinePool) ConvertTo(dstRaw conversion.Hub) error { if restored.Spec.AvailabilityZoneSubnetType != nil { dst.Spec.AvailabilityZoneSubnetType = restored.Spec.AvailabilityZoneSubnetType } + if restored.Spec.AWSLifecycleHooks != nil { + dst.Spec.AWSLifecycleHooks = restored.Spec.AWSLifecycleHooks + } return nil } diff --git a/exp/api/v1beta1/zz_generated.conversion.go b/exp/api/v1beta1/zz_generated.conversion.go index 585cbd1504..153b4a55b8 100644 --- a/exp/api/v1beta1/zz_generated.conversion.go +++ b/exp/api/v1beta1/zz_generated.conversion.go @@ -565,6 +565,7 @@ func autoConvert_v1beta2_AWSMachinePoolSpec_To_v1beta1_AWSMachinePoolSpec(in *v1 } out.CapacityRebalance = in.CapacityRebalance // WARNING: in.SuspendProcesses requires manual conversion: does not exist in peer-type + // WARNING: in.AWSLifecycleHooks requires manual conversion: does not exist in peer-type return nil } @@ -741,6 +742,7 @@ func autoConvert_v1beta2_AWSManagedMachinePoolSpec_To_v1beta1_AWSManagedMachineP } else { out.AWSLaunchTemplate = nil } + // WARNING: in.AWSLifecycleHooks requires manual conversion: does not exist in peer-type return nil } diff --git a/exp/api/v1beta2/awsmachinepool_types.go b/exp/api/v1beta2/awsmachinepool_types.go index 526876bcfd..d848d816df 100644 --- a/exp/api/v1beta2/awsmachinepool_types.go +++ b/exp/api/v1beta2/awsmachinepool_types.go @@ -101,6 +101,10 @@ type AWSMachinePoolSpec struct { // SuspendProcesses defines a list of processes to suspend for the given ASG. This is constantly reconciled. // If a process is removed from this list it will automatically be resumed. SuspendProcesses *SuspendProcessesTypes `json:"suspendProcesses,omitempty"` + + // AWSLifecycleHooks specifies lifecycle hooks for the autoscaling group. + // +optional + AWSLifecycleHooks []AWSLifecycleHook `json:"lifecycleHooks,omitempty"` } // SuspendProcessesTypes contains user friendly auto-completable values for suspended process names. diff --git a/exp/api/v1beta2/awsmachinepool_webhook.go b/exp/api/v1beta2/awsmachinepool_webhook.go index a4f6a44d41..c8942794c0 100644 --- a/exp/api/v1beta2/awsmachinepool_webhook.go +++ b/exp/api/v1beta2/awsmachinepool_webhook.go @@ -17,6 +17,7 @@ limitations under the License. package v1beta2 import ( + "fmt" "time" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -133,7 +134,6 @@ func (r *AWSMachinePool) validateAdditionalSecurityGroups() field.ErrorList { } return allErrs } - func (r *AWSMachinePool) validateSpotInstances() field.ErrorList { var allErrs field.ErrorList if r.Spec.AWSLaunchTemplate.SpotMarketOptions != nil && r.Spec.MixedInstancesPolicy != nil { @@ -162,6 +162,37 @@ func (r *AWSMachinePool) validateRefreshPreferences() field.ErrorList { return allErrs } +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)) @@ -176,6 +207,7 @@ func (r *AWSMachinePool) ValidateCreate() (admission.Warnings, error) { allErrs = append(allErrs, r.validateAdditionalSecurityGroups()...) allErrs = append(allErrs, r.validateSpotInstances()...) allErrs = append(allErrs, r.validateRefreshPreferences()...) + allErrs = append(allErrs, r.validateLifecycleHooks()...) if len(allErrs) == 0 { return nil, nil @@ -198,6 +230,7 @@ func (r *AWSMachinePool) ValidateUpdate(_ runtime.Object) (admission.Warnings, e allErrs = append(allErrs, r.validateAdditionalSecurityGroups()...) allErrs = append(allErrs, r.validateSpotInstances()...) allErrs = append(allErrs, r.validateRefreshPreferences()...) + allErrs = append(allErrs, r.validateLifecycleHooks()...) if len(allErrs) == 0 { return nil, nil diff --git a/exp/api/v1beta2/awsmachinepool_webhook_test.go b/exp/api/v1beta2/awsmachinepool_webhook_test.go index 0f14ad1c0a..fa1985f653 100644 --- a/exp/api/v1beta2/awsmachinepool_webhook_test.go +++ b/exp/api/v1beta2/awsmachinepool_webhook_test.go @@ -19,6 +19,7 @@ package v1beta2 import ( "strings" "testing" + "time" "github.com/aws/aws-sdk-go/aws" . "github.com/onsi/gomega" @@ -174,6 +175,33 @@ func TestAWSMachinePoolValidateCreate(t *testing.T) { }, wantErr: true, }, + { + name: "Should fail if either roleARN or notifcationARN is set but not both", + pool: &AWSMachinePool{ + Spec: AWSMachinePoolSpec{ + AWSLifecycleHooks: []AWSLifecycleHook{ + { + RoleARN: aws.String("role-arn"), + }, + }, + }, + }, + wantErr: true, + }, + { + name: "Should fail if the heartbeat timeout is less than 30 seconds", + pool: &AWSMachinePool{ + Spec: AWSMachinePoolSpec{ + AWSLifecycleHooks: []AWSLifecycleHook{ + { + RoleARN: aws.String("role-arn"), + HeartbeatTimeout: &metav1.Duration{Duration: 29 * time.Second}, + }, + }, + }, + }, + wantErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/exp/api/v1beta2/awsmanagedmachinepool_types.go b/exp/api/v1beta2/awsmanagedmachinepool_types.go index c7e70fcf55..62bbcbf69b 100644 --- a/exp/api/v1beta2/awsmanagedmachinepool_types.go +++ b/exp/api/v1beta2/awsmanagedmachinepool_types.go @@ -159,6 +159,10 @@ type AWSManagedMachinePoolSpec struct { // are prohibited (https://docs.aws.amazon.com/eks/latest/userguide/launch-templates.html). // +optional AWSLaunchTemplate *AWSLaunchTemplate `json:"awsLaunchTemplate,omitempty"` + + // AWSLifecycleHooks specifies lifecycle hooks for the managed node group. + // +optional + AWSLifecycleHooks []AWSLifecycleHook `json:"lifecycleHooks,omitempty"` } // ManagedMachinePoolScaling specifies scaling options. diff --git a/exp/api/v1beta2/awsmanagedmachinepool_webhook.go b/exp/api/v1beta2/awsmanagedmachinepool_webhook.go index effd87a2d1..aaa883750f 100644 --- a/exp/api/v1beta2/awsmanagedmachinepool_webhook.go +++ b/exp/api/v1beta2/awsmanagedmachinepool_webhook.go @@ -138,6 +138,10 @@ func (r *AWSManagedMachinePool) validateLaunchTemplate() field.ErrorList { return allErrs } +func (r *AWSManagedMachinePool) validateLifecycleHooks() field.ErrorList { + return validateLifecycleHooks(r.Spec.AWSLifecycleHooks) +} + // ValidateCreate will do any extra validation when creating a AWSManagedMachinePool. func (r *AWSManagedMachinePool) ValidateCreate() (admission.Warnings, error) { mmpLog.Info("AWSManagedMachinePool validate create", "managed-machine-pool", klog.KObj(r)) @@ -159,6 +163,9 @@ func (r *AWSManagedMachinePool) ValidateCreate() (admission.Warnings, error) { if errs := r.validateLaunchTemplate(); len(errs) > 0 { allErrs = append(allErrs, errs...) } + if errs := r.validateLifecycleHooks(); len(errs) > 0 { + allErrs = append(allErrs, errs...) + } allErrs = append(allErrs, r.Spec.AdditionalTags.Validate()...) @@ -196,6 +203,9 @@ func (r *AWSManagedMachinePool) ValidateUpdate(old runtime.Object) (admission.Wa if errs := r.validateLaunchTemplate(); len(errs) > 0 { allErrs = append(allErrs, errs...) } + if errs := r.validateLifecycleHooks(); len(errs) > 0 { + allErrs = append(allErrs, errs...) + } if len(allErrs) == 0 { return nil, nil diff --git a/exp/api/v1beta2/conditions_consts.go b/exp/api/v1beta2/conditions_consts.go index 2d052fae53..3c56c1b853 100644 --- a/exp/api/v1beta2/conditions_consts.go +++ b/exp/api/v1beta2/conditions_consts.go @@ -54,6 +54,17 @@ const ( InstanceRefreshNotReadyReason = "InstanceRefreshNotReady" // InstanceRefreshFailedReason used to report when there instance refresh is not initiated. InstanceRefreshFailedReason = "InstanceRefreshFailed" + + // 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. + LifecycleHookUpdateFailedReason = "LifecycleHookUpdateFailed" + // LifecycleHookDeletionFailedReason used for failures during lifecycle hook deletion. + LifecycleHookDeletionFailedReason = "LifecycleHookDeletionFailed" ) const ( diff --git a/exp/api/v1beta2/types.go b/exp/api/v1beta2/types.go index 0bc4009a2e..6b4f766e54 100644 --- a/exp/api/v1beta2/types.go +++ b/exp/api/v1beta2/types.go @@ -221,6 +221,71 @@ type AutoScalingGroup struct { CurrentlySuspendProcesses []string `json:"currentlySuspendProcesses,omitempty"` } +// AWSLifecycleHook describes an AWS lifecycle hook +type AWSLifecycleHook struct { + // The name of the lifecycle hook. + Name string `json:"name"` + + // The ARN of the notification target that Amazon EC2 Auto Scaling uses to + // notify you when an instance is in the transition state for the lifecycle hook. + // +optional + NotificationTargetARN *string `json:"notificationTargetARN,omitempty"` + + // The ARN of the IAM role that allows the Auto Scaling group to publish to the + // specified notification target. + // +optional + RoleARN *string `json:"roleARN,omitempty"` + + // The state of the EC2 instance to which to attach the lifecycle hook. + // +kubebuilder:validation:Enum="autoscaling:EC2_INSTANCE_LAUNCHING";"autoscaling:EC2_INSTANCE_TERMINATING" + LifecycleTransition LifecycleTransition `json:"lifecycleTransition"` + + // The maximum time, in seconds, that an instance can remain in a Pending:Wait or + // Terminating:Wait state. The maximum is 172800 seconds (48 hours) or 100 times + // HeartbeatTimeout, whichever is smaller. + // +optional + // +kubebuilder:validation:Format=duration + HeartbeatTimeout *metav1.Duration `json:"heartbeatTimeout,omitempty"` + + // The default result for the lifecycle hook. The possible values are CONTINUE and ABANDON. + // +optional + // +kubebuilder:validation:Enum=CONTINUE;ABANDON + // +kubebuilder:validation:default:=none + DefaultResult *DefaultResult `json:"defaultResult,omitempty"` + + // Contains additional metadata that will be passed to the notification target. + // +optional + NotificationMetadata *string `json:"notificationMetadata,omitempty"` +} + +// LifecycleTransition is the state of the EC2 instance to which to attach the lifecycle hook. +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" +) + +func (l *LifecycleTransition) String() string { + return string(*l) +} + +// DefaultResult is the default result for the lifecycle hook. +type DefaultResult 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" +) + +func (d *DefaultResult) String() string { + return string(*d) +} + // ASGStatus is a status string returned by the autoscaling API. type ASGStatus string diff --git a/exp/api/v1beta2/zz_generated.deepcopy.go b/exp/api/v1beta2/zz_generated.deepcopy.go index c8131307ab..e82ce2f3f4 100644 --- a/exp/api/v1beta2/zz_generated.deepcopy.go +++ b/exp/api/v1beta2/zz_generated.deepcopy.go @@ -148,6 +148,46 @@ func (in *AWSLaunchTemplate) DeepCopy() *AWSLaunchTemplate { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *AWSLifecycleHook) DeepCopyInto(out *AWSLifecycleHook) { + *out = *in + if in.NotificationTargetARN != nil { + in, out := &in.NotificationTargetARN, &out.NotificationTargetARN + *out = new(string) + **out = **in + } + if in.RoleARN != nil { + in, out := &in.RoleARN, &out.RoleARN + *out = new(string) + **out = **in + } + if in.HeartbeatTimeout != nil { + in, out := &in.HeartbeatTimeout, &out.HeartbeatTimeout + *out = new(v1.Duration) + **out = **in + } + if in.DefaultResult != nil { + in, out := &in.DefaultResult, &out.DefaultResult + *out = new(DefaultResult) + **out = **in + } + if in.NotificationMetadata != nil { + in, out := &in.NotificationMetadata, &out.NotificationMetadata + *out = new(string) + **out = **in + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AWSLifecycleHook. +func (in *AWSLifecycleHook) DeepCopy() *AWSLifecycleHook { + if in == nil { + return nil + } + out := new(AWSLifecycleHook) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *AWSMachinePool) DeepCopyInto(out *AWSMachinePool) { *out = *in @@ -277,6 +317,13 @@ func (in *AWSMachinePoolSpec) DeepCopyInto(out *AWSMachinePoolSpec) { *out = new(SuspendProcessesTypes) (*in).DeepCopyInto(*out) } + if in.AWSLifecycleHooks != nil { + in, out := &in.AWSLifecycleHooks, &out.AWSLifecycleHooks + *out = make([]AWSLifecycleHook, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AWSMachinePoolSpec. @@ -489,6 +536,13 @@ func (in *AWSManagedMachinePoolSpec) DeepCopyInto(out *AWSManagedMachinePoolSpec *out = new(AWSLaunchTemplate) (*in).DeepCopyInto(*out) } + if in.AWSLifecycleHooks != nil { + in, out := &in.AWSLifecycleHooks, &out.AWSLifecycleHooks + *out = make([]AWSLifecycleHook, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AWSManagedMachinePoolSpec. diff --git a/exp/controllers/awsmachinepool_controller.go b/exp/controllers/awsmachinepool_controller.go index 741cdcdb10..2ef08e60dc 100644 --- a/exp/controllers/awsmachinepool_controller.go +++ b/exp/controllers/awsmachinepool_controller.go @@ -298,6 +298,11 @@ 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) + return errors.Wrap(err, "failed to reconcile lifecycle hooks") + } + if annotations.ReplicasManagedByExternalAutoscaler(machinePoolScope.MachinePool) { // Set MachinePool replicas to the ASG DesiredCapacity if *machinePoolScope.MachinePool.Spec.Replicas != *asg.DesiredCapacity { @@ -607,6 +612,72 @@ 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 + } + } + + conditions.MarkTrue(scope.GetMachinePool(), expinfrav1.LifecycleHookReadyCondition) + return nil +} + func (r *AWSMachinePoolReconciler) getInfraCluster(ctx context.Context, log *logger.Logger, cluster *clusterv1.Cluster, awsMachinePool *expinfrav1.AWSMachinePool) (scope.EC2Scope, error) { var clusterScope *scope.ClusterScope var managedControlPlaneScope *scope.ManagedControlPlaneScope diff --git a/exp/controllers/awsmachinepool_controller_test.go b/exp/controllers/awsmachinepool_controller_test.go index 4902dbb7e7..748a6b5ff6 100644 --- a/exp/controllers/awsmachinepool_controller_test.go +++ b/exp/controllers/awsmachinepool_controller_test.go @@ -163,13 +163,14 @@ func TestAWSMachinePoolReconciler(t *testing.T) { recorder = record.NewFakeRecorder(2) reconciler = AWSMachinePoolReconciler{ + Client: testEnv.Client, ec2ServiceFactory: func(scope.EC2Scope) services.EC2Interface { return ec2Svc }, asgServiceFactory: func(cloud.ClusterScoper) services.ASGInterface { return asgSvc }, - reconcileServiceFactory: func(scope.EC2Scope) services.MachinePoolReconcileInterface { + reconcileServiceFactory: func(scope scope.EC2Scope) services.MachinePoolReconcileInterface { return reconSvc }, Recorder: recorder, @@ -323,6 +324,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { setSuspendedProcesses(t, g) ms.AWSMachinePool.Spec.SuspendProcesses.All = true reconSvc.EXPECT().ReconcileLaunchTemplate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + asgSvc.EXPECT().DescribeLifecycleHooks(gomock.Any()).Return(nil, nil) reconSvc.EXPECT().ReconcileTags(gomock.Any(), gomock.Any()).Return(nil) asgSvc.EXPECT().GetASGByName(gomock.Any()).Return(&expinfrav1.AutoScalingGroup{ Name: "name", @@ -363,6 +365,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { setSuspendedProcesses(t, g) reconSvc.EXPECT().ReconcileLaunchTemplate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + asgSvc.EXPECT().DescribeLifecycleHooks(gomock.Any()).Return(nil, nil) reconSvc.EXPECT().ReconcileTags(gomock.Any(), gomock.Any()).Return(nil) asgSvc.EXPECT().GetASGByName(gomock.Any()).Return(&expinfrav1.AutoScalingGroup{ Name: "name", @@ -388,6 +391,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { DesiredCapacity: ptr.To[int32](1), } reconSvc.EXPECT().ReconcileLaunchTemplate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + asgSvc.EXPECT().DescribeLifecycleHooks(gomock.Any()).Return(nil, nil) asgSvc.EXPECT().GetASGByName(gomock.Any()).Return(&asg, nil) asgSvc.EXPECT().SubnetIDs(gomock.Any()).Return([]string{}, nil) asgSvc.EXPECT().UpdateASG(gomock.Any()).Return(nil) @@ -424,8 +428,10 @@ func TestAWSMachinePoolReconciler(t *testing.T) { }, }, }, - Subnets: []string{"subnet1", "subnet2"}} + Subnets: []string{"subnet1", "subnet2"}, + } reconSvc.EXPECT().ReconcileLaunchTemplate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + asgSvc.EXPECT().DescribeLifecycleHooks(gomock.Any()).Return(nil, nil) reconSvc.EXPECT().ReconcileTags(gomock.Any(), gomock.Any()).Return(nil) asgSvc.EXPECT().GetASGByName(gomock.Any()).Return(&asg, nil).AnyTimes() asgSvc.EXPECT().SubnetIDs(gomock.Any()).Return([]string{"subnet2", "subnet1"}, nil).Times(1) @@ -442,8 +448,10 @@ func TestAWSMachinePoolReconciler(t *testing.T) { asg := expinfrav1.AutoScalingGroup{ MinSize: int32(0), MaxSize: int32(100), - Subnets: []string{"subnet1", "subnet2"}} + Subnets: []string{"subnet1", "subnet2"}, + } reconSvc.EXPECT().ReconcileLaunchTemplate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + asgSvc.EXPECT().DescribeLifecycleHooks(gomock.Any()).Return(nil, nil) reconSvc.EXPECT().ReconcileTags(gomock.Any(), gomock.Any()).Return(nil) asgSvc.EXPECT().GetASGByName(gomock.Any()).Return(&asg, nil).AnyTimes() asgSvc.EXPECT().SubnetIDs(gomock.Any()).Return([]string{"subnet1"}, nil).Times(1) @@ -460,8 +468,10 @@ func TestAWSMachinePoolReconciler(t *testing.T) { asg := expinfrav1.AutoScalingGroup{ MinSize: int32(0), MaxSize: int32(2), - Subnets: []string{}} + Subnets: []string{}, + } reconSvc.EXPECT().ReconcileLaunchTemplate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + asgSvc.EXPECT().DescribeLifecycleHooks(gomock.Any()).Return(nil, nil) reconSvc.EXPECT().ReconcileTags(gomock.Any(), gomock.Any()).Return(nil) asgSvc.EXPECT().GetASGByName(gomock.Any()).Return(&asg, nil).AnyTimes() asgSvc.EXPECT().SubnetIDs(gomock.Any()).Return([]string{}, nil).Times(1) @@ -535,6 +545,7 @@ 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) @@ -588,6 +599,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { MixedInstancesPolicy: awsMachinePool.Spec.MixedInstancesPolicy.DeepCopy(), }, nil }) + asgSvc.EXPECT().DescribeLifecycleHooks(gomock.Any()) 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) @@ -644,6 +656,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { MixedInstancesPolicy: awsMachinePool.Spec.MixedInstancesPolicy.DeepCopy(), }, nil }) + asgSvc.EXPECT().DescribeLifecycleHooks(gomock.Any()) 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) @@ -728,6 +741,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { MixedInstancesPolicy: awsMachinePool.Spec.MixedInstancesPolicy.DeepCopy(), }, nil }) + asgSvc.EXPECT().DescribeLifecycleHooks(gomock.Any()) 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) @@ -798,10 +812,187 @@ func TestAWSMachinePoolReconciler(t *testing.T) { g.Eventually(recorder.Events).Should(Receive(ContainSubstring("DeletionInProgress"))) }) }) -} + t.Run("Lifecycle Hooks", func(t *testing.T) { + t.Run("New lifecycle hook is added", func(t *testing.T) { + g := NewWithT(t) + setup(t, g) + defer teardown(t, g) + + newLifecycleHook := expinfrav1.AWSLifecycleHook{ + Name: "new-hook", + LifecycleTransition: "autoscaling:EC2_INSTANCE_LAUNCHING", + } + 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().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) { + g.Expect(scope.Name()).To(Equal("test")) + + // No difference to `AWSMachinePool.spec` + return &expinfrav1.AutoScalingGroup{ + Name: scope.Name(), + Subnets: []string{ + "subnet-1", + }, + MinSize: awsMachinePool.Spec.MinSize, + MaxSize: awsMachinePool.Spec.MaxSize, + 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) -//TODO: This was taken from awsmachine_controller_test, i think it should be moved to elsewhere in both locations like test/helpers. + err := reconciler.reconcileNormal(context.Background(), ms, cs, cs) + g.Expect(err).To(Succeed()) + }) + t.Run("Lifecycle hook to remove", func(t *testing.T) { + g := NewWithT(t) + setup(t, g) + 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{ + { + Name: "hook-to-remove", + LifecycleTransition: "autoscaling:EC2_INSTANCE_LAUNCHING", + }, + }, nil) + asgSvc.EXPECT().DeleteLifecycleHook(ms.Name(), &expinfrav1.AWSLifecycleHook{ + Name: "hook-to-remove", + LifecycleTransition: "autoscaling:EC2_INSTANCE_LAUNCHING", + }).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")) + + // No difference to `AWSMachinePool.spec` + return &expinfrav1.AutoScalingGroup{ + Name: scope.Name(), + Subnets: []string{ + "subnet-1", + }, + MinSize: awsMachinePool.Spec.MinSize, + MaxSize: awsMachinePool.Spec.MaxSize, + 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) + + err := reconciler.reconcileNormal(context.Background(), ms, cs, cs) + g.Expect(err).To(Succeed()) + }) + t.Run("One to add, one to remove", func(t *testing.T) { + g := NewWithT(t) + setup(t, g) + defer teardown(t, g) + newLifecycleHook := expinfrav1.AWSLifecycleHook{ + Name: "new-hook", + LifecycleTransition: "autoscaling:EC2_INSTANCE_LAUNCHING", + } + 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", + }, + { + Name: "hook-to-remove", + LifecycleTransition: "autoscaling:EC2_INSTANCE_LAUNCHING", + }, + }, nil) + asgSvc.EXPECT().DeleteLifecycleHook(ms.Name(), &expinfrav1.AWSLifecycleHook{ + Name: "hook-to-remove", + LifecycleTransition: "autoscaling:EC2_INSTANCE_LAUNCHING", + }).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")) + + // No difference to `AWSMachinePool.spec` + return &expinfrav1.AutoScalingGroup{ + Name: scope.Name(), + Subnets: []string{ + "subnet-1", + }, + MinSize: awsMachinePool.Spec.MinSize, + MaxSize: awsMachinePool.Spec.MaxSize, + 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) + + err := reconciler.reconcileNormal(context.Background(), ms, cs, cs) + g.Expect(err).To(Succeed()) + }) + t.Run("Update hook", func(t *testing.T) { + g := NewWithT(t) + setup(t, g) + defer teardown(t, g) + updateLifecycleHook := expinfrav1.AWSLifecycleHook{ + Name: "hook-to-update", + LifecycleTransition: "autoscaling:EC2_INSTANCE_TERMINATING", + } + 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{ + { + Name: "hook-to-update", + LifecycleTransition: "autoscaling:EC2_INSTANCE_LAUNCHING", + }, + }, 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")) + + // No difference to `AWSMachinePool.spec` + return &expinfrav1.AutoScalingGroup{ + Name: scope.Name(), + Subnets: []string{ + "subnet-1", + }, + MinSize: awsMachinePool.Spec.MinSize, + MaxSize: awsMachinePool.Spec.MaxSize, + 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) + + err := reconciler.reconcileNormal(context.Background(), ms, cs, cs) + g.Expect(err).To(Succeed()) + }) + }) +} +// 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 diff --git a/pkg/cloud/scope/machinepool.go b/pkg/cloud/scope/machinepool.go index 00e8abeadc..8fe2d2cecb 100644 --- a/pkg/cloud/scope/machinepool.go +++ b/pkg/cloud/scope/machinepool.go @@ -395,3 +395,7 @@ func (m *MachinePoolScope) LaunchTemplateName() string { func (m *MachinePoolScope) GetRuntimeObject() runtime.Object { return m.AWSMachinePool } + +func (m *MachinePoolScope) GetLifecycleHooks() []expinfrav1.AWSLifecycleHook { + return m.AWSMachinePool.Spec.AWSLifecycleHooks +} diff --git a/pkg/cloud/scope/managednodegroup.go b/pkg/cloud/scope/managednodegroup.go index e9421d7282..ab7c2761f8 100644 --- a/pkg/cloud/scope/managednodegroup.go +++ b/pkg/cloud/scope/managednodegroup.go @@ -411,3 +411,7 @@ func (s *ManagedMachinePoolScope) LaunchTemplateName() string { func (s *ManagedMachinePoolScope) GetRuntimeObject() runtime.Object { return s.ManagedMachinePool } + +func (m *ManagedMachinePoolScope) GetLifecycleHooks() []expinfrav1.AWSLifecycleHook { + return m.ManagedMachinePool.Spec.AWSLifecycleHooks +} diff --git a/pkg/cloud/services/autoscaling/autoscalinggroup.go b/pkg/cloud/services/autoscaling/autoscalinggroup.go index f8e8b18690..e710fa768e 100644 --- a/pkg/cloud/services/autoscaling/autoscalinggroup.go +++ b/pkg/cloud/services/autoscaling/autoscalinggroup.go @@ -331,7 +331,7 @@ func (s *Service) CanStartASGInstanceRefresh(scope *scope.MachinePoolScope) (boo return false, err } hasUnfinishedRefresh := false - if err == nil && len(refreshes.InstanceRefreshes) != 0 { + if len(refreshes.InstanceRefreshes) != 0 { for i := range refreshes.InstanceRefreshes { if *refreshes.InstanceRefreshes[i].Status == autoscaling.InstanceRefreshStatusInProgress || *refreshes.InstanceRefreshes[i].Status == autoscaling.InstanceRefreshStatusPending || diff --git a/pkg/cloud/services/autoscaling/lifecyclehook.go b/pkg/cloud/services/autoscaling/lifecyclehook.go new file mode 100644 index 0000000000..59f51f998e --- /dev/null +++ b/pkg/cloud/services/autoscaling/lifecyclehook.go @@ -0,0 +1,185 @@ +/* +Copyright 2018 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 asg + +import ( + "context" + "time" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/autoscaling" + "github.com/pkg/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + expinfrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2" +) + +// LifecycleHookNeedsUpdate returns true if the supplied expected lifecycle hook differs from the existing lifecycle hook. +func (s *Service) LifecycleHookNeedsUpdate(existing *expinfrav1.AWSLifecycleHook, expected *expinfrav1.AWSLifecycleHook) bool { + return existing.DefaultResult != expected.DefaultResult || + existing.HeartbeatTimeout != expected.HeartbeatTimeout || + existing.LifecycleTransition != expected.LifecycleTransition || + existing.NotificationTargetARN != expected.NotificationTargetARN || + existing.NotificationMetadata != expected.NotificationMetadata +} + +// GetLifecycleHooks returns the lifecycle hooks for the given AutoScalingGroup after retrieving them from the AWS API. +func (s *Service) DescribeLifecycleHooks(asgName string) ([]*expinfrav1.AWSLifecycleHook, error) { + input := &autoscaling.DescribeLifecycleHooksInput{ + AutoScalingGroupName: aws.String(asgName), + } + + out, err := s.ASGClient.DescribeLifecycleHooksWithContext(context.TODO(), input) + if err != nil { + return nil, errors.Wrapf(err, "failed to describe lifecycle hooks for AutoScalingGroup: %q", asgName) + } + + hooks := make([]*expinfrav1.AWSLifecycleHook, len(out.LifecycleHooks)) + for i, hook := range out.LifecycleHooks { + hooks[i] = s.SDKToLifecycleHook(hook) + } + + return hooks, nil +} + +// GetLifecycleHook returns a specific lifecycle hook for the given AutoScalingGroup after retrieving it from the AWS API. +func (s *Service) DescribeLifecycleHook(asgName string, hook *expinfrav1.AWSLifecycleHook) (*expinfrav1.AWSLifecycleHook, error) { + input := &autoscaling.DescribeLifecycleHooksInput{ + AutoScalingGroupName: aws.String(asgName), + LifecycleHookNames: []*string{aws.String(hook.Name)}, + } + + out, err := s.ASGClient.DescribeLifecycleHooksWithContext(context.TODO(), input) + if err != nil { + return nil, errors.Wrapf(err, "failed to describe lifecycle hook %q for AutoScalingGroup: %q", hook.Name, asgName) + } + + if len(out.LifecycleHooks) == 0 { + return nil, nil + } + + return s.SDKToLifecycleHook(out.LifecycleHooks[0]), nil +} + +// CreateLifecycleHook creates a lifecycle hook for the given AutoScalingGroup. +func (s *Service) CreateLifecycleHook(asgName string, hook *expinfrav1.AWSLifecycleHook) error { + input := &autoscaling.PutLifecycleHookInput{ + AutoScalingGroupName: aws.String(asgName), + LifecycleHookName: aws.String(hook.Name), + LifecycleTransition: aws.String(hook.LifecycleTransition.String()), + } + + // Optional parameters + if hook.DefaultResult != nil { + input.DefaultResult = aws.String(hook.DefaultResult.String()) + } + + if hook.HeartbeatTimeout != nil { + timeoutSeconds := hook.HeartbeatTimeout.Duration.Seconds() + input.HeartbeatTimeout = aws.Int64(int64(timeoutSeconds)) + } + + if hook.NotificationTargetARN != nil { + input.NotificationTargetARN = hook.NotificationTargetARN + } + + if hook.RoleARN != nil { + input.RoleARN = hook.RoleARN + } + + if hook.NotificationMetadata != nil { + input.NotificationMetadata = hook.NotificationMetadata + } + + if _, err := s.ASGClient.PutLifecycleHookWithContext(context.TODO(), input); err != nil { + return errors.Wrapf(err, "failed to create lifecycle hook %q for AutoScalingGroup: %q", hook.Name, asgName) + } + + return nil +} + +// UpdateLifecycleHook updates a lifecycle hook for the given AutoScalingGroup. +func (s *Service) UpdateLifecycleHook(asgName string, hook *expinfrav1.AWSLifecycleHook) error { + input := &autoscaling.PutLifecycleHookInput{ + AutoScalingGroupName: aws.String(asgName), + LifecycleHookName: aws.String(hook.Name), + LifecycleTransition: aws.String(hook.LifecycleTransition.String()), + } + + // Optional parameters + if hook.DefaultResult != nil { + input.DefaultResult = aws.String(hook.DefaultResult.String()) + } + + if hook.HeartbeatTimeout != nil { + timeoutSeconds := hook.HeartbeatTimeout.Duration.Seconds() + input.HeartbeatTimeout = aws.Int64(int64(timeoutSeconds)) + } + + if hook.NotificationTargetARN != nil { + input.NotificationTargetARN = hook.NotificationTargetARN + } + + if hook.RoleARN != nil { + input.RoleARN = hook.RoleARN + } + + if hook.NotificationMetadata != nil { + input.NotificationMetadata = hook.NotificationMetadata + } + + if _, err := s.ASGClient.PutLifecycleHookWithContext(context.TODO(), input); err != nil { + return errors.Wrapf(err, "failed to update lifecycle hook %q for AutoScalingGroup: %q", hook.Name, asgName) + } + + return nil +} + +// DeleteLifecycleHook deletes a lifecycle hook for the given AutoScalingGroup. +func (s *Service) DeleteLifecycleHook( + asgName string, + hook *expinfrav1.AWSLifecycleHook, +) error { + input := &autoscaling.DeleteLifecycleHookInput{ + AutoScalingGroupName: aws.String(asgName), + LifecycleHookName: aws.String(hook.Name), + } + + if _, err := s.ASGClient.DeleteLifecycleHookWithContext(context.TODO(), input); err != nil { + return errors.Wrapf(err, "failed to delete lifecycle hook %q for AutoScalingGroup: %q", hook.Name, asgName) + } + + return nil +} + +// SDKToLifecycleHook converts an AWS SDK LifecycleHook to the CAPA lifecycle hook type. +func (s *Service) SDKToLifecycleHook(hook *autoscaling.LifecycleHook) *expinfrav1.AWSLifecycleHook { + timeoutDuration := time.Duration(*hook.HeartbeatTimeout) * time.Second + metav1Duration := metav1.Duration{Duration: timeoutDuration} + defaultResult := expinfrav1.DefaultResult(*hook.DefaultResult) + lifecycleTransition := expinfrav1.LifecycleTransition(*hook.LifecycleTransition) + + return &expinfrav1.AWSLifecycleHook{ + Name: *hook.LifecycleHookName, + DefaultResult: &defaultResult, + HeartbeatTimeout: &metav1Duration, + LifecycleTransition: lifecycleTransition, + NotificationTargetARN: hook.NotificationTargetARN, + RoleARN: hook.RoleARN, + NotificationMetadata: hook.NotificationMetadata, + } +} diff --git a/pkg/cloud/services/eks/nodegroup.go b/pkg/cloud/services/eks/nodegroup.go index 763d14b494..215494a7e8 100644 --- a/pkg/cloud/services/eks/nodegroup.go +++ b/pkg/cloud/services/eks/nodegroup.go @@ -38,8 +38,14 @@ import ( "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/record" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/util/annotations" + "sigs.k8s.io/cluster-api/util/conditions" ) +var IgnoreLifecycleHooks = map[string]bool{ + "Launch-LC-Hook": true, + "Terminate-LC-Hook": true, +} + func (s *NodegroupService) describeNodegroup() (*eks.Nodegroup, error) { eksClusterName := s.scope.KubernetesClusterName() nodegroupName := s.scope.NodegroupName() @@ -150,7 +156,7 @@ func (s *NodegroupService) remoteAccess() (*eks.RemoteAccessConfig, error) { // SourceSecurityGroups is validated to be empty if PublicAccess is true // but just in case we use an empty list to take advantage of the documented // API behavior - var sSGs = []string{} + sSGs := []string{} if !pool.RemoteAccess.Public { sSGs = pool.RemoteAccess.SourceSecurityGroups @@ -571,6 +577,10 @@ func (s *NodegroupService) reconcileNodegroup(ctx context.Context) error { return errors.Wrapf(err, "failed to reconcile asg tags") } + if err := s.reconcileLifecycleHooks(ng); err != nil { + return errors.Wrapf(err, "failed to reconcile lifecyle hooks") + } + return nil } @@ -645,3 +655,82 @@ func (s *NodegroupService) waitForNodegroupActive() (*eks.Nodegroup, error) { return ng, nil } + +// ReconcileLifecycleHooks periodically reconciles a lifecycle hook for the ASG. +func (s *NodegroupService) reconcileLifecycleHooks(ng *eks.Nodegroup) error { + asg, err := s.describeASGs(ng) + if err != nil { + return err + } + + lifecyleHooks := s.scope.GetLifecycleHooks() + for i := range lifecyleHooks { + if err := s.reconcileLifecycleHook(*asg.AutoScalingGroupName, &lifecyleHooks[i]); 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 := s.ASGService.DescribeLifecycleHooks(*asg.AutoScalingGroupName) + if err != nil { + return err + } + for _, hook := range hooks { + found := false + if IgnoreLifecycleHooks[hook.Name] { + continue + } + for _, definedHook := range lifecyleHooks { + if hook.Name == definedHook.Name { + found = true + break + } + } + if !found { + s.scope.Info("Deleting extraneous lifecycle hook", "hook", hook.Name) + if err := s.ASGService.DeleteLifecycleHook(*asg.AutoScalingGroupName, hook); err != nil { + conditions.MarkFalse(s.scope.GetMachinePool(), expinfrav1.LifecycleHookReadyCondition, expinfrav1.LifecycleHookDeletionFailedReason, clusterv1.ConditionSeverityError, err.Error()) + return err + } + } + } + + return nil +} + +func (s *NodegroupService) reconcileLifecycleHook(asgName string, hook *expinfrav1.AWSLifecycleHook) error { + s.scope.Info("Checking for existing lifecycle hook") + // Ignore hooks that are not managed by the controller + if ignore, ok := IgnoreLifecycleHooks[hook.Name]; ok && ignore { + return nil + } + + existingHook, err := s.ASGService.DescribeLifecycleHook(asgName, hook) + if err != nil { + conditions.MarkUnknown(s.scope.GetMachinePool(), expinfrav1.LifecycleHookReadyCondition, expinfrav1.LifecycleHookNotFoundReason, err.Error()) + return err + } + + if existingHook == nil { + s.scope.Info("Creating lifecycle hook") + if err := s.ASGService.CreateLifecycleHook(asgName, hook); err != nil { + conditions.MarkFalse(s.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 := s.ASGService.LifecycleHookNeedsUpdate(existingHook, hook) + + if needsUpdate { + s.scope.Info("Updating lifecycle hook") + if err := s.ASGService.UpdateLifecycleHook(asgName, hook); err != nil { + conditions.MarkFalse(s.scope.GetMachinePool(), expinfrav1.LifecycleHookReadyCondition, expinfrav1.LifecycleHookUpdateFailedReason, clusterv1.ConditionSeverityError, err.Error()) + return err + } + } + + conditions.MarkTrue(s.scope.GetMachinePool(), expinfrav1.LifecycleHookReadyCondition) + return nil +} diff --git a/pkg/cloud/services/eks/service.go b/pkg/cloud/services/eks/service.go index 9160a398a1..6f6cb280fb 100644 --- a/pkg/cloud/services/eks/service.go +++ b/pkg/cloud/services/eks/service.go @@ -27,6 +27,8 @@ import ( "github.com/aws/aws-sdk-go/service/sts/stsiface" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/scope" + "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services" + asg "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services/autoscaling" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services/eks/iam" ) @@ -90,6 +92,7 @@ func NewService(controlPlaneScope *scope.ManagedControlPlaneScope, opts ...Servi // One alternative is to have a large list of functions from the ec2 client. type NodegroupService struct { scope *scope.ManagedMachinePoolScope + ASGService services.ASGInterface AutoscalingClient autoscalingiface.AutoScalingAPI EKSClient eksiface.EKSAPI iam.IAMService @@ -100,6 +103,7 @@ type NodegroupService struct { func NewNodegroupService(machinePoolScope *scope.ManagedMachinePoolScope) *NodegroupService { return &NodegroupService{ scope: machinePoolScope, + ASGService: asg.NewService(machinePoolScope.EC2Scope), AutoscalingClient: scope.NewASGClient(machinePoolScope, machinePoolScope, machinePoolScope, machinePoolScope.ManagedMachinePool), EKSClient: scope.NewEKSClient(machinePoolScope, machinePoolScope, machinePoolScope, machinePoolScope.ManagedMachinePool), IAMService: iam.IAMService{ diff --git a/pkg/cloud/services/interfaces.go b/pkg/cloud/services/interfaces.go index 5e0f3dc2e6..f711c117a2 100644 --- a/pkg/cloud/services/interfaces.go +++ b/pkg/cloud/services/interfaces.go @@ -50,6 +50,12 @@ type ASGInterface interface { SuspendProcesses(name string, processes []string) error ResumeProcesses(name string, processes []string) error SubnetIDs(scope *scope.MachinePoolScope) ([]string, error) + DescribeLifecycleHooks(asgName string) ([]*expinfrav1.AWSLifecycleHook, error) + DescribeLifecycleHook(asgName string, hook *expinfrav1.AWSLifecycleHook) (*expinfrav1.AWSLifecycleHook, error) + CreateLifecycleHook(asgName string, hook *expinfrav1.AWSLifecycleHook) error + UpdateLifecycleHook(asgName string, hook *expinfrav1.AWSLifecycleHook) error + DeleteLifecycleHook(asgName string, hook *expinfrav1.AWSLifecycleHook) error + LifecycleHookNeedsUpdate(incoming *expinfrav1.AWSLifecycleHook, existing *expinfrav1.AWSLifecycleHook) bool } // EC2Interface encapsulates the methods exposed to the machine diff --git a/pkg/cloud/services/mock_services/autoscaling_interface_mock.go b/pkg/cloud/services/mock_services/autoscaling_interface_mock.go index b860077f4f..7a417672cd 100644 --- a/pkg/cloud/services/mock_services/autoscaling_interface_mock.go +++ b/pkg/cloud/services/mock_services/autoscaling_interface_mock.go @@ -96,6 +96,20 @@ func (mr *MockASGInterfaceMockRecorder) CreateASG(arg0 interface{}) *gomock.Call return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateASG", reflect.TypeOf((*MockASGInterface)(nil).CreateASG), arg0) } +// CreateLifecycleHook mocks base method. +func (m *MockASGInterface) CreateLifecycleHook(arg0 string, arg1 *v1beta2.AWSLifecycleHook) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CreateLifecycleHook", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// CreateLifecycleHook indicates an expected call of CreateLifecycleHook. +func (mr *MockASGInterfaceMockRecorder) CreateLifecycleHook(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateLifecycleHook", reflect.TypeOf((*MockASGInterface)(nil).CreateLifecycleHook), arg0, arg1) +} + // DeleteASGAndWait mocks base method. func (m *MockASGInterface) DeleteASGAndWait(arg0 string) error { m.ctrl.T.Helper() @@ -110,6 +124,50 @@ func (mr *MockASGInterfaceMockRecorder) DeleteASGAndWait(arg0 interface{}) *gomo return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteASGAndWait", reflect.TypeOf((*MockASGInterface)(nil).DeleteASGAndWait), arg0) } +// DeleteLifecycleHook mocks base method. +func (m *MockASGInterface) DeleteLifecycleHook(arg0 string, arg1 *v1beta2.AWSLifecycleHook) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeleteLifecycleHook", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// DeleteLifecycleHook indicates an expected call of DeleteLifecycleHook. +func (mr *MockASGInterfaceMockRecorder) DeleteLifecycleHook(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteLifecycleHook", reflect.TypeOf((*MockASGInterface)(nil).DeleteLifecycleHook), arg0, arg1) +} + +// DescribeLifecycleHook mocks base method. +func (m *MockASGInterface) DescribeLifecycleHook(arg0 string, arg1 *v1beta2.AWSLifecycleHook) (*v1beta2.AWSLifecycleHook, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DescribeLifecycleHook", arg0, arg1) + ret0, _ := ret[0].(*v1beta2.AWSLifecycleHook) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// DescribeLifecycleHook indicates an expected call of DescribeLifecycleHook. +func (mr *MockASGInterfaceMockRecorder) DescribeLifecycleHook(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DescribeLifecycleHook", reflect.TypeOf((*MockASGInterface)(nil).DescribeLifecycleHook), arg0, arg1) +} + +// DescribeLifecycleHooks mocks base method. +func (m *MockASGInterface) DescribeLifecycleHooks(arg0 string) ([]*v1beta2.AWSLifecycleHook, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DescribeLifecycleHooks", arg0) + ret0, _ := ret[0].([]*v1beta2.AWSLifecycleHook) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// DescribeLifecycleHooks indicates an expected call of DescribeLifecycleHooks. +func (mr *MockASGInterfaceMockRecorder) DescribeLifecycleHooks(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DescribeLifecycleHooks", reflect.TypeOf((*MockASGInterface)(nil).DescribeLifecycleHooks), arg0) +} + // GetASGByName mocks base method. func (m *MockASGInterface) GetASGByName(arg0 *scope.MachinePoolScope) (*v1beta2.AutoScalingGroup, error) { m.ctrl.T.Helper() @@ -125,6 +183,20 @@ func (mr *MockASGInterfaceMockRecorder) GetASGByName(arg0 interface{}) *gomock.C return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetASGByName", reflect.TypeOf((*MockASGInterface)(nil).GetASGByName), arg0) } +// LifecycleHookNeedsUpdate mocks base method. +func (m *MockASGInterface) LifecycleHookNeedsUpdate(arg0, arg1 *v1beta2.AWSLifecycleHook) bool { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "LifecycleHookNeedsUpdate", arg0, arg1) + ret0, _ := ret[0].(bool) + return ret0 +} + +// LifecycleHookNeedsUpdate indicates an expected call of LifecycleHookNeedsUpdate. +func (mr *MockASGInterfaceMockRecorder) LifecycleHookNeedsUpdate(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "LifecycleHookNeedsUpdate", reflect.TypeOf((*MockASGInterface)(nil).LifecycleHookNeedsUpdate), arg0, arg1) +} + // ResumeProcesses mocks base method. func (m *MockASGInterface) ResumeProcesses(arg0 string, arg1 []string) error { m.ctrl.T.Helper() @@ -196,6 +268,20 @@ func (mr *MockASGInterfaceMockRecorder) UpdateASG(arg0 interface{}) *gomock.Call return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateASG", reflect.TypeOf((*MockASGInterface)(nil).UpdateASG), arg0) } +// UpdateLifecycleHook mocks base method. +func (m *MockASGInterface) UpdateLifecycleHook(arg0 string, arg1 *v1beta2.AWSLifecycleHook) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "UpdateLifecycleHook", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// UpdateLifecycleHook indicates an expected call of UpdateLifecycleHook. +func (mr *MockASGInterfaceMockRecorder) UpdateLifecycleHook(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateLifecycleHook", reflect.TypeOf((*MockASGInterface)(nil).UpdateLifecycleHook), arg0, arg1) +} + // UpdateResourceTags mocks base method. func (m *MockASGInterface) UpdateResourceTags(arg0 *string, arg1, arg2 map[string]string) error { m.ctrl.T.Helper() From e40a32c1ec317c47c6094faa62b1fa88ddf45bef Mon Sep 17 00:00:00 2001 From: Andreas Sommer Date: Wed, 20 Nov 2024 22:51:03 +0100 Subject: [PATCH 02/13] Address review comments --- exp/api/v1beta2/awsmachinepool_webhook.go | 28 ----- exp/api/v1beta2/conditions_consts.go | 2 - exp/api/v1beta2/types.go | 24 ++-- exp/api/v1beta2/validation.go | 50 ++++++++ exp/api/v1beta2/zz_generated.deepcopy.go | 2 +- exp/controllers/awsmachinepool_controller.go | 71 +---------- .../awsmachinepool_controller_test.go | 33 ++--- .../services/autoscaling/lifecyclehook.go | 119 +++++++++++++----- pkg/cloud/services/eks/nodegroup.go | 83 ++---------- pkg/cloud/services/interfaces.go | 2 - .../autoscaling_interface_mock.go | 29 ----- 11 files changed, 173 insertions(+), 270 deletions(-) create mode 100644 exp/api/v1beta2/validation.go diff --git a/exp/api/v1beta2/awsmachinepool_webhook.go b/exp/api/v1beta2/awsmachinepool_webhook.go index c8942794c0..085d2f62f5 100644 --- a/exp/api/v1beta2/awsmachinepool_webhook.go +++ b/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)) diff --git a/exp/api/v1beta2/conditions_consts.go b/exp/api/v1beta2/conditions_consts.go index 3c56c1b853..ff4cfd4c98 100644 --- a/exp/api/v1beta2/conditions_consts.go +++ b/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. diff --git a/exp/api/v1beta2/types.go b/exp/api/v1beta2/types.go index 6b4f766e54..07d1184e20 100644 --- a/exp/api/v1beta2/types.go +++ b/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) } diff --git a/exp/api/v1beta2/validation.go b/exp/api/v1beta2/validation.go new file mode 100644 index 0000000000..e369d6f395 --- /dev/null +++ b/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 +} diff --git a/exp/api/v1beta2/zz_generated.deepcopy.go b/exp/api/v1beta2/zz_generated.deepcopy.go index e82ce2f3f4..21b227ee9d 100644 --- a/exp/api/v1beta2/zz_generated.deepcopy.go +++ b/exp/api/v1beta2/zz_generated.deepcopy.go @@ -168,7 +168,7 @@ func (in *AWSLifecycleHook) DeepCopyInto(out *AWSLifecycleHook) { } if in.DefaultResult != nil { in, out := &in.DefaultResult, &out.DefaultResult - *out = new(DefaultResult) + *out = new(LifecycleHookDefaultResult) **out = **in } if in.NotificationMetadata != nil { diff --git a/exp/controllers/awsmachinepool_controller.go b/exp/controllers/awsmachinepool_controller.go index 2ef08e60dc..7dfa757c38 100644 --- a/exp/controllers/awsmachinepool_controller.go +++ b/exp/controllers/awsmachinepool_controller.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) { diff --git a/exp/controllers/awsmachinepool_controller_test.go b/exp/controllers/awsmachinepool_controller_test.go index 748a6b5ff6..cf7f063e92 100644 --- a/exp/controllers/awsmachinepool_controller_test.go +++ b/exp/controllers/awsmachinepool_controller_test.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 diff --git a/pkg/cloud/services/autoscaling/lifecyclehook.go b/pkg/cloud/services/autoscaling/lifecyclehook.go index 59f51f998e..8cc2385441 100644 --- a/pkg/cloud/services/autoscaling/lifecyclehook.go +++ b/pkg/cloud/services/autoscaling/lifecyclehook.go @@ -26,17 +26,12 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" expinfrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2" + "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services" + "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/logger" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + "sigs.k8s.io/cluster-api/util/conditions" ) -// LifecycleHookNeedsUpdate returns true if the supplied expected lifecycle hook differs from the existing lifecycle hook. -func (s *Service) LifecycleHookNeedsUpdate(existing *expinfrav1.AWSLifecycleHook, expected *expinfrav1.AWSLifecycleHook) bool { - return existing.DefaultResult != expected.DefaultResult || - existing.HeartbeatTimeout != expected.HeartbeatTimeout || - existing.LifecycleTransition != expected.LifecycleTransition || - existing.NotificationTargetARN != expected.NotificationTargetARN || - existing.NotificationMetadata != expected.NotificationMetadata -} - // GetLifecycleHooks returns the lifecycle hooks for the given AutoScalingGroup after retrieving them from the AWS API. func (s *Service) DescribeLifecycleHooks(asgName string) ([]*expinfrav1.AWSLifecycleHook, error) { input := &autoscaling.DescribeLifecycleHooksInput{ @@ -56,25 +51,6 @@ func (s *Service) DescribeLifecycleHooks(asgName string) ([]*expinfrav1.AWSLifec return hooks, nil } -// GetLifecycleHook returns a specific lifecycle hook for the given AutoScalingGroup after retrieving it from the AWS API. -func (s *Service) DescribeLifecycleHook(asgName string, hook *expinfrav1.AWSLifecycleHook) (*expinfrav1.AWSLifecycleHook, error) { - input := &autoscaling.DescribeLifecycleHooksInput{ - AutoScalingGroupName: aws.String(asgName), - LifecycleHookNames: []*string{aws.String(hook.Name)}, - } - - out, err := s.ASGClient.DescribeLifecycleHooksWithContext(context.TODO(), input) - if err != nil { - return nil, errors.Wrapf(err, "failed to describe lifecycle hook %q for AutoScalingGroup: %q", hook.Name, asgName) - } - - if len(out.LifecycleHooks) == 0 { - return nil, nil - } - - return s.SDKToLifecycleHook(out.LifecycleHooks[0]), nil -} - // CreateLifecycleHook creates a lifecycle hook for the given AutoScalingGroup. func (s *Service) CreateLifecycleHook(asgName string, hook *expinfrav1.AWSLifecycleHook) error { input := &autoscaling.PutLifecycleHookInput{ @@ -170,7 +146,7 @@ func (s *Service) DeleteLifecycleHook( func (s *Service) SDKToLifecycleHook(hook *autoscaling.LifecycleHook) *expinfrav1.AWSLifecycleHook { timeoutDuration := time.Duration(*hook.HeartbeatTimeout) * time.Second metav1Duration := metav1.Duration{Duration: timeoutDuration} - defaultResult := expinfrav1.DefaultResult(*hook.DefaultResult) + defaultResult := expinfrav1.LifecycleHookDefaultResult(*hook.DefaultResult) lifecycleTransition := expinfrav1.LifecycleTransition(*hook.LifecycleTransition) return &expinfrav1.AWSLifecycleHook{ @@ -183,3 +159,88 @@ func (s *Service) SDKToLifecycleHook(hook *autoscaling.LifecycleHook) *expinfrav NotificationMetadata: hook.NotificationMetadata, } } + +// ReconcileLifecycleHooks reconciles lifecycle hooks for an ASG +// by creating missing hooks, updating mismatching hooks and +// deleting extraneous hooks (except those specified in +// ignoreLifecycleHooks). +func ReconcileLifecycleHooks(asgService services.ASGInterface, asgName string, wantedLifecycleHooks []expinfrav1.AWSLifecycleHook, ignoreLifecycleHooks map[string]bool, storeConditionsOnObject conditions.Setter, log logger.Wrapper) error { + existingHooks, err := asgService.DescribeLifecycleHooks(asgName) + if err != nil { + return err + } + + for i := range wantedLifecycleHooks { + if ignoreLifecycleHooks[wantedLifecycleHooks[i].Name] { + log.Info("Not reconciling lifecycle hook since it's on the ignore list") + continue + } + + if err := reconcileLifecycleHook(asgService, asgName, &wantedLifecycleHooks[i], existingHooks, storeConditionsOnObject, log); err != nil { + return err + } + } + + for _, existingHook := range existingHooks { + found := false + if ignoreLifecycleHooks[existingHook.Name] { + continue + } + for _, wantedHook := range wantedLifecycleHooks { + if existingHook.Name == wantedHook.Name { + found = true + break + } + } + if !found { + log.Info("Deleting extraneous lifecycle hook", "hook", existingHook.Name) + if err := asgService.DeleteLifecycleHook(asgName, existingHook); err != nil { + conditions.MarkFalse(storeConditionsOnObject, expinfrav1.LifecycleHookReadyCondition, expinfrav1.LifecycleHookDeletionFailedReason, clusterv1.ConditionSeverityError, err.Error()) + return err + } + } + } + + return nil +} + +func lifecycleHookNeedsUpdate(existing *expinfrav1.AWSLifecycleHook, expected *expinfrav1.AWSLifecycleHook) bool { + return existing.DefaultResult != expected.DefaultResult || + existing.HeartbeatTimeout != expected.HeartbeatTimeout || + existing.LifecycleTransition != expected.LifecycleTransition || + existing.NotificationTargetARN != expected.NotificationTargetARN || + existing.NotificationMetadata != expected.NotificationMetadata +} + +func reconcileLifecycleHook(asgService services.ASGInterface, asgName string, wantedHook *expinfrav1.AWSLifecycleHook, existingHooks []*expinfrav1.AWSLifecycleHook, storeConditionsOnObject conditions.Setter, log logger.Wrapper) error { + log = log.WithValues("hook", wantedHook.Name) + + log.Info("Checking for existing lifecycle hook") + var existingHook *expinfrav1.AWSLifecycleHook + for _, h := range existingHooks { + if h.Name == wantedHook.Name { + existingHook = h + break + } + } + + if existingHook == nil { + log.Info("Creating lifecycle hook") + if err := asgService.CreateLifecycleHook(asgName, wantedHook); err != nil { + conditions.MarkFalse(storeConditionsOnObject, expinfrav1.LifecycleHookReadyCondition, expinfrav1.LifecycleHookCreationFailedReason, clusterv1.ConditionSeverityError, err.Error()) + return err + } + return nil + } + + if lifecycleHookNeedsUpdate(existingHook, wantedHook) { + log.Info("Updating lifecycle hook") + if err := asgService.UpdateLifecycleHook(asgName, wantedHook); err != nil { + conditions.MarkFalse(storeConditionsOnObject, expinfrav1.LifecycleHookReadyCondition, expinfrav1.LifecycleHookUpdateFailedReason, clusterv1.ConditionSeverityError, err.Error()) + return err + } + } + + conditions.MarkTrue(storeConditionsOnObject, expinfrav1.LifecycleHookReadyCondition) + return nil +} diff --git a/pkg/cloud/services/eks/nodegroup.go b/pkg/cloud/services/eks/nodegroup.go index 215494a7e8..239b884299 100644 --- a/pkg/cloud/services/eks/nodegroup.go +++ b/pkg/cloud/services/eks/nodegroup.go @@ -34,14 +34,15 @@ import ( expinfrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/awserrors" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/converters" + asg "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services/autoscaling" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services/wait" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/record" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/util/annotations" - "sigs.k8s.io/cluster-api/util/conditions" ) -var IgnoreLifecycleHooks = map[string]bool{ +// Built-in EKS lifecycle hooks should not be changed +var IgnoredEKSLifecycleHooks = map[string]bool{ "Launch-LC-Hook": true, "Terminate-LC-Hook": true, } @@ -656,81 +657,11 @@ func (s *NodegroupService) waitForNodegroupActive() (*eks.Nodegroup, error) { return ng, nil } -// ReconcileLifecycleHooks periodically reconciles a lifecycle hook for the ASG. +// reconcileLifecycleHooks periodically reconciles a lifecycle hook for the ASG. func (s *NodegroupService) reconcileLifecycleHooks(ng *eks.Nodegroup) error { - asg, err := s.describeASGs(ng) - if err != nil { - return err - } - - lifecyleHooks := s.scope.GetLifecycleHooks() - for i := range lifecyleHooks { - if err := s.reconcileLifecycleHook(*asg.AutoScalingGroupName, &lifecyleHooks[i]); 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 := s.ASGService.DescribeLifecycleHooks(*asg.AutoScalingGroupName) - if err != nil { - return err - } - for _, hook := range hooks { - found := false - if IgnoreLifecycleHooks[hook.Name] { - continue - } - for _, definedHook := range lifecyleHooks { - if hook.Name == definedHook.Name { - found = true - break - } - } - if !found { - s.scope.Info("Deleting extraneous lifecycle hook", "hook", hook.Name) - if err := s.ASGService.DeleteLifecycleHook(*asg.AutoScalingGroupName, hook); err != nil { - conditions.MarkFalse(s.scope.GetMachinePool(), expinfrav1.LifecycleHookReadyCondition, expinfrav1.LifecycleHookDeletionFailedReason, clusterv1.ConditionSeverityError, err.Error()) - return err - } - } - } - - return nil -} - -func (s *NodegroupService) reconcileLifecycleHook(asgName string, hook *expinfrav1.AWSLifecycleHook) error { - s.scope.Info("Checking for existing lifecycle hook") - // Ignore hooks that are not managed by the controller - if ignore, ok := IgnoreLifecycleHooks[hook.Name]; ok && ignore { - return nil - } - - existingHook, err := s.ASGService.DescribeLifecycleHook(asgName, hook) - if err != nil { - conditions.MarkUnknown(s.scope.GetMachinePool(), expinfrav1.LifecycleHookReadyCondition, expinfrav1.LifecycleHookNotFoundReason, err.Error()) - return err - } - - if existingHook == nil { - s.scope.Info("Creating lifecycle hook") - if err := s.ASGService.CreateLifecycleHook(asgName, hook); err != nil { - conditions.MarkFalse(s.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 := s.ASGService.LifecycleHookNeedsUpdate(existingHook, hook) - - if needsUpdate { - s.scope.Info("Updating lifecycle hook") - if err := s.ASGService.UpdateLifecycleHook(asgName, hook); err != nil { - conditions.MarkFalse(s.scope.GetMachinePool(), expinfrav1.LifecycleHookReadyCondition, expinfrav1.LifecycleHookUpdateFailedReason, clusterv1.ConditionSeverityError, err.Error()) - return err - } + if len(ng.Resources.AutoScalingGroups) == 0 { + return errors.New("no ASG defined for node group") } - conditions.MarkTrue(s.scope.GetMachinePool(), expinfrav1.LifecycleHookReadyCondition) - return nil + return asg.ReconcileLifecycleHooks(s.ASGService, *ng.Resources.AutoScalingGroups[0].Name, s.scope.GetLifecycleHooks(), IgnoredEKSLifecycleHooks, s.scope.GetMachinePool(), s.scope) } diff --git a/pkg/cloud/services/interfaces.go b/pkg/cloud/services/interfaces.go index f711c117a2..3a63a28ebb 100644 --- a/pkg/cloud/services/interfaces.go +++ b/pkg/cloud/services/interfaces.go @@ -51,11 +51,9 @@ type ASGInterface interface { ResumeProcesses(name string, processes []string) error SubnetIDs(scope *scope.MachinePoolScope) ([]string, error) DescribeLifecycleHooks(asgName string) ([]*expinfrav1.AWSLifecycleHook, error) - DescribeLifecycleHook(asgName string, hook *expinfrav1.AWSLifecycleHook) (*expinfrav1.AWSLifecycleHook, error) CreateLifecycleHook(asgName string, hook *expinfrav1.AWSLifecycleHook) error UpdateLifecycleHook(asgName string, hook *expinfrav1.AWSLifecycleHook) error DeleteLifecycleHook(asgName string, hook *expinfrav1.AWSLifecycleHook) error - LifecycleHookNeedsUpdate(incoming *expinfrav1.AWSLifecycleHook, existing *expinfrav1.AWSLifecycleHook) bool } // EC2Interface encapsulates the methods exposed to the machine diff --git a/pkg/cloud/services/mock_services/autoscaling_interface_mock.go b/pkg/cloud/services/mock_services/autoscaling_interface_mock.go index 7a417672cd..1f936dfb9e 100644 --- a/pkg/cloud/services/mock_services/autoscaling_interface_mock.go +++ b/pkg/cloud/services/mock_services/autoscaling_interface_mock.go @@ -138,21 +138,6 @@ func (mr *MockASGInterfaceMockRecorder) DeleteLifecycleHook(arg0, arg1 interface return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteLifecycleHook", reflect.TypeOf((*MockASGInterface)(nil).DeleteLifecycleHook), arg0, arg1) } -// DescribeLifecycleHook mocks base method. -func (m *MockASGInterface) DescribeLifecycleHook(arg0 string, arg1 *v1beta2.AWSLifecycleHook) (*v1beta2.AWSLifecycleHook, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DescribeLifecycleHook", arg0, arg1) - ret0, _ := ret[0].(*v1beta2.AWSLifecycleHook) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// DescribeLifecycleHook indicates an expected call of DescribeLifecycleHook. -func (mr *MockASGInterfaceMockRecorder) DescribeLifecycleHook(arg0, arg1 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DescribeLifecycleHook", reflect.TypeOf((*MockASGInterface)(nil).DescribeLifecycleHook), arg0, arg1) -} - // DescribeLifecycleHooks mocks base method. func (m *MockASGInterface) DescribeLifecycleHooks(arg0 string) ([]*v1beta2.AWSLifecycleHook, error) { m.ctrl.T.Helper() @@ -183,20 +168,6 @@ func (mr *MockASGInterfaceMockRecorder) GetASGByName(arg0 interface{}) *gomock.C return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetASGByName", reflect.TypeOf((*MockASGInterface)(nil).GetASGByName), arg0) } -// LifecycleHookNeedsUpdate mocks base method. -func (m *MockASGInterface) LifecycleHookNeedsUpdate(arg0, arg1 *v1beta2.AWSLifecycleHook) bool { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "LifecycleHookNeedsUpdate", arg0, arg1) - ret0, _ := ret[0].(bool) - return ret0 -} - -// LifecycleHookNeedsUpdate indicates an expected call of LifecycleHookNeedsUpdate. -func (mr *MockASGInterfaceMockRecorder) LifecycleHookNeedsUpdate(arg0, arg1 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "LifecycleHookNeedsUpdate", reflect.TypeOf((*MockASGInterface)(nil).LifecycleHookNeedsUpdate), arg0, arg1) -} - // ResumeProcesses mocks base method. func (m *MockASGInterface) ResumeProcesses(arg0 string, arg1 []string) error { m.ctrl.T.Helper() From cd6010d8fd1b10854f058c7144fc71dfc2f21480 Mon Sep 17 00:00:00 2001 From: Andreas Sommer Date: Wed, 20 Nov 2024 23:02:59 +0100 Subject: [PATCH 03/13] Fix lints --- pkg/cloud/scope/machinepool.go | 1 + pkg/cloud/scope/managednodegroup.go | 5 +++-- pkg/cloud/services/autoscaling/lifecyclehook.go | 2 +- pkg/cloud/services/eks/nodegroup.go | 3 ++- 4 files changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/cloud/scope/machinepool.go b/pkg/cloud/scope/machinepool.go index 8fe2d2cecb..affd68513b 100644 --- a/pkg/cloud/scope/machinepool.go +++ b/pkg/cloud/scope/machinepool.go @@ -396,6 +396,7 @@ func (m *MachinePoolScope) GetRuntimeObject() runtime.Object { return m.AWSMachinePool } +// GetLifecycleHooks returns the desired lifecycle hooks for the ASG. func (m *MachinePoolScope) GetLifecycleHooks() []expinfrav1.AWSLifecycleHook { return m.AWSMachinePool.Spec.AWSLifecycleHooks } diff --git a/pkg/cloud/scope/managednodegroup.go b/pkg/cloud/scope/managednodegroup.go index ab7c2761f8..c091f02efe 100644 --- a/pkg/cloud/scope/managednodegroup.go +++ b/pkg/cloud/scope/managednodegroup.go @@ -412,6 +412,7 @@ func (s *ManagedMachinePoolScope) GetRuntimeObject() runtime.Object { return s.ManagedMachinePool } -func (m *ManagedMachinePoolScope) GetLifecycleHooks() []expinfrav1.AWSLifecycleHook { - return m.ManagedMachinePool.Spec.AWSLifecycleHooks +// GetLifecycleHooks returns the desired lifecycle hooks for the ASG. +func (s *ManagedMachinePoolScope) GetLifecycleHooks() []expinfrav1.AWSLifecycleHook { + return s.ManagedMachinePool.Spec.AWSLifecycleHooks } diff --git a/pkg/cloud/services/autoscaling/lifecyclehook.go b/pkg/cloud/services/autoscaling/lifecyclehook.go index 8cc2385441..b0fa055d96 100644 --- a/pkg/cloud/services/autoscaling/lifecyclehook.go +++ b/pkg/cloud/services/autoscaling/lifecyclehook.go @@ -32,7 +32,7 @@ import ( "sigs.k8s.io/cluster-api/util/conditions" ) -// GetLifecycleHooks returns the lifecycle hooks for the given AutoScalingGroup after retrieving them from the AWS API. +// DescribeLifecycleHooks returns the lifecycle hooks for the given AutoScalingGroup after retrieving them from the AWS API. func (s *Service) DescribeLifecycleHooks(asgName string) ([]*expinfrav1.AWSLifecycleHook, error) { input := &autoscaling.DescribeLifecycleHooksInput{ AutoScalingGroupName: aws.String(asgName), diff --git a/pkg/cloud/services/eks/nodegroup.go b/pkg/cloud/services/eks/nodegroup.go index 239b884299..70cd05ad73 100644 --- a/pkg/cloud/services/eks/nodegroup.go +++ b/pkg/cloud/services/eks/nodegroup.go @@ -41,7 +41,8 @@ import ( "sigs.k8s.io/cluster-api/util/annotations" ) -// Built-in EKS lifecycle hooks should not be changed +// IgnoredEKSLifecycleHooks lists built-in EKS lifecycle hooks +// that should not be changed or deleted. var IgnoredEKSLifecycleHooks = map[string]bool{ "Launch-LC-Hook": true, "Terminate-LC-Hook": true, From a18c617e142c6ac58a0bcccac339a008bd26c8e4 Mon Sep 17 00:00:00 2001 From: Andreas Sommer Date: Thu, 21 Nov 2024 17:13:02 +0100 Subject: [PATCH 04/13] Add IAM permissions for lifecycle hooks --- .../cloudformation/bootstrap/cluster_api_controller.go | 3 +++ .../cloudformation/bootstrap/fixtures/customsuffix.yaml | 3 +++ .../cloudformation/bootstrap/fixtures/default.yaml | 3 +++ .../bootstrap/fixtures/with_all_secret_backends.yaml | 3 +++ .../bootstrap/fixtures/with_allow_assume_role.yaml | 3 +++ .../cloudformation/bootstrap/fixtures/with_bootstrap_user.yaml | 3 +++ .../bootstrap/fixtures/with_custom_bootstrap_user.yaml | 3 +++ .../bootstrap/fixtures/with_different_instance_profiles.yaml | 3 +++ .../cloudformation/bootstrap/fixtures/with_eks_console.yaml | 3 +++ .../bootstrap/fixtures/with_eks_default_roles.yaml | 3 +++ .../cloudformation/bootstrap/fixtures/with_eks_disable.yaml | 3 +++ .../cloudformation/bootstrap/fixtures/with_eks_kms_prefix.yaml | 3 +++ .../bootstrap/fixtures/with_extra_statements.yaml | 3 +++ .../cloudformation/bootstrap/fixtures/with_s3_bucket.yaml | 3 +++ .../bootstrap/fixtures/with_ssm_secret_backend.yaml | 3 +++ 15 files changed, 45 insertions(+) diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/cluster_api_controller.go b/cmd/clusterawsadm/cloudformation/bootstrap/cluster_api_controller.go index 049de10431..52ab2b5112 100644 --- a/cmd/clusterawsadm/cloudformation/bootstrap/cluster_api_controller.go +++ b/cmd/clusterawsadm/cloudformation/bootstrap/cluster_api_controller.go @@ -177,6 +177,9 @@ func (t Template) ControllersPolicy() *iamv1.PolicyDocument { "elasticloadbalancing:DeleteListener", "autoscaling:DescribeAutoScalingGroups", "autoscaling:DescribeInstanceRefreshes", + "autoscaling:DeleteLifecycleHook", + "autoscaling:DescribeLifecycleHooks", + "autoscaling:PutLifecycleHook", "ec2:CreateLaunchTemplate", "ec2:CreateLaunchTemplateVersion", "ec2:DescribeLaunchTemplates", diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/customsuffix.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/customsuffix.yaml index 7909fe12d5..6b4f907f72 100644 --- a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/customsuffix.yaml +++ b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/customsuffix.yaml @@ -237,6 +237,9 @@ Resources: - elasticloadbalancing:DeleteListener - autoscaling:DescribeAutoScalingGroups - autoscaling:DescribeInstanceRefreshes + - autoscaling:DeleteLifecycleHook + - autoscaling:DescribeLifecycleHooks + - autoscaling:PutLifecycleHook - ec2:CreateLaunchTemplate - ec2:CreateLaunchTemplateVersion - ec2:DescribeLaunchTemplates diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/default.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/default.yaml index a9290741ba..a3c9102ab2 100644 --- a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/default.yaml +++ b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/default.yaml @@ -237,6 +237,9 @@ Resources: - elasticloadbalancing:DeleteListener - autoscaling:DescribeAutoScalingGroups - autoscaling:DescribeInstanceRefreshes + - autoscaling:DeleteLifecycleHook + - autoscaling:DescribeLifecycleHooks + - autoscaling:PutLifecycleHook - ec2:CreateLaunchTemplate - ec2:CreateLaunchTemplateVersion - ec2:DescribeLaunchTemplates diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_all_secret_backends.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_all_secret_backends.yaml index fa7b5a4d95..3b0ced5ac5 100644 --- a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_all_secret_backends.yaml +++ b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_all_secret_backends.yaml @@ -243,6 +243,9 @@ Resources: - elasticloadbalancing:DeleteListener - autoscaling:DescribeAutoScalingGroups - autoscaling:DescribeInstanceRefreshes + - autoscaling:DeleteLifecycleHook + - autoscaling:DescribeLifecycleHooks + - autoscaling:PutLifecycleHook - ec2:CreateLaunchTemplate - ec2:CreateLaunchTemplateVersion - ec2:DescribeLaunchTemplates diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_allow_assume_role.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_allow_assume_role.yaml index 2390d86097..5cebf6e7ab 100644 --- a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_allow_assume_role.yaml +++ b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_allow_assume_role.yaml @@ -237,6 +237,9 @@ Resources: - elasticloadbalancing:DeleteListener - autoscaling:DescribeAutoScalingGroups - autoscaling:DescribeInstanceRefreshes + - autoscaling:DeleteLifecycleHook + - autoscaling:DescribeLifecycleHooks + - autoscaling:PutLifecycleHook - ec2:CreateLaunchTemplate - ec2:CreateLaunchTemplateVersion - ec2:DescribeLaunchTemplates diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_bootstrap_user.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_bootstrap_user.yaml index 930b879c2e..c71cb9d6ad 100644 --- a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_bootstrap_user.yaml +++ b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_bootstrap_user.yaml @@ -243,6 +243,9 @@ Resources: - elasticloadbalancing:DeleteListener - autoscaling:DescribeAutoScalingGroups - autoscaling:DescribeInstanceRefreshes + - autoscaling:DeleteLifecycleHook + - autoscaling:DescribeLifecycleHooks + - autoscaling:PutLifecycleHook - ec2:CreateLaunchTemplate - ec2:CreateLaunchTemplateVersion - ec2:DescribeLaunchTemplates diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_custom_bootstrap_user.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_custom_bootstrap_user.yaml index 50b9bb3182..aa3db2c042 100644 --- a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_custom_bootstrap_user.yaml +++ b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_custom_bootstrap_user.yaml @@ -243,6 +243,9 @@ Resources: - elasticloadbalancing:DeleteListener - autoscaling:DescribeAutoScalingGroups - autoscaling:DescribeInstanceRefreshes + - autoscaling:DeleteLifecycleHook + - autoscaling:DescribeLifecycleHooks + - autoscaling:PutLifecycleHook - ec2:CreateLaunchTemplate - ec2:CreateLaunchTemplateVersion - ec2:DescribeLaunchTemplates diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_different_instance_profiles.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_different_instance_profiles.yaml index 478967b404..9aea893cc7 100644 --- a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_different_instance_profiles.yaml +++ b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_different_instance_profiles.yaml @@ -237,6 +237,9 @@ Resources: - elasticloadbalancing:DeleteListener - autoscaling:DescribeAutoScalingGroups - autoscaling:DescribeInstanceRefreshes + - autoscaling:DeleteLifecycleHook + - autoscaling:DescribeLifecycleHooks + - autoscaling:PutLifecycleHook - ec2:CreateLaunchTemplate - ec2:CreateLaunchTemplateVersion - ec2:DescribeLaunchTemplates diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_console.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_console.yaml index ae2e279062..dea39d02d9 100644 --- a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_console.yaml +++ b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_console.yaml @@ -237,6 +237,9 @@ Resources: - elasticloadbalancing:DeleteListener - autoscaling:DescribeAutoScalingGroups - autoscaling:DescribeInstanceRefreshes + - autoscaling:DeleteLifecycleHook + - autoscaling:DescribeLifecycleHooks + - autoscaling:PutLifecycleHook - ec2:CreateLaunchTemplate - ec2:CreateLaunchTemplateVersion - ec2:DescribeLaunchTemplates diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_default_roles.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_default_roles.yaml index 3ca015276a..789f347fcc 100644 --- a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_default_roles.yaml +++ b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_default_roles.yaml @@ -237,6 +237,9 @@ Resources: - elasticloadbalancing:DeleteListener - autoscaling:DescribeAutoScalingGroups - autoscaling:DescribeInstanceRefreshes + - autoscaling:DeleteLifecycleHook + - autoscaling:DescribeLifecycleHooks + - autoscaling:PutLifecycleHook - ec2:CreateLaunchTemplate - ec2:CreateLaunchTemplateVersion - ec2:DescribeLaunchTemplates diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_disable.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_disable.yaml index 57c08e20cc..c092783d60 100644 --- a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_disable.yaml +++ b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_disable.yaml @@ -237,6 +237,9 @@ Resources: - elasticloadbalancing:DeleteListener - autoscaling:DescribeAutoScalingGroups - autoscaling:DescribeInstanceRefreshes + - autoscaling:DeleteLifecycleHook + - autoscaling:DescribeLifecycleHooks + - autoscaling:PutLifecycleHook - ec2:CreateLaunchTemplate - ec2:CreateLaunchTemplateVersion - ec2:DescribeLaunchTemplates diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_kms_prefix.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_kms_prefix.yaml index 0bacb55e5c..f34f670fbf 100644 --- a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_kms_prefix.yaml +++ b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_eks_kms_prefix.yaml @@ -237,6 +237,9 @@ Resources: - elasticloadbalancing:DeleteListener - autoscaling:DescribeAutoScalingGroups - autoscaling:DescribeInstanceRefreshes + - autoscaling:DeleteLifecycleHook + - autoscaling:DescribeLifecycleHooks + - autoscaling:PutLifecycleHook - ec2:CreateLaunchTemplate - ec2:CreateLaunchTemplateVersion - ec2:DescribeLaunchTemplates diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_extra_statements.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_extra_statements.yaml index b864e1c1b3..ad61a26906 100644 --- a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_extra_statements.yaml +++ b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_extra_statements.yaml @@ -243,6 +243,9 @@ Resources: - elasticloadbalancing:DeleteListener - autoscaling:DescribeAutoScalingGroups - autoscaling:DescribeInstanceRefreshes + - autoscaling:DeleteLifecycleHook + - autoscaling:DescribeLifecycleHooks + - autoscaling:PutLifecycleHook - ec2:CreateLaunchTemplate - ec2:CreateLaunchTemplateVersion - ec2:DescribeLaunchTemplates diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_s3_bucket.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_s3_bucket.yaml index b376d7cab8..9e84b2f223 100644 --- a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_s3_bucket.yaml +++ b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_s3_bucket.yaml @@ -237,6 +237,9 @@ Resources: - elasticloadbalancing:DeleteListener - autoscaling:DescribeAutoScalingGroups - autoscaling:DescribeInstanceRefreshes + - autoscaling:DeleteLifecycleHook + - autoscaling:DescribeLifecycleHooks + - autoscaling:PutLifecycleHook - ec2:CreateLaunchTemplate - ec2:CreateLaunchTemplateVersion - ec2:DescribeLaunchTemplates diff --git a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_ssm_secret_backend.yaml b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_ssm_secret_backend.yaml index edc07671d6..a11d38e58f 100644 --- a/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_ssm_secret_backend.yaml +++ b/cmd/clusterawsadm/cloudformation/bootstrap/fixtures/with_ssm_secret_backend.yaml @@ -237,6 +237,9 @@ Resources: - elasticloadbalancing:DeleteListener - autoscaling:DescribeAutoScalingGroups - autoscaling:DescribeInstanceRefreshes + - autoscaling:DeleteLifecycleHook + - autoscaling:DescribeLifecycleHooks + - autoscaling:PutLifecycleHook - ec2:CreateLaunchTemplate - ec2:CreateLaunchTemplateVersion - ec2:DescribeLaunchTemplates From 42003ff722187c151ffe815a311d94b233894a7b Mon Sep 17 00:00:00 2001 From: Andreas Sommer Date: Mon, 25 Nov 2024 19:06:43 +0100 Subject: [PATCH 05/13] Create lifecycle hooks together with ASG --- .../awsmachinepool_controller_test.go | 34 +++++++- .../services/autoscaling/autoscalinggroup.go | 81 +++++++----------- .../services/autoscaling/lifecyclehook.go | 83 +++++++++---------- 3 files changed, 100 insertions(+), 98 deletions(-) diff --git a/exp/controllers/awsmachinepool_controller_test.go b/exp/controllers/awsmachinepool_controller_test.go index cf7f063e92..8de7277ee8 100644 --- a/exp/controllers/awsmachinepool_controller_test.go +++ b/exp/controllers/awsmachinepool_controller_test.go @@ -304,7 +304,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { asgSvc.EXPECT().CreateASG(gomock.Any()).Return(&expinfrav1.AutoScalingGroup{ Name: "name", }, nil) - asgSvc.EXPECT().SuspendProcesses("name", []string{"Launch", "Terminate"}).Return(nil).AnyTimes().Times(0) + asgSvc.EXPECT().SuspendProcesses("name", []string{"Launch", "Terminate"}).Return(nil).Times(0) err := reconciler.reconcileNormal(context.Background(), ms, cs, cs) g.Expect(err).To(Succeed()) @@ -341,7 +341,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { "InstanceRefresh", "HealthCheck", "ReplaceUnhealthy", - })).Return(nil).AnyTimes().Times(1) + })).Return(nil).Times(1) err := reconciler.reconcileNormal(context.Background(), ms, cs, cs) g.Expect(err).To(Succeed()) @@ -373,8 +373,8 @@ func TestAWSMachinePoolReconciler(t *testing.T) { }, nil) asgSvc.EXPECT().SubnetIDs(gomock.Any()).Return([]string{}, nil).Times(1) asgSvc.EXPECT().UpdateASG(gomock.Any()).Return(nil).AnyTimes() - asgSvc.EXPECT().SuspendProcesses("name", []string{"Terminate"}).Return(nil).AnyTimes().Times(1) - asgSvc.EXPECT().ResumeProcesses("name", []string{"process3"}).Return(nil).AnyTimes().Times(1) + asgSvc.EXPECT().SuspendProcesses("name", []string{"Terminate"}).Return(nil).Times(1) + asgSvc.EXPECT().ResumeProcesses("name", []string{"process3"}).Return(nil).Times(1) err := reconciler.reconcileNormal(context.Background(), ms, cs, cs) g.Expect(err).To(Succeed()) @@ -813,6 +813,32 @@ func TestAWSMachinePoolReconciler(t *testing.T) { }) }) t.Run("Lifecycle Hooks", func(t *testing.T) { + t.Run("ASG created with lifecycle hooks", func(t *testing.T) { + g := NewWithT(t) + setup(t, g) + defer teardown(t, g) + + newLifecycleHook := expinfrav1.AWSLifecycleHook{ + Name: "new-hook", + LifecycleTransition: "autoscaling:EC2_INSTANCE_LAUNCHING", + } + ms.AWSMachinePool.Spec.AWSLifecycleHooks = append(ms.AWSMachinePool.Spec.AWSLifecycleHooks, newLifecycleHook) + + reconSvc.EXPECT().ReconcileLaunchTemplate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + + // New ASG must be created with lifecycle hooks (single AWS SDK call is enough) + // + // TODO: Since GetASGByName and CreateASG are both in the same interface, we can't inspect the actual + // `CreateAutoScalingGroupWithContext` requests parameters here. Make this better testable down to + // AWS SDK level and check `CreateAutoScalingGroupInput.LifecycleHookSpecificationList`. + asgSvc.EXPECT().GetASGByName(gomock.Any()).Return(nil, nil) + asgSvc.EXPECT().CreateASG(gomock.Any()).Return(&expinfrav1.AutoScalingGroup{ + Name: "name", + }, nil) + + err := reconciler.reconcileNormal(context.Background(), ms, cs, cs) + g.Expect(err).To(Succeed()) + }) t.Run("New lifecycle hook is added", func(t *testing.T) { g := NewWithT(t) setup(t, g) diff --git a/pkg/cloud/services/autoscaling/autoscalinggroup.go b/pkg/cloud/services/autoscaling/autoscalinggroup.go index e710fa768e..1c2c8c96d6 100644 --- a/pkg/cloud/services/autoscaling/autoscalinggroup.go +++ b/pkg/cloud/services/autoscaling/autoscalinggroup.go @@ -163,24 +163,17 @@ func (s *Service) CreateASG(machinePoolScope *scope.MachinePoolScope) (*expinfra return nil, fmt.Errorf("getting subnets for ASG: %w", err) } - input := &expinfrav1.AutoScalingGroup{ - Name: machinePoolScope.Name(), - MaxSize: machinePoolScope.AWSMachinePool.Spec.MaxSize, - MinSize: machinePoolScope.AWSMachinePool.Spec.MinSize, - Subnets: subnets, - DefaultCoolDown: machinePoolScope.AWSMachinePool.Spec.DefaultCoolDown, - DefaultInstanceWarmup: machinePoolScope.AWSMachinePool.Spec.DefaultInstanceWarmup, - CapacityRebalance: machinePoolScope.AWSMachinePool.Spec.CapacityRebalance, - MixedInstancesPolicy: machinePoolScope.AWSMachinePool.Spec.MixedInstancesPolicy, - } + name := machinePoolScope.Name() + s.scope.Info("Creating ASG", "name", name) // Default value of MachinePool replicas set by CAPI is 1. mpReplicas := *machinePoolScope.MachinePool.Spec.Replicas + var desiredCapacity *int32 // Check that MachinePool replicas number is between the minimum and maximum size of the AWSMachinePool. // Ignore the problem for externally managed clusters because MachinePool replicas will be updated to the right value automatically. if mpReplicas >= machinePoolScope.AWSMachinePool.Spec.MinSize && mpReplicas <= machinePoolScope.AWSMachinePool.Spec.MaxSize { - input.DesiredCapacity = &mpReplicas + desiredCapacity = &mpReplicas } else if !annotations.ReplicasManagedByExternalAutoscaler(machinePoolScope.MachinePool) { return nil, fmt.Errorf("incorrect number of replicas %d in MachinePool %v", mpReplicas, machinePoolScope.MachinePool.Name) } @@ -194,62 +187,46 @@ func (s *Service) CreateASG(machinePoolScope *scope.MachinePoolScope) (*expinfra // Set the cloud provider tag additionalTags[infrav1.ClusterAWSCloudProviderTagKey(s.scope.KubernetesClusterName())] = string(infrav1.ResourceLifecycleOwned) - input.Tags = infrav1.Build(infrav1.BuildParams{ - ClusterName: s.scope.KubernetesClusterName(), - Lifecycle: infrav1.ResourceLifecycleOwned, - Name: aws.String(machinePoolScope.Name()), - Role: aws.String("node"), - Additional: additionalTags, - }) - - s.scope.Info("Running instance") - if err := s.runPool(input, machinePoolScope.AWSMachinePool.Status.LaunchTemplateID); err != nil { - // Only record the failure event if the error is not related to failed dependencies. - // This is to avoid spamming failure events since the machine will be requeued by the actuator. - // if !awserrors.IsFailedDependency(errors.Cause(err)) { - // record.Warnf(scope.AWSMachinePool, "FailedCreate", "Failed to create instance: %v", err) - // } - s.scope.Error(err, "unable to create AutoScalingGroup") - return nil, err - } - record.Eventf(machinePoolScope.AWSMachinePool, "SuccessfulCreate", "Created new ASG: %s", machinePoolScope.Name()) - - return nil, nil -} - -func (s *Service) runPool(i *expinfrav1.AutoScalingGroup, launchTemplateID string) error { input := &autoscaling.CreateAutoScalingGroupInput{ - AutoScalingGroupName: aws.String(i.Name), - MaxSize: aws.Int64(int64(i.MaxSize)), - MinSize: aws.Int64(int64(i.MinSize)), - VPCZoneIdentifier: aws.String(strings.Join(i.Subnets, ", ")), - DefaultCooldown: aws.Int64(int64(i.DefaultCoolDown.Duration.Seconds())), - DefaultInstanceWarmup: aws.Int64(int64(i.DefaultInstanceWarmup.Duration.Seconds())), - CapacityRebalance: aws.Bool(i.CapacityRebalance), + AutoScalingGroupName: aws.String(name), + MaxSize: aws.Int64(int64(machinePoolScope.AWSMachinePool.Spec.MaxSize)), + MinSize: aws.Int64(int64(machinePoolScope.AWSMachinePool.Spec.MinSize)), + VPCZoneIdentifier: aws.String(strings.Join(subnets, ", ")), + DefaultCooldown: aws.Int64(int64(machinePoolScope.AWSMachinePool.Spec.DefaultCoolDown.Duration.Seconds())), + DefaultInstanceWarmup: aws.Int64(int64(machinePoolScope.AWSMachinePool.Spec.DefaultInstanceWarmup.Duration.Seconds())), + CapacityRebalance: aws.Bool(machinePoolScope.AWSMachinePool.Spec.CapacityRebalance), + LifecycleHookSpecificationList: getLifecycleHookSpecificationList(machinePoolScope.GetLifecycleHooks()), } - if i.DesiredCapacity != nil { - input.DesiredCapacity = aws.Int64(int64(aws.Int32Value(i.DesiredCapacity))) + if desiredCapacity != nil { + input.DesiredCapacity = aws.Int64(int64(aws.Int32Value(desiredCapacity))) } - if i.MixedInstancesPolicy != nil { - input.MixedInstancesPolicy = createSDKMixedInstancesPolicy(i.Name, i.MixedInstancesPolicy) + if machinePoolScope.AWSMachinePool.Spec.MixedInstancesPolicy != nil { + input.MixedInstancesPolicy = createSDKMixedInstancesPolicy(name, machinePoolScope.AWSMachinePool.Spec.MixedInstancesPolicy) } else { input.LaunchTemplate = &autoscaling.LaunchTemplateSpecification{ - LaunchTemplateId: aws.String(launchTemplateID), + LaunchTemplateId: aws.String(machinePoolScope.AWSMachinePool.Status.LaunchTemplateID), Version: aws.String(expinfrav1.LaunchTemplateLatestVersion), } } - if i.Tags != nil { - input.Tags = BuildTagsFromMap(i.Name, i.Tags) - } + input.Tags = BuildTagsFromMap(name, infrav1.Build(infrav1.BuildParams{ + ClusterName: s.scope.KubernetesClusterName(), + Lifecycle: infrav1.ResourceLifecycleOwned, + Name: aws.String(name), + Role: aws.String("node"), + Additional: additionalTags, + })) if _, err := s.ASGClient.CreateAutoScalingGroupWithContext(context.TODO(), input); err != nil { - return errors.Wrap(err, "failed to create autoscaling group") + s.scope.Error(err, "unable to create AutoScalingGroup") + return nil, errors.Wrap(err, "failed to create autoscaling group") } - return nil + record.Eventf(machinePoolScope.AWSMachinePool, "SuccessfulCreate", "Created new ASG: %s", machinePoolScope.Name()) + + return nil, nil } // DeleteASGAndWait will delete an ASG and wait until it is deleted. diff --git a/pkg/cloud/services/autoscaling/lifecyclehook.go b/pkg/cloud/services/autoscaling/lifecyclehook.go index b0fa055d96..d869761e77 100644 --- a/pkg/cloud/services/autoscaling/lifecyclehook.go +++ b/pkg/cloud/services/autoscaling/lifecyclehook.go @@ -51,35 +51,34 @@ func (s *Service) DescribeLifecycleHooks(asgName string) ([]*expinfrav1.AWSLifec return hooks, nil } -// CreateLifecycleHook creates a lifecycle hook for the given AutoScalingGroup. -func (s *Service) CreateLifecycleHook(asgName string, hook *expinfrav1.AWSLifecycleHook) error { - input := &autoscaling.PutLifecycleHookInput{ +func getPutLifecycleHookInput(asgName string, hook *expinfrav1.AWSLifecycleHook) (ret *autoscaling.PutLifecycleHookInput) { + ret = &autoscaling.PutLifecycleHookInput{ AutoScalingGroupName: aws.String(asgName), LifecycleHookName: aws.String(hook.Name), LifecycleTransition: aws.String(hook.LifecycleTransition.String()), + + // Optional + RoleARN: hook.RoleARN, + NotificationTargetARN: hook.NotificationTargetARN, + NotificationMetadata: hook.NotificationMetadata, } // Optional parameters if hook.DefaultResult != nil { - input.DefaultResult = aws.String(hook.DefaultResult.String()) + ret.DefaultResult = aws.String(hook.DefaultResult.String()) } if hook.HeartbeatTimeout != nil { timeoutSeconds := hook.HeartbeatTimeout.Duration.Seconds() - input.HeartbeatTimeout = aws.Int64(int64(timeoutSeconds)) + ret.HeartbeatTimeout = aws.Int64(int64(timeoutSeconds)) } - if hook.NotificationTargetARN != nil { - input.NotificationTargetARN = hook.NotificationTargetARN - } - - if hook.RoleARN != nil { - input.RoleARN = hook.RoleARN - } + return +} - if hook.NotificationMetadata != nil { - input.NotificationMetadata = hook.NotificationMetadata - } +// CreateLifecycleHook creates a lifecycle hook for the given AutoScalingGroup. +func (s *Service) CreateLifecycleHook(asgName string, hook *expinfrav1.AWSLifecycleHook) error { + input := getPutLifecycleHookInput(asgName, hook) if _, err := s.ASGClient.PutLifecycleHookWithContext(context.TODO(), input); err != nil { return errors.Wrapf(err, "failed to create lifecycle hook %q for AutoScalingGroup: %q", hook.Name, asgName) @@ -90,33 +89,7 @@ func (s *Service) CreateLifecycleHook(asgName string, hook *expinfrav1.AWSLifecy // UpdateLifecycleHook updates a lifecycle hook for the given AutoScalingGroup. func (s *Service) UpdateLifecycleHook(asgName string, hook *expinfrav1.AWSLifecycleHook) error { - input := &autoscaling.PutLifecycleHookInput{ - AutoScalingGroupName: aws.String(asgName), - LifecycleHookName: aws.String(hook.Name), - LifecycleTransition: aws.String(hook.LifecycleTransition.String()), - } - - // Optional parameters - if hook.DefaultResult != nil { - input.DefaultResult = aws.String(hook.DefaultResult.String()) - } - - if hook.HeartbeatTimeout != nil { - timeoutSeconds := hook.HeartbeatTimeout.Duration.Seconds() - input.HeartbeatTimeout = aws.Int64(int64(timeoutSeconds)) - } - - if hook.NotificationTargetARN != nil { - input.NotificationTargetARN = hook.NotificationTargetARN - } - - if hook.RoleARN != nil { - input.RoleARN = hook.RoleARN - } - - if hook.NotificationMetadata != nil { - input.NotificationMetadata = hook.NotificationMetadata - } + input := getPutLifecycleHookInput(asgName, hook) if _, err := s.ASGClient.PutLifecycleHookWithContext(context.TODO(), input); err != nil { return errors.Wrapf(err, "failed to update lifecycle hook %q for AutoScalingGroup: %q", hook.Name, asgName) @@ -160,6 +133,32 @@ func (s *Service) SDKToLifecycleHook(hook *autoscaling.LifecycleHook) *expinfrav } } +func getLifecycleHookSpecificationList(lifecycleHooks []expinfrav1.AWSLifecycleHook) (ret []*autoscaling.LifecycleHookSpecification) { + for _, hook := range lifecycleHooks { + spec := &autoscaling.LifecycleHookSpecification{ + LifecycleHookName: aws.String(hook.Name), + LifecycleTransition: aws.String(hook.LifecycleTransition.String()), + + // Optional + RoleARN: hook.RoleARN, + NotificationTargetARN: hook.NotificationTargetARN, + NotificationMetadata: hook.NotificationMetadata, + } + + // Optional parameters + if hook.DefaultResult != nil { + spec.DefaultResult = aws.String(hook.DefaultResult.String()) + } + + if hook.HeartbeatTimeout != nil { + timeoutSeconds := hook.HeartbeatTimeout.Duration.Seconds() + spec.HeartbeatTimeout = aws.Int64(int64(timeoutSeconds)) + } + } + + return +} + // ReconcileLifecycleHooks reconciles lifecycle hooks for an ASG // by creating missing hooks, updating mismatching hooks and // deleting extraneous hooks (except those specified in From 38754322dad89272e2468e0cd999af7b0b8e26d8 Mon Sep 17 00:00:00 2001 From: Andreas Sommer Date: Tue, 26 Nov 2024 17:56:14 +0100 Subject: [PATCH 06/13] Pass down context --- exp/controllers/awsmachinepool_controller.go | 6 ++--- .../awsmachinepool_controller_test.go | 10 +++---- .../services/autoscaling/lifecyclehook.go | 27 +++++++++---------- pkg/cloud/services/eks/nodegroup.go | 6 ++--- pkg/cloud/services/interfaces.go | 6 ++--- .../autoscaling_interface_mock.go | 25 ++++++++--------- 6 files changed, 39 insertions(+), 41 deletions(-) diff --git a/exp/controllers/awsmachinepool_controller.go b/exp/controllers/awsmachinepool_controller.go index 7dfa757c38..bd69e785dd 100644 --- a/exp/controllers/awsmachinepool_controller.go +++ b/exp/controllers/awsmachinepool_controller.go @@ -298,7 +298,7 @@ func (r *AWSMachinePoolReconciler) reconcileNormal(ctx context.Context, machineP return nil } - if err := r.reconcileLifecycleHooks(machinePoolScope, asgsvc); err != nil { + if err := r.reconcileLifecycleHooks(ctx, 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") } @@ -613,10 +613,10 @@ func machinePoolToInfrastructureMapFunc(gvk schema.GroupVersionKind) handler.Map } // reconcileLifecycleHooks periodically reconciles a lifecycle hook for the ASG. -func (r *AWSMachinePoolReconciler) reconcileLifecycleHooks(machinePoolScope *scope.MachinePoolScope, asgsvc services.ASGInterface) error { +func (r *AWSMachinePoolReconciler) reconcileLifecycleHooks(ctx context.Context, machinePoolScope *scope.MachinePoolScope, asgsvc services.ASGInterface) error { asgName := machinePoolScope.Name() - return asg.ReconcileLifecycleHooks(asgsvc, asgName, machinePoolScope.GetLifecycleHooks(), map[string]bool{}, machinePoolScope.GetMachinePool(), machinePoolScope) + return asg.ReconcileLifecycleHooks(ctx, 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) { diff --git a/exp/controllers/awsmachinepool_controller_test.go b/exp/controllers/awsmachinepool_controller_test.go index 8de7277ee8..4fe6cc1dfe 100644 --- a/exp/controllers/awsmachinepool_controller_test.go +++ b/exp/controllers/awsmachinepool_controller_test.go @@ -852,7 +852,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { reconSvc.EXPECT().ReconcileLaunchTemplate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) asgSvc.EXPECT().DescribeLifecycleHooks(gomock.Eq(ms.Name())).Return(nil, nil) - asgSvc.EXPECT().CreateLifecycleHook(ms.Name(), &newLifecycleHook).Return(nil) + asgSvc.EXPECT().CreateLifecycleHook(gomock.Any(), 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) { g.Expect(scope.Name()).To(Equal("test")) @@ -887,7 +887,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { LifecycleTransition: "autoscaling:EC2_INSTANCE_LAUNCHING", }, }, nil) - asgSvc.EXPECT().DeleteLifecycleHook(ms.Name(), &expinfrav1.AWSLifecycleHook{ + asgSvc.EXPECT().DeleteLifecycleHook(gomock.Any(), ms.Name(), &expinfrav1.AWSLifecycleHook{ Name: "hook-to-remove", LifecycleTransition: "autoscaling:EC2_INSTANCE_LAUNCHING", }).Return(nil) @@ -930,8 +930,8 @@ func TestAWSMachinePoolReconciler(t *testing.T) { LifecycleTransition: "autoscaling:EC2_INSTANCE_LAUNCHING", }, }, nil) - asgSvc.EXPECT().CreateLifecycleHook(ms.Name(), &newLifecycleHook).Return(nil) - asgSvc.EXPECT().DeleteLifecycleHook(ms.Name(), &expinfrav1.AWSLifecycleHook{ + asgSvc.EXPECT().CreateLifecycleHook(gomock.Any(), ms.Name(), &newLifecycleHook).Return(nil) + asgSvc.EXPECT().DeleteLifecycleHook(gomock.Any(), ms.Name(), &expinfrav1.AWSLifecycleHook{ Name: "hook-to-remove", LifecycleTransition: "autoscaling:EC2_INSTANCE_LAUNCHING", }).Return(nil) @@ -974,7 +974,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { LifecycleTransition: "autoscaling:EC2_INSTANCE_LAUNCHING", }, }, nil) - asgSvc.EXPECT().UpdateLifecycleHook(ms.Name(), &updateLifecycleHook).Return(nil) + asgSvc.EXPECT().UpdateLifecycleHook(gomock.Any(), 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")) diff --git a/pkg/cloud/services/autoscaling/lifecyclehook.go b/pkg/cloud/services/autoscaling/lifecyclehook.go index d869761e77..97e6be51ac 100644 --- a/pkg/cloud/services/autoscaling/lifecyclehook.go +++ b/pkg/cloud/services/autoscaling/lifecyclehook.go @@ -77,10 +77,10 @@ func getPutLifecycleHookInput(asgName string, hook *expinfrav1.AWSLifecycleHook) } // CreateLifecycleHook creates a lifecycle hook for the given AutoScalingGroup. -func (s *Service) CreateLifecycleHook(asgName string, hook *expinfrav1.AWSLifecycleHook) error { +func (s *Service) CreateLifecycleHook(ctx context.Context, asgName string, hook *expinfrav1.AWSLifecycleHook) error { input := getPutLifecycleHookInput(asgName, hook) - if _, err := s.ASGClient.PutLifecycleHookWithContext(context.TODO(), input); err != nil { + if _, err := s.ASGClient.PutLifecycleHookWithContext(ctx, input); err != nil { return errors.Wrapf(err, "failed to create lifecycle hook %q for AutoScalingGroup: %q", hook.Name, asgName) } @@ -88,10 +88,10 @@ func (s *Service) CreateLifecycleHook(asgName string, hook *expinfrav1.AWSLifecy } // UpdateLifecycleHook updates a lifecycle hook for the given AutoScalingGroup. -func (s *Service) UpdateLifecycleHook(asgName string, hook *expinfrav1.AWSLifecycleHook) error { +func (s *Service) UpdateLifecycleHook(ctx context.Context, asgName string, hook *expinfrav1.AWSLifecycleHook) error { input := getPutLifecycleHookInput(asgName, hook) - if _, err := s.ASGClient.PutLifecycleHookWithContext(context.TODO(), input); err != nil { + if _, err := s.ASGClient.PutLifecycleHookWithContext(ctx, input); err != nil { return errors.Wrapf(err, "failed to update lifecycle hook %q for AutoScalingGroup: %q", hook.Name, asgName) } @@ -99,16 +99,13 @@ func (s *Service) UpdateLifecycleHook(asgName string, hook *expinfrav1.AWSLifecy } // DeleteLifecycleHook deletes a lifecycle hook for the given AutoScalingGroup. -func (s *Service) DeleteLifecycleHook( - asgName string, - hook *expinfrav1.AWSLifecycleHook, -) error { +func (s *Service) DeleteLifecycleHook(ctx context.Context, asgName string, hook *expinfrav1.AWSLifecycleHook) error { input := &autoscaling.DeleteLifecycleHookInput{ AutoScalingGroupName: aws.String(asgName), LifecycleHookName: aws.String(hook.Name), } - if _, err := s.ASGClient.DeleteLifecycleHookWithContext(context.TODO(), input); err != nil { + if _, err := s.ASGClient.DeleteLifecycleHookWithContext(ctx, input); err != nil { return errors.Wrapf(err, "failed to delete lifecycle hook %q for AutoScalingGroup: %q", hook.Name, asgName) } @@ -163,7 +160,7 @@ func getLifecycleHookSpecificationList(lifecycleHooks []expinfrav1.AWSLifecycleH // by creating missing hooks, updating mismatching hooks and // deleting extraneous hooks (except those specified in // ignoreLifecycleHooks). -func ReconcileLifecycleHooks(asgService services.ASGInterface, asgName string, wantedLifecycleHooks []expinfrav1.AWSLifecycleHook, ignoreLifecycleHooks map[string]bool, storeConditionsOnObject conditions.Setter, log logger.Wrapper) error { +func ReconcileLifecycleHooks(ctx context.Context, asgService services.ASGInterface, asgName string, wantedLifecycleHooks []expinfrav1.AWSLifecycleHook, ignoreLifecycleHooks map[string]bool, storeConditionsOnObject conditions.Setter, log logger.Wrapper) error { existingHooks, err := asgService.DescribeLifecycleHooks(asgName) if err != nil { return err @@ -175,7 +172,7 @@ func ReconcileLifecycleHooks(asgService services.ASGInterface, asgName string, w continue } - if err := reconcileLifecycleHook(asgService, asgName, &wantedLifecycleHooks[i], existingHooks, storeConditionsOnObject, log); err != nil { + if err := reconcileLifecycleHook(ctx, asgService, asgName, &wantedLifecycleHooks[i], existingHooks, storeConditionsOnObject, log); err != nil { return err } } @@ -193,7 +190,7 @@ func ReconcileLifecycleHooks(asgService services.ASGInterface, asgName string, w } if !found { log.Info("Deleting extraneous lifecycle hook", "hook", existingHook.Name) - if err := asgService.DeleteLifecycleHook(asgName, existingHook); err != nil { + if err := asgService.DeleteLifecycleHook(ctx, asgName, existingHook); err != nil { conditions.MarkFalse(storeConditionsOnObject, expinfrav1.LifecycleHookReadyCondition, expinfrav1.LifecycleHookDeletionFailedReason, clusterv1.ConditionSeverityError, err.Error()) return err } @@ -211,7 +208,7 @@ func lifecycleHookNeedsUpdate(existing *expinfrav1.AWSLifecycleHook, expected *e existing.NotificationMetadata != expected.NotificationMetadata } -func reconcileLifecycleHook(asgService services.ASGInterface, asgName string, wantedHook *expinfrav1.AWSLifecycleHook, existingHooks []*expinfrav1.AWSLifecycleHook, storeConditionsOnObject conditions.Setter, log logger.Wrapper) error { +func reconcileLifecycleHook(ctx context.Context, asgService services.ASGInterface, asgName string, wantedHook *expinfrav1.AWSLifecycleHook, existingHooks []*expinfrav1.AWSLifecycleHook, storeConditionsOnObject conditions.Setter, log logger.Wrapper) error { log = log.WithValues("hook", wantedHook.Name) log.Info("Checking for existing lifecycle hook") @@ -225,7 +222,7 @@ func reconcileLifecycleHook(asgService services.ASGInterface, asgName string, wa if existingHook == nil { log.Info("Creating lifecycle hook") - if err := asgService.CreateLifecycleHook(asgName, wantedHook); err != nil { + if err := asgService.CreateLifecycleHook(ctx, asgName, wantedHook); err != nil { conditions.MarkFalse(storeConditionsOnObject, expinfrav1.LifecycleHookReadyCondition, expinfrav1.LifecycleHookCreationFailedReason, clusterv1.ConditionSeverityError, err.Error()) return err } @@ -234,7 +231,7 @@ func reconcileLifecycleHook(asgService services.ASGInterface, asgName string, wa if lifecycleHookNeedsUpdate(existingHook, wantedHook) { log.Info("Updating lifecycle hook") - if err := asgService.UpdateLifecycleHook(asgName, wantedHook); err != nil { + if err := asgService.UpdateLifecycleHook(ctx, asgName, wantedHook); err != nil { conditions.MarkFalse(storeConditionsOnObject, expinfrav1.LifecycleHookReadyCondition, expinfrav1.LifecycleHookUpdateFailedReason, clusterv1.ConditionSeverityError, err.Error()) return err } diff --git a/pkg/cloud/services/eks/nodegroup.go b/pkg/cloud/services/eks/nodegroup.go index 70cd05ad73..17518e033e 100644 --- a/pkg/cloud/services/eks/nodegroup.go +++ b/pkg/cloud/services/eks/nodegroup.go @@ -579,7 +579,7 @@ func (s *NodegroupService) reconcileNodegroup(ctx context.Context) error { return errors.Wrapf(err, "failed to reconcile asg tags") } - if err := s.reconcileLifecycleHooks(ng); err != nil { + if err := s.reconcileLifecycleHooks(ctx, ng); err != nil { return errors.Wrapf(err, "failed to reconcile lifecyle hooks") } @@ -659,10 +659,10 @@ func (s *NodegroupService) waitForNodegroupActive() (*eks.Nodegroup, error) { } // reconcileLifecycleHooks periodically reconciles a lifecycle hook for the ASG. -func (s *NodegroupService) reconcileLifecycleHooks(ng *eks.Nodegroup) error { +func (s *NodegroupService) reconcileLifecycleHooks(ctx context.Context, ng *eks.Nodegroup) error { if len(ng.Resources.AutoScalingGroups) == 0 { return errors.New("no ASG defined for node group") } - return asg.ReconcileLifecycleHooks(s.ASGService, *ng.Resources.AutoScalingGroups[0].Name, s.scope.GetLifecycleHooks(), IgnoredEKSLifecycleHooks, s.scope.GetMachinePool(), s.scope) + return asg.ReconcileLifecycleHooks(ctx, s.ASGService, *ng.Resources.AutoScalingGroups[0].Name, s.scope.GetLifecycleHooks(), IgnoredEKSLifecycleHooks, s.scope.GetMachinePool(), s.scope) } diff --git a/pkg/cloud/services/interfaces.go b/pkg/cloud/services/interfaces.go index 3a63a28ebb..06ee272539 100644 --- a/pkg/cloud/services/interfaces.go +++ b/pkg/cloud/services/interfaces.go @@ -51,9 +51,9 @@ type ASGInterface interface { ResumeProcesses(name string, processes []string) error SubnetIDs(scope *scope.MachinePoolScope) ([]string, error) DescribeLifecycleHooks(asgName string) ([]*expinfrav1.AWSLifecycleHook, error) - CreateLifecycleHook(asgName string, hook *expinfrav1.AWSLifecycleHook) error - UpdateLifecycleHook(asgName string, hook *expinfrav1.AWSLifecycleHook) error - DeleteLifecycleHook(asgName string, hook *expinfrav1.AWSLifecycleHook) error + CreateLifecycleHook(ctx context.Context, asgName string, hook *expinfrav1.AWSLifecycleHook) error + UpdateLifecycleHook(ctx context.Context, asgName string, hook *expinfrav1.AWSLifecycleHook) error + DeleteLifecycleHook(ctx context.Context, asgName string, hook *expinfrav1.AWSLifecycleHook) error } // EC2Interface encapsulates the methods exposed to the machine diff --git a/pkg/cloud/services/mock_services/autoscaling_interface_mock.go b/pkg/cloud/services/mock_services/autoscaling_interface_mock.go index 1f936dfb9e..fcee553a07 100644 --- a/pkg/cloud/services/mock_services/autoscaling_interface_mock.go +++ b/pkg/cloud/services/mock_services/autoscaling_interface_mock.go @@ -21,6 +21,7 @@ limitations under the License. package mock_services import ( + context "context" reflect "reflect" gomock "github.com/golang/mock/gomock" @@ -97,17 +98,17 @@ func (mr *MockASGInterfaceMockRecorder) CreateASG(arg0 interface{}) *gomock.Call } // CreateLifecycleHook mocks base method. -func (m *MockASGInterface) CreateLifecycleHook(arg0 string, arg1 *v1beta2.AWSLifecycleHook) error { +func (m *MockASGInterface) CreateLifecycleHook(arg0 context.Context, arg1 string, arg2 *v1beta2.AWSLifecycleHook) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateLifecycleHook", arg0, arg1) + ret := m.ctrl.Call(m, "CreateLifecycleHook", arg0, arg1, arg2) ret0, _ := ret[0].(error) return ret0 } // CreateLifecycleHook indicates an expected call of CreateLifecycleHook. -func (mr *MockASGInterfaceMockRecorder) CreateLifecycleHook(arg0, arg1 interface{}) *gomock.Call { +func (mr *MockASGInterfaceMockRecorder) CreateLifecycleHook(arg0, arg1, arg2 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateLifecycleHook", reflect.TypeOf((*MockASGInterface)(nil).CreateLifecycleHook), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateLifecycleHook", reflect.TypeOf((*MockASGInterface)(nil).CreateLifecycleHook), arg0, arg1, arg2) } // DeleteASGAndWait mocks base method. @@ -125,17 +126,17 @@ func (mr *MockASGInterfaceMockRecorder) DeleteASGAndWait(arg0 interface{}) *gomo } // DeleteLifecycleHook mocks base method. -func (m *MockASGInterface) DeleteLifecycleHook(arg0 string, arg1 *v1beta2.AWSLifecycleHook) error { +func (m *MockASGInterface) DeleteLifecycleHook(arg0 context.Context, arg1 string, arg2 *v1beta2.AWSLifecycleHook) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteLifecycleHook", arg0, arg1) + ret := m.ctrl.Call(m, "DeleteLifecycleHook", arg0, arg1, arg2) ret0, _ := ret[0].(error) return ret0 } // DeleteLifecycleHook indicates an expected call of DeleteLifecycleHook. -func (mr *MockASGInterfaceMockRecorder) DeleteLifecycleHook(arg0, arg1 interface{}) *gomock.Call { +func (mr *MockASGInterfaceMockRecorder) DeleteLifecycleHook(arg0, arg1, arg2 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteLifecycleHook", reflect.TypeOf((*MockASGInterface)(nil).DeleteLifecycleHook), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteLifecycleHook", reflect.TypeOf((*MockASGInterface)(nil).DeleteLifecycleHook), arg0, arg1, arg2) } // DescribeLifecycleHooks mocks base method. @@ -240,17 +241,17 @@ func (mr *MockASGInterfaceMockRecorder) UpdateASG(arg0 interface{}) *gomock.Call } // UpdateLifecycleHook mocks base method. -func (m *MockASGInterface) UpdateLifecycleHook(arg0 string, arg1 *v1beta2.AWSLifecycleHook) error { +func (m *MockASGInterface) UpdateLifecycleHook(arg0 context.Context, arg1 string, arg2 *v1beta2.AWSLifecycleHook) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "UpdateLifecycleHook", arg0, arg1) + ret := m.ctrl.Call(m, "UpdateLifecycleHook", arg0, arg1, arg2) ret0, _ := ret[0].(error) return ret0 } // UpdateLifecycleHook indicates an expected call of UpdateLifecycleHook. -func (mr *MockASGInterfaceMockRecorder) UpdateLifecycleHook(arg0, arg1 interface{}) *gomock.Call { +func (mr *MockASGInterfaceMockRecorder) UpdateLifecycleHook(arg0, arg1, arg2 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateLifecycleHook", reflect.TypeOf((*MockASGInterface)(nil).UpdateLifecycleHook), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateLifecycleHook", reflect.TypeOf((*MockASGInterface)(nil).UpdateLifecycleHook), arg0, arg1, arg2) } // UpdateResourceTags mocks base method. From deabb0e0fb93f79a25bb7bc79c0bb19a6ebed58d Mon Sep 17 00:00:00 2001 From: Andreas Sommer Date: Tue, 26 Nov 2024 18:12:27 +0100 Subject: [PATCH 07/13] Add webhook tests for all lifecycle hook fields and ensure the test covers what we think it does --- .../v1beta2/awsmachinepool_webhook_test.go | 129 +++++++++++++----- 1 file changed, 94 insertions(+), 35 deletions(-) diff --git a/exp/api/v1beta2/awsmachinepool_webhook_test.go b/exp/api/v1beta2/awsmachinepool_webhook_test.go index fa1985f653..bdb5755b79 100644 --- a/exp/api/v1beta2/awsmachinepool_webhook_test.go +++ b/exp/api/v1beta2/awsmachinepool_webhook_test.go @@ -42,9 +42,9 @@ func TestAWSMachinePoolValidateCreate(t *testing.T) { g := NewWithT(t) tests := []struct { - name string - pool *AWSMachinePool - wantErr bool + name string + pool *AWSMachinePool + wantErrToContain *string }{ { name: "pool with valid tags is accepted", @@ -56,8 +56,7 @@ func TestAWSMachinePoolValidateCreate(t *testing.T) { }, }, }, - - wantErr: false, + wantErrToContain: nil, }, { name: "invalid tags are rejected", @@ -71,7 +70,7 @@ func TestAWSMachinePoolValidateCreate(t *testing.T) { }, }, }, - wantErr: true, + wantErrToContain: ptr.To[string]("additionalTags"), }, { name: "Should fail if additional security groups are provided with both ID and Filters", @@ -88,7 +87,7 @@ func TestAWSMachinePoolValidateCreate(t *testing.T) { }}}, }, }, - wantErr: true, + wantErrToContain: ptr.To[string]("filter"), }, { name: "Should fail if both subnet ID and filters passed in AWSMachinePool spec", @@ -106,7 +105,7 @@ func TestAWSMachinePoolValidateCreate(t *testing.T) { }, }, }, - wantErr: true, + wantErrToContain: ptr.To[string]("filter"), }, { name: "Should pass if either subnet ID or filters passed in AWSMachinePool spec", @@ -123,7 +122,7 @@ func TestAWSMachinePoolValidateCreate(t *testing.T) { }, }, }, - wantErr: false, + wantErrToContain: nil, }, { name: "Ensure root volume with device name works (for clusterctl move)", @@ -138,7 +137,7 @@ func TestAWSMachinePoolValidateCreate(t *testing.T) { }, }, }, - wantErr: false, + wantErrToContain: nil, }, { name: "Should fail if both spot market options or mixed instances policy are set", @@ -152,7 +151,7 @@ func TestAWSMachinePoolValidateCreate(t *testing.T) { }, }, }, - wantErr: true, + wantErrToContain: ptr.To[string]("spotMarketOptions"), }, { name: "Should fail if MaxHealthyPercentage is set, but MinHealthyPercentage is not set", @@ -161,7 +160,7 @@ func TestAWSMachinePoolValidateCreate(t *testing.T) { RefreshPreferences: &RefreshPreferences{MaxHealthyPercentage: aws.Int64(100)}, }, }, - wantErr: true, + wantErrToContain: ptr.To[string]("minHealthyPercentage"), }, { name: "Should fail if the difference between MaxHealthyPercentage and MinHealthyPercentage is greater than 100", @@ -173,41 +172,98 @@ func TestAWSMachinePoolValidateCreate(t *testing.T) { }, }, }, - wantErr: true, + wantErrToContain: ptr.To[string]("minHealthyPercentage"), + }, + { + name: "Should fail if lifecycle hook only has roleARN, but not notificationTargetARN", + pool: &AWSMachinePool{ + Spec: AWSMachinePoolSpec{ + AWSLifecycleHooks: []AWSLifecycleHook{ + { + Name: "the-hook", + LifecycleTransition: LifecycleHookTransitionInstanceTerminating, + RoleARN: aws.String("role-arn"), + }, + }, + }, + }, + wantErrToContain: ptr.To[string]("notificationTargetARN"), + }, + { + name: "Should fail if lifecycle hook only has notificationTargetARN, but not roleARN", + pool: &AWSMachinePool{ + Spec: AWSMachinePoolSpec{ + AWSLifecycleHooks: []AWSLifecycleHook{ + { + Name: "the-hook", + LifecycleTransition: LifecycleHookTransitionInstanceTerminating, + NotificationTargetARN: aws.String("notification-target-arn"), + }, + }, + }, + }, + wantErrToContain: ptr.To[string]("roleARN"), + }, + { + name: "Should fail if the lifecycle hook heartbeat timeout is less than 30 seconds", + pool: &AWSMachinePool{ + Spec: AWSMachinePoolSpec{ + AWSLifecycleHooks: []AWSLifecycleHook{ + { + Name: "the-hook", + LifecycleTransition: LifecycleHookTransitionInstanceTerminating, + NotificationTargetARN: aws.String("notification-target-arn"), + RoleARN: aws.String("role-arn"), + HeartbeatTimeout: &metav1.Duration{Duration: 29 * time.Second}, + }, + }, + }, + }, + wantErrToContain: ptr.To[string]("heartbeatTimeout"), }, { - name: "Should fail if either roleARN or notifcationARN is set but not both", + name: "Should fail if the lifecycle hook heartbeat timeout is more than 172800 seconds", pool: &AWSMachinePool{ Spec: AWSMachinePoolSpec{ AWSLifecycleHooks: []AWSLifecycleHook{ { - RoleARN: aws.String("role-arn"), + Name: "the-hook", + LifecycleTransition: LifecycleHookTransitionInstanceTerminating, + NotificationTargetARN: aws.String("notification-target-arn"), + RoleARN: aws.String("role-arn"), + HeartbeatTimeout: &metav1.Duration{Duration: 172801 * time.Second}, }, }, }, }, - wantErr: true, + wantErrToContain: ptr.To[string]("heartbeatTimeout"), }, { - name: "Should fail if the heartbeat timeout is less than 30 seconds", + name: "Should succeed on correct lifecycle hook", pool: &AWSMachinePool{ Spec: AWSMachinePoolSpec{ AWSLifecycleHooks: []AWSLifecycleHook{ { - RoleARN: aws.String("role-arn"), - HeartbeatTimeout: &metav1.Duration{Duration: 29 * time.Second}, + Name: "the-hook", + LifecycleTransition: LifecycleHookTransitionInstanceTerminating, + NotificationTargetARN: aws.String("notification-target-arn"), + RoleARN: aws.String("role-arn"), + HeartbeatTimeout: &metav1.Duration{Duration: 180 * time.Second}, }, }, }, }, - wantErr: true, + wantErrToContain: nil, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { warn, err := tt.pool.ValidateCreate() - if tt.wantErr { - g.Expect(err).To(HaveOccurred()) + if tt.wantErrToContain != nil { + g.Expect(err).ToNot(BeNil()) + if err != nil { + g.Expect(err.Error()).To(ContainSubstring(*tt.wantErrToContain)) + } } else { g.Expect(err).To(Succeed()) } @@ -221,10 +277,10 @@ func TestAWSMachinePoolValidateUpdate(t *testing.T) { g := NewWithT(t) tests := []struct { - name string - new *AWSMachinePool - old *AWSMachinePool - wantErr bool + name string + new *AWSMachinePool + old *AWSMachinePool + wantErrToContain *string }{ { name: "adding tags is accepted", @@ -243,7 +299,7 @@ func TestAWSMachinePoolValidateUpdate(t *testing.T) { }, }, }, - wantErr: false, + wantErrToContain: nil, }, { name: "adding invalid tags is rejected", @@ -264,7 +320,7 @@ func TestAWSMachinePoolValidateUpdate(t *testing.T) { }, }, }, - wantErr: true, + wantErrToContain: ptr.To[string]("additionalTags"), }, { name: "Should fail update if both subnetID and filters passed in AWSMachinePool spec", @@ -289,7 +345,7 @@ func TestAWSMachinePoolValidateUpdate(t *testing.T) { }, }, }, - wantErr: true, + wantErrToContain: ptr.To[string]("filter"), }, { name: "Should pass update if either subnetID or filters passed in AWSMachinePool spec", @@ -313,7 +369,7 @@ func TestAWSMachinePoolValidateUpdate(t *testing.T) { }, }, }, - wantErr: false, + wantErrToContain: nil, }, { name: "Should fail update if both spec.awsLaunchTemplate.SpotMarketOptions and spec.MixedInstancesPolicy are passed in AWSMachinePool spec", @@ -334,7 +390,7 @@ func TestAWSMachinePoolValidateUpdate(t *testing.T) { }, }, }, - wantErr: true, + wantErrToContain: ptr.To[string]("spotMarketOptions"), }, { name: "Should fail if MaxHealthyPercentage is set, but MinHealthyPercentage is not set", @@ -343,7 +399,7 @@ func TestAWSMachinePoolValidateUpdate(t *testing.T) { RefreshPreferences: &RefreshPreferences{MaxHealthyPercentage: aws.Int64(100)}, }, }, - wantErr: true, + wantErrToContain: ptr.To[string]("minHealthyPercentage"), }, { name: "Should fail if the difference between MaxHealthyPercentage and MinHealthyPercentage is greater than 100", @@ -355,14 +411,17 @@ func TestAWSMachinePoolValidateUpdate(t *testing.T) { }, }, }, - wantErr: true, + wantErrToContain: ptr.To[string]("minHealthyPercentage"), }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { warn, err := tt.new.ValidateUpdate(tt.old.DeepCopy()) - if tt.wantErr { - g.Expect(err).To(HaveOccurred()) + if tt.wantErrToContain != nil { + g.Expect(err).ToNot(BeNil()) + if err != nil { + g.Expect(err.Error()).To(ContainSubstring(*tt.wantErrToContain)) + } } else { g.Expect(err).To(Succeed()) } From 530f1f2f8fb5420fdd0a7970e5cda4ebfdb5a5d2 Mon Sep 17 00:00:00 2001 From: Andreas Sommer Date: Tue, 26 Nov 2024 18:54:28 +0100 Subject: [PATCH 08/13] Undo whitespace-only change (merge mistake) --- exp/api/v1beta2/awsmachinepool_webhook.go | 1 + 1 file changed, 1 insertion(+) diff --git a/exp/api/v1beta2/awsmachinepool_webhook.go b/exp/api/v1beta2/awsmachinepool_webhook.go index 085d2f62f5..0ef2e001e3 100644 --- a/exp/api/v1beta2/awsmachinepool_webhook.go +++ b/exp/api/v1beta2/awsmachinepool_webhook.go @@ -133,6 +133,7 @@ func (r *AWSMachinePool) validateAdditionalSecurityGroups() field.ErrorList { } return allErrs } + func (r *AWSMachinePool) validateSpotInstances() field.ErrorList { var allErrs field.ErrorList if r.Spec.AWSLaunchTemplate.SpotMarketOptions != nil && r.Spec.MixedInstancesPolicy != nil { From 2bafaa97c1463a2c7d0b8c6012c50bd011e17f27 Mon Sep 17 00:00:00 2001 From: Andreas Sommer Date: Wed, 27 Nov 2024 13:18:11 +0100 Subject: [PATCH 09/13] Minor test EXPECT() improvement --- exp/controllers/awsmachinepool_controller_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/exp/controllers/awsmachinepool_controller_test.go b/exp/controllers/awsmachinepool_controller_test.go index 4fe6cc1dfe..287ebbe7a1 100644 --- a/exp/controllers/awsmachinepool_controller_test.go +++ b/exp/controllers/awsmachinepool_controller_test.go @@ -599,7 +599,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { MixedInstancesPolicy: awsMachinePool.Spec.MixedInstancesPolicy.DeepCopy(), }, nil }) - asgSvc.EXPECT().DescribeLifecycleHooks(gomock.Any()) + 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) @@ -656,7 +656,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { MixedInstancesPolicy: awsMachinePool.Spec.MixedInstancesPolicy.DeepCopy(), }, nil }) - asgSvc.EXPECT().DescribeLifecycleHooks(gomock.Any()) + 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) @@ -741,7 +741,7 @@ func TestAWSMachinePoolReconciler(t *testing.T) { MixedInstancesPolicy: awsMachinePool.Spec.MixedInstancesPolicy.DeepCopy(), }, nil }) - asgSvc.EXPECT().DescribeLifecycleHooks(gomock.Any()) + 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) From c869c4536353d7f16e6d3ed65cb7136419edaef3 Mon Sep 17 00:00:00 2001 From: Andreas Sommer Date: Wed, 27 Nov 2024 13:33:27 +0100 Subject: [PATCH 10/13] Fix hooks spec logic --- pkg/cloud/services/autoscaling/lifecyclehook.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/cloud/services/autoscaling/lifecyclehook.go b/pkg/cloud/services/autoscaling/lifecyclehook.go index 97e6be51ac..69e9df45aa 100644 --- a/pkg/cloud/services/autoscaling/lifecyclehook.go +++ b/pkg/cloud/services/autoscaling/lifecyclehook.go @@ -151,6 +151,8 @@ func getLifecycleHookSpecificationList(lifecycleHooks []expinfrav1.AWSLifecycleH timeoutSeconds := hook.HeartbeatTimeout.Duration.Seconds() spec.HeartbeatTimeout = aws.Int64(int64(timeoutSeconds)) } + + ret = append(ret, spec) } return From 7307fbd53749297ce432f13dfaac772ee6325009 Mon Sep 17 00:00:00 2001 From: Andreas Sommer Date: Thu, 28 Nov 2024 15:16:39 +0100 Subject: [PATCH 11/13] Fix lifecycle hook needs-update check for nil fields which get set to defaults by AWS --- .../services/autoscaling/lifecyclehook.go | 7 +- .../autoscaling/lifecyclehook_test.go | 129 ++++++++++++++++++ 2 files changed, 133 insertions(+), 3 deletions(-) create mode 100644 pkg/cloud/services/autoscaling/lifecyclehook_test.go diff --git a/pkg/cloud/services/autoscaling/lifecyclehook.go b/pkg/cloud/services/autoscaling/lifecyclehook.go index 69e9df45aa..fa18f57883 100644 --- a/pkg/cloud/services/autoscaling/lifecyclehook.go +++ b/pkg/cloud/services/autoscaling/lifecyclehook.go @@ -1,5 +1,5 @@ /* -Copyright 2018 The Kubernetes Authors. +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. @@ -24,6 +24,7 @@ import ( "github.com/aws/aws-sdk-go/service/autoscaling" "github.com/pkg/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" expinfrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services" @@ -203,8 +204,8 @@ func ReconcileLifecycleHooks(ctx context.Context, asgService services.ASGInterfa } func lifecycleHookNeedsUpdate(existing *expinfrav1.AWSLifecycleHook, expected *expinfrav1.AWSLifecycleHook) bool { - return existing.DefaultResult != expected.DefaultResult || - existing.HeartbeatTimeout != expected.HeartbeatTimeout || + return ptr.Deref(existing.DefaultResult, expinfrav1.LifecycleHookDefaultResultAbandon) != ptr.Deref(expected.DefaultResult, expinfrav1.LifecycleHookDefaultResultAbandon) || + ptr.Deref(existing.HeartbeatTimeout, metav1.Duration{Duration: 3600 * time.Second}) != ptr.Deref(expected.HeartbeatTimeout, metav1.Duration{Duration: 3600 * time.Second}) || existing.LifecycleTransition != expected.LifecycleTransition || existing.NotificationTargetARN != expected.NotificationTargetARN || existing.NotificationMetadata != expected.NotificationMetadata diff --git a/pkg/cloud/services/autoscaling/lifecyclehook_test.go b/pkg/cloud/services/autoscaling/lifecyclehook_test.go new file mode 100644 index 0000000000..0dc80443cd --- /dev/null +++ b/pkg/cloud/services/autoscaling/lifecyclehook_test.go @@ -0,0 +1,129 @@ +/* +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 asg + +import ( + "testing" + "time" + + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + expinfrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/exp/api/v1beta2" +) + +func TestLifecycleHookNeedsUpdate(t *testing.T) { + defaultResultAbandon := expinfrav1.LifecycleHookDefaultResultAbandon + defaultResultContinue := expinfrav1.LifecycleHookDefaultResultContinue + + tests := []struct { + name string + existing expinfrav1.AWSLifecycleHook + expected expinfrav1.AWSLifecycleHook + wantUpdate bool + }{ + { + name: "exactly equal", + existing: expinfrav1.AWSLifecycleHook{ + Name: "andreas-test", + LifecycleTransition: "autoscaling:EC2_INSTANCE_TERMINATING", + HeartbeatTimeout: &metav1.Duration{Duration: 3600 * time.Second}, + DefaultResult: &defaultResultAbandon, + }, + expected: expinfrav1.AWSLifecycleHook{ + Name: "andreas-test", + LifecycleTransition: "autoscaling:EC2_INSTANCE_TERMINATING", + HeartbeatTimeout: &metav1.Duration{Duration: 3600 * time.Second}, + DefaultResult: &defaultResultAbandon, + }, + wantUpdate: false, + }, + + { + name: "heartbeatTimeout and defaultResult not set in manifest, but set to defaults by AWS", + existing: expinfrav1.AWSLifecycleHook{ + Name: "andreas-test", + LifecycleTransition: "autoscaling:EC2_INSTANCE_TERMINATING", + // Describing with AWS SDK always fills these fields with the defaults + HeartbeatTimeout: &metav1.Duration{Duration: 3600 * time.Second}, + DefaultResult: &defaultResultAbandon, + }, + expected: expinfrav1.AWSLifecycleHook{ + Name: "andreas-test", + LifecycleTransition: "autoscaling:EC2_INSTANCE_TERMINATING", + }, + wantUpdate: false, + }, + + { + name: "transition differs", + existing: expinfrav1.AWSLifecycleHook{ + Name: "andreas-test", + LifecycleTransition: "autoscaling:EC2_INSTANCE_LAUNCHING", + HeartbeatTimeout: &metav1.Duration{Duration: 3600 * time.Second}, + DefaultResult: &defaultResultAbandon, + }, + expected: expinfrav1.AWSLifecycleHook{ + Name: "andreas-test", + LifecycleTransition: "autoscaling:EC2_INSTANCE_TERMINATING", + HeartbeatTimeout: &metav1.Duration{Duration: 3600 * time.Second}, + DefaultResult: &defaultResultAbandon, + }, + wantUpdate: true, + }, + + { + name: "heartbeat timeout differs", + existing: expinfrav1.AWSLifecycleHook{ + Name: "andreas-test", + LifecycleTransition: "autoscaling:EC2_INSTANCE_TERMINATING", + HeartbeatTimeout: &metav1.Duration{Duration: 3600 * time.Second}, + DefaultResult: &defaultResultAbandon, + }, + expected: expinfrav1.AWSLifecycleHook{ + Name: "andreas-test", + LifecycleTransition: "autoscaling:EC2_INSTANCE_TERMINATING", + HeartbeatTimeout: &metav1.Duration{Duration: 3601 * time.Second}, + DefaultResult: &defaultResultAbandon, + }, + wantUpdate: true, + }, + + { + name: "default result differs", + existing: expinfrav1.AWSLifecycleHook{ + Name: "andreas-test", + LifecycleTransition: "autoscaling:EC2_INSTANCE_TERMINATING", + HeartbeatTimeout: &metav1.Duration{Duration: 3600 * time.Second}, + DefaultResult: &defaultResultAbandon, + }, + expected: expinfrav1.AWSLifecycleHook{ + Name: "andreas-test", + LifecycleTransition: "autoscaling:EC2_INSTANCE_TERMINATING", + HeartbeatTimeout: &metav1.Duration{Duration: 3600 * time.Second}, + DefaultResult: &defaultResultContinue, + }, + wantUpdate: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + g.Expect(lifecycleHookNeedsUpdate(&tt.existing, &tt.expected)).To(Equal(tt.wantUpdate)) + }) + } +} From 003f84236a789c7a560dc9c47adfb5e878b9b5bb Mon Sep 17 00:00:00 2001 From: Andreas Sommer Date: Thu, 28 Nov 2024 16:31:14 +0100 Subject: [PATCH 12/13] Always update DefaultResult/HeartbeatTimeout settings if they drifted --- exp/api/v1beta2/types.go | 8 ++--- .../services/autoscaling/lifecyclehook.go | 32 ++++++++----------- 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/exp/api/v1beta2/types.go b/exp/api/v1beta2/types.go index 07d1184e20..7841544b5a 100644 --- a/exp/api/v1beta2/types.go +++ b/exp/api/v1beta2/types.go @@ -268,8 +268,8 @@ const ( LifecycleHookTransitionInstanceTerminating LifecycleTransition = "autoscaling:EC2_INSTANCE_TERMINATING" ) -func (l *LifecycleTransition) String() string { - return string(*l) +func (l LifecycleTransition) String() string { + return string(l) } // LifecycleHookDefaultResult is the default result for the lifecycle hook. @@ -282,8 +282,8 @@ const ( LifecycleHookDefaultResultAbandon LifecycleHookDefaultResult = "ABANDON" ) -func (d *LifecycleHookDefaultResult) String() string { - return string(*d) +func (d LifecycleHookDefaultResult) String() string { + return string(d) } // ASGStatus is a status string returned by the autoscaling API. diff --git a/pkg/cloud/services/autoscaling/lifecyclehook.go b/pkg/cloud/services/autoscaling/lifecyclehook.go index fa18f57883..8c6fca6bea 100644 --- a/pkg/cloud/services/autoscaling/lifecyclehook.go +++ b/pkg/cloud/services/autoscaling/lifecyclehook.go @@ -36,7 +36,7 @@ import ( // DescribeLifecycleHooks returns the lifecycle hooks for the given AutoScalingGroup after retrieving them from the AWS API. func (s *Service) DescribeLifecycleHooks(asgName string) ([]*expinfrav1.AWSLifecycleHook, error) { input := &autoscaling.DescribeLifecycleHooksInput{ - AutoScalingGroupName: aws.String(asgName), + AutoScalingGroupName: ptr.To(asgName), } out, err := s.ASGClient.DescribeLifecycleHooksWithContext(context.TODO(), input) @@ -54,9 +54,9 @@ func (s *Service) DescribeLifecycleHooks(asgName string) ([]*expinfrav1.AWSLifec func getPutLifecycleHookInput(asgName string, hook *expinfrav1.AWSLifecycleHook) (ret *autoscaling.PutLifecycleHookInput) { ret = &autoscaling.PutLifecycleHookInput{ - AutoScalingGroupName: aws.String(asgName), - LifecycleHookName: aws.String(hook.Name), - LifecycleTransition: aws.String(hook.LifecycleTransition.String()), + AutoScalingGroupName: ptr.To(asgName), + LifecycleHookName: ptr.To(hook.Name), + LifecycleTransition: ptr.To(hook.LifecycleTransition.String()), // Optional RoleARN: hook.RoleARN, @@ -64,15 +64,11 @@ func getPutLifecycleHookInput(asgName string, hook *expinfrav1.AWSLifecycleHook) NotificationMetadata: hook.NotificationMetadata, } - // Optional parameters - if hook.DefaultResult != nil { - ret.DefaultResult = aws.String(hook.DefaultResult.String()) - } - - if hook.HeartbeatTimeout != nil { - timeoutSeconds := hook.HeartbeatTimeout.Duration.Seconds() - ret.HeartbeatTimeout = aws.Int64(int64(timeoutSeconds)) - } + // For optional fields in the manifest, still fill in the AWS request parameters so any drifted lifecycle hook + // settings are reconciled to the desired state on update. Using AWS default values here. + ret.DefaultResult = ptr.To(ptr.Deref(hook.DefaultResult, expinfrav1.LifecycleHookDefaultResultAbandon).String()) + timeoutSeconds := ptr.Deref(hook.HeartbeatTimeout, metav1.Duration{Duration: 3600 * time.Second}).Duration.Seconds() + ret.HeartbeatTimeout = aws.Int64(int64(timeoutSeconds)) return } @@ -102,8 +98,8 @@ func (s *Service) UpdateLifecycleHook(ctx context.Context, asgName string, hook // DeleteLifecycleHook deletes a lifecycle hook for the given AutoScalingGroup. func (s *Service) DeleteLifecycleHook(ctx context.Context, asgName string, hook *expinfrav1.AWSLifecycleHook) error { input := &autoscaling.DeleteLifecycleHookInput{ - AutoScalingGroupName: aws.String(asgName), - LifecycleHookName: aws.String(hook.Name), + AutoScalingGroupName: ptr.To(asgName), + LifecycleHookName: ptr.To(hook.Name), } if _, err := s.ASGClient.DeleteLifecycleHookWithContext(ctx, input); err != nil { @@ -134,8 +130,8 @@ func (s *Service) SDKToLifecycleHook(hook *autoscaling.LifecycleHook) *expinfrav func getLifecycleHookSpecificationList(lifecycleHooks []expinfrav1.AWSLifecycleHook) (ret []*autoscaling.LifecycleHookSpecification) { for _, hook := range lifecycleHooks { spec := &autoscaling.LifecycleHookSpecification{ - LifecycleHookName: aws.String(hook.Name), - LifecycleTransition: aws.String(hook.LifecycleTransition.String()), + LifecycleHookName: ptr.To(hook.Name), + LifecycleTransition: ptr.To(hook.LifecycleTransition.String()), // Optional RoleARN: hook.RoleARN, @@ -145,7 +141,7 @@ func getLifecycleHookSpecificationList(lifecycleHooks []expinfrav1.AWSLifecycleH // Optional parameters if hook.DefaultResult != nil { - spec.DefaultResult = aws.String(hook.DefaultResult.String()) + spec.DefaultResult = ptr.To(hook.DefaultResult.String()) } if hook.HeartbeatTimeout != nil { From 6f6c0b86f134f5509ca6ffaf135d486616f9ff2d Mon Sep 17 00:00:00 2001 From: Sebastien Michel Date: Sun, 15 Dec 2024 18:18:55 +0100 Subject: [PATCH 13/13] fix: lint --- pkg/cloud/services/autoscaling/lifecyclehook_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/cloud/services/autoscaling/lifecyclehook_test.go b/pkg/cloud/services/autoscaling/lifecyclehook_test.go index 0dc80443cd..5aba913f64 100644 --- a/pkg/cloud/services/autoscaling/lifecyclehook_test.go +++ b/pkg/cloud/services/autoscaling/lifecyclehook_test.go @@ -122,8 +122,10 @@ func TestLifecycleHookNeedsUpdate(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + existing := tt.existing + expected := tt.expected g := NewWithT(t) - g.Expect(lifecycleHookNeedsUpdate(&tt.existing, &tt.expected)).To(Equal(tt.wantUpdate)) + g.Expect(lifecycleHookNeedsUpdate(&existing, &expected)).To(Equal(tt.wantUpdate)) }) } }