Repository: keikoproj/instance-manager
Pull request #186 (2020-11-05 19:28:58)

Initial basic support for osFamily label and Windows nodes
This PR adds support for a new label - `instancemgr.keikoproj.io/os-family` - which indicates that the targeted os needs to be handled differently than how the default behavior. This label is initially being used to differentiate between bootstrap and aws-auth behavior for Windows nodes, but could also be used for managing bootstrap/aws-auth differences for BottleRocket or other image types in the future.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/keikoproj/instance-manager/186)
<!-- Reviewable:end -->


eytan-avisror: @backjo LG 🥇 
Can you just also add some unit tests? you can use `make coverage` to generate a coverage report
codecov[bot]: # [Codecov](https://codecov.io/gh/keikoproj/instance-manager/pull/186?src=pr&el=h1) Report
> Merging [#186](https://codecov.io/gh/keikoproj/instance-manager/pull/186?src=pr&el=desc) into [master](https://codecov.io/gh/keikoproj/instance-manager/commit/ff9417b5e422d0c74fd5c75e9eb481adfb348189?el=desc) will **increase** coverage by `0.16%`.
> The diff coverage is `100.00%`.

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

```diff
@@            Coverage Diff             @@
##           master     #186      +/-   ##
==========================================
+ Coverage   89.10%   89.26%   +0.16%     
==========================================
  Files          15       15              
  Lines        1624     1649      +25     
==========================================
+ Hits         1447     1472      +25     
  Misses        109      109              
  Partials       68       68              
```


| [Impacted Files](https://codecov.io/gh/keikoproj/instance-manager/pull/186?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [controllers/provisioners/eks/create.go](https://codecov.io/gh/keikoproj/instance-manager/pull/186/diff?src=pr&el=tree#diff-Y29udHJvbGxlcnMvcHJvdmlzaW9uZXJzL2Vrcy9jcmVhdGUuZ28=) | `93.33% <100.00%> (+0.07%)` | :arrow_up: |
| [controllers/provisioners/eks/eks.go](https://codecov.io/gh/keikoproj/instance-manager/pull/186/diff?src=pr&el=tree#diff-Y29udHJvbGxlcnMvcHJvdmlzaW9uZXJzL2Vrcy9la3MuZ28=) | `91.89% <100.00%> (+1.89%)` | :arrow_up: |
| [controllers/provisioners/eks/helpers.go](https://codecov.io/gh/keikoproj/instance-manager/pull/186/diff?src=pr&el=tree#diff-Y29udHJvbGxlcnMvcHJvdmlzaW9uZXJzL2Vrcy9oZWxwZXJzLmdv) | `88.98% <100.00%> (+0.36%)` | :arrow_up: |
| [controllers/provisioners/eks/update.go](https://codecov.io/gh/keikoproj/instance-manager/pull/186/diff?src=pr&el=tree#diff-Y29udHJvbGxlcnMvcHJvdmlzaW9uZXJzL2Vrcy91cGRhdGUuZ28=) | `91.87% <100.00%> (+0.05%)` | :arrow_up: |
| [controllers/provisioners/eks/upgrade.go](https://codecov.io/gh/keikoproj/instance-manager/pull/186/diff?src=pr&el=tree#diff-Y29udHJvbGxlcnMvcHJvdmlzaW9uZXJzL2Vrcy91cGdyYWRlLmdv) | `94.59% <100.00%> (+0.07%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/keikoproj/instance-manager/pull/186?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/186?src=pr&el=footer). Last update [ff9417b...a6fd916](https://codecov.io/gh/keikoproj/instance-manager/pull/186?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

eytan-avisror: @backjo This seems mergeable to me.
I think we only need to remove the defer panic unless we think it's needed and also remove the empty file `test-bdd/templates/instance-group-windows.yaml` unless you want to add a functional test as well? This might be painful because as of now in order to run the test you need a cluster with both upgrade-manager & minion-manager. If you want we can add the BDD later - I intend to make the functional test automated per PR once we move to Github Actions
backjo: @eytan-avisror sounds good - I went ahead and removed the BDD. I've got a cluster with most of the right stuff on it, which is how I've been testing, but it needs a bit more work. 

Patch:
diff --git a/controllers/common/bootstrap.go b/controllers/common/bootstrap.go
index 5fc5978..34c7de5 100644
--- a/controllers/common/bootstrap.go
+++ b/controllers/common/bootstrap.go
@@ -16,21 +16,34 @@ limitations under the License.
 package common
 
 import (
+	"strings"
 	"time"
 
 	awsauth "github.com/keikoproj/aws-auth/pkg/mapper"
 	"k8s.io/client-go/kubernetes"
 )
 
-func GetNodeBootstrapUpsert(arn string) *awsauth.MapperArguments {
+func GetGroupsForOsFamily(osFamily string) []string {
+	if strings.EqualFold(osFamily, "windows") {
+		return []string{
+			"system:bootstrappers",
+			"system:nodes",
+			"eks:kube-proxy-windows",
+		}
+	} else {
+		return []string{
+			"system:bootstrappers",
+			"system:nodes",
+		}
+	}
+}
+
+func GetNodeBootstrapUpsert(arn string, osFamily string) *awsauth.MapperArguments {
 	return &awsauth.MapperArguments{
 		MapRoles: true,
 		RoleARN:  arn,
 		Username: "system:node:{{EC2PrivateDNSName}}",
-		Groups: []string{
-			"system:bootstrappers",
-			"system:nodes",
-		},
+		Groups: GetGroupsForOsFamily(osFamily),
 		WithRetries:   true,
 		MinRetryTime:  time.Millisecond * 100,
 		MaxRetryTime:  time.Second * 30,
@@ -38,15 +51,12 @@ func GetNodeBootstrapUpsert(arn string) *awsauth.MapperArguments {
 	}
 }
 
-func GetNodeBootstrapRemove(arn string) *awsauth.MapperArguments {
+func GetNodeBootstrapRemove(arn string, osFamily string) *awsauth.MapperArguments {
 	return &awsauth.MapperArguments{
 		MapRoles: true,
 		RoleARN:  arn,
 		Username: "system:node:{{EC2PrivateDNSName}}",
-		Groups: []string{
-			"system:bootstrappers",
-			"system:nodes",
-		},
+		Groups: GetGroupsForOsFamily(osFamily),
 		WithRetries:   true,
 		MinRetryTime:  time.Millisecond * 100,
 		MaxRetryTime:  time.Second * 30,
@@ -54,13 +64,13 @@ func GetNodeBootstrapRemove(arn string) *awsauth.MapperArguments {
 	}
 }
 
-func RemoveAuthConfigMap(kube kubernetes.Interface, arns []string) error {
+func RemoveAuthConfigMap(kube kubernetes.Interface, arns []string, osFamilies []string) error {
 	authMap := awsauth.New(kube, false)
-	for _, arn := range arns {
+	for index, arn := range arns {
 		if arn == "" {
 			continue
 		}
-		err := authMap.Remove(GetNodeBootstrapRemove(arn))
+		err := authMap.Remove(GetNodeBootstrapRemove(arn, osFamilies[index]))
 		if err != nil {
 			return err
 		}
@@ -68,13 +78,13 @@ func RemoveAuthConfigMap(kube kubernetes.Interface, arns []string) error {
 	return nil
 }
 
-func UpsertAuthConfigMap(kube kubernetes.Interface, arns []string) error {
+func UpsertAuthConfigMap(kube kubernetes.Interface, arns []string, osFamilies []string) error {
 	authMap := awsauth.New(kube, false)
-	for _, arn := range arns {
+	for index, arn := range arns {
 		if arn == "" {
 			continue
 		}
-		err := authMap.Upsert(GetNodeBootstrapUpsert(arn))
+		err := authMap.Upsert(GetNodeBootstrapUpsert(arn, osFamilies[index]))
 		if err != nil {
 			return err
 		}
diff --git a/controllers/provisioners/eks/create.go b/controllers/provisioners/eks/create.go
index 87f4ccb..e1d8d21 100644
--- a/controllers/provisioners/eks/create.go
+++ b/controllers/provisioners/eks/create.go
@@ -37,11 +37,12 @@ func (ctx *EksInstanceGroupContext) Create() error {
 		scalingConfig = state.GetScalingConfiguration()
 		configuration = instanceGroup.GetEKSConfiguration()
 		args          = ctx.GetBootstrapArgs()
+		kubeletArgs   = ctx.GetKubeletExtraArgs()
 
 		userDataPayload = ctx.GetUserDataStages()
 		clusterName     = configuration.GetClusterName()
 		mounts          = ctx.GetMountOpts()
-		userData        = ctx.GetBasicUserData(clusterName, args, userDataPayload, mounts)
+		userData        = ctx.GetBasicUserData(clusterName, args, kubeletArgs, userDataPayload, mounts)
 		sgs             = ctx.ResolveSecurityGroups()
 		spotPrice       = configuration.GetSpotPrice()
 	)
diff --git a/controllers/provisioners/eks/eks.go b/controllers/provisioners/eks/eks.go
index 6b189cc..df96adb 100644
--- a/controllers/provisioners/eks/eks.go
+++ b/controllers/provisioners/eks/eks.go
@@ -31,7 +31,10 @@ const (
 	ProvisionerName                     = "eks"
 	defaultLaunchConfigurationRetention = 2
 	OverrideDefaultLabelsAnnotationKey  = "instancemgr.keikoproj.io/default-labels"
+	OsFamilyAnnotation                  = "instancemgr.keikoproj.io/os-family"
 	ClusterAutoscalerEnabledAnnotation  = "instancemgr.keikoproj.io/cluster-autoscaler-enabled"
+
+	OsFamilyWindows                     = "windows"
 )
 
 var (
@@ -95,6 +98,7 @@ type MountOpts struct {
 
 type EKSUserData struct {
 	ClusterName   string
+	KubeletExtraArgs string
 	Arguments     string
 	PreBootstrap  []string
 	PostBootstrap []string
@@ -108,6 +112,18 @@ func (ctx *EksInstanceGroupContext) GetInstanceGroup() *v1alpha1.InstanceGroup {
 	return &v1alpha1.InstanceGroup{}
 }
 
+func (ctx *EksInstanceGroupContext) GetOsFamily() string {
+	var (
+		instanceGroup  = ctx.GetInstanceGroup()
+		annotations = instanceGroup.GetAnnotations()
+	)
+	if _, exists := annotations[OsFamilyAnnotation]; exists {
+		return annotations[OsFamilyAnnotation]
+	}
+	return "default"  // return "" is will also be fine here
+
+}
+
 func (ctx *EksInstanceGroupContext) GetUpgradeStrategy() *v1alpha1.AwsUpgradeStrategy {
 	if &ctx.InstanceGroup.Spec.AwsUpgradeStrategy != nil {
 		return &ctx.InstanceGroup.Spec.AwsUpgradeStrategy
diff --git a/controllers/provisioners/eks/helpers.go b/controllers/provisioners/eks/helpers.go
index 0a4840c..ab323f9 100644
--- a/controllers/provisioners/eks/helpers.go
+++ b/controllers/provisioners/eks/helpers.go
@@ -96,9 +96,20 @@ func (ctx *EksInstanceGroupContext) ResolveSecurityGroups() []string {
 	return resolved
 }
 
-func (ctx *EksInstanceGroupContext) GetBasicUserData(clusterName, args string, payload UserDataPayload, mounts []MountOpts) string {
-
-	var UserDataTemplate = `#!/bin/bash
+func (ctx *EksInstanceGroupContext) GetBasicUserData(clusterName, args string, kubeletExtraArgs string, payload UserDataPayload, mounts []MountOpts) string {
+      osFamily := ctx.GetOsFamily()
+	var UserDataTemplate string
+	if strings.EqualFold(osFamily, OsFamilyWindows) {
+		UserDataTemplate = `
+<powershell>
+  [string]$EKSBinDir = "$env:ProgramFiles\Amazon\EKS"
+  [string]$EKSBootstrapScriptName = 'Start-EKSBootstrap.ps1'
+  [string]$EKSBootstrapScriptFile = "$EKSBinDir\$EKSBootstrapScriptName"
+  & $EKSBootstrapScriptFile -EKSClusterName {{ .ClusterName }} -KubeletExtraArgs '{{ .KubeletExtraArgs }}' 3>&1 4>&1 5>&1 6>&1
+</powershell>
+		`
+	} else {
+		UserDataTemplate = `#!/bin/bash
 {{range $pre := .PreBootstrap}}{{$pre}}{{end}}
 {{- range .MountOptions}}
 mkfs.{{ .FileSystem | ToLower }} {{ .Device }}
@@ -113,9 +124,10 @@ set -o xtrace
 /etc/eks/bootstrap.sh {{ .ClusterName }} {{ .Arguments }}
 set +o xtrace
 {{range $post := .PostBootstrap}}{{$post}}{{end}}`
-
+	}
 	data := EKSUserData{
 		ClusterName:   clusterName,
+		KubeletExtraArgs: kubeletExtraArgs,
 		Arguments:     args,
 		PreBootstrap:  payload.PreBootstrap,
 		PostBootstrap: payload.PostBootstrap,
@@ -371,6 +383,10 @@ func (ctx *EksInstanceGroupContext) GetLabelList() []string {
 }
 
 func (ctx *EksInstanceGroupContext) GetBootstrapArgs() string {
+	return fmt.Sprintf("--kubelet-extra-args '%v'", ctx.GetKubeletExtraArgs())
+}
+
+func (ctx *EksInstanceGroupContext) GetKubeletExtraArgs() string {
 	var (
 		instanceGroup = ctx.GetInstanceGroup()
 		configuration = instanceGroup.GetEKSConfiguration()
@@ -379,7 +395,7 @@ func (ctx *EksInstanceGroupContext) GetBootstrapArgs() string {
 
 	labelsFlag := fmt.Sprintf("--node-labels=%v", strings.Join(ctx.GetLabelList(), ","))
 	taintsFlag := fmt.Sprintf("--register-with-taints=%v", strings.Join(ctx.GetTaintList(), ","))
-	return fmt.Sprintf("--kubelet-extra-args '%v %v %v'", labelsFlag, taintsFlag, bootstrapArgs)
+	return fmt.Sprintf("%v %v %v", labelsFlag, taintsFlag, bootstrapArgs)
 }
 
 func (ctx *EksInstanceGroupContext) discoverSpotPrice() error {
@@ -770,6 +786,7 @@ func (ctx *EksInstanceGroupContext) RemoveAuthRole(arn string) error {
 	defer ctx.Unlock()
 
 	var instanceGroup = ctx.GetInstanceGroup()
+	var osFamily = ctx.GetOsFamily()
 	var list = &unstructured.UnstructuredList{}
 	var sharedGroups = make([]string, 0)
 
@@ -798,5 +815,5 @@ func (ctx *EksInstanceGroupContext) RemoveAuthRole(arn string) error {
 		return nil
 	}
 
-	return common.RemoveAuthConfigMap(ctx.KubernetesClient.Kubernetes, []string{arn})
+	return common.RemoveAuthConfigMap(ctx.KubernetesClient.Kubernetes, []string{arn}, []string{osFamily})
 }
diff --git a/controllers/provisioners/eks/update.go b/controllers/provisioners/eks/update.go
index 4a4634a..de0dfab 100644
--- a/controllers/provisioners/eks/update.go
+++ b/controllers/provisioners/eks/update.go
@@ -36,10 +36,11 @@ func (ctx *EksInstanceGroupContext) Update() error {
 		scalingConfig   = state.GetScalingConfiguration()
 		configuration   = instanceGroup.GetEKSConfiguration()
 		args            = ctx.GetBootstrapArgs()
+		kubeletArgs     = ctx.GetKubeletExtraArgs()
 		userDataPayload = ctx.GetUserDataStages()
 		clusterName     = configuration.GetClusterName()
 		mounts          = ctx.GetMountOpts()
-		userData        = ctx.GetBasicUserData(clusterName, args, userDataPayload, mounts)
+		userData        = ctx.GetBasicUserData(clusterName, args, kubeletArgs, userDataPayload, mounts)
 		sgs             = ctx.ResolveSecurityGroups()
 		spotPrice       = configuration.GetSpotPrice()
 	)
@@ -150,7 +151,6 @@ func (ctx *EksInstanceGroupContext) UpdateScalingGroup(configName string) error
 	if err := ctx.UpdateMetricsCollection(asgName); err != nil {
 		return err
 	}
-
 	if err := ctx.UpdateLifecycleHooks(asgName); err != nil {
 		return err
 	}
diff --git a/controllers/provisioners/eks/upgrade.go b/controllers/provisioners/eks/upgrade.go
index 98dbe98..9e68abb 100644
--- a/controllers/provisioners/eks/upgrade.go
+++ b/controllers/provisioners/eks/upgrade.go
@@ -75,6 +75,7 @@ func (ctx *EksInstanceGroupContext) BootstrapNodes() error {
 	var (
 		state         = ctx.GetDiscoveredState()
 		instanceGroup = ctx.GetInstanceGroup()
+		osFamily      = ctx.GetOsFamily()
 		role          = state.GetRole()
 		roleARN       = aws.StringValue(role.Arn)
 	)
@@ -84,7 +85,7 @@ func (ctx *EksInstanceGroupContext) BootstrapNodes() error {
 	ctx.Lock()
 	defer ctx.Unlock()
 
-	return common.UpsertAuthConfigMap(ctx.KubernetesClient.Kubernetes, []string{roleARN})
+	return common.UpsertAuthConfigMap(ctx.KubernetesClient.Kubernetes, []string{roleARN}, []string{osFamily})
 }
 
 func (ctx *EksInstanceGroupContext) NewRollingUpdateRequest() *kubeprovider.RollingUpdateRequest {


Test patch:
diff --git a/controllers/provisioners/eks/eks_test.go b/controllers/provisioners/eks/eks_test.go
index e0de528..05bc014 100644
--- a/controllers/provisioners/eks/eks_test.go
+++ b/controllers/provisioners/eks/eks_test.go
@@ -131,6 +131,37 @@ func MockInstanceGroup() *v1alpha1.InstanceGroup {
 	}
 }
 
+func MockWindowsInstanceGroup() *v1alpha1.InstanceGroup {
+	return &v1alpha1.InstanceGroup{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "instance-group-1",
+			Namespace: "instance-manager",
+			Annotations: map[string]string{
+				ClusterAutoscalerEnabledAnnotation: "true",
+				OsFamilyAnnotation: OsFamilyWindows,
+			},
+		},
+		Spec: v1alpha1.InstanceGroupSpec{
+			Provisioner: ProvisionerName,
+			EKSSpec: &v1alpha1.EKSSpec{
+				MaxSize: 3,
+				MinSize: 1,
+				EKSConfiguration: &v1alpha1.EKSConfiguration{
+					EksClusterName: "my-cluster",
+					SuspendedProcesses: []string{
+						"AZRebalance",
+					},
+				},
+			},
+			AwsUpgradeStrategy: v1alpha1.AwsUpgradeStrategy{
+				CRDType:           &v1alpha1.CRDUpdateStrategy{},
+				RollingUpdateType: &v1alpha1.RollingUpdateStrategy{},
+			},
+		},
+		Status: v1alpha1.InstanceGroupStatus{},
+	}
+}
+
 func MockLaunchConfigFromInput(input *autoscaling.CreateLaunchConfigurationInput) *autoscaling.LaunchConfiguration {
 	return &autoscaling.LaunchConfiguration{
 		LaunchConfigurationName: input.LaunchConfigurationName,
diff --git a/controllers/provisioners/eks/helpers_test.go b/controllers/provisioners/eks/helpers_test.go
index 59b8103..b5632af 100644
--- a/controllers/provisioners/eks/helpers_test.go
+++ b/controllers/provisioners/eks/helpers_test.go
@@ -16,9 +16,11 @@ limitations under the License.
 package eks
 
 import (
+	"encoding/base64"
 	"fmt"
 	corev1 "k8s.io/api/core/v1"
 	"sort"
+	"strings"
 	"testing"
 
 	"github.com/aws/aws-sdk-go/aws"
@@ -501,6 +503,47 @@ func TestGetUserDataStages(t *testing.T) {
 	}
 }
 
+func TestBootstrapDataForOSFamily(t *testing.T) {
+	var (
+		k             = MockKubernetesClientSet()
+		linuxIg       = MockInstanceGroup()
+		windowsIg            = MockWindowsInstanceGroup()
+		asgMock       = NewAutoScalingMocker()
+		iamMock       = NewIamMocker()
+		eksMock       = NewEksMocker()
+		ec2Mock       = NewEc2Mocker()
+	)
+
+	w := MockAwsWorker(asgMock, iamMock, eksMock, ec2Mock)
+
+	tests := []struct {
+		ig        *v1alpha1.InstanceGroup
+		expectedScriptSubstrings string
+	}{
+		{
+			ig: linuxIg,
+			expectedScriptSubstrings: "/etc/eks/bootstrap.sh",
+		},
+		{
+			ig: windowsIg,
+			expectedScriptSubstrings: "<powershell>",
+		},
+	}
+
+	for i, tc := range tests {
+		t.Logf("Test #%v - %+v", i, tc)
+		ctx := MockContext(tc.ig, k, w)
+		basicUserData := ctx.GetBasicUserData("", "","", UserDataPayload{}, []MountOpts{} )
+		basicUserDataDecoded, _ := base64.StdEncoding.DecodeString(basicUserData)
+		basicUserDataString := string(basicUserDataDecoded)
+		if !strings.Contains(basicUserDataString, tc.expectedScriptSubstrings) {
+			t.Fatalf("Cound not find expected string %v script in %v",tc.expectedScriptSubstrings, basicUserDataString)
+		}
+	}
+
+
+}
+
 func TestUpdateLifecycleHooks(t *testing.T) {
 	var (
 		g             = gomega.NewGomegaWithT(t)