diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go index a0b36f97aab..ad91b707d22 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go @@ -22,11 +22,11 @@ import ( "context" "encoding/json" "fmt" - "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" "strconv" "strings" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud" + "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" computebeta "google.golang.org/api/compute/v0.beta" compute "google.golang.org/api/compute/v1" "k8s.io/api/core/v1" @@ -248,14 +248,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) @@ -317,7 +329,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 { @@ -328,6 +340,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, @@ -376,7 +411,7 @@ 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 } @@ -384,7 +419,7 @@ func (g *Cloud) ensureInternalFirewalls(loadBalancerName, ipAddress, clusterID s // Second firewall is for health checking nodes / services fwHCName := makeHealthCheckFirewallName(loadBalancerName, clusterID, sharedHealthCheck) hcSrcRanges := L4LoadBalancerSrcRanges() - 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) { diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go index d1918401fbb..17e8a342c20 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go @@ -21,7 +21,6 @@ package gce import ( "context" "fmt" - "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" "strings" "testing" @@ -29,6 +28,7 @@ import ( "github.com/stretchr/testify/require" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud" + "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/mock" computebeta "google.golang.org/api/compute/v0.beta" compute "google.golang.org/api/compute/v1" @@ -305,7 +305,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) @@ -590,12 +590,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 @@ -616,7 +691,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) @@ -633,7 +709,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) @@ -651,7 +728,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) diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_utils_test.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_utils_test.go index d4922b95752..93e8dff542b 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_utils_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_utils_test.go @@ -206,7 +206,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), }