Repository: crossplaneio/crossplane
Pull request #1112 (2019-12-05 06:26:36)

Backport #1091 to release-0.5 branch
### Description of your changes

This PR backports the commits from #1091 into the `release-0.5` branch.  Note that commit https://github.com/crossplaneio/crossplane/pull/1091/commits/0c922ed1d4c9f9e03c17416c9042bd548af23b59 is not included because it only touched the design doc that doesn't exist in the `release-0.5` branch.

### Checklist
<!--
Please run through the below readiness checklist. The first two items are
relevant to every Crossplane pull request.
-->
I have:
- [x] Run `make reviewable` to ensure this PR is ready for review.
- [x] Ensured this PR contains a neat, self documenting set of commits.
- [x] Updated any relevant [documentation] and [examples].
- [x] Reported all new error conditions into the log or as an event, as
  appropriate.

For more about what we believe makes a pull request complete, see our
[definition of done].

[documentation]: https://github.com/crossplaneio/crossplane/tree/master/docs
[examples]: https://github.com/crossplaneio/crossplane/tree/master/cluster/examples
[definition of done]: https://github.com/crossplaneio/crossplane/tree/master/design/one-pager-definition-of-done.md



Patch:
diff --git a/cmd/crossplane/main.go b/cmd/crossplane/main.go
index de46df68f..94616eb06 100644
--- a/cmd/crossplane/main.go
+++ b/cmd/crossplane/main.go
@@ -23,6 +23,7 @@ import (
 
 	"github.com/spf13/afero"
 	"gopkg.in/alecthomas/kingpin.v2"
+	apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client/config"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
@@ -159,5 +160,9 @@ func addToScheme(scheme *runtime.Scheme) error {
 		return err
 	}
 
+	if err := apiextensionsv1beta1.AddToScheme(scheme); err != nil {
+		return err
+	}
+
 	return nil
 }
diff --git a/pkg/controller/stacks/install/installjob.go b/pkg/controller/stacks/install/installjob.go
index 91825b2a6..082d0ee6c 100644
--- a/pkg/controller/stacks/install/installjob.go
+++ b/pkg/controller/stacks/install/installjob.go
@@ -40,6 +40,16 @@ import (
 	"github.com/crossplaneio/crossplane/pkg/stacks"
 )
 
