Clean up ALBs using `spec.ingressClassName` and ALB security groups #6389

aaroniscode · 2023-03-06T20:57:48Z

Description

ALBs created using ingressClassName in the Ingress spec are not cleaned up today. The code was only looking at the deprecated kubernetes.io/ingress.class annotation.

The code now checks if ingressClassName is set and if not, falls back to checking the annotation.

This is my first PR for eksctl so please guide me on required documentation. I didn't see any tests and wasn't sure if it was necessary. Let me know. Thanks.

Edit: added a second commit that fixes deletion of ALB security groups. It doesn't delete them, it just waits for the AWS LB Controller to delete them before deleting the node group.

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 aaroniscode 👋 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

Himangini

@aaroniscode Thanks for the contribution. Can you perhaps write up how you manually tested the changes?

aaroniscode · 2023-04-06T22:31:26Z

sure @Himangini

Testing was straightforward. Created a cluster using eksctl

eksctl create cluster -f alb.yaml

alb.yaml below:

apiVersion: eksctl.io/v1alpha5
kind: ClusterConfig

metadata:
  name: alb
  region: us-west-2
  version: "1.25"

addons:
- name: vpc-cni

cloudWatch:
  clusterLogging:
    enableTypes: ["*"]

iam:
  withOIDC: true

managedNodeGroups:
- name: main
  amiFamily: AmazonLinux2
  iam:
    attachPolicyARNs:
    - arn:aws:iam::aws:policy/AmazonEKSWorkerNodePolicy
    - arn:aws:iam::aws:policy/AmazonEC2ContainerRegistryReadOnly
    - arn:aws:iam::aws:policy/AmazonSSMManagedInstanceCore
  instanceType: t3.large
  minSize: 0
  desiredCapacity: 2
  maxSize: 10
  privateNetworking: true
  spot: false

Installed AWS LB Controller using instructions here: https://docs.aws.amazon.com/eks/latest/userguide/aws-load-balancer-controller.html.

Deploy the example game 2048 application

apiVersion: apps/v1
kind: Deployment
metadata:
  namespace: game-2048
  name: deployment-2048
spec:
  selector:
    matchLabels:
      app.kubernetes.io/name: app-2048
  replicas: 1
  template:
    metadata:
      labels:
        app.kubernetes.io/name: app-2048
    spec:
      containers:
      - image: public.ecr.aws/l6m2t8p7/docker-2048:latest
        imagePullPolicy: Always
        name: app-2048
        ports:
        - containerPort: 80
---
apiVersion: v1
kind: Service
metadata:
  namespace: game-2048
  name: service-2048
spec:
  ports:
    - port: 80
      targetPort: 80
      protocol: TCP
  type: ClusterIP
  selector:
    app.kubernetes.io/name: app-2048
---
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  namespace: game-2048
  name: ingress-2048
  annotations:
    alb.ingress.kubernetes.io/scheme: internet-facing
    alb.ingress.kubernetes.io/target-type: ip
spec:
  ingressClassName: alb
  rules:
    - http:
        paths:
        - path: /
          pathType: Prefix
          backend:
            service:
              name: service-2048
              port:
                number: 80

Then delete the cluster eksctl delete cluster alb

cPu1

I have left some suggestions but otherwise it LGTM 🙂.

pkg/elb/cleanup.go

cPu1 · 2023-05-11T12:39:49Z

pkg/elb/cleanup.go

