From 5753890a8fd224b49c5022c4f109b7605f4a56c6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Nussbaumer?=
 <clement.nussbaumer@postfinance.ch>
Date: Mon, 11 Mar 2024 16:44:32 +0100
Subject: [PATCH] fix: current node hash can never be in the map
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Clément Nussbaumer <clement.nussbaumer@postfinance.ch>
---
 internal/kubenurse/server.go             |  7 +++++--
 internal/servicecheck/neighbours.go      | 21 ++++++++++-----------
 internal/servicecheck/neighbours_test.go |  9 +--------
 3 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/internal/kubenurse/server.go b/internal/kubenurse/server.go
index 141f089..e91a5a8 100644
--- a/internal/kubenurse/server.go
+++ b/internal/kubenurse/server.go
@@ -143,8 +143,11 @@ func New(ctx context.Context, c client.Client) (*Server, error) { //nolint:funle
 	chk.NeighbourFilter = os.Getenv("KUBENURSE_NEIGHBOUR_FILTER")
 	neighLimit := os.Getenv("KUBENURSE_NEIGHBOUR_LIMIT")
 
-	if chk.NeighbourLimit, err = strconv.Atoi(neighLimit); err != nil {
-		return nil, err
+	if neighLimit != "" {
+		chk.NeighbourLimit, err = strconv.Atoi(neighLimit)
+		if err != nil {
+			return nil, err
+		}
 	}
 
 	//nolint:goconst // No need to make "false" a constant in my opinion, readability is better like this.
diff --git a/internal/servicecheck/neighbours.go b/internal/servicecheck/neighbours.go
index e2d57f2..78e1f01 100644
--- a/internal/servicecheck/neighbours.go
+++ b/internal/servicecheck/neighbours.go
@@ -13,7 +13,11 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
-var osHostname = os.Hostname //nolint:gochecknoglobals // used during testing
+//nolint:gochecknoglobals // used during testing
+var (
+	osHostname  = os.Hostname
+	currentNode string
+)
 
 // Neighbour represents a kubenurse which should be reachable
 type Neighbour struct {
@@ -60,7 +64,8 @@ func (c *Checker) GetNeighbours(ctx context.Context, namespace, labelSelector st
 			continue
 		}
 
-		if pod.Name == hostname { // only quey other pods, not the currently running pod
+		if pod.Name == hostname { // only query other pods, not the currently running pod
+			currentNode = pod.Spec.NodeName
 			continue
 		}
 
@@ -108,19 +113,13 @@ func (c *Checker) filterNeighbours(nh []*Neighbour) []*Neighbour {
 
 	slices.Sort(l)
 
-	currentHostName, _ := osHostname()
-	hostnameHash := sha256String(currentHostName)
-
-	if m[hostnameHash].NodeName != currentHostName {
-		panic("the current hostname hash doesn't match the value in the map")
-	}
-
-	idx, _ := slices.BinarySearch(l, hostnameHash)
+	currentNodeHash := sha256String(currentNode)
+	idx, _ := slices.BinarySearch(l, currentNodeHash)
 
 	filteredNeighbours := make([]*Neighbour, 0, c.NeighbourLimit)
 
 	for i := 0; i < c.NeighbourLimit; i++ {
-		hash := l[(idx+i+1)%len(l)]
+		hash := l[(idx+i)%len(l)]
 		filteredNeighbours = append(filteredNeighbours, m[hash])
 	}
 
diff --git a/internal/servicecheck/neighbours_test.go b/internal/servicecheck/neighbours_test.go
index cda075d..e2f1192 100644
--- a/internal/servicecheck/neighbours_test.go
+++ b/internal/servicecheck/neighbours_test.go
@@ -30,11 +30,6 @@ func TestNodeFiltering(t *testing.T) {
 	nh := generateNeighbours(n)
 	require.NotNil(t, nh)
 
-	trueOsHostname := osHostname
-	defer func() { osHostname = trueOsHostname }()
-
-	// fake client, with a dummy neighbour pod
-	// fakeClient := fake.NewFakeClient(&fakeNeighbourPod)
 	checker := Checker{
 		NeighbourLimit: neighbourLimit,
 	}
@@ -43,9 +38,7 @@ func TestNodeFiltering(t *testing.T) {
 		counter := make(map[string]int, n)
 
 		for i := range n {
-			osHostname = func() (name string, err error) {
-				return nh[i].NodeName, nil
-			}
+			currentNode = nh[i].NodeName
 			filtered := checker.filterNeighbours(nh)
 			require.Equal(t, neighbourLimit, len(filtered))