Skip to content

Commit 097feb1

Browse files
shraddhabangshraddhabang
committed
add clean up logic
1 parent fa0a554 commit 097feb1

File tree

6 files changed

+113
-173
lines changed

6 files changed

+113
-173
lines changed

‎controllers/gateway/config_resolver.go

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,14 @@ import (
77
elbv2gw "sigs.k8s.io/aws-load-balancer-controller/apis/gateway/v1beta1"
88
"sigs.k8s.io/aws-load-balancer-controller/pkg/gateway"
99
"sigs.k8s.io/aws-load-balancer-controller/pkg/gateway/gatewayutils"
10+
"sigs.k8s.io/aws-load-balancer-controller/pkg/k8s"
11+
"sigs.k8s.io/aws-load-balancer-controller/pkg/shared_constants"
1012
"sigs.k8s.io/controller-runtime/pkg/client"
1113
gwv1 "sigs.k8s.io/gateway-api/apis/v1"
1214
)
1315

1416
type gatewayConfigResolver interface {
15-
getLoadBalancerConfigForGateway(ctx context.Context, k8sClient client.Client, gw *gwv1.Gateway, gwClass *gwv1.GatewayClass) (elbv2gw.LoadBalancerConfiguration, error)
17+
getLoadBalancerConfigForGateway(ctx context.Context, k8sClient client.Client, finalizerManager k8s.FinalizerManager, gw *gwv1.Gateway, gwClass *gwv1.GatewayClass) (elbv2gw.LoadBalancerConfiguration, error)
1618
}
1719

1820
type gatewayConfigResolverImpl struct {
@@ -27,7 +29,7 @@ func newGatewayConfigResolver() gatewayConfigResolver {
2729
}
2830
}
2931

