Skip to content

Commit 674a2c2

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #3035 from TortillaZHawaii/arm-cleanup-code
Add flag to enable cleanup path for Deny Firewalls
2 parents 0eb8ad7 + d5d4b63 commit 674a2c2

6 files changed

Lines changed: 145 additions & 69 deletions

File tree

‎cmd/glbc/main.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -338,6 +338,7 @@ func main() {
338338
EnableL4ILBMixedProtocol: flags.F.EnableL4ILBMixedProtocol,
339339
EnableL4NetLBMixedProtocol: flags.F.EnableL4NetLBMixedProtocol,
340340
EnableL4DenyFirewalls: flags.F.EnableL4DenyFirewall,
341+
EnableL4DenyFirewallsRollbackCleanup: flags.F.EnableL4DenyFirewallRollbackCleanup,
341342
EnableL4ILBZonalAffinity: flags.F.EnableL4ILBZonalAffinity,
342343
EnableL4NetLBForwardingRulesOptimizations: flags.F.EnableL4NetLBForwardingRulesOptimizations,
343344
ReadOnlyMode: flags.F.ReadOnlyMode,

‎pkg/context/context.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,7 @@ type ControllerContextConfig struct {
139139
EnableL4NetLBMixedProtocol bool
140140
EnableL4NetLBForwardingRulesOptimizations bool
141141
EnableL4DenyFirewalls bool
142+
EnableL4DenyFirewallsRollbackCleanup bool
142143
}
143144

144145
// NewControllerContext returns a new shared set of informers.

‎pkg/flags/flags.go‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,7 @@ var F = struct {
142142
EnableL4ILBMixedProtocol bool
143143
EnableL4NetLBMixedProtocol bool
144144
EnableL4DenyFirewall bool
145+
EnableL4DenyFirewallRollbackCleanup bool
145146
EnableL4NetLBForwardingRulesOptimizations bool
146147
EnableIPV6OnlyNEG bool
147148
MultiProjectOwnerLabelKey string
@@ -355,7 +356,8 @@ L7 load balancing. CSV values accepted. Example: -node-port-ranges=80,8080,400-5
355356
flag.BoolVar(&F.EnableMultiProjectMode, "enable-multi-project-mode", false, "Enable running in multi-project mode.")
356357
flag.BoolVar(&F.EnableL4ILBMixedProtocol, "enable-l4ilb-mixed-protocol", false, "Enable support for mixed protocol L4 internal load balancers.")
357358
flag.BoolVar(&F.EnableL4NetLBMixedProtocol, "enable-l4netlb-mixed-protocol", false, "Enable support for mixed protocol L4 external load balancers.")
358-
flag.BoolVar(&F.EnableL4DenyFirewall, "enable-l4-deny-firewall", false, "Enable creation and updates of Deny VPC Firewall Rules for L4 external load balancers. Requires --enable-pinhole to be true.")
359+
flag.BoolVar(&F.EnableL4DenyFirewall, "enable-l4-deny-firewall", false, "Enable creation and updates of Deny VPC Firewall Rules for L4 external load balancers. Requires --enable-pinhole and --enable-l4-deny-firewall-rollback-cleanup to be true.")
360+
flag.BoolVar(&F.EnableL4DenyFirewallRollbackCleanup, "enable-l4-deny-firewall-rollback-cleanup", false, "Enable cleanup codepath of the deny firewalls for rollback. The reason for it not being enabled by default is the additional GCE API calls that are made for checking if the deny firewalls exist/deletion which will eat up the quota unnecessarily.")
359361
flag.StringVar(&F.ProviderConfigNameLabelKey, "provider-config-name-label-key", "cloud.gke.io/provider-config-name", "The label key for provider-config name, which is used to identify the provider-config of objects in multi-project mode.")
360362
flag.BoolVar(&F.EnableL4NetLBForwardingRulesOptimizations, "enable-l4netlb-forwarding-rules-optimizations", false, "Enable optimized processing of forwarding rules for L4 NetLB.")
361363
flag.BoolVar(&F.EnableIPV6OnlyNEG, "enable-ipv6-only-neg", false, "Enable support for IPV6 Only NEG's.")
@@ -382,8 +384,8 @@ func Validate() {
382384
klog.Fatalf("Invalid --override-health-check-src-cidrs flag: %v", err)
383385
}
384386

385-
if F.EnableL4DenyFirewall && !F.EnablePinhole {
386-
klog.Fatalf("The flag --enable-l4-deny-firewall requires --enable-pinhole to be true.")
387+
if F.EnableL4DenyFirewall && (!F.EnablePinhole || !F.EnableL4DenyFirewallRollbackCleanup) {
388+
klog.Fatalf("The flag --enable-l4-deny-firewall requires --enable-pinhole and --enable-l4-deny-firewall-rollback-cleanup to be true.")
387389
}
388390
}
389391

‎pkg/l4lb/l4netlbcontroller.go‎

Lines changed: 25 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -614,18 +614,19 @@ func (lc *L4NetLBController) syncInternal(service *v1.Service, svcLogger klog.Lo
614614
usesNegBackends := lc.shouldUseNEGBackends(service, svcLogger)
615615

616616
l4NetLBParams := &l4resources.L4NetLBParams{
617-
Service: service,
618-
Cloud: lc.ctx.Cloud,
619-
Namer: lc.namer,
620-
Recorder: lc.ctx.Recorder(service.Namespace),
621-
DualStackEnabled: lc.enableDualStack,
622-
StrongSessionAffinityEnabled: lc.enableStrongSessionAffinity,
623-
NetworkResolver: lc.networkResolver,
624-
EnableWeightedLB: lc.ctx.EnableWeightedL4NetLB,
625-
EnableMixedProtocol: lc.ctx.EnableL4NetLBMixedProtocol,
626-
DisableNodesFirewallProvisioning: lc.ctx.DisableL4LBFirewall,
627-
UseNEGs: usesNegBackends,
628-
UseDenyFirewalls: lc.ctx.EnableL4DenyFirewalls,
617+
Service: service,
618+
Cloud: lc.ctx.Cloud,
619+
Namer: lc.namer,
620+
Recorder: lc.ctx.Recorder(service.Namespace),
621+
DualStackEnabled: lc.enableDualStack,
622+
StrongSessionAffinityEnabled: lc.enableStrongSessionAffinity,
623+
NetworkResolver: lc.networkResolver,
624+
EnableWeightedLB: lc.ctx.EnableWeightedL4NetLB,
625+
EnableMixedProtocol: lc.ctx.EnableL4NetLBMixedProtocol,
626+
DisableNodesFirewallProvisioning: lc.ctx.DisableL4LBFirewall,
627+
UseNEGs: usesNegBackends,
628+
UseDenyFirewalls: lc.ctx.EnableL4DenyFirewalls,
629+
EnableDenyFirewallsRollbackCleanup: lc.ctx.EnableL4DenyFirewallsRollbackCleanup,
629630
}
630631
if lc.ctx.ConfigMapInformer != nil {
631632
l4NetLBParams.ConfigMapLister = lc.ctx.ConfigMapInformer.GetIndexer()
@@ -870,17 +871,18 @@ func (lc *L4NetLBController) garbageCollectRBSNetLB(key string, svc *v1.Service,
870871
}()
871872

872873
l4NetLBParams := &l4resources.L4NetLBParams{
873-
Service: svc,
874-
Cloud: lc.ctx.Cloud,
875-
Namer: lc.namer,
876-
Recorder: lc.ctx.Recorder(svc.Namespace),
877-
DualStackEnabled: lc.enableDualStack,
878-
StrongSessionAffinityEnabled: lc.enableStrongSessionAffinity,
879-
NetworkResolver: lc.networkResolver,
880-
EnableWeightedLB: lc.ctx.EnableWeightedL4NetLB,
881-
EnableMixedProtocol: lc.ctx.EnableL4NetLBMixedProtocol,
882-
DisableNodesFirewallProvisioning: lc.ctx.DisableL4LBFirewall,
883-
UseDenyFirewalls: lc.ctx.EnableL4DenyFirewalls,
874+
Service: svc,
875+
Cloud: lc.ctx.Cloud,
876+
Namer: lc.namer,
877+
Recorder: lc.ctx.Recorder(svc.Namespace),
878+
DualStackEnabled: lc.enableDualStack,
879+
StrongSessionAffinityEnabled: lc.enableStrongSessionAffinity,
880+
NetworkResolver: lc.networkResolver,
881+
EnableWeightedLB: lc.ctx.EnableWeightedL4NetLB,
882+
EnableMixedProtocol: lc.ctx.EnableL4NetLBMixedProtocol,
883+
DisableNodesFirewallProvisioning: lc.ctx.DisableL4LBFirewall,
884+
UseDenyFirewalls: lc.ctx.EnableL4DenyFirewalls,
885+
EnableDenyFirewallsRollbackCleanup: lc.ctx.EnableL4DenyFirewallsRollbackCleanup,
884886
}
885887
if lc.ctx.ConfigMapInformer != nil {
886888
l4NetLBParams.ConfigMapLister = lc.ctx.ConfigMapInformer.GetIndexer()

‎pkg/l4resources/denytest/deny_test.go‎

Lines changed: 72 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -305,9 +305,10 @@ func TestDenyRespectsDisableNodeFirewallProvisioning(t *testing.T) {
305305
}
306306
log := klog.TODO()
307307
l4netlb := l4resources.NewL4NetLB(&l4resources.L4NetLBParams{
308-
Service: svc,
309-
UseDenyFirewalls: true,
310-
DisableNodesFirewallProvisioning: true,
308+
Service: svc,
309+
UseDenyFirewalls: true,
310+
EnableDenyFirewallsRollbackCleanup: true,
311+
DisableNodesFirewallProvisioning: true,
311312
// other parameters
312313
Cloud: cloud,
313314
Namer: namer.NewL4Namer("ks123", namer.NewNamer("", "", log)),
@@ -619,10 +620,76 @@ func TestExportsCorrectDenyMetrics(t *testing.T) {
619620
}
620621
}
621622

623+
// TestSkipRollbackCleanupIfDisabled is a complimentary test to TestDenyRollback which verifies
624+
// that the cleanup logic on Ensure is skipped when the ArmDenyFirewallsRollbackCleanup is not enabled.
625+
func TestSkipRollbackCleanupIfDisabled(t *testing.T) {
626+
// Arrange
627+
ctx := t.Context()
628+
svc := helperService([]v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol})
629+
630+
gce, err := helperCloud(ctx)
631+
if err != nil {
632+
t.Fatal(err)
633+
}
634+
mockGCE := gce.Compute().(*cloud.MockGCE)
635+
getCalled := make(map[string]bool)
636+
// Function that is tested will check for deny firewalls if it is enabled before actually deleting them
637+
mockGCE.MockFirewalls.GetHook = func(ctx context.Context, key *meta.Key, m *cloud.MockFirewalls, options ...cloud.Option) (bool, *compute.Firewall, error) {
638+
getCalled[key.Name] = true
639+
return false, nil, nil // continue with normal hook
640+
}
641+
642+
log := klog.TODO()
643+
ensurer := helperL4NetLBWithoutCleanup(gce, log, svc)
644+
res := ensurer.EnsureFrontend([]string{nodeName}, svc, time.Now())
645+
if res.Error != nil {
646+
t.Fatal(res.Error)
647+
}
648+
649+
// Assert
650+
// No cleanup logic should be called - it is not armed
651+
for _, name := range []string{denyIPv4Name, denyIPv6Name} {
652+
if got := getCalled[name]; got {
653+
t.Errorf("Cleanup or other deny firewall logic for %v was executed even though the ArmDenyFirewallsRollbackCleanup and UseDenyFirewalls flags were set to false", name)
654+
}
655+
// cleanup for later test
656+
getCalled[name] = false
657+
}
658+
659+
// Verify that the cleanup logic is actually performed when it needs to be
660+
ensurer = helperL4NetLB(gce, log, svc, denyFirewallDisabled) // cleanup is armed
661+
res = ensurer.EnsureFrontend([]string{nodeName}, svc, time.Now())
662+
if res.Error != nil {
663+
t.Fatal(res.Error)
664+
}
665+
666+
for _, name := range []string{denyIPv4Name, denyIPv6Name} {
667+
if got := getCalled[name]; !got {
668+
t.Errorf("Cleanup for deny firewall %v logic has not been executed", name)
669+
}
670+
}
671+
}
672+
622673
func helperL4NetLB(cloud *gce.Cloud, log klog.Logger, svc *v1.Service, denyFirewall bool) *l4resources.L4NetLB {
623674
return l4resources.NewL4NetLB(&l4resources.L4NetLBParams{
624-
Service: svc,
625-
UseDenyFirewalls: denyFirewall,
675+
Service: svc,
676+
UseDenyFirewalls: denyFirewall,
677+
EnableDenyFirewallsRollbackCleanup: true,
678+
// other parameters
679+
Cloud: cloud,
680+
Namer: namer.NewL4Namer("ks123", namer.NewNamer("", "", log)),
681+
Recorder: record.NewFakeRecorder(100),
682+
NetworkResolver: network.NewFakeResolver(network.DefaultNetwork(cloud)),
683+
DualStackEnabled: true,
684+
EnableMixedProtocol: true,
685+
}, log)
686+
}
687+
688+
func helperL4NetLBWithoutCleanup(cloud *gce.Cloud, log klog.Logger, svc *v1.Service) *l4resources.L4NetLB {
689+
return l4resources.NewL4NetLB(&l4resources.L4NetLBParams{
690+
Service: svc,
691+
UseDenyFirewalls: false,
692+
EnableDenyFirewallsRollbackCleanup: false,
626693
// other parameters
627694
Cloud: cloud,
628695
Namer: namer.NewL4Namer("ks123", namer.NewNamer("", "", log)),

‎pkg/l4resources/l4netlb.go‎

Lines changed: 41 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -70,10 +70,11 @@ type L4NetLB struct {
7070
NamespacedName types.NamespacedName
7171
healthChecks healthchecksl4.L4HealthChecks
7272
// mixedManager is responsible for managing forwarding rules for mixed protocol lbs
73-
mixedManager *forwardingrules.MixedManagerNetLB
74-
forwardingRules ForwardingRulesProvider
75-
enableDualStack bool
76-
useDenyFirewalls bool
73+
mixedManager *forwardingrules.MixedManagerNetLB
74+
forwardingRules ForwardingRulesProvider
75+
enableDualStack bool
76+
useDenyFirewalls bool
77+
enableDenyFirewallsRollbackCleanup bool
7778
// represents if `enable strong session affinity` flag was set
7879
enableStrongSessionAffinity bool
7980
networkInfo network.NetworkInfo
@@ -129,19 +130,20 @@ func (r *L4NetLBSyncResult) SetMetricsForSuccessfulServiceSync() {
129130
}
130131

131132
type L4NetLBParams struct {
132-
Service *corev1.Service
133-
Cloud *gce.Cloud
134-
Namer namer.L4ResourcesNamer
135-
Recorder record.EventRecorder
136-
DualStackEnabled bool
137-
StrongSessionAffinityEnabled bool
138-
NetworkResolver network.Resolver
139-
EnableWeightedLB bool
140-
EnableMixedProtocol bool
141-
DisableNodesFirewallProvisioning bool
142-
UseNEGs bool
143-
UseDenyFirewalls bool
144-
ConfigMapLister cache.Store
133+
Service *corev1.Service
134+
Cloud *gce.Cloud
135+
Namer namer.L4ResourcesNamer
136+
Recorder record.EventRecorder
137+
DualStackEnabled bool
138+
StrongSessionAffinityEnabled bool
139+
NetworkResolver network.Resolver
140+
EnableWeightedLB bool
141+
EnableMixedProtocol bool
142+
DisableNodesFirewallProvisioning bool
143+
UseNEGs bool
144+
UseDenyFirewalls bool
145+
EnableDenyFirewallsRollbackCleanup bool
146+
ConfigMapLister cache.Store
145147
}
146148

147149
// NewL4NetLB creates a new Handler for the given L4NetLB service.
@@ -157,26 +159,27 @@ func NewL4NetLB(params *L4NetLBParams, logger klog.Logger) *L4NetLB {
157159
Service: params.Service,
158160
}
159161
l4netlb := &L4NetLB{
160-
cloud: params.Cloud,
161-
scope: meta.Regional,
162-
namer: params.Namer,
163-
recorder: params.Recorder,
164-
Service: params.Service,
165-
NamespacedName: types.NamespacedName{Name: params.Service.Name, Namespace: params.Service.Namespace},
166-
backendPool: backends.NewPoolWithConnectionTrackingPolicy(params.Cloud, params.Namer, params.StrongSessionAffinityEnabled),
167-
healthChecks: healthchecksl4.NewL4HealthChecks(params.Cloud, params.Recorder, logger, params.UseDenyFirewalls),
168-
forwardingRules: forwardingRulesProvider,
169-
mixedManager: mixedManager,
170-
enableDualStack: params.DualStackEnabled,
171-
enableStrongSessionAffinity: params.StrongSessionAffinityEnabled,
172-
networkResolver: params.NetworkResolver,
173-
enableWeightedLB: params.EnableWeightedLB,
174-
enableMixedProtocol: params.EnableMixedProtocol,
175-
disableNodesFirewallProvisioning: params.DisableNodesFirewallProvisioning,
176-
useDenyFirewalls: params.UseDenyFirewalls,
177-
useNEGs: params.UseNEGs,
178-
svcLogger: logger,
179-
configMapLister: params.ConfigMapLister,
162+
cloud: params.Cloud,
163+
scope: meta.Regional,
164+
namer: params.Namer,
165+
recorder: params.Recorder,
166+
Service: params.Service,
167+
NamespacedName: types.NamespacedName{Name: params.Service.Name, Namespace: params.Service.Namespace},
168+
backendPool: backends.NewPoolWithConnectionTrackingPolicy(params.Cloud, params.Namer, params.StrongSessionAffinityEnabled),
169+
healthChecks: healthchecksl4.NewL4HealthChecks(params.Cloud, params.Recorder, logger, params.UseDenyFirewalls),
170+
forwardingRules: forwardingRulesProvider,
171+
mixedManager: mixedManager,
172+
enableDualStack: params.DualStackEnabled,
173+
enableStrongSessionAffinity: params.StrongSessionAffinityEnabled,
174+
networkResolver: params.NetworkResolver,
175+
enableWeightedLB: params.EnableWeightedLB,
176+
enableMixedProtocol: params.EnableMixedProtocol,
177+
disableNodesFirewallProvisioning: params.DisableNodesFirewallProvisioning,
178+
useDenyFirewalls: params.UseDenyFirewalls,
179+
enableDenyFirewallsRollbackCleanup: params.EnableDenyFirewallsRollbackCleanup,
180+
useNEGs: params.UseNEGs,
181+
svcLogger: logger,
182+
configMapLister: params.ConfigMapLister,
180183
}
181184
return l4netlb
182185
}
@@ -641,7 +644,7 @@ func denyFirewall(namer func(namespace, name string) string, svc *corev1.Service
641644
}
642645

643646
func (l4netlb *L4NetLB) softlyCleanUpDenyFirewallsWhenRolledBack() error {
644-
if l4netlb.useDenyFirewalls {
647+
if l4netlb.useDenyFirewalls || !l4netlb.enableDenyFirewallsRollbackCleanup {
645648
return nil // We use deny firewalls, don't need to clean them up
646649
}
647650
logger := l4netlb.svcLogger.WithName("softlyCleanUpDenyFirewalls")

0 commit comments

Comments
 (0)