From 5384541e6b2fc62b20d1907ad6b3668b6276c875 Mon Sep 17 00:00:00 2001 From: Arsalan Khan Date: Mon, 4 Nov 2024 17:47:57 +0100 Subject: [PATCH 1/6] AttachPolicy custom metrics strategy DeletePolicy custom metrics strategy --- src/acceptance/api/api_test.go | 122 ++++++++++- src/acceptance/helpers/helpers.go | 15 +- src/autoscaler/api/broker/broker.go | 6 +- src/autoscaler/api/broker/broker_test.go | 2 +- .../policyvalidator/policy_json.schema.json | 31 ++- .../api/publicapiserver/public_api_handler.go | 88 +++++++- .../public_api_handler_test.go | 190 ++++++++++++++++-- src/autoscaler/db/db.go | 1 + src/autoscaler/db/sqldb/binding_sqldb.go | 27 ++- src/autoscaler/db/sqldb/binding_sqldb_test.go | 85 ++++++++ src/autoscaler/models/api.go | 2 +- 11 files changed, 537 insertions(+), 32 deletions(-) diff --git a/src/acceptance/api/api_test.go b/src/acceptance/api/api_test.go index 17fb5d0bd9..e975f1d747 100644 --- a/src/acceptance/api/api_test.go +++ b/src/acceptance/api/api_test.go @@ -112,6 +112,88 @@ 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"}]`)) }) + // custom metrics strategy - FIXME This test can be removed as it is covered by "should fail with 404 when retrieve policy" + It("should fail with 404 when trying to create custom metrics submission without policy", func() { + _, status := getPolicy() + Expect(status).To(Equal(404)) + }) + + It("should fail to create an invalid custom metrics submission", func() { + response, status := createPolicy(GenerateBindingsWithScalingPolicy("invalid-value", 0, 2, "memoryused", 30, 100)) + Expect(string(response)).Should(ContainSubstring(`custom metrics strategy not supported`)) + 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))) + /** + STEP: PUT 'https://autoscaler-3317.autoscaler.app-runtime-interfaces.ci.cloudfoundry.org/v1/apps/17ae5290-c63a-4836-81a6-42d9635c293a/policy' - /Users/I545443/SAPDevelop/asalan316/app-autoscaler-release/src/acceptance/api/api_test.go:308 @ 11/18/24 13:53:22.373 + [FAILED] Expected + : { + "instance_min_count": 1, + "instance_max_count": 2, + "scaling_rules": [ + { + "metric_type": "memoryused", + "breach_duration_secs": 60, + "threshold": 100, + "operator": "\u003e=", + "cool_down_secs": 60, + "adjustment": "+1" + }, + { + "metric_type": "memoryused", + "breach_duration_secs": 60, + "threshold": 30, + "operator": "\u003c", + "cool_down_secs": 60, + "adjustment": "-1" + } + ] + } + to match JSON of + : { + "configuration": { + "custom_metrics": { + "metric_submission_strategy": { + "allow_from": "" + } + } + }, + "instance_min_count": 1, + "instance_max_count": 2, + "scaling_rules": [ + { + "metric_type": "memoryused", + "breach_duration_secs": 60, + "threshold": 100, + "operator": ">=", + "cool_down_secs": 60, + "adjustment": "+1" + }, + { + "metric_type": "memoryused", + "breach_duration_secs": 60, + "threshold": 30, + "operator": "<", + "cool_down_secs": 60, + "adjustment": "-1" + } + ] + } + + + By("creating custom metrics submission with empty value ' '") + policy = GenerateBindingsWithScalingPolicy("", 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() { @@ -204,7 +286,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 +300,46 @@ var _ = Describe("AutoScaler Public API", func() { Expect(status).To(Equal(http.StatusForbidden)) }) }) + //FIXME + Context("and a custom metrics strategy is already set", func() { + var status int + var expectedPolicy string + var actualPolicy []byte + BeforeEach(func() { + 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)) + }) + // FIXME + XIt("should succeed to get a custom metrics strategy", func() { + actualPolicy, status = getPolicy() + Expect(string(actualPolicy)).Should(MatchJSON(expectedPolicy), "TODO: should not fail to get the policy") + 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(`{"code":"Bad Request","message":"error: custom metrics strategy not supported"}`)) + 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 is removed from the existing policy", func() { + It("should removed the custom metrics strategy and displays policy only", func() { + expectedPolicy = GenerateDynamicScaleOutPolicy(1, 2, "memoryused", 30) + actualPolicy, status = createPolicy(expectedPolicy) + Expect(string(actualPolicy)).Should(MatchJSON(expectedPolicy)) + 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..f6eb126ef5 100644 --- a/src/autoscaler/api/broker/broker.go +++ b/src/autoscaler/api/broker/broker.go @@ -752,12 +752,12 @@ func (b *Broker) GetBinding(ctx context.Context, instanceID string, bindingID st return result, nil } -func (b *Broker) buildConfigurationIfPresent(customMetricsStrategy string) (*models.BindingConfigWithScaling, *models.BindingConfig) { - var combinedConfig *models.BindingConfigWithScaling +func (b *Broker) buildConfigurationIfPresent(customMetricsStrategy string) (*models.BindingConfigWithPolicy, *models.BindingConfig) { + var combinedConfig *models.BindingConfigWithPolicy var bindingConfig *models.BindingConfig if customMetricsStrategy != "" && customMetricsStrategy != models.CustomMetricsSameApp { //if custom metric was given in the binding process - combinedConfig = &models.BindingConfigWithScaling{} + combinedConfig = &models.BindingConfigWithPolicy{} bindingConfig = &models.BindingConfig{} bindingConfig.SetCustomMetricsStrategy(customMetricsStrategy) combinedConfig.BindingConfig = *bindingConfig diff --git a/src/autoscaler/api/broker/broker_test.go b/src/autoscaler/api/broker/broker_test.go index 2092cb7d92..35964f9a4d 100644 --- a/src/autoscaler/api/broker/broker_test.go +++ b/src/autoscaler/api/broker/broker_test.go @@ -26,7 +26,7 @@ var _ = Describe("Broker", func() { fakePolicyDB *fakes.FakePolicyDB fakeCredentials *fakes.FakeCredentials testLogger = lagertest.NewTestLogger("test") - bindingConfigWithScaling *models.BindingConfigWithScaling + bindingConfigWithScaling *models.BindingConfigWithPolicy bindingConfig *models.BindingConfig ) diff --git a/src/autoscaler/api/policyvalidator/policy_json.schema.json b/src/autoscaler/api/policyvalidator/policy_json.schema.json index 2545852c27..2a68fe7d3a 100644 --- a/src/autoscaler/api/policyvalidator/policy_json.schema.json +++ b/src/autoscaler/api/policyvalidator/policy_json.schema.json @@ -20,6 +20,35 @@ } ], "properties": { + "configuration": { + "type": "object", + "properties": { + "custom_metrics": { + "type": "object", + "properties": { + "metric_submission_strategy": { + "type": "object", + "properties": { + "allow_from": { + "type": "string", + "enum": [ "bound_app", "same_app" + ] + } + }, + "required": [ + "allow_from" + ] + } + }, + "required": [ + "metric_submission_strategy" + ] + } + }, + "required": [ + "custom_metrics" + ] + }, "instance_min_count": { "$id": "#/properties/instance_min_count", "type": "integer", @@ -902,4 +931,4 @@ } } } -} \ No newline at end of file +} diff --git a/src/autoscaler/api/publicapiserver/public_api_handler.go b/src/autoscaler/api/publicapiserver/public_api_handler.go index f764d710f2..70e4fa5ad6 100644 --- a/src/autoscaler/api/publicapiserver/public_api_handler.go +++ b/src/autoscaler/api/publicapiserver/public_api_handler.go @@ -4,6 +4,7 @@ import ( "encoding/json" "errors" "fmt" + "github.com/pivotal-cf/brokerapi/v11/domain/apiresponses" "io" "net/http" "net/url" @@ -41,6 +42,11 @@ const ( ErrorMessageAppidIsRequired = "AppId is required" ) +var ( + ErrInvalidConfigurations = errors.New("invalid binding configurations provided") + ErrInvalidCustomMetricsStrategy = errors.New("error: custom metrics strategy not supported") +) + 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 { @@ -119,7 +125,20 @@ func (h *PublicApiHandler) AttachScalingPolicy(w http.ResponseWriter, r *http.Re writeErrorResponse(w, http.StatusInternalServerError, "Failed to read request body") 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 + } + // FIXME Move this validation code in a central place within api. This is a duplicate in broker.bind + bindingConfiguration, err = h.validateOrGetDefaultCustomMetricsStrategy(bindingConfiguration, logger) + if err != nil { + logger.Error(ErrInvalidConfigurations.Error(), err) + writeErrorResponse(w, http.StatusBadRequest, err.Error()) + return + } policy, errResults := h.policyValidator.ValidatePolicy(policyBytes) if errResults != nil { logger.Info("Failed to validate policy", lager.Data{"errResults": errResults}) @@ -134,6 +153,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 { @@ -141,11 +161,18 @@ func (h *PublicApiHandler) AttachScalingPolicy(w http.ResponseWriter, r *http.Re writeErrorResponse(w, http.StatusInternalServerError, err.Error()) return } - - response, err := json.Marshal(policy) + strategy := bindingConfiguration.GetCustomMetricsStrategy() + err = h.bindingdb.SetOrUpdateCustomMetricStrategy(r.Context(), appId, strategy, "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 + } + response, err := h.buildResponse(strategy, bindingConfiguration, policy) if err != nil { logger.Error("Failed to marshal policy", err) - writeErrorResponse(w, http.StatusInternalServerError, "Error marshaling policy") + writeErrorResponse(w, http.StatusInternalServerError, "Error building response") return } _, err = w.Write(response) @@ -186,7 +213,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 +242,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 +336,46 @@ func (h *PublicApiHandler) GetHealth(w http.ResponseWriter, _ *http.Request, _ m h.logger.Error(ActionWriteBody, err) } } + +func (h *PublicApiHandler) 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 (h *PublicApiHandler) getBindingConfigurationFromRequest(policyJson json.RawMessage, logger lager.Logger) (*models.BindingConfig, error) { + bindingConfiguration := &models.BindingConfig{} + var err error + if policyJson != nil { + err = json.Unmarshal(policyJson, &bindingConfiguration) + if err != nil { + actionReadBindingConfiguration := "read-binding-configurations" + logger.Error("unmarshal-binding-configuration", err) + return bindingConfiguration, apiresponses.NewFailureResponseBuilder( + ErrInvalidConfigurations, http.StatusBadRequest, actionReadBindingConfiguration). + WithErrorKey(actionReadBindingConfiguration). + Build() + } + } + return bindingConfiguration, err +} + +func (h *PublicApiHandler) buildResponse(strategy string, bindingConfiguration *models.BindingConfig, policy *models.ScalingPolicy) ([]byte, error) { + if strategy != "" && strategy != models.CustomMetricsSameApp { + bindingConfigWithPolicy := &models.BindingConfigWithPolicy{ + BindingConfig: *bindingConfiguration, + ScalingPolicy: *policy, + } + return json.Marshal(bindingConfigWithPolicy) + } + return json.Marshal(policy) +} diff --git a/src/autoscaler/api/publicapiserver/public_api_handler_test.go b/src/autoscaler/api/publicapiserver/public_api_handler_test.go index 73d526e246..511a3a3c9c 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,96 @@ 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 + }] + } + }` + invalidCustomMetricsConfigurationStr = `{ + "configuration": { + "custom_metrics": { + "metric_submission_strategy": { + "allow_from": "invalid" + } + } + }, + "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 +182,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{} @@ -209,7 +299,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 +310,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 +322,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 +338,86 @@ 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() { + pathVariables["appId"] = TEST_APP_ID + req, _ = http.NewRequest(http.MethodPut, "", bytes.NewBufferString("incorrect.json")) + }) + It("should not succeed and fail with 400", func() { + Expect(resp.Code).To(Equal(http.StatusInternalServerError)) + Expect(resp.Body.String()).To(ContainSubstring("Failed to read binding configuration request body")) + }) + }) + When("invalid configuration is provided with the policy", func() { + BeforeEach(func() { + pathVariables["appId"] = TEST_APP_ID + req, _ = http.NewRequest(http.MethodPut, "", bytes.NewBufferString(InvalidCustomMetricsConfigurationStr)) + schedulerStatus = 200 + }) + 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"}`)) + }) + }) + + When("save configuration returned error", func() { + BeforeEach(func() { + pathVariables["appId"] = TEST_APP_ID + req, _ = http.NewRequest(http.MethodPut, "", bytes.NewBufferString(validCustomMetricsConfigurationStr)) + 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() { + pathVariables["appId"] = TEST_APP_ID + req, _ = http.NewRequest(http.MethodPut, "", bytes.NewBufferString(validCustomMetricsConfigurationStr)) + schedulerStatus = 200 + }) + It("returns the policy and configuration with 201", func() { + Expect(resp.Code).To(Equal(http.StatusOK)) + Expect(resp.Body).To(MatchJSON(validCustomMetricsConfigurationStr)) + }) + }) + }) }) Describe("DetachScalingPolicy", func() { @@ -351,7 +491,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 +511,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 +522,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() { diff --git a/src/autoscaler/db/db.go b/src/autoscaler/db/db.go index de20e54781..06ce42d8ac 100644 --- a/src/autoscaler/db/db.go +++ b/src/autoscaler/db/db.go @@ -81,6 +81,7 @@ type BindingDB interface { 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..ecd9035cf4 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" @@ -313,7 +314,10 @@ func (bdb *BindingSQLDB) GetAppBindingByAppId(ctx context.Context, appId string) err := bdb.sqldb.QueryRowContext(ctx, query, appId).Scan(&bindingId) if err != nil { - bdb.logger.Error("get-service-binding-by-appid", err, lager.Data{"query": query, "appId": appId}) + bdb.logger.Debug("get-service-binding-by-appid", lager.Data{"query": query, "appId": appId, "error": err}) + if errors.Is(err, sql.ErrNoRows) { + return "", db.ErrDoesNotExist + } return "", err } return bindingId, nil @@ -436,6 +440,27 @@ 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.GetAppBindingByAppId(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) + 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, "appId": appId}) + return err + } + if rowsAffected, err := result.RowsAffected(); err != nil || rowsAffected == 0 { + 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..d3a2b2bcdf 100644 --- a/src/autoscaler/db/sqldb/binding_sqldb_test.go +++ b/src/autoscaler/db/sqldb/binding_sqldb_test.go @@ -749,6 +749,91 @@ 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", func() { + customMetricStrategy, _ := bdb.GetCustomMetricStrategyByAppId(context.Background(), testAppId) + Expect(customMetricStrategy).To(Equal("bound_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..7930d64a89 100644 --- a/src/autoscaler/models/api.go +++ b/src/autoscaler/models/api.go @@ -55,7 +55,7 @@ type ServiceBinding struct { CustomMetricsStrategy string `db:"custom_metrics_strategy"` } -type BindingConfigWithScaling struct { +type BindingConfigWithPolicy struct { BindingConfig ScalingPolicy } From f5504d145400d2d092bc48afa54fb1ddca14fff9 Mon Sep 17 00:00:00 2001 From: Arsalan Khan Date: Tue, 19 Nov 2024 17:48:55 +0100 Subject: [PATCH 2/6] GetScalingPolicy now get custom metrics configurations GetScalingPolicy => with refactor update custom metrics for mysql --- src/acceptance/api/api_test.go | 21 ++- src/autoscaler/api/broker/broker.go | 51 ++---- src/autoscaler/api/broker/broker_test.go | 9 +- .../api/brokerserver/broker_handler_test.go | 2 +- .../api/publicapiserver/public_api_handler.go | 56 +++---- .../public_api_handler_test.go | 82 ++++++++- src/autoscaler/db/db.go | 1 - src/autoscaler/db/sqldb/binding_sqldb.go | 31 ++-- src/autoscaler/db/sqldb/binding_sqldb_test.go | 15 +- src/autoscaler/models/binding_configs.go | 52 ++++++ src/autoscaler/models/binding_configs_test.go | 156 ++++++++++++++++++ src/autoscaler/models/policy.go | 3 + 12 files changed, 379 insertions(+), 100 deletions(-) create mode 100644 src/autoscaler/models/binding_configs_test.go diff --git a/src/acceptance/api/api_test.go b/src/acceptance/api/api_test.go index e975f1d747..357749b1e1 100644 --- a/src/acceptance/api/api_test.go +++ b/src/acceptance/api/api_test.go @@ -119,8 +119,8 @@ var _ = Describe("AutoScaler Public API", func() { }) It("should fail to create an invalid custom metrics submission", func() { - response, status := createPolicy(GenerateBindingsWithScalingPolicy("invalid-value", 0, 2, "memoryused", 30, 100)) - Expect(string(response)).Should(ContainSubstring(`custom metrics strategy not supported`)) + response, status := createPolicy(GenerateBindingsWithScalingPolicy("invalid-value", 1, 2, "memoryused", 30, 100)) + Expect(string(response)).Should(ContainSubstring(`configuration.custom_metrics.metric_submission_strategy.allow_from must be one of the following: \"bound_app\", \"same_app\""}`)) Expect(status).To(Equal(400)) }) @@ -300,7 +300,7 @@ var _ = Describe("AutoScaler Public API", func() { Expect(status).To(Equal(http.StatusForbidden)) }) }) - //FIXME + Context("and a custom metrics strategy is already set", func() { var status int var expectedPolicy string @@ -314,28 +314,31 @@ var _ = Describe("AutoScaler Public API", func() { _, status = deletePolicy() Expect(status).To(Equal(200)) }) - // FIXME - XIt("should succeed to get a custom metrics strategy", func() { + It("should succeed to get a custom metrics strategy", func() { actualPolicy, status = getPolicy() - Expect(string(actualPolicy)).Should(MatchJSON(expectedPolicy), "TODO: should not fail to get the policy") + 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(`{"code":"Bad Request","message":"error: custom metrics strategy not supported"}`)) + Expect(string(actualPolicy)).Should(ContainSubstring(`configuration.custom_metrics.metric_submission_strategy.allow_from must be one of the following: \"bound_app\", \"same_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 is removed from the existing policy", func() { + 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") + + By("updating policy without custom metrics strategy") expectedPolicy = GenerateDynamicScaleOutPolicy(1, 2, "memoryused", 30) actualPolicy, status = createPolicy(expectedPolicy) - Expect(string(actualPolicy)).Should(MatchJSON(expectedPolicy)) + Expect(string(actualPolicy)).Should(MatchJSON(expectedPolicy), "policy should be present only") Expect(status).To(Equal(200)) }) }) diff --git a/src/autoscaler/api/broker/broker.go b/src/autoscaler/api/broker/broker.go index f6eb126ef5..77b7bf252e 100644 --- a/src/autoscaler/api/broker/broker.go +++ b/src/autoscaler/api/broker/broker.go @@ -503,10 +503,13 @@ 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) + bindingConfiguration, err = bindingConfiguration.ValidateOrGetDefaultCustomMetricsStrategy(bindingConfiguration) if err != nil { - logger.Error("validate-or-get-default-custom-metric-strategy", err) - return result, err + actionName := "validate-or-get-default-custom-metric-strategy" + logger.Error(actionName, err) + return result, apiresponses.NewFailureResponseBuilder( + err, http.StatusBadRequest, actionName). + WithErrorKey(actionName).Build() } policy, err := b.getPolicyFromJsonRawMessage(policyJson, instanceID, details.PlanID) 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 { + andPolicy, err := models.DetermineBindingConfigAndPolicy(policy, serviceBinding.CustomMetricsStrategy) + if err != nil { + return domain.GetBindingSpec{}, apiresponses.NewFailureResponse(errors.New("failed to build policy and config response object"), http.StatusInternalServerError, "determine-BindingConfig-and-Policy") + } + result.Parameters = andPolicy } return result, nil } -func (b *Broker) buildConfigurationIfPresent(customMetricsStrategy string) (*models.BindingConfigWithPolicy, *models.BindingConfig) { - var combinedConfig *models.BindingConfigWithPolicy - var bindingConfig *models.BindingConfig - - if customMetricsStrategy != "" && customMetricsStrategy != models.CustomMetricsSameApp { //if custom metric was given in the binding process - combinedConfig = &models.BindingConfigWithPolicy{} - 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 35964f9a4d..5ea3c6e7e6 100644 --- a/src/autoscaler/api/broker/broker_test.go +++ b/src/autoscaler/api/broker/broker_test.go @@ -27,7 +27,6 @@ var _ = Describe("Broker", func() { fakeCredentials *fakes.FakeCredentials testLogger = lagertest.NewTestLogger("test") bindingConfigWithScaling *models.BindingConfigWithPolicy - bindingConfig *models.BindingConfig ) BeforeEach(func() { @@ -188,7 +187,7 @@ var _ = Describe("Broker", func() { }) Context("with configuration and policy", func() { BeforeEach(func() { - bindingConfig = &models.BindingConfig{Configuration: models.Configuration{CustomMetrics: models.CustomMetricsConfig{ + _ = &models.BindingConfig{Configuration: models.Configuration{CustomMetrics: models.CustomMetricsConfig{ MetricSubmissionStrategy: models.MetricsSubmissionStrategy{AllowFrom: "bound_app"}, }, }} @@ -208,10 +207,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") @@ -223,7 +218,7 @@ var _ = Describe("Broker", func() { }) It("returns the bindings with 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..8f4289ef01 100644 --- a/src/autoscaler/api/brokerserver/broker_handler_test.go +++ b/src/autoscaler/api/brokerserver/broker_handler_test.go @@ -992,7 +992,7 @@ var _ = Describe("BrokerHandler", func() { }) It("should fail with 400", func() { 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"}`)) + Expect(resp.Body.String()).To(MatchJSON(`{"error": "validate-or-get-default-custom-metric-strategy", "description": "error: custom metrics strategy not supported"}`)) }) }) diff --git a/src/autoscaler/api/publicapiserver/public_api_handler.go b/src/autoscaler/api/publicapiserver/public_api_handler.go index 70e4fa5ad6..ef666480b2 100644 --- a/src/autoscaler/api/publicapiserver/public_api_handler.go +++ b/src/autoscaler/api/publicapiserver/public_api_handler.go @@ -4,13 +4,14 @@ import ( "encoding/json" "errors" "fmt" - "github.com/pivotal-cf/brokerapi/v11/domain/apiresponses" "io" "net/http" "net/url" "os" "reflect" + "github.com/pivotal-cf/brokerapi/v11/domain/apiresponses" + "code.cloudfoundry.org/app-autoscaler/src/autoscaler/api/config" "code.cloudfoundry.org/app-autoscaler/src/autoscaler/api/policyvalidator" "code.cloudfoundry.org/app-autoscaler/src/autoscaler/api/schedulerclient" @@ -98,14 +99,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 service binding from database", err) + writeErrorResponse(w, http.StatusInternalServerError, "Error retrieving binding policy") + return + } + scalingPolicyResult, err := models.DetermineBindingConfigAndPolicy(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, scalingPolicyResult) } func (h *PublicApiHandler) AttachScalingPolicy(w http.ResponseWriter, r *http.Request, vars map[string]string) { @@ -132,13 +143,7 @@ func (h *PublicApiHandler) AttachScalingPolicy(w http.ResponseWriter, r *http.Re writeErrorResponse(w, http.StatusInternalServerError, errMessage) return } - // FIXME Move this validation code in a central place within api. This is a duplicate in broker.bind - bindingConfiguration, err = h.validateOrGetDefaultCustomMetricsStrategy(bindingConfiguration, logger) - if err != nil { - logger.Error(ErrInvalidConfigurations.Error(), err) - writeErrorResponse(w, http.StatusBadRequest, err.Error()) - return - } + policy, errResults := h.policyValidator.ValidatePolicy(policyBytes) if errResults != nil { logger.Info("Failed to validate policy", lager.Data{"errResults": errResults}) @@ -161,15 +166,23 @@ func (h *PublicApiHandler) AttachScalingPolicy(w http.ResponseWriter, r *http.Re writeErrorResponse(w, http.StatusInternalServerError, err.Error()) return } - strategy := bindingConfiguration.GetCustomMetricsStrategy() - err = h.bindingdb.SetOrUpdateCustomMetricStrategy(r.Context(), appId, strategy, "update") + + validatedBindingConfiguration, err := bindingConfiguration.ValidateOrGetDefaultCustomMetricsStrategy(bindingConfiguration) + if err != nil { + logger.Error(ErrInvalidConfigurations.Error(), err) + writeErrorResponse(w, http.StatusBadRequest, err.Error()) + return + } + strategy := validatedBindingConfiguration.GetCustomMetricsStrategy() + logger.Info("saving custom metric submission strategy", lager.Data{"strategy": validatedBindingConfiguration.GetCustomMetricsStrategy(), "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 } - response, err := h.buildResponse(strategy, bindingConfiguration, policy) + response, err := h.buildResponse(strategy, validatedBindingConfiguration, policy) if err != nil { logger.Error("Failed to marshal policy", err) writeErrorResponse(w, http.StatusInternalServerError, "Error building response") @@ -337,21 +350,6 @@ func (h *PublicApiHandler) GetHealth(w http.ResponseWriter, _ *http.Request, _ m } } -func (h *PublicApiHandler) 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 (h *PublicApiHandler) getBindingConfigurationFromRequest(policyJson json.RawMessage, logger lager.Logger) (*models.BindingConfig, error) { bindingConfiguration := &models.BindingConfig{} var err error diff --git a/src/autoscaler/api/publicapiserver/public_api_handler_test.go b/src/autoscaler/api/publicapiserver/public_api_handler_test.go index 511a3a3c9c..8fb630c297 100644 --- a/src/autoscaler/api/publicapiserver/public_api_handler_test.go +++ b/src/autoscaler/api/publicapiserver/public_api_handler_test.go @@ -214,6 +214,7 @@ var _ = Describe("PublicApiHandler", func() { }) }) }) + Describe("GetScalingPolicy", func() { JustBeforeEach(func() { handler.GetScalingPolicy(resp, req, pathVariables) @@ -283,6 +284,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() { @@ -381,6 +420,7 @@ var _ = Describe("PublicApiHandler", func() { Expect(resp.Body.String()).To(ContainSubstring("Failed to read binding configuration request body")) }) }) + When("invalid configuration is provided with the policy", func() { BeforeEach(func() { pathVariables["appId"] = TEST_APP_ID @@ -412,11 +452,22 @@ var _ = Describe("PublicApiHandler", func() { req, _ = http.NewRequest(http.MethodPut, "", bytes.NewBufferString(validCustomMetricsConfigurationStr)) schedulerStatus = 200 }) - It("returns the policy and configuration with 201", func() { + 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() { + pathVariables["appId"] = TEST_APP_ID + req, _ = http.NewRequest(http.MethodPut, "", bytes.NewBufferString(ValidPolicyStr)) + schedulerStatus = 200 + }) + It("returns the policy 200", func() { + Expect(resp.Body).To(MatchJSON(ValidPolicyStr)) + Expect(resp.Code).To(Equal(http.StatusOK)) + }) + }) }) }) @@ -996,3 +1047,32 @@ var _ = Describe("PublicApiHandler", func() { }) }) + +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 06ce42d8ac..5c919e3db6 100644 --- a/src/autoscaler/db/db.go +++ b/src/autoscaler/db/db.go @@ -78,7 +78,6 @@ 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 diff --git a/src/autoscaler/db/sqldb/binding_sqldb.go b/src/autoscaler/db/sqldb/binding_sqldb.go index ecd9035cf4..884e3599a5 100644 --- a/src/autoscaler/db/sqldb/binding_sqldb.go +++ b/src/autoscaler/db/sqldb/binding_sqldb.go @@ -308,19 +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.Debug("get-service-binding-by-appid", lager.Data{"query": query, "appId": appId, "error": err}) if errors.Is(err, sql.ErrNoRows) { - return "", db.ErrDoesNotExist + return nil, db.ErrDoesNotExist } - return "", err + 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 @@ -441,19 +445,22 @@ func (bdb *BindingSQLDB) GetCustomMetricStrategyByAppId(ctx context.Context, app } func (bdb *BindingSQLDB) SetOrUpdateCustomMetricStrategy(ctx context.Context, appId string, customMetricsStrategy string, actionName string) error { - - appBinding, err := bdb.GetAppBindingByAppId(ctx, appId) + 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) + 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, "appId": appId}) + 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(fmt.Sprintf("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") diff --git a/src/autoscaler/db/sqldb/binding_sqldb_test.go b/src/autoscaler/db/sqldb/binding_sqldb_test.go index d3a2b2bcdf..627b909b31 100644 --- a/src/autoscaler/db/sqldb/binding_sqldb_test.go +++ b/src/autoscaler/db/sqldb/binding_sqldb_test.go @@ -796,11 +796,24 @@ var _ = Describe("BindingSqldb", func() { err = bdb.CreateServiceBinding(context.Background(), testBindingId, testInstanceId, testAppId, "same_app") Expect(err).NotTo(HaveOccurred()) }) - It("should update the custom metrics strategy", func() { + 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" diff --git a/src/autoscaler/models/binding_configs.go b/src/autoscaler/models/binding_configs.go index 697fbe6b0e..66ea05b180 100644 --- a/src/autoscaler/models/binding_configs.go +++ b/src/autoscaler/models/binding_configs.go @@ -1,5 +1,10 @@ package models +import ( + "errors" + "fmt" +) + // BindingConfig /* The configuration object received as part of the binding parameters. Example config: { @@ -43,3 +48,50 @@ func (b *BindingConfig) GetCustomMetricsStrategy() string { func (b *BindingConfig) SetCustomMetricsStrategy(allowFrom string) { b.Configuration.CustomMetrics.MetricSubmissionStrategy.AllowFrom = allowFrom } + +/** + * DetermineBindingConfigAndPolicy determines 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 DetermineBindingConfigAndPolicy(scalingPolicy *ScalingPolicy, customMetricStrategy string) (interface{}, error) { + if scalingPolicy == nil { + return nil, fmt.Errorf("policy not found") + } + + combinedConfig, bindingConfig := buildConfigurationIfPresent(customMetricStrategy) + if combinedConfig != nil { //both are present + combinedConfig.ScalingPolicy = *scalingPolicy + combinedConfig.BindingConfig = *bindingConfig + return combinedConfig, nil + } + return scalingPolicy, nil +} + +func buildConfigurationIfPresent(customMetricsStrategy string) (*BindingConfigWithPolicy, *BindingConfig) { + var combinedConfig *BindingConfigWithPolicy + var bindingConfig *BindingConfig + + if customMetricsStrategy != "" && customMetricsStrategy != CustomMetricsSameApp { //if custom metric was given in the binding process + combinedConfig = &BindingConfigWithPolicy{} + bindingConfig = &BindingConfig{} + bindingConfig.SetCustomMetricsStrategy(customMetricsStrategy) + combinedConfig.BindingConfig = *bindingConfig + } + return combinedConfig, bindingConfig +} + +func (b *BindingConfig) ValidateOrGetDefaultCustomMetricsStrategy(bindingConfiguration *BindingConfig) (*BindingConfig, error) { + strategy := bindingConfiguration.GetCustomMetricsStrategy() + if strategy == "" { + bindingConfiguration.SetCustomMetricsStrategy(CustomMetricsSameApp) + } else if strategy != CustomMetricsBoundApp { + return nil, errors.New("error: custom metrics strategy not supported") + } + return bindingConfiguration, 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..da80a6fb0e --- /dev/null +++ b/src/autoscaler/models/binding_configs_test.go @@ -0,0 +1,156 @@ +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() { + + Describe("DetermineBindingConfigAndPolicy", func() { + var ( + scalingPolicy *ScalingPolicy + customMetricStrategy string + result interface{} + err error + ) + + JustBeforeEach(func() { + result, err = DetermineBindingConfigAndPolicy(scalingPolicy, customMetricStrategy) + }) + + Context("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(&BindingConfigWithPolicy{})) + combinedConfig := result.(*BindingConfigWithPolicy) + Expect(combinedConfig.ScalingPolicy).To(Equal(*scalingPolicy)) + Expect(combinedConfig.BindingConfig.GetCustomMetricsStrategy()).To(Equal(customMetricStrategy)) + }) + }) + + Context("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)) + }) + }) + + Context("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")) + }) + }) + }) + + var ( + bindingConfig *BindingConfig + ) + + 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 + incomingBindingConfig *BindingConfig + err error + ) + JustBeforeEach(func() { + validatedBindingConfig, err = bindingConfig.ValidateOrGetDefaultCustomMetricsStrategy(incomingBindingConfig) + }) + When("custom metrics strategy is empty", func() { + + BeforeEach(func() { + bindingConfig = &BindingConfig{} + incomingBindingConfig = &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{} + incomingBindingConfig = &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..40b400c187 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 manipulated by the user via the binding process and public API. If a change is required in the policy, +// think of other interaction points e.g., public api server. type ScalingPolicy struct { InstanceMin int `json:"instance_min_count"` InstanceMax int `json:"instance_max_count"` From 2d87ffdc626c4f41492448dd4637d239342ab908 Mon Sep 17 00:00:00 2001 From: Arsalan Khan Date: Tue, 26 Nov 2024 15:50:01 +0100 Subject: [PATCH 3/6] add schema validation for custom metrics strategy --- src/autoscaler/api/broker/broker.go | 12 +- .../api/brokerserver/broker_handler_test.go | 6 +- .../policyvalidator/policy_json.schema.json | 8 +- .../api/policyvalidator/policy_validator.go | 2 +- .../policyvalidator/policy_validator_test.go | 114 +++++++++++++++++- .../api/publicapiserver/public_api_handler.go | 6 +- .../public_api_handler_test.go | 2 +- src/autoscaler/db/sqldb/binding_sqldb.go | 2 +- 8 files changed, 129 insertions(+), 23 deletions(-) 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, From db65e28a2435fe0360ebe2eece302ba4416c995e Mon Sep 17 00:00:00 2001 From: Arsalan Khan Date: Tue, 26 Nov 2024 17:22:03 +0100 Subject: [PATCH 4/6] add public apis endpoints via restclient document openapi spec about binding configuration simplify, clean, refactor --- api/application-metric-api.openapi.yaml | 4 +- api/policy-api.openapi.yaml | 26 ++- docs/public-api.restclient | 87 ++++++++++ src/acceptance/api/api_test.go | 154 ++++++------------ src/acceptance/assets/app/go_app/deploy.sh | 9 +- src/autoscaler/api/broker/broker.go | 8 +- src/autoscaler/api/broker/broker_test.go | 8 +- .../api/publicapiserver/public_api_handler.go | 69 ++++---- .../public_api_handler_test.go | 110 +++++-------- src/autoscaler/models/api.go | 4 +- src/autoscaler/models/binding_configs.go | 40 ++--- src/autoscaler/models/binding_configs_test.go | 15 +- src/autoscaler/models/policy.go | 4 +- 13 files changed, 268 insertions(+), 270 deletions(-) create mode 100644 docs/public-api.restclient 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..68e94ff602 --- /dev/null +++ b/docs/public-api.restclient @@ -0,0 +1,87 @@ +###### +# 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 +######## + +@baseUrl = https://autoscaler.cf.stagingaws.hanavlab.ondemand.com +@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 357749b1e1..0b806c5446 100644 --- a/src/acceptance/api/api_test.go +++ b/src/acceptance/api/api_test.go @@ -112,15 +112,16 @@ 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"}]`)) }) - // custom metrics strategy - FIXME This test can be removed as it is covered by "should fail with 404 when retrieve policy" - It("should fail with 404 when trying to create custom metrics submission without policy", func() { - _, status := getPolicy() - Expect(status).To(Equal(404)) - }) - 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(ContainSubstring(`configuration.custom_metrics.metric_submission_strategy.allow_from must be one of the following: \"bound_app\", \"same_app\""}`)) + 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)) }) @@ -130,73 +131,11 @@ var _ = Describe("AutoScaler Public API", func() { response, status := createPolicy(policy) Expect(string(response)).Should(MatchJSON(policy)) Expect(status).To(Or(Equal(200), Equal(201))) - /** - STEP: PUT 'https://autoscaler-3317.autoscaler.app-runtime-interfaces.ci.cloudfoundry.org/v1/apps/17ae5290-c63a-4836-81a6-42d9635c293a/policy' - /Users/I545443/SAPDevelop/asalan316/app-autoscaler-release/src/acceptance/api/api_test.go:308 @ 11/18/24 13:53:22.373 - [FAILED] Expected - : { - "instance_min_count": 1, - "instance_max_count": 2, - "scaling_rules": [ - { - "metric_type": "memoryused", - "breach_duration_secs": 60, - "threshold": 100, - "operator": "\u003e=", - "cool_down_secs": 60, - "adjustment": "+1" - }, - { - "metric_type": "memoryused", - "breach_duration_secs": 60, - "threshold": 30, - "operator": "\u003c", - "cool_down_secs": 60, - "adjustment": "-1" - } - ] - } - to match JSON of - : { - "configuration": { - "custom_metrics": { - "metric_submission_strategy": { - "allow_from": "" - } - } - }, - "instance_min_count": 1, - "instance_max_count": 2, - "scaling_rules": [ - { - "metric_type": "memoryused", - "breach_duration_secs": 60, - "threshold": 100, - "operator": ">=", - "cool_down_secs": 60, - "adjustment": "+1" - }, - { - "metric_type": "memoryused", - "breach_duration_secs": 60, - "threshold": 30, - "operator": "<", - "cool_down_secs": 60, - "adjustment": "-1" - } - ] - } - - - By("creating custom metrics submission with empty value ' '") - policy = GenerateBindingsWithScalingPolicy("", 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 @@ -300,47 +239,48 @@ var _ = Describe("AutoScaler Public API", func() { Expect(status).To(Equal(http.StatusForbidden)) }) }) + }) - Context("and a custom metrics strategy is already set", func() { - var status int - var expectedPolicy string - var actualPolicy []byte - BeforeEach(func() { - 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(ContainSubstring(`configuration.custom_metrics.metric_submission_strategy.allow_from must be one of the following: \"bound_app\", \"same_app\""}`)) - Expect(status).To(Equal(400)) - }) - It("should succeed to update a valid custom metrics strategy", func() { + When("a scaling policy is set with custom metric strategy", func() { + var status int + var expectedPolicy string + var actualPolicy []byte + BeforeEach(func() { + 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)) - }) + 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") + 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)) - }) + 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/assets/app/go_app/deploy.sh b/src/acceptance/assets/app/go_app/deploy.sh index 75607ad8c4..f41235ec60 100755 --- a/src/acceptance/assets/app/go_app/deploy.sh +++ b/src/acceptance/assets/app/go_app/deploy.sh @@ -41,11 +41,14 @@ function deploy(){ fi # `create-org/space` is idempotent and will simply keep the potentially already existing org/space as is - cf create-org "${org}" - cf target -o "${org}" - cf create-space "${space}" + cf create-org "${org}" || true + cf target -o "${org}" || true + cf create-space "${space}" || true cf target -s "${space}" + echo "current org ${org}" + echo "current space ${space}" + local app_name app_domain service_name memory_mb service_broker service_plan app_name="test_app" app_domain="$(getConfItem apps_domain)" diff --git a/src/autoscaler/api/broker/broker.go b/src/autoscaler/api/broker/broker.go index 250cc886da..f347fb4402 100644 --- a/src/autoscaler/api/broker/broker.go +++ b/src/autoscaler/api/broker/broker.go @@ -508,7 +508,7 @@ func (b *Broker) Bind(ctx context.Context, instanceID string, bindingID string, logger.Error("get-default-policy", err) return result, err } - bindingConfiguration, err = bindingConfiguration.ValidateOrGetDefaultCustomMetricsStrategy(bindingConfiguration) + bindingConfiguration, err = bindingConfiguration.ValidateOrGetDefaultCustomMetricsStrategy() if err != nil { actionName := "validate-or-get-default-custom-metric-strategy" logger.Error(actionName, err) @@ -728,11 +728,11 @@ func (b *Broker) GetBinding(ctx context.Context, instanceID string, bindingID st return domain.GetBindingSpec{}, apiresponses.NewFailureResponse(errors.New("failed to retrieve scaling policy"), http.StatusInternalServerError, "get-policy") } if policy != nil { - andPolicy, err := models.DetermineBindingConfigAndPolicy(policy, serviceBinding.CustomMetricsStrategy) + policyAndBinding, err := models.GetBindingConfigAndPolicy(policy, serviceBinding.CustomMetricsStrategy) if err != nil { - return domain.GetBindingSpec{}, apiresponses.NewFailureResponse(errors.New("failed to build policy and config response object"), http.StatusInternalServerError, "determine-BindingConfig-and-Policy") + return domain.GetBindingSpec{}, apiresponses.NewFailureResponse(errors.New("failed to build policy and config object"), http.StatusInternalServerError, "determine-BindingConfig-and-Policy") } - result.Parameters = andPolicy + result.Parameters = policyAndBinding } return result, nil } diff --git a/src/autoscaler/api/broker/broker_test.go b/src/autoscaler/api/broker/broker_test.go index 5ea3c6e7e6..752b2d8534 100644 --- a/src/autoscaler/api/broker/broker_test.go +++ b/src/autoscaler/api/broker/broker_test.go @@ -26,7 +26,7 @@ var _ = Describe("Broker", func() { fakePolicyDB *fakes.FakePolicyDB fakeCredentials *fakes.FakeCredentials testLogger = lagertest.NewTestLogger("test") - bindingConfigWithScaling *models.BindingConfigWithPolicy + bindingConfigWithScaling *models.ScalingPolicyWithBindingConfig ) BeforeEach(func() { @@ -187,10 +187,6 @@ var _ = Describe("Broker", func() { }) Context("with configuration and policy", func() { BeforeEach(func() { - _ = &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") @@ -216,7 +212,7 @@ 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: nil})) }) diff --git a/src/autoscaler/api/publicapiserver/public_api_handler.go b/src/autoscaler/api/publicapiserver/public_api_handler.go index 8f896986de..9780afc79a 100644 --- a/src/autoscaler/api/publicapiserver/public_api_handler.go +++ b/src/autoscaler/api/publicapiserver/public_api_handler.go @@ -10,8 +10,6 @@ import ( "os" "reflect" - "github.com/pivotal-cf/brokerapi/v11/domain/apiresponses" - "code.cloudfoundry.org/app-autoscaler/src/autoscaler/api/config" "code.cloudfoundry.org/app-autoscaler/src/autoscaler/api/policyvalidator" "code.cloudfoundry.org/app-autoscaler/src/autoscaler/api/schedulerclient" @@ -43,10 +41,7 @@ const ( ErrorMessageAppidIsRequired = "AppId is required" ) -var ( - ErrInvalidConfigurations = errors.New("invalid binding configurations provided") - ErrInvalidCustomMetricsStrategy = errors.New("error: custom metrics strategy not supported") -) +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")) @@ -110,13 +105,13 @@ func (h *PublicApiHandler) GetScalingPolicy(w http.ResponseWriter, r *http.Reque writeErrorResponse(w, http.StatusInternalServerError, "Error retrieving binding policy") return } - scalingPolicyResult, err := models.DetermineBindingConfigAndPolicy(scalingPolicy, customMetricStrategy) + 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, scalingPolicyResult) + handlers.WriteJSONResponse(w, http.StatusOK, scalingPolicyWithCustomMetricStrategy) } func (h *PublicApiHandler) AttachScalingPolicy(w http.ResponseWriter, r *http.Request, vars map[string]string) { @@ -136,13 +131,6 @@ func (h *PublicApiHandler) AttachScalingPolicy(w http.ResponseWriter, r *http.Re writeErrorResponse(w, http.StatusInternalServerError, "Failed to read request body") 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 - } policy, errResults := h.policyValidator.ParseAndValidatePolicy(policyBytes) if errResults != nil { @@ -151,6 +139,14 @@ func (h *PublicApiHandler) AttachScalingPolicy(w http.ResponseWriter, r *http.Re 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 { @@ -167,14 +163,14 @@ func (h *PublicApiHandler) AttachScalingPolicy(w http.ResponseWriter, r *http.Re return } - validatedBindingConfiguration, err := bindingConfiguration.ValidateOrGetDefaultCustomMetricsStrategy(bindingConfiguration) + validatedBindingConfiguration, err := bindingConfiguration.ValidateOrGetDefaultCustomMetricsStrategy() if err != nil { logger.Error(ErrInvalidConfigurations.Error(), err) writeErrorResponse(w, http.StatusBadRequest, err.Error()) return } - strategy := validatedBindingConfiguration.GetCustomMetricsStrategy() - logger.Info("saving custom metric submission strategy", lager.Data{"strategy": strategy, "appId": appId}) + 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" @@ -182,13 +178,13 @@ func (h *PublicApiHandler) AttachScalingPolicy(w http.ResponseWriter, r *http.Re writeErrorResponse(w, http.StatusInternalServerError, actionName) return } - response, err := h.buildResponse(strategy, validatedBindingConfiguration, policy) + responseJson, err := buildResponse(policy, customMetricStrategy, logger) if err != nil { - logger.Error("Failed to marshal policy", err) + logger.Error("Failed to to build response", err) writeErrorResponse(w, http.StatusInternalServerError, "Error building response") return } - _, err = w.Write(response) + _, err = w.Write(responseJson) if err != nil { logger.Error("Failed to write body", err) } @@ -350,30 +346,29 @@ func (h *PublicApiHandler) GetHealth(w http.ResponseWriter, _ *http.Request, _ m } } -func (h *PublicApiHandler) getBindingConfigurationFromRequest(policyJson json.RawMessage, logger lager.Logger) (*models.BindingConfig, error) { +func (h *PublicApiHandler) getBindingConfigurationFromRequest(rawJson json.RawMessage, logger lager.Logger) (*models.BindingConfig, error) { bindingConfiguration := &models.BindingConfig{} var err error - if policyJson != nil { - err = json.Unmarshal(policyJson, &bindingConfiguration) + if rawJson != nil { + err = json.Unmarshal(rawJson, &bindingConfiguration) if err != nil { - actionReadBindingConfiguration := "read-binding-configurations" logger.Error("unmarshal-binding-configuration", err) - return bindingConfiguration, apiresponses.NewFailureResponseBuilder( - ErrInvalidConfigurations, http.StatusBadRequest, actionReadBindingConfiguration). - WithErrorKey(actionReadBindingConfiguration). - Build() + return bindingConfiguration, err } } return bindingConfiguration, err } -func (h *PublicApiHandler) buildResponse(strategy string, bindingConfiguration *models.BindingConfig, policy *models.ScalingPolicy) ([]byte, error) { - if strategy != "" && strategy != models.CustomMetricsSameApp { - bindingConfigWithPolicy := &models.BindingConfigWithPolicy{ - BindingConfig: *bindingConfiguration, - ScalingPolicy: *policy, - } - return json.Marshal(bindingConfigWithPolicy) +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 json.Marshal(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 35b4dfd125..1a57d088cf 100644 --- a/src/autoscaler/api/publicapiserver/public_api_handler_test.go +++ b/src/autoscaler/api/publicapiserver/public_api_handler_test.go @@ -139,36 +139,6 @@ var _ = Describe("PublicApiHandler", func() { }] } }` - invalidCustomMetricsConfigurationStr = `{ - "configuration": { - "custom_metrics": { - "metric_submission_strategy": { - "allow_from": "invalid" - } - } - }, - "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 @@ -412,61 +382,56 @@ var _ = Describe("PublicApiHandler", func() { Context("Binding Configuration", func() { When("reading binding configuration from request fails", func() { BeforeEach(func() { - pathVariables["appId"] = TEST_APP_ID - req, _ = http.NewRequest(http.MethodPut, "", bytes.NewBufferString("incorrect.json")) + req = setupRequest("incorrect.json", TEST_APP_ID, pathVariables) }) It("should not succeed and fail with 400", func() { - Expect(resp.Code).To(Equal(http.StatusInternalServerError)) - Expect(resp.Body.String()).To(ContainSubstring("Failed to read binding configuration request body")) + 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() { - pathVariables["appId"] = TEST_APP_ID - req, _ = http.NewRequest(http.MethodPut, "", bytes.NewBufferString(InvalidCustomMetricsConfigurationStr)) + 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)) - 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\""}]`)) }) }) + }) - When("save configuration returned error", func() { - BeforeEach(func() { - pathVariables["appId"] = TEST_APP_ID - req, _ = http.NewRequest(http.MethodPut, "", bytes.NewBufferString(validCustomMetricsConfigurationStr)) - 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("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() { - pathVariables["appId"] = TEST_APP_ID - req, _ = http.NewRequest(http.MethodPut, "", bytes.NewBufferString(validCustomMetricsConfigurationStr)) - 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("valid configuration is provided with the policy", func() { + BeforeEach(func() { + req = setupRequest(validCustomMetricsConfigurationStr, TEST_APP_ID, pathVariables) + schedulerStatus = 200 }) - When("configuration is removed but only policy is provided", func() { - BeforeEach(func() { - pathVariables["appId"] = TEST_APP_ID - req, _ = http.NewRequest(http.MethodPut, "", bytes.NewBufferString(ValidPolicyStr)) - schedulerStatus = 200 - }) - It("returns the policy 200", func() { - Expect(resp.Body).To(MatchJSON(ValidPolicyStr)) - Expect(resp.Code).To(Equal(http.StatusOK)) - }) + 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)) }) }) }) @@ -1048,8 +1013,13 @@ var _ = Describe("PublicApiHandler", func() { }) }) -func setupPolicy(policydb *fakes.FakePolicyDB) { - policydb.GetAppPolicyReturns(&models.ScalingPolicy{ +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{ diff --git a/src/autoscaler/models/api.go b/src/autoscaler/models/api.go index 7930d64a89..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 BindingConfigWithPolicy 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 66ea05b180..60d56fbdaf 100644 --- a/src/autoscaler/models/binding_configs.go +++ b/src/autoscaler/models/binding_configs.go @@ -10,11 +10,8 @@ import ( { "configuration": { "custom_metrics": { - "auth": { - "credential_type": "binding_secret" - }, "metric_submission_strategy": { - "allow_from": "bound_app or same_app" + "allow_from": "bound_app" } } } @@ -50,7 +47,7 @@ func (b *BindingConfig) SetCustomMetricsStrategy(allowFrom string) { } /** - * DetermineBindingConfigAndPolicy determines the binding configuration and policy based on the given parameters. + * 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 @@ -59,39 +56,32 @@ func (b *BindingConfig) SetCustomMetricsStrategy(allowFrom string) { * @throws an error if no policy or custom metrics strategy is found */ -func DetermineBindingConfigAndPolicy(scalingPolicy *ScalingPolicy, customMetricStrategy string) (interface{}, error) { +func GetBindingConfigAndPolicy(scalingPolicy *ScalingPolicy, customMetricStrategy string) (interface{}, error) { if scalingPolicy == nil { return nil, fmt.Errorf("policy not found") } - - combinedConfig, bindingConfig := buildConfigurationIfPresent(customMetricStrategy) - if combinedConfig != nil { //both are present - combinedConfig.ScalingPolicy = *scalingPolicy - combinedConfig.BindingConfig = *bindingConfig - return combinedConfig, nil + if customMetricStrategy != "" && customMetricStrategy != CustomMetricsSameApp { //if customMetricStrategy found + return buildCombinedConfig(scalingPolicy, customMetricStrategy), nil } return scalingPolicy, nil } -func buildConfigurationIfPresent(customMetricsStrategy string) (*BindingConfigWithPolicy, *BindingConfig) { - var combinedConfig *BindingConfigWithPolicy - var bindingConfig *BindingConfig +func buildCombinedConfig(scalingPolicy *ScalingPolicy, customMetricStrategy string) *ScalingPolicyWithBindingConfig { + bindingConfig := &BindingConfig{} + bindingConfig.SetCustomMetricsStrategy(customMetricStrategy) - if customMetricsStrategy != "" && customMetricsStrategy != CustomMetricsSameApp { //if custom metric was given in the binding process - combinedConfig = &BindingConfigWithPolicy{} - bindingConfig = &BindingConfig{} - bindingConfig.SetCustomMetricsStrategy(customMetricsStrategy) - combinedConfig.BindingConfig = *bindingConfig + return &ScalingPolicyWithBindingConfig{ + BindingConfig: bindingConfig, + ScalingPolicy: *scalingPolicy, } - return combinedConfig, bindingConfig } -func (b *BindingConfig) ValidateOrGetDefaultCustomMetricsStrategy(bindingConfiguration *BindingConfig) (*BindingConfig, error) { - strategy := bindingConfiguration.GetCustomMetricsStrategy() +func (b *BindingConfig) ValidateOrGetDefaultCustomMetricsStrategy() (*BindingConfig, error) { + strategy := b.GetCustomMetricsStrategy() if strategy == "" { - bindingConfiguration.SetCustomMetricsStrategy(CustomMetricsSameApp) + b.SetCustomMetricsStrategy(CustomMetricsSameApp) } else if strategy != CustomMetricsBoundApp { return nil, errors.New("error: custom metrics strategy not supported") } - return bindingConfiguration, nil + return b, nil } diff --git a/src/autoscaler/models/binding_configs_test.go b/src/autoscaler/models/binding_configs_test.go index da80a6fb0e..043a7ef143 100644 --- a/src/autoscaler/models/binding_configs_test.go +++ b/src/autoscaler/models/binding_configs_test.go @@ -8,7 +8,7 @@ import ( var _ = Describe("BindingConfigs", func() { - Describe("DetermineBindingConfigAndPolicy", func() { + Describe("GetBindingConfigAndPolicy", func() { var ( scalingPolicy *ScalingPolicy customMetricStrategy string @@ -17,7 +17,7 @@ var _ = Describe("BindingConfigs", func() { ) JustBeforeEach(func() { - result, err = DetermineBindingConfigAndPolicy(scalingPolicy, customMetricStrategy) + result, err = GetBindingConfigAndPolicy(scalingPolicy, customMetricStrategy) }) Context("when both scaling policy and custom metric strategy are present", func() { @@ -41,8 +41,8 @@ var _ = Describe("BindingConfigs", func() { It("should return combined configuration", func() { Expect(err).NotTo(HaveOccurred()) - Expect(result).To(BeAssignableToTypeOf(&BindingConfigWithPolicy{})) - combinedConfig := result.(*BindingConfigWithPolicy) + Expect(result).To(BeAssignableToTypeOf(&ScalingPolicyWithBindingConfig{})) + combinedConfig := result.(*ScalingPolicyWithBindingConfig) Expect(combinedConfig.ScalingPolicy).To(Equal(*scalingPolicy)) Expect(combinedConfig.BindingConfig.GetCustomMetricsStrategy()).To(Equal(customMetricStrategy)) }) @@ -116,17 +116,15 @@ var _ = Describe("BindingConfigs", func() { Context("ValidateOrGetDefaultCustomMetricsStrategy", func() { var ( validatedBindingConfig *BindingConfig - incomingBindingConfig *BindingConfig err error ) JustBeforeEach(func() { - validatedBindingConfig, err = bindingConfig.ValidateOrGetDefaultCustomMetricsStrategy(incomingBindingConfig) + validatedBindingConfig, err = bindingConfig.ValidateOrGetDefaultCustomMetricsStrategy() }) When("custom metrics strategy is empty", func() { BeforeEach(func() { bindingConfig = &BindingConfig{} - incomingBindingConfig = &BindingConfig{} }) It("should set the default custom metrics strategy", func() { Expect(err).NotTo(HaveOccurred()) @@ -136,8 +134,7 @@ var _ = Describe("BindingConfigs", func() { When("custom metrics strategy is unsupported", func() { BeforeEach(func() { - bindingConfig = &BindingConfig{} - incomingBindingConfig = &BindingConfig{ + bindingConfig = &BindingConfig{ Configuration: Configuration{ CustomMetrics: CustomMetricsConfig{ MetricSubmissionStrategy: MetricsSubmissionStrategy{ diff --git a/src/autoscaler/models/policy.go b/src/autoscaler/models/policy.go index 40b400c187..4ae132f22f 100644 --- a/src/autoscaler/models/policy.go +++ b/src/autoscaler/models/policy.go @@ -39,8 +39,8 @@ func (p *PolicyJson) GetAppPolicy() (*AppPolicy, error) { } // ScalingPolicy is a customer facing entity and represents the scaling policy for an application. -// It can be manipulated by the user via the binding process and public API. If a change is required in the policy, -// think of other interaction points e.g., public api server. +// 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"` From 3fc59be0c1e073022df443e839e0a8f02c6ecd41 Mon Sep 17 00:00:00 2001 From: Arsalan Khan Date: Fri, 29 Nov 2024 10:10:42 +0100 Subject: [PATCH 5/6] fix acceptance-release target --- Makefile | 2 +- src/acceptance/api/api_test.go | 1 + src/acceptance/assets/app/go_app/deploy.sh | 9 +++------ src/autoscaler/api/publicapiserver/public_api_handler.go | 2 +- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/Makefile b/Makefile index eb874f95f5..a4c63e11ee 100644 --- a/Makefile +++ b/Makefile @@ -358,7 +358,7 @@ mta-release: mta-build @echo " - building mtar release '${VERSION}' to dir: '${DEST}' " .PHONY: acceptance-release -acceptance-release: clean-acceptance go-mod-tidy go-mod-vendor build-test-app +acceptance-release: generate-fakes generate-openapi-generated-clients-and-servers clean-acceptance go-mod-tidy go-mod-vendor build-test-app @echo " - building acceptance test release '${VERSION}' to dir: '${DEST}' " @mkdir -p ${DEST} ${AUTOSCALER_DIR}/scripts/compile-acceptance-tests.sh diff --git a/src/acceptance/api/api_test.go b/src/acceptance/api/api_test.go index 0b806c5446..f190887f30 100644 --- a/src/acceptance/api/api_test.go +++ b/src/acceptance/api/api_test.go @@ -246,6 +246,7 @@ var _ = Describe("AutoScaler Public API", func() { 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)) diff --git a/src/acceptance/assets/app/go_app/deploy.sh b/src/acceptance/assets/app/go_app/deploy.sh index f41235ec60..75607ad8c4 100755 --- a/src/acceptance/assets/app/go_app/deploy.sh +++ b/src/acceptance/assets/app/go_app/deploy.sh @@ -41,14 +41,11 @@ function deploy(){ fi # `create-org/space` is idempotent and will simply keep the potentially already existing org/space as is - cf create-org "${org}" || true - cf target -o "${org}" || true - cf create-space "${space}" || true + cf create-org "${org}" + cf target -o "${org}" + cf create-space "${space}" cf target -s "${space}" - echo "current org ${org}" - echo "current space ${space}" - local app_name app_domain service_name memory_mb service_broker service_plan app_name="test_app" app_domain="$(getConfItem apps_domain)" diff --git a/src/autoscaler/api/publicapiserver/public_api_handler.go b/src/autoscaler/api/publicapiserver/public_api_handler.go index 9780afc79a..7a32612bb6 100644 --- a/src/autoscaler/api/publicapiserver/public_api_handler.go +++ b/src/autoscaler/api/publicapiserver/public_api_handler.go @@ -134,7 +134,7 @@ func (h *PublicApiHandler) AttachScalingPolicy(w http.ResponseWriter, r *http.Re 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 } From af3d2aa4ab1f8f6c2042df205ea9c7b1d92b78ae Mon Sep 17 00:00:00 2001 From: Arsalan Khan Date: Tue, 3 Dec 2024 17:52:02 +0100 Subject: [PATCH 6/6] Review Fixes --- Makefile | 2 +- docs/public-api.restclient | 3 ++- src/autoscaler/models/binding_configs_test.go | 14 ++++++-------- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/Makefile b/Makefile index a4c63e11ee..eb874f95f5 100644 --- a/Makefile +++ b/Makefile @@ -358,7 +358,7 @@ mta-release: mta-build @echo " - building mtar release '${VERSION}' to dir: '${DEST}' " .PHONY: acceptance-release -acceptance-release: generate-fakes generate-openapi-generated-clients-and-servers clean-acceptance go-mod-tidy go-mod-vendor build-test-app +acceptance-release: clean-acceptance go-mod-tidy go-mod-vendor build-test-app @echo " - building acceptance test release '${VERSION}' to dir: '${DEST}' " @mkdir -p ${DEST} ${AUTOSCALER_DIR}/scripts/compile-acceptance-tests.sh diff --git a/docs/public-api.restclient b/docs/public-api.restclient index 68e94ff602..261d6f61ae 100644 --- a/docs/public-api.restclient +++ b/docs/public-api.restclient @@ -4,7 +4,8 @@ # This endpoints works with vs code rest client extension ######## -@baseUrl = https://autoscaler.cf.stagingaws.hanavlab.ondemand.com +@system_domain = app-runtime-interfaces.ci.cloudfoundry.org +@baseUrl = https://autoscaler.{{system_domain}} @auth_token = bearer ### app guid diff --git a/src/autoscaler/models/binding_configs_test.go b/src/autoscaler/models/binding_configs_test.go index 043a7ef143..373426131d 100644 --- a/src/autoscaler/models/binding_configs_test.go +++ b/src/autoscaler/models/binding_configs_test.go @@ -8,7 +8,9 @@ import ( var _ = Describe("BindingConfigs", func() { - Describe("GetBindingConfigAndPolicy", func() { + var bindingConfig *BindingConfig + + Context("GetBindingConfigAndPolicy", func() { var ( scalingPolicy *ScalingPolicy customMetricStrategy string @@ -20,7 +22,7 @@ var _ = Describe("BindingConfigs", func() { result, err = GetBindingConfigAndPolicy(scalingPolicy, customMetricStrategy) }) - Context("when both scaling policy and custom metric strategy are present", func() { + When("both scaling policy and custom metric strategy are present", func() { BeforeEach(func() { scalingPolicy = &ScalingPolicy{ InstanceMax: 5, @@ -48,7 +50,7 @@ var _ = Describe("BindingConfigs", func() { }) }) - Context("when only scaling policy is present", func() { + When("only scaling policy is present", func() { BeforeEach(func() { scalingPolicy = &ScalingPolicy{ InstanceMax: 5, @@ -73,7 +75,7 @@ var _ = Describe("BindingConfigs", func() { }) }) - Context("when policy is not found", func() { + When("policy is not found", func() { BeforeEach(func() { scalingPolicy = nil customMetricStrategy = CustomMetricsBoundApp @@ -86,10 +88,6 @@ var _ = Describe("BindingConfigs", func() { }) }) - var ( - bindingConfig *BindingConfig - ) - Context("GetCustomMetricsStrategy", func() { It("should return the correct custom metrics strategy", func() { bindingConfig = &BindingConfig{