From b169012bff806cbc400d1e986007baaed96ff75b Mon Sep 17 00:00:00 2001 From: George Blue Date: Thu, 3 Oct 2024 13:14:35 +0100 Subject: [PATCH 1/4] chore: debug option to leave workspace on disk (#1025) --- docs/configuration.md | 10 ++++++++++ pkg/providers/tf/workspace/workspace.go | 21 ++++++++++++++++----- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 80df62af8..a58e7f609 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -78,6 +78,7 @@ no longer marked as primary. ## Broker Service Configuration Broker service configuration values: + | Environment Variable | Config File Value | Type | Description | |----------------------|------|-------------|------------------| | SECURITY_USER_NAME * | api.user | string |

Broker authentication username

| @@ -86,9 +87,18 @@ Broker service configuration values: | TLS_CERT_CHAIN | api.certCaBundle | string |

File path to a pem encoded certificate chain

| | TLS_PRIVATE_KEY | api.tlsKey | string |

File path to a pem encoded private key

| + +## Debugging +Values for debugging: + +| Environment Variable | Config File Value | Type | Description | +|----------------------------------------|---------------------------|------|-------------------------------------------------------------------------------------------------| +| CSB_DEBUG_LEAVE_WORKSPACE_DIR | debug.leave_workspace_dir | bool | Disables the cleanup of workspace directories, so you can inspect the files and run tf commands | + ## Feature flags Configuration Feature flags can be toggled through the following configuration values. See also [source code occurences of "toggles.Features.Toggle"](https://github.com/cloudfoundry/cloud-service-broker/search?q=toggles.Features.Toggle&type=code) + | Environment Variable | Config File Value | Type | Description | Default | |----------------------|------|-------------|------------------|----------| | GSB_COMPATIBILITY_ENABLE_BUILTIN_BROKERPAKS * | compatibility.enable_builtin_brokerpaks | Boolean |

Load brokerpaks that are built-in to the software.

| "true" | diff --git a/pkg/providers/tf/workspace/workspace.go b/pkg/providers/tf/workspace/workspace.go index 2da28988f..a54073c87 100644 --- a/pkg/providers/tf/workspace/workspace.go +++ b/pkg/providers/tf/workspace/workspace.go @@ -25,18 +25,27 @@ import ( "strings" "sync" + "github.com/spf13/viper" + "github.com/cloudfoundry/cloud-service-broker/v2/pkg/providers/tf/command" "github.com/cloudfoundry/cloud-service-broker/v2/pkg/providers/tf/executor" "github.com/hashicorp/go-version" ) -// DefaultInstanceName is the default name of an instance of a particular module. const ( - DefaultInstanceName = "instance" - binaryName = "tofu" + // DefaultInstanceName is the default name of an instance of a particular module. + DefaultInstanceName = "instance" + binaryName = "tofu" + leaveWorkspaceDirectoryConfigKey = "debug.leave_workspace_dir" + leaveWorkspaceDirectoryEnvVar = "CSB_DEBUG_LEAVE_WORKSPACE_DIR" ) +func init() { + viper.BindEnv(leaveWorkspaceDirectoryConfigKey, leaveWorkspaceDirectoryEnvVar) + viper.SetDefault(leaveWorkspaceDirectoryConfigKey, false) +} + // NewWorkspace creates a new TerraformWorkspace from a given template and variables to populate an instance of it. // The created instance will have the name specified by the DefaultInstanceName constant. func NewWorkspace(templateVars map[string]any, @@ -287,8 +296,10 @@ func (workspace *TerraformWorkspace) teardownFs() error { workspace.State = bytes - if err := os.RemoveAll(workspace.dir); err != nil { - return err + if !viper.GetBool(leaveWorkspaceDirectoryConfigKey) { + if err := os.RemoveAll(workspace.dir); err != nil { + return err + } } workspace.dir = "" From 69a15f49b22b653551914854d95b5c7b654c9620 Mon Sep 17 00:00:00 2001 From: George Blue Date: Thu, 3 Oct 2024 13:33:56 +0100 Subject: [PATCH 2/4] chore: report database error during LastOperation (#1107) We now differentiate between an instance not existing, and having an error getting the record from the database. All the other endpoints did this, so this just gets LastOperation to follow the same pattern --- brokerapi/broker/last_instance_operation.go | 18 ++++++++++++--- .../broker/last_instance_operation_test.go | 22 +++++++++++++++---- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/brokerapi/broker/last_instance_operation.go b/brokerapi/broker/last_instance_operation.go index 640a17f52..e8d4c191f 100644 --- a/brokerapi/broker/last_instance_operation.go +++ b/brokerapi/broker/last_instance_operation.go @@ -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. @@ -24,9 +24,21 @@ 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) diff --git a/brokerapi/broker/last_instance_operation_test.go b/brokerapi/broker/last_instance_operation_test.go index 5d1e949d5..2b4294adc 100644 --- a/brokerapi/broker/last_instance_operation_test.go +++ b/brokerapi/broker/last_instance_operation_test.go @@ -66,6 +66,7 @@ var _ = Describe("LastInstanceOperation", func() { } fakeStorage = &brokerfakes.FakeStorage{} + fakeStorage.ExistsServiceInstanceDetailsReturns(true, nil) fakeStorage.GetServiceInstanceDetailsReturns( storage.ServiceInstanceDetails{ GUID: instanceID, @@ -75,7 +76,9 @@ var _ = Describe("LastInstanceOperation", func() { ServiceGUID: offeringID, SpaceGUID: spaceID, OrganizationGUID: orgID, - }, nil) + }, + nil, + ) serviceBroker = must(broker.New(brokerConfig, fakeStorage, utils.NewLogger("brokers-test"))) @@ -228,6 +231,17 @@ 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")) @@ -235,7 +249,7 @@ var _ = Describe("LastInstanceOperation", func() { 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")) }) }) @@ -291,7 +305,7 @@ 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( @@ -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() { From eabd3d631efb9c27ceb763456a06f67c99016ca1 Mon Sep 17 00:00:00 2001 From: ifindlay-cci <84311346+ifindlay-cci@users.noreply.github.com> Date: Thu, 3 Oct 2024 16:52:15 +0100 Subject: [PATCH 3/4] fix: removed redundant data from service_instance_details table (#1109) * fix: removed OperationID and OperationType from service_instance_details table. Storing this data in the service_instance_details table is redundant, since this information is also stored the terraform_deployment table. If these two sources of truth became inconsistent, there is a possiblity that a service details could be deleted incorrectly following a failed deletion. Having a single source of truth prevents this incorrect behaviour. [#TPCF-26508](https://vmw-jira.broadcom.com/browse/TPCF-26508) * fix: Removed debugging logging - as per PR comments [#TPCF-26508](https://vmw-jira.broadcom.com/browse/TPCF-26508) * fix: Addressed 'go test' failures [#TPCF-26508](https://vmw-jira.broadcom.com/browse/TPCF-26508) * fix: Removed an unnecessary log line [#TPCF-26508](https://vmw-jira.broadcom.com/browse/TPCF-26508) --- brokerapi/broker/bind_test.go | 3 - brokerapi/broker/deprovision.go | 2 - brokerapi/broker/deprovision_test.go | 22 ++- brokerapi/broker/last_instance_operation.go | 11 +- .../broker/last_instance_operation_test.go | 40 ++-- brokerapi/broker/provision.go | 20 +- brokerapi/broker/provision_test.go | 10 +- brokerapi/broker/unbind_test.go | 5 - brokerapi/broker/update.go | 6 +- brokerapi/broker/update_test.go | 29 ++- dbservice/migrations.go | 10 +- dbservice/models/db.go | 2 +- dbservice/models/historical_db.go | 25 +++ internal/storage/service_instance_details.go | 6 - .../storage/service_instance_details_test.go | 16 -- .../brokerfakes/fake_service_provider.go | 176 ++++++++++++------ pkg/broker/service_provider.go | 9 +- pkg/providers/tf/bind.go | 4 +- pkg/providers/tf/bind_test.go | 5 +- pkg/providers/tf/clear_operation_type.go | 18 ++ pkg/providers/tf/deployment_manager.go | 21 ++- pkg/providers/tf/deployment_manager_test.go | 42 ++++- pkg/providers/tf/poll_instance.go | 2 +- pkg/providers/tf/poll_instance_test.go | 5 +- pkg/providers/tf/provider.go | 18 +- pkg/providers/tf/provision.go | 33 ++-- pkg/providers/tf/provision_test.go | 36 ++-- .../fake_deployment_manager_interface.go | 107 +++++++++-- pkg/providers/tf/unbind_test.go | 4 +- pkg/providers/tf/update.go | 17 +- pkg/providers/tf/update_test.go | 18 +- 31 files changed, 461 insertions(+), 261 deletions(-) create mode 100644 pkg/providers/tf/clear_operation_type.go diff --git a/brokerapi/broker/bind_test.go b/brokerapi/broker/bind_test.go index 48cf19cd8..f2d46db67 100644 --- a/brokerapi/broker/bind_test.go +++ b/brokerapi/broker/bind_test.go @@ -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" @@ -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) diff --git a/brokerapi/broker/deprovision.go b/brokerapi/broker/deprovision.go index db91b1d3d..eb8a0003b 100644 --- a/brokerapi/broker/deprovision.go +++ b/brokerapi/broker/deprovision.go @@ -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) } diff --git a/brokerapi/broker/deprovision_test.go b/brokerapi/broker/deprovision_test.go index 40dcad0ec..a543ad67d 100644 --- a/brokerapi/broker/deprovision_test.go +++ b/brokerapi/broker/deprovision_test.go @@ -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" @@ -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"))) @@ -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() { @@ -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) }) diff --git a/brokerapi/broker/last_instance_operation.go b/brokerapi/broker/last_instance_operation.go index e8d4c191f..a737891a3 100644 --- a/brokerapi/broker/last_instance_operation.go +++ b/brokerapi/broker/last_instance_operation.go @@ -46,9 +46,7 @@ func (broker *ServiceBroker) LastOperation(ctx context.Context, instanceID strin 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 } @@ -96,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 } diff --git a/brokerapi/broker/last_instance_operation_test.go b/brokerapi/broker/last_instance_operation_test.go index 2b4294adc..78ce90e72 100644 --- a/brokerapi/broker/last_instance_operation_test.go +++ b/brokerapi/broker/last_instance_operation_test.go @@ -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 @@ -70,8 +70,6 @@ var _ = Describe("LastInstanceOperation", func() { fakeStorage.GetServiceInstanceDetailsReturns( storage.ServiceInstanceDetails{ GUID: instanceID, - OperationType: models.ProvisionOperationType, - OperationGUID: operationID, PlanGUID: planID, ServiceGUID: offeringID, SpaceGUID: spaceID, @@ -92,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() { @@ -117,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)) }) }) @@ -131,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() { @@ -182,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() { @@ -207,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() { @@ -270,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() { @@ -289,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() { @@ -310,11 +310,11 @@ var _ = Describe("LastInstanceOperation", 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() { diff --git a/brokerapi/broker/provision.go b/brokerapi/broker/provision.go index be3104966..6404994ce 100644 --- a/brokerapi/broker/provision.go +++ b/brokerapi/broker/provision.go @@ -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" ) @@ -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) @@ -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 } diff --git a/brokerapi/broker/provision_test.go b/brokerapi/broker/provision_test.go index 3f06b9d11..668eb2b34 100644 --- a/brokerapi/broker/provision_test.go +++ b/brokerapi/broker/provision_test.go @@ -16,7 +16,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" @@ -32,7 +31,7 @@ var _ = Describe("Provision", func() { planID = "test-plan-id" offeringID = "test-service-id" newInstanceID = "test-instance-id" - operationID = "test-operation-id" + operationID = "tf:test-instance-id:" ) var ( @@ -45,10 +44,7 @@ var _ = Describe("Provision", func() { BeforeEach(func() { fakeServiceProvider = &pkgBrokerFakes.FakeServiceProvider{} - fakeServiceProvider.ProvisionReturns(storage.ServiceInstanceDetails{ - OperationType: models.ProvisionOperationType, - OperationGUID: operationID, - }, nil) + fakeServiceProvider.ProvisionReturns(nil) providerBuilder := func(logger lager.Logger, store pkgBroker.ServiceProviderStorage) pkgBroker.ServiceProvider { return fakeServiceProvider @@ -255,7 +251,7 @@ var _ = Describe("Provision", func() { When("provider provision errors", func() { BeforeEach(func() { - fakeServiceProvider.ProvisionReturns(storage.ServiceInstanceDetails{}, errors.New("cannot provision right now")) + fakeServiceProvider.ProvisionReturns(errors.New("cannot provision right now")) }) It("should error", func() { diff --git a/brokerapi/broker/unbind_test.go b/brokerapi/broker/unbind_test.go index c59b2b1bb..86979739b 100644 --- a/brokerapi/broker/unbind_test.go +++ b/brokerapi/broker/unbind_test.go @@ -8,7 +8,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" @@ -57,8 +56,6 @@ var _ = Describe("Unbind", func() { PlanGUID: planID, SpaceGUID: spaceID, OrganizationGUID: orgID, - OperationType: models.ProvisionOperationType, - OperationGUID: "provision-operation-GUID", }, nil) fakeStorage.GetBindRequestDetailsReturns(storage.JSONObject{"foo": "bar"}, nil) @@ -114,8 +111,6 @@ var _ = Describe("Unbind", func() { PlanGUID: planID, SpaceGUID: spaceID, OrganizationGUID: orgID, - OperationType: models.ProvisionOperationType, - OperationGUID: "provision-operation-GUID", }, nil) fakeStorage.GetBindRequestDetailsReturns(storage.JSONObject{"foo": "bar"}, nil) }) diff --git a/brokerapi/broker/update.go b/brokerapi/broker/update.go index 3960b96bf..fcbab5fc0 100644 --- a/brokerapi/broker/update.go +++ b/brokerapi/broker/update.go @@ -173,7 +173,7 @@ func (broker *ServiceBroker) doUpdate(ctx context.Context, serviceProvider broke return domain.UpdateServiceSpec{}, fmt.Errorf("tofu version check failed: %s", err.Error()) } - instanceDetails, err := serviceProvider.Update(ctx, vars) + err = serviceProvider.Update(ctx, vars) if err != nil { return domain.UpdateServiceSpec{}, err } @@ -183,8 +183,6 @@ func (broker *ServiceBroker) doUpdate(ctx context.Context, serviceProvider broke instance.PlanGUID = parsedDetails.PlanID } - instance.OperationType = instanceDetails.OperationType - instance.OperationGUID = instanceDetails.OperationID if err := broker.store.StoreServiceInstanceDetails(instance); err != nil { return domain.UpdateServiceSpec{}, fmt.Errorf("error saving instance details to database: %s. WARNING: this instance cannot be deprovisioned through cf. Contact your operator for cleanup", err) } @@ -197,7 +195,7 @@ func (broker *ServiceBroker) doUpdate(ctx context.Context, serviceProvider broke return domain.UpdateServiceSpec{ IsAsync: true, DashboardURL: "", - OperationData: instanceDetails.OperationID, + OperationData: generateTFInstanceID(instance.GUID), }, nil } diff --git a/brokerapi/broker/update_test.go b/brokerapi/broker/update_test.go index 135f028a4..e7d4098f4 100644 --- a/brokerapi/broker/update_test.go +++ b/brokerapi/broker/update_test.go @@ -35,7 +35,7 @@ var _ = Describe("Update", func() { offeringID = "test-service-id" newPlanID = "new-test-plan-id" instanceID = "test-instance-id" - updateOperationID = "update-operation-id" + updateOperationID = "tf:test-instance-id:" ) var ( @@ -129,8 +129,11 @@ var _ = Describe("Update", func() { PlanGUID: originalPlanID, SpaceGUID: spaceID, OrganizationGUID: orgID, - OperationType: models.ProvisionOperationType, - OperationGUID: "provision-operation-GUID", + }, nil) + fakeStorage.GetTerraformDeploymentReturns(storage.TerraformDeployment{ + ID: updateOperationID, + LastOperationType: models.ProvisionOperationType, + LastOperationState: "GOOG", }, nil) serviceBroker = must(broker.New(brokerConfig, fakeStorage, utils.NewLogger("brokers-test"))) @@ -157,10 +160,8 @@ var _ = Describe("Update", func() { Describe("update", func() { When("no plan or parameter changes are requested", func() { BeforeEach(func() { - fakeServiceProvider.UpdateReturns(models.ServiceInstanceDetails{ - OperationType: models.UpdateOperationType, - OperationID: updateOperationID, - }, nil) + fakeServiceProvider.UpdateReturns(nil) + fakeServiceProvider.PollInstanceReturns(true, "a message", models.UpdateOperationType, nil) }) It("should complete changing the instance operation type", func() { @@ -196,10 +197,7 @@ var _ = Describe("Update", func() { When("plan change is requested", func() { BeforeEach(func() { - fakeServiceProvider.UpdateReturns(models.ServiceInstanceDetails{ - OperationType: models.UpdateOperationType, - OperationID: updateOperationID, - }, nil) + fakeServiceProvider.UpdateReturns(nil) }) It("should do update async and not change planID", func() { @@ -230,10 +228,7 @@ var _ = Describe("Update", func() { When("parameter change is requested", func() { BeforeEach(func() { - fakeServiceProvider.UpdateReturns(models.ServiceInstanceDetails{ - OperationType: models.UpdateOperationType, - OperationID: updateOperationID, - }, nil) + fakeServiceProvider.UpdateReturns(nil) updateDetails = domain.UpdateDetails{ ServiceID: offeringID, @@ -286,7 +281,7 @@ var _ = Describe("Update", func() { When("provider update errors", func() { BeforeEach(func() { - fakeServiceProvider.UpdateReturns(models.ServiceInstanceDetails{}, errors.New("cannot update right now")) + fakeServiceProvider.UpdateReturns(errors.New("cannot update right now")) }) It("should error and not update the provision variables", func() { @@ -799,8 +794,6 @@ func expectOperationTypeToBeUpdated( ) { Expect(fakeStorage.StoreServiceInstanceDetailsCallCount()).To(Equal(1)) actualServiceInstanceDetails := fakeStorage.StoreServiceInstanceDetailsArgsForCall(0) - Expect(actualServiceInstanceDetails.OperationType).To(Equal("update")) - Expect(actualServiceInstanceDetails.OperationGUID).To(Equal(operationGUID)) Expect(actualServiceInstanceDetails.GUID).To(Equal(instanceID)) Expect(actualServiceInstanceDetails.ServiceGUID).To(Equal(offeringID)) Expect(actualServiceInstanceDetails.PlanGUID).To(Equal(newPlanID)) diff --git a/dbservice/migrations.go b/dbservice/migrations.go index 30a7dc3c7..21d3f8740 100644 --- a/dbservice/migrations.go +++ b/dbservice/migrations.go @@ -23,7 +23,7 @@ import ( "github.com/cloudfoundry/cloud-service-broker/v2/dbservice/models" ) -const numMigrations = 16 +const numMigrations = 18 // RunMigrations runs schema migrations on the provided service broker database to get it up to date func RunMigrations(db *gorm.DB) error { @@ -128,6 +128,14 @@ func RunMigrations(db *gorm.DB) error { return autoMigrateTables(db, &models.BindRequestDetailsV1{}) } + migrations[16] = func() error { + return db.Migrator().DropColumn(&models.ServiceInstanceDetailsV3{}, "operation_type") + } + + migrations[17] = func() error { + return db.Migrator().DropColumn(&models.ServiceInstanceDetailsV3{}, "operation_id") + } + var lastMigrationNumber = -1 // if we've run any migrations before, we should have a migrations table, so find the last one we ran diff --git a/dbservice/models/db.go b/dbservice/models/db.go index bc38d787e..0b5ee494d 100644 --- a/dbservice/models/db.go +++ b/dbservice/models/db.go @@ -34,7 +34,7 @@ const ( type ServiceBindingCredentials ServiceBindingCredentialsV2 // ServiceInstanceDetails holds information about provisioned services. -type ServiceInstanceDetails ServiceInstanceDetailsV3 +type ServiceInstanceDetails ServiceInstanceDetailsV4 // ProvisionRequestDetails holds user-defined properties passed to a call // to provision a service. diff --git a/dbservice/models/historical_db.go b/dbservice/models/historical_db.go index 55d6dd8a7..7752e6540 100644 --- a/dbservice/models/historical_db.go +++ b/dbservice/models/historical_db.go @@ -162,6 +162,31 @@ func (ServiceInstanceDetailsV3) TableName() string { return "service_instance_details" } +// ServiceInstanceDetailsV4 holds information about provisioned services. +type ServiceInstanceDetailsV4 struct { + ID string `gorm:"primary_key;type:varchar(255);not null"` + CreatedAt time.Time + UpdatedAt time.Time + DeletedAt *time.Time + + Name string + Location string + URL string + OtherDetails []byte `gorm:"type:blob"` + + ServiceID string + PlanID string + SpaceGUID string + OrganizationGUID string +} + +// TableName returns a consistent table name for +// gorm so multiple structs from different versions of the database all operate +// on the same table. +func (ServiceInstanceDetailsV4) TableName() string { + return "service_instance_details" +} + // ProvisionRequestDetailsV1 holds user-defined properties passed to a call // to provision a service. type ProvisionRequestDetailsV1 struct { diff --git a/internal/storage/service_instance_details.go b/internal/storage/service_instance_details.go index 78af047e9..b0793f2b4 100644 --- a/internal/storage/service_instance_details.go +++ b/internal/storage/service_instance_details.go @@ -17,8 +17,6 @@ type ServiceInstanceDetails struct { PlanGUID string SpaceGUID string OrganizationGUID string - OperationType string - OperationGUID string } func (s *Storage) StoreServiceInstanceDetails(d ServiceInstanceDetails) error { @@ -40,8 +38,6 @@ func (s *Storage) StoreServiceInstanceDetails(d ServiceInstanceDetails) error { m.PlanID = d.PlanGUID m.SpaceGUID = d.SpaceGUID m.OrganizationGUID = d.OrganizationGUID - m.OperationType = d.OperationType - m.OperationID = d.OperationGUID switch m.ID { case "": @@ -95,8 +91,6 @@ func (s *Storage) GetServiceInstanceDetails(guid string) (ServiceInstanceDetails PlanGUID: receiver.PlanID, SpaceGUID: receiver.SpaceGUID, OrganizationGUID: receiver.OrganizationGUID, - OperationType: receiver.OperationType, - OperationGUID: receiver.OperationID, }, nil } diff --git a/internal/storage/service_instance_details_test.go b/internal/storage/service_instance_details_test.go index 4bc717b8b..c29789a23 100644 --- a/internal/storage/service_instance_details_test.go +++ b/internal/storage/service_instance_details_test.go @@ -22,8 +22,6 @@ var _ = Describe("ServiceInstanceDetails", func() { PlanGUID: "fake-plan-guid", SpaceGUID: "fake-space-guid", OrganizationGUID: "fake-org-guid", - OperationType: "fake-operation-type", - OperationGUID: "fake-operation-guid", }) Expect(err).NotTo(HaveOccurred()) @@ -38,8 +36,6 @@ var _ = Describe("ServiceInstanceDetails", func() { Expect(receiver.PlanID).To(Equal("fake-plan-guid")) Expect(receiver.SpaceGUID).To(Equal("fake-space-guid")) Expect(receiver.OrganizationGUID).To(Equal("fake-org-guid")) - Expect(receiver.OperationType).To(Equal("fake-operation-type")) - Expect(receiver.OperationID).To(Equal("fake-operation-guid")) }) When("encoding fails", func() { @@ -67,8 +63,6 @@ var _ = Describe("ServiceInstanceDetails", func() { PlanGUID: "fake-plan-guid", SpaceGUID: "fake-space-guid", OrganizationGUID: "fake-org-guid", - OperationType: "fake-operation-type", - OperationGUID: "fake-operation-guid", }) Expect(err).NotTo(HaveOccurred()) @@ -83,8 +77,6 @@ var _ = Describe("ServiceInstanceDetails", func() { Expect(receiver.PlanID).To(Equal("fake-plan-guid")) Expect(receiver.SpaceGUID).To(Equal("fake-space-guid")) Expect(receiver.OrganizationGUID).To(Equal("fake-org-guid")) - Expect(receiver.OperationType).To(Equal("fake-operation-type")) - Expect(receiver.OperationID).To(Equal("fake-operation-guid")) }) }) }) @@ -106,8 +98,6 @@ var _ = Describe("ServiceInstanceDetails", func() { Expect(r.PlanGUID).To(Equal("fake-plan-id-2")) Expect(r.SpaceGUID).To(Equal("fake-space-guid-2")) Expect(r.OrganizationGUID).To(Equal("fake-org-guid-2")) - Expect(r.OperationType).To(Equal("fake-operation-type-2")) - Expect(r.OperationGUID).To(Equal("fake-operation-id-2")) }) When("decoding fails", func() { @@ -211,8 +201,6 @@ func addFakeServiceInstanceDetails() { PlanID: "fake-plan-id-1", SpaceGUID: "fake-space-guid-1", OrganizationGUID: "fake-org-guid-1", - OperationType: "fake-operation-type-1", - OperationID: "fake-operation-id-1", }).Error).NotTo(HaveOccurred()) Expect(db.Create(&models.ServiceInstanceDetails{ ID: "fake-id-2", @@ -224,8 +212,6 @@ func addFakeServiceInstanceDetails() { PlanID: "fake-plan-id-2", SpaceGUID: "fake-space-guid-2", OrganizationGUID: "fake-org-guid-2", - OperationType: "fake-operation-type-2", - OperationID: "fake-operation-id-2", }).Error).NotTo(HaveOccurred()) Expect(db.Create(&models.ServiceInstanceDetails{ ID: "fake-id-3", @@ -237,7 +223,5 @@ func addFakeServiceInstanceDetails() { PlanID: "fake-plan-id-3", SpaceGUID: "fake-space-guid-3", OrganizationGUID: "fake-org-guid-3", - OperationType: "fake-operation-type-3", - OperationID: "fake-operation-id-3", }).Error).NotTo(HaveOccurred()) } diff --git a/pkg/broker/brokerfakes/fake_service_provider.go b/pkg/broker/brokerfakes/fake_service_provider.go index a965087eb..53f9f9f56 100644 --- a/pkg/broker/brokerfakes/fake_service_provider.go +++ b/pkg/broker/brokerfakes/fake_service_provider.go @@ -5,7 +5,6 @@ import ( "context" "sync" - "github.com/cloudfoundry/cloud-service-broker/v2/dbservice/models" "github.com/cloudfoundry/cloud-service-broker/v2/internal/storage" "github.com/cloudfoundry/cloud-service-broker/v2/pkg/broker" "github.com/cloudfoundry/cloud-service-broker/v2/pkg/varcontext" @@ -49,6 +48,18 @@ type FakeServiceProvider struct { checkUpgradeAvailableReturnsOnCall map[int]struct { result1 error } + ClearOperationTypeStub func(context.Context, string) error + clearOperationTypeMutex sync.RWMutex + clearOperationTypeArgsForCall []struct { + arg1 context.Context + arg2 string + } + clearOperationTypeReturns struct { + result1 error + } + clearOperationTypeReturnsOnCall map[int]struct { + result1 error + } DeleteBindingDataStub func(context.Context, string, string) error deleteBindingDataMutex sync.RWMutex deleteBindingDataArgsForCall []struct { @@ -119,7 +130,7 @@ type FakeServiceProvider struct { result1 storage.JSONObject result2 error } - PollInstanceStub func(context.Context, string) (bool, string, error) + PollInstanceStub func(context.Context, string) (bool, string, string, error) pollInstanceMutex sync.RWMutex pollInstanceArgsForCall []struct { arg1 context.Context @@ -128,26 +139,26 @@ type FakeServiceProvider struct { pollInstanceReturns struct { result1 bool result2 string - result3 error + result3 string + result4 error } pollInstanceReturnsOnCall map[int]struct { result1 bool result2 string - result3 error + result3 string + result4 error } - ProvisionStub func(context.Context, *varcontext.VarContext) (storage.ServiceInstanceDetails, error) + ProvisionStub func(context.Context, *varcontext.VarContext) error provisionMutex sync.RWMutex provisionArgsForCall []struct { arg1 context.Context arg2 *varcontext.VarContext } provisionReturns struct { - result1 storage.ServiceInstanceDetails - result2 error + result1 error } provisionReturnsOnCall map[int]struct { - result1 storage.ServiceInstanceDetails - result2 error + result1 error } UnbindStub func(context.Context, string, string, *varcontext.VarContext) error unbindMutex sync.RWMutex @@ -163,19 +174,17 @@ type FakeServiceProvider struct { unbindReturnsOnCall map[int]struct { result1 error } - UpdateStub func(context.Context, *varcontext.VarContext) (models.ServiceInstanceDetails, error) + UpdateStub func(context.Context, *varcontext.VarContext) error updateMutex sync.RWMutex updateArgsForCall []struct { arg1 context.Context arg2 *varcontext.VarContext } updateReturns struct { - result1 models.ServiceInstanceDetails - result2 error + result1 error } updateReturnsOnCall map[int]struct { - result1 models.ServiceInstanceDetails - result2 error + result1 error } UpgradeBindingsStub func(context.Context, *varcontext.VarContext, []*varcontext.VarContext) error upgradeBindingsMutex sync.RWMutex @@ -396,6 +405,68 @@ func (fake *FakeServiceProvider) CheckUpgradeAvailableReturnsOnCall(i int, resul }{result1} } +func (fake *FakeServiceProvider) ClearOperationType(arg1 context.Context, arg2 string) error { + fake.clearOperationTypeMutex.Lock() + ret, specificReturn := fake.clearOperationTypeReturnsOnCall[len(fake.clearOperationTypeArgsForCall)] + fake.clearOperationTypeArgsForCall = append(fake.clearOperationTypeArgsForCall, struct { + arg1 context.Context + arg2 string + }{arg1, arg2}) + stub := fake.ClearOperationTypeStub + fakeReturns := fake.clearOperationTypeReturns + fake.recordInvocation("ClearOperationType", []interface{}{arg1, arg2}) + fake.clearOperationTypeMutex.Unlock() + if stub != nil { + return stub(arg1, arg2) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeServiceProvider) ClearOperationTypeCallCount() int { + fake.clearOperationTypeMutex.RLock() + defer fake.clearOperationTypeMutex.RUnlock() + return len(fake.clearOperationTypeArgsForCall) +} + +func (fake *FakeServiceProvider) ClearOperationTypeCalls(stub func(context.Context, string) error) { + fake.clearOperationTypeMutex.Lock() + defer fake.clearOperationTypeMutex.Unlock() + fake.ClearOperationTypeStub = stub +} + +func (fake *FakeServiceProvider) ClearOperationTypeArgsForCall(i int) (context.Context, string) { + fake.clearOperationTypeMutex.RLock() + defer fake.clearOperationTypeMutex.RUnlock() + argsForCall := fake.clearOperationTypeArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + +func (fake *FakeServiceProvider) ClearOperationTypeReturns(result1 error) { + fake.clearOperationTypeMutex.Lock() + defer fake.clearOperationTypeMutex.Unlock() + fake.ClearOperationTypeStub = nil + fake.clearOperationTypeReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeServiceProvider) ClearOperationTypeReturnsOnCall(i int, result1 error) { + fake.clearOperationTypeMutex.Lock() + defer fake.clearOperationTypeMutex.Unlock() + fake.ClearOperationTypeStub = nil + if fake.clearOperationTypeReturnsOnCall == nil { + fake.clearOperationTypeReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.clearOperationTypeReturnsOnCall[i] = struct { + result1 error + }{result1} +} + func (fake *FakeServiceProvider) DeleteBindingData(arg1 context.Context, arg2 string, arg3 string) error { fake.deleteBindingDataMutex.Lock() ret, specificReturn := fake.deleteBindingDataReturnsOnCall[len(fake.deleteBindingDataArgsForCall)] @@ -724,7 +795,7 @@ func (fake *FakeServiceProvider) GetTerraformOutputsReturnsOnCall(i int, result1 }{result1, result2} } -func (fake *FakeServiceProvider) PollInstance(arg1 context.Context, arg2 string) (bool, string, error) { +func (fake *FakeServiceProvider) PollInstance(arg1 context.Context, arg2 string) (bool, string, string, error) { fake.pollInstanceMutex.Lock() ret, specificReturn := fake.pollInstanceReturnsOnCall[len(fake.pollInstanceArgsForCall)] fake.pollInstanceArgsForCall = append(fake.pollInstanceArgsForCall, struct { @@ -739,9 +810,9 @@ func (fake *FakeServiceProvider) PollInstance(arg1 context.Context, arg2 string) return stub(arg1, arg2) } if specificReturn { - return ret.result1, ret.result2, ret.result3 + return ret.result1, ret.result2, ret.result3, ret.result4 } - return fakeReturns.result1, fakeReturns.result2, fakeReturns.result3 + return fakeReturns.result1, fakeReturns.result2, fakeReturns.result3, fakeReturns.result4 } func (fake *FakeServiceProvider) PollInstanceCallCount() int { @@ -750,7 +821,7 @@ func (fake *FakeServiceProvider) PollInstanceCallCount() int { return len(fake.pollInstanceArgsForCall) } -func (fake *FakeServiceProvider) PollInstanceCalls(stub func(context.Context, string) (bool, string, error)) { +func (fake *FakeServiceProvider) PollInstanceCalls(stub func(context.Context, string) (bool, string, string, error)) { fake.pollInstanceMutex.Lock() defer fake.pollInstanceMutex.Unlock() fake.PollInstanceStub = stub @@ -763,18 +834,19 @@ func (fake *FakeServiceProvider) PollInstanceArgsForCall(i int) (context.Context return argsForCall.arg1, argsForCall.arg2 } -func (fake *FakeServiceProvider) PollInstanceReturns(result1 bool, result2 string, result3 error) { +func (fake *FakeServiceProvider) PollInstanceReturns(result1 bool, result2 string, result3 string, result4 error) { fake.pollInstanceMutex.Lock() defer fake.pollInstanceMutex.Unlock() fake.PollInstanceStub = nil fake.pollInstanceReturns = struct { result1 bool result2 string - result3 error - }{result1, result2, result3} + result3 string + result4 error + }{result1, result2, result3, result4} } -func (fake *FakeServiceProvider) PollInstanceReturnsOnCall(i int, result1 bool, result2 string, result3 error) { +func (fake *FakeServiceProvider) PollInstanceReturnsOnCall(i int, result1 bool, result2 string, result3 string, result4 error) { fake.pollInstanceMutex.Lock() defer fake.pollInstanceMutex.Unlock() fake.PollInstanceStub = nil @@ -782,17 +854,19 @@ func (fake *FakeServiceProvider) PollInstanceReturnsOnCall(i int, result1 bool, fake.pollInstanceReturnsOnCall = make(map[int]struct { result1 bool result2 string - result3 error + result3 string + result4 error }) } fake.pollInstanceReturnsOnCall[i] = struct { result1 bool result2 string - result3 error - }{result1, result2, result3} + result3 string + result4 error + }{result1, result2, result3, result4} } -func (fake *FakeServiceProvider) Provision(arg1 context.Context, arg2 *varcontext.VarContext) (storage.ServiceInstanceDetails, error) { +func (fake *FakeServiceProvider) Provision(arg1 context.Context, arg2 *varcontext.VarContext) error { fake.provisionMutex.Lock() ret, specificReturn := fake.provisionReturnsOnCall[len(fake.provisionArgsForCall)] fake.provisionArgsForCall = append(fake.provisionArgsForCall, struct { @@ -807,9 +881,9 @@ func (fake *FakeServiceProvider) Provision(arg1 context.Context, arg2 *varcontex return stub(arg1, arg2) } if specificReturn { - return ret.result1, ret.result2 + return ret.result1 } - return fakeReturns.result1, fakeReturns.result2 + return fakeReturns.result1 } func (fake *FakeServiceProvider) ProvisionCallCount() int { @@ -818,7 +892,7 @@ func (fake *FakeServiceProvider) ProvisionCallCount() int { return len(fake.provisionArgsForCall) } -func (fake *FakeServiceProvider) ProvisionCalls(stub func(context.Context, *varcontext.VarContext) (storage.ServiceInstanceDetails, error)) { +func (fake *FakeServiceProvider) ProvisionCalls(stub func(context.Context, *varcontext.VarContext) error) { fake.provisionMutex.Lock() defer fake.provisionMutex.Unlock() fake.ProvisionStub = stub @@ -831,30 +905,27 @@ func (fake *FakeServiceProvider) ProvisionArgsForCall(i int) (context.Context, * return argsForCall.arg1, argsForCall.arg2 } -func (fake *FakeServiceProvider) ProvisionReturns(result1 storage.ServiceInstanceDetails, result2 error) { +func (fake *FakeServiceProvider) ProvisionReturns(result1 error) { fake.provisionMutex.Lock() defer fake.provisionMutex.Unlock() fake.ProvisionStub = nil fake.provisionReturns = struct { - result1 storage.ServiceInstanceDetails - result2 error - }{result1, result2} + result1 error + }{result1} } -func (fake *FakeServiceProvider) ProvisionReturnsOnCall(i int, result1 storage.ServiceInstanceDetails, result2 error) { +func (fake *FakeServiceProvider) ProvisionReturnsOnCall(i int, result1 error) { fake.provisionMutex.Lock() defer fake.provisionMutex.Unlock() fake.ProvisionStub = nil if fake.provisionReturnsOnCall == nil { fake.provisionReturnsOnCall = make(map[int]struct { - result1 storage.ServiceInstanceDetails - result2 error + result1 error }) } fake.provisionReturnsOnCall[i] = struct { - result1 storage.ServiceInstanceDetails - result2 error - }{result1, result2} + result1 error + }{result1} } func (fake *FakeServiceProvider) Unbind(arg1 context.Context, arg2 string, arg3 string, arg4 *varcontext.VarContext) error { @@ -921,7 +992,7 @@ func (fake *FakeServiceProvider) UnbindReturnsOnCall(i int, result1 error) { }{result1} } -func (fake *FakeServiceProvider) Update(arg1 context.Context, arg2 *varcontext.VarContext) (models.ServiceInstanceDetails, error) { +func (fake *FakeServiceProvider) Update(arg1 context.Context, arg2 *varcontext.VarContext) error { fake.updateMutex.Lock() ret, specificReturn := fake.updateReturnsOnCall[len(fake.updateArgsForCall)] fake.updateArgsForCall = append(fake.updateArgsForCall, struct { @@ -936,9 +1007,9 @@ func (fake *FakeServiceProvider) Update(arg1 context.Context, arg2 *varcontext.V return stub(arg1, arg2) } if specificReturn { - return ret.result1, ret.result2 + return ret.result1 } - return fakeReturns.result1, fakeReturns.result2 + return fakeReturns.result1 } func (fake *FakeServiceProvider) UpdateCallCount() int { @@ -947,7 +1018,7 @@ func (fake *FakeServiceProvider) UpdateCallCount() int { return len(fake.updateArgsForCall) } -func (fake *FakeServiceProvider) UpdateCalls(stub func(context.Context, *varcontext.VarContext) (models.ServiceInstanceDetails, error)) { +func (fake *FakeServiceProvider) UpdateCalls(stub func(context.Context, *varcontext.VarContext) error) { fake.updateMutex.Lock() defer fake.updateMutex.Unlock() fake.UpdateStub = stub @@ -960,30 +1031,27 @@ func (fake *FakeServiceProvider) UpdateArgsForCall(i int) (context.Context, *var return argsForCall.arg1, argsForCall.arg2 } -func (fake *FakeServiceProvider) UpdateReturns(result1 models.ServiceInstanceDetails, result2 error) { +func (fake *FakeServiceProvider) UpdateReturns(result1 error) { fake.updateMutex.Lock() defer fake.updateMutex.Unlock() fake.UpdateStub = nil fake.updateReturns = struct { - result1 models.ServiceInstanceDetails - result2 error - }{result1, result2} + result1 error + }{result1} } -func (fake *FakeServiceProvider) UpdateReturnsOnCall(i int, result1 models.ServiceInstanceDetails, result2 error) { +func (fake *FakeServiceProvider) UpdateReturnsOnCall(i int, result1 error) { fake.updateMutex.Lock() defer fake.updateMutex.Unlock() fake.UpdateStub = nil if fake.updateReturnsOnCall == nil { fake.updateReturnsOnCall = make(map[int]struct { - result1 models.ServiceInstanceDetails - result2 error + result1 error }) } fake.updateReturnsOnCall[i] = struct { - result1 models.ServiceInstanceDetails - result2 error - }{result1, result2} + result1 error + }{result1} } func (fake *FakeServiceProvider) UpgradeBindings(arg1 context.Context, arg2 *varcontext.VarContext, arg3 []*varcontext.VarContext) error { @@ -1128,6 +1196,8 @@ func (fake *FakeServiceProvider) Invocations() map[string][][]interface{} { defer fake.checkOperationConstraintsMutex.RUnlock() fake.checkUpgradeAvailableMutex.RLock() defer fake.checkUpgradeAvailableMutex.RUnlock() + fake.clearOperationTypeMutex.RLock() + defer fake.clearOperationTypeMutex.RUnlock() fake.deleteBindingDataMutex.RLock() defer fake.deleteBindingDataMutex.RUnlock() fake.deleteInstanceDataMutex.RLock() diff --git a/pkg/broker/service_provider.go b/pkg/broker/service_provider.go index f28cc0758..60532ceb4 100644 --- a/pkg/broker/service_provider.go +++ b/pkg/broker/service_provider.go @@ -18,7 +18,6 @@ import ( "context" "sync" - "github.com/cloudfoundry/cloud-service-broker/v2/dbservice/models" "github.com/cloudfoundry/cloud-service-broker/v2/internal/storage" "github.com/cloudfoundry/cloud-service-broker/v2/pkg/varcontext" ) @@ -33,10 +32,10 @@ import ( type ServiceProvider interface { // Provision creates the necessary resources that an instance of this service // needs to operate. - Provision(ctx context.Context, provisionContext *varcontext.VarContext) (storage.ServiceInstanceDetails, error) + Provision(ctx context.Context, provisionContext *varcontext.VarContext) error // Update makes necessary updates to resources so they match new desired configuration - Update(ctx context.Context, updateContext *varcontext.VarContext) (models.ServiceInstanceDetails, error) + Update(ctx context.Context, updateContext *varcontext.VarContext) error UpgradeInstance(ctx context.Context, instanceContext *varcontext.VarContext) (*sync.WaitGroup, error) UpgradeBindings(ctx context.Context, instanceContext *varcontext.VarContext, bindingContexts []*varcontext.VarContext) error @@ -57,7 +56,7 @@ type ServiceProvider interface { // If no error and no operationId are returned, then the deprovision is expected to have been completed successfully. Deprovision(ctx context.Context, instanceGUID string, vc *varcontext.VarContext) (*string, error) - PollInstance(ctx context.Context, instanceGUID string) (bool, string, error) + PollInstance(ctx context.Context, instanceGUID string) (bool, string, string, error) GetTerraformOutputs(ctx context.Context, instanceGUID string) (storage.JSONObject, error) @@ -65,6 +64,8 @@ type ServiceProvider interface { DeleteBindingData(ctx context.Context, instanceGUID, bindingID string) error + ClearOperationType(ctx context.Context, instanceGUID string) error + CheckUpgradeAvailable(deploymentID string) error CheckOperationConstraints(deploymentID string, operationType string) error diff --git a/pkg/providers/tf/bind.go b/pkg/providers/tf/bind.go index 286c1053d..076473c20 100644 --- a/pkg/providers/tf/bind.go +++ b/pkg/providers/tf/bind.go @@ -18,11 +18,13 @@ func (provider *TerraformProvider) Bind(ctx context.Context, bindContext *varcon "context": bindContext.ToMap(), }) - tfID, err := provider.create(ctx, bindContext, provider.serviceDefinition.BindSettings, models.BindOperationType) + err := provider.create(ctx, bindContext, provider.serviceDefinition.BindSettings, models.BindOperationType) if err != nil { return nil, fmt.Errorf("error from provider bind: %w", err) } + tfID := bindContext.GetString("tf_id") + if err := provider.Wait(ctx, tfID); err != nil { return nil, fmt.Errorf("error waiting for result: %w", err) } diff --git a/pkg/providers/tf/bind_test.go b/pkg/providers/tf/bind_test.go index 2217542e2..536ceccd2 100644 --- a/pkg/providers/tf/bind_test.go +++ b/pkg/providers/tf/bind_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" + "github.com/cloudfoundry/cloud-service-broker/v2/dbservice/models" "github.com/cloudfoundry/cloud-service-broker/v2/pkg/providers/tf/workspace/workspacefakes" "github.com/cloudfoundry/cloud-service-broker/v2/pkg/providers/tf/workspace" @@ -69,7 +70,7 @@ var _ = Describe("Bind", func() { It("returns the binding output", func() { fakeDeploymentManager.CreateAndSaveDeploymentReturns(deployment, nil) fakeDeploymentManager.GetTerraformDeploymentReturns(deployment, nil) - fakeDeploymentManager.OperationStatusReturns(true, "operation succeeded", nil) + fakeDeploymentManager.OperationStatusReturns(true, "operation succeeded", models.BindOperationType, nil) fakeInvokerBuilder.VersionedTerraformInvokerReturns(fakeDefaultInvoker) fakeDefaultInvoker.ApplyReturns(nil) fakeTerraformWorkspace.OutputsReturns(map[string]any{"username": "some-user"}, nil) @@ -153,7 +154,7 @@ var _ = Describe("Bind", func() { It("returns the error in last operation, if tofu apply fails", func() { fakeDeploymentManager.CreateAndSaveDeploymentReturns(deployment, nil) - fakeDeploymentManager.OperationStatusReturns(true, "operation failed", fmt.Errorf("tofu apply failed")) + fakeDeploymentManager.OperationStatusReturns(true, "operation failed", models.BindOperationType, fmt.Errorf("tofu apply failed")) fakeDeploymentManager.MarkOperationFinishedReturns(errors.New("couldnt do this now")) fakeInvokerBuilder.VersionedTerraformInvokerReturns(fakeDefaultInvoker) fakeDefaultInvoker.ApplyReturns(errors.New("some TF issue happened")) diff --git a/pkg/providers/tf/clear_operation_type.go b/pkg/providers/tf/clear_operation_type.go new file mode 100644 index 000000000..e5edfc923 --- /dev/null +++ b/pkg/providers/tf/clear_operation_type.go @@ -0,0 +1,18 @@ +package tf + +import ( + "context" + + "code.cloudfoundry.org/lager/v3" + "github.com/cloudfoundry/cloud-service-broker/v2/utils/correlation" +) + +func (provider *TerraformProvider) ClearOperationType(ctx context.Context, instanceGUID string) error { + provider.logger.Debug("ClearOperationType", correlation.ID(ctx), lager.Data{ + "instance": instanceGUID, + }) + + tfID := generateTfID(instanceGUID, "") + + return provider.ResetOperationType(tfID) +} diff --git a/pkg/providers/tf/deployment_manager.go b/pkg/providers/tf/deployment_manager.go index 3a3f6c19b..55ae07e47 100644 --- a/pkg/providers/tf/deployment_manager.go +++ b/pkg/providers/tf/deployment_manager.go @@ -6,6 +6,7 @@ import ( "code.cloudfoundry.org/lager/v3" + "github.com/cloudfoundry/cloud-service-broker/v2/dbservice/models" "github.com/cloudfoundry/cloud-service-broker/v2/internal/storage" "github.com/cloudfoundry/cloud-service-broker/v2/pkg/broker" "github.com/cloudfoundry/cloud-service-broker/v2/pkg/featureflags" @@ -86,19 +87,19 @@ func (d *DeploymentManager) MarkOperationFinished(deployment *storage.TerraformD return d.store.RemoveLockFile(deployment.ID) } -func (d *DeploymentManager) OperationStatus(deploymentID string) (bool, string, error) { +func (d *DeploymentManager) OperationStatus(deploymentID string) (bool, string, string, error) { deployment, err := d.store.GetTerraformDeployment(deploymentID) if err != nil { - return true, "", err + return true, "", "", err } switch deployment.LastOperationState { case Succeeded: - return true, deployment.LastOperationMessage, nil + return true, deployment.LastOperationMessage, deployment.LastOperationType, nil case Failed: - return true, deployment.LastOperationMessage, errors.New(deployment.LastOperationMessage) + return true, deployment.LastOperationMessage, deployment.LastOperationType, errors.New(deployment.LastOperationMessage) default: - return false, deployment.LastOperationMessage, nil + return false, deployment.LastOperationMessage, deployment.LastOperationType, nil } } @@ -157,3 +158,13 @@ func (d *DeploymentManager) GetBindingDeployments(deploymentID string) ([]storag } return bindingDeployments, nil } + +func (d *DeploymentManager) ResetOperationType(deploymentID string) error { + deployment, err := d.store.GetTerraformDeployment(deploymentID) + if err != nil { + return err + } + + deployment.LastOperationType = models.ClearOperationType + return d.store.StoreTerraformDeployment(deployment) +} diff --git a/pkg/providers/tf/deployment_manager_test.go b/pkg/providers/tf/deployment_manager_test.go index 30c8d5eb5..7f929c41e 100644 --- a/pkg/providers/tf/deployment_manager_test.go +++ b/pkg/providers/tf/deployment_manager_test.go @@ -5,6 +5,7 @@ import ( "code.cloudfoundry.org/lager/v3/lagertest" + "github.com/cloudfoundry/cloud-service-broker/v2/dbservice/models" "github.com/cloudfoundry/cloud-service-broker/v2/internal/storage" "github.com/cloudfoundry/cloud-service-broker/v2/pkg/broker" "github.com/cloudfoundry/cloud-service-broker/v2/pkg/broker/brokerfakes" @@ -257,10 +258,11 @@ var _ = Describe("DeploymentManager", func() { } fakeStore.GetTerraformDeploymentReturns(existingDeployment, nil) - completed, lastOpMessage, err := deploymentManager.OperationStatus(existingDeploymentID) + completed, lastOpMessage, operationType, err := deploymentManager.OperationStatus(existingDeploymentID) Expect(err).NotTo(HaveOccurred()) Expect(completed).To(BeTrue()) + Expect(operationType).To(Equal("update")) Expect(lastOpMessage).To(Equal("great update")) }) }) @@ -275,10 +277,11 @@ var _ = Describe("DeploymentManager", func() { } fakeStore.GetTerraformDeploymentReturns(existingDeployment, nil) - completed, lastOpMessage, err := deploymentManager.OperationStatus(existingDeploymentID) + completed, lastOpMessage, operationType, err := deploymentManager.OperationStatus(existingDeploymentID) Expect(err).To(MatchError("not so great update")) Expect(completed).To(BeTrue()) + Expect(operationType).To(Equal("update")) Expect(lastOpMessage).To(Equal("not so great update")) }) }) @@ -292,10 +295,11 @@ var _ = Describe("DeploymentManager", func() { } fakeStore.GetTerraformDeploymentReturns(existingDeployment, nil) - completed, lastOpMessage, err := deploymentManager.OperationStatus(existingDeploymentID) + completed, lastOpMessage, operationType, err := deploymentManager.OperationStatus(existingDeploymentID) Expect(err).NotTo(HaveOccurred()) Expect(completed).To(BeFalse()) + Expect(operationType).To(Equal("update")) Expect(lastOpMessage).To(Equal("still doing stuff")) }) }) @@ -303,7 +307,7 @@ var _ = Describe("DeploymentManager", func() { It("fails, when it errors getting the deployment", func() { fakeStore.GetTerraformDeploymentReturns(storage.TerraformDeployment{}, errors.New("cant get it now")) - _, _, err := deploymentManager.OperationStatus(existingDeploymentID) + _, _, _, err := deploymentManager.OperationStatus(existingDeploymentID) Expect(err).To(MatchError("cant get it now")) }) @@ -604,6 +608,36 @@ var _ = Describe("DeploymentManager", func() { }) }) + Describe("ResetOperationType", func() { + var ( + fakeStore brokerfakes.FakeServiceProviderStorage + deploymentManager *tf.DeploymentManager + existingDeployment storage.TerraformDeployment + ) + + const existingDeploymentID = "tf:instance:binding" + + BeforeEach(func() { + fakeStore = brokerfakes.FakeServiceProviderStorage{} + deploymentManager = tf.NewDeploymentManager(&fakeStore, lagertest.NewTestLogger("test")) + existingDeployment = storage.TerraformDeployment{ + ID: existingDeploymentID, + LastOperationType: "validation", + } + }) + + It("it clears the operation type", func() { + fakeStore.GetTerraformDeploymentReturns(existingDeployment, nil) + + err := deploymentManager.ResetOperationType(existingDeploymentID) + + Expect(err).NotTo(HaveOccurred()) + Expect(fakeStore.StoreTerraformDeploymentCallCount()).To(Equal(1)) + actualDeployment := fakeStore.StoreTerraformDeploymentArgsForCall(0) + Expect(actualDeployment.LastOperationType).To(Equal(models.ClearOperationType)) + }) + }) + Describe("GetBindingDeployments", func() { var ( fakeStore brokerfakes.FakeServiceProviderStorage diff --git a/pkg/providers/tf/poll_instance.go b/pkg/providers/tf/poll_instance.go index fb01548d0..8d0a39d5e 100644 --- a/pkg/providers/tf/poll_instance.go +++ b/pkg/providers/tf/poll_instance.go @@ -5,6 +5,6 @@ import ( ) // PollInstance returns the instance status of the backing job. -func (provider *TerraformProvider) PollInstance(_ context.Context, instanceGUID string) (bool, string, error) { +func (provider *TerraformProvider) PollInstance(_ context.Context, instanceGUID string) (bool, string, string, error) { return provider.OperationStatus(generateTfID(instanceGUID, "")) } diff --git a/pkg/providers/tf/poll_instance_test.go b/pkg/providers/tf/poll_instance_test.go index 7e593fd2a..104fdf9dc 100644 --- a/pkg/providers/tf/poll_instance_test.go +++ b/pkg/providers/tf/poll_instance_test.go @@ -17,13 +17,14 @@ var _ = Describe("PollInstance", func() { fakeInvokerBuilder := &tffakes.FakeTerraformInvokerBuilder{} fakeLogger := utils.NewLogger("test") - fakeDeploymentManager.OperationStatusReturns(true, "LO message", nil) + fakeDeploymentManager.OperationStatusReturns(true, "LO message", "update", nil) provider := tf.NewTerraformProvider(executor.TFBinariesContext{}, fakeInvokerBuilder, fakeLogger, tf.TfServiceDefinitionV1{}, fakeDeploymentManager) - finished, message, err := provider.PollInstance(context.TODO(), "instance-guid") + finished, message, operationType, err := provider.PollInstance(context.TODO(), "instance-guid") Expect(err).NotTo(HaveOccurred()) Expect(finished).To(BeTrue()) + Expect(operationType).To(Equal("update")) Expect(message).To(Equal("LO message")) Expect(fakeDeploymentManager.OperationStatusCallCount()).To(Equal(1)) diff --git a/pkg/providers/tf/provider.go b/pkg/providers/tf/provider.go index 6e2eaeae7..9a828a083 100644 --- a/pkg/providers/tf/provider.go +++ b/pkg/providers/tf/provider.go @@ -68,25 +68,25 @@ func (provider *TerraformProvider) VersionedInvoker(version *version.Version) in return provider.VersionedTerraformInvoker(version) } -func (provider *TerraformProvider) create(ctx context.Context, vars *varcontext.VarContext, action TfServiceDefinitionV1Action, operationType string) (string, error) { +func (provider *TerraformProvider) create(ctx context.Context, vars *varcontext.VarContext, action TfServiceDefinitionV1Action, operationType string) error { tfID := vars.GetString("tf_id") if err := vars.Error(); err != nil { - return "", err + return err } newWorkspace, err := workspace.NewWorkspace(vars.ToMap(), action.Template, action.Templates, []workspace.ParameterMapping{}, []string{}, []workspace.ParameterMapping{}) if err != nil { - return tfID, fmt.Errorf("error creating workspace: %w", err) + return fmt.Errorf("error creating workspace: %w", err) } deployment, err := provider.CreateAndSaveDeployment(tfID, newWorkspace) if err != nil { provider.logger.Error("deployment create failed", err) - return tfID, fmt.Errorf("deployment create failed: %w", err) + return fmt.Errorf("deployment create failed: %w", err) } if err := provider.MarkOperationStarted(&deployment, operationType); err != nil { - return tfID, fmt.Errorf("error marking job started: %w", err) + return fmt.Errorf("error marking job started: %w", err) } go func() { @@ -102,7 +102,7 @@ func (provider *TerraformProvider) create(ctx context.Context, vars *varcontext. } }() - return tfID, nil + return nil } func (provider *TerraformProvider) destroy(ctx context.Context, deploymentID string, templateVars map[string]any, operationType string) error { @@ -129,6 +129,7 @@ func (provider *TerraformProvider) destroy(ctx context.Context, deploymentID str tfWorkspace.Instances[0].Configuration = limitedConfig + fmt.Println("provider.MarkOperationStarted") if err := provider.MarkOperationStarted(&deployment, operationType); err != nil { return err } @@ -148,7 +149,7 @@ func (provider *TerraformProvider) Wait(ctx context.Context, id string) error { return nil case <-time.After(1 * time.Second): - isDone, _, err := provider.OperationStatus(id) + isDone, _, _, err := provider.OperationStatus(id) if isDone { return err } @@ -163,8 +164,9 @@ type DeploymentManagerInterface interface { CreateAndSaveDeployment(deploymentID string, workspace *workspace.TerraformWorkspace) (storage.TerraformDeployment, error) MarkOperationStarted(deployment *storage.TerraformDeployment, operationType string) error MarkOperationFinished(deployment *storage.TerraformDeployment, err error) error - OperationStatus(deploymentID string) (bool, string, error) + OperationStatus(deploymentID string) (bool, string, string, error) UpdateWorkspaceHCL(deploymentID string, serviceDefinitionAction TfServiceDefinitionV1Action, templateVars map[string]any) error GetBindingDeployments(deploymentID string) ([]storage.TerraformDeployment, error) DeleteTerraformDeployment(deploymentID string) error + ResetOperationType(deploymentID string) error } diff --git a/pkg/providers/tf/provision.go b/pkg/providers/tf/provision.go index d4c499462..1bdb7a545 100644 --- a/pkg/providers/tf/provision.go +++ b/pkg/providers/tf/provision.go @@ -8,7 +8,6 @@ import ( "code.cloudfoundry.org/lager/v3" "github.com/cloudfoundry/cloud-service-broker/v2/dbservice/models" "github.com/cloudfoundry/cloud-service-broker/v2/internal/steps" - "github.com/cloudfoundry/cloud-service-broker/v2/internal/storage" "github.com/cloudfoundry/cloud-service-broker/v2/pkg/broker" "github.com/cloudfoundry/cloud-service-broker/v2/pkg/providers/tf/invoker" "github.com/cloudfoundry/cloud-service-broker/v2/pkg/providers/tf/workspace" @@ -25,42 +24,36 @@ type ImportResource struct { // Provision creates the necessary resources that an instance of this service // needs to operate. -func (provider *TerraformProvider) Provision(ctx context.Context, provisionContext *varcontext.VarContext) (storage.ServiceInstanceDetails, error) { +func (provider *TerraformProvider) Provision(ctx context.Context, provisionContext *varcontext.VarContext) error { provider.logger.Debug("terraform-provision", correlation.ID(ctx), lager.Data{ "context": provisionContext.ToMap(), }) - var ( - tfID string - err error - ) + var err error switch provider.serviceDefinition.ProvisionSettings.IsTfImport(provisionContext) { case true: - tfID, err = provider.importCreate(ctx, provisionContext, provider.serviceDefinition.ProvisionSettings) + err = provider.importCreate(ctx, provisionContext, provider.serviceDefinition.ProvisionSettings) default: - tfID, err = provider.create(ctx, provisionContext, provider.serviceDefinition.ProvisionSettings, models.ProvisionOperationType) + err = provider.create(ctx, provisionContext, provider.serviceDefinition.ProvisionSettings, models.ProvisionOperationType) } if err != nil { - return storage.ServiceInstanceDetails{}, err + return err } - return storage.ServiceInstanceDetails{ - OperationGUID: tfID, - OperationType: models.ProvisionOperationType, - }, nil + return nil } -func (provider *TerraformProvider) importCreate(ctx context.Context, vars *varcontext.VarContext, action TfServiceDefinitionV1Action) (string, error) { +func (provider *TerraformProvider) importCreate(ctx context.Context, vars *varcontext.VarContext, action TfServiceDefinitionV1Action) error { varsMap := vars.ToMap() importParams, err := validateImportParams(action.ImportVariables, varsMap) if err != nil { - return "", err + return err } tfID := vars.GetString("tf_id") if err := vars.Error(); err != nil { - return "", err + return err } newWorkspace, err := workspace.NewWorkspace( @@ -71,17 +64,17 @@ func (provider *TerraformProvider) importCreate(ctx context.Context, vars *varco action.ImportParametersToDelete, evaluateParametersToAdd(action.ImportParametersToAdd)) if err != nil { - return tfID, fmt.Errorf("error creating workspace: %w", err) + return fmt.Errorf("error creating workspace: %w", err) } deployment, err := provider.CreateAndSaveDeployment(tfID, newWorkspace) if err != nil { provider.logger.Error("deployment create failed", err) - return tfID, fmt.Errorf("deployment create failed: %w", err) + return fmt.Errorf("deployment create failed: %w", err) } if err := provider.MarkOperationStarted(&deployment, models.ProvisionOperationType); err != nil { - return tfID, fmt.Errorf("error marking job started: %w", err) + return fmt.Errorf("error marking job started: %w", err) } go func() { @@ -122,7 +115,7 @@ func (provider *TerraformProvider) importCreate(ctx context.Context, vars *varco } }() - return tfID, nil + return nil } func validateImportParams(importVariables []broker.ImportVariable, varsMap map[string]any) ([]ImportResource, error) { diff --git a/pkg/providers/tf/provision_test.go b/pkg/providers/tf/provision_test.go index 8aaef029d..75abe6ed8 100644 --- a/pkg/providers/tf/provision_test.go +++ b/pkg/providers/tf/provision_test.go @@ -67,11 +67,9 @@ var _ = Describe("Provision", func() { fakeInvokerBuilder.VersionedTerraformInvokerReturns(fakeDefaultInvoker) provider := tf.NewTerraformProvider(executor.TFBinariesContext{}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - actualInstanceDetails, err := provider.Provision(context.TODO(), provisionContext) + err := provider.Provision(context.TODO(), provisionContext) Expect(err).NotTo(HaveOccurred()) - Expect(actualInstanceDetails.OperationGUID).To(Equal(expectedTfID)) - Expect(actualInstanceDetails.OperationType).To(Equal("provision")) By("checking the new saved deployment") Expect(fakeDeploymentManager.CreateAndSaveDeploymentCallCount()).To(Equal(1)) @@ -104,7 +102,7 @@ var _ = Describe("Provision", func() { provider := tf.NewTerraformProvider(executor.TFBinariesContext{}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - _, err = provider.Provision(context.TODO(), provisionContext) + err = provider.Provision(context.TODO(), provisionContext) Expect(err.Error()).To(ContainSubstring(`missing value for key "tf_id"`)) }) @@ -119,7 +117,7 @@ var _ = Describe("Provision", func() { provider := tf.NewTerraformProvider(executor.TFBinariesContext{}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - _, err := provider.Provision(context.TODO(), provisionContext) + err := provider.Provision(context.TODO(), provisionContext) Expect(err).To(MatchError(`error creating workspace: :1,17-17: Invalid block definition; Either a quoted string block label or an opening brace ("{") is expected here.`)) }) @@ -128,7 +126,7 @@ var _ = Describe("Provision", func() { fakeDeploymentManager.CreateAndSaveDeploymentReturns(storage.TerraformDeployment{}, errors.New("cant save now")) provider := tf.NewTerraformProvider(executor.TFBinariesContext{}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - _, err := provider.Provision(context.TODO(), provisionContext) + err := provider.Provision(context.TODO(), provisionContext) Expect(err).To(MatchError("deployment create failed: cant save now")) }) @@ -138,7 +136,7 @@ var _ = Describe("Provision", func() { fakeDeploymentManager.MarkOperationStartedReturns(errors.New("couldnt do this now")) provider := tf.NewTerraformProvider(executor.TFBinariesContext{}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - _, err := provider.Provision(context.TODO(), provisionContext) + err := provider.Provision(context.TODO(), provisionContext) Expect(err).To(MatchError("error marking job started: couldnt do this now")) }) @@ -149,7 +147,7 @@ var _ = Describe("Provision", func() { fakeDefaultInvoker.ApplyReturns(errors.New("some TF issue happened")) provider := tf.NewTerraformProvider(executor.TFBinariesContext{}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - _, err := provider.Provision(context.TODO(), provisionContext) + err := provider.Provision(context.TODO(), provisionContext) Expect(err).NotTo(HaveOccurred()) By("checking TF apply has been called") @@ -206,11 +204,9 @@ var _ = Describe("Provision", func() { fakeInvokerBuilder.VersionedTerraformInvokerReturns(fakeDefaultInvoker) provider := tf.NewTerraformProvider(executor.TFBinariesContext{}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - actualInstanceDetails, err := provider.Provision(context.TODO(), provisionContext) + err := provider.Provision(context.TODO(), provisionContext) Expect(err).NotTo(HaveOccurred()) - Expect(actualInstanceDetails.OperationGUID).To(Equal(expectedTfID)) - Expect(actualInstanceDetails.OperationType).To(Equal("provision")) By("checking the new saved deployment") Expect(fakeDeploymentManager.CreateAndSaveDeploymentCallCount()).To(Equal(1)) @@ -249,7 +245,7 @@ var _ = Describe("Provision", func() { provider := tf.NewTerraformProvider(executor.TFBinariesContext{}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - _, err = provider.Provision(context.TODO(), pc) + err = provider.Provision(context.TODO(), pc) Expect(err).To(MatchError("must provide values for all import parameters: import_input")) }) @@ -260,7 +256,7 @@ var _ = Describe("Provision", func() { provider := tf.NewTerraformProvider(executor.TFBinariesContext{}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - _, err = provider.Provision(context.TODO(), pc) + err = provider.Provision(context.TODO(), pc) Expect(err.Error()).To(ContainSubstring(`missing value for key "tf_id"`)) }) @@ -298,7 +294,7 @@ var _ = Describe("Provision", func() { provider := tf.NewTerraformProvider(executor.TFBinariesContext{}, fakeInvokerBuilder, fakeLogger, sd, fakeDeploymentManager) - _, err := provider.Provision(context.TODO(), provisionContext) + err := provider.Provision(context.TODO(), provisionContext) Expect(err.Error()).To(ContainSubstring(`error creating workspace: :1,17-17: Invalid block definition; Either a quoted string block label or an opening brace ("{") is expected here.`)) }) @@ -307,7 +303,7 @@ var _ = Describe("Provision", func() { fakeDeploymentManager.CreateAndSaveDeploymentReturns(storage.TerraformDeployment{}, errors.New("cant save now")) provider := tf.NewTerraformProvider(executor.TFBinariesContext{}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - _, err := provider.Provision(context.TODO(), provisionContext) + err := provider.Provision(context.TODO(), provisionContext) Expect(err).To(MatchError("deployment create failed: cant save now")) }) @@ -317,7 +313,7 @@ var _ = Describe("Provision", func() { fakeDeploymentManager.MarkOperationStartedReturns(errors.New("couldnt do this now")) provider := tf.NewTerraformProvider(executor.TFBinariesContext{}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - _, err := provider.Provision(context.TODO(), provisionContext) + err := provider.Provision(context.TODO(), provisionContext) Expect(err).To(MatchError("error marking job started: couldnt do this now")) }) @@ -329,7 +325,7 @@ var _ = Describe("Provision", func() { fakeDefaultInvoker.ImportReturns(errors.New("some TF import issue happened")) provider := tf.NewTerraformProvider(executor.TFBinariesContext{}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - _, err := provider.Provision(context.TODO(), provisionContext) + err := provider.Provision(context.TODO(), provisionContext) Expect(err).NotTo(HaveOccurred()) By("checking TF import has been called") @@ -347,7 +343,7 @@ var _ = Describe("Provision", func() { fakeDefaultInvoker.ShowReturns("", errors.New("some TF show issue happened")) provider := tf.NewTerraformProvider(executor.TFBinariesContext{}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - _, err := provider.Provision(context.TODO(), provisionContext) + err := provider.Provision(context.TODO(), provisionContext) Expect(err).NotTo(HaveOccurred()) By("checking TF show has been called") @@ -364,7 +360,7 @@ var _ = Describe("Provision", func() { fakeDefaultInvoker.PlanReturns(executor.ExecutionOutput{}, errors.New("some TF plan issue happened")) provider := tf.NewTerraformProvider(executor.TFBinariesContext{}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - _, err := provider.Provision(context.TODO(), provisionContext) + err := provider.Provision(context.TODO(), provisionContext) Expect(err).NotTo(HaveOccurred()) By("checking TF plan has been called") @@ -380,7 +376,7 @@ var _ = Describe("Provision", func() { fakeDefaultInvoker.ApplyReturns(errors.New("some TF apply issue happened")) provider := tf.NewTerraformProvider(executor.TFBinariesContext{}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - _, err := provider.Provision(context.TODO(), provisionContext) + err := provider.Provision(context.TODO(), provisionContext) Expect(err).NotTo(HaveOccurred()) By("checking TF apply has been called") diff --git a/pkg/providers/tf/tffakes/fake_deployment_manager_interface.go b/pkg/providers/tf/tffakes/fake_deployment_manager_interface.go index 60a2a8c6a..9a2ea75a0 100644 --- a/pkg/providers/tf/tffakes/fake_deployment_manager_interface.go +++ b/pkg/providers/tf/tffakes/fake_deployment_manager_interface.go @@ -85,7 +85,7 @@ type FakeDeploymentManagerInterface struct { markOperationStartedReturnsOnCall map[int]struct { result1 error } - OperationStatusStub func(string) (bool, string, error) + OperationStatusStub func(string) (bool, string, string, error) operationStatusMutex sync.RWMutex operationStatusArgsForCall []struct { arg1 string @@ -93,12 +93,25 @@ type FakeDeploymentManagerInterface struct { operationStatusReturns struct { result1 bool result2 string - result3 error + result3 string + result4 error } operationStatusReturnsOnCall map[int]struct { result1 bool result2 string - result3 error + result3 string + result4 error + } + ResetOperationTypeStub func(string) error + resetOperationTypeMutex sync.RWMutex + resetOperationTypeArgsForCall []struct { + arg1 string + } + resetOperationTypeReturns struct { + result1 error + } + resetOperationTypeReturnsOnCall map[int]struct { + result1 error } UpdateWorkspaceHCLStub func(string, tf.TfServiceDefinitionV1Action, map[string]any) error updateWorkspaceHCLMutex sync.RWMutex @@ -495,7 +508,7 @@ func (fake *FakeDeploymentManagerInterface) MarkOperationStartedReturnsOnCall(i }{result1} } -func (fake *FakeDeploymentManagerInterface) OperationStatus(arg1 string) (bool, string, error) { +func (fake *FakeDeploymentManagerInterface) OperationStatus(arg1 string) (bool, string, string, error) { fake.operationStatusMutex.Lock() ret, specificReturn := fake.operationStatusReturnsOnCall[len(fake.operationStatusArgsForCall)] fake.operationStatusArgsForCall = append(fake.operationStatusArgsForCall, struct { @@ -509,9 +522,9 @@ func (fake *FakeDeploymentManagerInterface) OperationStatus(arg1 string) (bool, return stub(arg1) } if specificReturn { - return ret.result1, ret.result2, ret.result3 + return ret.result1, ret.result2, ret.result3, ret.result4 } - return fakeReturns.result1, fakeReturns.result2, fakeReturns.result3 + return fakeReturns.result1, fakeReturns.result2, fakeReturns.result3, fakeReturns.result4 } func (fake *FakeDeploymentManagerInterface) OperationStatusCallCount() int { @@ -520,7 +533,7 @@ func (fake *FakeDeploymentManagerInterface) OperationStatusCallCount() int { return len(fake.operationStatusArgsForCall) } -func (fake *FakeDeploymentManagerInterface) OperationStatusCalls(stub func(string) (bool, string, error)) { +func (fake *FakeDeploymentManagerInterface) OperationStatusCalls(stub func(string) (bool, string, string, error)) { fake.operationStatusMutex.Lock() defer fake.operationStatusMutex.Unlock() fake.OperationStatusStub = stub @@ -533,18 +546,19 @@ func (fake *FakeDeploymentManagerInterface) OperationStatusArgsForCall(i int) st return argsForCall.arg1 } -func (fake *FakeDeploymentManagerInterface) OperationStatusReturns(result1 bool, result2 string, result3 error) { +func (fake *FakeDeploymentManagerInterface) OperationStatusReturns(result1 bool, result2 string, result3 string, result4 error) { fake.operationStatusMutex.Lock() defer fake.operationStatusMutex.Unlock() fake.OperationStatusStub = nil fake.operationStatusReturns = struct { result1 bool result2 string - result3 error - }{result1, result2, result3} + result3 string + result4 error + }{result1, result2, result3, result4} } -func (fake *FakeDeploymentManagerInterface) OperationStatusReturnsOnCall(i int, result1 bool, result2 string, result3 error) { +func (fake *FakeDeploymentManagerInterface) OperationStatusReturnsOnCall(i int, result1 bool, result2 string, result3 string, result4 error) { fake.operationStatusMutex.Lock() defer fake.operationStatusMutex.Unlock() fake.OperationStatusStub = nil @@ -552,14 +566,77 @@ func (fake *FakeDeploymentManagerInterface) OperationStatusReturnsOnCall(i int, fake.operationStatusReturnsOnCall = make(map[int]struct { result1 bool result2 string - result3 error + result3 string + result4 error }) } fake.operationStatusReturnsOnCall[i] = struct { result1 bool result2 string - result3 error - }{result1, result2, result3} + result3 string + result4 error + }{result1, result2, result3, result4} +} + +func (fake *FakeDeploymentManagerInterface) ResetOperationType(arg1 string) error { + fake.resetOperationTypeMutex.Lock() + ret, specificReturn := fake.resetOperationTypeReturnsOnCall[len(fake.resetOperationTypeArgsForCall)] + fake.resetOperationTypeArgsForCall = append(fake.resetOperationTypeArgsForCall, struct { + arg1 string + }{arg1}) + stub := fake.ResetOperationTypeStub + fakeReturns := fake.resetOperationTypeReturns + fake.recordInvocation("ResetOperationType", []interface{}{arg1}) + fake.resetOperationTypeMutex.Unlock() + if stub != nil { + return stub(arg1) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeDeploymentManagerInterface) ResetOperationTypeCallCount() int { + fake.resetOperationTypeMutex.RLock() + defer fake.resetOperationTypeMutex.RUnlock() + return len(fake.resetOperationTypeArgsForCall) +} + +func (fake *FakeDeploymentManagerInterface) ResetOperationTypeCalls(stub func(string) error) { + fake.resetOperationTypeMutex.Lock() + defer fake.resetOperationTypeMutex.Unlock() + fake.ResetOperationTypeStub = stub +} + +func (fake *FakeDeploymentManagerInterface) ResetOperationTypeArgsForCall(i int) string { + fake.resetOperationTypeMutex.RLock() + defer fake.resetOperationTypeMutex.RUnlock() + argsForCall := fake.resetOperationTypeArgsForCall[i] + return argsForCall.arg1 +} + +func (fake *FakeDeploymentManagerInterface) ResetOperationTypeReturns(result1 error) { + fake.resetOperationTypeMutex.Lock() + defer fake.resetOperationTypeMutex.Unlock() + fake.ResetOperationTypeStub = nil + fake.resetOperationTypeReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeDeploymentManagerInterface) ResetOperationTypeReturnsOnCall(i int, result1 error) { + fake.resetOperationTypeMutex.Lock() + defer fake.resetOperationTypeMutex.Unlock() + fake.ResetOperationTypeStub = nil + if fake.resetOperationTypeReturnsOnCall == nil { + fake.resetOperationTypeReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.resetOperationTypeReturnsOnCall[i] = struct { + result1 error + }{result1} } func (fake *FakeDeploymentManagerInterface) UpdateWorkspaceHCL(arg1 string, arg2 tf.TfServiceDefinitionV1Action, arg3 map[string]any) error { @@ -642,6 +719,8 @@ func (fake *FakeDeploymentManagerInterface) Invocations() map[string][][]interfa defer fake.markOperationStartedMutex.RUnlock() fake.operationStatusMutex.RLock() defer fake.operationStatusMutex.RUnlock() + fake.resetOperationTypeMutex.RLock() + defer fake.resetOperationTypeMutex.RUnlock() fake.updateWorkspaceHCLMutex.RLock() defer fake.updateWorkspaceHCLMutex.RUnlock() copiedInvocations := map[string][][]interface{}{} diff --git a/pkg/providers/tf/unbind_test.go b/pkg/providers/tf/unbind_test.go index ed24c5bec..9919f197d 100644 --- a/pkg/providers/tf/unbind_test.go +++ b/pkg/providers/tf/unbind_test.go @@ -79,7 +79,7 @@ var _ = Describe("Unbind", func() { It("destroys the binding", func() { fakeInvokerBuilder.VersionedTerraformInvokerReturns(fakeDefaultInvoker) fakeDeploymentManager.GetTerraformDeploymentReturns(deployment, nil) - fakeDeploymentManager.OperationStatusReturns(true, "operation succeeded", nil) + fakeDeploymentManager.OperationStatusReturns(true, "operation succeeded", "bind", nil) provider := tf.NewTerraformProvider(executor.TFBinariesContext{DefaultTfVersion: version.Must(version.NewVersion("1"))}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) @@ -141,7 +141,7 @@ var _ = Describe("Unbind", func() { fakeDeploymentManager.MarkOperationStartedReturns(nil) fakeInvokerBuilder.VersionedTerraformInvokerReturns(fakeDefaultInvoker) fakeDefaultInvoker.DestroyReturns(errors.New(expectedError)) - fakeDeploymentManager.OperationStatusReturns(true, "", errors.New(expectedError)) + fakeDeploymentManager.OperationStatusReturns(true, "", "destroy", errors.New(expectedError)) provider := tf.NewTerraformProvider(executor.TFBinariesContext{DefaultTfVersion: version.Must(version.NewVersion("1"))}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) diff --git a/pkg/providers/tf/update.go b/pkg/providers/tf/update.go index d3295c3fc..87166ab6a 100644 --- a/pkg/providers/tf/update.go +++ b/pkg/providers/tf/update.go @@ -12,33 +12,33 @@ import ( ) // Update makes necessary updates to resources, so they match new desired configuration -func (provider *TerraformProvider) Update(ctx context.Context, updateContext *varcontext.VarContext) (models.ServiceInstanceDetails, error) { +func (provider *TerraformProvider) Update(ctx context.Context, updateContext *varcontext.VarContext) error { provider.logger.Debug("update", correlation.ID(ctx), lager.Data{ "context": updateContext.ToMap(), }) if provider.serviceDefinition.ProvisionSettings.IsTfImport(updateContext) { - return models.ServiceInstanceDetails{}, fmt.Errorf("cannot update to subsume plan\n\nFor OpsMan Tile users see documentation here: https://via.vmw.com/ENs4\n\nFor Open Source users deployed via 'cf push' see documentation here: https://via.vmw.com/ENw4") + return fmt.Errorf("cannot update to subsume plan\n\nFor OpsMan Tile users see documentation here: https://via.vmw.com/ENs4\n\nFor Open Source users deployed via 'cf push' see documentation here: https://via.vmw.com/ENw4") } tfID := updateContext.GetString("tf_id") if err := updateContext.Error(); err != nil { - return models.ServiceInstanceDetails{}, err + return err } if err := provider.UpdateWorkspaceHCL(tfID, provider.serviceDefinition.ProvisionSettings, updateContext.ToMap()); err != nil { - return models.ServiceInstanceDetails{}, err + return err } deployment, err := provider.GetTerraformDeployment(tfID) if err != nil { - return models.ServiceInstanceDetails{}, err + return err } workspace := deployment.Workspace if err := provider.MarkOperationStarted(&deployment, models.UpdateOperationType); err != nil { - return models.ServiceInstanceDetails{}, err + return err } go func() { @@ -52,8 +52,5 @@ func (provider *TerraformProvider) Update(ctx context.Context, updateContext *va _ = provider.MarkOperationFinished(&deployment, err) }() - return models.ServiceInstanceDetails{ - OperationID: tfID, - OperationType: models.UpdateOperationType, - }, nil + return nil } diff --git a/pkg/providers/tf/update_test.go b/pkg/providers/tf/update_test.go index b3d0377a6..74bd9e78f 100644 --- a/pkg/providers/tf/update_test.go +++ b/pkg/providers/tf/update_test.go @@ -63,7 +63,7 @@ var _ = Describe("Update", func() { provider := tf.NewTerraformProvider(executor.TFBinariesContext{DefaultTfVersion: newVersion(tfVersion)}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - _, err := provider.Update(context.TODO(), varContext) + err := provider.Update(context.TODO(), varContext) Expect(err).NotTo(HaveOccurred()) Eventually(operationWasFinishedForDeployment(fakeDeploymentManager)).Should(Equal(deployment)) Expect(operationWasFinishedWithError(fakeDeploymentManager)()).To(BeNil()) @@ -81,7 +81,7 @@ var _ = Describe("Update", func() { fakeWorkspace.OutputsReturns(map[string]any{"status": "status from terraform"}, nil) provider := tf.NewTerraformProvider(executor.TFBinariesContext{DefaultTfVersion: newVersion(tfVersion)}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - _, err := provider.Update(context.TODO(), varContext) + err := provider.Update(context.TODO(), varContext) Expect(err).To(Succeed()) Eventually(operationWasFinishedForDeployment(fakeDeploymentManager)).Should(Equal(deployment)) Expect(operationWasFinishedWithError(fakeDeploymentManager)()).To(BeNil()) @@ -96,7 +96,7 @@ var _ = Describe("Update", func() { fakeDefaultInvoker.ApplyReturns(genericError) provider := tf.NewTerraformProvider(executor.TFBinariesContext{DefaultTfVersion: newVersion(tfVersion)}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - _, err := provider.Update(context.TODO(), varContext) + err := provider.Update(context.TODO(), varContext) Expect(err).To(Succeed()) Eventually(operationWasFinishedForDeployment(fakeDeploymentManager)).Should(Equal(deployment)) @@ -115,7 +115,7 @@ var _ = Describe("Update", func() { provider := tf.NewTerraformProvider(executor.TFBinariesContext{}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - _, err = provider.Update(context.TODO(), varContext) + err = provider.Update(context.TODO(), varContext) Expect(err).To(MatchError("cannot update to subsume plan\n\nFor OpsMan Tile users see documentation here: https://via.vmw.com/ENs4\n\nFor Open Source users deployed via 'cf push' see documentation here: https://via.vmw.com/ENw4")) }) }) @@ -127,7 +127,7 @@ var _ = Describe("Update", func() { provider := tf.NewTerraformProvider(executor.TFBinariesContext{}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - _, err = provider.Update(context.TODO(), varContext) + err = provider.Update(context.TODO(), varContext) Expect(err).To(MatchError(`1 error(s) occurred: missing value for key "tf_id"`)) }) }) @@ -138,7 +138,7 @@ var _ = Describe("Update", func() { provider := tf.NewTerraformProvider(executor.TFBinariesContext{}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - _, err := provider.Update(context.TODO(), varContext) + err := provider.Update(context.TODO(), varContext) Expect(err).To(MatchError(genericError)) }) }) @@ -148,7 +148,7 @@ var _ = Describe("Update", func() { fakeDeploymentManager.GetTerraformDeploymentReturns(storage.TerraformDeployment{}, genericError) provider := tf.NewTerraformProvider(executor.TFBinariesContext{}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - _, err := provider.Update(context.TODO(), varContext) + err := provider.Update(context.TODO(), varContext) Expect(err).To(MatchError(genericError)) }) }) @@ -159,7 +159,7 @@ var _ = Describe("Update", func() { fakeDeploymentManager.MarkOperationStartedReturns(genericError) provider := tf.NewTerraformProvider(executor.TFBinariesContext{}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - _, err := provider.Update(context.TODO(), varContext) + err := provider.Update(context.TODO(), varContext) Expect(err).To(MatchError(genericError)) }) @@ -175,7 +175,7 @@ var _ = Describe("Update", func() { provider := tf.NewTerraformProvider(executor.TFBinariesContext{DefaultTfVersion: newVersion(tfVersion)}, fakeInvokerBuilder, fakeLogger, fakeServiceDefinition, fakeDeploymentManager) - _, err := provider.Update(context.TODO(), varContext) + err := provider.Update(context.TODO(), varContext) Expect(err).NotTo(HaveOccurred()) Eventually(operationWasFinishedForDeployment(fakeDeploymentManager)).Should(Equal(deployment)) From f8bb85e3393a5bd5f6d4dec3254359b52122532f Mon Sep 17 00:00:00 2001 From: George Blue Date: Fri, 4 Oct 2024 09:03:20 +0100 Subject: [PATCH 4/4] chore: drop unused Location and URL database fields (#1108) --- dbservice/migrations.go | 17 +++++++++++------ dbservice/models/historical_db.go | 2 -- internal/storage/service_instance_details.go | 6 ------ .../storage/service_instance_details_test.go | 16 ---------------- 4 files changed, 11 insertions(+), 30 deletions(-) diff --git a/dbservice/migrations.go b/dbservice/migrations.go index 21d3f8740..7b7fcd084 100644 --- a/dbservice/migrations.go +++ b/dbservice/migrations.go @@ -23,7 +23,7 @@ import ( "github.com/cloudfoundry/cloud-service-broker/v2/dbservice/models" ) -const numMigrations = 18 +const numMigrations = 17 // RunMigrations runs schema migrations on the provided service broker database to get it up to date func RunMigrations(db *gorm.DB) error { @@ -129,11 +129,16 @@ func RunMigrations(db *gorm.DB) error { } migrations[16] = func() error { - return db.Migrator().DropColumn(&models.ServiceInstanceDetailsV3{}, "operation_type") - } - - migrations[17] = func() error { - return db.Migrator().DropColumn(&models.ServiceInstanceDetailsV3{}, "operation_id") + if err := db.Migrator().DropColumn(&models.ServiceInstanceDetailsV4{}, "operation_type"); err != nil { + return err + } + if err := db.Migrator().DropColumn(&models.ServiceInstanceDetailsV4{}, "operation_id"); err != nil { + return err + } + if err := db.Migrator().DropColumn(&models.ServiceInstanceDetailsV4{}, "location"); err != nil { + return err + } + return db.Migrator().DropColumn(&models.ServiceInstanceDetailsV4{}, "url") } var lastMigrationNumber = -1 diff --git a/dbservice/models/historical_db.go b/dbservice/models/historical_db.go index 7752e6540..58bb2a7b9 100644 --- a/dbservice/models/historical_db.go +++ b/dbservice/models/historical_db.go @@ -170,8 +170,6 @@ type ServiceInstanceDetailsV4 struct { DeletedAt *time.Time Name string - Location string - URL string OtherDetails []byte `gorm:"type:blob"` ServiceID string diff --git a/internal/storage/service_instance_details.go b/internal/storage/service_instance_details.go index b0793f2b4..f2c1c0e9d 100644 --- a/internal/storage/service_instance_details.go +++ b/internal/storage/service_instance_details.go @@ -10,8 +10,6 @@ import ( type ServiceInstanceDetails struct { GUID string Name string - Location string - URL string Outputs JSONObject ServiceGUID string PlanGUID string @@ -31,8 +29,6 @@ func (s *Storage) StoreServiceInstanceDetails(d ServiceInstanceDetails) error { } m.Name = d.Name - m.Location = d.Location - m.URL = d.URL m.OtherDetails = encoded m.ServiceID = d.ServiceGUID m.PlanID = d.PlanGUID @@ -84,8 +80,6 @@ func (s *Storage) GetServiceInstanceDetails(guid string) (ServiceInstanceDetails return ServiceInstanceDetails{ GUID: guid, Name: receiver.Name, - Location: receiver.Location, - URL: receiver.URL, Outputs: decoded, ServiceGUID: receiver.ServiceID, PlanGUID: receiver.PlanID, diff --git a/internal/storage/service_instance_details_test.go b/internal/storage/service_instance_details_test.go index c29789a23..83a15bd07 100644 --- a/internal/storage/service_instance_details_test.go +++ b/internal/storage/service_instance_details_test.go @@ -15,8 +15,6 @@ var _ = Describe("ServiceInstanceDetails", func() { err := store.StoreServiceInstanceDetails(storage.ServiceInstanceDetails{ GUID: "fake-guid", Name: "fake-name", - Location: "fake-location", - URL: "fake-url", Outputs: map[string]any{"foo": "bar"}, ServiceGUID: "fake-service-guid", PlanGUID: "fake-plan-guid", @@ -29,8 +27,6 @@ var _ = Describe("ServiceInstanceDetails", func() { Expect(db.Find(&receiver).Error).NotTo(HaveOccurred()) Expect(receiver.ID).To(Equal("fake-guid")) Expect(receiver.Name).To(Equal("fake-name")) - Expect(receiver.Location).To(Equal("fake-location")) - Expect(receiver.URL).To(Equal("fake-url")) Expect(receiver.OtherDetails).To(Equal([]byte(`{"encrypted":{"foo":"bar"}}`))) Expect(receiver.ServiceID).To(Equal("fake-service-guid")) Expect(receiver.PlanID).To(Equal("fake-plan-guid")) @@ -56,8 +52,6 @@ var _ = Describe("ServiceInstanceDetails", func() { err := store.StoreServiceInstanceDetails(storage.ServiceInstanceDetails{ GUID: "fake-id-1", Name: "fake-name", - Location: "fake-location", - URL: "fake-url", Outputs: map[string]any{"foo": "bar"}, ServiceGUID: "fake-service-guid", PlanGUID: "fake-plan-guid", @@ -70,8 +64,6 @@ var _ = Describe("ServiceInstanceDetails", func() { Expect(db.Where(`id = "fake-id-1"`).Find(&receiver).Error).NotTo(HaveOccurred()) Expect(receiver.ID).To(Equal("fake-id-1")) Expect(receiver.Name).To(Equal("fake-name")) - Expect(receiver.Location).To(Equal("fake-location")) - Expect(receiver.URL).To(Equal("fake-url")) Expect(receiver.OtherDetails).To(Equal([]byte(`{"encrypted":{"foo":"bar"}}`))) Expect(receiver.ServiceID).To(Equal("fake-service-guid")) Expect(receiver.PlanID).To(Equal("fake-plan-guid")) @@ -91,8 +83,6 @@ var _ = Describe("ServiceInstanceDetails", func() { Expect(err).NotTo(HaveOccurred()) Expect(r.GUID).To(Equal("fake-id-2")) - Expect(r.Location).To(Equal("fake-location-2")) - Expect(r.URL).To(Equal("fake-url-2")) Expect(r.Outputs).To(Equal(storage.JSONObject{"decrypted": map[string]any{"foo": "bar-2"}})) Expect(r.ServiceGUID).To(Equal("fake-service-id-2")) Expect(r.PlanGUID).To(Equal("fake-plan-id-2")) @@ -194,8 +184,6 @@ func addFakeServiceInstanceDetails() { Expect(db.Create(&models.ServiceInstanceDetails{ ID: "fake-id-1", Name: "fake-name-1", - Location: "fake-location-1", - URL: "fake-url-1", OtherDetails: []byte(`{"foo":"bar-1"}`), ServiceID: "fake-service-id-1", PlanID: "fake-plan-id-1", @@ -205,8 +193,6 @@ func addFakeServiceInstanceDetails() { Expect(db.Create(&models.ServiceInstanceDetails{ ID: "fake-id-2", Name: "fake-name-2", - Location: "fake-location-2", - URL: "fake-url-2", OtherDetails: []byte(`{"foo":"bar-2"}`), ServiceID: "fake-service-id-2", PlanID: "fake-plan-id-2", @@ -216,8 +202,6 @@ func addFakeServiceInstanceDetails() { Expect(db.Create(&models.ServiceInstanceDetails{ ID: "fake-id-3", Name: "fake-name-3", - Location: "fake-location-3", - URL: "fake-url-3", OtherDetails: []byte(`{"foo":"bar-3"}`), ServiceID: "fake-service-id-3", PlanID: "fake-plan-id-3",