From b259ba70f694f6c8dbca3a10750401c2c6c4f71c Mon Sep 17 00:00:00 2001 From: app-autoscaler-ci-bot Date: Tue, 29 Oct 2024 15:21:01 +0000 Subject: [PATCH] fix- instance params returns binding params clean up: remove programmtic focus --- src/acceptance/broker/broker_test.go | 61 +++++++------------ src/autoscaler/api/broker/broker.go | 38 ++++++------ .../api/brokerserver/broker_handler_test.go | 49 ++++++++++++++- 3 files changed, 91 insertions(+), 57 deletions(-) diff --git a/src/acceptance/broker/broker_test.go b/src/acceptance/broker/broker_test.go index 0230550136..3de58f87c2 100644 --- a/src/acceptance/broker/broker_test.go +++ b/src/acceptance/broker/broker_test.go @@ -95,31 +95,25 @@ var _ = Describe("AutoScaler Service Broker", func() { It("binds&unbinds with policy", func() { policyFile := "../assets/file/policy/all.json" - - err := helpers.BindServiceToAppWithPolicy(cfg, appName, instance.name(), policyFile) - Expect(err).NotTo(HaveOccurred()) - expectedPolicyFile := "../assets/file/policy/policy-with-configuration-same-app.json" - expectedPolicyWithConfig, err := os.ReadFile(expectedPolicyFile) - Expect(err).NotTo(HaveOccurred()) - bindingParameters := helpers.GetServiceCredentialBindingParameters(cfg, instance.name(), appName) - Expect(bindingParameters).Should(MatchJSON(expectedPolicyWithConfig)) - - instance.unbind(appName) - }) - It("binds&unbinds with configurations and policy", func() { - policyFile := "../assets/file/policy/policy-with-configuration.json" policy, err := os.ReadFile(policyFile) Expect(err).NotTo(HaveOccurred()) err = helpers.BindServiceToAppWithPolicy(cfg, appName, instance.name(), policyFile) Expect(err).NotTo(HaveOccurred()) - By("checking broker bind parameter response should have policy and configuration") + bindingParameters := helpers.GetServiceCredentialBindingParameters(cfg, instance.name(), appName) Expect(bindingParameters).Should(MatchJSON(policy)) 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) @@ -141,21 +135,17 @@ var _ = Describe("AutoScaler Service Broker", func() { instance.unbind(appName) }) - It("bind&unbinds without policy and gives only configuration", func() { - helpers.BindServiceToApp(cfg, appName, instance.name()) - By("checking broker bind parameter response does not have policy but configuration only") + It("binds&unbinds with configurations and policy", func() { + policyFile := "../assets/file/policy/policy-with-configuration.json" + policyWithConfig, err := os.ReadFile(policyFile) + Expect(err).NotTo(HaveOccurred()) + + err = helpers.BindServiceToAppWithPolicy(cfg, appName, instance.name(), policyFile) + Expect(err).NotTo(HaveOccurred()) + By("checking broker bind parameter response should have policy and configuration") bindingParameters := helpers.GetServiceCredentialBindingParameters(cfg, instance.name(), appName) - expectedJSON := `{ - "configuration": { - "custom_metrics": { - "metric_submission_strategy": { - "allow_from": "same_app" - } - } - } - }` - - Expect(bindingParameters).Should(MatchJSON(expectedJSON)) + Expect(bindingParameters).Should(MatchJSON(policyWithConfig)) + instance.unbind(appName) }) @@ -167,7 +157,7 @@ var _ = Describe("AutoScaler Service Broker", func() { Describe("allows setting default policies", func() { var instance serviceInstance var defaultPolicy []byte - var expectedDefaultPolicyWithConfigsJSON []byte + var policy []byte BeforeEach(func() { instance = createServiceWithParameters(cfg.ServicePlan, "../assets/file/policy/default_policy.json") @@ -183,13 +173,7 @@ var _ = Describe("AutoScaler Service Broker", func() { err = json.Unmarshal(defaultPolicy, &serviceParameters) Expect(err).NotTo(HaveOccurred()) - expectedDefaultPolicyWithConfigsJSON, err = os.ReadFile("../assets/file/policy/default_policy-with-configuration.json") - Expect(err).NotTo(HaveOccurred()) - var serviceParametersWithConfigs = struct { - Configuration interface{} `json:"configuration"` - DefaultPolicy interface{} `json:"default_policy"` - }{} - err = json.Unmarshal(expectedDefaultPolicyWithConfigsJSON, &serviceParametersWithConfigs) + policy, err = json.Marshal(serviceParameters.DefaultPolicy) Expect(err).NotTo(HaveOccurred()) }) @@ -200,9 +184,9 @@ var _ = Describe("AutoScaler Service Broker", func() { It("sets the default policy if no policy is set during binding and allows retrieving the policy via the binding parameters", func() { helpers.BindServiceToApp(cfg, appName, instance.name()) - By("checking broker bind parameter response should have default policy and configuration") + By("checking broker bind parameter response should have default policy") bindingParameters := helpers.GetServiceCredentialBindingParameters(cfg, instance.name(), appName) - Expect(bindingParameters).Should(MatchJSON(expectedDefaultPolicyWithConfigsJSON)) + Expect(bindingParameters).Should(MatchJSON(policy)) unbindService := cf.Cf("unbind-service", appName, instance.name()).Wait(cfg.DefaultTimeoutDuration()) Expect(unbindService).To(Exit(0), "failed unbinding service from app") @@ -222,7 +206,6 @@ var _ = Describe("AutoScaler Service Broker", func() { var instance serviceInstance It("should update a service instance from one plan to another plan", func() { servicePlans := GetServicePlans(cfg) - fmt.Println("servicePlans", servicePlans) source, target, err := servicePlans.getSourceAndTargetForPlanUpdate() Expect(err).NotTo(HaveOccurred(), "failed getting source and target service plans") instance = createService(source.Name) diff --git a/src/autoscaler/api/broker/broker.go b/src/autoscaler/api/broker/broker.go index c1367aa203..17f5d97102 100644 --- a/src/autoscaler/api/broker/broker.go +++ b/src/autoscaler/api/broker/broker.go @@ -711,34 +711,38 @@ func (b *Broker) GetBinding(ctx context.Context, instanceID string, bindingID st if err != nil { return result, err } - bindingConfig := &models.BindingConfig{} - bindingConfig.SetCustomMetricsStrategy(serviceBinding.CustomMetricsStrategy) policy, err := b.policydb.GetAppPolicy(ctx, serviceBinding.AppID) if err != nil { 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") } - - var combinedConfig *models.BindingConfigWithScaling - if bindingConfig.GetCustomMetricsStrategy() != "" { - combinedConfig = &models.BindingConfigWithScaling{BindingConfig: *bindingConfig} - } - if policy != nil { - areConfigAndPolicyPresent := combinedConfig != nil && policy.InstanceMin > 0 - if areConfigAndPolicyPresent { - combinedConfig.ScalingPolicy = *policy - result.Parameters = combinedConfig - } else { - result.Parameters = policy - } - } else if !b.isEmpty(bindingConfig) { + 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 } - return result, nil } +func (b *Broker) buildConfigurationIfPresent(customMetricsStrategy string) (*models.BindingConfigWithScaling, *models.BindingConfig) { + var combinedConfig *models.BindingConfigWithScaling + var bindingConfig *models.BindingConfig + + if customMetricsStrategy != "" && customMetricsStrategy != models.CustomMetricsSameApp { //if custom metric was not given in the binding process + combinedConfig = &models.BindingConfigWithScaling{} + bindingConfig = &models.BindingConfig{} + bindingConfig.SetCustomMetricsStrategy(customMetricsStrategy) + combinedConfig.BindingConfig = *bindingConfig + } + return combinedConfig, bindingConfig +} + func (b *Broker) isEmpty(bindingConfig *models.BindingConfig) bool { return reflect.DeepEqual(bindingConfig, &models.BindingConfig{}) } diff --git a/src/autoscaler/api/brokerserver/broker_handler_test.go b/src/autoscaler/api/brokerserver/broker_handler_test.go index b6d3bd6a98..62db670b7b 100644 --- a/src/autoscaler/api/brokerserver/broker_handler_test.go +++ b/src/autoscaler/api/brokerserver/broker_handler_test.go @@ -923,7 +923,7 @@ var _ = Describe("BrokerHandler", func() { verifyCredentialsGenerated(resp) }) }) - When("Binding configurations are present", func() { + When("Binding configurations and policy are present", func() { BeforeEach(func() { bindingPolicy = `{ "configuration": { @@ -1000,6 +1000,53 @@ var _ = Describe("BrokerHandler", func() { verifyCredentialsGenerated(resp) }) }) + When("Binding configurations are empty", func() { + BeforeEach(func() { + 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" + }] + }` + 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)) + + }) + 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) + }) + }) Context("credential-type is provided while binding", func() { BeforeEach(func() {