From 7fed6b4f3dc307871db3c3b016e1c09503d1aed3 Mon Sep 17 00:00:00 2001 From: Mark Mandel Date: Thu, 6 Sep 2018 20:36:08 -0700 Subject: [PATCH] Allocation is broken when using the generated go client When attempting to create a fleet allocation through the generated go client the following error would occur: `"error: "Internal error occurred: Internal error occurred: jsonpatch replace operation does not apply: doc is missing key: /status/GameServer"` This is because we didn't mark pointers in `FleetAllocation` to be json, `omitempty`. When not using the client (either yaml, or rest) the following JSON is sent: ```json { "apiVersion": "stable.agones.dev/v1alpha1", "kind": "FleetAllocation", "metadata": { "generateName": "simple-udp-", "namespace": "default" }, "spec": { "fleetName": "simple-udp" } } ``` Previous to this fix, when calling through the go client, the following JSON would get generated: ```json { "apiVersion": "stable.agones.dev/v1alpha1", "kind": "FleetAllocation", "metadata": { "creationTimestamp": null, "generateName": "allocatioon-", "namespace": "default" }, "spec": { "fleetName": "simple-fleet-6fb7c", "metadata": {} }, "status": { "GameServer": null } } ``` That `nil` `GameServer` value messes up the library that creates the JSONPatch. Now we have the `omitempty` declarations in the correct places, this is working correctly. We also now have e2e tests to make sure the issue does not end up replicated. Need some user testing to determine if a hotfix is appropriate for this bug, or if a workaround can be applied/this library can be used without a need for a complete redeployment. --- pkg/apis/stable/v1alpha1/fleetallocation.go | 4 +- pkg/fleetallocation/controller.go | 1 + pkg/fleetallocation/controller_test.go | 2 +- test/e2e/fleet_test.go | 59 +++++++++++++++++++++ test/e2e/framework/framework.go | 26 +++++++-- test/e2e/gameserver_test.go | 2 - test/e2e/main_test.go | 2 + 7 files changed, 87 insertions(+), 9 deletions(-) create mode 100644 test/e2e/fleet_test.go diff --git a/pkg/apis/stable/v1alpha1/fleetallocation.go b/pkg/apis/stable/v1alpha1/fleetallocation.go index 4a12c504fb..d016ec26b7 100644 --- a/pkg/apis/stable/v1alpha1/fleetallocation.go +++ b/pkg/apis/stable/v1alpha1/fleetallocation.go @@ -26,7 +26,7 @@ type FleetAllocation struct { metav1.ObjectMeta `json:"metadata,omitempty"` Spec FleetAllocationSpec `json:"spec"` - Status FleetAllocationStatus `json:"status"` + Status FleetAllocationStatus `json:"status,omitempty"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object @@ -56,7 +56,7 @@ type FleetAllocationMeta struct { // `GameServer` that has been allocated from // a Fleet type FleetAllocationStatus struct { - GameServer *GameServer + GameServer *GameServer `json:"gameServer,omitempty"` } // ValidateUpdate validates when an update occurs diff --git a/pkg/fleetallocation/controller.go b/pkg/fleetallocation/controller.go index 33efaa075a..87648edbd4 100644 --- a/pkg/fleetallocation/controller.go +++ b/pkg/fleetallocation/controller.go @@ -120,6 +120,7 @@ func (c *Controller) creationMutationHandler(review admv1beta1.AdmissionReview) c.logger.WithField("review", review).Info("creationMutationHandler") obj := review.Request.Object fa := &stablev1alpha1.FleetAllocation{} + err := json.Unmarshal(obj.Raw, fa) if err != nil { return review, errors.Wrapf(err, "error unmarshalling original FleetAllocation json: %s", obj.Raw) diff --git a/pkg/fleetallocation/controller_test.go b/pkg/fleetallocation/controller_test.go index e50bee52b5..ebcb1cae13 100644 --- a/pkg/fleetallocation/controller_test.go +++ b/pkg/fleetallocation/controller_test.go @@ -66,7 +66,7 @@ func TestControllerCreationMutationHandler(t *testing.T) { assert.Nil(t, err) assert.True(t, result.Response.Allowed, fmt.Sprintf("%#v", result.Response)) assert.Equal(t, admv1beta1.PatchTypeJSONPatch, *result.Response.PatchType) - assert.Contains(t, string(result.Response.Patch), "/status/GameServer") + assert.Contains(t, string(result.Response.Patch), "/status/gameServer") assert.Contains(t, string(result.Response.Patch), "/metadata/ownerReferences") } diff --git a/test/e2e/fleet_test.go b/test/e2e/fleet_test.go new file mode 100644 index 0000000000..dd69ca42f5 --- /dev/null +++ b/test/e2e/fleet_test.go @@ -0,0 +1,59 @@ +// Copyright 2018 Google Inc. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package e2e + +import ( + "testing" + + "agones.dev/agones/pkg/apis/stable/v1alpha1" + "github.com/stretchr/testify/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestCreateFleetAndAllocate(t *testing.T) { + t.Parallel() + + flt, err := framework.AgonesClient.StableV1alpha1().Fleets(defaultNs).Create(defaultFleet()) + assert.Nil(t, err) + + err = framework.WaitForFleetReady(flt) + assert.Nil(t, err, "fleet not ready") + + fa := &v1alpha1.FleetAllocation{ + ObjectMeta: metav1.ObjectMeta{GenerateName: "allocatioon-", Namespace: defaultNs}, + Spec: v1alpha1.FleetAllocationSpec{ + FleetName: flt.ObjectMeta.Name, + }, + } + + fa, err = framework.AgonesClient.StableV1alpha1().FleetAllocations(defaultNs).Create(fa) + assert.Nil(t, err) + assert.Equal(t, v1alpha1.Allocated, fa.Status.GameServer.Status.State) +} + +// defaultFleet returns a default fleet configuration +func defaultFleet() *v1alpha1.Fleet { + gs := defaultGameServer() + + return &v1alpha1.Fleet{ + ObjectMeta: metav1.ObjectMeta{GenerateName: "simple-fleet-", Namespace: defaultNs}, + Spec: v1alpha1.FleetSpec{ + Replicas: 3, + Template: v1alpha1.GameServerTemplateSpec{ + Spec: gs.Spec, + }, + }, + } +} diff --git a/test/e2e/framework/framework.go b/test/e2e/framework/framework.go index 0c4706464e..063788a618 100644 --- a/test/e2e/framework/framework.go +++ b/test/e2e/framework/framework.go @@ -24,13 +24,13 @@ import ( "agones.dev/agones/pkg/apis/stable/v1alpha1" "agones.dev/agones/pkg/client/clientset/versioned" "github.com/pkg/errors" - "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/sirupsen/logrus" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/kubernetes" // required to use gcloud login see: https://github.com/kubernetes/client-go/issues/242 _ "k8s.io/client-go/plugin/pkg/client/auth/gcp" "k8s.io/client-go/tools/clientcmd" - "github.com/sirupsen/logrus" ) // Framework is a testing framework @@ -90,7 +90,7 @@ func (f *Framework) WaitForGameServerState(gs *v1alpha1.GameServer, state v1alph var readyGs *v1alpha1.GameServer err := wait.PollImmediate(2*time.Second, timeout, func() (bool, error) { - readyGs, pollErr = f.AgonesClient.StableV1alpha1().GameServers(gs.Namespace).Get(gs.Name, v1.GetOptions{}) + readyGs, pollErr = f.AgonesClient.StableV1alpha1().GameServers(gs.Namespace).Get(gs.Name, metav1.GetOptions{}) if pollErr != nil { return false, nil @@ -109,10 +109,28 @@ func (f *Framework) WaitForGameServerState(gs *v1alpha1.GameServer, state v1alph return readyGs, nil } +// WaitForFleetReady waits for the Fleet to count all the GameServers in it as Ready +func (f *Framework) WaitForFleetReady(flt *v1alpha1.Fleet) error { + err := wait.PollImmediate(2*time.Second, 30*time.Second, func() (bool, error) { + fleet, err := f.AgonesClient.StableV1alpha1().Fleets(flt.ObjectMeta.Namespace).Get(flt.ObjectMeta.Name, metav1.GetOptions{}) + if err != nil { + return true, err + } + + return fleet.Status.ReadyReplicas == fleet.Spec.Replicas, nil + }) + return err +} + // CleanUp Delete all agones resources in a given namespace func (f *Framework) CleanUp(ns string) error { + err := f.AgonesClient.StableV1alpha1().Fleets(ns).DeleteCollection(&metav1.DeleteOptions{}, metav1.ListOptions{}) + if err != nil { + return err + } + return f.AgonesClient.StableV1alpha1().GameServers(ns). - DeleteCollection(&v1.DeleteOptions{}, v1.ListOptions{}) + DeleteCollection(&metav1.DeleteOptions{}, metav1.ListOptions{}) } // PingGameServer pings a gameserver and returns its reply diff --git a/test/e2e/gameserver_test.go b/test/e2e/gameserver_test.go index 9c2b4e3bfa..9f5170733e 100644 --- a/test/e2e/gameserver_test.go +++ b/test/e2e/gameserver_test.go @@ -27,8 +27,6 @@ import ( "k8s.io/apimachinery/pkg/util/wait" ) -const defaultNs = "default" - func TestCreateConnect(t *testing.T) { t.Parallel() gs := defaultGameServer() diff --git a/test/e2e/main_test.go b/test/e2e/main_test.go index c06d085bfd..31d3beaa75 100644 --- a/test/e2e/main_test.go +++ b/test/e2e/main_test.go @@ -25,6 +25,8 @@ import ( e2eframework "agones.dev/agones/test/e2e/framework" ) +const defaultNs = "default" + var framework *e2eframework.Framework func TestMain(m *testing.M) {