diff --git a/go-controller/pkg/ovn/controller/services/lb_config.go b/go-controller/pkg/ovn/controller/services/lb_config.go index 050c614281b..6a7633919f7 100644 --- a/go-controller/pkg/ovn/controller/services/lb_config.go +++ b/go-controller/pkg/ovn/controller/services/lb_config.go @@ -39,8 +39,9 @@ type lbConfig struct { hasNodePort bool } -func (c *lbConfig) makeNodeSwitchTargetIPs(node *nodeInfo, epIPs []string) []string { - targetIPs := epIPs +func (c *lbConfig) makeNodeSwitchTargetIPs(node *nodeInfo, epIPs []string) (targetIPs []string, changed bool) { + targetIPs = epIPs + changed = false if c.externalTrafficLocal { // for ExternalTrafficPolicy=Local, remove non-local endpoints from the router/switch targets @@ -53,11 +54,17 @@ func (c *lbConfig) makeNodeSwitchTargetIPs(node *nodeInfo, epIPs []string) []str targetIPs = util.FilterIPsSlice(targetIPs, node.nodeSubnets(), true) } - return targetIPs + // We potentially only removed stuff from the original slice, so just + // comparing lenghts is enough. + if len(targetIPs) != len(epIPs) { + changed = true + } + return } -func (c *lbConfig) makeNodeRouterTargetIPs(node *nodeInfo, epIPs []string, hostMasqueradeIP string) []string { - targetIPs := epIPs +func (c *lbConfig) makeNodeRouterTargetIPs(node *nodeInfo, epIPs []string, hostMasqueradeIP string) (targetIPs []string, changed bool) { + targetIPs = epIPs + changed = false if c.externalTrafficLocal { // for ExternalTrafficPolicy=Local, remove non-local endpoints from the router/switch targets @@ -67,9 +74,13 @@ func (c *lbConfig) makeNodeRouterTargetIPs(node *nodeInfo, epIPs []string, hostM // any targets local to the node need to have a special // harpin IP added, but only for the router LB - targetIPs = util.UpdateIPsSlice(targetIPs, node.nodeIPsStr(), []string{hostMasqueradeIP}) + targetIPs, updated := util.UpdateIPsSlice(targetIPs, node.nodeIPsStr(), []string{hostMasqueradeIP}) - return targetIPs + // We either only removed stuff from the original slice, or updated some IPs. + if len(targetIPs) != len(epIPs) || updated { + changed = true + } + return } // just used for consistent ordering @@ -347,12 +358,33 @@ func buildTemplateLBs(service *v1.Service, configs []lbConfig, nodes []nodeInfo, for range config.vips { klog.V(5).Infof(" buildTemplateLBs() service %s/%s adding rules", service.Namespace, service.Name) + + // If all targets have exactly the same IPs on all nodes there's + // no need to use a template, just use the same list of explicit + // targets on all nodes. + switchV4TargetNeedsTemplate := false + switchV6TargetNeedsTemplate := false + routerV4TargetNeedsTemplate := false + routerV6TargetNeedsTemplate := false + for _, node := range nodes { - switchV4targetips := config.makeNodeSwitchTargetIPs(&node, config.eps.V4IPs) - switchV6targetips := config.makeNodeSwitchTargetIPs(&node, config.eps.V6IPs) + switchV4targetips, changed := config.makeNodeSwitchTargetIPs(&node, config.eps.V4IPs) + if !switchV4TargetNeedsTemplate && changed { + switchV4TargetNeedsTemplate = true + } + switchV6targetips, changed := config.makeNodeSwitchTargetIPs(&node, config.eps.V6IPs) + if !switchV6TargetNeedsTemplate && changed { + switchV6TargetNeedsTemplate = true + } - routerV4targetips := config.makeNodeRouterTargetIPs(&node, config.eps.V4IPs, types.V4HostMasqueradeIP) - routerV6targetips := config.makeNodeRouterTargetIPs(&node, config.eps.V6IPs, types.V6HostMasqueradeIP) + routerV4targetips, changed := config.makeNodeRouterTargetIPs(&node, config.eps.V4IPs, types.V4HostMasqueradeIP) + if !routerV4TargetNeedsTemplate && changed { + routerV4TargetNeedsTemplate = true + } + routerV6targetips, changed := config.makeNodeRouterTargetIPs(&node, config.eps.V6IPs, types.V6HostMasqueradeIP) + if !routerV6TargetNeedsTemplate && changed { + routerV6TargetNeedsTemplate = true + } switchV4TemplateTarget.Value[node.chassisID] = addrsToString( joinHostsPort(switchV4targetips, config.eps.Port)) @@ -365,23 +397,62 @@ func buildTemplateLBs(service *v1.Service, configs []lbConfig, nodes []nodeInfo, joinHostsPort(routerV6targetips, config.eps.Port)) } - switchV4Rules = append(switchV4Rules, LBRule{ - Source: Addr{Template: nodeIPv4Template, Port: config.inport}, - Targets: []Addr{{Template: switchV4TemplateTarget}}, - }) - switchV6Rules = append(switchV6Rules, LBRule{ - Source: Addr{Template: nodeIPv6Template, Port: config.inport}, - Targets: []Addr{{Template: switchV6TemplateTarget}}, - }) + sharedV4Targets := []Addr{} + sharedV6Targets := []Addr{} + if !switchV4TargetNeedsTemplate || !routerV4TargetNeedsTemplate { + sharedV4Targets = joinHostsPort(config.eps.V4IPs, config.eps.Port) + } + if !switchV6TargetNeedsTemplate || !routerV6TargetNeedsTemplate { + sharedV6Targets = joinHostsPort(config.eps.V6IPs, config.eps.Port) + } - routerV4Rules = append(routerV4Rules, LBRule{ - Source: Addr{Template: nodeIPv4Template, Port: config.inport}, - Targets: []Addr{{Template: routerV4TemplateTarget}}, - }) - routerV6Rules = append(routerV6Rules, LBRule{ - Source: Addr{Template: nodeIPv6Template, Port: config.inport}, - Targets: []Addr{{Template: routerV6TemplateTarget}}, - }) + if switchV4TargetNeedsTemplate { + switchV4Rules = append(switchV4Rules, LBRule{ + Source: Addr{Template: nodeIPv4Template, Port: config.inport}, + Targets: []Addr{{Template: switchV4TemplateTarget}}, + }) + } else { + switchV4Rules = append(switchV4Rules, LBRule{ + Source: Addr{Template: nodeIPv4Template, Port: config.inport}, + Targets: sharedV4Targets, + }) + } + + if switchV6TargetNeedsTemplate { + switchV6Rules = append(switchV6Rules, LBRule{ + Source: Addr{Template: nodeIPv6Template, Port: config.inport}, + Targets: []Addr{{Template: switchV6TemplateTarget}}, + }) + } else { + switchV6Rules = append(switchV6Rules, LBRule{ + Source: Addr{Template: nodeIPv6Template, Port: config.inport}, + Targets: sharedV6Targets, + }) + } + + if routerV4TargetNeedsTemplate { + routerV4Rules = append(routerV4Rules, LBRule{ + Source: Addr{Template: nodeIPv4Template, Port: config.inport}, + Targets: []Addr{{Template: routerV4TemplateTarget}}, + }) + } else { + routerV4Rules = append(routerV4Rules, LBRule{ + Source: Addr{Template: nodeIPv4Template, Port: config.inport}, + Targets: sharedV4Targets, + }) + } + + if routerV6TargetNeedsTemplate { + routerV6Rules = append(routerV6Rules, LBRule{ + Source: Addr{Template: nodeIPv6Template, Port: config.inport}, + Targets: []Addr{{Template: routerV6TemplateTarget}}, + }) + } else { + routerV6Rules = append(routerV6Rules, LBRule{ + Source: Addr{Template: nodeIPv6Template, Port: config.inport}, + Targets: sharedV6Targets, + }) + } } } @@ -485,11 +556,11 @@ func buildPerNodeLBs(service *v1.Service, configs []lbConfig, nodes []nodeInfo) switchRules := make([]LBRule, 0, len(configs)) for _, config := range configs { - switchV4targetips := config.makeNodeSwitchTargetIPs(&node, config.eps.V4IPs) - switchV6targetips := config.makeNodeSwitchTargetIPs(&node, config.eps.V6IPs) + switchV4targetips, _ := config.makeNodeSwitchTargetIPs(&node, config.eps.V4IPs) + switchV6targetips, _ := config.makeNodeSwitchTargetIPs(&node, config.eps.V6IPs) - routerV4targetips := config.makeNodeRouterTargetIPs(&node, config.eps.V4IPs, types.V4HostMasqueradeIP) - routerV6targetips := config.makeNodeRouterTargetIPs(&node, config.eps.V6IPs, types.V6HostMasqueradeIP) + routerV4targetips, _ := config.makeNodeRouterTargetIPs(&node, config.eps.V4IPs, types.V4HostMasqueradeIP) + routerV6targetips, _ := config.makeNodeRouterTargetIPs(&node, config.eps.V6IPs, types.V6HostMasqueradeIP) routerV4targets := joinHostsPort(routerV4targetips, config.eps.Port) routerV6targets := joinHostsPort(routerV6targetips, config.eps.Port) diff --git a/go-controller/pkg/ovn/controller/services/services_controller_test.go b/go-controller/pkg/ovn/controller/services/services_controller_test.go index fa3e93219fa..08f64f0aeb2 100644 --- a/go-controller/pkg/ovn/controller/services/services_controller_test.go +++ b/go-controller/pkg/ovn/controller/services/services_controller_test.go @@ -331,17 +331,16 @@ func TestSyncServices(t *testing.T) { }, ExternalIDs: serviceExternalIDs(namespacedServiceName(ns, serviceName)), }, - nodeSwitchTemplateLoadBalancer(nodePort, serviceName, ns), - nodeRouterTemplateLoadBalancer(nodePort, serviceName, ns), + nodeMergedTemplateLoadBalancer(nodePort, serviceName, ns, outport, nodeAEndpointIP, nodeBEndpointIP), nodeLogicalSwitch(nodeA, initialLsGroups), nodeLogicalSwitch(nodeB, initialLsGroups), nodeLogicalRouter(nodeA, initialLrGroups), nodeLogicalRouter(nodeB, initialLrGroups), lbGroup(types.ClusterLBGroupName, loadBalancerClusterWideTCPServiceName(ns, serviceName)), - lbGroup(types.ClusterSwitchLBGroupName, nodeSwitchTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), - lbGroup(types.ClusterRouterLBGroupName, nodeRouterTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), - nodeTemplate(firstNode, nodePort, serviceName, ns, outport, nodeAEndpointIP, nodeBEndpointIP), - nodeTemplate(secondNode, nodePort, serviceName, ns, outport, nodeAEndpointIP, nodeBEndpointIP), + lbGroup(types.ClusterSwitchLBGroupName, nodeMergedTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), + lbGroup(types.ClusterRouterLBGroupName, nodeMergedTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), + nodeIPTemplate(firstNode), + nodeIPTemplate(secondNode), }, }, { @@ -414,17 +413,16 @@ func TestSyncServices(t *testing.T) { }, ExternalIDs: serviceExternalIDs(namespacedServiceName(ns, serviceName)), }, - nodeSwitchTemplateLoadBalancer(nodePort, serviceName, ns), - nodeRouterTemplateLoadBalancer(nodePort, serviceName, ns), + nodeMergedTemplateLoadBalancer(nodePort, serviceName, ns, outport, nodeAEndpointIP, nodeBEndpointIP), nodeLogicalSwitch(nodeA, initialLsGroups), nodeLogicalSwitch(nodeB, initialLsGroups), nodeLogicalRouter(nodeA, initialLrGroups), nodeLogicalRouter(nodeB, initialLrGroups), lbGroup(types.ClusterLBGroupName, loadBalancerClusterWideTCPServiceName(ns, serviceName)), - lbGroup(types.ClusterSwitchLBGroupName, nodeSwitchTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), - lbGroup(types.ClusterRouterLBGroupName, nodeRouterTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), - nodeTemplate(firstNode, nodePort, serviceName, ns, outport, nodeAEndpointIP, nodeBEndpointIP), - nodeTemplate(secondNode, nodePort, serviceName, ns, outport, nodeAEndpointIP, nodeBEndpointIP), + lbGroup(types.ClusterSwitchLBGroupName, nodeMergedTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), + lbGroup(types.ClusterRouterLBGroupName, nodeMergedTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), + nodeIPTemplate(firstNode), + nodeIPTemplate(secondNode), }, nodeToDelete: nodeConfig(nodeA, nodeAHostIP), dbStateAfterDeleting: []libovsdbtest.TestData{ @@ -438,19 +436,16 @@ func TestSyncServices(t *testing.T) { }, ExternalIDs: serviceExternalIDs(namespacedServiceName(ns, serviceName)), }, - nodeSwitchTemplateLoadBalancer(nodePort, serviceName, ns), - nodeRouterTemplateLoadBalancer(nodePort, serviceName, ns), + nodeMergedTemplateLoadBalancer(nodePort, serviceName, ns, outport, nodeAEndpointIP, nodeBEndpointIP), nodeLogicalSwitch(nodeA, initialLsGroups), nodeLogicalSwitch(nodeB, initialLsGroups), nodeLogicalRouter(nodeA, initialLrGroups), nodeLogicalRouter(nodeB, initialLrGroups), lbGroup(types.ClusterLBGroupName, loadBalancerClusterWideTCPServiceName(ns, serviceName)), - lbGroup(types.ClusterSwitchLBGroupName, nodeSwitchTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), - lbGroup(types.ClusterRouterLBGroupName, nodeRouterTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), - // node-A chassis template var is cleaned up by the DefaultNetworkController() - // deleteNodeEvent handler. And that doesn't get run in this test. - nodeTemplate(firstNode, nodePort, serviceName, ns, outport, nodeAEndpointIP, nodeBEndpointIP), - nodeTemplate(secondNode, nodePort, serviceName, ns, outport, nodeAEndpointIP, nodeBEndpointIP), + lbGroup(types.ClusterSwitchLBGroupName, nodeMergedTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), + lbGroup(types.ClusterRouterLBGroupName, nodeMergedTemplateLoadBalancerName(ns, serviceName, v1.IPv4Protocol)), + nodeIPTemplate(firstNode), + nodeIPTemplate(secondNode), }, }, } @@ -568,6 +563,14 @@ func nodeRouterTemplateLoadBalancerName(serviceNamespace string, serviceName str addressFamily) } +func nodeMergedTemplateLoadBalancerName(serviceNamespace string, serviceName string, addressFamily v1.IPFamily) string { + return fmt.Sprintf( + "Service_%s/%s_TCP_node_switch_template_%s_merged", + serviceNamespace, + serviceName, + addressFamily) +} + func servicesOptions() map[string]string { return map[string]string{ "event": "false", @@ -637,12 +640,18 @@ func nodeIPTemplate(node *nodeInfo) *nbdb.ChassisTemplateVar { } } -func nodeTemplate(node *nodeInfo, nodePort int32, serviceName string, serviceNamespace string, outputPort int32, endpointIPs ...string) *nbdb.ChassisTemplateVar { - template := nodeIPTemplate(node) - template.Variables[makeLBNodeIPTemplateName(v1.IPv4Protocol)] = node.nodeIPs[0].String() - template.Variables[makeTarget(serviceName, serviceNamespace, "TCP", nodePort, "node_switch_template", v1.IPv4Protocol)] = computeEndpoints(outputPort, endpointIPs...) - template.Variables[makeTarget(serviceName, serviceNamespace, "TCP", nodePort, "node_router_template", v1.IPv4Protocol)] = computeEndpoints(outputPort, endpointIPs...) - return template +func nodeMergedTemplateLoadBalancer(nodePort int32, serviceName string, serviceNamespace string, outputPort int32, endpointIPs ...string) *nbdb.LoadBalancer { + nodeTemplateIP := makeTemplate(makeLBNodeIPTemplateName(v1.IPv4Protocol)) + return &nbdb.LoadBalancer{ + UUID: nodeMergedTemplateLoadBalancerName(serviceNamespace, serviceName, v1.IPv4Protocol), + Name: nodeMergedTemplateLoadBalancerName(serviceNamespace, serviceName, v1.IPv4Protocol), + Options: templateServicesOptions(), + Protocol: &nbdb.LoadBalancerProtocolTCP, + Vips: map[string]string{ + endpoint(refTemplate(nodeTemplateIP.Name), nodePort): computeEndpoints(outputPort, endpointIPs...), + }, + ExternalIDs: serviceExternalIDs(namespacedServiceName(serviceNamespace, serviceName)), + } } func refTemplate(template string) string { diff --git a/go-controller/pkg/util/util.go b/go-controller/pkg/util/util.go index b002f965d81..b5bd023da43 100644 --- a/go-controller/pkg/util/util.go +++ b/go-controller/pkg/util/util.go @@ -168,9 +168,10 @@ func HashForOVN(s string) string { } // UpdateIPsSlice will search for values of oldIPs in the slice "s" and update it with newIPs values of same IP family -func UpdateIPsSlice(s, oldIPs, newIPs []string) []string { +func UpdateIPsSlice(s, oldIPs, newIPs []string) ([]string, bool) { n := make([]string, len(s)) copy(n, s) + updated := false for i, entry := range s { for _, oldIP := range oldIPs { if entry == oldIP { @@ -178,11 +179,13 @@ func UpdateIPsSlice(s, oldIPs, newIPs []string) []string { if utilnet.IsIPv6(net.ParseIP(oldIP)) { if utilnet.IsIPv6(net.ParseIP(newIP)) { n[i] = newIP + updated = true break } } else { if !utilnet.IsIPv6(net.ParseIP(newIP)) { n[i] = newIP + updated = true break } } @@ -191,7 +194,7 @@ func UpdateIPsSlice(s, oldIPs, newIPs []string) []string { } } } - return n + return n, updated } // FilterIPsSlice will filter a list of IPs by a list of CIDRs. By default, diff --git a/go-controller/pkg/util/util_unit_test.go b/go-controller/pkg/util/util_unit_test.go index 4de5f936a53..4e8437aa7aa 100644 --- a/go-controller/pkg/util/util_unit_test.go +++ b/go-controller/pkg/util/util_unit_test.go @@ -110,6 +110,7 @@ func TestUpdateIPsSlice(t *testing.T) { name string s, oldIPs, newIPs []string want []string + changed bool }{ { "Tests no matching IPs to remove", @@ -117,6 +118,7 @@ func TestUpdateIPsSlice(t *testing.T) { []string{"1.1.1.1"}, []string{"9.9.9.9", "fe99::1"}, []string{"192.168.1.1", "10.0.0.1", "127.0.0.2"}, + false, }, { "Tests some matching IPs to replace", @@ -124,6 +126,7 @@ func TestUpdateIPsSlice(t *testing.T) { []string{"10.0.0.1"}, []string{"9.9.9.9", "fe99::1"}, []string{"192.168.1.1", "9.9.9.9", "127.0.0.2"}, + true, }, { "Tests matching IPv6 to replace", @@ -131,6 +134,7 @@ func TestUpdateIPsSlice(t *testing.T) { []string{"3dfd::99ac"}, []string{"9.9.9.9", "fe99::1"}, []string{"fed9::5", "ab13::1e15", "fe99::1"}, + true, }, { "Tests match but nothing to replace with", @@ -138,6 +142,7 @@ func TestUpdateIPsSlice(t *testing.T) { []string{"3dfd::99ac"}, []string{"9.9.9.9"}, []string{"fed9::5", "ab13::1e15", "3dfd::99ac"}, + false, }, { "Tests with no newIPs", @@ -145,6 +150,7 @@ func TestUpdateIPsSlice(t *testing.T) { []string{"3dfd::99ac"}, []string{}, []string{"fed9::5", "ab13::1e15", "3dfd::99ac"}, + false, }, { "Tests with no newIPs or oldIPs", @@ -152,15 +158,20 @@ func TestUpdateIPsSlice(t *testing.T) { []string{}, []string{}, []string{"fed9::5", "ab13::1e15", "3dfd::99ac"}, + false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ans := UpdateIPsSlice(tt.s, tt.oldIPs, tt.newIPs) + ans, changed := UpdateIPsSlice(tt.s, tt.oldIPs, tt.newIPs) if !reflect.DeepEqual(ans, tt.want) { t.Errorf("got %v, want %v", ans, tt.want) } + + if tt.changed != changed { + t.Errorf("got changed %t, want %t", changed, tt.changed) + } }) } }