Add Fleet and Gameserverset Validation handler

Add Fleet and GSS Name length check in order to prevent GSS label overflowing.

install/helm/agones/templates/admissionregistration.yaml

@@ -39,7 +39,9 @@ webhooks:
       - apiGroups:
           - stable.agones.dev
         resources:
+          - "fleets"
           - "gameservers"
+          - "gameserversets"
           - "fleetallocations"
           - "fleetautoscalers"
         apiVersions:
@@ -49,6 +51,7 @@ webhooks:
       - apiGroups:
           - stable.agones.dev
         resources:
+          - "fleets"
           - "gameserversets"
           - "fleetallocations"
           - "fleetautoscalers"

install/yaml/install.yaml

@@ -1268,7 +1268,9 @@ webhooks:
       - apiGroups:
           - stable.agones.dev
         resources:
+          - "fleets"
           - "gameservers"
+          - "gameserversets"
           - "fleetallocations"
           - "fleetautoscalers"
         apiVersions:
@@ -1278,6 +1280,7 @@ webhooks:
       - apiGroups:
           - stable.agones.dev
         resources:
+          - "fleets"
           - "gameserversets"
           - "fleetallocations"
           - "fleetautoscalers"

pkg/apis/stable/v1alpha1/fleet.go

@@ -15,11 +15,14 @@
 package v1alpha1
 
 import (
+	"fmt"
+
 	"agones.dev/agones/pkg"
 	"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 (
@@ -134,6 +137,24 @@ func (f *Fleet) ApplyDefaults() {
 	f.ObjectMeta.Annotations[stable.VersionAnnotation] = pkg.Version
 }
 
+// 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.", f.ObjectMeta.Name),
+		})
+	}
+
+	return causes, len(causes) == 0
+}
+
 // UpperBoundReplicas returns whichever is smaller,
 // the value i, or the f.Spec.Replicas.
 func (f *Fleet) UpperBoundReplicas(i int32) int32 {

pkg/apis/stable/v1alpha1/fleet_test.go

@@ -21,6 +21,7 @@ import (
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/validation"
 )
 
 func TestFleetGameServerSetGameServer(t *testing.T) {
@@ -98,6 +99,21 @@ func TestSumStatusAllocatedReplicas(t *testing.T) {
 	assert.Equal(t, int32(5), SumStatusAllocatedReplicas([]*GameServerSet{gsSet1, gsSet2}))
 }
 
+func TestFleetName(t *testing.T) {
+	f := Fleet{}
+
+	nameLen := validation.LabelValueMaxLength + 1
+	bytes := make([]byte, nameLen)
+	for i := 0; i < nameLen; i++ {
+		bytes[i] = 'f'
+	}
+	f.Name = string(bytes)
+	causes, ok := f.Validate()
+	assert.False(t, ok)
+	assert.Len(t, causes, 1)
+	assert.Equal(t, "Name", causes[0].Field)
+}
+
 func TestSumStatusReplicas(t *testing.T) {
 	fixture := []*GameServerSet{
 		{Status: GameServerSetStatus{Replicas: 10}},

pkg/apis/stable/v1alpha1/gameserverset.go

@@ -15,10 +15,12 @@
 package v1alpha1
 
 import (
+	"fmt"
 	"reflect"
 
 	"agones.dev/agones/pkg/apis/stable"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/validation"
 )
 
 const (
@@ -75,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) (bool, []metav1.StatusCause) {
+func (gsSet *GameServerSet) ValidateUpdate(new *GameServerSet) ([]metav1.StatusCause, bool) {
 	var causes []metav1.StatusCause
 	if !reflect.DeepEqual(gsSet.Spec.Template, new.Spec.Template) {
 		causes = append(causes, metav1.StatusCause{
@@ -85,7 +87,21 @@ func (gsSet *GameServerSet) ValidateUpdate(new *GameServerSet) (bool, []metav1.S
 		})
 	}
 
-	return len(causes) == 0, causes
+	return causes, len(causes) == 0
+}
+
+// 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.", gsSet.ObjectMeta.Name),
+		})
+	}
+
+	return causes, len(causes) == 0
 }
 
 // GameServer returns a single GameServer derived

pkg/apis/stable/v1alpha1/gameserverset_test.go

@@ -20,6 +20,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/validation"
 )
 
 func TestGameServerSetGameServer(t *testing.T) {
@@ -69,19 +70,31 @@ func TestGameServerSetValidateUpdate(t *testing.T) {
 		},
 	}
 
-	ok, causes := gsSet.ValidateUpdate(gsSet.DeepCopy())
+	causes, ok := gsSet.ValidateUpdate(gsSet.DeepCopy())
 	assert.True(t, ok)
 	assert.Empty(t, causes)
 
 	newGSS := gsSet.DeepCopy()
 	newGSS.Spec.Replicas = 5
-	ok, causes = gsSet.ValidateUpdate(newGSS)
+	causes, ok = gsSet.ValidateUpdate(newGSS)
 	assert.True(t, ok)
 	assert.Empty(t, causes)
 
 	newGSS.Spec.Template.Spec.Ports[0].ContainerPort = 321
-	ok, causes = gsSet.ValidateUpdate(newGSS)
+	causes, ok = gsSet.ValidateUpdate(newGSS)
 	assert.False(t, ok)
 	assert.Len(t, causes, 1)
 	assert.Equal(t, "template", causes[0].Field)
+
+	newGSS = gsSet.DeepCopy()
+	nameLen := validation.LabelValueMaxLength + 1
+	bytes := make([]byte, nameLen)
+	for i := 0; i < nameLen; i++ {
+		bytes[i] = 'f'
+	}
+	newGSS.Name = string(bytes)
+	causes, ok = newGSS.Validate()
+	assert.False(t, ok)
+	assert.Len(t, causes, 1)
+	assert.Equal(t, "Name", causes[0].Field)
 }

