From a52125cc0aab2aa50dc24a53a17d9201e67359fe Mon Sep 17 00:00:00 2001 From: Pavithra Ramesh Date: Thu, 31 Oct 2019 16:02:47 -0700 Subject: [PATCH] Added new test, fixed existing tests. --- .../gce/gce_loadbalancer_internal_test.go | 90 +++++++++++++++++-- .../gce/gce_loadbalancer_utils_test.go | 2 +- 2 files changed, 85 insertions(+), 7 deletions(-) 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), }