EvictionController: Add bounds checks for instance slice operations

internal/controller/eviction_controller.go

@@ -53,6 +53,36 @@ const (
 	defaultPollTime        = 10 * time.Second
 )
 
+// popInstance removes and returns the last instance UUID from the slice.
+// Returns the modified slice and the UUID (empty string if slice was empty).
+func popInstance(instances []string) (remaining []string, uuid string) {
+	if len(instances) == 0 {
+		return instances, ""
+	}
+	return instances[:len(instances)-1], instances[len(instances)-1]
+}
+
+// peekInstance returns the last instance UUID without removing it.
+// Returns empty string if the slice is empty.
+func peekInstance(instances []string) string {
+	if len(instances) == 0 {
+		return ""
+	}
+	return instances[len(instances)-1]
+}
+
+// moveToBack moves the last instance to the front of the slice,
+// effectively deprioritizing it. Returns the modified slice.
+func moveToBack(instances []string) []string {
+	if len(instances) < 2 {
+		return instances
+	}
+	uuid := instances[len(instances)-1]
+	copy(instances[1:], instances[:len(instances)-1])
+	instances[0] = uuid
+	return instances
+}
+
 // +kubebuilder:rbac:groups=kvm.cloud.sap,resources=evictions,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=kvm.cloud.sap,resources=evictions/status,verbs=get;update;patch
 // +kubebuilder:rbac:groups=kvm.cloud.sap,resources=evictions/finalizers,verbs=update
@@ -224,9 +254,11 @@ func (r *EvictionReconciler) handleNotFound(ctx context.Context, eviction, base
 	if base == nil {
 		base = eviction.DeepCopy()
 	}
-	instances := &eviction.Status.OutstandingInstances
-	uuid := (*instances)[len(*instances)-1]
-	*instances = (*instances)[:len(*instances)-1]
+	var uuid string
+	eviction.Status.OutstandingInstances, uuid = popInstance(eviction.Status.OutstandingInstances)
+	if uuid == "" {
+		return nil
+	}
 	meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
 		Type:    kvmv1.ConditionTypeMigration,
 		Status:  metav1.ConditionFalse,
@@ -238,8 +270,10 @@ func (r *EvictionReconciler) handleNotFound(ctx context.Context, eviction, base
 
 func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Eviction) (ctrl.Result, error) {
 	base := eviction.DeepCopy()
-	instances := &eviction.Status.OutstandingInstances
-	uuid := (*instances)[len(*instances)-1]
+	uuid := peekInstance(eviction.Status.OutstandingInstances)
+	if uuid == "" {
+		return ctrl.Result{}, nil
+	}
 	log := logger.FromContext(ctx).WithName("Evict").WithValues("server", uuid)
 	logger.IntoContext(ctx, log)
 
@@ -265,8 +299,7 @@ func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Evic
 	case "ERROR":
 		// Needs manual intervention (or another operator fixes it)
 		// put it at the end of the list (beginning of array)
-		copy((*instances)[1:], (*instances)[:len(*instances)-1])
-		(*instances)[0] = uuid
+		eviction.Status.OutstandingInstances = moveToBack(eviction.Status.OutstandingInstances)
 		log.Info("error", "faultMessage", vm.Fault.Message)
 		meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
 			Type:    kvmv1.ConditionTypeMigration,
@@ -303,14 +336,13 @@ func (r *EvictionReconciler) evictNext(ctx context.Context, eviction *kvmv1.Evic
 		}
 
 		// All done
-		*instances = (*instances)[:len(*instances)-1]
+		eviction.Status.OutstandingInstances, _ = popInstance(eviction.Status.OutstandingInstances)
 		return ctrl.Result{}, r.updateStatus(ctx, eviction, base)
 	}
 
 	if vm.TaskState == "deleting" { //nolint:gocritic
 		// We just have to wait for it to be gone. Try the next one.
-		copy((*instances)[1:], (*instances)[:len(*instances)-1])
-		(*instances)[0] = uuid
+		eviction.Status.OutstandingInstances = moveToBack(eviction.Status.OutstandingInstances)
 
 		meta.SetStatusCondition(&eviction.Status.Conditions, metav1.Condition{
 			Type:    kvmv1.ConditionTypeMigration,

internal/controller/eviction_controller_test.go

@@ -35,6 +35,85 @@ import (
 	kvmv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1"
 )
 
+var _ = Describe("Instance slice helpers", func() {
+	Describe("peekInstance", func() {
+		It("returns empty string for empty slice", func() {
+			Expect(peekInstance([]string{})).To(Equal(""))
+		})
+
+		It("returns empty string for nil slice", func() {
+			Expect(peekInstance(nil)).To(Equal(""))
+		})
+
+		It("returns the last element", func() {
+			Expect(peekInstance([]string{"a", "b", "c"})).To(Equal("c"))
+		})
+
+		It("returns the only element for single-element slice", func() {
+			Expect(peekInstance([]string{"only"})).To(Equal("only"))
+		})
+
+		It("does not modify the slice", func() {
+			s := []string{"a", "b", "c"}
+			peekInstance(s)
+			Expect(s).To(Equal([]string{"a", "b", "c"}))
+		})
+	})
+
+	Describe("popInstance", func() {
+		It("returns empty string and unchanged slice for empty slice", func() {
+			s, uuid := popInstance([]string{})
+			Expect(uuid).To(Equal(""))
+			Expect(s).To(BeEmpty())
+		})
+
+		It("returns empty string and unchanged slice for nil slice", func() {
+			s, uuid := popInstance(nil)
+			Expect(uuid).To(Equal(""))
+			Expect(s).To(BeNil())
+		})
+
+		It("removes and returns the last element", func() {
+			s, uuid := popInstance([]string{"a", "b", "c"})
+			Expect(uuid).To(Equal("c"))
+			Expect(s).To(Equal([]string{"a", "b"}))
+		})
+
+		It("returns empty slice when popping last element", func() {
+			s, uuid := popInstance([]string{"only"})
+			Expect(uuid).To(Equal("only"))
+			Expect(s).To(BeEmpty())
+		})
+	})
+
+	Describe("moveToBack", func() {
+		It("returns unchanged slice for empty slice", func() {
+			s := moveToBack([]string{})
+			Expect(s).To(BeEmpty())
+		})
+
+		It("returns unchanged slice for nil slice", func() {
+			s := moveToBack(nil)
+			Expect(s).To(BeNil())
+		})
+
+		It("returns unchanged slice for single-element slice", func() {
+			s := moveToBack([]string{"only"})
+			Expect(s).To(Equal([]string{"only"}))
+		})
+
+		It("moves last element to front for two elements", func() {
+			s := moveToBack([]string{"a", "b"})
+			Expect(s).To(Equal([]string{"b", "a"}))
+		})
+
+		It("moves last element to front for multiple elements", func() {
+			s := moveToBack([]string{"a", "b", "c", "d"})
+			Expect(s).To(Equal([]string{"d", "a", "b", "c"}))
+		})
+	})
+})
+
 var _ = Describe("Eviction Controller", func() {
 	const (
 		resourceName   = "test-resource"