diff --git a/api/application-metric-api.openapi.yaml b/api/application-metric-api.openapi.yaml index cdcdd95f86..3d459df775 100644 --- a/api/application-metric-api.openapi.yaml +++ b/api/application-metric-api.openapi.yaml @@ -2,7 +2,7 @@ openapi: 3.0.0 info: title: Application Metric API description: | - List aggregated metrics of an application. AutoScaler collects the instances' metrics of an + List aggregated metrics of an application. AutoScaler collects the instances metrics of an application, and aggregate the raw data into an accumulated value for evaluation. This API is used to return the aggregated metric result of an application. @@ -22,7 +22,7 @@ paths: in: path required: true description: | - The GUID identifying the application for which the scaling history is fetched. + The GUID identifying the application for which the aggregated metric histories is fetched. It can be found in the `application_id` property of the JSON object stored in the `VCAP_APPLICATION` environment variable. diff --git a/api/policy-api.openapi.yaml b/api/policy-api.openapi.yaml index 6860cfd263..c00dc18992 100644 --- a/api/policy-api.openapi.yaml +++ b/api/policy-api.openapi.yaml @@ -20,14 +20,14 @@ paths: in: path required: true description: | - The GUID identifying the application for which the scaling history is fetched. + The GUID identifying the application for which the scaling policy is fetched. It can be found in the `application_id` property of the JSON object stored in the `VCAP_APPLICATION` environment variable. schema: $ref: "./shared_definitions.yaml#/schemas/GUID" put: - summary: Retrieves the Policy + summary: Create the Policy description: This API is used to create the policy tags: - Create Policy API V1 @@ -78,8 +78,11 @@ paths: components: schemas: Policy: - description: Object containing Policy + description: Object containing policy and optional configuration type: object + required: + - instance_min_count + - instance_max_count properties: instance_min_count: type: integer @@ -95,6 +98,23 @@ components: type: array items: $ref: '#/components/schemas/ScalingRule' + configuration: + type: object + properties: + custom_metrics: + type: object + properties: + metric_submission_strategy: + type: object + properties: + allow_from: + type: string + enum: + - bound_app + required: + - allow_from + required: + - metric_submission_strategy ScalingRule: type: object required: diff --git a/docs/public-api.restclient b/docs/public-api.restclient new file mode 100644 index 0000000000..261d6f61ae --- /dev/null +++ b/docs/public-api.restclient @@ -0,0 +1,88 @@ +###### +# Collection of rest endpoints offered by Application Autoscaler public api server +# These endpoints allows CRUD operations for the policy object +# This endpoints works with vs code rest client extension +######## + +@system_domain = app-runtime-interfaces.ci.cloudfoundry.org +@baseUrl = https://autoscaler.{{system_domain}} +@auth_token = bearer + +### app guid +@go_app_guid = 3e4d6cd4-08a3-4289-b09e-16333e1895c1 + +### check server health +GET {{baseUrl}}/health + + +### show current app policy +GET {{baseUrl}}/v1/apps/{{go_app_guid}}/policy +Authorization: {{auth_token}} + +### create a policy of a given app with configuration object +PUT {{baseUrl}}/v1/apps/{{consumer_guid}}/policy +content-type: application/json +Authorization: {{auth_token}} + +{ + "configuration": { + "custom_metrics": { + "metric_submission_strategy": { + "allow_from": "bound_app" + } + } + }, + "instance_min_count": 1, + "instance_max_count": 4, + "scaling_rules": [ + { + "metric_type": "test_metric", + "breach_duration_secs": 60, + "threshold": 200, + "operator": ">=", + "cool_down_secs": 60, + "adjustment": "+1" + }, + { + "metric_type": "test_metric", + "breach_duration_secs": 60, + "threshold": 100, + "operator": "<=", + "cool_down_secs": 60, + "adjustment": "-1" + } + ] +} + +### update a policy of a given app - wihout configuration +PUT {{baseUrl}}/v1/apps/{{go_app_guid}}/policy +content-type: application/json +Authorization: {{auth_token}} + +{ + "instance_min_count": 1, + "instance_max_count": 4, + "scaling_rules": [ + { + "metric_type": "test_metric", + "breach_duration_secs": 60, + "threshold": 200, + "operator": ">=", + "cool_down_secs": 60, + "adjustment": "+1" + }, + { + "metric_type": "test_metric", + "breach_duration_secs": 60, + "threshold": 100, + "operator": "<=", + "cool_down_secs": 60, + "adjustment": "-1" + } + ] +} + +### Delete a policy of an app +DELETE {{baseUrl}}/v1/apps/{{go_app_guid}}/policy +Authorization: {{auth_token}} + diff --git a/src/acceptance/api/api_test.go b/src/acceptance/api/api_test.go index 17fb5d0bd9..f190887f30 100644 --- a/src/acceptance/api/api_test.go +++ b/src/acceptance/api/api_test.go @@ -112,9 +112,30 @@ var _ = Describe("AutoScaler Public API", func() { Expect(string(response)).Should(ContainSubstring(`[{"context":"(root).instance_min_count","description":"Must be greater than or equal to 1"}]`)) }) + It("should fail to create an invalid custom metrics submission", func() { + By("creating custom metrics submission with invalid string") + response, status := createPolicy(GenerateBindingsWithScalingPolicy("invalid-value", 1, 2, "memoryused", 30, 100)) + Expect(string(response)).Should(MatchJSON(`[{"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(status).To(Equal(400)) + + By("creating custom metrics submission with empty value ' '") + policy := GenerateBindingsWithScalingPolicy("", 1, 2, "memoryused", 30, 100) + newPolicy, status := createPolicy(policy) + Expect(string(newPolicy)).Should(MatchJSON(`[{"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(status).To(Equal(400)) + }) + + It("should succeed to create an valid custom metrics submission", func() { + By("creating custom metrics submission with 'bound_app'") + policy := GenerateBindingsWithScalingPolicy("bound_app", 1, 2, "memoryused", 30, 100) + response, status := createPolicy(policy) + Expect(string(response)).Should(MatchJSON(policy)) + Expect(status).To(Or(Equal(200), Equal(201))) + }) + }) - When("a scaling policy is set", func() { + When("a scaling policy is set without custom metric strategy", func() { memThreshold := int64(10) var policy string @@ -204,7 +225,6 @@ var _ = Describe("AutoScaler Public API", func() { BeforeEach(func() { UnbindServiceFromApp(cfg, appName, instanceName) }) - It("should not be possible to get information from the API", func() { By("getting the policy") _, status := getPolicy() @@ -219,7 +239,51 @@ var _ = Describe("AutoScaler Public API", func() { Expect(status).To(Equal(http.StatusForbidden)) }) }) + }) + When("a scaling policy is set with custom metric strategy", func() { + var status int + var expectedPolicy string + var actualPolicy []byte + BeforeEach(func() { + BindServiceToApp(cfg, appName, instanceName) + expectedPolicy = GenerateBindingsWithScalingPolicy("bound_app", 1, 2, "memoryused", 30, 100) + actualPolicy, status = createPolicy(expectedPolicy) + Expect(status).To(Equal(200)) + }) + It("should succeed to delete a custom metrics strategy", func() { + _, status = deletePolicy() + Expect(status).To(Equal(200)) + }) + It("should succeed to get a custom metrics strategy", func() { + actualPolicy, status = getPolicy() + Expect(string(actualPolicy)).Should(MatchJSON(expectedPolicy)) + Expect(status).To(Equal(200)) + }) + It("should fail to update an invalid custom metrics strategy", func() { + expectedPolicy = GenerateBindingsWithScalingPolicy("invalid-update", 1, 2, "memoryused", 30, 100) + actualPolicy, status = createPolicy(expectedPolicy) + Expect(string(actualPolicy)).Should(MatchJSON(`[{"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(status).To(Equal(400)) + }) + It("should succeed to update a valid custom metrics strategy", func() { + + Expect(string(actualPolicy)).Should(MatchJSON(expectedPolicy)) + Expect(status).To(Equal(200)) + }) + + When("custom metrics strategy is removed from the existing policy", func() { + It("should removed the custom metrics strategy and displays policy only", func() { + Expect(string(actualPolicy)).Should(MatchJSON(expectedPolicy), "policy and custom metrics strategy should be present") + Expect(status).To(Equal(200)) + + By("updating policy without custom metrics strategy") + expectedPolicy = GenerateDynamicScaleOutPolicy(1, 2, "memoryused", 30) + actualPolicy, status = createPolicy(expectedPolicy) + Expect(string(actualPolicy)).Should(MatchJSON(expectedPolicy), "policy should be present only") + Expect(status).To(Equal(200)) + }) + }) }) }) diff --git a/src/acceptance/helpers/helpers.go b/src/acceptance/helpers/helpers.go index 1cb2124b3c..ee3c2bc9f4 100644 --- a/src/acceptance/helpers/helpers.go +++ b/src/acceptance/helpers/helpers.go @@ -38,23 +38,26 @@ type BindingConfig struct { Configuration Configuration `json:"configuration"` ScalingPolicy } - type Configuration struct { CustomMetrics CustomMetricsConfig `json:"custom_metrics"` } type CustomMetricsConfig struct { - Auth Auth `json:"auth,omitempty"` MetricSubmissionStrategy MetricsSubmissionStrategy `json:"metric_submission_strategy"` } -type Auth struct { - CredentialType string `json:"credential_type"` -} type MetricsSubmissionStrategy struct { AllowFrom string `json:"allow_from"` } +func (b *BindingConfig) GetCustomMetricsStrategy() string { + return b.Configuration.CustomMetrics.MetricSubmissionStrategy.AllowFrom +} + +func (b *BindingConfig) SetCustomMetricsStrategy(allowFrom string) { + b.Configuration.CustomMetrics.MetricSubmissionStrategy.AllowFrom = allowFrom +} + type ScalingPolicy struct { InstanceMin int `json:"instance_min_count"` InstanceMax int `json:"instance_max_count"` @@ -184,7 +187,7 @@ func ServicePlansUrl(cfg *config.Config, spaceGuid string) string { } func GenerateBindingsWithScalingPolicy(allowFrom string, instanceMin, instanceMax int, metricName string, scaleInThreshold, scaleOutThreshold int64) string { - bindingConfig := BindingConfig{ + bindingConfig := &BindingConfig{ Configuration: Configuration{CustomMetrics: CustomMetricsConfig{ MetricSubmissionStrategy: MetricsSubmissionStrategy{AllowFrom: allowFrom}, }}, diff --git a/src/autoscaler/api/broker/broker.go b/src/autoscaler/api/broker/broker.go index 19196d5fa6..f347fb4402 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,16 +503,19 @@ func (b *Broker) Bind(ctx context.Context, instanceID string, bindingID string, logger.Error("get-binding-configuration-from-request", err) return result, err } - bindingConfiguration, err = b.validateOrGetDefaultCustomMetricsStrategy(bindingConfiguration, logger) - if err != nil { - logger.Error("validate-or-get-default-custom-metric-strategy", 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() + if err != nil { + actionName := "validate-or-get-default-custom-metric-strategy" + logger.Error(actionName, err) + return result, apiresponses.NewFailureResponseBuilder( + err, http.StatusBadRequest, actionName). + WithErrorKey(actionName).Build() + } defaultCustomMetricsCredentialType := b.conf.DefaultCustomMetricsCredentialType customMetricsBindingAuthScheme, err := getOrDefaultCredentialType(policyJson, defaultCustomMetricsCredentialType, logger) if err != nil { @@ -593,21 +596,6 @@ func (b *Broker) Bind(ctx context.Context, instanceID string, bindingID string, return result, nil } -func (b *Broker) validateOrGetDefaultCustomMetricsStrategy(bindingConfiguration *models.BindingConfig, logger lager.Logger) (*models.BindingConfig, error) { - strategy := bindingConfiguration.GetCustomMetricsStrategy() - if strategy == "" { - bindingConfiguration.SetCustomMetricsStrategy(models.CustomMetricsSameApp) - } else if strategy != models.CustomMetricsBoundApp { - actionName := "verify-custom-metrics-strategy" - return bindingConfiguration, apiresponses.NewFailureResponseBuilder( - ErrInvalidCustomMetricsStrategy, http.StatusBadRequest, actionName). - WithErrorKey(actionName). - Build() - } - logger.Info("binding-configuration", lager.Data{"bindingConfiguration": bindingConfiguration}) - return bindingConfiguration, nil -} - func (b *Broker) getBindingConfigurationFromRequest(policyJson json.RawMessage, logger lager.Logger) (*models.BindingConfig, error) { bindingConfiguration := &models.BindingConfig{} var err error @@ -739,31 +727,16 @@ func (b *Broker) GetBinding(ctx context.Context, instanceID string, bindingID st b.logger.Error("get-binding", err, lager.Data{"instanceID": instanceID, "bindingID": bindingID, "fetchBindingDetails": details}) return domain.GetBindingSpec{}, apiresponses.NewFailureResponse(errors.New("failed to retrieve scaling policy"), http.StatusInternalServerError, "get-policy") } - combinedConfig, bindingConfig := b.buildConfigurationIfPresent(serviceBinding.CustomMetricsStrategy) - if policy != nil && combinedConfig != nil { //both are present - combinedConfig.ScalingPolicy = *policy - combinedConfig.BindingConfig = *bindingConfig - result.Parameters = combinedConfig - } else if policy != nil { // only policy was given - result.Parameters = policy - } else if combinedConfig != nil { // only configuration was given - result.Parameters = bindingConfig + if policy != nil { + policyAndBinding, err := models.GetBindingConfigAndPolicy(policy, serviceBinding.CustomMetricsStrategy) + if err != nil { + return domain.GetBindingSpec{}, apiresponses.NewFailureResponse(errors.New("failed to build policy and config object"), http.StatusInternalServerError, "determine-BindingConfig-and-Policy") + } + result.Parameters = policyAndBinding } return result, nil } -func (b *Broker) buildConfigurationIfPresent(customMetricsStrategy string) (*models.BindingConfigWithScaling, *models.BindingConfig) { - var combinedConfig *models.BindingConfigWithScaling - var bindingConfig *models.BindingConfig - - if customMetricsStrategy != "" && customMetricsStrategy != models.CustomMetricsSameApp { //if custom metric was given in the binding process - combinedConfig = &models.BindingConfigWithScaling{} - bindingConfig = &models.BindingConfig{} - bindingConfig.SetCustomMetricsStrategy(customMetricsStrategy) - combinedConfig.BindingConfig = *bindingConfig - } - return combinedConfig, bindingConfig -} func (b *Broker) getServiceBinding(ctx context.Context, bindingID string) (*models.ServiceBinding, error) { logger := b.logger.Session("get-service-binding", lager.Data{"bindingID": bindingID}) diff --git a/src/autoscaler/api/broker/broker_test.go b/src/autoscaler/api/broker/broker_test.go index 2092cb7d92..752b2d8534 100644 --- a/src/autoscaler/api/broker/broker_test.go +++ b/src/autoscaler/api/broker/broker_test.go @@ -26,8 +26,7 @@ var _ = Describe("Broker", func() { fakePolicyDB *fakes.FakePolicyDB fakeCredentials *fakes.FakeCredentials testLogger = lagertest.NewTestLogger("test") - bindingConfigWithScaling *models.BindingConfigWithScaling - bindingConfig *models.BindingConfig + bindingConfigWithScaling *models.ScalingPolicyWithBindingConfig ) BeforeEach(func() { @@ -188,10 +187,6 @@ var _ = Describe("Broker", func() { }) Context("with configuration and policy", func() { BeforeEach(func() { - bindingConfig = &models.BindingConfig{Configuration: models.Configuration{CustomMetrics: models.CustomMetricsConfig{ - MetricSubmissionStrategy: models.MetricsSubmissionStrategy{AllowFrom: "bound_app"}, - }, - }} fakeBindingDB.GetServiceBindingReturns(&models.ServiceBinding{ServiceBindingID: testBindingId, ServiceInstanceID: testInstanceId, AppID: testAppId, CustomMetricsStrategy: "bound_app"}, nil) bindingBytes, err := os.ReadFile("testdata/policy-with-configs.json") @@ -208,10 +203,6 @@ var _ = Describe("Broker", func() { }) Context("with configuration only", func() { BeforeEach(func() { - bindingConfig = &models.BindingConfig{Configuration: models.Configuration{CustomMetrics: models.CustomMetricsConfig{ - MetricSubmissionStrategy: models.MetricsSubmissionStrategy{AllowFrom: "bound_app"}, - }, - }} fakeBindingDB.GetServiceBindingReturns(&models.ServiceBinding{ServiceBindingID: testBindingId, ServiceInstanceID: testInstanceId, AppID: testAppId, CustomMetricsStrategy: "bound_app"}, nil) bindingBytes, err := os.ReadFile("testdata/with-configs.json") @@ -221,9 +212,9 @@ var _ = Describe("Broker", func() { Expect(err).ShouldNot(HaveOccurred()) fakePolicyDB.GetAppPolicyReturns(nil, nil) }) - It("returns the bindings with configs in parameters", func() { + It("returns no binding configs in parameters", func() { Expect(err).To(BeNil()) - Expect(Binding).To(Equal(domain.GetBindingSpec{Parameters: bindingConfig})) + Expect(Binding).To(Equal(domain.GetBindingSpec{Parameters: nil})) }) }) }) diff --git a/src/autoscaler/api/brokerserver/broker_handler_test.go b/src/autoscaler/api/brokerserver/broker_handler_test.go index c6bf14ac7d..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": "verify-custom-metrics-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 2545852c27..0c1e0a1e45 100644 --- a/src/autoscaler/api/policyvalidator/policy_json.schema.json +++ b/src/autoscaler/api/policyvalidator/policy_json.schema.json @@ -20,6 +20,33 @@ } ], "properties": { + "configuration": { + "type": "object", + "properties": { + "custom_metrics": { + "type": "object", + "properties": { + "metric_submission_strategy": { + "type": "object", + "properties": { + "allow_from": { + "type": "string", + "enum": [ + "bound_app" + ] + } + }, + "required": [ + "allow_from" + ] + } + }, + "required": [ + "metric_submission_strategy" + ] + } + } + }, "instance_min_count": { "$id": "#/properties/instance_min_count", "type": "integer", @@ -902,4 +929,4 @@ } } } -} \ No newline at end of file +} 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 f764d710f2..7a32612bb6 100644 --- a/src/autoscaler/api/publicapiserver/public_api_handler.go +++ b/src/autoscaler/api/publicapiserver/public_api_handler.go @@ -41,6 +41,8 @@ const ( ErrorMessageAppidIsRequired = "AppId is required" ) +var ErrInvalidConfigurations = errors.New("invalid binding configurations provided") + func NewPublicApiHandler(logger lager.Logger, conf *config.Config, policydb db.PolicyDB, bindingdb db.BindingDB, credentials cred_helper.Credentials) *PublicApiHandler { egClient, err := helpers.CreateHTTPSClient(&conf.EventGenerator.TLSClientCerts, helpers.DefaultClientConfig(), logger.Session("event_client")) if err != nil { @@ -92,14 +94,24 @@ func (h *PublicApiHandler) GetScalingPolicy(w http.ResponseWriter, r *http.Reque writeErrorResponse(w, http.StatusInternalServerError, "Error retrieving scaling policy") return } - if scalingPolicy == nil { logger.Info("policy doesn't exist") writeErrorResponse(w, http.StatusNotFound, "Policy Not Found") return } - - handlers.WriteJSONResponse(w, http.StatusOK, scalingPolicy) + customMetricStrategy, err := h.bindingdb.GetCustomMetricStrategyByAppId(r.Context(), appId) + if err != nil { + logger.Error("Failed to retrieve customMetricStrategy from database", err) + writeErrorResponse(w, http.StatusInternalServerError, "Error retrieving binding policy") + return + } + scalingPolicyWithCustomMetricStrategy, err := models.GetBindingConfigAndPolicy(scalingPolicy, customMetricStrategy) + if err != nil { + logger.Error("Failed to build policy and config response object", err) + writeErrorResponse(w, http.StatusInternalServerError, "Error retrieving binding policy") + return + } + handlers.WriteJSONResponse(w, http.StatusOK, scalingPolicyWithCustomMetricStrategy) } func (h *PublicApiHandler) AttachScalingPolicy(w http.ResponseWriter, r *http.Request, vars map[string]string) { @@ -120,13 +132,21 @@ 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}) + logger.Info("Failed to validate policy", lager.Data{"errResults": errResults, "policy": string(policyBytes)}) handlers.WriteJSONResponse(w, http.StatusBadRequest, errResults) return } + bindingConfiguration, err := h.getBindingConfigurationFromRequest(policyBytes, logger) + if err != nil { + errMessage := "Failed to read binding configuration request body" + logger.Error(errMessage, err) + writeErrorResponse(w, http.StatusInternalServerError, errMessage) + return + } + policyGuid := uuid.NewString() err = h.policydb.SaveAppPolicy(r.Context(), appId, policy, policyGuid) if err != nil { @@ -134,6 +154,7 @@ func (h *PublicApiHandler) AttachScalingPolicy(w http.ResponseWriter, r *http.Re writeErrorResponse(w, http.StatusInternalServerError, "Error saving policy") return } + h.logger.Info("creating/updating schedules", lager.Data{"policy": policy}) err = h.schedulerUtil.CreateOrUpdateSchedule(r.Context(), appId, policy, policyGuid) if err != nil { @@ -142,13 +163,28 @@ func (h *PublicApiHandler) AttachScalingPolicy(w http.ResponseWriter, r *http.Re return } - response, err := json.Marshal(policy) + validatedBindingConfiguration, err := bindingConfiguration.ValidateOrGetDefaultCustomMetricsStrategy() if err != nil { - logger.Error("Failed to marshal policy", err) - writeErrorResponse(w, http.StatusInternalServerError, "Error marshaling policy") + logger.Error(ErrInvalidConfigurations.Error(), err) + writeErrorResponse(w, http.StatusBadRequest, err.Error()) + return + } + customMetricStrategy := validatedBindingConfiguration.GetCustomMetricsStrategy() + logger.Info("saving custom metric submission strategy", lager.Data{"customMetricStrategy": customMetricStrategy, "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" + logger.Error(actionName, err) + writeErrorResponse(w, http.StatusInternalServerError, actionName) return } - _, err = w.Write(response) + responseJson, err := buildResponse(policy, customMetricStrategy, logger) + if err != nil { + logger.Error("Failed to to build response", err) + writeErrorResponse(w, http.StatusInternalServerError, "Error building response") + return + } + _, err = w.Write(responseJson) if err != nil { logger.Error("Failed to write body", err) } @@ -186,7 +222,7 @@ func (h *PublicApiHandler) DetachScalingPolicy(w http.ResponseWriter, r *http.Re writeErrorResponse(w, http.StatusInternalServerError, "Error retrieving service instance") return } - if serviceInstance.DefaultPolicy != "" { + if serviceInstance != nil && serviceInstance.DefaultPolicy != "" { policyStr := serviceInstance.DefaultPolicy policyGuidStr := serviceInstance.DefaultPolicyGuid logger.Info("saving default policy json for app", lager.Data{"policy": policyStr}) @@ -215,6 +251,14 @@ func (h *PublicApiHandler) DetachScalingPolicy(w http.ResponseWriter, r *http.Re } } } + err = h.bindingdb.SetOrUpdateCustomMetricStrategy(r.Context(), appId, "", "delete") + if err != nil { + actionName := "failed to delete custom metric submission strategy in the database" + logger.Error(actionName, err) + writeErrorResponse(w, http.StatusInternalServerError, actionName) + return + } + // find via the app id the binding -> service instance // default policy? then apply that @@ -301,3 +345,30 @@ func (h *PublicApiHandler) GetHealth(w http.ResponseWriter, _ *http.Request, _ m h.logger.Error(ActionWriteBody, err) } } + +func (h *PublicApiHandler) getBindingConfigurationFromRequest(rawJson json.RawMessage, logger lager.Logger) (*models.BindingConfig, error) { + bindingConfiguration := &models.BindingConfig{} + var err error + if rawJson != nil { + err = json.Unmarshal(rawJson, &bindingConfiguration) + if err != nil { + logger.Error("unmarshal-binding-configuration", err) + return bindingConfiguration, err + } + } + return bindingConfiguration, err +} + +func buildResponse(policy *models.ScalingPolicy, customMetricStrategy string, logger lager.Logger) ([]byte, error) { + scalingPolicyWithCustomMetricStrategy, err := models.GetBindingConfigAndPolicy(policy, customMetricStrategy) + if err != nil { + logger.Error("Failed to build policy and config response object", err) + return nil, errors.New("error: building binding and policy") + } + responseJson, err := json.Marshal(scalingPolicyWithCustomMetricStrategy) + if err != nil { + logger.Error("Failed to marshal policy", err) + return nil, errors.New("error: marshalling policy") + } + return responseJson, nil +} diff --git a/src/autoscaler/api/publicapiserver/public_api_handler_test.go b/src/autoscaler/api/publicapiserver/public_api_handler_test.go index 73d526e246..1a57d088cf 100644 --- a/src/autoscaler/api/publicapiserver/public_api_handler_test.go +++ b/src/autoscaler/api/publicapiserver/public_api_handler_test.go @@ -21,7 +21,7 @@ import ( var _ = Describe("PublicApiHandler", func() { const ( - INVALID_POLICY_STR = `{ + InvalidPolicyStr = `{ "instance_max_count":4, "scaling_rules":[ { @@ -31,7 +31,7 @@ var _ = Describe("PublicApiHandler", func() { "adjustment":"-1" }] }` - VALID_POLICY_STR = `{ + ValidPolicyStr = `{ "instance_min_count": 1, "instance_max_count": 5, "scaling_rules": [{ @@ -54,7 +54,7 @@ var _ = Describe("PublicApiHandler", func() { }] } }` - VALID_POLICY_STR_WITH_EXTRA_FIELDS = `{ + ValidPolicyStrWithExtraFields = `{ "instance_min_count": 1, "instance_max_count": 5, "scaling_rules": [{ @@ -79,6 +79,66 @@ var _ = Describe("PublicApiHandler", func() { }, "is_admin": true }` + InvalidCustomMetricsConfigurationStr = `{ + "configuration": { + "custom_metrics": { + "metric_submission_strategy": { + "allow_from": "same_app" + } + } + }, + "instance_min_count": 1, + "instance_max_count": 5, + "scaling_rules": [{ + "metric_type": "memoryused", + "breach_duration_secs": 300, + "threshold": 30, + "operator": ">", + "cool_down_secs": 300, + "adjustment": "-1" + }], + "schedules": { + "timezone": "Asia/Kolkata", + "recurring_schedule": [{ + "start_time": "10:00", + "end_time": "18:00", + "days_of_week": [1, 2, 3], + "instance_min_count": 1, + "instance_max_count": 10, + "initial_min_instance_count": 5 + }] + } + }` + validCustomMetricsConfigurationStr = `{ + "configuration": { + "custom_metrics": { + "metric_submission_strategy": { + "allow_from": "bound_app" + } + } + }, + "instance_min_count": 1, + "instance_max_count": 5, + "scaling_rules": [{ + "metric_type": "memoryused", + "breach_duration_secs": 300, + "threshold": 30, + "operator": ">", + "cool_down_secs": 300, + "adjustment": "-1" + }], + "schedules": { + "timezone": "Asia/Kolkata", + "recurring_schedule": [{ + "start_time": "10:00", + "end_time": "18:00", + "days_of_week": [1, 2, 3], + "instance_min_count": 1, + "instance_max_count": 10, + "initial_min_instance_count": 5 + }] + } + }` ) var ( policydb *fakes.FakePolicyDB @@ -92,7 +152,7 @@ var _ = Describe("PublicApiHandler", func() { BeforeEach(func() { policydb = &fakes.FakePolicyDB{} credentials = &fakes.FakeCredentials{} - bindingdb = nil + bindingdb = &fakes.FakeBindingDB{} resp = httptest.NewRecorder() req = httptest.NewRequest("GET", "/v1/info", nil) pathVariables = map[string]string{} @@ -124,6 +184,7 @@ var _ = Describe("PublicApiHandler", func() { }) }) }) + Describe("GetScalingPolicy", func() { JustBeforeEach(func() { handler.GetScalingPolicy(resp, req, pathVariables) @@ -193,6 +254,44 @@ var _ = Describe("PublicApiHandler", func() { Expect(strings.TrimSpace(resp.Body.String())).To(Equal(`{"instance_min_count":1,"instance_max_count":5,"scaling_rules":[{"metric_type":"memoryused","breach_duration_secs":300,"threshold":30,"operator":"<","cool_down_secs":300,"adjustment":"-1"}],"schedules":{"timezone":"Asia/Kolkata","recurring_schedule":[{"start_time":"10:00","end_time":"18:00","days_of_week":[1,2,3],"instance_min_count":1,"instance_max_count":10,"initial_min_instance_count":5}]}}`)) }) }) + Context("and custom metric strategy", func() { + When("custom metric strategy retrieval fails", func() { + BeforeEach(func() { + pathVariables["appId"] = TEST_APP_ID + setupPolicy(policydb) + bindingdb.GetCustomMetricStrategyByAppIdReturns("", fmt.Errorf("db error")) + }) + It("should fail with 500", func() { + Expect(resp.Code).To(Equal(http.StatusInternalServerError)) + Expect(resp.Body.String()).To(Equal(`{"code":"Internal Server Error","message":"Error retrieving binding policy"}`)) + }) + }) + When("custom metric strategy retrieved successfully", func() { + BeforeEach(func() { + pathVariables["appId"] = TEST_APP_ID + bindingdb.GetCustomMetricStrategyByAppIdReturns("bound_app", nil) + }) + When("custom metric strategy and policy are present", func() { + BeforeEach(func() { + setupPolicy(policydb) + }) + It("should return combined configuration with 200", func() { + Expect(resp.Code).To(Equal(http.StatusOK)) + Expect(resp.Body.String()).To(MatchJSON(validCustomMetricsConfigurationStr)) + }) + When("policy is present only", func() { + BeforeEach(func() { + setupPolicy(policydb) + bindingdb.GetCustomMetricStrategyByAppIdReturns("", nil) + }) + It("should return policy with 200", func() { + Expect(resp.Code).To(Equal(http.StatusOK)) + Expect(resp.Body.String()).To(MatchJSON(ValidPolicyStr)) + }) + }) + }) + }) + }) }) Describe("AttachScalingPolicy", func() { @@ -209,7 +308,7 @@ var _ = Describe("PublicApiHandler", func() { Context("When the policy is invalid", func() { BeforeEach(func() { pathVariables["appId"] = TEST_APP_ID - req, _ = http.NewRequest(http.MethodPut, "", bytes.NewBufferString(INVALID_POLICY_STR)) + req, _ = http.NewRequest(http.MethodPut, "", bytes.NewBufferString(InvalidPolicyStr)) }) It("should fail with 400", func() { Expect(resp.Code).To(Equal(http.StatusBadRequest)) @@ -220,7 +319,7 @@ var _ = Describe("PublicApiHandler", func() { Context("When save policy errors", func() { BeforeEach(func() { pathVariables["appId"] = TEST_APP_ID - req, _ = http.NewRequest(http.MethodPut, "", bytes.NewBufferString(VALID_POLICY_STR)) + req, _ = http.NewRequest(http.MethodPut, "", bytes.NewBufferString(ValidPolicyStr)) policydb.SaveAppPolicyReturns(fmt.Errorf("Failed to save policy")) }) It("should fail with 500", func() { @@ -232,7 +331,7 @@ var _ = Describe("PublicApiHandler", func() { Context("When scheduler returns non 200 and non 204 status code", func() { BeforeEach(func() { pathVariables["appId"] = TEST_APP_ID - req, _ = http.NewRequest(http.MethodPut, "", bytes.NewBufferString(VALID_POLICY_STR)) + req, _ = http.NewRequest(http.MethodPut, "", bytes.NewBufferString(ValidPolicyStr)) schedulerStatus = 500 msg, err := json.Marshal([]string{"err one", "err two"}) Expect(err).ToNot(HaveOccurred()) @@ -248,36 +347,93 @@ var _ = Describe("PublicApiHandler", func() { Context("When scheduler returns 200 status code", func() { BeforeEach(func() { pathVariables["appId"] = TEST_APP_ID - req, _ = http.NewRequest(http.MethodPut, "", bytes.NewBufferString(VALID_POLICY_STR)) + req, _ = http.NewRequest(http.MethodPut, "", bytes.NewBufferString(ValidPolicyStr)) schedulerStatus = 200 }) It("should succeed", func() { Expect(resp.Code).To(Equal(http.StatusOK)) + Expect(resp.Body.String()).To(MatchJSON(ValidPolicyStr)) }) }) Context("When providing extra fields", func() { BeforeEach(func() { pathVariables["appId"] = TEST_APP_ID - req, _ = http.NewRequest(http.MethodPut, "", bytes.NewBufferString(VALID_POLICY_STR_WITH_EXTRA_FIELDS)) + req, _ = http.NewRequest(http.MethodPut, "", bytes.NewBufferString(ValidPolicyStrWithExtraFields)) schedulerStatus = 200 }) It("should succeed and ignore the extra fields", func() { Expect(resp.Code).To(Equal(http.StatusOK)) - Expect(resp.Body).To(MatchJSON(VALID_POLICY_STR)) + Expect(resp.Body).To(MatchJSON(ValidPolicyStr)) }) }) Context("When scheduler returns 204 status code", func() { BeforeEach(func() { pathVariables["appId"] = TEST_APP_ID - req, _ = http.NewRequest(http.MethodPut, "", bytes.NewBufferString(VALID_POLICY_STR)) + req, _ = http.NewRequest(http.MethodPut, "", bytes.NewBufferString(ValidPolicyStr)) schedulerStatus = 204 }) It("should succeed", func() { Expect(resp.Code).To(Equal(http.StatusOK)) }) }) + + Context("Binding Configuration", func() { + When("reading binding configuration from request fails", func() { + BeforeEach(func() { + req = setupRequest("incorrect.json", TEST_APP_ID, pathVariables) + }) + It("should not succeed and fail with 400", func() { + Expect(resp.Body.String()).To(ContainSubstring("invalid character")) + Expect(resp.Code).To(Equal(http.StatusBadRequest)) + }) + }) + + When("invalid configuration is provided with the policy", func() { + BeforeEach(func() { + req = setupRequest(InvalidCustomMetricsConfigurationStr, TEST_APP_ID, pathVariables) + schedulerStatus = 200 + }) + It("should not succeed and fail with 400", func() { + Expect(resp.Body.String()).To(MatchJSON(`[{"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)) + }) + }) + }) + + When("save configuration returned error", func() { + BeforeEach(func() { + req = setupRequest(validCustomMetricsConfigurationStr, TEST_APP_ID, pathVariables) + schedulerStatus = 200 + bindingdb.SetOrUpdateCustomMetricStrategyReturns(fmt.Errorf("failed to save custom metrics configuration")) + }) + It("should not succeed and fail with internal server error", func() { + Expect(resp.Code).To(Equal(http.StatusInternalServerError)) + Expect(resp.Body.String()).To(MatchJSON(`{"code":"Internal Server Error","message":"failed to save custom metric submission strategy in the database"}`)) + }) + }) + + When("valid configuration is provided with the policy", func() { + BeforeEach(func() { + req = setupRequest(validCustomMetricsConfigurationStr, TEST_APP_ID, pathVariables) + schedulerStatus = 200 + }) + It("returns the policy and configuration with 200", func() { + Expect(resp.Code).To(Equal(http.StatusOK)) + Expect(resp.Body).To(MatchJSON(validCustomMetricsConfigurationStr)) + }) + }) + When("configuration is removed but only policy is provided", func() { + BeforeEach(func() { + req = setupRequest(ValidPolicyStr, TEST_APP_ID, pathVariables) + schedulerStatus = 200 + }) + It("returns the policy 200", func() { + Expect(resp.Body).To(MatchJSON(ValidPolicyStr)) + Expect(resp.Code).To(Equal(http.StatusOK)) + }) + }) }) Describe("DetachScalingPolicy", func() { @@ -351,7 +507,7 @@ var _ = Describe("PublicApiHandler", func() { Context("and there is a service instance with a default policy", func() { BeforeEach(func() { bindingdb.GetServiceInstanceByAppIdReturns(&models.ServiceInstance{ - DefaultPolicy: VALID_POLICY_STR, + DefaultPolicy: ValidPolicyStr, DefaultPolicyGuid: "test-policy-guid", }, nil) }) @@ -371,7 +527,7 @@ var _ = Describe("PublicApiHandler", func() { c, a, p, g := policydb.SaveAppPolicyArgsForCall(0) Expect(c).NotTo(BeNil()) Expect(a).To(Equal(TEST_APP_ID)) - Expect(p).To(MatchJSON(VALID_POLICY_STR)) + Expect(p).To(MatchJSON(ValidPolicyStr)) Expect(g).To(Equal("test-policy-guid")) }) }) @@ -382,11 +538,35 @@ var _ = Describe("PublicApiHandler", func() { Context("When scheduler returns 204 status code", func() { BeforeEach(func() { schedulerStatus = 204 + bindingdb.GetServiceInstanceByAppIdReturns(&models.ServiceInstance{}, nil) }) It("should succeed", func() { Expect(resp.Code).To(Equal(http.StatusOK)) }) }) + + Context("Custom Metrics Strategy Submission Configuration", func() { + When("delete configuration in db return errors", func() { + BeforeEach(func() { + schedulerStatus = 200 + bindingdb.SetOrUpdateCustomMetricStrategyReturns(fmt.Errorf("failed to delete custom metric submission strategy in the database")) + }) + It("should not succeed and fail with 500", func() { + Expect(resp.Code).To(Equal(http.StatusInternalServerError)) + Expect(resp.Body.String()).To(MatchJSON(`{"code":"Internal Server Error","message":"failed to delete custom metric submission strategy in the database"}`)) + }) + }) + When("binding exist for a valid app", func() { + BeforeEach(func() { + schedulerStatus = 200 + bindingdb.SetOrUpdateCustomMetricStrategyReturns(nil) + }) + It("delete the custom metric strategy and returns 200", func() { + Expect(resp.Code).To(Equal(http.StatusOK)) + Expect(resp.Body.String()).To(Equal(`{}`)) + }) + }) + }) }) Describe("GetAggregatedMetricsHistories", func() { @@ -832,3 +1012,37 @@ var _ = Describe("PublicApiHandler", func() { }) }) + +func setupRequest(requestBody, appId string, pathVariables map[string]string) *http.Request { + pathVariables["appId"] = appId + req, _ := http.NewRequest(http.MethodPut, "", bytes.NewBufferString(requestBody)) + return req +} +func setupPolicy(policyDb *fakes.FakePolicyDB) { + policyDb.GetAppPolicyReturns(&models.ScalingPolicy{ + InstanceMax: 5, + InstanceMin: 1, + ScalingRules: []*models.ScalingRule{ + { + MetricType: "memoryused", + BreachDurationSeconds: 300, + CoolDownSeconds: 300, + Threshold: 30, + Operator: ">", + Adjustment: "-1", + }}, + Schedules: &models.ScalingSchedules{ + Timezone: "Asia/Kolkata", + RecurringSchedules: []*models.RecurringSchedule{ + { + StartTime: "10:00", + EndTime: "18:00", + DaysOfWeek: []int{1, 2, 3}, + ScheduledInstanceMin: 1, + ScheduledInstanceMax: 10, + ScheduledInstanceInit: 5, + }, + }, + }, + }, nil) +} diff --git a/src/autoscaler/db/db.go b/src/autoscaler/db/db.go index de20e54781..5c919e3db6 100644 --- a/src/autoscaler/db/db.go +++ b/src/autoscaler/db/db.go @@ -78,9 +78,9 @@ type BindingDB interface { CountServiceInstancesInOrg(orgId string) (int, error) GetServiceBinding(ctx context.Context, serviceBindingId string) (*models.ServiceBinding, error) GetBindingIdsByInstanceId(ctx context.Context, instanceId string) ([]string, error) - GetAppBindingByAppId(ctx context.Context, appId string) (string, error) IsAppBoundToSameAutoscaler(ctx context.Context, appId string, appToScaleId string) (bool, error) GetCustomMetricStrategyByAppId(ctx context.Context, appId string) (string, error) + SetOrUpdateCustomMetricStrategy(ctx context.Context, appId string, customMetricsStrategy string, actionName string) error } type AppMetricDB interface { diff --git a/src/autoscaler/db/sqldb/binding_sqldb.go b/src/autoscaler/db/sqldb/binding_sqldb.go index a8ba4380b6..7fa38c3314 100644 --- a/src/autoscaler/db/sqldb/binding_sqldb.go +++ b/src/autoscaler/db/sqldb/binding_sqldb.go @@ -4,6 +4,7 @@ import ( "context" "database/sql" "errors" + "fmt" "time" "code.cloudfoundry.org/app-autoscaler/src/autoscaler/models" @@ -307,16 +308,23 @@ func (bdb *BindingSQLDB) DeleteServiceBindingByAppId(ctx context.Context, appId return nil } -func (bdb *BindingSQLDB) GetAppBindingByAppId(ctx context.Context, appId string) (string, error) { - var bindingId string - query := bdb.sqldb.Rebind("SELECT binding_id FROM binding WHERE app_id =?") - err := bdb.sqldb.QueryRowContext(ctx, query, appId).Scan(&bindingId) - +func (bdb *BindingSQLDB) getServiceBindingByAppId(ctx context.Context, appId string) (*models.ServiceBinding, error) { + dbServiceBinding := &dbServiceBinding{} + query := bdb.sqldb.Rebind("SELECT binding_id, service_instance_id, app_id, custom_metrics_strategy FROM binding WHERE app_id =?") + err := bdb.sqldb.GetContext(ctx, dbServiceBinding, query, appId) if err != nil { - bdb.logger.Error("get-service-binding-by-appid", err, lager.Data{"query": query, "appId": appId}) - return "", err + bdb.logger.Debug("get-service-binding-by-appid", lager.Data{"query": query, "appId": appId, "error": err}) + if errors.Is(err, sql.ErrNoRows) { + return nil, db.ErrDoesNotExist + } + return nil, err } - return bindingId, nil + return &models.ServiceBinding{ + ServiceBindingID: dbServiceBinding.ServiceBindingID, + ServiceInstanceID: dbServiceBinding.ServiceInstanceID, + AppID: dbServiceBinding.AppID, + CustomMetricsStrategy: dbServiceBinding.CustomMetricsStrategy.String, + }, nil } func (bdb *BindingSQLDB) CheckServiceBinding(appId string) bool { var count int @@ -436,6 +444,30 @@ func (bdb *BindingSQLDB) GetCustomMetricStrategyByAppId(ctx context.Context, app return customMetricsStrategy, nil } +func (bdb *BindingSQLDB) SetOrUpdateCustomMetricStrategy(ctx context.Context, appId string, customMetricsStrategy string, actionName string) error { + appBinding, err := bdb.getServiceBindingByAppId(ctx, appId) + if err != nil { + return err + } + query := bdb.sqldb.Rebind("UPDATE binding SET custom_metrics_strategy = ? WHERE binding_id = ?") + result, err := bdb.sqldb.ExecContext(ctx, query, nullableString(customMetricsStrategy), appBinding.ServiceBindingID) + if err != nil { + bdb.logger.Error(fmt.Sprintf("failed to %s custom metric submission strategy", actionName), err, + lager.Data{"query": query, "customMetricsStrategy": customMetricsStrategy, "bindingId": appBinding.ServiceInstanceID, "appId": appId}) + return err + } + if rowsAffected, err := result.RowsAffected(); err != nil || rowsAffected == 0 { + if customMetricsStrategy == appBinding.CustomMetricsStrategy { + 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, + lager.Data{"query": query, "customMetricsStrategy": customMetricsStrategy, "bindingId": appBinding, "appId": appId}) + return errors.New("no rows affected") + } + return nil +} + func (bdb *BindingSQLDB) fetchCustomMetricStrategyByAppId(ctx context.Context, appId string) (string, error) { var customMetricsStrategy sql.NullString query := bdb.sqldb.Rebind("SELECT custom_metrics_strategy FROM binding WHERE app_id =?") diff --git a/src/autoscaler/db/sqldb/binding_sqldb_test.go b/src/autoscaler/db/sqldb/binding_sqldb_test.go index 2c03da0be0..627b909b31 100644 --- a/src/autoscaler/db/sqldb/binding_sqldb_test.go +++ b/src/autoscaler/db/sqldb/binding_sqldb_test.go @@ -749,6 +749,104 @@ var _ = Describe("BindingSqldb", func() { }) }) + + Describe("SetOrUpdateCustomMetricStrategy", func() { + BeforeEach(func() { + err = bdb.CreateServiceInstance(context.Background(), models.ServiceInstance{ServiceInstanceId: testInstanceId, OrgId: testOrgGuid, SpaceId: testSpaceGuid, DefaultPolicy: policyJsonStr, DefaultPolicyGuid: policyGuid}) + Expect(err).NotTo(HaveOccurred()) + }) + Context("Update Custom Metrics Strategy", func() { + Context("With binding does not exist'", func() { + JustBeforeEach(func() { + err = bdb.SetOrUpdateCustomMetricStrategy(context.Background(), testAppId, "bound_app", "update") + }) + It("should not save the custom metrics strategy and fails ", func() { + Expect(err).To(HaveOccurred()) + }) + }) + Context("With binding exists'", func() { + JustBeforeEach(func() { + err = bdb.SetOrUpdateCustomMetricStrategy(context.Background(), testAppId, customMetricsStrategy, "update") + }) + When("custom metrics strategy is not present (already null)", func() { + BeforeEach(func() { + customMetricsStrategy = "bound_app" + err = bdb.CreateServiceBinding(context.Background(), testBindingId, testInstanceId, testAppId, "") + Expect(err).NotTo(HaveOccurred()) + }) + It("should save the custom metrics strategy", func() { + customMetricStrategy, _ := bdb.GetCustomMetricStrategyByAppId(context.Background(), testAppId) + Expect(customMetricStrategy).To(Equal("bound_app")) + }) + }) + When("custom metrics strategy is not present (already null)", func() { + BeforeEach(func() { + customMetricsStrategy = "same_app" + err = bdb.CreateServiceBinding(context.Background(), testBindingId, testInstanceId, testAppId, "") + Expect(err).NotTo(HaveOccurred()) + }) + It("should save the custom metrics strategy", func() { + customMetricStrategy, _ := bdb.GetCustomMetricStrategyByAppId(context.Background(), testAppId) + Expect(customMetricStrategy).To(Equal("same_app")) + }) + }) + When("custom metrics strategy is already present", func() { + BeforeEach(func() { + customMetricsStrategy = "bound_app" + err = bdb.CreateServiceBinding(context.Background(), testBindingId, testInstanceId, testAppId, "same_app") + Expect(err).NotTo(HaveOccurred()) + }) + It("should update the custom metrics strategy to bound_app", func() { + Expect(err).NotTo(HaveOccurred()) + customMetricStrategy, _ := bdb.GetCustomMetricStrategyByAppId(context.Background(), testAppId) + Expect(customMetricStrategy).To(Equal("bound_app")) + }) + }) + When("custom metrics strategy is already present as same_app", func() { + BeforeEach(func() { + customMetricsStrategy = "same_app" + err = bdb.CreateServiceBinding(context.Background(), testBindingId, testInstanceId, testAppId, "same_app") + Expect(err).NotTo(HaveOccurred()) + }) + It("should not update the same custom metrics strategy", func() { + Expect(err).NotTo(HaveOccurred()) + customMetricStrategy, _ := bdb.GetCustomMetricStrategyByAppId(context.Background(), testAppId) + Expect(customMetricStrategy).To(Equal("same_app")) + }) + }) + When("custom metrics strategy unknown value", func() { + BeforeEach(func() { + customMetricsStrategy = "invalid_value" + err = bdb.CreateServiceBinding(context.Background(), testBindingId, testInstanceId, testAppId, "same_app") + Expect(err).NotTo(HaveOccurred()) + }) + It("should throw an error", func() { + Expect(err.Error()).To(ContainSubstring("foreign key constraint")) + }) + }) + }) + }) + Context("Delete Custom Metrics Strategy", func() { + Context("With binding exists'", func() { + JustBeforeEach(func() { + err = bdb.SetOrUpdateCustomMetricStrategy(context.Background(), testAppId, customMetricsStrategy, "delete") + Expect(err).NotTo(HaveOccurred()) + }) + When("custom metrics strategy is already present", func() { + BeforeEach(func() { + customMetricsStrategy = "" + err = bdb.CreateServiceBinding(context.Background(), testBindingId, testInstanceId, testAppId, "bound_app") + Expect(err).NotTo(HaveOccurred()) + }) + It("should update the custom metrics strategy with empty values", func() { + customMetricStrategy, _ := bdb.GetCustomMetricStrategyByAppId(context.Background(), testAppId) + Expect(customMetricStrategy).To(Equal("")) + }) + }) + }) + }) + }) + }) func addProcessIdTo(id string) string { diff --git a/src/autoscaler/models/api.go b/src/autoscaler/models/api.go index 8f8664e15b..25a3656eb3 100644 --- a/src/autoscaler/models/api.go +++ b/src/autoscaler/models/api.go @@ -55,9 +55,9 @@ type ServiceBinding struct { CustomMetricsStrategy string `db:"custom_metrics_strategy"` } -type BindingConfigWithScaling struct { - BindingConfig +type ScalingPolicyWithBindingConfig struct { ScalingPolicy + *BindingConfig } type BindingRequestBody struct { diff --git a/src/autoscaler/models/binding_configs.go b/src/autoscaler/models/binding_configs.go index 697fbe6b0e..60d56fbdaf 100644 --- a/src/autoscaler/models/binding_configs.go +++ b/src/autoscaler/models/binding_configs.go @@ -1,15 +1,17 @@ package models +import ( + "errors" + "fmt" +) + // BindingConfig /* The configuration object received as part of the binding parameters. Example config: { "configuration": { "custom_metrics": { - "auth": { - "credential_type": "binding_secret" - }, "metric_submission_strategy": { - "allow_from": "bound_app or same_app" + "allow_from": "bound_app" } } } @@ -43,3 +45,43 @@ func (b *BindingConfig) GetCustomMetricsStrategy() string { func (b *BindingConfig) SetCustomMetricsStrategy(allowFrom string) { b.Configuration.CustomMetrics.MetricSubmissionStrategy.AllowFrom = allowFrom } + +/** + * GetBindingConfigAndPolicy combines the binding configuration and policy based on the given parameters. + * It establishes the relationship between the scaling policy and the custom metrics strategy. + * @param scalingPolicy the scaling policy + * @param customMetricStrategy the custom metric strategy + * @return the binding configuration and policy if both are present, the scaling policy if only the policy is present, +* the binding configuration if only the configuration is present + * @throws an error if no policy or custom metrics strategy is found +*/ + +func GetBindingConfigAndPolicy(scalingPolicy *ScalingPolicy, customMetricStrategy string) (interface{}, error) { + if scalingPolicy == nil { + return nil, fmt.Errorf("policy not found") + } + if customMetricStrategy != "" && customMetricStrategy != CustomMetricsSameApp { //if customMetricStrategy found + return buildCombinedConfig(scalingPolicy, customMetricStrategy), nil + } + return scalingPolicy, nil +} + +func buildCombinedConfig(scalingPolicy *ScalingPolicy, customMetricStrategy string) *ScalingPolicyWithBindingConfig { + bindingConfig := &BindingConfig{} + bindingConfig.SetCustomMetricsStrategy(customMetricStrategy) + + return &ScalingPolicyWithBindingConfig{ + BindingConfig: bindingConfig, + ScalingPolicy: *scalingPolicy, + } +} + +func (b *BindingConfig) ValidateOrGetDefaultCustomMetricsStrategy() (*BindingConfig, error) { + strategy := b.GetCustomMetricsStrategy() + if strategy == "" { + b.SetCustomMetricsStrategy(CustomMetricsSameApp) + } else if strategy != CustomMetricsBoundApp { + return nil, errors.New("error: custom metrics strategy not supported") + } + return b, nil +} diff --git a/src/autoscaler/models/binding_configs_test.go b/src/autoscaler/models/binding_configs_test.go new file mode 100644 index 0000000000..373426131d --- /dev/null +++ b/src/autoscaler/models/binding_configs_test.go @@ -0,0 +1,151 @@ +package models_test + +import ( + . "code.cloudfoundry.org/app-autoscaler/src/autoscaler/models" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("BindingConfigs", func() { + + var bindingConfig *BindingConfig + + Context("GetBindingConfigAndPolicy", func() { + var ( + scalingPolicy *ScalingPolicy + customMetricStrategy string + result interface{} + err error + ) + + JustBeforeEach(func() { + result, err = GetBindingConfigAndPolicy(scalingPolicy, customMetricStrategy) + }) + + When("both scaling policy and custom metric strategy are present", func() { + BeforeEach(func() { + scalingPolicy = &ScalingPolicy{ + InstanceMax: 5, + InstanceMin: 1, + ScalingRules: []*ScalingRule{ + { + MetricType: "memoryused", + BreachDurationSeconds: 300, + CoolDownSeconds: 300, + Threshold: 30, + Operator: ">", + Adjustment: "-1", + }, + }, + } + customMetricStrategy = CustomMetricsBoundApp + }) + + It("should return combined configuration", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(BeAssignableToTypeOf(&ScalingPolicyWithBindingConfig{})) + combinedConfig := result.(*ScalingPolicyWithBindingConfig) + Expect(combinedConfig.ScalingPolicy).To(Equal(*scalingPolicy)) + Expect(combinedConfig.BindingConfig.GetCustomMetricsStrategy()).To(Equal(customMetricStrategy)) + }) + }) + + When("only scaling policy is present", func() { + BeforeEach(func() { + scalingPolicy = &ScalingPolicy{ + InstanceMax: 5, + InstanceMin: 1, + ScalingRules: []*ScalingRule{ + { + MetricType: "memoryused", + BreachDurationSeconds: 300, + CoolDownSeconds: 300, + Threshold: 30, + Operator: ">", + Adjustment: "-1", + }, + }, + } + customMetricStrategy = "" + }) + + It("should return scaling policy", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(Equal(scalingPolicy)) + }) + }) + + When("policy is not found", func() { + BeforeEach(func() { + scalingPolicy = nil + customMetricStrategy = CustomMetricsBoundApp + }) + + It("should return an error", func() { + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(Equal("policy not found")) + }) + }) + }) + + Context("GetCustomMetricsStrategy", func() { + It("should return the correct custom metrics strategy", func() { + bindingConfig = &BindingConfig{ + Configuration: Configuration{ + CustomMetrics: CustomMetricsConfig{ + MetricSubmissionStrategy: MetricsSubmissionStrategy{ + AllowFrom: CustomMetricsBoundApp, + }, + }, + }, + } + Expect(bindingConfig.GetCustomMetricsStrategy()).To(Equal(CustomMetricsBoundApp)) + }) + }) + + Context("SetCustomMetricsStrategy", func() { + It("should set the custom metrics strategy correctly", func() { + bindingConfig = &BindingConfig{} + bindingConfig.SetCustomMetricsStrategy(CustomMetricsBoundApp) + Expect(bindingConfig.Configuration.CustomMetrics.MetricSubmissionStrategy.AllowFrom).To(Equal(CustomMetricsBoundApp)) + }) + }) + + Context("ValidateOrGetDefaultCustomMetricsStrategy", func() { + var ( + validatedBindingConfig *BindingConfig + err error + ) + JustBeforeEach(func() { + validatedBindingConfig, err = bindingConfig.ValidateOrGetDefaultCustomMetricsStrategy() + }) + When("custom metrics strategy is empty", func() { + + BeforeEach(func() { + bindingConfig = &BindingConfig{} + }) + It("should set the default custom metrics strategy", func() { + Expect(err).NotTo(HaveOccurred()) + Expect(validatedBindingConfig.GetCustomMetricsStrategy()).To(Equal(CustomMetricsSameApp)) + }) + }) + + When("custom metrics strategy is unsupported", func() { + BeforeEach(func() { + bindingConfig = &BindingConfig{ + Configuration: Configuration{ + CustomMetrics: CustomMetricsConfig{ + MetricSubmissionStrategy: MetricsSubmissionStrategy{ + AllowFrom: "unsupported_strategy", + }, + }, + }, + } + }) + It("should return an error", func() { + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(Equal("error: custom metrics strategy not supported")) + }) + }) + }) +}) diff --git a/src/autoscaler/models/policy.go b/src/autoscaler/models/policy.go index eb43c1b729..4ae132f22f 100644 --- a/src/autoscaler/models/policy.go +++ b/src/autoscaler/models/policy.go @@ -38,6 +38,9 @@ func (p *PolicyJson) GetAppPolicy() (*AppPolicy, error) { return &AppPolicy{AppId: p.AppId, ScalingPolicy: &scalingPolicy}, nil } +// ScalingPolicy is a customer facing entity and represents the scaling policy for an application. +// It can be created/deleted/retrieved by the user via the binding process and public api. If a change is required in the policy, +// the corresponding endpoints should be also be updated in the public api server. type ScalingPolicy struct { InstanceMin int `json:"instance_min_count"` InstanceMax int `json:"instance_max_count"`