Add validation of the fleet underlying gameserver (#771)

Now we perform additional validation on the Gameserver specification.
For #765.

pkg/apis/stable/v1alpha1/fleet.go

@@ -153,6 +153,14 @@ func (f *Fleet) Validate() ([]metav1.StatusCause, bool) {
 		})
 	}
 
+	// check gameserver specification in a fleet
+	gsSpec := f.Spec.Template.Spec
+	gsSpec.ApplyDefaults()
+	gsCauses, ok := gsSpec.Validate("")
+	if !ok {
+		causes = append(causes, gsCauses...)
+	}
+
 	return causes, len(causes) == 0
 }
 

pkg/apis/stable/v1alpha1/fleet_test.go

@@ -100,8 +100,30 @@ func TestSumStatusAllocatedReplicas(t *testing.T) {
 	assert.Equal(t, int32(5), SumStatusAllocatedReplicas([]*GameServerSet{gsSet1, gsSet2}))
 }
 
+func TestFleetGameserverSpec(t *testing.T) {
+	f := defaultFleet()
+	f.Spec.Template.Spec.Template =
+		corev1.PodTemplateSpec{
+			Spec: corev1.PodSpec{
+				Containers: []corev1.Container{{Name: "container", Image: "myimage"}, {Name: "container2", Image: "myimage"}},
+			},
+		}
+	causes, ok := f.Validate()
+
+	assert.False(t, ok)
+	assert.Len(t, causes, 2)
+	assert.Equal(t, "container", causes[0].Field)
+
+	f.Spec.Template.Spec.Container = "testing"
+	causes, ok = f.Validate()
+
+	assert.False(t, ok)
+	assert.Len(t, causes, 1)
+	assert.Equal(t, "Could not find a container named testing", causes[0].Message)
+}
+
 func TestFleetName(t *testing.T) {
-	f := Fleet{}
+	f := defaultFleet()
 
 	nameLen := validation.LabelValueMaxLength + 1
 	bytes := make([]byte, nameLen)
@@ -130,3 +152,20 @@ func TestSumStatusReplicas(t *testing.T) {
 
 	assert.Equal(t, int32(30), SumStatusReplicas(fixture))
 }
+
+func defaultFleet() *Fleet {
+	gs := GameServer{
+		Spec: GameServerSpec{
+			Template: corev1.PodTemplateSpec{
+				Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "testing", Image: "testing/image"}}}}},
+	}
+	return &Fleet{
+		ObjectMeta: metav1.ObjectMeta{GenerateName: "simple-fleet-", Namespace: "defaultNs"},
+		Spec: FleetSpec{
+			Replicas: 2,
+			Template: GameServerTemplateSpec{
+				Spec: gs.Spec,
+			},
+		},
+	}
+}

pkg/apis/stable/v1alpha1/gameserver.go

