Fix NodeGroup taints updates (#78)

Issue: aws-controllers-k8s/community#1950

Description of changes:

Add custom diff for taints + tests

Author: speedfl

.gitignore

@@ -5,4 +5,5 @@
 /docs/site
 bin
 build
-.env
+.env
+vendor

apis/v1alpha1/ack-generate-metadata.yaml

@@ -1,13 +1,13 @@
 ack_generate_info:
-  build_date: "2023-12-12T20:01:34Z"
-  build_hash: 2cc7afebf2d80513f9ed3aba95b9a2bd8dfe3d6a
-  go_version: go1.21.1
-  version: v0.27.1-7-g2cc7afe
-api_directory_checksum: a543a43cd1b4533a3bb3abb15a960b6358035ecc
+  build_date: "2023-12-13T14:11:41Z"
+  build_hash: 3653329ceeb20015851b8776a6061a3fb0ec2935
+  go_version: go1.20.6
+  version: v0.27.1-6-g3653329
+api_directory_checksum: 72b28de134a8fcb097e4f04b8482d7882a01940b
 api_version: v1alpha1
 aws_sdk_go_version: v1.48.4
 generator_config_info:
-  file_checksum: c5c0aeda28af64e2b7ead52aa3563fe6e35d6cf8
+  file_checksum: 9b2af7d38552d1e11a320576c802f375dcba3a06
   original_file_name: generator.yaml
 last_modification:
   reason: API generation

apis/v1alpha1/generator.yaml

@@ -168,6 +168,9 @@ resources:
           service_name: ec2
           resource: Subnet
           path: Status.SubnetID
+      Taints:
+        compare:
+          is_ignored: true
     renames:
       operations:
         CreateNodegroup:
@@ -198,7 +201,7 @@ resources:
         - ValidationError
     hooks:
       delta_pre_compare:
-        code: customPreCompare(a, b)
+        code: customPreCompare(delta, a, b)
       sdk_create_post_set_output:
         template_path: hooks/nodegroup/sdk_create_post_set_output.go.tpl
       sdk_read_one_post_set_output:

generator.yaml

@@ -168,6 +168,9 @@ resources:
           service_name: ec2
           resource: Subnet
           path: Status.SubnetID
+      Taints:
+        compare:
+          is_ignored: true
     renames:
       operations:
         CreateNodegroup:
@@ -198,7 +201,7 @@ resources:
         - ValidationError
     hooks:
       delta_pre_compare:
-        code: customPreCompare(a, b)
+        code: customPreCompare(delta, a, b)
       sdk_create_post_set_output:
         template_path: hooks/nodegroup/sdk_create_post_set_output.go.tpl
       sdk_read_one_post_set_output:

go.mod

@@ -9,6 +9,7 @@ require (
 	github.com/aws/aws-sdk-go v1.48.4
 	github.com/go-logr/logr v1.2.3
 	github.com/spf13/pflag v1.0.5
+	github.com/stretchr/testify v1.8.4
 	k8s.io/api v0.26.8
 	k8s.io/apimachinery v0.26.8
 	k8s.io/client-go v0.26.8
@@ -47,6 +48,7 @@ require (
 	github.com/modern-go/reflect2 v1.0.2 // indirect
 	github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
 	github.com/pkg/errors v0.9.1 // indirect
+	github.com/pmezard/go-difflib v1.0.0 // indirect
 	github.com/prometheus/client_golang v1.14.0 // indirect
 	github.com/prometheus/client_model v0.3.0 // indirect
 	github.com/prometheus/common v0.37.0 // indirect

go.sum

@@ -277,14 +277,15 @@ github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An
 github.com/stoewer/go-strcase v1.2.0/go.mod h1:IBiWB2sKIp3wVVQ3Y035++gc+knqhUQag1KpM8ahLw8=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
-github.com/stretchr/objx v0.4.0 h1:M2gUjqZET1qApGOWNSnZ49BAIMX4F/1plDv3+l31EJ4=
+github.com/stretchr/objx v0.5.0 h1:1zr/of2m5FGMsad5YfcqgdqdWrIhu+EBEJRhR1U7z/c=
 github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
 github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
 github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
 github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA=
 github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
 github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
-github.com/stretchr/testify v1.8.0 h1:pSgiaMZlXftHpm5L7V1+rVB+AZJydKsMxsQBIJw4PKk=
+github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk=
+github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo=
 github.com/yuin/goldmark v1.1.25/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
 github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
 github.com/yuin/goldmark v1.1.32/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=

pkg/resource/nodegroup/delta.go

@@ -16,6 +16,7 @@ package nodegroup
 import (
 	"context"
 	"fmt"
+	"reflect"
 	"time"
 
 	svcapitypes "github.com/aws-controllers-k8s/eks-controller/apis/v1alpha1"
@@ -61,6 +62,7 @@ var (
 // appropriately replaced with empty maps or structs depending on the default
 // output of the SDK.
 func customPreCompare(
+	delta *ackcompare.Delta,
 	a *resource,
 	b *resource,
 ) {
@@ -77,6 +79,27 @@ func customPreCompare(
 	if a.ko.Spec.Tags == nil && b.ko.Spec.Tags != nil {
 		a.ko.Spec.Tags = map[string]*string{}
 	}
+	if a.ko.Spec.Taints != nil && a.ko.Spec.Taints == nil || a.ko.Spec.Taints == nil && a.ko.Spec.Taints != nil {
+		delta.Add("Spec.Taints", a.ko.Spec.Taints, b.ko.Spec.Taints)
+	} else if a.ko.Spec.Taints != nil {
+		if len(a.ko.Spec.Taints) != len(b.ko.Spec.Taints) {
+			delta.Add("Spec.Taints", a.ko.Spec.Taints, b.ko.Spec.Taints)
+		} else {
+			for _, taintA := range a.ko.Spec.Taints {
+				var matched = false
+				for _, taintB := range b.ko.Spec.Taints {
+					if reflect.DeepEqual(taintA, taintB) {
+						matched = true
+						break
+					}
+				}
+				if !matched {
+					delta.Add("Spec.Taints", a.ko.Spec.Taints, b.ko.Spec.Taints)
+					break
+				}
+			}
+		}
+	}
 }
 
 // requeueWaitUntilCanModify returns a `ackrequeue.RequeueNeededAfter` struct

pkg/resource/nodegroup/hook.go

@@ -0,0 +1,134 @@
+package nodegroup
+
+import (
+	"testing"
+
+	"github.com/aws-controllers-k8s/eks-controller/apis/v1alpha1"
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestTaints(t *testing.T) {
+
+	noSchedule := "NO_SCHEDULE"
+	owner := "owner"
+	project := "project"
+
+	teamOne := "teamone"
+	projectOne := "projectone"
+
+	a := &resource{
+		ko: &v1alpha1.Nodegroup{
+			ObjectMeta: v1.ObjectMeta{
+				Name:      "test",
+				Namespace: "test",
+			},
+			Spec: v1alpha1.NodegroupSpec{
+				Taints: []*v1alpha1.Taint{
+					{
+						Effect: &noSchedule,
+						Key:    &owner,
+						Value:  &teamOne,
+					},
+					{
+						Effect: &noSchedule,
+						Key:    &project,
+						Value:  &projectOne,
+					},
+				},
+			},
+		},
+	}
+
+	b := &resource{
+		ko: &v1alpha1.Nodegroup{
+			ObjectMeta: v1.ObjectMeta{
+				Name:      "test",
+				Namespace: "test",
+			},
+			Spec: v1alpha1.NodegroupSpec{
+				Taints: []*v1alpha1.Taint{
+					{
+						Effect: &noSchedule,
+						Key:    &project,
+						Value:  &projectOne,
+					},
+					{
+						Effect: &noSchedule,
+						Key:    &owner,
+						Value:  &teamOne,
+					},
+				},
+			},
+		},
+	}
+
+	delta := &ackcompare.Delta{}
+	customPreCompare(delta, a, b)
+	assert.False(t, delta.DifferentAt("Spec.Taints"), "Taints are equals")
+
+	projectTwo := "projecttwo"
+
+	b = &resource{
+		ko: &v1alpha1.Nodegroup{
+			ObjectMeta: v1.ObjectMeta{
+				Name:      "test",
+				Namespace: "test",
+			},
+			Spec: v1alpha1.NodegroupSpec{
+				Taints: []*v1alpha1.Taint{
+					{
+						Effect: &noSchedule,
+						Key:    &project,
+						Value:  &projectTwo,
+					},
+					{
+						Effect: &noSchedule,
+						Key:    &owner,
+						Value:  &teamOne,
+					},
+				},
+			},
+		},
+	}
+
+	delta = &ackcompare.Delta{}
+	customPreCompare(delta, a, b)
+	assert.True(t, delta.DifferentAt("Spec.Taints"), "Taints are different")
+
+	other := "other"
+
+	b = &resource{
+		ko: &v1alpha1.Nodegroup{
+			ObjectMeta: v1.ObjectMeta{
+				Name:      "test",
+				Namespace: "test",
+			},
+			Spec: v1alpha1.NodegroupSpec{
+				Taints: []*v1alpha1.Taint{
+					{
+						Effect: &noSchedule,
+						Key:    &project,
+						Value:  &projectOne,
+					},
+					{
+						Effect: &noSchedule,
+						Key:    &owner,
+						Value:  &teamOne,
+					},
+					{
+						Effect: &noSchedule,
+						Key:    &other,
+						Value:  &other,
+					},
+				},
+			},
+		},
+	}
+
+	delta = &ackcompare.Delta{}
+	customPreCompare(delta, a, b)
+	assert.True(t, delta.DifferentAt("Spec.Taints"), "Taints have different length")
+}

pkg/resource/nodegroup/hook_test.go

@@ -153,6 +153,11 @@ def test_create_update_delete_nodegroup(self, simple_nodegroup, eks_client):
                         "value": "noexecuted",
                         "effect": "NO_EXECUTE"
                     },
+                    {
+                        "key": "owner",
+                        "value": "teamone",
+                        "effect": "NO_SCHEDULE"
+                    },
                 ],
             }
         }
@@ -171,10 +176,13 @@ def test_create_update_delete_nodegroup(self, simple_nodegroup, eks_client):
         assert aws_res["nodegroup"]["labels"]["toot"] == "shoot"
         assert aws_res["nodegroup"]["labels"]["boot"] == "snoot"
 
-        assert len(aws_res["nodegroup"]["taints"]) == 1
-        assert aws_res["nodegroup"]["taints"][0]["key"] == "ifbooted"
-        assert aws_res["nodegroup"]["taints"][0]["value"] == "noexecuted"
-        assert aws_res["nodegroup"]["taints"][0]["effect"] == "NO_EXECUTE"
+        assert len(aws_res["nodegroup"]["taints"]) == 2
+        assert aws_res["nodegroup"]["taints"][0]["key"] in ["ifbooted", "owner"]
+        assert aws_res["nodegroup"]["taints"][0]["value"] in ["noexecuted", "teamone"]
+        assert aws_res["nodegroup"]["taints"][0]["effect"] in ["NO_EXECUTE", "NO_SCHEDULE"]
+        assert aws_res["nodegroup"]["taints"][1]["key"] in ["ifbooted", "owner"]
+        assert aws_res["nodegroup"]["taints"][1]["value"] in ["noexecuted", "teamone"]
+        assert aws_res["nodegroup"]["taints"][1]["effect"] in ["NO_EXECUTE", "NO_SCHEDULE"]
 
         # Remove a label, update a label and remove a taint
         updates = {
@@ -183,7 +191,13 @@ def test_create_update_delete_nodegroup(self, simple_nodegroup, eks_client):
                     "toot": "updooted",
                     "boot": None
                 },
-                "taints": [],
+                "taints": [
+                    {
+                        "key": "ifbooted",
+                        "value": "noexecuted",
+                        "effect": "NO_EXECUTE"
+                    }
+                ],
             }
         }
 
@@ -201,4 +215,26 @@ def test_create_update_delete_nodegroup(self, simple_nodegroup, eks_client):
         assert aws_res["nodegroup"]["labels"]["toot"] == "updooted"
         assert "boot" not in aws_res["nodegroup"]["labels"]
 
+        assert len(aws_res["nodegroup"]["taints"]) == 1
+        assert aws_res["nodegroup"]["taints"][0]["key"] == "ifbooted"
+        assert aws_res["nodegroup"]["taints"][0]["value"] == "noexecuted"
+        assert aws_res["nodegroup"]["taints"][0]["effect"] == "NO_EXECUTE"
+
+        # Remove last taint
+        updates = {
+            "spec": {
+                "taints": [],
+            }
+        }
+
+        k8s.patch_custom_resource(ref, updates)
+        time.sleep(MODIFY_WAIT_AFTER_SECONDS)
+
+        wait_for_nodegroup_active(eks_client, cluster_name, nodegroup_name)
+
+        aws_res = eks_client.describe_nodegroup(
+            clusterName=cluster_name,
+            nodegroupName=nodegroup_name
+        )
+
         assert "taints" not in aws_res["nodegroup"]