Repository: tektoncd/operator
Pull request #1129 (2022-11-02 03:22:51)

fix deletion of resource which is breaking the pod
manifestival is breaking while deleting a resource: during deletion it first fetch the resource, if there occurs any error it still tries to access the resources which is causing
```
panic: runtime error: invalid memory address or nil pointer dereferenc
```
https://github.com/tektoncd/operator/blob/main/vendor/github.com/manifestival/manifestival/manifestival.go#L175 here when error is not nil, current is nil and then further it tries to access the resource kind which breaks the pod.

so we write our own delete func, which first fetch the resource, if not error occurs then calls delete.

Signed-off-by: Shivam Mukhade <smukhade@redhat.com>

# Changes

<!-- 🎉🎉🎉 Thank you for the PR!!! 🎉🎉🎉 -->

<!-- Describe your changes here- ideally you can get that description straight from
your descriptive commit message(s)! -->

# Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

- [x] Run `make test lint` before submitting a PR
- [x] Includes [tests](https://github.com/tektoncd/community/blob/master/standards.md#principles) (if functionality changed/added)
- [ ] Includes [docs](https://github.com/tektoncd/community/blob/master/standards.md#principles) (if user facing)
- [x] Commit messages follow [commit message best practices](https://github.com/tektoncd/community/blob/master/standards.md#commit-messages)

_See [the contribution guide](https://github.com/tektoncd/operator/blob/master/CONTRIBUTING.md) for more details._

# Release Notes

<!--
Describe any user facing changes here, or delete this block.

Examples of user facing changes:
- API changes
- Bug fixes
- Any changes in behavior
- Changes requiring upgrade notices or deprecation warnings

For pull requests with a release note:

```release-note
Your release note here
```

For pull requests that require additional action from users switching to the new release, include the string "action required" (case insensitive) in the release note:

```release-note
action required: your release note here
```

For pull requests that don't need to be mentioned at release time, use the `/release-note-none` Prow command to add the `release-note-none` label to the PR. You can also write the string "NONE" as a release note in your PR description:

```release-note
NONE
```
-->


tekton-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please ask for approval from **sm43** after the PR has been reviewed.

The full list of commands accepted by this bot can be found [here](https://prow.tekton.dev/command-help?repo=tektoncd%2Foperator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/tektoncd/operator/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["sm43"]} -->
tekton-robot: The following is the coverage report on the affected files.
Say `/test pull-tekton-operator-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/reconciler/kubernetes/tektoninstallerset/install.go](https://storage.cloud.google.com/tekton-prow/pr-logs/pull/tektoncd_operator/1129/pull-tekton-operator-go-coverage/1587646395144212480/artifacts/line-cov.html#file0) | 46.7% | 50.7% | 4.1

savitaashture: /lgtm
tekton-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/tektoncd/operator/pull/1129#pullrequestreview-1166445586" title="Approved">savitaashture</a>*

The full list of commands accepted by this bot can be found [here](https://prow.tekton.dev/command-help?repo=tektoncd%2Foperator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/tektoncd/operator/blob/main/OWNERS)~~ [savitaashture]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/pkg/reconciler/kubernetes/tektoninstallerset/install.go b/pkg/reconciler/kubernetes/tektoninstallerset/install.go
index 8145e6b0a..f3a4b9e21 100644
--- a/pkg/reconciler/kubernetes/tektoninstallerset/install.go
+++ b/pkg/reconciler/kubernetes/tektoninstallerset/install.go
@@ -440,3 +440,51 @@ func isJobCompleted(d *batchv1.Job) bool {
 	}
 	return false
 }
+
+// DeleteResources Deletes all resources except CRDs, PVCs and Namespace as they
+// are own by owner of TektonInstallerSet.
+// They will be deleted when the component CR is deleted
+func (i *installer) DeleteResources() error {
+	// delete clusterScope resources first
+	if err := i.delete(i.clusterScoped); err != nil {
+		return err
+	}
+	if err := i.delete(i.namespaceScoped); err != nil {
+		return err
+	}
+	if err := i.delete(i.deployment); err != nil {
+		return err
+	}
+	return nil
+}
+
+func (i *installer) delete(resources []unstructured.Unstructured) error {
+	for _, r := range resources {
+		if skipDeletion(r.GetKind()) {
+			continue
+		}
+		resource, err := i.mfClient.Get(&r)
+		if err != nil {
+			// if error occurs log and move on, as we have owner reference set for resources, those
+			// will be removed eventually and manifestival breaks the pod during uninstallation,
+			// when CRD is deleted, CRs are removed but when we delete installer set, manifestival
+			// breaks during deleting those CRs
+			i.logger.Errorf("failed to get resource, skipping deletion: %v/%v: %v ", r.GetKind(), r.GetName(), err)
+			continue
+		}
+		err = i.mfClient.Delete(resource)
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func skipDeletion(kind string) bool {
+	if kind == "Namespace" ||
+		kind == "PersistentVolumeClaim" ||
+		kind == "CustomResourceDefinition" {
+		return true
+	}
+	return false
+}
diff --git a/pkg/reconciler/kubernetes/tektoninstallerset/tektoninstallerset.go b/pkg/reconciler/kubernetes/tektoninstallerset/tektoninstallerset.go
index db967c2ac..fd07cf5d8 100644
--- a/pkg/reconciler/kubernetes/tektoninstallerset/tektoninstallerset.go
+++ b/pkg/reconciler/kubernetes/tektoninstallerset/tektoninstallerset.go
@@ -50,16 +50,12 @@ func (r *Reconciler) FinalizeKind(ctx context.Context, installerSet *v1alpha1.Te
 		return err
 	}
 
-	// Delete all resources except CRDs and Namespace as they are own by owner of
-	// TektonInstallerSet
-	// They will be deleted when the component CR is deleted
-	deleteManifests = deleteManifests.Filter(mf.Not(mf.Any(mf.ByKind("Namespace"), mf.CRDs, mf.ByKind("PersistentVolumeClaim"))))
-	err = deleteManifests.Delete()
+	installer := NewInstaller(&deleteManifests, r.mfClient, logger)
+	err = installer.DeleteResources()
 	if err != nil {
-		logger.Error("failed to delete resources")
+		logger.Error("failed to delete resources: ", err)
 		return err
 	}
-
 	return nil
 }
 


Test patch:
diff --git a/pkg/reconciler/kubernetes/tektoninstallerset/install_test.go b/pkg/reconciler/kubernetes/tektoninstallerset/install_test.go
index 565f9687c..487aa48e7 100644
--- a/pkg/reconciler/kubernetes/tektoninstallerset/install_test.go
+++ b/pkg/reconciler/kubernetes/tektoninstallerset/install_test.go
@@ -37,6 +37,7 @@ import (
 
 var (
 	serviceAccount = namespacedResource("v1", "ServiceAccount", "test", "test-service-account")
+	namespace      = namespacedResource("v1", "Namespace", "", "test-service-account")
 )
 
 // namespacedResource is an unstructured resource with the given apiVersion, kind, ns and name.
@@ -359,3 +360,26 @@ func TestJobFailed(t *testing.T) {
 		t.Fatal("Expected Error but got nil ")
 	}
 }
+
+func TestEnsureResources_DeleteResources(t *testing.T) {
+	fakeClient := fake.New(&serviceAccount, &namespace)
+	observer, _ := zapobserver.New(zap.InfoLevel)
+	logger := zap.New(observer).Sugar()
+
+	manifest, err := mf.ManifestFrom(mf.Slice([]unstructured.Unstructured{serviceAccount, namespace}))
+	if err != nil {
+		t.Fatalf("Failed to generate manifest: %v", err)
+	}
+
+	i := NewInstaller(&manifest, fakeClient, logger)
+
+	err = i.DeleteResources()
+	assert.NilError(t, err)
+
+	_, err = fakeClient.Get(&serviceAccount)
+	assert.Error(t, err, "ServiceAccount \"test-service-account\" not found")
+
+	// namespace must be not deleted
+	_, err = fakeClient.Get(&namespace)
+	assert.NilError(t, err)
+}