Fix ELB cleanup failure due to resource name end with a hyphen #7029

0xlen · 2023-08-31T01:23:41Z

Initiate the bug fix thread to mitigate the ELB cleanup issue due to incorrect resource name. (Fixes #7028)

Description

The current ELB cleanup logic doesn't handle the case when the trimmed ELB hostname string is ending with hyphen (-). This can cause the eksctl throw the error when making DescribeLoadBalancers API calls due to incorrect resource format. The issue can be reproduced by using the testing file [1] with the following error:

$ eksctl version
0.154.0
$ eksctl delete cluster --region us-west-2 --name eks
2023-08-31 00:44:39 [ℹ]  deleting EKS cluster "eks"
2023-08-31 00:44:40 [ℹ]  will drain 0 unmanaged nodegroup(s) in cluster "eks"
2023-08-31 00:44:40 [ℹ]  starting parallel draining, max in-flight of 1
2023-08-31 00:44:41 [ℹ]  deleted 0 Fargate profile(s)
2023-08-31 00:44:42 [ℹ]  cleaning up AWS load balancers created by Kubernetes objects of Kind Service or Ingress
Error: cannot obtain information for ALB from Ingress default/testcase-0: cannot obtain security groups for ALB k8s-default-testcase-93d8419948-: cannot describe ELB: operation error Elastic Load Balancing v2: DescribeLoadBalancers, https response error StatusCode: 400, RequestID: ......, api error ValidationError: The load balancer name 'k8s-default-testcase-93d8419948-' cannot end with a hyphen(-)

By fixing the resource name issue, the expected deletion can be successfully executed [2].

[1] https://gist.github.com/0xlen/d3bab9e46a8e797760e4731817543b28
[2] https://gist.github.com/0xlen/15cd7f43fca4cc8483e70ed7fc85bcb5

Checklist

Added tests that cover your change (if possible)
Added/modified documentation as required (such as the README.md, or the userdocs directory)
Manually tested
Made sure the title of the PR is a good description that can go into the release notes
(Core team) Added labels for change area (e.g. area/nodegroup) and kind (e.g. kind/improvement)

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Backfilled missing tests for code in same general area 🎉
Refactored something and made the world a better place 🌟

github-actions

Hello 0xlen 👋 Thank you for opening a Pull Request in eksctl project. The team will review the Pull Request and aim to respond within 1-10 business days. Meanwhile, please read about the Contribution and Code of Conduct guidelines here. You can find out more information about eksctl on our website

TiberiuGC · 2023-09-05T05:43:28Z

Hi @0xlen , thanks for opening a PR for this issue! 🚀

Before approving I wanted to confirm that trimming the ending - now properly covers all possible naming formats. Are you positive about that?

Fix the incorrect resource name as mentioned in the issue eksctl-io#7028 Refector the getting resource name logic from ELB hostname with unit test cases

0xlen · 2023-09-11T15:27:41Z

@TiberiuGC Thanks for the review. I found a bug for my original proposed fix so pushed an update again. I am positive right now after refactoring that part and added few unit test cases [1] (Real ELB test [2]). But I am not very sure if my proposal meets the criteria. Please let me know if you have any feedback.

[1] 5bcf25a#diff-581a28d52a0e2589ad7027966d2b578dfe8dce701181ce35e53c2d4b06f5b302
[2] https://gist.github.com/0xlen/15cd7f43fca4cc8483e70ed7fc85bcb5?permalink_comment_id=4688022#gistcomment-4688022

a-hilaly

Thank you @0xlen ! Left few questions/suggestions below.

pkg/elb/cleanup.go

pkg/elb/cleanup_test.go

pkg/elb/cleanup.go

a-hilaly

Great stuff @0xlen , thank you! I have one nit/question below

a-hilaly · 2023-09-26T21:14:12Z

pkg/elb/cleanup.go

@@ -199,6 +199,24 @@ func getServiceLoadBalancer(ctx context.Context, ec2API awsapi.EC2, elbAPI Descr
 	return &lb, nil
 }

+func getIngressELBName(ctx context.Context, hosts []string) (string, error) {


We can remove ctx context.Context from the params

Agree, I removed the unused ctx parameter.

a-hilaly · 2023-09-26T21:16:03Z

pkg/elb/cleanup.go

+	if err != nil {
+		logger.Debug("%s is ALB Ingress, but probably not provisioned or something other unexpected when getting ALB resource name, skip: %s", metadata.Name, err)
 		return nil, nil
 	}


Maybe we should error if the parsed name exceeds maximum of 32 characters is returned? thoughts @cPu1 ?

I was referring original general text as considering there can have cannot get the hostname or parsed name exceeds maximum of 32 characters. Would be interested to know what could be the proper error message and I am happy to revise that.

a-hilaly · 2023-09-28T18:56:31Z

@0xlen Thank you for iterating on this! LGTM. I'll let the eksctl experts have a say on this before we merge it.

TiberiuGC

LGTM 👍

…l-io#7029) * Fix ELB cleanup failure due to resource name end with a hyphen Fix the incorrect resource name as mentioned in the issue eksctl-io#7028 Refector the getting resource name logic from ELB hostname with unit test cases * Fix code review change for PR#7029

github-actions bot reviewed Aug 31, 2023

View reviewed changes

TiberiuGC added the kind/bug label Sep 5, 2023

Fix ELB cleanup failure due to resource name end with a hyphen

5bcf25a

Fix the incorrect resource name as mentioned in the issue eksctl-io#7028 Refector the getting resource name logic from ELB hostname with unit test cases

0xlen force-pushed the bugfix-elb-cleanup branch from 8f67ee7 to 5bcf25a Compare September 11, 2023 15:14

0xlen mentioned this pull request Sep 20, 2023

[Bug] eksctl fail to delete cluster if the Application load balancer name created by Ingress is 31 characters #7028

Closed

a-hilaly reviewed Sep 20, 2023

View reviewed changes

pkg/elb/cleanup.go Outdated Show resolved Hide resolved

pkg/elb/cleanup.go Outdated Show resolved Hide resolved

pkg/elb/cleanup_test.go Outdated Show resolved Hide resolved

pkg/elb/cleanup.go Outdated Show resolved Hide resolved

0xlen force-pushed the bugfix-elb-cleanup branch 2 times, most recently from 4989bf8 to b73c890 Compare September 26, 2023 18:26

0xlen requested a review from a-hilaly September 26, 2023 18:27

a-hilaly approved these changes Sep 26, 2023

View reviewed changes

Fix code review change for PR#7029

05e32e4

0xlen force-pushed the bugfix-elb-cleanup branch from c141c66 to 05e32e4 Compare September 27, 2023 21:40

TiberiuGC approved these changes Oct 6, 2023

View reviewed changes

TiberiuGC enabled auto-merge (squash) October 6, 2023 07:36

Merge branch 'main' into bugfix-elb-cleanup

2aa7ac2

TiberiuGC merged commit 31fb456 into eksctl-io:main Oct 6, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ELB cleanup failure due to resource name end with a hyphen #7029

Fix ELB cleanup failure due to resource name end with a hyphen #7029

0xlen commented Aug 31, 2023 •

edited by TiberiuGC

Loading

github-actions bot left a comment

TiberiuGC commented Sep 5, 2023

0xlen commented Sep 11, 2023 •

edited

Loading

a-hilaly left a comment

a-hilaly left a comment

a-hilaly Sep 26, 2023

0xlen Sep 27, 2023 •

edited

Loading

a-hilaly Sep 26, 2023

0xlen Sep 28, 2023

a-hilaly commented Sep 28, 2023

TiberiuGC left a comment

Fix ELB cleanup failure due to resource name end with a hyphen #7029

Fix ELB cleanup failure due to resource name end with a hyphen #7029

Conversation

0xlen commented Aug 31, 2023 • edited by TiberiuGC Loading

Description

Checklist

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

github-actions bot left a comment

Choose a reason for hiding this comment

TiberiuGC commented Sep 5, 2023

0xlen commented Sep 11, 2023 • edited Loading

a-hilaly left a comment

Choose a reason for hiding this comment

a-hilaly left a comment

Choose a reason for hiding this comment

a-hilaly Sep 26, 2023

Choose a reason for hiding this comment

0xlen Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

a-hilaly Sep 26, 2023

Choose a reason for hiding this comment

0xlen Sep 28, 2023

Choose a reason for hiding this comment

a-hilaly commented Sep 28, 2023

TiberiuGC left a comment

Choose a reason for hiding this comment

0xlen commented Aug 31, 2023 •

edited by TiberiuGC

Loading

0xlen commented Sep 11, 2023 •

edited

Loading

0xlen Sep 27, 2023 •

edited

Loading