✨ Support EKS upgrade policy #5471

phuhung273 · 2025-04-18T08:49:46Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Support EKS upgrade policy

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 #5183

Special notes for your reviewer:

Checklist:

Release note:

Support EKS upgrade policy

k8s-ci-robot · 2025-04-18T08:49:55Z

Welcome @phuhung273!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-aws 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-aws has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-04-18T08:49:56Z

Hi @phuhung273. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

phuhung273 · 2025-05-12T11:42:01Z

Hi @richardcase, can you please help me review this PR if you have time

richardcase · 2025-05-15T15:05:12Z

/ok-to-test

richardcase

Thanks for this @phuhung273 .

Feel free to ping me on the issue or in slack if you want a hand with anything.

richardcase · 2025-05-15T15:27:24Z

controlplane/eks/api/v1beta2/awsmanagedcontrolplane_types.go

+	// The default value is EXTENDED. Use STANDARD to disable extended support.
+	// +kubebuilder:validation:Enum=EXTENDED;STANDARD
+	// +optional
+	UpgradePolicy *string `json:"upgradePolicy,omitempty"`


We generally use a string type alias for something like this and then variables defined for each supported value. Like this: https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/main/controlplane/eks/api/v1beta2/types.go#L147-L157.

Also, the actual string values are normally lowercase so extended instead of EXTENDED. This does mean that when creating the AWS request that some conversion is done, like this for addons: https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/main/pkg/cloud/services/eks/addons.go#L212

Thank you, I didn't know this. Updated to use string type alias.

richardcase · 2025-05-15T15:32:17Z

pkg/cloud/services/eks/cluster.go

