Repository: crossplaneio/crossplane
Pull request #1091 (2019-11-21 06:12:37)

Cleanup CRDs by label, not garbage collection
### Description of your changes

Fixes https://github.com/crossplaneio/crossplane/issues/895

(Cluster)StackInstall is namespaced, and CRDs are cluster scoped. Namespaced resources cannot own cluster scoped resources, so we can't rely on garbage collection to cleanup CRDs that were installed by a StackInstall. This uses labels instead.

I have removed the draft qualifier on this PR now so it is ready for full review.  The approach here uses the labels that are defined in the [stack relationship labels design doc](https://github.com/negz/crossplane/blob/refrace/design/one-pager-stack-relationship-labels.md), which has been updated by this PR also to be in alignment with the current implementation.

I think that this is a good approach for a `v0.5.1` patch release, but @displague is thinking about the longer term approach and how it aligns with ownership, garbage collection, multiple stack installs and versions, parent / child relationships, etc. 

Below is an example of a labeled CRD after stack-gcp has been installed:

```yaml
> kubectl get crd gkeclusters.compute.gcp.crossplane.io -o yaml
...
labels:
    app.kubernetes.io/managed-by: stack-manager
    core.crossplane.io/parent-group: stacks.crossplane.io
    core.crossplane.io/parent-kind: ClusterStackInstall
    core.crossplane.io/parent-name: stack-gcp
    core.crossplane.io/parent-namespace: gcp
    core.crossplane.io/parent-uid: 858ab465-ff60-49d8-a3e7-624ce841f339
    core.crossplane.io/parent-version: v1alpha1
name: gkeclusters.compute.gcp.crossplane.io
...
```

### Checklist
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], [examples], or [release notes].
- [x] Updated the RBAC permissions in [`clusterrole.yaml`] to include any new types.

[documentation]: https://github.com/crossplaneio/crossplane/tree/master/docs
[examples]: https://github.com/crossplaneio/crossplane/tree/master/cluster/examples
[release notes]: https://github.com/crossplaneio/crossplane/tree/master/PendingReleaseNotes.md
[`clusterrole.yaml`]: https://github.com/crossplaneio/crossplane/blob/master/cluster/charts/crossplane/templates/clusterrole.yaml

