Automated cherry pick of #84622: Create ILB firewall name with prefix "k8s-fw".

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

@@ -256,14 +256,26 @@ func (g *Cloud) ensureInternalLoadBalancerDeleted(clusterName, clusterID string,
 		return err
 	}
 
-	klog.V(2).Infof("ensureInternalLoadBalancerDeleted(%v): deleting firewall for traffic", loadBalancerName)
-	if err := ignoreNotFound(g.DeleteFirewall(loadBalancerName)); err != nil {
-		if isForbidden(err) && g.OnXPN() {
-			klog.V(2).Infof("ensureInternalLoadBalancerDeleted(%v): could not delete traffic firewall on XPN cluster. Raising event.", loadBalancerName)
-			g.raiseFirewallChangeNeededEvent(svc, FirewallToGCloudDeleteCmd(loadBalancerName, g.NetworkProjectID()))
-		} else {
+	deleteFunc := func(fwName string) error {
+		if err := ignoreNotFound(g.DeleteFirewall(fwName)); err != nil {
+			if isForbidden(err) && g.OnXPN() {
+				klog.V(2).Infof("ensureInternalLoadBalancerDeleted(%v): could not delete traffic firewall on XPN cluster. Raising event.", loadBalancerName)
+				g.raiseFirewallChangeNeededEvent(svc, FirewallToGCloudDeleteCmd(fwName, g.NetworkProjectID()))
+				return nil
+			}
 			return err
 		}
+		return nil
+	}
+	fwName := MakeFirewallName(loadBalancerName)
+	klog.V(2).Infof("ensureInternalLoadBalancerDeleted(%v): deleting firewall %s for traffic",
+		loadBalancerName, fwName)
+	if err := deleteFunc(fwName); err != nil {
+		return err
+	}
+	klog.V(2).Infof("ensureInternalLoadBalancerDeleted(%v): deleting legacy name firewall for traffic", loadBalancerName)
+	if err := deleteFunc(loadBalancerName); err != nil {
+		return err
 	}
 
 	hcName := makeHealthCheckName(loadBalancerName, clusterID, sharedHealthCheck)
@@ -325,7 +337,7 @@ func (g *Cloud) teardownInternalHealthCheckAndFirewall(svc *v1.Service, hcName s
 	return nil
 }
 
-func (g *Cloud) ensureInternalFirewall(svc *v1.Service, fwName, fwDesc string, sourceRanges []string, ports []string, protocol v1.Protocol, nodes []*v1.Node) error {
+func (g *Cloud) ensureInternalFirewall(svc *v1.Service, fwName, fwDesc string, sourceRanges []string, ports []string, protocol v1.Protocol, nodes []*v1.Node, legacyFwName string) error {
 	klog.V(2).Infof("ensureInternalFirewall(%v): checking existing firewall", fwName)
 	targetTags, err := g.GetNodeTags(nodeNames(nodes))
 	if err != nil {
@@ -336,6 +348,29 @@ func (g *Cloud) ensureInternalFirewall(svc *v1.Service, fwName, fwDesc string, s
 	if err != nil && !isNotFound(err) {
 		return err
 	}
+	// TODO(84821) Remove legacyFwName logic after 3 releases, so there would have been atleast 2 master upgrades that would
+	// have triggered service sync and deletion of the legacy rules.
+	if legacyFwName != "" {
+		// Check for firewall named with the legacy naming scheme and delete if found.
+		legacyFirewall, err := g.GetFirewall(legacyFwName)
+		if err != nil && !isNotFound(err) {
+			return err
+		}
+		if legacyFirewall != nil && existingFirewall != nil {
+			// Delete the legacyFirewall rule if the new one was already created. If not, it will be deleted in the
+			// next sync or when the service is deleted.
+			defer func() {
+				err = g.DeleteFirewall(legacyFwName)
+				if err != nil {
+					klog.Errorf("Failed to delete legacy firewall %s for service %s/%s, err %v",
+						legacyFwName, svc.Namespace, svc.Name, err)
+				} else {
+					klog.V(2).Infof("Successfully deleted legacy firewall %s for service %s/%s",
+						legacyFwName, svc.Namespace, svc.Name)
+				}
+			}()
+		}
+	}
 
 	expectedFirewall := &compute.Firewall{
 		Name:         fwName,
@@ -384,15 +419,15 @@ func (g *Cloud) ensureInternalFirewalls(loadBalancerName, ipAddress, clusterID s
 	if err != nil {
 		return err
 	}
-	err = g.ensureInternalFirewall(svc, loadBalancerName, fwDesc, sourceRanges.StringSlice(), ports, protocol, nodes)
+	err = g.ensureInternalFirewall(svc, MakeFirewallName(loadBalancerName), fwDesc, sourceRanges.StringSlice(), ports, protocol, nodes, loadBalancerName)
 	if err != nil {
 		return err
 	}
 
 	// Second firewall is for health checking nodes / services
 	fwHCName := makeHealthCheckFirewallName(loadBalancerName, clusterID, sharedHealthCheck)
 	hcSrcRanges := LoadBalancerSrcRanges()
-	return g.ensureInternalFirewall(svc, fwHCName, "", hcSrcRanges, []string{healthCheckPort}, v1.ProtocolTCP, nodes)
+	return g.ensureInternalFirewall(svc, fwHCName, "", hcSrcRanges, []string{healthCheckPort}, v1.ProtocolTCP, nodes, "")
 }
 
 func (g *Cloud) ensureInternalHealthCheck(name string, svcName types.NamespacedName, shared bool, path string, port int32) (*compute.HealthCheck, error) {

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

@@ -255,7 +255,7 @@ func TestEnsureInternalLoadBalancerClearPreviousResources(t *testing.T) {
 	rule, _ := gce.GetRegionForwardingRule(lbName, gce.region)
 	assert.NotEqual(t, existingFwdRule, rule)
 
-	firewall, err := gce.GetFirewall(lbName)
+	firewall, err := gce.GetFirewall(MakeFirewallName(lbName))
 	require.NoError(t, err)
 	assert.NotEqual(t, firewall, existingFirewall)
 
@@ -539,12 +539,87 @@ func TestClearPreviousInternalResources(t *testing.T) {
 	assert.Nil(t, hc1, "HealthCheck should be deleted.")
 }
 
+func TestEnsureInternalFirewallDeletesLegacyFirewall(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)
+
+	c := gce.c.(*cloud.MockGCE)
+	c.MockFirewalls.InsertHook = nil
+	c.MockFirewalls.UpdateHook = nil
+
+	nodes, err := createAndInsertNodes(gce, []string{"test-node-1"}, vals.ZoneName)
+	require.NoError(t, err)
+	sourceRange := []string{"10.0.0.0/20"}
+	// Manually create a firewall rule with the legacy name - lbName
+	gce.ensureInternalFirewall(
+		svc,
+		lbName,
+		"firewall with legacy name",
+		sourceRange,
+		[]string{"123"},
+		v1.ProtocolTCP,
+		nodes,
+		"")
+	if err != nil {
+		t.Errorf("Unexpected error %v when ensuring legacy firewall %s for svc %+v", err, lbName, svc)
+	}
+
+	// Now ensure the firewall again with the correct name to simulate a sync after updating to new code.
+	err = gce.ensureInternalFirewall(
+		svc,
+		fwName,
+		"firewall with new name",
+		sourceRange,
+		[]string{"123", "456"},
+		v1.ProtocolTCP,
+		nodes,
+		lbName)
+	if err != nil {
+		t.Errorf("Unexpected error %v when ensuring firewall %s for svc %+v", err, fwName, svc)
+	}
+
+	existingFirewall, err := gce.GetFirewall(fwName)
+	require.Nil(t, err)
+	require.NotNil(t, existingFirewall)
+	// Existing firewall will not be deleted yet since this was the first sync with the new rule created.
+	existingLegacyFirewall, err := gce.GetFirewall(lbName)
+	require.Nil(t, err)
+	require.NotNil(t, existingLegacyFirewall)
+
+	// Now ensure the firewall again to simulate a second sync where the old rule will be deleted.
+	err = gce.ensureInternalFirewall(
+		svc,
+		fwName,
+		"firewall with new name",
+		sourceRange,
+		[]string{"123", "456", "789"},
+		v1.ProtocolTCP,
+		nodes,
+		lbName)
+	if err != nil {
+		t.Errorf("Unexpected error %v when ensuring firewall %s for svc %+v", err, fwName, svc)
+	}
+
+	existingFirewall, err = gce.GetFirewall(fwName)
+	require.Nil(t, err)
+	require.NotNil(t, existingFirewall)
+	existingLegacyFirewall, err = gce.GetFirewall(lbName)
+	require.NotNil(t, err)
+	require.Nil(t, existingLegacyFirewall)
+
+}
+
 func TestEnsureInternalFirewallSucceedsOnXPN(t *testing.T) {
 	gce, err := fakeGCECloud(DefaultTestClusterValues())
 	require.NoError(t, err)
 	vals := DefaultTestClusterValues()
 	svc := fakeLoadbalancerService(string(LBTypeInternal))
-	fwName := gce.GetLoadBalancerName(context.TODO(), "", svc)
+	lbName := gce.GetLoadBalancerName(context.TODO(), "", svc)
+	fwName := MakeFirewallName(lbName)
 
 	c := gce.c.(*cloud.MockGCE)
 	c.MockFirewalls.InsertHook = mock.InsertFirewallsUnauthorizedErrHook
@@ -565,7 +640,8 @@ func TestEnsureInternalFirewallSucceedsOnXPN(t *testing.T) {
 		sourceRange,
 		[]string{"123"},
 		v1.ProtocolTCP,
-		nodes)
+		nodes,
+		lbName)
 	require.Nil(t, err, "Should success when XPN is on.")
 
 	checkEvent(t, recorder, FilewallChangeMsg, true)
@@ -582,7 +658,8 @@ func TestEnsureInternalFirewallSucceedsOnXPN(t *testing.T) {
 		sourceRange,
 		[]string{"123"},
 		v1.ProtocolTCP,
-		nodes)
+		nodes,
+		lbName)
 	require.Nil(t, err)
 	existingFirewall, err := gce.GetFirewall(fwName)
 	require.Nil(t, err)
@@ -600,7 +677,8 @@ func TestEnsureInternalFirewallSucceedsOnXPN(t *testing.T) {
 		sourceRange,
 		[]string{"123"},
 		v1.ProtocolTCP,
-		nodes)
+		nodes,
+		lbName)
 	require.Nil(t, err, "Should success when XPN is on.")
 
 	checkEvent(t, recorder, FilewallChangeMsg, true)

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

@@ -204,7 +204,7 @@ func assertInternalLbResources(t *testing.T, gce *Cloud, apiService *v1.Service,
 
 	// Check that Firewalls are created for the LoadBalancer and the HealthCheck
 	fwNames := []string{
-		lbName, // Firewalls for internal LBs are named the same name as the loadbalancer.
+		MakeFirewallName(lbName),
 		makeHealthCheckFirewallName(lbName, vals.ClusterID, true),
 	}