Repository: keikoproj/instance-manager (go)

Issue:
Optimize AWS API calls
Right now we are making quite a bit of API calls when instance groups get reconciled, when there are many instance groups we can get throttled pretty hard.

We should try to optimize and possibly reduce the amount of the calls being made.
This will help with both adding caching #87 and help reduce throttling

#### Create
- The `CreateUpdateScalingGroupRole` method does a `GetRole` to check existence, it is already done by `CloudDiscovery`.
- The `CreateUpdateScalingGroupRole` method does a `GetInstanceProfile` to check existence, it is already done by `CloudDiscovery`.
- The `CreateUpdateScalingGroupRole` method does a `WaitUntilInstanceProfileExists`, this is not needed when there is propagation delay
- The `CreateScalingGroup` method does not need to call `GetAutoscalingGroup` after create, it can be discovered by the next reconcile
- The `CreateLaunchConfiguration` method does not need to call `GetAutoscalingLaunchConfig` after creating it, if we refactor to get the name from `Create()` which can be passed to `CreateScalingGroup` after.

#### Update
- The `UpdateScalingGroup` method can avoid doing a `GetAutoscalingGroup` after update, if we refactor `UpdateNodeReadyCondition` to requeue if there is no scalinggroup in state.
- The `UpdateScalingGroup` method can avoid calling `UpdateScalingGroup` if Min/Max or subnets had not changed as these are the only thing it updates.
- The `UpdateScalingGroup` method could cache the scaling group tags, and decide if call to `CreateOrUpdateTags` or `DeleteTags` is required
- `IsDesiredNodesReady` should use a cached version of nodes, which we should cache in `CloudDiscovery`



Pull request description:
AWS API Optimization - IAM
Fixes #112 

This optimizes the IAM calls we make by only trying to attach managed policies if they are not attached.

- [x] BDD test

