From 92f387cb1394246ada82a483e84a60326e1cf879 Mon Sep 17 00:00:00 2001 From: Michal Dulko Date: Fri, 1 Sep 2023 13:12:31 +0200 Subject: [PATCH] [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 | 2 +- go.sum | 4 ++-- pkg/openstack/loadbalancer.go | 37 ++++++++++++++++------------------- 3 files changed, 20 insertions(+), 23 deletions(-) diff --git a/go.mod b/go.mod index b21e5a62f1..afff06b531 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 3960ae7006..6925fb6cc5 100644 --- a/go.sum +++ b/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= diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go index 2e86eee555..3a24909218 100644 --- a/pkg/openstack/loadbalancer.go +++ b/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)