Skip to content

Commit 2d9605d

Browse files
authored
Merge pull request #4549 from zac-nixon/main
[gateway api] remove route count check for deleting gateway
2 parents 7f2837a + c0c24da commit 2d9605d

File tree

5 files changed

+288
-22
lines changed

5 files changed

+288
-22
lines changed

controllers/gateway/gateway_controller.go

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,10 @@ package gateway
33
import (
44
"context"
55
"fmt"
6-
"sigs.k8s.io/aws-load-balancer-controller/pkg/certs"
76
"time"
87

8+
"sigs.k8s.io/aws-load-balancer-controller/pkg/certs"
9+
910
"sigs.k8s.io/aws-load-balancer-controller/pkg/shared_utils"
1011

1112
elbv2types "github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2/types"
@@ -220,6 +221,8 @@ func (r *gatewayReconciler) reconcileHelper(ctx context.Context, req reconcile.R
220221
return err
221222
}
222223

224+
isDeleting := isGatewayDeleting(gw)
225+
223226
loaderResults, err := r.gatewayLoader.LoadRoutesForGateway(ctx, *gw, r.routeFilter, r.controllerName)
224227

225228
if err != nil || loaderResults.ValidationResults.HasErrors {
@@ -253,7 +256,7 @@ func (r *gatewayReconciler) reconcileHelper(ctx context.Context, req reconcile.R
253256
}
254257
}
255258

256-
stack, lb, newAddOnConfig, backendSGRequired, secrets, err := r.buildModel(ctx, gw, mergedLbConfig, allRoutes, currentAddOns)
259+
stack, lb, newAddOnConfig, backendSGRequired, secrets, err := r.buildModel(ctx, gw, mergedLbConfig, allRoutes, currentAddOns, isDeleting)
257260

258261
if err != nil {
259262
return err
@@ -298,14 +301,6 @@ func (r *gatewayReconciler) reconcileHelper(ctx context.Context, req reconcile.R
298301
}
299302

300303
func (r *gatewayReconciler) reconcileDelete(ctx context.Context, gw *gwv1.Gateway, stack core.Stack, routes map[int32][]routeutils.RouteDescriptor) error {
301-
for _, routeList := range routes {
302-
if len(routeList) != 0 {
303-
err := errors.Errorf("Gateway deletion invoked with routes attached [%s]", generateRouteList(routes))
304-
r.eventRecorder.Event(gw, corev1.EventTypeWarning, k8s.GatewayEventReasonFailedDeleteWithRoutesAttached, err.Error())
305-
return err
306-
}
307-
}
308-
309304
if k8s.HasFinalizer(gw, r.finalizer) {
310305
err := r.deployModel(ctx, gw, stack, nil)
311306
if err != nil {
@@ -367,8 +362,8 @@ func (r *gatewayReconciler) deployModel(ctx context.Context, gw *gwv1.Gateway, s
367362
return nil
368363
}
369364

370-
func (r *gatewayReconciler) buildModel(ctx context.Context, gw *gwv1.Gateway, cfg elbv2gw.LoadBalancerConfiguration, listenerToRoute map[int32][]routeutils.RouteDescriptor, currentAddonConfig []addon.Addon) (core.Stack, *elbv2model.LoadBalancer, []addon.AddonMetadata, bool, []types.NamespacedName, error) {
371-
stack, lb, newAddOnConfig, backendSGRequired, secrets, err := r.modelBuilder.Build(ctx, gw, cfg, listenerToRoute, currentAddonConfig, r.secretsManager, r.targetGroupNameToArnMapper)
365+
func (r *gatewayReconciler) buildModel(ctx context.Context, gw *gwv1.Gateway, cfg elbv2gw.LoadBalancerConfiguration, listenerToRoute map[int32][]routeutils.RouteDescriptor, currentAddonConfig []addon.Addon, isDelete bool) (core.Stack, *elbv2model.LoadBalancer, []addon.AddonMetadata, bool, []types.NamespacedName, error) {
366+
stack, lb, newAddOnConfig, backendSGRequired, secrets, err := r.modelBuilder.Build(ctx, gw, cfg, listenerToRoute, currentAddonConfig, r.secretsManager, r.targetGroupNameToArnMapper, isDelete)
372367
if err != nil {
373368
r.eventRecorder.Event(gw, corev1.EventTypeWarning, k8s.GatewayEventReasonFailedBuildModel, fmt.Sprintf("Failed build model due to %v", err))
374369
return nil, nil, nil, false, nil, err

controllers/gateway/utils.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,3 +190,8 @@ func getServicesFromRoutes(listenerRouteMap map[int32][]routeutils.RouteDescript
190190
}
191191
return res.UnsortedList()
192192
}
193+
194+
// isGatewayDeleting returns true if the gateway has a deletion timestamp set
195+
func isGatewayDeleting(gw *gwv1.Gateway) bool {
196+
return gw.DeletionTimestamp != nil && !gw.DeletionTimestamp.IsZero()
197+
}

pkg/gateway/model/base_model_builder.go

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,15 @@ package model
22

33
import (
44
"context"
5+
"strconv"
6+
57
"k8s.io/apimachinery/pkg/types"
68
"sigs.k8s.io/aws-load-balancer-controller/pkg/addon"
79
"sigs.k8s.io/aws-load-balancer-controller/pkg/certs"
810
config2 "sigs.k8s.io/aws-load-balancer-controller/pkg/gateway"
911
modelAddons "sigs.k8s.io/aws-load-balancer-controller/pkg/gateway/model/addons"
1012
"sigs.k8s.io/aws-load-balancer-controller/pkg/shared_utils"
1113
"sigs.k8s.io/controller-runtime/pkg/client"
12-
"strconv"
1314

1415
"github.com/go-logr/logr"
1516
"github.com/pkg/errors"
@@ -33,7 +34,7 @@ import (
3334
// Builder builds the model stack for a Gateway resource.
3435
type Builder interface {
3536
// Build model stack for a gateway
36-
Build(ctx context.Context, gw *gwv1.Gateway, lbConf elbv2gw.LoadBalancerConfiguration, routes map[int32][]routeutils.RouteDescriptor, currentAddonConfig []addon.Addon, secretsManager k8s.SecretsManager, targetGroupNameToArnMapper shared_utils.TargetGroupARNMapper) (core.Stack, *elbv2model.LoadBalancer, []addon.AddonMetadata, bool, []types.NamespacedName, error)
37+
Build(ctx context.Context, gw *gwv1.Gateway, lbConf elbv2gw.LoadBalancerConfiguration, routes map[int32][]routeutils.RouteDescriptor, currentAddonConfig []addon.Addon, secretsManager k8s.SecretsManager, targetGroupNameToArnMapper shared_utils.TargetGroupARNMapper, isDelete bool) (core.Stack, *elbv2model.LoadBalancer, []addon.AddonMetadata, bool, []types.NamespacedName, error)
3738
}
3839

3940
// NewModelBuilder construct a new baseModelBuilder
@@ -122,19 +123,16 @@ type baseModelBuilder struct {
122123
defaultIPType elbv2model.IPAddressType
123124
}
124125

125-
func (baseBuilder *baseModelBuilder) Build(ctx context.Context, gw *gwv1.Gateway, lbConf elbv2gw.LoadBalancerConfiguration, routes map[int32][]routeutils.RouteDescriptor, currentAddonConfig []addon.Addon, secretsManager k8s.SecretsManager, targetGroupNameToArnMapper shared_utils.TargetGroupARNMapper) (core.Stack, *elbv2model.LoadBalancer, []addon.AddonMetadata, bool, []types.NamespacedName, error) {
126+
func (baseBuilder *baseModelBuilder) Build(ctx context.Context, gw *gwv1.Gateway, lbConf elbv2gw.LoadBalancerConfiguration, routes map[int32][]routeutils.RouteDescriptor, currentAddonConfig []addon.Addon, secretsManager k8s.SecretsManager, targetGroupNameToArnMapper shared_utils.TargetGroupARNMapper, isDelete bool) (core.Stack, *elbv2model.LoadBalancer, []addon.AddonMetadata, bool, []types.NamespacedName, error) {
126127
stack := core.NewDefaultStack(core.StackID(k8s.NamespacedName(gw)))
127-
var isPreDelete bool
128-
if gw.DeletionTimestamp != nil && !gw.DeletionTimestamp.IsZero() {
128+
if isDelete {
129129
if baseBuilder.isDeleteProtected(lbConf) {
130130
return nil, nil, nil, false, nil, errors.Errorf("Unable to delete gateway %+v because deletion protection is enabled.", k8s.NamespacedName(gw))
131131
}
132132

133133
if len(currentAddonConfig) == 0 {
134134
return stack, nil, nil, false, nil, nil
135135
}
136-
137-
isPreDelete = true
138136
}
139137

140138
/* Basic LB stuff (Scheme, IP Address Type) */
@@ -171,7 +169,7 @@ func (baseBuilder *baseModelBuilder) Build(ctx context.Context, gw *gwv1.Gateway
171169
}
172170

173171
addOnCfg := lbConf
174-
if isPreDelete {
172+
if isDelete {
175173
addOnCfg = elbv2gw.LoadBalancerConfiguration{}
176174
}
177175

Lines changed: 268 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,268 @@
1+
package model
2+
3+
import (
4+
"testing"
5+
6+
"github.com/go-logr/logr"
7+
"github.com/stretchr/testify/assert"
8+
elbv2gw "sigs.k8s.io/aws-load-balancer-controller/apis/gateway/v1beta1"
9+
elbv2model "sigs.k8s.io/aws-load-balancer-controller/pkg/model/elbv2"
10+
"sigs.k8s.io/aws-load-balancer-controller/pkg/shared_constants"
11+
)
12+
13+
func Test_baseModelBuilder_isDeleteProtected(t *testing.T) {
14+
tests := []struct {
15+
name string
16+
lbConf elbv2gw.LoadBalancerConfiguration
17+
want bool
18+
}{
19+
{
20+
name: "deletion protection enabled",
21+
lbConf: elbv2gw.LoadBalancerConfiguration{
22+
Spec: elbv2gw.LoadBalancerConfigurationSpec{
23+
LoadBalancerAttributes: []elbv2gw.LoadBalancerAttribute{
24+
{
25+
Key: shared_constants.LBAttributeDeletionProtection,
26+
Value: "true",
27+
},
28+
},
29+
},
30+
},
31+
want: true,
32+
},
33+
{
34+
name: "deletion protection disabled",
35+
lbConf: elbv2gw.LoadBalancerConfiguration{
36+
Spec: elbv2gw.LoadBalancerConfigurationSpec{
37+
LoadBalancerAttributes: []elbv2gw.LoadBalancerAttribute{
38+
{
39+
Key: shared_constants.LBAttributeDeletionProtection,
40+
Value: "false",
41+
},
42+
},
43+
},
44+
},
45+
want: false,
46+
},
47+
{
48+
name: "deletion protection not specified",
49+
lbConf: elbv2gw.LoadBalancerConfiguration{
50+
Spec: elbv2gw.LoadBalancerConfigurationSpec{
51+
LoadBalancerAttributes: []elbv2gw.LoadBalancerAttribute{},
52+
},
53+
},
54+
want: false,
55+
},
56+
{
57+
name: "deletion protection with invalid value",
58+
lbConf: elbv2gw.LoadBalancerConfiguration{
59+
Spec: elbv2gw.LoadBalancerConfigurationSpec{
60+
LoadBalancerAttributes: []elbv2gw.LoadBalancerAttribute{
61+
{
62+
Key: shared_constants.LBAttributeDeletionProtection,
63+
Value: "invalid",
64+
},
65+
},
66+
},
67+
},
68+
want: false,
69+
},
70+
{
71+
name: "deletion protection among other attributes",
72+
lbConf: elbv2gw.LoadBalancerConfiguration{
73+
Spec: elbv2gw.LoadBalancerConfigurationSpec{
74+
LoadBalancerAttributes: []elbv2gw.LoadBalancerAttribute{
75+
{
76+
Key: "idle_timeout.timeout_seconds",
77+
Value: "60",
78+
},
79+
{
80+
Key: shared_constants.LBAttributeDeletionProtection,
81+
Value: "true",
82+
},
83+
{
84+
Key: "access_logs.s3.enabled",
85+
Value: "false",
86+
},
87+
},
88+
},
89+
},
90+
want: true,
91+
},
92+
{
93+
name: "empty config",
94+
lbConf: elbv2gw.LoadBalancerConfiguration{},
95+
want: false,
96+
},
97+
}
98+
99+
for _, tt := range tests {
100+
t.Run(tt.name, func(t *testing.T) {
101+
builder := &baseModelBuilder{
102+
logger: logr.Discard(),
103+
}
104+
got := builder.isDeleteProtected(tt.lbConf)
105+
assert.Equal(t, tt.want, got)
106+
})
107+
}
108+
}
109+
110+
func Test_baseModelBuilder_buildLoadBalancerScheme(t *testing.T) {
111+
internetFacing := elbv2gw.LoadBalancerScheme(elbv2model.LoadBalancerSchemeInternetFacing)
112+
internal := elbv2gw.LoadBalancerScheme(elbv2model.LoadBalancerSchemeInternal)
113+
unknown := elbv2gw.LoadBalancerScheme("unknown")
114+
115+
tests := []struct {
116+
name string
117+
lbConf elbv2gw.LoadBalancerConfiguration
118+
defaultScheme elbv2model.LoadBalancerScheme
119+
want elbv2model.LoadBalancerScheme
120+
wantErr bool
121+
}{
122+
{
123+
name: "internet-facing scheme",
124+
lbConf: elbv2gw.LoadBalancerConfiguration{
125+
Spec: elbv2gw.LoadBalancerConfigurationSpec{
126+
Scheme: &internetFacing,
127+
},
128+
},
129+
defaultScheme: elbv2model.LoadBalancerSchemeInternal,
130+
want: elbv2model.LoadBalancerSchemeInternetFacing,
131+
wantErr: false,
132+
},
133+
{
134+
name: "internal scheme",
135+
lbConf: elbv2gw.LoadBalancerConfiguration{
136+
Spec: elbv2gw.LoadBalancerConfigurationSpec{
137+
Scheme: &internal,
138+
},
139+
},
140+
defaultScheme: elbv2model.LoadBalancerSchemeInternetFacing,
141+
want: elbv2model.LoadBalancerSchemeInternal,
142+
wantErr: false,
143+
},
144+
{
145+
name: "nil scheme uses default",
146+
lbConf: elbv2gw.LoadBalancerConfiguration{
147+
Spec: elbv2gw.LoadBalancerConfigurationSpec{
148+
Scheme: nil,
149+
},
150+
},
151+
defaultScheme: elbv2model.LoadBalancerSchemeInternal,
152+
want: elbv2model.LoadBalancerSchemeInternal,
153+
wantErr: false,
154+
},
155+
{
156+
name: "unknown scheme returns error",
157+
lbConf: elbv2gw.LoadBalancerConfiguration{
158+
Spec: elbv2gw.LoadBalancerConfigurationSpec{
159+
Scheme: &unknown,
160+
},
161+
},
162+
defaultScheme: elbv2model.LoadBalancerSchemeInternal,
163+
want: "",
164+
wantErr: true,
165+
},
166+
}
167+
168+
for _, tt := range tests {
169+
t.Run(tt.name, func(t *testing.T) {
170+
builder := &baseModelBuilder{
171+
defaultLoadBalancerScheme: tt.defaultScheme,
172+
}
173+
got, err := builder.buildLoadBalancerScheme(tt.lbConf)
174+
if tt.wantErr {
175+
assert.Error(t, err)
176+
return
177+
}
178+
assert.NoError(t, err)
179+
assert.Equal(t, tt.want, got)
180+
})
181+
}
182+
}
183+
184+
func Test_baseModelBuilder_buildLoadBalancerIPAddressType(t *testing.T) {
185+
ipv4 := elbv2gw.LoadBalancerIpAddressType(elbv2model.IPAddressTypeIPV4)
186+
dualStack := elbv2gw.LoadBalancerIpAddressType(elbv2model.IPAddressTypeDualStack)
187+
dualStackWithoutPublicIPv4 := elbv2gw.LoadBalancerIpAddressType(elbv2model.IPAddressTypeDualStackWithoutPublicIPV4)
188+
unknown := elbv2gw.LoadBalancerIpAddressType("unknown")
189+
190+
tests := []struct {
191+
name string
192+
lbConf elbv2gw.LoadBalancerConfiguration
193+
defaultType elbv2model.IPAddressType
194+
want elbv2model.IPAddressType
195+
wantErr bool
196+
}{
197+
{
198+
name: "ipv4 address type",
199+
lbConf: elbv2gw.LoadBalancerConfiguration{
200+
Spec: elbv2gw.LoadBalancerConfigurationSpec{
201+
IpAddressType: &ipv4,
202+
},
203+
},
204+
defaultType: elbv2model.IPAddressTypeDualStack,
205+
want: elbv2model.IPAddressTypeIPV4,
206+
wantErr: false,
207+
},
208+
{
209+
name: "dualstack address type",
210+
lbConf: elbv2gw.LoadBalancerConfiguration{
211+
Spec: elbv2gw.LoadBalancerConfigurationSpec{
212+
IpAddressType: &dualStack,
213+
},
214+
},
215+
defaultType: elbv2model.IPAddressTypeIPV4,
216+
want: elbv2model.IPAddressTypeDualStack,
217+
wantErr: false,
218+
},
219+
{
220+
name: "dualstack without public ipv4 address type",
221+
lbConf: elbv2gw.LoadBalancerConfiguration{
222+
Spec: elbv2gw.LoadBalancerConfigurationSpec{
223+
IpAddressType: &dualStackWithoutPublicIPv4,
224+
},
225+
},
226+
defaultType: elbv2model.IPAddressTypeIPV4,
227+
want: elbv2model.IPAddressTypeDualStackWithoutPublicIPV4,
228+
wantErr: false,
229+
},
230+
{
231+
name: "nil address type uses default",
232+
lbConf: elbv2gw.LoadBalancerConfiguration{
233+
Spec: elbv2gw.LoadBalancerConfigurationSpec{
234+
IpAddressType: nil,
235+
},
236+
},
237+
defaultType: elbv2model.IPAddressTypeIPV4,
238+
want: elbv2model.IPAddressTypeIPV4,
239+
wantErr: false,
240+
},
241+
{
242+
name: "unknown address type returns error",
243+
lbConf: elbv2gw.LoadBalancerConfiguration{
244+
Spec: elbv2gw.LoadBalancerConfigurationSpec{
245+
IpAddressType: &unknown,
246+
},
247+
},
248+
defaultType: elbv2model.IPAddressTypeIPV4,
249+
want: "",
250+
wantErr: true,
251+
},
252+
}
253+
254+
for _, tt := range tests {
255+
t.Run(tt.name, func(t *testing.T) {
256+
builder := &baseModelBuilder{
257+
defaultIPType: tt.defaultType,
258+
}
259+
got, err := builder.buildLoadBalancerIPAddressType(tt.lbConf)
260+
if tt.wantErr {
261+
assert.Error(t, err)
262+
return
263+
}
264+
assert.NoError(t, err)
265+
assert.Equal(t, tt.want, got)
266+
})
267+
}
268+
}

0 commit comments

Comments
 (0)