Make sure module crs get deleted before module operator (#195)

* ignore helmClient not found error
use same pattern for RemoveCRs RemoveCRDs

* don't delete namespace while installation

* remove error nil check

* rename noResourceFound to uninstallSuccess
write test for this function

* fix test

* change testEnv.stop to use eventually to check repeatedly.

* refactor test to pass lint check

Author: ruanxin

controllers/suite_test.go

@@ -165,8 +165,7 @@ var _ = AfterSuite(func() {
 	cancel()
 	By("tearing down the test environment")
 	server.Close()
-	err := testEnv.Stop()
-	Expect(err).NotTo(HaveOccurred())
-	err = os.RemoveAll(helmCacheHome)
+	Eventually(func() error { return testEnv.Stop() }, standardTimeout, standardInterval).Should(Succeed())
+	err := os.RemoveAll(helmCacheHome)
 	Expect(err).NotTo(HaveOccurred())
 })

pkg/manifest/helm_processor.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"reflect"
+	"strings"
 	"sync"
 	"time"
 
@@ -168,7 +169,7 @@ func (h *helm) Uninstall(stringifedManifest string, info *types.InstallInfo, tra
 
 	// uninstall resources
 	_, err = h.uninstallResources(resourceLists)
-	if err != nil {
+	if IgnoreHelmClientNotFound(err) != nil {
 		return false, err
 	}
 
@@ -218,6 +219,14 @@ func (h *helm) Uninstall(stringifedManifest string, info *types.InstallInfo, tra
 	return true, nil
 }
 
+func IgnoreHelmClientNotFound(err error) error {
+	// Refactoring this error check after this PR get merged and released https://github.com/helm/helm/pull/11591
+	if err != nil && strings.Contains(err.Error(), "object not found, skipping delete") {
+		return nil
+	}
+	return err
+}
+
 // uninstallChartCRDs uses a types.InstallInfo to lookup a chart and then tries to remove all CRDs found within it
 // and its dependencies. It can be used to forcefully remove all CRDs from a chart when CRDs were not included in the
 // render. Unlike our optimized installation, this one is built sequentially as we do not need the performance
@@ -343,8 +352,7 @@ func (h *helm) uninstallResources(resourceLists types.ResourceLists) (*kube.Resu
 	var response *kube.Result
 	var delErrors []error
 	if resourceLists.Installed != nil {
-		// add namespace to deleted resources
-		response, delErrors = h.clients.KubeClient().Delete(resourceLists.GetResourcesToBeDeleted())
+		response, delErrors = h.clients.KubeClient().Delete(resourceLists.Installed)
 		if len(delErrors) > 0 {
 			var wrappedError error
 			for _, err := range delErrors {

pkg/manifest/operations.go

@@ -7,7 +7,6 @@ import (
 	"github.com/go-logr/logr"
 	"helm.sh/helm/v3/pkg/cli"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	"k8s.io/apimachinery/pkg/api/meta"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	manifestClient "github.com/kyma-project/module-manager/pkg/client"
@@ -16,6 +15,7 @@ import (
 	"github.com/kyma-project/module-manager/pkg/resource"
 	"github.com/kyma-project/module-manager/pkg/types"
 	"github.com/kyma-project/module-manager/pkg/util"
+	"k8s.io/apimachinery/pkg/api/meta"
 )
 
 type Operations struct {
@@ -35,6 +35,11 @@ type OperationOptions struct {
 	Cache              types.RendererCache
 }
 
+var (
+	ErrCRsNotRemoved         = errors.New("CustomResources not completely removed")
+	ErrCRDsNotRemoved        = errors.New("CRDs not completely removed")
+	ErrUninstallInconsistent = errors.New("uninstallation inconsistent")
+)
 // InstallChart installs the resources based on types.InstallInfo and an appropriate rendering mechanism.
 func InstallChart(options OperationOptions) (bool, error) {
 	ops, err := NewOperations(options)
@@ -273,9 +278,9 @@ func (o *Operations) uninstall() (bool, error) {
 	// delete crs first - proceed only if not found
 	// proceed if CR type doesn't exist anymore - since associated CRDs might be deleted from resource uninstallation
 	// since there might be a deletion process to be completed by other manifest resources
-	deleted, err := resource.RemoveCRs(o.installInfo.Ctx, o.installInfo.CustomResources, o.client)
-	if !meta.IsNoMatchError(err) && (err != nil || !deleted) {
-		return false, err
+	crDeleted := resource.RemoveCRs(o.installInfo.Ctx, o.installInfo.CustomResources, o.client)
+	if !crDeleted {
+		return false, ErrCRsNotRemoved
 	}
 
 	// process manifest
@@ -291,14 +296,18 @@ func (o *Operations) uninstall() (bool, error) {
 	// uninstall resources
 	consistent, err := o.renderSrc.Uninstall(parsedFile.GetContent(),
 		o.installInfo, o.resourceTransforms, o.postRuns)
-	if err != nil || !consistent {
+	if !consistent {
+		return false, ErrUninstallInconsistent
+	}
+	if !UninstallSuccess(err) {
 		return false, err
 	}
 
 	// delete crds last - if not present ignore!
-	if err := resource.RemoveCRDs(o.installInfo.Ctx, o.installInfo.Crds, o.client); err != nil {
-		return false, err
-	}
+	crdDeleted :=  resource.RemoveCRDs(o.installInfo.Ctx, o.installInfo.Crds, o.client)
+		if !crdDeleted {
+			return false, ErrCRDsNotRemoved
+		}
 
 	// custom states check
 	if o.installInfo.CheckFn != nil {
@@ -307,6 +316,10 @@ func (o *Operations) uninstall() (bool, error) {
 	return true, err
 }
 
+func UninstallSuccess(err error) bool {
+	return err == nil || apierrors.IsNotFound(err) || meta.IsNoMatchError(err)
+}
+
 func (o *Operations) getManifestForChartPath(installInfo *types.InstallInfo) *types.ParsedFile {
 	// 1. check provided manifest file
 	// It is expected for installInfo.ChartPath to contain ONE .yaml or .yml file,

pkg/manifest/operations_test.go

@@ -0,0 +1,61 @@
+package manifest_test
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/kyma-project/module-manager/pkg/manifest"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	apimeta "k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func Test_UninstallSuccess(t *testing.T) {
+	t.Parallel()
+	type args struct {
+		err error
+	}
+	tests := []struct {
+		name string
+		args args
+		want bool
+	}{
+		{
+			name: "when no error, expect uninstall successfully",
+			args: args{err: nil},
+			want: true,
+		},
+		{
+			name: "when api not found error, expect uninstall successfully",
+			args: args{err: &apierrors.StatusError{ErrStatus: metav1.Status{
+				Status: metav1.StatusFailure,
+				Reason: metav1.StatusReasonNotFound,
+			}}},
+			want: true,
+		},
+		{
+			name: "when api no kind match error, expect uninstall successfully",
+			args: args{err: &apimeta.NoKindMatchError{}},
+			want: true,
+		},
+		{
+			name: "when api no resource match error, expect uninstall successfully",
+			args: args{err: &apimeta.NoResourceMatchError{}},
+			want: true,
+		},
+		{
+			name: "when receive normal error, expect uninstall successfully",
+			args: args{err: errors.New("some unexpected error")},
+			want: false,
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			if got := manifest.UninstallSuccess(tt.args.err); got != tt.want {
+				t.Errorf("UninstallSuccess() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

pkg/resource/operations.go

@@ -20,6 +20,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/yaml"
 	"sigs.k8s.io/controller-runtime/pkg/log"
@@ -230,46 +232,38 @@ func CheckCRs(ctx context.Context, crs []*unstructured.Unstructured, runtimeClie
 
 func RemoveCRDs(ctx context.Context, crds []*apiextensionsv1.CustomResourceDefinition,
 	runtimeClient client.Client,
-) error {
-	for _, crd := range crds {
-		existingCrd := apiextensionsv1.CustomResourceDefinition{}
-		if err := runtimeClient.Get(ctx, client.ObjectKeyFromObject(crd), &existingCrd); err != nil {
-			if client.IgnoreNotFound(err) != nil {
-				return err
-			}
+) bool {
+	deleted := true
+	for _, resource := range crds {
+		if resourceDeleted(ctx, resource, runtimeClient) {
 			continue
 		}
-
-		if err := runtimeClient.Delete(ctx, &existingCrd); err != nil {
-			return err
-		}
+		deleted = false
 	}
-	return nil
+	return deleted
 }
 
-func RemoveCRs(ctx context.Context, crs []*unstructured.Unstructured,
-	runtimeClient client.Client,
-) (bool, error) {
+func RemoveCRs(ctx context.Context, crs []*unstructured.Unstructured, runtimeClient client.Client) bool {
 	deleted := true
 	for _, resource := range crs {
-		existingCustomResource := unstructured.Unstructured{}
-		existingCustomResource.SetGroupVersionKind(resource.GroupVersionKind())
-		customResourceKey := client.ObjectKey{
-			Name:      resource.GetName(),
-			Namespace: resource.GetNamespace(),
-		}
-		if err := runtimeClient.Get(ctx, customResourceKey, &existingCustomResource); err != nil {
-			if client.IgnoreNotFound(err) != nil {
-				return false, err
-			}
-			// resource deleted
+		if resourceDeleted(ctx, resource, runtimeClient) {
 			continue
 		}
-
 		deleted = false
-		if err := runtimeClient.Delete(ctx, resource); err != nil {
-			return false, err
-		}
 	}
-	return deleted, nil
+	return deleted
+}
+
+func resourceDeleted(ctx context.Context, resource client.Object, runtimeClient client.Client) bool {
+	logger := log.FromContext(ctx)
+	err := runtimeClient.Delete(ctx, resource)
+	// only not found or no resource match can make sure resource deleted
+	if apierrors.IsNotFound(err) || meta.IsNoMatchError(err) {
+		return true
+	}
+	if err != nil {
+		logger.V(util.DebugLogLevel).Error(err, "RemoveResources")
+		return false
+	}
+	return false
 }

pkg/types/installs.go

@@ -157,10 +157,6 @@ func (r *ResourceLists) GetWaitForResources() kube.ResourceList {
 	return append(r.Target, r.Namespace...)
 }
 
-func (r *ResourceLists) GetResourcesToBeDeleted() kube.ResourceList {
-	return append(r.Installed, r.Namespace...)
-}
-
 // InstallInfo represents deployment information artifacts to be processed.
 type InstallInfo struct {
 	// ChartInfo represents chart information to be processed