From f43a424e11da25ba27e0f615a8eee01603f11064 Mon Sep 17 00:00:00 2001 From: Andrew Karpow Date: Wed, 8 Apr 2026 11:36:21 -0400 Subject: [PATCH] EvictionController: Add bounds checks for instance slice operations Extract slice operations into helper functions (peekInstance, popInstance, moveToBack) with proper empty slice handling to prevent potential panics when accessing OutstandingInstances. Add unit tests for the helpers. --- internal/controller/eviction_controller.go | 52 +++++++++--- .../controller/eviction_controller_test.go | 79 +++++++++++++++++++ 2 files changed, 121 insertions(+), 10 deletions(-) diff --git a/internal/controller/eviction_controller.go b/internal/controller/eviction_controller.go index b8fc678..cf06015 100644 --- a/internal/controller/eviction_controller.go +++ b/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, diff --git a/internal/controller/eviction_controller_test.go b/internal/controller/eviction_controller_test.go index d70d855..85606ee 100644 --- a/internal/controller/eviction_controller_test.go +++ b/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"