manage-security-groups: Only add SGs to LB members (#2455) (#2466)

Previous code for manage-security-groups were adding the SGs to all the
ports attached to the Node. That doesn't make a lot of sense, we only
need this traffic opened to the LB members and we only add one port of
the Node as the LB member. Moreover we've even tried to add SGs to the
ports that had `port_security_enabled=false` which obviously failed.

This patch makes sure that we only add the SG opening NodePort traffic
to the port that is actually plugged into the LB. Moreover it adds a
check for `port_security_enabled` to make sure that ports with it
disabled are ignored.

Author: dulek

pkg/openstack/loadbalancer.go

@@ -717,25 +717,52 @@ func getSubnetIDForLB(network *gophercloud.ServiceClient, node corev1.Node, pref
 	return "", cpoerrors.ErrNotFound
 }
 
-// applyNodeSecurityGroupIDForLB associates the security group with all the ports on the nodes.
-func applyNodeSecurityGroupIDForLB(network *gophercloud.ServiceClient, nodes []*corev1.Node, sg string) error {
+// isPortMember returns true if IP and subnetID are one of the FixedIPs on the port
+func isPortMember(port PortWithPortSecurity, IP string, subnetID string) bool {
+	for _, fixedIP := range port.FixedIPs {
+		if (subnetID == "" || subnetID == fixedIP.SubnetID) && IP == fixedIP.IPAddress {
+			return true
+		}
+	}
+	return false
+}
+
+// applyNodeSecurityGroupIDForLB associates the security group with the ports being members of the LB on the nodes.
+func applyNodeSecurityGroupIDForLB(network *gophercloud.ServiceClient, svcConf *serviceConfig, nodes []*corev1.Node, sg string) error {
 	for _, node := range nodes {
 		serverID, _, err := instanceIDFromProviderID(node.Spec.ProviderID)
 		if err != nil {
 			return fmt.Errorf("error getting server ID from the node: %w", err)
 		}
+
+		addr, _ := nodeAddressForLB(node, svcConf.preferredIPFamily)
+		if addr == "" {
+			// If node has no viable address let's ignore it.
+			continue
+		}
+
 		listOpts := neutronports.ListOpts{DeviceID: serverID}
-		allPorts, err := openstackutil.GetPorts(network, listOpts)
+		allPorts, err := openstackutil.GetPorts[PortWithPortSecurity](network, listOpts)
 		if err != nil {
 			return err
 		}
 
 		for _, port := range allPorts {
+			// You can't assign an SG to a port with port_security_enabled=false, skip them.
+			if !port.PortSecurityEnabled {
+				continue
+			}
+
 			// If the Security Group is already present on the port, skip it.
 			if slices.Contains(port.SecurityGroups, sg) {
 				continue
 			}
 
+			// Only add SGs to the port actually attached to the LB
+			if !isPortMember(port, addr, svcConf.lbMemberSubnetID) {
+				continue
+			}
+
 			// Add the security group ID as a tag to the port in order to find all these ports when removing the security group.
 			// We're doing that before actually applying the SG as if tagging would fail we wouldn't be able to find the port
 			// when deleting the SG and operation would be stuck forever. It's better to find more ports than not all of them.
@@ -766,7 +793,7 @@ func applyNodeSecurityGroupIDForLB(network *gophercloud.ServiceClient, nodes []*
 func disassociateSecurityGroupForLB(network *gophercloud.ServiceClient, sg string) error {
 	// Find all the ports that have the security group associated.
 	listOpts := neutronports.ListOpts{TagsAny: sg}
-	allPorts, err := openstackutil.GetPorts(network, listOpts)
+	allPorts, err := openstackutil.GetPorts[neutronports.Port](network, listOpts)
 	if err != nil {
 		return err
 	}
@@ -2353,7 +2380,7 @@ func (lbaas *LbaasV2) ensureAndUpdateOctaviaSecurityGroup(clusterName string, ap
 		}
 	}
 
-	if err := applyNodeSecurityGroupIDForLB(lbaas.network, nodes, lbSecGroupID); err != nil {
+	if err := applyNodeSecurityGroupIDForLB(lbaas.network, svcConf, nodes, lbSecGroupID); err != nil {
 		return err
 	}
 	return nil

pkg/openstack/openstack.go

@@ -27,6 +27,7 @@ import (
 	"github.com/gophercloud/gophercloud"
 	"github.com/gophercloud/gophercloud/openstack/compute/v2/extensions/availabilityzones"
 	"github.com/gophercloud/gophercloud/openstack/compute/v2/servers"
+	"github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/portsecurity"
 	"github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/trunk_details"
 	neutronports "github.com/gophercloud/gophercloud/openstack/networking/v2/ports"
 	"github.com/spf13/pflag"
@@ -74,6 +75,11 @@ type PortWithTrunkDetails struct {
 	trunk_details.TrunkDetailsExt
 }
 
+type PortWithPortSecurity struct {
+	neutronports.Port
+	portsecurity.PortSecurityExt
+}
+
 // LoadBalancer is used for creating and maintaining load balancers
 type LoadBalancer struct {
 	secret  *gophercloud.ServiceClient

pkg/util/openstack/network.go

@@ -140,15 +140,16 @@ func getSubnet(networkSubnet string, subnetList []subnets.Subnet) *subnets.Subne
 }
 
 // GetPorts gets all the filtered ports.
-func GetPorts(client *gophercloud.ServiceClient, listOpts neutronports.ListOpts) ([]neutronports.Port, error) {
+func GetPorts[PortType interface{}](client *gophercloud.ServiceClient, listOpts neutronports.ListOpts) ([]PortType, error) {
 	mc := metrics.NewMetricContext("port", "list")
 	allPages, err := neutronports.List(client, listOpts).AllPages()
 	if mc.ObserveRequest(err) != nil {
-		return []neutronports.Port{}, err
+		return []PortType{}, err
 	}
-	allPorts, err := neutronports.ExtractPorts(allPages)
+	var allPorts []PortType
+	err = neutronports.ExtractPortsInto(allPages, &allPorts)
 	if err != nil {
-		return []neutronports.Port{}, err
+		return []PortType{}, err
 	}
 
 	return allPorts, nil