Fix panic when OpenStack server is deleted by an external agent

The panic happened because we did not correctly handle that
GetInstanceStatus returns a nil server if the server does not exist,
rather than a 404 error.

We had the same oversight in the OpenStackServer controller. It looks
like this would have resulted in recreating the server.

Author: mdbooth

api/v1beta1/conditions_consts.go

@@ -44,6 +44,9 @@ const (
 	OpenStackErrorReason = "OpenStackError"
 	// DependencyFailedReason indicates that a dependent object failed.
 	DependencyFailedReason = "DependencyFailed"
+
+	// ServerUnexpectedDeletedMessage is the message used when the server is unexpectedly deleted via an external agent.
+	ServerUnexpectedDeletedMessage = "The server was unexpectedly deleted"
 )
 
 const (

controllers/openstackmachine_controller.go

@@ -365,6 +365,11 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
 		return ctrl.Result{}, err
 	}
 
+	result := r.reconcileMachineState(scope, openStackMachine, machine, machineServer)
+	if result != nil {
+		return *result, nil
+	}
+
 	computeService, err := compute.NewService(scope)
 	if err != nil {
 		return ctrl.Result{}, err
@@ -377,6 +382,12 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
 		return ctrl.Result{}, err
 	}
 
+	if instanceStatus == nil {
+		msg := "server has been unexpectedly deleted"
+		conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceDeletedReason, clusterv1.ConditionSeverityError, msg)
+		openStackMachine.SetFailure(capoerrors.DeprecatedCAPIUpdateMachineError, errors.New(msg))
+	}
+
 	instanceNS, err := instanceStatus.NetworkStatus()
 	if err != nil {
 		return ctrl.Result{}, fmt.Errorf("get network status: %w", err)
@@ -393,22 +404,15 @@ func (r *OpenStackMachineReconciler) reconcileNormal(ctx context.Context, scope
 	})
 	openStackMachine.Status.Addresses = addresses
 
-	result := r.reconcileMachineState(scope, openStackMachine, machine, machineServer)
-	if result != nil {
-		return *result, nil
-	}
-
-	if !util.IsControlPlaneMachine(machine) {
-		scope.Logger().Info("Not a Control plane machine, no floating ip reconcile needed, Reconciled Machine create successfully")
-		return ctrl.Result{}, nil
-	}
+	if util.IsControlPlaneMachine(machine) {
+		err = r.reconcileAPIServerLoadBalancer(scope, openStackCluster, openStackMachine, instanceStatus, instanceNS, clusterResourceName)
+		if err != nil {
+			return ctrl.Result{}, err
+		}
 
-	err = r.reconcileAPIServerLoadBalancer(scope, openStackCluster, openStackMachine, instanceStatus, instanceNS, clusterResourceName)
-	if err != nil {
-		return ctrl.Result{}, err
+		conditions.MarkTrue(openStackMachine, infrav1.APIServerIngressReadyCondition)
 	}
 
-	conditions.MarkTrue(openStackMachine, infrav1.APIServerIngressReadyCondition)
 	scope.Logger().Info("Reconciled Machine create successfully")
 	return ctrl.Result{}, nil
 }

controllers/openstackserver_controller.go