+	securityGroupIDs, err := getSecurityGroupsOwnedByLoadBalancer(ctx, ec2API, elbAPI, elbv2API, clusterName, name, application)
+	cleanup()
+	if err != nil {
+		return nil, fmt.Errorf("cannot obtain security groups for ALB %s: %s", name, err)


Suggested change

return nil, fmt.Errorf("cannot obtain security groups for ALB %s: %s", name, err)

return nil, fmt.Errorf("cannot obtain security groups for ALB %s: %w", name, err)

cPu1 · 2023-05-11T12:41:14Z

pkg/elb/cleanup.go

 	for _, tag := range tags {
-		if aws.ToString(tag.Key) == clusterTagKey {
+		if aws.ToString(tag.Key) == k8sClusterTagKey || aws.ToString(tag.Key) == elbv2ClusterTagKey {


Alternatively:

Suggested change

if aws.ToString(tag.Key) == k8sClusterTagKey || aws.ToString(tag.Key) == elbv2ClusterTagKey {

switch aws.ToString(tag.Key) {

case k8sClusterTagKey, elbv2ClusterTagKey:

pkg/elb/cleanup.go

cPu1 · 2023-05-11T13:33:28Z

pkg/elb/cleanup.go

+	ctx, cleanup := context.WithTimeout(ctx, 30*time.Second)
+	securityGroupIDs, err := getSecurityGroupsOwnedByLoadBalancer(ctx, ec2API, elbAPI, elbv2API, clusterName, name, application)
+	cleanup()


I know this is taken from getServiceLoadBalancer but a more idiomatic way is:

Suggested change

ctx, cleanup := context.WithTimeout(ctx, 30*time.Second)

securityGroupIDs, err := getSecurityGroupsOwnedByLoadBalancer(ctx, ec2API, elbAPI, elbv2API, clusterName, name, application)

cleanup()

ctx, cleanup := context.WithTimeout(ctx, 30*time.Second)

defer cleanup()

securityGroupIDs, err := getSecurityGroupsOwnedByLoadBalancer(ctx, ec2API, elbAPI, elbv2API, clusterName, name, application)

cPu1 · 2023-05-11T13:58:15Z

pkg/elb/cleanup.go

+
+		lb, err := getIngressLoadBalancer(ctx, ec2API, elbAPI, elbv2API, clusterConfig.Metadata.Name, i)
+		if err != nil {
+			return fmt.Errorf("cannot obtain information for ALB from Ingress %s/%s: %s",


Suggested change

return fmt.Errorf("cannot obtain information for ALB from Ingress %s/%s: %s",

return fmt.Errorf("cannot obtain information for ALB from Ingress %s/%s: %w",

cPu1 · 2023-05-11T14:00:16Z

pkg/elb/cleanup.go

-	if loadBalancerKind == network {
+	elbv2API DescribeLoadBalancersAPIV2, clusterName string, loadBalancerName string, loadBalancerKind loadBalancerKind) (map[string]struct{}, error) {
+
+	groupIds := []string{}


Minor nit:

Suggested change

groupIds := []string{}

var groupIDs []string

done ... I had to rename the variable in a few other places

cPu1 · 2023-05-11T14:08:22Z

pkg/elb/cleanup.go

+		Names: []string{name},
+	})
+	if err != nil {
+		if isELBNotFoundErr(err) {


A not-found error returned from elbv2API.DescribeLoadBalancers will not pass isELBNotFoundErr. The error returned would be of type elasticloadbalancingv2types.LoadBalancerNotFoundException which is not handled by isELBNotFoundErr.

thanks for catching this ... I added a new isELBv2NotFoundErr function that looks for the correct exception in the right package.

aaroniscode · 2023-05-17T02:06:52Z

@cPu1 thanks for the code review and the suggestions! I applied all of your suggestions in a new commit.

cPu1

LGTM. Thanks for the great contribution, Aaron! 🎉

fix to clean up ALBs using IngressClassName

eb75dfe

github-actions bot reviewed Mar 6, 2023

View reviewed changes

fix to clean up ALB security groups

3298b2c

aaroniscode changed the title ~~fix to clean up ALBs using spec.ingressClassName~~ fix to clean up ALBs using spec.ingressClassName and ALB security groups Mar 7, 2023

Himangini reviewed Apr 5, 2023

View reviewed changes

github-actions bot added the stale label May 7, 2023

Himangini removed the stale label May 10, 2023

cPu1 reviewed May 11, 2023

View reviewed changes

address code review suggestions

fde1911

aaroniscode requested a review from cPu1 May 17, 2023 02:07

Himangini added the kind/bug label May 17, 2023

Himangini changed the title ~~fix to clean up ALBs using spec.ingressClassName and ALB security groups~~ Clean up ALBs using spec.ingressClassName and ALB security groups May 17, 2023

cPu1 added 2 commits May 23, 2023 16:08

Merge branch 'main' into fix_cleanup_alb

ffe2c48

Use %w verb for errors

4e02eac

cPu1 approved these changes May 23, 2023

View reviewed changes

Merge branch 'main' into fix_cleanup_alb

08bd92c

Himangini merged commit 5e56099 into eksctl-io:main May 26, 2023
10 checks passed

fbuchmeier-abi mentioned this pull request Feb 13, 2024

[Bug] Cluster Deletion fails with "Error: deadline surpassed waiting for AWS load balancers to be deleted" #7548

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up ALBs using `spec.ingressClassName` and ALB security groups #6389

Clean up ALBs using `spec.ingressClassName` and ALB security groups #6389

aaroniscode commented Mar 6, 2023 •

edited by Himangini

github-actions bot left a comment

Himangini left a comment

aaroniscode commented Apr 6, 2023

cPu1 left a comment

cPu1 May 11, 2023

cPu1 May 11, 2023

cPu1 May 11, 2023

aaroniscode May 17, 2023

cPu1 May 11, 2023

aaroniscode May 17, 2023

cPu1 May 11, 2023

aaroniscode May 17, 2023

cPu1 May 11, 2023

aaroniscode May 17, 2023

aaroniscode commented May 17, 2023

cPu1 left a comment

	return nil, fmt.Errorf("cannot obtain security groups for ALB %s: %s", name, err)
	return nil, fmt.Errorf("cannot obtain security groups for ALB %s: %w", name, err)

	if aws.ToString(tag.Key) == k8sClusterTagKey \|\| aws.ToString(tag.Key) == elbv2ClusterTagKey {
	switch aws.ToString(tag.Key) {
	case k8sClusterTagKey, elbv2ClusterTagKey:

	return fmt.Errorf("cannot obtain information for ALB from Ingress %s/%s: %s",
	return fmt.Errorf("cannot obtain information for ALB from Ingress %s/%s: %w",

Clean up ALBs using spec.ingressClassName and ALB security groups #6389

Clean up ALBs using spec.ingressClassName and ALB security groups #6389

Conversation

aaroniscode commented Mar 6, 2023 • edited by Himangini

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

Himangini left a comment

Choose a reason for hiding this comment

aaroniscode commented Apr 6, 2023

cPu1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaroniscode commented May 17, 2023

cPu1 left a comment

Choose a reason for hiding this comment

Clean up ALBs using `spec.ingressClassName` and ALB security groups #6389

Clean up ALBs using `spec.ingressClassName` and ALB security groups #6389

aaroniscode commented Mar 6, 2023 •

edited by Himangini