Merge pull request #233 from SovereignCloudStack/ani/issues/232

🌱 Removing the temporary shouldDelete option to kubeClient.Apply that was not used anyway and always set on true

Author: janiskemper

internal/controller/clusteraddon_controller.go

@@ -238,7 +238,7 @@ func (r *ClusterAddonReconciler) Reconcile(ctx context.Context, req reconcile.Re
 			if clusterAddon.Spec.Version != metadata.Versions.Components.ClusterAddon {
 				clusterAddon.Status.Ready = false
 
-				shouldRequeue, err := r.templateAndApplyClusterAddonHelmChart(ctx, in, true)
+				shouldRequeue, err := r.templateAndApplyClusterAddonHelmChart(ctx, in)
 				if err != nil {
 					conditions.MarkFalse(clusterAddon, csov1alpha1.HelmChartAppliedCondition, csov1alpha1.FailedToApplyObjectsReason, clusterv1.ConditionSeverityError, "failed to apply: %s", err.Error())
 					return ctrl.Result{}, fmt.Errorf("failed to apply helm chart: %w", err)
@@ -273,7 +273,7 @@ func (r *ClusterAddonReconciler) Reconcile(ctx context.Context, req reconcile.Re
 
 		// if condition is false we have not yet successfully applied the helm chart
 		if conditions.IsFalse(clusterAddon, csov1alpha1.HelmChartAppliedCondition) {
-			shouldRequeue, err := r.templateAndApplyClusterAddonHelmChart(ctx, in, true)
+			shouldRequeue, err := r.templateAndApplyClusterAddonHelmChart(ctx, in)
 			if err != nil {
 				conditions.MarkFalse(clusterAddon, csov1alpha1.HelmChartAppliedCondition, csov1alpha1.FailedToApplyObjectsReason, clusterv1.ConditionSeverityError, "failed to apply: %s", err.Error())
 				return ctrl.Result{}, fmt.Errorf("failed to apply helm chart: %w", err)
@@ -718,7 +718,7 @@ func (r *ClusterAddonReconciler) getAddonStagesInput(restConfig *rest.Config, cl
 	return addonStages, nil
 }
 
-func (r *ClusterAddonReconciler) templateAndApplyClusterAddonHelmChart(ctx context.Context, in *templateAndApplyClusterAddonInput, shouldDelete bool) (bool, error) {
+func (r *ClusterAddonReconciler) templateAndApplyClusterAddonHelmChart(ctx context.Context, in *templateAndApplyClusterAddonInput) (bool, error) {
 	clusterAddonChart := in.clusterAddonChartPath
 	var shouldRequeue bool
 
@@ -734,7 +734,7 @@ func (r *ClusterAddonReconciler) templateAndApplyClusterAddonHelmChart(ctx conte
 
 	kubeClient := r.KubeClientFactory.NewClient(clusterAddonNamespace, in.restConfig)
 
-	newResources, shouldRequeue, err := kubeClient.Apply(ctx, helmTemplate, in.clusterAddon.Status.Resources, shouldDelete)
+	newResources, shouldRequeue, err := kubeClient.Apply(ctx, helmTemplate, in.clusterAddon.Status.Resources)
 	if err != nil {
 		return false, fmt.Errorf("failed to apply objects from cluster addon Helm chart: %w", err)
 	}

internal/controller/clusteraddon_controller_test.go

@@ -61,7 +61,7 @@ var _ = Describe("ClusterAddonReconciler", func() {
 			},
 		}
 
-		testEnv.KubeClient.On("Apply", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]*csov1alpha1.Resource{}, false, nil)
+		testEnv.KubeClient.On("Apply", mock.Anything, mock.Anything, mock.Anything).Return([]*csov1alpha1.Resource{}, false, nil)
 	})
 
 	AfterEach(func() {
@@ -240,7 +240,7 @@ var _ = Describe("ClusterAddonReconciler", func() {
 			}, timeout, interval).Should(BeTrue())
 
 			By("checking Update method was called")
-			Expect(testEnv.KubeClient.AssertCalled(GinkgoT(), "Apply", mock.Anything, mock.Anything, mock.Anything, mock.Anything)).To(BeTrue())
+			Expect(testEnv.KubeClient.AssertCalled(GinkgoT(), "Apply", mock.Anything, mock.Anything, mock.Anything)).To(BeTrue())
 		})
 
 		It("should not call update if the ClusterAddon version does not change in the ClusterClass update", func() {

internal/controller/clusteraddoncreate_controller_test.go

@@ -69,7 +69,7 @@ var _ = Describe("ClusterAddonCreateReconciler", func() {
 
 		key = types.NamespacedName{Name: fmt.Sprintf("cluster-addon-%s", cluster.Name), Namespace: testNs.Name}
 
-		testEnv.KubeClient.On("Apply", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]*csov1alpha1.Resource{}, false, nil)
+		testEnv.KubeClient.On("Apply", mock.Anything, mock.Anything, mock.Anything).Return([]*csov1alpha1.Resource{}, false, nil)
 	})
 
 	AfterEach(func() {

internal/controller/clusterstackrelease_controller.go

@@ -288,7 +288,7 @@ func (r *ClusterStackReleaseReconciler) templateAndApply(ctx context.Context, re
 		return false, fmt.Errorf("template is empty")
 	}
 
-	newResources, shouldRequeue, err := kubeClient.Apply(ctx, template, clusterStackRelease.Status.Resources, true)
+	newResources, shouldRequeue, err := kubeClient.Apply(ctx, template, clusterStackRelease.Status.Resources)
 	if err != nil {
 		conditions.MarkFalse(clusterStackRelease, csov1alpha1.HelmChartAppliedCondition, csov1alpha1.FailedToApplyObjectsReason, clusterv1.ConditionSeverityError, "failed to apply")
 		return false, fmt.Errorf("failed to apply cluster class helm chart: %w", err)

internal/controller/clusterstackrelease_controller_test.go

@@ -311,7 +311,7 @@ var _ = Describe("ClusterStackRelease validation", func() {
 				},
 			}
 
-			testEnv.KubeClient.On("Apply", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]*csov1alpha1.Resource{}, false, nil)
+			testEnv.KubeClient.On("Apply", mock.Anything, mock.Anything, mock.Anything).Return([]*csov1alpha1.Resource{}, false, nil)
 		})
 
 		AfterEach(func() {

pkg/kube/fake/kube.go

@@ -41,7 +41,7 @@ func NewFactory() kubeclient.Factory {
 }
 
 // Apply applies the given yaml object.
-func (*kube) Apply(_ context.Context, _ []byte, _ []*csov1alpha1.Resource, _ bool) (_ []*csov1alpha1.Resource, _ bool, _ error) {
+func (*kube) Apply(_ context.Context, _ []byte, _ []*csov1alpha1.Resource) (_ []*csov1alpha1.Resource, _ bool, _ error) {
 	return nil, false, nil
 }
 

pkg/kube/kube.go

@@ -37,7 +37,7 @@ 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, shouldDelete bool) (newResources []*csov1alpha1.Resource, shouldRequeue bool, err error)
+	Apply(ctx context.Context, template []byte, oldResources []*csov1alpha1.Resource) (newResources []*csov1alpha1.Resource, shouldRequeue bool, err error)
 	Delete(ctx context.Context, template []byte, oldResources []*csov1alpha1.Resource) (newResources []*csov1alpha1.Resource, shouldRequeue bool, err error)
 
 	ApplyNewClusterStack(ctx context.Context, oldTemplate, newTemplate []byte) (newResources []*csov1alpha1.Resource, shouldRequeue bool, err error)
@@ -163,7 +163,7 @@ func (k *kube) DeleteNewClusterStack(ctx context.Context, template []byte) (newR
 	return newResources, shouldRequeue, nil
 }
 
-func (k *kube) Apply(ctx context.Context, template []byte, oldResources []*csov1alpha1.Resource, shouldDelete bool) (newResources []*csov1alpha1.Resource, shouldRequeue bool, err error) {
+func (k *kube) Apply(ctx context.Context, template []byte, oldResources []*csov1alpha1.Resource) (newResources []*csov1alpha1.Resource, shouldRequeue bool, err error) {
 	logger := log.FromContext(ctx)
 
 	objs, err := parseK8sYaml(template)
@@ -212,32 +212,29 @@ func (k *kube) Apply(ctx context.Context, template []byte, oldResources []*csov1
 		newResources = append(newResources, resource)
 	}
 
-	// TODO: cleanup shouldDelete
-	if shouldDelete {
-		// make a diff between new objs and oldResources to find out
-		// a) if an object is in oldResources and synced and not in new objs, then delete should be attempted
-		// then, all objs should be applied by create or update
-		// at the end, we should delete objects that are supposed to be deleted
-		for _, resource := range resourcesToBeDeleted(oldResources, objs) {
-			// call the function and get dynamic.ResourceInterface
-			// getDynamicResourceInterface
-			logger.Info("resource are being deleted", "kind", resource.Kind, "name", resource.Name, "namespace", resource.Namespace)
-
-			dr, err := GetDynamicResourceInterface(k.Namespace, k.RestConfig, resource.GroupVersionKind())
-			if err != nil {
-				return nil, false, fmt.Errorf("failed to get dynamic resource interface: %w", 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
-			}
+	// make a diff between new objs and oldResources to find out
+	// a) if an object is in oldResources and synced and not in new objs, then delete should be attempted
+	// then, all objs should be applied by create or update
+	// at the end, we should delete objects that are supposed to be deleted
+	for _, resource := range resourcesToBeDeleted(oldResources, objs) {
+		// call the function and get dynamic.ResourceInterface
+		// getDynamicResourceInterface
+		logger.Info("resource are being deleted", "kind", resource.Kind, "name", resource.Name, "namespace", resource.Namespace)
+
+		dr, err := GetDynamicResourceInterface(k.Namespace, k.RestConfig, resource.GroupVersionKind())
+		if err != nil {
+			return nil, false, fmt.Errorf("failed to get dynamic resource interface: %w", 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
 		}
 	}