Updates per reviewer comments

pkg/sdkserver/localsdk.go

@@ -34,8 +34,6 @@ import (
 	"github.com/mennanov/fmutils"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/proto"
 	"k8s.io/apimachinery/pkg/util/yaml"
 )
@@ -597,15 +595,15 @@ func (l *LocalSDKServer) GetCounter(ctx context.Context, in *alpha.GetCounterReq
 		return nil, errors.Errorf("%s not enabled", runtime.FeatureCountsAndLists)
 	}
 
-	l.logger.Info("Getting Counter")
+	l.logger.WithField("name", in.Name).Info("Getting Counter")
 	l.recordRequest("getcounter")
 	l.gsMutex.RLock()
 	defer l.gsMutex.RUnlock()
 
 	if counter, ok := l.gs.Status.Counters[in.Name]; ok {
 		return &alpha.Counter{Name: in.Name, Count: &counter.Count, Capacity: &counter.Capacity}, nil
 	}
-	return nil, status.Errorf(codes.NotFound, "%s Counter NOT_FOUND", in.Name)
+	return nil, errors.Errorf("NOT_FOUND. %s Counter not found", in.Name)
 }
 
 // UpdateCounter returns the updated Counter. Returns NOT_FOUND if the Counter does not exist.
@@ -617,14 +615,15 @@ func (l *LocalSDKServer) UpdateCounter(ctx context.Context, in *alpha.UpdateCoun
 		return nil, errors.Errorf("%s not enabled", runtime.FeatureCountsAndLists)
 	}
 
-	l.logger.Info("Update Counter")
+	l.logger.WithField("name", in.Counter.Name).Info("Updating Counter")
 	l.recordRequest("updatecounter")
 	l.gsMutex.RLock()
 	defer l.gsMutex.RUnlock()
 
 	name := in.Counter.Name
 	if counter, ok := l.gs.Status.Counters[name]; ok {
 		// Check if the UpdateMask paths are valid, return INVALID_ARGUMENT if not.
+		// TODO: https://google.aip.dev/134, "Update masks must support a special value *, meaning full replacement".
 		if in.UpdateMask.IsValid(counter.ProtoReflect().Interface()) {
 			// Create *alpha.Counter from *sdk.GameServer_Status_CounterStatus for merging.
 			tmpCounter := &alpha.Counter{Name: name, Count: &counter.Count, Capacity: &counter.Capacity}
@@ -637,9 +636,9 @@ func (l *LocalSDKServer) UpdateCounter(ctx context.Context, in *alpha.UpdateCoun
 			l.gs.Status.Counters[name].Capacity = *tmpCounter.Capacity
 			return &alpha.Counter{Name: name, Count: &l.gs.Status.Counters[name].Count, Capacity: &l.gs.Status.Counters[name].Capacity}, nil
 		}
-		return nil, status.Errorf(codes.InvalidArgument, "Field Mask Path(s) %v are invalid for Counter. Use valid %v", in.UpdateMask.GetPaths(), counter.ProtoReflect().Descriptor().Fields())
+		return nil, errors.Errorf("INVALID_ARGUMENT. Field Mask Path(s) %v are invalid for Counter. Use valid %v", in.UpdateMask.GetPaths(), counter.ProtoReflect().Descriptor().Fields())
 	}
-	return nil, status.Errorf(codes.NotFound, "%s Counter NOT_FOUND", name)
+	return nil, errors.Errorf("NOT_FOUND. %s Counter not found", name)
 }
 
 // Close tears down all the things

pkg/sdkserver/localsdk_test.go

