From 59f3634f6c39368a05f70391a127cb050d1e80a2 Mon Sep 17 00:00:00 2001 From: Alexander Apalikov Date: Thu, 23 May 2019 17:17:34 +0300 Subject: [PATCH] Add missing validation for GameServerSet, refactor Add common file which contains all common validations for GS, GSSet and Fleet. Add const for some err messages. GSSet now checks Name on Update and GS Spec on Create. --- pkg/apis/stable/v1alpha1/common.go | 68 +++++++++++++++++++ pkg/apis/stable/v1alpha1/fleet.go | 27 +++----- pkg/apis/stable/v1alpha1/gameserver.go | 18 ++--- pkg/apis/stable/v1alpha1/gameserver_test.go | 34 ++++++---- pkg/apis/stable/v1alpha1/gameserverset.go | 22 +++--- .../stable/v1alpha1/gameserverset_test.go | 23 ++++++- test/e2e/fleet_test.go | 4 +- 7 files changed, 136 insertions(+), 60 deletions(-) create mode 100644 pkg/apis/stable/v1alpha1/common.go diff --git a/pkg/apis/stable/v1alpha1/common.go b/pkg/apis/stable/v1alpha1/common.go new file mode 100644 index 0000000000..16272da13b --- /dev/null +++ b/pkg/apis/stable/v1alpha1/common.go @@ -0,0 +1,68 @@ +// Copyright 2019 Google LLC All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package v1alpha1 + +import ( + "fmt" + + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/validation" +) + +// 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" + ErrPortPolicyStatic = "PortPolicy must be Static" +) + +// crdName is an interface to get Name and Kind of CRD +type crdName interface { + GetName() string + GetObjectKind() schema.ObjectKind +} + +// validateName Check NameSize of a CRD +func validateName(cs crdName) []v1.StatusCause { + var causes []v1.StatusCause + name := cs.GetName() + kind := cs.GetObjectKind().GroupVersionKind().Kind + // make sure the Name of a Fleet does not oversize the Label size in GSS and GS + if len(name) > validation.LabelValueMaxLength { + causes = append(causes, v1.StatusCause{ + Type: v1.CauseTypeFieldValueInvalid, + Field: fmt.Sprintf("Name"), + Message: fmt.Sprintf("Length of %s '%s' name should be no more than 63 characters.", kind, name), + }) + } + return causes +} + +// gsSpec is an interface which contains all necessary +// functions to perform common validations against it +type gsSpec interface { + GetGameServerSpec() *GameServerSpec +} + +// validateGSSpec Check GameserverSpec of a CRD +// Used by Fleet and Gameserverset +func validateGSSpec(gs gsSpec) []v1.StatusCause { + gsSpec := gs.GetGameServerSpec() + gsSpec.ApplyDefaults() + causes, _ := gsSpec.Validate("") + + return causes +} diff --git a/pkg/apis/stable/v1alpha1/fleet.go b/pkg/apis/stable/v1alpha1/fleet.go index 12cd6f82bc..2dd5570de6 100644 --- a/pkg/apis/stable/v1alpha1/fleet.go +++ b/pkg/apis/stable/v1alpha1/fleet.go @@ -15,15 +15,12 @@ package v1alpha1 import ( - "fmt" - "agones.dev/agones/pkg" "agones.dev/agones/pkg/apis" "agones.dev/agones/pkg/apis/stable" appsv1 "k8s.io/api/apps/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" - "k8s.io/apimachinery/pkg/util/validation" ) const ( @@ -138,26 +135,20 @@ func (f *Fleet) ApplyDefaults() { f.ObjectMeta.Annotations[stable.VersionAnnotation] = pkg.Version } +// GetGameServerSpec get underlying Gameserver specification +func (f *Fleet) GetGameServerSpec() *GameServerSpec { + return &f.Spec.Template.Spec +} + // Validate validates the Fleet configuration. // If a Fleet is invalid there will be > 0 values in // the returned array func (f *Fleet) Validate() ([]metav1.StatusCause, bool) { - var causes []metav1.StatusCause - - // make sure the Name of a Fleet does not oversize the Label size in GSS and GS - if len(f.Name) > validation.LabelValueMaxLength { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Field: fmt.Sprintf("Name"), - Message: fmt.Sprintf("Length of Fleet '%s' name should be no more than 63 characters.", f.ObjectMeta.Name), - }) - } + causes := validateName(f) - // check gameserver specification in a fleet - gsSpec := f.Spec.Template.Spec - gsSpec.ApplyDefaults() - gsCauses, ok := gsSpec.Validate("") - if !ok { + // check Gameserver specification in a Fleet + gsCauses := validateGSSpec(f) + if len(gsCauses) > 0 { causes = append(causes, gsCauses...) } diff --git a/pkg/apis/stable/v1alpha1/gameserver.go b/pkg/apis/stable/v1alpha1/gameserver.go index 8cfdbe987e..a531185ea0 100644 --- a/pkg/apis/stable/v1alpha1/gameserver.go +++ b/pkg/apis/stable/v1alpha1/gameserver.go @@ -28,7 +28,6 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" - "k8s.io/apimachinery/pkg/util/validation" ) const ( @@ -280,7 +279,7 @@ func (gss GameServerSpec) Validate(devAddress string) ([]metav1.StatusCause, boo causes = append(causes, metav1.StatusCause{ Type: metav1.CauseTypeFieldValueRequired, Field: fmt.Sprintf("%s.portPolicy", p.Name), - Message: fmt.Sprintf("PortPolicy must be Static"), + Message: fmt.Sprint(ErrPortPolicyStatic), }) } } @@ -290,7 +289,7 @@ func (gss GameServerSpec) Validate(devAddress string) ([]metav1.StatusCause, boo causes = append(causes, metav1.StatusCause{ Type: metav1.CauseTypeFieldValueInvalid, Field: "container", - Message: "Container is required when using multiple containers in the pod template", + Message: ErrContainerRequired, }) } @@ -300,7 +299,7 @@ func (gss GameServerSpec) Validate(devAddress string) ([]metav1.StatusCause, boo causes = append(causes, metav1.StatusCause{ Type: metav1.CauseTypeFieldValueInvalid, Field: fmt.Sprintf("%s.hostPort", p.Name), - Message: "HostPort cannot be specified with a Dynamic PortPolicy", + Message: ErrHostPortDynamic, }) } } @@ -323,16 +322,7 @@ func (gss GameServerSpec) Validate(devAddress string) ([]metav1.StatusCause, boo // 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), - }) - } + causes := validateName(gs) // make sure the host port is specified if this is a development server devAddress, _ := gs.GetDevAddress() diff --git a/pkg/apis/stable/v1alpha1/gameserver_test.go b/pkg/apis/stable/v1alpha1/gameserver_test.go index 466af94568..11dec02746 100644 --- a/pkg/apis/stable/v1alpha1/gameserver_test.go +++ b/pkg/apis/stable/v1alpha1/gameserver_test.go @@ -299,21 +299,7 @@ func TestGameServerValidate(t *testing.T) { } func TestGameServerPod(t *testing.T) { - fixture := &GameServer{ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default", UID: "1234"}, - Spec: GameServerSpec{ - Ports: []GameServerPort{ - { - ContainerPort: 7777, - HostPort: 9999, - PortPolicy: Static, - }, - }, - Template: corev1.PodTemplateSpec{ - Spec: corev1.PodSpec{ - Containers: []corev1.Container{{Name: "container", Image: "container/image"}}, - }, - }, - }, Status: GameServerStatus{State: GameServerStateCreating}} + fixture := defaultGameServer() fixture.ApplyDefaults() pod, err := fixture.Pod() @@ -505,3 +491,21 @@ func TestGameServerApplyToPodGameServerContainer(t *testing.T) { assert.True(t, p2.Spec.Containers[0].TTY) assert.False(t, p2.Spec.Containers[1].TTY) } + +func defaultGameServer() *GameServer { + return &GameServer{ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default", UID: "1234"}, + Spec: GameServerSpec{ + Ports: []GameServerPort{ + { + ContainerPort: 7777, + HostPort: 9999, + PortPolicy: Static, + }, + }, + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "container", Image: "container/image"}}, + }, + }, + }, Status: GameServerStatus{State: GameServerStateCreating}} +} diff --git a/pkg/apis/stable/v1alpha1/gameserverset.go b/pkg/apis/stable/v1alpha1/gameserverset.go index 51651b5e11..2b2fe0fcbc 100644 --- a/pkg/apis/stable/v1alpha1/gameserverset.go +++ b/pkg/apis/stable/v1alpha1/gameserverset.go @@ -15,13 +15,11 @@ package v1alpha1 import ( - "fmt" "reflect" "agones.dev/agones/pkg/apis" "agones.dev/agones/pkg/apis/stable" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/validation" ) const ( @@ -79,7 +77,7 @@ type GameServerSetStatus struct { // ValidateUpdate validates when updates occur. The argument // is the new GameServerSet, being passed into the old GameServerSet func (gsSet *GameServerSet) ValidateUpdate(new *GameServerSet) ([]metav1.StatusCause, bool) { - var causes []metav1.StatusCause + causes := validateName(new) if !reflect.DeepEqual(gsSet.Spec.Template, new.Spec.Template) { causes = append(causes, metav1.StatusCause{ Type: metav1.CauseTypeFieldValueInvalid, @@ -93,18 +91,22 @@ func (gsSet *GameServerSet) ValidateUpdate(new *GameServerSet) ([]metav1.StatusC // Validate validates when Create occur. Check the name szie func (gsSet *GameServerSet) Validate() ([]metav1.StatusCause, bool) { - var causes []metav1.StatusCause - if len(gsSet.Name) > validation.LabelValueMaxLength { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Field: "Name", - Message: fmt.Sprintf("Length of GameServerSet '%s' name should be no more than 63 characters.", gsSet.ObjectMeta.Name), - }) + causes := validateName(gsSet) + + // check Gameserver specification in a Gameserverset + gsCauses := validateGSSpec(gsSet) + if len(gsCauses) > 0 { + causes = append(causes, gsCauses...) } return causes, len(causes) == 0 } +// GetGameServerSpec get underlying Gameserver specification +func (gsSet *GameServerSet) GetGameServerSpec() *GameServerSpec { + return &gsSet.Spec.Template.Spec +} + // GameServer returns a single GameServer derived // from the GameSever template func (gsSet *GameServerSet) GameServer() *GameServer { diff --git a/pkg/apis/stable/v1alpha1/gameserverset_test.go b/pkg/apis/stable/v1alpha1/gameserverset_test.go index 422a86fadd..9b2192534f 100644 --- a/pkg/apis/stable/v1alpha1/gameserverset_test.go +++ b/pkg/apis/stable/v1alpha1/gameserverset_test.go @@ -59,13 +59,15 @@ func TestGameServerSetGameServer(t *testing.T) { assert.True(t, metav1.IsControlledBy(gs, &gsSet)) } +// TestGameServerSetValidateUpdate test GameServerSet Validate() and ValidateUpdate() func TestGameServerSetValidateUpdate(t *testing.T) { + gsSpec := defaultGameServer().Spec gsSet := GameServerSet{ ObjectMeta: metav1.ObjectMeta{Name: "test"}, Spec: GameServerSetSpec{ Replicas: 10, Template: GameServerTemplateSpec{ - Spec: GameServerSpec{Ports: []GameServerPort{{ContainerPort: 1234}}}, + Spec: gsSpec, }, }, } @@ -103,4 +105,23 @@ func TestGameServerSetValidateUpdate(t *testing.T) { causes, ok = newGSS.Validate() assert.True(t, ok) assert.Len(t, causes, 0) + + newGSS = gsSet.DeepCopy() + newGSS.Name = string(bytes) + causes, ok = gsSet.ValidateUpdate(newGSS) + assert.False(t, ok) + assert.Len(t, causes, 1) + assert.Equal(t, "Name", causes[0].Field) + + gsSet.Spec.Template.Spec.Template = + corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "container", Image: "myimage"}, {Name: "container2", Image: "myimage"}}, + }, + } + causes, ok = gsSet.Validate() + + assert.False(t, ok) + assert.Len(t, causes, 2) + assert.Equal(t, "container", causes[0].Field) } diff --git a/test/e2e/fleet_test.go b/test/e2e/fleet_test.go index 6ff17e9f75..149e275cd3 100644 --- a/test/e2e/fleet_test.go +++ b/test/e2e/fleet_test.go @@ -524,7 +524,7 @@ func TestFleetGSSpecValidation(t *testing.T) { 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 "} + CausesMessages := []string{v1alpha1.ErrContainerRequired, "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) @@ -547,7 +547,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, "HostPort cannot be specified with a Dynamic PortPolicy", statusErr.Status().Details.Causes[0].Message) + assert.Equal(t, v1alpha1.ErrHostPortDynamic, statusErr.Status().Details.Causes[0].Message) fltPort.Spec.Template.Spec.Ports[0].PortPolicy = v1alpha1.Static fltPort.Spec.Template.Spec.Ports[0].HostPort = 0