@@ -764,22 +764,13 @@ func applyNodeSecurityGroupIDForLB(network *gophercloud.ServiceClient, svcConf *
764764 continue
765765 }
766766
767- // Add the security group ID as a tag to the port in order to find all these ports when removing the security group.
768- // We're doing that before actually applying the SG as if tagging would fail we wouldn't be able to find the port
769- // when deleting the SG and operation would be stuck forever. It's better to find more ports than not all of them.
770- mc := metrics .NewMetricContext ("port_tag" , "add" )
771- err := neutrontags .Add (network , "ports" , port .ID , sg ).ExtractErr ()
772- if mc .ObserveRequest (err ) != nil {
773- return fmt .Errorf ("failed to add tag %s to port %s: %v" , sg , port .ID , err )
774- }
775-
776767 // Add the SG to the port
777768 // TODO(dulek): This isn't an atomic operation. In order to protect from lost update issues we should use
778769 // `revision_number` handling to make sure our update to `security_groups` field wasn't preceded
779770 // by a different one. Same applies to a removal of the SG.
780771 newSGs := append (port .SecurityGroups , sg )
781772 updateOpts := neutronports.UpdateOpts {SecurityGroups : & newSGs }
782- mc = metrics .NewMetricContext ("port" , "update" )
773+ mc : =metrics .NewMetricContext ("port" , "update" )
783774 res := neutronports .Update (network , port .ID , updateOpts )
784775 if mc .ObserveRequest (res .Err ) != nil {
785776 return fmt .Errorf ("failed to update security group for port %s: %v" , port .ID , res .Err )
@@ -793,7 +784,7 @@ func applyNodeSecurityGroupIDForLB(network *gophercloud.ServiceClient, svcConf *
793784// disassociateSecurityGroupForLB removes the given security group from the ports
794785func disassociateSecurityGroupForLB (network * gophercloud.ServiceClient , sg string ) error {
795786 // Find all the ports that have the security group associated.
796- listOpts := neutronports.ListOpts {TagsAny : sg }
787+ listOpts := neutronports.ListOpts {SecurityGroups : [] string { sg } }
797788 allPorts , err := openstackutil .GetPorts [neutronports.Port ](network , listOpts )
798789 if err != nil {
799790 return err
@@ -809,17 +800,23 @@ func disassociateSecurityGroupForLB(network *gophercloud.ServiceClient, sg strin
809800
810801 // Update port security groups
811802 newSGs := existingSGs .List ()
803+ // TODO(dulek): This should be done using Neutron's revision_number to make sure
804+ // we don't trigger a lost update issue.
812805 updateOpts := neutronports.UpdateOpts {SecurityGroups : & newSGs }
813806 mc := metrics .NewMetricContext ("port" , "update" )
814807 res := neutronports .Update (network , port .ID , updateOpts )
815808 if mc .ObserveRequest (res .Err ) != nil {
816809 return fmt .Errorf ("failed to update security group for port %s: %v" , port .ID , res .Err )
817810 }
818- // Remove the security group ID tag from the port.
819- mc = metrics .NewMetricContext ("port_tag" , "delete" )
820- err := neutrontags .Delete (network , "ports" , port .ID , sg ).ExtractErr ()
821- if mc .ObserveRequest (err ) != nil {
822- return fmt .Errorf ("failed to remove tag %s to port %s: %v" , sg , port .ID , res .Err )
811+
812+ // Remove the security group ID tag from the port. Please note we don't tag ports with SG IDs anymore,
813+ // so this stays for backward compatibility. It's reasonable to delete it in the future. 404s are ignored.
814+ if slices .Contains (port .Tags , sg ) {
815+ mc = metrics .NewMetricContext ("port_tag" , "delete" )
816+ err := neutrontags .Delete (network , "ports" , port .ID , sg ).ExtractErr ()
817+ if mc .ObserveRequest (err ) != nil {
818+ return fmt .Errorf ("failed to remove tag %s to port %s: %v" , sg , port .ID , res .Err )
819+ }
823820 }
824821 }
825822
@@ -2087,7 +2084,7 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName
20872084 } else {
20882085 // Attempt to delete the SG if `manage-security-groups` is disabled. When CPO is reconfigured to enable it we
20892086 // will reconcile the LB and create the SG. This is to make sure it works the same in the opposite direction.
2090- if err := lbaas .EnsureSecurityGroupDeleted (clusterName , service ); err != nil {
2087+ if err := lbaas .ensureSecurityGroupDeleted (clusterName , service ); err != nil {
20912088 return status , err
20922089 }
20932090 }
@@ -2593,15 +2590,15 @@ func (lbaas *LbaasV2) ensureLoadBalancerDeleted(ctx context.Context, clusterName
25932590
25942591 // Delete the Security Group. We're doing that even if `manage-security-groups` is disabled to make sure we don't
25952592 // orphan created SGs even if CPO got reconfigured.
2596- if err := lbaas .EnsureSecurityGroupDeleted (clusterName , service ); err != nil {
2593+ if err := lbaas .ensureSecurityGroupDeleted (clusterName , service ); err != nil {
25972594 return err
25982595 }
25992596
26002597 return nil
26012598}
26022599
2603- // EnsureSecurityGroupDeleted deleting security group for specific loadbalancer service.
2604- func (lbaas * LbaasV2 ) EnsureSecurityGroupDeleted (_ string , service * corev1.Service ) error {
2600+ // ensureSecurityGroupDeleted deleting security group for specific loadbalancer service.
2601+ func (lbaas * LbaasV2 ) ensureSecurityGroupDeleted (_ string , service * corev1.Service ) error {
26052602 // Generate Name
26062603 lbSecGroupName := getSecurityGroupName (service )
26072604 lbSecGroupID , err := secgroups .IDFromName (lbaas .network , lbSecGroupName )
0 commit comments