Refine update patterns for Cluster and Nodegroup resources (#100)

Prior to this patch, we observed that there was some missing logic for
handling `Cluster` and `Nodegroup` updates, especially when dealing with
version upgrades - making it a painful/incomplete UX when using the
eks-controller.

This commit enhances the logic for managing updates and upgrades of
cluster and nodegroups resources, enabling users to experience a better
declarative resource model. Here's what the controller can do now:

`Cluster` resources:
- Introduces `Cluster` version rolling update, allowing users to specify
  any version higher than the "current" one, even if the EKS API doesn't
  allow it. The controller will incrementally upgrade the the cluster
  version until it reaches the desired state. e.g `1.25` -> `1.29`

`Nodegroup` resources:
- Allows updates for `ReleaseVersion` and `LaunchTemplate`
- Enhances the delta/comparison logic for both `Version` and
  `ReleaseVersion` fields.
- Carefully crafts the `UpdateNodeGroupVersion` API input to avoid
  situations where the controller/resource can get trapped in a infinite
  update loop (`1.27` -> `1.28` -> `1.27`)
- Set `DiskSize`, `RemoteAccess`, `NodeRole` and `Subnets` as immutable
  fields

Additionally this commit is also adding e2e tests for the features/bug
fixes mentioned above

Signed-off-by: Amine Hilaly <[email protected]>

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Author: a-hilaly

apis/v1alpha1/ack-generate-metadata.yaml

@@ -1,13 +1,13 @@
 ack_generate_info:
-  build_date: "2024-02-03T01:09:21Z"
+  build_date: "2024-02-08T04:09:29Z"
   build_hash: 5b4565ec2712d29988b8123aeeed6a4af57467bf
   go_version: go1.21.5
   version: v0.29.2-4-g5b4565e
 api_directory_checksum: d960a9f06b58cc445e5ab21fb26ee6d92c441374
 api_version: v1alpha1
 aws_sdk_go_version: v1.49.13
 generator_config_info:
-  file_checksum: 7473dd4afcf1e30d99f20e77cbad94e2df5c7093
+  file_checksum: 89a00ff29a62ca0e86a8c08b275a6e9e708004b0
   original_file_name: generator.yaml
 last_modification:
   reason: API generation

apis/v1alpha1/generator.yaml

@@ -232,6 +232,11 @@ resources:
           service_name: iam
           resource: Role
           path: Status.ACKResourceMetadata.ARN
+        is_immutable: true
+      DiskSize:
+        is_immutable: true
+      RemoteAccess:
+        is_immutable: true
       RemoteAccess.SourceSecurityGroups:
         references:
           service_name: ec2
@@ -242,9 +247,16 @@ resources:
           service_name: ec2
           resource: Subnet
           path: Status.SubnetID
+        is_immutable: true
       Taints:
         compare:
           is_ignored: true
+      ReleaseVersion:
+        compare:
+          is_ignored: true
+      Version:
+        compare:
+          is_ignored: true
     renames:
       operations:
         CreateNodegroup:

generator.yaml

@@ -232,6 +232,11 @@ resources:
           service_name: iam
           resource: Role
           path: Status.ACKResourceMetadata.ARN
+        is_immutable: true
+      DiskSize:
+        is_immutable: true
+      RemoteAccess:
+        is_immutable: true
       RemoteAccess.SourceSecurityGroups:
         references:
           service_name: ec2
@@ -242,9 +247,16 @@ resources:
           service_name: ec2
           resource: Subnet
           path: Status.SubnetID
+        is_immutable: true
       Taints:
         compare:
           is_ignored: true
+      ReleaseVersion:
+        compare:
+          is_ignored: true
+      Version:
+        compare:
+          is_ignored: true
     renames:
       operations:
         CreateNodegroup:

pkg/resource/cluster/hook.go

@@ -19,14 +19,16 @@ import (
 	"fmt"
 	"time"
 
-	"github.com/aws-controllers-k8s/eks-controller/pkg/tags"
 	ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
 	ackcondition "github.com/aws-controllers-k8s/runtime/pkg/condition"
 	ackerr "github.com/aws-controllers-k8s/runtime/pkg/errors"
 	ackrequeue "github.com/aws-controllers-k8s/runtime/pkg/requeue"
 	ackrtlog "github.com/aws-controllers-k8s/runtime/pkg/runtime/log"
 	svcsdk "github.com/aws/aws-sdk-go/service/eks"
 	corev1 "k8s.io/api/core/v1"
+
+	"github.com/aws-controllers-k8s/eks-controller/pkg/tags"
+	"github.com/aws-controllers-k8s/eks-controller/pkg/util"
 )
 
 const (
@@ -246,7 +248,7 @@ func (rm *resourceManager) customUpdate(
 	}
 
 	if delta.DifferentAt("Spec.Version") {
-		if err := rm.updateVersion(ctx, desired); err != nil {
+		if err := rm.updateVersion(ctx, desired, latest); err != nil {
 			awserr, ok := ackerr.AWSError(err)
 
 			// Check to see if we've raced an async update call and need to
@@ -264,16 +266,45 @@ func (rm *resourceManager) customUpdate(
 	return updatedRes, nil
 }
 
+// updateVersion updates the cluster version to the next possible version.
+//
+// This function isn't supposed to blindly update the cluster version to the
+// desired version. It should increment the minor version of the observed
+// version and update the cluster to that version. The reconciliation mechanism
+// should ensure that the desired version is eventually reached.
 func (rm *resourceManager) updateVersion(
 	ctx context.Context,
-	r *resource,
+	desired, latest *resource,
 ) (err error) {
 	rlog := ackrtlog.FromContext(ctx)
 	exit := rlog.Trace("rm.updateVersion")
 	defer exit(err)
+
+	// If the desired version is less than the observed version, we can't update
+	// the cluster to an older version.
+	// Note that the desired and observed versions are guaranteed to be never be
+	// equal at this stage, as the delta comparison would have caught that.
+	compareResult, err := util.CompareEKSKubernetesVersions(*desired.ko.Spec.Version, *latest.ko.Spec.Version)
+	if err != nil {
+		return ackerr.NewTerminalError(fmt.Errorf("failed to compare the desired and observed versions: %v", err))
+	}
+	if compareResult != 1 {
+		return ackerr.NewTerminalError(
+			fmt.Errorf("desired cluster version is less than the observed version: %s < %s",
+				*desired.ko.Spec.Version, *latest.ko.Spec.Version,
+			),
+		)
+	}
+
+	// Compure the next minor version of the desired version
+	nextVersion, err := util.IncrementEKSMinorVersion(*latest.ko.Spec.Version)
+	if err != nil {
+		return ackerr.NewTerminalError(fmt.Errorf("failed to compute the next minor version: %v", err))
+	}
+
 	input := &svcsdk.UpdateClusterVersionInput{
-		Name:    r.ko.Spec.Name,
-		Version: r.ko.Spec.Version,
+		Name:    desired.ko.Spec.Name,
+		Version: &nextVersion,
 	}
 
 	_, err = rm.sdkapi.UpdateClusterVersionWithContext(ctx, input)

pkg/resource/nodegroup/delta.go

@@ -18,10 +18,12 @@ import (
 	"fmt"
 	"reflect"
 	"strconv"
+	"strings"
 	"time"
 
 	ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
 	ackcondition "github.com/aws-controllers-k8s/runtime/pkg/condition"
+	ackerr "github.com/aws-controllers-k8s/runtime/pkg/errors"
 	ackrequeue "github.com/aws-controllers-k8s/runtime/pkg/requeue"
 	ackrtlog "github.com/aws-controllers-k8s/runtime/pkg/runtime/log"
 	svcsdk "github.com/aws/aws-sdk-go/service/eks"
@@ -30,6 +32,7 @@ import (
 
 	svcapitypes "github.com/aws-controllers-k8s/eks-controller/apis/v1alpha1"
 	"github.com/aws-controllers-k8s/eks-controller/pkg/tags"
+	"github.com/aws-controllers-k8s/eks-controller/pkg/util"
 )
 
 // Taken from the list of nodegroup statuses on the boto3 documentation
@@ -104,6 +107,20 @@ func customPreCompare(
 			}
 		}
 	}
+	// only compare releaseVersion if it's provided by the user. Note that there will always be a
+	// ReleaseVersion in the observed state (after a successful creation).
+	if a.ko.Spec.ReleaseVersion != nil && *a.ko.Spec.ReleaseVersion != "" {
+		if *a.ko.Spec.ReleaseVersion != *b.ko.Spec.ReleaseVersion {
+			delta.Add("Spec.ReleaseVersion", a.ko.Spec.ReleaseVersion, b.ko.Spec.ReleaseVersion)
+		}
+	}
+	// only compare version if it's provided by the user. Note that there will always be a
+	// Version in the observed state (after a successful creation).
+	if a.ko.Spec.Version != nil && *a.ko.Spec.Version != "" {
+		if *a.ko.Spec.Version != *b.ko.Spec.Version {
+			delta.Add("Spec.Version", a.ko.Spec.Version, b.ko.Spec.Version)
+		}
+	}
 }
 
 func getDesiredSizeManagedByAnnotation(nodegroup *svcapitypes.Nodegroup) (string, bool) {
@@ -239,6 +256,12 @@ func (rm *resourceManager) customUpdate(
 	rlog := ackrtlog.FromContext(ctx)
 	exit := rlog.Trace("rm.customUpdate")
 	defer exit(err)
+
+	if immutableFieldChanges := rm.getImmutableFieldChanges(delta); len(immutableFieldChanges) > 0 {
+		msg := fmt.Sprintf("Immutable Spec fields have been modified: %s", strings.Join(immutableFieldChanges, ","))
+		return nil, ackerr.NewTerminalError(fmt.Errorf(msg))
+	}
+
 	if delta.DifferentAt("Spec.Tags") {
 		err := tags.SyncTags(
 			ctx, rm.sdkapi, rm.metrics,
@@ -282,8 +305,55 @@ func (rm *resourceManager) customUpdate(
 		}
 		return returnNodegroupUpdating(updatedRes)
 	}
-	if delta.DifferentAt("Spec.Version") {
-		if err := rm.updateVersion(ctx, desired); err != nil {
+
+	// At the stage we know that at least one of Version, ReleaseVersion or
+	// LaunchTemplate has changed. The API does not allow using LaunchTemplate
+	// with either Version or ReleaseVersion. So we need to check if the user
+	// has provided a valid desired state.
+	// There is no need to manually set a terminal condition here as the
+	// controller will automatically set a terminal condition if the api
+	// returns an InvalidParameterException
+
+	if delta.DifferentAt("Spec.Version") || delta.DifferentAt("Spec.ReleaseVersion") || delta.DifferentAt("Spec.LaunchTemplate") {
+		// Before trying to trigger a Nodegroup version update, we need to ensure that the user
+		// has provided a valid desired state. For context the EKS UpdateNodegroupVersion API
+		// accepts optional parameters Version and ReleaseVersion.
+		//
+		// The following are the valid combinations of the Version and ReleaseVersion parameters:
+		// 1. None of the parameters are provided
+		// 2. Only the Version parameter is provided
+		// 3. Only the ReleaseVersion parameter is provided
+		// 4. Both the Version and ReleaseVersion parameters are provided and they match
+		//
+		// The first case is not applicable here as it's counterintuitive in a declarative
+		// model to not provide a desired state and have the controller trigger a blind update.
+
+		// We need to set a terminal condition if the user provides both a version and release version
+		// and they do not match. This is needed because the controller could potentially start alternating
+		// between the non-matching version and release version in the spec and the observed state.
+		if desired.ko.Spec.Version != nil && desired.ko.Spec.ReleaseVersion != nil &&
+			*desired.ko.Spec.Version != "" && *desired.ko.Spec.ReleaseVersion != "" {
+
+			// First parse the user provided release version and desired release
+			desiredReleaseVersionTrimmed, err := util.GetEKSVersionFromReleaseVersion(*desired.ko.Spec.ReleaseVersion)
+			if err != nil {
+				return nil, ackerr.NewTerminalError(err)
+			}
+
+			// Set a terminal condition if the release version and version do not match.
+			// e.g if the user provides a release version of 1.16.8-20211201 and a version of 1.17
+			// They will either need to provide one of the following:
+			// 2. A version
+			// 1. A release version
+			// 3. A version and release version that matches (e.g 1.16 and 1.16.8-20211201)
+			if desiredReleaseVersionTrimmed != *desired.ko.Spec.Version {
+				return nil, ackerr.NewTerminalError(
+					fmt.Errorf("version and release version do not match: %s and %s", *desired.ko.Spec.Version, desiredReleaseVersionTrimmed),
+				)
+			}
+		}
+
+		if err := rm.updateVersion(ctx, delta, desired); err != nil {
 			return nil, err
 		}
 		return returnNodegroupUpdating(updatedRes)
@@ -380,39 +450,51 @@ func newUpdateTaintsPayload(
 }
 
 func newUpdateNodegroupVersionPayload(
+	delta *ackcompare.Delta,
 	desired *resource,
 ) *svcsdk.UpdateNodegroupVersionInput {
 	input := &svcsdk.UpdateNodegroupVersionInput{
-		NodegroupName:  desired.ko.Spec.Name,
-		ClusterName:    desired.ko.Spec.ClusterName,
-		Version:        desired.ko.Spec.Version,
-		ReleaseVersion: desired.ko.Spec.ReleaseVersion,
+		NodegroupName: desired.ko.Spec.Name,
+		ClusterName:   desired.ko.Spec.ClusterName,
+	}
+
+	if delta.DifferentAt("Spec.Version") {
+		input.Version = desired.ko.Spec.Version
+	}
+
+	if delta.DifferentAt("Spec.ReleaseVersion") {
+		input.ReleaseVersion = desired.ko.Spec.ReleaseVersion
 	}
 
-	if desired.ko.Spec.LaunchTemplate != nil {
-		input.SetLaunchTemplate(&svcsdk.LaunchTemplateSpecification{
-			Id:      desired.ko.Spec.LaunchTemplate.ID,
-			Name:    desired.ko.Spec.LaunchTemplate.Name,
-			Version: desired.ko.Spec.LaunchTemplate.Version,
-		})
+	if delta.DifferentAt("Spec.LaunchTemplate") {
+		// We need to be careful here to not access a nil pointer
+		if desired.ko.Spec.LaunchTemplate != nil {
+			input.SetLaunchTemplate(&svcsdk.LaunchTemplateSpecification{
+				Id:      desired.ko.Spec.LaunchTemplate.ID,
+				Name:    desired.ko.Spec.LaunchTemplate.Name,
+				Version: desired.ko.Spec.LaunchTemplate.Version,
+			})
+		}
 	}
 
+	// If the force annotation is set, we set the force flag on the input
+	// payload.
 	if getUpdateNodeGroupForceAnnotation(desired.ko.ObjectMeta) {
 		input.SetForce(true)
 	}
-
 	return input
 }
 
 func (rm *resourceManager) updateVersion(
 	ctx context.Context,
+	delta *ackcompare.Delta,
 	r *resource,
 ) (err error) {
 	rlog := ackrtlog.FromContext(ctx)
 	exit := rlog.Trace("rm.updateVersion")
 	defer exit(err)
 
-	input := newUpdateNodegroupVersionPayload(r)
+	input := newUpdateNodegroupVersionPayload(delta, r)
 
 	_, err = rm.sdkapi.UpdateNodegroupVersionWithContext(ctx, input)
 	rm.metrics.RecordAPICall("UPDATE", "UpdateNodegroupVersion", err)

pkg/resource/nodegroup/hook.go

@@ -424,6 +424,10 @@ func Test_resourceManager_newUpdateScalingConfigPayload_ManagedByDefault(t *test
 }
 
 func Test_newUpdateNodegroupPayload(t *testing.T) {
+	delta := ackcompare.NewDelta()
+	delta.Add("Spec.Version", nil, nil)
+	delta.Add("Spec.LaunchTemplate", nil, nil)
+
 	type args struct {
 		r *resource
 	}
@@ -511,7 +515,7 @@ func Test_newUpdateNodegroupPayload(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := newUpdateNodegroupVersionPayload(tt.args.r)
+			got := newUpdateNodegroupVersionPayload(delta, tt.args.r)
 			assert.Equal(t, tt.wantVersion, *got.Version)
 			if tt.wantForce {
 				assert.NotNil(t, got.Force)