From a52a0c62fdce217ae1b49df42aa8606b4646c13c Mon Sep 17 00:00:00 2001
From: Spencer Hance <shance@google.com>
Date: Wed, 11 Sep 2019 15:56:21 -0700
Subject: [PATCH] Fix ILB bugs in syncing firewall

Fixes a bug where ilb subnet is added multiple times to firewall and causes syncing failures
Fixes a bug where ilb is enabled + no ilb ingress leads to appending multiple empty strings to firewall and cases syncing failures
De-dupes firewall src ranges
---
 pkg/firewalls/controller.go     | 18 ++++++++++++------
 pkg/firewalls/firewalls.go      | 13 +++++++++----
 pkg/firewalls/firewalls_test.go |  4 ++++
 3 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/pkg/firewalls/controller.go b/pkg/firewalls/controller.go
index aab9bef9f9..2819c33444 100644
--- a/pkg/firewalls/controller.go
+++ b/pkg/firewalls/controller.go
@@ -17,9 +17,8 @@ limitations under the License.
 package firewalls
 
 import (
+	"errors"
 	"fmt"
-	"k8s.io/ingress-gce/pkg/flags"
-	"k8s.io/ingress-gce/pkg/loadbalancers/features"
 	"reflect"
 	"time"
 
@@ -32,6 +31,8 @@ import (
 	"k8s.io/ingress-gce/pkg/common/operator"
 	"k8s.io/ingress-gce/pkg/context"
 	"k8s.io/ingress-gce/pkg/controller/translator"
+	"k8s.io/ingress-gce/pkg/flags"
+	"k8s.io/ingress-gce/pkg/loadbalancers/features"
 	"k8s.io/ingress-gce/pkg/utils"
 	"k8s.io/klog"
 	"k8s.io/legacy-cloud-providers/gce"
@@ -42,6 +43,8 @@ var (
 	queueKey = &v1beta1.Ingress{
 		ObjectMeta: metav1.ObjectMeta{Name: "queueKey"},
 	}
+
+	ErrNoILBIngress = errors.New("no ILB Ingress found")
 )
 
 // FirewallController synchronizes the firewall rule for all ingresses.
@@ -170,9 +173,12 @@ func (fwc *FirewallController) sync(key string) error {
 	if flags.F.EnableL7Ilb {
 		ilbRange, err := fwc.ilbFirewallSrcRange(gceIngresses)
 		if err != nil {
-			return err
+			if err != features.ErrSubnetNotFound && err != ErrNoILBIngress {
+				return err
+			}
+		} else {
+			additionalRanges = append(additionalRanges, ilbRange)
 		}
-		additionalRanges = append(additionalRanges, ilbRange)
 	}
 
 	// Ensure firewall rule for the cluster and pass any NEG endpoint ports.
@@ -204,10 +210,10 @@ func (fwc *FirewallController) ilbFirewallSrcRange(gceIngresses []*v1beta1.Ingre
 	if ilbEnabled {
 		L7ILBSrcRange, err := features.ILBSubnetSourceRange(fwc.ctx.Cloud, fwc.ctx.Cloud.Region())
 		if err != nil {
-			return "", fmt.Errorf("error unable to get ILB subnet source ranges: %v", err)
+			return "", err
 		}
 		return L7ILBSrcRange, nil
 	}
 
-	return "", nil
+	return "", ErrNoILBIngress
 }
diff --git a/pkg/firewalls/firewalls.go b/pkg/firewalls/firewalls.go
index 05c1c9eaea..962a05c124 100644
--- a/pkg/firewalls/firewalls.go
+++ b/pkg/firewalls/firewalls.go
@@ -77,13 +77,18 @@ func (fr *FirewallRules) Sync(nodeNames, additionalPorts, additionalRanges []str
 	}
 	sort.Strings(targetTags)
 
-	ports := sets.NewString(additionalPorts...)
-	fr.srcRanges = append(fr.srcRanges, additionalRanges...)
-	ports.Insert(fr.portRanges...)
+	// De-dupe ports
+	ports := sets.NewString(fr.portRanges...)
+	ports.Insert(additionalPorts...)
+
+	// De-dupe srcRanges
+	ranges := sets.NewString(fr.srcRanges...)
+	ranges.Insert(additionalRanges...)
+
 	expectedFirewall := &compute.Firewall{
 		Name:         name,
 		Description:  "GCE L7 firewall rule",
-		SourceRanges: fr.srcRanges,
+		SourceRanges: ranges.UnsortedList(),
 		Network:      fr.cloud.NetworkURL(),
 		Allowed: []*compute.FirewallAllowed{
 			{
diff --git a/pkg/firewalls/firewalls_test.go b/pkg/firewalls/firewalls_test.go
index fadbeae415..d6aa96f8e5 100644
--- a/pkg/firewalls/firewalls_test.go
+++ b/pkg/firewalls/firewalls_test.go
@@ -141,6 +141,10 @@ func TestFirewallPoolSyncRanges(t *testing.T) {
 			desc:             "Multiple ranges",
 			additionalRanges: []string{"10.128.0.0/24", "10.132.0.0/24", "10.134.0.0/24"},
 		},
+		{
+			desc:             "Duplicate ranges",
+			additionalRanges: []string{"10.128.0.0/24", "10.132.0.0/24", "10.134.0.0/24", "10.132.0.0/24", "10.134.0.0/24"},
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {