Skip to content

Commit 32d22cf

Browse files
markmandelRobert Bailey
markmandel
and
Robert Bailey
authored
Validate GameServerAllocation metadata (agones-dev#2449)
With this change, if the user sends a metadata value with either invalid labels or annotations, they will get an appropriate validation failure result - rather than an Allocation with an `UnAllocated` response. Closes agones-dev#2282 Co-authored-by: Robert Bailey <robertbailey@google.com>
1 parent b0f3b95 commit 32d22cf

3 files changed

Lines changed: 147 additions & 23 deletions

File tree

‎pkg/apis/allocation/v1/gameserverallocation.go‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,11 @@ import (
2020
"agones.dev/agones/pkg/apis"
2121
agonesv1 "agones.dev/agones/pkg/apis/agones/v1"
2222
"agones.dev/agones/pkg/util/runtime"
23+
apivalidation "k8s.io/apimachinery/pkg/api/validation"
2324
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
25+
metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
2426
"k8s.io/apimachinery/pkg/labels"
27+
validationfield "k8s.io/apimachinery/pkg/util/validation/field"
2528
)
2629

2730
const (
@@ -238,6 +241,36 @@ type MetaPatch struct {
238241
Annotations map[string]string `json:"annotations,omitempty"`
239242
}
240243

244+
// Validate returns if the labels and/or annotations that are to be applied to a `GameServer` post
245+
// allocation are valid.
246+
func (mp *MetaPatch) Validate() ([]metav1.StatusCause, bool) {
247+
var causes []metav1.StatusCause
248+
249+
errs := metav1validation.ValidateLabels(mp.Labels, validationfield.NewPath("labels"))
250+
if len(errs) != 0 {
251+
for _, v := range errs {
252+
causes = append(causes, metav1.StatusCause{
253+
Type: metav1.CauseTypeFieldValueInvalid,
254+
Field: "metadata.labels",
255+
Message: v.Error(),
256+
})
257+
}
258+
}
259+
260+
errs = apivalidation.ValidateAnnotations(mp.Annotations, validationfield.NewPath("annotations"))
261+
if len(errs) != 0 {
262+
for _, v := range errs {
263+
causes = append(causes, metav1.StatusCause{
264+
Type: metav1.CauseTypeFieldValueInvalid,
265+
Field: "metadata.annotations",
266+
Message: v.Error(),
267+
})
268+
}
269+
}
270+
271+
return causes, len(causes) == 0
272+
}
273+
241274
// GameServerAllocationStatus is the status for an GameServerAllocation resource
242275
type GameServerAllocationStatus struct {
243276
// GameServerState is the current state of an GameServerAllocation, e.g. Allocated, or UnAllocated
@@ -298,6 +331,10 @@ func (gsa *GameServerAllocation) Validate() ([]metav1.StatusCause, bool) {
298331
}
299332
}
300333

334+
if c, ok := gsa.Spec.MetaPatch.Validate(); !ok {
335+
causes = append(causes, c...)
336+
}
337+
301338
return causes, len(causes) == 0
302339
}
303340

‎pkg/apis/allocation/v1/gameserverallocation_test.go‎

Lines changed: 69 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,18 @@ func TestGameServerSelectorValidate(t *testing.T) {
219219
fields: []string{"fieldName"},
220220
},
221221
},
222+
"invalid label keys": {
223+
selector: &GameServerSelector{
224+
LabelSelector: metav1.LabelSelector{
225+
MatchLabels: map[string]string{"$$$$": "true"},
226+
},
227+
},
228+
expected: expected{
229+
valid: false,
230+
causeLen: 1,
231+
fields: []string{"fieldName"},
232+
},
233+
},
222234
}
223235

224236
for k, v := range fixtures {
@@ -235,6 +247,58 @@ func TestGameServerSelectorValidate(t *testing.T) {
235247
}
236248
}
237249

