add schema validation for custom metrics strategy

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

Hide file tree

Showing 8 changed files with 129 additions and 23 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 {
@@ -503,6 +503,11 @@ func (b *Broker) Bind(ctx context.Context, instanceID string, bindingID string,
 		logger.Error("get-binding-configuration-from-request", err)
 		return result, err
 	}
+	policy, err := b.getPolicyFromJsonRawMessage(policyJson, instanceID, details.PlanID)
+	if err != nil {
+		logger.Error("get-default-policy", err)
+		return result, err
+	}
 	bindingConfiguration, err = bindingConfiguration.ValidateOrGetDefaultCustomMetricsStrategy(bindingConfiguration)
 	if err != nil {
 		actionName := "validate-or-get-default-custom-metric-strategy"
@@ -511,11 +516,6 @@ func (b *Broker) Bind(ctx context.Context, instanceID string, bindingID string,
 			err, http.StatusBadRequest, actionName).
 			WithErrorKey(actionName).Build()
 	}
-	policy, err := b.getPolicyFromJsonRawMessage(policyJson, instanceID, details.PlanID)
-	if err != nil {
-		logger.Error("get-default-policy", err)
-		return result, err
-	}
 	defaultCustomMetricsCredentialType := b.conf.DefaultCustomMetricsCredentialType
 	customMetricsBindingAuthScheme, err := getOrDefaultCredentialType(policyJson, defaultCustomMetricsCredentialType, logger)
 	if err != nil {

diff --git a/src/autoscaler/api/brokerserver/broker_handler_test.go b/src/autoscaler/api/brokerserver/broker_handler_test.go
@@ -991,9 +991,8 @@ var _ = Describe("BrokerHandler", func() {
 					verifyScheduleIsUpdatedInScheduler(testAppId, bindingPolicy)
 				})
 				It("should fail with 400", func() {
+					Expect(resp.Body.String()).To(ContainSubstring("{\"description\":\"invalid policy provided: [{\\\"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\\\\\\\"\\\"}]\"}"))
 					Expect(resp.Code).To(Equal(http.StatusBadRequest))
-					Expect(resp.Body.String()).To(MatchJSON(`{"error": "validate-or-get-default-custom-metric-strategy", "description": "error: custom metrics strategy not supported"}`))
-
 				})
 			})
 			When("are empty", func() {
@@ -1048,9 +1047,6 @@ var _ = Describe("BrokerHandler", func() {
 					bindingPolicy = `{
 				  "configuration": {
 					"custom_metrics": {
-					  "auth": {
-						"credential_type": "binding_secret"
-					  },
 					  "metric_submission_strategy": {
 						"allow_from": "bound_app"
 					  }

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
@@ -106,7 +106,7 @@ func (h *PublicApiHandler) GetScalingPolicy(w http.ResponseWriter, r *http.Reque
 	}
 	customMetricStrategy, err := h.bindingdb.GetCustomMetricStrategyByAppId(r.Context(), appId)
 	if err != nil {
-		logger.Error("Failed to retrieve service binding from database", err)
+		logger.Error("Failed to retrieve customMetricStrategy from database", err)
 		writeErrorResponse(w, http.StatusInternalServerError, "Error retrieving binding policy")
 		return
 	}
@@ -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)
@@ -174,7 +174,7 @@ func (h *PublicApiHandler) AttachScalingPolicy(w http.ResponseWriter, r *http.Re
 		return
 	}
 	strategy := validatedBindingConfiguration.GetCustomMetricsStrategy()
-	logger.Info("saving custom metric submission strategy", lager.Data{"strategy": validatedBindingConfiguration.GetCustomMetricsStrategy(), "appId": appId})
+	logger.Info("saving custom metric submission strategy", lager.Data{"strategy": strategy, "appId": appId})
 	err = h.bindingdb.SetOrUpdateCustomMetricStrategy(r.Context(), appId, validatedBindingConfiguration.GetCustomMetricsStrategy(), "update")
 	if err != nil {
 		actionName := "failed to save custom metric submission strategy in the database"

diff --git a/src/autoscaler/api/publicapiserver/public_api_handler_test.go b/src/autoscaler/api/publicapiserver/public_api_handler_test.go
@@ -429,7 +429,7 @@ var _ = Describe("PublicApiHandler", func() {
 				})
 				It("should not succeed and fail with 400", func() {
 					Expect(resp.Code).To(Equal(http.StatusBadRequest))
-					Expect(resp.Body.String()).To(MatchJSON(`{"code":"Bad Request","message":"error: custom metrics strategy not supported"}`))
+					Expect(resp.Body.String()).To(Equal(`[{"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\""}]`))
 				})
 			})
 

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,