Skip to content

Commit 03de3e4

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #4225 from shraddhabang/fixfinalizers
[feat: gw api] Fix edge cases for lb config finalizers
2 parents 877fc4a + 21fd491 commit 03de3e4

24 files changed

+850
-592
lines changed

‎config/rbac/role.yaml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,12 +135,14 @@ rules:
135135
verbs:
136136
- get
137137
- list
138+
- patch
138139
- watch
139140
- apiGroups:
140141
- gateway.k8s.aws
141142
resources:
142143
- loadbalancerconfigurations/finalizers
143144
verbs:
145+
- patch
144146
- update
145147
- apiGroups:
146148
- gateway.k8s.aws
@@ -157,12 +159,14 @@ rules:
157159
verbs:
158160
- get
159161
- list
162+
- patch
160163
- watch
161164
- apiGroups:
162165
- gateway.k8s.aws
163166
resources:
164167
- targetgroupconfigurations/finalizers
165168
verbs:
169+
- patch
166170
- update
167171
- apiGroups:
168172
- gateway.k8s.aws
@@ -186,6 +190,7 @@ rules:
186190
resources:
187191
- gatewayclasses/finalizers
188192
verbs:
193+
- patch
189194
- update
190195
- apiGroups:
191196
- gateway.networking.k8s.io
@@ -209,6 +214,7 @@ rules:
209214
resources:
210215
- gateways/finalizers
211216
verbs:
217+
- patch
212218
- update
213219
- apiGroups:
214220
- gateway.networking.k8s.io

‎controllers/gateway/config_resolver.go

Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,15 @@ import (
66
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
77
elbv2gw "sigs.k8s.io/aws-load-balancer-controller/apis/gateway/v1beta1"
88
"sigs.k8s.io/aws-load-balancer-controller/pkg/gateway"
9+
"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"
912
"sigs.k8s.io/controller-runtime/pkg/client"
1013
gwv1 "sigs.k8s.io/gateway-api/apis/v1"
1114
)
1215

1316
type gatewayConfigResolver interface {
14-
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)
1518
}
1619