@@ -433,9 +433,18 @@ func (r *OpenStackServerReconciler) getOrCreateServer(ctx context.Context, logge
 
 	if openStackServer.Status.InstanceID != nil {
 		instanceStatus, err = computeService.GetInstanceStatus(*openStackServer.Status.InstanceID)
-		if err != nil {
-			logger.Info("Unable to get OpenStack instance", "name", openStackServer.Name)
-			conditions.MarkFalse(openStackServer, infrav1.InstanceReadyCondition, infrav1.OpenStackErrorReason, clusterv1.ConditionSeverityError, "%s", err.Error())
+		if err != nil || instanceStatus == nil {
+			logger.Info("Unable to get OpenStack instance", "name", openStackServer.Name, "id", *openStackServer.Status.InstanceID)
+			var msg string
+			var reason string
+			if err != nil {
+				msg = err.Error()
+				reason = infrav1.OpenStackErrorReason
+			} else {
+				msg = infrav1.ServerUnexpectedDeletedMessage
+				reason = infrav1.InstanceNotFoundReason
+			}
+			conditions.MarkFalse(openStackServer, infrav1.InstanceReadyCondition, reason, clusterv1.ConditionSeverityError, msg)
 			return nil, err
 		}
 	}
@@ -450,11 +459,6 @@ func (r *OpenStackServerReconciler) getOrCreateServer(ctx context.Context, logge
 			logger.Info("Server already exists", "name", openStackServer.Name, "id", instanceStatus.ID())
 			return instanceStatus, nil
 		}
-		if openStackServer.Status.InstanceID != nil {
-			logger.Info("Not reconciling server in failed state. The previously existing OpenStack instance is no longer available")
-			conditions.MarkFalse(openStackServer, infrav1.InstanceReadyCondition, infrav1.InstanceNotFoundReason, clusterv1.ConditionSeverityError, "virtual machine no longer exists")
-			return nil, nil
-		}
 
 		logger.Info("Server does not exist, creating Server", "name", openStackServer.Name)
 		instanceSpec, err := r.serverToInstanceSpec(ctx, openStackServer)
@@ -468,6 +472,8 @@ func (r *OpenStackServerReconciler) getOrCreateServer(ctx context.Context, logge
 			openStackServer.Status.InstanceState = &infrav1.InstanceStateError
 			return nil, fmt.Errorf("create OpenStack instance: %w", err)
 		}
+		// We reached a point where a server was created with no error but we can't predict its state yet which is why we don't update conditions yet.
+		// The actual state of the server is checked in the next reconcile loops.
 		return instanceStatus, nil
 	}
 	return instanceStatus, nil

controllers/openstackserver_controller_test.go

@@ -31,8 +31,11 @@ import (
 	"github.com/gophercloud/gophercloud/v2/openstack/networking/v2/ports"
 	. "github.com/onsi/gomega" //nolint:revive
 	"go.uber.org/mock/gomock"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/ptr"
+	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	"sigs.k8s.io/cluster-api/util/conditions"
 
 	infrav1alpha1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha1"
 	infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1"
@@ -548,3 +551,184 @@ func Test_OpenStackServerReconcileCreate(t *testing.T) {
 		})
 	}
 }
