Repository: keikoproj/instance-manager
Pull request #129 (2020-05-30 01:23:52)

Add kubernetes.io/role label via node reconciler
This bypasses the (very annoying) mechanism put forth in https://github.com/kubernetes/enhancements/blob/master/keps/sig-auth/0000-20170814-bounding-self-labeling-kubelets.md

This will enable the instance-group controller, to label any node that has the label `node.kubernetes.io/role=role` populated, with the respective `kubernetes.io/role=role` which cannot be added during bootstrap in 1.16.

### Previous Behavior (before #125 was merged)

#### 1.15
```
node-role.kubernetes.io/instance-group-1="" => added as default kubelet arguments
node.kubernetes.io/role=instance-group-1 => added as default kubelet arguments
```
#### 1.16 (nodes fail to join #120)
```
node-role.kubernetes.io/instance-group-1="" => added as default kubelet arguments
node.kubernetes.io/role=instance-group-1 => added as default kubelet arguments
```

### Current Behavior (after #125 was merged)
#### 1.15
```
node-role.kubernetes.io/instance-group-1="" => added as kubelet arguments
node.kubernetes.io/instance-group-1="" => added as default kubelet arguments
```
#### 1.16
```
node.kubernetes.io/role=instance-group-1 =>  added as default kubelet arguments
```

### New Behavior (after this PR is merged)
#### 1.15
```
node.kubernetes.io/role=instance-group-1 => added as default kubelet arguments
node-role.kubernetes.io/instance-group-1="" => added as default kubelet arguments
kubernetes.io/role=instance-group-1 => added by controller
```

#### 1.16
```
node.kubernetes.io/role=instance-group-1 => added as default kubelet arguments
kubernetes.io/role=instance-group-1 => added by controller
```

### Migration path
#### Before control plane upgrade to 1.16
- Update controller to 0.7.0
- Nodes will now have all 3 labels
#### Upgrade control plane to 1.16
- Scale down cluster autoscaler prior to control plane upgrade, to avoid new nodes joining with the 1.15 labels
- After the upgrade, the next reconcile of the instance groups will trigger a rolling update, wait for it to complete
- Nodes will rotate to remove the old style label, you will be left with the two 1.16 labels


#### Overriding default behavior and implementing your own migration
If you want to migrate instance groups to EKS 1.16 in some different manner, you can also control the above described behavior with the following:
- By running the controller with the flag `--node-relabel` you can control whether the controller will re-label `node.kubernetes.io/role=instance-group-name` with `kubernetes.io/role=instance-group-name`, setting the flag to false will avoid this relabeling and only the default kubelet role labels will be applied, you can use this along with something such as a mutating webhook on node creation to avoid the time window introduced by the controller watch.
- If you want to override the default role labels that gets added, you can annotate your instance group with `instancemgr.keikoproj.io/default-labels: some-label/key=value,other-label/key=value`, this means the controller will not add the role labels by default, and will only add what is provided in the annotation in addition to labels provided in the spec.

