Skip to content

Safeguard gcpfirewalls cr creation to not return sync errors in Shadow run #2905

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Safeguard gcpfirewalls cr creation to not return sync errors in Shadow run #2905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/firewalls/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ func NewFirewallController(
sourceRanges := lbSourceRanges(logger, flags.F.OverrideHealthCheckSourceCIDRs)
logger.Info("Using source ranges", "ranges", sourceRanges)
if enableCR {
firewallCRPool := NewFirewallCRPool(ctx.FirewallClient, ctx.Cloud, ctx.ClusterNamer, sourceRanges, portRanges, disableFWEnforcement, logger)
firewallCRPool := NewFirewallCRPool(ctx.FirewallClient, ctx.Cloud, ctx.ClusterNamer, sourceRanges, portRanges, !disableFWEnforcement, logger)
compositeFirewallPool.pools = append(compositeFirewallPool.pools, firewallCRPool)
}
if !disableFWEnforcement {
Expand Down
23 changes: 17 additions & 6 deletions pkg/firewalls/firewalls_l7_cr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@ type FirewallCR struct {
// all the port ranges to open with each call to Sync()
nodePortRanges []string
firewallClient firewallclient.Interface
dryRun bool
// If set to true, CRs are marked as "disabled" and errors are not
// propagated. Firewalls should be still created/updated by L7LB, not the
// Platform Firewall
dryRun bool

logger klog.Logger
}
Expand Down Expand Up @@ -85,13 +88,13 @@ func (fr *FirewallCR) Sync(nodeNames, additionalPorts, additionalRanges []string
if err != nil {
return err
}
return ensureFirewallCR(fr.firewallClient, expectedFirewallCR, fr.logger)
return ensureFirewallCR(fr.firewallClient, expectedFirewallCR, fr.logger, fr.dryRun)
}

// ensureFirewallCR creates/updates the firewall CR
// On CR update, it will read the conditions to see if there are errors updated by PFW controller.
// If the Spec was updated by others, it will reconcile the Spec.
func ensureFirewallCR(client firewallclient.Interface, expectedFWCR *gcpfirewallv1.GCPFirewall, logger klog.Logger) error {
func ensureFirewallCR(client firewallclient.Interface, expectedFWCR *gcpfirewallv1.GCPFirewall, logger klog.Logger, dryRun bool) error {
fw := client.NetworkingV1().GCPFirewalls()
currentFWCR, err := fw.Get(context.Background(), expectedFWCR.Name, metav1.GetOptions{})
logger.V(3).Info("ensureFirewallCR Get CR", "currentFirewallCR", fmt.Sprintf("%+v", currentFWCR), "err", err)
Expand All @@ -103,6 +106,10 @@ func ensureFirewallCR(client firewallclient.Interface, expectedFWCR *gcpfirewall
}
return err
}
if currentFWCR.DeletionTimestamp != nil {
logger.V(3).Info("ensureFirewallCR: The CR contains DeletionTimestamp. Skipping Ensure", "currentFirewallDeletionTimestamp", fmt.Sprintf("%+v", currentFWCR.DeletionTimestamp))
return nil
}
if !reflect.DeepEqual(currentFWCR.Spec, expectedFWCR.Spec) {
// Update the current firewall CR
logger.V(3).Info("ensureFirewallCR Update CR", "currentFirewallCR", fmt.Sprintf("%+v", currentFWCR.Spec), "expectedFirewallCR", fmt.Sprintf("%+v", expectedFWCR.Spec))
Expand All @@ -116,7 +123,11 @@ func ensureFirewallCR(client firewallclient.Interface, expectedFWCR *gcpfirewall
con.Reason == string(gcpfirewallv1.FirewallRuleReasonSyncError) {
// Use recorder to emit the cmd in Sync()
logger.V(3).Info("ensureFirewallCR: Could not enforce Firewall CR", "currentFirewallCRName", currentFWCR.Name, "reason", con.Reason)
return fmt.Errorf(con.Reason)
if dryRun {
return nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we not want to return this error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the point of the dryRun - If we are in the "testing" flow, we don't want CRs to eventually cause disruptions to the L7LB (error in PFW could cause L7LB to receive infinte errors)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should whether we use the error or not happen at the caller instead of here?

Also wondering should we add a metric for the error so we can monitor this. There is the log line, but it will be hard to see that we are doing okay like that

} else {
return fmt.Errorf(con.Reason)
}
}
}
return nil
Expand All @@ -130,14 +141,14 @@ func deleteFirewallCR(client firewallclient.Interface, name string, logger klog.
}

// NewFirewallCR constructs the firewall CR from name, ports and ranges
func NewFirewallCR(name string, ports, srcRanges, dstRanges []string, enforced bool) (*gcpfirewallv1.GCPFirewall, error) {
func NewFirewallCR(name string, ports, srcRanges, dstRanges []string, dryRun bool) (*gcpfirewallv1.GCPFirewall, error) {
firewallCR := &gcpfirewallv1.GCPFirewall{
ObjectMeta: metav1.ObjectMeta{
Name: name,
},
Spec: gcpfirewallv1.GCPFirewallSpec{
Action: gcpfirewallv1.ActionAllow,
Disabled: !enforced,
Disabled: dryRun,
},
}
var protocolPorts []gcpfirewallv1.ProtocolPort
Expand Down
89 changes: 75 additions & 14 deletions pkg/firewalls/firewalls_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"strings"
"testing"

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

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

}

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

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

// Remove nodes
nodes = []string{"node-a", "node-c"}
if err := fp.Sync(nodes, nil, nil, true); err != nil {
t.Errorf("unexpected err when syncing firewall, err: %v", err)
}
verifyFirewallRule(fwp, ruleName, nodes, srcRanges, portRanges(), t)
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, t)
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, true, t)
}

func TestFirewallPoolSyncSrcRanges(t *testing.T) {
Expand All @@ -112,7 +113,7 @@ func TestFirewallPoolSyncSrcRanges(t *testing.T) {
t.Fatal(err)
}

verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, t)
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, true, t)

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

