diff --git a/pkg/apis/stable/v1alpha1/fleet.go b/pkg/apis/stable/v1alpha1/fleet.go index c5feac8e5a..12cd6f82bc 100644 --- a/pkg/apis/stable/v1alpha1/fleet.go +++ b/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 } diff --git a/pkg/apis/stable/v1alpha1/fleet_test.go b/pkg/apis/stable/v1alpha1/fleet_test.go index 107a9ddb92..36c2ce25c8 100644 --- a/pkg/apis/stable/v1alpha1/fleet_test.go +++ b/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, + }, + }, + } +} diff --git a/pkg/apis/stable/v1alpha1/gameserver.go b/pkg/apis/stable/v1alpha1/gameserver.go index 7265d6827e..8cfdbe987e 100644 --- a/pkg/apis/stable/v1alpha1/gameserver.go +++ b/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,6 +225,7 @@ 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 } @@ -227,43 +233,32 @@ func (gs *GameServer) applyStateDefaults() { } // 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 diff --git a/test/e2e/fleet_test.go b/test/e2e/fleet_test.go index d31716edf9..6ff17e9f75 100644 --- a/test/e2e/fleet_test.go +++ b/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) { diff --git a/test/e2e/fleetautoscaler_test.go b/test/e2e/fleetautoscaler_test.go index 32fd114224..9d98831dc4 100644 --- a/test/e2e/fleetautoscaler_test.go +++ b/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