Repository: keikoproj/instance-manager
Pull request #337 (2021-11-12 19:30:42)

Stop passing in ClusterCA and Endpoint information for Windows nodes
The Windows EKS bootstrap script currently contains a bug related to ServiceCidr calculations when passing in ClusterCA and APIEndpoint information - see https://github.com/awslabs/amazon-eks-ami/issues/805. Until the bootstrap script allows us to provide the ServiceCIDR, we should stop passing in the ClusterCA and Endpoint information to avoid incorrect CIDRs from being calculated. 

codecov[bot]: # [Codecov](https://codecov.io/gh/keikoproj/instance-manager/pull/337?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keikoproj) Report
> Merging [#337](https://codecov.io/gh/keikoproj/instance-manager/pull/337?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keikoproj) (a26f39e) into [master](https://codecov.io/gh/keikoproj/instance-manager/commit/5215605396f290bd86918bb0c7e6e59e43a01e80?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keikoproj) (5215605) will **decrease** coverage by `0.03%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/keikoproj/instance-manager/pull/337/graphs/tree.svg?width=650&height=150&src=pr&token=IJbjmSBliL&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keikoproj)](https://codecov.io/gh/keikoproj/instance-manager/pull/337?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keikoproj)

```diff
@@            Coverage Diff             @@
##           master     #337      +/-   ##
==========================================
- Coverage   51.08%   51.05%   -0.04%     
==========================================
  Files          33       33              
  Lines        4504     4501       -3     
==========================================
- Hits         2301     2298       -3     
  Misses       2062     2062              
  Partials      141      141              
```


| [Impacted Files](https://codecov.io/gh/keikoproj/instance-manager/pull/337?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keikoproj) | Coverage Δ | |
|---|---|---|
| [controllers/provisioners/eks/helpers.go](https://codecov.io/gh/keikoproj/instance-manager/pull/337/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keikoproj#diff-Y29udHJvbGxlcnMvcHJvdmlzaW9uZXJzL2Vrcy9oZWxwZXJzLmdv) | `91.36% <ø> (-0.04%)` | :arrow_down: |

------

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

eytan-avisror: Since it's a bug workaround, should we have a flag to control this behavior? e.g. `--disable-windows-cluster-ca-detection` or similar? it might be too specific, but that way we won't need another release when it's fixed and we end up giving more flexibility
WDYT
backjo: @eytan-avisror it depends on how they fix it - if the fix is to require passing in a `ServiceCidr` parameter, we'd need another update to propagate that information. I'm fine flagging it for now. 
backjo: @eytan-avisror - added a flag for the controller to allow toggling injection. 

Patch:
diff --git a/controllers/instancegroup_controller.go b/controllers/instancegroup_controller.go
index e6f57ff..881a2f3 100644
--- a/controllers/instancegroup_controller.go
+++ b/controllers/instancegroup_controller.go
@@ -43,17 +43,18 @@ import (
 // InstanceGroupReconciler reconciles an InstanceGroup object
 type InstanceGroupReconciler struct {
 	client.Client
-	SpotRecommendationTime float64
-	ConfigNamespace        string
-	NodeRelabel            bool
-	Log                    logr.Logger
-	MaxParallel            int
-	Auth                   *InstanceGroupAuthenticator
-	ConfigMap              *corev1.ConfigMap
-	Namespaces             map[string]corev1.Namespace
-	NamespacesLock         *sync.RWMutex
-	ConfigRetention        int
-	Metrics                *common.MetricsCollector
+	SpotRecommendationTime     float64
+	ConfigNamespace            string
+	NodeRelabel                bool
+	Log                        logr.Logger
+	MaxParallel                int
+	Auth                       *InstanceGroupAuthenticator
+	ConfigMap                  *corev1.ConfigMap
+	Namespaces                 map[string]corev1.Namespace
+	NamespacesLock             *sync.RWMutex
+	ConfigRetention            int
+	Metrics                    *common.MetricsCollector
+	DisableWinClusterInjection bool
 }
 
 type InstanceGroupAuthenticator struct {
@@ -134,13 +135,14 @@ func (r *InstanceGroupReconciler) Reconcile(ctxt context.Context, req ctrl.Reque
 	r.SetFinalizer(instanceGroup)
 
 	input := provisioners.ProvisionerInput{
-		AwsWorker:       r.Auth.Aws,
-		Kubernetes:      r.Auth.Kubernetes,
-		Configuration:   r.ConfigMap,
-		InstanceGroup:   instanceGroup,
-		Log:             r.Log,
-		ConfigRetention: r.ConfigRetention,
-		Metrics:         r.Metrics,
+		AwsWorker:                  r.Auth.Aws,
+		Kubernetes:                 r.Auth.Kubernetes,
+		Configuration:              r.ConfigMap,
+		InstanceGroup:              instanceGroup,
+		Log:                        r.Log,
+		ConfigRetention:            r.ConfigRetention,
+		Metrics:                    r.Metrics,
+		DisableWinClusterInjection: r.DisableWinClusterInjection,
 	}
 
 	var (
diff --git a/controllers/provisioners/eks/eks.go b/controllers/provisioners/eks/eks.go
index dd15a5c..e7bed62 100644
--- a/controllers/provisioners/eks/eks.go
+++ b/controllers/provisioners/eks/eks.go
@@ -71,13 +71,14 @@ func New(p provisioners.ProvisionerInput) *EksInstanceGroupContext {
 	)
 
 	ctx := &EksInstanceGroupContext{
-		InstanceGroup:    instanceGroup,
-		KubernetesClient: p.Kubernetes,
-		AwsWorker:        p.AwsWorker,
-		Log:              p.Log.WithName("eks"),
-		ResourcePrefix:   fmt.Sprintf("%v-%v-%v", configuration.GetClusterName(), instanceGroup.GetNamespace(), instanceGroup.GetName()),
-		ConfigRetention:  p.ConfigRetention,
-		Metrics:          p.Metrics,
+		InstanceGroup:              instanceGroup,
+		KubernetesClient:           p.Kubernetes,
+		AwsWorker:                  p.AwsWorker,
+		Log:                        p.Log.WithName("eks"),
+		ResourcePrefix:             fmt.Sprintf("%v-%v-%v", configuration.GetClusterName(), instanceGroup.GetNamespace(), instanceGroup.GetName()),
+		ConfigRetention:            p.ConfigRetention,
+		Metrics:                    p.Metrics,
+		DisableWinClusterInjection: p.DisableWinClusterInjection,
 	}
 
 	ctx.SetState(v1alpha1.ReconcileInit)
@@ -89,15 +90,16 @@ func New(p provisioners.ProvisionerInput) *EksInstanceGroupContext {
 
 type EksInstanceGroupContext struct {
 	sync.Mutex
-	InstanceGroup    *v1alpha1.InstanceGroup
-	KubernetesClient kubeprovider.KubernetesClientSet
-	AwsWorker        awsprovider.AwsWorker
-	DiscoveredState  *DiscoveredState
-	Log              logr.Logger
-	Configuration    *provisioners.ProvisionerConfiguration
-	ConfigRetention  int
-	ResourcePrefix   string
-	Metrics          *common.MetricsCollector
+	InstanceGroup              *v1alpha1.InstanceGroup
+	KubernetesClient           kubeprovider.KubernetesClientSet
+	AwsWorker                  awsprovider.AwsWorker
+	DiscoveredState            *DiscoveredState
+	Log                        logr.Logger
+	Configuration              *provisioners.ProvisionerConfiguration
+	ConfigRetention            int
+	ResourcePrefix             string
+	Metrics                    *common.MetricsCollector
+	DisableWinClusterInjection bool
 }
 
 type UserDataPayload struct {
diff --git a/controllers/provisioners/eks/helpers.go b/controllers/provisioners/eks/helpers.go
index 6ff3a3b..87ca462 100644
--- a/controllers/provisioners/eks/helpers.go
+++ b/controllers/provisioners/eks/helpers.go
@@ -539,7 +539,7 @@ func (ctx *EksInstanceGroupContext) GetBootstrapArgs() string {
 	var sb strings.Builder
 	switch strings.ToLower(osFamily) {
 	case OsFamilyWindows:
-		if state.Cluster != nil {
+		if state.Cluster != nil && !ctx.DisableWinClusterInjection {
 			sb.WriteString(fmt.Sprintf("-Base64ClusterCA %v ", aws.StringValue(state.Cluster.CertificateAuthority.Data)))
 			sb.WriteString(fmt.Sprintf("-APIServerEndpoint %v ", aws.StringValue(state.Cluster.Endpoint)))
 		}
diff --git a/controllers/provisioners/provisioners.go b/controllers/provisioners/provisioners.go
index b2985ee..85befff 100644
--- a/controllers/provisioners/provisioners.go
+++ b/controllers/provisioners/provisioners.go
@@ -27,13 +27,14 @@ const (
 )
 
 type ProvisionerInput struct {
-	AwsWorker       awsprovider.AwsWorker
-	Kubernetes      kubeprovider.KubernetesClientSet
-	InstanceGroup   *v1alpha1.InstanceGroup
-	Configuration   *corev1.ConfigMap
-	Log             logr.Logger
-	ConfigRetention int
-	Metrics         *common.MetricsCollector
+	AwsWorker                  awsprovider.AwsWorker
+	Kubernetes                 kubeprovider.KubernetesClientSet
+	InstanceGroup              *v1alpha1.InstanceGroup
+	Configuration              *corev1.ConfigMap
+	Log                        logr.Logger
+	ConfigRetention            int
+	Metrics                    *common.MetricsCollector
+	DisableWinClusterInjection bool
 }
 
 var (
diff --git a/main.go b/main.go
index fbc4151..af78bf8 100644
--- a/main.go
+++ b/main.go
@@ -65,15 +65,16 @@ func main() {
 	printVersion()
 
 	var (
-		metricsAddr            string
-		configNamespace        string
-		spotRecommendationTime float64
-		enableLeaderElection   bool
-		nodeRelabel            bool
-		maxParallel            int
-		maxAPIRetries          int
-		configRetention        int
-		err                    error
+		metricsAddr                string
+		configNamespace            string
+		spotRecommendationTime     float64
+		enableLeaderElection       bool
+		nodeRelabel                bool
+		disableWinClusterInjection bool
+		maxParallel                int
+		maxAPIRetries              int
+		configRetention            int
+		err                        error
 	)
 
 	flag.IntVar(&maxParallel, "max-workers", 5, "The number of maximum parallel reconciles")
@@ -85,6 +86,8 @@ func main() {
 	flag.BoolVar(&enableLeaderElection, "enable-leader-election", false,
 		"Enable leader election for controller manager. Enabling this will ensure there is only one active controller manager.")
 	flag.BoolVar(&nodeRelabel, "node-relabel", true, "relabel nodes as they join with kubernetes.io/role label via controller")
+	flag.BoolVar(&disableWinClusterInjection, "disable-windows-cluster-ca-injection", false, "Setting this to true will cause the ClusterCA and Endpoint to not be injected for Windows nodes")
+
 	flag.Parse()
 	ctrl.SetLogger(zap.New(zap.UseDevMode(true)))
 
@@ -147,17 +150,18 @@ func main() {
 	}
 
 	err = (&controllers.InstanceGroupReconciler{
-		Metrics:                controllerCollector,
-		ConfigMap:              cm,
-		ConfigRetention:        configRetention,
-		SpotRecommendationTime: spotRecommendationTime,
-		ConfigNamespace:        configNamespace,
-		Namespaces:             make(map[string]corev1.Namespace),
-		NamespacesLock:         &sync.RWMutex{},
-		NodeRelabel:            nodeRelabel,
-		Client:                 mgr.GetClient(),
-		Log:                    ctrl.Log.WithName("controllers").WithName("instancegroup"),
-		MaxParallel:            maxParallel,
+		Metrics:                    controllerCollector,
+		ConfigMap:                  cm,
+		ConfigRetention:            configRetention,
+		SpotRecommendationTime:     spotRecommendationTime,
+		ConfigNamespace:            configNamespace,
+		Namespaces:                 make(map[string]corev1.Namespace),
+		NamespacesLock:             &sync.RWMutex{},
+		NodeRelabel:                nodeRelabel,
+		DisableWinClusterInjection: disableWinClusterInjection,
+		Client:                     mgr.GetClient(),
+		Log:                        ctrl.Log.WithName("controllers").WithName("instancegroup"),
+		MaxParallel:                maxParallel,
 		Auth: &controllers.InstanceGroupAuthenticator{
 			Aws:        awsWorker,
 			Kubernetes: kube,


Test patch:
diff --git a/controllers/provisioners/eks/helpers_test.go b/controllers/provisioners/eks/helpers_test.go
index 9e09ff4..71f77be 100644
--- a/controllers/provisioners/eks/helpers_test.go
+++ b/controllers/provisioners/eks/helpers_test.go
@@ -249,6 +249,83 @@ func TestGetBasicUserDataWindows(t *testing.T) {
 	}
 }
 
+func TestGetBasicUserDataWindowsWithInjectionDisabled(t *testing.T) {
+	var (
+		k             = MockKubernetesClientSet()
+		ig            = MockInstanceGroup()
+		asgMock       = NewAutoScalingMocker()
+		iamMock       = NewIamMocker()
+		eksMock       = NewEksMocker()
+		ec2Mock       = NewEc2Mocker()
+		ssmMock       = NewSsmMocker()
+		configuration = ig.GetEKSConfiguration()
+	)
+
+	w := MockAwsWorker(asgMock, iamMock, eksMock, ec2Mock, ssmMock)
+	ctx := MockContext(ig, k, w)
+
+	configuration.BootstrapOptions = &v1alpha1.BootstrapOptions{
+		MaxPods: 4,
+	}
+	configuration.Labels = map[string]string{
+		"foo": "bar",
+	}
+	configuration.Taints = []corev1.Taint{
+		{
+			Key:    "foo",
+			Value:  "bar",
+			Effect: "NoSchedule",
+		},
+	}
+
+	configuration.BootstrapArguments = "--eviction-hard=memory.available<300Mi,nodefs.available<5% --system-reserved=memory=2.5Gi --v=2"
+	configuration.UserData = []v1alpha1.UserDataStage{
+		{
+			Stage: "PreBootstrap",
+			Data:  "foo",
+		},
+		{
+			Stage: "PostBootstrap",
+			Data:  "bar",
+		},
+	}
+
+	ig.Annotations[OsFamilyAnnotation] = OsFamilyWindows
+
+	expectedDataWindows := `
+<powershell>
+  foo
+  [string]$EKSBinDir = "$env:ProgramFiles\Amazon\EKS"
+  [string]$EKSBootstrapScriptName = 'Start-EKSBootstrap.ps1'
+  [string]$EKSBootstrapScriptFile = "$EKSBinDir\$EKSBootstrapScriptName"
+  [string]$IMDSToken=(curl -UseBasicParsing -Method PUT "http://169.254.169.254/latest/api/token" -H @{ "X-aws-ec2-metadata-token-ttl-seconds" = "21600"} | % { Echo $_.Content})
+  [string]$InstanceID=(curl -UseBasicParsing -Method GET "http://169.254.169.254/latest/meta-data/instance-id" -H @{ "X-aws-ec2-metadata-token" = "$IMDSToken"} | % { Echo $_.Content})
+  [string]$Lifecycle = Get-ASAutoScalingInstance $InstanceID | % { Echo $_.LifecycleState}
+  if ($Lifecycle -like "*Warmed*") {
+    Echo "Not starting Kubelet due to warmed state."
+    & C:\ProgramData\Amazon\EC2-Windows\Launch\Scripts\InitializeInstance.ps1 –Schedule
+  } else {
+    & $EKSBootstrapScriptFile -EKSClusterName foo -KubeletExtraArgs '--node-labels=foo=bar,instancemgr.keikoproj.io/image=ami-123456789012,node.kubernetes.io/role=instance-group-1 --register-with-taints=foo=bar:NoSchedule --eviction-hard=memory.available<300Mi,nodefs.available<5% --system-reserved=memory=2.5Gi --v=2 --max-pods=4' 3>&1 4>&1 5>&1 6>&1
+    bar
+  }
+</powershell>`
+
+	ctx.DisableWinClusterInjection = true
+	var (
+		args            = ctx.GetBootstrapArgs()
+		kubeletArgs     = ctx.GetKubeletExtraArgs()
+		userDataPayload = ctx.GetUserDataStages()
+		mounts          = ctx.GetMountOpts()
+	)
+
+	userData := ctx.GetBasicUserData("foo", args, kubeletArgs, userDataPayload, mounts)
+	basicUserDataDecoded, _ := base64.StdEncoding.DecodeString(userData)
+	basicUserDataString := string(basicUserDataDecoded)
+	if basicUserDataString != expectedDataWindows {
+		t.Fatalf("\nExpected: START>%v<END\n Got: START>%v<END", expectedDataWindows, basicUserDataString)
+	}
+}
+
 func TestCustomNetworkingMaxPods(t *testing.T) {
 	var (
 		k       = MockKubernetesClientSet()