various refactorings

.github/workflows/e2e.yml

@@ -0,0 +1,46 @@
+# This is a basic workflow to help you get started with Actions
+
+name: End to end tests
+
+# Controls when the workflow will run
+on:
+  # Triggers the workflow on push or pull request events but only for the main branch
+  push:
+    branches: [main]
+  pull_request:
+    branches: [main]
+
+  # Allows you to run this workflow manually from the Actions tab
+  workflow_dispatch:
+    inputs:
+        debug_enabled:
+          description: 'Run the build with tmate debugging enabled (https://github.com/marketplace/actions/debugging-with-tmate)'     
+          required: false
+          default: "false"
+
+# A workflow run is made up of one or more jobs that can run sequentially or in parallel
+jobs:
+  # This workflow contains a single job called "build"
+  build:
+    # The type of runner that the job will run on
+    runs-on: ubuntu-latest
+    # Steps represent a sequence of tasks that will be executed as part of the job
+    steps:
+      # Checks-out your repository under $GITHUB_WORKSPACE, so your job can access it
+      - uses: actions/checkout@v2
+      - name: Setup Go environment
+        uses: actions/setup-go@v2.1.3
+        with:
+          # The Go version to download (if necessary) and use. Supports semver spec and ranges.
+          go-version: 1.18.x
+      - name: build Docker images
+        run: make builddockerlocal
+      - name: install kind binaries
+        run: make installkind
+      - name: create kind cluster
+        run: make createkindcluster
+      - name: run end-to-end tests
+        run: make e2elocal
+      - name: Setup tmate session # instructions on https://github.com/marketplace/actions/debugging-with-tmate
+        if: ${{ failure() && github.event_name == 'workflow_dispatch' && github.event.inputs.debug_enabled == 'true' }}
+        uses: mxschmitt/action-tmate@v3

.github/workflows/main.yml → .github/workflows/unit-tests.yml

@@ -1,6 +1,4 @@
-# This is a basic workflow to help you get started with Actions
-
-name: CI
+name: Unit tests
 
 # Controls when the workflow will run
 on:
@@ -12,12 +10,7 @@ on:
 
   # Allows you to run this workflow manually from the Actions tab
   workflow_dispatch:
-    inputs:
-        debug_enabled:
-          description: 'Run the build with tmate debugging enabled (https://github.com/marketplace/actions/debugging-with-tmate)'     
-          required: false
-          default: "false"
-
+
 # A workflow run is made up of one or more jobs that can run sequentially or in parallel
 jobs:
   # This workflow contains a single job called "build"
@@ -59,15 +52,4 @@ jobs:
       - name: latency server server unit tests
         run: cd cmd/latencyserver && go test -race
       - name: operator unit tests
-        run: IMAGE_NAME_SAMPLE=thundernetes-netcore IMAGE_NAME_INIT_CONTAINER=thundernetes-initcontainer TAG=$(git rev-list HEAD --max-count=1 --abbrev-commit) make -C pkg/operator test
-      - name: build Docker images
-        run: make builddockerlocal
-      - name: install kind binaries
-        run: make installkind
-      - name: create kind cluster
-        run: make createkindcluster
-      - name: run end-to-end tests
-        run: make e2elocal
-      - name: Setup tmate session # instructions on https://github.com/marketplace/actions/debugging-with-tmate
-        if: ${{ failure() && github.event_name == 'workflow_dispatch' && github.event.inputs.debug_enabled == 'true' }}
-        uses: mxschmitt/action-tmate@v3
+        run: IMAGE_NAME_SAMPLE=thundernetes-netcore IMAGE_NAME_INIT_CONTAINER=thundernetes-initcontainer TAG=$(git rev-list HEAD --max-count=1 --abbrev-commit) make -C pkg/operator test

cmd/e2e/utilities_test.go

