Merge pull request #244 from dulek/backport-port-security-fix

OCPBUGS-22974: [release-4.14] manage-security-groups: Only add SGs to LB members (kubernetes#2455)

pkg/openstack/loadbalancer.go

@@ -715,20 +715,42 @@ 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
@@ -743,6 +765,11 @@ func applyNodeSecurityGroupIDForLB(network *gophercloud.ServiceClient, nodes []*
 				return fmt.Errorf("failed to add tag %s to port %s: %v", sg, port.ID, err)
 			}
 
+			// Only add SGs to the port actually attached to the LB
+			if !isPortMember(port, addr, svcConf.lbMemberSubnetID) {
+				continue
+			}
+
 			// Add the SG to the port
 			// TODO(dulek): This isn't an atomic operation. In order to protect from lost update issues we should use
 			//              `revision_number` handling to make sure our update to `security_groups` field wasn't preceded
@@ -764,7 +791,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
 	}
@@ -2343,7 +2370,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

vendor/modules.txt

@@ -183,6 +183,7 @@ github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/external
 github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/layer3/extraroutes
 github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/layer3/floatingips
 github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/layer3/routers
+github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/portsecurity
 github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/security/groups
 github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/security/rules
 github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/trunk_details