From 39e1c3607efba3aa694ddf9f554512685656a169 Mon Sep 17 00:00:00 2001 From: Blaz Zupan Date: Tue, 28 May 2024 13:50:38 -0700 Subject: [PATCH] Forward individual ports for NetLB with 5 or less service ports GCE forwarding rules have a limit of 5 forwarded ports. In the external L4 load balancer, we currently turn a set of exposed ports into a port range from minPort to maxPort and forward all ports in the range, which needlessly forwards traffic for ports that are not supposed to be exposed. With this change, if 5 or less ports are exposed, we expose each distinct, otherwise we use the old mechanism of exposing a port range. This is similar to have the Internal Load Balancer is setup, except the ILB exposes all ports if more than 5 ports need to be exposed --- pkg/flags/flags.go | 2 + pkg/l4lb/l4netlbcontroller_test.go | 26 +- pkg/loadbalancers/forwarding_rules.go | 40 ++- pkg/loadbalancers/forwarding_rules_ipv6.go | 15 +- pkg/loadbalancers/forwarding_rules_test.go | 276 ++++++++++++--------- pkg/utils/utils.go | 27 +- pkg/utils/utils_test.go | 55 ++-- 7 files changed, 274 insertions(+), 167 deletions(-) diff --git a/pkg/flags/flags.go b/pkg/flags/flags.go index 05dd25196e..416f48f98a 100644 --- a/pkg/flags/flags.go +++ b/pkg/flags/flags.go @@ -128,6 +128,7 @@ var ( DisableFWEnforcement bool EnableIngressRegionalExternal bool DisableIngressGlobalExternal bool + EnableDiscretePortForwarding bool }{ GCERateLimitScale: 1.0, } @@ -304,6 +305,7 @@ L7 load balancing. CSV values accepted. Example: -node-port-ranges=80,8080,400-5 flag.BoolVar(&F.DisableFWEnforcement, "disable-fw-enforcement", false, "Disable Ingress controller to enforce the firewall rules. If set to true, Ingress Controller stops creating GCE firewall rules. We can only enable this if enable-firewall-cr sets to true.") flag.BoolVar(&F.EnableIngressRegionalExternal, "enable-ingress-regional-external", false, "Enable L7 Ingress Regional External.") flag.BoolVar(&F.DisableIngressGlobalExternal, "disable-ingress-global-external", false, "Disable L7 Ingress Global External. Should be used when Regional External is enabled.") + flag.BoolVar(&F.EnableDiscretePortForwarding, "enable-discrete-port-forwarding", false, "Enable forwarding of individual ports instead of port ranges.") } func Validate() { diff --git a/pkg/l4lb/l4netlbcontroller_test.go b/pkg/l4lb/l4netlbcontroller_test.go index 51259fae48..d6b75b527d 100644 --- a/pkg/l4lb/l4netlbcontroller_test.go +++ b/pkg/l4lb/l4netlbcontroller_test.go @@ -48,6 +48,7 @@ import ( "k8s.io/ingress-gce/pkg/backends" "k8s.io/ingress-gce/pkg/composite" ingctx "k8s.io/ingress-gce/pkg/context" + "k8s.io/ingress-gce/pkg/flags" "k8s.io/ingress-gce/pkg/healthchecksl4" "k8s.io/ingress-gce/pkg/loadbalancers" "k8s.io/ingress-gce/pkg/metrics" @@ -134,7 +135,7 @@ func deleteNetLBService(lc *L4NetLBController, svc *v1.Service) { lc.ctx.ServiceInformer.GetIndexer().Delete(svc) } -func checkForwardingRule(lc *L4NetLBController, svc *v1.Service, expectedPortRange string) error { +func checkForwardingRule(lc *L4NetLBController, svc *v1.Service, expectedPortRange string, expectedPorts []string) error { if len(svc.Spec.Ports) == 0 { return fmt.Errorf("There are no ports in service!") } @@ -146,6 +147,9 @@ func checkForwardingRule(lc *L4NetLBController, svc *v1.Service, expectedPortRan if fwdRule.PortRange != expectedPortRange { return fmt.Errorf("Port Range Mismatch %v != %v", expectedPortRange, fwdRule.PortRange) } + if !utils.EqualStringSets(fwdRule.Ports, expectedPorts) { + return fmt.Errorf("Port List Mismatch %v != %v", expectedPorts, fwdRule.Ports) + } return nil } @@ -386,7 +390,7 @@ func TestProcessMultipleNetLBServices(t *testing.T) { t.Errorf("%v", err) } expectedPortRange := fmt.Sprintf("%d-%d", svc.Spec.Ports[0].Port, svc.Spec.Ports[0].Port) - if err := checkForwardingRule(lc, svc, expectedPortRange); err != nil { + if err := checkForwardingRule(lc, svc, expectedPortRange, nil); err != nil { t.Errorf("Check forwarding rule error: %v", err) } deleteNetLBService(lc, svc) @@ -401,6 +405,8 @@ func TestForwardingRuleWithPortRange(t *testing.T) { for _, tc := range []struct { svcName string ports []int32 + discretePorts bool + expectedPorts []string expectedPortRange string }{ { @@ -423,7 +429,21 @@ func TestForwardingRuleWithPortRange(t *testing.T) { ports: []int32{8081, 80, 8080, 123}, expectedPortRange: "80-8081", }, + { + svcName: "DiscretePortsLessThanMax", + ports: []int32{8081, 80, 8080, 123}, + discretePorts: true, + expectedPorts: []string{"80", "123", "8080", "8081"}, + expectedPortRange: "", + }, + { + svcName: "DiscretePortsMoreThanMax", + ports: []int32{8081, 80, 8080, 123, 666, 555}, + discretePorts: true, + expectedPortRange: "80-8081", + }, } { + flags.F.EnableDiscretePortForwarding = tc.discretePorts svc := test.NewL4NetLBRBSServiceMultiplePorts(tc.svcName, tc.ports) svc.UID = types.UID(svc.Name + fmt.Sprintf("-%d", rand.Intn(1001))) addNetLBService(lc, svc) @@ -443,7 +463,7 @@ func TestForwardingRuleWithPortRange(t *testing.T) { if err := checkBackendService(lc, svc); err != nil { t.Errorf("Check backend service err: %v", err) } - if err := checkForwardingRule(lc, newSvc, tc.expectedPortRange); err != nil { + if err := checkForwardingRule(lc, newSvc, tc.expectedPortRange, tc.expectedPorts); err != nil { t.Errorf("Check forwarding rule error: %v", err) } deleteNetLBService(lc, svc) diff --git a/pkg/loadbalancers/forwarding_rules.go b/pkg/loadbalancers/forwarding_rules.go index 854b8b8fd2..ee805e28e8 100644 --- a/pkg/loadbalancers/forwarding_rules.go +++ b/pkg/loadbalancers/forwarding_rules.go @@ -33,6 +33,7 @@ import ( "k8s.io/ingress-gce/pkg/annotations" "k8s.io/ingress-gce/pkg/composite" "k8s.io/ingress-gce/pkg/events" + "k8s.io/ingress-gce/pkg/flags" "k8s.io/ingress-gce/pkg/translator" "k8s.io/ingress-gce/pkg/utils" "k8s.io/ingress-gce/pkg/utils/namer" @@ -40,8 +41,8 @@ import ( ) const ( - // maxL4ILBPorts is the maximum number of ports that can be specified in an L4 ILB Forwarding Rule - maxL4ILBPorts = 5 + // maxForwardedPorts is the maximum number of ports that can be specified in an Forwarding Rule + maxForwardedPorts = 5 // addressAlreadyInUseMessageExternal is the error message string returned by the compute API // when creating an external forwarding rule that uses a conflicting IP address. addressAlreadyInUseMessageExternal = "Specified IP address is in-use and would result in a conflict." @@ -245,7 +246,7 @@ func (l4 *L4) ensureIPv4ForwardingRule(bsLink string, options gce.ILBOptions, ex AllowGlobalAccess: options.AllowGlobalAccess, Description: frDesc, } - if len(ports) > maxL4ILBPorts { + if len(ports) > maxForwardedPorts { fr.Ports = nil fr.AllPorts = true } @@ -345,8 +346,10 @@ func (l4netlb *L4NetLB) ensureIPv4ForwardingRule(bsLink string) (*composite.Forw }() } - portRange, protocol := utils.MinMaxPortRangeAndProtocol(l4netlb.Service.Spec.Ports) - + svcPorts := l4netlb.Service.Spec.Ports + ports := utils.GetPorts(svcPorts) + portRange := utils.MinMaxPortRange(svcPorts) + protocol := utils.GetProtocol(svcPorts) serviceKey := utils.ServiceKeyFunc(l4netlb.Service.Namespace, l4netlb.Service.Name) frDesc, err := utils.MakeL4LBServiceDescription(serviceKey, ipToUse, version, false, utils.XLB) if err != nil { @@ -357,12 +360,16 @@ func (l4netlb *L4NetLB) ensureIPv4ForwardingRule(bsLink string) (*composite.Forw Name: frName, Description: frDesc, IPAddress: ipToUse, - IPProtocol: protocol, + IPProtocol: string(protocol), PortRange: portRange, LoadBalancingScheme: string(cloud.SchemeExternal), BackendService: bsLink, NetworkTier: netTier.ToGCEValue(), } + if len(ports) <= maxForwardedPorts && flags.F.EnableDiscretePortForwarding { + fr.Ports = ports + fr.PortRange = "" + } if existingFwdRule != nil { if existingFwdRule.NetworkTier != fr.NetworkTier { @@ -428,8 +435,7 @@ func Equal(fr1, fr2 *composite.ForwardingRule) (bool, error) { return fr1.IPAddress == fr2.IPAddress && fr1.IPProtocol == fr2.IPProtocol && fr1.LoadBalancingScheme == fr2.LoadBalancingScheme && - utils.EqualStringSets(fr1.Ports, fr2.Ports) && - fr1.PortRange == fr2.PortRange && + equalPorts(fr1.Ports, fr2.Ports, fr1.PortRange, fr2.PortRange) && utils.EqualCloudResourceIDs(id1, id2) && fr1.AllowGlobalAccess == fr2.AllowGlobalAccess && fr1.AllPorts == fr2.AllPorts && @@ -438,6 +444,24 @@ func Equal(fr1, fr2 *composite.ForwardingRule) (bool, error) { fr1.NetworkTier == fr2.NetworkTier, nil } +// equalPorts compares two port ranges or slices of ports. Before comparison, +// slices of ports are converted into a port range from smallest to largest +// port. This is done so we don't unnecessarily recreate forwarding rules +// when upgrading from port ranges to distinct ports, because recreating +// forwarding rules is traffic impacting. +func equalPorts(ports1, ports2 []string, portRange1, portRange2 string) bool { + if !flags.F.EnableDiscretePortForwarding { + return utils.EqualStringSets(ports1, ports2) && portRange1 == portRange2 + } + if len(ports1) != 0 && portRange1 == "" { + portRange1 = utils.MinMaxPortRange(ports1) + } + if len(ports2) != 0 && portRange2 == "" { + portRange2 = utils.MinMaxPortRange(ports2) + } + return portRange1 == portRange2 +} + func equalResourcePaths(rp1, rp2 string) bool { return rp1 == rp2 || utils.EqualResourceIDs(rp1, rp2) } diff --git a/pkg/loadbalancers/forwarding_rules_ipv6.go b/pkg/loadbalancers/forwarding_rules_ipv6.go index 71897a1021..2fdb8b1561 100644 --- a/pkg/loadbalancers/forwarding_rules_ipv6.go +++ b/pkg/loadbalancers/forwarding_rules_ipv6.go @@ -30,6 +30,7 @@ import ( "k8s.io/ingress-gce/pkg/annotations" "k8s.io/ingress-gce/pkg/composite" "k8s.io/ingress-gce/pkg/events" + "k8s.io/ingress-gce/pkg/flags" "k8s.io/ingress-gce/pkg/utils" "k8s.io/klog/v2" ) @@ -112,7 +113,7 @@ func (l4 *L4) buildExpectedIPv6ForwardingRule(bsLink string, options gce.ILBOpti AllowGlobalAccess: options.AllowGlobalAccess, NetworkTier: cloud.NetworkTierPremium.ToGCEValue(), } - if len(ports) > maxL4ILBPorts { + if len(ports) > maxForwardedPorts { fr.Ports = nil fr.AllPorts = true } @@ -252,12 +253,14 @@ func (l4netlb *L4NetLB) buildExpectedIPv6ForwardingRule(bsLink, ipv6AddressToUse } svcPorts := l4netlb.Service.Spec.Ports - portRange, protocol := utils.MinMaxPortRangeAndProtocol(svcPorts) + ports := utils.GetPorts(svcPorts) + portRange := utils.MinMaxPortRange(svcPorts) + protocol := utils.GetProtocol(svcPorts) fr := &composite.ForwardingRule{ Name: frName, Description: frDesc, IPAddress: ipv6AddressToUse, - IPProtocol: protocol, + IPProtocol: string(protocol), PortRange: portRange, LoadBalancingScheme: string(cloud.SchemeExternal), BackendService: bsLink, @@ -265,6 +268,10 @@ func (l4netlb *L4NetLB) buildExpectedIPv6ForwardingRule(bsLink, ipv6AddressToUse NetworkTier: netTier.ToGCEValue(), Subnetwork: subnetworkURL, } + if len(ports) <= maxForwardedPorts && flags.F.EnableDiscretePortForwarding { + fr.Ports = utils.GetPorts(svcPorts) + fr.PortRange = "" + } return fr, nil } @@ -292,7 +299,7 @@ func EqualIPv6ForwardingRules(fr1, fr2 *composite.ForwardingRule) (bool, error) } return fr1.IPProtocol == fr2.IPProtocol && fr1.LoadBalancingScheme == fr2.LoadBalancingScheme && - utils.EqualStringSets(fr1.Ports, fr2.Ports) && + equalPorts(fr1.Ports, fr2.Ports, fr1.PortRange, fr2.PortRange) && utils.EqualCloudResourceIDs(id1, id2) && fr1.AllowGlobalAccess == fr2.AllowGlobalAccess && fr1.AllPorts == fr2.AllPorts && diff --git a/pkg/loadbalancers/forwarding_rules_test.go b/pkg/loadbalancers/forwarding_rules_test.go index 7bc7cb5cde..fb1ece698f 100644 --- a/pkg/loadbalancers/forwarding_rules_test.go +++ b/pkg/loadbalancers/forwarding_rules_test.go @@ -17,6 +17,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/cloud-provider-gcp/providers/gce" "k8s.io/ingress-gce/pkg/composite" + "k8s.io/ingress-gce/pkg/flags" "k8s.io/ingress-gce/pkg/forwardingrules" "k8s.io/ingress-gce/pkg/network" "k8s.io/ingress-gce/pkg/test" @@ -103,155 +104,205 @@ func TestGetEffectiveIP(t *testing.T) { func TestForwardingRulesEqual(t *testing.T) { t.Parallel() - fwdRules := []*composite.ForwardingRule{ - { - Name: "empty-ip-address-fwd-rule", - IPAddress: "", - Ports: []string{"123"}, - IPProtocol: "TCP", - LoadBalancingScheme: string(cloud.SchemeInternal), - BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", - }, - { - Name: "tcp-fwd-rule", - IPAddress: "10.0.0.0", - Ports: []string{"123"}, - IPProtocol: "TCP", - LoadBalancingScheme: string(cloud.SchemeInternal), - BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", - }, - { - Name: "udp-fwd-rule", - IPAddress: "10.0.0.0", - Ports: []string{"123"}, - IPProtocol: "UDP", - LoadBalancingScheme: string(cloud.SchemeInternal), - BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", - }, - { - Name: "global-access-fwd-rule", - IPAddress: "10.0.0.0", - Ports: []string{"123"}, - IPProtocol: "TCP", - LoadBalancingScheme: string(cloud.SchemeInternal), - AllowGlobalAccess: true, - BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", - }, - { - Name: "fwd-rule-bs-link1", - IPAddress: "10.0.0.0", - Ports: []string{"123"}, - IPProtocol: "TCP", - LoadBalancingScheme: string(cloud.SchemeInternal), - BackendService: "http://compute.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", - }, - { - Name: "fwd-rule-bs-link2", - IPAddress: "10.0.0.0", - Ports: []string{"123"}, - IPProtocol: "TCP", - LoadBalancingScheme: string(cloud.SchemeInternal), - BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", - }, - { - Name: "udp-fwd-rule-all-ports", - 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", - NetworkTier: cloud.NetworkTierPremium.ToGCEValue(), - }, - { - Name: "fwd-rule-bs-link2-standard-ntier", - IPAddress: "10.0.0.0", - Ports: []string{"123"}, - IPProtocol: "TCP", - LoadBalancingScheme: string(cloud.SchemeInternal), - BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", - NetworkTier: string(cloud.NetworkTierStandard), - }, - { - Name: "fwd-rule-bs-link2-premium-ntier", - IPAddress: "10.0.0.0", - Ports: []string{"123"}, - IPProtocol: "TCP", - LoadBalancingScheme: string(cloud.SchemeInternal), - BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", - NetworkTier: cloud.NetworkTierPremium.ToGCEValue(), - }, - } - - frPortRange1 := &composite.ForwardingRule{ + fwdRuleTCP := &composite.ForwardingRule{ Name: "tcp-fwd-rule", IPAddress: "10.0.0.0", - PortRange: "2-3", + Ports: []string{"123"}, IPProtocol: "TCP", LoadBalancingScheme: string(cloud.SchemeInternal), BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", } - frPortRange2 := &composite.ForwardingRule{ - Name: "tcp-fwd-rule", + fwdRuleUDP := &composite.ForwardingRule{ + Name: "udp-fwd-rule", IPAddress: "10.0.0.0", - PortRange: "1-2", - IPProtocol: "TCP", + Ports: []string{"123"}, + IPProtocol: "UDP", LoadBalancingScheme: string(cloud.SchemeInternal), BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", } for _, tc := range []struct { - desc string - oldFwdRule *composite.ForwardingRule - newFwdRule *composite.ForwardingRule - expectEqual bool + desc string + oldFwdRule *composite.ForwardingRule + newFwdRule *composite.ForwardingRule + discretePortForwarding bool + expectEqual bool }{ { - desc: "empty ip address does not match valid ip", - oldFwdRule: fwdRules[0], - newFwdRule: fwdRules[1], + desc: "empty ip address does not match valid ip", + oldFwdRule: &composite.ForwardingRule{ + Name: "empty-ip-address-fwd-rule", + IPAddress: "", + Ports: []string{"123"}, + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", + }, + newFwdRule: fwdRuleTCP, expectEqual: false, }, { - desc: "global access enabled", - oldFwdRule: fwdRules[1], - newFwdRule: fwdRules[3], + desc: "global access enabled", + oldFwdRule: fwdRuleTCP, + newFwdRule: &composite.ForwardingRule{ + Name: "global-access-fwd-rule", + IPAddress: "10.0.0.0", + Ports: []string{"123"}, + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + AllowGlobalAccess: true, + BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", + }, expectEqual: false, }, { desc: "IP protocol changed", - oldFwdRule: fwdRules[1], - newFwdRule: fwdRules[2], + oldFwdRule: fwdRuleTCP, + newFwdRule: fwdRuleUDP, expectEqual: false, }, { desc: "same forwarding rule", - oldFwdRule: fwdRules[3], - newFwdRule: fwdRules[3], + oldFwdRule: fwdRuleTCP, + newFwdRule: fwdRuleTCP, expectEqual: true, }, { - desc: "same forwarding rule, different basepath", - oldFwdRule: fwdRules[4], - newFwdRule: fwdRules[5], + desc: "same forwarding rule, different basepath", + oldFwdRule: &composite.ForwardingRule{ + Name: "fwd-rule-bs-link1", + IPAddress: "10.0.0.0", + Ports: []string{"123"}, + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + BackendService: "http://compute.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", + }, + newFwdRule: &composite.ForwardingRule{ + Name: "fwd-rule-bs-link2", + IPAddress: "10.0.0.0", + Ports: []string{"123"}, + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", + }, expectEqual: true, }, { - desc: "same forwarding rule, one uses ALL keyword for ports", - oldFwdRule: fwdRules[2], - newFwdRule: fwdRules[6], + desc: "same forwarding rule, one uses ALL keyword for ports", + oldFwdRule: fwdRuleUDP, + newFwdRule: &composite.ForwardingRule{ + Name: "udp-fwd-rule-all-ports", + 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", + NetworkTier: cloud.NetworkTierPremium.ToGCEValue(), + }, expectEqual: false, }, { - desc: "network tier mismatch", - oldFwdRule: fwdRules[6], - newFwdRule: fwdRules[7], + desc: "network tier mismatch", + oldFwdRule: &composite.ForwardingRule{ + Name: "fwd-rule-bs-link2-standard-ntier", + IPAddress: "10.0.0.0", + Ports: []string{"123"}, + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", + NetworkTier: string(cloud.NetworkTierStandard), + }, + newFwdRule: &composite.ForwardingRule{ + Name: "fwd-rule-bs-link2-premium-ntier", + IPAddress: "10.0.0.0", + Ports: []string{"123"}, + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", + NetworkTier: cloud.NetworkTierPremium.ToGCEValue(), + }, + expectEqual: false, + }, + { + desc: "same forwarding rule, port range mismatch", + oldFwdRule: &composite.ForwardingRule{ + Name: "tcp-fwd-rule", + IPAddress: "10.0.0.0", + PortRange: "1-2", + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", + }, + newFwdRule: &composite.ForwardingRule{ + Name: "tcp-fwd-rule", + IPAddress: "10.0.0.0", + PortRange: "2-3", + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", + }, + expectEqual: false, + }, + { + desc: "port range discrete ports disabled", + oldFwdRule: &composite.ForwardingRule{ + Name: "tcp-fwd-rule", + IPAddress: "10.0.0.0", + Ports: []string{"1", "3"}, + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", + }, + newFwdRule: &composite.ForwardingRule{ + Name: "tcp-fwd-rule", + IPAddress: "10.0.0.0", + PortRange: "1-3", + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", + }, expectEqual: false, }, { - desc: "same forwarding rule, different port ranges", - oldFwdRule: frPortRange1, - newFwdRule: frPortRange2, + desc: "port range discrete ports enabled", + oldFwdRule: &composite.ForwardingRule{ + Name: "tcp-fwd-rule", + IPAddress: "10.0.0.0", + Ports: []string{"1", "3"}, + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", + }, + newFwdRule: &composite.ForwardingRule{ + Name: "tcp-fwd-rule", + IPAddress: "10.0.0.0", + PortRange: "1-3", + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", + }, + discretePortForwarding: true, + expectEqual: true, + }, + { + desc: "same forwarding rule, ports vs port ranges mismatch", + oldFwdRule: &composite.ForwardingRule{ + Name: "tcp-fwd-rule", + IPAddress: "10.0.0.0", + Ports: []string{"1", "5"}, + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", + }, + newFwdRule: &composite.ForwardingRule{ + Name: "tcp-fwd-rule", + IPAddress: "10.0.0.0", + PortRange: "1-3", + IPProtocol: "TCP", + LoadBalancingScheme: string(cloud.SchemeInternal), + BackendService: "http://www.googleapis.com/projects/test/regions/us-central1/backendServices/bs1", + }, expectEqual: false, }, { @@ -326,6 +377,7 @@ func TestForwardingRulesEqual(t *testing.T) { }, } { t.Run(tc.desc, func(t *testing.T) { + flags.F.EnableDiscretePortForwarding = tc.discretePortForwarding got, err := Equal(tc.oldFwdRule, tc.newFwdRule) if err != nil { t.Errorf("forwardingRulesEqual(_, _) = %v, want nil error", err) diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index 7ddc9b72d5..9acfabb36e 100644 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -741,27 +741,38 @@ func GetServicePortRanges(svcPorts []api_v1.ServicePort) []string { return GetPortRanges(portInts) } -func minMaxPort(svcPorts []api_v1.ServicePort) (int32, int32) { +func minMaxPort[T api_v1.ServicePort | string](svcPorts []T) (int32, int32) { minPort := int32(65536) maxPort := int32(0) for _, svcPort := range svcPorts { - if svcPort.Port < minPort { - minPort = svcPort.Port + port := func(value any) int32 { + switch value.(type) { + case api_v1.ServicePort: + return value.(api_v1.ServicePort).Port + case string: + i, _ := strconv.ParseInt(value.(string), 10, 32) + return int32(i) + default: + return 0 + } + }(svcPort) + if port < minPort { + minPort = port } - if svcPort.Port > maxPort { - maxPort = svcPort.Port + if port > maxPort { + maxPort = port } } return minPort, maxPort } -func MinMaxPortRangeAndProtocol(svcPorts []api_v1.ServicePort) (portRange, protocol string) { +func MinMaxPortRange[T api_v1.ServicePort | string](svcPorts []T) string { if len(svcPorts) == 0 { - return "", "" + return "" } minPort, maxPort := minMaxPort(svcPorts) - return fmt.Sprintf("%d-%d", minPort, maxPort), string(svcPorts[0].Protocol) + return fmt.Sprintf("%d-%d", minPort, maxPort) } // TranslateAffinityType converts the k8s affinity type to the GCE affinity type. diff --git a/pkg/utils/utils_test.go b/pkg/utils/utils_test.go index 5a6123524a..9c2351c5b8 100644 --- a/pkg/utils/utils_test.go +++ b/pkg/utils/utils_test.go @@ -1317,7 +1317,7 @@ func TestComputeBasePath(t *testing.T) { } } -func TestMinMaxPortRangeAndProtocol(t *testing.T) { +func TestMinMaxPortRange(t *testing.T) { for _, tc := range []struct { svcPorts []api_v1.ServicePort @@ -1326,56 +1326,47 @@ func TestMinMaxPortRangeAndProtocol(t *testing.T) { }{ { svcPorts: []api_v1.ServicePort{ - {Port: 1, Protocol: "TCP"}, - {Port: 10, Protocol: "TCP"}, - {Port: 100, Protocol: "TCP"}}, - expectedRange: "1-100", - expectedProtocol: "TCP", + {Port: 1}, + {Port: 10}, + {Port: 100}}, + expectedRange: "1-100", }, { svcPorts: []api_v1.ServicePort{ - {Port: 10, Protocol: "TCP"}, - {Port: 1, Protocol: "TCP"}, - {Port: 50, Protocol: "TCP"}, - {Port: 100, Protocol: "TCP"}, - {Port: 90, Protocol: "TCP"}}, - expectedRange: "1-100", - expectedProtocol: "TCP", + {Port: 10}, + {Port: 1}, + {Port: 50}, + {Port: 100}, + {Port: 90}}, + expectedRange: "1-100", }, { svcPorts: []api_v1.ServicePort{ - {Port: 10, Protocol: "TCP"}}, - expectedRange: "10-10", - expectedProtocol: "TCP", + {Port: 10}}, + expectedRange: "10-10", }, { svcPorts: []api_v1.ServicePort{ - {Port: 100, Protocol: "TCP"}, - {Port: 10, Protocol: "TCP"}}, - expectedRange: "10-100", - expectedProtocol: "TCP", + {Port: 100}, + {Port: 10}}, + expectedRange: "10-100", }, { svcPorts: []api_v1.ServicePort{ - {Port: 100, Protocol: "TCP"}, - {Port: 50, Protocol: "TCP"}, - {Port: 10, Protocol: "TCP"}}, - expectedRange: "10-100", - expectedProtocol: "TCP", + {Port: 100}, + {Port: 50}, + {Port: 10}}, + expectedRange: "10-100", }, { - svcPorts: []api_v1.ServicePort{}, - expectedRange: "", - expectedProtocol: "", + svcPorts: []api_v1.ServicePort{}, + expectedRange: "", }, } { - portsRange, protocol := MinMaxPortRangeAndProtocol(tc.svcPorts) + portsRange := MinMaxPortRange(tc.svcPorts) if portsRange != tc.expectedRange { t.Errorf("PortRange mismatch %v != %v", tc.expectedRange, portsRange) } - if protocol != tc.expectedProtocol { - t.Errorf("protocol mismatch %v != %v", protocol, tc.expectedProtocol) - } } }