diff --git a/install/helm/agones/templates/admissionregistration.yaml b/install/helm/agones/templates/admissionregistration.yaml index 787429793d..bb038310e1 100644 --- a/install/helm/agones/templates/admissionregistration.yaml +++ b/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" diff --git a/install/yaml/install.yaml b/install/yaml/install.yaml index 42673dc826..8720fb760f 100644 --- a/install/yaml/install.yaml +++ b/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" diff --git a/pkg/apis/stable/v1alpha1/fleet.go b/pkg/apis/stable/v1alpha1/fleet.go index caf4a51f6b..b69d3c775f 100644 --- a/pkg/apis/stable/v1alpha1/fleet.go +++ b/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() (bool, []metav1.StatusCause) { + 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 len(causes) == 0, causes +} + // UpperBoundReplicas returns whichever is smaller, // the value i, or the f.Spec.Replicas. func (f *Fleet) UpperBoundReplicas(i int32) int32 { diff --git a/pkg/apis/stable/v1alpha1/fleet_test.go b/pkg/apis/stable/v1alpha1/fleet_test.go index 8c39b37dfc..48bf325405 100644 --- a/pkg/apis/stable/v1alpha1/fleet_test.go +++ b/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) + ok, causes := 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}}, diff --git a/pkg/apis/stable/v1alpha1/gameserverset.go b/pkg/apis/stable/v1alpha1/gameserverset.go index 9511e99f78..cfb8b379bd 100644 --- a/pkg/apis/stable/v1alpha1/gameserverset.go +++ b/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 ( @@ -88,6 +90,20 @@ func (gsSet *GameServerSet) ValidateUpdate(new *GameServerSet) (bool, []metav1.S return len(causes) == 0, causes } +// Validate validates when Create occur. Check the name szie +func (gsSet *GameServerSet) Validate() (bool, []metav1.StatusCause) { + 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 len(causes) == 0, causes +} + // 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 cf56b0d443..02b4eee819 100644 --- a/pkg/apis/stable/v1alpha1/gameserverset_test.go +++ b/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) { @@ -84,4 +85,16 @@ func TestGameServerSetValidateUpdate(t *testing.T) { 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) + ok, causes = newGSS.Validate() + assert.False(t, ok) + assert.Len(t, causes, 1) + assert.Equal(t, "Name", causes[0].Field) } diff --git a/pkg/fleets/controller.go b/pkg/fleets/controller.go index a01aa15c4d..07d0c5739f 100644 --- a/pkg/fleets/controller.go +++ b/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) + } + + ok, causes := 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 diff --git a/pkg/gameserversets/controller.go b/pkg/gameserversets/controller.go index 8f39c711ac..b6ed2776ab 100644 --- a/pkg/gameserversets/controller.go +++ b/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{ @@ -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) + } + + ok, causes := 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, } diff --git a/site/content/en/docs/Reference/fleet.md b/site/content/en/docs/Reference/fleet.md index cb910f2ada..c7adcc9717 100644 --- a/site/content/en/docs/Reference/fleet.md +++ b/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 diff --git a/test/e2e/fleet_test.go b/test/e2e/fleet_test.go index f7445f5b14..5af0ed29b0 100644 --- a/test/e2e/fleet_test.go +++ b/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.