From 2479f754d4dd23fe7e960af3dd6d025c77db9154 Mon Sep 17 00:00:00 2001 From: Pavithra Ramesh Date: Mon, 1 Mar 2021 07:19:40 -0800 Subject: [PATCH] Support > 5 ports in L4 ILB. Added logic to set AllPorts field if more than 5 ports are specified. --- .../gce/gce_loadbalancer_internal.go | 15 ++- .../gce/gce_loadbalancer_internal_test.go | 98 +++++++++++++++++++ 2 files changed, 111 insertions(+), 2 deletions(-) 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 0f5e9fc1b08..8db1b8ec136 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 @@ -47,6 +47,8 @@ const ( ILBFinalizerV2 = "gke.networking.io/l4-ilb-v2" // maxInstancesPerInstanceGroup defines maximum number of VMs per InstanceGroup. maxInstancesPerInstanceGroup = 1000 + // maxL4ILBPorts is the maximum number of ports that can be specified in an L4 ILB Forwarding Rule. Beyond this, "AllPorts" field should be used. + maxL4ILBPorts = 5 ) func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v1.Service, existingFwdRule *compute.ForwardingRule, nodes []*v1.Node) (*v1.LoadBalancerStatus, error) { @@ -187,6 +189,10 @@ func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v if options.AllowGlobalAccess { newFwdRule.AllowGlobalAccess = options.AllowGlobalAccess } + if len(ports) > maxL4ILBPorts { + newFwdRule.Ports = nil + newFwdRule.AllPorts = true + } fwdRuleDeleted := false if existingFwdRule != nil && !forwardingRulesEqual(existingFwdRule, newFwdRule) { @@ -969,13 +975,18 @@ func (g *Cloud) ensureInternalForwardingRule(existingFwdRule, newFwdRule *comput func forwardingRulesEqual(old, new *compute.ForwardingRule) bool { // basepath could have differences like compute.googleapis.com vs www.googleapis.com, compare resourceIDs oldResourceID, err := cloud.ParseResourceURL(old.BackendService) - klog.Errorf("forwardingRulesEqual(): failed to parse backend resource URL from existing FR, err - %v", err) + if err != nil { + klog.Errorf("forwardingRulesEqual(): failed to parse backend resource URL from existing FR, err - %v", err) + } newResourceID, err := cloud.ParseResourceURL(new.BackendService) - klog.Errorf("forwardingRulesEqual(): failed to parse resource URL from new FR, err - %v", err) + if err != nil { + klog.Errorf("forwardingRulesEqual(): failed to parse resource URL from new FR, err - %v", err) + } return (old.IPAddress == "" || new.IPAddress == "" || old.IPAddress == new.IPAddress) && old.IPProtocol == new.IPProtocol && old.LoadBalancingScheme == new.LoadBalancingScheme && equalStringSets(old.Ports, new.Ports) && + old.AllPorts == new.AllPorts && oldResourceID.Equal(newResourceID) && old.AllowGlobalAccess == new.AllowGlobalAccess && old.Subnetwork == new.Subnetwork 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 ff3386ee195..d8e9a91b43f 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 @@ -1351,6 +1351,15 @@ func TestForwardingRulesEqual(t *testing.T) { AllowGlobalAccess: true, BackendService: "http://compute.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", }, + { + Name: "udp-fwd-rule-allports", + IPAddress: "10.0.0.0", + Ports: []string{"123"}, + AllPorts: true, + IPProtocol: "UDP", + LoadBalancingScheme: string(cloud.SchemeInternal), + BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", + }, } for _, tc := range []struct { @@ -1389,6 +1398,12 @@ func TestForwardingRulesEqual(t *testing.T) { newFwdRule: fwdRules[4], expect: true, }, + { + desc: "same forwarding rule, one uses AllPorts", + oldFwdRule: fwdRules[2], + newFwdRule: fwdRules[5], + expect: false, + }, } { t.Run(tc.desc, func(t *testing.T) { got := forwardingRulesEqual(tc.oldFwdRule, tc.newFwdRule) @@ -1722,3 +1737,86 @@ func TestEnsureInternalLoadBalancerModifyProtocol(t *testing.T) { } assertInternalLbResourcesDeleted(t, gce, svc, vals, true) } + +func TestEnsureInternalLoadBalancerAllPorts(t *testing.T) { + t.Parallel() + + vals := DefaultTestClusterValues() + gce, err := fakeGCECloud(vals) + require.NoError(t, err) + nodeNames := []string{"test-node-1"} + nodes, err := createAndInsertNodes(gce, nodeNames, vals.ZoneName) + require.NoError(t, err) + svc := fakeLoadbalancerService(string(LBTypeInternal)) + svc, err = gce.client.CoreV1().Services(svc.Namespace).Create(context.TODO(), svc, metav1.CreateOptions{}) + require.NoError(t, err) + lbName := gce.GetLoadBalancerName(context.TODO(), "", svc) + status, err := createInternalLoadBalancer(gce, svc, nil, nodeNames, vals.ClusterName, vals.ClusterID, vals.ZoneName) + if err != nil { + t.Errorf("Unexpected error %v", err) + } + assert.NotEmpty(t, status.Ingress) + fwdRule, err := gce.GetRegionForwardingRule(lbName, gce.region) + if err != nil { + t.Errorf("gce.GetRegionForwardingRule(%q, %q) = %v, want nil", lbName, gce.region, err) + } + if fwdRule.Ports[0] != "123" { + t.Errorf("Unexpected port value %v, expected [123]", fwdRule.Ports) + } + + // Change service spec to use more than 5 ports + svc.Spec.Ports = []v1.ServicePort{ + {Name: "testport", Port: int32(8080), Protocol: "TCP"}, + {Name: "testport", Port: int32(8090), Protocol: "TCP"}, + {Name: "testport", Port: int32(8100), Protocol: "TCP"}, + {Name: "testport", Port: int32(8200), Protocol: "TCP"}, + {Name: "testport", Port: int32(8300), Protocol: "TCP"}, + {Name: "testport", Port: int32(8400), Protocol: "TCP"}, + } + status, err = gce.EnsureLoadBalancer(context.Background(), vals.ClusterName, svc, nodes) + if err != nil { + t.Errorf("Unexpected error %v", err) + } + assert.NotEmpty(t, status.Ingress) + fwdRule, err = gce.GetRegionForwardingRule(lbName, gce.region) + if err != nil { + t.Errorf("gce.GetRegionForwardingRule(%q, %q) = %v, want nil", lbName, gce.region, err) + } + if !fwdRule.AllPorts { + t.Errorf("Unexpected AllPorts false value, expected true, FR - %v", fwdRule) + } + if len(fwdRule.Ports) != 0 { + t.Errorf("Unexpected port value %v, expected empty list", fwdRule.Ports) + } + + // Change service spec back to use < 5 ports + svc.Spec.Ports = []v1.ServicePort{ + {Name: "testport", Port: int32(8090), Protocol: "TCP"}, + {Name: "testport", Port: int32(8100), Protocol: "TCP"}, + {Name: "testport", Port: int32(8300), Protocol: "TCP"}, + {Name: "testport", Port: int32(8400), Protocol: "TCP"}, + } + expectPorts := []string{"8090", "8100", "8300", "8400"} + status, err = gce.EnsureLoadBalancer(context.Background(), vals.ClusterName, svc, nodes) + if err != nil { + t.Errorf("Unexpected error %v", err) + } + assert.NotEmpty(t, status.Ingress) + fwdRule, err = gce.GetRegionForwardingRule(lbName, gce.region) + if err != nil { + t.Errorf("gce.GetRegionForwardingRule(%q, %q) = %v, want nil", lbName, gce.region, err) + } + if fwdRule.AllPorts { + t.Errorf("Unexpected AllPorts true value, expected false, FR - %v", fwdRule) + } + if !equalStringSets(fwdRule.Ports, expectPorts) { + t.Errorf("Unexpected port value %v, expected %v", fwdRule.Ports, expectPorts) + } + + // Delete the service + err = gce.EnsureLoadBalancerDeleted(context.Background(), vals.ClusterName, svc) + if err != nil { + t.Errorf("Unexpected error %v", err) + } + assertInternalLbResourcesDeleted(t, gce, svc, vals, true) +}