@@ -505,7 +505,7 @@ func verifyGameServerDetail(ctx context.Context, kubeClient client.Client, gameS
 		return fmt.Errorf("expected %d connected players, got %d", expectedConnectedPlayersCount, gameServerDetail.Spec.ConnectedPlayersCount)
 	}
 	if len(gameServerDetail.Spec.ConnectedPlayers) != len(expectedConnectedPlayers) {
-		return fmt.Errorf("expected %d connected players, got %d", len(expectedConnectedPlayers), len(gameServerDetail.Spec.ConnectedPlayers))
+		return fmt.Errorf("expected %d connected players' IDs, got %d", len(expectedConnectedPlayers), len(gameServerDetail.Spec.ConnectedPlayers))
 	}
 
 	for i := 0; i < len(gameServerDetail.Spec.ConnectedPlayers); i++ {

cmd/initcontainer/Dockerfile

@@ -1,5 +1,4 @@
 # build stage
-# build stage
 ARG revision
 FROM thundernetes-src:$revision as builder
 

cmd/initcontainer/main.go

@@ -115,9 +115,9 @@ func main() {
 // parseBuildMetadata parses the build metadata from the corresponding environment variable
 func parseBuildMetadata() map[string]string {
 	buildMetadata := make(map[string]string)
-	if os.Getenv("PF_GAMESERVER_BUILD_METADATA") != "" {
-		metadata := os.Getenv("PF_GAMESERVER_BUILD_METADATA")
-		s := strings.Split(metadata, "?")
+	envMetadata := os.Getenv("PF_GAMESERVER_BUILD_METADATA")
+	if envMetadata != "" {
+		s := strings.Split(envMetadata, "?")
 		for _, s2 := range s {
 			if s2 == "" {
 				continue

go.mod

@@ -3,6 +3,7 @@ module github.com/playfab/thundernetes
 go 1.18
 
 require (
+	github.com/caarlos0/env/v6 v6.9.3
 	github.com/gin-gonic/gin v1.7.4
 	github.com/go-logr/logr v1.2.0
 	github.com/google/uuid v1.2.0

go.sum

@@ -88,6 +88,8 @@ github.com/bgentry/speakeasy v0.1.0/go.mod h1:+zsyZBPWlz7T6j88CTgSN5bM796AkVf0kB
 github.com/bketelsen/crypt v0.0.3-0.20200106085610-5cbc8cc4026c/go.mod h1:MKsuJmJgSg28kpZDP6UIiPt0e0Oz0kqKNGyRaWEPv84=
 github.com/blang/semver/v4 v4.0.0/go.mod h1:IbckMUScFkM3pff0VJDNKRiT6TG/YpiHIM2yvyW5YoQ=
 github.com/boombuler/barcode v1.0.0/go.mod h1:paBWMcWSl3LHKBqUq+rly7CNSldXjb2rDl3JlRe0mD8=
+github.com/caarlos0/env/v6 v6.9.3 h1:Tyg69hoVXDnpO5Qvpsu8EoquarbPyQb+YwExWHP8wWU=
+github.com/caarlos0/env/v6 v6.9.3/go.mod h1:hvp/ryKXKipEkcuYjs9mI4bBCg+UI0Yhgm5Zu0ddvwc=
 github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU=
 github.com/certifi/gocertifi v0.0.0-20191021191039-0944d244cd40/go.mod h1:sGbDF6GwGcLpkNXPUTkMRoywsNa/ol15pxFe6ERfguA=
 github.com/certifi/gocertifi v0.0.0-20200922220541-2c3bb06c6054/go.mod h1:sGbDF6GwGcLpkNXPUTkMRoywsNa/ol15pxFe6ERfguA=

pkg/config/config.go

@@ -14,7 +14,7 @@ const (
 	ExpandENVParam  = "config-expand-env"
 )
 
-// Parses the configFile and expandENV params via separate flag set as these
+// ParseConfigFileAndEnvParams parses the configFile and expandENV params via separate flag set as these
 // parameters need to be processed in order to load the config file
 // AND THEN process any remaining CLI params to override values.
 func ParseConfigFileAndEnvParams(args []string) (string, bool) {
@@ -36,7 +36,7 @@ func ParseConfigFileAndEnvParams(args []string) (string, bool) {
 	return configFile, expandENV
 }
 
-// Replaces ${var} or $var to the values of the current environment variables.
+// ExpandEnvInConfigFile replaces ${var} or $var to the values of the current environment variables.
 // Default values can be specified with ${var:default} format
 func ExpandEnvInConfigFile(config []byte) []byte {
 	return []byte(os.Expand(string(config), func(envName string) string {

pkg/operator/controllers/allocation_api_server.go

@@ -137,6 +137,7 @@ func (s *AllocationApiServer) Start(ctx context.Context) error {
 }
 
 // setupIndexers sets up the necessary indexers for the GameServer objects
+// specifically, these indexers will allow us to get gameservers by status.sessionID and by spec.BuildID
 func (s *AllocationApiServer) setupIndexers(mgr ctrl.Manager) error {
 	if err := mgr.GetFieldIndexer().IndexField(context.Background(), &mpsv1alpha1.GameServer{}, statusSessionId, func(rawObj client.Object) []string {
 		gs := rawObj.(*mpsv1alpha1.GameServer)
@@ -155,7 +156,7 @@ func (s *AllocationApiServer) setupIndexers(mgr ctrl.Manager) error {
 	return nil
 }
 
-// SetupWithManager sets up the allocation api controller with the manager
+// SetupWithManager sets up the allocation API controller with the manager
 func (s *AllocationApiServer) SetupWithManager(mgr ctrl.Manager) error {
 	err := s.setupIndexers(mgr)
 	if err != nil {
@@ -180,11 +181,11 @@ func (s *AllocationApiServer) Reconcile(ctx context.Context, req ctrl.Request) (
 	var gs mpsv1alpha1.GameServer
 	if err := s.Client.Get(ctx, req.NamespacedName, &gs); err != nil {
 		if apierrors.IsNotFound(err) {
-			log.Info("Unable to fetch GameServer, it was deleted - deleting from queue")
+			log.Info("Unable to fetch GameServer, it was deleted - deleting from queue", "namespace", req.Namespace, "name", req.Name)
 			s.gameServerQueue.RemoveFromQueue(req.Namespace, req.Name)
 			return ctrl.Result{}, nil
 		}
-		log.Error(err, "unable to fetch GameServer")
+		log.Error(err, "unable to fetch GameServer", "namespace", req.Namespace, "name", req.Name)
 		return ctrl.Result{}, err
 	}
 	// we only put a GameServer to the queue if it has reached the StandingBy state
@@ -239,14 +240,13 @@ func (s *AllocationApiServer) handleAllocationRequest(w http.ResponseWriter, r *
 	if err != nil {
 		if apierrors.IsNotFound(err) {
 			notFoundError(w, s.logger, err, "not found")
-			return
 		} else {
 			internalServerError(w, s.logger, err, "error listing")
-			return
 		}
+		return
 	}
 	if len(gameServerBuilds.Items) == 0 {
-		notFoundError(w, s.logger, errors.New("build not found"), fmt.Sprintf("Build with ID %s not found", args.BuildID))
+		notFoundError(w, s.logger, errors.New("GameServerBuild not found"), fmt.Sprintf("GameServerBuild with ID %s not found", args.BuildID))
 		return
 	}
 
@@ -283,7 +283,7 @@ func (s *AllocationApiServer) handleAllocationRequest(w http.ResponseWriter, r *
 	// allocation using the heap
 	for i := 0; i < allocationTries; i++ {
 		if i > 0 {
-			s.logger.Info("allocation retrying", "buildID", args.BuildID, "retry count", i, "sessionID", args.SessionID)
+			s.logger.Info("retrying allocation", "buildID", args.BuildID, "retry count", i, "sessionID", args.SessionID)
 		}
 		gs := s.gameServerQueue.PopFromQueue(args.BuildID)
 		if gs == nil {
@@ -315,11 +315,11 @@ func (s *AllocationApiServer) handleAllocationRequest(w http.ResponseWriter, r *
 		err = s.Client.Status().Patch(ctx, &gs2, patch)
 		if err != nil {
 			if apierrors.IsConflict(err) {
-				s.logger.Info("error conflict patching game server", "error", err, "sessionID", args.SessionID, "buildID", args.BuildID, "retry", i)
+				s.logger.Info("conflict error patching game server", "error", err, "sessionID", args.SessionID, "buildID", args.BuildID, "retry", i)
 			} else if apierrors.IsNotFound(err) {
 				s.logger.Info("error not found patching game server", "error", err, "sessionID", args.SessionID, "buildID", args.BuildID, "retry", i)
 			} else {
-				s.logger.Error(err, "error patching game server", "sessionID", args.SessionID, "buildID", args.BuildID, "retry", i)
+				s.logger.Error(err, "uknown error patching game server", "sessionID", args.SessionID, "buildID", args.BuildID, "retry", i)
 			}
 			// in case of any error, trigger a reconciliation for this GameServer object
 			// so it's re-added to the queue
@@ -349,7 +349,7 @@ func (s *AllocationApiServer) handleAllocationRequest(w http.ResponseWriter, r *
 	// if we reach this point, it means that we have tried multiple times and failed
 	// we return the last error, if possible
 	if err == nil {
-		err = errors.New("unknown error")
+		err = errors.New("unknown error, exceeded the maximum number of retries")
 	}
 	s.logger.Info("Error allocating", "sessionID", args.SessionID, "buildID", args.BuildID, "error", err)
 	internalServerError(w, s.logger, err, "error allocating")

pkg/operator/controllers/allocation_api_server_test.go

@@ -15,15 +15,15 @@ import (
 	mpsv1alpha1 "github.com/playfab/thundernetes/pkg/operator/api/v1alpha1"
 )
 
-const (
-	buildName1     string = "testbuild"
-	buildNamespace string = "default"
-	buildID1       string = "acb84898-cf73-46e2-8057-314ac557d85d"
-	sessionID1     string = "d5f075a4-517b-4bf4-8123-dfa0021aa169"
-	gsName         string = "testgs"
-)
-
 var _ = Describe("allocation API service input validation tests", func() {
+	const (
+		buildName1     string = "testbuild"
+		buildNamespace string = "default"
+		buildID1       string = "acb84898-cf73-46e2-8057-314ac557d85d"
+		sessionID1     string = "d5f075a4-517b-4bf4-8123-dfa0021aa169"
+		gsName         string = "testgs"
+	)
+
 	It("empty body should return error", func() {
 		req := httptest.NewRequest(http.MethodPost, "/api/v1/allocate", nil)
 		w := httptest.NewRecorder()
@@ -121,6 +121,14 @@ var _ = Describe("allocation API service input validation tests", func() {
 
 var _ = Describe("allocation API service queue tests", func() {
 	ctx := context.Background()
+	const (
+		buildName1     string = "testbuild"
+		buildNamespace string = "default"
+		buildID1       string = "acb84898-cf73-46e2-8057-314ac557d85d"
+		sessionID1     string = "d5f075a4-517b-4bf4-8123-dfa0021aa169"
+		gsName         string = "testgs"
+	)
+
 	It("should update queue properly during allocations", func() {
 		// create a GameServerBuild with 2 standingBy servers
 		gsb := testGenerateGameServerBuild(buildName1, buildNamespace, buildID1, 2, 4, false)

pkg/operator/controllers/controller_utils.go

@@ -3,15 +3,12 @@ package controllers
 import (
 	"bytes"
 	"context"
-	"errors"
 	"fmt"
 	"math/rand"
-	"os"
 	"strconv"
 	"strings"
 	"time"
 
-	"github.com/go-logr/logr"
 	mpsv1alpha1 "github.com/playfab/thundernetes/pkg/operator/api/v1alpha1"
 	corev1 "k8s.io/api/core/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -34,17 +31,20 @@ const (
 
 	LabelBuildID          = "BuildID"
 	LabelBuildName        = "BuildName"
-	LabelOwningGameServer = "OwningGameServer"
-	LabelOwningOperator   = "OwningOperator"
+	LabelOwningGameServer = "mps.playfab.com/OwningGameServer"
+	LabelOwningOperator   = "mps.playfab.com/OwningOperator"
 	LabelNodeName         = "NodeName"
 
-	GsdkConfigFile                = DataVolumeMountPath + "/Config/gsdkConfig.json"
-	LogDirectory                  = DataVolumeMountPath + "/GameLogs/"
-	CertificatesDirectory         = DataVolumeMountPath + "/GameCertificates"
+	GsdkConfigFile    = DataVolumeMountPath + "/Config/gsdkConfig.json"
+	GsdkConfigFileWin = DataVolumeMountPathWin + "\\Config\\gsdkConfig.json"
+
+	LogDirectory    = DataVolumeMountPath + "/GameLogs/"
+	LogDirectoryWin = DataVolumeMountPathWin + "\\GameLogs\\"
+
+	CertificatesDirectory    = DataVolumeMountPath + "/GameCertificates"
+	CertificatesDirectoryWin = DataVolumeMountPathWin + "\\GameCertificates"
+
 	GameSharedContentDirectory    = DataVolumeMountPath + "/GameSharedContent"
-	GsdkConfigFileWin             = DataVolumeMountPathWin + "\\Config\\gsdkConfig.json"
-	LogDirectoryWin               = DataVolumeMountPathWin + "\\GameLogs\\"
-	CertificatesDirectoryWin      = DataVolumeMountPathWin + "\\GameCertificates"
 	GameSharedContentDirectoryWin = DataVolumeMountPathWin + "\\GameSharedContent"
 
 	DaemonSetPort int32 = 56001
@@ -459,19 +459,3 @@ func getValueByState(gs *mpsv1alpha1.GameServer) int {
 		return 3
 	}
 }
-
-// GetInitContainerImages returns the init container images from the environment variables
-func GetInitContainerImages(l logr.Logger) (string, string) {
-	initContainerImageLinux := os.Getenv("THUNDERNETES_INIT_CONTAINER_IMAGE")
-	if initContainerImageLinux == "" {
-		l.Error(errors.New("THUNDERNETES_INIT_CONTAINER_IMAGE is not set, setting to a mock value"), "")
-		initContainerImageLinux = "testInitContainerImage"
-	}
-	initContainerImageWin := os.Getenv("THUNDERNETES_INIT_CONTAINER_IMAGE_WIN")
-	if initContainerImageWin == "" {
-		l.Error(errors.New("THUNDERNETES_INIT_CONTAINER_IMAGE_WIN is not set, setting to a mock value"), "")
-		initContainerImageWin = "testInitContainerImageWin"
-	}
-	l.Info("init container images", "linux", initContainerImageLinux, "win", initContainerImageWin)
-	return initContainerImageLinux, initContainerImageWin
-}

pkg/operator/controllers/gameserver_controller.go

@@ -33,6 +33,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/log"
+	"sigs.k8s.io/controller-runtime/pkg/manager"
 
 	mpsv1alpha1 "github.com/playfab/thundernetes/pkg/operator/api/v1alpha1"
 )
@@ -58,8 +59,23 @@ type GameServerReconciler struct {
 	GetNodeDetailsProvider  func(ctx context.Context, r client.Reader, nodeName string) (string, string, int, error) // we abstract this for testing purposes
 }
 
-// we request secret RBAC access here so they can be potentially used by the allocation API service (for GameServer allocations)
+func NewGameServerReconciler(mgr manager.Manager,
+	portRegistry *PortRegistry,
+	getNodeDetailsProvider func(ctx context.Context, r client.Reader, nodeName string) (string, string, int, error),
+	initContainerImageLinux string,
+	initContainerImageWin string) *GameServerReconciler {
+	return &GameServerReconciler{
+		Client:                  mgr.GetClient(),
+		Scheme:                  mgr.GetScheme(),
+		PortRegistry:            portRegistry,
+		Recorder:                mgr.GetEventRecorderFor("GameServer"),
+		GetNodeDetailsProvider:  getNodeDetailsProvider,
+		InitContainerImageLinux: initContainerImageLinux,
+		InitContainerImageWin:   initContainerImageWin,
+	}
+}
 
+// we request secret RBAC access here so they can be potentially used by the allocation API service (for GameServer allocations)
 // the gameserverapi uses the same manager role, so we need to add get, list and watch for gameserverdetails
 
 //+kubebuilder:rbac:groups=mps.playfab.com,resources=gameservers,verbs=get;list;watch;create;update;patch;delete