From a99a9fe22b3707c7b0be9dc909afe6f3029d8db1 Mon Sep 17 00:00:00 2001 From: aimuz Date: Thu, 13 Jul 2023 11:28:06 +0800 Subject: [PATCH] refactor(allocation): Reimplement the Validate method using "field.ErrorList" Signed-off-by: aimuz --- examples/crd-client/main.go | 2 +- .../allocation/v1/gameserverallocation.go | 229 +++++------------- .../v1/gameserverallocation_test.go | 223 ++++++----------- pkg/gameserverallocations/allocator.go | 22 +- pkg/gameserverallocations/allocator_test.go | 20 +- pkg/gameserverallocations/find_test.go | 16 +- test/e2e/gameserverallocation_test.go | 2 +- 7 files changed, 161 insertions(+), 353 deletions(-) diff --git a/examples/crd-client/main.go b/examples/crd-client/main.go index 94a82cab7f..077b02e2ab 100644 --- a/examples/crd-client/main.go +++ b/examples/crd-client/main.go @@ -112,7 +112,7 @@ func main() { ctx := context.Background() newGS, err := agonesClient.AgonesV1().GameServers(gs.Namespace).Create(ctx, gs, metav1.CreateOptions{}) if err != nil { - logrus.Fatal("Unable to create GameServer: %v", err) + logrus.Fatalf("Unable to create GameServer: %v", err) } logrus.Infof("New GameServer name is: %s", newGS.ObjectMeta.Name) diff --git a/pkg/apis/allocation/v1/gameserverallocation.go b/pkg/apis/allocation/v1/gameserverallocation.go index 348ee2f209..43aebe30e5 100644 --- a/pkg/apis/allocation/v1/gameserverallocation.go +++ b/pkg/apis/allocation/v1/gameserverallocation.go @@ -21,11 +21,12 @@ import ( "agones.dev/agones/pkg/apis" agonesv1 "agones.dev/agones/pkg/apis/agones/v1" "agones.dev/agones/pkg/util/runtime" + apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation" apivalidation "k8s.io/apimachinery/pkg/api/validation" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation" "k8s.io/apimachinery/pkg/labels" - validationfield "k8s.io/apimachinery/pkg/util/validation/field" + field "k8s.io/apimachinery/pkg/util/validation/field" ) const ( @@ -369,155 +370,98 @@ func (s *GameServerSelector) matchLists(gs *agonesv1.GameServer) bool { } // Validate validates that the selection fields have valid values -func (s *GameServerSelector) Validate(field string) ([]metav1.StatusCause, bool) { - var causes []metav1.StatusCause +func (s *GameServerSelector) Validate(fldPath *field.Path) field.ErrorList { + var allErrs field.ErrorList _, err := metav1.LabelSelectorAsSelector(&s.LabelSelector) if err != nil { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Message: fmt.Sprintf("Error converting label selector: %s", err), - Field: field, - }) + allErrs = append(allErrs, field.Invalid(fldPath.Child("labelSelector"), s.LabelSelector, fmt.Sprintf("Error converting label selector: %s", err))) } if runtime.FeatureEnabled(runtime.FeatureStateAllocationFilter) { if s.GameServerState != nil && !(*s.GameServerState == agonesv1.GameServerStateAllocated || *s.GameServerState == agonesv1.GameServerStateReady) { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Message: "GameServerState value can only be Allocated or Ready", - Field: field, - }) + allErrs = append(allErrs, field.Invalid(fldPath.Child("gameServerState"), *s.GameServerState, "GameServerState must be either Allocated or Ready")) } } if runtime.FeatureEnabled(runtime.FeaturePlayerAllocationFilter) && s.Players != nil { if s.Players.MinAvailable < 0 { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Message: "Players.MinAvailable must be greater than zero", - Field: field, - }) + allErrs = append(allErrs, field.Invalid(fldPath.Child("players").Child("minAvailable"), s.Players.MinAvailable, apimachineryvalidation.IsNegativeErrorMsg)) } if s.Players.MaxAvailable < 0 { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Message: "Players.MaxAvailable must be greater than zero", - Field: field, - }) + allErrs = append(allErrs, field.Invalid(fldPath.Child("players").Child("maxAvailable"), s.Players.MaxAvailable, apimachineryvalidation.IsNegativeErrorMsg)) } if s.Players.MinAvailable > s.Players.MaxAvailable { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Message: "Players.MinAvailable must be less than Players.MaxAvailable", - Field: field, - }) + allErrs = append(allErrs, field.Invalid(fldPath.Child("players").Child("minAvailable"), s.Players.MinAvailable, "minAvailable cannot be greater than maxAvailable")) } } - if !runtime.FeatureEnabled(runtime.FeatureCountsAndLists) && (s.Counters != nil || s.Lists != nil) { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Message: "Feature CountsAndLists must be enabled if Counters or Lists are specified", - Field: field, - }) - } - if runtime.FeatureEnabled(runtime.FeatureCountsAndLists) { if s.Counters != nil { - causes = append(causes, s.validateCounters(field)...) + allErrs = append(allErrs, validateCounters(s.Counters, fldPath.Child("counters"))...) + } + if s.Lists != nil { + allErrs = append(allErrs, validateLists(s.Lists, fldPath.Child("lists"))...) + } + } else { + if s.Counters != nil { + allErrs = append(allErrs, field.Forbidden(fldPath.Child("counters"), "Feature CountsAndLists must be enabled")) } if s.Lists != nil { - causes = append(causes, s.validateLists(field)...) + allErrs = append(allErrs, field.Forbidden(fldPath.Child("lists"), "Feature CountsAndLists must be enabled")) } } - return causes, len(causes) == 0 + return allErrs } // validateCounters validates that the selection field has valid values for CounterSelectors -func (s *GameServerSelector) validateCounters(field string) []metav1.StatusCause { - var causes []metav1.StatusCause - - for _, counterSelector := range s.Counters { +func validateCounters(counters map[string]CounterSelector, fldPath *field.Path) field.ErrorList { + var allErrs field.ErrorList + for key, counterSelector := range counters { + keyPath := fldPath.Key(key) if counterSelector.MinCount < 0 { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Message: "CounterSelector.MinCount must be greater than zero", - Field: field, - }) + allErrs = append(allErrs, field.Invalid(keyPath.Child("minCount"), counterSelector.MinCount, apimachineryvalidation.IsNegativeErrorMsg)) } if counterSelector.MaxCount < 0 { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Message: "CounterSelector.MaxCount must be greater than zero", - Field: field, - }) + allErrs = append(allErrs, field.Invalid(keyPath.Child("maxCount"), counterSelector.MaxCount, apimachineryvalidation.IsNegativeErrorMsg)) } if (counterSelector.MaxCount < counterSelector.MinCount) && (counterSelector.MaxCount != 0) { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Message: "CounterSelector.MaxCount must zero or greater than counterSelector.MinCount", - Field: field, - }) + allErrs = append(allErrs, field.Invalid(keyPath, counterSelector.MaxCount, fmt.Sprintf("maxCount must zero or greater than minCount %d", counterSelector.MinCount))) } if counterSelector.MinAvailable < 0 { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Message: "CounterSelector.MinAvailable must be greater than zero", - Field: field, - }) + allErrs = append(allErrs, field.Invalid(keyPath.Child("minAvailable"), counterSelector.MinAvailable, apimachineryvalidation.IsNegativeErrorMsg)) } if counterSelector.MaxAvailable < 0 { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Message: "CounterSelector.MaxAvailable must be greater than zero", - Field: field, - }) + allErrs = append(allErrs, field.Invalid(keyPath.Child("maxAvailable"), counterSelector.MaxAvailable, apimachineryvalidation.IsNegativeErrorMsg)) } if (counterSelector.MaxAvailable < counterSelector.MinAvailable) && (counterSelector.MaxAvailable != 0) { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Message: "CounterSelector.MaxAvailable must zero or greater than counterSelector.MinAvailable", - Field: field, - }) + allErrs = append(allErrs, field.Invalid(keyPath, counterSelector.MaxAvailable, fmt.Sprintf("maxAvailable must zero or greater than minAvailable %d", counterSelector.MinAvailable))) } } - return causes + return allErrs } // validateLists validates that the selection field has valid values for ListSelectors -func (s *GameServerSelector) validateLists(field string) []metav1.StatusCause { - var causes []metav1.StatusCause - - for _, listSelector := range s.Lists { +func validateLists(lists map[string]ListSelector, fldPath *field.Path) field.ErrorList { + var allErrs field.ErrorList + for key, listSelector := range lists { + keyPath := fldPath.Key(key) if listSelector.MinAvailable < 0 { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Message: "ListSelector.MinAvailable must be greater than zero", - Field: field, - }) + allErrs = append(allErrs, field.Invalid(keyPath.Child("minAvailable"), listSelector.MinAvailable, apimachineryvalidation.IsNegativeErrorMsg)) } if listSelector.MaxAvailable < 0 { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Message: "ListSelector.MaxAvailable must be greater than zero", - Field: field, - }) + allErrs = append(allErrs, field.Invalid(keyPath.Child("maxAvailable"), listSelector.MaxAvailable, apimachineryvalidation.IsNegativeErrorMsg)) } if (listSelector.MaxAvailable < listSelector.MinAvailable) && (listSelector.MaxAvailable != 0) { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Message: "ListSelector.MaxAvailable must zero or greater than ListSelector.MinAvailable", - Field: field, - }) + allErrs = append(allErrs, field.Invalid(keyPath, listSelector.MaxAvailable, fmt.Sprintf("maxAvailable must zero or greater than minAvailable %d", listSelector.MinAvailable))) } } - return causes + return allErrs } // MultiClusterSetting specifies settings for multi-cluster allocation. @@ -534,32 +478,10 @@ type MetaPatch struct { // Validate returns if the labels and/or annotations that are to be applied to a `GameServer` post // allocation are valid. -func (mp *MetaPatch) Validate() ([]metav1.StatusCause, bool) { - var causes []metav1.StatusCause - - errs := metav1validation.ValidateLabels(mp.Labels, validationfield.NewPath("labels")) - if len(errs) != 0 { - for _, v := range errs { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Field: "metadata.labels", - Message: v.Error(), - }) - } - } - - errs = apivalidation.ValidateAnnotations(mp.Annotations, validationfield.NewPath("annotations")) - if len(errs) != 0 { - for _, v := range errs { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Field: "metadata.annotations", - Message: v.Error(), - }) - } - } - - return causes, len(causes) == 0 +func (mp *MetaPatch) Validate(fldPath *field.Path) field.ErrorList { + allErrs := metav1validation.ValidateLabels(mp.Labels, fldPath.Child("labels")) + allErrs = append(allErrs, apivalidation.ValidateAnnotations(mp.Annotations, fldPath.Child("annotations"))...) + return allErrs } // Priority is a sorting option for GameServers with Counters or Lists based on the count or @@ -575,26 +497,17 @@ type Priority struct { } // Validate returns if the Priority is valid. -func (p *Priority) validate(field string) ([]metav1.StatusCause, bool) { - var causes []metav1.StatusCause - +func (p *Priority) validate(fldPath *field.Path) field.ErrorList { + var allErrs field.ErrorList if !(p.PriorityType == GameServerAllocationPriorityCounter || p.PriorityType == GameServerAllocationPriorityList) { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Message: "Priority.Sort must be either `Counter` or `List`", - Field: field, - }) + allErrs = append(allErrs, field.NotSupported(fldPath.Child("priorityType"), p.PriorityType, []string{GameServerAllocationPriorityCounter, GameServerAllocationPriorityList})) } if !(p.Order == GameServerAllocationAscending || p.Order == GameServerAllocationDescending || p.Order == "") { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Message: "Priority.Order must be either `Ascending` or `Descending`", - Field: field, - }) + allErrs = append(allErrs, field.NotSupported(fldPath.Child("order"), p.Order, []string{GameServerAllocationAscending, GameServerAllocationDescending})) } - return causes, len(causes) == 0 + return allErrs } // GameServerAllocationStatus is the status for an GameServerAllocation resource @@ -638,56 +551,34 @@ func (gsa *GameServerAllocation) ApplyDefaults() { // Validate validation for the GameServerAllocation // Validate should be called before attempting to Match any of the GameServer selectors. -func (gsa *GameServerAllocation) Validate() ([]metav1.StatusCause, bool) { - var causes []metav1.StatusCause - - valid := false - for _, v := range []apis.SchedulingStrategy{apis.Packed, apis.Distributed} { - if gsa.Spec.Scheduling == v { - valid = true - } - } - if !valid { - causes = append(causes, metav1.StatusCause{Type: metav1.CauseTypeFieldValueInvalid, - Field: "spec.scheduling", - Message: fmt.Sprintf("Invalid value: %s, value must be either Packed or Distributed", gsa.Spec.Scheduling)}) +func (gsa *GameServerAllocation) Validate() field.ErrorList { + var allErrs field.ErrorList + specPath := field.NewPath("spec") + if gsa.Spec.Scheduling != apis.Packed && gsa.Spec.Scheduling != apis.Distributed { + allErrs = append(allErrs, field.NotSupported(specPath.Child("scheduling"), string(gsa.Spec.Scheduling), []string{string(apis.Packed), string(apis.Distributed)})) } - if c, ok := gsa.Spec.Required.Validate("spec.required"); !ok { - causes = append(causes, c...) - } + allErrs = append(allErrs, gsa.Spec.Required.Validate(specPath.Child("required"))...) for i := range gsa.Spec.Preferred { - if c, ok := gsa.Spec.Preferred[i].Validate(fmt.Sprintf("spec.preferred[%d]", i)); !ok { - causes = append(causes, c...) - } + allErrs = append(allErrs, gsa.Spec.Preferred[i].Validate(specPath.Child("preferred").Index(i))...) } for i := range gsa.Spec.Selectors { - if c, ok := gsa.Spec.Selectors[i].Validate(fmt.Sprintf("spec.selectors[%d]", i)); !ok { - causes = append(causes, c...) - } + allErrs = append(allErrs, gsa.Spec.Selectors[i].Validate(specPath.Child("selectors").Index(i))...) } if !runtime.FeatureEnabled(runtime.FeatureCountsAndLists) && (gsa.Spec.Priorities != nil) { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Message: "Feature CountsAndLists must be enabled if Priorities is specified", - Field: "spec.priorities", - }) + allErrs = append(allErrs, field.Forbidden(specPath.Child("priorities"), "Feature CountsAndLists must be enabled if Priorities is specified")) } if runtime.FeatureEnabled(runtime.FeatureCountsAndLists) && (gsa.Spec.Priorities != nil) { + pPath := specPath.Child("priorities") for i := range gsa.Spec.Priorities { - if c, ok := gsa.Spec.Priorities[i].validate(fmt.Sprintf("spec.priorities[%d]", i)); !ok { - causes = append(causes, c...) - } + allErrs = append(allErrs, gsa.Spec.Priorities[i].validate(pPath.Index(i))...) } } - if c, ok := gsa.Spec.MetaPatch.Validate(); !ok { - causes = append(causes, c...) - } - - return causes, len(causes) == 0 + allErrs = append(allErrs, gsa.Spec.MetaPatch.Validate(specPath.Child("metadata"))...) + return allErrs } // Converter converts game server allocation required and preferred fields to selectors field. diff --git a/pkg/apis/allocation/v1/gameserverallocation_test.go b/pkg/apis/allocation/v1/gameserverallocation_test.go index fdf754917f..a27f4a6321 100644 --- a/pkg/apis/allocation/v1/gameserverallocation_test.go +++ b/pkg/apis/allocation/v1/gameserverallocation_test.go @@ -24,6 +24,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/validation/field" ) func TestGameServerAllocationApplyDefaults(t *testing.T) { @@ -157,44 +158,28 @@ func TestGameServerSelectorValidate(t *testing.T) { assert.NoError(t, runtime.ParseFeatures(fmt.Sprintf("%s=true&%s=true&%s=true", runtime.FeaturePlayerAllocationFilter, runtime.FeatureStateAllocationFilter, runtime.FeatureCountsAndLists))) - type expected struct { - valid bool - causeLen int - fields []string - } - allocated := agonesv1.GameServerStateAllocated starting := agonesv1.GameServerStateStarting fixtures := map[string]struct { selector *GameServerSelector - expected expected + want field.ErrorList }{ "valid": { selector: &GameServerSelector{GameServerState: &allocated, Players: &PlayerSelector{ MinAvailable: 0, MaxAvailable: 10, }}, - expected: expected{ - valid: true, - causeLen: 0, - }, }, "nil values": { selector: &GameServerSelector{}, - expected: expected{ - valid: true, - causeLen: 0, - }, }, "invalid state": { selector: &GameServerSelector{ GameServerState: &starting, }, - expected: expected{ - valid: false, - causeLen: 1, - fields: []string{"fieldName"}, + want: field.ErrorList{ + field.Invalid(field.NewPath("fieldName.gameServerState"), starting, "GameServerState must be either Allocated or Ready"), }, }, "invalid min value": { @@ -203,10 +188,8 @@ func TestGameServerSelectorValidate(t *testing.T) { MinAvailable: -10, }, }, - expected: expected{ - valid: false, - causeLen: 1, - fields: []string{"fieldName"}, + want: field.ErrorList{ + field.Invalid(field.NewPath("fieldName", "players", "minAvailable"), int64(-10), "must be greater than or equal to 0"), }, }, "invalid max value": { @@ -216,10 +199,9 @@ func TestGameServerSelectorValidate(t *testing.T) { MaxAvailable: -20, }, }, - expected: expected{ - valid: false, - causeLen: 2, - fields: []string{"fieldName", "fieldName"}, + want: field.ErrorList{ + field.Invalid(field.NewPath("fieldName", "players", "minAvailable"), int64(-30), "must be greater than or equal to 0"), + field.Invalid(field.NewPath("fieldName", "players", "maxAvailable"), int64(-20), "must be greater than or equal to 0"), }, }, "invalid min/max value": { @@ -229,10 +211,8 @@ func TestGameServerSelectorValidate(t *testing.T) { MaxAvailable: 5, }, }, - expected: expected{ - valid: false, - causeLen: 1, - fields: []string{"fieldName"}, + want: field.ErrorList{ + field.Invalid(field.NewPath("fieldName", "players", "minAvailable"), int64(10), "minAvailable cannot be greater than maxAvailable"), }, }, "invalid label keys": { @@ -241,10 +221,12 @@ func TestGameServerSelectorValidate(t *testing.T) { MatchLabels: map[string]string{"$$$$": "true"}, }, }, - expected: expected{ - valid: false, - causeLen: 1, - fields: []string{"fieldName"}, + want: field.ErrorList{ + field.Invalid( + field.NewPath("fieldName", "labelSelector"), + metav1.LabelSelector{MatchLabels: map[string]string{"$$$$": "true"}}, + `Error converting label selector: key: Invalid value: "$$$$": name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')`, + ), }, }, "invalid min/max Counter available value": { @@ -256,10 +238,9 @@ func TestGameServerSelectorValidate(t *testing.T) { }, }, }, - expected: expected{ - valid: false, - causeLen: 2, - fields: []string{"fieldName", "fieldName"}, + want: field.ErrorList{ + field.Invalid(field.NewPath("fieldName", "counters[counter]", "minAvailable"), int64(-1), "must be greater than or equal to 0"), + field.Invalid(field.NewPath("fieldName", "counters[counter]", "maxAvailable"), int64(-1), "must be greater than or equal to 0"), }, }, "invalid max less than min Counter available value": { @@ -271,10 +252,8 @@ func TestGameServerSelectorValidate(t *testing.T) { }, }, }, - expected: expected{ - valid: false, - causeLen: 1, - fields: []string{"fieldName"}, + want: field.ErrorList{ + field.Invalid(field.NewPath("fieldName", "counters[foo]"), int64(1), "maxAvailable must zero or greater than minAvailable 10"), }, }, "invalid min/max Counter count value": { @@ -286,10 +265,9 @@ func TestGameServerSelectorValidate(t *testing.T) { }, }, }, - expected: expected{ - valid: false, - causeLen: 2, - fields: []string{"fieldName", "fieldName"}, + want: field.ErrorList{ + field.Invalid(field.NewPath("fieldName", "counters[counter]", "minCount"), int64(-1), "must be greater than or equal to 0"), + field.Invalid(field.NewPath("fieldName", "counters[counter]", "maxCount"), int64(-1), "must be greater than or equal to 0"), }, }, "invalid max less than min Counter count value": { @@ -301,10 +279,8 @@ func TestGameServerSelectorValidate(t *testing.T) { }, }, }, - expected: expected{ - valid: false, - causeLen: 1, - fields: []string{"fieldName"}, + want: field.ErrorList{ + field.Invalid(field.NewPath("fieldName", "counters[foo]"), int64(1), "maxCount must zero or greater than minCount 10"), }, }, "invalid min/max List value": { @@ -316,10 +292,9 @@ func TestGameServerSelectorValidate(t *testing.T) { }, }, }, - expected: expected{ - valid: false, - causeLen: 2, - fields: []string{"fieldName", "fieldName"}, + want: field.ErrorList{ + field.Invalid(field.NewPath("fieldName", "lists[list]", "minAvailable"), int64(-11), "must be greater than or equal to 0"), + field.Invalid(field.NewPath("fieldName", "lists[list]", "maxAvailable"), int64(-11), "must be greater than or equal to 0"), }, }, "invalid max less than min List value": { @@ -331,10 +306,8 @@ func TestGameServerSelectorValidate(t *testing.T) { }, }, }, - expected: expected{ - valid: false, - causeLen: 1, - fields: []string{"fieldName"}, + want: field.ErrorList{ + field.Invalid(field.NewPath("fieldName", "lists[list]"), int64(2), "maxAvailable must zero or greater than minAvailable 11"), }, }, } @@ -342,13 +315,8 @@ func TestGameServerSelectorValidate(t *testing.T) { for k, v := range fixtures { t.Run(k, func(t *testing.T) { v.selector.ApplyDefaults() - causes, valid := v.selector.Validate("fieldName") - assert.Equal(t, v.expected.valid, valid) - assert.Len(t, causes, v.expected.causeLen) - - for i := range v.expected.fields { - assert.Equal(t, v.expected.fields[i], causes[i].Field) - } + allErrs := v.selector.Validate(field.NewPath("fieldName")) + assert.ElementsMatch(t, v.want, allErrs) }) } } @@ -360,15 +328,9 @@ func TestGameServerPriorityValidate(t *testing.T) { defer runtime.FeatureTestMutex.Unlock() assert.NoError(t, runtime.ParseFeatures(fmt.Sprintf("%s=true", runtime.FeatureCountsAndLists))) - type expected struct { - valid bool - causeLen int - fields []string - } - fixtures := map[string]struct { - gsa *GameServerAllocation - expected expected + gsa *GameServerAllocation + want field.ErrorList }{ "valid Counter Ascending": { gsa: &GameServerAllocation{ @@ -381,10 +343,6 @@ func TestGameServerPriorityValidate(t *testing.T) { }, }, }, - expected: expected{ - valid: true, - causeLen: 0, - }, }, "valid Counter Descending": { gsa: &GameServerAllocation{ @@ -397,10 +355,6 @@ func TestGameServerPriorityValidate(t *testing.T) { }, }, }, - expected: expected{ - valid: true, - causeLen: 0, - }, }, "valid Counter empty Order": { gsa: &GameServerAllocation{ @@ -413,10 +367,6 @@ func TestGameServerPriorityValidate(t *testing.T) { }, }, }, - expected: expected{ - valid: true, - causeLen: 0, - }, }, "invalid counter type and order": { gsa: &GameServerAllocation{ @@ -429,9 +379,9 @@ func TestGameServerPriorityValidate(t *testing.T) { }, }, }, - expected: expected{ - valid: false, - causeLen: 2, + want: field.ErrorList{ + field.NotSupported(field.NewPath("spec", "priorities[0]", "priorityType"), "counter", []string{"Counter", "List"}), + field.NotSupported(field.NewPath("spec", "priorities[0]", "order"), "descending", []string{"Ascending", "Descending"}), }, }, "valid List Ascending": { @@ -445,10 +395,6 @@ func TestGameServerPriorityValidate(t *testing.T) { }, }, }, - expected: expected{ - valid: true, - causeLen: 0, - }, }, "valid List Descending": { gsa: &GameServerAllocation{ @@ -461,10 +407,6 @@ func TestGameServerPriorityValidate(t *testing.T) { }, }, }, - expected: expected{ - valid: true, - causeLen: 0, - }, }, "valid List empty Order": { gsa: &GameServerAllocation{ @@ -477,10 +419,6 @@ func TestGameServerPriorityValidate(t *testing.T) { }, }, }, - expected: expected{ - valid: true, - causeLen: 0, - }, }, "invalid list type and order": { gsa: &GameServerAllocation{ @@ -493,9 +431,9 @@ func TestGameServerPriorityValidate(t *testing.T) { }, }, }, - expected: expected{ - valid: false, - causeLen: 2, + want: field.ErrorList{ + field.NotSupported(field.NewPath("spec", "priorities[0]", "priorityType"), "list", []string{"Counter", "List"}), + field.NotSupported(field.NewPath("spec", "priorities[0]", "order"), "ascending", []string{"Ascending", "Descending"}), }, }, } @@ -503,13 +441,8 @@ func TestGameServerPriorityValidate(t *testing.T) { for k, v := range fixtures { t.Run(k, func(t *testing.T) { v.gsa.ApplyDefaults() - causes, valid := v.gsa.Validate() - assert.Equal(t, v.expected.valid, valid) - assert.Len(t, causes, v.expected.causeLen) - - for i := range v.expected.fields { - assert.Equal(t, v.expected.fields[i], causes[i].Field) - } + allErrs := v.gsa.Validate() + assert.ElementsMatch(t, v.want, allErrs) }) } } @@ -522,48 +455,41 @@ func TestMetaPatchValidate(t *testing.T) { Labels: nil, Annotations: nil, } - causes, valid := mp.Validate() - assert.True(t, valid) - assert.Empty(t, causes) + path := field.NewPath("spec", "metadata") + allErrs := mp.Validate(path) + assert.Len(t, allErrs, 0) mp.Labels = map[string]string{} mp.Annotations = map[string]string{} - causes, valid = mp.Validate() - assert.True(t, valid) - assert.Empty(t, causes) + allErrs = mp.Validate(path) + assert.Len(t, allErrs, 0) mp.Labels["foo"] = "bar" mp.Annotations["bar"] = "foo" - causes, valid = mp.Validate() - assert.True(t, valid) - assert.Empty(t, causes) + allErrs = mp.Validate(path) + assert.Len(t, allErrs, 0) // invalid label invalid := mp.DeepCopy() invalid.Labels["$$$$"] = "no" - - causes, valid = invalid.Validate() - assert.False(t, valid) - require.Len(t, causes, 1) - assert.Equal(t, "metadata.labels", causes[0].Field) + allErrs = invalid.Validate(path) + assert.Len(t, allErrs, 1) + assert.Equal(t, "spec.metadata.labels", allErrs[0].Field) // invalid annotation invalid = mp.DeepCopy() invalid.Annotations["$$$$"] = "no" - causes, valid = invalid.Validate() - assert.False(t, valid) - require.Len(t, causes, 1) - assert.Equal(t, "metadata.annotations", causes[0].Field) + allErrs = invalid.Validate(path) + require.Len(t, allErrs, 1) + assert.Equal(t, "spec.metadata.annotations", allErrs[0].Field) // invalid both invalid.Labels["$$$$"] = "no" - causes, valid = invalid.Validate() - - assert.False(t, valid) - require.Len(t, causes, 2) - assert.Equal(t, "metadata.labels", causes[0].Field) - assert.Equal(t, "metadata.annotations", causes[1].Field) + allErrs = invalid.Validate(path) + require.Len(t, allErrs, 2) + assert.Equal(t, "spec.metadata.labels", allErrs[0].Field) + assert.Equal(t, "spec.metadata.annotations", allErrs[1].Field) } func TestGameServerSelectorMatches(t *testing.T) { @@ -1247,18 +1173,16 @@ func TestGameServerAllocationValidate(t *testing.T) { gsa := &GameServerAllocation{} gsa.ApplyDefaults() - causes, ok := gsa.Validate() - assert.True(t, ok) - assert.Empty(t, causes) + allErrs := gsa.Validate() + assert.Len(t, allErrs, 0) gsa.Spec.Scheduling = "FLERG" - causes, ok = gsa.Validate() - assert.False(t, ok) - assert.Len(t, causes, 1) + allErrs = gsa.Validate() + assert.Len(t, allErrs, 1) - assert.Equal(t, metav1.CauseTypeFieldValueInvalid, causes[0].Type) - assert.Equal(t, "spec.scheduling", causes[0].Field) + assert.Equal(t, field.ErrorTypeNotSupported, allErrs[0].Type) + assert.Equal(t, "spec.scheduling", allErrs[0].Field) runtime.FeatureTestMutex.Lock() defer runtime.FeatureTestMutex.Unlock() @@ -1282,13 +1206,12 @@ func TestGameServerAllocationValidate(t *testing.T) { } gsa.ApplyDefaults() - causes, ok = gsa.Validate() - assert.False(t, ok) - assert.Len(t, causes, 4) - assert.Equal(t, "spec.required", causes[0].Field) - assert.Equal(t, "spec.preferred[0]", causes[1].Field) - assert.Equal(t, "spec.preferred[0]", causes[2].Field) - assert.Equal(t, "metadata.labels", causes[3].Field) + allErrs = gsa.Validate() + assert.Len(t, allErrs, 4) + 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.preferred[0].players.minAvailable", allErrs[2].Field) + assert.Equal(t, "spec.metadata.labels", allErrs[3].Field) } func TestGameServerAllocationConverter(t *testing.T) { diff --git a/pkg/gameserverallocations/allocator.go b/pkg/gameserverallocations/allocator.go index 383293c913..5d85305e78 100644 --- a/pkg/gameserverallocations/allocator.go +++ b/pkg/gameserverallocations/allocator.go @@ -20,7 +20,6 @@ import ( "crypto/x509" goErrors "errors" "fmt" - "net/http" "strings" "time" @@ -48,6 +47,7 @@ import ( "k8s.io/apimachinery/pkg/labels" k8sruntime "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + runtimeschema "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/wait" informercorev1 "k8s.io/client-go/informers/core/v1" "k8s.io/client-go/kubernetes" @@ -199,26 +199,20 @@ func (c *Allocator) Allocate(ctx context.Context, gsa *allocationv1.GameServerAl latency.setRequest(gsa) // server side validation - if causes, ok := gsa.Validate(); !ok { - s := &metav1.Status{ - Status: metav1.StatusFailure, - Message: fmt.Sprintf("GameServerAllocation is invalid: Invalid value: %#v", gsa), - Reason: metav1.StatusReasonInvalid, - Details: &metav1.StatusDetails{ - Kind: "GameServerAllocation", - Group: allocationv1.SchemeGroupVersion.Group, - Causes: causes, - }, - Code: http.StatusUnprocessableEntity, + if errs := gsa.Validate(); len(errs) > 0 { + kind := runtimeschema.GroupKind{ + Group: allocationv1.SchemeGroupVersion.Group, + Kind: "GameServerAllocation", } - + statusErr := k8serrors.NewInvalid(kind, gsa.Name, errs) + s := &statusErr.ErrStatus var gvks []schema.GroupVersionKind gvks, _, err := apiserver.Scheme.ObjectKinds(s) if err != nil { return nil, errors.Wrap(err, "could not find objectkinds for status") } - c.loggerForGameServerAllocation(gsa).Debug("GameServerAllocation is invalid") + c.loggerForGameServerAllocation(gsa).Debug("GameServerAllocation is invalid") s.TypeMeta = metav1.TypeMeta{Kind: gvks[0].Kind, APIVersion: gvks[0].Version} return s, nil } diff --git a/pkg/gameserverallocations/allocator_test.go b/pkg/gameserverallocations/allocator_test.go index ea3fa5cc60..67883c7569 100644 --- a/pkg/gameserverallocations/allocator_test.go +++ b/pkg/gameserverallocations/allocator_test.go @@ -87,8 +87,8 @@ func TestAllocatorAllocate(t *testing.T) { MetaPatch: fam, }} gsa.ApplyDefaults() - _, ok := gsa.Validate() - require.True(t, ok) + errs := gsa.Validate() + require.Len(t, errs, 0) gs, err := a.allocate(ctx, &gsa) require.NoError(t, err) @@ -165,8 +165,8 @@ func TestAllocatorAllocatePriority(t *testing.T) { Selectors: []allocationv1.GameServerSelector{{LabelSelector: metav1.LabelSelector{MatchLabels: map[string]string{agonesv1.FleetNameLabel: f.ObjectMeta.Name}}}}, }} gsa.ApplyDefaults() - _, ok := gsa.Validate() - require.True(t, ok) + errs := gsa.Validate() + require.Len(t, errs, 0) t.Run(name, func(t *testing.T) { test(t, a, gsa.DeepCopy()) @@ -468,8 +468,8 @@ func TestAllocatorAllocateOnGameServerUpdateError(t *testing.T) { gsa.ApplyDefaults() // without converter we don't end up with at least one selector gsa.Converter() - _, ok := gsa.Validate() - require.True(t, ok) + errs := gsa.Validate() + require.Len(t, errs, 0) require.Len(t, gsa.Spec.Selectors, 1) // try the private method @@ -524,8 +524,8 @@ func TestAllocatorRunLocalAllocations(t *testing.T) { Selectors: []allocationv1.GameServerSelector{{LabelSelector: metav1.LabelSelector{MatchLabels: map[string]string{agonesv1.FleetNameLabel: f.ObjectMeta.Name}}}}, }} gsa.ApplyDefaults() - _, ok := gsa.Validate() - require.True(t, ok) + errs := gsa.Validate() + require.Len(t, errs, 0) // line up 3 in a batch j1 := request{gsa: gsa.DeepCopy(), response: make(chan response)} @@ -581,8 +581,8 @@ func TestAllocatorRunLocalAllocations(t *testing.T) { Selectors: []allocationv1.GameServerSelector{{LabelSelector: metav1.LabelSelector{MatchLabels: map[string]string{agonesv1.FleetNameLabel: "thereisnofleet"}}}}, }} gsa.ApplyDefaults() - _, ok := gsa.Validate() - require.True(t, ok) + errs := gsa.Validate() + require.Len(t, errs, 0) j1 := request{gsa: gsa.DeepCopy(), response: make(chan response)} a.pendingRequests <- j1 diff --git a/pkg/gameserverallocations/find_test.go b/pkg/gameserverallocations/find_test.go index 2613bbca97..b98b8c6de8 100644 --- a/pkg/gameserverallocations/find_test.go +++ b/pkg/gameserverallocations/find_test.go @@ -72,8 +72,8 @@ func TestFindGameServerForAllocationPacked(t *testing.T) { } emptyGSA.ApplyDefaults() emptyGSA.Converter() - _, ok := emptyGSA.Validate() - require.True(t, ok) + allErrs := emptyGSA.Validate() + require.Len(t, allErrs, 0) require.Len(t, emptyGSA.Spec.Selectors, 1) gs, index, err := findGameServerForAllocation(emptyGSA, list) @@ -290,12 +290,12 @@ func TestFindGameServerForAllocationPacked(t *testing.T) { require.NoError(t, runtime.ParseFeatures(v.features)) gsa.ApplyDefaults() - _, ok := gsa.Validate() - require.True(t, ok) + allErrs := gsa.Validate() + require.Len(t, allErrs, 0) twoLabelsGsa.ApplyDefaults() - _, ok = twoLabelsGsa.Validate() - require.True(t, ok) + allErrs = twoLabelsGsa.Validate() + require.Len(t, allErrs, 0) controller, m := newFakeController() c := controller.allocator.allocationCache @@ -337,8 +337,8 @@ func TestFindGameServerForAllocationDistributed(t *testing.T) { }, } gsa.ApplyDefaults() - _, ok := gsa.Validate() - require.True(t, ok) + allErrs := gsa.Validate() + require.Len(t, allErrs, 0) gsList := []agonesv1.GameServer{ {ObjectMeta: metav1.ObjectMeta{Name: "gs1", Namespace: defaultNs, Labels: labels}, diff --git a/test/e2e/gameserverallocation_test.go b/test/e2e/gameserverallocation_test.go index 1a2e970577..3d3205984b 100644 --- a/test/e2e/gameserverallocation_test.go +++ b/test/e2e/gameserverallocation_test.go @@ -513,7 +513,7 @@ func TestGameServerAllocationMetaDataPatch(t *testing.T) { result, err = framework.AgonesClient.AllocationV1().GameServerAllocations(framework.Namespace).Create(ctx, gsa.DeepCopy(), metav1.CreateOptions{}) log.WithField("result", result).WithError(err).Info("Failed allocation") require.Error(t, err) - require.Contains(t, err.Error(), "GameServerAllocation is invalid") + require.Contains(t, err.Error(), `GameServerAllocation.allocation.agones.dev "" is invalid`) } func TestGameServerAllocationPreferredSelection(t *testing.T) {