From 682717bfc2e1faf79bb640ca044b14169e0f849a Mon Sep 17 00:00:00 2001 From: Ivy Gooch Date: Fri, 29 Sep 2023 22:21:57 +0000 Subject: [PATCH] Adds CounterAction and ListAction validation --- pkg/allocation/converters/converter.go | 2 - .../allocation/v1/gameserverallocation.go | 56 ++++++- .../v1/gameserverallocation_test.go | 149 +++++++++++++++++- 3 files changed, 199 insertions(+), 8 deletions(-) diff --git a/pkg/allocation/converters/converter.go b/pkg/allocation/converters/converter.go index cfc2e05a74..6a8fb73f03 100644 --- 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 index 3982f20c8c..4a9acc5883 100644 --- 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 index 4814aea089..7fc3cc347f 100644 --- 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) {