From 634465788211f3c3dab4ef0726af5b2a0e3c370b Mon Sep 17 00:00:00 2001 From: ederst Date: Tue, 26 Nov 2024 18:15:20 +0100 Subject: [PATCH] fix(openstack): determine ports to delete based on tags --- cloudmock/openstack/mocknetworking/ports.go | 22 ++++ upup/pkg/fi/cloudup/openstack/cloud.go | 37 +++++-- upup/pkg/fi/cloudup/openstack/cloud_test.go | 114 ++++++++++++++++++++ 3 files changed, 162 insertions(+), 11 deletions(-) diff --git a/cloudmock/openstack/mocknetworking/ports.go b/cloudmock/openstack/mocknetworking/ports.go index a72f083ab91d2..11d6130c7fa03 100644 --- a/cloudmock/openstack/mocknetworking/ports.go +++ b/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) } diff --git a/upup/pkg/fi/cloudup/openstack/cloud.go b/upup/pkg/fi/cloudup/openstack/cloud.go index e4e274e8a20b2..b138886cd0d59 100644 --- a/upup/pkg/fi/cloudup/openstack/cloud.go +++ b/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 diff --git a/upup/pkg/fi/cloudup/openstack/cloud_test.go b/upup/pkg/fi/cloudup/openstack/cloud_test.go index 4aa916dd6379e..396d6f2b25b18 100644 --- a/upup/pkg/fi/cloudup/openstack/cloud_test.go +++ b/upup/pkg/fi/cloudup/openstack/cloud_test.go @@ -22,6 +22,7 @@ import ( "net/http" "net/http/httptest" "reflect" + "slices" "sort" "testing" @@ -29,7 +30,9 @@ import ( "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": { + "control-plane-0": 1, + "worker": 2, + "worker-2": 2, + }, + "my-cluster-2": { + "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) + } + }) + } +}