Patch:
diff --git a/controllers/providers/aws/aws.go b/controllers/providers/aws/aws.go
index c198b94..b49019b 100644
--- a/controllers/providers/aws/aws.go
+++ b/controllers/providers/aws/aws.go
@@ -50,7 +50,7 @@ type AwsWorker struct {
 }
 
 var (
-	DefaultInstanceProfilePropagationDelay = time.Second * 20
+	DefaultInstanceProfilePropagationDelay = time.Second * 25
 	DefaultWaiterDuration                  = time.Second * 5
 	DefaultWaiterRetries                   = 12
 
@@ -286,7 +286,51 @@ func (w *AwsWorker) DeleteScalingGroupRole(name string, managedPolicies []string
 	return nil
 }
 
-func (w *AwsWorker) CreateUpdateScalingGroupRole(name string, managedPolicies []string) (*iam.Role, *iam.InstanceProfile, error) {
+func (w *AwsWorker) AttachManagedPolicies(name string, managedPolicies []string) error {
+	for _, policy := range managedPolicies {
+		_, err := w.IamClient.AttachRolePolicy(&iam.AttachRolePolicyInput{
+			RoleName:  aws.String(name),
+			PolicyArn: aws.String(policy),
+		})
+		if err != nil {
+			return errors.Wrap(err, "failed to attach role policies")
+		}
+	}
+	return nil
+}
+
+func (w *AwsWorker) DetachManagedPolicies(name string, managedPolicies []string) error {
+	for _, policy := range managedPolicies {
+		_, err := w.IamClient.DetachRolePolicy(&iam.DetachRolePolicyInput{
+			RoleName:  aws.String(name),
+			PolicyArn: aws.String(policy),
+		})
+		if err != nil {
+			return errors.Wrap(err, "failed to detach role policies")
+		}
+	}
+	return nil
+}
+
+func (w *AwsWorker) ListRolePolicies(name string) ([]*iam.AttachedPolicy, error) {
+	policies := []*iam.AttachedPolicy{}
+	err := w.IamClient.ListAttachedRolePoliciesPages(
+		&iam.ListAttachedRolePoliciesInput{
+			RoleName: aws.String(name),
+		},
+		func(page *iam.ListAttachedRolePoliciesOutput, lastPage bool) bool {
+			for _, p := range page.AttachedPolicies {
+				policies = append(policies, p)
+			}
+			return page.Marker != nil
+		})
+	if err != nil {
+		return policies, err
+	}
+	return policies, nil
+}
+
+func (w *AwsWorker) CreateScalingGroupRole(name string) (*iam.Role, *iam.InstanceProfile, error) {
 	var (
 		assumeRolePolicyDocument = `{
 			"Version": "2012-10-17",
@@ -323,30 +367,21 @@ func (w *AwsWorker) CreateUpdateScalingGroupRole(name string, managedPolicies []
 		}
 		createdProfile = out.InstanceProfile
 		time.Sleep(DefaultInstanceProfilePropagationDelay)
-	} else {
-		createdProfile = instanceProfile
-	}
 
-	_, err := w.IamClient.AddRoleToInstanceProfile(&iam.AddRoleToInstanceProfileInput{
-		InstanceProfileName: aws.String(name),
-		RoleName:            aws.String(name),
-	})
-	if err != nil {
-		if aerr, ok := err.(awserr.Error); ok {
-			if aerr.Code() != iam.ErrCodeLimitExceededException {
-				return createdRole, createdProfile, errors.Wrap(err, "failed to attach instance-profile")
-			}
-		}
-	}
-
-	for _, policy := range managedPolicies {
-		_, err = w.IamClient.AttachRolePolicy(&iam.AttachRolePolicyInput{
-			RoleName:  aws.String(name),
-			PolicyArn: aws.String(policy),
+		_, err = w.IamClient.AddRoleToInstanceProfile(&iam.AddRoleToInstanceProfileInput{
+			InstanceProfileName: aws.String(name),
+			RoleName:            aws.String(name),
 		})
 		if err != nil {
-			return createdRole, createdProfile, errors.Wrap(err, "failed to attach policies")
+			if aerr, ok := err.(awserr.Error); ok {
+				if aerr.Code() != iam.ErrCodeLimitExceededException {
+					return createdRole, createdProfile, errors.Wrap(err, "failed to attach instance-profile")
+				}
+			}
 		}
+
+	} else {
+		createdProfile = instanceProfile
 	}
 
 	return createdRole, createdProfile, nil
diff --git a/controllers/provisioners/eks/cloud.go b/controllers/provisioners/eks/cloud.go
index 064420a..f57c133 100644
--- a/controllers/provisioners/eks/cloud.go
+++ b/controllers/provisioners/eks/cloud.go
@@ -40,6 +40,7 @@ type DiscoveredState struct {
 	LaunchConfiguration           *autoscaling.LaunchConfiguration
 	ActiveLaunchConfigurationName string
 	IAMRole                       *iam.Role
+	AttachedPolicies              []*iam.AttachedPolicy
 	InstanceProfile               *iam.InstanceProfile
 	Publisher                     kubeprovider.EventPublisher
 }
@@ -80,6 +81,14 @@ func (ctx *EksInstanceGroupContext) CloudDiscovery() error {
 	if val, ok := ctx.AwsWorker.RoleExist(roleName); ok {
 		state.SetRole(val)
 		status.SetNodesArn(aws.StringValue(val.Arn))
+
+		if !configuration.HasExistingRole() {
+			policies, err := ctx.AwsWorker.ListRolePolicies(roleName)
+			if err != nil {
+				return errors.Wrap(err, "failed to list attached role policies")
+			}
+			state.SetAttachedPolicies(policies)
+		}
 	}
 
 	if val, ok := ctx.AwsWorker.InstanceProfileExist(instanceProfileName); ok {
@@ -184,6 +193,15 @@ func (d *DiscoveredState) SetOwnedScalingGroups(groups []*autoscaling.Group) {
 func (d *DiscoveredState) GetOwnedScalingGroups() []*autoscaling.Group {
 	return d.OwnedScalingGroups
 }
+func (d *DiscoveredState) SetAttachedPolicies(policies []*iam.AttachedPolicy) {
+	d.AttachedPolicies = policies
+}
+func (d *DiscoveredState) GetAttachedPolicies() []*iam.AttachedPolicy {
+	if d.AttachedPolicies == nil {
+		d.AttachedPolicies = []*iam.AttachedPolicy{}
+	}
+	return d.AttachedPolicies
+}
 func (d *DiscoveredState) SetLaunchConfiguration(lc *autoscaling.LaunchConfiguration) {
 	if lc != nil {
 		d.LaunchConfiguration = lc
diff --git a/controllers/provisioners/eks/create.go b/controllers/provisioners/eks/create.go
index 4b4dc15..6243c16 100644
--- a/controllers/provisioners/eks/create.go
+++ b/controllers/provisioners/eks/create.go
@@ -122,25 +122,28 @@ func (ctx *EksInstanceGroupContext) CreateLaunchConfiguration(name string) error
 
 func (ctx *EksInstanceGroupContext) CreateManagedRole() error {
 	var (
-		instanceGroup      = ctx.GetInstanceGroup()
-		state              = ctx.GetDiscoveredState()
-		configuration      = instanceGroup.GetEKSConfiguration()
-		additionalPolicies = configuration.GetManagedPolicies()
-		roleName           = ctx.ResourcePrefix
+		instanceGroup = ctx.GetInstanceGroup()
+		state         = ctx.GetDiscoveredState()
+		configuration = instanceGroup.GetEKSConfiguration()
+		roleName      = ctx.ResourcePrefix
 	)
 
 	if configuration.HasExistingRole() {
+		// avoid updating if using an existing role
 		return nil
 	}
 
-	// create a controller-owned role for the instancegroup
-	managedPolicies := ctx.GetManagedPoliciesList(additionalPolicies)
+	role, profile, err := ctx.AwsWorker.CreateScalingGroupRole(roleName)
+	if err != nil {
+		return errors.Wrap(err, "failed to create scaling group role")
+	}
 
-	role, profile, err := ctx.AwsWorker.CreateUpdateScalingGroupRole(roleName, managedPolicies)
+	err = ctx.UpdateManagedPolicies(roleName)
 	if err != nil {
-		return err
+		return errors.Wrap(err, "failed to update managed policies")
 	}
-	ctx.Log.Info("created managed role", "instancegroup", instanceGroup.GetName(), "iamrole", roleName)
+
+	ctx.Log.Info("reconciled managed role", "instancegroup", instanceGroup.GetName(), "iamrole", roleName)
 
 	state.SetRole(role)
 	state.SetInstanceProfile(profile)
diff --git a/controllers/provisioners/eks/helpers.go b/controllers/provisioners/eks/helpers.go
index 683837f..502034f 100644
--- a/controllers/provisioners/eks/helpers.go
+++ b/controllers/provisioners/eks/helpers.go
@@ -310,6 +310,7 @@ func (ctx *EksInstanceGroupContext) UpdateNodeReadyCondition() bool {
 		if !state.IsNodesReady() {
 			state.Publisher.Publish(kubeprovider.NodesReadyEvent, "instancegroup", instanceGroup.GetName(), "instances", instances)
 		}
+		ctx.Log.Info("desired nodes are ready", "instancegroup", instanceGroup.GetName(), "instances", instances)
 		state.SetNodesReady(true)
 		conditions = append(conditions, v1alpha1.NewInstanceGroupCondition(v1alpha1.NodesReady, corev1.ConditionTrue))
 		status.SetConditions(conditions)
@@ -319,6 +320,7 @@ func (ctx *EksInstanceGroupContext) UpdateNodeReadyCondition() bool {
 	if state.IsNodesReady() {
 		state.Publisher.Publish(kubeprovider.NodesNotReadyEvent, "instancegroup", instanceGroup.GetName(), "instances", instances)
 	}
+	ctx.Log.Info("desired nodes are not ready", "instancegroup", instanceGroup.GetName(), "instances", instances)
 	state.SetNodesReady(false)
 	conditions = append(conditions, v1alpha1.NewInstanceGroupCondition(v1alpha1.NodesReady, corev1.ConditionFalse))
 	status.SetConditions(conditions)
diff --git a/controllers/provisioners/eks/update.go b/controllers/provisioners/eks/update.go
index 03ed2c7..3e907d7 100644
--- a/controllers/provisioners/eks/update.go
+++ b/controllers/provisioners/eks/update.go
@@ -319,3 +319,51 @@ func (ctx *EksInstanceGroupContext) LaunchConfigurationDrifted() bool {
 
 	return drift
 }
+
+func (ctx *EksInstanceGroupContext) UpdateManagedPolicies(roleName string) error {
+	var (
+		instanceGroup      = ctx.GetInstanceGroup()
+		state              = ctx.GetDiscoveredState()
+		configuration      = instanceGroup.GetEKSConfiguration()
+		additionalPolicies = configuration.GetManagedPolicies()
+		needsAttach        = make([]string, 0)
+		needsDetach        = make([]string, 0)
+	)
+
+	managedPolicies := ctx.GetManagedPoliciesList(additionalPolicies)
+	attachedPolicies := state.GetAttachedPolicies()
+
+	attachedArns := make([]string, 0)
+	for _, p := range attachedPolicies {
+		attachedArns = append(attachedArns, aws.StringValue(p.PolicyArn))
+	}
+
+	for _, policy := range managedPolicies {
+		if !common.ContainsString(attachedArns, policy) {
+			needsAttach = append(needsAttach, policy)
+		}
+	}
+
+	if len(attachedArns) == 0 {
+		needsAttach = managedPolicies
+	}
+
+	for _, policy := range attachedArns {
+		if !common.ContainsString(managedPolicies, policy) {
+			needsDetach = append(needsDetach, policy)
+		}
+	}
+
+	err := ctx.AwsWorker.AttachManagedPolicies(roleName, needsAttach)
+	if err != nil {
+		return err
+	}
+
+	err = ctx.AwsWorker.DetachManagedPolicies(roleName, needsDetach)
+	if err != nil {
+		return err
+	}
+
+	ctx.Log.Info("updated managed policies", "instancegroup", instanceGroup.GetName(), "iamrole", roleName)
+	return nil
+}


Test patch:
diff --git a/controllers/provisioners/eks/eks_test.go b/controllers/provisioners/eks/eks_test.go
index f51779d..1ab90dd 100644
--- a/controllers/provisioners/eks/eks_test.go
+++ b/controllers/provisioners/eks/eks_test.go
@@ -154,6 +154,19 @@ func MockCustomResourceDefinition() *unstructured.Unstructured {
 	}
 }
 
+func MockAttachedPolicies(policies ...string) []*iam.AttachedPolicy {
+	mock := []*iam.AttachedPolicy{}
+	for _, p := range policies {
+		arn := fmt.Sprintf("%v/%v", awsprovider.IAMPolicyPrefix, p)
+		policy := &iam.AttachedPolicy{
+			PolicyName: aws.String(p),
+			PolicyArn:  aws.String(arn),
+		}
+		mock = append(mock, policy)
+	}
+	return mock
+}
+
 func MockTagDescription(key, value string) *autoscaling.TagDescription {
 	return &autoscaling.TagDescription{
 		Key:   aws.String(key),
@@ -359,10 +372,30 @@ type MockIamClient struct {
 	AddRoleToInstanceProfileErr       error
 	RemoveRoleFromInstanceProfileErr  error
 	AttachRolePolicyErr               error
+	AttachRolePolicyCallCount         int
 	DetachRolePolicyErr               error
+	DetachRolePolicyCallCount         int
 	WaitUntilInstanceProfileExistsErr error
+	ListAttachedRolePoliciesErr       error
 	Role                              *iam.Role
 	InstanceProfile                   *iam.InstanceProfile
+	AttachedPolicies                  []*iam.AttachedPolicy
+}
+
+func (i *MockIamClient) ListAttachedRolePolicies(input *iam.ListAttachedRolePoliciesInput) (*iam.ListAttachedRolePoliciesOutput, error) {
+	if i.AttachedPolicies != nil {
+		return &iam.ListAttachedRolePoliciesOutput{AttachedPolicies: i.AttachedPolicies}, i.ListAttachedRolePoliciesErr
+	}
+	return &iam.ListAttachedRolePoliciesOutput{}, i.ListAttachedRolePoliciesErr
+}
+
+func (i *MockIamClient) ListAttachedRolePoliciesPages(input *iam.ListAttachedRolePoliciesInput, callback func(*iam.ListAttachedRolePoliciesOutput, bool) bool) error {
+	page, err := i.ListAttachedRolePolicies(input)
+	if err != nil {
+		return err
+	}
+	callback(page, false)
+	return nil
 }
 
 func (i *MockIamClient) CreateRole(input *iam.CreateRoleInput) (*iam.CreateRoleOutput, error) {
@@ -400,10 +433,12 @@ func (i *MockIamClient) RemoveRoleFromInstanceProfile(input *iam.RemoveRoleFromI
 }
 
 func (i *MockIamClient) AttachRolePolicy(input *iam.AttachRolePolicyInput) (*iam.AttachRolePolicyOutput, error) {
+	i.AttachRolePolicyCallCount++
 	return &iam.AttachRolePolicyOutput{}, i.AttachRolePolicyErr
 }
 
 func (i *MockIamClient) DetachRolePolicy(input *iam.DetachRolePolicyInput) (*iam.DetachRolePolicyOutput, error) {
+	i.DetachRolePolicyCallCount++
 	return &iam.DetachRolePolicyOutput{}, i.DetachRolePolicyErr
 }
 
diff --git a/controllers/provisioners/eks/update_test.go b/controllers/provisioners/eks/update_test.go
index aaeb2c3..606b191 100644
--- a/controllers/provisioners/eks/update_test.go
+++ b/controllers/provisioners/eks/update_test.go
@@ -420,3 +420,51 @@ func TestScalingGroupUpdatePredicate(t *testing.T) {
 		g.Expect(got).To(gomega.Equal(tc.expected))
 	}
 }
+
+func TestUpdateManagedPolicies(t *testing.T) {
+	var (
+		g             = gomega.NewGomegaWithT(t)
+		k             = MockKubernetesClientSet()
+		ig            = MockInstanceGroup()
+		configuration = ig.GetEKSConfiguration()
+		asgMock       = NewAutoScalingMocker()
+		iamMock       = NewIamMocker()
+	)
+
+	w := MockAwsWorker(asgMock, iamMock)
+	ctx := MockContext(ig, k, w)
+
+	tests := []struct {
+		attachedPolicies   []*iam.AttachedPolicy
+		additionalPolicies []string
+		expectedAttached   int
+		expectedDetached   int
+	}{
+		// default policies attached, no changes needed
+		{attachedPolicies: MockAttachedPolicies(DefaultManagedPolicies...), additionalPolicies: []string{}, expectedAttached: 0, expectedDetached: 0},
+		// default policies not attached
+		{attachedPolicies: MockAttachedPolicies(), additionalPolicies: []string{}, expectedAttached: 3, expectedDetached: 0},
+		// additional policies need to be attached
+		{attachedPolicies: MockAttachedPolicies(DefaultManagedPolicies...), additionalPolicies: []string{"policy-1", "policy-2"}, expectedAttached: 2, expectedDetached: 0},
+		// additional policies need to be detached
+		{attachedPolicies: MockAttachedPolicies("AmazonEKSWorkerNodePolicy", "AmazonEKS_CNI_Policy", "AmazonEC2ContainerRegistryReadOnly", "policy-1"), additionalPolicies: []string{}, expectedAttached: 0, expectedDetached: 1},
+		// additional policies need to be attached & detached
+		{attachedPolicies: MockAttachedPolicies("AmazonEKSWorkerNodePolicy", "AmazonEKS_CNI_Policy", "AmazonEC2ContainerRegistryReadOnly", "policy-1"), additionalPolicies: []string{"policy-2"}, expectedAttached: 1, expectedDetached: 1},
+	}
+
+	for _, tc := range tests {
+		iamMock.AttachRolePolicyCallCount = 0
+		iamMock.DetachRolePolicyCallCount = 0
+		ctx.SetDiscoveredState(&DiscoveredState{
+			Publisher: kubeprovider.EventPublisher{
+				Client: k.Kubernetes,
+			},
+			AttachedPolicies: tc.attachedPolicies,
+		})
+		configuration.SetManagedPolicies(tc.additionalPolicies)
+		err := ctx.UpdateManagedPolicies("some-role")
+		g.Expect(err).NotTo(gomega.HaveOccurred())
+		g.Expect(iamMock.AttachRolePolicyCallCount).To(gomega.Equal(tc.expectedAttached))
+		g.Expect(iamMock.DetachRolePolicyCallCount).To(gomega.Equal(tc.expectedDetached))
+	}
+}