diff --git a/src/autoscaler/api/broker/broker.go b/src/autoscaler/api/broker/broker.go index 77b7bf252e..250cc886da 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 { @@ -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 index 8f4289ef01..7c23f7be89 100644 --- 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 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..8f896986de 100644 --- 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 index 8fb630c297..35b4dfd125 100644 --- 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 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,