Fix uninstallation bugs in decliarative library (#196)

* add helm client not found error check to all place where KubeClient().Delete get called
remove check namespace in checkWaitForResources
return only error but not (bool, error) to simplify return condition handling

* refactoring uninstallResources

* bring back ErrUninstallInconsistent

Author: ruanxin

pkg/manifest/helm_processor.go

@@ -128,9 +128,9 @@ func (h *helm) Install(stringifedManifest string, info *types.InstallInfo, trans
 		"resource", client.ObjectKeyFromObject(info.BaseResource).String())
 
 	// verify resources
-	if ready, err := h.verifyResources(info.Ctx, resourceLists,
-		info.CheckReadyStates, types.OperationCreate); !ready || err != nil {
-		return ready, err
+	if err := h.checkTargetResources(info.Ctx, resourceLists.Target,
+		types.OperationCreate, info.CheckReadyStates); err != nil {
+		return false, err
 	}
 
 	// update helm repositories
@@ -168,8 +168,8 @@ func (h *helm) Uninstall(stringifedManifest string, info *types.InstallInfo, tra
 	}
 
 	// uninstall resources
-	_, err = h.uninstallResources(resourceLists)
-	if IgnoreHelmClientNotFound(err) != nil {
+	err = h.uninstallResources(resourceLists.Installed)
+	if err != nil {
 		return false, err
 	}
 
@@ -187,10 +187,10 @@ func (h *helm) Uninstall(stringifedManifest string, info *types.InstallInfo, tra
 		"resource", client.ObjectKeyFromObject(info.BaseResource).String())
 
 	// verify resource uninstallation
-	if ready, err := h.verifyResources(
-		info.Ctx, resourceLists,
-		info.CheckReadyStates, types.OperationDelete); !ready || err != nil {
-		return ready, err
+	if err := h.checkTargetResources(
+		info.Ctx, resourceLists.Target, types.OperationDelete,
+		info.CheckReadyStates); err != nil {
+		return false, err
 	}
 
 	// include CRDs means that the Chart will include the CRDs during rendering, if this is set to false,
@@ -219,12 +219,12 @@ func (h *helm) Uninstall(stringifedManifest string, info *types.InstallInfo, tra
 	return true, nil
 }
 
-func IgnoreHelmClientNotFound(err error) error {
+func isHelmClientNotFound(err error) bool {
 	// 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 true
 	}
-	return err
+	return false
 }
 
 // uninstallChartCRDs uses a types.InstallInfo to lookup a chart and then tries to remove all CRDs found within it
@@ -244,23 +244,21 @@ func (h *helm) uninstallChartCRDs(info *types.InstallInfo) error {
 	if err != nil {
 		return err
 	}
-	resList, err := h.getTargetResources(info.Ctx, crds.String(), nil, nil, false)
+	resourceList, err := h.getTargetResources(info.Ctx, crds.String(), nil, nil, false)
+	if resourceList == nil {
+		return nil
+	}
 	if err != nil {
 		return err
 	}
-
-	_, deleteErrs := h.clients.KubeClient().Delete(resList)
-	criticalDeleteErrs := make([]error, 0, len(deleteErrs))
-	for i := range deleteErrs {
-		if deleteErrs[i] != nil && !apierrors.IsNotFound(deleteErrs[i]) {
-			criticalDeleteErrs = append(criticalDeleteErrs, deleteErrs[i])
+	_, deleteErrs := h.clients.KubeClient().Delete(resourceList)
+	if len(deleteErrs) > 0 {
+		filteredErrs := filterNotFoundError(deleteErrs)
+		if len(filteredErrs) > 0 {
+			return types.NewMultiError(filteredErrs)
 		}
 	}
 
-	if len(criticalDeleteErrs) > 0 {
-		return types.NewMultiError(criticalDeleteErrs)
-	}
-
 	return nil
 }
 
@@ -299,21 +297,21 @@ func (h *helm) IsConsistent(stringifedManifest string, info *types.InstallInfo,
 	}
 
 	// verify resources
-	ready, err := h.verifyResources(info.Ctx, resourceLists, info.CheckReadyStates, types.OperationCreate)
+	if err := h.checkTargetResources(info.Ctx,
+		resourceLists.Target,
+		types.OperationCreate,
+		info.CheckReadyStates); err != nil {
+		return false, err
+	}
 
 	h.logger.V(util.DebugLogLevel).Info("consistency check finished",
-		"consistent", ready,
 		"create count", len(result.Created),
 		"update count", len(result.Updated),
 		"chart", info.ChartName,
 		"release", info.ReleaseName,
 		"resource", client.ObjectKeyFromObject(info.BaseResource).String(),
 		"time", time.Since(startConsistencyCheck).String())
 
-	if !ready || err != nil {
-		return ready, err
-	}
-
 	// update helm repositories
 	if info.UpdateRepositories {
 		if err = h.updateRepos(info.Ctx); err != nil {
@@ -324,13 +322,6 @@ func (h *helm) IsConsistent(stringifedManifest string, info *types.InstallInfo,
 	return true, nil
 }
 
-func (h *helm) verifyResources(ctx context.Context, resourceLists types.ResourceLists, verifyReadyStates bool,
-	operationType types.HelmOperation,
-) (bool, error) {
-	return h.checkWaitForResources(ctx, resourceLists.GetWaitForResources(), operationType,
-		verifyReadyStates)
-}
-
 func (h *helm) installResources(resourceLists types.ResourceLists, force bool) (*kube.Result, error) {
 	// create namespace resource first!
 	if len(resourceLists.Namespace) > 0 {
@@ -348,25 +339,35 @@ func (h *helm) installResources(resourceLists types.ResourceLists, force bool) (
 	return h.clients.KubeClient().Update(resourceLists.Installed, resourceLists.Target, force)
 }
 
-func (h *helm) uninstallResources(resourceLists types.ResourceLists) (*kube.Result, error) {
-	var response *kube.Result
-	var delErrors []error
-	if resourceLists.Installed != nil {
-		response, delErrors = h.clients.KubeClient().Delete(resourceLists.Installed)
-		if len(delErrors) > 0 {
-			var wrappedError error
-			for _, err := range delErrors {
-				wrappedError = fmt.Errorf("%w", err)
+func (h *helm) uninstallResources(installedResources kube.ResourceList) error {
+	var deleteErrs []error
+	if installedResources != nil {
+		_, deleteErrs = h.clients.KubeClient().Delete(installedResources)
+		if len(deleteErrs) > 0 {
+			filteredErrs := filterNotFoundError(deleteErrs)
+			if len(filteredErrs) > 0 {
+				return types.NewMultiError(filteredErrs)
 			}
-			return nil, wrappedError
 		}
 	}
-	return response, nil
+	return nil
 }
 
-func (h *helm) checkWaitForResources(ctx context.Context, targetResources kube.ResourceList,
-	operation types.HelmOperation, verifyWithoutTimeout bool,
-) (bool, error) {
+func filterNotFoundError(delErrors []error) []error {
+	wrappedErrors := make([]error, 0)
+	for _, err := range delErrors {
+		if isHelmClientNotFound(err) || apierrors.IsNotFound(err) {
+			continue
+		}
+		wrappedErrors = append(wrappedErrors, err)
+	}
+	return wrappedErrors
+}
+
+func (h *helm) checkTargetResources(ctx context.Context,
+	targetResources kube.ResourceList,
+	operation types.HelmOperation,
+	verifyWithoutTimeout bool) error {
 	// verifyWithoutTimeout flag checks native resources are in their respective ready states
 	// without a timeout defined
 	if verifyWithoutTimeout {
@@ -375,7 +376,7 @@ func (h *helm) checkWaitForResources(ctx context.Context, targetResources kube.R
 		}
 		clientSet, err := h.clients.KubernetesClientSet()
 		if err != nil {
-			return false, err
+			return err
 		}
 		readyChecker := kube.NewReadyChecker(clientSet,
 			func(format string, args ...interface{}) {
@@ -390,20 +391,20 @@ func (h *helm) checkWaitForResources(ctx context.Context, targetResources kube.R
 	// if Wait or WaitForJobs is enabled, resources are verified to be in ready state with a timeout
 	if !h.clients.Install().Wait || h.clients.Install().Timeout == 0 {
 		// return here as ready, since waiting flags were not set
-		return true, nil
+		return nil
 	}
 
 	if operation == types.OperationDelete {
 		// WaitForDelete reports an error if resources are not deleted in the specified timeout
-		return true, h.clients.KubeClient().WaitForDelete(targetResources, h.clients.Install().Timeout)
+		return h.clients.KubeClient().WaitForDelete(targetResources, h.clients.Install().Timeout)
 	}
 
 	if h.clients.Install().WaitForJobs {
 		// WaitWithJobs reports an error if resources are not deleted in the specified timeout
-		return true, h.clients.KubeClient().WaitWithJobs(targetResources, h.clients.Install().Timeout)
+		return h.clients.KubeClient().WaitWithJobs(targetResources, h.clients.Install().Timeout)
 	}
 	// Wait reports an error if resources are not deleted in the specified timeout
-	return true, h.clients.KubeClient().Wait(targetResources, h.clients.Install().Timeout)
+	return h.clients.KubeClient().Wait(targetResources, h.clients.Install().Timeout)
 }
 
 func (h *helm) updateRepos(ctx context.Context) error {

pkg/manifest/helm_processor_util.go

@@ -3,6 +3,7 @@ package manifest
 import (
 	"context"
 
+	"errors"
 	"helm.sh/helm/v3/pkg/kube"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
@@ -11,20 +12,23 @@ import (
 	"k8s.io/cli-runtime/pkg/resource"
 )
 
-func checkResourcesDeleted(targetResources kube.ResourceList) (bool, error) {
-	resourcesDeleted := true
-	err := targetResources.Visit(func(info *resource.Info, err error) error {
+var ErrResourceNotReady = errors.New("resource not ready")
+var ErrResourceNotDeleted = errors.New("resource not deleted")
+
+func checkResourcesDeleted(targetResources kube.ResourceList) error {
+	return targetResources.Visit(func(info *resource.Info, err error) error {
 		if err != nil {
 			return err
 		}
 		err = info.Get()
-		if err == nil || !apierrors.IsNotFound(err) {
-			resourcesDeleted = false
+		if err == nil {
+			return ErrResourceNotDeleted
+		}
+		if !apierrors.IsNotFound(err) {
 			return err
 		}
 		return nil
 	})
-	return resourcesDeleted, err
 }
 
 func setNamespaceIfNotPresent(targetNamespace string, resourceInfo *resource.Info,
@@ -60,20 +64,18 @@ func overrideNamespace(resourceList kube.ResourceList, targetNamespace string) e
 	})
 }
 
-func checkReady(ctx context.Context, resourceList kube.ResourceList,
-	readyChecker kube.ReadyChecker,
-) (bool, error) {
-	resourcesReady := true
-	err := resourceList.Visit(func(info *resource.Info, err error) error {
+func checkReady(ctx context.Context, resourceList kube.ResourceList, readyChecker kube.ReadyChecker) error {
+	return resourceList.Visit(func(info *resource.Info, err error) error {
 		if err != nil {
 			return err
 		}
-
-		if ready, err := readyChecker.IsReady(ctx, info); !ready || err != nil {
-			resourcesReady = ready
+		ready, err := readyChecker.IsReady(ctx, info)
+		if !ready {
+			return ErrResourceNotReady
+		}
+		if err != nil {
 			return err
 		}
 		return nil
 	})
-	return resourcesReady, err
 }

pkg/manifest/operations.go

@@ -40,6 +40,7 @@ var (
 	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)
@@ -296,18 +297,18 @@ func (o *Operations) uninstall() (bool, error) {
 	// uninstall resources
 	consistent, err := o.renderSrc.Uninstall(parsedFile.GetContent(),
 		o.installInfo, o.resourceTransforms, o.postRuns)
-	if !consistent {
-		return false, ErrUninstallInconsistent
-	}
 	if !UninstallSuccess(err) {
 		return false, err
 	}
+	if !consistent {
+		return false, ErrUninstallInconsistent
+	}
 
 	// delete crds last - if not present ignore!
-	crdDeleted :=  resource.RemoveCRDs(o.installInfo.Ctx, o.installInfo.Crds, o.client)
-		if !crdDeleted {
-			return false, ErrCRDsNotRemoved
-		}
+	crdDeleted := resource.RemoveCRDs(o.installInfo.Ctx, o.installInfo.Crds, o.client)
+	if !crdDeleted {
+		return false, ErrCRDsNotRemoved
+	}
 
 	// custom states check
 	if o.installInfo.CheckFn != nil {

pkg/types/installs.go

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