[occm] Lookup ports by SG and not tags (#2355)

* Update gophercloud to v1.6.0

* Lookup ports by SG and not tags

When creating an SG for an LB we tag the ports we attach the SG to in
order to be able to find them easily later on when the SG will be
getting deleted. We do that to simulate an API allowing us to filter
ports by attached SGs, but turns out - such an API already exists in
Neutron and Gophercloud v1.6.0 allows to use it.

This commit makes sure that when deleting an SG we're discovering ports
to remove it from by querying the Neutron filtering by SG. Moreover
tagging the ports with the SGs is removed to as not being necessary
anymore.

I've left the code removing the SG tags from ports for backward
compatibility.

go.mod

@@ -5,7 +5,7 @@ go 1.20
 require (
 	github.com/container-storage-interface/spec v1.8.0
 	github.com/go-chi/chi/v5 v5.0.8
-	github.com/gophercloud/gophercloud v1.4.0
+	github.com/gophercloud/gophercloud v1.6.0
 	github.com/gophercloud/utils v0.0.0-20230330070308-5bd5e1d608f8
 	github.com/hashicorp/go-version v1.6.0
 	github.com/kubernetes-csi/csi-lib-utils v0.13.0

go.sum

@@ -223,8 +223,8 @@ github.com/googleapis/gax-go/v2 v2.0.4/go.mod h1:0Wqv26UfaUD9n4G6kQubkQ+KchISgw+
 github.com/googleapis/gax-go/v2 v2.0.5/go.mod h1:DWXyrwAJ9X0FpwwEdw+IPEYBICEFu5mhpdKc/us6bOk=
 github.com/googleapis/google-cloud-go-testing v0.0.0-20200911160855-bcd43fbb19e8/go.mod h1:dvDLG8qkwmyD9a/MJJN3XJcT3xFxOKAvTZGvuZmac9g=
 github.com/gophercloud/gophercloud v1.3.0/go.mod h1:aAVqcocTSXh2vYFZ1JTvx4EQmfgzxRcNupUfxZbBNDM=
-github.com/gophercloud/gophercloud v1.4.0 h1:RqEu43vaX0lb0LanZr5BylK5ICVxjpFFoc0sxivyuHU=
-github.com/gophercloud/gophercloud v1.4.0/go.mod h1:aAVqcocTSXh2vYFZ1JTvx4EQmfgzxRcNupUfxZbBNDM=
+github.com/gophercloud/gophercloud v1.6.0 h1:JwJN1bauRnWPba5ueWs9IluONHteXPWjjK+MvfM4krY=
+github.com/gophercloud/gophercloud v1.6.0/go.mod h1:aAVqcocTSXh2vYFZ1JTvx4EQmfgzxRcNupUfxZbBNDM=
 github.com/gophercloud/utils v0.0.0-20230330070308-5bd5e1d608f8 h1:K9r5WEeAiaEgFZsuOP0OYjE4TtyFcCLG1nI08t9AP6A=
 github.com/gophercloud/utils v0.0.0-20230330070308-5bd5e1d608f8/go.mod h1:VSalo4adEk+3sNkmVJLnhHoOyOYYS8sTWLG4mv5BKto=
 github.com/gorilla/websocket v1.4.2 h1:+/TMaTYc4QFitKJxsQ7Yye35DkWvkdLcvGKqM+x0Ufc=

pkg/openstack/loadbalancer.go

@@ -732,22 +732,13 @@ func applyNodeSecurityGroupIDForLB(network *gophercloud.ServiceClient, nodes []*
 				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.
-			mc := metrics.NewMetricContext("port_tag", "add")
-			err := neutrontags.Add(network, "ports", port.ID, sg).ExtractErr()
-			if mc.ObserveRequest(err) != nil {
-				return fmt.Errorf("failed to add tag %s to port %s: %v", sg, port.ID, err)
-			}
-
 			// 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
 			//              by a different one. Same applies to a removal of the SG.
 			newSGs := append(port.SecurityGroups, sg)
 			updateOpts := neutronports.UpdateOpts{SecurityGroups: &newSGs}
-			mc = metrics.NewMetricContext("port", "update")
+			mc := metrics.NewMetricContext("port", "update")
 			res := neutronports.Update(network, port.ID, updateOpts)
 			if mc.ObserveRequest(res.Err) != nil {
 				return fmt.Errorf("failed to update security group for port %s: %v", port.ID, res.Err)
@@ -761,7 +752,7 @@ func applyNodeSecurityGroupIDForLB(network *gophercloud.ServiceClient, nodes []*
 // disassociateSecurityGroupForLB removes the given security group from the ports
 func disassociateSecurityGroupForLB(network *gophercloud.ServiceClient, sg string) error {
 	// Find all the ports that have the security group associated.
-	listOpts := neutronports.ListOpts{TagsAny: sg}
+	listOpts := neutronports.ListOpts{SecurityGroups: []string{sg}}
 	allPorts, err := openstackutil.GetPorts(network, listOpts)
 	if err != nil {
 		return err
@@ -777,17 +768,23 @@ func disassociateSecurityGroupForLB(network *gophercloud.ServiceClient, sg strin
 
 		// Update port security groups
 		newSGs := existingSGs.List()
+		// TODO(dulek): This should be done using Neutron's revision_number to make sure
+		//              we don't trigger a lost update issue.
 		updateOpts := neutronports.UpdateOpts{SecurityGroups: &newSGs}
 		mc := metrics.NewMetricContext("port", "update")
 		res := neutronports.Update(network, port.ID, updateOpts)
 		if mc.ObserveRequest(res.Err) != nil {
 			return fmt.Errorf("failed to update security group for port %s: %v", port.ID, res.Err)
 		}
-		// Remove the security group ID tag from the port.
-		mc = metrics.NewMetricContext("port_tag", "delete")
-		err := neutrontags.Delete(network, "ports", port.ID, sg).ExtractErr()
-		if mc.ObserveRequest(err) != nil {
-			return fmt.Errorf("failed to remove tag %s to port %s: %v", sg, port.ID, res.Err)
+
+		// Remove the security group ID tag from the port. Please note we don't tag ports with SG IDs anymore,
+		// so this stays for backward compatibility. It's reasonable to delete it in the future. 404s are ignored.
+		if slices.Contains(port.Tags, sg) {
+			mc = metrics.NewMetricContext("port_tag", "delete")
+			err := neutrontags.Delete(network, "ports", port.ID, sg).ExtractErr()
+			if mc.ObserveRequest(err) != nil {
+				return fmt.Errorf("failed to remove tag %s to port %s: %v", sg, port.ID, res.Err)
+			}
 		}
 	}
 
@@ -2020,7 +2017,7 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName
 	} else {
 		// Attempt to delete the SG if `manage-security-groups` is disabled. When CPO is reconfigured to enable it we
 		// will reconcile the LB and create the SG. This is to make sure it works the same in the opposite direction.
-		if err := lbaas.EnsureSecurityGroupDeleted(clusterName, service); err != nil {
+		if err := lbaas.ensureSecurityGroupDeleted(clusterName, service); err != nil {
 			return status, err
 		}
 	}
@@ -2521,15 +2518,15 @@ func (lbaas *LbaasV2) ensureLoadBalancerDeleted(ctx context.Context, clusterName
 
 	// Delete the Security Group. We're doing that even if `manage-security-groups` is disabled to make sure we don't
 	// orphan created SGs even if CPO got reconfigured.
-	if err := lbaas.EnsureSecurityGroupDeleted(clusterName, service); err != nil {
+	if err := lbaas.ensureSecurityGroupDeleted(clusterName, service); err != nil {
 		return err
 	}
 
 	return nil
 }
 
-// EnsureSecurityGroupDeleted deleting security group for specific loadbalancer service.
-func (lbaas *LbaasV2) EnsureSecurityGroupDeleted(_ string, service *corev1.Service) error {
+// ensureSecurityGroupDeleted deleting security group for specific loadbalancer service.
+func (lbaas *LbaasV2) ensureSecurityGroupDeleted(_ string, service *corev1.Service) error {
 	// Generate Name
 	lbSecGroupName := getSecurityGroupName(service)
 	lbSecGroupID, err := secgroups.IDFromName(lbaas.network, lbSecGroupName)