aws-node-termination-handler improvements from review (#957)

giantswarm · Dec 10, 2024 · 7914eef · 7914eef
1 parent fce64d8
commit 7914eef
Show file tree

Hide file tree

Showing 7 changed files with 47 additions and 17 deletions.
diff --git a/helm/cluster-aws/README.md b/helm/cluster-aws/README.md
@@ -24,7 +24,6 @@ Properties within the `.global.providerSpecific` object
 | `global.providerSpecific.ami` | **Amazon machine image (AMI)** - If specified, this image will be used to provision EC2 instances.|**Type:** `string`<br/>|
 | `global.providerSpecific.awsAccountId` | **AWS Account ID** - Only used when rendering the chart template locally, you shouldn't use this value. Used to calculate the IRSA service account issuer when using the China region.|**Type:** `string`<br/>|
 | `global.providerSpecific.awsClusterRoleIdentityName` | **Cluster role identity name** - Name of an AWSClusterRoleIdentity object. Learn more at https://docs.giantswarm.io/getting-started/cloud-provider-accounts/cluster-api/aws/#configure-the-awsclusterroleidentity .|**Type:** `string`<br/>**Value pattern:** `^[-a-zA-Z0-9_\.]{1,63}$`<br/>**Default:** `"default"`|
-| `global.providerSpecific.awsPartition` | **AWS Partition** - Only used when rendering the chart template locally, you shouldn't use this value.|**Type:** `string`<br/>|
 | `global.providerSpecific.flatcarAwsAccount` | **AWS account owning Flatcar image** - AWS account ID owning the Flatcar Container Linux AMI.|**Type:** `string`<br/>**Default:** `"706635527432"`|
 | `global.providerSpecific.instanceMetadataOptions` | **Instance metadata options** - Instance metadata options for the EC2 instances in the cluster.|**Type:** `object`<br/>|
 | `global.providerSpecific.instanceMetadataOptions.httpTokens` | **HTTP tokens** - The state of token usage for your instance metadata requests. If you set this parameter to `optional`, you can use either IMDSv1 or IMDSv2. If you set this parameter to `required`, you must use a IMDSv2 to access the instance metadata endpoint. Learn more at [What’s new in IMDSv2](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/configuring-instance-metadata-service.html).|**Type:** `string`<br/>**Default:** `"required"`|
@@ -372,6 +371,7 @@ For Giant Swarm internal use only, not stable, or not supported by UIs.
 
 | **Property** | **Description** | **More Details** |
 | :----------- | :-------------- | :--------------- |
+| `internal.awsPartition` | **AWS Partition** - Only used when rendering the chart template locally, you shouldn't use this value.|**Type:** `string`<br/>|
 | `internal.hashSalt` | **Hash salt** - If specified, this token is used as a salt to the hash suffix of some resource names. Can be used to force-recreate some resources.|**Type:** `string`<br/>|
 
 ### Metadata

diff --git a/helm/cluster-aws/ci/ci-values.yaml b/helm/cluster-aws/ci/ci-values.yaml
@@ -16,7 +16,6 @@ global:
   providerSpecific:
     region: "eu-west-1"
     awsAccountId: "1234567890"
-    awsPartition: "aws"
   components:
     containerd:
       containerRegistries:
@@ -27,6 +26,9 @@ global:
             password: abcdef
         - endpoint: quay.io
 
+internal:
+  awsPartition: "aws"
+
 cluster:
   internal:
     ephemeralConfiguration:

diff --git a/helm/cluster-aws/ci/test-local-registry-cache-values.yaml b/helm/cluster-aws/ci/test-local-registry-cache-values.yaml
@@ -16,7 +16,6 @@ global:
   providerSpecific:
     region: "eu-west-1"
     awsAccountId: "1234567890"
-    awsPartition: "aws"
   managementCluster: test
   components:
     containerd:
@@ -30,6 +29,9 @@ global:
           - docker.io
         port: 32767
 
+internal:
+  awsPartition: "aws"
+
 cluster:
   internal:
     ephemeralConfiguration:

diff --git a/helm/cluster-aws/ci/test-multiple-authenticated-mirrors-values.yaml b/helm/cluster-aws/ci/test-multiple-authenticated-mirrors-values.yaml
@@ -16,7 +16,6 @@ global:
   providerSpecific:
     region: "eu-west-1"
     awsAccountId: "1234567890"
-    awsPartition: "aws"
   managementCluster: test
   components:
     containerd:
@@ -32,6 +31,9 @@ global:
               username: example
               password: password
 
+internal:
+  awsPartition: "aws"
+
 cluster:
   internal:
     ephemeralConfiguration:

diff --git a/helm/cluster-aws/templates/_awspartition.tpl b/helm/cluster-aws/templates/_awspartition.tpl
@@ -12,7 +12,7 @@ Output: The AWS partition (e.g., "aws", "aws-cn")
 
 {{- define "aws-partition" -}}
 {{- $roleName := .Values.global.providerSpecific.awsClusterRoleIdentityName -}}
-{{- $partition := .Values.global.providerSpecific.awsPartition -}}
+{{- $partition := .Values.internal.awsPartition -}}
 {{- $role := (lookup "infrastructure.cluster.x-k8s.io/v1beta2" "AWSClusterRoleIdentity" "" $roleName) -}}
 {{- if $role -}}
 {{- $partition = (include "extractAWSPartition" $role.spec.roleARN) -}}

diff --git a/helm/cluster-aws/templates/aws-nth-helmrelease.yaml b/helm/cluster-aws/templates/aws-nth-helmrelease.yaml
@@ -1,6 +1,31 @@
 {{/* Default Helm values for the app */}}
-{{/* See schema for the appropriate app version here https://github.com/giantswarm/aws-ebs-csi-driver-app/blob/master/helm/aws-ebs-csi-driver-app/values.schema.json */}}
+{{/* See schema for the appropriate app version here https://github.com/giantswarm/aws-nth-bundle/blob/main/helm/aws-nth-bundle/values.schema.json */}}
 {{- define "defaultAwsNodeTerminationHandlerHelmValues" }}
+awsNodeTerminationHandler:
+  values:
+    image:
+      registry: {{ include "awsContainerImageRegistry" $ }}
+
+    # Allow running on control plane nodes. On deletion, CAPI will first delete the worker nodes
+    # and we still want aws-node-termination-handler, if it's even still running and the HelmRelease
+    # not deleted yet, to take care of the last workers' EC2 lifecycle hooks since they otherwise
+    # won't be completed, resulting in unnecessary waiting time before AWS can terminate the
+    # instances (see `AWSMachinePool.spec.lifecycleHooks["aws-node-termination-handler"].heartbeatTimeout`).
+    # This runs on workers by default but allows moving pods to control plane nodes. Requires
+    # queue processing mode i.e. running as `Deployment`, not `DaemonSet`.
+    affinity:
+      nodeAffinity:
+        preferredDuringSchedulingIgnoredDuringExecution:
+          - preference:
+              matchExpressions:
+                - key: node-role.kubernetes.io/control-plane
+                  operator: DoesNotExist
+            weight: 10
+    tolerations:
+      - effect: NoSchedule
+        operator: Exists
+        key: node-role.kubernetes.io/control-plane
+
 clusterID: {{ include "resource.default.name" $ }}
 {{- if (.Values.global.connectivity.proxy).enabled }}
 proxy:
@@ -25,7 +50,6 @@ metadata:
     cluster-apps-operator.giantswarm.io/watching: ""
     {{- include "labels.common" . | nindent 4 }}
 spec:
-  suspend: false # It can be unsuspended by the post-install/post-upgrade hook. Useful if we need to populate some fields later on.
   releaseName: aws-nth-bundle
   chart:
     spec:
@@ -42,13 +66,13 @@ spec:
   install:
     remediation:
       retries: 30
-  {{- $AwsNodeTerminationHandlerHelmValues := (include "defaultAwsNodeTerminationHandlerHelmValues" .) | fromYaml -}}
+  {{- $awsNodeTerminationHandlerHelmValues := (include "defaultAwsNodeTerminationHandlerHelmValues" .) | fromYaml -}}
   {{- $customAwsNodeTerminationHandlerHelmValues := $.Values.global.apps.awsNodeTerminationHandler.values -}}
   {{- if $customAwsNodeTerminationHandlerHelmValues }}
-  {{- $AwsNodeTerminationHandlerHelmValues = merge (deepCopy $customAwsNodeTerminationHandlerHelmValues) $AwsNodeTerminationHandlerHelmValues -}}
+  {{- $awsNodeTerminationHandlerHelmValues = merge (deepCopy $customAwsNodeTerminationHandlerHelmValues) $awsNodeTerminationHandlerHelmValues -}}
   {{- end }}
-  {{- if $AwsNodeTerminationHandlerHelmValues }}
-  values: {{- $AwsNodeTerminationHandlerHelmValues | toYaml | nindent 4 }}
+  {{- if $awsNodeTerminationHandlerHelmValues }}
+  values: {{- $awsNodeTerminationHandlerHelmValues | toYaml | nindent 4 }}
   {{- end }}
   {{- if $.Values.global.apps.awsNodeTerminationHandler.extraConfigs }}
   valuesFrom:

diff --git a/helm/cluster-aws/values.schema.json b/helm/cluster-aws/values.schema.json
@@ -711,7 +711,7 @@
                         "awsNodeTerminationHandler": {
                             "$ref": "#/$defs/helmRelease",
                             "type": "object",
-                            "title": "AWS EBS CSI driver",
+                            "title": "AWS Node Termination Handler",
                             "description": "Configuration of aws-nth-bundle. For all available values see https://github.com/giantswarm/aws-nth-bundle."
                         },
                         "awsPodIdentityWebhook": {
@@ -1756,11 +1756,6 @@
                             "minLength": 1,
                             "pattern": "^[-a-zA-Z0-9_\\.]{1,63}$"
                         },
-                        "awsPartition": {
-                            "type": "string",
-                            "title": "AWS Partition",
-                            "description": "Only used when rendering the chart template locally, you shouldn't use this value."
-                        },
                         "flatcarAwsAccount": {
                             "type": "string",
                             "title": "AWS account owning Flatcar image",
@@ -1812,6 +1807,11 @@
             "title": "Internal",
             "description": "For Giant Swarm internal use only, not stable, or not supported by UIs.",
             "properties": {
+                "awsPartition": {
+                    "type": "string",
+                    "title": "AWS Partition",
+                    "description": "Only used when rendering the chart template locally, you shouldn't use this value."
+                },
                 "hashSalt": {
                     "type": "string",
                     "title": "Hash salt",