upbound-bot: [![80% (-1.19%) vs master 81%](https://jenkinsci.upbound.io//coverage-status-icon/?coverage=0.7974&masterCoverage=0.8093)](https://jenkinsci.upbound.io/job/crossplaneio/job/crossplane/job/build/job/PR-1091/2/)
negz: > Is this preferable to moving forward with #1023?

I don't know.

I'm not planning to do further work on this, because I don't feel I have a good enough understanding of the current state and goals for stacks to fix it in a reasonable way. I'll leave the PR open in draft for now, since it sparked some discussion.
hasheddan: @jbw976 @lukeweber @displague wondering if anyone has thoughts here, Would be good to fix this in one way or another soon.
negz: > Were you already able to run this scenario manually to see if it worked?

@jbw976 I have not. Would it make sense for someone from the stacks squad to take ownership of this issue?
jbw976: Sure @negz, I'd be happy to drive this PR to completion.  I'll take it for a test spin and fix any potential issues I find.  Thanks for getting this so far already! 🙇 
upbound-bot: [![80% (-1.19%) vs master 81%](https://jenkinsci.upbound.io//coverage-status-icon/?coverage=0.7974&masterCoverage=0.8093)](https://jenkinsci.upbound.io/job/crossplaneio/job/crossplane/job/build/job/PR-1091/3/)
upbound-bot: [![80% (-1.19%) vs master 81%](https://jenkinsci.upbound.io//coverage-status-icon/?coverage=0.7974&masterCoverage=0.8093)](https://jenkinsci.upbound.io/job/crossplaneio/job/crossplane/job/build/job/PR-1091/4/)

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/design/one-pager-stack-relationship-labels.md b/design/one-pager-stack-relationship-labels.md
index bfcc7c9e6..ded790c91 100644
--- a/design/one-pager-stack-relationship-labels.md
+++ b/design/one-pager-stack-relationship-labels.md
@@ -12,6 +12,8 @@
 
 #### These are the labels we are proposing to add:
 
+- **`core.crossplane.io/parent-group`**
+- **`core.crossplane.io/parent-version`**
 - **`core.crossplane.io/parent-kind`**
 - **`core.crossplane.io/parent-name`**
 - **`core.crossplane.io/parent-namespace`**
@@ -24,7 +26,7 @@ We can leverage CRD labels to make this easier, as long as we have a standard fo
 Additionally as long as we define this label on all children CRD's, we should then be able to query
 all children of a given parent object using those labels.
 
-Per the proposal, it seems that the closest example of what I am referring to is shown on 
+Per the proposal, it seems that the closest example of what I am referring to is shown on
 [this page][common-labels] within the `app.kubernetes.io/part-of` label. However since we are not
 exactly using these labels for the intentions defined within that page, we should use something
 following those patterns instead.
@@ -38,6 +40,13 @@ The primary reason for this was stated on [that page][common-labels]:
 
 ## Design
 
+The group, version, and kind is captured as three individual fields to provide the most flexibility
+for consumers of this metadata. Since consumers will have access to all 3 fields, they can on-demand
+build any of the numerous GVK related formats available in the ecosystem. It was initially
+considered to combine the group and version in the typical format of `my.group/v1alpha1`, but labels
+cannot contain any slash characters. Therefore, storing the group, version, and kind as separate
+fields seems reasonable.
+
 > _**Important Note**: These files are post processed yaml outputs, **NOT** static package files.
 > The purpose of showing these is only to better outline and explain the relationships with respect
 > to this specific example._
@@ -45,18 +54,39 @@ The primary reason for this was stated on [that page][common-labels]:
 Wordpress example file tree structure of the example files below:
 
 ```text
-stack.yaml
+stackinstall.yaml
 |
 `-- crd.yaml
+|
+`-- stack.yaml
     |
-    `-- resource.yaml
+    `-- wordpress.yaml
         |-- kubernetesapplication.yaml
         |-- kubernetescluster.yaml
         |-- mysqlinstance.yaml
         |-- ...
 ```
 
-### Example wordpress `stack.yaml`
+### Example wordpress `crd.yaml` (parented by `StackInstall`)
+
+```yaml
+apiVersion: apiextensions.k8s.io/v1beta1
+kind: CustomResourceDefinition
+metadata:
+  creationTimestamp: null
+  name: wordpressinstances.wordpress.samples.stacks.crossplane.io
+  labels:
+    core.crossplane.io/parent-group: "stacks.crossplane.io"
+    core.crossplane.io/parent-version: "v1alpha1"
+    core.crossplane.io/parent-kind: "StackInstall"
+    core.crossplane.io/parent-name: "sample-stack-wordpress"
+    core.crossplane.io/parent-namespace: "app-project1-dev"
+    core.crossplane.io/parent-uid: "858ab465-ff60-49d8-a3e7-624ce841f339"
+    app.kubernetes.io/managed-by: stack-manager
+...
+```
+
+### Example wordpress `stack.yaml` (parented by `StackInstall`)
 
 ```yaml
 apiVersion: stacks.crossplane.io/v1alpha1
@@ -76,27 +106,16 @@ metadata:
   uid: ec52c8c2-379f-45ec-9458-e40f070f8d2e
   labels:
     app.kubernetes.io/managed-by: stack-manager
-...
-```
-
-### Example wordpress `crd.yaml`
-
-```yaml
-apiVersion: apiextensions.k8s.io/v1beta1
-kind: CustomResourceDefinition
-metadata:
-  creationTimestamp: null
-  name: wordpressinstances.wordpress.samples.stacks.crossplane.io
-  labels:
-    core.crossplane.io/parent-kind: "stack.stacks.crossplane.io"
+    core.crossplane.io/parent-group: "stacks.crossplane.io"
+    core.crossplane.io/parent-version: "v1alpha1"
+    core.crossplane.io/parent-kind: "StackInstall"
     core.crossplane.io/parent-name: "sample-stack-wordpress"
     core.crossplane.io/parent-namespace: "app-project1-dev"
-    core.crossplane.io/parent-uid: "ec52c8c2-379f-45ec-9458-e40f070f8d2e"
-    app.kubernetes.io/managed-by: stack-manager
+    core.crossplane.io/parent-uid: "858ab465-ff60-49d8-a3e7-624ce841f339"
 ...
 ```
 
-### Example wordpress `resource.yaml`
+### Example wordpress `wordpress.yaml` (parented by `Stack`)
 
 ```yaml
 apiVersion: wordpress.samples.stacks.crossplane.io/v1alpha1
@@ -110,7 +129,9 @@ metadata:
   selfLink: /apis/wordpress.samples.stacks.crossplane.io/v1alpha1/namespaces/app-project1-dev/wordpressinstances/my-wordpressinstance
   uid: f2d13a15-1f9b-40a7-a173-a40abefa61bf
   labels:
-    core.crossplane.io/parent-kind: "stack.stacks.crossplane.io"
+    core.crossplane.io/parent-kind: "Stack"
+    core.crossplane.io/parent-group: "stacks.crossplane.io"
+    core.crossplane.io/parent-version: "v1alpha1"
     core.crossplane.io/parent-name: "sample-stack-wordpress"
     core.crossplane.io/parent-namespace: "app-project1-dev"
     core.crossplane.io/parent-uid: "ec52c8c2-379f-45ec-9458-e40f070f8d2e"
@@ -118,7 +139,7 @@ metadata:
 ...
 ```
 
-### Example wordpress `kubernetesapplication.yaml`
+### Example wordpress `kubernetesapplication.yaml` (parented by `WordpressInstance`)
 
 ```yaml
 apiVersion: workload.crossplane.io/v1alpha1
@@ -127,7 +148,9 @@ metadata:
   name: wordpress-app-wordpress
   labels:
     stack: sample-stack-wordpress
-    core.crossplane.io/parent-kind: "wordpressinstances.wordpress.samples.stacks.crossplane.io"
+    core.crossplane.io/parent-kind: "WordpressInstance"
+    core.crossplane.io/parent-group: "wordpress.samples.stacks.crossplane.io"
+    core.crossplane.io/parent-version: "v1alpha1"
     core.crossplane.io/parent-name: "my-wordpressinstance"
     core.crossplane.io/parent-namespace: "app-project1-dev"
     core.crossplane.io/parent-uid: "f2d13a15-1f9b-40a7-a173-a40abefa61bf"
@@ -135,7 +158,7 @@ metadata:
 ...
 ```
 
-### Example wordpress `kubernetescluster.yaml`
+### Example wordpress `kubernetescluster.yaml` (parented by `WordpressInstance`)
 
 ```yaml
 apiVersion: compute.crossplane.io/v1alpha1
@@ -144,7 +167,9 @@ metadata:
   name: wordpress-cluster-wordpress
   labels:
     stack: sample-stack-wordpress
-    core.crossplane.io/parent-kind: "wordpressinstances.wordpress.samples.stacks.crossplane.io"
+    core.crossplane.io/parent-kind: "WordpressInstance"
+    core.crossplane.io/parent-group: "wordpress.samples.stacks.crossplane.io"
+    core.crossplane.io/parent-version: "v1alpha1"
     core.crossplane.io/parent-name: "my-wordpressinstance"
     core.crossplane.io/parent-namespace: "app-project1-dev"
     core.crossplane.io/parent-uid: "f2d13a15-1f9b-40a7-a173-a40abefa61bf"
@@ -152,7 +177,7 @@ metadata:
 ...
 ```
 
-### Example wordpress `mysqlinstance.yaml`
+### Example wordpress `mysqlinstance.yaml` (parented by `WordpressInstance`)
 
 ```yaml
 apiVersion: database.crossplane.io/v1alpha1
@@ -161,7 +186,9 @@ metadata:
   name: wordpress-mysql-wordpress
   labels:
     stack: sample-stack-wordpress
-    core.crossplane.io/parent-kind: "wordpressinstances.wordpress.samples.stacks.crossplane.io"
+    core.crossplane.io/parent-kind: "WordpressInstance"
+    core.crossplane.io/parent-group: "wordpress.samples.stacks.crossplane.io"
+    core.crossplane.io/parent-version: "v1alpha1"
     core.crossplane.io/parent-name: "my-wordpressinstance"
     core.crossplane.io/parent-namespace: "app-project1-dev"
     core.crossplane.io/parent-uid: "f2d13a15-1f9b-40a7-a173-a40abefa61bf"
@@ -229,4 +256,4 @@ useful, just not for describing a multi-level hierarchy.
 
 <!-- Recurring Links -->
 
-[common-labels]: https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/#labels
\ No newline at end of file
+[common-labels]: https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/#labels
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