Skip to content

Commit bf975f0

Browse files
maciejriedlmaciejriedl
committed
Make sure that during gcpfirewalls cr creation, sync errors are not returned in Shadow run (when l7LB still reconciles the FWs).
Also fix dryRun flag to properly mean "testing" flow (the logic of the flag usage has not been changed)
1 parent daa7c4f commit bf975f0

File tree

3 files changed

+92
-20
lines changed

3 files changed

+92
-20
lines changed

‎pkg/firewalls/controller.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ func NewFirewallController(
106106
sourceRanges := lbSourceRanges(logger, flags.F.OverrideHealthCheckSourceCIDRs)
107107
logger.Info("Using source ranges", "ranges", sourceRanges)
108108
if enableCR {
109-
firewallCRPool := NewFirewallCRPool(ctx.FirewallClient, ctx.Cloud, ctx.ClusterNamer, sourceRanges, portRanges, disableFWEnforcement, logger)
109+
firewallCRPool := NewFirewallCRPool(ctx.FirewallClient, ctx.Cloud, ctx.ClusterNamer, sourceRanges, portRanges, !disableFWEnforcement, logger)
110110
compositeFirewallPool.pools = append(compositeFirewallPool.pools, firewallCRPool)
111111
}
112112
if !disableFWEnforcement {

‎pkg/firewalls/firewalls_l7_cr.go‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,10 @@ type FirewallCR struct {
4141
// all the port ranges to open with each call to Sync()
4242
nodePortRanges []string
4343
firewallClient firewallclient.Interface
44-
dryRun bool
44+
// If set to true, CRs are marked as "disabled" and errors are not
45+
// propagated. Firewalls should be still created/updated by l7LB, not the
46+
// Platform Firewall
47+
dryRun bool
4548

4649
logger klog.Logger
4750
}
@@ -81,17 +84,17 @@ func (fr *FirewallCR) Sync(nodeNames, additionalPorts, additionalRanges []string
8184
ranges.Insert(additionalRanges...)
8285

8386
fr.logger.V(3).Info("Firewall CR is enabled.")
84-
expectedFirewallCR, err := NewFirewallCR(name, ports.List(), ranges.UnsortedList(), []string{}, fr.dryRun)
87+
expectedFirewallCR, err := NewFirewallCR(name, ports.List(), ranges.UnsortedList(), []string{}, !fr.dryRun)
8588
if err != nil {
8689
return err
8790
}
88-
return ensureFirewallCR(fr.firewallClient, expectedFirewallCR, fr.logger)
91+
return ensureFirewallCR(fr.firewallClient, expectedFirewallCR, fr.logger, fr.dryRun)
8992
}
9093

9194
// ensureFirewallCR creates/updates the firewall CR
9295
// On CR update, it will read the conditions to see if there are errors updated by PFW controller.
9396
// If the Spec was updated by others, it will reconcile the Spec.
94-
func ensureFirewallCR(client firewallclient.Interface, expectedFWCR *gcpfirewallv1.GCPFirewall, logger klog.Logger) error {
97+
func ensureFirewallCR(client firewallclient.Interface, expectedFWCR *gcpfirewallv1.GCPFirewall, logger klog.Logger, dryRun bool) error {
9598
fw := client.NetworkingV1().GCPFirewalls()
9699
currentFWCR, err := fw.Get(context.Background(), expectedFWCR.Name, metav1.GetOptions{})
97100
logger.V(3).Info("ensureFirewallCR Get CR", "currentFirewallCR", fmt.Sprintf("%+v", currentFWCR), "err", err)
@@ -103,6 +106,10 @@ func ensureFirewallCR(client firewallclient.Interface, expectedFWCR *gcpfirewall
103106
}
104107
return err
105108
}
109+
if currentFWCR.DeletionTimestamp != nil {
110+
logger.V(3).Info("ensureFirewallCR: The CR contains DeletionTimestamp. Skipping Ensure", "currentFirewallDeletionTimestamp", fmt.Sprintf("%+v", currentFWCR.DeletionTimestamp))
111+
return nil
112+
}
106113
if !reflect.DeepEqual(currentFWCR.Spec, expectedFWCR.Spec) {
107114
// Update the current firewall CR
108115
logger.V(3).Info("ensureFirewallCR Update CR", "currentFirewallCR", fmt.Sprintf("%+v", currentFWCR.Spec), "expectedFirewallCR", fmt.Sprintf("%+v", expectedFWCR.Spec))
@@ -116,7 +123,11 @@ func ensureFirewallCR(client firewallclient.Interface, expectedFWCR *gcpfirewall
116123
con.Reason == string(gcpfirewallv1.FirewallRuleReasonSyncError) {
117124
// Use recorder to emit the cmd in Sync()
118125
logger.V(3).Info("ensureFirewallCR: Could not enforce Firewall CR", "currentFirewallCRName", currentFWCR.Name, "reason", con.Reason)
119-
return fmt.Errorf(con.Reason)
126+
if dryRun {
127+
return nil
128+
} else {
129+
return fmt.Errorf(con.Reason)
130+
}
120131
}
121132
}
122133
return nil

‎pkg/firewalls/firewalls_test.go‎

Lines changed: 75 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"strings"
2424
"testing"
2525

26+
gcpfirewallv1 "github.com/GoogleCloudPlatform/gke-networking-api/apis/gcpfirewall/v1"
2627
"k8s.io/klog/v2"
2728

2829
firewallclient "github.com/GoogleCloudPlatform/gke-networking-api/client/gcpfirewall/clientset/versioned/fake"
@@ -58,7 +59,7 @@ func TestFirewallPoolSync(t *testing.T) {
5859
if err := fcrp.Sync(nodes, nil, nil, true); err != nil {
5960
t.Fatal(err)
6061
}
61-
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, t)
62+
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, true, t)
6263

6364
}
6465

@@ -76,23 +77,23 @@ func TestFirewallPoolSyncNodes(t *testing.T) {
7677
if err := fcrp.Sync(nodes, nil, nil, true); err != nil {
7778
t.Fatal(err)
7879
}
79-
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, t)
80+
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, true, t)
8081

8182
// Add nodes
8283
nodes = append(nodes, "node-d", "node-e")
8384
if err := fp.Sync(nodes, nil, nil, true); err != nil {
8485
t.Errorf("unexpected err when syncing firewall, err: %v", err)
8586
}
8687
verifyFirewallRule(fwp, ruleName, nodes, srcRanges, portRanges(), t)
87-
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, t)
88+
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, true, t)
8889

