review comments

controllers/gce/firewalls/firewalls.go

@@ -80,18 +80,19 @@ func (fr *FirewallRules) Sync(nodePorts []int64, nodeNames []string) error {
 	existingCIDRs := sets.NewString(rule.SourceRanges...)
 
 	// Do not update if ports and source cidrs are not outdated.
-	// NOTE: We are not checking if nodeNames matches the firwall targetTags
+	// NOTE: We are not checking if nodeNames matches the firewall targetTags
 	if requiredPorts.Equal(existingPorts) && requiredCIDRs.Equal(existingCIDRs) {
+		glog.V(4).Info("Firewall does not need update of ports or source ranges")
 		return nil
 	}
 
-	glog.V(3).Infof("Firewall rule %v already exists, updating nodeports %v", name, nodePorts)
+	glog.V(3).Infof("Firewall %v already exists, updating nodeports %v", name, nodePorts)
-	return fr.cloud.UpdateFirewall(suffix, "GCE L7 firewall rule", fr.srcRanges, nodePorts, nodeNames)
+	return fr.cloud.UpdateFirewall(suffix, "GCE L7 firewall", fr.srcRanges, nodePorts, nodeNames)
 }
 
 // Shutdown shuts down this firewall rules manager.
 func (fr *FirewallRules) Shutdown() error {
-	glog.Infof("Deleting firewall rule with suffix %v", fr.namer.FrSuffix())
+	glog.Infof("Deleting firewall with suffix %v", fr.namer.FrSuffix())
 	err := fr.cloud.DeleteFirewall(fr.namer.FrSuffix())
 	if err != nil && utils.IsHTTPErrorCode(err, 404) {
 		glog.Infof("Firewall with suffix %v didn't exist at Shutdown", fr.namer.FrSuffix())

controllers/gce/firewalls/firewalls_test.go

@@ -1,3 +1,19 @@
+/*
+Copyright 2015 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
 package firewalls
 
 import (
@@ -9,6 +25,8 @@ import (
 	netset "k8s.io/kubernetes/pkg/util/net/sets"
 )
 
+const allCIDR = "0.0.0.0/0"
+
 func TestSyncFirewallPool(t *testing.T) {
 	namer := utils.NewNamer("ABC", "XYZ")
 	fwp := NewFakeFirewallsProvider(namer)
@@ -32,13 +50,12 @@ func TestSyncFirewallPool(t *testing.T) {
 	}
 	verifyFirewallRule(fwp, ruleName, nodePorts, nodes, l7SrcRanges, t)
 
-	all := "0.0.0.0/0"
+	srcRanges, _ := netset.ParseIPNets(allCIDR)
-	srcRanges, _ := netset.ParseIPNets(all)
 	err = fwp.UpdateFirewall(namer.FrSuffix(), "", srcRanges, nodePorts, nodes)
 	if err != nil {
 		t.Errorf("failed to update firewall rule, err: %v", err)
 	}
-	verifyFirewallRule(fwp, ruleName, nodePorts, nodes, []string{all}, t)
+	verifyFirewallRule(fwp, ruleName, nodePorts, nodes, []string{allCIDR}, t)
 
 	// Run Sync and expect l7 src ranges to be returned
 	err = fp.Sync(nodePorts, nodes)
@@ -47,7 +64,7 @@ func TestSyncFirewallPool(t *testing.T) {
 	}
 	verifyFirewallRule(fwp, ruleName, nodePorts, nodes, l7SrcRanges, t)
 
-	// Add node and expect firwall to remain the same
+	// Add node and expect firewall to remain the same
 	// NOTE: See computeHostTag(..) in gce cloudprovider
 	nodes = []string{"node-a", "node-b", "node-c", "node-d"}
 	err = fp.Sync(nodePorts, nodes)
@@ -81,12 +98,12 @@ func verifyFirewallRule(fwp *fakeFirewallsProvider, ruleName string, expectedPor
 		t.Errorf("could not retrieve firewall via cloud api, err %v", err)
 	}
 
-	// Verify firwall rule has correct ports
+	// Verify firewall rule has correct ports
 	if !sets.NewString(f.Allowed[0].Ports...).Equal(sets.NewString(strPorts...)) {
 		t.Errorf("allowed ports doesn't equal expected ports, Actual: %v, Expected: %v", f.Allowed[0].Ports, strPorts)
 	}
 
-	// Verify firwall rule has correct CIDRs
+	// Verify firewall rule has correct CIDRs
 	if !sets.NewString(f.SourceRanges...).Equal(sets.NewString(expectedCIDRs...)) {
 		t.Errorf("source CIDRs doesn't equal expected CIDRs. Actual: %v, Expected: %v", f.SourceRanges, expectedCIDRs)
 	}