diff --git a/pkg/apis/agones/v1/common.go b/pkg/apis/agones/v1/common.go index fab9e1c957..82f463fb68 100644 --- a/pkg/apis/agones/v1/common.go +++ b/pkg/apis/agones/v1/common.go @@ -28,7 +28,7 @@ import ( // Block of const Error messages const ( ErrContainerRequired = "Container is required when using multiple containers in the pod template" - ErrHostPortDynamic = "HostPort cannot be specified with a Dynamic PortPolicy" + ErrHostPort = "HostPort cannot be specified with a Dynamic or Passthrough PortPolicy" ErrPortPolicyStatic = "PortPolicy must be Static" ErrContainerPortRequired = "ContainerPort must be defined for Dynamic and Static PortPolicies" ErrContainerPortPassthrough = "ContainerPort cannot be specified with Passthrough PortPolicy" diff --git a/pkg/apis/agones/v1/gameserver.go b/pkg/apis/agones/v1/gameserver.go index 002d8b872b..ba97fae70e 100644 --- a/pkg/apis/agones/v1/gameserver.go +++ b/pkg/apis/agones/v1/gameserver.go @@ -360,18 +360,30 @@ func (gss *GameServerSpec) Validate(devAddress string) ([]metav1.StatusCause, bo causes = append(causes, metav1.StatusCause{ Type: metav1.CauseTypeFieldValueNotSupported, Field: "players", - Message: fmt.Sprintf("Value cannot be set unless feature flag %s is enabled,", runtime.FeaturePlayerTracking), + Message: fmt.Sprintf("Value cannot be set unless feature flag %s is enabled", runtime.FeaturePlayerTracking), }) } } + if !runtime.FeatureEnabled(runtime.FeatureContainerPortAllocation) { + for _, p := range gss.Ports { + if p.Container != nil { + causes = append(causes, metav1.StatusCause{ + Type: metav1.CauseTypeFieldValueNotSupported, + Field: fmt.Sprintf("%s.container", p.Name), + Message: fmt.Sprintf("Value cannot be set unless feature flag %s is enabled", runtime.FeatureContainerPortAllocation), + }) + } + } + } + if devAddress != "" { // verify that the value is a valid IP address. if net.ParseIP(devAddress) == nil { causes = append(causes, metav1.StatusCause{ Type: metav1.CauseTypeFieldValueInvalid, Field: fmt.Sprintf("annotations.%s", DevAddressAnnotation), - Message: fmt.Sprintf("Value '%s' of annotation '%s' must be a valid IP address.", DevAddressAnnotation, devAddress), + Message: fmt.Sprintf("Value '%s' of annotation '%s' must be a valid IP address", devAddress, DevAddressAnnotation), }) } @@ -380,7 +392,7 @@ func (gss *GameServerSpec) Validate(devAddress string) ([]metav1.StatusCause, bo causes = append(causes, metav1.StatusCause{ Type: metav1.CauseTypeFieldValueRequired, Field: fmt.Sprintf("%s.hostPort", p.Name), - Message: fmt.Sprintf("HostPort is required if GameServer is annotated with %s", DevAddressAnnotation), + Message: fmt.Sprintf("HostPort is required if GameServer is annotated with '%s'", DevAddressAnnotation), }) } if p.PortPolicy != Static { @@ -435,11 +447,11 @@ func (gss *GameServerSpec) Validate(devAddress string) ([]metav1.StatusCause, bo causes = append(causes, metav1.StatusCause{ Type: metav1.CauseTypeFieldValueInvalid, Field: fmt.Sprintf("%s.hostPort", p.Name), - Message: ErrHostPortDynamic, + Message: ErrHostPort, }) } - if p.Container != nil && gss.Container != "" { + if p.Container != nil && gss.Container != "" && runtime.FeatureEnabled(runtime.FeatureContainerPortAllocation) { _, _, err := gss.FindContainer(*p.Container) if err != nil { causes = append(causes, metav1.StatusCause{ @@ -572,22 +584,13 @@ func (gs *GameServer) FindGameServerContainer() (int, corev1.Container, error) { // ApplyToPodContainer applies func(v1.Container) to the specified container in the pod. // Returns an error if the container is not found. func (gs *GameServer) ApplyToPodContainer(pod *corev1.Pod, containerName string, f func(corev1.Container) corev1.Container) error { - var container corev1.Container - containerIndex := -1 for i, c := range pod.Spec.Containers { if c.Name == containerName { - container = c - containerIndex = i + pod.Spec.Containers[i] = f(c) + return nil } } - if containerIndex == -1 { - return errors.Errorf("failed to find container named %q in pod spec", containerName) - } - - container = f(container) - pod.Spec.Containers[containerIndex] = container - - return nil + return errors.Errorf("failed to find container named %s in pod spec", containerName) } // Pod creates a new Pod from the PodTemplateSpec diff --git a/pkg/apis/agones/v1/gameserver_test.go b/pkg/apis/agones/v1/gameserver_test.go index f90be83c14..4f203b4713 100644 --- a/pkg/apis/agones/v1/gameserver_test.go +++ b/pkg/apis/agones/v1/gameserver_test.go @@ -18,6 +18,7 @@ import ( "fmt" "strings" "testing" + "time" "agones.dev/agones/pkg" "agones.dev/agones/pkg/apis" @@ -25,6 +26,7 @@ import ( "agones.dev/agones/pkg/util/runtime" "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/validation" ) @@ -33,6 +35,73 @@ const ( ipFixture = "127.1.1.1" ) +func TestStatus(t *testing.T) { + name := "test-name" + port := int32(7788) + p := GameServerPort{Name: name, HostPort: port} + + res := p.Status() + assert.Equal(t, name, res.Name) + assert.Equal(t, port, res.Port) +} + +func TestIsBeingDeleted(t *testing.T) { + deletionTimestamp := metav1.Date(2009, 11, 17, 20, 34, 58, 651387237, time.UTC) + var testCases = []struct { + description string + gs *GameServer + expected bool + }{ + { + description: "ready gs, is not being deleted", + gs: &GameServer{ + ObjectMeta: metav1.ObjectMeta{ + DeletionTimestamp: nil, + }, + Status: GameServerStatus{State: GameServerStateReady}, + }, + expected: false, + }, + { + description: "DeletionTimestamp is set, gs is being deleted", + gs: &GameServer{ + ObjectMeta: metav1.ObjectMeta{ + DeletionTimestamp: &deletionTimestamp, + }, + Status: GameServerStatus{State: GameServerStateReady}, + }, + expected: true, + }, + { + description: "gs status is GameServerStateShutdown, gs is being deleted", + gs: &GameServer{ + ObjectMeta: metav1.ObjectMeta{ + DeletionTimestamp: nil, + }, + Status: GameServerStatus{State: GameServerStateShutdown}, + }, + expected: true, + }, + { + description: "gs status is GameServerStateShutdown and DeletionTimestamp is set, gs is being deleted", + gs: &GameServer{ + ObjectMeta: metav1.ObjectMeta{ + DeletionTimestamp: &deletionTimestamp, + }, + Status: GameServerStatus{State: GameServerStateShutdown}, + }, + expected: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + result := tc.gs.IsBeingDeleted() + assert.Equal(t, tc.expected, result) + }) + } +} + func TestGameServerFindGameServerContainer(t *testing.T) { t.Parallel() @@ -357,186 +426,759 @@ func TestGameServerApplyDefaults(t *testing.T) { } } +// nolint:dupl func TestGameServerValidate(t *testing.T) { - gs := GameServer{ - Spec: GameServerSpec{ - Template: corev1.PodTemplateSpec{ - Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "testing", Image: "testing/image"}}}}}, - } - gs.ApplyDefaults() - causes, ok := gs.Validate() - assert.True(t, ok) - assert.Empty(t, causes) + t.Parallel() - containerWithPort := "anotherTest" - gs = GameServer{ - Spec: GameServerSpec{ - Container: "", - Ports: []GameServerPort{{ - Name: "main", - HostPort: 5001, - PortPolicy: Dynamic, - }, { - Name: "sidecar", - HostPort: 5002, - PortPolicy: Static, - ContainerPort: 5002, - Container: &containerWithPort, - }}, - Template: corev1.PodTemplateSpec{ - Spec: corev1.PodSpec{Containers: []corev1.Container{ - {Name: "testing", Image: "testing/image"}, - {Name: "anothertest", Image: "testing/image"}, - }}}}, - } - causes, ok = gs.Validate() - var fields []string - for _, f := range causes { - fields = append(fields, f.Field) - } - assert.False(t, ok) - assert.Len(t, causes, 4) - assert.Contains(t, fields, "container") - assert.Contains(t, fields, "main.hostPort") - assert.Contains(t, fields, "main.containerPort") - assert.Equal(t, causes[0].Type, metav1.CauseTypeFieldValueInvalid) - - gs = GameServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: "dev-game", - Namespace: "default", - Annotations: map[string]string{DevAddressAnnotation: "invalid-ip"}, + longNameLen64 := strings.Repeat("f", validation.LabelValueMaxLength+1) + + var testCases = []struct { + description string + gs GameServer + applyDefaults bool + isValid bool + causesExpected []metav1.StatusCause + }{ + { + description: "Valid game server", + gs: GameServer{ + Spec: GameServerSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "testing", Image: "testing/image"}}}}}, + }, + applyDefaults: true, + isValid: true, }, - Spec: GameServerSpec{ - Ports: []GameServerPort{{Name: "main", ContainerPort: 7777, PortPolicy: Static}}, + { + description: "Invalid gs: container, containerPort, hostPort", + gs: GameServer{ + Spec: GameServerSpec{ + Container: "", + Ports: []GameServerPort{{ + Name: "main", + HostPort: 5001, + PortPolicy: Dynamic, + }, { + Name: "sidecar", + HostPort: 5002, + PortPolicy: Static, + ContainerPort: 5002, + }}, + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{Containers: []corev1.Container{ + {Name: "testing", Image: "testing/image"}, + {Name: "anothertest", Image: "testing/image"}, + }}}}, + }, + applyDefaults: false, + isValid: false, + causesExpected: []metav1.StatusCause{ + {Type: metav1.CauseTypeFieldValueInvalid, Message: "Container is required when using multiple containers in the pod template", Field: "container"}, + {Type: metav1.CauseTypeFieldValueInvalid, Message: "Could not find a container named ", Field: "container"}, + {Type: metav1.CauseTypeFieldValueInvalid, Message: "ContainerPort must be defined for Dynamic and Static PortPolicies", Field: "main.containerPort"}, + {Type: metav1.CauseTypeFieldValueInvalid, Message: "HostPort cannot be specified with a Dynamic or Passthrough PortPolicy", Field: "main.hostPort"}, + }, + }, + { + description: "DevAddressAnnotation: Invalid IP, no host port", + gs: GameServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dev-game", + Namespace: "default", + Annotations: map[string]string{DevAddressAnnotation: "invalid-ip"}, + }, + Spec: GameServerSpec{ + Ports: []GameServerPort{{Name: "main", ContainerPort: 7777, PortPolicy: Static}}, + }, + }, + applyDefaults: false, + isValid: false, + causesExpected: []metav1.StatusCause{ + {Type: metav1.CauseTypeFieldValueInvalid, Message: "Value 'invalid-ip' of annotation 'agones.dev/dev-address' must be a valid IP address", Field: "annotations.agones.dev/dev-address"}, + {Type: metav1.CauseTypeFieldValueRequired, Message: "HostPort is required if GameServer is annotated with 'agones.dev/dev-address'", Field: "main.hostPort"}, + }, + }, + { + description: "Long gs name", + gs: GameServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: longNameLen64, + }, + TypeMeta: metav1.TypeMeta{ + Kind: "test-kind", + }, + Spec: GameServerSpec{ + Container: "my_image", + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "my_image", Image: "foo/my_image"}, + }, + }, + }, + }, + }, + applyDefaults: false, + isValid: false, + causesExpected: []metav1.StatusCause{ + { + Type: metav1.CauseTypeFieldValueInvalid, Message: fmt.Sprintf("Length of test-kind '%s' name should be no more than 63 characters.", longNameLen64), Field: "Name", + }, + }, + }, + { + description: "Long gs GenerateName is not validated on agones side", + gs: GameServer{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: longNameLen64, + }, + TypeMeta: metav1.TypeMeta{ + Kind: "test-kind", + }, + Spec: GameServerSpec{ + Container: "my_image", + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "my_image", Image: "foo/my_image"}, + }, + }, + }, + }, + }, + applyDefaults: false, + isValid: true, + causesExpected: []metav1.StatusCause{}, + }, + { + description: "Long label key is invalid", + gs: GameServer{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: longNameLen64, + }, + TypeMeta: metav1.TypeMeta{ + Kind: "test-kind", + }, + Spec: GameServerSpec{ + Container: "my_image", + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "my_image", Image: "foo/my_image"}, + }, + }, + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{longNameLen64: ""}, + }, + }, + }, + }, + applyDefaults: false, + isValid: false, + causesExpected: []metav1.StatusCause{ + { + // error message is coming from k8s.io/apimachinery/pkg/apis/meta/v1/validation + Type: metav1.CauseTypeFieldValueInvalid, Message: fmt.Sprintf("labels: Invalid value: %q: name part must be no more than 63 characters", longNameLen64), Field: "labels", + }, + }, + }, + { + description: "Long label value is invalid", + gs: GameServer{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "ok-name", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "test-kind", + }, + Spec: GameServerSpec{ + Container: "my_image", + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "my_image", Image: "foo/my_image"}, + }, + }, + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{"agones.dev/longValueKey": longNameLen64}, + }, + }, + }, + }, + applyDefaults: false, + isValid: false, + causesExpected: []metav1.StatusCause{ + { + // error message is coming from k8s.io/apimachinery/pkg/apis/meta/v1/validation + Type: metav1.CauseTypeFieldValueInvalid, Message: fmt.Sprintf("labels: Invalid value: %q: must be no more than 63 characters", longNameLen64), Field: "labels", + }, + }, + }, + { + description: "Long annotation key is invalid", + gs: GameServer{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "ok-name", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "test-kind", + }, + Spec: GameServerSpec{ + Container: "my_image", + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "my_image", Image: "foo/my_image"}, + }, + }, + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{longNameLen64: longNameLen64}, + }, + }, + }, + }, + applyDefaults: false, + isValid: false, + causesExpected: []metav1.StatusCause{ + { + // error message is coming from k8s.io/apimachinery/pkg/apis/meta/v1/validation + Type: metav1.CauseTypeFieldValueInvalid, Message: fmt.Sprintf("annotations: Invalid value: %q: name part must be no more than 63 characters", longNameLen64), Field: "annotations", + }, + }, + }, + { + description: "Invalid character in annotation key", + gs: GameServer{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "ok-name", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "test-kind", + }, + Spec: GameServerSpec{ + Container: "my_image", + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "my_image", Image: "foo/my_image"}, + }, + }, + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"agones.dev/short±Name": longNameLen64}, + }, + }, + }, + }, + applyDefaults: false, + isValid: false, + causesExpected: []metav1.StatusCause{ + { + // error message is coming from k8s.io/apimachinery/pkg/apis/meta/v1/validation + Type: metav1.CauseTypeFieldValueInvalid, Message: fmt.Sprintf("annotations: Invalid value: %q: name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')", "agones.dev/short±Name"), Field: "annotations", + }, + }, + }, + { + description: "Valid annotation key", + gs: GameServer{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "ok-name", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "test-kind", + }, + Spec: GameServerSpec{ + Container: "my_image", + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "my_image", Image: "foo/my_image"}, + }, + }, + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"agones.dev/shortName": longNameLen64}, + }, + }, + }, + }, + applyDefaults: false, + isValid: true, + causesExpected: []metav1.StatusCause{}, + }, + { + description: "Check ContainerPort and HostPort with different policies", + gs: GameServer{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "ok-name", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "test-kind", + }, + Spec: GameServerSpec{ + Ports: []GameServerPort{ + {Name: "one", PortPolicy: Passthrough, ContainerPort: 1294}, + {PortPolicy: Passthrough, Name: "two", HostPort: 7890}, + {PortPolicy: Dynamic, Name: "three", HostPort: 7890, ContainerPort: 1294}, + }, + Container: "my_image", + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "my_image", Image: "foo/my_image"}, + }, + }, + }, + }, + }, + applyDefaults: true, + isValid: false, + causesExpected: []metav1.StatusCause{ + {Type: metav1.CauseTypeFieldValueInvalid, Message: "ContainerPort cannot be specified with Passthrough PortPolicy", Field: "one.containerPort"}, + {Type: metav1.CauseTypeFieldValueInvalid, Message: "HostPort cannot be specified with a Dynamic or Passthrough PortPolicy", Field: "two.hostPort"}, + {Type: metav1.CauseTypeFieldValueInvalid, Message: "HostPort cannot be specified with a Dynamic or Passthrough PortPolicy", Field: "three.hostPort"}, + }, + }, + { + description: "PortPolicy must be Static with HostPort specified", + gs: GameServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dev-game", + Namespace: "default", + Annotations: map[string]string{DevAddressAnnotation: ipFixture}, + }, + Spec: GameServerSpec{ + Ports: []GameServerPort{ + {PortPolicy: Passthrough, Name: "main", HostPort: 7890, ContainerPort: 7777}, + }, + Container: "my_image", + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "my_image", Image: "foo/my_image"}, + }, + }, + }, + }, + }, + applyDefaults: true, + isValid: false, + causesExpected: []metav1.StatusCause{ + {Type: metav1.CauseTypeFieldValueRequired, Message: "PortPolicy must be Static", Field: "main.portPolicy"}, + }, + }, + { + description: "ContainerPort is less than zero", + gs: GameServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dev-game", + Namespace: "default", + }, + Spec: GameServerSpec{ + Ports: []GameServerPort{{Name: "main", + ContainerPort: -4, + PortPolicy: Dynamic}}, + Container: "testing", + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{Containers: []corev1.Container{ + {Name: "testing", Image: "testing/image"}, + }}}, + }, + }, + applyDefaults: false, + isValid: false, + causesExpected: []metav1.StatusCause{ + {Type: metav1.CauseTypeFieldValueInvalid, Message: "ContainerPort must be defined for Dynamic and Static PortPolicies", Field: "main.containerPort"}, + }, + }, + { + description: "CPU Request > Limit", + gs: GameServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dev-game", + Namespace: "default", + }, + Spec: GameServerSpec{ + Ports: []GameServerPort{{Name: "main", + ContainerPort: 7777, + PortPolicy: Dynamic, + }}, + Container: "testing", + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{Containers: []corev1.Container{ + { + Name: "testing", + Image: "testing/image", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("50m"), + corev1.ResourceMemory: resource.MustParse("32Mi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("30m"), + corev1.ResourceMemory: resource.MustParse("32Mi"), + }, + }, + }, + }}}, + }, + }, + applyDefaults: false, + isValid: false, + causesExpected: []metav1.StatusCause{ + {Type: metav1.CauseTypeFieldValueInvalid, Message: "Request must be less than or equal to cpu limit", Field: "container"}, + }, + }, + { + description: "CPU negative request", + gs: GameServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dev-game", + Namespace: "default", + }, + Spec: GameServerSpec{ + Ports: []GameServerPort{{Name: "main", + ContainerPort: 7777, + PortPolicy: Dynamic, + }}, + Container: "testing", + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{Containers: []corev1.Container{ + { + Name: "testing", + Image: "testing/image", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("-30m"), + corev1.ResourceMemory: resource.MustParse("32Mi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("30m"), + corev1.ResourceMemory: resource.MustParse("32Mi"), + }, + }, + }, + }}}, + }, + }, + applyDefaults: false, + isValid: false, + causesExpected: []metav1.StatusCause{ + {Type: metav1.CauseTypeFieldValueInvalid, Message: "Resource cpu request value must be non negative", Field: "container"}, + }, + }, + { + description: "CPU negative limit", + gs: GameServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dev-game", + Namespace: "default", + }, + Spec: GameServerSpec{ + Ports: []GameServerPort{{Name: "main", + ContainerPort: 7777, + PortPolicy: Dynamic, + }}, + Container: "testing", + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{Containers: []corev1.Container{ + { + Name: "testing", + Image: "testing/image", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("30m"), + corev1.ResourceMemory: resource.MustParse("32Mi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("-30m"), + corev1.ResourceMemory: resource.MustParse("32Mi"), + }, + }, + }, + }}}, + }, + }, + applyDefaults: false, + isValid: false, + causesExpected: []metav1.StatusCause{ + {Type: metav1.CauseTypeFieldValueInvalid, Message: "Request must be less than or equal to cpu limit", Field: "container"}, + {Type: metav1.CauseTypeFieldValueInvalid, Message: "Resource cpu limit value must be non negative", Field: "container"}, + }, + }, + { + description: "Memory Request > Limit", + gs: GameServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dev-game", + Namespace: "default", + }, + Spec: GameServerSpec{ + Ports: []GameServerPort{{Name: "main", + ContainerPort: 7777, + PortPolicy: Dynamic, + }}, + Container: "testing", + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{Containers: []corev1.Container{ + { + Name: "testing", + Image: "testing/image", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("30m"), + corev1.ResourceMemory: resource.MustParse("55Mi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("30m"), + corev1.ResourceMemory: resource.MustParse("32Mi"), + }, + }, + }, + }}}, + }, + }, + applyDefaults: false, + isValid: false, + causesExpected: []metav1.StatusCause{ + {Type: metav1.CauseTypeFieldValueInvalid, Message: "Request must be less than or equal to memory limit", Field: "container"}, + }, + }, + { + description: "Memory negative request", + gs: GameServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dev-game", + Namespace: "default", + }, + Spec: GameServerSpec{ + Ports: []GameServerPort{{Name: "main", + ContainerPort: 7777, + PortPolicy: Dynamic, + }}, + Container: "testing", + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{Containers: []corev1.Container{ + { + Name: "testing", + Image: "testing/image", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("30m"), + corev1.ResourceMemory: resource.MustParse("-32Mi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("30m"), + corev1.ResourceMemory: resource.MustParse("32Mi"), + }, + }, + }, + }}}, + }, + }, + applyDefaults: false, + isValid: false, + causesExpected: []metav1.StatusCause{ + {Type: metav1.CauseTypeFieldValueInvalid, Message: "Resource memory request value must be non negative", Field: "container"}, + }, + }, + { + description: "Memory negative limit", + gs: GameServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dev-game", + Namespace: "default", + }, + Spec: GameServerSpec{ + Ports: []GameServerPort{{Name: "main", + ContainerPort: 7777, + PortPolicy: Dynamic, + }}, + Container: "testing", + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{Containers: []corev1.Container{ + { + Name: "testing", + Image: "testing/image", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("30m"), + corev1.ResourceMemory: resource.MustParse("32Mi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("30m"), + corev1.ResourceMemory: resource.MustParse("-32Mi"), + }, + }, + }, + }}}, + }, + }, + applyDefaults: false, + isValid: false, + causesExpected: []metav1.StatusCause{ + {Type: metav1.CauseTypeFieldValueInvalid, Message: "Request must be less than or equal to memory limit", Field: "container"}, + {Type: metav1.CauseTypeFieldValueInvalid, Message: "Resource memory limit value must be non negative", Field: "container"}, + }, }, } - causes, ok = gs.Validate() - for _, f := range causes { - fields = append(fields, f.Field) + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + if tc.applyDefaults { + tc.gs.ApplyDefaults() + } + + causes, ok := tc.gs.Validate() + + assert.Equal(t, tc.isValid, ok) + assert.ElementsMatch(t, tc.causesExpected, causes, "causes check") + }) } - assert.False(t, ok) - assert.Len(t, causes, 2) - assert.Contains(t, fields, fmt.Sprintf("annotations.%s", DevAddressAnnotation)) - assert.Contains(t, fields, "main.hostPort") - assert.Equal(t, causes[1].Type, metav1.CauseTypeFieldValueRequired) +} - gs = GameServer{ - Spec: GameServerSpec{ - Container: "my_image", - Template: corev1.PodTemplateSpec{ - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - {Name: "my_image", Image: "foo/my_image"}, +func TestGameServerValidateFeatures(t *testing.T) { + t.Parallel() + runtime.FeatureTestMutex.Lock() + defer runtime.FeatureTestMutex.Unlock() + + portContainerName := "another-container" + + var testCases = []struct { + description string + feature string + gs GameServer + isValid bool + causesExpected []metav1.StatusCause + }{ + { + description: "ContainerPortAllocation, invalid container name, container was not found", + feature: fmt.Sprintf("%s=true", runtime.FeatureContainerPortAllocation), + gs: GameServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dev-game", + Namespace: "default", + }, + Spec: GameServerSpec{ + Ports: []GameServerPort{ + { + Name: "main", + ContainerPort: 7777, + PortPolicy: Dynamic, + Container: &portContainerName, + }, }, + Container: "testing", + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{Containers: []corev1.Container{ + {Name: "testing", Image: "testing/image"}, + }}}, }, }, + isValid: false, + causesExpected: []metav1.StatusCause{ + {Type: metav1.CauseTypeFieldValueInvalid, Message: "Container must be empty or the name of a container in the pod template", Field: "main.container"}, + }, }, - } - - longName := strings.Repeat("f", validation.LabelValueMaxLength+1) - gs.Name = longName - causes, ok = gs.Validate() - assert.False(t, ok) - assert.Len(t, causes, 1) - assert.Equal(t, "Name", causes[0].Field) - - gs.Name = "" - gs.GenerateName = longName - causes, ok = gs.Validate() - assert.True(t, ok) - assert.Len(t, causes, 0) - - gs.Spec.Template.ObjectMeta.Labels = make(map[string]string) - gs.Spec.Template.ObjectMeta.Labels[longName] = "" - causes, ok = gs.Validate() - assert.False(t, ok) - assert.Len(t, causes, 1) - assert.Equal(t, "labels", causes[0].Field) - - gs.Spec.Template.ObjectMeta.Labels = make(map[string]string) - gs.Spec.Template.ObjectMeta.Labels["agones.dev/longValueKey"] = longName - causes, ok = gs.Validate() - assert.False(t, ok) - assert.Len(t, causes, 1) - assert.Equal(t, "labels", causes[0].Field) - - // Validate Labels and Annotations - gs.Spec.Template.ObjectMeta.Annotations = make(map[string]string) - gs.Spec.Template.ObjectMeta.Annotations[longName] = longName - causes, ok = gs.Validate() - assert.False(t, ok) - assert.Len(t, causes, 2) - - // No errors if valid Annotation was used - gs.Spec.Template.ObjectMeta.Labels = make(map[string]string) - gs.Spec.Template.ObjectMeta.Annotations = make(map[string]string) - shortName := "agones.dev/shortName" - gs.Spec.Template.ObjectMeta.Annotations[shortName] = "shortValue" - causes, ok = gs.Validate() - assert.True(t, ok) - assert.Len(t, causes, 0) - - gs.Spec.Template.ObjectMeta.Annotations[shortName] = longName - causes, ok = gs.Validate() - assert.True(t, ok) - assert.Len(t, causes, 0) - - gs.Spec.Template.ObjectMeta.Annotations["agones.dev/short±Name"] = "shortValue" - causes, ok = gs.Validate() - assert.False(t, ok) - assert.Len(t, causes, 1) - assert.Equal(t, "annotations", causes[0].Field) - - gs = GameServer{ - Spec: GameServerSpec{ - Ports: []GameServerPort{{Name: "one", PortPolicy: Passthrough, ContainerPort: 1294}, {PortPolicy: Passthrough, Name: "two", HostPort: 7890}}, - Template: corev1.PodTemplateSpec{ - Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "testing", Image: "testing/image"}}}}, + { + description: "ContainerPortAllocation, Port fileld Container should not be set if FeatureContainerPortAllocation is not enabled", + feature: fmt.Sprintf("%s=false", runtime.FeatureContainerPortAllocation), + gs: GameServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dev-game", + Namespace: "default", + }, + Spec: GameServerSpec{ + Ports: []GameServerPort{ + { + Name: "main", + ContainerPort: 7777, + PortPolicy: Dynamic, + Container: &portContainerName, + }, + }, + Container: "testing", + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{Containers: []corev1.Container{ + {Name: "testing", Image: "testing/image"}, + }}}, + }, + }, + isValid: false, + causesExpected: []metav1.StatusCause{ + {Type: metav1.CauseTypeFieldValueNotSupported, Message: "Value cannot be set unless feature flag ContainerPortAllocation is enabled", Field: "main.container"}, + }, + }, + { + description: "ContainerPortAllocation enabled, OK scenario", + feature: fmt.Sprintf("%s=true", runtime.FeatureContainerPortAllocation), + gs: GameServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "dev-game", + Namespace: "default", + }, + Spec: GameServerSpec{ + Ports: []GameServerPort{ + { + Name: "main", + ContainerPort: 7777, + PortPolicy: Dynamic, + }, + }, + Container: "testing", + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{Containers: []corev1.Container{ + {Name: "testing", Image: "testing/image"}, + }}}, + }, + }, + isValid: true, + causesExpected: []metav1.StatusCause{}, + }, + { + description: "PlayerTracking is disabled, Players field specified", + feature: fmt.Sprintf("%s=false", runtime.FeaturePlayerTracking), + gs: GameServer{ + Spec: GameServerSpec{ + Container: "testing", + Players: &PlayersSpec{InitialCapacity: 10}, + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "testing", Image: "testing/image"}}}}}, + }, + isValid: false, + causesExpected: []metav1.StatusCause{ + {Type: metav1.CauseTypeFieldValueNotSupported, Message: "Value cannot be set unless feature flag PlayerTracking is enabled", Field: "players"}, + }, + }, + { + description: "PlayerTracking is enabled, Players field specified", + feature: fmt.Sprintf("%s=true", runtime.FeaturePlayerTracking), + gs: GameServer{ + Spec: GameServerSpec{ + Container: "testing", + Players: &PlayersSpec{InitialCapacity: 10}, + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "testing", Image: "testing/image"}}}}}, + }, + isValid: true, + causesExpected: []metav1.StatusCause{}, }, } - gs.ApplyDefaults() - causes, ok = gs.Validate() - for _, f := range causes { - fields = append(fields, f.Field) + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + err := runtime.ParseFeatures(tc.feature) + assert.NoError(t, err) + + causes, ok := tc.gs.Validate() + + assert.Equal(t, tc.isValid, ok) + assert.ElementsMatch(t, tc.causesExpected, causes, "causes check") + }) } - assert.False(t, ok) - assert.Len(t, causes, 2) - assert.Contains(t, fields, "one.containerPort") - assert.Contains(t, fields, "two.hostPort") - - t.Run("validation of "+string(runtime.FeaturePlayerTracking), func(t *testing.T) { - gs := GameServer{ - Spec: GameServerSpec{ - Players: &PlayersSpec{InitialCapacity: 10}, - Template: corev1.PodTemplateSpec{ - Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "testing", Image: "testing/image"}}}}}, - } - gs.ApplyDefaults() - causes, ok := gs.Validate() - for _, f := range causes { - fields = append(fields, f.Field) - } - assert.False(t, ok) - assert.Len(t, causes, 1) - assert.Contains(t, fields, "players") - - err := runtime.ParseFeatures(string(runtime.FeaturePlayerTracking) + "=true") - assert.NoError(t, err) - gs = GameServer{ - Spec: GameServerSpec{ - Players: &PlayersSpec{InitialCapacity: 10}, - Template: corev1.PodTemplateSpec{ - Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "testing", Image: "testing/image"}}}}}, - } - gs.ApplyDefaults() - causes, ok = gs.Validate() - assert.True(t, ok) - assert.Empty(t, causes) - }) } -func TestGameServerPod(t *testing.T) { +func TestGameServerPodNoErrors(t *testing.T) { + t.Parallel() fixture := defaultGameServer() fixture.ApplyDefaults() @@ -552,10 +1194,73 @@ func TestGameServerPod(t *testing.T) { assert.Equal(t, fixture.Spec.Ports[0].ContainerPort, pod.Spec.Containers[0].Ports[0].ContainerPort) assert.Equal(t, corev1.Protocol("UDP"), pod.Spec.Containers[0].Ports[0].Protocol) assert.True(t, metav1.IsControlledBy(pod, fixture)) +} + +func TestGameServerPod_ContainerNotFound_ErrReturned(t *testing.T) { + t.Parallel() + runtime.FeatureTestMutex.Lock() + defer runtime.FeatureTestMutex.Unlock() + + containerName1 := "Container1" + fixture := &GameServer{ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default", UID: "1234"}, + Spec: GameServerSpec{ + Container: "can-not-find-this-name", + Ports: []GameServerPort{ + { + Container: &containerName1, + ContainerPort: 7777, + HostPort: 9999, + PortPolicy: Static, + }, + }, + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "Container2", Image: "container/image"}}, + }, + }, + }, Status: GameServerStatus{State: GameServerStateCreating}} + + var testCases = []struct { + errExpected string + сontainerPortAllocationEnabled bool + }{ + { + errExpected: "failed to find container named Container1 in pod spec", + сontainerPortAllocationEnabled: true, + }, + { + errExpected: "Could not find a container named can-not-find-this-name", + сontainerPortAllocationEnabled: false, + }, + } + + for _, tc := range testCases { + t.Run(fmt.Sprintf("сontainerPortAllocationEnabled: %t", tc.сontainerPortAllocationEnabled), func(t *testing.T) { + if tc.сontainerPortAllocationEnabled { + err := runtime.ParseFeatures(string(runtime.FeatureContainerPortAllocation) + "=true") + assert.NoError(t, err) + } else { + err := runtime.ParseFeatures(string(runtime.FeatureContainerPortAllocation) + "=false") + assert.NoError(t, err) + } + + _, err := fixture.Pod() + if assert.NotNil(t, err, "Pod should return an error") { + assert.Equal(t, tc.errExpected, err.Error()) + } + }) + } + +} + +func TestGameServerPod_WithSidecar_NoErrros(t *testing.T) { + t.Parallel() + fixture := defaultGameServer() + fixture.ApplyDefaults() sidecar := corev1.Container{Name: "sidecar", Image: "container/sidecar"} fixture.Spec.Template.Spec.ServiceAccountName = "other-agones-sdk" - pod, err = fixture.Pod(sidecar) + pod, err := fixture.Pod(sidecar) assert.Nil(t, err, "Pod should not return an error") assert.Equal(t, fixture.ObjectMeta.Name, pod.ObjectMeta.Name) assert.Len(t, pod.Spec.Containers, 2, "Should have two containers") @@ -793,35 +1498,74 @@ func TestGameServerIsBeforeReady(t *testing.T) { func TestGameServerApplyToPodContainer(t *testing.T) { t.Parallel() + type expected struct { + err string + tty bool + } - name := "mycontainer" - gs := &GameServer{ - Spec: GameServerSpec{ - Container: name, - Template: corev1.PodTemplateSpec{ - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - {Name: name, Image: "foo/mycontainer"}, - {Name: "notmycontainer", Image: "foo/notmycontainer"}, + var testCases = []struct { + description string + gs *GameServer + expected expected + }{ + { + description: "OK, no error", + gs: &GameServer{ + Spec: GameServerSpec{ + Container: "mycontainer", + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "mycontainer", Image: "foo/mycontainer"}, + {Name: "notmycontainer", Image: "foo/notmycontainer"}, + }, + }, + }, + }, + }, + expected: expected{ + err: "", + tty: true, + }, + }, + { + description: "container not found, error is returned", + gs: &GameServer{ + Spec: GameServerSpec{ + Container: "mycontainer-WRONG-NAME", + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "mycontainer", Image: "foo/mycontainer"}, + {Name: "notmycontainer", Image: "foo/notmycontainer"}, + }, + }, }, }, }, + expected: expected{ + err: "failed to find container named mycontainer-WRONG-NAME in pod spec", + tty: false, + }, }, } - pod := &corev1.Pod{Spec: *gs.Spec.Template.Spec.DeepCopy()} - - err := gs.ApplyToPodContainer(pod, gs.Spec.Container, func(c corev1.Container) corev1.Container { - // easy thing to change and test for - c.TTY = true - - return c - }) - - assert.NoError(t, err) - assert.Len(t, pod.Spec.Containers, 2) - assert.True(t, pod.Spec.Containers[0].TTY) - assert.False(t, pod.Spec.Containers[1].TTY) + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + pod := &corev1.Pod{Spec: *tc.gs.Spec.Template.Spec.DeepCopy()} + result := tc.gs.ApplyToPodContainer(pod, tc.gs.Spec.Container, func(c corev1.Container) corev1.Container { + // easy thing to change and test for + c.TTY = true + return c + }) + + if tc.expected.err != "" && assert.NotNil(t, result) { + assert.Equal(t, tc.expected.err, result.Error()) + } + assert.Equal(t, tc.expected.tty, pod.Spec.Containers[0].TTY) + assert.False(t, pod.Spec.Containers[1].TTY) + }) + } } func defaultGameServer() *GameServer { diff --git a/test/e2e/fleet_test.go b/test/e2e/fleet_test.go index cfec5b0649..ec1eb3d3dd 100644 --- a/test/e2e/fleet_test.go +++ b/test/e2e/fleet_test.go @@ -627,7 +627,7 @@ func TestFleetGSSpecValidation(t *testing.T) { statusErr, ok = err.(*k8serrors.StatusError) assert.True(t, ok) assert.Len(t, statusErr.Status().Details.Causes, 1) - assert.Equal(t, agonesv1.ErrHostPortDynamic, statusErr.Status().Details.Causes[0].Message) + assert.Equal(t, agonesv1.ErrHostPort, statusErr.Status().Details.Causes[0].Message) fltPort.Spec.Template.Spec.Ports[0].PortPolicy = agonesv1.Static fltPort.Spec.Template.Spec.Ports[0].HostPort = 0