Added double write for both metapatch and metadata

googleforgames · May 7, 2021 · 5b95961 · 5b95961
1 parent 1270ba9
commit 5b95961
Show file tree

Hide file tree

Showing 7 changed files with 51 additions and 33 deletions.
diff --git a/cmd/allocator/main.go b/cmd/allocator/main.go
@@ -412,11 +412,7 @@ type serviceHandler struct {
 // Allocate implements the Allocate gRPC method definition
 func (h *serviceHandler) Allocate(ctx context.Context, in *pb.AllocationRequest) (*pb.AllocationResponse, error) {
 	logger.WithField("request", in).Infof("allocation request received.")
-	gsa, err := converters.ConvertAllocationRequestToGSA(in)
-	if err != nil {
-		logger.WithField("request", in).WithError(err).Info("Failed to convert request to GSA")
-		return nil, err
-	}
+	gsa := converters.ConvertAllocationRequestToGSA(in)
 	resultObj, err := h.allocationCallback(gsa)
 	if err != nil {
 		logger.WithField("gsa", gsa).WithError(err).Info("allocation failed")

diff --git a/pkg/allocation/converters/converter.go b/pkg/allocation/converters/converter.go
@@ -16,8 +16,6 @@
 package converters
 
 import (
-	"errors"
-
 	pb "agones.dev/agones/pkg/allocation/go"
 	"agones.dev/agones/pkg/apis"
 	agonesv1 "agones.dev/agones/pkg/apis/agones/v1"
@@ -28,9 +26,9 @@ import (
 )
 
 // ConvertAllocationRequestToGSA converts AllocationRequest to GameServerAllocation V1 (GSA)
-func ConvertAllocationRequestToGSA(in *pb.AllocationRequest) (*allocationv1.GameServerAllocation, error) {
+func ConvertAllocationRequestToGSA(in *pb.AllocationRequest) *allocationv1.GameServerAllocation {
 	if in == nil {
-		return nil, nil
+		return nil
 	}
 
 	gsa := &allocationv1.GameServerAllocation{
@@ -56,9 +54,6 @@ func ConvertAllocationRequestToGSA(in *pb.AllocationRequest) (*allocationv1.Game
 	metadata := in.GetMetadata()
 	if metadata == nil {
 		metadata = in.GetMetaPatch()
-	} else if in.GetMetaPatch() != nil {
-		// If both are set, reject the call.
-		return nil, errors.New("Allocation request cannot have both metadata and metapatch fields set")
 	}
 
 	if metadata != nil {
@@ -71,7 +66,7 @@ func ConvertAllocationRequestToGSA(in *pb.AllocationRequest) (*allocationv1.Game
 	if ls := convertLabelSelectorToInternalLabelSelector(in.GetRequiredGameServerSelector()); ls != nil {
 		gsa.Spec.Required = *ls
 	}
-	return gsa, nil
+	return gsa
 }
 
 // ConvertGSAToAllocationRequest converts AllocationRequest to GameServerAllocation V1 (GSA)
@@ -92,6 +87,13 @@ func ConvertGSAToAllocationRequest(in *allocationv1.GameServerAllocation) *pb.Al
 			Labels:      in.Spec.MetaPatch.Labels,
 			Annotations: in.Spec.MetaPatch.Annotations,
 		},
+		// MetaPatch is deprecated, but we do a double write here to both metapatch and metadata
+		// to ensure that multi-cluster allocation still works when one cluster has the field
+		// and another one does not have the field yet.
+		MetaPatch: &pb.MetaPatch{
+			Labels:      in.Spec.MetaPatch.Labels,
+			Annotations: in.Spec.MetaPatch.Annotations,
+		},
 	}
 
 	if in.Spec.MultiClusterSetting.Enabled {

diff --git a/pkg/allocation/converters/converter_test.go b/pkg/allocation/converters/converter_test.go
@@ -15,7 +15,6 @@
 package converters
 
 import (
-	"errors"
 	"testing"
 
 	pb "agones.dev/agones/pkg/allocation/go"
@@ -34,7 +33,6 @@ func TestConvertAllocationRequestToGameServerAllocation(t *testing.T) {
 		in                 *pb.AllocationRequest
 		want               *allocationv1.GameServerAllocation
 		skipConvertFromGSA bool
-		wantError          error
 	}{
 		{
 			name: "all fields are set",
@@ -71,6 +69,11 @@ func TestConvertAllocationRequestToGameServerAllocation(t *testing.T) {
 						"i": "j",
 					},
 				},
+				MetaPatch: &pb.MetaPatch{
+					Labels: map[string]string{
+						"i": "j",
+					},
+				},
 			},
 			want: &allocationv1.GameServerAllocation{
 				ObjectMeta: metav1.ObjectMeta{
@@ -182,7 +185,7 @@ func TestConvertAllocationRequestToGameServerAllocation(t *testing.T) {
 			skipConvertFromGSA: true,
 		},
 		{
-			name: "Rejects both metadata and metapatch field",
+			name: "Prefers metadata over metapatch field",
 			in: &pb.AllocationRequest{
 				Namespace:                    "",
 				MultiClusterSetting:          &pb.MultiClusterSetting{},
@@ -200,18 +203,31 @@ func TestConvertAllocationRequestToGameServerAllocation(t *testing.T) {
 					},
 				},
 			},
-			want:               nil,
+			want: &allocationv1.GameServerAllocation{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "",
+				},
+				Spec: allocationv1.GameServerAllocationSpec{
+					MultiClusterSetting: allocationv1.MultiClusterSetting{
+						Enabled: false,
+					},
+					Scheduling: apis.Distributed,
+					MetaPatch: allocationv1.MetaPatch{
+						Labels: map[string]string{
+							"a": "b",
+						},
+					},
+				},
+			},
 			skipConvertFromGSA: true,
-			wantError:          errors.New("Allocation request cannot have both metadata and metapatch fields set"),
 		},
 	}
 	for _, tc := range tests {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
 
-			out, err := ConvertAllocationRequestToGSA(tc.in)
-			assert.Equal(t, tc.wantError, err, "\"%s\" error: got %v, want %v", tc.name, err, tc.wantError)
+			out := ConvertAllocationRequestToGSA(tc.in)
 			assert.Equal(t, tc.want, out, "mismatch with want after conversion: \"%s\"", tc.name)
 
 			if !tc.skipConvertFromGSA {
@@ -247,6 +263,7 @@ func TestConvertGSAToAllocationRequestEmpty(t *testing.T) {
 				RequiredGameServerSelector: &pb.LabelSelector{},
 				Scheduling:                 pb.AllocationRequest_Distributed,
 				Metadata:                   &pb.MetaPatch{},
+				MetaPatch:                  &pb.MetaPatch{},
 			},
 		}, {
 			name: "empty object",
@@ -259,6 +276,7 @@ func TestConvertGSAToAllocationRequestEmpty(t *testing.T) {
 				MultiClusterSetting:        &pb.MultiClusterSetting{},
 				RequiredGameServerSelector: &pb.LabelSelector{},
 				Metadata:                   &pb.MetaPatch{},
+				MetaPatch:                  &pb.MetaPatch{},
 			},
 		},
 	}

diff --git a/pkg/allocation/go/allocation.pb.go b/pkg/allocation/go/allocation.pb.go
diff --git a/pkg/allocation/go/allocation.swagger.json b/pkg/allocation/go/allocation.swagger.json
@@ -91,7 +91,7 @@
         },
         "metaPatch": {
           "$ref": "#/definitions/allocationMetaPatch",
-          "description": "Deprecated: Please use metadata instead."
+          "title": "Deprecated: Please use metadata instead. This field is ignored if the\nmetadata field is set"
         },
         "metadata": {
           "$ref": "#/definitions/allocationMetaPatch",

diff --git a/proto/allocation/allocation.proto b/proto/allocation/allocation.proto
@@ -48,7 +48,8 @@ message AllocationRequest {
     Distributed = 1;
   }
 
-  // Deprecated: Please use metadata instead.
+  // Deprecated: Please use metadata instead. This field is ignored if the
+  // metadata field is set
   MetaPatch metaPatch = 6;
 
   // Metadata is optional custom metadata that is added to the game server at

diff --git a/site/content/en/docs/Advanced/allocator-service.md b/site/content/en/docs/Advanced/allocator-service.md
@@ -174,7 +174,7 @@ You should expect to see the following output:
 ### Sending Data to the Game Server
 
 {{% feature publishVersion="0.15.0" %}}
-The service accepts a `metadata` field, which can be used to apply `labels` and `annotations` to the allocated `GameServer`.
+The service accepts a `metadata` field, which can be used to apply `labels` and `annotations` to the allocated `GameServer`. The old `metaPatch` fields is now deprecated, but can still be used for compatibility. If both `metadata` and `metaPatch` fields are set, `metaPatch` is ignored.
 {{% /feature %}}
 {{% feature expiryVersion="0.15.0" %}}
 The service accepts a `metaPatch` field, which can be used to apply `labels` and `annotations` to the allocated `GameServer`.