Skip to content

Commit

Permalink
Merge pull request #3304 from cloudfoundry/fix-get-binding
Browse files Browse the repository at this point in the history 
Fix(Broker): Return the Same Instance Binding Parameters As Given In Binding Process
  • Loading branch information
@asalan316
asalan316 authored Nov 6, 2024
2 parents fd1c86e + 47e56be commit 2d31a89
Show file tree
Hide file tree
Showing 4 changed files with 227 additions and 95 deletions.
59 changes: 21 additions & 38 deletions src/acceptance/broker/broker_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,25 +95,12 @@ 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))

Expand Down Expand Up @@ -141,21 +128,24 @@ var _ = Describe("AutoScaler Service Broker", func() {
instance.unbind(appName)
})

It("bind&unbinds without policy and gives only configuration", func() {
It("bind&unbinds without policy", func() {
helpers.BindServiceToApp(cfg, appName, instance.name())
By("checking broker bind parameter response does not have policy but configuration only")
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("{}"))
instance.unbind(appName)
})

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)
Expect(bindingParameters).Should(MatchJSON(policyWithConfig))

instance.unbind(appName)
})

Expand All @@ -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")
Expand All @@ -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())
})

Expand All @@ -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")
Expand All @@ -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)
Expand Down
109 changes: 68 additions & 41 deletions src/autoscaler/api/broker/broker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"errors"
"fmt"
"net/http"
"reflect"
"regexp"
"strings"

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -711,38 +733,37 @@ 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) isEmpty(bindingConfig *models.BindingConfig) bool {
return reflect.DeepEqual(bindingConfig, &models.BindingConfig{})
}
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 given in the binding process
combinedConfig = &models.BindingConfigWithScaling{}
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})

Expand Down Expand Up @@ -887,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
}
Loading

0 comments on commit 2d31a89

Please sign in to comment.