add schema validation for custom metrics strategy

cloudfoundry · Nov 26, 2024 · 0abaf26 · 0abaf26
1 parent 7fa2187
commit 0abaf26
Show file tree

Hide file tree

Showing 6 changed files with 120 additions and 10 deletions.
diff --git a/src/autoscaler/api/broker/broker.go b/src/autoscaler/api/broker/broker.go
@@ -171,7 +171,7 @@ func (b *Broker) getPolicyFromJsonRawMessage(policyJson json.RawMessage, instanc
 }
 
 func (b *Broker) validateAndCheckPolicy(rawJson json.RawMessage, instanceID string, planID string) (*models.ScalingPolicy, error) {
-	policy, errResults := b.policyValidator.ValidatePolicy(rawJson)
+	policy, errResults := b.policyValidator.ParseAndValidatePolicy(rawJson)
 	logger := b.logger.Session("validate-and-check-policy", lager.Data{"instanceID": instanceID, "policy": policy, "planID": planID, "errResults": errResults})
 
 	if errResults != nil {

diff --git a/src/autoscaler/api/policyvalidator/policy_json.schema.json b/src/autoscaler/api/policyvalidator/policy_json.schema.json
@@ -31,7 +31,8 @@
               "properties": {
                 "allow_from": {
                   "type": "string",
-                  "enum": [ "bound_app", "same_app"
+                  "enum": [
+                    "bound_app"
                   ]
                 }
               },
@@ -44,10 +45,7 @@
             "metric_submission_strategy"
           ]
         }
-      },
-      "required": [
-        "custom_metrics"
-      ]
+      }
     },
     "instance_min_count": {
       "$id": "#/properties/instance_min_count",

diff --git a/src/autoscaler/api/policyvalidator/policy_validator.go b/src/autoscaler/api/policyvalidator/policy_validator.go
@@ -113,7 +113,7 @@ func NewPolicyValidator(policySchemaPath string, lowerCPUThreshold int, upperCPU
 	return policyValidator
 }
 
-func (pv *PolicyValidator) ValidatePolicy(rawJson json.RawMessage) (*models.ScalingPolicy, ValidationErrors) {
+func (pv *PolicyValidator) ParseAndValidatePolicy(rawJson json.RawMessage) (*models.ScalingPolicy, ValidationErrors) {
 	policyLoader := gojsonschema.NewBytesLoader(rawJson)
 	policy := &models.ScalingPolicy{}
 

diff --git a/src/autoscaler/api/policyvalidator/policy_validator_test.go b/src/autoscaler/api/policyvalidator/policy_validator_test.go
@@ -58,7 +58,7 @@ var _ = Describe("PolicyValidator", func() {
 		)
 	})
 	JustBeforeEach(func() {
-		policy, errResult = policyValidator.ValidatePolicy(json.RawMessage(policyString))
+		policy, errResult = policyValidator.ParseAndValidatePolicy(json.RawMessage(policyString))
 		policyBytes, err := json.Marshal(policy)
 		Expect(err).ToNot(HaveOccurred())
 		policyJson = string(policyBytes)
@@ -2564,6 +2564,118 @@ var _ = Describe("PolicyValidator", func() {
 
 		})
 	})
+	Context("Binding Configuration with custom metrics strategy", func() {
+		When("custom_metrics is missing", func() {
+			BeforeEach(func() {
+				policyString = `{
+                "instance_max_count":4,
+                "instance_min_count":1,
+                "scaling_rules":[
+                    {
+                        "metric_type":"memoryutil",
+                        "breach_duration_secs": 300,
+                        "threshold":90,
+                        "operator":">=",
+                        "adjustment": "+1"
+                    }
+                ]
+            }`
+			})
+			It("should not fail", func() {
+				Expect(errResult).To(BeNil())
+			})
+		})
+		When("allow_from is missing in metric_submission_strategy", func() {
+			BeforeEach(func() {
+				policyString = `{
+                "instance_max_count":4,
+                "instance_min_count":1,
+                "scaling_rules":[
+                    {
+                        "metric_type":"memoryutil",
+                        "breach_duration_secs": 300,
+                        "threshold":90,
+                        "operator":">=",
+                        "adjustment": "+1"
+                    }
+                ],
+                "configuration": {
+                    "custom_metrics": {
+                        "metric_submission_strategy": {}
+                    }
+                }
+            }`
+			})
+			It("should fail", func() {
+				Expect(errResult).To(Equal([]PolicyValidationErrors{
+					{
+						Context:     "(root).configuration.custom_metrics.metric_submission_strategy",
+						Description: "allow_from is required",
+					},
+				}))
+			})
+		})
+		When("allow_from is invalid in metric_submission_strategy", func() {
+			BeforeEach(func() {
+				policyString = `{
+                "instance_max_count":4,
+                "instance_min_count":1,
+                "scaling_rules":[
+                    {
+                        "metric_type":"memoryutil",
+                        "breach_duration_secs": 300,
+                        "threshold":90,
+                        "operator":">=",
+                        "adjustment": "+1"
+                    }
+                ],
+                "configuration": {
+                    "custom_metrics": {
+                        "metric_submission_strategy": {
+                            "allow_from": "invalid_value"
+                        }
+                    }
+                }
+            }`
+			})
+			It("should fail", func() {
+				Expect(errResult).To(Equal([]PolicyValidationErrors{
+					{
+						Context:     "(root).configuration.custom_metrics.metric_submission_strategy.allow_from",
+						Description: "configuration.custom_metrics.metric_submission_strategy.allow_from must be one of the following: \"bound_app\"",
+					},
+				}))
+			})
+		})
+		When("allow_from is valid in metric_submission_strategy", func() {
+			BeforeEach(func() {
+				policyString = `{
+                "instance_max_count":4,
+                "instance_min_count":1,
+                "scaling_rules":[
+                    {
+                        "metric_type":"memoryutil",
+                        "breach_duration_secs": 300,
+                        "threshold":90,
+                        "operator":">=",
+                        "adjustment": "+1"
+                    }
+                ],
+                "configuration": {
+                    "custom_metrics": {
+                        "metric_submission_strategy": {
+                            "allow_from": "bound_app"
+                        }
+                    }
+                }
+            }`
+			})
+			It("should succeed", func() {
+
+				Expect(errResult).To(BeNil())
+			})
+		})
+	})
 
 	Context("If the policy string is valid json with required parameters", func() {
 		BeforeEach(func() {

diff --git a/src/autoscaler/api/publicapiserver/public_api_handler.go b/src/autoscaler/api/publicapiserver/public_api_handler.go
@@ -144,7 +144,7 @@ func (h *PublicApiHandler) AttachScalingPolicy(w http.ResponseWriter, r *http.Re
 		return
 	}
 
-	policy, errResults := h.policyValidator.ValidatePolicy(policyBytes)
+	policy, errResults := h.policyValidator.ParseAndValidatePolicy(policyBytes)
 	if errResults != nil {
 		logger.Info("Failed to validate policy", lager.Data{"errResults": errResults})
 		handlers.WriteJSONResponse(w, http.StatusBadRequest, errResults)

diff --git a/src/autoscaler/db/sqldb/binding_sqldb.go b/src/autoscaler/db/sqldb/binding_sqldb.go
@@ -458,7 +458,7 @@ func (bdb *BindingSQLDB) SetOrUpdateCustomMetricStrategy(ctx context.Context, ap
 	}
 	if rowsAffected, err := result.RowsAffected(); err != nil || rowsAffected == 0 {
 		if customMetricsStrategy == appBinding.CustomMetricsStrategy {
-			bdb.logger.Info(fmt.Sprintf("custom metrics strategy already exists"), lager.Data{"query": query, "customMetricsStrategy": customMetricsStrategy, "bindingId": appBinding, "appId": appId})
+			bdb.logger.Info("custom metrics strategy already exists", lager.Data{"query": query, "customMetricsStrategy": customMetricsStrategy, "bindingId": appBinding, "appId": appId})
 			return nil
 		}
 		bdb.logger.Error(fmt.Sprintf("failed to %s custom metric submission strategy", actionName), err,