fix: certificate deletion logic

Author: Kamil Przybyl

pkg/alb/ingress/alb_spec.go

@@ -5,7 +5,6 @@ import (
 	"crypto/x509"
 	"encoding/pem"
 	"fmt"
-	"log"
 	"net/netip"
 	"sort"
 	"strconv"
@@ -299,53 +298,31 @@ func (r *IngressClassReconciler) loadCerts(
 	return false, certificateIDs, nil
 }
 
-// cleanupCerts deletes the certificates from the Certificates API that are no longer associated with any Ingress in the IngressClass
-func (r *IngressClassReconciler) cleanupCerts(ctx context.Context, ingressClass *networkingv1.IngressClass, ingresses []*networkingv1.Ingress) error {
-	// Prepare a map of secret names that are currently being used by the ingresses
-	usedSecrets := map[string]bool{}
-	for _, ingress := range ingresses {
-		for _, tls := range ingress.Spec.TLS {
-			if tls.SecretName == "" {
-				continue
-			}
-			// Retrieve the TLS Secret
-			tlsSecret := &corev1.Secret{}
-			err := r.Client.Get(ctx, types.NamespacedName{Namespace: ingress.Namespace, Name: tls.SecretName}, tlsSecret)
-			if err != nil {
-				log.Printf("failed to get TLS secret %s: %v", tls.SecretName, err)
-				continue
-			}
-			certName := getCertName(ingressClass, ingress, tlsSecret)
-			usedSecrets[certName] = true
-		}
-	}
-
-	certificatesList, err := r.CertificateClient.ListCertificate(ctx, r.ProjectID, r.Region)
-	if err != nil {
-		return fmt.Errorf("failed to list certificates: %w", err)
-	}
-
-	if certificatesList == nil || certificatesList.Items == nil {
-		return nil // No certificates to clean up
-	}
-	for _, cert := range certificatesList.Items {
-		certID := *cert.Id
-		certName := *cert.Name
-
-		// The certificatesList contains all certificates in the project, so we need to filter them by the ALB IngressClass UID.
-		if !strings.HasPrefix(certName, generateShortUID(ingressClass.UID)) {
-			continue
-		}
-
-		// If the tls secret is no longer in referenced, delete the certificate
-		if _, inUse := usedSecrets[certName]; !inUse {
-			err := r.CertificateClient.DeleteCertificate(ctx, r.ProjectID, r.Region, certID)
-			if err != nil {
-				return fmt.Errorf("failed to delete certificate %s: %v", certName, err)
-			}
-		}
-	}
-	return nil
+// cleanupCerts deletes all certificates from the Certificates API that are associated with this IngressClass.
+func (r *IngressClassReconciler) cleanupCerts(ctx context.Context, ingressClass *networkingv1.IngressClass) error {
+    // We use the IngressClass UID to identify certificates for this specific class.
+	// A shortened version is used because that is how the names were generated on creation.
+	// Note: While a UID collision between clusters is technically possible, it is almost impossible in practice.
+    classPrefix := generateShortUID(ingressClass.UID)
+
+    certificatesList, err := r.CertificateClient.ListCertificate(ctx, r.ProjectID, r.Region)
+    if err != nil {
+        return fmt.Errorf("failed to list certificates: %w", err)
+    }
+
+    if certificatesList == nil || certificatesList.Items == nil {
+        return nil // No certificates to clean up
+    }
+
+    for _, cert := range certificatesList.Items {
+        if strings.HasPrefix(*cert.Name, classPrefix) {
+            err := r.CertificateClient.DeleteCertificate(ctx, r.ProjectID, r.Region, *cert.Id)
+            if err != nil {
+                return fmt.Errorf("failed to delete orphaned certificate %s: %v", *cert.Name, err)
+            }
+        }
+    }
+    return nil
 }
 
 // isCertReady checks if the certificate chain is complete (leaf + intermediates).

pkg/alb/ingress/ingressclass_controller.go

@@ -39,7 +39,7 @@ import (
 )
 
 const (
-	// finalizerName is the name of the finalizer that is added to the IngressClass
+	// finalizerName is the name of the finalizer that is added to Ingress and IngressClass
 	finalizerName = "stackit.cloud/alb-ingress"
 	// controllerName is the name of the ALB controller that the IngressClass should point to for reconciliation
 	controllerName = "stackit.cloud/alb-ingress"
@@ -105,7 +105,7 @@ func (r *IngressClassReconciler) Reconcile(ctx context.Context, req ctrl.Request
 	}
 
 	if len(albIngressList) < 1 {
-		err := r.handleIngressClassWithoutIngresses(ctx, albIngressList, ingressClass)
+		err := r.handleIngressClassWithoutIngresses(ctx, ingressClass)
 		if err != nil {
 			return ctrl.Result{}, fmt.Errorf("failed to reconcile %s IngressClass with no Ingresses: %w", getAlbName(ingressClass), err)
 		}
@@ -281,17 +281,18 @@ func (r *IngressClassReconciler) updateStatus(ctx context.Context, ingresses []*
 	return ctrl.Result{}, nil
 }
 
-// handleIngressClassWithoutIngresses handles the state of the IngressClass that is not referenced by any Ingress
+// handleIngressClassWithoutIngresses handles the case where an IngressClass exists 
+// but is not referenced by any Ingresses. In this scenario, we delete the associated ALB
+// and clean up certificates to avoid billing for unused resources.
 func (r *IngressClassReconciler) handleIngressClassWithoutIngresses(
 	ctx context.Context,
-	ingresses []*networkingv1.Ingress,
 	ingressClass *networkingv1.IngressClass,
 ) error {
 	err := r.ALBClient.DeleteLoadBalancer(ctx, r.ProjectID, r.Region, getAlbName(ingressClass))
 	if err != nil {
 		return fmt.Errorf("failed to delete load balancer: %w", err)
 	}
-	err = r.cleanupCerts(ctx, ingressClass, ingresses)
+	err = r.cleanupCerts(ctx, ingressClass)
 	if err != nil {
 		return fmt.Errorf("failed to clean up certificates: %w", err)
 	}