🐛 Fix deleting resources from Helm charts (#31)

There are two problems with deleting resources from Helm charts right
now: The first one is that the objects stay in the status.resources
list, even after they are deleted. The second is that instead of
returning an error when a deletion failed, we returned an error on
success.

Instead, as we base deletion of objects based on the status.resources
list (this can be changed in the future), we keep resources that are
supposed to be deleted but failed to in the status as not-synced.
Additionally, we requeue and try the deletion another time, just as we
do with applying currently.

As the current Update method is unused since we changed from create to
apply, it is deleted. Mocks are generated freshly as well.

Signed-off-by: janiskemper <[email protected]>

Author: janiskemper

internal/controller/clusteraddon_controller.go

@@ -200,6 +200,7 @@ func (r *ClusterAddonReconciler) Reconcile(ctx context.Context, req reconcile.Re
 
 			shouldRequeue, err := r.templateAndApplyClusterAddonHelmChart(ctx, in)
 			if err != nil {
+				conditions.MarkFalse(clusterAddon, csov1alpha1.HelmChartAppliedCondition, csov1alpha1.FailedToApplyObjectsReason, clusterv1.ConditionSeverityError, "failed to apply")
 				return ctrl.Result{}, fmt.Errorf("failed to apply helm chart: %w", err)
 			}
 			if shouldRequeue {
@@ -240,6 +241,7 @@ func (r *ClusterAddonReconciler) Reconcile(ctx context.Context, req reconcile.Re
 
 		shouldRequeue, err := r.templateAndApplyClusterAddonHelmChart(ctx, in)
 		if err != nil {
+			conditions.MarkFalse(clusterAddon, csov1alpha1.HelmChartAppliedCondition, csov1alpha1.FailedToApplyObjectsReason, clusterv1.ConditionSeverityError, "failed to apply")
 			return ctrl.Result{}, fmt.Errorf("failed to apply helm chart: %w", err)
 		}
 		if shouldRequeue {

pkg/kube/helpers.go

@@ -105,17 +105,13 @@ func parseK8sYaml(template []byte) ([]*unstructured.Unstructured, error) {
 	return objs, nil
 }
 
-func getResourceMap(resources []*csov1alpha1.Resource) (resourceMap map[types.NamespacedName]*csov1alpha1.Resource, notToApply []*csov1alpha1.Resource) {
-	resourceMap = make(map[types.NamespacedName]*csov1alpha1.Resource)
-	notToApply = make([]*csov1alpha1.Resource, 0, len(resources))
+func getResourceMap(resources []*csov1alpha1.Resource) map[types.NamespacedName]*csov1alpha1.Resource {
+	resourceMap := make(map[types.NamespacedName]*csov1alpha1.Resource)
 
 	for i, resource := range resources {
-		if resource.Status == csov1alpha1.ResourceStatusSynced {
-			notToApply = append(notToApply, resources[i])
-		}
 		resourceMap[types.NamespacedName{Name: resource.Name, Namespace: resource.Namespace}] = resources[i]
 	}
-	return resourceMap, notToApply
+	return resourceMap
 }
 
 func setLabel(target *unstructured.Unstructured, key, val string) error {

pkg/kube/kube.go

@@ -22,6 +22,7 @@ import (
 	"fmt"
 
 	csov1alpha1 "github.com/SovereignCloudStack/cluster-stack-operator/api/v1alpha1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes"
@@ -37,7 +38,6 @@ type kube struct {
 // Client has all the meathod for helm chart kube operation.
 type Client interface {
 	Apply(ctx context.Context, template []byte, oldResources []*csov1alpha1.Resource) (newResources []*csov1alpha1.Resource, shouldRequeue bool, err error)
-	Update(ctx context.Context, template []byte, oldResources []*csov1alpha1.Resource) (newResources []*csov1alpha1.Resource, shouldRequeue bool, err error)
 	Delete(template []byte) error
 }
 
@@ -72,12 +72,13 @@ func (k *kube) Apply(ctx context.Context, template []byte, oldResources []*csov1
 		return nil, false, fmt.Errorf("couldn't parse k8s yaml: %w", err)
 	}
 
-	resourceMap, newResources := getResourceMap(oldResources)
+	resourceMap := getResourceMap(oldResources)
 	for _, obj := range objs {
 		oldResource, found := resourceMap[types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()}]
 
 		// do nothing if synced
 		if found && oldResource.Status == csov1alpha1.ResourceStatusSynced {
+			newResources = append(newResources, oldResource)
 			continue
 		}
 
@@ -103,45 +104,7 @@ func (k *kube) Apply(ctx context.Context, template []byte, oldResources []*csov1
 			reterr := fmt.Errorf("failed to apply object: %w", err)
 			resource.Error = reterr.Error()
 			resource.Status = csov1alpha1.ResourceStatusNotSynced
-			logger.Error(reterr, "failed to apply object", "obj", obj.GetObjectKind().GroupVersionKind())
-			shouldRequeue = true
-		} else {
-			resource.Status = csov1alpha1.ResourceStatusSynced
-		}
-
-		newResources = append(newResources, resource)
-	}
-
-	return newResources, shouldRequeue, nil
-}
-
-func (k *kube) Update(ctx context.Context, template []byte, oldResources []*csov1alpha1.Resource) (newResources []*csov1alpha1.Resource, shouldRequeue bool, err error) {
-	logger := log.FromContext(ctx)
-
-	objs, err := parseK8sYaml(template)
-	if err != nil {
-		return nil, false, fmt.Errorf("couldn't parse k8s yaml: %w", err)
-	}
-
-	for _, obj := range objs {
-		if err := setLabel(obj, ObjectLabelKeyOwned, ObjectLabelValueOwned); err != nil {
-			return nil, false, fmt.Errorf("error setting label: %w", err)
-		}
-
-		// call the function and get dynamic.ResourceInterface
-		// getDynamicResourceInterface
-		dr, err := getDynamicResourceInterface(k.Namespace, k.RestConfig, obj.GroupVersionKind())
-		if err != nil {
-			return nil, false, fmt.Errorf("failed to get dynamic resource interface: %w", err)
-		}
-
-		resource := csov1alpha1.NewResourceFromUnstructured(obj)
-
-		if _, err := dr.Apply(ctx, obj.GetName(), obj, metav1.ApplyOptions{FieldManager: "kubectl", Force: true}); err != nil {
-			reterr := fmt.Errorf("failed to apply object: %w", err)
-			resource.Error = reterr.Error()
-			resource.Status = csov1alpha1.ResourceStatusNotSynced
-			logger.Error(reterr, "failed to apply object", "obj", obj.GetObjectKind().GroupVersionKind())
+			logger.Error(reterr, "failed to apply object", "obj", obj.GetObjectKind().GroupVersionKind(), "name", obj.GetName(), "namespace", obj.GetNamespace())
 			shouldRequeue = true
 		} else {
 			resource.Status = csov1alpha1.ResourceStatusSynced
@@ -163,10 +126,18 @@ func (k *kube) Update(ctx context.Context, template []byte, oldResources []*csov
 			return nil, false, fmt.Errorf("failed to get dynamic resource interface: %w", err)
 		}
 
-		if err := dr.Delete(ctx, resource.Name, metav1.DeleteOptions{}); err == nil {
-			return nil, false, fmt.Errorf("failed to delete object %q of namespace %q: %w", resource.Name, resource.Namespace, err)
+		if err := dr.Delete(ctx, resource.Name, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) {
+			reterr := fmt.Errorf("failed to delete object: %w", err)
+			logger.Error(reterr, "failed to delete object", "obj", resource.GroupVersionKind(), "namespacedName", resource.NamespacedName())
+
+			// append resource to status and requeue again to be able to retry deletion
+			resource.Status = csov1alpha1.ResourceStatusNotSynced
+			resource.Error = reterr.Error()
+			newResources = append(newResources, resource)
+			shouldRequeue = true
 		}
 	}
+
 	return newResources, shouldRequeue, nil
 }