From fdba36babb2c4b46e759c99cca50ac7eba2ee06f Mon Sep 17 00:00:00 2001 From: Michal Dulko Date: Fri, 3 Nov 2023 11:03:55 +0100 Subject: [PATCH] manage-security-groups: Only add SGs to LB members (#2455) 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. --- pkg/openstack/loadbalancer.go | 37 ++++++++++++++++++++++++++++++----- pkg/openstack/openstack.go | 6 ++++++ pkg/util/openstack/network.go | 9 +++++---- 3 files changed, 43 insertions(+), 9 deletions(-) diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go index fab2ad5008..6c829e33b7 100644 --- a/pkg/openstack/loadbalancer.go +++ b/pkg/openstack/loadbalancer.go @@ -728,25 +728,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 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 @@ -768,7 +795,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{SecurityGroups: []string{sg}} - allPorts, err := openstackutil.GetPorts(network, listOpts) + allPorts, err := openstackutil.GetPorts[neutronports.Port](network, listOpts) if err != nil { return err } @@ -2382,7 +2409,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 diff --git a/pkg/openstack/openstack.go b/pkg/openstack/openstack.go index c0c8bcd16a..b67a58caf3 100644 --- a/pkg/openstack/openstack.go +++ b/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" @@ -78,6 +79,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 diff --git a/pkg/util/openstack/network.go b/pkg/util/openstack/network.go index 0a811a2ae4..889086f480 100644 --- a/pkg/util/openstack/network.go +++ b/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