Remove clean up logic and fix the lbcfg in use logic

Author: shraddhabang

controllers/gateway/eventhandlers/gateway_class_events.go

@@ -3,7 +3,6 @@ package eventhandlers
 import (
 	"context"
 	"github.com/go-logr/logr"
-	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/client-go/util/workqueue"
 	"sigs.k8s.io/aws-load-balancer-controller/pkg/gateway/gatewayutils"
@@ -53,13 +52,6 @@ func (h *enqueueRequestsForGatewayClassEvent) Update(ctx context.Context, e even
 
 // Delete is not implemented for this handler as GatewayClass deletion should be finalized and is prevented while referenced by Gateways
 func (h *enqueueRequestsForGatewayClassEvent) Delete(ctx context.Context, e event.TypedDeleteEvent[*gatewayv1.GatewayClass], queue workqueue.TypedRateLimitingInterface[reconcile.Request]) {
-	gwClass := e.Object
-	h.logger.V(1).Info("enqueue gatewayclass delete event", "gatewayclass", gwClass.Name)
-	// remove the load balancer configuration finalizer if there are any referred by this gwclass
-	if err := gatewayutils.RemoveLoadBalancerConfigurationFinalizers(ctx, nil, gwClass, h.k8sClient, h.finalizerManager, sets.New(h.gwController)); err != nil {
-		h.logger.V(1).Info("failed to remove finalizers on load balancer configuration for ", "gateway class", gwClass.Name)
-		return
-	}
 }
 
 func (h *enqueueRequestsForGatewayClassEvent) Generic(ctx context.Context, e event.TypedGenericEvent[*gatewayv1.GatewayClass], queue workqueue.TypedRateLimitingInterface[reconcile.Request]) {

controllers/gateway/eventhandlers/gatewayclass/load_balancer_configuration_events.go

@@ -52,7 +52,7 @@ func (h *enqueueRequestsForLoadBalancerConfigurationEvent) Update(ctx context.Co
 	lbconfigNew := e.ObjectNew
 	h.logger.V(1).Info("enqueue loadbalancerconfiguration update event", "loadbalancerconfiguration", k8s.NamespacedName(lbconfigNew))
 	// to remove finalizers on this residual unused lb config so that the deletion can be done on these
-	if !lbconfigNew.DeletionTimestamp.IsZero() && k8s.HasFinalizer(lbconfigNew, shared_constants.LoadBalancerConfigurationFinalizer) && !gatewayutils.IsLBConfigInUse(ctx, lbconfigNew, nil, nil, h.k8sClient, h.gwControllers) {
+	if !lbconfigNew.DeletionTimestamp.IsZero() && k8s.HasFinalizer(lbconfigNew, shared_constants.LoadBalancerConfigurationFinalizer) && !gatewayutils.IsLBConfigInUse(ctx, lbconfigNew, nil, nil, h.k8sClient) {
 		if err := h.finalizerManager.RemoveFinalizers(ctx, lbconfigNew, shared_constants.LoadBalancerConfigurationFinalizer); err != nil {
 			h.logger.V(1).Info("failed to remove finalizers on load balancer configuration as its currently in use", "load balancer configuration", lbconfigNew.Name)
 			return

controllers/gateway/gateway_controller.go

@@ -165,13 +165,7 @@ type gatewayReconciler struct {
 func (r *gatewayReconciler) Reconcile(ctx context.Context, req reconcile.Request) (ctrl.Result, error) {
 	r.reconcileTracker(req.NamespacedName)
 	err := r.reconcileHelper(ctx, req)
-	res, reconcileErr := runtime.HandleReconcileError(err, r.logger)
-	if reconcileErr != nil {
-		return res, reconcileErr
-	}
-	//if reconcile was successful, then clean up residual finalizers on unused resources
-	r.cleanUpResidualFinalizers()
-	return res, nil
+	return runtime.HandleReconcileError(err, r.logger)
 }
 
 func (r *gatewayReconciler) reconcileHelper(ctx context.Context, req reconcile.Request) error {
@@ -258,7 +252,7 @@ func (r *gatewayReconciler) reconcileDelete(ctx context.Context, gw *gwv1.Gatewa
 			return err
 		}
 		// remove load balancer configuration finalizer
-		if err := gatewayutils.RemoveLoadBalancerConfigurationFinalizers(ctx, gw, gwClass, r.k8sClient, r.finalizerManager, sets.New(r.controllerName)); err != nil {
+		if err := gatewayutils.RemoveLoadBalancerConfigurationFinalizers(ctx, gw, gwClass, r.k8sClient, r.finalizerManager); err != nil {
 			r.eventRecorder.Event(gw, corev1.EventTypeWarning, k8s.LoadBalancerConfigurationEventReasonFailedRemoveFinalizer, fmt.Sprintf("Failed remove load balancer configuration finalizer due to %v", err))
 			return err
 		}
@@ -545,23 +539,3 @@ func isGatewayProgrammed(lbStatus elbv2model.LoadBalancerStatus) bool {
 	return lbStatus.ProvisioningState.Code == elbv2types.LoadBalancerStateEnumActive || lbStatus.ProvisioningState.Code == elbv2types.LoadBalancerStateEnumActiveImpaired
 
 }
-
-// We remove the finalizers on any lb config which is not being used anymore by any gateways so that these lb configs can be deleted safely
-// This is a low priority task and hence we dont want to block reconciler by throwing any errors if the removal of finalizers fails for some reason. It may be retried later.
-func (r *gatewayReconciler) cleanUpResidualFinalizers() {
-	lbConfigList := &elbv2gw.LoadBalancerConfigurationList{}
-	// TODO: Implement cache
-	if err := r.k8sClient.List(context.Background(), lbConfigList); err != nil {
-		r.logger.Error(err, "failed to fetch load balancer configurations for clean up residual finalizersß")
-		return
-	}
-	for i, _ := range lbConfigList.Items {
-		lbConfig := &lbConfigList.Items[i]
-		if k8s.HasFinalizer(lbConfig, shared_constants.LoadBalancerConfigurationFinalizer) && !gatewayutils.IsLBConfigInUse(context.Background(), lbConfig, nil, nil, r.k8sClient, sets.New(r.controllerName)) {
-			if err := r.finalizerManager.RemoveFinalizers(context.Background(), lbConfig, shared_constants.LoadBalancerConfigurationFinalizer); err != nil {
-				r.logger.V(1).Info("failed to remove finalizers on load balancer configuration", "load balancer configuration", k8s.NamespacedName(lbConfig))
-			}
-			return
-		}
-	}
-}

pkg/gateway/gatewayutils/lb_config_utils.go

@@ -6,13 +6,14 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
 	elbv2gw "sigs.k8s.io/aws-load-balancer-controller/apis/gateway/v1beta1"
+	"sigs.k8s.io/aws-load-balancer-controller/pkg/gateway/constants"
 	"sigs.k8s.io/aws-load-balancer-controller/pkg/k8s"
 	"sigs.k8s.io/aws-load-balancer-controller/pkg/shared_constants"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	gwv1 "sigs.k8s.io/gateway-api/apis/v1"
 )
 
-func RemoveLoadBalancerConfigurationFinalizers(ctx context.Context, gw *gwv1.Gateway, gwClass *gwv1.GatewayClass, k8sClient client.Client, manager k8s.FinalizerManager, controllerNames sets.Set[string]) error {
+func RemoveLoadBalancerConfigurationFinalizers(ctx context.Context, gw *gwv1.Gateway, gwClass *gwv1.GatewayClass, k8sClient client.Client, manager k8s.FinalizerManager) error {
 	// remove finalizer from lbConfig - gatewayClass
 	if gwClass != nil {
 		gatewayClassLBConfig, err := ResolveLoadBalancerConfig(ctx, k8sClient, gwClass.Spec.ParametersRef)
@@ -22,7 +23,7 @@ func RemoveLoadBalancerConfigurationFinalizers(ctx context.Context, gw *gwv1.Gat
 		// remove finalizer if it exists and it not in use
 		if gatewayClassLBConfig != nil &&
 			k8s.HasFinalizer(gatewayClassLBConfig, shared_constants.LoadBalancerConfigurationFinalizer) &&
-			!IsLBConfigInUse(ctx, gatewayClassLBConfig, gw, gwClass, k8sClient, controllerNames) {
+			!IsLBConfigInUse(ctx, gatewayClassLBConfig, gw, gwClass, k8sClient) {
 			if err := manager.RemoveFinalizers(ctx, gatewayClassLBConfig, shared_constants.LoadBalancerConfigurationFinalizer); err != nil {
 				return err
 			}
@@ -38,7 +39,7 @@ func RemoveLoadBalancerConfigurationFinalizers(ctx context.Context, gw *gwv1.Gat
 		// remove finalizer if it exists and it is not in use
 		if gatewayLBConfig != nil &&
 			k8s.HasFinalizer(gatewayLBConfig, shared_constants.LoadBalancerConfigurationFinalizer) &&
-			!IsLBConfigInUse(ctx, gatewayLBConfig, gw, gwClass, k8sClient, controllerNames) {
+			!IsLBConfigInUse(ctx, gatewayLBConfig, gw, gwClass, k8sClient) {
 			if err := manager.RemoveFinalizers(ctx, gatewayLBConfig, shared_constants.LoadBalancerConfigurationFinalizer); err != nil {
 				return err
 			}
@@ -68,10 +69,14 @@ func ResolveLoadBalancerConfig(ctx context.Context, k8sClient client.Client, ref
 	return lbConf, err
 }
 
-func IsLBConfigInUse(ctx context.Context, lbConfig *elbv2gw.LoadBalancerConfiguration, gw *gwv1.Gateway, gwClass *gwv1.GatewayClass, k8sClient client.Client, controllerNames sets.Set[string]) bool {
+func IsLBConfigInUse(ctx context.Context, lbConfig *elbv2gw.LoadBalancerConfiguration, gw *gwv1.Gateway, gwClass *gwv1.GatewayClass, k8sClient client.Client) bool {
+	//we want to make sure that we check the lb config is being used either by L4 or L7 gateways
+	controllerNames := sets.New(constants.ALBGatewayController, constants.NLBGatewayController)
 	return IsLBConfigInUseByGatewayClass(ctx, lbConfig, gw, gwClass, k8sClient, controllerNames) ||
 		IsLBConfigInUseByGateway(ctx, lbConfig, gw, k8sClient, controllerNames)
 }
+
+// checks if the lbconfig is indirectly being used by any gateways of the gwclass
 func IsLBConfigInUseByGatewayClass(ctx context.Context, lbConfig *elbv2gw.LoadBalancerConfiguration, currGw *gwv1.Gateway, gwClass *gwv1.GatewayClass, k8sClient client.Client, controllerNames sets.Set[string]) bool {
 	// fetch all the gateway classes referenced by lb config
 	gwClassesUsingLBConfig := GetImpactedGatewayClassesFromLbConfig(ctx, k8sClient, lbConfig, controllerNames)
@@ -94,24 +99,27 @@ func IsLBConfigInUseByGatewayClass(ctx context.Context, lbConfig *elbv2gw.LoadBa
 	// iterate through each GatewayClass identified as referencing the LoadBalancerConfiguration
 	// the lbconfig is deemed to be in active use if any of these GatewayClasses
 	// are found to be managing one or more active Gateway resources.
+	gwsUsingLBConfig := make([]*gwv1.Gateway, 0)
 	for _, controllerName := range controllerNames.UnsortedList() {
 		for _, gwClassUsingLBConfig := range gwClassesUsingLBConfig {
 			gwList := GetGatewaysManagedByGatewayClass(ctx, k8sClient, gwClassUsingLBConfig, controllerName)
-			if currGw == nil {
-				return len(gwList) > 0
-			}
-			//skip the current gw from the list if it is not nil
-			for _, gw := range gwList {
-				if k8s.NamespacedName(currGw) != k8s.NamespacedName(gw) {
-					return true
-				}
-			}
+			gwsUsingLBConfig = append(gwsUsingLBConfig, gwList...)
+		}
+	}
+	if currGw == nil {
+		return len(gwsUsingLBConfig) > 0
+	}
+	//skip the current gw from the list if it is not nil
+	for _, gw := range gwsUsingLBConfig {
+		if k8s.NamespacedName(currGw) != k8s.NamespacedName(gw) {
+			return true
 		}
 	}
 
 	return false
 }
 
+// checks if lbconfig is directly being used by any gateways
 func IsLBConfigInUseByGateway(ctx context.Context, lbConfig *elbv2gw.LoadBalancerConfiguration, gw *gwv1.Gateway, k8sClient client.Client, controllerNames sets.Set[string]) bool {
 	var gwsUsingLBConfig []*gwv1.Gateway
 	for _, controllerName := range controllerNames.UnsortedList() {

pkg/gateway/gatewayutils/lb_config_utils_test.go

@@ -8,7 +8,6 @@ import (
 	"github.com/stretchr/testify/assert"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/apimachinery/pkg/util/sets"
 	elbv2gw "sigs.k8s.io/aws-load-balancer-controller/apis/gateway/v1beta1"
 	mock_client "sigs.k8s.io/aws-load-balancer-controller/mocks/controller-runtime/client"
 	"sigs.k8s.io/aws-load-balancer-controller/pkg/gateway/constants"
@@ -85,7 +84,6 @@ func Test_RemoveLoadBalancerConfigurationFinalizers(t *testing.T) {
 	k8sClient := mock_client.NewMockClient(ctrl)
 	k8sFinalizerManager := k8s.NewMockFinalizerManager(ctrl)
 	ctx := context.Background()
-	controllerName := "test-controller"
 	testGwName := "test-gw"
 	testNamespace := "test-ns"
 	testLbConfigName := "test-lb-config"
@@ -124,14 +122,15 @@ func Test_RemoveLoadBalancerConfigurationFinalizers(t *testing.T) {
 						obj.Finalizers = []string{shared_constants.LoadBalancerConfigurationFinalizer}
 						return nil
 					})
-
+				k8sClient.EXPECT().
+					List(ctx, &gwv1.GatewayClassList{}, gomock.Any()).
+					Return(nil)
 				k8sClient.EXPECT().
 					List(ctx, &gwv1.GatewayList{}, gomock.Any()).
 					Return(nil)
 				k8sClient.EXPECT().
-					List(ctx, &gwv1.GatewayClassList{}, gomock.Any()).
+					List(ctx, &gwv1.GatewayList{}, gomock.Any()).
 					Return(nil)
-
 				k8sFinalizerManager.EXPECT().
 					RemoveFinalizers(ctx, gomock.Any(), shared_constants.LoadBalancerConfigurationFinalizer).
 					Return(nil)
@@ -162,14 +161,15 @@ func Test_RemoveLoadBalancerConfigurationFinalizers(t *testing.T) {
 						obj.Finalizers = []string{shared_constants.LoadBalancerConfigurationFinalizer}
 						return nil
 					})
-
+				k8sClient.EXPECT().
+					List(ctx, &gwv1.GatewayClassList{}, gomock.Any()).
+					Return(nil)
 				k8sClient.EXPECT().
 					List(ctx, &gwv1.GatewayList{}, gomock.Any()).
 					Return(nil)
 				k8sClient.EXPECT().
-					List(ctx, &gwv1.GatewayClassList{}, gomock.Any()).
+					List(ctx, &gwv1.GatewayList{}, gomock.Any()).
 					Return(nil)
-
 				k8sFinalizerManager.EXPECT().
 					RemoveFinalizers(ctx, gomock.Any(), shared_constants.LoadBalancerConfigurationFinalizer).
 					Return(fmt.Errorf("failed to remove finalizer"))
@@ -181,7 +181,7 @@ func Test_RemoveLoadBalancerConfigurationFinalizers(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			tt.setupMocks()
-			err := RemoveLoadBalancerConfigurationFinalizers(ctx, tt.gateway, tt.gatewayClass, k8sClient, k8sFinalizerManager, sets.New(controllerName))
+			err := RemoveLoadBalancerConfigurationFinalizers(ctx, tt.gateway, tt.gatewayClass, k8sClient, k8sFinalizerManager)
 			if tt.wantErr {
 				assert.Error(t, err)
 			} else {
@@ -198,7 +198,6 @@ func Test_isLBConfigInUse(t *testing.T) {
 
 	k8sClient := mock_client.NewMockClient(ctrl)
 	ctx := context.Background()
-	controllerName := "test-controller"
 	testNamespace := "test-ns"
 
 	tests := []struct {
@@ -267,7 +266,7 @@ func Test_isLBConfigInUse(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			tt.setupMocks()
-			got := IsLBConfigInUse(ctx, tt.lbConfig, tt.gateway, tt.gatewayClass, k8sClient, sets.New(controllerName))
+			got := IsLBConfigInUse(ctx, tt.lbConfig, tt.gateway, tt.gatewayClass, k8sClient)
 			assert.Equal(t, tt.want, got)
 		})
 	}