feat: add label to exclude from external loadbalancers

[derbauer97]

Summary

Not yet i can open one if needed. Problem is that if i have a NodePort to expose for example my nginx ingress controller to an Elastic Loadbalancer. The Loadbalancer is not notified if i terminate one of our Nodes. Therefore, Kubernetes invented the exclude-for-external-loadbalancers label. When i add this label to a node the aws-loadbalancer-controller starts deregistering the Node from their TargetGroup which prevents Connections issue during Rolling Update.

For Reference: https://kubernetes.io/docs/reference/labels-annotations-taints/#node-kubernetes-io-exclude-from-external-load-balancers

Implementation in Karpenter: aws/karpenter-provider-aws#2518

Checklist

• Tested and/or added tests to validate that the changes work as intended, if applicable.
• Updated documentation in README.md, if applicable.

[TwiN]

Could you make this configuration disabled by default & expose it via an environment variable?

[derbauer97]

Hey,

thanks for reviewing bei PR. Why you want to have this behavior optional in my opinion this has only advantages. If you do not have a loadbalancer controller nothing will happen when adding the lable. If you have one the Loadbalancer will deregister the nodes properly. Also i do not know any implementation in which this is optional.

For the Label value we can go for something else. We call your tool Twin internally because its much shorter than aws-rolling-update-handler. Maybe i got influenced by this ^^ But i think it is a good idea to use something which allows to track which controller added the label. Do you have any ideas. We can go for aws-eks-rolling-update-handler if you want to.

[TwiN]

@derbauer97 I don't blame you for shortening the application name, I regret giving it such a long name sometimes as well 😂

The label's value should probably be true as it seems to be the most commonly used value for this annotation, even in Kubernetes' own documentation: https://kubernetes.io/docs/reference/labels-annotations-taints/#node-kubernetes-io-exclude-from-external-load-balancers

That being said, the main reason why I'd want to make this an opt-in feature rather than an opt-out feature is that making it opt-out would make this a breaking change as it modifies the current behavior of the application, and we don't know how every user's clusters are configured.

Once time has passed and the feature has more users, I may consider enabling this by default, but for now, I'd rather not expose users to unnecessary risks.

[derbauer97]

Allright understand i will build it optional.

Do you have any preferences for the implementation:

1. We can always set the label but if "EXLUDE_FROM_EXTERNAL_LOAD_BALANCERS" not enabled set to false otherwise set to true
2. do no set the label at all apart from EXLUDE_FROM_EXTERNAL_LOAD_BALANCERS is set to true then we set the label

We can also change the variable name it is a bit long but i do not know a better name :D

[derbauer97]

I pushed the second option. Hope for your Feedback!

[TwiN]

Awesome!

Now next problem is that there are no tests 😅

The very similar AnnotateNodeByAutoScalingInstance function at

aws-eks-asg-rolling-update-handler/k8s/util.go

Line 90 in 3a78a52

func AnnotateNodeByAutoScalingInstance(client ClientAPI, instance *autoscaling.Instance, key, value string) error {

doesn't have tests directly in util_test.go either, but it is tested here:

aws-eks-asg-rolling-update-handler/main_test.go

Lines 222 to 234 in 3a78a52

	if mockClient.Counter["UpdateNode"] != 1 {
	t.Error("Node should've been annotated, meaning that UpdateNode should've been called once")
	}
	oldNode = mockClient.Nodes[oldNode.Name]
	if _, ok := oldNode.GetAnnotations()[k8s.AnnotationRollingUpdateStartedTimestamp]; !ok {
	t.Error("Node should've been annotated with", k8s.AnnotationRollingUpdateStartedTimestamp)
	}
	if _, ok := oldNode.GetAnnotations()[k8s.AnnotationRollingUpdateDrainedTimestamp]; ok {
	t.Error("Node shouldn't have been drained yet, therefore shouldn't have been annotated with", k8s.AnnotationRollingUpdateDrainedTimestamp)
	}
	if _, ok := oldNode.GetAnnotations()[k8s.AnnotationRollingUpdateTerminatedTimestamp]; ok {
	t.Error("Node shouldn't have been terminated yet, therefore shouldn't have been annotated with", k8s.AnnotationRollingUpdateTerminatedTimestamp)
	}

I'd expect a separate test - something like TestHandleRollingUpgrade_withExcludeFromExternalLoadBalancers - to ensure that the label node.kubernetes.io/exclude-from-external-load-balancers has been added successfully if ExcludeFromExternalLoadBalancers is set to true.

This will also require updating

aws-eks-asg-rolling-update-handler/config/config.go

Line 138 in 3a78a52

func Set(autoScalingGroupNames []string, ignoreDaemonSets, deleteEmptyDirData, eagerCordoning bool) {

so that a new bool excludeFromExternalLoadBalancers can be set.

[derbauer97]

Sure your are right! ;)

I added a test with excludeFromExternalLoadBalancers which succeeds. I did not handle the case that the node has no labels at all which i think is very unlikely especially in aw. Because certain default labels are always added and we are not handling this for annotations either.

Also i put the Label Definition to the k8s module which is cleaner in my opinion and then labels are treated like the annotation you are already adding to the nodes.

Ready for your Feedback :D

[JPfeifer21]

Any ETA when this feature gets released?
We would need this too.

Thanks in advance!

[derbauer97]

Hey @TwiN,

is there anything i can do to get this merged. I adressed all your change requests i think. If there is anything else please let me know. Would be cool to get this feature in upstream.

[TwiN]

@derbauer97 Thank you for the contribution!

[TwiN]

@derbauer97 @JPfeifer21 Could you let me know if it works as intended on the latest tag?

Once you've confirmed, I'll go ahead and release it as part of a new version (v1.9.0).

[TwiN]

I just noticed you made a typo in the environment variable 😂

[TwiN]

Resolved by 3b8647b

[derbauer97]

Oh damn it :D thanks for fixing this!

[derbauer97]

I tested it seems to work fine. So feel free to release a new version.

[dinesh]

@TwiN can you please release a new version having this fix?