8990
// Remove nodes
9091
nodes = []string{"node-a", "node-c"}
9192
if err := fp.Sync(nodes, nil, nil, true); err != nil {
9293
t.Errorf("unexpected err when syncing firewall, err: %v", err)
9394
}
9495
verifyFirewallRule(fwp, ruleName, nodes, srcRanges, portRanges(), t)
95-
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, t)
96+
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, true, t)
9697
}
9798

9899
func TestFirewallPoolSyncSrcRanges(t *testing.T) {
@@ -112,7 +113,7 @@ func TestFirewallPoolSyncSrcRanges(t *testing.T) {
112113
t.Fatal(err)
113114
}
114115

115-
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, t)
116+
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, true, t)
116117

117118
// Manually modify source ranges to bad values.
118119
f, _ := fwp.GetFirewall(ruleName)
@@ -125,7 +126,7 @@ func TestFirewallPoolSyncSrcRanges(t *testing.T) {
125126
t.Errorf("unexpected err when syncing firewall, err: %v", err)
126127
}
127128
verifyFirewallRule(fwp, ruleName, nodes, srcRanges, portRanges(), t)
128-
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, t)
129+
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, true, t)
129130
}
130131

131132
func TestFirewallPoolSyncPorts(t *testing.T) {
@@ -146,7 +147,7 @@ func TestFirewallPoolSyncPorts(t *testing.T) {
146147
if err := fcrp.Sync(nodes, nil, nil, true); err != nil {
147148
t.Fatal(err)
148149
}
149-
verifyFirewallCR(fwClient, ruleName, srcRanges, emptyPortRanges, true, t)
150+
verifyFirewallCR(fwClient, ruleName, srcRanges, emptyPortRanges, true, true, t)
150151

151152
// Verify a preset ports' list
152153
fp = NewFirewallPool(fwp, defaultNamer, srcRanges, portRanges(), klog.TODO())
@@ -160,7 +161,7 @@ func TestFirewallPoolSyncPorts(t *testing.T) {
160161
if err := fcrp.Sync(nodes, nil, nil, true); err != nil {
161162
t.Fatal(err)
162163
}
163-
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, t)
164+
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, true, t)
164165

165166
// Manually modify port list to bad values.
166167
f, _ := fwp.GetFirewall(ruleName)
@@ -178,7 +179,7 @@ func TestFirewallPoolSyncPorts(t *testing.T) {
178179
if err := fcrp.Sync(nodes, nil, nil, true); err != nil {
179180
t.Fatal(err)
180181
}
181-
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, t)
182+
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, true, t)
182183