250+
func TestMetaPatchValidate(t *testing.T) {
251+
t.Parallel()
252+
253+
// valid
254+
mp := &MetaPatch{
255+
Labels: nil,
256+
Annotations: nil,
257+
}
258+
causes, valid := mp.Validate()
259+
assert.True(t, valid)
260+
assert.Empty(t, causes)
261+
262+
mp.Labels = map[string]string{}
263+
mp.Annotations = map[string]string{}
264+
causes, valid = mp.Validate()
265+
assert.True(t, valid)
266+
assert.Empty(t, causes)
267+
268+
mp.Labels["foo"] = "bar"
269+
mp.Annotations["bar"] = "foo"
270+
causes, valid = mp.Validate()
271+
assert.True(t, valid)
272+
assert.Empty(t, causes)
273+
274+
// invalid label
275+
invalid := mp.DeepCopy()
276+
invalid.Labels["$$$$"] = "no"
277+
278+
causes, valid = invalid.Validate()
279+
assert.False(t, valid)
280+
require.Len(t, causes, 1)
281+
assert.Equal(t, "metadata.labels", causes[0].Field)
282+
283+
// invalid annotation
284+
invalid = mp.DeepCopy()
285+
invalid.Annotations["$$$$"] = "no"
286+
287+
causes, valid = invalid.Validate()
288+
assert.False(t, valid)
289+
require.Len(t, causes, 1)
290+
assert.Equal(t, "metadata.annotations", causes[0].Field)
291+
292+
// invalid both
293+
invalid.Labels["$$$$"] = "no"
294+
causes, valid = invalid.Validate()
295+
296+
assert.False(t, valid)
297+
require.Len(t, causes, 2)
298+
assert.Equal(t, "metadata.labels", causes[0].Field)
299+
assert.Equal(t, "metadata.annotations", causes[1].Field)
300+
}
301+
238302
func TestGameServerSelectorMatches(t *testing.T) {
239303
t.Parallel()
240304

@@ -453,16 +517,20 @@ func TestGameServerAllocationValidate(t *testing.T) {
453517
Preferred: []GameServerSelector{
454518
{Players: &PlayerSelector{MaxAvailable: -10}},
455519
},
520+
MetaPatch: MetaPatch{
521+
Labels: map[string]string{"$$$": "foo"},
522+
},
456523
},
457524
}
458525
gsa.ApplyDefaults()
459526

460527
causes, ok = gsa.Validate()
461528
assert.False(t, ok)
462-
assert.Len(t, causes, 3)
529+
assert.Len(t, causes, 4)
463530
assert.Equal(t, "spec.required", causes[0].Field)
464531
assert.Equal(t, "spec.preferred[0]", causes[1].Field)
465532
assert.Equal(t, "spec.preferred[0]", causes[2].Field)
533+
assert.Equal(t, "metadata.labels", causes[3].Field)
466534
}
467535

