Skip to content

fix(installer): trim EKS cluster name from both ARN and eksctl formats #39500

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 9, 2025
Merged

fix(installer): trim EKS cluster name from both ARN and eksctl formats #39500

merged 1 commit into from
Jun 9, 2025

Conversation

zzuckerfrei
Copy link
Contributor

Clusters created with eksctl often use a kube-context like <cluster-name>.<region>.eksctl.io,
which was previously not correctly trimmed during auto-detection.

This led to validation errors such as:
cluster name must not be more than 32 characters.

This commit introduces trimEKSClusterName() to correctly handle both:

  • ARN format: arn:aws:eks:<region>:<account>:cluster/<name>

  • eksctl FQDN format: <name>.<region>.eksctl.io

Changes include:

  • Applying trimming logic in autodetectAndValidate() before replacing disallowed characters (_, ., :)

  • Adding unit tests for edge cases across both formats

This ensures consistent and robust cluster name parsing across different EKS setups.

Fixes: #39482

Improve EKS cluster name auto-detection by supporting both ARN and eksctl FQDN formats to avoid validation errors caused by overly long names.

@zzuckerfrei zzuckerfrei requested review from a team as code owners May 12, 2025 14:06
@zzuckerfrei zzuckerfrei requested a review from squeed May 12, 2025 14:06
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 12, 2025
@github-actions github-actions bot added cilium-cli This PR contains changes related with cilium-cli cilium-cli-exclusive This PR only impacts cilium-cli binary kind/community-contribution This was a contribution made by a community member. labels May 12, 2025
squeed
squeed reviewed May 19, 2025
View reviewed changes
squeed
squeed reviewed May 19, 2025
View reviewed changes
@squeed
Copy link
Contributor

squeed commented May 19, 2025 •
edited
Loading

Looks reasonable! Just a few small requests, nothing exciting. Thanks for the PR!

(feel free to re-request review, otherwise it gets lost in the noise.)

@zzuckerfrei
Copy link
Contributor Author

Looks reasonable! Just a few small requests, nothing exciting. Thanks for the PR!

(feel free to re-request review, otherwise it gets lost in the noise.)

Thanks for the review! 🙏

I'll update the prefix to :cluster/ and add a docblock for trimEKSClusterName() as suggested.

@zzuckerfrei zzuckerfrei requested a review from squeed May 20, 2025 12:10
@zzuckerfrei
Copy link
Contributor Author

I've updated the prefix and added the docblock as suggested. Re-requesting review now 🙇‍♂️

@squeed squeed added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label May 21, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 21, 2025
@squeed
Copy link
Contributor

squeed commented May 21, 2025

Looks good. Image build failure looks like a blip, I'm re-triggering everything.

@squeed
Copy link
Contributor

squeed commented May 21, 2025

/test

@julianwiedmann
Copy link
Member

Looking at the CI results, this will need a rebase (to pick up some multi-pool IPAM changes that are tested by the workflow). Will do so now ...

@julianwiedmann julianwiedmann force-pushed the fix/eks-trimEKSClusterName branch from 3b56978 to 832687d Compare May 21, 2025 13:56
@julianwiedmann
Copy link
Member

/test

@julianwiedmann julianwiedmann enabled auto-merge May 21, 2025 14:30
@squeed
Copy link
Contributor

squeed commented May 21, 2025

The unit test you added is failing:

--- FAIL: Test_trimEKSClusterName (0.00s)

    autodetect_test.go:43: 
        	Error Trace:	/home/runner/work/cilium/cilium/cilium-cli/install/autodetect_test.go:43
        	Error:      	Not equal: 
        	            	expected: "my-cluster"
        	            	actual  : "cluster/my-cluster"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-my-cluster
        	            	+cluster/my-cluster
        	Test:       	Test_trimEKSClusterName

@zzuckerfrei
Copy link
Contributor Author

Hi, I noticed a test case failed after changing the prefix from "cluster/" to ":cluster/".

The failed test case uses "cluster/my-cluster" as the cluster name, which doesn’t seem to reflect a common EKS naming pattern.

Given this, I’m considering a few possible directions:

  1. Update the test case to use a more realistic EKS-style input, such as an ARN (e.g., arn:aws:eks:...:cluster/my-cluster) or an eksctl FQDN (e.g., my-cluster.region.eksctl.io).

  2. Reconsider the prefix change, and possibly go back to using "cluster/" if that better supports legacy or edge cases.

  3. Remove the failing test case if we think the input format is too unlikely to support.

I’d really appreciate your thoughts on which direction would be best here. 🙂

The unit test you added is failing:

