[karthik120710]

Description

Summary

• RemoveNodePool in utility/kubernetes.go panicked when called with an empty slice due to an unchecked s[len(s)-1] index access
• Added an early return guard clause when len(s) == 0 to prevent the runtime panic

Changes

• utility/kubernetes.go: Add empty slice guard at the top of RemoveNodePool

Closes #606

[karthik120710]

hi @giornetta i have found these minor gap fix the panic state when slice is empty and fix these issue .

[fulviodenza]

Thanks for spotting this, @karthik120710! The guard does stop the panic on an empty slice. Looking at RemoveNodePool more closely the same root cause leads to a more serious bug, and I think we should fix both in this PR.

key starts at 0, and the loop only changes it when an ID matches. If no pool matches, the swap and truncate still run, so the function removes s[0]. The caller (cmd/kubernetes/kubernetes_nodepool_delete.go:73) then sends that shortened list to UpdateKubernetesCluster,

which means that a command like this:
civo k8s node-pool delete mycluster typo99
leads to silently deleteing the cluster's first node pool.

And that passing the same pool ID twice removes the matching pool, then a second, unrelated one.

I suggest you to add these testcases into utility/kubernetes_test.go and fix the issue based on these (you will see that at the current state, just one of these tests passes):

func TestRemoveNodePool(t *testing.T) {
	pools := func(ids ...string) []civogo.KubernetesClusterPoolConfig {
		s := []civogo.KubernetesClusterPoolConfig{}
		for _, id := range ids {
			s = append(s, civogo.KubernetesClusterPoolConfig{ID: id})
		}
		return s
	}

	tests := []struct {
		name       string
		pools      []civogo.KubernetesClusterPoolConfig
		remove     []string
		wantPools  []string
		wantRemove []string
	}{
		{
			name:       "removes the first pool",
			pools:      pools("aaaaaa-1", "bbbbbb-2", "cccccc-3"),
			remove:     []string{"aaaaaa"},
			wantPools:  []string{"bbbbbb-2", "cccccc-3"},
			wantRemove: []string{"aaaaaa-1"},
		},
		{
			name:       "removes a middle pool",
			pools:      pools("aaaaaa-1", "bbbbbb-2", "cccccc-3"),
			remove:     []string{"bbbbbb"},
			wantPools:  []string{"aaaaaa-1", "cccccc-3"},
			wantRemove: []string{"bbbbbb-2"},
		},
		{
			name:       "removes the last pool",
			pools:      pools("aaaaaa-1", "bbbbbb-2", "cccccc-3"),
			remove:     []string{"cccccc"},
			wantPools:  []string{"aaaaaa-1", "bbbbbb-2"},
			wantRemove: []string{"cccccc-3"},
		},
		{
			name:       "empty slice does not panic",
			pools:      pools(),
			remove:     []string{"aaaaaa"},
			wantPools:  []string{},
			wantRemove: []string{},
		},
		{
			name:       "unknown ID leaves pools unchanged",
			pools:      pools("aaaaaa-1", "bbbbbb-2"),
			remove:     []string{"zzzzzz"},
			wantPools:  []string{"aaaaaa-1", "bbbbbb-2"},
			wantRemove: []string{},
		},
		{
			name:       "same ID twice removes only the matching pool",
			pools:      pools("aaaaaa-1", "bbbbbb-2", "cccccc-3"),
			remove:     []string{"bbbbbb", "bbbbbb"},
			wantPools:  []string{"aaaaaa-1", "cccccc-3"},
			wantRemove: []string{"bbbbbb-2"},
		},
		{
			name:       "ID fragment in the middle of another ID does not match",
			pools:      pools("aaaaaa-bbbbbb", "cccccc-1"),
			remove:     []string{"bbbbbb"},
			wantPools:  []string{"aaaaaa-bbbbbb", "cccccc-1"},
			wantRemove: []string{},
		},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			defer func() {
				if r := recover(); r != nil {
					t.Errorf("RemoveNodePool panicked: %v", r)
				}
			}()

			got := tt.pools
			names := []string{}
			for _, id := range tt.remove {
				got, names = RemoveNodePool(got, id, names)
			}

			gotIDs := []string{}
			for _, p := range got {
				gotIDs = append(gotIDs, p.ID)
			}
			slices.Sort(gotIDs)

			if !slices.Equal(gotIDs, tt.wantPools) {
				t.Errorf("remaining pools = %v, want %v", gotIDs, tt.wantPools)
			}
			if !slices.Equal(names, tt.wantRemove) {
				t.Errorf("removed names = %v, want %v", names, tt.wantRemove)
			}
		})
	}
}

to help you working on the fix.

[karthik120710]

hi @fulviodenza i have implemented ur suggetions , sorry for the delay .

[fulviodenza]

Thanks @karthik120710, this looks good! RemoveNodePool now leaves the pools untouched when nothing matches, a repeated ID no longer removes an unrelated pool, and all the test cases pass (CI is green too).

A few things before we merge:

1. A wrong ID still reports success

RemoveNodePool is fixed, but the caller in cmd/kubernetes/kubernetes_nodepool_delete.go never checks whether anything was actually removed. So civo k8s node-pool delete mycluster typo99 now calls UpdateKubernetesCluster with the unchanged pools, prints The node pool () has been deleted from the cluster (mycluster) and exits 0. With -o json it even lists typo99 as a deleted ID.

Could you fail before calling the API when an ID doesn't match? Something like:

kubernetesPoolNameList = nil
notFound := []string{}
for _, kubeList := range kubernetesNodePoolList {
	before := len(kubernetesPoolNameList)
	nodePool, kubernetesPoolNameList = utility.RemoveNodePool(nodePool, kubeList.Name, kubernetesPoolNameList)
	if len(kubernetesPoolNameList) == before {
		notFound = append(notFound, kubeList.Name)
	}
}

if len(notFound) > 0 {
	utility.Error("No node pool found matching %s in cluster %s", strings.Join(notFound, ", "), kubernetesFindCluster.Name)
	os.Exit(1)
}

It would also be nice if the JSON output loop printed the matched full IDs from kubernetesPoolNameList instead of the strings the user typed.

2. PR title and description

They only describe the empty-slice guard. Could you update them to cover the wrong-pool deletion fix and the switch from strings.Contains to strings.HasPrefix? The second one is a behaviour change: a fragment from the middle of an ID used to match and is now rejected.

3. Commit message

The commit message for 26c496a says "add documentation and issues files", but that commit only touches utility/kubernetes.go and the test. We can fix this when squashing on merge, something like fix(kubernetes): RemoveNodePool removed the wrong pool when no ID matched.

Nit (optional)

The len(s) == 0 guard is now redundant: with an empty slice the loop never runs, key stays -1 and the key == -1 check already returns early. Fine to drop it.

[fulviodenza]

Whenever you fix these, we are ready to merge

[karthik120710]

@fulviodenza ready to merge.

[fulviodenza]

LGTM!