468536
func TestGameServerAllocationConverter(t *testing.T) {

‎test/e2e/gameserverallocation_test.go‎

Lines changed: 41 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -376,15 +376,30 @@ func TestGameServerAllocationMetaDataPatch(t *testing.T) {
376376
t.Parallel()
377377
ctx := context.Background()
378378

379-
gs := framework.DefaultGameServer(framework.Namespace)
380-
gs.ObjectMeta.Labels = map[string]string{"test": t.Name()}
379+
log := logrus.WithField("test", t.Name())
380+
createAndAllocate := func(input *allocationv1.GameServerAllocation) *allocationv1.GameServerAllocation {
381+
gs := framework.DefaultGameServer(framework.Namespace)
382+
gs.ObjectMeta.Labels = map[string]string{"test": t.Name()}
383+
gs, err := framework.CreateGameServerAndWaitUntilReady(t, framework.Namespace, gs)
384+
require.NoError(t, err)
385+
386+
log.WithField("gs", gs.ObjectMeta.Name).Info("👍 created and ready")
387+
388+
// poll, as it may take a moment for the allocation cache to be populated
389+
err = wait.PollImmediate(time.Second, 30*time.Second, func() (bool, error) {
390+
input, err = framework.AgonesClient.AllocationV1().GameServerAllocations(framework.Namespace).Create(ctx, input, metav1.CreateOptions{})
391+
if err != nil {
392+
log.WithError(err).Info("Failed, trying again...")
393+
return false, err
394+
}
381395

382-
gs, err := framework.CreateGameServerAndWaitUntilReady(t, framework.Namespace, gs)
383-
if !assert.Nil(t, err) {
384-
assert.FailNow(t, "could not create GameServer")
396+
return allocationv1.GameServerAllocationAllocated == input.Status.State, nil
397+
})
398+
require.NoError(t, err)
399+
return input
385400
}
386-
defer framework.AgonesClient.AgonesV1().GameServers(framework.Namespace).Delete(ctx, gs.ObjectMeta.Name, metav1.DeleteOptions{}) // nolint: errcheck
387401

402+
// two standard labels
388403
gsa := &allocationv1.GameServerAllocation{ObjectMeta: metav1.ObjectMeta{GenerateName: "allocation-"},
389404
Spec: allocationv1.GameServerAllocationSpec{
390405
Selectors: []allocationv1.GameServerSelector{{LabelSelector: metav1.LabelSelector{MatchLabels: map[string]string{"test": t.Name()}}}},
@@ -393,25 +408,29 @@ func TestGameServerAllocationMetaDataPatch(t *testing.T) {
393408
Annotations: map[string]string{"dog": "good"},
394409
},
395410
}}
411+
result := createAndAllocate(gsa)
412+
defer framework.AgonesClient.AgonesV1().GameServers(framework.Namespace).Delete(ctx, result.Status.GameServerName, metav1.DeleteOptions{}) // nolint: errcheck
396413

397-
err = wait.PollImmediate(time.Second, 30*time.Second, func() (bool, error) {
398-
gsa, err = framework.AgonesClient.AllocationV1().GameServerAllocations(framework.Namespace).Create(ctx, gsa.DeepCopy(), metav1.CreateOptions{})
414+
gs, err := framework.AgonesClient.AgonesV1().GameServers(framework.Namespace).Get(ctx, result.Status.GameServerName, metav1.GetOptions{})
415+
require.NoError(t, err)
416+
assert.Equal(t, "blue", gs.ObjectMeta.Labels["red"])
417+
assert.Equal(t, "good", gs.ObjectMeta.Annotations["dog"])
399418

400-
if err != nil {
401-
return true, err
402-
}
419+
// use special characters that are valid
420+
gsa.Spec.MetaPatch = allocationv1.MetaPatch{Labels: map[string]string{"blue-frog.fred_thing": "test"}}
421+
result = createAndAllocate(gsa)
422+
defer framework.AgonesClient.AgonesV1().GameServers(framework.Namespace).Delete(ctx, result.Status.GameServerName, metav1.DeleteOptions{}) // nolint: errcheck
403423

404-
return allocationv1.GameServerAllocationAllocated == gsa.Status.State, nil
405-
})
406-
if err != nil {
407-
assert.FailNow(t, err.Error())
408-
}
409-
410-
gs, err = framework.AgonesClient.AgonesV1().GameServers(framework.Namespace).Get(ctx, gsa.Status.GameServerName, metav1.GetOptions{})
411-
if assert.Nil(t, err) {
412-
assert.Equal(t, "blue", gs.ObjectMeta.Labels["red"])
413-
assert.Equal(t, "good", gs.ObjectMeta.Annotations["dog"])
414-
}
424+
gs, err = framework.AgonesClient.AgonesV1().GameServers(framework.Namespace).Get(ctx, result.Status.GameServerName, metav1.GetOptions{})
425+
require.NoError(t, err)
426+
assert.Equal(t, "test", gs.ObjectMeta.Labels["blue-frog.fred_thing"])
427+
428+
// throw something invalid at it.
429+
gsa.Spec.MetaPatch = allocationv1.MetaPatch{Labels: map[string]string{"$$$$$$$": "test"}}
430+
result, err = framework.AgonesClient.AllocationV1().GameServerAllocations(framework.Namespace).Create(ctx, gsa.DeepCopy(), metav1.CreateOptions{})
431+
log.WithField("result", result).WithError(err).Info("Failed allocation")
432+
require.Error(t, err)
433+
require.Contains(t, err.Error(), "GameServerAllocation is invalid")
415434
}
416435

417436
func TestGameServerAllocationPreferredSelection(t *testing.T) {

0 commit comments

Comments
 (0)