From 7914eef1e4252b95981565b219f5723e48e7e8ea Mon Sep 17 00:00:00 2001 From: Andreas Sommer Date: Tue, 10 Dec 2024 10:05:29 +0100 Subject: [PATCH] aws-node-termination-handler improvements from review (#957) --- helm/cluster-aws/README.md | 2 +- helm/cluster-aws/ci/ci-values.yaml | 4 ++- .../ci/test-local-registry-cache-values.yaml | 4 ++- ...multiple-authenticated-mirrors-values.yaml | 4 ++- helm/cluster-aws/templates/_awspartition.tpl | 2 +- .../templates/aws-nth-helmrelease.yaml | 36 +++++++++++++++---- helm/cluster-aws/values.schema.json | 12 +++---- 7 files changed, 47 insertions(+), 17 deletions(-) diff --git a/helm/cluster-aws/README.md b/helm/cluster-aws/README.md index eac98192..1000bd65 100644 --- 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`
| | `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`
| | `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`
**Value pattern:** `^[-a-zA-Z0-9_\.]{1,63}$`
**Default:** `"default"`| -| `global.providerSpecific.awsPartition` | **AWS Partition** - Only used when rendering the chart template locally, you shouldn't use this value.|**Type:** `string`
| | `global.providerSpecific.flatcarAwsAccount` | **AWS account owning Flatcar image** - AWS account ID owning the Flatcar Container Linux AMI.|**Type:** `string`
**Default:** `"706635527432"`| | `global.providerSpecific.instanceMetadataOptions` | **Instance metadata options** - Instance metadata options for the EC2 instances in the cluster.|**Type:** `object`
| | `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`
**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`
| | `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`
| ### Metadata diff --git a/helm/cluster-aws/ci/ci-values.yaml b/helm/cluster-aws/ci/ci-values.yaml index 7c52a2da..6cb942d9 100644 --- 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 index ce0c8cd4..f033fc26 100644 --- 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 index 90831c87..d75896fb 100644 --- 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 index 1c7182dd..dfba44c4 100644 --- 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 index 18837494..c16ee9bf 100644 --- 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 index 8d1f38d3..1f262e59 100644 --- 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",