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 Fix the incorrect resource name as mentioned in the issue #7028 Refector the getting resource name logic from ELB hostname with unit test cases * Fix code review change for PR#7029
eksctl-io · Oct 6, 2023 · 31fb456 · 31fb456
1 parent 761362c
commit 31fb456
Show file tree

Hide file tree

Showing 3 changed files with 153 additions and 9 deletions.
diff --git a/pkg/elb/cleanup.go b/pkg/elb/cleanup.go
@@ -199,6 +199,24 @@ func getServiceLoadBalancer(ctx context.Context, ec2API awsapi.EC2, elbAPI Descr
 	return &lb, nil
 }
 
+func getIngressELBName(hosts []string) (string, error) {
+	// Expected e.g. bf647c9e-default-appingres-350b-1622159649.eu-central-1.elb.amazonaws.com where AWS ALB name is
+	// bf647c9e-default-appingres-350b (cannot be longer than 32 characters).
+	hostNameParts := strings.Split(hosts[0], ".")
+	if len(hostNameParts[0]) == 0 {
+		return "", fmt.Errorf("cannot get the hostname: %v", hostNameParts)
+	}
+	name := strings.TrimPrefix(hostNameParts[0], "internal-") // Trim 'internal-' prefix for ALB DNS name which is not a part of name.
+	idIdx := strings.LastIndex(name, "-")
+	if (idIdx) != -1 {
+		name = name[:idIdx] // Remove the ELB ID and last hyphen at the end of the hostname (ELB name cannot end with a hyphen)
+	}
+	if len(name) > 32 {
+		return "", fmt.Errorf("parsed name exceeds maximum of 32 characters: %s", name)
+	}
+	return name, nil
+}
+
 func getIngressLoadBalancer(ctx context.Context, ec2API awsapi.EC2, elbAPI DescribeLoadBalancersAPI, elbv2API DescribeLoadBalancersAPIV2,
 	clusterName string, ingress Ingress) (*loadBalancer, error) {
 	metadata := ingress.GetMetadata()
@@ -215,17 +233,12 @@ func getIngressLoadBalancer(ctx context.Context, ec2API awsapi.EC2, elbAPI Descr
 		logger.Debug("%s is ALB Ingress, but probably not provisioned, skip", metadata.Name)
 		return nil, nil
 	}
-	// Expected e.g. bf647c9e-default-appingres-350b-1622159649.eu-central-1.elb.amazonaws.com where AWS ALB name is
-	// bf647c9e-default-appingres-350b (cannot be longer than 32 characters).
-	hostNameParts := strings.Split(hosts[0], ".")
-	if len(hostNameParts[0]) == 0 {
-		logger.Debug("%s is ALB Ingress, but probably not provisioned or something other unexpected, skip", metadata.Name)
+	name, err := getIngressELBName(hosts)
+	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
 	}
-	name := strings.TrimPrefix(hostNameParts[0], "internal-") // Trim 'internal-' prefix for ALB DNS name which is not a part of name.
-	if len(name) > 32 {
-		name = name[:32]
-	}
+	logger.Debug("ALB resource name: %s", name)
 	ctx, cleanup := context.WithTimeout(ctx, 30*time.Second)
 	defer cleanup()
 	securityGroupIDs, err := getSecurityGroupsOwnedByLoadBalancer(ctx, ec2API, elbAPI, elbv2API, clusterName, name, application)

diff --git a/pkg/elb/cleanup_test.go b/pkg/elb/cleanup_test.go
@@ -0,0 +1,118 @@
+package elb
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("ELB Cleanup", func() {
+	When("Getting ingress load balancers to clean up", func() {
+		It("should get the right ELB hostname", func() {
+
+			testCases := []struct {
+				hostname string
+				expected string
+			}{
+				{
+					hostname: "bf647c9e-default-appingres-350b-1622159649.eu-central-1.elb.amazonaws.com",
+					expected: "bf647c9e-default-appingres-350b",
+				},
+				{
+					hostname: "internal-k8s-default-testcase-93d8419948-541000771.us-west-2.elb.amazonaws.com",
+					expected: "k8s-default-testcase-93d8419948",
+				},
+				{
+					hostname: "abcdefgh-default-bugfixtest-001-1356525548.us-west-2.elb.amazonaws.com",
+					expected: "abcdefgh-default-bugfixtest-001",
+				},
+				{
+					hostname: "internal-abcdefgh-default-bugfixtest-002-541910110.us-west-2.elb.amazonaws.com",
+					expected: "abcdefgh-default-bugfixtest-002",
+				},
+				{
+					hostname: "abcdefghijklmnopqrstuvwxyz012345-2118752702.us-west-2.elb.amazonaws.com",
+					expected: "abcdefghijklmnopqrstuvwxyz012345",
+				},
+				{
+					hostname: "internal-abcdefghijklmnopqrstuvwxyz999999-67491582.us-west-2.elb.amazonaws.com",
+					expected: "abcdefghijklmnopqrstuvwxyz999999",
+				},
+				{
+					hostname: "k8s-default-testcase-98cdbf582b-1474733506.us-west-2.elb.amazonaws.com",
+					expected: "k8s-default-testcase-98cdbf582b",
+				},
+				{
+					hostname: "internal-k8s-default-testcase-fb10378931-824853021.us-west-2.elb.amazonaws.com",
+					expected: "k8s-default-testcase-fb10378931",
+				},
+				{
+					hostname: "abcdefghijklmnopqrstuvw000-1623371943.us-west-2.elb.amazonaws.com",
+					expected: "abcdefghijklmnopqrstuvw000",
+				},
+				{
+					hostname: "internal-abcdefghijklmnopqrstuvw001-774959707.us-west-2.elb.amazonaws.com",
+					expected: "abcdefghijklmnopqrstuvw001",
+				},
+				{
+					hostname: "myloadbalancer-1234567890.us-west-2.elb.amazonaws.com",
+					expected: "myloadbalancer",
+				},
+				{
+					hostname: "my-loadbalancer-1234567890.us-west-2.elb.amazonaws.com",
+					expected: "my-loadbalancer",
+				},
+			}
+
+			for _, tc := range testCases {
+				name, err := getIngressELBName([]string{tc.hostname})
+				Expect(err).NotTo(HaveOccurred())
+				Expect(name).To(Equal(tc.expected))
+			}
+		})
+	})
+
+	When("Getting ingress load balancers but cannot get the hostname", func() {
+		It("should have error", func() {
+
+			testCases := []struct {
+				hostname string
+			}{
+				{
+					hostname: "",
+				},
+				{
+					hostname: ".us-east-1.elb.amazonaws.com",
+				},
+			}
+
+			for _, tc := range testCases {
+				_, err := getIngressELBName([]string{tc.hostname})
+				Expect(err).To(HaveOccurred(), "Expected an error for hostname: %s", tc.hostname)
+			}
+		})
+	})
+
+	When("Getting ingress load balancers but parsed ELB name exceeds 32 characters", func() {
+		It("should have error", func() {
+
+			testCases := []struct {
+				hostname string
+			}{
+				{
+					hostname: "this-is-not-an-expected-elb-resource-name.us-east-1.elb.amazonaws.com",
+				},
+				{
+					hostname: "this-is-not-an-expected-elb-resource-name-1234567890.us-east-1.elb.amazonaws.com",
+				},
+				{
+					hostname: "internal-this-is-not-an-expected-elb-resource-name-1234567890.us-east-1.elb.amazonaws.com",
+				},
+			}
+
+			for _, tc := range testCases {
+				_, err := getIngressELBName([]string{tc.hostname})
+				Expect(err).To(HaveOccurred(), "Expected an error for hostname: %s", tc.hostname)
+			}
+		})
+	})
+})
diff --git a/pkg/elb/elb_suite_test.go b/pkg/elb/elb_suite_test.go
@@ -0,0 +1,13 @@
+package elb_test
+
+import (
+	"testing"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+func TestElb(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Elb Suite")
+}