+
+func TestOpenStackServerReconciler_getOrCreateServer(t *testing.T) {
+	tests := []struct {
+		name            string
+		openStackServer *infrav1alpha1.OpenStackServer
+		setupMocks      func(r *recorders)
+		wantServer      *servers.Server
+		wantErr         bool
+		wantCondition   *clusterv1.Condition
+	}{
+		{
+			name: "instanceID set in status but server not found",
+			openStackServer: &infrav1alpha1.OpenStackServer{
+				Status: infrav1alpha1.OpenStackServerStatus{
+					InstanceID: ptr.To(instanceUUID),
+				},
+			},
+			setupMocks: func(r *recorders) {
+				r.compute.GetServer(instanceUUID).Return(nil, gophercloud.ErrUnexpectedResponseCode{Actual: 404})
+			},
+			wantErr: false,
+			wantCondition: &clusterv1.Condition{
+				Type:    infrav1.InstanceReadyCondition,
+				Status:  corev1.ConditionFalse,
+				Reason:  infrav1.InstanceNotFoundReason,
+				Message: infrav1.ServerUnexpectedDeletedMessage,
+			},
+		},
+		{
+			name: "instanceID set in status but server not found with error",
+			openStackServer: &infrav1alpha1.OpenStackServer{
+				Status: infrav1alpha1.OpenStackServerStatus{
+					InstanceID: ptr.To(instanceUUID),
+				},
+			},
+			setupMocks: func(r *recorders) {
+				r.compute.GetServer(instanceUUID).Return(nil, fmt.Errorf("error"))
+			},
+			wantErr: true,
+			wantCondition: &clusterv1.Condition{
+				Type:    infrav1.InstanceReadyCondition,
+				Status:  corev1.ConditionFalse,
+				Reason:  infrav1.OpenStackErrorReason,
+				Message: "get server \"" + instanceUUID + "\" detail failed: error",
+			},
+		},
+		{
+			name: "instanceStatus is nil but server found with machine name",
+			openStackServer: &infrav1alpha1.OpenStackServer{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: openStackServerName,
+				},
+				Status: infrav1alpha1.OpenStackServerStatus{},
+			},
+			setupMocks: func(r *recorders) {
+				r.compute.ListServers(servers.ListOpts{
+					Name: "^" + openStackServerName + "$",
+				}).Return([]servers.Server{{ID: instanceUUID}}, nil)
+			},
+			wantErr: false,
+			wantServer: &servers.Server{
+				ID: instanceUUID,
+			},
+		},
+		{
+			name: "instanceStatus is nil and server not found and then created",
+			openStackServer: &infrav1alpha1.OpenStackServer{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: openStackServerName,
+				},
+				Status: infrav1alpha1.OpenStackServerStatus{
+					Resolved: &infrav1alpha1.ResolvedServerSpec{
+						ImageID:  imageUUID,
+						FlavorID: flavorUUID,
+						Ports:    defaultResolvedPorts,
+					},
+				},
+			},
+			setupMocks: func(r *recorders) {
+				r.compute.ListServers(servers.ListOpts{
+					Name: "^" + openStackServerName + "$",
+				}).Return([]servers.Server{}, nil)
+				r.compute.CreateServer(gomock.Any(), gomock.Any()).Return(&servers.Server{ID: instanceUUID}, nil)
+			},
+			wantErr: false,
+			wantServer: &servers.Server{
+				ID: instanceUUID,
+			},
+			// It's off but no condition is set because the server creation was kicked off but we
+			// don't know the result yet in this function.
+		},
+		{
+			name: "instanceStatus is nil and server not found and then created with error",
+			openStackServer: &infrav1alpha1.OpenStackServer{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: openStackServerName,
+				},
+				Status: infrav1alpha1.OpenStackServerStatus{
+					Resolved: &infrav1alpha1.ResolvedServerSpec{
+						ImageID:  imageUUID,
+						FlavorID: flavorUUID,
+						Ports:    defaultResolvedPorts,
+					},
+				},
+			},
+			setupMocks: func(r *recorders) {
+				r.compute.ListServers(servers.ListOpts{
+					Name: "^" + openStackServerName + "$",
+				}).Return([]servers.Server{}, nil)
+				r.compute.CreateServer(gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("error"))
+			},
+			wantErr: true,
+			wantCondition: &clusterv1.Condition{
+				Type:    infrav1.InstanceReadyCondition,
+				Status:  corev1.ConditionFalse,
+				Reason:  infrav1.InstanceCreateFailedReason,
+				Message: "error creating Openstack instance: " + "error",
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewGomegaWithT(t)
+			log := testr.New(t)
+
+			mockCtrl := gomock.NewController(t)
+			defer mockCtrl.Finish()
+
+			mockScopeFactory := scope.NewMockScopeFactory(mockCtrl, "")
+			scopeWithLogger := scope.NewWithLogger(mockScopeFactory, log)
+
+			computeRecorder := mockScopeFactory.ComputeClient.EXPECT()
+			imageRecorder := mockScopeFactory.ImageClient.EXPECT()
+			networkRecorder := mockScopeFactory.NetworkClient.EXPECT()
+			volumeRecorder := mockScopeFactory.VolumeClient.EXPECT()
+
+			recorders := &recorders{
+				compute: computeRecorder,
+				image:   imageRecorder,
+				network: networkRecorder,
+				volume:  volumeRecorder,
+			}
+
+			if tt.setupMocks != nil {
+				tt.setupMocks(recorders)
+			}
+
+			computeService, err := compute.NewService(scopeWithLogger)
+			g.Expect(err).ToNot(HaveOccurred())
+
+			reconciler := OpenStackServerReconciler{}
+			status, err := reconciler.getOrCreateServer(ctx, log, tt.openStackServer, computeService, []string{portUUID})
+
+			// Check error result
+			if tt.wantErr {
+				g.Expect(err).To(HaveOccurred())
+			} else {
+				g.Expect(err).ToNot(HaveOccurred())
+			}
+
+			// Check instance status
+			if tt.wantServer != nil {
+				g.Expect(status.ID()).To(Equal(tt.wantServer.ID))
+			}
+
+			// Check the condition is set correctly
+			if tt.wantCondition != nil {
+				// print openstackServer conditions
+				for _, condition := range tt.openStackServer.Status.Conditions {
+					t.Logf("Condition: %s, Status: %s, Reason: %s", condition.Type, condition.Status, condition.Reason)
+				}
+				conditionType := conditions.Get(tt.openStackServer, tt.wantCondition.Type)
+				g.Expect(conditionType).ToNot(BeNil())
+				g.Expect(conditionType.Status).To(Equal(tt.wantCondition.Status))
+				g.Expect(conditionType.Reason).To(Equal(tt.wantCondition.Reason))
+				g.Expect(conditionType.Message).To(Equal(tt.wantCondition.Message))
+			}
+		})
+	}
+}