Fixes, more e2e tests and logging for multi-cluster allocation (#1077)

* create server TLS and client secrets for allocator service

* Add more logging to allocation, fixed allocation URL for remote allocation and more testing

cmd/allocator/main.go

@@ -186,14 +186,16 @@ func (h *httpHandler) allocateHandler(w http.ResponseWriter, r *http.Request) {
 	gsa := allocationv1.GameServerAllocation{}
 	if err := json.NewDecoder(r.Body).Decode(&gsa); err != nil {
 		http.Error(w, "invalid request", http.StatusBadRequest)
+		logger.WithError(err).Info("bad request")
 		return
 	}
+	logger.WithField("gsa", gsa).Infof("allocation request received")
 
 	allocation := h.agonesClient.AllocationV1().GameServerAllocations(gsa.ObjectMeta.Namespace)
 	allocatedGsa, err := allocation.Create(&gsa)
 	if err != nil {
 		http.Error(w, err.Error(), httpCode(err))
-		logger.Debug(err)
+		logger.WithField("gsa", gsa).WithError(err).Info("calling allocation extension API failed")
 		return
 	}
 	w.Header().Set("Content-Type", "application/json")

install/helm/agones/templates/crds/gameserverallocationpolicy.yaml

@@ -65,7 +65,6 @@ spec:
                   type: string
               required:
               - clusterName
-              - allocationEndpoints
               - secretName
               - namespace
               type: object

install/yaml/install.yaml

@@ -755,7 +755,6 @@ spec:
                   type: string
               required:
               - clusterName
-              - allocationEndpoints
               - secretName
               - namespace
               type: object

pkg/apis/multicluster/v1alpha1/gameserverallocationpolicy.go

@@ -34,10 +34,10 @@ type GameServerAllocationPolicySpec struct {
 type ClusterConnectionInfo struct {
 	// Optional: the name of the targeted cluster
 	ClusterName string `json:"clusterName"`
-	// The endpoints for the allocator service in the targeted cluster
-	// There should be at least one endpoint or the policy is ineffective
+	// The endpoints for the allocator service in the targeted cluster.
+	// If the AllocationEndpoints is not set, the allocation happens on local cluster.
 	// If there are multiple endpoints any of the endpoints that can handle allocation request should suffice
-	AllocationEndpoints []string `json:"allocationEndpoints"`
+	AllocationEndpoints []string `json:"allocationEndpoints,omitempty"`
 	// The name of the secret that contains TLS client certificates to connect the allocator server in the targeted cluster
 	SecretName string `json:"secretName"`
 	// The cluster namespace from which to allocate gameservers

pkg/gameserverallocations/allocator.go

@@ -68,6 +68,7 @@ const (
 	// from the topNGameServerCount of Ready gameservers
 	// to reduce the contention while allocating gameservers.
 	topNGameServerDefaultCount = 100
+	allocatorRequestURLFmt     = "https://%s/v1alpha1/gameserverallocation"
 )
 
 const (
@@ -177,6 +178,7 @@ func (c *Allocator) Allocate(gsa *allocationv1.GameServerAllocation, stop <-chan
 		if err != nil {
 			return nil, errors.Wrap(err, "could not find objectkinds for status")
 		}
+		c.loggerForGameServerAllocation(gsa).Info("GameServerAllocation is invalid")
 
 		status.TypeMeta = metav1.TypeMeta{Kind: gvks[0].Kind, APIVersion: gvks[0].Version}
 		return status, nil
@@ -192,6 +194,7 @@ func (c *Allocator) Allocate(gsa *allocationv1.GameServerAllocation, stop <-chan
 	}
 
 	if err != nil {
+		c.loggerForGameServerAllocation(gsa).WithError(err).Error("allocation failed")
 		return nil, err
 	}
 
@@ -216,6 +219,9 @@ func (c *Allocator) allocateFromLocalCluster(gsa *allocationv1.GameServerAllocat
 	err := Retry(allocationRetry, func() error {
 		var err error
 		gs, err = c.allocate(gsa, stop)
+		if err != nil {
+			c.loggerForGameServerAllocation(gsa).WithError(err).Warn("failed to allocate. Retrying... ")
+		}
 		return err
 	})
 
@@ -265,12 +271,22 @@ func (c *Allocator) applyMultiClusterAllocation(gsa *allocationv1.GameServerAllo
 		if connectionInfo == nil {
 			break
 		}
-		if connectionInfo.ClusterName == gsa.ObjectMeta.ClusterName {
-			result, err = c.allocateFromLocalCluster(gsa, stop)
-			c.baseLogger.Error(err)
+		if len(connectionInfo.AllocationEndpoints) == 0 {
+			// Change the naemspace to the policy namespace and allocate locally
+			gsaCopy := gsa
+			if gsa.Namespace != connectionInfo.Namespace {
+				gsaCopy = gsa.DeepCopy()
+				gsaCopy.Namespace = connectionInfo.Namespace
+			}
+			result, err = c.allocateFromLocalCluster(gsaCopy, stop)
+			if err != nil {
+				c.loggerForGameServerAllocation(gsaCopy).WithError(err).Error("self-allocation failed")
+			}
 		} else {
 			result, err = c.allocateFromRemoteCluster(*gsa, connectionInfo, gsa.ObjectMeta.Namespace)
-			c.baseLogger.Error(err)
+			if err != nil {
+				c.loggerForGameServerAllocation(gsa).WithField("allocConnInfo", connectionInfo).WithError(err).Error("remote-allocation failed")
+			}
 		}
 		if result != nil {
 			return result, nil
@@ -303,7 +319,9 @@ func (c *Allocator) allocateFromRemoteCluster(gsa allocationv1.GameServerAllocat
 
 	// TODO: Retry on transient error --> response.StatusCode >= 500
 	for i, endpoint := range connectionInfo.AllocationEndpoints {
-		response, err := client.Post(endpoint, "application/json", bytes.NewBuffer(body))
+		c.loggerForGameServerAllocation(&gsa).WithField("endpoint", endpoint).Info("forwarding allocation request")
+		requestURL := fmt.Sprintf(allocatorRequestURLFmt, endpoint)
+		response, err := client.Post(requestURL, "application/json", bytes.NewBuffer(body))
 		if err != nil {
 			return nil, err
 		}
@@ -317,7 +335,7 @@ func (c *Allocator) allocateFromRemoteCluster(gsa allocationv1.GameServerAllocat
 		// failing with 5xx http status, try the next endpoint. Otherwise, return the error response.
 		if response.StatusCode >= 500 && (i+1) < len(connectionInfo.AllocationEndpoints) {
 			// If there is a server error try a different endpoint
-			c.baseLogger.WithError(err).WithField("endpoint", endpoint).Warn("The request sent failed, trying next endpoint")
+			c.loggerForGameServerAllocation(&gsa).WithError(err).WithField("endpoint", endpoint).Warn("The request failed. Trying next endpoint")
 			continue
 		}
 		if response.StatusCode >= 400 {

pkg/gameserverallocations/controller_test.go

@@ -23,6 +23,7 @@ import (
 	"fmt"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"strconv"
 	"testing"
 	"time"
@@ -802,10 +803,9 @@ func TestMultiClusterAllocationFromLocal(t *testing.T) {
 							Priority: 1,
 							Weight:   200,
 							ConnectionInfo: multiclusterv1alpha1.ClusterConnectionInfo{
-								AllocationEndpoints: []string{"localhost"},
-								ClusterName:         "multicluster",
-								SecretName:          "localhostsecret",
-								Namespace:           "ns1",
+								ClusterName: "multicluster",
+								SecretName:  "localhostsecret",
+								Namespace:   defaultNs,
 							},
 						},
 						ObjectMeta: metav1.ObjectMeta{
@@ -831,9 +831,8 @@ func TestMultiClusterAllocationFromLocal(t *testing.T) {
 
 		gsa := &allocationv1.GameServerAllocation{
 			ObjectMeta: metav1.ObjectMeta{
-				Namespace:   defaultNs,
-				Name:        "alloc1",
-				ClusterName: "multicluster",
+				Namespace: defaultNs,
+				Name:      "alloc1",
 			},
 			Spec: allocationv1.GameServerAllocationSpec{
 				MultiClusterSetting: allocationv1.MultiClusterSetting{
@@ -851,6 +850,7 @@ func TestMultiClusterAllocationFromLocal(t *testing.T) {
 		ret, err := executeAllocation(gsa, c)
 		assert.NoError(t, err)
 		assert.Equal(t, gsa.Spec.Required, ret.Spec.Required)
+		assert.Equal(t, gsa.Namespace, ret.Namespace)
 		expectedState := allocationv1.GameServerAllocationAllocated
 		assert.True(t, expectedState == ret.Status.State, "Failed: %s vs %s", expectedState, ret.Status.State)
 	})
@@ -920,6 +920,7 @@ func TestMultiClusterAllocationFromRemote(t *testing.T) {
 			_, _ = w.Write(response)
 		}))
 		defer server.Close()
+		serverURL := parseURL(t, server.URL)
 
 		// Set client CA for server
 		certpool := x509.NewCertPool()
@@ -938,7 +939,7 @@ func TestMultiClusterAllocationFromRemote(t *testing.T) {
 							Priority: 1,
 							Weight:   200,
 							ConnectionInfo: multiclusterv1alpha1.ClusterConnectionInfo{
-								AllocationEndpoints: []string{server.URL, "non-existing"},
+								AllocationEndpoints: []string{serverURL.Host, "non-existing"},
 								ClusterName:         clusterName,
 								SecretName:          secretName,
 								Namespace:           targetedNamespace,
@@ -996,6 +997,7 @@ func TestMultiClusterAllocationFromRemote(t *testing.T) {
 			http.Error(w, "test error message", 500)
 		}))
 		defer server.Close()
+		serverURL := parseURL(t, server.URL)
 
 		// Set client CA for server
 		certpool := x509.NewCertPool()
@@ -1013,7 +1015,7 @@ func TestMultiClusterAllocationFromRemote(t *testing.T) {
 							Priority: 1,
 							Weight:   200,
 							ConnectionInfo: multiclusterv1alpha1.ClusterConnectionInfo{
-								AllocationEndpoints: []string{server.URL},
+								AllocationEndpoints: []string{serverURL.Host},
 								ClusterName:         clusterName,
 								SecretName:          secretName,
 							},
@@ -1069,6 +1071,7 @@ func TestMultiClusterAllocationFromRemote(t *testing.T) {
 			http.Error(w, "test error message", 500)
 		}))
 		defer unhealthyServer.Close()
+		unhealthyServerURL := parseURL(t, unhealthyServer.URL)
 
 		// Set client CA for unhealthy server
 		certpool := x509.NewCertPool()
@@ -1088,6 +1091,7 @@ func TestMultiClusterAllocationFromRemote(t *testing.T) {
 			_, _ = w.Write(response)
 		}))
 		defer healthyServer.Close()
+		healthyServerURL := parseURL(t, healthyServer.URL)
 		healthyServer.TLS = unhealthyServer.TLS
 
 		// Allocation policy reactor
@@ -1100,7 +1104,7 @@ func TestMultiClusterAllocationFromRemote(t *testing.T) {
 							Priority: 1,
 							Weight:   200,
 							ConnectionInfo: multiclusterv1alpha1.ClusterConnectionInfo{
-								AllocationEndpoints: []string{unhealthyServer.URL, healthyServer.URL},
+								AllocationEndpoints: []string{unhealthyServerURL.Host, healthyServerURL.Host},
 								ClusterName:         clusterName,
 								SecretName:          secretName,
 							},
@@ -1230,7 +1234,7 @@ func executeAllocation(gsa *allocationv1.GameServerAllocation, c *Controller) (*
 		return nil, err
 	}
 	rec := httptest.NewRecorder()
-	if err = c.processAllocationRequest(rec, r, defaultNs, stop); err != nil {
+	if err = c.processAllocationRequest(rec, r, gsa.Namespace, stop); err != nil {
 		return nil, err
 	}
 
@@ -1330,6 +1334,14 @@ func getPEMFromDER(der []byte) []byte {
 	return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der})
 }
 
+func parseURL(t *testing.T, rawURL string) *url.URL {
+	serverURL, err := url.Parse(rawURL)
+	if err != nil {
+		t.Fatalf("failed to parse URL %s: %s", rawURL, err)
+	}
+	return serverURL
+}
+
 var clientCert = []byte(`-----BEGIN CERTIFICATE-----
 MIIDuzCCAqOgAwIBAgIUduDWtqpUsp3rZhCEfUrzI05laVIwDQYJKoZIhvcNAQEL
 BQAwbTELMAkGA1UEBhMCR0IxDzANBgNVBAgMBkxvbmRvbjEPMA0GA1UEBwwGTG9u