From 0abaf26b574dcc5c1d9023985a6309b5448ca445 Mon Sep 17 00:00:00 2001 From: Arsalan Khan Date: Tue, 26 Nov 2024 15:50:01 +0100 Subject: [PATCH] add schema validation for custom metrics strategy --- src/autoscaler/api/broker/broker.go | 2 +- .../policyvalidator/policy_json.schema.json | 8 +- .../api/policyvalidator/policy_validator.go | 2 +- .../policyvalidator/policy_validator_test.go | 114 +++++++++++++++++- .../api/publicapiserver/public_api_handler.go | 2 +- src/autoscaler/db/sqldb/binding_sqldb.go | 2 +- 6 files changed, 120 insertions(+), 10 deletions(-) diff --git a/src/autoscaler/api/broker/broker.go b/src/autoscaler/api/broker/broker.go index 77b7bf252e..3109a04f45 100644 --- 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 index 2a68fe7d3a..0c1e0a1e45 100644 --- 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 index 6f55d2c0ef..02daa82fe5 100644 --- 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 index db03ddd471..a712acbcbb 100644 --- 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 index ef666480b2..6163158f4d 100644 --- 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 index 884e3599a5..7fa38c3314 100644 --- 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,