Skip to content

Commit

Permalink
Merge branch 'main' into arm64
Browse files Browse the repository at this point in the history
  • Loading branch information
@blgm
blgm authored Oct 4, 2024
2 parents faf8c23 + f8bb85e commit f8f283f
Show file tree
Hide file tree
Showing 33 changed files with 523 additions and 295 deletions.
3 changes: 0 additions & 3 deletions brokerapi/broker/bind_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (

"github.com/cloudfoundry/cloud-service-broker/v2/brokerapi/broker"
"github.com/cloudfoundry/cloud-service-broker/v2/brokerapi/broker/brokerfakes"
"github.com/cloudfoundry/cloud-service-broker/v2/dbservice/models"
"github.com/cloudfoundry/cloud-service-broker/v2/internal/storage"
pkgBroker "github.com/cloudfoundry/cloud-service-broker/v2/pkg/broker"
pkgBrokerFakes "github.com/cloudfoundry/cloud-service-broker/v2/pkg/broker/brokerfakes"
Expand Down Expand Up @@ -61,8 +60,6 @@ var _ = Describe("Bind", func() {
PlanGUID: planID,
SpaceGUID: spaceID,
OrganizationGUID: orgID,
OperationType: models.ProvisionOperationType,
OperationGUID: "provision-operation-GUID",
Outputs: map[string]any{"fakeInstanceOutput": "fakeInstanceValue"},
}, nil)

Expand Down
2 changes: 0 additions & 2 deletions brokerapi/broker/deprovision.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,6 @@ func (broker *ServiceBroker) Deprovision(ctx context.Context, instanceID string,
OperationData: *operationID,
}

instance.OperationType = models.DeprovisionOperationType
instance.OperationGUID = *operationID
if err := broker.store.StoreServiceInstanceDetails(instance); err != nil {
return response, fmt.Errorf("error saving instance details to database: %s. WARNING: this instance will remain visible in cf. Contact your operator for cleanup", err)
}
Expand Down
22 changes: 12 additions & 10 deletions brokerapi/broker/deprovision_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"code.cloudfoundry.org/lager/v3"
"github.com/cloudfoundry/cloud-service-broker/v2/brokerapi/broker"
"github.com/cloudfoundry/cloud-service-broker/v2/brokerapi/broker/brokerfakes"
"github.com/cloudfoundry/cloud-service-broker/v2/dbservice/models"
"github.com/cloudfoundry/cloud-service-broker/v2/internal/storage"
pkgBroker "github.com/cloudfoundry/cloud-service-broker/v2/pkg/broker"
pkgBrokerFakes "github.com/cloudfoundry/cloud-service-broker/v2/pkg/broker/brokerfakes"
Expand Down Expand Up @@ -79,8 +78,10 @@ var _ = Describe("Deprovision", func() {
GUID: instanceToDeleteID,
SpaceGUID: "test-space",
OrganizationGUID: "test-org",
OperationType: "provision",
OperationGUID: operationID,
}, nil)
fakeStorage.GetTerraformDeploymentReturns(storage.TerraformDeployment{
ID: instanceToDeleteID,
LastOperationType: "provision",
}, nil)

serviceBroker = must(broker.New(brokerConfig, fakeStorage, utils.NewLogger("brokers-test")))
Expand Down Expand Up @@ -110,8 +111,6 @@ var _ = Describe("Deprovision", func() {
Expect(fakeStorage.StoreServiceInstanceDetailsCallCount()).To(Equal(1))
actualSIDetails := fakeStorage.StoreServiceInstanceDetailsArgsForCall(0)
Expect(actualSIDetails.GUID).To(Equal(instanceToDeleteID))
Expect(actualSIDetails.OperationType).To(Equal(models.DeprovisionOperationType))
Expect(actualSIDetails.OperationGUID).To(Equal(operationID))
})

When("provider deprovision errors", func() {
Expand Down Expand Up @@ -139,11 +138,14 @@ var _ = Describe("Deprovision", func() {
When("offering does not exists", func() {
BeforeEach(func() {
fakeStorage.GetServiceInstanceDetailsReturns(storage.ServiceInstanceDetails{
ServiceGUID: "non-existent-offering",
PlanGUID: planID,
GUID: instanceToDeleteID,
OperationType: "provision",
OperationGUID: "opGUID",
ServiceGUID: "non-existent-offering",
PlanGUID: planID,
GUID: instanceToDeleteID,
}, nil)

fakeStorage.GetTerraformDeploymentReturns(storage.TerraformDeployment{
LastOperationType: "provision",
ID: instanceToDeleteID,
}, nil)
})

Expand Down
29 changes: 21 additions & 8 deletions brokerapi/broker/last_instance_operation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@ import (
"context"
"fmt"

"code.cloudfoundry.org/lager/v3"
"github.com/pivotal-cf/brokerapi/v11/domain"
"github.com/pivotal-cf/brokerapi/v11/domain/apiresponses"

"code.cloudfoundry.org/lager/v3"
"github.com/cloudfoundry/cloud-service-broker/v2/dbservice/models"
"github.com/cloudfoundry/cloud-service-broker/v2/pkg/broker"
"github.com/cloudfoundry/cloud-service-broker/v2/utils/correlation"
"github.com/pivotal-cf/brokerapi/v11/domain"
)

// LastOperation fetches last operation state for a service instance.
Expand All @@ -24,19 +24,29 @@ func (broker *ServiceBroker) LastOperation(ctx context.Context, instanceID strin
"operation_data": details.OperationData,
})

// make sure that instance actually exists
// Technically according to OSBAPI we should return HTTP 410 Gone only if the operation is a Deprovision,
// and for Provision or Update we should return an HTTP 404 Not Found, but we do not do this, and the
// pivotal-cf/brokerapi/apiresponses package does not provide the required error to do this.
exists, err := broker.store.ExistsServiceInstanceDetails(instanceID)
switch {
case err != nil:
return domain.LastOperation{}, fmt.Errorf("database error checking for existing instance: %s", err)
case !exists:
return domain.LastOperation{}, apiresponses.ErrInstanceDoesNotExist
}

instance, err := broker.store.GetServiceInstanceDetails(instanceID)
if err != nil {
return domain.LastOperation{}, apiresponses.ErrInstanceDoesNotExist
return domain.LastOperation{}, fmt.Errorf("error getting service instance details: %w", err)
}

_, serviceProvider, err := broker.getDefinitionAndProvider(instance.ServiceGUID)
if err != nil {
return domain.LastOperation{}, err
}

lastOperationType := instance.OperationType

done, message, err := serviceProvider.PollInstance(ctx, instance.GUID)
done, message, lastOperationType, err := serviceProvider.PollInstance(ctx, instance.GUID)
if err != nil {
return domain.LastOperation{State: domain.Failed, Description: err.Error()}, nil
}
Expand Down Expand Up @@ -84,11 +94,14 @@ func (broker *ServiceBroker) updateStateOnOperationCompletion(ctx context.Contex
}

details.Outputs = outs
details.OperationGUID = ""
details.OperationType = models.ClearOperationType
if err := broker.store.StoreServiceInstanceDetails(details); err != nil {
return fmt.Errorf("error saving instance details to database %v", err)
}

err = service.ClearOperationType(ctx, instanceID)
if err != nil {
return fmt.Errorf("error clearing operation type from database %v", err)
}

return nil
}
62 changes: 38 additions & 24 deletions brokerapi/broker/last_instance_operation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ var _ = Describe("LastInstanceOperation", func() {
fakeServiceProvider = &pkgBrokerFakes.FakeServiceProvider{}
expectedTFOutput = storage.JSONObject{"output": "value"}
fakeServiceProvider.GetTerraformOutputsReturns(expectedTFOutput, nil)
fakeServiceProvider.PollInstanceReturns(true, "operation complete", nil)
fakeServiceProvider.PollInstanceReturns(true, "operation complete", models.ProvisionOperationType, nil)

providerBuilder := func(logger lager.Logger, store pkgBroker.ServiceProviderStorage) pkgBroker.ServiceProvider {
return fakeServiceProvider
Expand All @@ -66,16 +66,17 @@ var _ = Describe("LastInstanceOperation", func() {
}

fakeStorage = &brokerfakes.FakeStorage{}
fakeStorage.ExistsServiceInstanceDetailsReturns(true, nil)
fakeStorage.GetServiceInstanceDetailsReturns(
storage.ServiceInstanceDetails{
GUID: instanceID,
OperationType: models.ProvisionOperationType,
OperationGUID: operationID,
PlanGUID: planID,
ServiceGUID: offeringID,
SpaceGUID: spaceID,
OrganizationGUID: orgID,
}, nil)
},
nil,
)

serviceBroker = must(broker.New(brokerConfig, fakeStorage, utils.NewLogger("brokers-test")))

Expand All @@ -89,7 +90,7 @@ var _ = Describe("LastInstanceOperation", func() {
Describe("operation complete", func() {
Describe("provision", func() {
BeforeEach(func() {
fakeServiceProvider.PollInstanceReturns(true, "operation complete", nil)
fakeServiceProvider.PollInstanceReturns(true, "operation complete", models.ProvisionOperationType, nil)
})

It("should complete provision", func() {
Expand All @@ -114,12 +115,16 @@ var _ = Describe("LastInstanceOperation", func() {
actualSIDetails := fakeStorage.StoreServiceInstanceDetailsArgsForCall(0)
Expect(actualSIDetails.GUID).To(Equal(instanceID))
Expect(actualSIDetails.Outputs).To(Equal(expectedTFOutput))
Expect(actualSIDetails.OperationGUID).To(BeEmpty())
Expect(actualSIDetails.OperationType).To(BeEmpty())
Expect(actualSIDetails.ServiceGUID).To(Equal(offeringID))
Expect(actualSIDetails.PlanGUID).To(Equal(planID))
Expect(actualSIDetails.SpaceGUID).To(Equal(spaceID))
Expect(actualSIDetails.OrganizationGUID).To(Equal(orgID))

By("validating that the operation type is cleared")
Expect(fakeServiceProvider.ClearOperationTypeCallCount()).To(Equal(1))
actualContext, actualInstanceID = fakeServiceProvider.ClearOperationTypeArgsForCall(0)
Expect(actualContext.Value(middlewares.OriginatingIdentityKey)).To(Equal(expectedHeader))
Expect(actualInstanceID).To(Equal(instanceID))
})
})

Expand All @@ -128,15 +133,13 @@ var _ = Describe("LastInstanceOperation", func() {
fakeStorage.GetServiceInstanceDetailsReturns(
storage.ServiceInstanceDetails{
GUID: instanceID,
OperationType: models.DeprovisionOperationType,
OperationGUID: operationID,
PlanGUID: planID,
ServiceGUID: offeringID,
SpaceGUID: spaceID,
OrganizationGUID: orgID,
}, nil)

fakeServiceProvider.PollInstanceReturns(true, "operation complete", nil)
fakeServiceProvider.PollInstanceReturns(true, "operation complete", models.DeprovisionOperationType, nil)
})

It("should complete deprovision", func() {
Expand Down Expand Up @@ -179,7 +182,7 @@ var _ = Describe("LastInstanceOperation", func() {

Describe("operation in progress", func() {
BeforeEach(func() {
fakeServiceProvider.PollInstanceReturns(false, "operation in progress still", nil)
fakeServiceProvider.PollInstanceReturns(false, "operation in progress still", models.ProvisionOperationType, nil)
})

It("should not update the service instance", func() {
Expand All @@ -204,7 +207,7 @@ var _ = Describe("LastInstanceOperation", func() {

Describe("operation failed", func() {
BeforeEach(func() {
fakeServiceProvider.PollInstanceReturns(false, "there was an error", errors.New("some error happened"))
fakeServiceProvider.PollInstanceReturns(false, "there was an error", models.ProvisionOperationType, errors.New("some error happened"))
})

It("should set operation to failed", func() {
Expand All @@ -228,14 +231,25 @@ var _ = Describe("LastInstanceOperation", func() {
})

Describe("storage errors", func() {
Context("storage error when checking whether SI exists", func() {
BeforeEach(func() {
fakeStorage.ExistsServiceInstanceDetailsReturns(false, errors.New("failed to check whether SI exists"))
})

It("should error", func() {
_, err := serviceBroker.LastOperation(context.TODO(), instanceID, pollDetails)
Expect(err).To(MatchError("database error checking for existing instance: failed to check whether SI exists"))
})
})

Context("storage errors when getting SI details", func() {
BeforeEach(func() {
fakeStorage.GetServiceInstanceDetailsReturns(storage.ServiceInstanceDetails{}, errors.New("failed to get SI details"))
})

It("should error", func() {
_, err := serviceBroker.LastOperation(context.TODO(), instanceID, pollDetails)
Expect(err).To(MatchError("instance does not exist"))
Expect(err).To(MatchError("error getting service instance details: failed to get SI details"))
})
})

Expand All @@ -256,11 +270,11 @@ var _ = Describe("LastInstanceOperation", func() {
BeforeEach(func() {
fakeStorage.GetServiceInstanceDetailsReturns(
storage.ServiceInstanceDetails{
GUID: instanceID,
OperationType: models.DeprovisionOperationType,
ServiceGUID: offeringID,
GUID: instanceID,
ServiceGUID: offeringID,
}, nil)
fakeStorage.DeleteServiceInstanceDetailsReturns(errors.New("failed to delete SI details"))
fakeServiceProvider.PollInstanceReturns(true, "operation complete", models.DeprovisionOperationType, nil)
})

It("should error", func() {
Expand All @@ -275,11 +289,11 @@ var _ = Describe("LastInstanceOperation", func() {
BeforeEach(func() {
fakeStorage.GetServiceInstanceDetailsReturns(
storage.ServiceInstanceDetails{
GUID: instanceID,
OperationType: models.DeprovisionOperationType,
ServiceGUID: offeringID,
GUID: instanceID,
ServiceGUID: offeringID,
}, nil)
fakeStorage.DeleteProvisionRequestDetailsReturns(errors.New("failed to delete provision params"))
fakeServiceProvider.PollInstanceReturns(true, "operation complete", models.DeprovisionOperationType, nil)
})

It("should error", func() {
Expand All @@ -291,16 +305,16 @@ var _ = Describe("LastInstanceOperation", func() {
})
})

Describe("Service provider error", func() {
Describe("service provider error", func() {
Context("service provider errors when deleting provider instance data", func() {
BeforeEach(func() {
fakeStorage.GetServiceInstanceDetailsReturns(
storage.ServiceInstanceDetails{
GUID: instanceID,
OperationType: models.DeprovisionOperationType,
ServiceGUID: offeringID,
GUID: instanceID,
ServiceGUID: offeringID,
}, nil)
fakeServiceProvider.DeleteInstanceDataReturns(errors.New("failed to delete provider instance data"))
fakeServiceProvider.PollInstanceReturns(true, "operation complete", models.DeprovisionOperationType, nil)
})

It("should error", func() {
Expand All @@ -314,7 +328,7 @@ var _ = Describe("LastInstanceOperation", func() {

Describe("service instance does not exist", func() {
BeforeEach(func() {
fakeStorage.GetServiceInstanceDetailsReturns(storage.ServiceInstanceDetails{}, errors.New("does not exist"))
fakeStorage.ExistsServiceInstanceDetailsReturns(false, nil)
})

It("should return HTTP 410 as per OSBAPI spec", func() {
Expand Down
20 changes: 12 additions & 8 deletions brokerapi/broker/provision.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/pivotal-cf/brokerapi/v11/domain/apiresponses"

"github.com/cloudfoundry/cloud-service-broker/v2/internal/paramparser"
"github.com/cloudfoundry/cloud-service-broker/v2/internal/storage"
"github.com/cloudfoundry/cloud-service-broker/v2/utils/correlation"
"github.com/cloudfoundry/cloud-service-broker/v2/utils/request"
)
Expand Down Expand Up @@ -81,18 +82,19 @@ func (broker *ServiceBroker) Provision(ctx context.Context, instanceID string, d
return domain.ProvisionedServiceSpec{}, err
}

// get instance details
instanceDetails, err := serviceProvider.Provision(ctx, vars)
err = serviceProvider.Provision(ctx, vars)
if err != nil {
return domain.ProvisionedServiceSpec{}, err
}

// save instance details
instanceDetails.ServiceGUID = parsedDetails.ServiceID
instanceDetails.GUID = instanceID
instanceDetails.PlanGUID = parsedDetails.PlanID
instanceDetails.SpaceGUID = parsedDetails.SpaceGUID
instanceDetails.OrganizationGUID = parsedDetails.OrganizationGUID
instanceDetails := storage.ServiceInstanceDetails{
ServiceGUID: parsedDetails.ServiceID,
GUID: instanceID,
PlanGUID: parsedDetails.PlanID,
SpaceGUID: parsedDetails.SpaceGUID,
OrganizationGUID: parsedDetails.OrganizationGUID,
}

if err := broker.store.StoreServiceInstanceDetails(instanceDetails); err != nil {
return domain.ProvisionedServiceSpec{}, fmt.Errorf("error saving instance details to database: %s. WARNING: this instance cannot be deprovisioned through cf. Contact your operator for cleanup", err)
Expand All @@ -104,5 +106,7 @@ func (broker *ServiceBroker) Provision(ctx context.Context, instanceID string, d
return domain.ProvisionedServiceSpec{}, fmt.Errorf("error saving provision request details to database: %s. Services relying on async provisioning will not be able to complete provisioning", err)
}

return domain.ProvisionedServiceSpec{IsAsync: true, DashboardURL: "", OperationData: instanceDetails.OperationGUID}, nil
operationID := generateTFInstanceID(instanceDetails.GUID)

return domain.ProvisionedServiceSpec{IsAsync: true, DashboardURL: "", OperationData: operationID}, nil
}
Loading

0 comments on commit f8f283f

Please sign in to comment.