From de7e157a8dc2ca59a7bc81a2884ad19da7b89b0e Mon Sep 17 00:00:00 2001
From: Mark Mandel <markmandel@google.com>
Date: Fri, 7 Feb 2020 18:20:48 -0800
Subject: [PATCH] Including feature flag locking.

Required to allow tests to pass with race checking enabled.
---
 pkg/apis/agones/v1/gameserver.go      |  3 +--
 pkg/apis/agones/v1/gameserver_test.go | 10 ++++------
 pkg/util/runtime/features.go          | 17 +++++++++++++++++
 pkg/util/runtime/features_test.go     |  3 +++
 4 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/pkg/apis/agones/v1/gameserver.go b/pkg/apis/agones/v1/gameserver.go
index e5012d9464..5001f4d4ab 100644
--- a/pkg/apis/agones/v1/gameserver.go
+++ b/pkg/apis/agones/v1/gameserver.go
@@ -19,11 +19,10 @@ import (
 	"fmt"
 	"net"
 
-	"agones.dev/agones/pkg/util/runtime"
-
 	"agones.dev/agones/pkg"
 	"agones.dev/agones/pkg/apis"
 	"agones.dev/agones/pkg/apis/agones"
+	"agones.dev/agones/pkg/util/runtime"
 	"github.com/mattbaird/jsonpatch"
 	"github.com/pkg/errors"
 	admregv1b "k8s.io/api/admissionregistration/v1beta1"
diff --git a/pkg/apis/agones/v1/gameserver_test.go b/pkg/apis/agones/v1/gameserver_test.go
index a62c3e7df1..5e99a28fcc 100644
--- a/pkg/apis/agones/v1/gameserver_test.go
+++ b/pkg/apis/agones/v1/gameserver_test.go
@@ -17,14 +17,12 @@ package v1
 import (
 	"fmt"
 	"strings"
-	"sync"
 	"testing"
 
-	"agones.dev/agones/pkg/util/runtime"
-
 	"agones.dev/agones/pkg"
 	"agones.dev/agones/pkg/apis"
 	"agones.dev/agones/pkg/apis/agones"
+	"agones.dev/agones/pkg/util/runtime"
 	"github.com/stretchr/testify/assert"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -328,13 +326,13 @@ func TestGameServerApplyDefaults(t *testing.T) {
 		},
 	}
 
+	runtime.FeatureTestMutex.Lock()
+	defer runtime.FeatureTestMutex.Unlock()
+
 	// otherwise the race condition detector is not happy.
-	mtx := sync.Mutex{}
 	for name, test := range data {
 		t.Run(name, func(t *testing.T) {
-			mtx.Lock()
 			err := runtime.ParseFeatures(test.featureFlags)
-			mtx.Unlock()
 			assert.NoError(t, err)
 
 			test.gameServer.ApplyDefaults()
diff --git a/pkg/util/runtime/features.go b/pkg/util/runtime/features.go
index 08e8bbf7b5..ed227515b1 100644
--- a/pkg/util/runtime/features.go
+++ b/pkg/util/runtime/features.go
@@ -17,6 +17,7 @@ package runtime
 import (
 	"net/url"
 	"strconv"
+	"sync"
 
 	"github.com/pkg/errors"
 	"github.com/spf13/pflag"
@@ -44,6 +45,14 @@ var (
 	// featureGates is the storage of what features are enabled
 	// or disabled.
 	featureGates map[Feature]bool
+
+	// featureMutex ensures that updates to featureGates don't happen at the same time as reads.
+	// this is mostly to protect tests which can change gates in parallel.
+	featureMutex = sync.RWMutex{}
+
+	// FeatureTestMutex is a mutex to be shared between tests to ensure that a test that involves changing featureGates
+	// cannot accidentally run at the same time as another test that also changing feature flags.
+	FeatureTestMutex sync.Mutex
 )
 
 // Feature is a type for defining feature gates.
@@ -70,6 +79,9 @@ func ParseFeaturesFromEnv() error {
 // ParseFeatures parses the url encoded query string of features and stores the value
 // for later retrieval
 func ParseFeatures(queryString string) error {
+	featureMutex.Lock()
+	defer featureMutex.Unlock()
+
 	features := map[Feature]bool{}
 	// copy the defaults into this map
 	for k, v := range featureDefaults {
@@ -101,12 +113,17 @@ func ParseFeatures(queryString string) error {
 
 // FeatureEnabled returns if a Feature is enabled or not
 func FeatureEnabled(feature Feature) bool {
+	featureMutex.RLock()
+	defer featureMutex.RUnlock()
 	return featureGates[feature]
 }
 
 // EncodeFeatures returns the feature set as a URL encoded query string
 func EncodeFeatures() string {
 	values := url.Values{}
+	featureMutex.RLock()
+	defer featureMutex.RUnlock()
+
 	for k, v := range featureGates {
 		values.Add(string(k), strconv.FormatBool(v))
 	}
diff --git a/pkg/util/runtime/features_test.go b/pkg/util/runtime/features_test.go
index 70c44c1dd4..25219887fc 100644
--- a/pkg/util/runtime/features_test.go
+++ b/pkg/util/runtime/features_test.go
@@ -26,6 +26,9 @@ import (
 
 func TestFeatures(t *testing.T) {
 	t.Parallel()
+	FeatureTestMutex.Lock()
+	defer FeatureTestMutex.Unlock()
+
 	// stable feature flag state
 	featureDefaults = map[Feature]bool{
 		FeatureExample:  true,