Skip to content

Commit cfa27e3

Browse files
rnarainrnarain
authored
Address LaunchTemplate version update issues while updating Nodegroup (#196)
Issue #, if available: - aws-controllers-k8s/community#2758 - aws-controllers-k8s/community#2645 **Description of changes:** This PR fixes two related issues with nodegroup updates: 1. Builds on previous work: Extends the fix from #181 by @sudohikumar 2. The controller was continuously trying to call updateVersion with Version/ReleaseVersion even though the EKS API doesn't allow these fields when using custom AMI with LaunchTemplate resulting in below terminal errors. I ran into all these terminal errors during my testing with custom amis. ``` - message: 'InvalidParameterException: Launch template details can''t be null for Custom ami type node group' ``` ``` - message: 'InvalidParameterException: You cannot specify the field releaseVersion when using custom AMIs.' ``` ``` - message: 'invalid release version: ami-<xxxx>' ``` **Testing:** - Kubernetes Server Version: v1.34.2-eks-b3126f4 - Ack controller version: local ack controller built with latest code - logs: [logs.txt](https://github.com/user-attachments/files/24828122/logs.txt) By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
1 parent 7a773a9 commit cfa27e3

2 files changed

Lines changed: 247 additions & 5 deletions

File tree

‎pkg/resource/nodegroup/hook.go‎

Lines changed: 62 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -331,7 +331,7 @@ func (rm *resourceManager) customUpdate(
331331
if desired.ko.Spec.Version != nil && desired.ko.Spec.ReleaseVersion != nil &&
332332
*desired.ko.Spec.Version != "" && *desired.ko.Spec.ReleaseVersion != "" {
333333

334-
if !isAMITypeBottlerocket(desired.ko.Spec.AMIType) {
334+
if !isAMITypeBottlerocket(desired.ko.Spec.AMIType) && !isAMITypeCustom(desired) {
335335
// First parse the user provided release version and desired release
336336
desiredReleaseVersionTrimmed, err := util.GetEKSVersionFromReleaseVersion(*desired.ko.Spec.ReleaseVersion)
337337
if err != nil {
@@ -352,6 +352,19 @@ func (rm *resourceManager) customUpdate(
352352
}
353353
}
354354

355+
// Check if this is a custom AMI with LaunchTemplate scenario
356+
if isAMITypeCustom(desired) {
357+
// For custom AMI with LaunchTemplate, we cannot update Version or ReleaseVersion via API
358+
// Only proceed with update if LaunchTemplate itself changed
359+
if !delta.DifferentAt("Spec.LaunchTemplate") {
360+
// Need this function to have the spec in sync with AWS
361+
rm.preserveVersionFields(updatedRes, latest)
362+
// No update needed, just return
363+
rm.setStatusDefaults(updatedRes.ko)
364+
return updatedRes, nil
365+
}
366+
}
367+
355368
if err := rm.updateVersion(ctx, delta, desired); err != nil {
356369
return nil, err
357370
}
@@ -377,6 +390,25 @@ func isAMITypeBottlerocket(amiType *string) bool {
377390
return false
378391
}
379392

393+
// preserveVersionFields copies Version and ReleaseVersion from latest to updated resource
394+
// This is needed when version updates are skipped (e.g., custom AMI with LaunchTemplate)
395+
// And we still need the Version,ReleaseVersion fields to be in sync with AWS
396+
func (rm *resourceManager) preserveVersionFields(updated *resource, latest *resource) {
397+
if latest.ko.Spec.Version != nil {
398+
updated.ko.Spec.Version = latest.ko.Spec.Version
399+
}
400+
if latest.ko.Spec.ReleaseVersion != nil {
401+
updated.ko.Spec.ReleaseVersion = latest.ko.Spec.ReleaseVersion
402+
}
403+
}
404+
405+
// isAMITypeCustom checks if the nodegroup uses a custom AMI
406+
func isAMITypeCustom(r *resource) bool {
407+
return r.ko.Spec.LaunchTemplate != nil &&
408+
r.ko.Spec.AMIType != nil &&
409+
*r.ko.Spec.AMIType == string(svcapitypes.AMITypes_CUSTOM)
410+
}
411+
380412
// newUpdateLabelsPayload determines which of the labels should be added or
381413
// updated, and which labels should be removed, based on the desired vs the
382414
// latest
@@ -483,14 +515,39 @@ func newUpdateNodegroupVersionPayload(
483515
if delta.DifferentAt("Spec.LaunchTemplate") {
484516
// We need to be careful here to not access a nil pointer
485517
if desired.ko.Spec.LaunchTemplate != nil {
486-
input.LaunchTemplate = &svcsdktypes.LaunchTemplateSpecification{
487-
Id: desired.ko.Spec.LaunchTemplate.ID,
488-
Name: desired.ko.Spec.LaunchTemplate.Name,
489-
Version: desired.ko.Spec.LaunchTemplate.Version,
518+
input.LaunchTemplate = &svcsdktypes.LaunchTemplateSpecification{}
519+
520+
if delta.DifferentAt("Spec.LaunchTemplate.ID") {
521+
input.LaunchTemplate.Id = desired.ko.Spec.LaunchTemplate.ID
522+
}
523+
524+
if delta.DifferentAt("Spec.LaunchTemplate.Name") {
525+
input.LaunchTemplate.Name = desired.ko.Spec.LaunchTemplate.Name
490526
}
527+
528+
// If neither the Name nor ID fields have changed, we need to
529+
// set the one of ID/Name field in the payload
530+
// preference is given to ID over Name
531+
if !delta.DifferentAt("Spec.LaunchTemplate.Name") && !delta.DifferentAt("Spec.LaunchTemplate.ID") {
532+
if desired.ko.Spec.LaunchTemplate.ID != nil {
533+
input.LaunchTemplate.Id = desired.ko.Spec.LaunchTemplate.ID
534+
} else if desired.ko.Spec.LaunchTemplate.Name != nil {
535+
input.LaunchTemplate.Name = desired.ko.Spec.LaunchTemplate.Name
536+
}
537+
}
538+
539+
input.LaunchTemplate.Version = desired.ko.Spec.LaunchTemplate.Version
491540
}
492541
}
493542

543+
// If LaunchTemplate is being used with a custom AMI,
544+
// we cannot set Version or ReleaseVersion in the update request as per the EKS API specification.
545+
// https://docs.aws.amazon.com/eks/latest/APIReference/API_UpdateNodegroupVersion.html#API_UpdateNodegroupVersion_RequestBody
546+
if isAMITypeCustom(desired) {
547+
input.ReleaseVersion = nil
548+
input.Version = nil
549+
}
550+
494551
// If the force annotation is set, we set the force flag on the input
495552
// payload.
496553
if getUpdateNodeGroupForceAnnotation(desired.ko.ObjectMeta) {

‎pkg/resource/nodegroup/hook_test.go‎

Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,12 +21,14 @@ import (
2121

2222
ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
2323
"github.com/aws/aws-sdk-go-v2/aws"
24+
"github.com/aws/aws-sdk-go-v2/service/eks"
2425
svcsdktypes "github.com/aws/aws-sdk-go-v2/service/eks/types"
2526
"github.com/stretchr/testify/assert"
2627
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2728
"sigs.k8s.io/controller-runtime/pkg/log/zap"
2829

2930
"github.com/aws-controllers-k8s/eks-controller/apis/v1alpha1"
31+
svcapitypes "github.com/aws-controllers-k8s/eks-controller/apis/v1alpha1"
3032
)
3133

3234
func TestTaints(t *testing.T) {
@@ -551,6 +553,189 @@ func Test_newUpdateNodegroupPayload(t *testing.T) {
551553
}
552554
}
553555

556+
func Test_newUpdateNodegroupPayloadWithLaunchTemplate(t *testing.T) {
557+
delta := ackcompare.NewDelta()
558+
delta.Add("Spec.Version", nil, nil)
559+
delta.Add("Spec.LaunchTemplate", nil, nil)
560+
561+
type args struct {
562+
r *resource
563+
}
564+
tests := []struct {
565+
name string
566+
args args
567+
wantPayload *eks.UpdateNodegroupVersionInput
568+
}{
569+
{
570+
name: "only id in launch template",
571+
args: args{
572+
r: &resource{
573+
ko: &v1alpha1.Nodegroup{
574+
Spec: v1alpha1.NodegroupSpec{
575+
LaunchTemplate: &v1alpha1.LaunchTemplateSpecification{
576+
ID: aws.String("lt-12345"),
577+
Version: aws.String("1"),
578+
},
579+
},
580+
},
581+
},
582+
},
583+
wantPayload: &eks.UpdateNodegroupVersionInput{
584+
ClusterName: nil,
585+
NodegroupName: nil,
586+
LaunchTemplate: &svcsdktypes.LaunchTemplateSpecification{
587+
Id: aws.String("lt-12345"),
588+
Version: aws.String("1"),
589+
},
590+
},
591+
},
592+
{
593+
name: "only name in launch template",
594+
args: args{
595+
r: &resource{
596+
ko: &v1alpha1.Nodegroup{
597+
Spec: v1alpha1.NodegroupSpec{
598+
LaunchTemplate: &v1alpha1.LaunchTemplateSpecification{
599+
Name: aws.String("my-launch-template"),
600+
Version: aws.String("1"),
601+
},
602+
},
603+
},
604+
},
605+
},
606+
wantPayload: &eks.UpdateNodegroupVersionInput{
607+
ClusterName: nil,
608+
NodegroupName: nil,
609+
LaunchTemplate: &svcsdktypes.LaunchTemplateSpecification{
610+
Name: aws.String("my-launch-template"),
611+
Version: aws.String("1"),
612+
},
613+
},
614+
},
615+
{
616+
name: "id and name in launch template",
617+
args: args{
618+
r: &resource{
619+
ko: &v1alpha1.Nodegroup{
620+
Spec: v1alpha1.NodegroupSpec{
621+
LaunchTemplate: &v1alpha1.LaunchTemplateSpecification{
622+
ID: aws.String("lt-12345"),
623+
Name: aws.String("my-launch-template"),
624+
Version: aws.String("1"),
625+
},
626+
},
627+
},
628+
},
629+
},
630+
wantPayload: &eks.UpdateNodegroupVersionInput{
631+
ClusterName: nil,
632+
NodegroupName: nil,
633+
LaunchTemplate: &svcsdktypes.LaunchTemplateSpecification{
634+
Id: aws.String("lt-12345"),
635+
Version: aws.String("1"),
636+
},
637+
},
638+
},
639+
}
640+
641+
for _, tt := range tests {
642+
t.Run(tt.name, func(t *testing.T) {
643+
got := newUpdateNodegroupVersionPayload(delta, tt.args.r)
644+
assert.Equal(t, tt.wantPayload, got)
645+
})
646+
}
647+
}
648+
649+
func Test_customUpdateWithCustomAMIAndLaunchTemplate(t *testing.T) {
650+
customAMI := string(svcapitypes.AMITypes_CUSTOM)
651+
652+
tests := []struct {
653+
name string
654+
desired *resource
655+
latest *resource
656+
expectVersionInSpec bool
657+
expectedVersion string
658+
expectedRelease string
659+
}{
660+
{
661+
name: "custom AMI with launch template - version fields preserved from latest",
662+
desired: &resource{
663+
ko: &v1alpha1.Nodegroup{
664+
Spec: v1alpha1.NodegroupSpec{
665+
Name: aws.String("test-ng"),
666+
ClusterName: aws.String("test-cluster"),
667+
AMIType: &customAMI,
668+
Version: aws.String("1.28"),
669+
ReleaseVersion: aws.String("1.28.1-20231201"),
670+
LaunchTemplate: &v1alpha1.LaunchTemplateSpecification{
671+
ID: aws.String("lt-12345"),
672+
Version: aws.String("2"),
673+
},
674+
},
675+
Status: v1alpha1.NodegroupStatus{
676+
Status: aws.String(StatusActive),
677+
},
678+
},
679+
},
680+
latest: &resource{
681+
ko: &v1alpha1.Nodegroup{
682+
Spec: v1alpha1.NodegroupSpec{
683+
Name: aws.String("test-ng"),
684+
ClusterName: aws.String("test-cluster"),
685+
AMIType: &customAMI,
686+
Version: aws.String("1.27"),
687+
ReleaseVersion: aws.String("1.27.5-20231101"),
688+
LaunchTemplate: &v1alpha1.LaunchTemplateSpecification{
689+
ID: aws.String("lt-12345"),
690+
Version: aws.String("1"),
691+
},
692+
},
693+
Status: v1alpha1.NodegroupStatus{
694+
Status: aws.String(StatusActive),
695+
},
696+
},
697+
},
698+
expectVersionInSpec: true,
699+
expectedVersion: "1.27", // Should preserve from latest, not use desired
700+
expectedRelease: "1.27.5-20231101", // Should preserve from latest, not use desired
701+
},
702+
}
703+
704+
for _, tt := range tests {
705+
t.Run(tt.name, func(t *testing.T) {
706+
delta := newResourceDelta(tt.desired, tt.latest)
707+
708+
// Verify that the update payload does NOT include version/releaseVersion
709+
if delta.DifferentAt("Spec.LaunchTemplate") {
710+
payload := newUpdateNodegroupVersionPayload(delta, tt.desired)
711+
assert.Nil(t, payload.Version,
712+
"Version should be nil in update payload for custom AMI with launch template")
713+
assert.Nil(t, payload.ReleaseVersion,
714+
"ReleaseVersion should be nil in update payload for custom AMI with launch template")
715+
assert.NotNil(t, payload.LaunchTemplate,
716+
"LaunchTemplate should be present in update payload")
717+
}
718+
719+
// Verify that version and releaseVersion are preserved in spec
720+
// even though they won't be in the update payload
721+
if tt.expectVersionInSpec {
722+
// After customUpdate logic, the version fields should be preserved from latest
723+
rm := &resourceManager{}
724+
rm.preserveVersionFields(tt.desired, tt.latest)
725+
726+
assert.NotNil(t, tt.desired.ko.Spec.Version)
727+
assert.Equal(t, tt.expectedVersion, *tt.desired.ko.Spec.Version,
728+
"Version should be preserved from latest, not from desired")
729+
730+
assert.NotNil(t, tt.desired.ko.Spec.ReleaseVersion)
731+
assert.Equal(t, tt.expectedRelease, *tt.desired.ko.Spec.ReleaseVersion,
732+
"ReleaseVersion should be preserved from latest, not from desired")
733+
}
734+
735+
})
736+
}
737+
}
738+
554739
func Test_AMITypeBottlerocket(t *testing.T) {
555740
tests := []struct {
556741
name string

0 commit comments

Comments
 (0)