+	}
+
+	if err := wait.WaitForWithRetryable(wait.NewBackoff(), func() (bool, error) {
+		if _, err := s.EKSClient.UpdateClusterConfig(input); err != nil {


There is already a UpdateClusterConfig call in the reconciler, it would be better to use this instead of having a new separate update. https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/main/pkg/cloud/services/eks/cluster.go#L543

Sure, updated to use current UpdateClusterConfig call

phuhung273 · 2025-05-21T10:59:25Z

Hi @richardcase, if you have time, please help me review the latest update

phuhung273 · 2025-06-16T13:17:20Z

Hi @richardcase, can you help me review this

phuhung273 · 2025-08-13T11:46:32Z

Hi @richardcase, can you help review the change addressing your comments.

damdo · 2025-09-04T08:07:39Z

controlplane/eks/api/v1beta2/awsmanagedcontrolplane_types.go

+	// The support policy to use for the cluster.
+	// Extended support indicates that the cluster will not be automatically upgraded
+	// when it leaves the standard support period, and will enter extended support.
+	// Clusters in extended support have higher costs.
+	// The default value is extended. Use standard to disable extended support.
+	// +kubebuilder:validation:Enum=extended;standard
+	// +optional
+	UpgradePolicy *UpgradePolicy `json:"upgradePolicy,omitempty"`


Does it have to be a pointer? Considering it is marked as optional If we omit it it'll be empty right?
Then in that case we can default it to extended.

Yes agree. As this is a type alias for a string, we can do without the pointer.

In case current cluster is STANDARD, eg: by workaround since CAPA doesn't support yet. Defaulting to EXTENDED will suddenly switch all clusters to EXTENDED, increasing the cost.

There is a testcase for this situation: if the field is nil, don't do anything https://github.com/phuhung273/cluster-api-provider-aws/blob/00efef15c7cb5263a6409d2b998f8583d5e0d1bd/pkg/cloud/services/eks/cluster_test.go#L703-L710

Do you think it is a valid case ?

What is the default in the SDK for EKS if nothing is specified as a policy at the moment? If the default is "standard" then we should keep standard as default here IMO. And that wouldn't cause the issue you are talking about

By default:

CreateCluster is EXTENDED

UpdateCluster doesn't touch that param if not specified.

The case im mentioning is when user have other stuff managing clusters on top of CAPA.

Yes I agree. In that case we can add a comment that explains how CAPA will behave when the user omits the value or sets it to "".
i.e. It will let the AWS Platform choose whatever default value is there for the upgrade policy (at the time of writing Extended, but this may change in the future).

Seems like we have an agreement on: Extended, Standard and omitted (""). Thank you both.

I think for new clusters we should explicitly state what the default is in our API.

Regarding to this point, we should explicitly set EXTENDED in createCluster if user ommit rite ?

Regarding to this point, we should explicitly set EXTENDED in createCluster if user ommit rite ?

We should omit setting it in createCluster if the user omits it

Thanks for the confirmation, let me update the PR

I've updated to: Extended, Standard or omitted (""). PTAL @damdo @richardcase, thanks.

richardcase

Looks good @phuhung273 .

It would also be worth adding a line to the docs so state that by default we create a cluster with "extended" support, perhaps somehwre here: https://cluster-api-aws.sigs.k8s.io/topics/eks/creating-a-cluster

richardcase · 2025-09-04T08:19:38Z

controlplane/eks/api/v1beta2/awsmanagedcontrolplane_types.go

+	// The support policy to use for the cluster.
+	// Extended support indicates that the cluster will not be automatically upgraded
+	// when it leaves the standard support period, and will enter extended support.
+	// Clusters in extended support have higher costs.
+	// The default value is extended. Use standard to disable extended support.
+	// +kubebuilder:validation:Enum=extended;standard
+	// +optional
+	UpgradePolicy *UpgradePolicy `json:"upgradePolicy,omitempty"`


Yes agree. As this is a type alias for a string, we can do without the pointer.

richardcase · 2025-09-04T08:22:08Z

pkg/cloud/services/eks/cluster.go

 	return true, nil
 }
+
+func convertUpgradePolicy(input ekscontrolplanev1.UpgradePolicy) ekstypes.SupportType {


We sometimes but these in the "convert" package when converting to/from SDK types.

Sure, can see a similar converter package. I moved this function there.

phuhung273 · 2025-09-05T02:14:05Z

/test pull-cluster-api-provider-aws-e2e-blocking

phuhung273 · 2025-09-24T23:13:09Z

/test pull-cluster-api-provider-aws-e2e

phuhung273 · 2025-09-25T00:40:11Z

/test pull-cluster-api-provider-aws-e2e

phuhung273 · 2025-09-25T00:44:23Z

/test pull-cluster-api-provider-aws-e2e

phuhung273 · 2025-09-25T00:49:09Z

Look like test infra issue

phuhung273 · 2025-09-25T05:35:59Z

/test pull-cluster-api-provider-aws-e2e

damdo · 2025-09-25T10:02:56Z

/test pull-cluster-api-provider-aws-e2e

phuhung273 · 2025-09-25T14:18:07Z

/test pull-cluster-api-provider-aws-e2e

damdo · 2025-09-25T14:27:28Z

@phuhung273 context for that error here: https://kubernetes.slack.com/archives/CD6U2V71N/p1758795545213209

phuhung273 · 2025-09-25T14:30:32Z

Context for that error here: https://kubernetes.slack.com/archives/CD6U2V71N/p1758795545213209

This one is spot instance test so if it is flaky i'm not surprised 😅 So i think it make sense to retry.

But this run https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_cluster-api-provider-aws/5471/pull-cluster-api-provider-aws-e2e/1971153449110212608 it failed 3 other cases, while the spot one you're referring passed.

damdo · 2025-09-25T14:43:53Z

Context for that error here: https://kubernetes.slack.com/archives/CD6U2V71N/p1758795545213209

This one is spot instance test so if it is flaky i'm not surprised 😅 So i think it make sense to retry.

But this run https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_cluster-api-provider-aws/5471/pull-cluster-api-provider-aws-e2e/1971153449110212608 it failed 3 other cases, while the spot one you're referring passed.

The underlying issue is the same one. If we fix that, we fix all of them. Let's keep chatting on that thread

damdo · 2025-09-26T06:53:53Z

/test pull-cluster-api-provider-aws-e2e

Co-authored-by: Damiano Donati <[email protected]>

…olplanes.yaml Co-authored-by: Faiq <[email protected]>

Co-authored-by: Faiq <[email protected]>

Co-authored-by: Damiano Donati <[email protected]>

phuhung273 · 2025-09-26T11:18:27Z

Just rebase to see if thing gets better.

phuhung273 · 2025-09-26T11:56:28Z

/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-eks

k8s-ci-robot · 2025-09-26T16:04:39Z

@phuhung273: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-aws-e2e	`52f04d0`	link	false	`/test pull-cluster-api-provider-aws-e2e`
pull-cluster-api-provider-aws-e2e-eks	`52f04d0`	link	false	`/test pull-cluster-api-provider-aws-e2e-eks`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 18, 2025

k8s-ci-robot requested review from Ankitasw and dlipovetsky April 18, 2025 08:49

k8s-ci-robot added the needs-priority label Apr 18, 2025

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 18, 2025

phuhung273 force-pushed the eks-upgrade-policy branch from 70e3003 to fd458b2 Compare April 18, 2025 08:54

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 15, 2025

richardcase reviewed May 15, 2025

View reviewed changes

phuhung273 force-pushed the eks-upgrade-policy branch from 24f8b66 to 353e345 Compare May 17, 2025 10:54

phuhung273 requested a review from richardcase May 17, 2025 16:13

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 4, 2025

phuhung273 force-pushed the eks-upgrade-policy branch from 4b49e38 to 00efef1 Compare June 15, 2025 11:19

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 15, 2025

damdo reviewed Sep 4, 2025

View reviewed changes

richardcase reviewed Sep 4, 2025

View reviewed changes

phuhung273 force-pushed the eks-upgrade-policy branch 3 times, most recently from b5b2dc7 to 12b6056 Compare September 4, 2025 17:20

phuhung273 and others added 15 commits September 26, 2025 18:14

Support EKS upgrade policy

ccbd674

Apply suggestions from code review

d0679f6

Co-authored-by: Damiano Donati <[email protected]>

regenerate

2fc1ddc

Apply suggestions from code review

3547892

Co-authored-by: Damiano Donati <[email protected]>

Update pkg/cloud/services/eks/cluster.go

6ed5b9f

Co-authored-by: Damiano Donati <[email protected]>

remove log

eb86dfc

Update config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontr…

7b7644b

…olplanes.yaml Co-authored-by: Faiq <[email protected]>

docstring typo

3747845

set NotReady if cluster was automatically upgraded

eae6822

Update pkg/cloud/services/eks/cluster.go

cf699a3

Co-authored-by: Faiq <[email protected]>

fix version compare logic

eadfcea

e2e

5b9d107

Apply suggestions from code review

cd93d4a

Co-authored-by: Damiano Donati <[email protected]>

syntax

5e9c0f2

WaitForEKSClusterUpgradePolicy fail early on NotFound

52f04d0

phuhung273 force-pushed the eks-upgrade-policy branch from a8f7d49 to 52f04d0 Compare September 26, 2025 11:16

✨ Support EKS upgrade policy #5471

Are you sure you want to change the base?

✨ Support EKS upgrade policy #5471

Conversation

phuhung273 commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Apr 18, 2025

Uh oh!

k8s-ci-robot commented Apr 18, 2025

Uh oh!

phuhung273 commented May 12, 2025

Uh oh!

richardcase commented May 15, 2025

Uh oh!

richardcase left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phuhung273 commented May 21, 2025

Uh oh!

phuhung273 commented Jun 16, 2025

Uh oh!

phuhung273 commented Aug 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phuhung273 Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phuhung273 Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damdo Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richardcase left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phuhung273 commented Sep 5, 2025

Uh oh!

phuhung273 commented Sep 24, 2025

Uh oh!

phuhung273 commented Sep 25, 2025

Uh oh!

phuhung273 commented Sep 25, 2025

Uh oh!

phuhung273 commented Sep 25, 2025

Uh oh!

phuhung273 commented Sep 25, 2025

Uh oh!

damdo commented Sep 25, 2025

phuhung273 commented Apr 18, 2025 •

edited

Loading

phuhung273 Sep 4, 2025 •

edited

Loading

phuhung273 Sep 4, 2025 •

edited

Loading

damdo Sep 5, 2025 •

edited

Loading

phuhung273 commented Sep 25, 2025 •

edited

Loading