From 6ee255162e6657d352b992aaff10165bf13fbf8c Mon Sep 17 00:00:00 2001 From: George Blue Date: Thu, 15 Feb 2024 14:14:47 +0000 Subject: [PATCH] test: reproduce failed cleanups We have an issue where we fail to clean up if there is an issue with the Terraform state. Here we introduce some tests that capture that behaviour. By reproducing the error in a test, we can test-drive a solution and have better confidence that we have fixed the issue. [#185835561](https://www.pivotaltracker.com/story/show/185835561) --- integrationtest/binding_cleanup_test.go | 71 +++++++++++++++++++ .../fixtures/binding-cleanup/fake-bad-bind.tf | 4 ++ .../binding-cleanup/fake-bad-service.yml | 27 +++++++ .../binding-cleanup/fake-good-bind.tf | 5 ++ .../binding-cleanup/fake-good-service.yml | 27 +++++++ .../binding-cleanup/fake-provision.tf | 3 + .../fixtures/binding-cleanup/manifest.yml | 19 +++++ .../fake-string-provision.tf | 4 ++ .../provision-cleanup/fake-string-service.yml | 20 ++++++ .../fixtures/provision-cleanup/manifest.yml | 18 +++++ integrationtest/provision_cleanup_test.go | 65 +++++++++++++++++ internal/testdrive/broker_create_binding.go | 4 +- internal/testdrive/broker_provision.go | 8 +-- 13 files changed, 268 insertions(+), 7 deletions(-) create mode 100644 integrationtest/binding_cleanup_test.go create mode 100644 integrationtest/fixtures/binding-cleanup/fake-bad-bind.tf create mode 100644 integrationtest/fixtures/binding-cleanup/fake-bad-service.yml create mode 100644 integrationtest/fixtures/binding-cleanup/fake-good-bind.tf create mode 100644 integrationtest/fixtures/binding-cleanup/fake-good-service.yml create mode 100644 integrationtest/fixtures/binding-cleanup/fake-provision.tf create mode 100644 integrationtest/fixtures/binding-cleanup/manifest.yml create mode 100644 integrationtest/fixtures/provision-cleanup/fake-string-provision.tf create mode 100644 integrationtest/fixtures/provision-cleanup/fake-string-service.yml create mode 100644 integrationtest/fixtures/provision-cleanup/manifest.yml create mode 100644 integrationtest/provision_cleanup_test.go diff --git a/integrationtest/binding_cleanup_test.go b/integrationtest/binding_cleanup_test.go new file mode 100644 index 000000000..83f62c70c --- /dev/null +++ b/integrationtest/binding_cleanup_test.go @@ -0,0 +1,71 @@ +package integrationtest_test + +import ( + "encoding/json" + "fmt" + + "github.com/cloudfoundry/cloud-service-broker/dbservice/models" + "github.com/cloudfoundry/cloud-service-broker/integrationtest/packer" + "github.com/cloudfoundry/cloud-service-broker/internal/testdrive" + "github.com/cloudfoundry/cloud-service-broker/pkg/providers/tf/workspace" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Binding Cleanup", func() { + const ( + badServiceOfferingGUID = "81b4ebaa-cc08-11ee-bd34-0f8547e428e9" + badServicePlanGUID = "9ff671e2-cc08-11ee-bb95-3facf049ac9d" + goodServiceOfferingGUID = "7779a92a-cc0b-11ee-85c4-4b4aa590c58a" + goodServicePlanGUID = "911ce91e-cc0b-11ee-a5e8-33dbc3f841a1" + ) + + var ( + brokerpak string + broker *testdrive.Broker + ) + + BeforeEach(func() { + brokerpak = must(packer.BuildBrokerpak(csb, fixtures("binding-cleanup"))) + broker = must(testdrive.StartBroker(csb, brokerpak, database, testdrive.WithOutputs(GinkgoWriter, GinkgoWriter))) + + DeferCleanup(func() { + Expect(broker.Stop()).To(Succeed()) + cleanup(brokerpak) + }) + }) + + It("does not need to clean up after a binding failed cleanly", func() { + By("provisioning successfully") + instance := must(broker.Provision(badServiceOfferingGUID, badServicePlanGUID)) + + By("failing to bind") + binding, err := broker.CreateBinding(instance) + Expect(err).To(MatchError(ContainSubstring("error performing bind: error waiting for result: bind failed: Error: Missing required argument"))) + + By("seeing an HTTP 410 Gone error") + Expect(broker.DeleteBinding(instance, binding.GUID)).To(MatchError(ContainSubstring("unexpected status code 410"))) + }) + + // This test captures an incorrect behavior that we want to fix + It("fails to clean up after a corrupted binding", func() { + By("provisioning successfully") + instance := must(broker.Provision(goodServiceOfferingGUID, goodServicePlanGUID)) + + By("binding successfully") + binding := must(broker.CreateBinding(instance)) + + By("corrupting the state as if terraform had been killed") + invalidWorkspace := must(json.Marshal(workspace.TerraformWorkspace{State: []byte(`{"foo`)})) // Base64-encoded truncated JSON + Expect( + dbConn.Model(&models.TerraformDeployment{}). + Where("id = ?", fmt.Sprintf("tf:%s:%s", instance.GUID, binding.GUID)). + Update("workspace", invalidWorkspace). + Error, + ).To(Succeed()) + + By("failing to clean up the binding") + Expect(broker.DeleteBinding(instance, binding.GUID)).To(MatchError(ContainSubstring("failed to unbind: invalid workspace state unexpected end of JSON input"))) + }) +}) diff --git a/integrationtest/fixtures/binding-cleanup/fake-bad-bind.tf b/integrationtest/fixtures/binding-cleanup/fake-bad-bind.tf new file mode 100644 index 000000000..9de1f243c --- /dev/null +++ b/integrationtest/fixtures/binding-cleanup/fake-bad-bind.tf @@ -0,0 +1,4 @@ +// Fails as required "length" parameter is missing +resource "random_string" "random" {} + +output bind_output { value = random_string.random.result } \ No newline at end of file diff --git a/integrationtest/fixtures/binding-cleanup/fake-bad-service.yml b/integrationtest/fixtures/binding-cleanup/fake-bad-service.yml new file mode 100644 index 000000000..e390a1cc7 --- /dev/null +++ b/integrationtest/fixtures/binding-cleanup/fake-bad-service.yml @@ -0,0 +1,27 @@ +version: 1 +name: fake-bad-service +id: 81b4ebaa-cc08-11ee-bd34-0f8547e428e9 +description: description +display_name: Fake +image_url: https://example.com/icon.jpg +documentation_url: https://example.com +support_url: https://example.com/support.html +plans: +- name: standard + id: 9ff671e2-cc08-11ee-bb95-3facf049ac9d + description: Standard plan + display_name: Standard +provision: + template_refs: + main: fake-provision.tf + outputs: + - field_name: provision_output + type: string + details: provision output +bind: + template_refs: + main: fake-bad-bind.tf + outputs: + - field_name: bind_output + type: string + details: bind output diff --git a/integrationtest/fixtures/binding-cleanup/fake-good-bind.tf b/integrationtest/fixtures/binding-cleanup/fake-good-bind.tf new file mode 100644 index 000000000..fcbed15be --- /dev/null +++ b/integrationtest/fixtures/binding-cleanup/fake-good-bind.tf @@ -0,0 +1,5 @@ +resource "random_string" "random" { + length = 10 +} + +output bind_output { value = random_string.random.result } \ No newline at end of file diff --git a/integrationtest/fixtures/binding-cleanup/fake-good-service.yml b/integrationtest/fixtures/binding-cleanup/fake-good-service.yml new file mode 100644 index 000000000..0a6b29dea --- /dev/null +++ b/integrationtest/fixtures/binding-cleanup/fake-good-service.yml @@ -0,0 +1,27 @@ +version: 1 +name: fake-good-service +id: 7779a92a-cc0b-11ee-85c4-4b4aa590c58a +description: description +display_name: Fake +image_url: https://example.com/icon.jpg +documentation_url: https://example.com +support_url: https://example.com/support.html +plans: +- name: standard + id: 911ce91e-cc0b-11ee-a5e8-33dbc3f841a1 + description: Standard plan + display_name: Standard +provision: + template_refs: + main: fake-provision.tf + outputs: + - field_name: provision_output + type: string + details: provision output +bind: + template_refs: + main: fake-good-bind.tf + outputs: + - field_name: bind_output + type: string + details: bind output diff --git a/integrationtest/fixtures/binding-cleanup/fake-provision.tf b/integrationtest/fixtures/binding-cleanup/fake-provision.tf new file mode 100644 index 000000000..adbe4074b --- /dev/null +++ b/integrationtest/fixtures/binding-cleanup/fake-provision.tf @@ -0,0 +1,3 @@ +resource "random_uuid" "random" {} + +output provision_output { value = random_uuid.random.result } \ No newline at end of file diff --git a/integrationtest/fixtures/binding-cleanup/manifest.yml b/integrationtest/fixtures/binding-cleanup/manifest.yml new file mode 100644 index 000000000..a712de76d --- /dev/null +++ b/integrationtest/fixtures/binding-cleanup/manifest.yml @@ -0,0 +1,19 @@ +packversion: 1 +name: fake-brokerpak +version: 0.1.0 +metadata: + author: noone@nowhere.com +platforms: +- os: linux + arch: amd64 +- os: darwin + arch: amd64 +terraform_binaries: +- name: terraform + version: 1.5.7 + source: https://github.com/hashicorp/terraform/archive/v1.5.7.zip +- name: terraform-provider-random + version: 3.1.0 +service_definitions: +- fake-good-service.yml +- fake-bad-service.yml diff --git a/integrationtest/fixtures/provision-cleanup/fake-string-provision.tf b/integrationtest/fixtures/provision-cleanup/fake-string-provision.tf new file mode 100644 index 000000000..c53142595 --- /dev/null +++ b/integrationtest/fixtures/provision-cleanup/fake-string-provision.tf @@ -0,0 +1,4 @@ +// Fails as required "length" parameter is missing +resource "random_string" "random" {} + +output provision_output { value = random_string.random.result } \ No newline at end of file diff --git a/integrationtest/fixtures/provision-cleanup/fake-string-service.yml b/integrationtest/fixtures/provision-cleanup/fake-string-service.yml new file mode 100644 index 000000000..eb55fb8b8 --- /dev/null +++ b/integrationtest/fixtures/provision-cleanup/fake-string-service.yml @@ -0,0 +1,20 @@ +version: 1 +name: fake-string-service +id: cfeda8d0-cbf3-11ee-be53-73f17d1c612b +description: description +display_name: Fake +image_url: https://example.com/icon.jpg +documentation_url: https://example.com +support_url: https://example.com/support.html +plans: +- name: standard + id: d8fbab66-cbf3-11ee-ab90-d7299e1fcf96 + description: Standard plan + display_name: Standard +provision: + template_refs: + main: fake-string-provision.tf + outputs: + - field_name: provision_output + type: string + details: provision output \ No newline at end of file diff --git a/integrationtest/fixtures/provision-cleanup/manifest.yml b/integrationtest/fixtures/provision-cleanup/manifest.yml new file mode 100644 index 000000000..ddace4617 --- /dev/null +++ b/integrationtest/fixtures/provision-cleanup/manifest.yml @@ -0,0 +1,18 @@ +packversion: 1 +name: fake-brokerpak +version: 0.1.0 +metadata: + author: noone@nowhere.com +platforms: +- os: linux + arch: amd64 +- os: darwin + arch: amd64 +terraform_binaries: +- name: terraform + version: 1.5.7 + source: https://github.com/hashicorp/terraform/archive/v1.5.7.zip +- name: terraform-provider-random + version: 3.1.0 +service_definitions: +- fake-string-service.yml diff --git a/integrationtest/provision_cleanup_test.go b/integrationtest/provision_cleanup_test.go new file mode 100644 index 000000000..1102f25de --- /dev/null +++ b/integrationtest/provision_cleanup_test.go @@ -0,0 +1,65 @@ +package integrationtest_test + +import ( + "encoding/json" + "fmt" + + "github.com/cloudfoundry/cloud-service-broker/dbservice/models" + "github.com/cloudfoundry/cloud-service-broker/integrationtest/packer" + "github.com/cloudfoundry/cloud-service-broker/internal/testdrive" + "github.com/cloudfoundry/cloud-service-broker/pkg/providers/tf/workspace" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Provision Cleanup", func() { + const ( + serviceOfferingGUID = "cfeda8d0-cbf3-11ee-be53-73f17d1c612b" + servicePlanGUID = "d8fbab66-cbf3-11ee-ab90-d7299e1fcf96" + ) + + var ( + brokerpak string + broker *testdrive.Broker + ) + + BeforeEach(func() { + brokerpak = must(packer.BuildBrokerpak(csb, fixtures("provision-cleanup"))) + broker = must(testdrive.StartBroker(csb, brokerpak, database, testdrive.WithOutputs(GinkgoWriter, GinkgoWriter))) + + DeferCleanup(func() { + Expect(broker.Stop()).To(Succeed()) + cleanup(brokerpak) + }) + }) + + // This test captures an incorrect behavior that we want to fix + It("fails to clean up after a provision failed with empty state", func() { + By("failing to provision") + instance, err := broker.Provision(serviceOfferingGUID, servicePlanGUID) + Expect(err).To(MatchError("provision failed with state: failed")) + + By("failing to clean up") + Expect(broker.Deprovision(instance)).To(MatchError(ContainSubstring("failed to delete: workspace state not generated"))) + }) + + // This test captures an incorrect behavior that we want to fix + It("fails to clean up after a provision failed with corrupted state", func() { + By("failing to provision") + instance, err := broker.Provision(serviceOfferingGUID, servicePlanGUID) + Expect(err).To(MatchError("provision failed with state: failed")) + + By("corrupting the state as if terraform had been killed") + invalidWorkspace := must(json.Marshal(workspace.TerraformWorkspace{State: []byte(`{"foo`)})) // Base64-encoded truncated JSON + Expect( + dbConn.Model(&models.TerraformDeployment{}). + Where("id = ?", fmt.Sprintf("tf:%s:", instance.GUID)). + Update("workspace", invalidWorkspace). + Error, + ).To(Succeed()) + + By("failing to clean up") + Expect(broker.Deprovision(instance)).To(MatchError(ContainSubstring("failed to delete: invalid workspace state unexpected end of JSON input"))) + }) +}) diff --git a/internal/testdrive/broker_create_binding.go b/internal/testdrive/broker_create_binding.go index c85f95ac4..3d4a71375 100644 --- a/internal/testdrive/broker_create_binding.go +++ b/internal/testdrive/broker_create_binding.go @@ -35,9 +35,9 @@ func (b *Broker) CreateBinding(s ServiceInstance, opts ...CreateBindingOption) ( bindResponse = b.Client.Bind(s.GUID, cfg.guid, s.ServiceOfferingGUID, s.ServicePlanGUID, uuid.New(), cfg.params) switch { case bindResponse.Error != nil: - return ServiceBinding{}, bindResponse.Error + return ServiceBinding{GUID: cfg.guid}, bindResponse.Error case bindResponse.StatusCode != http.StatusCreated: - return ServiceBinding{}, &UnexpectedStatusError{StatusCode: bindResponse.StatusCode, ResponseBody: bindResponse.ResponseBody} + return ServiceBinding{GUID: cfg.guid}, &UnexpectedStatusError{StatusCode: bindResponse.StatusCode, ResponseBody: bindResponse.ResponseBody} default: return ServiceBinding{ GUID: cfg.guid, diff --git a/internal/testdrive/broker_provision.go b/internal/testdrive/broker_provision.go index 8bc5ed010..027e088f1 100644 --- a/internal/testdrive/broker_provision.go +++ b/internal/testdrive/broker_provision.go @@ -49,21 +49,19 @@ func (b *Broker) Provision(serviceOfferingGUID, servicePlanGUID string, opts ... case err != nil: return err case state != domain.Succeeded: - return fmt.Errorf("provison failed with state: %s", state) + return fmt.Errorf("provision failed with state: %s", state) default: return nil } }, ) - if err != nil { - return ServiceInstance{}, err - } + // If it fails, we still return the GUIDs for cleanup return ServiceInstance{ GUID: cfg.guid, ServicePlanGUID: servicePlanGUID, ServiceOfferingGUID: serviceOfferingGUID, - }, nil + }, err } func WithProvisionParams(params any) ProvisionOption {