Repository: kubernetes-sigs/cluster-api-provider-aws (go)

Issue:
Kubernetes API server rolling restarts experience client-side disruption
/kind bug

**What steps did you take and what happened:**
[A clear and concise description of what the bug is.]

An [automated test](https://github.com/openshift/origin/blob/6767b922e0d15c31c3fc349c659987ce1922d650/test/extended/apiserver/rollout.go#L36) that forces kube API server restarts observed that roughly 30 seconds after the kube api-server returns HTTP 500s from `/readyz` pre-shutdown, clients are getting `ECONNRESET` when trying to read responses.

**What did you expect to happen:**

Clients would not have their connections interrupted.

**Anything else you would like to add:**
[Miscellaneous information that will assist in solving the issue.]

The `target_health_state.unhealthy.draining_interval_seconds` attribute should probably be set to something higher than the default (which is 0) in order to make client interactions more resilient to kube-api-server restarts.

This draining interval is separate from the health check interval seconds we currently set - that one just tells AWS how often to invoke the health check.

See the [TargetGroupAttribute docs](https://docs.aws.amazon.com/elasticloadbalancing/latest/APIReference/API_TargetGroupAttribute.html) for specifics.

This was observed during OpenShift testing, but is applicable generally to anyone restarting their KAS.

/area networking
/triage accepted

Pull request:
🐛 Enable NLB connection draining for graceful apiserver shutdown.
<!-- Thanks for this PR! If this is your first PR please read the [contributing guide](../CONTRIBUTING.md) -->
<!-- If this PR is still work-in-progress and is being open for visibility please prefix the title with `WIP:` -->

**What type of PR is this?**

<!--
Add one of the following kinds:
/kind feature
/kind bug
/kind api-change
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind flake
/kind regression
/kind support
-->

/kind bug

**What this PR does / why we need it**:

<!-- Enter a description of the change and why this change is needed -->

The kube-apiserver expects to terminate connections itself during graceful shutdown. As soon as kube-apiserver has received SIGTERM, its /readyz endpoint begins serving HTTP 500 responses. To allow time for load balancers to mark it unhealthy, it continues accepting new connections and serving requests on existing connections for a period of time (controlled by the --shutdown-delay-duration option). Once the shutdown delay has elapsed, it stops accepting new requests and drains in-flight requests before exiting.

By default, NLBs immediately terminate established connections when a target becomes unhealthy. This causes client-facing disruption for clients connected via NLB to a kube-apiserver instance that is shutting down.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #5475

**Special notes for your reviewer**:

As mentioned in the issue, OpenShift runs a test that continually performs rolling restarts of kube-apiserver in a 3-HA cluster. Throughout the test, clients make requests to the API and report any errors they observe. These reports include errors that aren't visible to the server.

Here's a timeline of what we currently see:

![before](https://github.com/user-attachments/assets/fcceb3c5-0bee-4dd3-8716-f547414311d0)

The gray bars represent the period during which a kube-apiserver is shutting down, and each row is a different kube-apiserver instance. The red bars are client-facing errors reported by the polling test client. All of the error samples are `read: connection reset by peer` occurring approximately 30 seconds after the start of each kube-apiserver shutdown window.

With this patch applied, the polling test client doesn't see any errors:

![after](https://github.com/user-attachments/assets/d7f4ad1e-8221-4d5d-a240-37e2bf877c60)


**Checklist**:
<!-- Put an "X" character inside the brackets of each completed task. Some may be optional depending on the PR in which case these can be deleted 

 Please add an icon to the title of this PR, the icon will be either ⚠️ (:warning:, major or breaking changes), ✨ (:sparkles:, feature additions), 🐛 (:bug:, patch and bugfixes), 📖 (:book:, documentation or proposals), or 🌱 (:seedling:, minor or other) 
-->

- [X] squashed commits
- [ ] includes documentation
- [ ] includes emoji in title 
- [X] ~adds~ updates unit tests
- [ ] adds or updates e2e tests

**Release note**:
<!--  Write your release note:
1. Enter your extended release note in the below block. If the PR requires additional action from users switching to the new release, include the string "action required".
2. If no release note is required, just write "NONE".
-->
```release-note
Enable NLB target group connection draining to allow for graceful shutdown of apiserver processes  
```


Patch:
diff --git a/api/v1beta2/network_types.go b/api/v1beta2/network_types.go
index 9974cbddb..00a775a54 100644
--- a/api/v1beta2/network_types.go
+++ b/api/v1beta2/network_types.go
@@ -207,6 +207,14 @@ type TargetGroupAttribute string
 var (
 	// TargetGroupAttributeEnablePreserveClientIP defines the attribute key for enabling preserve client IP.
 	TargetGroupAttributeEnablePreserveClientIP = "preserve_client_ip.enabled"
+
+	// TargetGroupAttributeEnableConnectionTermination defines the attribute key for terminating
+	// established connections to unhealthy targets.
+	TargetGroupAttributeEnableConnectionTermination = "target_health_state.unhealthy.connection_termination.enabled"
+
+	// TargetGroupAttributeUnhealthyDrainingIntervalSeconds defines the attribute key for the
+	// unhealthy target connection draining interval.
+	TargetGroupAttributeUnhealthyDrainingIntervalSeconds = "target_health_state.unhealthy.draining_interval_seconds"
 )
 
 // LoadBalancerAttribute defines a set of attributes for a V2 load balancer.
diff --git a/pkg/cloud/services/elb/loadbalancer.go b/pkg/cloud/services/elb/loadbalancer.go
index 25aefa9b4..6887692dd 100644
--- a/pkg/cloud/services/elb/loadbalancer.go
+++ b/pkg/cloud/services/elb/loadbalancer.go
@@ -1687,16 +1687,32 @@ func (s *Service) reconcileTargetGroupsAndListeners(ctx context.Context, lbARN s
 			}
 			createdTargetGroups = append(createdTargetGroups, group)
 
+			targetGroupAttributeInput := &elbv2.ModifyTargetGroupAttributesInput{TargetGroupArn: group.TargetGroupArn}
+
+			if lbSpec.LoadBalancerType == infrav1.LoadBalancerTypeNLB {
+				targetGroupAttributeInput.Attributes = append(targetGroupAttributeInput.Attributes,
+					elbv2types.TargetGroupAttribute{
+						Key:   aws.String(infrav1.TargetGroupAttributeEnableConnectionTermination),
+						Value: aws.String("false"),
+					},
+					elbv2types.TargetGroupAttribute{
+						Key:   aws.String(infrav1.TargetGroupAttributeUnhealthyDrainingIntervalSeconds),
+						Value: aws.String("300"),
+					},
+				)
+			}
+
 			if !lbSpec.PreserveClientIP {
-				targetGroupAttributeInput := &elbv2.ModifyTargetGroupAttributesInput{
-					TargetGroupArn: group.TargetGroupArn,
-					Attributes: []elbv2types.TargetGroupAttribute{
-						{
-							Key:   aws.String(infrav1.TargetGroupAttributeEnablePreserveClientIP),
-							Value: aws.String("false"),
-						},
+				targetGroupAttributeInput.Attributes = append(targetGroupAttributeInput.Attributes,
+					elbv2types.TargetGroupAttribute{
+						Key:   aws.String(infrav1.TargetGroupAttributeEnablePreserveClientIP),
+						Value: aws.String("false"),
 					},
-				}
+				)
+			}
+
+			if len(targetGroupAttributeInput.Attributes) > 0 {
+				s.scope.Debug("configuring target group attributes", "attributes", targetGroupAttributeInput)
 				if _, err := s.ELBV2Client.ModifyTargetGroupAttributes(ctx, targetGroupAttributeInput); err != nil {
 					return nil, nil, errors.Wrapf(err, "failed to modify target group attribute")
 				}


Test patch:
diff --git a/controllers/helpers_test.go b/controllers/helpers_test.go
index d68669c40..05f103cfb 100644
--- a/controllers/helpers_test.go
+++ b/controllers/helpers_test.go
@@ -393,6 +393,14 @@ func mockedModifyTargetGroupAttributes(t *testing.T, m *mocks.MockELBV2APIMockRe
 	m.ModifyTargetGroupAttributes(gomock.Any(), gomock.Eq(&elbv2.ModifyTargetGroupAttributesInput{
 		TargetGroupArn: tgArn,
 		Attributes: []elbv2types.TargetGroupAttribute{
+			{
+				Key:   aws.String(infrav1.TargetGroupAttributeEnableConnectionTermination),
+				Value: aws.String("false"),
+			},
+			{
+				Key:   aws.String(infrav1.TargetGroupAttributeUnhealthyDrainingIntervalSeconds),
+				Value: aws.String("300"),
+			},
 			{
 				Key:   aws.String(infrav1.TargetGroupAttributeEnablePreserveClientIP),
 				Value: aws.String("false"),
diff --git a/pkg/cloud/services/elb/loadbalancer_test.go b/pkg/cloud/services/elb/loadbalancer_test.go
index 805167b3b..5bf8baae0 100644
--- a/pkg/cloud/services/elb/loadbalancer_test.go
+++ b/pkg/cloud/services/elb/loadbalancer_test.go
@@ -1543,6 +1543,14 @@ func TestReconcileTargetGroupsAndListeners(t *testing.T) {
 				m.ModifyTargetGroupAttributes(gomock.Any(), gomock.Eq(&elbv2.ModifyTargetGroupAttributesInput{
 					TargetGroupArn: aws.String(tgArn),
 					Attributes: []elbv2types.TargetGroupAttribute{
+						{
+							Key:   aws.String(infrav1.TargetGroupAttributeEnableConnectionTermination),
+							Value: aws.String("false"),
+						},
+						{
+							Key:   aws.String(infrav1.TargetGroupAttributeUnhealthyDrainingIntervalSeconds),
+							Value: aws.String("300"),
+						},
 						{
 							Key:   aws.String(infrav1.TargetGroupAttributeEnablePreserveClientIP),
 							Value: aws.String("false"),
@@ -1661,6 +1669,14 @@ func TestReconcileTargetGroupsAndListeners(t *testing.T) {
 				m.ModifyTargetGroupAttributes(gomock.Any(), &elbv2.ModifyTargetGroupAttributesInput{
 					TargetGroupArn: aws.String(tgArn),
 					Attributes: []elbv2types.TargetGroupAttribute{
+						{
+							Key:   aws.String(infrav1.TargetGroupAttributeEnableConnectionTermination),
+							Value: aws.String("false"),
+						},
+						{
+							Key:   aws.String(infrav1.TargetGroupAttributeUnhealthyDrainingIntervalSeconds),
+							Value: aws.String("300"),
+						},
 						{
 							Key:   aws.String(infrav1.TargetGroupAttributeEnablePreserveClientIP),
 							Value: aws.String("false"),
@@ -1767,6 +1783,14 @@ func TestReconcileTargetGroupsAndListeners(t *testing.T) {
 				m.ModifyTargetGroupAttributes(gomock.Any(), &elbv2.ModifyTargetGroupAttributesInput{
 					TargetGroupArn: aws.String(tgArn),
 					Attributes: []elbv2types.TargetGroupAttribute{
+						{
+							Key:   aws.String(infrav1.TargetGroupAttributeEnableConnectionTermination),
+							Value: aws.String("false"),
+						},
+						{
+							Key:   aws.String(infrav1.TargetGroupAttributeUnhealthyDrainingIntervalSeconds),
+							Value: aws.String("300"),
+						},
 						{
 							Key:   aws.String(infrav1.TargetGroupAttributeEnablePreserveClientIP),
 							Value: aws.String("false"),
@@ -1860,6 +1884,19 @@ func TestReconcileTargetGroupsAndListeners(t *testing.T) {
 						},
 					},
 				}, nil)
+				m.ModifyTargetGroupAttributes(gomock.Any(), gomock.Eq(&elbv2.ModifyTargetGroupAttributesInput{
+					TargetGroupArn: aws.String(tgArn),
+					Attributes: []elbv2types.TargetGroupAttribute{
+						{
+							Key:   aws.String(infrav1.TargetGroupAttributeEnableConnectionTermination),
+							Value: aws.String("false"),
+						},
+						{
+							Key:   aws.String(infrav1.TargetGroupAttributeUnhealthyDrainingIntervalSeconds),
+							Value: aws.String("300"),
+						},
+					},
+				})).Return(nil, nil)
 				m.DescribeListeners(gomock.Any(), &elbv2.DescribeListenersInput{
 					LoadBalancerArn: aws.String(elbArn),
 				}).Return(&elbv2.DescribeListenersOutput{
@@ -2000,6 +2037,14 @@ func TestReconcileTargetGroupsAndListeners(t *testing.T) {
 				m.ModifyTargetGroupAttributes(gomock.Any(), &elbv2.ModifyTargetGroupAttributesInput{
 					TargetGroupArn: aws.String(tgArn),
 					Attributes: []elbv2types.TargetGroupAttribute{
+						{
+							Key:   aws.String(infrav1.TargetGroupAttributeEnableConnectionTermination),
+							Value: aws.String("false"),
+						},
+						{
+							Key:   aws.String(infrav1.TargetGroupAttributeUnhealthyDrainingIntervalSeconds),
+							Value: aws.String("300"),
+						},
 						{
 							Key:   aws.String(infrav1.TargetGroupAttributeEnablePreserveClientIP),
 							Value: aws.String("false"),
@@ -2115,6 +2160,14 @@ func TestReconcileTargetGroupsAndListeners(t *testing.T) {
 				m.ModifyTargetGroupAttributes(gomock.Any(), &elbv2.ModifyTargetGroupAttributesInput{
 					TargetGroupArn: aws.String(tgArn),
 					Attributes: []elbv2types.TargetGroupAttribute{
+						{
+							Key:   aws.String(infrav1.TargetGroupAttributeEnableConnectionTermination),
+							Value: aws.String("false"),
+						},
+						{
+							Key:   aws.String(infrav1.TargetGroupAttributeUnhealthyDrainingIntervalSeconds),
+							Value: aws.String("300"),
+						},
 						{
 							Key:   aws.String(infrav1.TargetGroupAttributeEnablePreserveClientIP),
 							Value: aws.String("false"),
@@ -2403,6 +2456,14 @@ func TestReconcileV2LB(t *testing.T) {
 				m.ModifyTargetGroupAttributes(gomock.Any(), gomock.Eq(&elbv2.ModifyTargetGroupAttributesInput{
 					TargetGroupArn: aws.String(tgArn),
 					Attributes: []elbv2types.TargetGroupAttribute{
+						{
+							Key:   aws.String(infrav1.TargetGroupAttributeEnableConnectionTermination),
+							Value: aws.String("false"),
+						},
+						{
+							Key:   aws.String(infrav1.TargetGroupAttributeUnhealthyDrainingIntervalSeconds),
+							Value: aws.String("300"),
+						},
 						{
 							Key:   aws.String(infrav1.TargetGroupAttributeEnablePreserveClientIP),
 							Value: aws.String("false"),