From 603425ca23b1a3310dc940692e0534365029fc6d Mon Sep 17 00:00:00 2001 From: Kevin Cross Date: Wed, 17 Aug 2022 14:04:14 +0100 Subject: [PATCH 1/6] Wip --- .../api/brokerserver/broker_handler.go | 33 ++- .../api/brokerserver/broker_handler_test.go | 37 +++- src/autoscaler/api/plancheck/plan_checker.go | 14 +- .../api/plancheck/plan_checker_test.go | 2 +- src/autoscaler/cf/client.go | 97 ++++----- src/autoscaler/cf/service_plans.go | 22 -- src/autoscaler/fakes/fake_cf_client.go | 167 ++++++++++---- src/autoscaler/fakes/fake_plan_checker.go | 203 ++++++++++++++++++ src/autoscaler/fakes/fakes.go | 1 + 9 files changed, 439 insertions(+), 137 deletions(-) create mode 100644 src/autoscaler/fakes/fake_plan_checker.go diff --git a/src/autoscaler/api/brokerserver/broker_handler.go b/src/autoscaler/api/brokerserver/broker_handler.go index 55c53e3b52..4d192dbaad 100644 --- a/src/autoscaler/api/brokerserver/broker_handler.go +++ b/src/autoscaler/api/brokerserver/broker_handler.go @@ -37,7 +37,7 @@ type BrokerHandler struct { schedulerUtil *schedulerutil.SchedulerUtil quotaManagementClient *quota.Client catalog []domain.Service - planChecker *plancheck.PlanChecker + PlanChecker plancheck.PlanChecker cfClient cf.CFClient credentials cred_helper.Credentials } @@ -59,7 +59,7 @@ func NewBrokerHandler(logger lager.Logger, conf *config.Config, bindingdb db.Bin policyValidator: policyvalidator.NewPolicyValidator(conf.PolicySchemaPath, conf.ScalingRules.CPU.LowerThreshold, conf.ScalingRules.CPU.UpperThreshold), schedulerUtil: schedulerutil.NewSchedulerUtil(conf, logger), quotaManagementClient: quota.NewClient(conf, logger), - planChecker: plancheck.NewPlanChecker(conf.PlanCheck, logger), + PlanChecker: plancheck.NewPlanChecker(conf.PlanCheck, logger), cfClient: cfClient, credentials: credentials, } @@ -181,7 +181,7 @@ func (h *BrokerHandler) planDefinitionExceeded(policyStr string, planID string, writeErrorResponse(w, http.StatusInternalServerError, "Error reading policy") return true } - ok, checkResult, err := h.planChecker.CheckPlan(policy, planID) + ok, checkResult, err := h.PlanChecker.CheckPlan(policy, planID) if err != nil { h.logger.Error("failed to check policy for plan adherence", err, lager.Data{"instanceId": instanceId, "policyStr": policyStr}) writeErrorResponse(w, http.StatusInternalServerError, "Error generating validating policy") @@ -283,7 +283,7 @@ func (h *BrokerHandler) UpdateServiceInstance(w http.ResponseWriter, r *http.Req } } - existingServicePlan, err := h.GetBrokerCatalogPlanId(instanceId) + existingServicePlan, err := h.getBrokerCatalogPlanId(instanceId) newServicePlan := body.PlanID if newServicePlan != "" { if err != nil { @@ -292,7 +292,7 @@ func (h *BrokerHandler) UpdateServiceInstance(w http.ResponseWriter, r *http.Req return } if !(existingServicePlan == newServicePlan) { - isPlanUpdatable, err := h.planChecker.IsPlanUpdatable(existingServicePlan) + isPlanUpdatable, err := h.PlanChecker.IsPlanUpdatable(existingServicePlan) if err != nil { h.logger.Error("Plan not found", err) writeErrorResponse(w, http.StatusBadRequest, "Unable to retrieve the service plan") @@ -331,7 +331,7 @@ func (h *BrokerHandler) UpdateServiceInstance(w http.ResponseWriter, r *http.Req } updatedDefaultPolicy = validatedPolicy - servicePlan, err := h.GetBrokerCatalogPlanId(instanceId) + servicePlan, err := h.getBrokerCatalogPlanId(instanceId) if err != nil { h.logger.Error("failed-to-retrieve-service-plan-of-service-instance", err, lager.Data{"instanceId": instanceId}) writeErrorResponse(w, http.StatusInternalServerError, "Error validating policy") @@ -452,8 +452,8 @@ func (h *BrokerHandler) UpdateServiceInstance(w http.ResponseWriter, r *http.Req } } -func (h *BrokerHandler) GetBrokerCatalogPlanId(instanceId string) (string, error) { - return h.cfClient.GetServicePlan(instanceId) +func (h *BrokerHandler) getBrokerCatalogPlanId(instanceId string) (string, error) { + return h.getServicePlanBrokerCatalogId(instanceId) } func (h *BrokerHandler) DeleteServiceInstance(w http.ResponseWriter, _ *http.Request, vars map[string]string) { @@ -732,3 +732,20 @@ func deleteBinding(h *BrokerHandler, bindingId string, serviceInstanceId string) return nil } + +func (h *BrokerHandler) getServicePlanBrokerCatalogId(serviceInstanceGuid string) (string, error) { + serviceInstance, err := h.cfClient.GetServiceInstance(serviceInstanceGuid) + if err != nil { + return "", err + } + servicePlanGuid := serviceInstance.Relationships.ServicePlan.Data.Guid + h.logger.Info("found-guid", lager.Data{"servicePlanGuid": servicePlanGuid}) + + servicePlan, err := h.cfClient.GetServicePlanResource(servicePlanGuid) + if err != nil { + return "", fmt.Errorf("cf-client-get-service-plan: failed to translate Cloud Controller service plan to broker service plan: %w", err) + } + brokerPlanGuid := servicePlan.BrokerCatalog.Id + + return brokerPlanGuid, nil +} diff --git a/src/autoscaler/api/brokerserver/broker_handler_test.go b/src/autoscaler/api/brokerserver/broker_handler_test.go index 74efb21654..c650415b9f 100644 --- a/src/autoscaler/api/brokerserver/broker_handler_test.go +++ b/src/autoscaler/api/brokerserver/broker_handler_test.go @@ -2,6 +2,7 @@ package brokerserver_test import ( "bytes" + "code.cloudfoundry.org/app-autoscaler/src/autoscaler/cf" "database/sql" "encoding/json" "errors" @@ -489,16 +490,34 @@ var _ = Describe("BrokerHandler", func() { instanceUpdateRequestBody.Parameters = ¶meters body, err = json.Marshal(instanceUpdateRequestBody) Expect(err).NotTo(HaveOccurred()) - fakecfClient.GetServicePlanReturns("a-plan-id", nil) + fakecfClient.GetServiceInstanceReturns(&cf.ServiceInstance{ + Relationships: cf.ServiceInstanceRelationships{ + ServicePlan: cf.ServicePlanRelation{ + Data: cf.ServicePlanData{Guid: "a-service-plan-guid"}}}, + }, nil) + fakecfClient.GetServicePlanResourceReturns(&cf.ServicePlan{}, nil) bindingdb.GetServiceInstanceReturns(&models.ServiceInstance{}, nil) }) It("succeeds with 200", func() { Expect(resp.Code).To(Equal(http.StatusOK), DebugTestInfo()) + Expect(fakecfClient.GetServiceInstanceArgsForCall(0)).To(Equal(testInstanceId)) + Expect(fakecfClient.GetServicePlanResourceArgsForCall(0)).To(Equal("a-service-plan-guid")) }) It("retrieves the service instance", func() { Expect(bindingdb.GetServiceInstanceCallCount()).To(Equal(1)) Expect(bindingdb.GetServiceInstanceArgsForCall(0)).To(Equal(testInstanceId)) }) + + Context("with a fake plan checker", func() { + fakePlanChecker := &fakes.FakePlanChecker{} + JustBeforeEach(func() { + handler.PlanChecker = fakePlanChecker + }) + It("it uses the correct plan id", func() { + Expect(fakePlanChecker.CheckPlanArgsForCall(0)).To(Equal("asd")) + }) + }) + }) Context("When a default policy is present and there was previously not a default policy", func() { BeforeEach(func() { @@ -520,7 +539,7 @@ var _ = Describe("BrokerHandler", func() { bindingdb.GetAppIdsByInstanceIdReturns([]string{"app-id-1", "app-id-2"}, nil) policydb.SetOrUpdateDefaultAppPolicyReturns([]string{"app-id-2"}, nil) verifyScheduleIsUpdatedInScheduler("app-id-2", testDefaultPolicy) - fakecfClient.GetServicePlanReturns("a-plan-id", nil) + //fakecfClient.GetServicePlanReturns("a-plan-id", nil) }) It("succeeds with 200, saves the default policy, and sets the default policy on the already bound apps", func() { By("returning 200") @@ -570,7 +589,7 @@ var _ = Describe("BrokerHandler", func() { bindingdb.GetAppIdsByInstanceIdReturns([]string{"app-id-1", "app-id-2"}, nil) policydb.SetOrUpdateDefaultAppPolicyReturns([]string{"app-id-2"}, nil) verifyScheduleIsUpdatedInScheduler("app-id-2", testDefaultPolicy) - fakecfClient.GetServicePlanReturns("a-plan-id", nil) + //fakecfClient.GetServicePlanReturns("a-plan-id", nil) }) It("succeeds with 200, saves the default policy, and updates the default policy", func() { By("returning 200") @@ -622,7 +641,7 @@ var _ = Describe("BrokerHandler", func() { Expect(err).To(BeNil()) policydb.GetAppPolicyReturns(&encodedTestDefaultPolicy, nil) verifyScheduleIsDeletedInScheduler("app-id-2") - fakecfClient.GetServicePlanReturns("a-plan-id", nil) + //fakecfClient.GetServicePlanReturns("a-plan-id", nil) }) It("succeeds with 200 and removes the default policy", func() { By("returning 200") @@ -678,7 +697,7 @@ var _ = Describe("BrokerHandler", func() { ServiceInstanceId: testInstanceId, }, nil) bindingdb.GetAppIdsByInstanceIdReturns([]string{"app-id-2", "app-id-1"}, nil) - fakecfClient.GetServicePlanReturns("a-plan-id", nil) + //fakecfClient.GetServicePlanReturns("a-plan-id", nil) }) It("fails with 400", func() { Expect(resp.Code).To(Equal(http.StatusBadRequest)) @@ -698,7 +717,7 @@ var _ = Describe("BrokerHandler", func() { } body, err = json.Marshal(instanceUpdateRequestBody) Expect(err).NotTo(HaveOccurred()) - fakecfClient.GetServicePlanReturns("a-plan-id", nil) + //fakecfClient.GetServicePlanReturns("a-plan-id", nil) bindingdb.GetServiceInstanceReturns(&models.ServiceInstance{ ServiceInstanceId: testInstanceId, }, nil) @@ -718,7 +737,7 @@ var _ = Describe("BrokerHandler", func() { } body, err = json.Marshal(instanceUpdateRequestBody) Expect(err).NotTo(HaveOccurred()) - fakecfClient.GetServicePlanReturns("a-plan-id", nil) + //fakecfClient.GetServicePlanReturns("a-plan-id", nil) bindingdb.GetServiceInstanceReturns(&models.ServiceInstance{ ServiceInstanceId: testInstanceId, DefaultPolicy: testDefaultPolicy, @@ -745,7 +764,7 @@ var _ = Describe("BrokerHandler", func() { } body, err = json.Marshal(instanceUpdateRequestBody) Expect(err).NotTo(HaveOccurred()) - fakecfClient.GetServicePlanReturns("a-plan-id-not-updatable", nil) + //fakecfClient.GetServicePlanReturns("a-plan-id-not-updatable", nil) bindingdb.GetServiceInstanceReturns(&models.ServiceInstance{ ServiceInstanceId: testInstanceId, }, nil) @@ -776,7 +795,7 @@ var _ = Describe("BrokerHandler", func() { }, nil) policydb.SetOrUpdateDefaultAppPolicyReturns([]string{"app-id-2"}, nil) verifyScheduleIsUpdatedInScheduler("app-id-2", testDefaultPolicy) - fakecfClient.GetServicePlanReturns("a-plan-id", nil) + //fakecfClient.GetServicePlanReturns("a-plan-id", nil) bindingdb.GetAppIdsByInstanceIdReturns([]string{"app-id-1", "app-id-2"}, nil) }) It("fails with 400", func() { diff --git a/src/autoscaler/api/plancheck/plan_checker.go b/src/autoscaler/api/plancheck/plan_checker.go index 43a06c586e..ce870d1921 100644 --- a/src/autoscaler/api/plancheck/plan_checker.go +++ b/src/autoscaler/api/plancheck/plan_checker.go @@ -9,19 +9,23 @@ import ( "code.cloudfoundry.org/lager" ) -type PlanChecker struct { +type planChecker struct { conf *config.PlanCheckConfig logger lager.Logger } +type PlanChecker interface { + CheckPlan(policy models.ScalingPolicy, planID string) (bool, string, error) + IsPlanUpdatable(planID string) (bool, error) +} -func NewPlanChecker(config *config.PlanCheckConfig, logger lager.Logger) *PlanChecker { - return &PlanChecker{ +func NewPlanChecker(config *config.PlanCheckConfig, logger lager.Logger) *planChecker { + return &planChecker{ conf: config, logger: logger.Session("plan-checker"), } } -func (pc PlanChecker) CheckPlan(policy models.ScalingPolicy, planID string) (bool, string, error) { +func (pc planChecker) CheckPlan(policy models.ScalingPolicy, planID string) (bool, string, error) { if pc.conf == nil { pc.logger.Info("plan-checker-not-configured-allowing-all") return true, "", nil @@ -54,7 +58,7 @@ func (pc PlanChecker) CheckPlan(policy models.ScalingPolicy, planID string) (boo } } -func (pc PlanChecker) IsPlanUpdatable(planID string) (bool, error) { +func (pc planChecker) IsPlanUpdatable(planID string) (bool, error) { if pc.conf == nil { return true, nil } diff --git a/src/autoscaler/api/plancheck/plan_checker_test.go b/src/autoscaler/api/plancheck/plan_checker_test.go index a0998857e3..7f9b6156f2 100644 --- a/src/autoscaler/api/plancheck/plan_checker_test.go +++ b/src/autoscaler/api/plancheck/plan_checker_test.go @@ -15,7 +15,7 @@ var _ = Describe("Plan check operations", func() { var ( quotaConfig *config.PlanCheckConfig validationResult string - qmc *plancheck.PlanChecker + qmc *plancheck.planChecker ok bool err error testPolicy models.ScalingPolicy diff --git a/src/autoscaler/cf/client.go b/src/autoscaler/cf/client.go index 79c4846bbe..2bb6865554 100644 --- a/src/autoscaler/cf/client.go +++ b/src/autoscaler/cf/client.go @@ -29,58 +29,60 @@ const ( defaultPerPage = 100 ) -type Tokens struct { - AccessToken string `json:"access_token"` - ExpiresIn int64 `json:"expires_in"` -} +type ( + Tokens struct { + AccessToken string `json:"access_token"` + ExpiresIn int64 `json:"expires_in"` + } -type IntrospectionResponse struct { - Active bool `json:"active"` - Email string `json:"email"` - ClientId string `json:"client_id"` -} + IntrospectionResponse struct { + Active bool `json:"active"` + Email string `json:"email"` + ClientId string `json:"client_id"` + } -type Endpoints struct { - AuthEndpoint string `json:"authorization_endpoint"` - TokenEndpoint string `json:"token_endpoint"` - DopplerEndpoint string `json:"doppler_logging_endpoint"` -} + Endpoints struct { + AuthEndpoint string `json:"authorization_endpoint"` + TokenEndpoint string `json:"token_endpoint"` + DopplerEndpoint string `json:"doppler_logging_endpoint"` + } -var _ CFClient = &Client{} + CFClient interface { + Login() error + RefreshAuthToken() (string, error) + GetTokens() (Tokens, error) + GetEndpoints() Endpoints + GetApp(string) (*App, error) + GetAppProcesses(string) (Processes, error) + GetAppAndProcesses(string) (*AppAndProcesses, error) + ScaleAppWebProcess(string, int) error + IsUserAdmin(userToken string) (bool, error) + IsUserSpaceDeveloper(userToken string, appId string) (bool, error) + IsTokenAuthorized(token, clientId string) (bool, error) + GetServiceInstance(serviceInstanceGuid string) (*ServiceInstance, error) + GetServicePlanResource(servicePlanGuid string) (*ServicePlan, error) + } -type CFClient interface { - Login() error - RefreshAuthToken() (string, error) - GetTokens() (Tokens, error) - GetEndpoints() Endpoints - GetApp(string) (*App, error) - GetAppProcesses(string) (Processes, error) - GetAppAndProcesses(string) (*AppAndProcesses, error) - ScaleAppWebProcess(string, int) error - IsUserAdmin(userToken string) (bool, error) - IsUserSpaceDeveloper(userToken string, appId string) (bool, error) - IsTokenAuthorized(token, clientId string) (bool, error) - GetServicePlan(serviceInstanceGuid string) (string, error) -} + Client struct { + logger lager.Logger + conf *Config + clk clock.Clock + tokens Tokens + endpoints Endpoints + infoURL string + tokenURL string + introspectTokenURL string + loginForm url.Values + authHeader string + httpClient *http.Client + lock *sync.Mutex + grantTime time.Time + retryClient *http.Client + brokerPlanGuid *Memoizer[string, string] + } +) -type Client struct { - logger lager.Logger - conf *Config - clk clock.Clock - tokens Tokens - endpoints Endpoints - infoURL string - tokenURL string - introspectTokenURL string - loginForm url.Values - authHeader string - httpClient *http.Client - lock *sync.Mutex - grantTime time.Time - retryClient *http.Client - servicePlan *Memoizer[string, string] - brokerPlanGuid *Memoizer[string, string] -} +var _ CFClient = &Client{} func NewCFClient(conf *Config, logger lager.Logger, clk clock.Clock) *Client { c := &Client{} @@ -104,7 +106,6 @@ func NewCFClient(conf *Config, logger lager.Logger, clk clock.Clock) *Client { c.retryClient = createRetryClient(conf, c.httpClient, logger) c.lock = &sync.Mutex{} - c.servicePlan = NewMemoizer(c.getServicePlan) c.brokerPlanGuid = NewMemoizer(c.getBrokerPlanGuid) if c.conf.PerPage == 0 { diff --git a/src/autoscaler/cf/service_plans.go b/src/autoscaler/cf/service_plans.go index b78534e6b4..55d15876f8 100644 --- a/src/autoscaler/cf/service_plans.go +++ b/src/autoscaler/cf/service_plans.go @@ -50,25 +50,3 @@ func (c *Client) GetServicePlanResource(servicePlanGuid string) (*ServicePlan, e } return plan, nil } - -// GetServicePlan -// This function does not really get a service plan but the service plan's broker catalog id -func (c *Client) GetServicePlan(serviceInstanceGuid string) (string, error) { - return c.servicePlan.Func(serviceInstanceGuid) -} - -func (c *Client) getServicePlan(serviceInstanceGuid string) (string, error) { - result, err := c.GetServiceInstance(serviceInstanceGuid) - if err != nil { - return "", err - } - - servicePlanGuid := result.Relationships.ServicePlan.Data.Guid - c.logger.Info("found-guid", lager.Data{"servicePlanGuid": servicePlanGuid}) - brokerPlanGuid, err := c.GetBrokerPlanGuid(servicePlanGuid) - if err != nil { - c.logger.Error("cc-plan-to-broker-plan", err) - return "", fmt.Errorf("cf-client-get-service-plan: failed to translate Cloud Controller service plan to broker service plan: %w", err) - } - return brokerPlanGuid, nil -} diff --git a/src/autoscaler/fakes/fake_cf_client.go b/src/autoscaler/fakes/fake_cf_client.go index 7d03f6000b..77cfd23a1a 100644 --- a/src/autoscaler/fakes/fake_cf_client.go +++ b/src/autoscaler/fakes/fake_cf_client.go @@ -57,17 +57,30 @@ type FakeCFClient struct { getEndpointsReturnsOnCall map[int]struct { result1 cf.Endpoints } - GetServicePlanStub func(string) (string, error) - getServicePlanMutex sync.RWMutex - getServicePlanArgsForCall []struct { + GetServiceInstanceStub func(string) (*cf.ServiceInstance, error) + getServiceInstanceMutex sync.RWMutex + getServiceInstanceArgsForCall []struct { arg1 string } - getServicePlanReturns struct { - result1 string + getServiceInstanceReturns struct { + result1 *cf.ServiceInstance result2 error } - getServicePlanReturnsOnCall map[int]struct { - result1 string + getServiceInstanceReturnsOnCall map[int]struct { + result1 *cf.ServiceInstance + result2 error + } + GetServicePlanResourceStub func(string) (*cf.ServicePlan, error) + getServicePlanResourceMutex sync.RWMutex + getServicePlanResourceArgsForCall []struct { + arg1 string + } + getServicePlanResourceReturns struct { + result1 *cf.ServicePlan + result2 error + } + getServicePlanResourceReturnsOnCall map[int]struct { + result1 *cf.ServicePlan result2 error } GetTokensStub func() (cf.Tokens, error) @@ -406,16 +419,16 @@ func (fake *FakeCFClient) GetEndpointsReturnsOnCall(i int, result1 cf.Endpoints) }{result1} } -func (fake *FakeCFClient) GetServicePlan(arg1 string) (string, error) { - fake.getServicePlanMutex.Lock() - ret, specificReturn := fake.getServicePlanReturnsOnCall[len(fake.getServicePlanArgsForCall)] - fake.getServicePlanArgsForCall = append(fake.getServicePlanArgsForCall, struct { +func (fake *FakeCFClient) GetServiceInstance(arg1 string) (*cf.ServiceInstance, error) { + fake.getServiceInstanceMutex.Lock() + ret, specificReturn := fake.getServiceInstanceReturnsOnCall[len(fake.getServiceInstanceArgsForCall)] + fake.getServiceInstanceArgsForCall = append(fake.getServiceInstanceArgsForCall, struct { arg1 string }{arg1}) - stub := fake.GetServicePlanStub - fakeReturns := fake.getServicePlanReturns - fake.recordInvocation("GetServicePlan", []interface{}{arg1}) - fake.getServicePlanMutex.Unlock() + stub := fake.GetServiceInstanceStub + fakeReturns := fake.getServiceInstanceReturns + fake.recordInvocation("GetServiceInstance", []interface{}{arg1}) + fake.getServiceInstanceMutex.Unlock() if stub != nil { return stub(arg1) } @@ -425,47 +438,111 @@ func (fake *FakeCFClient) GetServicePlan(arg1 string) (string, error) { return fakeReturns.result1, fakeReturns.result2 } -func (fake *FakeCFClient) GetServicePlanCallCount() int { - fake.getServicePlanMutex.RLock() - defer fake.getServicePlanMutex.RUnlock() - return len(fake.getServicePlanArgsForCall) +func (fake *FakeCFClient) GetServiceInstanceCallCount() int { + fake.getServiceInstanceMutex.RLock() + defer fake.getServiceInstanceMutex.RUnlock() + return len(fake.getServiceInstanceArgsForCall) } -func (fake *FakeCFClient) GetServicePlanCalls(stub func(string) (string, error)) { - fake.getServicePlanMutex.Lock() - defer fake.getServicePlanMutex.Unlock() - fake.GetServicePlanStub = stub +func (fake *FakeCFClient) GetServiceInstanceCalls(stub func(string) (*cf.ServiceInstance, error)) { + fake.getServiceInstanceMutex.Lock() + defer fake.getServiceInstanceMutex.Unlock() + fake.GetServiceInstanceStub = stub } -func (fake *FakeCFClient) GetServicePlanArgsForCall(i int) string { - fake.getServicePlanMutex.RLock() - defer fake.getServicePlanMutex.RUnlock() - argsForCall := fake.getServicePlanArgsForCall[i] +func (fake *FakeCFClient) GetServiceInstanceArgsForCall(i int) string { + fake.getServiceInstanceMutex.RLock() + defer fake.getServiceInstanceMutex.RUnlock() + argsForCall := fake.getServiceInstanceArgsForCall[i] return argsForCall.arg1 } -func (fake *FakeCFClient) GetServicePlanReturns(result1 string, result2 error) { - fake.getServicePlanMutex.Lock() - defer fake.getServicePlanMutex.Unlock() - fake.GetServicePlanStub = nil - fake.getServicePlanReturns = struct { - result1 string +func (fake *FakeCFClient) GetServiceInstanceReturns(result1 *cf.ServiceInstance, result2 error) { + fake.getServiceInstanceMutex.Lock() + defer fake.getServiceInstanceMutex.Unlock() + fake.GetServiceInstanceStub = nil + fake.getServiceInstanceReturns = struct { + result1 *cf.ServiceInstance result2 error }{result1, result2} } -func (fake *FakeCFClient) GetServicePlanReturnsOnCall(i int, result1 string, result2 error) { - fake.getServicePlanMutex.Lock() - defer fake.getServicePlanMutex.Unlock() - fake.GetServicePlanStub = nil - if fake.getServicePlanReturnsOnCall == nil { - fake.getServicePlanReturnsOnCall = make(map[int]struct { - result1 string +func (fake *FakeCFClient) GetServiceInstanceReturnsOnCall(i int, result1 *cf.ServiceInstance, result2 error) { + fake.getServiceInstanceMutex.Lock() + defer fake.getServiceInstanceMutex.Unlock() + fake.GetServiceInstanceStub = nil + if fake.getServiceInstanceReturnsOnCall == nil { + fake.getServiceInstanceReturnsOnCall = make(map[int]struct { + result1 *cf.ServiceInstance result2 error }) } - fake.getServicePlanReturnsOnCall[i] = struct { - result1 string + fake.getServiceInstanceReturnsOnCall[i] = struct { + result1 *cf.ServiceInstance + result2 error + }{result1, result2} +} + +func (fake *FakeCFClient) GetServicePlanResource(arg1 string) (*cf.ServicePlan, error) { + fake.getServicePlanResourceMutex.Lock() + ret, specificReturn := fake.getServicePlanResourceReturnsOnCall[len(fake.getServicePlanResourceArgsForCall)] + fake.getServicePlanResourceArgsForCall = append(fake.getServicePlanResourceArgsForCall, struct { + arg1 string + }{arg1}) + stub := fake.GetServicePlanResourceStub + fakeReturns := fake.getServicePlanResourceReturns + fake.recordInvocation("GetServicePlanResource", []interface{}{arg1}) + fake.getServicePlanResourceMutex.Unlock() + if stub != nil { + return stub(arg1) + } + if specificReturn { + return ret.result1, ret.result2 + } + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *FakeCFClient) GetServicePlanResourceCallCount() int { + fake.getServicePlanResourceMutex.RLock() + defer fake.getServicePlanResourceMutex.RUnlock() + return len(fake.getServicePlanResourceArgsForCall) +} + +func (fake *FakeCFClient) GetServicePlanResourceCalls(stub func(string) (*cf.ServicePlan, error)) { + fake.getServicePlanResourceMutex.Lock() + defer fake.getServicePlanResourceMutex.Unlock() + fake.GetServicePlanResourceStub = stub +} + +func (fake *FakeCFClient) GetServicePlanResourceArgsForCall(i int) string { + fake.getServicePlanResourceMutex.RLock() + defer fake.getServicePlanResourceMutex.RUnlock() + argsForCall := fake.getServicePlanResourceArgsForCall[i] + return argsForCall.arg1 +} + +func (fake *FakeCFClient) GetServicePlanResourceReturns(result1 *cf.ServicePlan, result2 error) { + fake.getServicePlanResourceMutex.Lock() + defer fake.getServicePlanResourceMutex.Unlock() + fake.GetServicePlanResourceStub = nil + fake.getServicePlanResourceReturns = struct { + result1 *cf.ServicePlan + result2 error + }{result1, result2} +} + +func (fake *FakeCFClient) GetServicePlanResourceReturnsOnCall(i int, result1 *cf.ServicePlan, result2 error) { + fake.getServicePlanResourceMutex.Lock() + defer fake.getServicePlanResourceMutex.Unlock() + fake.GetServicePlanResourceStub = nil + if fake.getServicePlanResourceReturnsOnCall == nil { + fake.getServicePlanResourceReturnsOnCall = make(map[int]struct { + result1 *cf.ServicePlan + result2 error + }) + } + fake.getServicePlanResourceReturnsOnCall[i] = struct { + result1 *cf.ServicePlan result2 error }{result1, result2} } @@ -902,8 +979,10 @@ func (fake *FakeCFClient) Invocations() map[string][][]interface{} { defer fake.getAppProcessesMutex.RUnlock() fake.getEndpointsMutex.RLock() defer fake.getEndpointsMutex.RUnlock() - fake.getServicePlanMutex.RLock() - defer fake.getServicePlanMutex.RUnlock() + fake.getServiceInstanceMutex.RLock() + defer fake.getServiceInstanceMutex.RUnlock() + fake.getServicePlanResourceMutex.RLock() + defer fake.getServicePlanResourceMutex.RUnlock() fake.getTokensMutex.RLock() defer fake.getTokensMutex.RUnlock() fake.isTokenAuthorizedMutex.RLock() diff --git a/src/autoscaler/fakes/fake_plan_checker.go b/src/autoscaler/fakes/fake_plan_checker.go new file mode 100644 index 0000000000..da7a3fb7fa --- /dev/null +++ b/src/autoscaler/fakes/fake_plan_checker.go @@ -0,0 +1,203 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package fakes + +import ( + "sync" + + "code.cloudfoundry.org/app-autoscaler/src/autoscaler/api/plancheck" + "code.cloudfoundry.org/app-autoscaler/src/autoscaler/models" +) + +type FakePlanChecker struct { + CheckPlanStub func(models.ScalingPolicy, string) (bool, string, error) + checkPlanMutex sync.RWMutex + checkPlanArgsForCall []struct { + arg1 models.ScalingPolicy + arg2 string + } + checkPlanReturns struct { + result1 bool + result2 string + result3 error + } + checkPlanReturnsOnCall map[int]struct { + result1 bool + result2 string + result3 error + } + IsPlanUpdatableStub func(string) (bool, error) + isPlanUpdatableMutex sync.RWMutex + isPlanUpdatableArgsForCall []struct { + arg1 string + } + isPlanUpdatableReturns struct { + result1 bool + result2 error + } + isPlanUpdatableReturnsOnCall map[int]struct { + result1 bool + result2 error + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *FakePlanChecker) CheckPlan(arg1 models.ScalingPolicy, arg2 string) (bool, string, error) { + fake.checkPlanMutex.Lock() + ret, specificReturn := fake.checkPlanReturnsOnCall[len(fake.checkPlanArgsForCall)] + fake.checkPlanArgsForCall = append(fake.checkPlanArgsForCall, struct { + arg1 models.ScalingPolicy + arg2 string + }{arg1, arg2}) + stub := fake.CheckPlanStub + fakeReturns := fake.checkPlanReturns + fake.recordInvocation("CheckPlan", []interface{}{arg1, arg2}) + fake.checkPlanMutex.Unlock() + if stub != nil { + return stub(arg1, arg2) + } + if specificReturn { + return ret.result1, ret.result2, ret.result3 + } + return fakeReturns.result1, fakeReturns.result2, fakeReturns.result3 +} + +func (fake *FakePlanChecker) CheckPlanCallCount() int { + fake.checkPlanMutex.RLock() + defer fake.checkPlanMutex.RUnlock() + return len(fake.checkPlanArgsForCall) +} + +func (fake *FakePlanChecker) CheckPlanCalls(stub func(models.ScalingPolicy, string) (bool, string, error)) { + fake.checkPlanMutex.Lock() + defer fake.checkPlanMutex.Unlock() + fake.CheckPlanStub = stub +} + +func (fake *FakePlanChecker) CheckPlanArgsForCall(i int) (models.ScalingPolicy, string) { + fake.checkPlanMutex.RLock() + defer fake.checkPlanMutex.RUnlock() + argsForCall := fake.checkPlanArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + +func (fake *FakePlanChecker) CheckPlanReturns(result1 bool, result2 string, result3 error) { + fake.checkPlanMutex.Lock() + defer fake.checkPlanMutex.Unlock() + fake.CheckPlanStub = nil + fake.checkPlanReturns = struct { + result1 bool + result2 string + result3 error + }{result1, result2, result3} +} + +func (fake *FakePlanChecker) CheckPlanReturnsOnCall(i int, result1 bool, result2 string, result3 error) { + fake.checkPlanMutex.Lock() + defer fake.checkPlanMutex.Unlock() + fake.CheckPlanStub = nil + if fake.checkPlanReturnsOnCall == nil { + fake.checkPlanReturnsOnCall = make(map[int]struct { + result1 bool + result2 string + result3 error + }) + } + fake.checkPlanReturnsOnCall[i] = struct { + result1 bool + result2 string + result3 error + }{result1, result2, result3} +} + +func (fake *FakePlanChecker) IsPlanUpdatable(arg1 string) (bool, error) { + fake.isPlanUpdatableMutex.Lock() + ret, specificReturn := fake.isPlanUpdatableReturnsOnCall[len(fake.isPlanUpdatableArgsForCall)] + fake.isPlanUpdatableArgsForCall = append(fake.isPlanUpdatableArgsForCall, struct { + arg1 string + }{arg1}) + stub := fake.IsPlanUpdatableStub + fakeReturns := fake.isPlanUpdatableReturns + fake.recordInvocation("IsPlanUpdatable", []interface{}{arg1}) + fake.isPlanUpdatableMutex.Unlock() + if stub != nil { + return stub(arg1) + } + if specificReturn { + return ret.result1, ret.result2 + } + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *FakePlanChecker) IsPlanUpdatableCallCount() int { + fake.isPlanUpdatableMutex.RLock() + defer fake.isPlanUpdatableMutex.RUnlock() + return len(fake.isPlanUpdatableArgsForCall) +} + +func (fake *FakePlanChecker) IsPlanUpdatableCalls(stub func(string) (bool, error)) { + fake.isPlanUpdatableMutex.Lock() + defer fake.isPlanUpdatableMutex.Unlock() + fake.IsPlanUpdatableStub = stub +} + +func (fake *FakePlanChecker) IsPlanUpdatableArgsForCall(i int) string { + fake.isPlanUpdatableMutex.RLock() + defer fake.isPlanUpdatableMutex.RUnlock() + argsForCall := fake.isPlanUpdatableArgsForCall[i] + return argsForCall.arg1 +} + +func (fake *FakePlanChecker) IsPlanUpdatableReturns(result1 bool, result2 error) { + fake.isPlanUpdatableMutex.Lock() + defer fake.isPlanUpdatableMutex.Unlock() + fake.IsPlanUpdatableStub = nil + fake.isPlanUpdatableReturns = struct { + result1 bool + result2 error + }{result1, result2} +} + +func (fake *FakePlanChecker) IsPlanUpdatableReturnsOnCall(i int, result1 bool, result2 error) { + fake.isPlanUpdatableMutex.Lock() + defer fake.isPlanUpdatableMutex.Unlock() + fake.IsPlanUpdatableStub = nil + if fake.isPlanUpdatableReturnsOnCall == nil { + fake.isPlanUpdatableReturnsOnCall = make(map[int]struct { + result1 bool + result2 error + }) + } + fake.isPlanUpdatableReturnsOnCall[i] = struct { + result1 bool + result2 error + }{result1, result2} +} + +func (fake *FakePlanChecker) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.checkPlanMutex.RLock() + defer fake.checkPlanMutex.RUnlock() + fake.isPlanUpdatableMutex.RLock() + defer fake.isPlanUpdatableMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *FakePlanChecker) recordInvocation(key string, args []interface{}) { + fake.invocationsMutex.Lock() + defer fake.invocationsMutex.Unlock() + if fake.invocations == nil { + fake.invocations = map[string][][]interface{}{} + } + if fake.invocations[key] == nil { + fake.invocations[key] = [][]interface{}{} + } + fake.invocations[key] = append(fake.invocations[key], args) +} + +var _ plancheck.PlanChecker = new(FakePlanChecker) diff --git a/src/autoscaler/fakes/fakes.go b/src/autoscaler/fakes/fakes.go index 46441b5237..c295a8bcfe 100644 --- a/src/autoscaler/fakes/fakes.go +++ b/src/autoscaler/fakes/fakes.go @@ -17,3 +17,4 @@ package fakes //go:generate counterfeiter -o ./fake_grpc.go ../eventgenerator/client grpcDialOptions //go:generate counterfeiter -o ./fake_tls_config.go ../eventgenerator/client TLSConfig //go:generate counterfeiter -o ./fake_envelope_processor_creator.go ../envelopeprocessor EnvelopeProcessorCreator +//go:generate counterfeiter -o ./fake_plan_checker.go ../api/plancheck PlanChecker From 67110fa52a568ceaf5924f3692217cdd3fe513c9 Mon Sep 17 00:00:00 2001 From: Kevin Cross Date: Thu, 18 Aug 2022 12:07:04 +0100 Subject: [PATCH 2/6] Added tests for the handler function --- .../api/brokerserver/broker_handler.go | 12 +- .../api/brokerserver/broker_handler_test.go | 143 ++++++++++++------ src/autoscaler/cf/client.go | 2 +- src/autoscaler/cf/service_plans.go | 6 +- src/autoscaler/cf/service_plans_test.go | 8 +- src/autoscaler/fakes/fake_cf_client.go | 78 +++++----- src/autoscaler/testhelpers/mocks_test.go | 2 +- 7 files changed, 148 insertions(+), 103 deletions(-) diff --git a/src/autoscaler/api/brokerserver/broker_handler.go b/src/autoscaler/api/brokerserver/broker_handler.go index 4d192dbaad..969bac1c05 100644 --- a/src/autoscaler/api/brokerserver/broker_handler.go +++ b/src/autoscaler/api/brokerserver/broker_handler.go @@ -284,13 +284,13 @@ func (h *BrokerHandler) UpdateServiceInstance(w http.ResponseWriter, r *http.Req } existingServicePlan, err := h.getBrokerCatalogPlanId(instanceId) + if err != nil { + h.logger.Error("Error retrieving broker plan Id", err, lager.Data{"instanceId": instanceId}) + writeErrorResponse(w, http.StatusInternalServerError, "Error retrieving broker plan Id") + return + } newServicePlan := body.PlanID if newServicePlan != "" { - if err != nil { - h.logger.Error("failed-to-retrieve-service-plan-of-service-instance", err, lager.Data{"instanceId": instanceId}) - writeErrorResponse(w, http.StatusInternalServerError, "Error validating policy") - return - } if !(existingServicePlan == newServicePlan) { isPlanUpdatable, err := h.PlanChecker.IsPlanUpdatable(existingServicePlan) if err != nil { @@ -741,7 +741,7 @@ func (h *BrokerHandler) getServicePlanBrokerCatalogId(serviceInstanceGuid string servicePlanGuid := serviceInstance.Relationships.ServicePlan.Data.Guid h.logger.Info("found-guid", lager.Data{"servicePlanGuid": servicePlanGuid}) - servicePlan, err := h.cfClient.GetServicePlanResource(servicePlanGuid) + servicePlan, err := h.cfClient.GetServicePlan(servicePlanGuid) if err != nil { return "", fmt.Errorf("cf-client-get-service-plan: failed to translate Cloud Controller service plan to broker service plan: %w", err) } diff --git a/src/autoscaler/api/brokerserver/broker_handler_test.go b/src/autoscaler/api/brokerserver/broker_handler_test.go index c650415b9f..f65b037497 100644 --- a/src/autoscaler/api/brokerserver/broker_handler_test.go +++ b/src/autoscaler/api/brokerserver/broker_handler_test.go @@ -2,7 +2,6 @@ package brokerserver_test import ( "bytes" - "code.cloudfoundry.org/app-autoscaler/src/autoscaler/cf" "database/sql" "encoding/json" "errors" @@ -11,6 +10,8 @@ import ( "net/http" "net/http/httptest" + "code.cloudfoundry.org/app-autoscaler/src/autoscaler/cf" + . "code.cloudfoundry.org/app-autoscaler/src/autoscaler/api/brokerserver" "code.cloudfoundry.org/app-autoscaler/src/autoscaler/db" "code.cloudfoundry.org/app-autoscaler/src/autoscaler/fakes" @@ -35,6 +36,7 @@ var _ = Describe("BrokerHandler", func() { bindingdb *fakes.FakeBindingDB policydb *fakes.FakePolicyDB fakeCredentials *fakes.FakeCredentials + fakePlanChecker *fakes.FakePlanChecker handler *BrokerHandler resp *httptest.ResponseRecorder @@ -47,6 +49,7 @@ var _ = Describe("BrokerHandler", func() { installQuotaAPIHandlers() fakecfClient = &fakes.FakeCFClient{} fakeCredentials = &fakes.FakeCredentials{} + fakePlanChecker = nil }) JustBeforeEach(func() { @@ -58,6 +61,9 @@ var _ = Describe("BrokerHandler", func() { Name: "standard", }}, }}, fakecfClient, fakeCredentials) + if fakePlanChecker != nil { + handler.PlanChecker = fakePlanChecker + } }) Describe("GetBrokerCatalog", func() { @@ -385,6 +391,15 @@ var _ = Describe("BrokerHandler", func() { var err error var instanceUpdateRequestBody *models.InstanceUpdateRequestBody var body []byte + servicePlanGuid := "a-service-plan-guid" + setupCfClient := func(brokerCatalogId string) { + fakecfClient.GetServiceInstanceReturns(&cf.ServiceInstance{ + Relationships: cf.ServiceInstanceRelationships{ + ServicePlan: cf.ServicePlanRelation{ + Data: cf.ServicePlanData{Guid: servicePlanGuid}}}, + }, nil) + fakecfClient.GetServicePlanReturns(&cf.ServicePlan{BrokerCatalog: cf.BrokerCatalog{Id: brokerCatalogId}}, nil) + } JustBeforeEach(func() { req, err = http.NewRequest(http.MethodPut, "", bytes.NewReader(body)) handler.UpdateServiceInstance(resp, req, map[string]string{"instanceId": testInstanceId}) @@ -458,6 +473,7 @@ var _ = Describe("BrokerHandler", func() { body, err = json.Marshal(instanceUpdateRequestBody) Expect(err).NotTo(HaveOccurred()) bindingdb.GetServiceInstanceReturns(&models.ServiceInstance{}, nil) + setupCfClient("a-plan-id") }) It("fails with 400", func() { Expect(resp.Code).To(Equal(http.StatusBadRequest), DebugTestInfo()) @@ -484,42 +500,26 @@ var _ = Describe("BrokerHandler", func() { }) }) Context("When all mandatory parameters are present", func() { + BeforeEach(func() { emptyPolicyParameter := json.RawMessage("\n{\t}\n") parameters := models.InstanceParameters{DefaultPolicy: &emptyPolicyParameter} instanceUpdateRequestBody.Parameters = ¶meters body, err = json.Marshal(instanceUpdateRequestBody) Expect(err).NotTo(HaveOccurred()) - fakecfClient.GetServiceInstanceReturns(&cf.ServiceInstance{ - Relationships: cf.ServiceInstanceRelationships{ - ServicePlan: cf.ServicePlanRelation{ - Data: cf.ServicePlanData{Guid: "a-service-plan-guid"}}}, - }, nil) - fakecfClient.GetServicePlanResourceReturns(&cf.ServicePlan{}, nil) + setupCfClient("some-broker-guid") bindingdb.GetServiceInstanceReturns(&models.ServiceInstance{}, nil) }) It("succeeds with 200", func() { Expect(resp.Code).To(Equal(http.StatusOK), DebugTestInfo()) - Expect(fakecfClient.GetServiceInstanceArgsForCall(0)).To(Equal(testInstanceId)) - Expect(fakecfClient.GetServicePlanResourceArgsForCall(0)).To(Equal("a-service-plan-guid")) }) It("retrieves the service instance", func() { Expect(bindingdb.GetServiceInstanceCallCount()).To(Equal(1)) Expect(bindingdb.GetServiceInstanceArgsForCall(0)).To(Equal(testInstanceId)) }) - - Context("with a fake plan checker", func() { - fakePlanChecker := &fakes.FakePlanChecker{} - JustBeforeEach(func() { - handler.PlanChecker = fakePlanChecker - }) - It("it uses the correct plan id", func() { - Expect(fakePlanChecker.CheckPlanArgsForCall(0)).To(Equal("asd")) - }) - }) - }) Context("When a default policy is present and there was previously not a default policy", func() { + BeforeEach(func() { d := json.RawMessage(testDefaultPolicy) instanceUpdateRequestBody = &models.InstanceUpdateRequestBody{ @@ -539,32 +539,77 @@ var _ = Describe("BrokerHandler", func() { bindingdb.GetAppIdsByInstanceIdReturns([]string{"app-id-1", "app-id-2"}, nil) policydb.SetOrUpdateDefaultAppPolicyReturns([]string{"app-id-2"}, nil) verifyScheduleIsUpdatedInScheduler("app-id-2", testDefaultPolicy) - //fakecfClient.GetServicePlanReturns("a-plan-id", nil) + setupCfClient("a-plan-id") }) - It("succeeds with 200, saves the default policy, and sets the default policy on the already bound apps", func() { - By("returning 200") - Expect(resp.Code).To(Equal(http.StatusOK), DebugTestInfo()) + Context("successfully", func() { + BeforeEach(func() { setupCfClient("a-plan-id") }) + It("succeeds with 200, saves the default policy, and sets the default policy on the already bound apps", func() { + By("returning 200") + Expect(resp.Code).To(Equal(http.StatusOK), DebugTestInfo()) - By("saving the default policy") - Expect(bindingdb.UpdateServiceInstanceCallCount()).To(Equal(1)) - serviceInstance := bindingdb.UpdateServiceInstanceArgsForCall(0) - Expect(serviceInstance.ServiceInstanceId).To(Equal(testInstanceId)) - Expect(serviceInstance.DefaultPolicy).To(MatchJSON(testDefaultPolicy)) - Expect(serviceInstance.DefaultPolicyGuid).To(HaveLen(36)) + By("saving the default policy") + Expect(bindingdb.UpdateServiceInstanceCallCount()).To(Equal(1)) + serviceInstance := bindingdb.UpdateServiceInstanceArgsForCall(0) + Expect(serviceInstance.ServiceInstanceId).To(Equal(testInstanceId)) + Expect(serviceInstance.DefaultPolicy).To(MatchJSON(testDefaultPolicy)) + Expect(serviceInstance.DefaultPolicyGuid).To(HaveLen(36)) - By("setting the default policy on the already bound apps") - Expect(bindingdb.GetAppIdsByInstanceIdCallCount()).To(Equal(1)) - lookedUpInstance := bindingdb.GetAppIdsByInstanceIdArgsForCall(0) - Expect(lookedUpInstance).To(Equal(testInstanceId)) - Expect(policydb.SetOrUpdateDefaultAppPolicyCallCount()).To(Equal(1)) - appsUpdated, oldPolicyGuid, policySet, policySetGuid := policydb.SetOrUpdateDefaultAppPolicyArgsForCall(0) - Expect(oldPolicyGuid).To(BeEmpty()) - Expect(policySetGuid).To(Equal(serviceInstance.DefaultPolicyGuid)) - Expect(policySet).To(Equal(serviceInstance.DefaultPolicy)) - Expect(appsUpdated).To(Equal([]string{"app-id-1", "app-id-2"})) + By("setting the default policy on the already bound apps") + Expect(bindingdb.GetAppIdsByInstanceIdCallCount()).To(Equal(1)) + lookedUpInstance := bindingdb.GetAppIdsByInstanceIdArgsForCall(0) + Expect(lookedUpInstance).To(Equal(testInstanceId)) + Expect(policydb.SetOrUpdateDefaultAppPolicyCallCount()).To(Equal(1)) + appsUpdated, oldPolicyGuid, policySet, policySetGuid := policydb.SetOrUpdateDefaultAppPolicyArgsForCall(0) + Expect(oldPolicyGuid).To(BeEmpty()) + Expect(policySetGuid).To(Equal(serviceInstance.DefaultPolicyGuid)) + Expect(policySet).To(Equal(serviceInstance.DefaultPolicy)) + Expect(appsUpdated).To(Equal([]string{"app-id-1", "app-id-2"})) - By("updating the scheduler") - Expect(schedulerServer.ReceivedRequests()).To(HaveLen(1)) + By("updating the scheduler") + Expect(schedulerServer.ReceivedRequests()).To(HaveLen(1)) + + Expect(fakecfClient.GetServiceInstanceArgsForCall(0)).To(Equal(testInstanceId)) + Expect(fakecfClient.GetServicePlanArgsForCall(0)).To(Equal(servicePlanGuid)) + }) + }) + Context("with a fake plan checker", func() { + BeforeEach(func() { setupCfClient("a-plan-id") }) + BeforeEach(func() { fakePlanChecker = &fakes.FakePlanChecker{} }) + It("it uses the correct plan id", func() { + _, s := fakePlanChecker.CheckPlanArgsForCall(0) + Expect(s).To(Equal("a-plan-id")) + }) + }) + Context("When there are cf client errors", func() { + When("get service_instance fails", func() { + BeforeEach(func() { + fakecfClient.GetServiceInstanceReturns(nil, errors.New("SomeError")) + }) + + It("Fails correctly", func() { + Expect(resp.Code).To(Equal(http.StatusInternalServerError), DebugTestInfo()) + bodyBytes, err := ioutil.ReadAll(resp.Body) + Expect(err).NotTo(HaveOccurred()) + Expect(string(bodyBytes)).To(MatchJSON(`{"code": "Internal Server Error","message": "Error retrieving broker plan Id"}`)) + }) + }) + + When("get service_plan fails", func() { + BeforeEach(func() { + fakecfClient.GetServiceInstanceReturns(&cf.ServiceInstance{ + Relationships: cf.ServiceInstanceRelationships{ + ServicePlan: cf.ServicePlanRelation{ + Data: cf.ServicePlanData{Guid: servicePlanGuid}}}, + }, nil) + fakecfClient.GetServicePlanReturns(nil, errors.New("SomeError")) + }) + It("Fails correctly", func() { + Expect(resp.Code).To(Equal(http.StatusInternalServerError), DebugTestInfo()) + bodyBytes, err := ioutil.ReadAll(resp.Body) + Expect(err).NotTo(HaveOccurred()) + Expect(string(bodyBytes)).To(MatchJSON(`{"code": "Internal Server Error","message": "Error retrieving broker plan Id"}`)) + }) + }) }) }) Context("When a default policy is present and there was previously a default policy", func() { @@ -589,7 +634,7 @@ var _ = Describe("BrokerHandler", func() { bindingdb.GetAppIdsByInstanceIdReturns([]string{"app-id-1", "app-id-2"}, nil) policydb.SetOrUpdateDefaultAppPolicyReturns([]string{"app-id-2"}, nil) verifyScheduleIsUpdatedInScheduler("app-id-2", testDefaultPolicy) - //fakecfClient.GetServicePlanReturns("a-plan-id", nil) + setupCfClient("a-plan-id") }) It("succeeds with 200, saves the default policy, and updates the default policy", func() { By("returning 200") @@ -641,7 +686,7 @@ var _ = Describe("BrokerHandler", func() { Expect(err).To(BeNil()) policydb.GetAppPolicyReturns(&encodedTestDefaultPolicy, nil) verifyScheduleIsDeletedInScheduler("app-id-2") - //fakecfClient.GetServicePlanReturns("a-plan-id", nil) + setupCfClient("a-plan-id") }) It("succeeds with 200 and removes the default policy", func() { By("returning 200") @@ -697,7 +742,7 @@ var _ = Describe("BrokerHandler", func() { ServiceInstanceId: testInstanceId, }, nil) bindingdb.GetAppIdsByInstanceIdReturns([]string{"app-id-2", "app-id-1"}, nil) - //fakecfClient.GetServicePlanReturns("a-plan-id", nil) + setupCfClient("a-plan-id") }) It("fails with 400", func() { Expect(resp.Code).To(Equal(http.StatusBadRequest)) @@ -717,7 +762,7 @@ var _ = Describe("BrokerHandler", func() { } body, err = json.Marshal(instanceUpdateRequestBody) Expect(err).NotTo(HaveOccurred()) - //fakecfClient.GetServicePlanReturns("a-plan-id", nil) + setupCfClient("a-plan-id") bindingdb.GetServiceInstanceReturns(&models.ServiceInstance{ ServiceInstanceId: testInstanceId, }, nil) @@ -737,7 +782,7 @@ var _ = Describe("BrokerHandler", func() { } body, err = json.Marshal(instanceUpdateRequestBody) Expect(err).NotTo(HaveOccurred()) - //fakecfClient.GetServicePlanReturns("a-plan-id", nil) + setupCfClient("a-plan-id") bindingdb.GetServiceInstanceReturns(&models.ServiceInstance{ ServiceInstanceId: testInstanceId, DefaultPolicy: testDefaultPolicy, @@ -764,7 +809,7 @@ var _ = Describe("BrokerHandler", func() { } body, err = json.Marshal(instanceUpdateRequestBody) Expect(err).NotTo(HaveOccurred()) - //fakecfClient.GetServicePlanReturns("a-plan-id-not-updatable", nil) + setupCfClient("a-plan-id-not-updatable") bindingdb.GetServiceInstanceReturns(&models.ServiceInstance{ ServiceInstanceId: testInstanceId, }, nil) @@ -795,7 +840,7 @@ var _ = Describe("BrokerHandler", func() { }, nil) policydb.SetOrUpdateDefaultAppPolicyReturns([]string{"app-id-2"}, nil) verifyScheduleIsUpdatedInScheduler("app-id-2", testDefaultPolicy) - //fakecfClient.GetServicePlanReturns("a-plan-id", nil) + setupCfClient("a-plan-id") bindingdb.GetAppIdsByInstanceIdReturns([]string{"app-id-1", "app-id-2"}, nil) }) It("fails with 400", func() { diff --git a/src/autoscaler/cf/client.go b/src/autoscaler/cf/client.go index 2bb6865554..19f0909329 100644 --- a/src/autoscaler/cf/client.go +++ b/src/autoscaler/cf/client.go @@ -60,7 +60,7 @@ type ( IsUserSpaceDeveloper(userToken string, appId string) (bool, error) IsTokenAuthorized(token, clientId string) (bool, error) GetServiceInstance(serviceInstanceGuid string) (*ServiceInstance, error) - GetServicePlanResource(servicePlanGuid string) (*ServicePlan, error) + GetServicePlan(servicePlanGuid string) (*ServicePlan, error) } Client struct { diff --git a/src/autoscaler/cf/service_plans.go b/src/autoscaler/cf/service_plans.go index 55d15876f8..436d72761f 100644 --- a/src/autoscaler/cf/service_plans.go +++ b/src/autoscaler/cf/service_plans.go @@ -29,7 +29,7 @@ func (c *Client) GetBrokerPlanGuid(ccServicePlanGuid string) (string, error) { } func (c *Client) getBrokerPlanGuid(ccServicePlanGuid string) (string, error) { - result, err := c.GetServicePlanResource(ccServicePlanGuid) + result, err := c.GetServicePlan(ccServicePlanGuid) if err != nil { return "", err } @@ -42,11 +42,11 @@ func (c *Client) getBrokerPlanGuid(ccServicePlanGuid string) (string, error) { /*GetServicePlanResource * v3 api docs https://v3-apidocs.cloudfoundry.org/version/3.123.0/index.html#service-plans */ -func (c *Client) GetServicePlanResource(servicePlanGuid string) (*ServicePlan, error) { +func (c *Client) GetServicePlan(servicePlanGuid string) (*ServicePlan, error) { theUrl := fmt.Sprintf("/v3/service_plans/%s", servicePlanGuid) plan, err := ResourceRetriever[*ServicePlan]{c}.Get(theUrl) if err != nil { - return plan, fmt.Errorf("failed GetServicePlanResource(%s): %w", servicePlanGuid, err) + return plan, fmt.Errorf("failed GetServicePlan(%s): %w", servicePlanGuid, err) } return plan, nil } diff --git a/src/autoscaler/cf/service_plans_test.go b/src/autoscaler/cf/service_plans_test.go index 8ab1b3a7b3..b1da5c2d99 100644 --- a/src/autoscaler/cf/service_plans_test.go +++ b/src/autoscaler/cf/service_plans_test.go @@ -57,7 +57,7 @@ var _ = Describe("Cf client Service Plans", func() { } }) - Describe("GetServicePlanResource", func() { + Describe("GetServicePlan", func() { When("get service plans succeeds", func() { BeforeEach(func() { @@ -70,7 +70,7 @@ var _ = Describe("Cf client Service Plans", func() { }) It("returns correct struct", func() { - servicePlan, err := cfc.GetServicePlanResource("test_guid") + servicePlan, err := cfc.GetServicePlan("test_guid") Expect(err).NotTo(HaveOccurred()) Expect(servicePlan).To(Equal(&cf.ServicePlan{Guid: "d67b2fe4-665c-4bf2-9ccc-e080c49d48d4", BrokerCatalog: cf.BrokerCatalog{ @@ -90,9 +90,9 @@ var _ = Describe("Cf client Service Plans", func() { }) It("should return correct error", func() { - _, err := cfc.GetServicePlanResource("test_guid") + _, err := cfc.GetServicePlan("test_guid") Expect(err).To(HaveOccurred()) - Expect(err).To(MatchError(MatchRegexp(`failed GetServicePlanResource\(test_guid\):.*cf.ServicePlan.*GET.*'UnknownError'.*`))) + Expect(err).To(MatchError(MatchRegexp(`failed GetServicePlan\(test_guid\):.*cf.ServicePlan.*GET.*'UnknownError'.*`))) }) }) diff --git a/src/autoscaler/fakes/fake_cf_client.go b/src/autoscaler/fakes/fake_cf_client.go index 77cfd23a1a..c914689fc7 100644 --- a/src/autoscaler/fakes/fake_cf_client.go +++ b/src/autoscaler/fakes/fake_cf_client.go @@ -70,16 +70,16 @@ type FakeCFClient struct { result1 *cf.ServiceInstance result2 error } - GetServicePlanResourceStub func(string) (*cf.ServicePlan, error) - getServicePlanResourceMutex sync.RWMutex - getServicePlanResourceArgsForCall []struct { + GetServicePlanStub func(string) (*cf.ServicePlan, error) + getServicePlanMutex sync.RWMutex + getServicePlanArgsForCall []struct { arg1 string } - getServicePlanResourceReturns struct { + getServicePlanReturns struct { result1 *cf.ServicePlan result2 error } - getServicePlanResourceReturnsOnCall map[int]struct { + getServicePlanReturnsOnCall map[int]struct { result1 *cf.ServicePlan result2 error } @@ -483,16 +483,16 @@ func (fake *FakeCFClient) GetServiceInstanceReturnsOnCall(i int, result1 *cf.Ser }{result1, result2} } -func (fake *FakeCFClient) GetServicePlanResource(arg1 string) (*cf.ServicePlan, error) { - fake.getServicePlanResourceMutex.Lock() - ret, specificReturn := fake.getServicePlanResourceReturnsOnCall[len(fake.getServicePlanResourceArgsForCall)] - fake.getServicePlanResourceArgsForCall = append(fake.getServicePlanResourceArgsForCall, struct { +func (fake *FakeCFClient) GetServicePlan(arg1 string) (*cf.ServicePlan, error) { + fake.getServicePlanMutex.Lock() + ret, specificReturn := fake.getServicePlanReturnsOnCall[len(fake.getServicePlanArgsForCall)] + fake.getServicePlanArgsForCall = append(fake.getServicePlanArgsForCall, struct { arg1 string }{arg1}) - stub := fake.GetServicePlanResourceStub - fakeReturns := fake.getServicePlanResourceReturns - fake.recordInvocation("GetServicePlanResource", []interface{}{arg1}) - fake.getServicePlanResourceMutex.Unlock() + stub := fake.GetServicePlanStub + fakeReturns := fake.getServicePlanReturns + fake.recordInvocation("GetServicePlan", []interface{}{arg1}) + fake.getServicePlanMutex.Unlock() if stub != nil { return stub(arg1) } @@ -502,46 +502,46 @@ func (fake *FakeCFClient) GetServicePlanResource(arg1 string) (*cf.ServicePlan, return fakeReturns.result1, fakeReturns.result2 } -func (fake *FakeCFClient) GetServicePlanResourceCallCount() int { - fake.getServicePlanResourceMutex.RLock() - defer fake.getServicePlanResourceMutex.RUnlock() - return len(fake.getServicePlanResourceArgsForCall) +func (fake *FakeCFClient) GetServicePlanCallCount() int { + fake.getServicePlanMutex.RLock() + defer fake.getServicePlanMutex.RUnlock() + return len(fake.getServicePlanArgsForCall) } -func (fake *FakeCFClient) GetServicePlanResourceCalls(stub func(string) (*cf.ServicePlan, error)) { - fake.getServicePlanResourceMutex.Lock() - defer fake.getServicePlanResourceMutex.Unlock() - fake.GetServicePlanResourceStub = stub +func (fake *FakeCFClient) GetServicePlanCalls(stub func(string) (*cf.ServicePlan, error)) { + fake.getServicePlanMutex.Lock() + defer fake.getServicePlanMutex.Unlock() + fake.GetServicePlanStub = stub } -func (fake *FakeCFClient) GetServicePlanResourceArgsForCall(i int) string { - fake.getServicePlanResourceMutex.RLock() - defer fake.getServicePlanResourceMutex.RUnlock() - argsForCall := fake.getServicePlanResourceArgsForCall[i] +func (fake *FakeCFClient) GetServicePlanArgsForCall(i int) string { + fake.getServicePlanMutex.RLock() + defer fake.getServicePlanMutex.RUnlock() + argsForCall := fake.getServicePlanArgsForCall[i] return argsForCall.arg1 } -func (fake *FakeCFClient) GetServicePlanResourceReturns(result1 *cf.ServicePlan, result2 error) { - fake.getServicePlanResourceMutex.Lock() - defer fake.getServicePlanResourceMutex.Unlock() - fake.GetServicePlanResourceStub = nil - fake.getServicePlanResourceReturns = struct { +func (fake *FakeCFClient) GetServicePlanReturns(result1 *cf.ServicePlan, result2 error) { + fake.getServicePlanMutex.Lock() + defer fake.getServicePlanMutex.Unlock() + fake.GetServicePlanStub = nil + fake.getServicePlanReturns = struct { result1 *cf.ServicePlan result2 error }{result1, result2} } -func (fake *FakeCFClient) GetServicePlanResourceReturnsOnCall(i int, result1 *cf.ServicePlan, result2 error) { - fake.getServicePlanResourceMutex.Lock() - defer fake.getServicePlanResourceMutex.Unlock() - fake.GetServicePlanResourceStub = nil - if fake.getServicePlanResourceReturnsOnCall == nil { - fake.getServicePlanResourceReturnsOnCall = make(map[int]struct { +func (fake *FakeCFClient) GetServicePlanReturnsOnCall(i int, result1 *cf.ServicePlan, result2 error) { + fake.getServicePlanMutex.Lock() + defer fake.getServicePlanMutex.Unlock() + fake.GetServicePlanStub = nil + if fake.getServicePlanReturnsOnCall == nil { + fake.getServicePlanReturnsOnCall = make(map[int]struct { result1 *cf.ServicePlan result2 error }) } - fake.getServicePlanResourceReturnsOnCall[i] = struct { + fake.getServicePlanReturnsOnCall[i] = struct { result1 *cf.ServicePlan result2 error }{result1, result2} @@ -981,8 +981,8 @@ func (fake *FakeCFClient) Invocations() map[string][][]interface{} { defer fake.getEndpointsMutex.RUnlock() fake.getServiceInstanceMutex.RLock() defer fake.getServiceInstanceMutex.RUnlock() - fake.getServicePlanResourceMutex.RLock() - defer fake.getServicePlanResourceMutex.RUnlock() + fake.getServicePlanMutex.RLock() + defer fake.getServicePlanMutex.RUnlock() fake.getTokensMutex.RLock() defer fake.getTokensMutex.RUnlock() fake.isTokenAuthorizedMutex.RLock() diff --git a/src/autoscaler/testhelpers/mocks_test.go b/src/autoscaler/testhelpers/mocks_test.go index 68899ec9cb..e8e85d65e4 100644 --- a/src/autoscaler/testhelpers/mocks_test.go +++ b/src/autoscaler/testhelpers/mocks_test.go @@ -211,7 +211,7 @@ var _ = Describe("Cf cloud controller", func() { DeferCleanup(mocks.Close) }) It("will return success", func() { - roles, err := cfc.GetServicePlanResource("a-broker-plan-id") + roles, err := cfc.GetServicePlan("a-broker-plan-id") Expect(err).NotTo(HaveOccurred()) Expect(roles).To(Equal(&cf.ServicePlan{BrokerCatalog: cf.BrokerCatalog{Id: "a-broker-plan-id"}})) }) From 882ae66325fa024a98960adeb497636763072d90 Mon Sep 17 00:00:00 2001 From: Kevin Cross Date: Thu, 18 Aug 2022 13:47:48 +0100 Subject: [PATCH 3/6] Fixing plancheck tests --- .../api/brokerserver/broker_handler.go | 5 ++++- .../api/brokerserver/broker_handler_test.go | 15 +++++++++++-- .../api/plancheck/plan_checker_test.go | 22 +++++++++---------- 3 files changed, 28 insertions(+), 14 deletions(-) diff --git a/src/autoscaler/api/brokerserver/broker_handler.go b/src/autoscaler/api/brokerserver/broker_handler.go index 969bac1c05..7e94f69ba0 100644 --- a/src/autoscaler/api/brokerserver/broker_handler.go +++ b/src/autoscaler/api/brokerserver/broker_handler.go @@ -40,6 +40,7 @@ type BrokerHandler struct { PlanChecker plancheck.PlanChecker cfClient cf.CFClient credentials cred_helper.Credentials + brokerCatalogPlanId *cf.Memoizer[string, string] } var emptyJSONObject = regexp.MustCompile(`^\s*{\s*}\s*$`) @@ -50,7 +51,7 @@ var errorDeleteServiceBinding = errors.New("Error deleting service binding") var errorCredentialNotDeleted = errors.New("Failed to delete custom metrics credential for unbinding") func NewBrokerHandler(logger lager.Logger, conf *config.Config, bindingdb db.BindingDB, policydb db.PolicyDB, catalog []domain.Service, cfClient cf.CFClient, credentials cred_helper.Credentials) *BrokerHandler { - return &BrokerHandler{ + handler := &BrokerHandler{ logger: logger, conf: conf, bindingdb: bindingdb, @@ -63,6 +64,8 @@ func NewBrokerHandler(logger lager.Logger, conf *config.Config, bindingdb db.Bin cfClient: cfClient, credentials: credentials, } + handler.brokerCatalogPlanId = cf.NewMemoizer(handler.getBrokerCatalogPlanId) + return handler } func writeErrorResponse(w http.ResponseWriter, statusCode int, message string) { diff --git a/src/autoscaler/api/brokerserver/broker_handler_test.go b/src/autoscaler/api/brokerserver/broker_handler_test.go index f65b037497..163c7b5d5b 100644 --- a/src/autoscaler/api/brokerserver/broker_handler_test.go +++ b/src/autoscaler/api/brokerserver/broker_handler_test.go @@ -400,10 +400,11 @@ var _ = Describe("BrokerHandler", func() { }, nil) fakecfClient.GetServicePlanReturns(&cf.ServicePlan{BrokerCatalog: cf.BrokerCatalog{Id: brokerCatalogId}}, nil) } - JustBeforeEach(func() { + callUpdateServiceInstance := func() { req, err = http.NewRequest(http.MethodPut, "", bytes.NewReader(body)) handler.UpdateServiceInstance(resp, req, map[string]string{"instanceId": testInstanceId}) - }) + } + JustBeforeEach(callUpdateServiceInstance) BeforeEach(func() { instanceUpdateRequestBody = &models.InstanceUpdateRequestBody{ BrokerCommonRequestBody: models.BrokerCommonRequestBody{ @@ -611,6 +612,16 @@ var _ = Describe("BrokerHandler", func() { }) }) }) + Context("Broker Catalog PlanId is cached", func() { + JustBeforeEach(callUpdateServiceInstance) + BeforeEach(func() { + setupCfClient("a-plan-id") + }) + It("it call getBrokerCatalogPlanId only once", func() { + Expect(fakecfClient.GetServiceInstanceCallCount()).To(Equal(1)) + Expect(fakecfClient.GetServicePlanCallCount()).To(Equal(1)) + }) + }) }) Context("When a default policy is present and there was previously a default policy", func() { BeforeEach(func() { diff --git a/src/autoscaler/api/plancheck/plan_checker_test.go b/src/autoscaler/api/plancheck/plan_checker_test.go index 7f9b6156f2..fe1e516b78 100644 --- a/src/autoscaler/api/plancheck/plan_checker_test.go +++ b/src/autoscaler/api/plancheck/plan_checker_test.go @@ -15,7 +15,7 @@ var _ = Describe("Plan check operations", func() { var ( quotaConfig *config.PlanCheckConfig validationResult string - qmc *plancheck.planChecker + planChecker plancheck.PlanChecker ok bool err error testPolicy models.ScalingPolicy @@ -24,12 +24,12 @@ var _ = Describe("Plan check operations", func() { BeforeEach(func() {}) JustBeforeEach(func() { - qmc = plancheck.NewPlanChecker(quotaConfig, lagertest.NewTestLogger("Quota")) + planChecker = plancheck.NewPlanChecker(quotaConfig, lagertest.NewTestLogger("Quota")) }) Context("when not configured", func() { JustBeforeEach(func() { - ok, validationResult, err = qmc.CheckPlan(testPolicy, testPlanId) + ok, validationResult, err = planChecker.CheckPlan(testPolicy, testPlanId) }) BeforeEach(func() { testPolicy = models.ScalingPolicy{ @@ -52,7 +52,7 @@ var _ = Describe("Plan check operations", func() { Context("IsUpdatable", func() { It("it should return true", func() { - isPlanUpdatable, err := qmc.IsPlanUpdatable("any-plan") + isPlanUpdatable, err := planChecker.IsPlanUpdatable("any-plan") Expect(isPlanUpdatable).To(BeTrue()) Expect(err).NotTo(HaveOccurred()) }) @@ -61,7 +61,7 @@ var _ = Describe("Plan check operations", func() { Context("when configured", func() { Context("CheckPlan", func() { JustBeforeEach(func() { - ok, validationResult, err = qmc.CheckPlan(testPolicy, testPlanId) + ok, validationResult, err = planChecker.CheckPlan(testPolicy, testPlanId) }) BeforeEach(func() { testPolicy = models.ScalingPolicy{ @@ -201,17 +201,17 @@ var _ = Describe("Plan check operations", func() { } }) It("is plan updatable", func() { - isPlanUpdatable, err := qmc.IsPlanUpdatable("updatable-plan") + isPlanUpdatable, err := planChecker.IsPlanUpdatable("updatable-plan") Expect(isPlanUpdatable).To(Equal(true)) Expect(err).To(BeNil()) }) It("is plan not updatable", func() { - isPlanUpdatable, err := qmc.IsPlanUpdatable("non-updatable-plan") + isPlanUpdatable, err := planChecker.IsPlanUpdatable("non-updatable-plan") Expect(isPlanUpdatable).To(Equal(false)) Expect(err).To(BeNil()) }) It("if plan does not exist", func() { - isPlanUpdatable, err := qmc.IsPlanUpdatable("non-existent-plan") + isPlanUpdatable, err := planChecker.IsPlanUpdatable("non-existent-plan") Expect(isPlanUpdatable).To(Equal(false)) Expect(err.Error()).To(Equal("unknown plan id \"non-existent-plan\"")) }) @@ -238,17 +238,17 @@ var _ = Describe("Plan check operations", func() { } }) It("is plan updatable", func() { - isPlanUpdatable, err := qmc.IsPlanUpdatable("updatable-plan") + isPlanUpdatable, err := planChecker.IsPlanUpdatable("updatable-plan") Expect(isPlanUpdatable).To(Equal(true)) Expect(err).To(BeNil()) }) It("is plan not updatable", func() { - isPlanUpdatable, err := qmc.IsPlanUpdatable("non-updatable-plan") + isPlanUpdatable, err := planChecker.IsPlanUpdatable("non-updatable-plan") Expect(isPlanUpdatable).To(Equal(false)) Expect(err).To(BeNil()) }) It("if plan does not exist", func() { - isPlanUpdatable, err := qmc.IsPlanUpdatable("non-existent-plan") + isPlanUpdatable, err := planChecker.IsPlanUpdatable("non-existent-plan") Expect(isPlanUpdatable).To(Equal(false)) Expect(err.Error()).To(Equal("unknown plan id \"non-existent-plan\"")) }) From ede139089ea90041486eab6e162cd7cece626570 Mon Sep 17 00:00:00 2001 From: Arsalan Khan Date: Thu, 18 Aug 2022 16:55:53 +0200 Subject: [PATCH 4/6] refactor cache(memoizer) and include tests around it --- .../api/brokerserver/broker_handler.go | 29 ++++++++++--------- .../api/brokerserver/broker_handler_test.go | 6 ++-- src/autoscaler/cf/client.go | 3 -- src/autoscaler/cf/service_plans.go | 17 ----------- .../{cf => helpers/memoizer}/memoizer.go | 4 +-- .../{cf => helpers/memoizer}/memoizer_test.go | 12 ++++---- 6 files changed, 27 insertions(+), 44 deletions(-) rename src/autoscaler/{cf => helpers/memoizer}/memoizer.go (90%) rename src/autoscaler/{cf => helpers/memoizer}/memoizer_test.go (84%) diff --git a/src/autoscaler/api/brokerserver/broker_handler.go b/src/autoscaler/api/brokerserver/broker_handler.go index 7e94f69ba0..36a3fd957a 100644 --- a/src/autoscaler/api/brokerserver/broker_handler.go +++ b/src/autoscaler/api/brokerserver/broker_handler.go @@ -1,6 +1,7 @@ package brokerserver import ( + "code.cloudfoundry.org/app-autoscaler/src/autoscaler/helpers/memoizer" "database/sql" "encoding/json" "fmt" @@ -29,18 +30,18 @@ import ( ) type BrokerHandler struct { - logger lager.Logger - conf *config.Config - bindingdb db.BindingDB - policydb db.PolicyDB - policyValidator *policyvalidator.PolicyValidator - schedulerUtil *schedulerutil.SchedulerUtil - quotaManagementClient *quota.Client - catalog []domain.Service - PlanChecker plancheck.PlanChecker - cfClient cf.CFClient - credentials cred_helper.Credentials - brokerCatalogPlanId *cf.Memoizer[string, string] + logger lager.Logger + conf *config.Config + bindingdb db.BindingDB + policydb db.PolicyDB + policyValidator *policyvalidator.PolicyValidator + schedulerUtil *schedulerutil.SchedulerUtil + quotaManagementClient *quota.Client + catalog []domain.Service + PlanChecker plancheck.PlanChecker + cfClient cf.CFClient + credentials cred_helper.Credentials + servicePlanBrokerCatalogId *memoizer.Memoizer[string, string] } var emptyJSONObject = regexp.MustCompile(`^\s*{\s*}\s*$`) @@ -64,7 +65,7 @@ func NewBrokerHandler(logger lager.Logger, conf *config.Config, bindingdb db.Bin cfClient: cfClient, credentials: credentials, } - handler.brokerCatalogPlanId = cf.NewMemoizer(handler.getBrokerCatalogPlanId) + handler.servicePlanBrokerCatalogId = memoizer.New(handler.getServicePlanBrokerCatalogId) return handler } @@ -456,7 +457,7 @@ func (h *BrokerHandler) UpdateServiceInstance(w http.ResponseWriter, r *http.Req } func (h *BrokerHandler) getBrokerCatalogPlanId(instanceId string) (string, error) { - return h.getServicePlanBrokerCatalogId(instanceId) + return h.servicePlanBrokerCatalogId.Func(instanceId) } func (h *BrokerHandler) DeleteServiceInstance(w http.ResponseWriter, _ *http.Request, vars map[string]string) { diff --git a/src/autoscaler/api/brokerserver/broker_handler_test.go b/src/autoscaler/api/brokerserver/broker_handler_test.go index 163c7b5d5b..3231d6a978 100644 --- a/src/autoscaler/api/brokerserver/broker_handler_test.go +++ b/src/autoscaler/api/brokerserver/broker_handler_test.go @@ -613,13 +613,15 @@ var _ = Describe("BrokerHandler", func() { }) }) Context("Broker Catalog PlanId is cached", func() { + + JustBeforeEach(func() { verifyScheduleIsUpdatedInScheduler("app-id-2", testDefaultPolicy) }) JustBeforeEach(callUpdateServiceInstance) BeforeEach(func() { setupCfClient("a-plan-id") }) It("it call getBrokerCatalogPlanId only once", func() { - Expect(fakecfClient.GetServiceInstanceCallCount()).To(Equal(1)) - Expect(fakecfClient.GetServicePlanCallCount()).To(Equal(1)) + Expect(fakecfClient.GetServiceInstanceCallCount()).To(Equal(1), "GetServiceInstanceCallCount") + Expect(fakecfClient.GetServicePlanCallCount()).To(Equal(1), "GetServicePlanCallCount") }) }) }) diff --git a/src/autoscaler/cf/client.go b/src/autoscaler/cf/client.go index 19f0909329..0310d843c0 100644 --- a/src/autoscaler/cf/client.go +++ b/src/autoscaler/cf/client.go @@ -78,7 +78,6 @@ type ( lock *sync.Mutex grantTime time.Time retryClient *http.Client - brokerPlanGuid *Memoizer[string, string] } ) @@ -106,8 +105,6 @@ func NewCFClient(conf *Config, logger lager.Logger, clk clock.Clock) *Client { c.retryClient = createRetryClient(conf, c.httpClient, logger) c.lock = &sync.Mutex{} - c.brokerPlanGuid = NewMemoizer(c.getBrokerPlanGuid) - if c.conf.PerPage == 0 { c.conf.PerPage = defaultPerPage } diff --git a/src/autoscaler/cf/service_plans.go b/src/autoscaler/cf/service_plans.go index 436d72761f..8c36e08a30 100644 --- a/src/autoscaler/cf/service_plans.go +++ b/src/autoscaler/cf/service_plans.go @@ -2,8 +2,6 @@ package cf import ( "fmt" - - "code.cloudfoundry.org/lager" ) type ServicePlanEntity struct { @@ -24,21 +22,6 @@ type ( } ) -func (c *Client) GetBrokerPlanGuid(ccServicePlanGuid string) (string, error) { - return c.brokerPlanGuid.Func(ccServicePlanGuid) -} - -func (c *Client) getBrokerPlanGuid(ccServicePlanGuid string) (string, error) { - result, err := c.GetServicePlan(ccServicePlanGuid) - if err != nil { - return "", err - } - - brokerPlanGuid := result.BrokerCatalog.Id - c.logger.Info("found-guid", lager.Data{"brokerPlanGuid": brokerPlanGuid}) - return brokerPlanGuid, nil -} - /*GetServicePlanResource * v3 api docs https://v3-apidocs.cloudfoundry.org/version/3.123.0/index.html#service-plans */ diff --git a/src/autoscaler/cf/memoizer.go b/src/autoscaler/helpers/memoizer/memoizer.go similarity index 90% rename from src/autoscaler/cf/memoizer.go rename to src/autoscaler/helpers/memoizer/memoizer.go index f6c1ebb3d1..2fe4aeabc7 100644 --- a/src/autoscaler/cf/memoizer.go +++ b/src/autoscaler/helpers/memoizer/memoizer.go @@ -1,8 +1,8 @@ -package cf +package memoizer import "sync" -func NewMemoizer[T comparable, R any](funcToMemoize func(T) (R, error)) *Memoizer[T, R] { +func New[T comparable, R any](funcToMemoize func(T) (R, error)) *Memoizer[T, R] { return &Memoizer[T, R]{fn: funcToMemoize, cache: make(map[T]R)} } diff --git a/src/autoscaler/cf/memoizer_test.go b/src/autoscaler/helpers/memoizer/memoizer_test.go similarity index 84% rename from src/autoscaler/cf/memoizer_test.go rename to src/autoscaler/helpers/memoizer/memoizer_test.go index 557370b949..0b57d1e3be 100644 --- a/src/autoscaler/cf/memoizer_test.go +++ b/src/autoscaler/helpers/memoizer/memoizer_test.go @@ -1,12 +1,12 @@ -package cf_test +package memoizer_test import ( + "code.cloudfoundry.org/app-autoscaler/src/autoscaler/helpers/memoizer" "errors" "sync" "testing" "time" - "code.cloudfoundry.org/app-autoscaler/src/autoscaler/cf" "github.com/stretchr/testify/assert" ) @@ -23,7 +23,7 @@ func testFunc(counter *int) func(string) (string, error) { func TestMemoiser_miss(t *testing.T) { counter := 0 - memo := cf.NewMemoizer(testFunc(&counter)) + memo := memoizer.New(testFunc(&counter)) res, err := memo.Func("string") assert.Equal(t, "str", res) assert.Nil(t, err) @@ -32,7 +32,7 @@ func TestMemoiser_miss(t *testing.T) { func TestMemoiser_hit(t *testing.T) { counter := 0 - memo := cf.NewMemoizer(testFunc(&counter)) + memo := memoizer.New(testFunc(&counter)) res, err := memo.Func("string") assert.Equal(t, "str", res) assert.Nil(t, err) @@ -44,7 +44,7 @@ func TestMemoiser_hit(t *testing.T) { func TestMemoiser_errorsDontGetCached(t *testing.T) { counter := 0 - memo := cf.NewMemoizer(testFunc(&counter)) + memo := memoizer.New(testFunc(&counter)) _, err := memo.Func("st") assert.NotNil(t, err) _, err = memo.Func("st") @@ -59,7 +59,7 @@ func TestMemoiser_errorsDontGetCached(t *testing.T) { func TestMemoiser_Thread_test(t *testing.T) { counter := 0 numThreads := 100 - memo := cf.NewMemoizer(testFunc(&counter)) + memo := memoizer.New(testFunc(&counter)) mu := sync.RWMutex{} mu.Lock() wg := sync.WaitGroup{} From 3a41263f0660f644611992b8234b5b23c8f02285 Mon Sep 17 00:00:00 2001 From: Arsalan Khan Date: Thu, 18 Aug 2022 17:26:44 +0200 Subject: [PATCH 5/6] WIP:refactor newServicePlan/existingServicePlan --- src/autoscaler/api/brokerserver/broker_handler.go | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/autoscaler/api/brokerserver/broker_handler.go b/src/autoscaler/api/brokerserver/broker_handler.go index 36a3fd957a..662e03c4d2 100644 --- a/src/autoscaler/api/brokerserver/broker_handler.go +++ b/src/autoscaler/api/brokerserver/broker_handler.go @@ -286,15 +286,17 @@ func (h *BrokerHandler) UpdateServiceInstance(w http.ResponseWriter, r *http.Req return } } - existingServicePlan, err := h.getBrokerCatalogPlanId(instanceId) if err != nil { h.logger.Error("Error retrieving broker plan Id", err, lager.Data{"instanceId": instanceId}) writeErrorResponse(w, http.StatusInternalServerError, "Error retrieving broker plan Id") return } + //TODO check some edge cases around service Plan + servicePlan := existingServicePlan newServicePlan := body.PlanID if newServicePlan != "" { + servicePlan = newServicePlan if !(existingServicePlan == newServicePlan) { isPlanUpdatable, err := h.PlanChecker.IsPlanUpdatable(existingServicePlan) if err != nil { @@ -354,14 +356,6 @@ func (h *BrokerHandler) UpdateServiceInstance(w http.ResponseWriter, r *http.Req } updatedDefaultPolicyGuid = policyGuid.String() } - - var servicePlan string - if newServicePlan != "" { - servicePlan = newServicePlan - } else { - servicePlan = existingServicePlan - } - allBoundApps, err := h.bindingdb.GetAppIdsByInstanceId(serviceInstance.ServiceInstanceId) if err != nil { h.logger.Error("failed to retrieve bound apps", err, lager.Data{"instanceId": instanceId}) From 3247c7744c0be77e020661526e1cc9bb0ea66341 Mon Sep 17 00:00:00 2001 From: Kevin Cross Date: Fri, 19 Aug 2022 15:44:22 +0100 Subject: [PATCH 6/6] finished testing and refactoring out a method. --- .github/workflows/bosh-release-checks.yaml | 6 +- Makefile | 5 ++ packages/golangapiserver/spec | 3 + .../api/brokerserver/broker_handler.go | 79 +++++++++++++------ .../api/brokerserver/broker_handler_test.go | 29 ++++++- src/autoscaler/api/brokerserver/error.go | 40 ++++++++++ src/autoscaler/api/brokerserver/error_test.go | 36 +++++++++ src/autoscaler/go.mod | 1 + src/autoscaler/go.sum | 2 + .../helpers/memoizer/memoizer_test.go | 3 +- 10 files changed, 174 insertions(+), 30 deletions(-) create mode 100644 src/autoscaler/api/brokerserver/error.go create mode 100644 src/autoscaler/api/brokerserver/error_test.go diff --git a/.github/workflows/bosh-release-checks.yaml b/.github/workflows/bosh-release-checks.yaml index 303b155a3d..4c6f58ef9c 100644 --- a/.github/workflows/bosh-release-checks.yaml +++ b/.github/workflows/bosh-release-checks.yaml @@ -14,9 +14,7 @@ jobs: - uses: ./.github/actions/setup_go - name: sync-package-specs - run: | - source .envrc - ./scripts/sync-package-specs + run: make package-specs - name: Check if there is any change run: | @@ -24,7 +22,7 @@ jobs: [ $(git status --porcelain | wc -l) -eq 0 ] || \ { \ git status;\ - echo "::error::Specs are out of date, run ./scripts/update && ./scripts/sync-package-specs to update";\ + echo "::error::Specs are out of date, run \"make package-specs\" to update";\ exit 1;\ } diff --git a/Makefile b/Makefile index 13a64bfa80..a1ab8f7748 100644 --- a/Makefile +++ b/Makefile @@ -294,3 +294,8 @@ acceptance-tests: clean-deploy: @echo " - Cleaning up deployment '${DEPLOYMENT_NAME}' name prefix:'${NAME_PREFIX}'" @ci/autoscaler/scripts/cleanup-autoscaler.sh + +.PHONY: package-specs +package-specs: update + @echo " - Updating the package specs" + @./scripts/sync-package-specs \ No newline at end of file diff --git a/packages/golangapiserver/spec b/packages/golangapiserver/spec index 86d80227d2..c330a62d3a 100644 --- a/packages/golangapiserver/spec +++ b/packages/golangapiserver/spec @@ -25,6 +25,7 @@ files: - autoscaler/db/sqldb/* # gosub - autoscaler/healthendpoint/* # gosub - autoscaler/helpers/* # gosub +- autoscaler/helpers/memoizer/* # gosub - autoscaler/metricsforwarder/server/common/* # gosub - autoscaler/models/* # gosub - autoscaler/ratelimiter/* # gosub @@ -82,6 +83,8 @@ files: - autoscaler/vendor/golang.org/x/sys/internal/unsafeheader/* # gosub - autoscaler/vendor/golang.org/x/sys/unix/* # gosub - autoscaler/vendor/golang.org/x/time/rate/* # gosub +- autoscaler/vendor/golang.org/x/xerrors/* # gosub +- autoscaler/vendor/golang.org/x/xerrors/internal/* # gosub - autoscaler/vendor/google.golang.org/protobuf/encoding/prototext/* # gosub - autoscaler/vendor/google.golang.org/protobuf/encoding/protowire/* # gosub - autoscaler/vendor/google.golang.org/protobuf/internal/descfmt/* # gosub diff --git a/src/autoscaler/api/brokerserver/broker_handler.go b/src/autoscaler/api/brokerserver/broker_handler.go index 662e03c4d2..4be68c1b32 100644 --- a/src/autoscaler/api/brokerserver/broker_handler.go +++ b/src/autoscaler/api/brokerserver/broker_handler.go @@ -1,7 +1,6 @@ package brokerserver import ( - "code.cloudfoundry.org/app-autoscaler/src/autoscaler/helpers/memoizer" "database/sql" "encoding/json" "fmt" @@ -9,6 +8,8 @@ import ( "net/http" "regexp" + "code.cloudfoundry.org/app-autoscaler/src/autoscaler/helpers/memoizer" + "code.cloudfoundry.org/app-autoscaler/src/autoscaler/api/plancheck" "code.cloudfoundry.org/app-autoscaler/src/autoscaler/api/quota" "code.cloudfoundry.org/app-autoscaler/src/autoscaler/cf" @@ -75,6 +76,16 @@ func writeErrorResponse(w http.ResponseWriter, statusCode int, message string) { Message: message}) } +func (h *BrokerHandler) writeError(w http.ResponseWriter, err error) { + var brokerErr *BrokerError + if errors.As(err, &brokerErr) { + brokerErr.sendResponse(w, h.logger) + } else { + h.logger.Error("Unexpected error", err) + writeErrorResponse(w, http.StatusInternalServerError, "Unknown Error") + } +} + func (h *BrokerHandler) GetBrokerCatalog(w http.ResponseWriter, _ *http.Request, _ map[string]string) { catalog, err := ioutil.ReadFile(h.conf.CatalogPath) if err != nil { @@ -188,6 +199,7 @@ func (h *BrokerHandler) planDefinitionExceeded(policyStr string, planID string, ok, checkResult, err := h.PlanChecker.CheckPlan(policy, planID) if err != nil { h.logger.Error("failed to check policy for plan adherence", err, lager.Data{"instanceId": instanceId, "policyStr": policyStr}) + //TODO this should be a 400 writeErrorResponse(w, http.StatusInternalServerError, "Error generating validating policy") return true } @@ -286,31 +298,14 @@ func (h *BrokerHandler) UpdateServiceInstance(w http.ResponseWriter, r *http.Req return } } - existingServicePlan, err := h.getBrokerCatalogPlanId(instanceId) + + newServicePlan := body.PlanID + servicePlan, err := h.getExistingOrUpdatedServicePlan(instanceId, body.PlanID) if err != nil { - h.logger.Error("Error retrieving broker plan Id", err, lager.Data{"instanceId": instanceId}) - writeErrorResponse(w, http.StatusInternalServerError, "Error retrieving broker plan Id") + h.writeError(w, err) return } - //TODO check some edge cases around service Plan - servicePlan := existingServicePlan - newServicePlan := body.PlanID - if newServicePlan != "" { - servicePlan = newServicePlan - if !(existingServicePlan == newServicePlan) { - isPlanUpdatable, err := h.PlanChecker.IsPlanUpdatable(existingServicePlan) - if err != nil { - h.logger.Error("Plan not found", err) - writeErrorResponse(w, http.StatusBadRequest, "Unable to retrieve the service plan") - return - } - if !isPlanUpdatable { - h.logger.Error("The Plan is not updatable", nil) - writeErrorResponse(w, http.StatusBadRequest, "The plan is not updatable") - return - } - } - } + var updatedDefaultPolicy string var updatedDefaultPolicyGuid string if body.Parameters != nil && body.Parameters.DefaultPolicy != nil { @@ -450,6 +445,44 @@ func (h *BrokerHandler) UpdateServiceInstance(w http.ResponseWriter, r *http.Req } } +func (h *BrokerHandler) getExistingOrUpdatedServicePlan(instanceId string, updateToPlan string) (string, error) { + existingServicePlan, err := h.getBrokerCatalogPlanId(instanceId) + servicePlan := existingServicePlan + + var brokerErr error + if err != nil { + brokerErr = &BrokerError{ + Status: http.StatusInternalServerError, + Message: "Error retrieving broker plan Id", + Err: err, + Data: lager.Data{"instanceId": instanceId}, + } + } else { + if updateToPlan != "" { + servicePlan = updateToPlan + if existingServicePlan != updateToPlan { + isPlanUpdatable, err := h.PlanChecker.IsPlanUpdatable(existingServicePlan) + if err != nil { + brokerErr = &BrokerError{ + Status: http.StatusBadRequest, + Message: "Unable to retrieve the service plan", + Err: err, + Data: lager.Data{"instanceId": instanceId, "existingServicePlan": existingServicePlan, "newServicePlan": updateToPlan}, + } + } else if !isPlanUpdatable { + brokerErr = &BrokerError{ + Status: http.StatusBadRequest, + Message: "The plan is not updatable", + Err: err, + Data: lager.Data{"instanceId": instanceId, "existingServicePlan": existingServicePlan, "newServicePlan": updateToPlan}, + } + } + } + } + } + return servicePlan, brokerErr +} + func (h *BrokerHandler) getBrokerCatalogPlanId(instanceId string) (string, error) { return h.servicePlanBrokerCatalogId.Func(instanceId) } diff --git a/src/autoscaler/api/brokerserver/broker_handler_test.go b/src/autoscaler/api/brokerserver/broker_handler_test.go index 3231d6a978..c79ca5d957 100644 --- a/src/autoscaler/api/brokerserver/broker_handler_test.go +++ b/src/autoscaler/api/brokerserver/broker_handler_test.go @@ -670,6 +670,7 @@ var _ = Describe("BrokerHandler", func() { By("updating the scheduler") Expect(schedulerServer.ReceivedRequests()).To(HaveLen(1)) + }) }) Context("When the default is set to be removed and there was previously a default policy", func() { @@ -790,17 +791,21 @@ var _ = Describe("BrokerHandler", func() { instanceUpdateRequestBody = &models.InstanceUpdateRequestBody{ BrokerCommonRequestBody: models.BrokerCommonRequestBody{ ServiceID: "a-service-id", - PlanID: "a-plan-id-2", + PlanID: "a-plan-id", }, } body, err = json.Marshal(instanceUpdateRequestBody) Expect(err).NotTo(HaveOccurred()) setupCfClient("a-plan-id") + + bindingdb.GetAppIdsByInstanceIdReturns([]string{"app-id-1", "app-id-2"}, nil) + bindingdb.GetServiceInstanceReturns(&models.ServiceInstance{ ServiceInstanceId: testInstanceId, DefaultPolicy: testDefaultPolicy, DefaultPolicyGuid: "default-policy-guid", }, nil) + policydb.GetAppPolicyReturns(&models.ScalingPolicy{}, nil) }) It("Succeeds and leaves the old default policy in place", func() { @@ -809,6 +814,8 @@ var _ = Describe("BrokerHandler", func() { Expect(serviceInstance.ServiceInstanceId).To(Equal(testInstanceId)) Expect(serviceInstance.DefaultPolicy).To(MatchJSON(testDefaultPolicy)) Expect(serviceInstance.DefaultPolicyGuid).To(Equal("default-policy-guid")) + Expect(policydb.GetAppPolicyArgsForCall(0)).To(Equal("app-id-1")) + Expect(policydb.GetAppPolicyArgsForCall(1)).To(Equal("app-id-2")) }) }) @@ -829,7 +836,25 @@ var _ = Describe("BrokerHandler", func() { }) It("fails with 400", func() { Expect(resp.Code).To(Equal(http.StatusBadRequest)) - Expect(resp.Body.String()).To(Equal(`{"code":"Bad Request","message":"The plan is not updatable"}`)) + Expect(resp.Body.String()).To(MatchJSON(`{"code":"Bad Request","message":"The plan is not updatable"}`)) + }) + }) + Context("When the service does not exist", func() { + BeforeEach(func() { + instanceUpdateRequestBody = &models.InstanceUpdateRequestBody{ + BrokerCommonRequestBody: models.BrokerCommonRequestBody{ + ServiceID: "a-service-id", + PlanID: "non-existing-plan", + }, + } + body, err = json.Marshal(instanceUpdateRequestBody) + Expect(err).NotTo(HaveOccurred()) + setupCfClient("non-existing-plan-catalog-id") + bindingdb.GetServiceInstanceReturns(nil, db.ErrDoesNotExist) + }) + It("fails with 404", func() { + Expect(resp.Code).To(Equal(http.StatusNotFound)) + Expect(resp.Body.String()).To(MatchJSON(`{"code": "Not Found","message": "Failed to find service instance to update"}`)) }) }) Context("Update service plan and policy both are updated together", func() { diff --git a/src/autoscaler/api/brokerserver/error.go b/src/autoscaler/api/brokerserver/error.go new file mode 100644 index 0000000000..14c4395690 --- /dev/null +++ b/src/autoscaler/api/brokerserver/error.go @@ -0,0 +1,40 @@ +package brokerserver + +import ( + "fmt" + "net/http" + + "code.cloudfoundry.org/lager" + "golang.org/x/xerrors" +) + +var _ error = &BrokerError{} +var _ xerrors.Wrapper = &BrokerError{} + +type BrokerError struct { + Status int + Message string + Err error + Data lager.Data +} + +func (b BrokerError) sendResponse(w http.ResponseWriter, logger lager.Logger) { + logger.Error(b.Message, b.Err, b.Data) + writeErrorResponse(w, b.Status, b.Message) +} + +func (b BrokerError) Error() string { + wrapped := "" + if b.Err != nil { + wrapped = ": " + b.Err.Error() + } + message := b.Message + if message == "" { + message = "uninitialised" + } + return fmt.Sprintf("%s, statusCode(%d)%s", message, b.Status, wrapped) +} + +func (b BrokerError) Unwrap() error { + return b.Err +} diff --git a/src/autoscaler/api/brokerserver/error_test.go b/src/autoscaler/api/brokerserver/error_test.go new file mode 100644 index 0000000000..6f4f0d58a4 --- /dev/null +++ b/src/autoscaler/api/brokerserver/error_test.go @@ -0,0 +1,36 @@ +package brokerserver_test + +import ( + "errors" + "fmt" + + "code.cloudfoundry.org/app-autoscaler/src/autoscaler/api/brokerserver" + "code.cloudfoundry.org/lager" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("BrokerError", func() { + Context("When a brokereError.Error() is called and has an err wrapped", func() { + It("has the correct error string", func() { + err2 := errors.New("some error message") + var err error = &brokerserver.BrokerError{ + Status: 404, + Message: "Some message", + Err: fmt.Errorf("wrapping err: %w", err2), + Data: lager.Data{"some": "Data"}, + } + Expect(errors.Is(err, err2)).To(BeTrue()) + Expect(err.Error()).To(Equal("Some message, statusCode(404): wrapping err: some error message")) + }) + }) + Context("When a brokereError.Error() is called with nil error", func() { + It("has the correct error string", func() { + err2 := errors.New("some error message") + err := &brokerserver.BrokerError{} + Expect(errors.Is(err, err2)).To(BeFalse()) + Expect(err.Unwrap()).To(BeNil()) + Expect(err.Error()).To(Equal("uninitialised, statusCode(0)")) + }) + }) +}) diff --git a/src/autoscaler/go.mod b/src/autoscaler/go.mod index 1749b5caa4..78bf3b4f7e 100644 --- a/src/autoscaler/go.mod +++ b/src/autoscaler/go.mod @@ -46,6 +46,7 @@ require ( golang.org/x/net v0.0.0-20220722155237-a158d28d115b golang.org/x/oauth2 v0.0.0-20220411215720-9780585627b5 golang.org/x/time v0.0.0-20210611083556-38a9dc6acbc6 + golang.org/x/xerrors v0.0.0-20220517211312-f3a8303e98df google.golang.org/grpc v1.48.0 gopkg.in/yaml.v2 v2.4.0 gopkg.in/yaml.v3 v3.0.1 diff --git a/src/autoscaler/go.sum b/src/autoscaler/go.sum index bb238ff64c..4326ebfa96 100644 --- a/src/autoscaler/go.sum +++ b/src/autoscaler/go.sum @@ -1219,6 +1219,8 @@ golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8T golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= +golang.org/x/xerrors v0.0.0-20220517211312-f3a8303e98df h1:5Pf6pFKu98ODmgnpvkJ3kFUOQGGLIzLIkbzUHp47618= +golang.org/x/xerrors v0.0.0-20220517211312-f3a8303e98df/go.mod h1:K8+ghG5WaK9qNqU5K3HdILfMLy1f3aNYFI/wnl100a8= google.golang.org/api v0.4.0/go.mod h1:8k5glujaEP+g9n7WNsDg8QP6cUVNI86fCNMcbazEtwE= google.golang.org/api v0.7.0/go.mod h1:WtwebWUNSVBH/HAw79HIFXZNqEvBhG+Ra+ax0hx3E3M= google.golang.org/api v0.8.0/go.mod h1:o4eAsZoiT+ibD93RtjEohWalFOjRDx6CVaqeizhEnKg= diff --git a/src/autoscaler/helpers/memoizer/memoizer_test.go b/src/autoscaler/helpers/memoizer/memoizer_test.go index 0b57d1e3be..a67a48dc64 100644 --- a/src/autoscaler/helpers/memoizer/memoizer_test.go +++ b/src/autoscaler/helpers/memoizer/memoizer_test.go @@ -1,12 +1,13 @@ package memoizer_test import ( - "code.cloudfoundry.org/app-autoscaler/src/autoscaler/helpers/memoizer" "errors" "sync" "testing" "time" + "code.cloudfoundry.org/app-autoscaler/src/autoscaler/helpers/memoizer" + "github.com/stretchr/testify/assert" )