Make manage-security-groups work with OVN (#2291)

OVN keeps the source IP of the incoming traffic. This means that the SG
on the members of the LB (nodes) needs to have a rule opening traffic
not only from inside the cluster, but also elsewhere. This can
restricted by using the `Service.Spec.LoadBalancerSourceRanges` and this
commit also makes sure the setting is taken into account.

Author: dulek

pkg/openstack/loadbalancer.go

@@ -1393,8 +1393,10 @@ func (lbaas *LbaasV2) buildListenerCreateOpt(port corev1.ServicePort, svcConf *s
 		listenerCreateOpt.Protocol = listeners.ProtocolHTTP
 	}
 
-	if len(svcConf.allowedCIDR) > 0 {
-		listenerCreateOpt.AllowedCIDRs = svcConf.allowedCIDR
+	if openstackutil.IsOctaviaFeatureSupported(lbaas.lb, openstackutil.OctaviaFeatureVIPACL, lbaas.opts.LBProvider) {
+		if len(svcConf.allowedCIDR) > 0 {
+			listenerCreateOpt.AllowedCIDRs = svcConf.allowedCIDR
+		}
 	}
 	return listenerCreateOpt
 }
@@ -1751,18 +1753,19 @@ func (lbaas *LbaasV2) checkService(service *corev1.Service, nodes []*corev1.Node
 		svcConf.timeoutTCPInspect = getIntFromServiceAnnotation(service, ServiceAnnotationLoadBalancerTimeoutTCPInspect, 0)
 	}
 
-	var listenerAllowedCIDRs []string
 	sourceRanges, err := GetLoadBalancerSourceRanges(service, svcConf.preferredIPFamily)
 	if err != nil {
 		return fmt.Errorf("failed to get source ranges for loadbalancer service %s: %v", serviceName, err)
 	}
 	if openstackutil.IsOctaviaFeatureSupported(lbaas.lb, openstackutil.OctaviaFeatureVIPACL, lbaas.opts.LBProvider) {
 		klog.V(4).Info("LoadBalancerSourceRanges is suppported")
-		listenerAllowedCIDRs = sourceRanges.StringSlice()
+		svcConf.allowedCIDR = sourceRanges.StringSlice()
+	} else if lbaas.opts.LBProvider == "ovn" && lbaas.opts.ManageSecurityGroups {
+		klog.V(4).Info("LoadBalancerSourceRanges will be enforced on the SG created and attached to LB members")
+		svcConf.allowedCIDR = sourceRanges.StringSlice()
 	} else {
-		klog.Warning("LoadBalancerSourceRanges is ignored")
+		klog.Warning("LoadBalancerSourceRanges are ignored")
 	}
-	svcConf.allowedCIDR = listenerAllowedCIDRs
 
 	if openstackutil.IsOctaviaFeatureSupported(lbaas.lb, openstackutil.OctaviaFeatureFlavors, lbaas.opts.LBProvider) {
 		svcConf.flavorID = getStringFromServiceAnnotation(service, ServiceAnnotationLoadBalancerFlavorID, lbaas.opts.FlavorID)
@@ -2009,7 +2012,7 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName
 	status := lbaas.createLoadBalancerStatus(service, svcConf, addr)
 
 	if lbaas.opts.ManageSecurityGroups {
-		err := lbaas.ensureSecurityGroup(clusterName, service, nodes, loadbalancer, svcConf.preferredIPFamily, svcConf.lbMemberSubnetID)
+		err := lbaas.ensureAndUpdateOctaviaSecurityGroup(clusterName, service, nodes, svcConf)
 		if err != nil {
 			return status, fmt.Errorf("failed when reconciling security groups for LB service %v/%v: %v", service.Namespace, service.Name, err)
 		}
@@ -2070,14 +2073,6 @@ func (lbaas *LbaasV2) ensureSecurityRule(sgRuleCreateOpts rules.CreateOpts) erro
 	return nil
 }
 
-// ensureSecurityGroup ensures security group exist for specific loadbalancer service.
-// Creating security group for specific loadbalancer service when it does not exist.
-func (lbaas *LbaasV2) ensureSecurityGroup(clusterName string, apiService *corev1.Service, nodes []*corev1.Node,
-	loadbalancer *loadbalancers.LoadBalancer, preferredIPFamily corev1.IPFamily, memberSubnetID string) error {
-
-	return lbaas.ensureAndUpdateOctaviaSecurityGroup(clusterName, apiService, nodes, memberSubnetID)
-}
-
 func (lbaas *LbaasV2) updateOctaviaLoadBalancer(ctx context.Context, clusterName string, service *corev1.Service, nodes []*corev1.Node) error {
 	svcConf := new(serviceConfig)
 	var err error
@@ -2140,7 +2135,7 @@ func (lbaas *LbaasV2) updateOctaviaLoadBalancer(ctx context.Context, clusterName
 	}
 
 	if lbaas.opts.ManageSecurityGroups {
-		err := lbaas.updateSecurityGroup(clusterName, service, nodes, svcConf.lbMemberSubnetID)
+		err := lbaas.ensureAndUpdateOctaviaSecurityGroup(clusterName, service, nodes, svcConf)
 		if err != nil {
 			return fmt.Errorf("failed to update Security Group for loadbalancer service %s: %v", serviceName, err)
 		}
@@ -2204,7 +2199,7 @@ func getRulesToCreateAndDelete(wantedRules []rules.CreateOpts, existingRules []r
 }
 
 // ensureAndUpdateOctaviaSecurityGroup handles the creation and update of the security group and the securiry rules for the octavia load balancer
-func (lbaas *LbaasV2) ensureAndUpdateOctaviaSecurityGroup(clusterName string, apiService *corev1.Service, nodes []*corev1.Node, memberSubnetID string) error {
+func (lbaas *LbaasV2) ensureAndUpdateOctaviaSecurityGroup(clusterName string, apiService *corev1.Service, nodes []*corev1.Node, svcConf *serviceConfig) error {
 	// get service ports
 	ports := apiService.Spec.Ports
 	if len(ports) == 0 {
@@ -2238,16 +2233,23 @@ func (lbaas *LbaasV2) ensureAndUpdateOctaviaSecurityGroup(clusterName string, ap
 	}
 
 	mc := metrics.NewMetricContext("subnet", "get")
-	subnet, err := subnets.Get(lbaas.network, memberSubnetID).Extract()
+	subnet, err := subnets.Get(lbaas.network, svcConf.lbMemberSubnetID).Extract()
 	if mc.ObserveRequest(err) != nil {
 		return fmt.Errorf(
-			"failed to find subnet %s from openstack: %v", memberSubnetID, err)
+			"failed to find subnet %s from openstack: %v", svcConf.lbMemberSubnetID, err)
 	}
 
 	etherType := rules.EtherType4
 	if netutils.IsIPv6CIDRString(subnet.CIDR) {
 		etherType = rules.EtherType6
 	}
+	cidrs := []string{subnet.CIDR}
+	if lbaas.opts.LBProvider == "ovn" {
+		// OVN keeps the source IP of the incoming traffic. This means that we cannot just open the LB range, but we
+		// need to open for the whole world. This can be restricted by using the service.spec.loadBalancerSourceRanges.
+		// svcConf.allowedCIDR will give us the ranges calculated by GetLoadBalancerSourceRanges() earlier.
+		cidrs = svcConf.allowedCIDR
+	}
 
 	existingRules, err := getSecurityGroupRules(lbaas.network, rules.ListOpts{SecGroupID: lbSecGroupID})
 	if err != nil {
@@ -2260,6 +2262,8 @@ func (lbaas *LbaasV2) ensureAndUpdateOctaviaSecurityGroup(clusterName string, ap
 	wantedRules := make([]rules.CreateOpts, 0, len(ports)+1)
 
 	if apiService.Spec.HealthCheckNodePort != 0 {
+		// TODO(dulek): How should this work with OVN…? Do we need to allow all?
+		//              Probably the traffic goes from the compute node?
 		wantedRules = append(wantedRules,
 			rules.CreateOpts{
 				Direction:      rules.DirIngress,
@@ -2277,17 +2281,19 @@ func (lbaas *LbaasV2) ensureAndUpdateOctaviaSecurityGroup(clusterName string, ap
 		if port.NodePort == 0 { // It's 0 when AllocateLoadBalancerNodePorts=False
 			continue
 		}
-		wantedRules = append(wantedRules,
-			rules.CreateOpts{
-				Direction:      rules.DirIngress,
-				Protocol:       rules.RuleProtocol(port.Protocol),
-				EtherType:      etherType,
-				RemoteIPPrefix: subnet.CIDR,
-				SecGroupID:     lbSecGroupID,
-				PortRangeMin:   int(port.NodePort),
-				PortRangeMax:   int(port.NodePort),
-			},
-		)
+		for _, cidr := range cidrs {
+			wantedRules = append(wantedRules,
+				rules.CreateOpts{
+					Direction:      rules.DirIngress,
+					Protocol:       rules.RuleProtocol(port.Protocol),
+					EtherType:      etherType,
+					RemoteIPPrefix: cidr,
+					SecGroupID:     lbSecGroupID,
+					PortRangeMin:   int(port.NodePort),
+					PortRangeMax:   int(port.NodePort),
+				},
+			)
+		}
 	}
 
 	toCreate, toDelete := getRulesToCreateAndDelete(wantedRules, existingRules)
@@ -2321,11 +2327,6 @@ func (lbaas *LbaasV2) ensureAndUpdateOctaviaSecurityGroup(clusterName string, ap
 	return nil
 }
 
-// updateSecurityGroup updating security group for specific loadbalancer service.
-func (lbaas *LbaasV2) updateSecurityGroup(clusterName string, apiService *corev1.Service, nodes []*corev1.Node, memberSubnetID string) error {
-	return lbaas.ensureAndUpdateOctaviaSecurityGroup(clusterName, apiService, nodes, memberSubnetID)
-}
-
 // EnsureLoadBalancerDeleted deletes the specified load balancer
 func (lbaas *LbaasV2) EnsureLoadBalancerDeleted(ctx context.Context, clusterName string, service *corev1.Service) error {
 	mc := metrics.NewMetricContext("loadbalancer", "delete")