@@ -187,31 +187,36 @@ type GameServerStatusPort struct {
 func (gs *GameServer) ApplyDefaults() {
 	gs.ObjectMeta.Finalizers = append(gs.ObjectMeta.Finalizers, stable.GroupName)
 
-	gs.applyContainerDefaults()
-	gs.applyPortDefaults()
+	gs.Spec.ApplyDefaults()
 	gs.applyStateDefaults()
-	gs.applyHealthDefaults()
-	gs.applySchedulingDefaults()
+}
+
+// ApplyDefaults applies default values to the GameServerSpec if they are not already populated
+func (gss *GameServerSpec) ApplyDefaults() {
+	gss.applyContainerDefaults()
+	gss.applyPortDefaults()
+	gss.applyHealthDefaults()
+	gss.applySchedulingDefaults()
 }
 
 // applyContainerDefaults applues the container defaults
-func (gs *GameServer) applyContainerDefaults() {
-	if len(gs.Spec.Template.Spec.Containers) == 1 {
-		gs.Spec.Container = gs.Spec.Template.Spec.Containers[0].Name
+func (gss *GameServerSpec) applyContainerDefaults() {
+	if len(gss.Template.Spec.Containers) == 1 {
+		gss.Container = gss.Template.Spec.Containers[0].Name
 	}
 }
 
 // applyHealthDefaults applies health checking defaults
-func (gs *GameServer) applyHealthDefaults() {
-	if !gs.Spec.Health.Disabled {
-		if gs.Spec.Health.PeriodSeconds <= 0 {
-			gs.Spec.Health.PeriodSeconds = 5
+func (gss *GameServerSpec) applyHealthDefaults() {
+	if !gss.Health.Disabled {
+		if gss.Health.PeriodSeconds <= 0 {
+			gss.Health.PeriodSeconds = 5
 		}
-		if gs.Spec.Health.FailureThreshold <= 0 {
-			gs.Spec.Health.FailureThreshold = 3
+		if gss.Health.FailureThreshold <= 0 {
+			gss.Health.FailureThreshold = 3
 		}
-		if gs.Spec.Health.InitialDelaySeconds <= 0 {
-			gs.Spec.Health.InitialDelaySeconds = 5
+		if gss.Health.InitialDelaySeconds <= 0 {
+			gss.Health.InitialDelaySeconds = 5
 		}
 	}
 }
@@ -220,50 +225,40 @@ func (gs *GameServer) applyHealthDefaults() {
 func (gs *GameServer) applyStateDefaults() {
 	if gs.Status.State == "" {
 		gs.Status.State = GameServerStateCreating
+		// applyStateDefaults() should be called after applyPortDefaults()
 		if gs.HasPortPolicy(Dynamic) {
 			gs.Status.State = GameServerStatePortAllocation
 		}
 	}
 }
 
 // applyPortDefaults applies default values for all ports
-func (gs *GameServer) applyPortDefaults() {
-	for i, p := range gs.Spec.Ports {
+func (gss *GameServerSpec) applyPortDefaults() {
+	for i, p := range gss.Ports {
 		// basic spec
 		if p.PortPolicy == "" {
-			gs.Spec.Ports[i].PortPolicy = Dynamic
+			gss.Ports[i].PortPolicy = Dynamic
 		}
 
 		if p.Protocol == "" {
-			gs.Spec.Ports[i].Protocol = "UDP"
+			gss.Ports[i].Protocol = "UDP"
 		}
 	}
 }
 
-func (gs *GameServer) applySchedulingDefaults() {
-	if gs.Spec.Scheduling == "" {
-		gs.Spec.Scheduling = apis.Packed
+func (gss *GameServerSpec) applySchedulingDefaults() {
+	if gss.Scheduling == "" {
+		gss.Scheduling = apis.Packed
 	}
 }
 
-// Validate validates the GameServer configuration.
-// If a GameServer is invalid there will be > 0 values in
+// Validate validates the GameServerSpec configuration.
+// devAddress is a specific IP address used for local Gameservers, for fleets "" is used
+// If a GameServer Spec is invalid there will be > 0 values in
 // the returned array
-func (gs *GameServer) Validate() ([]metav1.StatusCause, bool) {
+func (gss GameServerSpec) Validate(devAddress string) ([]metav1.StatusCause, bool) {
 	var causes []metav1.StatusCause
-
-	// Long GS name would produce an invalid Label for Pod
-	if len(gs.Name) > validation.LabelValueMaxLength {
-		causes = append(causes, metav1.StatusCause{
-			Type:    metav1.CauseTypeFieldValueInvalid,
-			Field:   fmt.Sprintf("Name"),
-			Message: fmt.Sprintf("Length of Gameserver '%s' name should be no more than 63 characters.", gs.ObjectMeta.Name),
-		})
-	}
-
-	// make sure the host port is specified if this is a development server
-	devAddress, hasDevAddress := gs.GetDevAddress()
-	if hasDevAddress {
+	if devAddress != "" {
 		// verify that the value is a valid IP address.
 		if net.ParseIP(devAddress) == nil {
 			causes = append(causes, metav1.StatusCause{
@@ -273,7 +268,7 @@ func (gs *GameServer) Validate() ([]metav1.StatusCause, bool) {
 			})
 		}
 
-		for _, p := range gs.Spec.Ports {
+		for _, p := range gss.Ports {
 			if p.HostPort == 0 {
 				causes = append(causes, metav1.StatusCause{
 					Type:    metav1.CauseTypeFieldValueRequired,
@@ -291,7 +286,7 @@ func (gs *GameServer) Validate() ([]metav1.StatusCause, bool) {
 		}
 	} else {
 		// make sure a name is specified when there is multiple containers in the pod.
-		if len(gs.Spec.Container) == 0 && len(gs.Spec.Template.Spec.Containers) > 1 {
+		if len(gss.Container) == 0 && len(gss.Template.Spec.Containers) > 1 {
 			causes = append(causes, metav1.StatusCause{
 				Type:    metav1.CauseTypeFieldValueInvalid,
 				Field:   "container",
@@ -300,7 +295,7 @@ func (gs *GameServer) Validate() ([]metav1.StatusCause, bool) {
 		}
 
 		// no host port when using dynamic PortPolicy
-		for _, p := range gs.Spec.Ports {
+		for _, p := range gss.Ports {
 			if p.HostPort > 0 && p.PortPolicy == Dynamic {
 				causes = append(causes, metav1.StatusCause{
 					Type:    metav1.CauseTypeFieldValueInvalid,
@@ -311,7 +306,7 @@ func (gs *GameServer) Validate() ([]metav1.StatusCause, bool) {
 		}
 
 		// make sure the container value points to a valid container
-		_, _, err := gs.FindGameServerContainer()
+		_, _, err := gss.FindGameServerContainer()
 		if err != nil {
 			causes = append(causes, metav1.StatusCause{
 				Type:    metav1.CauseTypeFieldValueInvalid,
@@ -320,7 +315,29 @@ func (gs *GameServer) Validate() ([]metav1.StatusCause, bool) {
 			})
 		}
 	}
+	return causes, len(causes) == 0
+
+}
+
+// Validate validates the GameServer configuration.
+// If a GameServer is invalid there will be > 0 values in
+// the returned array
+func (gs *GameServer) Validate() ([]metav1.StatusCause, bool) {
+	var causes []metav1.StatusCause
+
+	// Long GS name would produce an invalid Label for Pod
+	if len(gs.Name) > validation.LabelValueMaxLength {
+		causes = append(causes, metav1.StatusCause{
+			Type:    metav1.CauseTypeFieldValueInvalid,
+			Field:   fmt.Sprintf("Name"),
+			Message: fmt.Sprintf("Length of Gameserver '%s' name should be no more than 63 characters.", gs.ObjectMeta.Name),
+		})
+	}
 
+	// make sure the host port is specified if this is a development server
+	devAddress, _ := gs.GetDevAddress()
+	gssCauses, _ := gs.Spec.Validate(devAddress)
+	causes = append(causes, gssCauses...)
 	return causes, len(causes) == 0
 }
 
@@ -341,16 +358,23 @@ func (gs *GameServer) IsBeingDeleted() bool {
 }
 
 // FindGameServerContainer returns the container that is specified in
-// spec.gameServer.container. Returns the index and the value.
+// gameServer.Spec.Container. Returns the index and the value.
 // Returns an error if not found
-func (gs *GameServer) FindGameServerContainer() (int, corev1.Container, error) {
-	for i, c := range gs.Spec.Template.Spec.Containers {
-		if c.Name == gs.Spec.Container {
+func (gss *GameServerSpec) FindGameServerContainer() (int, corev1.Container, error) {
+	for i, c := range gss.Template.Spec.Containers {
+		if c.Name == gss.Container {
 			return i, c, nil
 		}
 	}
 
-	return -1, corev1.Container{}, errors.Errorf("Could not find a container named %s", gs.Spec.Container)
+	return -1, corev1.Container{}, errors.Errorf("Could not find a container named %s", gss.Container)
+}
+
+// FindGameServerContainer returns the container that is specified in
+// gameServer.Spec.Container. Returns the index and the value.
+// Returns an error if not found
+func (gs *GameServer) FindGameServerContainer() (int, corev1.Container, error) {
+	return gs.Spec.FindGameServerContainer()
 }
 
 // ApplyToPodGameServerContainer applies func(v1.Container) to the pod's gameserver container

test/e2e/fleet_test.go

@@ -494,8 +494,72 @@ func TestFleetAllocationDuringGameServerDeletion(t *testing.T) {
 	})
 }
 
+// TestFleetGSSpecValidation is built to test Fleet's underlying Gameserver template
+// validation. Gameserver Spec contained in a Fleet should be valid to create a fleet.
+func TestFleetGSSpecValidation(t *testing.T) {
+	t.Parallel()
+	alpha1 := framework.AgonesClient.StableV1alpha1()
+
+	// check two Containers in Gameserver Spec Template validation
+	flt := defaultFleet()
+	containerName := "container2"
+	flt.Spec.Template.Spec.Template =
+		corev1.PodTemplateSpec{
+			Spec: corev1.PodSpec{
+				Containers: []corev1.Container{{Name: "container", Image: "myImage"}, {Name: containerName, Image: "myImage2"}},
+			},
+		}
+	flt.Spec.Template.Spec.Container = "testing"
+	_, err := alpha1.Fleets(defaultNs).Create(flt)
+	assert.NotNil(t, err)
+	statusErr, ok := err.(*k8serrors.StatusError)
+	assert.True(t, ok)
+	assert.Len(t, statusErr.Status().Details.Causes, 1)
+	assert.Equal(t, metav1.CauseTypeFieldValueInvalid, statusErr.Status().Details.Causes[0].Type)
+	assert.Equal(t, "Could not find a container named testing", statusErr.Status().Details.Causes[0].Message)
+
+	flt.Spec.Template.Spec.Container = ""
+	_, err = alpha1.Fleets(defaultNs).Create(flt)
+	assert.NotNil(t, err)
+	statusErr, ok = err.(*k8serrors.StatusError)
+	assert.True(t, ok)
+	assert.Len(t, statusErr.Status().Details.Causes, 2)
+	CausesMessages := []string{"Container is required when using multiple containers in the pod template", "Could not find a container named "}
+	assert.Equal(t, metav1.CauseTypeFieldValueInvalid, statusErr.Status().Details.Causes[0].Type)
+	assert.Contains(t, CausesMessages, statusErr.Status().Details.Causes[0].Message)
+	assert.Equal(t, metav1.CauseTypeFieldValueInvalid, statusErr.Status().Details.Causes[1].Type)
+	assert.Contains(t, CausesMessages, statusErr.Status().Details.Causes[1].Message)
+
+	// use valid name for a container, one of two defined above
+	flt.Spec.Template.Spec.Container = containerName
+	_, err = alpha1.Fleets(defaultNs).Create(flt)
+	if assert.Nil(t, err) {
+		defer alpha1.Fleets(defaultNs).Delete(flt.ObjectMeta.Name, nil) // nolint:errcheck
+	}
+
+	// check port configuration validation
+	fltPort := defaultFleet()
+
+	fltPort.Spec.Template.Spec.Ports = []v1alpha1.GameServerPort{{Name: "Dyn", HostPort: 5555, PortPolicy: v1alpha1.Dynamic, ContainerPort: 5555}}
+
+	_, err = alpha1.Fleets(defaultNs).Create(fltPort)
+	assert.NotNil(t, err)
+	statusErr, ok = err.(*k8serrors.StatusError)
+	assert.True(t, ok)
+	assert.Len(t, statusErr.Status().Details.Causes, 1)
+	assert.Equal(t, "HostPort cannot be specified with a Dynamic PortPolicy", statusErr.Status().Details.Causes[0].Message)
+
+	fltPort.Spec.Template.Spec.Ports[0].PortPolicy = v1alpha1.Static
+	fltPort.Spec.Template.Spec.Ports[0].HostPort = 0
+	fltPort.Spec.Template.Spec.Ports[0].ContainerPort = 5555
+	_, err = alpha1.Fleets(defaultNs).Create(fltPort)
+	if assert.Nil(t, err) {
+		defer alpha1.Fleets(defaultNs).Delete(fltPort.ObjectMeta.Name, nil) // nolint:errcheck
+	}
+}
+
 // TestFleetNameValidation is built to test Fleet Name length validation,
-// Fleet Name should have at most 63 chars
+// Fleet Name should have at most 63 chars.
 func TestFleetNameValidation(t *testing.T) {
 	t.Parallel()
 	alpha1 := framework.AgonesClient.StableV1alpha1()
@@ -512,14 +576,13 @@ func TestFleetNameValidation(t *testing.T) {
 	statusErr, ok := err.(*k8serrors.StatusError)
 	assert.True(t, ok)
 	assert.True(t, len(statusErr.Status().Details.Causes) > 0)
-	assert.Equal(t, statusErr.Status().Details.Causes[0].Type, metav1.CauseTypeFieldValueInvalid)
+	assert.Equal(t, metav1.CauseTypeFieldValueInvalid, statusErr.Status().Details.Causes[0].Type)
 	goodFlt := defaultFleet()
 	goodFlt.Name = string(bytes[0 : nameLen-1])
 	goodFlt, err = alpha1.Fleets(defaultNs).Create(goodFlt)
 	if assert.Nil(t, err) {
 		defer alpha1.Fleets(defaultNs).Delete(goodFlt.ObjectMeta.Name, nil) // nolint:errcheck
 	}
-
 }
 
 func assertSuccessOrUpdateConflict(t *testing.T, err error) {

test/e2e/fleetautoscaler_test.go

@@ -104,7 +104,7 @@ func TestAutoscalerBasicFunctions(t *testing.T) {
 
 	// check that we are able to scale
 	framework.WaitForFleetAutoScalerCondition(t, fas, func(fas *v1alpha1.FleetAutoscaler) bool {
-		return fas.Status.ScalingLimited == false
+		return !fas.Status.ScalingLimited
 	})
 
 	// patch autoscaler to a maxReplicas count equal to current replicas count
@@ -113,7 +113,7 @@ func TestAutoscalerBasicFunctions(t *testing.T) {
 
 	// check that we are not able to scale
 	framework.WaitForFleetAutoScalerCondition(t, fas, func(fas *v1alpha1.FleetAutoscaler) bool {
-		return fas.Status.ScalingLimited == true
+		return fas.Status.ScalingLimited
 	})
 
 	// delete the allocated GameServer and watch the fleet scale down