--- FAIL: Test_trimEKSClusterName (0.00s)

    autodetect_test.go:43: 
        	Error Trace:	/home/runner/work/cilium/cilium/cilium-cli/install/autodetect_test.go:43
        	Error:      	Not equal: 
        	            	expected: "my-cluster"
        	            	actual  : "cluster/my-cluster"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-my-cluster
        	            	+cluster/my-cluster
        	Test:       	Test_trimEKSClusterName

@squeed
Copy link
Contributor

squeed commented May 26, 2025

@zzuckerfrei let's update the test case to reflect a real cluster naming convention :-). But great question.

auto-merge was automatically disabled May 29, 2025 05:25

Head branch was pushed to by a user without write access

@zzuckerfrei zzuckerfrei requested review from a team as code owners May 29, 2025 05:25
@zzuckerfrei
Copy link
Contributor Author

Seems like the unrelated groups/reviewers are added due to some reasons (e.g. wrong target branch, out of date local, etc), I just removed all, and re-add cilium-cli group

Hi, sorry about the noise earlier — I mistakenly pushed a broken rebase that included a huge number of unrelated changes and reviewers.

Thanks a lot for cleaning up the reviewers list and for your patience!

@zzuckerfrei zzuckerfrei reopened this May 29, 2025
@zzuckerfrei zzuckerfrei requested a review from squeed May 29, 2025 06:19
@tklauser
Copy link
Member

tklauser commented Jun 3, 2025

/test

@tklauser tklauser enabled auto-merge June 3, 2025 08:53
tklauser
tklauser requested changes Jun 3, 2025
View reviewed changes
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zzuckerfrei could you please squash the three commits into a single one? Other than that LGTM.

auto-merge was automatically disabled June 5, 2025 09:01

Head branch was pushed to by a user without write access

@zzuckerfrei zzuckerfrei force-pushed the fix/eks-trimEKSClusterName branch from 1f773bf to fd448eb Compare June 5, 2025 09:01
@zzuckerfrei zzuckerfrei requested a review from tklauser June 5, 2025 09:04
@tklauser
Copy link
Member

tklauser commented Jun 5, 2025

/test

@tklauser tklauser enabled auto-merge June 5, 2025 09:07
tklauser
tklauser approved these changes Jun 5, 2025
View reviewed changes
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@tklauser
Copy link
Member

tklauser commented Jun 6, 2025

Rebased to pull in fixes for the CI failures.

@zzuckerfrei @tklauser
fix(installer): trim EKS cluster name from both ARN and eksctl formats
e6f5f2c 
Clusters created with eksctl often use a context name like <cluster-name>.<region>.eksctl.io,
which previously wasn't correctly trimmed during auto-detection. This could cause validation errors
such as 'cluster name must not be more than 32 characters'.

This commit introduces a new helper function trimEKSClusterName() to correctly handle:
- ARN format: arn:aws:eks:<region>:<account>:cluster/<name>
- eksctl FQDN format: <name>.<region>.eksctl.io

Also includes unit tests to validate this logic:
- Removed unrealistic cluster/my-cluster input
- Added examples using ARN-style names and eksctl FQDNs
- Maintains edge case coverage for invalid and empty inputs

Signed-off-by: zzuckerfrei <seonmin1219@gmail.com>
@tklauser tklauser force-pushed the fix/eks-trimEKSClusterName branch from fd448eb to e6f5f2c Compare June 6, 2025 16:11
@tklauser
Copy link
Member

tklauser commented Jun 6, 2025

/test

1 similar comment
@tklauser
Copy link
Member

tklauser commented Jun 7, 2025

/test

@zzuckerfrei
Copy link
Contributor Author

Hi, @tklauser,

Thank you so much for the rebase and all your help with this PR! 🙏

I noticed there are still some CI failures. Is there anything I can do on my end to help resolve them, or should I wait for the infrastructure issues to be resolved?

Really appreciate your time and patience!

@tklauser
Copy link
Member

tklauser commented Jun 9, 2025

I noticed there are still some CI failures. Is there anything I can do on my end to help resolve them, or should I wait for the infrastructure issues to be resolved?

The failures look unrelated to this change and are likely flaky tests. I've re-triggered the failing workflows.

@tklauser tklauser added this pull request to the merge queue Jun 9, 2025
Merged via the queue into cilium:main with commit 6809fca Jun 9, 2025
68 checks passed
@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Jun 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tklauser tklauser tklauser approved these changes

@squeed squeed squeed approved these changes

Assignees
No one assigned
Labels
cilium-cli This PR contains changes related with cilium-cli cilium-cli-exclusive This PR only impacts cilium-cli binary kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cilium-cli fails to detect EKS cluster name with eksctl.io context format
5 participants
@zzuckerfrei @squeed @julianwiedmann @sayboras @tklauser