bgpv1: Local internalTrafficPolicy support for ClusterIP advertisements

Fixes: 31389 Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
cilium · Mar 19, 2024 · f50fc00 · f50fc00
1 parent ed4e650
commit f50fc00
Show file tree

Hide file tree

Showing 2 changed files with 87 additions and 62 deletions.
diff --git a/pkg/bgpv1/manager/reconciler/service.go b/pkg/bgpv1/manager/reconciler/service.go
@@ -118,7 +118,7 @@ func (r *ServiceReconciler) populateLocalServices(localNodeName string) (localSe
 
 endpointsLoop:
 	for _, eps := range epList {
-		svc, exists, err := r.resolveSvcFromEndpoints(eps)
+		_, exists, err := r.resolveSvcFromEndpoints(eps)
 		if err != nil {
 			// Cannot resolve service from endpoints. We have nothing to do here.
 			continue
@@ -129,11 +129,6 @@ endpointsLoop:
 			continue
 		}
 
-		// We only need Endpoints tracking for externalTrafficPolicy=Local
-		if svc.Spec.ExternalTrafficPolicy != slim_corev1.ServiceExternalTrafficPolicyLocal {
-			continue
-		}
-
 		svcID := eps.ServiceID
 
 		for _, be := range eps.Backends {
@@ -295,72 +290,101 @@ func (r *ServiceReconciler) svcDesiredRoutes(newc *v2alpha1api.CiliumBGPVirtualR
 	if !svcSelector.Matches(serviceLabelSet(svc)) {
 		return nil, nil
 	}
-	// Ignore externalTrafficPolicy == Local && no local endpoints.
-	if svc.Spec.ExternalTrafficPolicy == slim_corev1.ServiceExternalTrafficPolicyLocal &&
-		!hasLocalEndpoints(svc, ls) {
-		return nil, nil
-	}
+
 	var desiredRoutes []netip.Prefix
 	// Loop over the service advertisements and determine the desired routes.
 	for _, svcAdv := range newc.ServiceAdvertisements {
 		switch svcAdv {
 		case v2alpha1api.BGPLoadBalancerIPAddr:
-			if svc.Spec.Type != slim_corev1.ServiceTypeLoadBalancer {
-				continue
-			}
-			// Ignore service managed by an unsupported LB class.
-			if svc.Spec.LoadBalancerClass != nil && *svc.Spec.LoadBalancerClass != v2alpha1api.BGPLoadBalancerClass {
-				// The service is managed by a different LB class.
-				continue
-			}
-			for _, ingress := range svc.Status.LoadBalancer.Ingress {
-				if ingress.IP == "" {
-					continue
-				}
-				addr, err := netip.ParseAddr(ingress.IP)
-				if err != nil {
-					continue
-				}
-				desiredRoutes = append(desiredRoutes, netip.PrefixFrom(addr, addr.BitLen()))
-			}
+			desiredRoutes = append(desiredRoutes, r.lbSvcDesiredRoutes(svc, ls)...)
 		case v2alpha1api.BGPClusterIPAddr:
-			if svc.Spec.ClusterIP == "" || len(svc.Spec.ClusterIPs) == 0 || svc.Spec.ClusterIP == corev1.ClusterIPNone {
-				continue
-			}
-			ips := sets.New[string]()
-			if svc.Spec.ClusterIP != "" {
-				ips.Insert(svc.Spec.ClusterIP)
-			}
-			for _, clusterIP := range svc.Spec.ClusterIPs {
-				if clusterIP == "" || clusterIP == corev1.ClusterIPNone {
-					continue
-				}
-				ips.Insert(clusterIP)
-			}
-			for _, ip := range sets.List(ips) {
-				addr, err := netip.ParseAddr(ip)
-				if err != nil {
-					continue
-				}
-				desiredRoutes = append(desiredRoutes, netip.PrefixFrom(addr, addr.BitLen()))
-			}
+			desiredRoutes = append(desiredRoutes, r.clusterIPDesiredRoutes(svc, ls)...)
 		case v2alpha1api.BGPExternalIPAddr:
-			for _, extIP := range svc.Spec.ExternalIPs {
-				if extIP == "" {
-					continue
-				}
-				addr, err := netip.ParseAddr(extIP)
-				if err != nil {
-					continue
-				}
-				desiredRoutes = append(desiredRoutes, netip.PrefixFrom(addr, addr.BitLen()))
-			}
+			desiredRoutes = append(desiredRoutes, r.externalIPDesiredRoutes(svc, ls)...)
 		}
 	}
 
 	return desiredRoutes, err
 }
 
+func (r *ServiceReconciler) externalIPDesiredRoutes(svc *slim_corev1.Service, ls localServices) []netip.Prefix {
+	var desiredRoutes []netip.Prefix
+	// Ignore externalTrafficPolicy == Local && no local endpoints.
+	if svc.Spec.ExternalTrafficPolicy == slim_corev1.ServiceExternalTrafficPolicyLocal &&
+		!hasLocalEndpoints(svc, ls) {
+		return desiredRoutes
+	}
+	for _, extIP := range svc.Spec.ExternalIPs {
+		if extIP == "" {
+			continue
+		}
+		addr, err := netip.ParseAddr(extIP)
+		if err != nil {
+			continue
+		}
+		desiredRoutes = append(desiredRoutes, netip.PrefixFrom(addr, addr.BitLen()))
+	}
+	return desiredRoutes
+}
+
+func (r *ServiceReconciler) clusterIPDesiredRoutes(svc *slim_corev1.Service, ls localServices) []netip.Prefix {
+	var desiredRoutes []netip.Prefix
+	// Ignore internalTrafficPolicy == Local && no local endpoints.
+	if svc.Spec.InternalTrafficPolicy != nil && *svc.Spec.InternalTrafficPolicy == slim_corev1.ServiceInternalTrafficPolicyLocal &&
+		!hasLocalEndpoints(svc, ls) {
+		return desiredRoutes
+	}
+	if svc.Spec.ClusterIP == "" || len(svc.Spec.ClusterIPs) == 0 || svc.Spec.ClusterIP == corev1.ClusterIPNone {
+		return desiredRoutes
+	}
+	ips := sets.New[string]()
+	if svc.Spec.ClusterIP != "" {
+		ips.Insert(svc.Spec.ClusterIP)
+	}
+	for _, clusterIP := range svc.Spec.ClusterIPs {
+		if clusterIP == "" || clusterIP == corev1.ClusterIPNone {
+			continue
+		}
+		ips.Insert(clusterIP)
+	}
+	for _, ip := range sets.List(ips) {
+		addr, err := netip.ParseAddr(ip)
+		if err != nil {
+			continue
+		}
+		desiredRoutes = append(desiredRoutes, netip.PrefixFrom(addr, addr.BitLen()))
+	}
+	return desiredRoutes
+}
+
+func (r *ServiceReconciler) lbSvcDesiredRoutes(svc *slim_corev1.Service, ls localServices) []netip.Prefix {
+	var desiredRoutes []netip.Prefix
+	if svc.Spec.Type != slim_corev1.ServiceTypeLoadBalancer {
+		return desiredRoutes
+	}
+	// Ignore externalTrafficPolicy == Local && no local endpoints.
+	if svc.Spec.ExternalTrafficPolicy == slim_corev1.ServiceExternalTrafficPolicyLocal &&
+		!hasLocalEndpoints(svc, ls) {
+		return desiredRoutes
+	}
+	// Ignore service managed by an unsupported LB class.
+	if svc.Spec.LoadBalancerClass != nil && *svc.Spec.LoadBalancerClass != v2alpha1api.BGPLoadBalancerClass {
+		// The service is managed by a different LB class.
+		return desiredRoutes
+	}
+	for _, ingress := range svc.Status.LoadBalancer.Ingress {
+		if ingress.IP == "" {
+			continue
+		}
+		addr, err := netip.ParseAddr(ingress.IP)
+		if err != nil {
+			continue
+		}
+		desiredRoutes = append(desiredRoutes, netip.PrefixFrom(addr, addr.BitLen()))
+	}
+	return desiredRoutes
+}
+
 // reconcileService gets the desired routes of a given service and makes sure that is what is being announced.
 func (r *ServiceReconciler) reconcileService(ctx context.Context, sc *instance.ServerWithConfig, newc *v2alpha1api.CiliumBGPVirtualRouter, svc *slim_corev1.Service, ls localServices) error {
 

diff --git a/pkg/bgpv1/manager/reconciler/service_test.go b/pkg/bgpv1/manager/reconciler/service_test.go
@@ -740,14 +740,15 @@ func TestServiceReconcilerWithClusterIP(t *testing.T) {
 	svc1NonClusterIP.Spec.ClusterIP = "None"
 	svc1NonClusterIP.Spec.ClusterIPs = append(svc1NonClusterIP.Spec.ClusterIPs, "None")
 	svc1ETPLocal := svc1.DeepCopy()
-	svc1ETPLocal.Spec.ExternalTrafficPolicy = slim_corev1.ServiceExternalTrafficPolicyLocal
+	internalTrafficPolicyLocal := slim_corev1.ServiceInternalTrafficPolicyLocal
+	svc1ETPLocal.Spec.InternalTrafficPolicy = &internalTrafficPolicyLocal
 
 	svc1ETPLocalTwoIngress := svc1TwoIngress.DeepCopy()
-	svc1ETPLocalTwoIngress.Spec.ExternalTrafficPolicy = slim_corev1.ServiceExternalTrafficPolicyLocal
+	svc1ETPLocalTwoIngress.Spec.InternalTrafficPolicy = &internalTrafficPolicyLocal
 
 	svc1IPv6ETPLocal := svc1.DeepCopy()
 	svc1IPv6ETPLocal.Spec.ClusterIPs = append(svc1IPv6ETPLocal.Spec.ClusterIPs, clusterIPV6)
-	svc1IPv6ETPLocal.Spec.ExternalTrafficPolicy = slim_corev1.ServiceExternalTrafficPolicyLocal
+	svc1IPv6ETPLocal.Spec.InternalTrafficPolicy = &internalTrafficPolicyLocal
 
 	svc1LbClass := svc1.DeepCopy()
 	svc1LbClass.Spec.LoadBalancerClass = pointer.String(v2alpha1api.BGPLoadBalancerClass)