From 47e56be458af3f9375f1a61b53f75433ea2589a0 Mon Sep 17 00:00:00 2001 From: Arsalan Khan Date: Mon, 4 Nov 2024 11:54:33 +0100 Subject: [PATCH] allow only bound_app strategy --- src/acceptance/broker/broker_test.go | 14 +- src/autoscaler/api/broker/broker.go | 77 +++++---- .../api/brokerserver/broker_handler_test.go | 148 +++++++++++++----- src/autoscaler/models/binding_configs.go | 3 +- 4 files changed, 170 insertions(+), 72 deletions(-) diff --git a/src/acceptance/broker/broker_test.go b/src/acceptance/broker/broker_test.go index 3de58f87c2..cb042b7af2 100644 --- a/src/acceptance/broker/broker_test.go +++ b/src/acceptance/broker/broker_test.go @@ -107,13 +107,6 @@ var _ = Describe("AutoScaler Service Broker", func() { instance.unbind(appName) }) - It("bind&unbinds without policy", func() { - helpers.BindServiceToApp(cfg, appName, instance.name()) - bindingParameters := helpers.GetServiceCredentialBindingParameters(cfg, instance.name(), appName) - Expect(bindingParameters).Should(MatchJSON("{}")) - instance.unbind(appName) - }) - It("binds&unbinds with policy having credential-type as x509", func() { policyFile := "../assets/file/policy/policy-with-credential-type.json" _, err := os.ReadFile(policyFile) @@ -135,6 +128,13 @@ var _ = Describe("AutoScaler Service Broker", func() { instance.unbind(appName) }) + It("bind&unbinds without policy", func() { + helpers.BindServiceToApp(cfg, appName, instance.name()) + bindingParameters := helpers.GetServiceCredentialBindingParameters(cfg, instance.name(), appName) + Expect(bindingParameters).Should(MatchJSON("{}")) + instance.unbind(appName) + }) + It("binds&unbinds with configurations and policy", func() { policyFile := "../assets/file/policy/policy-with-configuration.json" policyWithConfig, err := os.ReadFile(policyFile) diff --git a/src/autoscaler/api/broker/broker.go b/src/autoscaler/api/broker/broker.go index 17f5d97102..19196d5fa6 100644 --- a/src/autoscaler/api/broker/broker.go +++ b/src/autoscaler/api/broker/broker.go @@ -7,7 +7,6 @@ import ( "errors" "fmt" "net/http" - "reflect" "regexp" "strings" @@ -499,24 +498,16 @@ func (b *Broker) Bind(ctx context.Context, instanceID string, bindingID string, if details.RawParameters != nil { policyJson = details.RawParameters } - bindingConfiguration := &models.BindingConfig{} - if policyJson != nil { - err := json.Unmarshal(policyJson, &bindingConfiguration) - if err != nil { - actionReadBindingConfiguration := "read-binding-configurations" - logger.Error("unmarshal-binding-configuration", err) - return result, apiresponses.NewFailureResponseBuilder( - ErrInvalidConfigurations, http.StatusBadRequest, actionReadBindingConfiguration). - WithErrorKey(actionReadBindingConfiguration). - Build() - } + bindingConfiguration, err := b.getBindingConfigurationFromRequest(policyJson, logger) + if err != nil { + logger.Error("get-binding-configuration-from-request", err) + return result, err } - // set the default custom metrics strategy if not provided - if bindingConfiguration.GetCustomMetricsStrategy() == "" { - bindingConfiguration.SetCustomMetricsStrategy(models.CustomMetricsSameApp) + bindingConfiguration, err = b.validateOrGetDefaultCustomMetricsStrategy(bindingConfiguration, logger) + if err != nil { + logger.Error("validate-or-get-default-custom-metric-strategy", err) + return result, err } - logger.Info("binding-configuration", lager.Data{"bindingConfiguration": bindingConfiguration}) - policy, err := b.getPolicyFromJsonRawMessage(policyJson, instanceID, details.PlanID) if err != nil { logger.Error("get-default-policy", err) @@ -549,7 +540,6 @@ func (b *Broker) Bind(ctx context.Context, instanceID string, bindingID string, if err := b.handleExistingBindingsResiliently(ctx, instanceID, appGUID, logger); err != nil { return result, err } - // save custom metrics strategy check - bindingConfiguration.CustomMetricsConfig.MetricSubmissionStrategy ! == "" err = createServiceBinding(ctx, b.bindingdb, bindingID, instanceID, appGUID, bindingConfiguration.GetCustomMetricsStrategy()) if err != nil { @@ -603,6 +593,38 @@ 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 + 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 getOrDefaultCredentialType(policyJson json.RawMessage, credentialTypeConfig string, logger lager.Logger) (*models.CustomMetricsBindingAuthScheme, error) { credentialType := &models.CustomMetricsBindingAuthScheme{CredentialType: credentialTypeConfig} if len(policyJson) != 0 { @@ -734,7 +756,7 @@ func (b *Broker) buildConfigurationIfPresent(customMetricsStrategy string) (*mod var combinedConfig *models.BindingConfigWithScaling var bindingConfig *models.BindingConfig - if customMetricsStrategy != "" && customMetricsStrategy != models.CustomMetricsSameApp { //if custom metric was not given in the binding process + if customMetricsStrategy != "" && customMetricsStrategy != models.CustomMetricsSameApp { //if custom metric was given in the binding process combinedConfig = &models.BindingConfigWithScaling{} bindingConfig = &models.BindingConfig{} bindingConfig.SetCustomMetricsStrategy(customMetricsStrategy) @@ -742,11 +764,6 @@ func (b *Broker) buildConfigurationIfPresent(customMetricsStrategy string) (*mod } return combinedConfig, bindingConfig } - -func (b *Broker) isEmpty(bindingConfig *models.BindingConfig) bool { - return reflect.DeepEqual(bindingConfig, &models.BindingConfig{}) -} - func (b *Broker) getServiceBinding(ctx context.Context, bindingID string) (*models.ServiceBinding, error) { logger := b.logger.Session("get-service-binding", lager.Data{"bindingID": bindingID}) @@ -891,8 +908,14 @@ func isValidCredentialType(credentialType string) bool { } func createServiceBinding(ctx context.Context, bindingDB db.BindingDB, bindingID, instanceID, appGUID string, customMetricsStrategy string) error { - if customMetricsStrategy == models.CustomMetricsBoundApp || customMetricsStrategy == models.CustomMetricsSameApp { - return bindingDB.CreateServiceBinding(ctx, bindingID, instanceID, appGUID, customMetricsStrategy) + switch customMetricsStrategy { + case models.CustomMetricsBoundApp, models.CustomMetricsSameApp: + err := bindingDB.CreateServiceBinding(ctx, bindingID, instanceID, appGUID, customMetricsStrategy) + if err != nil { + return err + } + default: + return ErrInvalidCustomMetricsStrategy } - return ErrInvalidCustomMetricsStrategy + return nil } diff --git a/src/autoscaler/api/brokerserver/broker_handler_test.go b/src/autoscaler/api/brokerserver/broker_handler_test.go index 62db670b7b..c6bf14ac7d 100644 --- a/src/autoscaler/api/brokerserver/broker_handler_test.go +++ b/src/autoscaler/api/brokerserver/broker_handler_test.go @@ -923,16 +923,14 @@ var _ = Describe("BrokerHandler", func() { verifyCredentialsGenerated(resp) }) }) - When("Binding configurations and policy are present", func() { - BeforeEach(func() { - bindingPolicy = `{ + Context("Binding configurations", func() { + When("invalid custom strategy provided in the binding parameters", func() { + BeforeEach(func() { + bindingPolicy = `{ "configuration": { "custom_metrics": { - "auth": { - "credential_type": "binding_secret" - }, "metric_submission_strategy": { - "allow_from": "bound_app" + "allow_from": "same_app" } } }, @@ -961,10 +959,10 @@ var _ = Describe("BrokerHandler", func() { "adjustment":"-1" }] }` - bindingRequestBody.Policy = json.RawMessage(bindingPolicy) - body, err = json.Marshal(bindingRequestBody) - Expect(err).NotTo(HaveOccurred()) - bindingPolicy = `{ + bindingRequestBody.Policy = json.RawMessage(bindingPolicy) + body, err = json.Marshal(bindingRequestBody) + Expect(err).NotTo(HaveOccurred()) + bindingPolicy = `{ "instance_max_count":4, "instance_min_count":1, "schedules": { @@ -990,19 +988,17 @@ var _ = Describe("BrokerHandler", func() { "adjustment":"-1" }] }` - verifyScheduleIsUpdatedInScheduler(testAppId, bindingPolicy) - }) - It("succeeds with 201", func() { - Expect(resp.Code).To(Equal(http.StatusCreated)) - By("updating the scheduler") - Expect(schedulerServer.ReceivedRequests()).To(HaveLen(1)) - Expect(bindingdb.CreateServiceBindingCallCount()).To(Equal(1)) - verifyCredentialsGenerated(resp) + verifyScheduleIsUpdatedInScheduler(testAppId, bindingPolicy) + }) + 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"}`)) + + }) }) - }) - When("Binding configurations are empty", func() { - BeforeEach(func() { - bindingPolicy = `{ + When("are empty", func() { + BeforeEach(func() { + bindingPolicy = `{ "instance_max_count":4, "instance_min_count":1, "schedules": { @@ -1028,23 +1024,101 @@ var _ = Describe("BrokerHandler", func() { "adjustment":"-1" }] }` - bindingRequestBody.Policy = json.RawMessage(bindingPolicy) - body, err = json.Marshal(bindingRequestBody) - Expect(err).NotTo(HaveOccurred()) + bindingRequestBody.Policy = json.RawMessage(bindingPolicy) + body, err = json.Marshal(bindingRequestBody) + Expect(err).NotTo(HaveOccurred()) - verifyScheduleIsUpdatedInScheduler(testAppId, bindingPolicy) - }) - It("set the default custom metrics strategy", func() { - _, _, _, _, customMetricsStrategy := bindingdb.CreateServiceBindingArgsForCall(0) - Expect(customMetricsStrategy).To(Equal(models.CustomMetricsSameApp)) + verifyScheduleIsUpdatedInScheduler(testAppId, bindingPolicy) + }) + It("set the default custom metrics strategy", func() { + _, _, _, _, customMetricsStrategy := bindingdb.CreateServiceBindingArgsForCall(0) + Expect(customMetricsStrategy).To(Equal(models.CustomMetricsSameApp)) + }) + It("succeeds with 201", func() { + Expect(resp.Code).To(Equal(http.StatusCreated)) + By("updating the scheduler") + Expect(schedulerServer.ReceivedRequests()).To(HaveLen(1)) + Expect(bindingdb.CreateServiceBindingCallCount()).To(Equal(1)) + verifyCredentialsGenerated(resp) + }) }) - It("succeeds with 201", func() { - Expect(resp.Code).To(Equal(http.StatusCreated)) - By("updating the scheduler") - Expect(schedulerServer.ReceivedRequests()).To(HaveLen(1)) - Expect(bindingdb.CreateServiceBindingCallCount()).To(Equal(1)) - verifyCredentialsGenerated(resp) + When("policy and binding configuration are present", func() { + BeforeEach(func() { + bindingPolicy = `{ + "configuration": { + "custom_metrics": { + "auth": { + "credential_type": "binding_secret" + }, + "metric_submission_strategy": { + "allow_from": "bound_app" + } + } + }, + "instance_max_count":4, + "instance_min_count":1, + "schedules": { + "timezone": "Asia/Shanghai", + "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 + }] + }, + "scaling_rules":[ + { + "metric_type":"memoryused", + "threshold":30, + "operator":"<", + "adjustment":"-1" + }] + }` + bindingRequestBody.Policy = json.RawMessage(bindingPolicy) + body, err = json.Marshal(bindingRequestBody) + Expect(err).NotTo(HaveOccurred()) + bindingPolicy = `{ + "instance_max_count":4, + "instance_min_count":1, + "schedules": { + "timezone": "Asia/Shanghai", + "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 + }] + }, + "scaling_rules":[ + { + "metric_type":"memoryused", + "threshold":30, + "operator":"<", + "adjustment":"-1" + }] + }` + verifyScheduleIsUpdatedInScheduler(testAppId, bindingPolicy) + }) + It("succeeds with 201", func() { + Expect(resp.Code).To(Equal(http.StatusCreated)) + By("updating the scheduler") + Expect(schedulerServer.ReceivedRequests()).To(HaveLen(1)) + Expect(bindingdb.CreateServiceBindingCallCount()).To(Equal(1)) + verifyCredentialsGenerated(resp) + }) }) }) diff --git a/src/autoscaler/models/binding_configs.go b/src/autoscaler/models/binding_configs.go index 1efc50a693..697fbe6b0e 100644 --- a/src/autoscaler/models/binding_configs.go +++ b/src/autoscaler/models/binding_configs.go @@ -17,7 +17,8 @@ package models const ( CustomMetricsBoundApp = "bound_app" - CustomMetricsSameApp = "same_app" + // CustomMetricsSameApp default value if not specified + CustomMetricsSameApp = "same_app" ) type BindingConfig struct {