Update game server Patch to fail if the resource version is changed o…

…r not set
googleforgames · May 2, 2024 · 1a1b28e · 1a1b28e
1 parent 6b0f24d
commit 1a1b28e
Show file tree

Hide file tree

Showing 4 changed files with 26 additions and 17 deletions.
diff --git a/pkg/apis/agones/v1/gameserver.go b/pkg/apis/agones/v1/gameserver.go
@@ -857,8 +857,10 @@ func (gs *GameServer) CountPorts(f func(policy PortPolicy) bool) int {
 	return count
 }
 
-// Patch creates a JSONPatch to move the current GameServer
-// to the passed in delta GameServer
+// Patch creates a JSONPatch to move the current GameServer to the passed in delta GameServer.
+// Returned Patch includes a "test" operation that will cause the GameServers.Patch() operation to
+// fail if the Game Server has been updated (ResourceVersion has changed) in between when the Patch
+// was created and applied.
 func (gs *GameServer) Patch(delta *GameServer) ([]byte, error) {
 	var result []byte
 
@@ -877,7 +879,13 @@ func (gs *GameServer) Patch(delta *GameServer) ([]byte, error) {
 		return result, errors.Wrapf(err, "error creating patch for GameServer %s", gs.ObjectMeta.Name)
 	}
 
-	result, err = json.Marshal(patch)
+	// Per https://jsonpatch.com/ "Tests that the specified value is set in the document. If the test
+	// fails, then the patch as a whole should not apply."
+	// Used here to check the object has not been updated (has not changed ResourceVersion).
+	patches := []jsonpatch.JsonPatchOperation{{Operation: "test", Path: "/metadata/resourceVersion", Value: gs.ObjectMeta.ResourceVersion}}
+	patches = append(patches, patch...)
+
+	result, err = json.Marshal(patches)
 	return result, errors.Wrapf(err, "error creating json for patch for GameServer %s", gs.ObjectMeta.Name)
 }
 

