diff --git a/src/acceptance/api/api_test.go b/src/acceptance/api/api_test.go index 357749b1e1..efbbfd84c9 100644 --- a/src/acceptance/api/api_test.go +++ b/src/acceptance/api/api_test.go @@ -112,12 +112,6 @@ 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", 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\""}`)) 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..0445ec94bd 100644 --- a/src/autoscaler/api/publicapiserver/public_api_handler.go +++ b/src/autoscaler/api/publicapiserver/public_api_handler.go @@ -110,13 +110,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) { @@ -167,14 +167,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 +182,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,11 +350,11 @@ 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) @@ -367,13 +367,17 @@ func (h *PublicApiHandler) getBindingConfigurationFromRequest(policyJson json.Ra 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) + //writeErrorResponse(w, http.StatusInternalServerError, "Error retrieving binding policy") + 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/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"`