Ignore extra fields in policies (#566)

Extra fields in a policy should be silently ignored for backwards compatibility, but never persisted or returned.
cloudfoundry · Apr 12, 2022 · 7a453d0 · 7a453d0
1 parent 475246d
commit 7a453d0
Show file tree

Hide file tree

Showing 7 changed files with 206 additions and 23 deletions.
diff --git a/src/acceptance/api/api_test.go b/src/acceptance/api/api_test.go
@@ -97,6 +97,14 @@ var _ = Describe("AutoScaler Public API", func() {
 			Expect(string(newPolicy)).Should(MatchJSON(policy))
 		})
 
+		It("should succeed to create a valid policy but remove any extra fields", func() {
+			policyWithExtraFields, validPolicy := GenerateDynamicScaleOutPolicyWithExtraFields(1, 2, "memoryused", 30)
+			newPolicy, status := createPolicy(policyWithExtraFields)
+			Expect(status).To(Or(Equal(200), Equal(201)))
+			Expect(string(newPolicy)).ShouldNot(MatchJSON(policyWithExtraFields))
+			Expect(string(newPolicy)).Should(MatchJSON(validPolicy))
+		})
+
 		It("should fail to create an invalid policy", func() {
 			response, status := createPolicy(GenerateDynamicScaleOutPolicy(0, 2, "memoryused", 30))
 			Expect(status).To(Equal(400))
@@ -132,6 +140,14 @@ var _ = Describe("AutoScaler Public API", func() {
 			Expect(string(newPolicy)).Should(MatchJSON(policy))
 		})
 
+		It("should succeed to update a valid policy but remove any extra fields", func() {
+			policyWithExtraFields, validPolicy := GenerateDynamicScaleOutPolicyWithExtraFields(1, 2, "memoryused", memThreshold)
+			newPolicy, status := createPolicy(policyWithExtraFields)
+			Expect(status).To(Or(Equal(200), Equal(201)))
+			Expect(string(newPolicy)).ShouldNot(MatchJSON(policyWithExtraFields))
+			Expect(string(newPolicy)).Should(MatchJSON(validPolicy))
+		})
+
 		It("should fail to update an invalid policy", func() {
 			By("return 400 when the new policy is invalid")
 			_, status := createPolicy(GenerateDynamicScaleOutPolicy(0, 2, "memoryused", 30))

diff --git a/src/acceptance/helpers/helpers.go b/src/acceptance/helpers/helpers.go
@@ -61,6 +61,16 @@ type ScalingPolicy struct {
 	Schedules    *ScalingSchedules `json:"schedules,omitempty"`
 }
 
+type ScalingPolicyWithExtraFields struct {
+	IsAdmin      bool                           `json:"is_admin"`
+	IsSSO        bool                           `json:"is_sso"`
+	Role         string                         `json:"role"`
+	InstanceMin  int                            `json:"instance_min_count"`
+	InstanceMax  int                            `json:"instance_max_count"`
+	ScalingRules []*ScalingRulesWithExtraFields `json:"scaling_rules,omitempty"`
+	Schedules    *ScalingSchedules              `json:"schedules,omitempty"`
+}
+
 type ScalingRule struct {
 	MetricType            string `json:"metric_type"`
 	BreachDurationSeconds int    `json:"breach_duration_secs"`
@@ -70,6 +80,11 @@ type ScalingRule struct {
 	Adjustment            string `json:"adjustment"`
 }
 
+type ScalingRulesWithExtraFields struct {
+	StatsWindowSeconds int `json:"stats_window_secs"`
+	ScalingRule
+}
+
 type ScalingSchedules struct {
 	Timezone              string                  `json:"timezone,omitempty"`
 	RecurringSchedules    []*RecurringSchedule    `json:"recurring_schedule,omitempty"`
@@ -137,6 +152,47 @@ func GenerateDynamicScaleOutPolicy(instanceMin, instanceMax int, metricName stri
 	return string(bytes)
 }
 
+func GenerateDynamicScaleOutPolicyWithExtraFields(instanceMin, instanceMax int, metricName string, threshold int64) (string, string) {
+	scalingOutRule := ScalingRule{
+		MetricType:            metricName,
+		BreachDurationSeconds: TestBreachDurationSeconds,
+		Threshold:             threshold,
+		Operator:              ">=",
+		CoolDownSeconds:       TestCoolDownSeconds,
+		Adjustment:            "+1",
+	}
+
+	scalingOutRuleWithExtraFields := ScalingRulesWithExtraFields{
+		StatsWindowSeconds: 666,
+		ScalingRule:        scalingOutRule,
+	}
+
+	policy := ScalingPolicy{
+		InstanceMin:  instanceMin,
+		InstanceMax:  instanceMax,
+		ScalingRules: []*ScalingRule{&scalingOutRule},
+	}
+
+	policyWithExtraFields := ScalingPolicyWithExtraFields{
+		IsAdmin:      true,
+		IsSSO:        true,
+		Role:         "admin",
+		InstanceMin:  instanceMin,
+		InstanceMax:  instanceMax,
+		ScalingRules: []*ScalingRulesWithExtraFields{&scalingOutRuleWithExtraFields},
+	}
+
+	validBytes, err := MarshalWithoutHTMLEscape(policy)
+	Expect(err).NotTo(HaveOccurred())
+
+	extraBytes, err := MarshalWithoutHTMLEscape(policyWithExtraFields)
+	Expect(err).NotTo(HaveOccurred())
+
+	Expect(extraBytes).NotTo(MatchJSON(validBytes))
+
+	return string(extraBytes), string(validBytes)
+}
+
 func GenerateDynamicScaleInPolicy(instanceMin, instanceMax int, metricName string, threshold int64) string {
 	scalingInRule := ScalingRule{
 		MetricType:            metricName,

diff --git a/src/autoscaler/api/brokerserver/broker_handler.go b/src/autoscaler/api/brokerserver/broker_handler.go
@@ -122,12 +122,13 @@ func (h *BrokerHandler) CreateServiceInstance(w http.ResponseWriter, r *http.Req
 	}
 	policyGuidStr := ""
 	if policyStr != "" {
-		errResults, valid := h.policyValidator.ValidatePolicy(policyStr)
+		errResults, valid, validatedPolicy := h.policyValidator.ValidatePolicy(policyStr)
 		if !valid {
 			h.logger.Error("failed to validate policy", err, lager.Data{"instanceId": instanceId, "policy": policyStr})
 			handlers.WriteJSONResponse(w, http.StatusBadRequest, errResults)
 			return
 		}
+		policyStr = validatedPolicy
 
 		if h.planDefinitionExceeded(policyStr, body.PlanID, instanceId, w) {
 			return
@@ -322,12 +323,13 @@ func (h *BrokerHandler) UpdateServiceInstance(w http.ResponseWriter, r *http.Req
 	h.logger.Info("update-service-instance", lager.Data{"instanceId": instanceId, "serviceId": body.ServiceID, "planId": body.PlanID, "updatedDefaultPolicy": updatedDefaultPolicy})
 
 	if updatedDefaultPolicy != "" && updatedDefaultPolicyGuid == "" {
-		errResults, valid := h.policyValidator.ValidatePolicy(updatedDefaultPolicy)
+		errResults, valid, validatedPolicy := h.policyValidator.ValidatePolicy(updatedDefaultPolicy)
 		if !valid {
 			h.logger.Error("failed to validate policy", err, lager.Data{"instanceId": instanceId, "policy": updatedDefaultPolicy})
 			handlers.WriteJSONResponse(w, http.StatusBadRequest, errResults)
 			return
 		}
+		updatedDefaultPolicy = validatedPolicy
 
 		servicePlan, err := h.cfClient.GetServicePlan(instanceId)
 		if err != nil {
@@ -525,12 +527,13 @@ func (h *BrokerHandler) BindServiceInstance(w http.ResponseWriter, r *http.Reque
 
 	policyStr := string(body.Policy)
 	if policyStr != "" {
-		errResults, valid := h.policyValidator.ValidatePolicy(policyStr)
+		errResults, valid, validatedPolicyStr := h.policyValidator.ValidatePolicy(policyStr)
 		if !valid {
 			h.logger.Error("failed to validate policy", err, lager.Data{"appId": body.AppID, "policy": policyStr})
 			handlers.WriteJSONResponse(w, http.StatusBadRequest, errResults)
 			return
 		}
+		policyStr = validatedPolicyStr
 
 		if h.planDefinitionExceeded(policyStr, body.PlanID, instanceId, w) {
 			return

diff --git a/src/autoscaler/api/policyvalidator/policy_validator.go b/src/autoscaler/api/policyvalidator/policy_validator.go
@@ -83,19 +83,19 @@ func NewPolicyValidator(policySchemaPath string, lowerThreshold int, upperThresh
 	return policyValidator
 }
 
-func (pv *PolicyValidator) ValidatePolicy(policyStr string) (*[]PolicyValidationErrors, bool) {
+func (pv *PolicyValidator) ValidatePolicy(policyStr string) (*[]PolicyValidationErrors, bool, string) {
 	policyLoader := gojsonschema.NewStringLoader(policyStr)
 
 	result, err := gojsonschema.Validate(pv.policySchemaLoader, policyLoader)
 	if err != nil {
 		resultErrors := []PolicyValidationErrors{
 			{Context: "(root)", Description: err.Error()},
 		}
-		return &resultErrors, false
+		return &resultErrors, false, ""
 	}
 
 	if !result.Valid() {
-		return getErrorsObject(result.Errors()), false
+		return getErrorsObject(result.Errors()), false, ""
 	}
 
 	policy := models.ScalingPolicy{}
@@ -104,15 +104,24 @@ func (pv *PolicyValidator) ValidatePolicy(policyStr string) (*[]PolicyValidation
 		resultErrors := []PolicyValidationErrors{
 			{Context: "(root)", Description: err.Error()},
 		}
-		return &resultErrors, false
+		return &resultErrors, false, ""
 	}
 
 	pv.validateAttributes(&policy, result)
 
 	if len(result.Errors()) > 0 {
-		return getErrorsObject(result.Errors()), false
+		return getErrorsObject(result.Errors()), false, ""
 	}
-	return nil, true
+
+	validatedPolicyStr, err := json.Marshal(policy)
+	if err != nil {
+		resultErrors := []PolicyValidationErrors{
+			{Context: "(root)", Description: err.Error()},
+		}
+		return &resultErrors, false, ""
+	}
+
+	return nil, true, string(validatedPolicyStr)
 }
 
 func (pv *PolicyValidator) validateAttributes(policy *models.ScalingPolicy, result *gojsonschema.Result) {