Manage Security Groups for Octavia (#1984)

This patch makes it possible to use the manage-security-groups with octavia load balancer. It creates a new function `ensureAndUpdateOctaviaSecurityGroup` which is called on `EnsureLoadBalancer` and `UpdateLoadBalancer` and handles the security group and security rule creation for Octavia Load Balancer. To make the changes not bigger than needed, it hooks into the `updateSecurityGroup` and `ensureSecurityGroup` function first, checks if Octavia is used and uses the new code. Otherwise the legacy code is called. So the removal of the legacy code should be quite easy. Also it adds a missing security rule for the HealthCheckNodePort. Signed-off-by: Sven Haardiek <sven.haardiek@uni-muenster.de> Signed-off-by: Sven Haardiek <sven.haardiek@uni-muenster.de>
kubernetes · Sep 28, 2022 · 42f4ede · 42f4ede
1 parent 80dffd2
commit 42f4ede
Show file tree

Hide file tree

Showing 2 changed files with 166 additions and 5 deletions.
diff --git a/.../openstack-cloud-controller-manager/using-openstack-cloud-controller-manager.md b/.../openstack-cloud-controller-manager/using-openstack-cloud-controller-manager.md
@@ -211,8 +211,6 @@ Although the openstack-cloud-controller-manager was initially implemented with N
 * `manage-security-groups`
   If the Neutron security groups should be managed separately. Default: false
 
-  This option is not supported for Octavia. The worker nodes and the Octavia amphorae are usually in the same subnet, so it's sufficient to config the port security group rules manually for worker nodes, to allow the traffic coming from the the subnet IP range to the node port range(i.e. 30000-32767).
-
 * `create-monitor`
   Indicates whether or not to create a health monitor for the service load balancer. A health monitor required for services that declare `externalTrafficPolicy: Local`. Default: false
 

diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go
@@ -862,6 +862,20 @@ func applyNodeSecurityGroupIDForLB(compute *gophercloud.ServiceClient, network *
 		}
 
 		for _, port := range allPorts {
+			// If the Security Group is already present on the port, skip it.
+			// As soon as this only supports Go 1.18, this can be replaces by
+			// slices.Contains.
+			if func() bool {
+				for _, currentSG := range port.SecurityGroups {
+					if currentSG == sg {
+						return true
+					}
+				}
+				return false
+			}() {
+				continue
+			}
+
 			newSGs := append(port.SecurityGroups, sg)
 			updateOpts := neutronports.UpdateOpts{SecurityGroups: &newSGs}
 			mc := metrics.NewMetricContext("port", "update")
@@ -2633,10 +2647,64 @@ func (lbaas *LbaasV2) getSubnet(subnet string) (*subnets.Subnet, error) {
 	return nil, fmt.Errorf("found multiple subnets with name %s", subnet)
 }
 
+// ensureSecurityRule ensures to create a security rule for a defined security
+// group, if it not present.
+func (lbaas *LbaasV2) ensureSecurityRule(
+	direction rules.RuleDirection,
+	protocol rules.RuleProtocol,
+	etherType rules.RuleEtherType,
+	remoteIPPrefix, secGroupID string,
+	portRangeMin, portRangeMax int,
+) error {
+	sgListopts := rules.ListOpts{
+		Direction:      string(direction),
+		Protocol:       string(protocol),
+		PortRangeMax:   portRangeMin,
+		PortRangeMin:   portRangeMax,
+		RemoteIPPrefix: remoteIPPrefix,
+		SecGroupID:     secGroupID,
+	}
+	sgRules, err := getSecurityGroupRules(lbaas.network, sgListopts)
+	if err != nil && !cpoerrors.IsNotFound(err) {
+		return fmt.Errorf(
+			"failed to find security group rules in %s: %v", secGroupID, err)
+	}
+	if len(sgRules) != 0 {
+		return nil
+	}
+
+	sgRuleCreateOpts := rules.CreateOpts{
+		Direction:      direction,
+		Protocol:       protocol,
+		PortRangeMax:   portRangeMin,
+		PortRangeMin:   portRangeMax,
+		RemoteIPPrefix: remoteIPPrefix,
+		SecGroupID:     secGroupID,
+		EtherType:      etherType,
+	}
+
+	mc := metrics.NewMetricContext("security_group_rule", "create")
+	_, err = rules.Create(lbaas.network, sgRuleCreateOpts).Extract()
+	if mc.ObserveRequest(err) != nil {
+		return fmt.Errorf(
+			"failed to create rule for security group %s: %v",
+			secGroupID, err)
+	}
+	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 {
+
+	if lbaas.opts.UseOctavia {
+		return lbaas.ensureAndUpdateOctaviaSecurityGroup(clusterName, apiService, nodes, memberSubnetID)
+	}
+
+	// Following code is just for legacy Neutron-LBaaS support which has been deprecated since OpenStack stable/queens
+	// and not recommended using in production. No new features should be added.
+
 	// find node-security-group for service
 	var err error
 	if len(lbaas.opts.NodeSecurityGroupIDs) == 0 && !lbaas.opts.UseOctavia {
@@ -2928,7 +2996,7 @@ func (lbaas *LbaasV2) updateOctaviaLoadBalancer(ctx context.Context, clusterName
 	}
 
 	if lbaas.opts.ManageSecurityGroups {
-		err := lbaas.updateSecurityGroup(clusterName, service, nodes)
+		err := lbaas.updateSecurityGroup(clusterName, service, nodes, svcConf.lbMemberSubnetID)
 		if err != nil {
 			return fmt.Errorf("failed to update Security Group for loadbalancer service %s: %v", serviceName, err)
 		}
@@ -3092,7 +3160,8 @@ func (lbaas *LbaasV2) updateLoadBalancer(ctx context.Context, clusterName string
 	}
 
 	if lbaas.opts.ManageSecurityGroups {
-		err := lbaas.updateSecurityGroup(clusterName, service, nodes)
+		// MemberSubnetID is not needed, since this is the legacy call
+		err := lbaas.updateSecurityGroup(clusterName, service, nodes, "")
 		if err != nil {
 			return fmt.Errorf("failed to update Security Group for loadbalancer service %s: %v", serviceName, err)
 		}
@@ -3101,8 +3170,102 @@ func (lbaas *LbaasV2) updateLoadBalancer(ctx context.Context, clusterName string
 	return nil
 }
 
+// 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 {
+	// get service ports
+	ports := apiService.Spec.Ports
+	if len(ports) == 0 {
+		return fmt.Errorf("no ports provided to openstack load balancer")
+	}
+
+	// ensure security group for LB
+	lbSecGroupName := getSecurityGroupName(apiService)
+	lbSecGroupID, err := secgroups.IDFromName(lbaas.network, lbSecGroupName)
+	if err != nil {
+		// If the security group of LB not exist, create it later
+		if isSecurityGroupNotFound(err) {
+			lbSecGroupID = ""
+		} else {
+			return fmt.Errorf("error occurred finding security group: %s: %v", lbSecGroupName, err)
+		}
+	}
+	if len(lbSecGroupID) == 0 {
+		// create security group
+		lbSecGroupCreateOpts := groups.CreateOpts{
+			Name:        lbSecGroupName,
+			Description: fmt.Sprintf("Security Group for %s/%s Service LoadBalancer in cluster %s", apiService.Namespace, apiService.Name, clusterName),
+		}
+
+		mc := metrics.NewMetricContext("security_group", "create")
+		lbSecGroup, err := groups.Create(lbaas.network, lbSecGroupCreateOpts).Extract()
+		if mc.ObserveRequest(err) != nil {
+			return fmt.Errorf("failed to create Security Group for loadbalancer service %s/%s: %v", apiService.Namespace, apiService.Name, err)
+		}
+		lbSecGroupID = lbSecGroup.ID
+	}
+
+	mc := metrics.NewMetricContext("subnet", "get")
+	subnet, err := subnets.Get(lbaas.network, memberSubnetID).Extract()
+	if mc.ObserveRequest(err) != nil {
+		return fmt.Errorf(
+			"failed to find subnet %s from openstack: %v", memberSubnetID, err)
+	}
+
+	etherType := rules.EtherType4
+	if netutils.IsIPv6CIDRString(subnet.CIDR) {
+		etherType = rules.EtherType6
+	}
+
+	if apiService.Spec.HealthCheckNodePort != 0 {
+		err = lbaas.ensureSecurityRule(
+			rules.DirIngress,
+			rules.ProtocolTCP,
+			etherType,
+			subnet.CIDR,
+			lbSecGroupID,
+			int(apiService.Spec.HealthCheckNodePort),
+			int(apiService.Spec.HealthCheckNodePort),
+		)
+		if err != nil {
+			return fmt.Errorf(
+				"failed to apply security rule for health check node port, %w",
+				err)
+		}
+	}
+
+	// ensure rules for node security group
+	for _, port := range ports {
+		err = lbaas.ensureSecurityRule(
+			rules.DirIngress,
+			rules.RuleProtocol(port.Protocol),
+			etherType,
+			subnet.CIDR,
+			lbSecGroupID,
+			int(port.NodePort),
+			int(port.NodePort),
+		)
+		if err != nil {
+			return fmt.Errorf(
+				"failed to apply security rule for port %d, %w",
+				port.NodePort, err)
+		}
+
+		if err := applyNodeSecurityGroupIDForLB(lbaas.compute, lbaas.network, nodes, lbSecGroupID); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 // updateSecurityGroup updating security group for specific loadbalancer service.
-func (lbaas *LbaasV2) updateSecurityGroup(_ string, apiService *corev1.Service, nodes []*corev1.Node) error {
+func (lbaas *LbaasV2) updateSecurityGroup(clusterName string, apiService *corev1.Service, nodes []*corev1.Node, memberSubnetID string) error {
+	if lbaas.opts.UseOctavia {
+		return lbaas.ensureAndUpdateOctaviaSecurityGroup(clusterName, apiService, nodes, memberSubnetID)
+	}
+
+	// Following code is just for legacy Neutron-LBaaS support which has been deprecated since OpenStack stable/queens
+	// and not recommended using in production. No new features should be added.
+
 	originalNodeSecurityGroupIDs := lbaas.opts.NodeSecurityGroupIDs
 
 	var err error