codecov[bot]: # [Codecov](https://codecov.io/gh/keikoproj/instance-manager/pull/129?src=pr&el=h1) Report
> Merging [#129](https://codecov.io/gh/keikoproj/instance-manager/pull/129?src=pr&el=desc) into [master](https://codecov.io/gh/keikoproj/instance-manager/commit/138d4b1ae3b0f7f987f0825ffda45040ba3a5880&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

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

```diff
@@           Coverage Diff           @@
##           master     #129   +/-   ##
=======================================
  Coverage   88.87%   88.87%           
=======================================
  Files          10       10           
  Lines        1187     1187           
=======================================
  Hits         1055     1055           
  Misses         91       91           
  Partials       41       41           
```



------

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


Patch:
diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS
index d60a141..70d46ee 100644
--- a/.github/CODEOWNERS
+++ b/.github/CODEOWNERS
@@ -6,4 +6,5 @@
 # These owners will be the default owners for everything in
 # the repo. Unless a later match takes precedence,
 # review when someone opens a pull request.
-*       @keikoproj/authorized-approvers
\ No newline at end of file
+*       @keikoproj/authorized-approvers
+*       @keikoproj/instance-manager-approvers
\ No newline at end of file
diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml
index 46f23ef..4c3730b 100644
--- a/config/rbac/role.yaml
+++ b/config/rbac/role.yaml
@@ -3,7 +3,6 @@
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRole
 metadata:
-  creationTimestamp: null
   name: instance-manager
 rules:
 - apiGroups:
@@ -43,6 +42,7 @@ rules:
   - nodes
   verbs:
   - list
+  - patch
 - apiGroups:
   - instancemgr.keikoproj.io
   resources:
diff --git a/controllers/common/utils.go b/controllers/common/utils.go
index 9dbdec5..e508c6b 100644
--- a/controllers/common/utils.go
+++ b/controllers/common/utils.go
@@ -69,6 +69,10 @@ func SliceEmpty(slice []string) bool {
 	return len(slice) == 0
 }
 
+func MapEmpty(m map[string]string) bool {
+	return len(m) == 0
+}
+
 func StringEmpty(str string) bool {
 	return str == ""
 }
diff --git a/controllers/instancegroup_controller.go b/controllers/instancegroup_controller.go
index 4e69804..a9dd81b 100644
--- a/controllers/instancegroup_controller.go
+++ b/controllers/instancegroup_controller.go
@@ -17,6 +17,7 @@ package controllers
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"os"
 	"strings"
@@ -48,6 +49,7 @@ import (
 type InstanceGroupReconciler struct {
 	client.Client
 	SpotRecommendationTime float64
+	NodeRelabel            bool
 	Log                    logr.Logger
 	ControllerConfPath     string
 	MaxParallel            int
@@ -110,7 +112,7 @@ func (r *InstanceGroupReconciler) NewProvisionerInput(instanceGroup *v1alpha1.In
 	return input, nil
 }
 
-// +kubebuilder:rbac:groups=core,resources=nodes,verbs=list
+// +kubebuilder:rbac:groups=core,resources=nodes,verbs=list;patch
 // +kubebuilder:rbac:groups=core,resources=events,verbs=get;list;watch;create
 // +kubebuilder:rbac:groups=core,resources=configmaps,verbs=get;list;create;update;patch
 // +kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=get;list;watch;create;update;patch;delete
@@ -217,13 +219,76 @@ func (r *InstanceGroupReconciler) Reconcile(req ctrl.Request) (ctrl.Result, erro
 }
 
 func (r *InstanceGroupReconciler) SetupWithManager(mgr ctrl.Manager) error {
-	return ctrl.NewControllerManagedBy(mgr).
-		For(&v1alpha1.InstanceGroup{}).
-		Watches(&source.Kind{Type: &corev1.Event{}}, &handler.EnqueueRequestsFromMapFunc{
-			ToRequests: handler.ToRequestsFunc(r.spotEventReconciler),
-		}).
-		WithOptions(controller.Options{MaxConcurrentReconciles: r.MaxParallel}).
-		Complete(r)
+	switch r.NodeRelabel {
+	case true:
+		return ctrl.NewControllerManagedBy(mgr).
+			For(&v1alpha1.InstanceGroup{}).
+			Watches(&source.Kind{Type: &corev1.Event{}}, &handler.EnqueueRequestsFromMapFunc{
+				ToRequests: handler.ToRequestsFunc(r.spotEventReconciler),
+			}).
+			Watches(&source.Kind{Type: &corev1.Node{}}, &handler.EnqueueRequestsFromMapFunc{
+				ToRequests: handler.ToRequestsFunc(r.nodeReconciler),
+			}).
+			WithOptions(controller.Options{MaxConcurrentReconciles: r.MaxParallel}).
+			Complete(r)
+	default:
+		return ctrl.NewControllerManagedBy(mgr).
+			For(&v1alpha1.InstanceGroup{}).
+			Watches(&source.Kind{Type: &corev1.Event{}}, &handler.EnqueueRequestsFromMapFunc{
+				ToRequests: handler.ToRequestsFunc(r.spotEventReconciler),
+			}).
+			WithOptions(controller.Options{MaxConcurrentReconciles: r.MaxParallel}).
+			Complete(r)
+	}
+}
+
+type NodeLabels struct {
+	Labels map[string]string `json:"labels,omitempty"`
+}
+
+type LabelPatch struct {
+	Metadata *NodeLabels `json:"metadata,omitempty"`
+}
+
+func (r *InstanceGroupReconciler) nodeReconciler(obj handler.MapObject) []ctrl.Request {
+	var (
+		nodeName          = obj.Meta.GetName()
+		nodeLabels        = obj.Meta.GetLabels()
+		roleLabelKey      = "kubernetes.io/role"
+		bootstrapLabelKey = "node.kubernetes.io/role"
+	)
+
+	// if node already has a role label, don't modify it
+	if _, ok := nodeLabels[roleLabelKey]; ok {
+		return nil
+	}
+
+	// if node does not have the bootstrap label, don't modify it
+	var val string
+	var ok bool
+	if val, ok = nodeLabels[bootstrapLabelKey]; !ok {
+		return nil
+	}
+
+	nodeLabels[roleLabelKey] = val
+
+	labelPatch := &LabelPatch{
+		Metadata: &NodeLabels{
+			Labels: nodeLabels,
+		},
+	}
+
+	patchJSON, err := json.Marshal(labelPatch)
+	if err != nil {
+		r.Log.Error(err, "failed to marshal node labels", "node", nodeName, "patch", string(patchJSON))
+		return nil
+	}
+
+	if _, err = r.Auth.Kubernetes.Kubernetes.CoreV1().Nodes().Patch(nodeName, types.StrategicMergePatchType, patchJSON); err != nil {
+		r.Log.Error(err, "failed to patch node labels", "node", nodeName)
+	}
+
+	return nil
 }
 
 func (r *InstanceGroupReconciler) spotEventReconciler(obj handler.MapObject) []ctrl.Request {
diff --git a/controllers/provisioners/eks/eks.go b/controllers/provisioners/eks/eks.go
index ad173fa..b6b1183 100644
--- a/controllers/provisioners/eks/eks.go
+++ b/controllers/provisioners/eks/eks.go
@@ -29,6 +29,7 @@ import (
 const (
 	ProvisionerName                     = "eks"
 	defaultLaunchConfigurationRetention = 2
+	OverrideDefaultLabelsAnnotationKey  = "instancemgr.keikoproj.io/default-labels"
 )
 
 var (
diff --git a/controllers/provisioners/eks/helpers.go b/controllers/provisioners/eks/helpers.go
index f50471c..1285b5e 100644
--- a/controllers/provisioners/eks/helpers.go
+++ b/controllers/provisioners/eks/helpers.go
@@ -146,16 +146,28 @@ func (ctx *EksInstanceGroupContext) GetLabelList() []string {
 	var (
 		labelList     []string
 		instanceGroup = ctx.GetInstanceGroup()
+		annotations   = instanceGroup.GetAnnotations()
 		configuration = instanceGroup.GetEKSConfiguration()
 		customLabels  = configuration.GetLabels()
 	)
+
+	defer sort.Strings(labelList)
+
 	// get custom labels
 	if len(customLabels) > 0 {
 		for k, v := range customLabels {
 			labelList = append(labelList, fmt.Sprintf("%v=%v", k, v))
 		}
 	}
-	sort.Strings(labelList)
+
+	// allow override default labels
+	if val, ok := annotations[OverrideDefaultLabelsAnnotationKey]; ok {
+		overrideLabels := strings.Split(val, ",")
+		for _, label := range overrideLabels {
+			labelList = append(labelList, label)
+		}
+		return labelList
+	}
 
 	// add the new style role label
 	labelList = append(labelList, fmt.Sprintf(RoleNewLabelFmt, instanceGroup.GetName()))
diff --git a/main.go b/main.go
index f361f92..a07bf7f 100644
--- a/main.go
+++ b/main.go
@@ -62,6 +62,7 @@ func main() {
 		metricsAddr            string
 		spotRecommendationTime float64
 		enableLeaderElection   bool
+		nodeRelabel            bool
 		controllerConfPath     string
 		maxParallel            int
 		err                    error
@@ -73,6 +74,7 @@ func main() {
 	flag.StringVar(&controllerConfPath, "controller-config", "/etc/config/controller.conf", "The controller config file")
 	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.Parse()
 	ctrl.SetLogger(zap.Logger(true))
 
@@ -119,6 +121,7 @@ func main() {
 
 	err = (&controllers.InstanceGroupReconciler{
 		SpotRecommendationTime: spotRecommendationTime,
+		NodeRelabel:            nodeRelabel,
 		Client:                 mgr.GetClient(),
 		Log:                    ctrl.Log.WithName("controllers").WithName("instancegroup"),
 		ControllerConfPath:     controllerConfPath,


Test patch:
diff --git a/controllers/provisioners/eks/helpers_test.go b/controllers/provisioners/eks/helpers_test.go
index 0355b48..e94fea5 100644
--- a/controllers/provisioners/eks/helpers_test.go
+++ b/controllers/provisioners/eks/helpers_test.go
@@ -131,70 +131,55 @@ func TestGetEnabledMetrics(t *testing.T) {
 
 func TestGetLabelList(t *testing.T) {
 	var (
-		g                        = gomega.NewGomegaWithT(t)
-		k                        = MockKubernetesClientSet()
-		ig                       = MockInstanceGroup()
-		configuration            = ig.GetEKSConfiguration()
-		asgMock                  = NewAutoScalingMocker()
-		iamMock                  = NewIamMocker()
-		eksMock                  = NewEksMocker()
-		expectedLabels115        = []string{"node.kubernetes.io/role=instance-group-1", "node-role.kubernetes.io/instance-group-1=\"\""}
-		expectedLabels116        = []string{"node.kubernetes.io/role=instance-group-1"}
-		expectedLabelsWithCustom = []string{"node.kubernetes.io/role=instance-group-1", "testing.kubernetes.io=customlabel"}
+		g                          = gomega.NewGomegaWithT(t)
+		k                          = MockKubernetesClientSet()
+		ig                         = MockInstanceGroup()
+		configuration              = ig.GetEKSConfiguration()
+		asgMock                    = NewAutoScalingMocker()
+		iamMock                    = NewIamMocker()
+		eksMock                    = NewEksMocker()
+		expectedLabels115          = []string{"node.kubernetes.io/role=instance-group-1", "node-role.kubernetes.io/instance-group-1=\"\""}
+		expectedLabels116          = []string{"node.kubernetes.io/role=instance-group-1"}
+		expectedLabelsWithCustom   = []string{"custom.kubernetes.io=customlabel", "node.kubernetes.io/role=instance-group-1"}
+		expectedLabelsWithOverride = []string{"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"}
 	)
 
 	w := MockAwsWorker(asgMock, iamMock, eksMock)
 	ctx := MockContext(ig, k, w)
 
-	ctx.SetDiscoveredState(&DiscoveredState{
-		Publisher: kubeprovider.EventPublisher{
-			Client: k.Kubernetes,
-		},
-		Cluster: &eks.Cluster{
-			Version: aws.String("1.15"),
-		},
-	})
-
-	labels := ctx.GetLabelList()
-
-	g.Expect(labels).To(gomega.ConsistOf(expectedLabels115))
-
-	ctx.SetDiscoveredState(&DiscoveredState{
-		Publisher: kubeprovider.EventPublisher{
-			Client: k.Kubernetes,
-		},
-		Cluster: &eks.Cluster{
-			Version: aws.String("1.16"),
-		},
-	})
-
-	labels = ctx.GetLabelList()
-
-	g.Expect(labels).To(gomega.ConsistOf(expectedLabels116))
-	configuration.SetLabels(map[string]string{"testing.kubernetes.io": "customlabel"})
-	ctx.SetDiscoveredState(&DiscoveredState{
-		Publisher: kubeprovider.EventPublisher{
-			Client: k.Kubernetes,
-		},
-		Cluster: &eks.Cluster{
-			Version: aws.String("1.16"),
-		},
-	})
-
-	labels = ctx.GetLabelList()
-
-	g.Expect(labels).To(gomega.ConsistOf(expectedLabelsWithCustom))
-	configuration.SetLabels(map[string]string{})
-	ctx.SetDiscoveredState(&DiscoveredState{
-		Publisher: kubeprovider.EventPublisher{
-			Client: k.Kubernetes,
-		},
-		Cluster: &eks.Cluster{
-			Version: aws.String(""),
-		},
-	})
-
-	labels = ctx.GetLabelList()
-
-	g.Expect(labels).To(gomega.ConsistOf(expectedLabels115))
+	tests := []struct {
+		clusterVersion           string
+		instanceGroupLabels      map[string]string
+		instanceGroupAnnotations map[string]string
+		expectedLabels           []string
+	}{
+		// Default labels with missing cluster version
+		{clusterVersion: "", expectedLabels: expectedLabels115},
+		// Kubernetes 1.15 default labels
+		{clusterVersion: "1.15", expectedLabels: expectedLabels115},
+		// Kubernetes 1.16 default labels
+		{clusterVersion: "1.16", expectedLabels: expectedLabels116},
+		// Kubernetes 1.16 default labels with custom labels
+		{clusterVersion: "1.16", instanceGroupLabels: map[string]string{"custom.kubernetes.io": "customlabel"}, expectedLabels: expectedLabelsWithCustom},
+		// custom labels with override labels
+		{clusterVersion: "1.16", instanceGroupAnnotations: overrideAnnotation, instanceGroupLabels: map[string]string{"custom.kubernetes.io": "customlabel"}, expectedLabels: expectedLabelsWithOverride},
+	}
+
+	for i, tc := range tests {
+		t.Logf("Test #%v - %+v", i, tc)
+		configuration.SetLabels(tc.instanceGroupLabels)
+		ig.SetAnnotations(tc.instanceGroupAnnotations)
+		ctx.SetDiscoveredState(&DiscoveredState{
+			Publisher: kubeprovider.EventPublisher{
+				Client: k.Kubernetes,
+			},
+			Cluster: &eks.Cluster{
+				Version: aws.String(tc.clusterVersion),
+			},
+		})
+
+		labels := ctx.GetLabelList()
+		g.Expect(labels).To(gomega.Equal(tc.expectedLabels))
+	}
 }