@@ -30,6 +30,7 @@ import (
 	"agones.dev/agones/pkg/util/runtime"
 	"github.com/google/go-cmp/cmp"
 	"github.com/stretchr/testify/assert"
+	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/testing/protocmp"
 	"google.golang.org/protobuf/types/known/fieldmaskpb"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -669,9 +670,9 @@ func TestLocalSDKServerUpdateCounter(t *testing.T) {
 	defer runtime.FeatureTestMutex.Unlock()
 	assert.NoError(t, runtime.ParseFeatures(string(runtime.FeatureCountsAndLists)+"=true"))
 
-	name := "sessions"
+	counterName := "sessions"
 	counters := map[string]agonesv1.CounterStatus{
-		name: {Count: 1, Capacity: 100},
+		counterName: {Count: 1, Capacity: 100},
 	}
 	fixture := &agonesv1.GameServer{
 		ObjectMeta: metav1.ObjectMeta{Name: "stuff"},
@@ -703,33 +704,29 @@ func TestLocalSDKServerUpdateCounter(t *testing.T) {
 	})
 	assert.NoError(t, err)
 
-	ONE := int64(1)
-	HUNDRED := int64(100)
-	notupdated := &alpha.Counter{Name: name, Count: &ONE, Capacity: &HUNDRED}
-	startingCounter, err := l.GetCounter(context.Background(), &alpha.GetCounterRequest{Name: "sessions"})
-	assert.NoError(t, err)
-	if diff := cmp.Diff(notupdated, startingCounter, protocmp.Transform()); diff != "" {
-		t.Errorf("unexpected difference:\n%v", diff)
+	// Check UpdateCounter only updates fields in the FieldMask
+	got, err := l.UpdateCounter(context.Background(), &alpha.UpdateCounterRequest{
+		Counter: &alpha.Counter{
+			Name:     counterName,
+			Count:    proto.Int64(9),
+			Capacity: proto.Int64(999),
+		},
+		UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"count"}},
+	})
+	want := &alpha.Counter{
+		Name:     counterName,
+		Count:    proto.Int64(9),
+		Capacity: proto.Int64(100),
 	}
-
-	count := int64(9)
-	capacity := int64(999)
-	// Note: the field mask filter in UpdateCounter will remove fields from the request object that
-	// are not included in the FieldMask Paths. Here `Name` and `Capacity` will be removed from
-	// the `updated` object after we call `l.UpdateCounter()`.
-	updated := &alpha.Counter{Name: name, Count: &count, Capacity: &capacity}
-	fm := fieldmaskpb.FieldMask{Paths: []string{"count"}}
-	expected := &alpha.Counter{Name: name, Count: &count, Capacity: &HUNDRED}
-	counterRequest := alpha.UpdateCounterRequest{Counter: updated, UpdateMask: &fm}
-	counter, err := l.UpdateCounter(context.Background(), &counterRequest)
 	assert.NoError(t, err)
-	if diff := cmp.Diff(expected, counter, protocmp.Transform()); diff != "" {
+	if diff := cmp.Diff(want, got, protocmp.Transform()); diff != "" {
 		t.Errorf("unexpected difference:\n%v", diff)
 	}
 
-	endingCounter, err := l.GetCounter(context.Background(), &alpha.GetCounterRequest{Name: "sessions"})
+	// Confirm updated Counter has been properly written to the GameServer
+	got, err = l.GetCounter(context.Background(), &alpha.GetCounterRequest{Name: counterName})
 	assert.NoError(t, err)
-	if diff := cmp.Diff(expected, endingCounter, protocmp.Transform()); diff != "" {
+	if diff := cmp.Diff(want, got, protocmp.Transform()); diff != "" {
 		t.Errorf("unexpected difference:\n%v", diff)
 	}
 }

pkg/sdkserver/sdk.go

@@ -18,6 +18,7 @@ import (
 	agonesv1 "agones.dev/agones/pkg/apis/agones/v1"
 	"agones.dev/agones/pkg/sdk"
 	"agones.dev/agones/pkg/util/runtime"
+	"google.golang.org/protobuf/proto"
 )
 
 const (
@@ -81,8 +82,7 @@ func convert(gs *agonesv1.GameServer) *sdk.GameServer {
 		if gs.Status.Counters != nil {
 			counters := make(map[string]*sdk.GameServer_Status_CounterStatus, len(gs.Status.Counters))
 			for key, counter := range gs.Status.Counters {
-				tmpCounter := counter.DeepCopy()
-				counters[key] = &sdk.GameServer_Status_CounterStatus{Count: tmpCounter.Count, Capacity: tmpCounter.Capacity}
+				counters[key] = &sdk.GameServer_Status_CounterStatus{Count: *proto.Int64(counter.Count), Capacity: *proto.Int64(counter.Capacity)}
 			}
 			result.Status.Counters = counters
 		}