Merge pull request #1648 from spjmurray/fix_port_leak

🐛 Fix Port Leaks

controllers/openstackcluster_controller.go

@@ -237,8 +237,9 @@ func deleteBastion(scope scope.Scope, cluster *clusterv1.Cluster, openStackClust
 			}
 		}
 
-		rootVolume := openStackCluster.Spec.Bastion.Instance.RootVolume
-		if err = computeService.DeleteInstance(openStackCluster, instanceStatus, instanceName, rootVolume); err != nil {
+		instanceSpec := bastionToInstanceSpec(openStackCluster, cluster.Name)
+
+		if err = computeService.DeleteInstance(openStackCluster, openStackCluster, instanceStatus, instanceSpec); err != nil {
 			handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to delete bastion: %w", err))
 			return fmt.Errorf("failed to delete bastion: %w", err)
 		}

controllers/openstackmachine_controller.go

@@ -273,7 +273,9 @@ func (r *OpenStackMachineReconciler) reconcileDelete(scope scope.Scope, cluster
 		}
 	}
 
-	if err := computeService.DeleteInstance(openStackMachine, instanceStatus, openStackMachine.Name, openStackMachine.Spec.RootVolume); err != nil {
+	instanceSpec := machineToInstanceSpec(openStackCluster, machine, openStackMachine, "")
+
+	if err := computeService.DeleteInstance(openStackCluster, openStackMachine, instanceStatus, instanceSpec); err != nil {
 		conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceDeleteFailedReason, clusterv1.ConditionSeverityError, "Deleting instance failed: %v", err)
 		return ctrl.Result{}, fmt.Errorf("delete instance: %w", err)
 	}

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)
 }
@@ -550,7 +542,7 @@ func (s *Service) GetManagementPort(openStackCluster *infrav1.OpenStackCluster,
 	return &allPorts[0], nil
 }
 
-func (s *Service) DeleteInstance(eventObject runtime.Object, instanceStatus *InstanceStatus, instanceName string, rootVolume *infrav1.RootVolume) error {
+func (s *Service) DeleteInstance(openStackCluster *infrav1.OpenStackCluster, eventObject runtime.Object, instanceStatus *InstanceStatus, instanceSpec *InstanceSpec) error {
 	if instanceStatus == nil {
 		/*
 			We create a boot-from-volume instance in 2 steps:
@@ -570,8 +562,8 @@ func (s *Service) DeleteInstance(eventObject runtime.Object, instanceStatus *Ins
 			Note that we don't need to separately delete the root volume when deleting the instance because
 			DeleteOnTermination will ensure it is deleted in that case.
 		*/
-		if hasRootVolume(rootVolume) {
-			name := rootVolumeName(instanceName)
+		if hasRootVolume(instanceSpec.RootVolume) {
+			name := rootVolumeName(instanceSpec.Name)
 			volume, err := s.getVolumeByName(name)
 			if err != nil {
 				return err
@@ -620,7 +612,12 @@ func (s *Service) DeleteInstance(eventObject runtime.Object, instanceStatus *Ins
 
 	// delete port of error instance
 	if instanceStatus.State() == infrav1.InstanceStateError {
-		if err := networkingService.GarbageCollectErrorInstancesPort(eventObject, instanceStatus.Name()); err != nil {
+		portOpts, err := s.constructPorts(openStackCluster, instanceSpec)
+		if err != nil {
+			return err
+		}
+
+		if err := networkingService.GarbageCollectErrorInstancesPort(eventObject, instanceSpec.Name, portOpts); err != nil {
 			return err
 		}
 	}

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)
 			}
 		})
@@ -819,7 +820,13 @@ func TestService_DeleteInstance(t *testing.T) {
 			if err != nil {
 				t.Fatalf("Failed to create service: %v", err)
 			}
-			if err := s.DeleteInstance(tt.eventObject, tt.instanceStatus(), openStackMachineName, tt.rootVolume); (err != nil) != tt.wantErr {
+
+			instanceSpec := &InstanceSpec{
+				Name:       openStackMachineName,
+				RootVolume: tt.rootVolume,
+			}
+
+			if err := s.DeleteInstance(&infrav1.OpenStackCluster{}, tt.eventObject, tt.instanceStatus(), instanceSpec); (err != nil) != tt.wantErr {
 				t.Errorf("Service.DeleteInstance() error = %v, wantErr %v", err, tt.wantErr)
 			}
 		})

pkg/cloud/services/networking/port.go

@@ -289,18 +289,38 @@ func (s *Service) DeletePorts(openStackCluster *infrav1.OpenStackCluster) error
 	return nil
 }
 
-func (s *Service) GarbageCollectErrorInstancesPort(eventObject runtime.Object, instanceName string) error {
-	portList, err := s.client.ListPort(ports.ListOpts{
-		Name: instanceName,
-	})
-	if err != nil {
-		return err
-	}
-	for _, p := range portList {
-		if err := s.DeletePort(eventObject, p.ID); err != nil {
+func (s *Service) GarbageCollectErrorInstancesPort(eventObject runtime.Object, instanceName string, portOpts []infrav1.PortOpts) error {
+	for i := range portOpts {
+		portOpt := &portOpts[i]
+
+		portName := GetPortName(instanceName, portOpt, i)
+
+		// TODO: whould be nice if gophercloud could be persuaded to accept multiple
+		// names as is allowed by the API in order to reduce API traffic.
+		portList, err := s.client.ListPort(ports.ListOpts{Name: portName})
+		if err != nil {
+			return err
+		}
+
+		// NOTE: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/issues/1476
+		// It is up to the end user to specify a UNIQUE cluster name when provisioning in the
+		// same project, otherwise things will alias and we could delete more than we should.
+		if len(portList) > 1 {
+			return fmt.Errorf("garbage collection of port %s failed, found %d ports with the same name", portName, len(portList))
+		}
+
+		if err := s.DeletePort(eventObject, portList[0].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,85 @@ 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 {
+		// man is the name of the test.
+		name string
+		// expect allows definition of any expected calls to the mock.
+		expect func(m *mock.MockNetworkClientMockRecorder)
+		// portOpts defines the instance ports as defined in the OSM spec.
+		portOpts []infrav1.PortOpts
+		// wantErr defines whether the test is supposed to fail.
+		wantErr bool
+	}{
+		{
+			name: "garbage collects all ports for an instance",
+			expect: func(m *mock.MockNetworkClientMockRecorder) {
+				o1 := ports.ListOpts{
+					Name: portName1,
+				}
+				p1 := []ports.Port{
+					{
+						ID:   portID1,
+						Name: portName1,
+					},
+				}
+				m.ListPort(o1).Return(p1, nil)
+				m.DeletePort(portID1)
+				o2 := ports.ListOpts{
+					Name: portName2,
+				}
+				p2 := []ports.Port{
+					{
+						ID:   portID2,
+						Name: portName2,
+					},
+				}
+
+				m.ListPort(o2).Return(p2, nil)
+				m.DeletePort(portID2)
+			},
+			portOpts: []infrav1.PortOpts{
+				{},
+				{},
+			},
+			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,
+				tt.portOpts,
+			)
+			if tt.wantErr {
+				g.Expect(err).To(HaveOccurred())
+			} else {
+				g.Expect(err).NotTo(HaveOccurred())
+			}
+		})
+	}
+}
+
 func pointerTo(b bool) *bool {
 	return &b
 }