+// Labels used to track ownership across namespaces and scopes.
+const (
+	labelParentGroup     = "core.crossplane.io/parent-group"
+	labelParentVersion   = "core.crossplane.io/parent-version"
+	labelParentKind      = "core.crossplane.io/parent-kind"
+	labelParentNamespace = "core.crossplane.io/parent-namespace"
+	labelParentName      = "core.crossplane.io/parent-name"
+	labelParentUID       = "core.crossplane.io/parent-uid"
+)
+
 var (
 	jobBackoff                = int32(0)
 	registryDirName           = ".registry"
@@ -219,6 +229,9 @@ func (jc *stackInstallJobCompleter) readPodLogs(namespace, name string) (*bytes.
 	return b, nil
 }
 
+// createJobOutputObject names, labels, sets ownership, and creates resources
+// resulting from a StackInstall or ClusterStackInstall. These expected resources
+// are currently CRD and Stack objects.
 func (jc *stackInstallJobCompleter) createJobOutputObject(ctx context.Context, obj *unstructured.Unstructured,
 	i v1alpha1.StackInstaller, job *batchv1.Job) error {
 
@@ -227,21 +240,36 @@ func (jc *stackInstallJobCompleter) createJobOutputObject(ctx context.Context, o
 		return nil
 	}
 
+	// when the current object is a Stack object, make sure the name and namespace are
+	// set to match the current StackInstall (if they haven't already been set). Also,
+	// set the owner reference of the Stack to be the StackInstall.
 	if isStackObject(obj) {
-		// the current object is a Stack object, make sure the name and namespace are
-		// set to match the current StackInstall (if they haven't already been set)
 		if obj.GetName() == "" {
 			obj.SetName(i.GetName())
 		}
 		if obj.GetNamespace() == "" {
 			obj.SetNamespace(i.GetNamespace())
 		}
+
+		obj.SetOwnerReferences([]metav1.OwnerReference{
+			meta.AsOwner(meta.ReferenceTo(i, i.GroupVersionKind())),
+		})
 	}
 
-	// set an owner reference on the object
-	obj.SetOwnerReferences([]metav1.OwnerReference{
-		meta.AsOwner(meta.ReferenceTo(i, i.GroupVersionKind())),
-	})
+	// We want to clean up any installed CRDS when we're deleted. We can't rely
+	// on garbage collection because a namespaced object (StackInstall) can't
+	// own a cluster scoped object (CustomResourceDefinition), so we use labels
+	// instead.
+	gvk := i.GroupVersionKind()
+	labels := map[string]string{
+		labelParentGroup:     gvk.Group,
+		labelParentVersion:   gvk.Version,
+		labelParentKind:      gvk.Kind,
+		labelParentNamespace: i.GetNamespace(),
+		labelParentName:      i.GetName(),
+		labelParentUID:       string(i.GetUID()),
+	}
+	meta.AddLabels(obj, labels)
 
 	// TODO(displague) pass/inject a controller specific logger
 	log.V(logging.Debug).Info(
@@ -251,7 +279,13 @@ func (jc *stackInstallJobCompleter) createJobOutputObject(ctx context.Context, o
 		"namespace", obj.GetNamespace(),
 		"apiVersion", obj.GetAPIVersion(),
 		"kind", obj.GetKind(),
-		"ownerRefs", obj.GetOwnerReferences())
+		"parentGroup", labels[labelParentGroup],
+		"parentVersion", labels[labelParentVersion],
+		"parentKind", labels[labelParentKind],
+		"parentName", labels[labelParentName],
+		"parentNamespace", labels[labelParentNamespace],
+		"parentUID", labels[labelParentUID],
+	)
 	if err := jc.client.Create(ctx, obj); err != nil && !kerrors.IsAlreadyExists(err) {
 		return errors.Wrapf(err, "failed to create object %s from job output %s", obj.GetName(), job.Name)
 	}
diff --git a/pkg/controller/stacks/install/stackinstall.go b/pkg/controller/stacks/install/stackinstall.go
index 0670b0ac8..9ea3f4042 100644
--- a/pkg/controller/stacks/install/stackinstall.go
+++ b/pkg/controller/stacks/install/stackinstall.go
@@ -25,6 +25,7 @@ import (
 
 	batchv1 "k8s.io/api/batch/v1"
 	corev1 "k8s.io/api/core/v1"
+	apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/client-go/kubernetes"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -36,6 +37,7 @@ import (
 	runtimev1alpha1 "github.com/crossplaneio/crossplane-runtime/apis/core/v1alpha1"
 	"github.com/crossplaneio/crossplane-runtime/pkg/logging"
 	"github.com/crossplaneio/crossplane-runtime/pkg/meta"
+	runtimeresource "github.com/crossplaneio/crossplane-runtime/pkg/resource"
 	"github.com/crossplaneio/crossplane/apis/stacks/v1alpha1"
 	stacks "github.com/crossplaneio/crossplane/pkg/stacks"
 )
@@ -43,6 +45,7 @@ import (
 const (
 	reconcileTimeout      = 1 * time.Minute
 	requeueAfterOnSuccess = 10 * time.Second
+	installFinalizer      = "finalizer.stackinstall.crossplane.io"
 )
 
 var (
@@ -113,6 +116,10 @@ func (r *Reconciler) Reconcile(req reconcile.Request) (reconcile.Result, error)
 
 	handler := r.factory.newHandler(ctx, stackInstaller, r.kube, r.kubeclient, executorinfo)
 
+	if meta.WasDeleted(stackInstaller) {
+		return handler.delete(ctx)
+	}
+
 	return handler.sync(ctx)
 }
 
@@ -121,6 +128,7 @@ type handler interface {
 	sync(context.Context) (reconcile.Result, error)
 	create(context.Context) (reconcile.Result, error)
 	update(context.Context) (reconcile.Result, error)
+	delete(context.Context) (reconcile.Result, error)
 }
 
 // stackInstallHandler is a concrete implementation of the handler interface
@@ -169,6 +177,12 @@ func (h *stackInstallHandler) sync(ctx context.Context) (reconcile.Result, error
 // that the Stack does not yet exist, so the caller should confirm that before calling.
 func (h *stackInstallHandler) create(ctx context.Context) (reconcile.Result, error) {
 	h.ext.SetConditions(runtimev1alpha1.Creating())
+
+	meta.AddFinalizer(h.ext, installFinalizer)
+	if err := h.kube.Update(ctx, h.ext); err != nil {
+		return fail(ctx, h.kube, h.ext, err)
+	}
+
 	jobRef := h.ext.InstallJob()
 
 	if jobRef == nil {
@@ -237,6 +251,38 @@ func (h *stackInstallHandler) update(ctx context.Context) (reconcile.Result, err
 	return reconcile.Result{}, nil
 }
 
+// delete performs clean up (finalizer) actions when a StackInstall is being deleted.
+// This function ensures that all the resources (e.g., CRDs) that this StackInstall owns
+// are also cleaned up.
+func (h *stackInstallHandler) delete(ctx context.Context) (reconcile.Result, error) {
+	gvk := h.ext.GroupVersionKind()
+	labels := map[string]string{
+		labelParentGroup:     gvk.Group,
+		labelParentVersion:   gvk.Version,
+		labelParentKind:      gvk.Kind,
+		labelParentNamespace: h.ext.GetNamespace(),
+		labelParentName:      h.ext.GetName(),
+		labelParentUID:       string(h.ext.GetUID()),
+	}
+	list := &apiextensionsv1beta1.CustomResourceDefinitionList{}
+	if err := h.kube.List(ctx, list, client.MatchingLabels(labels)); err != nil {
+		return fail(ctx, h.kube, h.ext, err)
+	}
+
+	for i := range list.Items {
+		if err := h.kube.Delete(ctx, &list.Items[i]); runtimeresource.IgnoreNotFound(err) != nil {
+			return fail(ctx, h.kube, h.ext, err)
+		}
+	}
+
+	meta.RemoveFinalizer(h.ext, installFinalizer)
+	if err := h.kube.Update(ctx, h.ext); err != nil {
+		return fail(ctx, h.kube, h.ext, err)
+	}
+
+	return reconcile.Result{}, nil
+}
+
 // ************************************************************************************************
 // Helper functions
 // ************************************************************************************************


Test patch:
diff --git a/pkg/controller/stacks/install/installjob_test.go b/pkg/controller/stacks/install/installjob_test.go
index 75632ccd5..2f6929cea 100644
--- a/pkg/controller/stacks/install/installjob_test.go
+++ b/pkg/controller/stacks/install/installjob_test.go
@@ -21,6 +21,7 @@ import (
 	"context"
 	"io"
 	"io/ioutil"
+	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -28,11 +29,14 @@ import (
 	batchv1 "k8s.io/api/batch/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/yaml"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	runtimev1alpha1 "github.com/crossplaneio/crossplane-runtime/apis/core/v1alpha1"
+	"github.com/crossplaneio/crossplane-runtime/pkg/meta"
 	"github.com/crossplaneio/crossplane-runtime/pkg/test"
 	"github.com/crossplaneio/crossplane/apis/stacks/v1alpha1"
 	"github.com/crossplaneio/crossplane/pkg/stacks"
@@ -134,6 +138,22 @@ spec:
 status:
  Conditions: null
 `
+
+	crdRaw = `---
+apiVersion: apiextensions.k8s.io/v1beta1
+kind: CustomResourceDefinition
+metadata:
+  name: mytypes.samples.upbound.io
+spec:
+  group: samples.upbound.io
+  names:
+  kind: Mytype
+  listKind: MytypeList
+  plural: mytypes
+  singular: mytype
+  scope: Namespaced
+  version: v1alpha1
+`
 )
 
 var (
@@ -170,6 +190,30 @@ func job(jm ...jobModifier) *batchv1.Job {
 	return j
 }
 
+// unstructuredObj modifiers
+type unstructuredObjModifier func(*unstructured.Unstructured)
+
+// withUnstructuredObjLabels modifies an existing unstructured object with the given labels
+func withUnstructuredObjLabels(labels map[string]string) unstructuredObjModifier {
+	return func(u *unstructured.Unstructured) {
+		meta.AddLabels(u, labels)
+	}
+}
+
+// unstructuredObj creates a new default unstructured object (derived from the crdRaw const)
+func unstructuredObj(uom ...unstructuredObjModifier) *unstructured.Unstructured {
+	r := strings.NewReader(crdRaw)
+	d := yaml.NewYAMLOrJSONDecoder(r, 4096)
+	obj := &unstructured.Unstructured{}
+	d.Decode(&obj)
+
+	for _, m := range uom {
+		m(obj)
+	}
+
+	return obj
+}
+
 type mockJobCompleter struct {
 	MockHandleJobCompletion func(ctx context.Context, i v1alpha1.StackInstaller, job *batchv1.Job) error
 }
@@ -200,8 +244,6 @@ func (m *mockReadCloser) Close() (err error) {
 }
 
 func TestHandleJobCompletion(t *testing.T) {
-	errBoom := errors.New("boom")
-
 	type want struct {
 		ext *v1alpha1.StackInstall
 		err error
@@ -384,6 +426,7 @@ func TestCreate(t *testing.T) {
 			handler: &stackInstallHandler{
 				kube: &test.MockClient{
 					MockCreate:       func(ctx context.Context, obj runtime.Object, _ ...client.CreateOption) error { return nil },
+					MockUpdate:       func(ctx context.Context, obj runtime.Object, _ ...client.UpdateOption) error { return nil },
 					MockStatusUpdate: func(ctx context.Context, obj runtime.Object, _ ...client.UpdateOption) error { return nil },
 				},
 				executorInfo: &stacks.ExecutorInfo{Image: stackPackageImage},
@@ -393,6 +436,7 @@ func TestCreate(t *testing.T) {
 				result: requeueOnSuccess,
 				err:    nil,
 				ext: resource(
+					withFinalizers(installFinalizer),
 					withConditions(runtimev1alpha1.Creating(), runtimev1alpha1.ReconcileSuccess()),
 					withInstallJob(&corev1.ObjectReference{Name: resourceName, Namespace: namespace}),
 				),
@@ -406,6 +450,7 @@ func TestCreate(t *testing.T) {
 						// GET Job returns an uncompleted job
 						return nil
 					},
+					MockUpdate:       func(ctx context.Context, obj runtime.Object, _ ...client.UpdateOption) error { return nil },
 					MockStatusUpdate: func(ctx context.Context, obj runtime.Object, _ ...client.UpdateOption) error { return nil },
 				},
 				ext: resource(
@@ -415,6 +460,7 @@ func TestCreate(t *testing.T) {
 				result: requeueOnSuccess,
 				err:    nil,
 				ext: resource(
+					withFinalizers(installFinalizer),
 					withConditions(runtimev1alpha1.Creating(), runtimev1alpha1.ReconcileSuccess()),
 					withInstallJob(&corev1.ObjectReference{Name: resourceName, Namespace: namespace}),
 				),
@@ -429,6 +475,7 @@ func TestCreate(t *testing.T) {
 						*obj.(*batchv1.Job) = *(job(withJobConditions(batchv1.JobComplete, "")))
 						return nil
 					},
+					MockUpdate:       func(ctx context.Context, obj runtime.Object, _ ...client.UpdateOption) error { return nil },
 					MockStatusUpdate: func(ctx context.Context, obj runtime.Object, _ ...client.UpdateOption) error { return nil },
 				},
 				jobCompleter: &mockJobCompleter{
@@ -442,6 +489,7 @@ func TestCreate(t *testing.T) {
 				result: requeueOnSuccess,
 				err:    nil,
 				ext: resource(
+					withFinalizers(installFinalizer),
 					withConditions(runtimev1alpha1.Available(), runtimev1alpha1.ReconcileSuccess()),
 					withInstallJob(&corev1.ObjectReference{Name: resourceName, Namespace: namespace}),
 				),
@@ -456,6 +504,7 @@ func TestCreate(t *testing.T) {
 						*obj.(*batchv1.Job) = *(job(withJobConditions(batchv1.JobFailed, "mock job failure message")))
 						return nil
 					},
+					MockUpdate:       func(ctx context.Context, obj runtime.Object, _ ...client.UpdateOption) error { return nil },
 					MockStatusUpdate: func(ctx context.Context, obj runtime.Object, _ ...client.UpdateOption) error { return nil },
 				},
 				jobCompleter: &mockJobCompleter{
@@ -469,6 +518,7 @@ func TestCreate(t *testing.T) {
 				result: resultRequeue,
 				err:    nil,
 				ext: resource(
+					withFinalizers(installFinalizer),
 					withConditions(
 						runtimev1alpha1.Creating(),
 						runtimev1alpha1.ReconcileError(errors.New("mock job failure message")),
@@ -498,3 +548,92 @@ func TestCreate(t *testing.T) {
 		})
 	}
 }
+
+func TestCreateJobOutputObject(t *testing.T) {
+	wantLabels := map[string]string{
+		labelParentGroup:     "stacks.crossplane.io",
+		labelParentVersion:   "v1alpha1",
+		labelParentKind:      "StackInstall",
+		labelParentNamespace: namespace,
+		labelParentName:      resourceName,
+		labelParentUID:       uidString,
+	}
+
+	type want struct {
+		err error
+		obj *unstructured.Unstructured
+	}
+
+	tests := []struct {
+		name           string
+		jobCompleter   *stackInstallJobCompleter
+		stackInstaller *v1alpha1.StackInstall
+		job            *batchv1.Job
+		obj            *unstructured.Unstructured
+		want           want
+	}{
+		{
+			name: "NilObj",
+			jobCompleter: &stackInstallJobCompleter{
+				client: &test.MockClient{
+					MockCreate: func(ctx context.Context, obj runtime.Object, _ ...client.CreateOption) error { return nil },
+				},
+			},
+			stackInstaller: resource(),
+			job:            job(),
+			obj:            nil,
+			want: want{
+				err: nil,
+				obj: nil,
+			},
+		},
+		{
+			name: "CreateError",
+			jobCompleter: &stackInstallJobCompleter{
+				client: &test.MockClient{
+					MockCreate: func(ctx context.Context, obj runtime.Object, _ ...client.CreateOption) error {
+						return errBoom
+					},
+				},
+			},
+			stackInstaller: resource(),
+			job:            job(),
+			obj:            unstructuredObj(),
+			want: want{
+				err: errors.Wrapf(errBoom, "failed to create object mytypes.samples.upbound.io from job output cool-stackinstall"),
+				obj: unstructuredObj(withUnstructuredObjLabels(wantLabels)),
+			},
+		},
+		{
+			name: "CreateSuccess",
+			jobCompleter: &stackInstallJobCompleter{
+				client: &test.MockClient{
+					MockCreate: func(ctx context.Context, obj runtime.Object, _ ...client.CreateOption) error { return nil },
+				},
+			},
+			stackInstaller: resource(),
+			job:            job(),
+			obj:            unstructuredObj(),
+			want: want{
+				err: nil,
+				obj: unstructuredObj(withUnstructuredObjLabels(wantLabels)),
+			},
+		},
+	}
+
+	ctx := context.Background()
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotErr := tt.jobCompleter.createJobOutputObject(ctx, tt.obj, tt.stackInstaller, tt.job)
+
+			if diff := cmp.Diff(tt.want.err, gotErr, test.EquateErrors()); diff != "" {
+				t.Errorf("createJobOutputObject(): -want error, +got error:\n%s", diff)
+			}
+
+			if diff := cmp.Diff(tt.want.obj, tt.obj, test.EquateConditions()); diff != "" {
+				t.Errorf("createJobOutputObject(): -want obj, +got obj:\n%v", diff)
+			}
+		})
+	}
+}
diff --git a/pkg/controller/stacks/install/stackinstall_test.go b/pkg/controller/stacks/install/stackinstall_test.go
index 2d3f3133d..423ce82ec 100644
--- a/pkg/controller/stacks/install/stackinstall_test.go
+++ b/pkg/controller/stacks/install/stackinstall_test.go
@@ -19,10 +19,12 @@ package install
 import (
 	"context"
 	"testing"
+	"time"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
+	apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -42,13 +44,15 @@ import (
 
 const (
 	namespace         = "cool-namespace"
-	uid               = types.UID("definitely-a-uuid")
+	uidString         = "definitely-a-uuid"
+	uid               = types.UID(uidString)
 	resourceName      = "cool-stackinstall"
 	stackPackageImage = "cool/stack-package:rad"
 )
 
 var (
-	ctx = context.Background()
+	ctx     = context.Background()
+	errBoom = errors.New("boom")
 )
 
 func init() {
@@ -61,10 +65,19 @@ var _ reconcile.Reconciler = &Reconciler{}
 // Resource modifiers
 type resourceModifier func(v1alpha1.StackInstaller)
 
+func withFinalizers(finalizers ...string) resourceModifier {
+	return func(r v1alpha1.StackInstaller) { r.SetFinalizers(finalizers) }
+}
 func withConditions(c ...runtimev1alpha1.Condition) resourceModifier {
 	return func(r v1alpha1.StackInstaller) { r.SetConditions(c...) }
 }
 
+func withDeletionTimestamp(t time.Time) resourceModifier {
+	return func(r v1alpha1.StackInstaller) {
+		r.SetDeletionTimestamp(&metav1.Time{Time: t})
+	}
+}
+
 func withInstallJob(jobRef *corev1.ObjectReference) resourceModifier {
 	return func(r v1alpha1.StackInstaller) { r.SetInstallJob(jobRef) }
 }
@@ -123,6 +136,7 @@ type mockHandler struct {
 	MockSync   func(context.Context) (reconcile.Result, error)
 	MockCreate func(context.Context) (reconcile.Result, error)
 	MockUpdate func(context.Context) (reconcile.Result, error)
+	MockDelete func(context.Context) (reconcile.Result, error)
 }
 
 func (m *mockHandler) sync(ctx context.Context) (reconcile.Result, error) {
@@ -137,6 +151,10 @@ func (m *mockHandler) update(ctx context.Context) (reconcile.Result, error) {
 	return m.MockUpdate(ctx)
 }
 
+func (m *mockHandler) delete(ctx context.Context) (reconcile.Result, error) {
+	return m.MockDelete(ctx)
+}
+
 type mockExecutorInfoDiscoverer struct {
 	MockDiscoverExecutorInfo func(ctx context.Context) (*stacks.ExecutorInfo, error)
 }
@@ -158,7 +176,7 @@ func TestReconcile(t *testing.T) {
 		want want
 	}{
 		{
-			name: "SuccessfulSync",
+			name: "SuccessfulSyncStackInstall",
 			req:  reconcile.Request{NamespacedName: types.NamespacedName{Name: resourceName, Namespace: namespace}},
 			rec: &Reconciler{
 				kube: &test.MockClient{
@@ -186,7 +204,7 @@ func TestReconcile(t *testing.T) {
 			want: want{result: reconcile.Result{}, err: nil},
 		},
 		{
-			name: "SuccessfulSync",
+			name: "SuccessfulSyncClusterStackInstall",
 			req:  reconcile.Request{NamespacedName: types.NamespacedName{Name: resourceName, Namespace: namespace}},
 			rec: &Reconciler{
 				kube: &test.MockClient{
@@ -213,7 +231,34 @@ func TestReconcile(t *testing.T) {
 			},
 			want: want{result: reconcile.Result{}, err: nil},
 		},
-
+		{
+			name: "SuccessfulDelete",
+			req:  reconcile.Request{NamespacedName: types.NamespacedName{Name: resourceName, Namespace: namespace}},
+			rec: &Reconciler{
+				kube: &test.MockClient{
+					MockGet: func(ctx context.Context, key client.ObjectKey, obj runtime.Object) error {
+						*obj.(*v1alpha1.StackInstall) = *(resource(withDeletionTimestamp(time.Now())))
+						return nil
+					},
+				},
+				stackinator: func() v1alpha1.StackInstaller { return &v1alpha1.StackInstall{} },
+				executorInfoDiscoverer: &mockExecutorInfoDiscoverer{
+					MockDiscoverExecutorInfo: func(ctx context.Context) (*stacks.ExecutorInfo, error) {
+						return &stacks.ExecutorInfo{Image: stackPackageImage}, nil
+					},
+				},
+				factory: &mockFactory{
+					MockNewHandler: func(context.Context, v1alpha1.StackInstaller, client.Client, kubernetes.Interface, *stacks.ExecutorInfo) handler {
+						return &mockHandler{
+							MockDelete: func(context.Context) (reconcile.Result, error) {
+								return reconcile.Result{}, nil
+							},
+						}
+					},
+				},
+			},
+			want: want{result: reconcile.Result{}, err: nil},
+		},
 		{
 			name: "DiscoverExecutorInfoFailed",
 			req:  reconcile.Request{NamespacedName: types.NamespacedName{Name: resourceName, Namespace: namespace}},
@@ -282,6 +327,162 @@ func TestReconcile(t *testing.T) {
 	}
 }
 
+// TestStackInstallDelete tests the delete function of the stack install handler
+func TestStackInstallDelete(t *testing.T) {
+	tn := time.Now()
+
+	type want struct {
+		result reconcile.Result
+		err    error
+		si     *v1alpha1.StackInstall
+	}
+
+	tests := []struct {
+		name    string
+		handler *stackInstallHandler
+		want    want
+	}{
+		{
+			name: "FailList",
+			handler: &stackInstallHandler{
+				// stack install starts with a finalizer and a deletion timestamp
+				ext: resource(withFinalizers(installFinalizer), withDeletionTimestamp(tn)),
+				kube: &test.MockClient{
+					MockList: func(ctx context.Context, list runtime.Object, opts ...client.ListOption) error {
+						return errBoom
+					},
+					MockDelete:       func(ctx context.Context, obj runtime.Object, opts ...client.DeleteOption) error { return nil },
+					MockUpdate:       func(ctx context.Context, obj runtime.Object, opts ...client.UpdateOption) error { return nil },
+					MockStatusUpdate: func(ctx context.Context, obj runtime.Object, _ ...client.UpdateOption) error { return nil },
+				},
+			},
+			want: want{
+				result: resultRequeue,
+				err:    nil,
+				si: resource(
+					withFinalizers(installFinalizer),
+					withDeletionTimestamp(tn),
+					withConditions(runtimev1alpha1.ReconcileError(errBoom))),
+			},
+		},
+		{
+			name: "FailDelete",
+			handler: &stackInstallHandler{
+				// stack install starts with a finalizer and a deletion timestamp
+				ext: resource(withFinalizers(installFinalizer), withDeletionTimestamp(tn)),
+				kube: &test.MockClient{
+					MockList: func(ctx context.Context, list runtime.Object, opts ...client.ListOption) error {
+						// set the list's items to a fake list of CRDs to delete
+						list.(*apiextensionsv1beta1.CustomResourceDefinitionList).Items = []apiextensionsv1beta1.CustomResourceDefinition{
+							{
+								ObjectMeta: metav1.ObjectMeta{
+									Namespace: namespace,
+									Name:      "crdToDelete",
+								},
+							},
+						}
+						return nil
+					},
+					MockDelete: func(ctx context.Context, obj runtime.Object, opts ...client.DeleteOption) error {
+						return errBoom
+					},
+					MockUpdate:       func(ctx context.Context, obj runtime.Object, opts ...client.UpdateOption) error { return nil },
+					MockStatusUpdate: func(ctx context.Context, obj runtime.Object, _ ...client.UpdateOption) error { return nil },
+				},
+			},
+			want: want{
+				result: resultRequeue,
+				err:    nil,
+				si: resource(
+					withFinalizers(installFinalizer),
+					withDeletionTimestamp(tn),
+					withConditions(runtimev1alpha1.ReconcileError(errBoom))),
+			},
+		},
+		{
+			name: "FailUpdate",
+			handler: &stackInstallHandler{
+				// stack install starts with a finalizer and a deletion timestamp
+				ext: resource(withFinalizers(installFinalizer), withDeletionTimestamp(tn)),
+				kube: &test.MockClient{
+					MockList: func(ctx context.Context, list runtime.Object, opts ...client.ListOption) error {
+						// set the list's items to a fake list of CRDs to delete
+						list.(*apiextensionsv1beta1.CustomResourceDefinitionList).Items = []apiextensionsv1beta1.CustomResourceDefinition{
+							{
+								ObjectMeta: metav1.ObjectMeta{
+									Namespace: namespace,
+									Name:      "crdToDelete",
+								},
+							},
+						}
+						return nil
+					},
+					MockDelete: func(ctx context.Context, obj runtime.Object, opts ...client.DeleteOption) error { return nil },
+					MockUpdate: func(ctx context.Context, obj runtime.Object, opts ...client.UpdateOption) error {
+						return errBoom
+					},
+					MockStatusUpdate: func(ctx context.Context, obj runtime.Object, _ ...client.UpdateOption) error { return nil },
+				},
+			},
+			want: want{
+				result: resultRequeue,
+				err:    nil,
+				si: resource(
+					// the finalizer will have been removed from our test object at least in memory
+					// (even though the update call to the API server failed)
+					withDeletionTimestamp(tn),
+					withConditions(runtimev1alpha1.ReconcileError(errBoom))),
+			},
+		},
+		{
+			name: "SuccessfulDelete",
+			handler: &stackInstallHandler{
+				// stack install starts with a finalizer and a deletion timestamp
+				ext: resource(withFinalizers(installFinalizer), withDeletionTimestamp(tn)),
+				kube: &test.MockClient{
+					MockList: func(ctx context.Context, list runtime.Object, opts ...client.ListOption) error {
+						// set the list's items to a fake list of CRDs to delete
+						list.(*apiextensionsv1beta1.CustomResourceDefinitionList).Items = []apiextensionsv1beta1.CustomResourceDefinition{
+							{
+								ObjectMeta: metav1.ObjectMeta{
+									Namespace: namespace,
+									Name:      "crdToDelete",
+								},
+							},
+						}
+						return nil
+					},
+					MockDelete: func(ctx context.Context, obj runtime.Object, opts ...client.DeleteOption) error { return nil },
+					MockUpdate: func(ctx context.Context, obj runtime.Object, opts ...client.UpdateOption) error { return nil },
+				},
+			},
+			want: want{
+				result: reconcile.Result{},
+				err:    nil,
+				si:     resource(withDeletionTimestamp(tn)), // finalizers get removed by delete function
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotResult, gotErr := tt.handler.delete(ctx)
+
+			if diff := cmp.Diff(tt.want.err, gotErr, test.EquateErrors()); diff != "" {
+				t.Errorf("delete() -want error, +got error:\n%s", diff)
+			}
+
+			if diff := cmp.Diff(tt.want.result, gotResult); diff != "" {
+				t.Errorf("delete() -want result, +got result:\n%v", diff)
+			}
+
+			if diff := cmp.Diff(tt.want.si, tt.handler.ext, test.EquateConditions()); diff != "" {
+				t.Errorf("delete() -want stackInstall, +got stackInstall:\n%v", diff)
+			}
+		})
+	}
+}
+
 func TestHandlerFactory(t *testing.T) {
 	tests := []struct {
 		name    string