pkg/fleets/controller.go

@@ -19,6 +19,7 @@ import (
 	"reflect"
 
 	"agones.dev/agones/pkg/apis/stable"
+	"agones.dev/agones/pkg/apis/stable/v1alpha1"
 	stablev1alpha1 "agones.dev/agones/pkg/apis/stable/v1alpha1"
 	"agones.dev/agones/pkg/client/clientset/versioned"
 	getterv1alpha1 "agones.dev/agones/pkg/client/clientset/versioned/typed/stable/v1alpha1"
@@ -96,6 +97,8 @@ func NewController(
 	c.recorder = eventBroadcaster.NewRecorder(scheme.Scheme, corev1.EventSource{Component: "fleet-controller"})
 
 	wh.AddHandler("/mutate", stablev1alpha1.Kind("Fleet"), admv1beta1.Create, c.creationMutationHandler)
+	wh.AddHandler("/validate", v1alpha1.Kind("Fleet"), admv1beta1.Create, c.creationValidationHandler)
+	wh.AddHandler("/validate", v1alpha1.Kind("Fleet"), admv1beta1.Update, c.creationValidationHandler)
 
 	fInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
 		AddFunc: c.workerqueue.Enqueue,
@@ -160,6 +163,41 @@ func (c *Controller) creationMutationHandler(review admv1beta1.AdmissionReview)
 	return review, nil
 }
 
+// creationValidationHandler that validates a Fleet when it is created
+// Should only be called on Fleet create and Update operations.
+func (c *Controller) creationValidationHandler(review admv1beta1.AdmissionReview) (admv1beta1.AdmissionReview, error) {
+	c.logger.WithField("review", review).Info("creationValidationHandler")
+
+	obj := review.Request.Object
+	fleet := &stablev1alpha1.Fleet{}
+	err := json.Unmarshal(obj.Raw, fleet)
+	if err != nil {
+		return review, errors.Wrapf(err, "error unmarshalling original Fleet json: %s", obj.Raw)
+	}
+
+	causes, ok := fleet.Validate()
+	if !ok {
+		review.Response.Allowed = false
+		details := metav1.StatusDetails{
+			Name:   review.Request.Name,
+			Group:  review.Request.Kind.Group,
+			Kind:   review.Request.Kind.Kind,
+			Causes: causes,
+		}
+		review.Response.Result = &metav1.Status{
+			Status:  metav1.StatusFailure,
+			Message: "Fleet configuration is invalid",
+			Reason:  metav1.StatusReasonInvalid,
+			Details: &details,
+		}
+
+		c.logger.WithField("review", review).Info("Invalid Fleet")
+		return review, nil
+	}
+
+	return review, nil
+}
+
 // Run the Fleet controller. Will block until stop is closed.
 // Runs threadiness number workers to process the rate limited queue
 func (c *Controller) Run(workers int, stop <-chan struct{}) error {
@@ -434,6 +472,9 @@ func (c *Controller) rollingUpdateRest(fleet *stablev1alpha1.Fleet, rest []*stab
 // updateFleetStatus gets the GameServerSets for this Fleet and then
 // calculates the counts for the status, and updates the Fleet
 func (c *Controller) updateFleetStatus(fleet *stablev1alpha1.Fleet) error {
+
+	c.logger.WithField("key", fleet.Name).Info("Update Fleet Status")
+
 	list, err := ListGameServerSetsByFleetOwner(c.gameServerSetLister, fleet)
 	if err != nil {
 		return err

pkg/gameserversets/controller.go

@@ -112,6 +112,7 @@ func NewController(
 	eventBroadcaster.StartRecordingToSink(&typedcorev1.EventSinkImpl{Interface: kubeClient.CoreV1().Events("")})
 	c.recorder = eventBroadcaster.NewRecorder(scheme.Scheme, corev1.EventSource{Component: "gameserverset-controller"})
 
+	wh.AddHandler("/validate", v1alpha1.Kind("GameServerSet"), admv1beta1.Create, c.creationValidationHandler)
 	wh.AddHandler("/validate", v1alpha1.Kind("GameServerSet"), admv1beta1.Update, c.updateValidationHandler)
 
 	gsSetInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
@@ -180,7 +181,7 @@ func (c *Controller) updateValidationHandler(review admv1beta1.AdmissionReview)
 		return review, errors.Wrapf(err, "error unmarshalling old GameServerSet json: %s", oldObj.Raw)
 	}
 
-	ok, causes := oldGss.ValidateUpdate(newGss)
+	causes, ok := oldGss.ValidateUpdate(newGss)
 	if !ok {
 		review.Response.Allowed = false
 		details := metav1.StatusDetails{
@@ -191,7 +192,42 @@ func (c *Controller) updateValidationHandler(review admv1beta1.AdmissionReview)
 		}
 		review.Response.Result = &metav1.Status{
 			Status:  metav1.StatusFailure,
-			Message: "GameServer update is invalid",
+			Message: "GameServerSet update is invalid",
+			Reason:  metav1.StatusReasonInvalid,
+			Details: &details,
+		}
+
+		c.logger.WithField("review", review).Info("Invalid GameServerSet update")
+		return review, nil
+	}
+
+	return review, nil
+}
+
+// creationValidationHandler that validates a GameServerSet when is created
+// Should only be called on gameserverset create operations.
+func (c *Controller) creationValidationHandler(review admv1beta1.AdmissionReview) (admv1beta1.AdmissionReview, error) {
+	c.logger.WithField("review", review).Info("creationValidationHandler")
+
+	newGss := &v1alpha1.GameServerSet{}
+
+	newObj := review.Request.Object
+	if err := json.Unmarshal(newObj.Raw, newGss); err != nil {
+		return review, errors.Wrapf(err, "error unmarshalling new GameServerSet json: %s", newObj.Raw)
+	}
+
+	causes, ok := newGss.Validate()
+	if !ok {
+		review.Response.Allowed = false
+		details := metav1.StatusDetails{
+			Name:   review.Request.Name,
+			Group:  review.Request.Kind.Group,
+			Kind:   review.Request.Kind.Kind,
+			Causes: causes,
+		}
+		review.Response.Result = &metav1.Status{
+			Status:  metav1.StatusFailure,
+			Message: "GameServerSet update is invalid",
 			Reason:  metav1.StatusReasonInvalid,
 			Details: &details,
 		}

site/content/en/docs/Reference/fleet.md

@@ -55,6 +55,8 @@ attach specific [annotations](https://kubernetes.io/docs/concepts/overview/worki
 and [labels](https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/) to your resource. 
 This is a very common pattern in the Kubernetes ecosystem.
 
+The length of the `name` field of the fleet should be at most 63 characters.
+
 The `spec` field is the actual `Fleet` specification and it is composed as follow:
 
 - `replicas` is the number of `GameServers` to keep Ready or Allocated in this Fleet

test/e2e/fleet_test.go

@@ -30,6 +30,7 @@ import (
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/util/retry"
 )
@@ -489,6 +490,32 @@ func TestFleetAllocationDuringGameServerDeletion(t *testing.T) {
 	})
 }
 
+// TestFleetNameValidation is built to test Fleet Name length validation,
+// Fleet Name should have at most 63 chars
+func TestFleetNameValidation(t *testing.T) {
+	t.Parallel()
+	alpha1 := framework.AgonesClient.StableV1alpha1()
+
+	flt := defaultFleet()
+	nameLen := validation.LabelValueMaxLength + 1
+	bytes := make([]byte, nameLen)
+	for i := 0; i < nameLen; i++ {
+		bytes[i] = 'f'
+	}
+	flt.Name = string(bytes)
+	_, err := alpha1.Fleets(defaultNs).Create(flt)
+	assert.NotNil(t, err)
+	statusErr := err.(*k8serrors.StatusError)
+	assert.Equal(t, statusErr.Status().Details.Causes[0].Type, metav1.CauseTypeFieldValueInvalid)
+	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) {
 	if !k8serrors.IsConflict(err) {
 		// update conflicts are sometimes ok, we simply lost the race.