Repository: keikoproj/instance-manager (go)

Issue:
Possible bootstrapping issues
When two instance groups are using provided roles which are the same, when once instance-group gets deleted it will remove the role from aws-auth, leading to nodes going to NotReady.

We should emphasize on docs that every instance group should have it's own role.
Alternatively we can get all roles of IGs on the cluster and validate them whenever we call bootstrap, this would re-add a role if it's removed by a deleting IG

In addition, when nodes are NotReady it cannot reach `BootstrapNodes()` code, so if the reason for nodes not being ready is bootstrapping, they will be stuck forever.

We should call `BootstrapNodes` in update flow, before `UpdateNodeReadyCondition`.

Pull request description:
bootstrap improvements
Fixes #101 

- add locking around bootstrap, and call bootstrap on updates before we wait for nodes
- make sure not to remove auth role if it's shared between instancegroups

- [x] BDD test

Patch:
diff --git a/controllers/providers/kubernetes/utils.go b/controllers/providers/kubernetes/utils.go
index 9f7bb84..5865707 100644
--- a/controllers/providers/kubernetes/utils.go
+++ b/controllers/providers/kubernetes/utils.go
@@ -24,10 +24,12 @@ import (
 	"strings"
 
 	"github.com/ghodss/yaml"
+	"github.com/keikoproj/instance-manager/api/v1alpha1"
 	"github.com/keikoproj/instance-manager/controllers/common"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/kubernetes"
@@ -40,6 +42,16 @@ type KubernetesClientSet struct {
 	KubeDynamic dynamic.Interface
 }
 
+func GetUnstructuredInstanceGroup(instanceGroup *v1alpha1.InstanceGroup) (*unstructured.Unstructured, error) {
+	var obj = &unstructured.Unstructured{}
+	content, err := runtime.DefaultUnstructuredConverter.ToUnstructured(instanceGroup)
+	if err != nil {
+		return obj, err
+	}
+	obj.Object = content
+	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 {
diff --git a/controllers/provisioners/eks/create.go b/controllers/provisioners/eks/create.go
index dc0a8f2..6443745 100644
--- a/controllers/provisioners/eks/create.go
+++ b/controllers/provisioners/eks/create.go
@@ -58,7 +58,7 @@ func (ctx *EksInstanceGroupContext) Create() error {
 	return nil
 }
 
-func (ctx EksInstanceGroupContext) CreateScalingGroup() error {
+func (ctx *EksInstanceGroupContext) CreateScalingGroup() error {
 	var (
 		instanceGroup = ctx.GetInstanceGroup()
 		spec          = instanceGroup.GetEKSSpec()
diff --git a/controllers/provisioners/eks/delete.go b/controllers/provisioners/eks/delete.go
index dc33be7..72c1f03 100644
--- a/controllers/provisioners/eks/delete.go
+++ b/controllers/provisioners/eks/delete.go
@@ -17,7 +17,6 @@ package eks
 
 import (
 	"github.com/keikoproj/instance-manager/api/v1alpha1"
-	"github.com/keikoproj/instance-manager/controllers/common"
 	"github.com/pkg/errors"
 
 	"github.com/aws/aws-sdk-go/aws"
@@ -38,8 +37,10 @@ func (ctx *EksInstanceGroupContext) Delete() error {
 		return errors.Wrap(err, "failed to delete scaling group")
 	}
 
-	// if scaling group is deleted, defer removal from aws-auth
-	defer common.RemoveAuthConfigMap(ctx.KubernetesClient.Kubernetes, []string{roleARN})
+	// if scaling group is deleted, remove the role from aws-auth if it's not in use by other groups
+	if err := ctx.RemoveAuthRole(roleARN); err != nil {
+		return errors.Wrap(err, "failed to remove auth role")
+	}
 
 	// delete launchconfig
 	err = ctx.DeleteLaunchConfiguration()
diff --git a/controllers/provisioners/eks/eks.go b/controllers/provisioners/eks/eks.go
index 4727a24..4d1470d 100644
--- a/controllers/provisioners/eks/eks.go
+++ b/controllers/provisioners/eks/eks.go
@@ -16,6 +16,8 @@ limitations under the License.
 package eks
 
 import (
+	"sync"
+
 	"github.com/go-logr/logr"
 	"github.com/keikoproj/instance-manager/api/v1alpha1"
 	awsprovider "github.com/keikoproj/instance-manager/controllers/providers/aws"
@@ -71,6 +73,7 @@ type EksDefaultConfiguration struct {
 }
 
 type EksInstanceGroupContext struct {
+	sync.Mutex
 	InstanceGroup    *v1alpha1.InstanceGroup
 	KubernetesClient kubeprovider.KubernetesClientSet
 	AwsWorker        awsprovider.AwsWorker
diff --git a/controllers/provisioners/eks/helpers.go b/controllers/provisioners/eks/helpers.go
index 7402601..e0c0dfb 100644
--- a/controllers/provisioners/eks/helpers.go
+++ b/controllers/provisioners/eks/helpers.go
@@ -24,10 +24,13 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/autoscaling"
 	"github.com/keikoproj/instance-manager/api/v1alpha1"
+	"github.com/keikoproj/instance-manager/controllers/common"
 	awsprovider "github.com/keikoproj/instance-manager/controllers/providers/aws"
 	kubeprovider "github.com/keikoproj/instance-manager/controllers/providers/kubernetes"
 	"github.com/keikoproj/instance-manager/controllers/provisioners"
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
 
 func (ctx *EksInstanceGroupContext) GetLaunchConfigurationInput(name string) *autoscaling.CreateLaunchConfigurationInput {
@@ -329,3 +332,39 @@ func (ctx *EksInstanceGroupContext) GetManagedPoliciesList(additionalPolicies []
 	}
 	return managedPolicies
 }
+
+func (ctx *EksInstanceGroupContext) RemoveAuthRole(arn string) error {
+	ctx.Lock()
+	defer ctx.Unlock()
+
+	var instanceGroup = ctx.GetInstanceGroup()
+	var list = &unstructured.UnstructuredList{}
+	var sharedGroups = make([]string, 0)
+
+	list, err := ctx.KubernetesClient.KubeDynamic.Resource(v1alpha1.GroupVersionResource).List(metav1.ListOptions{})
+	if err != nil {
+		return err
+	}
+
+	// find objects which share the same nodesInstanceRoleArn
+	for _, obj := range list.Items {
+		if val, ok, _ := unstructured.NestedString(obj.Object, "status", "nodesInstanceRoleArn"); ok {
+			if strings.EqualFold(arn, val) {
+				sharedGroups = append(sharedGroups, obj.GetName())
+			}
+		}
+	}
+
+	// If there are other instance groups using the same role we should not remove it from aws-auth
+	if len(sharedGroups) > 1 {
+		ctx.Log.Info(
+			"skipping removal of auth role, is used by another instancegroup",
+			"instancegroup", instanceGroup.GetName(),
+			"arn", arn,
+			"conflict", strings.Join(sharedGroups, ","),
+		)
+		return nil
+	}
+
+	return common.RemoveAuthConfigMap(ctx.KubernetesClient.Kubernetes, []string{arn})
+}
diff --git a/controllers/provisioners/eks/update.go b/controllers/provisioners/eks/update.go
index 65712ac..d0a9801 100644
--- a/controllers/provisioners/eks/update.go
+++ b/controllers/provisioners/eks/update.go
@@ -63,6 +63,13 @@ func (ctx *EksInstanceGroupContext) Update() error {
 		return errors.Wrap(err, "failed to update scaling group")
 	}
 
+	// 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 {
+		ctx.Log.Error(err, "failed to bootstrap role", "instancegroup", instanceGroup.GetName())
+	}
+
 	// update readiness conditions
 	nodesReady := ctx.UpdateNodeReadyCondition()
 	if nodesReady {
diff --git a/controllers/provisioners/eks/upgrade.go b/controllers/provisioners/eks/upgrade.go
index 125c175..8e83ebf 100644
--- a/controllers/provisioners/eks/upgrade.go
+++ b/controllers/provisioners/eks/upgrade.go
@@ -74,10 +74,16 @@ func (ctx *EksInstanceGroupContext) UpgradeNodes() error {
 
 func (ctx *EksInstanceGroupContext) BootstrapNodes() error {
 	var (
-		state   = ctx.GetDiscoveredState()
-		role    = state.GetRole()
-		roleARN = aws.StringValue(role.Arn)
+		state         = ctx.GetDiscoveredState()
+		instanceGroup = ctx.GetInstanceGroup()
+		role          = state.GetRole()
+		roleARN       = aws.StringValue(role.Arn)
 	)
+	ctx.Log.Info("bootstrapping arn to aws-auth", "instancegroup", instanceGroup.GetName(), "arn", roleARN)
+
+	// lock to guarantee Upsert and Remove cannot conflict when roles are shared between instancegroups
+	ctx.Lock()
+	defer ctx.Unlock()
 
 	return common.UpsertAuthConfigMap(ctx.KubernetesClient.Kubernetes, []string{roleARN})
 }


Test patch:
diff --git a/controllers/provisioners/eks/delete_test.go b/controllers/provisioners/eks/delete_test.go
index 5927105..bb3c3eb 100644
--- a/controllers/provisioners/eks/delete_test.go
+++ b/controllers/provisioners/eks/delete_test.go
@@ -18,12 +18,16 @@ package eks
 import (
 	"testing"
 
+	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/awserr"
 	"github.com/aws/aws-sdk-go/service/autoscaling"
 	"github.com/aws/aws-sdk-go/service/iam"
+	awsauth "github.com/keikoproj/aws-auth/pkg/mapper"
 	"github.com/keikoproj/instance-manager/api/v1alpha1"
+	kubeprovider "github.com/keikoproj/instance-manager/controllers/providers/kubernetes"
 	"github.com/onsi/gomega"
 	"github.com/pkg/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 func TestDeletePositive(t *testing.T) {
@@ -122,5 +126,71 @@ func TestDeleteAutoScalingGroupNegative(t *testing.T) {
 	g.Expect(err).To(gomega.HaveOccurred())
 	g.Expect(ctx.GetState()).To(gomega.Equal(v1alpha1.ReconcileDeleting))
 	asgMock.DeleteAutoScalingGroupErr = nil
+}
+
+func TestRemoveAuthRoleNegative(t *testing.T) {
+	var (
+		g       = gomega.NewGomegaWithT(t)
+		k       = MockKubernetesClientSet()
+		ig      = MockInstanceGroup()
+		ig2     = MockInstanceGroup()
+		asgMock = NewAutoScalingMocker()
+		iamMock = NewIamMocker()
+	)
+
+	w := MockAwsWorker(asgMock, iamMock)
+	ctx := MockContext(ig, k, w)
+
+	// two instancegroups with same role arn
+	ig.Status.NodesArn = "same-role"
+	ig2.Name = "instance-group-2"
+	ig2.Namespace = "different-namespace"
+	ig2.Status.NodesArn = "same-role"
+
+	igObj, err := kubeprovider.GetUnstructuredInstanceGroup(ig)
+	g.Expect(err).NotTo(gomega.HaveOccurred())
+	ig2Obj, err := kubeprovider.GetUnstructuredInstanceGroup(ig2)
+	g.Expect(err).NotTo(gomega.HaveOccurred())
+
+	_, err = ctx.KubernetesClient.KubeDynamic.Resource(v1alpha1.GroupVersionResource).Namespace("instance-manager").Create(igObj, metav1.CreateOptions{})
+	g.Expect(err).NotTo(gomega.HaveOccurred())
+
+	_, err = ctx.KubernetesClient.KubeDynamic.Resource(v1alpha1.GroupVersionResource).Namespace(ig2.Namespace).Create(ig2Obj, metav1.CreateOptions{})
+	g.Expect(err).NotTo(gomega.HaveOccurred())
 
+	ctx.SetDiscoveredState(&DiscoveredState{
+		IAMRole: &iam.Role{
+			Arn: aws.String("same-role"),
+		},
+		ScalingGroup: &autoscaling.Group{},
+	})
+
+	ctx.BootstrapNodes()
+
+	// Only one role is added to aws-auth
+	auth, _, err := awsauth.ReadAuthMap(k.Kubernetes)
+	g.Expect(err).NotTo(gomega.HaveOccurred())
+	g.Expect(len(auth.MapRoles)).To(gomega.Equal(1))
+
+	// after delete, the role should not be deleted
+	err = ctx.Delete()
+	g.Expect(err).NotTo(gomega.HaveOccurred())
+	g.Expect(ctx.GetState()).To(gomega.Equal(v1alpha1.ReconcileDeleting))
+
+	auth, _, err = awsauth.ReadAuthMap(k.Kubernetes)
+	g.Expect(err).NotTo(gomega.HaveOccurred())
+	g.Expect(len(auth.MapRoles)).To(gomega.Equal(1))
+
+	// this time the role should be successfully removed
+	err = ctx.KubernetesClient.KubeDynamic.Resource(v1alpha1.GroupVersionResource).Namespace(ig2.Namespace).Delete(ig2.Name, &metav1.DeleteOptions{})
+	g.Expect(err).NotTo(gomega.HaveOccurred())
+
+	err = ctx.Delete()
+	g.Expect(err).NotTo(gomega.HaveOccurred())
+	g.Expect(ctx.GetState()).To(gomega.Equal(v1alpha1.ReconcileDeleting))
+
+	auth, _, err = awsauth.ReadAuthMap(k.Kubernetes)
+	t.Log(auth)
+	g.Expect(err).NotTo(gomega.HaveOccurred())
+	g.Expect(len(auth.MapRoles)).To(gomega.Equal(0))
 }