Fix validation bug in FleetAutoscaler (googleforgames#2242)

* Fix validation bug in FleetAutoscaler * Fix bug when sync was not set for FleetAutoscaler * Regenerate install.yaml from helm template * Add test for CustomFasSyncInterval in FleetAutoscaler

Author: yoshd

install/helm/agones/templates/extensions.yaml

@@ -137,6 +137,15 @@ webhooks:
  - "v1"
 operations:
  - CREATE
+ - apiGroups:
+ - autoscaling.agones.dev
+resources:
+ - "fleetautoscalers"
+apiVersions:
+ - "v1"
+operations:
+ - CREATE
+ - UPDATE
 {{- end }}
 ---
 apiVersion: v1

install/yaml/install.yaml

@@ -13591,7 +13591,16 @@ webhooks:
  apiVersions:
  - "v1"
  operations:
- - CREATE
+ - CREATE
+ - apiGroups:
+ - autoscaling.agones.dev
+ resources:
+ - "fleetautoscalers"
+ apiVersions:
+ - "v1"
+ operations:
+ - CREATE
+ - UPDATE
 ---
 # Source: agones/templates/priority-class.yaml
 apiVersion: scheduling.k8s.io/v1

pkg/apis/autoscaling/v1/fleetautoscaler.go

@@ -204,7 +204,9 @@ func (fas *FleetAutoscaler) Validate(causes []metav1.StatusCause) []metav1.Statu
 causes=fas.Spec.Policy.Webhook.ValidateWebhookPolicy(causes)
  }
 
-causes=fas.Spec.Sync.FixedInterval.ValidateFixedIntervalSync(causes)
+ifruntime.FeatureEnabled(runtime.FeatureCustomFasSyncInterval){
+causes=fas.Spec.Sync.FixedInterval.ValidateFixedIntervalSync(causes)
+ }
 returncauses
 }
 

pkg/apis/autoscaling/v1/fleetautoscaler_test.go