func TestFirewallPoolSyncPorts(t *testing.T) {
Expand All @@ -146,7 +147,7 @@ func TestFirewallPoolSyncPorts(t *testing.T) {
if err := fcrp.Sync(nodes, nil, nil, true); err != nil {
t.Fatal(err)
}
verifyFirewallCR(fwClient, ruleName, srcRanges, emptyPortRanges, true, t)
verifyFirewallCR(fwClient, ruleName, srcRanges, emptyPortRanges, true, true, t)

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

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

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

if err := fp.Sync(nodes, negTargetports, nil, false); err != nil {
t.Errorf("unexpected err when syncing firewall, err: %v", err)
Expand All @@ -200,7 +201,50 @@ func TestFirewallPoolSyncPorts(t *testing.T) {
if err := fcrp.Sync(nodes, negTargetports, nil, false); err != nil {
t.Fatal(err)
}
verifyFirewallCR(fwClient, ruleName, srcRanges, negTargetports, true, t)
verifyFirewallCR(fwClient, ruleName, srcRanges, negTargetports, true, true, t)
}

func TestFirewallCRSyncDryRun(t *testing.T) {
t.Parallel()
testCases := []struct {
desc string
dryRun bool
wantError bool
}{
{
desc: "dry run",
dryRun: true,
wantError: false,
},
{
desc: "full mode",
dryRun: false,
wantError: true,
},
}
for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
fwp := NewFakeFirewallsProvider(false, false)
nodes := []string{"node-a", "node-b", "node-c"}

fwClient := firewallclient.NewSimpleClientset()
fcrp := NewFirewallCRPool(fwClient, fwp, defaultNamer, srcRanges, portRanges(), tc.dryRun, klog.TODO())
// Create CR
if err := fcrp.Sync(nodes, nil, nil, true); err != nil {
t.Fatal(err)
}
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, tc.dryRun, t)

markCRWithReconciliationError(fwClient, t)

err := fcrp.Sync(nodes, nil, nil, true)
if (err != nil) != tc.wantError {
t.Errorf("Sync() = %v, want error %v", err, tc.wantError)
}
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, tc.dryRun, t)
})
}

}

func TestFirewallPoolSyncGetServerError(t *testing.T) {
Expand Down Expand Up @@ -266,7 +310,7 @@ func TestFirewallPoolSyncRanges(t *testing.T) {
if err := fcrp.Sync(nodes, nil, tc.additionalRanges, true); err != nil {
t.Fatal(err)
}
verifyFirewallCR(fwClient, ruleName, resultRanges, portRanges(), true, t)
verifyFirewallCR(fwClient, ruleName, resultRanges, portRanges(), true, true, t)
})
}
}
Expand All @@ -285,7 +329,7 @@ func TestFirewallPoolGC(t *testing.T) {
if err := fcrp.Sync(nodes, nil, nil, true); err != nil {
t.Fatal(err)
}
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, t)
verifyFirewallCR(fwClient, ruleName, srcRanges, portRanges(), true, true, t)

if err := fp.GC(); err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -394,7 +438,7 @@ func validateXPNError(err error, op string, t *testing.T) {
}
}

func verifyFirewallCR(firewallclient *firewallclient.Clientset, ruleName string, sourceRanges, expectedPorts []string, crEnabled bool, t *testing.T) {
func verifyFirewallCR(firewallclient *firewallclient.Clientset, ruleName string, sourceRanges, expectedPorts []string, crEnabled bool, dryRun bool, t *testing.T) {
if !crEnabled {
fw := firewallclient.NetworkingV1().GCPFirewalls()
actualFW, _ := fw.Get(context.TODO(), ruleName, metav1.GetOptions{})
Expand All @@ -413,6 +457,11 @@ func verifyFirewallCR(firewallclient *firewallclient.Clientset, ruleName string,
if actualFW.Spec.Action != "ALLOW" {
t.Errorf("Action isn't ALLOW")
}

if actualFW.Spec.Disabled != dryRun {
t.Errorf("Spec.Disabled should equal {%v}", dryRun)
}

ports := sets.NewString(expectedPorts...)
srcranges := sets.NewString(sourceRanges...)

Expand Down Expand Up @@ -472,3 +521,15 @@ func verifyFirewallRule(fwp *fakeFirewallsProvider, ruleName string, expectedNod
t.Errorf("source CIDRs doesn't equal expected CIDRs. Actual: %v, Expected: %v", f.SourceRanges, sourceRanges)
}
}

func markCRWithReconciliationError(firewallclient *firewallclient.Clientset, t *testing.T) {
fw, _ := firewallclient.NetworkingV1().GCPFirewalls().Get(context.TODO(), ruleName, metav1.GetOptions{})
if fw == nil {
t.Errorf("firewallCR not found")
}
fw.Status.Conditions = []metav1.Condition{{Reason: string(gcpfirewallv1.FirewallRuleReasonSyncError)}}
_, err := firewallclient.NetworkingV1().GCPFirewalls().Update(context.TODO(), fw, metav1.UpdateOptions{})
if err != nil {
t.Errorf("could not update firewall CR, err %v", err)
}
}