Validate the .scheduling field in the fleet and gameserverset

Adds a cloud product hook specific to the scheduling field, for use in the fleet / gameserverset APIs. This might seem narrowly scoped, but this field is the only one that propagates down outside the template. Modifies one e2e to test that Autopilot rejects appropriately. Adds several skips to other tests. Towards #2777
googleforgames · Jan 20, 2023 · 5bff3a5 · 5bff3a5
1 parent 18bb127
commit 5bff3a5
Show file tree

Hide file tree

Showing 7 changed files with 43 additions and 17 deletions.
diff --git a/pkg/apis/agones/v1/apihooks.go b/pkg/apis/agones/v1/apihooks.go
@@ -15,6 +15,7 @@
 package v1
 
 import (
+	"agones.dev/agones/pkg/apis"
 	"agones.dev/agones/pkg/util/runtime"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
@@ -28,6 +29,9 @@ type APIHooks interface {
 	// ValidateGameServerSpec is called by GameServer.Validate to allow for product specific validation.
 	ValidateGameServerSpec(*GameServerSpec) []metav1.StatusCause
 
+	// ValidateScheduling is called by Fleet and GameServerSet Validate() to allow for product specific validation of scheduling strategy.
+	ValidateScheduling(apis.SchedulingStrategy) []metav1.StatusCause
+
 	// MutateGameServerPodSpec is called by createGameServerPod to allow for product specific pod mutation.
 	MutateGameServerPodSpec(*GameServerSpec, *corev1.PodSpec) error
 
@@ -48,8 +52,9 @@ func RegisterAPIHooks(hooks APIHooks) {
 
 type generic struct{}
 
-func (generic) ValidateGameServerSpec(*GameServerSpec) []metav1.StatusCause    { return nil }
-func (generic) MutateGameServerPodSpec(*GameServerSpec, *corev1.PodSpec) error { return nil }
+func (generic) ValidateGameServerSpec(*GameServerSpec) []metav1.StatusCause     { return nil }
+func (generic) ValidateScheduling(apis.SchedulingStrategy) []metav1.StatusCause { return nil }
+func (generic) MutateGameServerPodSpec(*GameServerSpec, *corev1.PodSpec) error  { return nil }
 
 // SetEviction sets disruptions controls based on GameServer.Status.Eviction.
 func (generic) SetEviction(safe EvictionSafe, pod *corev1.Pod) error {

diff --git a/pkg/apis/agones/v1/fleet.go b/pkg/apis/agones/v1/fleet.go
@@ -190,6 +190,9 @@ func (f *Fleet) Validate() ([]metav1.StatusCause, bool) {
 	if len(gsCauses) > 0 {
 		causes = append(causes, gsCauses...)
 	}
+	if productCauses := apiHooks.ValidateScheduling(f.Spec.Scheduling); len(productCauses) > 0 {
+		causes = append(causes, productCauses...)
+	}
 	objMetaCauses := validateObjectMeta(&f.Spec.Template.ObjectMeta)
 	if len(objMetaCauses) > 0 {
 		causes = append(causes, objMetaCauses...)

diff --git a/pkg/apis/agones/v1/gameserverset.go b/pkg/apis/agones/v1/gameserverset.go
@@ -106,7 +106,9 @@ func (gsSet *GameServerSet) Validate() ([]metav1.StatusCause, bool) {
 	if len(gsCauses) > 0 {
 		causes = append(causes, gsCauses...)
 	}
-
+	if productCauses := apiHooks.ValidateScheduling(gsSet.Spec.Scheduling); len(productCauses) > 0 {
+		causes = append(causes, productCauses...)
+	}
 	objMetaCauses := validateObjectMeta(&gsSet.Spec.Template.ObjectMeta)
 	if len(objMetaCauses) > 0 {
 		causes = append(causes, objMetaCauses...)

diff --git a/pkg/cloudproduct/gke/gke.go b/pkg/cloudproduct/gke/gke.go
@@ -112,8 +112,8 @@ func (*gkeAutopilot) NewPortAllocator(minPort, maxPort int32,
 
 func (*gkeAutopilot) WaitOnFreePorts() bool { return true }
 
-func (*gkeAutopilot) ValidateGameServerSpec(gss *agonesv1.GameServerSpec) []metav1.StatusCause {
-	var causes []metav1.StatusCause
+func (g *gkeAutopilot) ValidateGameServerSpec(gss *agonesv1.GameServerSpec) []metav1.StatusCause {
+	causes := g.ValidateScheduling(gss.Scheduling)
 	for _, p := range gss.Ports {
 		if p.PortPolicy != agonesv1.Dynamic {
 			causes = append(causes, metav1.StatusCause{
@@ -123,13 +123,6 @@ func (*gkeAutopilot) ValidateGameServerSpec(gss *agonesv1.GameServerSpec) []meta
 			})
 		}
 	}
-	if gss.Scheduling != apis.Packed {
-		causes = append(causes, metav1.StatusCause{
-			Type:    metav1.CauseTypeFieldValueInvalid,
-			Field:   "scheduling",
-			Message: errSchedulingMustBePacked,
-		})
-	}
 	// See SetEviction comment below for why we block EvictionSafeOnUpgrade.
 	if gss.Eviction.Safe == agonesv1.EvictionSafeOnUpgrade {
 		causes = append(causes, metav1.StatusCause{
@@ -141,6 +134,17 @@ func (*gkeAutopilot) ValidateGameServerSpec(gss *agonesv1.GameServerSpec) []meta
 	return causes
 }
 
+func (*gkeAutopilot) ValidateScheduling(ss apis.SchedulingStrategy) []metav1.StatusCause {
+	if ss != apis.Packed {
+		return []metav1.StatusCause{{
+			Type:    metav1.CauseTypeFieldValueInvalid,
+			Field:   "scheduling",
+			Message: errSchedulingMustBePacked,
+		}}
+	}
+	return nil
+}
+
 func (*gkeAutopilot) MutateGameServerPodSpec(gss *agonesv1.GameServerSpec, podSpec *corev1.PodSpec) error {
 	podSpecSeccompUnconfined(podSpec)
 	return nil

diff --git a/pkg/cloudproduct/gke/gke_test.go b/pkg/cloudproduct/gke/gke_test.go
@@ -133,18 +133,18 @@ func TestValidateGameServer(t *testing.T) {
 			want: []metav1.StatusCause{
 				{
 					Type:    "FieldValueInvalid",
-					Message: "portPolicy must be Dynamic on GKE Autopilot",
-					Field:   "bad-udp.portPolicy",
+					Message: "scheduling strategy must be Packed on GKE Autopilot",
+					Field:   "scheduling",
 				},
 				{
 					Type:    "FieldValueInvalid",
 					Message: "portPolicy must be Dynamic on GKE Autopilot",
-					Field:   "another-bad-udp.portPolicy",
+					Field:   "bad-udp.portPolicy",
 				},
 				{
 					Type:    "FieldValueInvalid",
-					Message: "scheduling strategy must be Packed on GKE Autopilot",
-					Field:   "scheduling",
+					Message: "portPolicy must be Dynamic on GKE Autopilot",
+					Field:   "another-bad-udp.portPolicy",
 				},
 				{
 					Type:    "FieldValueInvalid",

diff --git a/test/e2e/fleet_test.go b/test/e2e/fleet_test.go
@@ -1165,6 +1165,7 @@ func TestUpdateFleetScheduling(t *testing.T) {
 
 	t.Run("Updating Spec.Scheduling on fleet should be updated in GameServer",
 		func(t *testing.T) {
+			framework.SkipOnCloudProduct(t, "gke-autopilot", "Autopilot does not support Distributed scheduling")
 			client := framework.AgonesClient.AgonesV1()
 
 			flt := defaultFleet(framework.Namespace)

diff --git a/test/e2e/gameserverallocation_test.go b/test/e2e/gameserverallocation_test.go
@@ -51,6 +51,11 @@ func TestCreateFleetAndGameServerAllocate(t *testing.T) {
 			fleet := defaultFleet(framework.Namespace)
 			fleet.Spec.Scheduling = strategy
 			flt, err := fleets.Create(ctx, fleet, metav1.CreateOptions{})
+			if strategy != apis.Packed && framework.CloudProduct == "gke-autopilot" {
+				// test that Autopilot rejects anything but Packed and skip the rest of the test
+				assert.ErrorContains(t, err, "configuration is invalid")
+				return
+			}
 			if assert.NoError(t, err) {
 				defer fleets.Delete(ctx, flt.ObjectMeta.Name, metav1.DeleteOptions{}) // nolint:errcheck
 			}
@@ -268,6 +273,9 @@ func TestMultiClusterAllocationOnLocalCluster(t *testing.T) {
 	for _, strategy := range fixtures {
 		strategy := strategy
 		t.Run(string(strategy), func(t *testing.T) {
+			if strategy == apis.Distributed {
+				framework.SkipOnCloudProduct(t, "gke-autopilot", "Autopilot does not support Distributed scheduling")
+			}
 			t.Parallel()
 			ctx := context.Background()
 
@@ -405,6 +413,9 @@ func TestCreateFullFleetAndCantGameServerAllocate(t *testing.T) {
 		strategy := strategy
 
 		t.Run(string(strategy), func(t *testing.T) {
+			if strategy == apis.Distributed {
+				framework.SkipOnCloudProduct(t, "gke-autopilot", "Autopilot does not support Distributed scheduling")
+			}
 			t.Parallel()
 			ctx := context.Background()