@@ -17,6 +17,7 @@ package v1
 import (
 "testing"
 
+"agones.dev/agones/pkg/util/runtime"
 "github.com/stretchr/testify/assert"
  admregv1 "k8s.io/api/admissionregistration/v1"
  metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -114,6 +115,11 @@ func TestFleetAutoscalerValidateUpdate(t *testing.T){
  })
 
 t.Run("bad sync interval seconds", func(t*testing.T){
+if!runtime.FeatureEnabled(runtime.FeatureCustomFasSyncInterval){
+// Do not run test if FeatureCustomFasSyncInterval is not enabled
+t.Skip()
+ }
+
 fas:=defaultFixture()
 fas.Spec.Sync.FixedInterval.Seconds=0
 

pkg/fleetautoscalers/controller.go

@@ -36,6 +36,7 @@ import (
 "agones.dev/agones/pkg/util/webhooks"
 "agones.dev/agones/pkg/util/workerqueue"
 "github.com/heptiolabs/healthcheck"
+"github.com/mattbaird/jsonpatch"
 "github.com/pkg/errors"
 "github.com/sirupsen/logrus"
  admissionv1 "k8s.io/api/admission/v1"
@@ -107,6 +108,8 @@ func NewController(
 c.recorder=eventBroadcaster.NewRecorder(scheme.Scheme, corev1.EventSource{Component: "fleetautoscaler-controller"})
 
 kind:=autoscalingv1.Kind("FleetAutoscaler")
+wh.AddHandler("/mutate", kind, admissionv1.Create, c.mutationHandler)
+wh.AddHandler("/mutate", kind, admissionv1.Update, c.mutationHandler)
 wh.AddHandler("/validate", kind, admissionv1.Create, c.validationHandler)
 wh.AddHandler("/validate", kind, admissionv1.Update, c.validationHandler)
 
@@ -165,6 +168,41 @@ func (c *Controller) loggerForFleetAutoscaler(fas *autoscalingv1.FleetAutoscaler
 returnc.loggerForFleetAutoscalerKey(fasName).WithField("fas", fas)
 }
 
+// creationMutationHandler is the handler for the mutating webhook that sets the
+// the default values on the FleetAutoscaler
+func (c*Controller) mutationHandler(review admissionv1.AdmissionReview) (admissionv1.AdmissionReview, error){
+obj:=review.Request.Object
+fas:=&autoscalingv1.FleetAutoscaler{}
+err:=json.Unmarshal(obj.Raw, fas)
+iferr!=nil{
+c.baseLogger.WithField("review", review).WithError(err).Error("validationHandler")
+returnreview, errors.Wrapf(err, "error unmarshalling original FleetAutoscaler json: %s", obj.Raw)
+ }
+
+fas.ApplyDefaults()
+
+newFas, err:=json.Marshal(fas)
+iferr!=nil{
+returnreview, errors.Wrapf(err, "error marshalling default applied FleetAutoscaler %s to json", fas.ObjectMeta.Name)
+ }
+
+patch, err:=jsonpatch.CreatePatch(obj.Raw, newFas)
+iferr!=nil{
+returnreview, errors.Wrapf(err, "error creating patch for FleetAutoscaler %s", fas.ObjectMeta.Name)
+ }
+
+jsonPatch, err:=json.Marshal(patch)
+iferr!=nil{
+returnreview, errors.Wrapf(err, "error creating json for patch for FleetAutoScaler %s", fas.ObjectMeta.Name)
+ }
+
+pt:=admissionv1.PatchTypeJSONPatch
+review.Response.PatchType=&pt
+review.Response.Patch=jsonPatch
+
+returnreview, nil
+}
+
 // validationHandler will intercept when a FleetAutoscaler is created, and
 // validate its settings.
 func (c*Controller) validationHandler(review admissionv1.AdmissionReview) (admissionv1.AdmissionReview, error){

pkg/fleetautoscalers/controller_test.go

@@ -30,8 +30,10 @@ import (
  utilruntime "agones.dev/agones/pkg/util/runtime"
 "agones.dev/agones/pkg/util/webhooks"
 "github.com/heptiolabs/healthcheck"
+"github.com/mattbaird/jsonpatch"
 "github.com/pkg/errors"
 "github.com/stretchr/testify/assert"
+"github.com/stretchr/testify/require"
  admissionv1 "k8s.io/api/admission/v1"
  admregv1 "k8s.io/api/admissionregistration/v1"
  metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -63,6 +65,142 @@ var (
 gvk=metav1.GroupVersionKind(agonesv1.SchemeGroupVersion.WithKind("FleetAutoscaler"))
 )
 
+funcTestControllerCreationMutationHandler(t*testing.T){
+t.Parallel()
+
+typeexpectedstruct{
+responseAllowedbool
+patches []jsonpatch.JsonPatchOperation
+errstring
+ }
+
+vartestCases= []struct{
+descriptionstring
+fixtureinterface{}
+expectedexpected
+ }{
+{
+description: "OK",
+fixture: &autoscalingv1.FleetAutoscaler{
+ObjectMeta: metav1.ObjectMeta{
+Name: "fas-1",
+Namespace: "default",
+Generation: 2,
+ },
+Spec: autoscalingv1.FleetAutoscalerSpec{
+FleetName: "fleet-1",
+Policy: autoscalingv1.FleetAutoscalerPolicy{
+Type: autoscalingv1.BufferPolicyType,
+Buffer: &autoscalingv1.BufferPolicy{
+BufferSize: intstr.FromInt(5),
+MaxReplicas: 100,
+ },
+ },
+Sync: &autoscalingv1.FleetAutoscalerSync{
+Type: autoscalingv1.FixedIntervalSyncType,
+FixedInterval: autoscalingv1.FixedIntervalSync{
+Seconds: 30,
+ },
+ },
+ },
+ },
+expected: expected{
+responseAllowed: true,
+patches: []jsonpatch.JsonPatchOperation{},
+ },
+ },
+{
+description: "OK",
+// Spec.Sync is not defined
+fixture: &autoscalingv1.FleetAutoscaler{
+ObjectMeta: metav1.ObjectMeta{
+Name: "fas-1",
+Namespace: "default",
+Generation: 2,
+ },
+Spec: autoscalingv1.FleetAutoscalerSpec{
+FleetName: "fleet-1",
+Policy: autoscalingv1.FleetAutoscalerPolicy{
+Type: autoscalingv1.BufferPolicyType,
+Buffer: &autoscalingv1.BufferPolicy{
+BufferSize: intstr.FromInt(5),
+MaxReplicas: 100,
+ },
+ },
+ },
+ },
+expected: expected{
+responseAllowed: true,
+patches: []jsonpatch.JsonPatchOperation{
+{
+Operation: "add",
+Path: "/spec/sync",
+Value: map[string]interface{}{
+"fixedInterval": map[string]interface{}{
+"seconds": float64(30),
+ },
+"type": "FixedInterval",
+ },
+ },
+ },
+ },
+ },
+{
+description: "Wrong request object, err expected",
+fixture: "WRONG DATA",
+expected: expected{
+err: `error unmarshalling original FleetAutoscaler json: "WRONG DATA": json: cannot unmarshal string into Go value of type v1.FleetAutoscaler`,
+ },
+ },
+ }
+
+c, _:=newFakeController()
+
+for_, tc:=rangetestCases{
+t.Run(tc.description, func(t*testing.T){
+raw, err:=json.Marshal(tc.fixture)
+require.NoError(t, err)
+
+review:= admissionv1.AdmissionReview{
+Request: &admissionv1.AdmissionRequest{
+Kind: gvk,
+Operation: admissionv1.Create,
+Object: runtime.RawExtension{
+Raw: raw,
+ },
+ },
+Response: &admissionv1.AdmissionResponse{Allowed: true},
+ }
+
+result, err:=c.mutationHandler(review)
+
+iferr!=nil&&tc.expected.err!=""{
+require.Equal(t, tc.expected.err, err.Error())
+ } else{
+assert.True(t, result.Response.Allowed)
+assert.Equal(t, admissionv1.PatchTypeJSONPatch, *result.Response.PatchType)
+
+patch:=&jsonpatch.ByPath{}
+err=json.Unmarshal(result.Response.Patch, patch)
+require.NoError(t, err)
+
+ifutilruntime.FeatureEnabled(utilruntime.FeatureCustomFasSyncInterval){
+found:=false
+
+for_, expected:=rangetc.expected.patches{
+for_, p:=range*patch{
+ifassert.ObjectsAreEqual(p, expected){
+found=true
+ }
+ }
+assert.True(t, found, "Could not find operation %#v in patch %v", expected, *patch)
+ }
+ }
+ }
+ })
+ }
+}
+
 funcTestControllerCreationValidationHandler(t*testing.T){
 t.Parallel()
 

test/e2e/fleetautoscaler_test.go

@@ -24,6 +24,7 @@ import (
 
  agonesv1 "agones.dev/agones/pkg/apis/agones/v1"
  autoscalingv1 "agones.dev/agones/pkg/apis/autoscaling/v1"
+"agones.dev/agones/pkg/util/runtime"
  e2e "agones.dev/agones/test/e2e/framework"
 "github.com/pkg/errors"
 "github.com/sirupsen/logrus"
@@ -60,7 +61,8 @@ func TestAutoscalerBasicFunctions(t *testing.T){
 framework.AssertFleetCondition(t, flt, e2e.FleetReadyCount(flt.Spec.Replicas))
 
 fleetautoscalers:=framework.AgonesClient.AutoscalingV1().FleetAutoscalers(framework.Namespace)
-fas, err:=fleetautoscalers.Create(ctx, defaultFleetAutoscaler(flt, framework.Namespace), metav1.CreateOptions{})
+defaultFas:=defaultFleetAutoscaler(flt, framework.Namespace)
+fas, err:=fleetautoscalers.Create(ctx, defaultFas, metav1.CreateOptions{})
 ifassert.Nil(t, err){
 deferfleetautoscalers.Delete(ctx, fas.ObjectMeta.Name, metav1.DeleteOptions{}) // nolint:errcheck
  } else{
@@ -69,6 +71,11 @@ func TestAutoscalerBasicFunctions(t *testing.T){
 return
  }
 
+// If the CustomFasSyncInterval feature flag is enabled, the value of fas.spec.sync are equal
+ifruntime.FeatureEnabled(runtime.FeatureCustomFasSyncInterval){
+assert.Equal(t, defaultFas.Spec.Sync.FixedInterval.Seconds, fas.Spec.Sync.FixedInterval.Seconds)
+ }
+
 // the fleet autoscaler should scale the fleet up now up to BufferSize
 bufferSize:=int32(fas.Spec.Policy.Buffer.BufferSize.IntValue())
 framework.AssertFleetCondition(t, flt, e2e.FleetReadyCount(bufferSize))
@@ -132,6 +139,54 @@ func TestAutoscalerBasicFunctions(t *testing.T){
  })
 }
 
+funcTestFleetAutoscalerDefaultSyncInterval(t*testing.T){
+t.Parallel()
+ctx:=context.Background()
+
+stable:=framework.AgonesClient.AgonesV1()
+fleets:=stable.Fleets(framework.Namespace)
+flt, err:=fleets.Create(ctx, defaultFleet(framework.Namespace), metav1.CreateOptions{})
+ifassert.Nil(t, err){
+deferfleets.Delete(ctx, flt.ObjectMeta.Name, metav1.DeleteOptions{}) // nolint:errcheck
+ }
+
+framework.AssertFleetCondition(t, flt, e2e.FleetReadyCount(flt.Spec.Replicas))
+
+fleetautoscalers:=framework.AgonesClient.AutoscalingV1().FleetAutoscalers(framework.Namespace)
+dummyFleetName:="dummy-fleet"
+defaultFas:=&autoscalingv1.FleetAutoscaler{
+ObjectMeta: metav1.ObjectMeta{
+Name: dummyFleetName+"-autoscaler",
+Namespace: framework.Namespace,
+ },
+Spec: autoscalingv1.FleetAutoscalerSpec{
+FleetName: dummyFleetName,
+Policy: autoscalingv1.FleetAutoscalerPolicy{
+Type: autoscalingv1.BufferPolicyType,
+Buffer: &autoscalingv1.BufferPolicy{
+BufferSize: intstr.FromInt(3),
+MaxReplicas: 10,
+ },
+ },
+ },
+ }
+fas, err:=fleetautoscalers.Create(ctx, defaultFas, metav1.CreateOptions{})
+ifassert.Nil(t, err){
+deferfleetautoscalers.Delete(ctx, fas.ObjectMeta.Name, metav1.DeleteOptions{}) // nolint:errcheck
+ } else{
+// if we could not create the autoscaler, their is no point going further
+logrus.Error("Failed creating autoscaler, aborting TestFleetAutoscalerDefaultSyncInterval")
+return
+ }
+
+// If the CustomFasSyncInterval feature flag is enabled, fas.spec.sync should be set to its default value
+ifruntime.FeatureEnabled(runtime.FeatureCustomFasSyncInterval){
+defaultSyncIntervalFas:=&autoscalingv1.FleetAutoscaler{}
+defaultSyncIntervalFas.ApplyDefaults()
+assert.Equal(t, defaultSyncIntervalFas.Spec.Sync.FixedInterval.Seconds, fas.Spec.Sync.FixedInterval.Seconds)
+ }
+}
+
 // TestFleetAutoScalerRollingUpdate - test fleet with RollingUpdate strategy work with
 // FleetAutoscaler, verify that number of GameServers does not goes down below RollingUpdate strategy
 // defined level on Fleet updates.