Skip to content

Commit

Permalink
PWX-35601 Add length check before accessing in round robin (#2399)
Browse files Browse the repository at this point in the history
* Add length check before accessing in round robin

This PR adds a few check before accessing the filteredNodes:
1. If the length is zero, we return empty string and false (isRemote)
2. Before accessing the array, we take a reminder of the currentIndex with the length of the array.

JIRA: PWX-35601

Signed-off-by: dahuang <[email protected]>
  • Loading branch information
dahuang-purestorage authored Jan 11, 2024
1 parent 844d27c commit 7d0e1f9
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 2 deletions.
8 changes: 6 additions & 2 deletions pkg/loadbalancer/roundrobin.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,9 @@ func (rr *roundRobin) GetRemoteNode() (string, bool, error) {
// Get target node info and set next round robbin node.
// nextNode is always lastNode + 1 mod (numOfNodes), to loop back to zero
targetNodeEndpoint, isRemoteConn := rr.getTargetAndIncrement(filteredNodes, selfNode.Id)
if targetNodeEndpoint == "" {
return "", false, errors.New("target node not found")
}

return targetNodeEndpoint, isRemoteConn, nil
}
Expand Down Expand Up @@ -142,9 +145,10 @@ func (rr *roundRobin) getTargetAndIncrement(filteredNodes []*api.Node, selfNodeI
targetNodeNumber int
isRemoteConn bool
)
if rr.nextCreateNodeNumber != 0 {
targetNodeNumber = rr.nextCreateNodeNumber
if len(filteredNodes) == 0 {
return "", false
}
targetNodeNumber = rr.nextCreateNodeNumber % len(filteredNodes)
targetNode := filteredNodes[targetNodeNumber]
if targetNode.Id != selfNodeID {
// NodeID set on the cluster object is this node's ID.
Expand Down
26 changes: 26 additions & 0 deletions pkg/loadbalancer/roundrobin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,3 +93,29 @@ func TestGetRemoteNodeWithDomains(t *testing.T) {
}
}
}

// TestGetTargetAndIncrementIndexOutOfBound tests the case when getTargetAndIncrement
// causes index out of bound error. This happens when the round robin index is not
// checked against node array length before accessing it.
func TestGetTargetAndIncrementIndexOutOfBound(t *testing.T) {
filteredNodes := []*api.Node{
{
Id: "1",
MgmtIp: "1",
},
{
Id: "2",
MgmtIp: "2",
},
}
rr := &roundRobin{nextCreateNodeNumber: len(filteredNodes)}
endpoint, isRemote := rr.getTargetAndIncrement(filteredNodes, "")
require.True(t, isRemote, "isRemote is not as expected")
require.Equal(t, "1", endpoint, "target endpoint is not as expected")

filteredNodes = []*api.Node{}
rr.nextCreateNodeNumber = 0
endpoint, isRemote = rr.getTargetAndIncrement(filteredNodes, "")
require.False(t, isRemote, "isRemote is not as expected")
require.Equal(t, "", endpoint, "target endpoint is not as expected")
}

0 comments on commit 7d0e1f9

Please sign in to comment.