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)
cloudfoundry · Oct 3, 2024 · eabd3d6 · eabd3d6
1 parent 69a15f4
commit eabd3d6
Show file tree

Hide file tree

Showing 31 changed files with 461 additions and 261 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
 }