From 1cd4ac9e4694c2a68975c7067304e585d4042a7f Mon Sep 17 00:00:00 2001 From: Arsalan Khan Date: Mon, 21 Oct 2024 21:52:39 +0200 Subject: [PATCH] implement GetBinding to include binding configs --- .../policy/policy-with-configuration.json | 3 + src/acceptance/broker/broker_test.go | 1 + src/acceptance/helpers/helpers.go | 64 ++----------------- .../setup_performance_test.go | 4 +- src/autoscaler/api/broker/broker.go | 24 +++++-- src/autoscaler/api/broker/broker_test.go | 64 ++++++++++++++++--- .../broker/testdata/policy-with-configs.json | 19 ++++++ .../api/broker/testdata/with-configs.json | 9 +++ .../api/brokerserver/broker_handler_test.go | 41 ++++++++++++ .../api/db/servicebroker.db.changelog.yaml | 1 - src/autoscaler/db/sqldb/binding_sqldb.go | 2 +- src/autoscaler/db/sqldb/binding_sqldb_test.go | 7 +- src/autoscaler/models/api.go | 12 +++- 13 files changed, 168 insertions(+), 83 deletions(-) create mode 100644 src/autoscaler/api/broker/testdata/policy-with-configs.json create mode 100644 src/autoscaler/api/broker/testdata/with-configs.json diff --git a/src/acceptance/assets/file/policy/policy-with-configuration.json b/src/acceptance/assets/file/policy/policy-with-configuration.json index 94dcf15ae0..8f3eef9381 100644 --- a/src/acceptance/assets/file/policy/policy-with-configuration.json +++ b/src/acceptance/assets/file/policy/policy-with-configuration.json @@ -1,6 +1,9 @@ { "configuration": { "custom_metrics": { + "auth": { + "credential_type": "" + }, "metric_submission_strategy": { "allow_from": "bound_app" } diff --git a/src/acceptance/broker/broker_test.go b/src/acceptance/broker/broker_test.go index 73d540caad..90865a78de 100644 --- a/src/acceptance/broker/broker_test.go +++ b/src/acceptance/broker/broker_test.go @@ -109,6 +109,7 @@ var _ = Describe("AutoScaler Service Broker", func() { It("binds&unbinds with configurations and policy", func() { policyFile := "../assets/file/policy/policy-with-configuration.json" policy, err := os.ReadFile(policyFile) + fmt.Println("policy", string(policy)) //FIXME Expect(err).NotTo(HaveOccurred()) err = helpers.BindServiceToAppWithPolicy(cfg, appName, instance.name(), policyFile) diff --git a/src/acceptance/helpers/helpers.go b/src/acceptance/helpers/helpers.go index 8ec26ac6e4..1cb2124b3c 100644 --- a/src/acceptance/helpers/helpers.go +++ b/src/acceptance/helpers/helpers.go @@ -183,17 +183,6 @@ func ServicePlansUrl(cfg *config.Config, spaceGuid string) string { return url.String() } -func GenerateBindingConfiguration(allowFrom string) string { - bindingConfig := BindingConfig{ - Configuration: Configuration{CustomMetrics: CustomMetricsConfig{ - MetricSubmissionStrategy: MetricsSubmissionStrategy{AllowFrom: allowFrom}, - }}, - } - marshalledBinding, err := MarshalWithoutHTMLEscape(bindingConfig) - Expect(err).NotTo(HaveOccurred()) - return string(marshalledBinding) -} - func GenerateBindingsWithScalingPolicy(allowFrom string, instanceMin, instanceMax int, metricName string, scaleInThreshold, scaleOutThreshold int64) string { bindingConfig := BindingConfig{ Configuration: Configuration{CustomMetrics: CustomMetricsConfig{ @@ -272,27 +261,6 @@ func GenerateDynamicScaleOutPolicyWithExtraFields(instanceMin, instanceMax int, return string(extraBytes), string(validBytes) } -func GenerateDynamicScaleInPolicy(instanceMin, instanceMax int, metricName string, threshold int64) string { - scalingInRule := ScalingRule{ - MetricType: metricName, - BreachDurationSeconds: TestBreachDurationSeconds, - Threshold: threshold, - Operator: "<", - CoolDownSeconds: TestCoolDownSeconds, - Adjustment: "-1", - } - - policy := ScalingPolicy{ - InstanceMin: instanceMin, - InstanceMax: instanceMax, - ScalingRules: []*ScalingRule{&scalingInRule}, - } - marshaled, err := MarshalWithoutHTMLEscape(policy) - Expect(err).NotTo(HaveOccurred()) - - return string(marshaled) -} - func GenerateDynamicScaleOutAndInPolicy(instanceMin, instanceMax int, metricName string, scaleInWhenBelowThreshold int64, scaleOutWhenGreaterOrEqualThreshold int64) string { policy := buildScaleOutScaleInPolicy(instanceMin, instanceMax, metricName, scaleInWhenBelowThreshold, scaleOutWhenGreaterOrEqualThreshold) marshaled, err := MarshalWithoutHTMLEscape(policy) @@ -512,15 +480,15 @@ func MarshalWithoutHTMLEscape(v interface{}) ([]byte, error) { func CreatePolicy(cfg *config.Config, appName, appGUID, policy string) string { GinkgoHelper() - instanceName, _ := createPolicy(cfg, appName, appGUID, policy) + instanceName, _ := createPolicy(cfg, appName, policy) return instanceName } -func CreatePolicyWithErr(cfg *config.Config, appName, appGUID, policy string) (string, error) { - return createPolicy(cfg, appName, appGUID, policy) +func CreatePolicyWithErr(cfg *config.Config, appName, policy string) (string, error) { + return createPolicy(cfg, appName, policy) } -func createPolicy(cfg *config.Config, appName, appGUID, policy string) (string, error) { +func createPolicy(cfg *config.Config, appName, policy string) (string, error) { GinkgoHelper() instanceName := generator.PrefixedRandomName(cfg.Prefix, cfg.InstancePrefix) err := Retry(defaultRetryAttempt, defaultRetryAfter, func() error { return CreateServiceWithPlan(cfg, cfg.ServicePlan, instanceName) }) @@ -628,23 +596,6 @@ func GetServiceCredentialBindingParameters(cfg *config.Config, instanceName stri return strings.TrimSpace(string(cmd.Out.Contents())) } -func CreatePolicyWithAPI(cfg *config.Config, appGUID, policy string) { - GinkgoHelper() - oauthToken := OauthToken(cfg) - client := GetHTTPClient(cfg) - - policyURL := fmt.Sprintf("%s%s", cfg.ASApiEndpoint, strings.Replace(PolicyPath, "{appId}", appGUID, -1)) - req, err := http.NewRequest("PUT", policyURL, bytes.NewBuffer([]byte(policy))) - Expect(err).ShouldNot(HaveOccurred()) - req.Header.Add("Authorization", oauthToken) - req.Header.Add("Content-Type", "application/json") - - resp, err := client.Do(req) - Expect(err).ShouldNot(HaveOccurred()) - defer func() { _ = resp.Body.Close() }() - Expect(resp).Should(HaveHTTPStatus(200, 201), "assigning policy by putting to %s", policyURL) -} - func GetHTTPClient(cfg *config.Config) *http.Client { return &http.Client{ Transport: &http.Transport{ @@ -679,13 +630,6 @@ func GetAppGuid(cfg *config.Config, appName string) (string, error) { return appGuid, err } -func FailOnCommandFailuref(command *Session, format string, args ...any) *Session { - if command.ExitCode() != 0 { - Fail(fmt.Sprintf(format, args...)) - } - return command -} - func SetLabel(cfg *config.Config, appGUID string, labelKey string, labelValue string) { GinkgoHelper() cmd := cf.Cf("curl", "--fail", fmt.Sprintf("/v3/apps/%s", appGUID), "-X", "PATCH", "-d", fmt.Sprintf(`{"metadata": {"labels": {"%s": "%s"}}}`, labelKey, labelValue)).Wait(cfg.DefaultTimeoutDuration()) diff --git a/src/acceptance/setup_performance/setup_performance_test.go b/src/acceptance/setup_performance/setup_performance_test.go index d6d2462b91..7506a46717 100644 --- a/src/acceptance/setup_performance/setup_performance_test.go +++ b/src/acceptance/setup_performance/setup_performance_test.go @@ -92,12 +92,12 @@ func pushAppAndBindService(appName string, runningApps *int32, pendingApps *sync } policy := helpers.GenerateDynamicScaleOutAndInPolicy( 1, 2, "test_metric", 500, 500) - appGUID, err := helpers.GetAppGuid(cfg, appName) + _, err = helpers.GetAppGuid(cfg, appName) if err != nil { errors.Store(appName, err) return } - _, err = helpers.CreatePolicyWithErr(cfg, appName, appGUID, policy) + _, err = helpers.CreatePolicyWithErr(cfg, appName, policy) if err != nil { errors.Store(appName, err) return diff --git a/src/autoscaler/api/broker/broker.go b/src/autoscaler/api/broker/broker.go index 399c993b32..3f24efe847 100644 --- a/src/autoscaler/api/broker/broker.go +++ b/src/autoscaler/api/broker/broker.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "net/http" + "reflect" "regexp" "strings" @@ -498,9 +499,6 @@ func (b *Broker) Bind(ctx context.Context, instanceID string, bindingID string, if details.RawParameters != nil { policyJson = details.RawParameters } - - // extract custom metrics configs to determine metric submission strategy and set to default if not provided - bindingConfiguration := &models.BindingConfig{} if policyJson != nil { err := json.Unmarshal(policyJson, &bindingConfiguration) @@ -713,6 +711,8 @@ func (b *Broker) GetBinding(ctx context.Context, instanceID string, bindingID st if err != nil { return result, err } + bindingConfig := &models.BindingConfig{} + bindingConfig.SetDefaultCustomMetricsStrategy(serviceBinding.CustomMetricsStrategy) policy, err := b.policydb.GetAppPolicy(ctx, serviceBinding.AppID) if err != nil { @@ -720,13 +720,29 @@ 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") } + var combinedConfig *models.BindingConfigWithScaling + if bindingConfig.GetCustomMetricsStrategy() != "" { + combinedConfig = &models.BindingConfigWithScaling{BindingConfig: *bindingConfig} + } if policy != nil { - result.Parameters = policy + areConfigAndPolicyPresent := combinedConfig != nil && policy.InstanceMin > 0 + if areConfigAndPolicyPresent { + combinedConfig.ScalingPolicy = *policy + result.Parameters = combinedConfig + } else { + result.Parameters = policy + } + } else if !b.isEmpty(bindingConfig) { + result.Parameters = bindingConfig } return result, nil } +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}) diff --git a/src/autoscaler/api/broker/broker_test.go b/src/autoscaler/api/broker/broker_test.go index e9ffec9abd..be123e87cc 100644 --- a/src/autoscaler/api/broker/broker_test.go +++ b/src/autoscaler/api/broker/broker_test.go @@ -2,6 +2,7 @@ package broker_test import ( "encoding/json" + "os" "code.cloudfoundry.org/app-autoscaler/src/autoscaler/api/broker" "code.cloudfoundry.org/app-autoscaler/src/autoscaler/db" @@ -19,12 +20,14 @@ import ( var _ = Describe("Broker", func() { var ( - aBroker *broker.Broker - err error - fakeBindingDB *fakes.FakeBindingDB - fakePolicyDB *fakes.FakePolicyDB - fakeCredentials *fakes.FakeCredentials - testLogger = lagertest.NewTestLogger("test") + aBroker *broker.Broker + err error + fakeBindingDB *fakes.FakeBindingDB + fakePolicyDB *fakes.FakePolicyDB + fakeCredentials *fakes.FakeCredentials + testLogger = lagertest.NewTestLogger("test") + bindingConfigWithScaling *models.BindingConfigWithScaling + bindingConfig *models.BindingConfig ) BeforeEach(func() { @@ -153,11 +156,10 @@ var _ = Describe("Broker", func() { }) }) Context("when the binding exists", func() { - BeforeEach(func() { - fakeBindingDB.GetServiceBindingReturns(&models.ServiceBinding{ServiceBindingID: testBindingId, ServiceInstanceID: testInstanceId, AppID: testAppId}, nil) - }) Context("without policy", func() { BeforeEach(func() { + fakeBindingDB.GetServiceBindingReturns(&models.ServiceBinding{ServiceBindingID: testBindingId, + ServiceInstanceID: testInstanceId, AppID: testAppId}, nil) fakePolicyDB.GetAppPolicyReturns(nil, nil) }) It("returns the empty binding without parameters", func() { @@ -175,6 +177,8 @@ var _ = Describe("Broker", func() { }) Context("with policy", func() { BeforeEach(func() { + fakeBindingDB.GetServiceBindingReturns(&models.ServiceBinding{ServiceBindingID: testBindingId, + ServiceInstanceID: testInstanceId, AppID: testAppId}, nil) fakePolicyDB.GetAppPolicyReturns(scalingPolicy, nil) }) It("returns the Binding with parameters", func() { @@ -182,6 +186,48 @@ var _ = Describe("Broker", func() { Expect(Binding).To(Equal(domain.GetBindingSpec{Parameters: scalingPolicy})) }) }) + Context("with configuration and policy", func() { + BeforeEach(func() { + bindingConfig = &models.BindingConfig{Configuration: models.Configuration{CustomMetrics: models.CustomMetricsConfig{ + Auth: models.Auth{}, + 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") + Expect(err).ShouldNot(HaveOccurred()) + + err = json.Unmarshal(bindingBytes, &bindingConfigWithScaling) + Expect(err).ShouldNot(HaveOccurred()) + fakePolicyDB.GetAppPolicyReturns(scalingPolicy, nil) + }) + It("returns the Binding with configs and policy in parameters", func() { + Expect(err).To(BeNil()) + Expect(Binding).To(Equal(domain.GetBindingSpec{Parameters: bindingConfigWithScaling})) + }) + }) + Context("with configuration only", func() { + BeforeEach(func() { + bindingConfig = &models.BindingConfig{Configuration: models.Configuration{CustomMetrics: models.CustomMetricsConfig{ + Auth: models.Auth{}, + 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") + Expect(err).ShouldNot(HaveOccurred()) + + err = json.Unmarshal(bindingBytes, &bindingConfigWithScaling) + Expect(err).ShouldNot(HaveOccurred()) + fakePolicyDB.GetAppPolicyReturns(nil, nil) + }) + It("returns the bindings with configs in parameters", func() { + Expect(err).To(BeNil()) + Expect(Binding).To(Equal(domain.GetBindingSpec{Parameters: bindingConfig})) + }) + }) }) }) diff --git a/src/autoscaler/api/broker/testdata/policy-with-configs.json b/src/autoscaler/api/broker/testdata/policy-with-configs.json new file mode 100644 index 0000000000..3b2c08934a --- /dev/null +++ b/src/autoscaler/api/broker/testdata/policy-with-configs.json @@ -0,0 +1,19 @@ +{ + "configuration": { + "custom_metrics": { + "metric_submission_strategy": { + "allow_from": "bound_app" + } + } + }, + "instance_min_count": 1, + "instance_max_count": 5, + "scaling_rules": [ + { + "metric_type": "memoryused", + "threshold": 30, + "operator": "<", + "adjustment": "-1" + } + ] +} diff --git a/src/autoscaler/api/broker/testdata/with-configs.json b/src/autoscaler/api/broker/testdata/with-configs.json new file mode 100644 index 0000000000..6e51c6d47b --- /dev/null +++ b/src/autoscaler/api/broker/testdata/with-configs.json @@ -0,0 +1,9 @@ +{ + "configuration": { + "custom_metrics": { + "metric_submission_strategy": { + "allow_from": "bound_app" + } + } + } +} diff --git a/src/autoscaler/api/brokerserver/broker_handler_test.go b/src/autoscaler/api/brokerserver/broker_handler_test.go index 190b3748a3..b6d3bd6a98 100644 --- a/src/autoscaler/api/brokerserver/broker_handler_test.go +++ b/src/autoscaler/api/brokerserver/broker_handler_test.go @@ -1500,6 +1500,47 @@ var _ = Describe("BrokerHandler", func() { }) }) }) + + Describe("GetBinding", func() { + var ( + err error + bindingPolicy string + ) + + JustBeforeEach(func() { + req, err = http.NewRequest(http.MethodGet, "", nil) + Expect(err).NotTo(HaveOccurred()) + handler.GetBinding(resp, req) + }) + + Context("Binding configurations are exist", func() { + BeforeEach(func() { + bindingPolicy = `{ + "configuration": { + "custom_metrics": { + "metric_submission_strategy": { + "allow_from": "bound_app" + } + } + }, + "instance_max_count":4, + "instance_min_count":1, + "scaling_rules":[ + { + "metric_type":"memoryused", + "threshold":30, + "operator":"<", + "adjustment":"-1" + }] + }` + Expect(bindingPolicy).NotTo(BeEmpty()) + + }) + It("succeeds with 200", func() { + Expect(resp.Code).To(Equal(http.StatusPreconditionFailed)) + }) + }) + }) }) func verifyCredentialsGenerated(resp *httptest.ResponseRecorder) { diff --git a/src/autoscaler/api/db/servicebroker.db.changelog.yaml b/src/autoscaler/api/db/servicebroker.db.changelog.yaml index b09831d154..62f3a831e4 100644 --- a/src/autoscaler/api/db/servicebroker.db.changelog.yaml +++ b/src/autoscaler/api/db/servicebroker.db.changelog.yaml @@ -165,7 +165,6 @@ databaseChangeLog: - column: name: custom_metrics_strategy value: 'same_app' - - changeSet: id: 7 author: Arsalan diff --git a/src/autoscaler/db/sqldb/binding_sqldb.go b/src/autoscaler/db/sqldb/binding_sqldb.go index 8a165a29f5..8345a1b8da 100644 --- a/src/autoscaler/db/sqldb/binding_sqldb.go +++ b/src/autoscaler/db/sqldb/binding_sqldb.go @@ -244,7 +244,7 @@ func (bdb *BindingSQLDB) GetServiceBinding(ctx context.Context, serviceBindingId serviceBinding := &models.ServiceBinding{} - query := bdb.sqldb.Rebind("SELECT binding_id, service_instance_id, app_id FROM binding WHERE binding_id =?") + query := bdb.sqldb.Rebind("SELECT binding_id, service_instance_id, app_id, custom_metrics_strategy FROM binding WHERE binding_id =?") err := bdb.sqldb.GetContext(ctx, serviceBinding, query, serviceBindingId) if err != nil { diff --git a/src/autoscaler/db/sqldb/binding_sqldb_test.go b/src/autoscaler/db/sqldb/binding_sqldb_test.go index d99b5fb572..e509d0119b 100644 --- a/src/autoscaler/db/sqldb/binding_sqldb_test.go +++ b/src/autoscaler/db/sqldb/binding_sqldb_test.go @@ -439,9 +439,10 @@ var _ = Describe("BindingSqldb", func() { It("should return what was created", func() { Expect(err).NotTo(HaveOccurred()) Expect(retrievedServiceBinding).To(Equal(&models.ServiceBinding{ - ServiceBindingID: testBindingId, - ServiceInstanceID: testInstanceId, - AppID: testAppId, + ServiceBindingID: testBindingId, + ServiceInstanceID: testInstanceId, + AppID: testAppId, + CustomMetricsStrategy: "same_app", })) }) }) diff --git a/src/autoscaler/models/api.go b/src/autoscaler/models/api.go index 9d9d87153b..8f8664e15b 100644 --- a/src/autoscaler/models/api.go +++ b/src/autoscaler/models/api.go @@ -49,9 +49,15 @@ type ServiceInstance struct { } type ServiceBinding struct { - ServiceBindingID string `db:"binding_id"` - ServiceInstanceID string `db:"service_instance_id"` - AppID string `db:"app_id"` + ServiceBindingID string `db:"binding_id"` + ServiceInstanceID string `db:"service_instance_id"` + AppID string `db:"app_id"` + CustomMetricsStrategy string `db:"custom_metrics_strategy"` +} + +type BindingConfigWithScaling struct { + BindingConfig + ScalingPolicy } type BindingRequestBody struct {