183184
// Verify additional ports are included
184185
negTargetports := []string{"80", "443", "8080"}
@@ -190,7 +191,7 @@ func TestFirewallPoolSyncPorts(t *testing.T) {
190191
if err := fcrp.Sync(nodes, negTargetports, nil, true); err != nil {
191192
t.Fatal(err)
192193
}
193-
verifyFirewallCR(fwClient, ruleName, srcRanges, append(portRanges(), negTargetports...), true, t)
194+
verifyFirewallCR(fwClient, ruleName, srcRanges, append(portRanges(), negTargetports...), true, true, t)
194195

195196
if err := fp.Sync(nodes, negTargetports, nil, false); err != nil {
196197
t.Errorf("unexpected err when syncing firewall, err: %v", err)
@@ -200,7 +201,50 @@ func TestFirewallPoolSyncPorts(t *testing.T) {
200201
if err := fcrp.Sync(nodes, negTargetports, nil, false); err != nil {
201202
t.Fatal(err)
202203
}
203-
verifyFirewallCR(fwClient, ruleName, srcRanges, negTargetports, true, t)
204+
verifyFirewallCR(fwClient, ruleName, srcRanges, negTargetports, true, true, t)
205+
}
206+
207+
func TestFirewallCRSyncDryRun(t *testing.T) {
208+
t.Parallel()
209+
testCases := []struct {
210+
desc string
211+
dryRun bool
212+
wantError bool
213+
}{
214+
{
215+
desc: "dry run",
216+
dryRun: true,
217+
wantError: false,
218+
},
219+
{
220+
desc: "full mode",
221+
dryRun: false,
222+
wantError: true,
223+
},
224+
}
225+
for _, tc := range testCases {
226+
t.Run(tc.desc, func(t *testing.T) {
227+
fwp := NewFakeFirewallsProvider(false, false)
228+
nodes := []string{"node-a", "node-b", "node-c"}
229+
230+
fwClient := firewallclient.NewSimpleClientset()
231+
fcrp := NewFirewallCRPool(fwClient, fwp, defaultNamer, srcRanges, portRanges(), tc.dryRun, klog.TODO())
232+
// Create CR
233+
if err := fcrp.Sync(nodes, nil, nil, true); err != nil {
234+
t.Fatal(err)
235+
}
236+
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, tc.dryRun, t)
237+
238+
markCRWithReconciliationError(fwClient, t)
239+
240+
err := fcrp.Sync(nodes, nil, nil, true)
241+
if (err != nil) != tc.wantError {
242+
t.Errorf("Sync() = %v, want error %v", err, tc.wantError)
243+
}
244+
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, tc.dryRun, t)
245+
})
246+
}
247+
204248
}
205249

206250
func TestFirewallPoolSyncGetServerError(t *testing.T) {
@@ -266,7 +310,7 @@ func TestFirewallPoolSyncRanges(t *testing.T) {
266310
if err := fcrp.Sync(nodes, nil, tc.additionalRanges, true); err != nil {
267311
t.Fatal(err)
268312
}
269-
verifyFirewallCR(fwClient, ruleName, resultRanges, portRanges(), true, t)
313+
verifyFirewallCR(fwClient, ruleName, resultRanges, portRanges(), true, true, t)
270314
})
271315
}
272316
}
@@ -285,7 +329,7 @@ func TestFirewallPoolGC(t *testing.T) {
285329
if err := fcrp.Sync(nodes, nil, nil, true); err != nil {
286330
t.Fatal(err)
287331
}
288-
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, t)
332+
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, true, t)
289333

290334
if err := fp.GC(); err != nil {
291335
t.Fatal(err)
@@ -394,7 +438,7 @@ func validateXPNError(err error, op string, t *testing.T) {
394438
}
395439
}
396440

397-
func verifyFirewallCR(firewallclient *firewallclient.Clientset, ruleName string, sourceRanges, expectedPorts []string, crEnabled bool, t *testing.T) {
441+
func verifyFirewallCR(firewallclient *firewallclient.Clientset, ruleName string, sourceRanges, expectedPorts []string, crEnabled bool, dryRun bool, t *testing.T) {
398442
if !crEnabled {
399443
fw := firewallclient.NetworkingV1().GCPFirewalls()
400444
actualFW, _ := fw.Get(context.TODO(), ruleName, metav1.GetOptions{})
@@ -413,6 +457,11 @@ func verifyFirewallCR(firewallclient *firewallclient.Clientset, ruleName string,
413457
if actualFW.Spec.Action != "ALLOW" {
414458
t.Errorf("Action isn't ALLOW")
415459
}
460+
461+
if actualFW.Spec.Disabled != dryRun {
462+
t.Errorf("Spec.Disabled should equal {%v}", dryRun)
463+
}
464+
416465
ports := sets.NewString(expectedPorts...)
417466
srcranges := sets.NewString(sourceRanges...)
418467

@@ -472,3 +521,15 @@ func verifyFirewallRule(fwp *fakeFirewallsProvider, ruleName string, expectedNod
472521
t.Errorf("source CIDRs doesn't equal expected CIDRs. Actual: %v, Expected: %v", f.SourceRanges, sourceRanges)
473522
}
474523
}
524+
525+
func markCRWithReconciliationError(firewallclient *firewallclient.Clientset, t *testing.T) {
526+
fw, _ := firewallclient.NetworkingV1().GCPFirewalls().Get(context.TODO(), ruleName, metav1.GetOptions{})
527+
if fw == nil {
528+
t.Errorf("firewallCR not found")
529+
}
530+
fw.Status.Conditions = []metav1.Condition{{Reason: string(gcpfirewallv1.FirewallRuleReasonSyncError)}}
531+
_, err := firewallclient.NetworkingV1().GCPFirewalls().Update(context.TODO(), fw, metav1.UpdateOptions{})
532+
if err != nil {
533+
t.Errorf("could not update firewall CR, err %v", err)
534+
}
535+
}

0 commit comments

Comments
 (0)