Repository: keikoproj/instance-manager
Pull request #124 (2020-05-27 03:25:17)

AWS API Optimization - Update
Ref #112 

This refactors the update flow to remove redundant API calls

- [x] BDD Passing

codecov[bot]: # [Codecov](https://codecov.io/gh/keikoproj/instance-manager/pull/124?src=pr&el=h1) Report
> Merging [#124](https://codecov.io/gh/keikoproj/instance-manager/pull/124?src=pr&el=desc) into [master](https://codecov.io/gh/keikoproj/instance-manager/commit/358dcb438d5ad7bf645ff891845ef9cc916aae34&el=desc) will **decrease** coverage by `1.71%`.
> The diff coverage is `63.38%`.

[![Impacted file tree graph](https://codecov.io/gh/keikoproj/instance-manager/pull/124/graphs/tree.svg?width=650&height=150&src=pr&token=IJbjmSBliL)](https://codecov.io/gh/keikoproj/instance-manager/pull/124?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #124      +/-   ##
==========================================
- Coverage   87.59%   85.87%   -1.72%     
==========================================
  Files          10       10              
  Lines        1072     1126      +54     
==========================================
+ Hits          939      967      +28     
- Misses         96      116      +20     
- Partials       37       43       +6     
```


| [Impacted Files](https://codecov.io/gh/keikoproj/instance-manager/pull/124?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [controllers/provisioners/eks/helpers.go](https://codecov.io/gh/keikoproj/instance-manager/pull/124/diff?src=pr&el=tree#diff-Y29udHJvbGxlcnMvcHJvdmlzaW9uZXJzL2Vrcy9oZWxwZXJzLmdv) | `82.46% <50.00%> (-0.49%)` | :arrow_down: |
| [controllers/provisioners/eks/update.go](https://codecov.io/gh/keikoproj/instance-manager/pull/124/diff?src=pr&el=tree#diff-Y29udHJvbGxlcnMvcHJvdmlzaW9uZXJzL2Vrcy91cGRhdGUuZ28=) | `88.17% <61.40%> (-10.59%)` | :arrow_down: |
| [controllers/provisioners/eks/cloud.go](https://codecov.io/gh/keikoproj/instance-manager/pull/124/diff?src=pr&el=tree#diff-Y29udHJvbGxlcnMvcHJvdmlzaW9uZXJzL2Vrcy9jbG91ZC5nbw==) | `89.70% <75.00%> (-0.92%)` | :arrow_down: |
| [controllers/provisioners/eks/upgrade.go](https://codecov.io/gh/keikoproj/instance-manager/pull/124/diff?src=pr&el=tree#diff-Y29udHJvbGxlcnMvcHJvdmlzaW9uZXJzL2Vrcy91cGdyYWRlLmdv) | `94.80% <100.00%> (+0.06%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/keikoproj/instance-manager/pull/124?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/keikoproj/instance-manager/pull/124?src=pr&el=footer). Last update [358dcb4...cee1d8e](https://codecov.io/gh/keikoproj/instance-manager/pull/124?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/api/v1alpha1/instancegroup_types.go b/api/v1alpha1/instancegroup_types.go
index b4116a2..f368cab 100644
--- a/api/v1alpha1/instancegroup_types.go
+++ b/api/v1alpha1/instancegroup_types.go
@@ -363,6 +363,9 @@ func (c *EKSConfiguration) GetTags() []map[string]string {
 	}
 	return c.Tags
 }
+func (c *EKSConfiguration) SetTags(tags []map[string]string) {
+	c.Tags = tags
+}
 func (c *EKSConfiguration) GetSubnets() []string {
 	if c.Subnets == nil {
 		return []string{}
diff --git a/controllers/common/utils.go b/controllers/common/utils.go
index 0651784..9dbdec5 100644
--- a/controllers/common/utils.go
+++ b/controllers/common/utils.go
@@ -44,6 +44,27 @@ func ContainsEqualFold(slice []string, s string) bool {
 	return false
 }
 
+func StringMapSliceContains(m []map[string]string, contains map[string]string) bool {
+	for _, obj := range m {
+		if reflect.DeepEqual(obj, contains) {
+			return true
+		}
+	}
+	return false
+}
+
+func StringSliceEqualFold(x []string, y []string) bool {
+	if len(x) != len(y) {
+		return false
+	}
+	for _, element := range x {
+		if !ContainsEqualFold(y, element) {
+			return false
+		}
+	}
+	return true
+}
+
 func SliceEmpty(slice []string) bool {
 	return len(slice) == 0
 }
diff --git a/controllers/providers/aws/aws.go b/controllers/providers/aws/aws.go
index 1584ae3..c198b94 100644
--- a/controllers/providers/aws/aws.go
+++ b/controllers/providers/aws/aws.go
@@ -146,24 +146,29 @@ func (w *AwsWorker) CreateScalingGroup(input *autoscaling.CreateAutoScalingGroup
 	return nil
 }
 
-func (w *AwsWorker) UpdateScalingGroup(input *autoscaling.UpdateAutoScalingGroupInput, upTags []*autoscaling.Tag, rmTags []*autoscaling.Tag) error {
-	_, err := w.AsgClient.CreateOrUpdateTags(&autoscaling.CreateOrUpdateTagsInput{
-		Tags: upTags,
-	})
-	if err != nil {
-		return err
+func (w *AwsWorker) UpdateScalingGroupTags(add []*autoscaling.Tag, remove []*autoscaling.Tag) error {
+	if len(add) > 0 {
+		_, err := w.AsgClient.CreateOrUpdateTags(&autoscaling.CreateOrUpdateTagsInput{
+			Tags: add,
+		})
+		if err != nil {
+			return err
+		}
 	}
 
-	if len(rmTags) > 0 {
-		_, err = w.AsgClient.DeleteTags(&autoscaling.DeleteTagsInput{
-			Tags: rmTags,
+	if len(remove) > 0 {
+		_, err := w.AsgClient.DeleteTags(&autoscaling.DeleteTagsInput{
+			Tags: remove,
 		})
 		if err != nil {
 			return err
 		}
 	}
+	return nil
+}
 
-	_, err = w.AsgClient.UpdateAutoScalingGroup(input)
+func (w *AwsWorker) UpdateScalingGroup(input *autoscaling.UpdateAutoScalingGroupInput) error {
+	_, err := w.AsgClient.UpdateAutoScalingGroup(input)
 	if err != nil {
 		return err
 	}
@@ -516,21 +521,6 @@ func (w *AwsWorker) DescribeAutoscalingLaunchConfigs() ([]*autoscaling.LaunchCon
 	return launchConfigurations, nil
 }
 
-func (w *AwsWorker) GetAutoscalingGroup(name string) (*autoscaling.Group, error) {
-	var asg *autoscaling.Group
-	out, err := w.AsgClient.DescribeAutoScalingGroups(&autoscaling.DescribeAutoScalingGroupsInput{})
-	if err != nil {
-		return asg, err
-	}
-	for _, group := range out.AutoScalingGroups {
-		n := aws.StringValue(group.AutoScalingGroupName)
-		if strings.EqualFold(name, n) {
-			asg = group
-		}
-	}
-	return asg, nil
-}
-
 func (w *AwsWorker) EnableMetrics(asgName string, metrics []string) error {
 	if common.SliceEmpty(metrics) {
 		return nil
diff --git a/controllers/providers/kubernetes/rollingupdate.go b/controllers/providers/kubernetes/rollingupdate.go
index ff8ca96..6a84c5f 100644
--- a/controllers/providers/kubernetes/rollingupdate.go
+++ b/controllers/providers/kubernetes/rollingupdate.go
@@ -17,7 +17,7 @@ package kubernetes
 
 import (
 	awsprovider "github.com/keikoproj/instance-manager/controllers/providers/aws"
-	"k8s.io/client-go/kubernetes"
+	corev1 "k8s.io/api/core/v1"
 	ctrl "sigs.k8s.io/controller-runtime"
 )
 
@@ -31,7 +31,7 @@ var (
 
 type RollingUpdateRequest struct {
 	AwsWorker        awsprovider.AwsWorker
-	Kubernetes       kubernetes.Interface
+	ClusterNodes     *corev1.NodeList
 	ScalingGroupName string
 	MaxUnavailable   int
 	DesiredCapacity  int
@@ -61,7 +61,7 @@ func ProcessRollingUpgradeStrategy(req *RollingUpdateRequest) (bool, error) {
 		req.MaxUnavailable = req.DesiredCapacity
 	}
 
-	ok, err := IsMinNodesReady(req.Kubernetes, req.AllInstances, req.MaxUnavailable)
+	ok, err := IsMinNodesReady(req.ClusterNodes, req.AllInstances, req.MaxUnavailable)
 	if err != nil {
 		return false, err
 	}
diff --git a/controllers/providers/kubernetes/utils.go b/controllers/providers/kubernetes/utils.go
index 5865707..6da66af 100644
--- a/controllers/providers/kubernetes/utils.go
+++ b/controllers/providers/kubernetes/utils.go
@@ -52,12 +52,7 @@ func GetUnstructuredInstanceGroup(instanceGroup *v1alpha1.InstanceGroup) (*unstr
 	return obj, nil
 }
 
-func IsDesiredNodesReady(kube kubernetes.Interface, instanceIds []string, desiredCount int) (bool, error) {
-	nodes, err := kube.CoreV1().Nodes().List(metav1.ListOptions{})
-	if err != nil {
-		return false, err
-	}
-
+func IsDesiredNodesReady(nodes *corev1.NodeList, instanceIds []string, desiredCount int) (bool, error) {
 	if len(instanceIds) != desiredCount {
 		return false, nil
 	}
@@ -72,12 +67,7 @@ func IsDesiredNodesReady(kube kubernetes.Interface, instanceIds []string, desire
 	return false, nil
 }
 
-func IsMinNodesReady(kube kubernetes.Interface, instanceIds []string, minCount int) (bool, error) {
-	nodes, err := kube.CoreV1().Nodes().List(metav1.ListOptions{})
-	if err != nil {
-		return false, err
-	}
-
+func IsMinNodesReady(nodes *corev1.NodeList, instanceIds []string, minCount int) (bool, error) {
 	// if count of instances in scaling group is not over min, requeue
 	if len(instanceIds) < minCount {
 		return false, nil
diff --git a/controllers/provisioners/eks/cloud.go b/controllers/provisioners/eks/cloud.go
index 219d21d..064420a 100644
--- a/controllers/provisioners/eks/cloud.go
+++ b/controllers/provisioners/eks/cloud.go
@@ -22,6 +22,7 @@ import (
 	"github.com/keikoproj/instance-manager/api/v1alpha1"
 	kubeprovider "github.com/keikoproj/instance-manager/controllers/providers/kubernetes"
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/autoscaling"
@@ -32,6 +33,7 @@ import (
 type DiscoveredState struct {
 	Provisioned                   bool
 	NodesReady                    bool
+	ClusterNodes                  *corev1.NodeList
 	OwnedScalingGroups            []*autoscaling.Group
 	ScalingGroup                  *autoscaling.Group
 	LaunchConfigurations          []*autoscaling.LaunchConfiguration
@@ -59,6 +61,12 @@ func (ctx *EksInstanceGroupContext) CloudDiscovery() error {
 		ResourceVersion: instanceGroup.GetResourceVersion(),
 	}
 
+	nodes, err := ctx.KubernetesClient.Kubernetes.CoreV1().Nodes().List(metav1.ListOptions{})
+	if err != nil {
+		return errors.Wrap(err, "failed to list cluster nodes")
+	}
+	state.SetClusterNodes(nodes)
+
 	var roleName, instanceProfileName string
 	if configuration.HasExistingRole() {
 		roleName = configuration.GetRoleName()
@@ -242,3 +250,9 @@ func (d *DiscoveredState) SetNodesReady(condition bool) {
 func (d *DiscoveredState) IsNodesReady() bool {
 	return d.NodesReady
 }
+func (d *DiscoveredState) SetClusterNodes(nodes *corev1.NodeList) {
+	d.ClusterNodes = nodes
+}
+func (d *DiscoveredState) GetClusterNodes() *corev1.NodeList {
+	return d.ClusterNodes
+}
diff --git a/controllers/provisioners/eks/helpers.go b/controllers/provisioners/eks/helpers.go
index d19950b..683837f 100644
--- a/controllers/provisioners/eks/helpers.go
+++ b/controllers/provisioners/eks/helpers.go
@@ -87,30 +87,25 @@ func (ctx *EksInstanceGroupContext) GetAddedTags(asgName string) []*autoscaling.
 
 func (ctx *EksInstanceGroupContext) GetRemovedTags(asgName string) []*autoscaling.Tag {
 	var (
-		existingTags []*autoscaling.Tag
 		removal      []*autoscaling.Tag
 		state        = ctx.GetDiscoveredState()
 		scalingGroup = state.GetScalingGroup()
 		addedTags    = ctx.GetAddedTags(asgName)
 	)
 
-	// get existing tags
 	for _, tag := range scalingGroup.Tags {
-		existingTags = append(existingTags, ctx.AwsWorker.NewTag(aws.StringValue(tag.Key), aws.StringValue(tag.Value), asgName))
-	}
-
-	// find removals against incoming tags
-	for _, tag := range existingTags {
 		var match bool
 		for _, t := range addedTags {
-			if reflect.DeepEqual(t, tag) {
+			if aws.StringValue(t.Key) == aws.StringValue(tag.Key) {
 				match = true
 			}
 		}
 		if !match {
-			removal = append(removal, tag)
+			matchedTag := ctx.AwsWorker.NewTag(aws.StringValue(tag.Key), aws.StringValue(tag.Value), asgName)
+			removal = append(removal, matchedTag)
 		}
 	}
+
 	return removal
 }
 
@@ -285,8 +280,13 @@ func (ctx *EksInstanceGroupContext) UpdateNodeReadyCondition() bool {
 		status        = instanceGroup.GetStatus()
 		scalingGroup  = state.GetScalingGroup()
 		desiredCount  = int(aws.Int64Value(scalingGroup.DesiredCapacity))
+		nodes         = state.GetClusterNodes()
 	)
 
+	if scalingGroup == nil {
+		return false
+	}
+
 	ctx.Log.Info("waiting for node readiness conditions", "instancegroup", instanceGroup.GetName())
 	if len(scalingGroup.Instances) != desiredCount {
 		// if instances don't match desired, a scaling activity is in progress
@@ -301,7 +301,7 @@ func (ctx *EksInstanceGroupContext) UpdateNodeReadyCondition() bool {
 	instances := strings.Join(instanceIds, ",")
 
 	var conditions []v1alpha1.InstanceGroupCondition
-	ok, err := kubeprovider.IsDesiredNodesReady(ctx.KubernetesClient.Kubernetes, instanceIds, desiredCount)
+	ok, err := kubeprovider.IsDesiredNodesReady(nodes, instanceIds, desiredCount)
 	if err != nil {
 		ctx.Log.Error(err, "could not update node conditions", "instancegroup", instanceGroup.GetName())
 		return false
diff --git a/controllers/provisioners/eks/update.go b/controllers/provisioners/eks/update.go
index e28d81b..03ed2c7 100644
--- a/controllers/provisioners/eks/update.go
+++ b/controllers/provisioners/eks/update.go
@@ -18,6 +18,7 @@ package eks
 import (
 	"fmt"
 	"reflect"
+	"strings"
 
 	"github.com/pkg/errors"
 
@@ -63,8 +64,7 @@ func (ctx *EksInstanceGroupContext) Update() error {
 
 	// we should try to bootstrap the role before we wait for nodes to be ready
 	// to avoid getting locked if someone made a manual change to aws-auth
-	err = ctx.BootstrapNodes()
-	if err != nil {
+	if err = ctx.BootstrapNodes(); err != nil {
 		ctx.Log.Info("failed to bootstrap role, will retry", "error", err, "instancegroup", instanceGroup.GetName())
 	}
 
@@ -93,25 +93,27 @@ func (ctx *EksInstanceGroupContext) UpdateScalingGroup() error {
 		rmTags        = ctx.GetRemovedTags(asgName)
 	)
 
-	err := ctx.AwsWorker.UpdateScalingGroup(&autoscaling.UpdateAutoScalingGroupInput{
-		AutoScalingGroupName:    aws.String(asgName),
-		LaunchConfigurationName: aws.String(state.GetActiveLaunchConfigurationName()),
-		MinSize:                 aws.Int64(spec.GetMinSize()),
-		MaxSize:                 aws.Int64(spec.GetMaxSize()),
-		VPCZoneIdentifier:       aws.String(common.ConcatenateList(configuration.GetSubnets(), ",")),
-	}, tags, rmTags)
-	if err != nil {
-		return err
-	}
-	ctx.Log.Info("updated scaling group", "instancegroup", instanceGroup.GetName(), "scalinggroup", asgName)
+	if ctx.ScalingGroupUpdateNeeded() {
+		err := ctx.AwsWorker.UpdateScalingGroup(&autoscaling.UpdateAutoScalingGroupInput{
+			AutoScalingGroupName:    aws.String(asgName),
+			LaunchConfigurationName: aws.String(state.GetActiveLaunchConfigurationName()),
+			MinSize:                 aws.Int64(spec.GetMinSize()),
+			MaxSize:                 aws.Int64(spec.GetMaxSize()),
+			VPCZoneIdentifier:       aws.String(common.ConcatenateList(configuration.GetSubnets(), ",")),
+		})
+		if err != nil {
+			return err
+		}
 
-	scalingGroup, err = ctx.AwsWorker.GetAutoscalingGroup(asgName)
-	if err != nil {
-		return err
+		ctx.Log.Info("updated scaling group", "instancegroup", instanceGroup.GetName(), "scalinggroup", asgName)
 	}
 
-	if scalingGroup != nil {
-		state.SetScalingGroup(scalingGroup)
+	if ctx.TagsUpdateNeeded() {
+		err := ctx.AwsWorker.UpdateScalingGroupTags(tags, rmTags)
+		if err != nil {
+			return err
+		}
+		ctx.Log.Info("updated scaling group tags", "instancegroup", instanceGroup.GetName(), "scalinggroup", asgName)
 	}
 
 	if err := ctx.UpdateMetricsCollection(asgName); err != nil {
@@ -141,6 +143,69 @@ func (ctx *EksInstanceGroupContext) RotationNeeded() bool {
 	return false
 }
 
+func (ctx *EksInstanceGroupContext) TagsUpdateNeeded() bool {
+	var (
+		instanceGroup = ctx.GetInstanceGroup()
+		configuration = instanceGroup.GetEKSConfiguration()
+		state         = ctx.GetDiscoveredState()
+		scalingGroup  = state.GetScalingGroup()
+		asgName       = aws.StringValue(scalingGroup.AutoScalingGroupName)
+		rmTags        = ctx.GetRemovedTags(asgName)
+	)
+
+	if len(rmTags) > 0 {
+		return true
+	}
+
+	existingTags := make([]map[string]string, 0)
+	for _, tag := range scalingGroup.Tags {
+		tagSet := map[string]string{
+			"key":   aws.StringValue(tag.Key),
+			"value": aws.StringValue(tag.Value),
+		}
+		existingTags = append(existingTags, tagSet)
+	}
+
+	for _, tag := range configuration.GetTags() {
+		if !common.StringMapSliceContains(existingTags, tag) {
+			return true
+		}
+	}
+
+	return false
+}
+
+func (ctx *EksInstanceGroupContext) ScalingGroupUpdateNeeded() bool {
+	var (
+		instanceGroup  = ctx.GetInstanceGroup()
+		spec           = instanceGroup.GetEKSSpec()
+		configuration  = instanceGroup.GetEKSConfiguration()
+		state          = ctx.GetDiscoveredState()
+		scalingGroup   = state.GetScalingGroup()
+		zoneIdentifier = aws.StringValue(scalingGroup.VPCZoneIdentifier)
+		groupSubnets   = strings.Split(zoneIdentifier, ",")
+		specSubnets    = configuration.GetSubnets()
+	)
+
+	if state.GetActiveLaunchConfigurationName() != aws.StringValue(scalingGroup.LaunchConfigurationName) {
+		return true
+	}
+
+	if spec.GetMinSize() != aws.Int64Value(scalingGroup.MinSize) {
+		return true
+	}
+
+	if spec.GetMaxSize() != aws.Int64Value(scalingGroup.MaxSize) {
+		return true
+	}
+
+	if !common.StringSliceEqualFold(specSubnets, groupSubnets) {
+		return true
+	}
+
+	return false
+}
+
 func (ctx *EksInstanceGroupContext) LaunchConfigurationDrifted() bool {
 	var (
 		state         = ctx.GetDiscoveredState()
diff --git a/controllers/provisioners/eks/upgrade.go b/controllers/provisioners/eks/upgrade.go
index 5545d01..98dbe98 100644
--- a/controllers/provisioners/eks/upgrade.go
+++ b/controllers/provisioners/eks/upgrade.go
@@ -92,6 +92,7 @@ func (ctx *EksInstanceGroupContext) NewRollingUpdateRequest() *kubeprovider.Roll
 		needsUpdate        []string
 		allInstances       []string
 		instanceGroup      = ctx.GetInstanceGroup()
+		state              = ctx.GetDiscoveredState()
 		scalingGroup       = ctx.GetDiscoveredState().GetScalingGroup()
 		activeLaunchConfig = aws.StringValue(scalingGroup.LaunchConfigurationName)
 		desiredCount       = int(aws.Int64Value(scalingGroup.DesiredCapacity))
@@ -122,7 +123,7 @@ func (ctx *EksInstanceGroupContext) NewRollingUpdateRequest() *kubeprovider.Roll
 
 	return &kubeprovider.RollingUpdateRequest{
 		AwsWorker:        ctx.AwsWorker,
-		Kubernetes:       ctx.KubernetesClient.Kubernetes,
+		ClusterNodes:     state.GetClusterNodes(),
 		MaxUnavailable:   unavailableInt,
 		DesiredCapacity:  desiredCount,
 		AllInstances:     allInstances,


Test patch:
diff --git a/controllers/provisioners/eks/eks_test.go b/controllers/provisioners/eks/eks_test.go
index f1c64e2..f51779d 100644
--- a/controllers/provisioners/eks/eks_test.go
+++ b/controllers/provisioners/eks/eks_test.go
@@ -168,6 +168,7 @@ func MockScalingGroup(name string, t ...*autoscaling.TagDescription) *autoscalin
 		Tags:                    t,
 		MinSize:                 aws.Int64(3),
 		MaxSize:                 aws.Int64(6),
+		VPCZoneIdentifier:       aws.String("subnet-1,subnet-2,subnet-3"),
 	}
 }
 
diff --git a/controllers/provisioners/eks/update_test.go b/controllers/provisioners/eks/update_test.go
index 6d228fe..aaeb2c3 100644
--- a/controllers/provisioners/eks/update_test.go
+++ b/controllers/provisioners/eks/update_test.go
@@ -27,6 +27,7 @@ import (
 	"github.com/onsi/gomega"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 func TestUpdateScalingGroupPositive(t *testing.T) {
@@ -40,7 +41,15 @@ func TestUpdateScalingGroupPositive(t *testing.T) {
 
 	w := MockAwsWorker(asgMock, iamMock)
 	ctx := MockContext(ig, k, w)
+
+	mockTags := []map[string]string{
+		{
+			"key":   "some-tag",
+			"value": "some-different-value",
+		},
+	}
 	ig.GetEKSConfiguration().SetMetricsCollection([]string{"GroupMinSize", "GroupMaxSize", "GroupDesiredCapacity"})
+	ig.GetEKSConfiguration().SetTags(mockTags)
 
 	// avoid drift / rotation
 	input := ctx.GetLaunchConfigurationInput("some-launch-config")
@@ -55,18 +64,15 @@ func TestUpdateScalingGroupPositive(t *testing.T) {
 				LaunchConfigurationName: aws.String("some-launch-config"),
 			},
 		},
+		Tags: []*autoscaling.TagDescription{
+			{
+				Key:   aws.String("some-tag"),
+				Value: aws.String("some-value"),
+			},
+		},
 	}
 	asgMock.AutoScalingGroups = []*autoscaling.Group{mockScalingGroup}
 
-	ctx.SetDiscoveredState(&DiscoveredState{
-		Publisher: kubeprovider.EventPublisher{
-			Client: k.Kubernetes,
-		},
-		ScalingGroup:                  mockScalingGroup,
-		ActiveLaunchConfigurationName: "some-launch-config",
-		LaunchConfiguration:           mockLaunchConfig,
-	})
-
 	// create matching node object
 	mockNode := &corev1.Node{
 		Spec: corev1.NodeSpec{
@@ -84,8 +90,22 @@ func TestUpdateScalingGroupPositive(t *testing.T) {
 	_, err := k.Kubernetes.CoreV1().Nodes().Create(mockNode)
 	g.Expect(err).NotTo(gomega.HaveOccurred())
 
+	nodes, err := k.Kubernetes.CoreV1().Nodes().List(metav1.ListOptions{})
+	g.Expect(err).NotTo(gomega.HaveOccurred())
+
+	ctx.SetDiscoveredState(&DiscoveredState{
+		Publisher: kubeprovider.EventPublisher{
+			Client: k.Kubernetes,
+		},
+		ScalingGroup:                  mockScalingGroup,
+		ActiveLaunchConfigurationName: "some-launch-config",
+		LaunchConfiguration:           mockLaunchConfig,
+		ClusterNodes:                  nodes,
+	})
+
 	err = ctx.Update()
 	g.Expect(err).NotTo(gomega.HaveOccurred())
+	g.Expect(ctx.TagsUpdateNeeded()).To(gomega.BeTrue())
 	g.Expect(ctx.GetState()).To(gomega.Equal(v1alpha1.ReconcileModified))
 }
 
@@ -110,21 +130,15 @@ func TestUpdateWithDriftRotationPositive(t *testing.T) {
 				LaunchConfigurationName: aws.String("some-launch-config"),
 			},
 		},
+		Tags: []*autoscaling.TagDescription{
+			{
+				Key:   aws.String("some-tag"),
+				Value: aws.String("some-value"),
+			},
+		},
 	}
 	asgMock.AutoScalingGroups = []*autoscaling.Group{mockScalingGroup}
 
-	// missing launch config causes drift
-	ctx.SetDiscoveredState(&DiscoveredState{
-		Publisher: kubeprovider.EventPublisher{
-			Client: k.Kubernetes,
-		},
-		ScalingGroup:                  mockScalingGroup,
-		ActiveLaunchConfigurationName: "some-launch-config",
-		InstanceProfile: &iam.InstanceProfile{
-			Arn: aws.String("some-instance-arn"),
-		},
-	})
-
 	// create matching node object
 	mockNode := &corev1.Node{
 		Spec: corev1.NodeSpec{
@@ -142,8 +156,25 @@ func TestUpdateWithDriftRotationPositive(t *testing.T) {
 	_, err := k.Kubernetes.CoreV1().Nodes().Create(mockNode)
 	g.Expect(err).NotTo(gomega.HaveOccurred())
 
+	nodes, err := k.Kubernetes.CoreV1().Nodes().List(metav1.ListOptions{})
+	g.Expect(err).NotTo(gomega.HaveOccurred())
+
+	// missing launch config causes drift
+	ctx.SetDiscoveredState(&DiscoveredState{
+		Publisher: kubeprovider.EventPublisher{
+			Client: k.Kubernetes,
+		},
+		ScalingGroup:                  mockScalingGroup,
+		ActiveLaunchConfigurationName: "some-launch-config",
+		InstanceProfile: &iam.InstanceProfile{
+			Arn: aws.String("some-instance-arn"),
+		},
+		ClusterNodes: nodes,
+	})
+
 	err = ctx.Update()
 	g.Expect(err).NotTo(gomega.HaveOccurred())
+	g.Expect(ctx.TagsUpdateNeeded()).To(gomega.BeTrue())
 	g.Expect(ctx.GetState()).To(gomega.Equal(v1alpha1.ReconcileInitUpgrade))
 }
 
@@ -176,15 +207,6 @@ func TestUpdateWithRotationPositive(t *testing.T) {
 	}
 	asgMock.AutoScalingGroups = []*autoscaling.Group{mockScalingGroup}
 
-	ctx.SetDiscoveredState(&DiscoveredState{
-		Publisher: kubeprovider.EventPublisher{
-			Client: k.Kubernetes,
-		},
-		ScalingGroup:                  mockScalingGroup,
-		ActiveLaunchConfigurationName: "some-launch-config",
-		LaunchConfiguration:           mockLaunchConfig,
-	})
-
 	// create matching node object
 	mockNode := &corev1.Node{
 		Spec: corev1.NodeSpec{
@@ -202,6 +224,19 @@ func TestUpdateWithRotationPositive(t *testing.T) {
 	_, err := k.Kubernetes.CoreV1().Nodes().Create(mockNode)
 	g.Expect(err).NotTo(gomega.HaveOccurred())
 
+	nodes, err := k.Kubernetes.CoreV1().Nodes().List(metav1.ListOptions{})
+	g.Expect(err).NotTo(gomega.HaveOccurred())
+
+	ctx.SetDiscoveredState(&DiscoveredState{
+		Publisher: kubeprovider.EventPublisher{
+			Client: k.Kubernetes,
+		},
+		ScalingGroup:                  mockScalingGroup,
+		ActiveLaunchConfigurationName: "some-launch-config",
+		LaunchConfiguration:           mockLaunchConfig,
+		ClusterNodes:                  nodes,
+	})
+
 	err = ctx.Update()
 	g.Expect(err).NotTo(gomega.HaveOccurred())
 	g.Expect(ctx.GetState()).To(gomega.Equal(v1alpha1.ReconcileInitUpgrade))
@@ -299,15 +334,8 @@ func TestUpdateScalingGroupNegative(t *testing.T) {
 		},
 	})
 
-	asgMock.DescribeAutoScalingGroupsErr = errors.New("some-describe-error")
-	err := ctx.Update()
-	t.Log(err)
-	g.Expect(err).To(gomega.HaveOccurred())
-	g.Expect(ctx.GetState()).To(gomega.Equal(v1alpha1.ReconcileModifying))
-	asgMock.DescribeAutoScalingGroupsErr = nil
-
 	asgMock.UpdateAutoScalingGroupErr = errors.New("some-update-error")
-	err = ctx.Update()
+	err := ctx.Update()
 	t.Log(err)
 	g.Expect(err).To(gomega.HaveOccurred())
 	g.Expect(ctx.GetState()).To(gomega.Equal(v1alpha1.ReconcileModifying))
@@ -342,3 +370,53 @@ func TestUpdateScalingGroupNegative(t *testing.T) {
 	g.Expect(err).To(gomega.HaveOccurred())
 	g.Expect(ctx.GetState()).To(gomega.Equal(v1alpha1.ReconcileModifying))
 }
+
+func TestScalingGroupUpdatePredicate(t *testing.T) {
+	var (
+		g             = gomega.NewGomegaWithT(t)
+		k             = MockKubernetesClientSet()
+		ig            = MockInstanceGroup()
+		spec          = ig.GetEKSSpec()
+		configuration = ig.GetEKSConfiguration()
+		asgMock       = NewAutoScalingMocker()
+		iamMock       = NewIamMocker()
+	)
+
+	w := MockAwsWorker(asgMock, iamMock)
+	ctx := MockContext(ig, k, w)
+	spec.MinSize = int64(3)
+	spec.MaxSize = int64(6)
+	configuration.SetSubnets([]string{"subnet-1", "subnet-2", "subnet-3"})
+
+	mockScalingGroupMin := MockScalingGroup("asg-1")
+	mockScalingGroupMin.MinSize = aws.Int64(0)
+	mockScalingGroupMax := MockScalingGroup("asg-2")
+	mockScalingGroupMax.MaxSize = aws.Int64(0)
+	mockScalingGroupSubnets := MockScalingGroup("asg-3")
+	mockScalingGroupSubnets.VPCZoneIdentifier = aws.String("subnet-0")
+	mockScalingGroupLaunchConfig := MockScalingGroup("asg-4")
+	mockScalingGroupLaunchConfig.LaunchConfigurationName = aws.String("different-name")
+
+	tests := []struct {
+		input    *autoscaling.Group
+		expected bool
+	}{
+		{input: MockScalingGroup("asg-0"), expected: false},
+		{input: mockScalingGroupLaunchConfig, expected: true},
+		{input: mockScalingGroupMin, expected: true},
+		{input: mockScalingGroupMax, expected: true},
+		{input: mockScalingGroupSubnets, expected: true},
+	}
+
+	for _, tc := range tests {
+		ctx.SetDiscoveredState(&DiscoveredState{
+			Publisher: kubeprovider.EventPublisher{
+				Client: k.Kubernetes,
+			},
+			ScalingGroup:                  tc.input,
+			ActiveLaunchConfigurationName: "some-launch-configuration",
+		})
+		got := ctx.ScalingGroupUpdateNeeded()
+		g.Expect(got).To(gomega.Equal(tc.expected))
+	}
+}
diff --git a/controllers/provisioners/eks/upgrade_test.go b/controllers/provisioners/eks/upgrade_test.go
index 0abd9c8..9cde740 100644
--- a/controllers/provisioners/eks/upgrade_test.go
+++ b/controllers/provisioners/eks/upgrade_test.go
@@ -265,6 +265,9 @@ func TestUpgradeRollingUpdateStrategyPositive(t *testing.T) {
 			g.Expect(err).NotTo(gomega.HaveOccurred())
 		}
 
+		nodes, err := k.Kubernetes.CoreV1().Nodes().List(metav1.ListOptions{})
+		g.Expect(err).NotTo(gomega.HaveOccurred())
+
 		ig.SetUpgradeStrategy(MockAwsRollingUpdateStrategy(&tc.maxUnavailable))
 		ctx.SetDiscoveredState(&DiscoveredState{
 			Publisher: kubeprovider.EventPublisher{
@@ -276,6 +279,7 @@ func TestUpgradeRollingUpdateStrategyPositive(t *testing.T) {
 				Instances:               tc.scalingInstances,
 				DesiredCapacity:         aws.Int64(int64(len(tc.scalingInstances))),
 			},
+			ClusterNodes: nodes,
 		})
 
 		ig.SetState(v1alpha1.ReconcileModifying)