Fix Port Leaks

When an instance fails to come up, there's a bug where the port doesn't
get cleaned up, and eventually you will exhaust your quota. The code
does attempt to garbage collect, but I suspect at some point in the past
ports were suffixed with an index, but the garbage collection code
wasn't made aware of this, so lists nothing. Replace the APi "filtering"
that doesn't work with a manual filter that does prefix matching. As I'm
a good boy, regression tests have been added to protect the
functionality.

Author: spjmurray

pkg/cloud/services/compute/instance.go

@@ -283,7 +283,7 @@ func (s *Service) createInstanceImpl(eventObject runtime.Object, openStackCluste
 		if len(instanceSpec.Tags) > 0 {
 			iTags = instanceSpec.Tags
 		}
-		portName := getPortName(instanceSpec.Name, portOpts, i)
+		portName := networking.GetPortName(instanceSpec.Name, portOpts, i)
 		port, err := networkingService.GetOrCreatePort(eventObject, clusterName, portName, portOpts, securityGroups, iTags)
 		if err != nil {
 			return nil, err
@@ -380,14 +380,6 @@ func (s *Service) createInstanceImpl(eventObject runtime.Object, openStackCluste
 	return createdInstance, nil
 }
 
-// getPortName appends a suffix to an instance name in order to try and get a unique name per port.
-func getPortName(instanceName string, opts *infrav1.PortOpts, netIndex int) string {
-	if opts != nil && opts.NameSuffix != "" {
-		return fmt.Sprintf("%s-%s", instanceName, opts.NameSuffix)
-	}
-	return fmt.Sprintf("%s-%d", instanceName, netIndex)
-}
-
 func rootVolumeName(instanceName string) string {
 	return fmt.Sprintf("%s-root", instanceName)
 }

pkg/cloud/services/compute/instance_test.go

@@ -48,6 +48,7 @@ import (
 	infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha7"
 	"sigs.k8s.io/cluster-api-provider-openstack/pkg/clients"
 	"sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock"
+	"sigs.k8s.io/cluster-api-provider-openstack/pkg/cloud/services/networking"
 	"sigs.k8s.io/cluster-api-provider-openstack/pkg/scope"
 )
 
@@ -110,7 +111,7 @@ func Test_getPortName(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := getPortName(tt.args.instanceName, tt.args.opts, tt.args.netIndex); got != tt.want {
+			if got := networking.GetPortName(tt.args.instanceName, tt.args.opts, tt.args.netIndex); got != tt.want {
 				t.Errorf("getPortName() = %v, want %v", got, tt.want)
 			}
 		})

pkg/cloud/services/networking/port.go

@@ -290,17 +290,32 @@ func (s *Service) DeletePorts(openStackCluster *infrav1.OpenStackCluster) error
 }
 
 func (s *Service) GarbageCollectErrorInstancesPort(eventObject runtime.Object, instanceName string) error {
-	portList, err := s.client.ListPort(ports.ListOpts{
-		Name: instanceName,
-	})
+	// NOTE: when creating an instance, ports are named using getPortName(), which
+	// features an index, so we must list all ports and delete those that start with
+	// the instance name.  Specifying the name in the list options will only return
+	// a direct match.
+	portList, err := s.client.ListPort(ports.ListOpts{})
 	if err != nil {
 		return err
 	}
+
 	for _, p := range portList {
+		if !strings.HasPrefix(p.Name, instanceName) {
+			continue
+		}
+
 		if err := s.DeletePort(eventObject, p.ID); err != nil {
 			return err
 		}
 	}
 
 	return nil
 }
+
+// GetPortName appends a suffix to an instance name in order to try and get a unique name per port.
+func GetPortName(instanceName string, opts *infrav1.PortOpts, netIndex int) string {
+	if opts != nil && opts.NameSuffix != "" {
+		return fmt.Sprintf("%s-%s", instanceName, opts.NameSuffix)
+	}
+	return fmt.Sprintf("%s-%d", instanceName, netIndex)
+}

pkg/cloud/services/networking/port_test.go

@@ -535,6 +535,68 @@ func Test_GetOrCreatePort(t *testing.T) {
 	}
 }
 
+func Test_GarbageCollectErrorInstancesPort(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	instanceName := "foo"
+	portID1 := "dc6e0ae3-dad6-4240-a9cb-e541916f20d3"
+	portID2 := "a38ab1cb-c2cc-4c1b-9d1d-696ec73356d2"
+	portName1 := GetPortName(instanceName, nil, 0)
+	portName2 := GetPortName(instanceName, nil, 1)
+
+	tests := []struct {
+		name    string
+		expect  func(m *mock.MockNetworkClientMockRecorder)
+		wantErr bool
+	}{
+		{
+			name: "garbage collects all ports for an instance",
+			expect: func(m *mock.MockNetworkClientMockRecorder) {
+				p := []ports.Port{
+					{
+						ID:   "9278e096-5c63-4ffb-9289-2bacf948dc51",
+						Name: "bar-0",
+					},
+					{
+						ID:   portID1,
+						Name: portName1,
+					},
+					{
+						ID:   portID2,
+						Name: portName2,
+					},
+				}
+				m.ListPort(ports.ListOpts{}).Return(p, nil)
+				m.DeletePort(portID1)
+				m.DeletePort(portID2)
+			},
+			wantErr: false,
+		},
+	}
+
+	eventObject := &infrav1.OpenStackMachine{}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+			mockClient := mock.NewMockNetworkClient(mockCtrl)
+			tt.expect(mockClient.EXPECT())
+			s := Service{
+				client: mockClient,
+			}
+			err := s.GarbageCollectErrorInstancesPort(
+				eventObject,
+				instanceName,
+			)
+			if tt.wantErr {
+				g.Expect(err).To(HaveOccurred())
+			} else {
+				g.Expect(err).NotTo(HaveOccurred())
+			}
+		})
+	}
+}
+
 func pointerTo(b bool) *bool {
 	return &b
 }