fix(openstack): determine ports to delete based on tags

Author: ederst

cloudmock/openstack/mocknetworking/ports.go

@@ -22,6 +22,7 @@ import (
 	"net/http"
 	"net/url"
 	"regexp"
+	"slices"
 	"strings"
 
 	"k8s.io/kops/upup/pkg/fi"
@@ -83,6 +84,22 @@ func (m *MockClient) mockPorts() {
 	m.Mux.HandleFunc("/ports", handler)
 }
 
+func containsAll(list []string, subList []string) bool {
+	for _, item := range subList {
+		if !slices.Contains(list, item) {
+			return false
+		}
+	}
+	return true
+}
+
+func parseTags(tags string) []string {
+	if tags == "" {
+		return []string{}
+	}
+	return strings.Split(tags, ",")
+}
+
 func (m *MockClient) listPorts(w http.ResponseWriter, vals url.Values) {
 	w.WriteHeader(http.StatusOK)
 
@@ -91,6 +108,8 @@ func (m *MockClient) listPorts(w http.ResponseWriter, vals url.Values) {
 	idFilter := vals.Get("id")
 	networkFilter := vals.Get("network_id")
 	deviceFilter := vals.Get("device_id")
+	tags := parseTags(vals.Get("tags"))
+
 	for _, p := range m.ports {
 		if nameFilter != "" && nameFilter != p.Name {
 			continue
@@ -104,6 +123,9 @@ func (m *MockClient) listPorts(w http.ResponseWriter, vals url.Values) {
 		if idFilter != "" && idFilter != p.ID {
 			continue
 		}
+		if len(tags) > 0 && !containsAll(p.Tags, tags) {
+			continue
+		}
 		ports = append(ports, p)
 	}
 

upup/pkg/fi/cloudup/openstack/cloud.go

@@ -617,6 +617,29 @@ func InstanceInClusterAndIG(instance servers.Server, clusterName string, instanc
 	return true
 }
 
+func deletePorts(c OpenstackCloud, instanceGroupName string, clusterName string) error {
+	tags := []string{
+		fmt.Sprintf("%s=%s", TagClusterName, clusterName),
+		fmt.Sprintf("%s=%s", TagKopsInstanceGroup, instanceGroupName),
+	}
+
+	ports, err := c.ListPorts(ports.ListOpts{Tags: strings.Join(tags, ",")})
+	if err != nil {
+		return fmt.Errorf("could not list ports %v", err)
+	}
+
+	for _, port := range ports {
+		klog.V(2).Infof("Delete port '%s' (%s)", port.Name, port.ID)
+		err := c.DeletePort(port.ID)
+
+		if err != nil {
+			return fmt.Errorf("could not delete port %q: %v", port.ID, err)
+		}
+	}
+
+	return nil
+}
+
 func deleteGroup(c OpenstackCloud, g *cloudinstances.CloudInstanceGroup) error {
 	cluster := g.Raw.(*kops.Cluster)
 	allInstances, err := c.ListInstances(servers.ListOpts{
@@ -639,18 +662,10 @@ func deleteGroup(c OpenstackCloud, g *cloudinstances.CloudInstanceGroup) error {
 			return fmt.Errorf("could not delete instance %q: %v", instance.ID, err)
 		}
 	}
-	ports, err := c.ListPorts(ports.ListOpts{})
-	if err != nil {
-		return fmt.Errorf("could not list ports %v", err)
-	}
 
-	for _, port := range ports {
-		if strings.HasPrefix(port.Name, fmt.Sprintf("port-%s", g.InstanceGroup.Name)) && fi.ArrayContains(port.Tags, fmt.Sprintf("%s=%s", TagClusterName, cluster.Name)) {
-			err := c.DeletePort(port.ID)
-			if err != nil {
-				return fmt.Errorf("could not delete port %q: %v", port.ID, err)
-			}
-		}
+	err = deletePorts(c, g.InstanceGroup.Name, cluster.Name)
+	if err != nil {
+		return err
 	}
 
 	sgName := g.InstanceGroup.Name

upup/pkg/fi/cloudup/openstack/cloud_test.go

@@ -22,14 +22,17 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"reflect"
+	"slices"
 	"sort"
 	"testing"
 
 	"github.com/gophercloud/gophercloud/v2"
 	"github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servers"
 	"github.com/gophercloud/gophercloud/v2/openstack/loadbalancer/v2/loadbalancers"
 	l3floatingips "github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/floatingips"
+	"github.com/gophercloud/gophercloud/v2/openstack/networking/v2/ports"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/kops/cloudmock/openstack/mocknetworking"
 	"k8s.io/kops/pkg/apis/kops"
 	"k8s.io/kops/upup/pkg/fi"
 	"k8s.io/kops/util/pkg/vfs"
@@ -662,3 +665,114 @@ func Test_BuildClients(t *testing.T) {
 		})
 	}
 }
+
+func setupMockCloudForDeletePortsTest(portDefinitions map[string]map[string]int) (*MockCloud, error) {
+	cloud := InstallMockOpenstackCloud("mock-central-1")
+	cloud.MockNeutronClient = mocknetworking.CreateClient()
+
+	for clusterName, instanceGroups := range portDefinitions {
+		for instanceGroup, n := range instanceGroups {
+			for i := 0; i < n; i++ {
+				port, err := cloud.CreatePort(ports.CreateOpts{
+					Name:      fmt.Sprintf("port-%s-%d-%s", instanceGroup, i+1, clusterName),
+					NetworkID: "mock-network-id",
+				})
+				if err != nil {
+					return nil, fmt.Errorf("error creating port: %v", err)
+				}
+
+				err = cloud.AppendTag(ResourceTypePort, port.ID, fmt.Sprintf("%s=%s", TagClusterName, clusterName))
+				if err != nil {
+					return nil, fmt.Errorf("error appending tag: %v", err)
+				}
+				err = cloud.AppendTag(ResourceTypePort, port.ID, fmt.Sprintf("%s=%s", TagKopsInstanceGroup, instanceGroup))
+				if err != nil {
+					return nil, fmt.Errorf("error appending tag: %v", err)
+				}
+			}
+		}
+	}
+
+	return cloud, nil
+}
+
+func Test_deletePorts(t *testing.T) {
+	testCases := []struct {
+		description   string
+		clusterName   string
+		instanceGroup string
+		expectedPorts []string
+	}{
+		{
+			description:   "Only delete ports of worker IG of my-cluster",
+			clusterName:   "my-cluster",
+			instanceGroup: "worker",
+			expectedPorts: []string{
+				"port-control-plane-0-1-my-cluster",
+				"port-worker-2-1-my-cluster",
+				"port-worker-2-2-my-cluster",
+				"port-control-plane-0-1-my-cluster-2",
+				"port-worker-1-my-cluster-2",
+				"port-worker-2-my-cluster-2",
+				"port-worker-2-1-my-cluster-2",
+				"port-worker-2-2-my-cluster-2",
+			},
+		},
+		{
+			description:   "Only delete ports of worker-2 IG of my-cluster",
+			clusterName:   "my-cluster",
+			instanceGroup: "worker-2",
+			expectedPorts: []string{
+				"port-control-plane-0-1-my-cluster",
+				"port-worker-1-my-cluster",
+				"port-worker-2-my-cluster",
+				"port-control-plane-0-1-my-cluster-2",
+				"port-worker-1-my-cluster-2",
+				"port-worker-2-my-cluster-2",
+				"port-worker-2-1-my-cluster-2",
+				"port-worker-2-2-my-cluster-2",
+			},
+		},
+	}
+
+	portDefinitions := map[string]map[string]int{
+		"my-cluster": map[string]int{
+			"control-plane-0": 1,
+			"worker":          2,
+			"worker-2":        2,
+		},
+		"my-cluster-2": map[string]int{
+			"control-plane-0": 1,
+			"worker":          2,
+			"worker-2":        2,
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.description, func(t *testing.T) {
+			cloud, err := setupMockCloudForDeletePortsTest(portDefinitions)
+			if err != nil {
+				t.Errorf("error while setting up test: %v", err)
+			}
+
+			deletePorts(cloud, testCase.instanceGroup, testCase.clusterName)
+
+			allPorts, err := cloud.ListPorts(ports.ListOpts{})
+			if err != nil {
+				t.Errorf("error while listing ports: %v", err)
+			}
+
+			actualPorts := []string{}
+			for _, port := range allPorts {
+				actualPorts = append(actualPorts, port.Name)
+			}
+
+			slices.Sort(actualPorts)
+			slices.Sort(testCase.expectedPorts)
+
+			if !reflect.DeepEqual(actualPorts, testCase.expectedPorts) {
+				t.Errorf("ports differ: expected\n%+#v\n\tgot:\n%+#v\n", testCase.expectedPorts, actualPorts)
+			}
+		})
+	}
+}