Repository: keikoproj/instance-manager (go)

Issue:
Remove AmazonEKS_CNI_Policy from DefaultManagedPolicies
AmazonEKS_CNI_Policy is currently part of DefaultManagedPolicies, which are automatically added to the roles created and associated with nodes provisioned by the EKS provisioner.

In the AWS docs, it is recommended to remove it and use IAM Roles for Service Accounts instead. 
We should remove it from the defaults or offer a way for users to opt-out of attaching it. 

Pull request:
feat: annotation for IRSA enabled nodes
Fixes #258 
Signed-off-by: Eytan Avisror <eytan_avisror@intuit.com>

Patch:
diff --git a/controllers/provisioners/eks/eks.go b/controllers/provisioners/eks/eks.go
index 233f66b..98917b5 100644
--- a/controllers/provisioners/eks/eks.go
+++ b/controllers/provisioners/eks/eks.go
@@ -32,7 +32,8 @@ import (
 const (
 	ProvisionerName                     = "eks"
 	defaultLaunchConfigurationRetention = 2
-	OverrideDefaultLabelsAnnotationKey  = "instancemgr.keikoproj.io/default-labels"
+	OverrideDefaultLabelsAnnotation     = "instancemgr.keikoproj.io/default-labels"
+	IRSAEnabledAnnotation               = "instancemgr.keikoproj.io/irsa-enabled"
 	OsFamilyAnnotation                  = "instancemgr.keikoproj.io/os-family"
 	ClusterAutoscalerEnabledAnnotation  = "instancemgr.keikoproj.io/cluster-autoscaler-enabled"
 	CustomNetworkingEnabledAnnotation   = "instancemgr.keikoproj.io/custom-networking-enabled"
@@ -51,7 +52,8 @@ var (
 	InstanceMgrLifecycleLabel = "instancemgr.keikoproj.io/lifecycle"
 	InstanceMgrLabelFmt       = "instancemgr.keikoproj.io/%s=%s"
 
-	DefaultManagedPolicies = []string{"AmazonEKSWorkerNodePolicy", "AmazonEKS_CNI_Policy", "AmazonEC2ContainerRegistryReadOnly"}
+	DefaultManagedPolicies = []string{"AmazonEKSWorkerNodePolicy", "AmazonEC2ContainerRegistryReadOnly"}
+	CNIManagedPolicy       = "AmazonEKS_CNI_Policy"
 )
 
 // New constructs a new instance group provisioner of EKS type
diff --git a/controllers/provisioners/eks/helpers.go b/controllers/provisioners/eks/helpers.go
index d2f61a3..aea37bf 100644
--- a/controllers/provisioners/eks/helpers.go
+++ b/controllers/provisioners/eks/helpers.go
@@ -400,7 +400,7 @@ func (ctx *EksInstanceGroupContext) GetComputedLabels() map[string]string {
 	}
 
 	// allow override default labels
-	if val, ok := annotations[OverrideDefaultLabelsAnnotationKey]; ok {
+	if val, ok := annotations[OverrideDefaultLabelsAnnotation]; ok {
 		isOverride = true
 		overrideLabels := strings.Split(val, ",")
 		for _, label := range overrideLabels {
@@ -883,6 +883,11 @@ func (ctx *EksInstanceGroupContext) UpdateLifecycleHooks(asgName string) error {
 }
 
 func (ctx *EksInstanceGroupContext) GetManagedPoliciesList(additionalPolicies []string) []string {
+	var (
+		instanceGroup = ctx.GetInstanceGroup()
+		annotations   = instanceGroup.GetAnnotations()
+	)
+
 	managedPolicies := make([]string, 0)
 	for _, name := range additionalPolicies {
 		switch {
@@ -895,10 +900,21 @@ func (ctx *EksInstanceGroupContext) GetManagedPoliciesList(additionalPolicies []
 		}
 	}
 
+	var irsaEnabled bool
+	if val, ok := annotations[IRSAEnabledAnnotation]; ok {
+		if strings.EqualFold(val, "true") {
+			irsaEnabled = true
+		}
+	}
+
 	for _, name := range DefaultManagedPolicies {
 		managedPolicies = append(managedPolicies, fmt.Sprintf("%s/%s", awsprovider.IAMPolicyPrefix, name))
 	}
 
+	if !irsaEnabled {
+		managedPolicies = append(managedPolicies, fmt.Sprintf("%s/%s", awsprovider.IAMPolicyPrefix, CNIManagedPolicy))
+	}
+
 	return managedPolicies
 }
 
diff --git a/docs/EKS.md b/docs/EKS.md
index 1a9ac2b..1279270 100644
--- a/docs/EKS.md
+++ b/docs/EKS.md
@@ -551,6 +551,7 @@ Individual namespaces can opt-out by adding the annotation `instancemgr.keikopro
 |:--------------:|:------:|:----------------:|:-------:|
 |instancemgr.keikoproj.io/config-excluded|Namespace|"true"|settings this annotation on a namespace will allow opt-out from a configuration configmap, all instancegroups under such namespace will not use configmap boundaries and default values|
 |instancemgr.keikoproj.io/cluster-autoscaler-enabled|InstanceGroup|"true"|setting this annotation to true will add the relevant cluster-autoscaler EC2 tags according to cluster name, taints, and labels|
+|instancemgr.keikoproj.io/irsa-enabled|InstanceGroup|"true"|setting this annotation to true will remove the AmazonEKS_CNI_Policy from the default managed policies attached to the node role, this should only be used when nodes are using IAM Roles for Service Accounts (IRSA) and the aws-node daemonset is using an IRSA role which contains this policy|
 |instancemgr.keikoproj.io/os-family|InstanceGroup|either "windows", "bottlerocket", or "amazonlinux2" (default)|this is required if you are running a windows or bottlerocket based AMI, by default the controller will try to bootstrap an amazonlinux2 AMI|
 |instancemgr.keikoproj.io/default-labels|InstanceGroup|comma-seprarated key-value string e.g. "label1=value1,label2=value2"|allows overriding the default node labels added by the controller, by default the role label is added depending on the cluster version|
 |instancemgr.keikoproj.io/custom-networking-enabled|InstanceGroup|"true"|setting this annotation to true will automatically calculate the correct setting for max pods and pass it to the kubelet|


Test patch:
diff --git a/controllers/provisioners/eks/helpers_test.go b/controllers/provisioners/eks/helpers_test.go
index f38c6c6..36de578 100644
--- a/controllers/provisioners/eks/helpers_test.go
+++ b/controllers/provisioners/eks/helpers_test.go
@@ -451,7 +451,7 @@ func TestGetLabelList(t *testing.T) {
 		expectedLabels116          = []string{defaultLifecycleLabel, "node.kubernetes.io/role=instance-group-1"}
 		expectedLabelsWithCustom   = []string{defaultLifecycleLabel, "custom.kubernetes.io=customlabel", "node.kubernetes.io/role=instance-group-1"}
 		expectedLabelsWithOverride = []string{defaultLifecycleLabel, "custom.kubernetes.io=customlabel", "override.kubernetes.io=instance-group-1", "override2.kubernetes.io=instance-group-1"}
-		overrideAnnotation         = map[string]string{OverrideDefaultLabelsAnnotationKey: "override.kubernetes.io=instance-group-1,override2.kubernetes.io=instance-group-1"}
+		overrideAnnotation         = map[string]string{OverrideDefaultLabelsAnnotation: "override.kubernetes.io=instance-group-1,override2.kubernetes.io=instance-group-1"}
 		expectedSpotLabel          = []string{"instancemgr.keikoproj.io/lifecycle=spot", "node-role.kubernetes.io/instance-group-1=\"\"", "node.kubernetes.io/role=instance-group-1"}
 		expectedMixedLabel         = []string{"instancemgr.keikoproj.io/lifecycle=mixed", "node-role.kubernetes.io/instance-group-1=\"\"", "node.kubernetes.io/role=instance-group-1"}
 	)
diff --git a/controllers/provisioners/eks/update_test.go b/controllers/provisioners/eks/update_test.go
index b2c3c13..64dc808 100644
--- a/controllers/provisioners/eks/update_test.go
+++ b/controllers/provisioners/eks/update_test.go
@@ -623,6 +623,9 @@ func TestUpdateManagedPolicies(t *testing.T) {
 		ec2Mock       = NewEc2Mocker()
 	)
 
+	defaultPolicies := []string{"AmazonEKSWorkerNodePolicy", "AmazonEKS_CNI_Policy", "AmazonEC2ContainerRegistryReadOnly"}
+	defaultPoliciesIrsa := []string{"AmazonEKSWorkerNodePolicy", "AmazonEC2ContainerRegistryReadOnly"}
+
 	w := MockAwsWorker(asgMock, iamMock, eksMock, ec2Mock)
 	ctx := MockContext(ig, k, w)
 
@@ -631,24 +634,37 @@ func TestUpdateManagedPolicies(t *testing.T) {
 		additionalPolicies []string
 		expectedAttached   int
 		expectedDetached   int
+		irsaEnabled        bool
 	}{
 		// default policies attached, no changes needed
-		{attachedPolicies: MockAttachedPolicies(DefaultManagedPolicies...), additionalPolicies: []string{}, expectedAttached: 0, expectedDetached: 0},
+		{attachedPolicies: MockAttachedPolicies(defaultPolicies...), additionalPolicies: []string{}, expectedAttached: 0, expectedDetached: 0},
+		// when IRSA is enabled, cni policy needs to be detached
+		{attachedPolicies: MockAttachedPolicies(defaultPolicies...), additionalPolicies: []string{}, irsaEnabled: true, expectedAttached: 0, expectedDetached: 1},
+		// when IRSA is disabled, cni policy needs to be attached
+		{attachedPolicies: MockAttachedPolicies(defaultPoliciesIrsa...), additionalPolicies: []string{}, irsaEnabled: false, expectedAttached: 1, 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},
+		{attachedPolicies: MockAttachedPolicies(defaultPolicies...), additionalPolicies: []string{"policy-1", "policy-2"}, expectedAttached: 2, expectedDetached: 0},
 		// additional policies with ARN
-		{attachedPolicies: MockAttachedPolicies(DefaultManagedPolicies...), additionalPolicies: []string{"arn:aws:iam::aws:policy/policy-1", "arn:aws:iam::12345679012:policy/policy-2"}, expectedAttached: 2, expectedDetached: 0},
+		{attachedPolicies: MockAttachedPolicies(defaultPolicies...), additionalPolicies: []string{"arn:aws:iam::aws:policy/policy-1", "arn:aws:iam::12345679012:policy/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 {
+	for i, tc := range tests {
+		t.Logf("test #%v", i)
 		iamMock.AttachRolePolicyCallCount = 0
 		iamMock.DetachRolePolicyCallCount = 0
+
+		if tc.irsaEnabled {
+			ig.Annotations[IRSAEnabledAnnotation] = "true"
+		} else {
+			ig.Annotations[IRSAEnabledAnnotation] = "false"
+		}
+
 		ctx.SetDiscoveredState(&DiscoveredState{
 			Publisher: kubeprovider.EventPublisher{
 				Client: k.Kubernetes,