1720
type gatewayConfigResolverImpl struct {
@@ -22,11 +25,11 @@ type gatewayConfigResolverImpl struct {
2225
func newGatewayConfigResolver() gatewayConfigResolver {
2326
return &gatewayConfigResolverImpl{
2427
configMergeFn: gateway.NewLoadBalancerConfigMerger().Merge,
25-
configResolverFn: resolveLoadBalancerConfig,
28+
configResolverFn: gatewayutils.ResolveLoadBalancerConfig,
2629
}
2730
}
2831

29-
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) {
3033

3134
// If the Gateway Class isn't accepted, we shouldn't try to reconcile this Gateway.
3235
derivedStatusIndx, ok := deriveAcceptedConditionIndex(gwClass)
@@ -42,6 +45,12 @@ func (resolver *gatewayConfigResolverImpl) getLoadBalancerConfigForGateway(ctx c
4245
}
4346

4447
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+
}
4554
storedVersion := getStoredProcessedConfig(gwClass)
4655
if storedVersion == nil || *storedVersion != gatewayClassLBConfig.ResourceVersion {
4756
var safeVersion string
@@ -52,24 +61,23 @@ func (resolver *gatewayConfigResolverImpl) getLoadBalancerConfigForGateway(ctx c
5261
}
5362
}
5463

55-
var gwParametersRef *gwv1.ParametersReference
56-
if gw.Spec.Infrastructure != nil && gw.Spec.Infrastructure.ParametersRef != nil {
57-
// Convert local param ref -> namespaced param ref
58-
ns := gwv1.Namespace(gw.Namespace)
59-
gwParametersRef = &gwv1.ParametersReference{
60-
Group: gw.Spec.Infrastructure.ParametersRef.Group,
61-
Kind: gw.Spec.Infrastructure.ParametersRef.Kind,
62-
Name: gw.Spec.Infrastructure.ParametersRef.Name,
63-
Namespace: &ns,
64-
}
65-
}
64+
var gwParametersRef = gatewayutils.GetNamespacedParamRefForGateway(gw)
6665

6766
gatewayLBConfig, err := resolver.configResolverFn(ctx, k8sClient, gwParametersRef)
6867

6968
if err != nil {
7069
return elbv2gw.LoadBalancerConfiguration{}, err
7170
}
7271

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+
7381
if gatewayClassLBConfig == nil && gatewayLBConfig == nil {
7482
return elbv2gw.LoadBalancerConfiguration{}, nil
7583
}

‎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/gateway_class_events.go

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"github.com/go-logr/logr"
66
"k8s.io/client-go/tools/record"
77
"k8s.io/client-go/util/workqueue"
8+
"sigs.k8s.io/aws-load-balancer-controller/pkg/gateway/gatewayutils"
89
"sigs.k8s.io/aws-load-balancer-controller/pkg/k8s"
910
"sigs.k8s.io/controller-runtime/pkg/client"
1011
"sigs.k8s.io/controller-runtime/pkg/event"
@@ -15,23 +16,25 @@ import (
1516

1617
// NewEnqueueRequestsForGatewayClassEvent creates handler for GatewayClass resources
1718
func NewEnqueueRequestsForGatewayClassEvent(
18-
k8sClient client.Client, eventRecorder record.EventRecorder, gwController string, logger logr.Logger) handler.TypedEventHandler[*gatewayv1.GatewayClass, reconcile.Request] {
19+
k8sClient client.Client, eventRecorder record.EventRecorder, gwController string, finalizerManager k8s.FinalizerManager, logger logr.Logger) handler.TypedEventHandler[*gatewayv1.GatewayClass, reconcile.Request] {
1920
return &enqueueRequestsForGatewayClassEvent{
20-
k8sClient: k8sClient,
21-
eventRecorder: eventRecorder,
22-
gwController: gwController,
23-
logger: logger,
21+
k8sClient: k8sClient,
22+
finalizerManager: finalizerManager,
23+
eventRecorder: eventRecorder,
24+
gwController: gwController,
25+
logger: logger,
2426
}
2527
}
2628

2729
var _ handler.TypedEventHandler[*gatewayv1.GatewayClass, reconcile.Request] = (*enqueueRequestsForGatewayClassEvent)(nil)
2830

2931
// enqueueRequestsForGatewayClassEvent handles GatewayClass events
3032
type enqueueRequestsForGatewayClassEvent struct {
31-
k8sClient client.Client
32-
eventRecorder record.EventRecorder
33-
gwController string
34-
logger logr.Logger
33+
k8sClient client.Client
34+
eventRecorder record.EventRecorder
35+
gwController string
36+
finalizerManager k8s.FinalizerManager
37+
logger logr.Logger
3538
}
3639

3740
func (h *enqueueRequestsForGatewayClassEvent) Create(ctx context.Context, e event.TypedCreateEvent[*gatewayv1.GatewayClass], queue workqueue.TypedRateLimitingInterface[reconcile.Request]) {
@@ -57,14 +60,13 @@ func (h *enqueueRequestsForGatewayClassEvent) Generic(ctx context.Context, e eve
5760
}
5861

5962
func (h *enqueueRequestsForGatewayClassEvent) enqueueImpactedGateways(ctx context.Context, gwClass *gatewayv1.GatewayClass, queue workqueue.TypedRateLimitingInterface[reconcile.Request]) {
60-
gwList := GetGatewaysManagedByLBController(ctx, h.k8sClient, h.gwController)
63+
gwList := gatewayutils.GetGatewaysManagedByGatewayClass(ctx, h.k8sClient, gwClass, h.gwController)
6164

6265
for _, gw := range gwList {
63-
if string(gw.Spec.GatewayClassName) == gwClass.Name {
64-
h.logger.V(1).Info("enqueue gateway for gatewayclass event",
65-
"gatewayclass", gwClass.GetName(),
66-
"gateway", k8s.NamespacedName(gw))
67-
queue.Add(reconcile.Request{NamespacedName: k8s.NamespacedName(gw)})
68-
}
66+
h.logger.V(1).Info("enqueue gateway for gatewayclass event",
67+
"gatewayclass", gwClass.GetName(),
68+
"gateway", k8s.NamespacedName(gw))
69+
queue.Add(reconcile.Request{NamespacedName: k8s.NamespacedName(gw)})
70+
6971
}
7072
}

‎controllers/gateway/eventhandlers/gateway_events.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"github.com/go-logr/logr"
66
"k8s.io/client-go/tools/record"
77
"k8s.io/client-go/util/workqueue"
8+
"sigs.k8s.io/aws-load-balancer-controller/pkg/gateway/gatewayutils"
89
"sigs.k8s.io/aws-load-balancer-controller/pkg/k8s"
910
"sigs.k8s.io/controller-runtime/pkg/client"
1011
"sigs.k8s.io/controller-runtime/pkg/event"
@@ -62,7 +63,7 @@ func (h *enqueueRequestsForGatewayEvent) enqueueImpactedGateway(ctx context.Cont
6263
if gw == nil {
6364
return
6465
}
65-
if IsGatewayManagedByLBController(ctx, h.k8sClient, gw, h.gwController) {
66+
if gatewayutils.IsGatewayManagedByLBController(ctx, h.k8sClient, gw, h.gwController) {
6667
h.logger.V(1).Info("enqueue gateway",
6768
"gateway", k8s.NamespacedName(gw))
6869
queue.Add(reconcile.Request{NamespacedName: k8s.NamespacedName(gw)})

0 commit comments

Comments
 (0)