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

@@ -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
+}

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...)
 	}
 

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()

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}}
+}

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 {

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)
 }

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