Merge pull request #111186 from sugangli/pinhole-fw

Fix firewallneedsupdate and firewallRuleEqual

staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_external.go

@@ -23,6 +23,7 @@ import (
 	"context"
 	"fmt"
 	"net/http"
+	"reflect"
 	"strconv"
 	"strings"
 
@@ -902,6 +903,13 @@ func (g *Cloud) firewallNeedsUpdate(name, serviceName, ipAddress string, ports [
 	if !sourceRanges.Equal(actualSourceRanges) {
 		return true, true, nil
 	}
+
+	destinationRanges := []string{ipAddress}
+
+	if !reflect.DeepEqual(destinationRanges, fw.DestinationRanges) {
+		return true, true, nil
+	}
+
 	return true, false, nil
 }
 

staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_external_test.go

@@ -1044,6 +1044,18 @@ func TestFirewallNeedsUpdate(t *testing.T) {
 			needsUpdate:  true,
 			hasErr:       false,
 		},
+		"When the destination ranges are not equal.": {
+			lbName:       lbName,
+			ipAddr:       "8.8.8.8",
+			ports:        svc.Spec.Ports,
+			ipnet:        ipnet,
+			fwIPProtocol: "tcp",
+			getHook:      nil,
+			sourceRange:  fw.SourceRanges[0],
+			exists:       true,
+			needsUpdate:  true,
+			hasErr:       false,
+		},
 		"When basic flow without exceptions.": {
 			lbName:       lbName,
 			ipAddr:       ipAddr,
@@ -1139,6 +1151,7 @@ func TestFirewallNeedsUpdate(t *testing.T) {
 			fw.SourceRanges[0] = tc.sourceRange
 			fw, err = gce.GetFirewall(MakeFirewallName(lbName))
 			require.Equal(t, fw.SourceRanges[0], tc.sourceRange)
+			require.Equal(t, fw.DestinationRanges[0], status.Ingress[0].IP)
 
 			c := gce.c.(*cloud.MockGCE)
 			c.MockFirewalls.GetHook = tc.getHook

staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go

@@ -776,6 +776,7 @@ func firewallRuleEqual(a, b *compute.Firewall) bool {
 		a.Allowed[0].IPProtocol == b.Allowed[0].IPProtocol &&
 		equalStringSets(a.Allowed[0].Ports, b.Allowed[0].Ports) &&
 		equalStringSets(a.SourceRanges, b.SourceRanges) &&
+		equalStringSets(a.DestinationRanges, b.DestinationRanges) &&
 		equalStringSets(a.TargetTags, b.TargetTags)
 }
 

staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go

@@ -1690,7 +1690,7 @@ func TestEnsureInternalFirewallPortRanges(t *testing.T) {
 	destinationIP := "10.1.2.3"
 	sourceRange := []string{"10.0.0.0/20"}
 	// Manually create a firewall rule with the legacy name - lbName
-	gce.ensureInternalFirewall(
+	err = gce.ensureInternalFirewall(
 		svc,
 		fwName,
 		"firewall with legacy name",
@@ -1713,6 +1713,65 @@ func TestEnsureInternalFirewallPortRanges(t *testing.T) {
 	}
 }
 
+func TestEnsureInternalFirewallDestinations(t *testing.T) {
+	gce, err := fakeGCECloud(DefaultTestClusterValues())
+	require.NoError(t, err)
+	vals := DefaultTestClusterValues()
+	svc := fakeLoadbalancerService(string(LBTypeInternal))
+	lbName := gce.GetLoadBalancerName(context.TODO(), "", svc)
+	fwName := MakeFirewallName(lbName)
+
+	nodes, err := createAndInsertNodes(gce, []string{"test-node-1"}, vals.ZoneName)
+	require.NoError(t, err)
+
+	destinationIP := "10.1.2.3"
+	sourceRange := []string{"10.0.0.0/20"}
+
+	err = gce.ensureInternalFirewall(
+		svc,
+		fwName,
+		"firewall with legacy name",
+		destinationIP,
+		sourceRange,
+		[]string{"8080"},
+		v1.ProtocolTCP,
+		nodes,
+		"")
+	if err != nil {
+		t.Errorf("Unexpected error %v when ensuring firewall %s for svc %+v", err, fwName, svc)
+	}
+	existingFirewall, err := gce.GetFirewall(fwName)
+	if err != nil || existingFirewall == nil || len(existingFirewall.Allowed) == 0 {
+		t.Errorf("Unexpected error %v when looking up firewall %s, Got firewall %+v", err, fwName, existingFirewall)
+	}
+
+	newDestinationIP := "20.1.2.3"
+
+	err = gce.ensureInternalFirewall(
+		svc,
+		fwName,
+		"firewall with legacy name",
+		newDestinationIP,
+		sourceRange,
+		[]string{"8080"},
+		v1.ProtocolTCP,
+		nodes,
+		"")
+	if err != nil {
+		t.Errorf("Unexpected error %v when ensuring firewall %s for svc %+v", err, fwName, svc)
+	}
+
+	updatedFirewall, err := gce.GetFirewall(fwName)
+	if err != nil || updatedFirewall == nil || len(updatedFirewall.Allowed) == 0 {
+		t.Errorf("Unexpected error %v when looking up firewall %s, Got firewall %+v", err, fwName, existingFirewall)
+	}
+
+	if reflect.DeepEqual(existingFirewall.DestinationRanges, updatedFirewall.DestinationRanges) {
+		t.Errorf("DestinationRanges is not updated. existingFirewall.DestinationRanges: %v, updatedFirewall.DestinationRanges: %v", existingFirewall.DestinationRanges, updatedFirewall.DestinationRanges)
+	}
+
+}
+
 func TestEnsureInternalLoadBalancerFinalizer(t *testing.T) {
 	t.Parallel()
 

staging/src/k8s.io/legacy-cloud-providers/gce/gce_util.go

@@ -78,6 +78,7 @@ func fakeGCECloud(vals TestClusterValues) (*Cloud, error) {
 	mockGCE.MockRegionBackendServices.UpdateHook = mock.UpdateRegionBackendServiceHook
 	mockGCE.MockHealthChecks.UpdateHook = mock.UpdateHealthCheckHook
 	mockGCE.MockFirewalls.UpdateHook = mock.UpdateFirewallHook
+	mockGCE.MockFirewalls.PatchHook = mock.UpdateFirewallHook
 
 	keyGA := meta.GlobalKey("key-ga")
 	mockGCE.MockZones.Objects[*keyGA] = &cloud.MockZonesObj{