Skip to content

Commit

Permalink
Allocation is broken when using the generated go client
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
markmandel authored and cyriltovena committed Sep 7, 2018
1 parent 3128070 commit 7fed6b4
Show file tree
Hide file tree
Showing 7 changed files with 87 additions and 9 deletions.
4 changes: 2 additions & 2 deletions pkg/apis/stable/v1alpha1/fleetallocation.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions pkg/fleetallocation/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion pkg/fleetallocation/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}

Expand Down
59 changes: 59 additions & 0 deletions test/e2e/fleet_test.go
Original file line number Diff line number Diff line change
@@ -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,
},
},
}
}
26 changes: 22 additions & 4 deletions test/e2e/framework/framework.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
2 changes: 0 additions & 2 deletions test/e2e/gameserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@ import (
"k8s.io/apimachinery/pkg/util/wait"
)

const defaultNs = "default"

func TestCreateConnect(t *testing.T) {
t.Parallel()
gs := defaultGameServer()
Expand Down
2 changes: 2 additions & 0 deletions test/e2e/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import (
e2eframework "agones.dev/agones/test/e2e/framework"
)

const defaultNs = "default"

var framework *e2eframework.Framework

func TestMain(m *testing.M) {
Expand Down

0 comments on commit 7fed6b4

Please sign in to comment.