30-
func (resolver *gatewayConfigResolverImpl) getLoadBalancerConfigForGateway(ctx context.Context, k8sClient client.Client, gw *gwv1.Gateway, gwClass *gwv1.GatewayClass) (elbv2gw.LoadBalancerConfiguration, error) {
32+
func (resolver *gatewayConfigResolverImpl) getLoadBalancerConfigForGateway(ctx context.Context, k8sClient client.Client, finalizerManager k8s.FinalizerManager, gw *gwv1.Gateway, gwClass *gwv1.GatewayClass) (elbv2gw.LoadBalancerConfiguration, error) {
3133

3234
// If the Gateway Class isn't accepted, we shouldn't try to reconcile this Gateway.
3335
derivedStatusIndx, ok := deriveAcceptedConditionIndex(gwClass)
@@ -43,6 +45,12 @@ func (resolver *gatewayConfigResolverImpl) getLoadBalancerConfigForGateway(ctx c
4345
}
4446

4547
if gatewayClassLBConfig != nil {
48+
// Add finalizers on lb config only when they are referred by gateway indirectly through the gateway class. We call the lb config is in use in such cases.
49+
if !k8s.HasFinalizer(gatewayClassLBConfig, shared_constants.LoadBalancerConfigurationFinalizer) {
50+
if err := finalizerManager.AddFinalizers(ctx, gatewayClassLBConfig, shared_constants.LoadBalancerConfigurationFinalizer); err != nil {
51+
return elbv2gw.LoadBalancerConfiguration{}, errors.Errorf("failed to add finalizers on load balancer configuration %s", k8s.NamespacedName(gatewayClassLBConfig))
52+
}
53+
}
4654
storedVersion := getStoredProcessedConfig(gwClass)
4755
if storedVersion == nil || *storedVersion != gatewayClassLBConfig.ResourceVersion {
4856
var safeVersion string
@@ -61,6 +69,15 @@ func (resolver *gatewayConfigResolverImpl) getLoadBalancerConfigForGateway(ctx c
6169
return elbv2gw.LoadBalancerConfiguration{}, err
6270
}
6371

72+
if gatewayLBConfig != nil {
73+
// Add finalizers on lb config only when they are referred by gateway directly. We call the lb config is in use in such cases.
74+
if !k8s.HasFinalizer(gatewayLBConfig, shared_constants.LoadBalancerConfigurationFinalizer) {
75+
if err := finalizerManager.AddFinalizers(ctx, gatewayLBConfig, shared_constants.LoadBalancerConfigurationFinalizer); err != nil {
76+
return elbv2gw.LoadBalancerConfiguration{}, errors.Errorf("failed to add finalizers on load balancer configuration %s", k8s.NamespacedName(gatewayLBConfig))
77+
}
78+
}
79+
}
80+
6481
if gatewayClassLBConfig == nil && gatewayLBConfig == nil {
6582
return elbv2gw.LoadBalancerConfiguration{}, nil
6683
}

‎controllers/gateway/config_resolver_test.go

Lines changed: 50 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,14 @@ package gateway
22

33
import (
44
"context"
5+
"github.com/golang/mock/gomock"
56
"github.com/pkg/errors"
67
"github.com/stretchr/testify/assert"
78
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
89
elbv2gw "sigs.k8s.io/aws-load-balancer-controller/apis/gateway/v1beta1"
10+
mock_client "sigs.k8s.io/aws-load-balancer-controller/mocks/controller-runtime/client"
11+
"sigs.k8s.io/aws-load-balancer-controller/pkg/k8s"
12+
"sigs.k8s.io/aws-load-balancer-controller/pkg/shared_constants"
913
"sigs.k8s.io/controller-runtime/pkg/client"
1014
gwv1 "sigs.k8s.io/gateway-api/apis/v1"
1115
"testing"
@@ -15,6 +19,11 @@ func Test_getLoadBalancerConfigForGateway(t *testing.T) {
1519
mergedConfigName := "mergedConfig"
1620
gwClassName := "gwclass"
1721
gwName := "gw"
22+
ctrl := gomock.NewController(t)
23+
defer ctrl.Finish()
24+
25+
k8sClient := mock_client.NewMockClient(ctrl)
26+
k8sFinalizerManager := k8s.NewMockFinalizerManager(ctrl)
1827

1928
testCases := []struct {
2029
name string
@@ -27,12 +36,15 @@ func Test_getLoadBalancerConfigForGateway(t *testing.T) {
2736
resolvedGatewayClassConfig *elbv2gw.LoadBalancerConfiguration
2837
resolvedGatewayConfig *elbv2gw.LoadBalancerConfiguration
2938

39+
setupMocks func()
40+
3041
expectErr bool
3142
expected elbv2gw.LoadBalancerConfiguration
3243
}{
3344
{
3445
name: "gw class isnt accepted",
3546
inputGatewayClass: &gwv1.GatewayClass{},
47+
setupMocks: func() {},
3648
expectErr: true,
3749
},
3850
{
@@ -47,7 +59,8 @@ func Test_getLoadBalancerConfigForGateway(t *testing.T) {
4759
},
4860
},
4961
},
50-
expectErr: true,
62+
setupMocks: func() {},
63+
expectErr: true,
5164
},
5265
{
5366
name: "gw class isnt accepted -- condition is explicitly false",
@@ -61,7 +74,8 @@ func Test_getLoadBalancerConfigForGateway(t *testing.T) {
6174
},
6275
},
6376
},
64-
expectErr: true,
77+
setupMocks: func() {},
78+
expectErr: true,
6579
},
6680
{
6781
name: "gw class accepted -- fail to get gw class config",
@@ -107,6 +121,11 @@ func Test_getLoadBalancerConfigForGateway(t *testing.T) {
107121
ObjectMeta: metav1.ObjectMeta{ResourceVersion: "1"},
108122
}, nil
109123
},
124+
setupMocks: func() {
125+
k8sFinalizerManager.EXPECT().
126+
AddFinalizers(context.Background(), gomock.Any(), shared_constants.LoadBalancerConfigurationFinalizer).
127+
Return(nil)
128+
},
110129
expectErr: true,
111130
},
112131
{
@@ -156,6 +175,11 @@ func Test_getLoadBalancerConfigForGateway(t *testing.T) {
156175
ObjectMeta: metav1.ObjectMeta{ResourceVersion: "1"},
157176
}, nil
158177
},
178+
setupMocks: func() {
179+
k8sFinalizerManager.EXPECT().
180+
AddFinalizers(context.Background(), gomock.Any(), shared_constants.LoadBalancerConfigurationFinalizer).
181+
Return(nil)
182+
},
159183
expectErr: true,
160184
},
161185
{
@@ -188,7 +212,8 @@ func Test_getLoadBalancerConfigForGateway(t *testing.T) {
188212
}
189213
return nil, errors.New("bad thing")
190214
},
191-
expected: elbv2gw.LoadBalancerConfiguration{},
215+
setupMocks: func() {},
216+
expected: elbv2gw.LoadBalancerConfiguration{},
192217
},
193218
{
194219
name: "gw class accepted -- only gw class configs",
@@ -234,6 +259,11 @@ func Test_getLoadBalancerConfigForGateway(t *testing.T) {
234259
expected: elbv2gw.LoadBalancerConfiguration{
235260
ObjectMeta: metav1.ObjectMeta{Name: "gwclass", ResourceVersion: "1"},
236261
},
262+
setupMocks: func() {
263+
k8sFinalizerManager.EXPECT().
264+
AddFinalizers(context.Background(), gomock.Any(), shared_constants.LoadBalancerConfigurationFinalizer).
265+
Return(nil)
266+
},
237267
},
238268
{
239269
name: "gw class accepted -- only gw config",
@@ -278,6 +308,11 @@ func Test_getLoadBalancerConfigForGateway(t *testing.T) {
278308
expected: elbv2gw.LoadBalancerConfiguration{
279309
ObjectMeta: metav1.ObjectMeta{Name: "gw", ResourceVersion: "1"},
280310
},
311+
setupMocks: func() {
312+
k8sFinalizerManager.EXPECT().
313+
AddFinalizers(context.Background(), gomock.Any(), shared_constants.LoadBalancerConfigurationFinalizer).
314+
Return(nil)
315+
},
281316
},
282317
{
283318
name: "gw class accepted -- both gw and gwclass have config - perform merge",
@@ -327,6 +362,11 @@ func Test_getLoadBalancerConfigForGateway(t *testing.T) {
327362
expected: elbv2gw.LoadBalancerConfiguration{
328363
ObjectMeta: metav1.ObjectMeta{Name: mergedConfigName},
329364
},
365+
setupMocks: func() {
366+
k8sFinalizerManager.EXPECT().
367+
AddFinalizers(context.Background(), gomock.Any(), shared_constants.LoadBalancerConfigurationFinalizer).
368+
Return(nil).Times(2)
369+
},
330370
},
331371
{
332372
name: "gw class accepted -- but processed config version has a mismatch",
@@ -369,6 +409,11 @@ func Test_getLoadBalancerConfigForGateway(t *testing.T) {
369409
ObjectMeta: metav1.ObjectMeta{Name: "gwclass", ResourceVersion: "1"},
370410
}, nil
371411
},
412+
setupMocks: func() {
413+
k8sFinalizerManager.EXPECT().
414+
AddFinalizers(context.Background(), gomock.Any(), shared_constants.LoadBalancerConfigurationFinalizer).
415+
Return(nil)
416+
},
372417
expectErr: true,
373418
},
374419
}
@@ -384,8 +429,8 @@ func Test_getLoadBalancerConfigForGateway(t *testing.T) {
384429
},
385430
configResolverFn: tc.configResolverFn,
386431
}
387-
388-
result, err := r.getLoadBalancerConfigForGateway(context.Background(), nil, tc.inputGateway, tc.inputGatewayClass)
432+
tc.setupMocks()
433+
result, err := r.getLoadBalancerConfigForGateway(context.Background(), k8sClient, k8sFinalizerManager, tc.inputGateway, tc.inputGatewayClass)
389434
if tc.expectErr {
390435
assert.Error(t, err)
391436
return

‎controllers/gateway/eventhandlers/gatewayclass/load_balancer_configuration_events.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,13 +43,15 @@ type enqueueRequestsForLoadBalancerConfigurationEvent struct {
4343
}
4444

4545
func (h *enqueueRequestsForLoadBalancerConfigurationEvent) Create(ctx context.Context, e event.TypedCreateEvent[*elbv2gw.LoadBalancerConfiguration], queue workqueue.TypedRateLimitingInterface[reconcile.Request]) {
46-
// We don't need to respond to create events.
46+
lbconfig := e.Object
47+
h.logger.V(1).Info("enqueue loadbalancerconfiguration create event", "loadbalancerconfiguration", k8s.NamespacedName(lbconfig))
48+
h.enqueueImpactedGatewayClass(ctx, lbconfig, queue)
4749
}
4850

4951
func (h *enqueueRequestsForLoadBalancerConfigurationEvent) Update(ctx context.Context, e event.TypedUpdateEvent[*elbv2gw.LoadBalancerConfiguration], queue workqueue.TypedRateLimitingInterface[reconcile.Request]) {
5052
lbconfigNew := e.ObjectNew
5153
h.logger.V(1).Info("enqueue loadbalancerconfiguration update event", "loadbalancerconfiguration", k8s.NamespacedName(lbconfigNew))
52-
// to clean up any residual unsed lb configs with finalizers on them
54+
// to remove finalizers on this residual unused lb config so that the deletion can be done on these
5355
if !lbconfigNew.DeletionTimestamp.IsZero() && k8s.HasFinalizer(lbconfigNew, shared_constants.LoadBalancerConfigurationFinalizer) && !gatewayutils.IsLBConfigInUse(ctx, lbconfigNew, nil, nil, h.k8sClient, h.gwControllers) {
5456
if err := h.finalizerManager.RemoveFinalizers(ctx, lbconfigNew, shared_constants.LoadBalancerConfigurationFinalizer); err != nil {
5557
h.logger.V(1).Info("failed to remove finalizers on load balancer configuration as its currently in use", "load balancer configuration", lbconfigNew.Name)

‎controllers/gateway/gateway_controller.go

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,13 @@ type gatewayReconciler struct {
165165
func (r *gatewayReconciler) Reconcile(ctx context.Context, req reconcile.Request) (ctrl.Result, error) {
166166
r.reconcileTracker(req.NamespacedName)
167167
err := r.reconcileHelper(ctx, req)
168-
return runtime.HandleReconcileError(err, r.logger)
168+
res, reconcileErr := runtime.HandleReconcileError(err, r.logger)
169+
if reconcileErr != nil {
170+
return res, reconcileErr
171+
}
172+
//if reconcile was successful, then clean up residual finalizers on unused resources
173+
r.cleanUpResidualFinalizers()
174+
return res, nil
169175
}
170176

171177
func (r *gatewayReconciler) reconcileHelper(ctx context.Context, req reconcile.Request) error {
@@ -194,7 +200,7 @@ func (r *gatewayReconciler) reconcileHelper(ctx context.Context, req reconcile.R
194200
return nil
195201
}
196202

197-
mergedLbConfig, err := r.cfgResolver.getLoadBalancerConfigForGateway(ctx, r.k8sClient, gw, gwClass)
203+
mergedLbConfig, err := r.cfgResolver.getLoadBalancerConfigForGateway(ctx, r.k8sClient, r.finalizerManager, gw, gwClass)
198204

199205
if err != nil {
200206
statusErr := r.updateGatewayStatusFailure(ctx, gw, gwv1.GatewayReasonInvalid, err.Error())
@@ -273,12 +279,6 @@ func (r *gatewayReconciler) reconcileUpdate(ctx context.Context, gw *gwv1.Gatewa
273279
return err
274280
}
275281

276-
// add load balancer configuration finalizer
277-
if err := gatewayutils.AddLoadBalancerConfigurationFinalizers(ctx, gw, gwClass, r.k8sClient, r.finalizerManager, r.controllerName); err != nil {
278-
r.eventRecorder.Event(gw, corev1.EventTypeWarning, k8s.LoadBalancerConfigurationEventReasonFailedAddFinalizer, fmt.Sprintf("Failed add load balancer configuration finalizer due to %v", err))
279-
return err
280-
}
281-
282282
err := r.deployModel(ctx, gw, stack)
283283
if err != nil {
284284
return err
@@ -545,3 +545,23 @@ func isGatewayProgrammed(lbStatus elbv2model.LoadBalancerStatus) bool {
545545
return lbStatus.ProvisioningState.Code == elbv2types.LoadBalancerStateEnumActive || lbStatus.ProvisioningState.Code == elbv2types.LoadBalancerStateEnumActiveImpaired
546546

547547
}
548+
549+
// 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
550+
// 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.
551+
func (r *gatewayReconciler) cleanUpResidualFinalizers() {
552+
lbConfigList := &elbv2gw.LoadBalancerConfigurationList{}
553+
// TODO: Implement cache
554+
if err := r.k8sClient.List(context.Background(), lbConfigList); err != nil {
555+
r.logger.Error(err, "failed to fetch load balancer configurations for clean up residual finalizersß")
556+
return
557+
}
558+
for i, _ := range lbConfigList.Items {
559+
lbConfig := &lbConfigList.Items[i]
560+
if k8s.HasFinalizer(lbConfig, shared_constants.LoadBalancerConfigurationFinalizer) && !gatewayutils.IsLBConfigInUse(context.Background(), lbConfig, nil, nil, r.k8sClient, sets.New(r.controllerName)) {
561+
if err := r.finalizerManager.RemoveFinalizers(context.Background(), lbConfig, shared_constants.LoadBalancerConfigurationFinalizer); err != nil {
562+
r.logger.V(1).Info("failed to remove finalizers on load balancer configuration", "load balancer configuration", k8s.NamespacedName(lbConfig))
563+
}
564+
return
565+
}
566+
}
567+
}

‎pkg/gateway/gatewayutils/lb_config_utils.go

Lines changed: 12 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -6,45 +6,12 @@ import (
66
"k8s.io/apimachinery/pkg/types"
77
"k8s.io/apimachinery/pkg/util/sets"
88
elbv2gw "sigs.k8s.io/aws-load-balancer-controller/apis/gateway/v1beta1"
9-
"sigs.k8s.io/aws-load-balancer-controller/pkg/gateway/constants"
109
"sigs.k8s.io/aws-load-balancer-controller/pkg/k8s"
1110
"sigs.k8s.io/aws-load-balancer-controller/pkg/shared_constants"
1211
"sigs.k8s.io/controller-runtime/pkg/client"
1312
gwv1 "sigs.k8s.io/gateway-api/apis/v1"
1413
)
1514

16-
// AddLoadBalancerConfigurationFinalizers add finalizer to load balancer configuration when it is in use by gateway or gatewayClass
17-
func AddLoadBalancerConfigurationFinalizers(ctx context.Context, gw *gwv1.Gateway, gwClass *gwv1.GatewayClass, k8sClient client.Client, manager k8s.FinalizerManager, controllerName string) error {
18-
// add finalizer to lbConfig referred by gatewayClass
19-
if gwClass.Spec.ParametersRef != nil && string(gwClass.Spec.ParametersRef.Kind) == constants.LoadBalancerConfiguration {
20-
lbConfig := &elbv2gw.LoadBalancerConfiguration{}
21-
if err := k8sClient.Get(ctx, types.NamespacedName{
22-
Namespace: string(*gwClass.Spec.ParametersRef.Namespace),
23-
Name: gwClass.Spec.ParametersRef.Name,
24-
}, lbConfig); err != nil {
25-
return client.IgnoreNotFound(err)
26-
}
27-
if err := manager.AddFinalizers(ctx, lbConfig, shared_constants.LoadBalancerConfigurationFinalizer); err != nil {
28-
return err
29-
}
30-
}
31-
32-
// add finalizer to lbConfig referred by gateway
33-
if gw.Spec.Infrastructure != nil && gw.Spec.Infrastructure.ParametersRef != nil && string(gw.Spec.Infrastructure.ParametersRef.Kind) == constants.LoadBalancerConfiguration {
34-
lbConfig := &elbv2gw.LoadBalancerConfiguration{}
35-
if err := k8sClient.Get(ctx, types.NamespacedName{
36-
Namespace: gw.Namespace,
37-
Name: gw.Spec.Infrastructure.ParametersRef.Name,
38-
}, lbConfig); err != nil {
39-
return client.IgnoreNotFound(err)
40-
}
41-
if err := manager.AddFinalizers(ctx, lbConfig, shared_constants.LoadBalancerConfigurationFinalizer); err != nil {
42-
return err
43-
}
44-
}
45-
return nil
46-
}
47-
4815
func RemoveLoadBalancerConfigurationFinalizers(ctx context.Context, gw *gwv1.Gateway, gwClass *gwv1.GatewayClass, k8sClient client.Client, manager k8s.FinalizerManager, controllerNames sets.Set[string]) error {
4916
// remove finalizer from lbConfig - gatewayClass
5017
if gwClass != nil {
@@ -102,10 +69,10 @@ func ResolveLoadBalancerConfig(ctx context.Context, k8sClient client.Client, ref
10269
}
10370

10471
func IsLBConfigInUse(ctx context.Context, lbConfig *elbv2gw.LoadBalancerConfiguration, gw *gwv1.Gateway, gwClass *gwv1.GatewayClass, k8sClient client.Client, controllerNames sets.Set[string]) bool {
105-
return IsLBConfigInUseByGatewayClass(ctx, lbConfig, gwClass, k8sClient, controllerNames) ||
72+
return IsLBConfigInUseByGatewayClass(ctx, lbConfig, gw, gwClass, k8sClient, controllerNames) ||
10673
IsLBConfigInUseByGateway(ctx, lbConfig, gw, k8sClient, controllerNames)
10774
}
108-
func IsLBConfigInUseByGatewayClass(ctx context.Context, lbConfig *elbv2gw.LoadBalancerConfiguration, gwClass *gwv1.GatewayClass, k8sClient client.Client, controllerNames sets.Set[string]) bool {
75+
func IsLBConfigInUseByGatewayClass(ctx context.Context, lbConfig *elbv2gw.LoadBalancerConfiguration, currGw *gwv1.Gateway, gwClass *gwv1.GatewayClass, k8sClient client.Client, controllerNames sets.Set[string]) bool {
10976
// fetch all the gateway classes referenced by lb config
11077
gwClassesUsingLBConfig := GetImpactedGatewayClassesFromLbConfig(ctx, k8sClient, lbConfig, controllerNames)
11178

@@ -129,8 +96,15 @@ func IsLBConfigInUseByGatewayClass(ctx context.Context, lbConfig *elbv2gw.LoadBa
12996
// are found to be managing one or more active Gateway resources.
13097
for _, controllerName := range controllerNames.UnsortedList() {
13198
for _, gwClassUsingLBConfig := range gwClassesUsingLBConfig {
132-
if len(GetGatewaysManagedByGatewayClass(ctx, k8sClient, gwClassUsingLBConfig, controllerName)) > 0 {
133-
return true
99+
gwList := GetGatewaysManagedByGatewayClass(ctx, k8sClient, gwClassUsingLBConfig, controllerName)
100+
if currGw == nil {
101+
return len(gwList) > 0
102+
}
103+
//skip the current gw from the list if it is not nil
104+
for _, gw := range gwList {
105+
if k8s.NamespacedName(currGw) != k8s.NamespacedName(gw) {
106+
return true
107+
}
134108
}
135109
}
136110
}
@@ -149,7 +123,7 @@ func IsLBConfigInUseByGateway(ctx context.Context, lbConfig *elbv2gw.LoadBalance
149123
}
150124
// check if lbConfig is referred by any other gateway
151125
for _, gwUsingLBConfig := range gwsUsingLBConfig {
152-
if gwUsingLBConfig.Name != gw.Name || gwUsingLBConfig.Namespace != gw.Namespace {
126+
if k8s.NamespacedName(gwUsingLBConfig) != k8s.NamespacedName(gw) {
153127
return true
154128
}
155129
}

0 commit comments

Comments
 (0)