From 31fb456ac1531ae5468e3169ceb1e340f1d54575 Mon Sep 17 00:00:00 2001 From: Eason Cao Date: Fri, 6 Oct 2023 08:53:06 +0100 Subject: [PATCH] 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 --- pkg/elb/cleanup.go | 31 +++++++--- pkg/elb/cleanup_test.go | 118 ++++++++++++++++++++++++++++++++++++++ pkg/elb/elb_suite_test.go | 13 +++++ 3 files changed, 153 insertions(+), 9 deletions(-) create mode 100644 pkg/elb/cleanup_test.go create mode 100644 pkg/elb/elb_suite_test.go diff --git a/pkg/elb/cleanup.go b/pkg/elb/cleanup.go index f99713d183..69d5c9fa6a 100644 --- 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 new file mode 100644 index 0000000000..127b84ef40 --- /dev/null +++ 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 new file mode 100644 index 0000000000..d0cbede813 --- /dev/null +++ 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") +}