Adds CounterAction and ListAction validation

googleforgames · Sep 29, 2023 · 682717b · 682717b
1 parent a035841
commit 682717b
Show file tree

Hide file tree

Showing 3 changed files with 199 additions and 8 deletions.
diff --git a/pkg/allocation/converters/converter.go b/pkg/allocation/converters/converter.go
@@ -507,8 +507,6 @@ func convertAllocationCountersToGSACounterActions(in map[string]*pb.CounterActio
 			ca.Capacity = &capacity
 		}
 
-		// TODO: Action and Amount must be used together. Do we want to check that both Action & Amount are not nil,
-		// or both of Action & Amount are nil? Or add an error in CounterActions in pkg/apis/allocation/v1/gameserverallocation.go?
 		out[k] = ca
 	}
 	return out

diff --git a/pkg/apis/allocation/v1/gameserverallocation.go b/pkg/apis/allocation/v1/gameserverallocation.go
@@ -448,6 +448,41 @@ func validateLists(lists map[string]ListSelector, fldPath *field.Path) field.Err
 	return allErrs
 }
 
+// validateCounterActions validates that the Counters field has valid values for CounterActions
+func validateCounterActions(counters map[string]CounterAction, fldPath *field.Path) field.ErrorList {
+	var allErrs field.ErrorList
+	for key, counterAction := range counters {
+		keyPath := fldPath.Key(key)
+		if counterAction.Amount != nil && *counterAction.Amount < 0 {
+			allErrs = append(allErrs, field.Invalid(keyPath.Child("amount"), counterAction.Amount, apivalidation.IsNegativeErrorMsg))
+		}
+		if counterAction.Capacity != nil && *counterAction.Capacity < 0 {
+			allErrs = append(allErrs, field.Invalid(keyPath.Child("capacity"), counterAction.Capacity, apivalidation.IsNegativeErrorMsg))
+		}
+		if counterAction.Amount != nil && counterAction.Action == nil {
+			allErrs = append(allErrs, field.Invalid(keyPath, counterAction.Action, fmt.Sprint("action must be \"Increment\" or \"Decrement\" if the amount is not nil")))
+		}
+		if counterAction.Amount == nil && counterAction.Action != nil {
+			allErrs = append(allErrs, field.Invalid(keyPath, counterAction.Amount, fmt.Sprint("amount must not be nil if action is not nil")))
+		}
+	}
+
+	return allErrs
+}
+
+// validateListActions validates that the Lists field has valid values for ListActions
+func validateListActions(lists map[string]ListAction, fldPath *field.Path) field.ErrorList {
+	var allErrs field.ErrorList
+	for key, listAction := range lists {
+		keyPath := fldPath.Key(key)
+		if listAction.Capacity != nil && *listAction.Capacity < 0 {
+			allErrs = append(allErrs, field.Invalid(keyPath.Child("capacity"), listAction.Capacity, apivalidation.IsNegativeErrorMsg))
+		}
+	}
+
+	return allErrs
+}
+
 // MultiClusterSetting specifies settings for multi-cluster allocation.
 type MultiClusterSetting struct {
 	Enabled        bool                 `json:"enabled,omitempty"`
@@ -525,8 +560,25 @@ func (gsa *GameServerAllocation) Validate() field.ErrorList {
 		allErrs = append(allErrs, gsa.Spec.Selectors[i].Validate(specPath.Child("selectors").Index(i))...)
 	}
 
-	if !runtime.FeatureEnabled(runtime.FeatureCountsAndLists) && (gsa.Spec.Priorities != nil) {
-		allErrs = append(allErrs, field.Forbidden(specPath.Child("priorities"), "Feature CountsAndLists must be enabled if Priorities is specified"))
+	if !runtime.FeatureEnabled(runtime.FeatureCountsAndLists) {
+		if gsa.Spec.Priorities != nil {
+			allErrs = append(allErrs, field.Forbidden(specPath.Child("priorities"), "Feature CountsAndLists must be enabled if Priorities is specified"))
+		}
+		if gsa.Spec.Counters != nil {
+			allErrs = append(allErrs, field.Forbidden(specPath.Child("counters"), "Feature CountsAndLists must be enabled if Counters is specified"))
+		}
+		if gsa.Spec.Lists != nil {
+			allErrs = append(allErrs, field.Forbidden(specPath.Child("lists"), "Feature CountsAndLists must be enabled if Lists is specified"))
+		}
+	}
+
+	if runtime.FeatureEnabled(runtime.FeatureCountsAndLists) {
+		if gsa.Spec.Counters != nil {
+			allErrs = append(allErrs, validateCounterActions(gsa.Spec.Counters, specPath.Child("counters"))...)
+		}
+		if gsa.Spec.Lists != nil {
+			allErrs = append(allErrs, validateListActions(gsa.Spec.Lists, specPath.Child("lists"))...)
+		}
 	}
 
 	allErrs = append(allErrs, gsa.Spec.MetaPatch.Validate(specPath.Child("metadata"))...)

diff --git a/pkg/apis/allocation/v1/gameserverallocation_test.go b/pkg/apis/allocation/v1/gameserverallocation_test.go
@@ -16,6 +16,7 @@ package v1
 
 import (
 	"fmt"
+	"sort"
 	"testing"
 
 	"agones.dev/agones/pkg/apis"
@@ -1038,6 +1039,135 @@ func TestGameServerListActions(t *testing.T) {
 	}
 }
 
+func TestValidateCounterActions(t *testing.T) {
+	t.Parallel()
+
+	runtime.FeatureTestMutex.Lock()
+	defer runtime.FeatureTestMutex.Unlock()
+	assert.NoError(t, runtime.ParseFeatures(fmt.Sprintf("%s=true", runtime.FeatureCountsAndLists)))
+
+	fieldPath := field.NewPath("spec.Counters")
+	decrement := agonesv1.GameServerPriorityDecrement
+	increment := agonesv1.GameServerPriorityIncrement
+
+	testScenarios := map[string]struct {
+		counterActions map[string]CounterAction
+		wantErr        bool
+	}{
+		"Valid CounterActions": {
+			counterActions: map[string]CounterAction{
+				"foo": {
+					Action: &increment,
+					Amount: int64Pointer(10),
+				},
+				"bar": {
+					Capacity: int64Pointer(100),
+				},
+				"baz": {
+					Action:   &decrement,
+					Amount:   int64Pointer(1000),
+					Capacity: int64Pointer(0),
+				},
+			},
+			wantErr: false,
+		},
+		"Negative Amount": {
+			counterActions: map[string]CounterAction{
+				"foo": {
+					Action: &increment,
+					Amount: int64Pointer(-1),
+				},
+			},
+			wantErr: true,
+		},
+		"Negative Capacity": {
+			counterActions: map[string]CounterAction{
+				"foo": {
+					Capacity: int64Pointer(-20),
+				},
+			},
+			wantErr: true,
+		},
+		"Amount but no Action": {
+			counterActions: map[string]CounterAction{
+				"foo": {
+					Amount: int64Pointer(10),
+				},
+			},
+			wantErr: true,
+		},
+		"Action but no Amount": {
+			counterActions: map[string]CounterAction{
+				"foo": {
+					Action: &decrement,
+				},
+			},
+			wantErr: true,
+		},
+	}
+
+	for test, testScenario := range testScenarios {
+		t.Run(test, func(t *testing.T) {
+			allErrs := validateCounterActions(testScenario.counterActions, fieldPath)
+			if testScenario.wantErr {
+				assert.NotNil(t, allErrs)
+			} else {
+				assert.Nil(t, allErrs)
+			}
+		})
+	}
+}
+
+func TestValidateListActions(t *testing.T) {
+	t.Parallel()
+
+	runtime.FeatureTestMutex.Lock()
+	defer runtime.FeatureTestMutex.Unlock()
+	assert.NoError(t, runtime.ParseFeatures(fmt.Sprintf("%s=true", runtime.FeatureCountsAndLists)))
+
+	fieldPath := field.NewPath("spec.Lists")
+
+	testScenarios := map[string]struct {
+		listActions map[string]ListAction
+		wantErr     bool
+	}{
+		"Valid ListActions": {
+			listActions: map[string]ListAction{
+				"foo": {
+					AddValues: []string{"hello", "world"},
+					Capacity:  int64Pointer(10),
+				},
+				"bar": {
+					Capacity: int64Pointer(0),
+				},
+				"baz": {
+					AddValues: []string{},
+				},
+			},
+			wantErr: false,
+		},
+		"Negative Capacity": {
+			listActions: map[string]ListAction{
+				"foo": {
+					Capacity: int64Pointer(-20),
+				},
+			},
+			wantErr: true,
+		},
+	}
+
+	for test, testScenario := range testScenarios {
+		t.Run(test, func(t *testing.T) {
+			allErrs := validateListActions(testScenario.listActions, fieldPath)
+			if testScenario.wantErr {
+				assert.NotNil(t, allErrs)
+			} else {
+				assert.Nil(t, allErrs)
+			}
+		})
+	}
+}
+
 func TestGameServerAllocationValidate(t *testing.T) {
 	t.Parallel()
 
@@ -1057,7 +1187,9 @@ func TestGameServerAllocationValidate(t *testing.T) {
 
 	runtime.FeatureTestMutex.Lock()
 	defer runtime.FeatureTestMutex.Unlock()
-	assert.NoError(t, runtime.ParseFeatures(fmt.Sprintf("%s=true", runtime.FeaturePlayerAllocationFilter)))
+	assert.NoError(t, runtime.ParseFeatures(fmt.Sprintf("%s=true&%s=false",
+		runtime.FeaturePlayerAllocationFilter,
+		runtime.FeatureCountsAndLists)))
 
 	// invalid player selection
 	gsa = &GameServerAllocation{
@@ -1073,16 +1205,25 @@ func TestGameServerAllocationValidate(t *testing.T) {
 			MetaPatch: MetaPatch{
 				Labels: map[string]string{"$$$": "foo"},
 			},
+			Priorities: []agonesv1.Priority{},
+			Counters:   map[string]CounterAction{},
+			Lists:      map[string]ListAction{},
 		},
 	}
 	gsa.ApplyDefaults()
 
 	allErrs = gsa.Validate()
-	assert.Len(t, allErrs, 4)
+	sort.Slice(allErrs, func(i, j int) bool {
+		return allErrs[i].Field > allErrs[j].Field
+	})
+	assert.Len(t, allErrs, 7)
 	assert.Equal(t, "spec.required.players.minAvailable", allErrs[0].Field)
-	assert.Equal(t, "spec.preferred[0].players.maxAvailable", allErrs[1].Field)
+	assert.Equal(t, "spec.priorities", allErrs[1].Field)
 	assert.Equal(t, "spec.preferred[0].players.minAvailable", allErrs[2].Field)
-	assert.Equal(t, "spec.metadata.labels", allErrs[3].Field)
+	assert.Equal(t, "spec.preferred[0].players.maxAvailable", allErrs[3].Field)
+	assert.Equal(t, "spec.metadata.labels", allErrs[4].Field)
+	assert.Equal(t, "spec.lists", allErrs[5].Field)
+	assert.Equal(t, "spec.counters", allErrs[6].Field)
 }
 
 func TestGameServerAllocationConverter(t *testing.T) {