diff --git a/pkg/apis/agones/v1/gameserver_test.go b/pkg/apis/agones/v1/gameserver_test.go
@@ -1431,7 +1431,7 @@ func TestGameServerCountPorts(t *testing.T) {
 }
 
 func TestGameServerPatch(t *testing.T) {
-	fixture := &GameServer{ObjectMeta: metav1.ObjectMeta{Name: "lucy"},
+	fixture := &GameServer{ObjectMeta: metav1.ObjectMeta{Name: "lucy", ResourceVersion: "1234"},
 		Spec: GameServerSpec{Container: "goat"}}
 
 	delta := fixture.DeepCopy()
@@ -1441,6 +1441,7 @@ func TestGameServerPatch(t *testing.T) {
 	assert.Nil(t, err)
 
 	assert.Contains(t, string(patch), `{"op":"replace","path":"/spec/container","value":"bear"}`)
+	assert.Contains(t, string(patch), `{"op":"test","path":"/metadata/resourceVersion","value":"1234"}`)
 }
 
 func TestGameServerGetDevAddress(t *testing.T) {

diff --git a/pkg/sdkserver/sdkserver.go b/pkg/sdkserver/sdkserver.go
@@ -456,7 +456,7 @@ func (s *SDKServer) patchGameServer(ctx context.Context, gs, gsCopy *agonesv1.Ga
 	if err != nil {
 		return nil, err
 	}
-	fmt.Println("PATCH", string(patch))
+
 	return s.gameServerGetter.GameServers(s.namespace).Patch(ctx, gs.GetObjectMeta().GetName(), types.JSONPatchType, patch, metav1.PatchOptions{})
 }
 

diff --git a/pkg/sdkserver/sdkserver_test.go b/pkg/sdkserver/sdkserver_test.go
@@ -172,7 +172,7 @@ func TestSidecarRun(t *testing.T) {
 
 			gs := agonesv1.GameServer{
 				ObjectMeta: metav1.ObjectMeta{
-					Name: "test", Namespace: "default",
+					Name: "test", Namespace: "default", ResourceVersion: "0",
 				},
 				Spec: agonesv1.GameServerSpec{
 					Health: agonesv1.Health{Disabled: false, FailureThreshold: 1, PeriodSeconds: 1, InitialDelaySeconds: 0},
@@ -309,7 +309,7 @@ func TestSDKServerSyncGameServer(t *testing.T) {
 			updated := false
 			gs := agonesv1.GameServer{ObjectMeta: metav1.ObjectMeta{
 				UID:  "1234",
-				Name: sc.gameServerName, Namespace: sc.namespace,
+				Name: sc.gameServerName, Namespace: sc.namespace, ResourceVersion: "0",
 				Labels: map[string]string{}, Annotations: map[string]string{}},
 			}
 
@@ -381,7 +381,7 @@ func TestSidecarUpdateState(t *testing.T) {
 
 			m.AgonesClient.AddReactor("list", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) {
 				gs := agonesv1.GameServer{
-					ObjectMeta: metav1.ObjectMeta{Name: sc.gameServerName, Namespace: sc.namespace},
+					ObjectMeta: metav1.ObjectMeta{Name: sc.gameServerName, Namespace: sc.namespace, ResourceVersion: "0"},
 					Status:     agonesv1.GameServerStatus{},
 				}
 
@@ -466,7 +466,7 @@ func TestSidecarUnhealthyMessage(t *testing.T) {
 
 	gs := agonesv1.GameServer{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: "test", Namespace: "default",
+			Name: "test", Namespace: "default", ResourceVersion: "0",
 		},
 		Spec: agonesv1.GameServerSpec{},
 		Status: agonesv1.GameServerStatus{
@@ -938,7 +938,7 @@ func TestSDKServerReserveTimeoutOnRun(t *testing.T) {
 
 	gs := agonesv1.GameServer{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: "test", Namespace: "default",
+			Name: "test", Namespace: "default", ResourceVersion: "0",
 		},
 		Status: agonesv1.GameServerStatus{
 			State: agonesv1.GameServerStateReserved,
@@ -1000,7 +1000,7 @@ func TestSDKServerReserveTimeout(t *testing.T) {
 
 	gs := agonesv1.GameServer{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: "test", Namespace: "default",
+			Name: "test", Namespace: "default", ResourceVersion: "0",
 		},
 		Spec: agonesv1.GameServerSpec{Health: agonesv1.Health{Disabled: true}},
 	}
@@ -1277,7 +1277,7 @@ func TestSDKServerUpdateCounter(t *testing.T) {
 
 			gs := agonesv1.GameServer{
 				ObjectMeta: metav1.ObjectMeta{
-					Name: "test", Namespace: "default", Generation: 1,
+					Name: "test", Namespace: "default", ResourceVersion: "0", Generation: 1,
 				},
 				Spec: agonesv1.GameServerSpec{
 					SdkServer: agonesv1.SdkServer{
@@ -1430,7 +1430,7 @@ func TestSDKServerAddListValue(t *testing.T) {
 
 			gs := agonesv1.GameServer{
 				ObjectMeta: metav1.ObjectMeta{
-					Name: "test", Namespace: "default", Generation: 1,
+					Name: "test", Namespace: "default", ResourceVersion: "0", Generation: 1,
 				},
 				Spec: agonesv1.GameServerSpec{
 					SdkServer: agonesv1.SdkServer{
@@ -1579,7 +1579,7 @@ func TestSDKServerRemoveListValue(t *testing.T) {
 
 			gs := agonesv1.GameServer{
 				ObjectMeta: metav1.ObjectMeta{
-					Name: "test", Namespace: "default", Generation: 1,
+					Name: "test", Namespace: "default", ResourceVersion: "0", Generation: 1,
 				},
 				Spec: agonesv1.GameServerSpec{
 					SdkServer: agonesv1.SdkServer{
@@ -1737,7 +1737,7 @@ func TestSDKServerUpdateList(t *testing.T) {
 
 			gs := agonesv1.GameServer{
 				ObjectMeta: metav1.ObjectMeta{
-					Name: "test", Namespace: "default", Generation: 1,
+					Name: "test", Namespace: "default", ResourceVersion: "0", Generation: 1,
 				},
 				Spec: agonesv1.GameServerSpec{
 					SdkServer: agonesv1.SdkServer{
@@ -1865,7 +1865,7 @@ func TestSDKServerPlayerCapacity(t *testing.T) {
 
 	gs := agonesv1.GameServer{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: "test", Namespace: "default",
+			Name: "test", Namespace: "default", ResourceVersion: "0",
 		},
 		Spec: agonesv1.GameServerSpec{
 			SdkServer: agonesv1.SdkServer{
@@ -2018,7 +2018,7 @@ func TestSDKServerPlayerConnectAndDisconnect(t *testing.T) {
 	capacity := int64(3)
 	gs := agonesv1.GameServer{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: "test", Namespace: "default",
+			Name: "test", Namespace: "default", ResourceVersion: "0",
 		},
 		Spec: agonesv1.GameServerSpec{
 			SdkServer: agonesv1.SdkServer{